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}`);