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.
This commit is contained in:
+28
-3
@@ -1,7 +1,28 @@
|
|||||||
// The review pipeline: diff fetch -> conventions -> LLM -> parse -> status +
|
// The review pipeline: diff fetch -> conventions -> LLM -> parse -> status +
|
||||||
// comment -> ledger. Fail-open with visibility: if the LLM/router path dies,
|
// comment -> ledger. Fail-open with visibility: if the LLM/router path dies,
|
||||||
// we post a 'warning' commit status saying review unavailable — never a silent
|
// we post a commit status saying review unavailable — never a silent block,
|
||||||
// block, never a silent pass marked success.
|
// 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 { chat } from './router.js';
|
||||||
import { extractJson } from './jsonExtract.js';
|
import { extractJson } from './jsonExtract.js';
|
||||||
@@ -331,7 +352,11 @@ function allDiffSourcesFailed(tried) {
|
|||||||
return new Error(`all diff sources failed: ${msg}`);
|
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 { owner, repo, sha } = job;
|
||||||
const fullRepo = `${owner}/${repo}`;
|
const fullRepo = `${owner}/${repo}`;
|
||||||
log(`[${fullRepo}@${sha.slice(0, 8)}] fail-open: ${error}`);
|
log(`[${fullRepo}@${sha.slice(0, 8)}] fail-open: ${error}`);
|
||||||
|
|||||||
Reference in New Issue
Block a user