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

[dimakis]

Summary

• The checkCentaurReview gate polled a nonexistent Centaur API endpoint (/api/reviews?pr=...), making the centaur_review signal watch permanently stuck
• Rewritten to check GitHub issue comments directly via gh api for ## Centaur Review comments
• Parses review severity: LGTM = pass, critical/warning = fail (triggers retry), info/style = pass
• Fixes type mismatch: gate config sends repo + pr but old function expected pr_url

Test plan

• Run PR Shepherd on a PR with existing Centaur review — verify centaur_review gate resolves
• Verify LGTM review resolves as pass
• Verify review with critical/warning findings resolves as fail (triggering retry cycle)

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 4 issue(s) (3 warning).

server/signal-processor.ts

The rewrite from HTTP polling to gh api comment checking is a good direction, but has a null-handling bug on empty results, a regression risk for existing pr_url-format gate configs, and lacks tests for the new logic.

• 🟡 bugs (L271): 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]
• 🟡 regressions (L263): 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]
• 🔵 unsafe_assumptions (L279): 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]

server/__tests__/signal-processor.test.ts

The rewrite from HTTP polling to gh api comment checking is a good direction, but has a null-handling bug on empty results, a regression risk for existing pr_url-format gate configs, and lacks tests for the new logic.

• 🟡 missing_tests: No unit tests cover the new checkCentaurReview implementation. Key untested paths: LGTM → pass, critical/warning findings → fail, info-only findings → pass, no matching comment → not resolved, and gh api failure → not resolved. The existing tests only cover SignalProcessor lifecycle and TaskStore.findActiveSignalTasks, not the gate checker functions. Given the project's TDD requirement, tests should accompany this change. [fixable]

---

Reviewed at 2579772

[dimakis]

🟡 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]

[dimakis]

🟡 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]

[dimakis]

🔵 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]

Centaur Review

Found 5 issue(s) (3 warning).

server/signal-processor.ts

The rewrite from HTTP-fetch to gh api is a good direction, but has a null-dereference bug on empty results (accidentally caught), a backward-compatibility gap with pr_url-based gate configs, and no test coverage for the new logic.

• 🟡 bugs (L274): 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]
• 🟡 regressions (L263): 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]
• 🔵 unsafe_assumptions (L280): 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]
• 🔵 style (L285): 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]

server/__tests__/signal-processor.test.ts

The rewrite from HTTP-fetch to gh api is a good direction, but has a null-dereference bug on empty results (accidentally caught), a backward-compatibility gap with pr_url-based gate configs, and no test coverage for the new logic.

• 🟡 missing_tests: No unit tests cover the new checkCentaurReview implementation. The existing tests exercise SignalProcessor lifecycle and task-store matching but never invoke checkGate/checkCentaurReview directly. The three new code paths (LGTM → pass, critical/warning → fail, info-only → pass) and the null/empty edge case all lack coverage. These would need execFileAsync to be mocked (same pattern as would be needed for checkGhCi/checkGhReview). [fixable]

---

Reviewed at 81a2345

[dimakis]

🟡 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]

[dimakis]

🟡 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]

[dimakis]

🔵 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]

[dimakis]

🔵 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]