From 0db44a4d7348923904e064fe341a6ee080c42cc8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 22:36:42 -0400 Subject: [PATCH 1/3] fix(review): run claude-cli models directly --- src/lib/queue.js | 28 +++++++++++++-- src/lib/router.js | 51 ++++++++++++++++++++++++++- src/server.js | 11 +++++- test/queue.test.js | 57 +++++++++++++++++++++++++++++++ test/routerClient.test.js | 72 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 215 insertions(+), 4 deletions(-) create mode 100644 test/queue.test.js diff --git a/src/lib/queue.js b/src/lib/queue.js index de79637..82816e3 100644 --- a/src/lib/queue.js +++ b/src/lib/queue.js @@ -5,20 +5,44 @@ export class ReviewQueue { constructor() { this.repoChains = new Map(); this.globalChain = Promise.resolve(); + this.dedupeGenerations = new Map(); this.pending = 0; } // Serialize fn per repo key. fn receives a `withGlobal` helper that // serializes the wrapped section across ALL repos (use around LLM calls). - enqueue(repoKey, fn) { + enqueue(repoKey, fn, { dedupeKey = null, dedupeKeys = [], onSuperseded = null } = {}) { const prev = this.repoChains.get(repoKey) || Promise.resolve(); + const generationKeys = [...new Set([dedupeKey, ...dedupeKeys].filter(Boolean))] + .map((key) => `${repoKey}:${key}`); + const generations = new Map(generationKeys.map((key) => [ + key, + (this.dedupeGenerations.get(key) || 0) + 1 + ])); + for (const [key, generation] of generations) { + this.dedupeGenerations.set(key, generation); + } this.pending++; const next = prev .catch(() => {}) - .then(() => fn(this.withGlobal.bind(this))) + .then(() => { + const superseded = [...generations].some( + ([key, generation]) => this.dedupeGenerations.get(key) !== generation + ); + if (superseded) { + onSuperseded?.(); + return { superseded: true }; + } + return fn(this.withGlobal.bind(this)); + }) .finally(() => { this.pending--; if (this.repoChains.get(repoKey) === next) this.repoChains.delete(repoKey); + for (const [key, generation] of generations) { + if (this.dedupeGenerations.get(key) === generation) { + this.dedupeGenerations.delete(key); + } + } }); this.repoChains.set(repoKey, next); return next; diff --git a/src/lib/router.js b/src/lib/router.js index 258b0d9..d1b7b3b 100644 --- a/src/lib/router.js +++ b/src/lib/router.js @@ -2,7 +2,56 @@ // Real model answer lives in .message.content; top-level .content can carry an // upstream-down apology — always prefer .message.content. -export async function chat({ routerUrl, model, messages, agentId, sessionId, tenantId, timeoutMs }) { +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; + +const execFileAsync = promisify(execFile); + +async function chatWithClaudeCli({ model, messages, timeoutMs, claudeBin = 'claude' }) { + const modelId = model.slice('claude-cli/'.length); + if (!modelId) throw new Error('claude-cli model id is required'); + + const prompt = messages + .map(({ role, content }) => `${String(role || 'user').toUpperCase()}:\n${String(content || '')}`) + .join('\n\n'); + const env = { ...process.env }; + delete env.ANTHROPIC_API_KEY; + delete env.ANTHROPIC_TOKEN; + delete env.ANTHROPIC_AUTH_TOKEN; + + let stdout; + try { + ({ stdout } = await execFileAsync(claudeBin, [ + '-p', prompt, + '--model', modelId, + '--output-format', 'json', + '--tools', '', + '--permission-mode', 'dontAsk', + '--no-session-persistence' + ], { + env, + timeout: timeoutMs, + killSignal: 'SIGTERM', + maxBuffer: 4 * 1024 * 1024 + })); + } catch (error) { + const timedOut = error?.killed || error?.signal === 'SIGTERM' || error?.code === 'ETIMEDOUT'; + const exitCode = Number.isInteger(error?.code) ? ` (exit ${error.code})` : ''; + throw new Error(`claude-cli ${timedOut ? 'timed out' : 'failed'}${exitCode}`); + } + const data = JSON.parse(stdout); + const content = data?.result ?? data?.content; + if (typeof content !== 'string' || !content.trim()) { + throw new Error('claude-cli returned empty content'); + } + return { content, servedModel: model }; +} + +export async function chat(options) { + const { routerUrl, model, messages, agentId, sessionId, tenantId, timeoutMs } = options; + if (model.startsWith('claude-cli/')) { + return chatWithClaudeCli(options); + } const res = await fetch(`${routerUrl.replace(/\/$/, '')}/v1/chat`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, diff --git a/src/server.js b/src/server.js index cdddb9c..15732b6 100644 --- a/src/server.js +++ b/src/server.js @@ -37,9 +37,18 @@ function dispatchJob(queueRowId, job) { { startJob(db, queueRowId); return runReview({ cfg, gitea, adminGitea, db, job, withGlobal, log }); + }, + { + dedupeKeys: [ + job.prIndex != null ? `pr:${job.prIndex}` : null, + `sha:${job.sha}` + ].filter(Boolean) } ).then( - (r) => finishJob(db, queueRowId, r?.ok ? (r.verdict || 'ok') : `error:${String(r?.error || '').slice(0, 200)}`), + (r) => { + if (r?.superseded) return finishJob(db, queueRowId, 'superseded'); + return finishJob(db, queueRowId, r?.ok ? (r.verdict || 'ok') : `error:${String(r?.error || '').slice(0, 200)}`); + }, (e) => { log(`review pipeline error for ${job.owner}/${job.repo}@${job.sha}: ${e.stack || e.message}`); finishJob(db, queueRowId, `crash:${String(e.message || e).slice(0, 200)}`); diff --git a/test/queue.test.js b/test/queue.test.js new file mode 100644 index 0000000..02fb48a --- /dev/null +++ b/test/queue.test.js @@ -0,0 +1,57 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { ReviewQueue } from '../src/lib/queue.js'; + +test('newer jobs supersede queued work for the same pull request', async () => { + const queue = new ReviewQueue(); + const ran = []; + const superseded = []; + + const first = queue.enqueue( + 'nirpa/hermes-agent', + async () => ran.push('old'), + { dedupeKey: 'pr:86', onSuperseded: () => superseded.push('old') } + ); + const second = queue.enqueue( + 'nirpa/hermes-agent', + async () => ran.push('new'), + { dedupeKey: 'pr:86', onSuperseded: () => superseded.push('new') } + ); + + await Promise.all([first, second]); + assert.deepEqual(ran, ['new']); + assert.deepEqual(superseded, ['old']); + assert.equal(queue.pending, 0); +}); + +test('different pull requests are not deduplicated', async () => { + const queue = new ReviewQueue(); + const ran = []; + + await Promise.all([ + queue.enqueue('nirpa/hermes-agent', async () => ran.push('pr-86'), { dedupeKey: 'pr:86' }), + queue.enqueue('nirpa/hermes-agent', async () => ran.push('pr-87'), { dedupeKey: 'pr:87' }) + ]); + + assert.deepEqual(ran, ['pr-86', 'pr-87']); +}); + +test('shared sha key deduplicates push and pull-request webhooks', async () => { + const queue = new ReviewQueue(); + const ran = []; + + const push = queue.enqueue( + 'nirpa/hermes-agent', + async () => ran.push('push'), + { dedupeKeys: ['sha:final'] } + ); + const pullRequest = queue.enqueue( + 'nirpa/hermes-agent', + async () => ran.push('pull-request'), + { dedupeKeys: ['pr:86', 'sha:final'] } + ); + + await Promise.all([push, pullRequest]); + assert.deepEqual(ran, ['pull-request']); +}); diff --git a/test/routerClient.test.js b/test/routerClient.test.js index 7ada51f..91c1268 100644 --- a/test/routerClient.test.js +++ b/test/routerClient.test.js @@ -1,5 +1,8 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; +import { chmod, mkdtemp, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { chat } from '../src/lib/router.js'; test('review router client disables model fallbacks explicitly', async () => { @@ -32,3 +35,72 @@ test('review router client disables model fallbacks explicitly', async () => { assert.equal(body.raw, true); assert.equal(body.stream, false); }); + +test('claude-cli models bypass the router and use subscription credentials', async () => { + const dir = await mkdtemp(join(tmpdir(), 'granthi-review-claude-')); + const fakeClaude = join(dir, 'claude'); + await writeFile(fakeClaude, `#!/bin/sh +case " $* " in + *" --model claude-sonnet-4-6 "*) ;; + *) exit 21 ;; +esac +case " $* " in + *" --tools --permission-mode dontAsk --no-session-persistence "*) ;; + *) exit 22 ;; +esac +[ -z "\${ANTHROPIC_API_KEY:-}" ] || exit 23 +[ -z "\${ANTHROPIC_TOKEN:-}" ] || exit 24 +printf '%s' '{"result":"{\\"axes\\":{},\\"overall\\":90,\\"grade\\":\\"A\\",\\"findings\\":[]}"}' +`); + await chmod(fakeClaude, 0o700); + + const originalFetch = globalThis.fetch; + const originalApiKey = process.env.ANTHROPIC_API_KEY; + const originalToken = process.env.ANTHROPIC_TOKEN; + globalThis.fetch = async () => { throw new Error('router must not be called'); }; + process.env.ANTHROPIC_API_KEY = 'test-api-key'; + process.env.ANTHROPIC_TOKEN = 'test-token'; + + try { + const result = await chat({ + routerUrl: 'http://router.test', + model: 'claude-cli/claude-sonnet-4-6', + messages: [{ role: 'user', content: 'review this diff' }], + agentId: 'code-reviewer', + sessionId: 'review-test', + tenantId: 'nirlab', + timeoutMs: 1000, + claudeBin: fakeClaude + }); + assert.match(result.content, /"overall":90/); + assert.equal(result.servedModel, 'claude-cli/claude-sonnet-4-6'); + } finally { + globalThis.fetch = originalFetch; + if (originalApiKey === undefined) delete process.env.ANTHROPIC_API_KEY; + else process.env.ANTHROPIC_API_KEY = originalApiKey; + if (originalToken === undefined) delete process.env.ANTHROPIC_TOKEN; + else process.env.ANTHROPIC_TOKEN = originalToken; + } +}); + +test('claude-cli failures do not leak the review prompt', async () => { + const dir = await mkdtemp(join(tmpdir(), 'granthi-review-claude-error-')); + const fakeClaude = join(dir, 'claude'); + await writeFile(fakeClaude, '#!/bin/sh\nprintf "%s\\n" "$*" >&2\nexit 25\n'); + await chmod(fakeClaude, 0o700); + + await assert.rejects( + chat({ + routerUrl: 'http://router.test', + model: 'claude-cli/claude-sonnet-4-6', + messages: [{ role: 'user', content: 'SECRET_REVIEW_DIFF_MARKER' }], + timeoutMs: 1000, + claudeBin: fakeClaude + }), + (error) => { + assert.match(error.message, /claude-cli failed/); + assert.doesNotMatch(error.message, /SECRET_REVIEW_DIFF_MARKER/); + return true; + } + ); +}); From a12f94f4594a29eaf0482a6bfb42b8f321dd4a97 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 08:21:41 -0400 Subject: [PATCH 2/3] fix(review): fail-open must post success, because CONTEXT is a required check granthi-review is a REQUIRED status check on main, and Gitea counts a required context as satisfied only on 'success'. failOpen posted 'warning' with the description 'review unavailable - not blocking', so it did precisely what it promised not to do: it blocked, while announcing it would not. Measured: shreai PR #209 sat unmergeable for six days on a claude-cli failure from 2026-08-23, every other gate green. The failure is also non-deterministic. The same commit a49573f66 produced 'Grade A (86/100) - pass' on one review and a fail-open minutes later with no code change. A state that flips on identical input is infrastructure noise, not a review signal, and must not decide whether code can merge. The gate is not weakened: reviews that actually run still gate, and a real 'fail' verdict still fails. Only 'could not review' now passes, which is what fail-open has always meant. The reason still rides in the description and the error is still persisted to the reviews table. --- src/lib/review.js | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/src/lib/review.js b/src/lib/review.js index b8b3d19..8b8bd92 100644 --- a/src/lib/review.js +++ b/src/lib/review.js @@ -1,7 +1,28 @@ // The review pipeline: diff fetch -> conventions -> LLM -> parse -> status + // comment -> ledger. Fail-open with visibility: if the LLM/router path dies, -// we post a 'warning' commit status saying review unavailable — never a silent -// block, never a silent pass marked success. +// we post a commit status saying review unavailable — never a silent block, +// and never a silent pass: the reason always rides in the description and the +// error is always recorded in the reviews table. +// +// That status is 'success', and this is deliberate — it used to be 'warning'. +// CONTEXT is a REQUIRED status check on main, and Gitea's branch protection +// counts a required context as satisfied ONLY on 'success'. So 'warning' did +// the exact thing this comment promised not to do: it blocked, silently, while +// the description said "not blocking". Measured 2026-08-29 on shreai PR #209 — +// unmergeable for six days on a 'claude-cli failed (exit 1)' from 2026-08-23, +// with every other gate green. +// +// The failure is also non-deterministic, which is what settles the argument: +// the SAME commit (a49573f66) produced 'Grade A (86/100) — pass' on one review +// and a fail-open on the next, minutes apart, with no code change between. A +// state that flips on identical input is infrastructure noise, not a review +// signal, and must not be the thing that decides whether code can merge. +// +// The gate is NOT weakened: a review that actually RUNS still gates normally, +// and a real 'fail' verdict still fails. Only the "we could not review this" +// case passes, which is what fail-open has always meant here. Do not "restore" +// 'warning' without first removing CONTEXT from the required status checks — +// the two settings contradict each other, and picking both is what caused this. import { chat } from './router.js'; import { extractJson } from './jsonExtract.js'; @@ -331,7 +352,11 @@ function allDiffSourcesFailed(tried) { return new Error(`all diff sources failed: ${msg}`); } -async function failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, error, state = 'warning', description }) { +// state defaults to 'success' because CONTEXT is a required check — see the +// header. 'success' here means "this did not review, and is not blocking", +// which is exactly what the description says. The error is still persisted to +// the reviews table below, so a fail-open is never invisible. +async function failOpen({ cfg, db, job, log, postStatus, dashboardUrl, t0, error, state = 'success', description }) { const { owner, repo, sha } = job; const fullRepo = `${owner}/${repo}`; log(`[${fullRepo}@${sha.slice(0, 8)}] fail-open: ${error}`); From 2099a16a1f2ca959486e5baf8c166c36c4c19645 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:11:03 -0400 Subject: [PATCH 3/3] test(review): prove fail-open status contract --- README.md | 4 ++-- src/lib/review.js | 4 ++-- test/reviewFailOpen.test.js | 45 +++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 test/reviewFailOpen.test.js diff --git a/README.md b/README.md index 12481d8..68615b3 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,7 @@ Dependencies: **none**. Node ≥ 22.13 (uses `node:sqlite`, global `fetch`). - Commit status context: `granthi-review`. verdict pass → `success`; warn → `success` with warning description; fail → `failure`. - Branch protection on pilot repos requires the `granthi-review` context but does **not** apply to admins — an admin merge is the override. The blocking comment says so explicitly. -- **Fail-open with visibility**: if the router/LLM is down or output is unparseable after retry, the service posts status `warning` with "review unavailable" — it never blocks silently and never fakes a pass. +- **Fail-open with visibility**: if the router/LLM is down or output is unparseable after retry, the service posts status `success` with "review unavailable" so the required check does not block. The error remains visible in the description and review ledger. - Bare pushes (no PR): commit status + ledger + dashboard only. Gitea 1.27 has no commit-comment API endpoint, so no comment is posted for pushes. ## Config @@ -96,7 +96,7 @@ and retries the post once. Without `admin_token` the 403 surfaces as before. - **Full push-range review**: pushes are reviewed as ONE compare diff `before...after` (web compare `.diff` route — the 1.27 API has no compare diff variant). New-branch pushes (`before` = zero sha) fall back to the head commit diff. No more first-20-commits slicing under a head-sha status. - **Truncated diff ≠ pass**: when the 60KB cap truncates the diff, the prompt and the status description both say `partial review (diff truncated)` and the verdict is capped at `warn` (a confirmed-critical `fail` still fails). -- **Diff fetch failure ≠ pass**: any diff-fetch failure (throw, non-200, empty body for a non-empty push/PR) short-circuits to status `warning` — `review unavailable — diff fetch failed (not blocking)` — with a ledger row. A diffless prompt is never sent. +- **Diff fetch failure ≠ review**: any diff-fetch failure (throw, non-200, empty body for a non-empty push/PR) short-circuits to status `success` — `review unavailable — diff fetch failed (not blocking)` — with the error preserved in a ledger row. A diffless prompt is never sent. - **Crash-safe queue**: every accepted job is persisted to sqlite (`queue_jobs`) before the pending status posts; on startup, rows that never reached a final state are re-enqueued, so a restart no longer strands a sha at `pending`. - **403 self-heal**: see `admin_token` above. `scripts/wire-repos.mjs` wires the webhook + bot collaborator across ALL repos on the peer (idempotent; re-runnable). diff --git a/src/lib/review.js b/src/lib/review.js index 8b8bd92..5872688 100644 --- a/src/lib/review.js +++ b/src/lib/review.js @@ -67,8 +67,8 @@ export async function runReview({ cfg, gitea, adminGitea, db, job, withGlobal, l })); // 1. diff — any fetch failure (throw, non-200, or empty body for a job that - // has commits) short-circuits to a 'warning' status. A diffless prompt would - // make the model "review" nothing and pass; never send one. + // has commits) short-circuits to a non-blocking 'success' status. A diffless + // prompt would make the model "review" nothing and pass; never send one. let diff = '', truncated = false, diffBytes = 0, partialDiff = false, partialNote = null; try { const fetched = await fetchDiff({ diff --git a/test/reviewFailOpen.test.js b/test/reviewFailOpen.test.js new file mode 100644 index 0000000..4bba744 --- /dev/null +++ b/test/reviewFailOpen.test.js @@ -0,0 +1,45 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { openDb } from '../src/lib/db.js'; +import { runReview } from '../src/lib/review.js'; + +const HEAD = 'a'.repeat(40); + +test('an unavailable review posts success without hiding the failure', async () => { + const statuses = []; + const db = openDb(':memory:'); + const gitea = { + postStatus: async (_owner, _repo, _sha, body) => statuses.push(body), + getCommitDiff: async () => { throw new Error('review service unavailable'); } + }; + const job = { + owner: 'o', repo: 'r', sha: HEAD, before: null, prIndex: null, + event: 'push', pusher: 'tester', authors: [], commitMessages: [] + }; + + const result = await runReview({ + cfg: { + publicBaseUrl: 'https://reviews.example.test', + maxDiffBytes: 1024, + model: 'claude-cli/claude-sonnet-4-6', + botUsername: 'reviewer' + }, + gitea, + adminGitea: null, + db, + job, + withGlobal: async (fn) => fn(), + log: () => {} + }); + + assert.equal(result.ok, false); + assert.deepEqual(statuses.map(({ state }) => state), ['pending', 'success']); + assert.equal(statuses[1].context, 'granthi-review'); + assert.equal(statuses[1].description, 'review unavailable — diff fetch failed (not blocking)'); + + const persisted = db.prepare( + 'SELECT status_state, error FROM reviews WHERE repo = ? AND sha = ?' + ).get('o/r', HEAD); + assert.equal(persisted.status_state, 'success'); + assert.match(persisted.error, /review service unavailable/); +});