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
+170 −16
Open

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

Changes from 1 commit
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
46 changes: 32 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,41 @@ async function checkGhReview(config: { repo: string; pr: number | string }): Pro
}
}

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

@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: Existing gate configs using the pr_url format (e.g. { type: 'centaur_review', pr_url: 'https://...' }) will fail during polling — config.repo and config.pr will be undefined, producing a bad gh api path. The REST resolve endpoint in app.ts:1048 and the test at signal-processor.test.ts:251 still support pr_url matching, creating an inconsistency. Either migrate the resolve endpoint and tests away from pr_url, or handle both formats in checkCentaurReview. [fixable]

config: { 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