From 2579772652e781d7a82d2c983b191ec2d7a7f5e9 Mon Sep 17 00:00:00 2001 From: dimakis Date: Sun, 5 Jul 2026 22:45:56 +0100 Subject: [PATCH 1/3] fix(signals): rewrite centaur_review gate to check GitHub comments The old implementation called a nonexistent Centaur API endpoint (/api/reviews?pr=...) that always 404'd, making the centaur_review signal watch permanently stuck. Now checks GitHub issue comments directly via gh CLI for "## Centaur Review" comments. Parses severity: - LGTM = pass - Critical/warning findings = fail (triggers retry cycle) - Info/style only = pass Also fixes type mismatch: gate config passes repo+pr but the old function expected pr_url. Co-Authored-By: Claude Opus 4.6 --- server/signal-processor.ts | 46 ++++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/server/signal-processor.ts b/server/signal-processor.ts index 0bc6f2e9..e22448c8 100644 --- a/server/signal-processor.ts +++ b/server/signal-processor.ts @@ -186,7 +186,7 @@ export async function checkGate(config: GateConfig): Promise { 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': @@ -260,23 +260,41 @@ async function checkGhReview(config: { repo: string; pr: number | string }): Pro } } -async function checkCentaurReview(config: { pr_url: string }): Promise { +async function checkCentaurReview( + config: { repo: string; pr: number | string }, +): Promise { 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', + ]); + if (!stdout.trim()) return { resolved: false, status: 'fail' }; + + const comment = JSON.parse(stdout) as { body: string; created_at: string }; + const body = comment.body; + + // LGTM with no issues = pass + if (body.includes('LGTM')) { + 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); + 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' }; } } From 81a234549b4140041c00ea2bdbbecc82d780afc4 Mon Sep 17 00:00:00 2001 From: dimakis Date: Sun, 5 Jul 2026 23:01:54 +0100 Subject: [PATCH 2/3] style(signals): fix prettier formatting in signal-processor Co-Authored-By: Claude Opus 4.6 --- server/signal-processor.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/server/signal-processor.ts b/server/signal-processor.ts index e22448c8..92e868a6 100644 --- a/server/signal-processor.ts +++ b/server/signal-processor.ts @@ -260,9 +260,10 @@ async function checkGhReview(config: { repo: string; pr: number | string }): Pro } } -async function checkCentaurReview( - config: { repo: string; pr: number | string }, -): Promise { +async function checkCentaurReview(config: { + repo: string; + pr: number | string; +}): Promise { try { const { stdout } = await execFileAsync('gh', [ 'api', From d81f7d339807263f584c19639d680b557006b0a3 Mon Sep 17 00:00:00 2001 From: dimakis Date: Tue, 7 Jul 2026 22:52:06 +0100 Subject: [PATCH 3/3] fix(signals): address Centaur review findings on centaur_review gate - Fix null handling: guard against jq returning "null" string for empty results - Add pr_url backward compatibility: parse legacy pr_url configs into repo+pr - Check severity before LGTM: prevents false pass when review contains both - Add format contract comment for severity regexes - Add 10 unit tests for checkCentaurReview covering all code paths Co-Authored-By: Claude Opus 4.6 --- server/__tests__/signal-processor.test.ts | 125 +++++++++++++++++++++- server/signal-processor.ts | 36 ++++--- 2 files changed, 148 insertions(+), 13 deletions(-) diff --git a/server/__tests__/signal-processor.test.ts b/server/__tests__/signal-processor.test.ts index b910788e..3f4ddf2a 100644 --- a/server/__tests__/signal-processor.test.ts +++ b/server/__tests__/signal-processor.test.ts @@ -2,8 +2,30 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { join } from 'path'; import { mkdirSync, rmSync } from 'fs'; import { tmpdir } from 'os'; +import { promisify } from 'util'; import { TaskStore } from '../task-store.js'; -import { SignalProcessor } from '../signal-processor.js'; +import { SignalProcessor, checkGate } from '../signal-processor.js'; + +// vi.hoisted runs before vi.mock hoisting, so these are available in the factory +const { execFilePromisified } = vi.hoisted(() => ({ + execFilePromisified: vi.fn<() => Promise<{ stdout: string; stderr: string }>>(), +})); + +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal(); + const mockFn = Object.assign(vi.fn(), { + [promisify.custom]: execFilePromisified, + }); + return { ...actual, execFile: mockFn }; +}); + +function mockExecResult(stdout: string): void { + execFilePromisified.mockResolvedValue({ stdout, stderr: '' }); +} + +function mockExecError(err: Error): void { + execFilePromisified.mockRejectedValue(err); +} const TEST_DIR = join(tmpdir(), `mitzo-signal-test-${process.pid}`); @@ -380,6 +402,107 @@ describe('SignalProcessor', () => { }); }); + describe('checkGate — centaur_review', () => { + afterEach(() => { + execFilePromisified.mockReset(); + }); + + it('returns pass for LGTM review', async () => { + const reviewBody = '## Centaur Review\n\nLGTM — no issues found.'; + mockExecResult(JSON.stringify({ body: reviewBody, created_at: '2026-07-05T20:00:00Z' })); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(true); + expect(result.status).toBe('pass'); + }); + + it('returns fail for review with critical findings', async () => { + const reviewBody = + '## Centaur Review\n\nFound **3** issue(s) (1 critical, 2 warning).\n\n- details...'; + mockExecResult(JSON.stringify({ body: reviewBody, created_at: '2026-07-05T20:00:00Z' })); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(true); + expect(result.status).toBe('fail'); + expect(result.artifacts).toMatchObject({ hasCritical: true, hasWarning: true }); + }); + + it('returns fail for review with warnings only', async () => { + const reviewBody = '## Centaur Review\n\nFound **2** issue(s) (2 warning).\n\n- details...'; + mockExecResult(JSON.stringify({ body: reviewBody, created_at: '2026-07-05T20:00:00Z' })); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(true); + expect(result.status).toBe('fail'); + expect(result.artifacts).toMatchObject({ hasCritical: false, hasWarning: true }); + }); + + it('returns pass for info/style-only findings', async () => { + const reviewBody = '## Centaur Review\n\nFound **1** issue(s).\n\n- 🔵 style: minor nit'; + mockExecResult(JSON.stringify({ body: reviewBody, created_at: '2026-07-05T20:00:00Z' })); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(true); + expect(result.status).toBe('pass'); + }); + + it('checks severity before LGTM (prevents false pass)', async () => { + // A review that contains both LGTM text and critical findings + const reviewBody = + '## Centaur Review\n\nFound **1** issue(s) (1 critical).\n\n- LGTM overall but 1 critical issue'; + mockExecResult(JSON.stringify({ body: reviewBody, created_at: '2026-07-05T20:00:00Z' })); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(true); + expect(result.status).toBe('fail'); + }); + + it('returns not-resolved when no matching comment exists (empty output)', async () => { + mockExecResult(''); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(false); + }); + + it('returns not-resolved when jq returns null (no matching comment)', async () => { + mockExecResult('null'); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(false); + }); + + it('returns not-resolved when gh api fails', async () => { + mockExecError(new Error('gh api error')); + + const result = await checkGate({ type: 'centaur_review', repo: 'org/repo', pr: 1 }); + expect(result.resolved).toBe(false); + }); + + it('handles pr_url config format (backward compat)', async () => { + const reviewBody = '## Centaur Review\n\nLGTM — no issues found.'; + mockExecResult(JSON.stringify({ body: reviewBody, created_at: '2026-07-05T20:00:00Z' })); + + const result = await checkGate({ + type: 'centaur_review', + pr_url: 'https://github.com/dimakis/mitzo/pull/360', + } as unknown as Parameters[0]); + expect(result.resolved).toBe(true); + expect(result.status).toBe('pass'); + // Verify the promisified function was called with repo/pr extracted from pr_url + expect(execFilePromisified).toHaveBeenCalled(); + const callStr = JSON.stringify(execFilePromisified.mock.calls[0]); + expect(callStr).toContain('repos/dimakis/mitzo/issues/360/comments'); + }); + + it('returns not-resolved for pr_url that cannot be parsed', async () => { + const result = await checkGate({ + type: 'centaur_review', + pr_url: 'https://example.com/not-a-github-url', + } as unknown as Parameters[0]); + expect(result.resolved).toBe(false); + }); + }); + it('stores failure artifacts in annotations on retry', () => { const goal = store.create({ title: 'Goal' }); const agent = store.create({ diff --git a/server/signal-processor.ts b/server/signal-processor.ts index 92e868a6..dc223f79 100644 --- a/server/signal-processor.ts +++ b/server/signal-processor.ts @@ -186,7 +186,9 @@ export async function checkGate(config: GateConfig): Promise { case 'gh_review': return checkGhReview(config as GateConfig & { repo: string; pr: number | string }); case 'centaur_review': - return checkCentaurReview(config as GateConfig & { repo: string; pr: number | string }); + return checkCentaurReview( + config as GateConfig & { repo?: string; pr?: number | string; pr_url?: string }, + ); case 'compound': return checkCompound(config as GateConfig & { all: GateConfig[] }); case 'human_approval': @@ -261,27 +263,37 @@ async function checkGhReview(config: { repo: string; pr: number | string }): Pro } async function checkCentaurReview(config: { - repo: string; - pr: number | string; + repo?: string; + pr?: number | string; + pr_url?: string; }): Promise { + // Support both repo+pr and legacy pr_url gate config formats + let repo = config.repo; + let pr = config.pr; + if (!repo && !pr && config.pr_url) { + const match = config.pr_url.match(/github\.com\/([^/]+\/[^/]+)\/pull\/(\d+)/); + if (match) { + repo = match[1]; + pr = match[2]; + } + } + if (!repo || !pr) return { resolved: false, status: 'fail' }; + try { const { stdout } = await execFileAsync('gh', [ 'api', - `repos/${config.repo}/issues/${config.pr}/comments`, + `repos/${repo}/issues/${pr}/comments`, '--jq', '[.[] | select(.body | startswith("## Centaur Review")) | {body: .body, created_at: .created_at}] | last', ]); - if (!stdout.trim()) return { resolved: false, status: 'fail' }; + if (!stdout.trim() || stdout.trim() === 'null') return { resolved: false, status: 'fail' }; const comment = JSON.parse(stdout) as { body: string; created_at: string }; + if (!comment) return { resolved: false, status: 'fail' }; const body = comment.body; - // LGTM with no issues = pass - if (body.includes('LGTM')) { - return { resolved: true, status: 'pass', artifacts: { review: body } }; - } - - // Has findings — resolved (review exists) but status depends on severity + // Check severity first — a review with findings takes precedence even if "LGTM" appears + // Regexes match Centaur's summary format: "Found **N** issue(s) (X critical, Y warning)" const hasCritical = /\d+\s+critical/.test(body); const hasWarning = /\d+\s+warning/.test(body); @@ -293,7 +305,7 @@ async function checkCentaurReview(config: { }; } - // Review exists but only info/style — pass + // LGTM or info/style only — pass return { resolved: true, status: 'pass', artifacts: { review: body } }; } catch { return { resolved: false, status: 'fail' };