Skip to content

fix(signals): rewrite centaur_review gate to check GitHub comments #444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dimakis wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(signals): rewrite centaur_review gate to check GitHub comments #444

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2579772
fix(signals): rewrite centaur_review gate to check GitHub comments
dimakis Jul 5, 2026
81a2345
style(signals): fix prettier formatting in signal-processor
dimakis Jul 5, 2026
d81f7d3
fix(signals): address Centaur review findings on centaur_review gate
dimakis Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 33 additions & 14 deletions server/signal-processor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ export async function checkGate(config: GateConfig): Promise<GateResult> {
case 'gh_review':
return checkGhReview(config as GateConfig & { repo: string; pr: number | string });
case 'centaur_review':
return checkCentaurReview(config as GateConfig & { pr_url: string });
return checkCentaurReview(config as GateConfig & { repo: string; pr: number | string });
case 'compound':
return checkCompound(config as GateConfig & { all: GateConfig[] });
case 'human_approval':
Expand Down Expand Up @@ -260,23 +260,42 @@ async function checkGhReview(config: { repo: string; pr: number | string }): Pro
}
}

async function checkCentaurReview(config: { pr_url: string }): Promise<GateResult> {
async function checkCentaurReview(config: {

@dimakis dimakis Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 regressions: The function signature changed from { pr_url: string } to { repo: string; pr: number | string }, but the signal resolve endpoint in app.ts:1048 still supports matching centaur_review tasks by pr_url, and the test at signal-processor.test.ts:251 creates gate configs with only pr_url. If any existing tasks were created with pr_url-only configs, polling via checkGate will cast them to { repo, pr } (both undefined), causing gh api repos/undefined/issues/undefined/comments to fail silently every poll cycle. Either migrate the resolve endpoint and tests to drop pr_url support, or handle both config shapes in checkCentaurReview. [fixable]

repo: string;
pr: number | string;
}): Promise<GateResult> {
try {
const res = await fetch(
`http://localhost:8642/api/reviews?pr=${encodeURIComponent(config.pr_url)}`,
);
if (!res.ok) return { resolved: false, status: 'fail' };
const data = (await res.json()) as { status?: string; review?: unknown };

if (data.status === 'approved') {
return { resolved: true, status: 'pass', artifacts: { review: data.review } };
const { stdout } = await execFileAsync('gh', [
'api',
`repos/${config.repo}/issues/${config.pr}/comments`,
'--jq',
'[.[] | select(.body | startswith("## Centaur Review")) | {body: .body, created_at: .created_at}] | last',

@dimakis dimakis Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 bugs: When no comments match the select, jq evaluates [] | last → null. stdout becomes "null", which passes the !stdout.trim() check. JSON.parse("null") returns null, and null.body throws a TypeError. The function still returns the correct result (caught by catch → resolved: false), but it relies on an exception for normal control flow. Guard against this with if (!stdout.trim() || stdout.trim() === 'null'). [fixable]

]);
if (!stdout.trim()) return { resolved: false, status: 'fail' };

@dimakis dimakis Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 bugs: When no Centaur Review comment exists on the PR, the jq filter [...] | last outputs the string null (not empty). "null".trim() is truthy so the empty-check passes, JSON.parse("null") returns JS null, and null.body throws a TypeError. The catch block returns the correct result (resolved: false), so behavior is accidentally correct — but relying on an exception for a normal control flow path is fragile. Add an explicit null check after JSON.parse, e.g. if (!comment) return { resolved: false, status: 'fail' };. [fixable]


const comment = JSON.parse(stdout) as { body: string; created_at: string };
const body = comment.body;

// LGTM with no issues = pass
if (body.includes('LGTM')) {

@dimakis dimakis Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 unsafe_assumptions: body.includes('LGTM') is checked before the critical/warning regexes. If a review contains both 'LGTM' and findings (e.g. "LGTM overall but 1 warning about..."), it will short-circuit to pass. Consider checking for critical/warning first, or making the LGTM check more precise (e.g. checking for "LGTM" on its own line or as a standalone verdict). [fixable]

@dimakis dimakis Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 unsafe_assumptions: body.includes('LGTM') is a loose substring match. If a review contains 'LGTM' anywhere in a finding description or quoted text (e.g., "the user expects LGTM but..."), this would incorrectly classify a review-with-findings as a pass. Consider anchoring to the summary section — e.g., matching only the summary line pattern like /^.*LGTM/m after the header, or checking a structured section. [fixable]

return { resolved: true, status: 'pass', artifacts: { review: body } };
}
if (data.status === 'changes_requested') {
return { resolved: true, status: 'fail', artifacts: { review: data.review } };

// Has findings — resolved (review exists) but status depends on severity
const hasCritical = /\d+\s+critical/.test(body);

@dimakis dimakis Jul 5, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 style: The regexes /\d+\s+critical/ and /\d+\s+warning/ are tightly coupled to the Centaur review output format (e.g., "2 critical, 3 warning"). If the format changes (e.g., to "critical: 2"), these silently stop matching and all reviews fall through to the info-only pass path. Consider extracting these patterns to constants or adding a comment noting the expected format contract. [fixable]

const hasWarning = /\d+\s+warning/.test(body);

if (hasCritical || hasWarning) {
return {
resolved: true,
status: 'fail',
artifacts: { review: body, hasCritical, hasWarning },
};
}
return { resolved: false, status: 'fail' };

// Review exists but only info/style — pass
return { resolved: true, status: 'pass', artifacts: { review: body } };
} catch {
// Centaur might not be running — that's fine, just not resolved
return { resolved: false, status: 'fail' };
}
}
Expand Down
Loading