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

Show file tree
Hide file tree
Changes from all 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
125 changes: 124 additions & 1 deletion server/__tests__/signal-processor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<typeof import('child_process')>();
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}`);

Expand Down Expand Up @@ -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<typeof checkGate>[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<typeof checkGate>[0]);
expect(result.resolved).toBe(false);
});
});

it('stores failure artifacts in annotations on retry', () => {
const goal = store.create({ title: 'Goal' });
const agent = store.create({
Expand Down
61 changes: 46 additions & 15 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,9 @@ 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; pr_url?: string },
);
case 'compound':
return checkCompound(config as GateConfig & { all: GateConfig[] });
case 'human_approval':
Expand Down Expand Up @@ -260,23 +262,52 @@ async function checkGhReview(config: { repo: string; pr: number | string }): Pro
}
}

async function checkCentaurReview(config: { pr_url: 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 } };
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;
pr_url?: string;
}): Promise<GateResult> {
// 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 (data.status === 'changes_requested') {
return { resolved: true, status: 'fail', artifacts: { review: data.review } };
}
if (!repo || !pr) return { resolved: false, status: 'fail' };

try {
const { stdout } = await execFileAsync('gh', [
'api',
`repos/${repo}/issues/${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() || 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;

// 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);

@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' };

// LGTM or info/style only — 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