Skip to content

Commit a623142

Browse files
committed
fix(queue): guard the verdict derivation instead of the pass, so the webhook path is covered (#10227)
#10204 split the verdict-stability backoff (#10184) across two kinds of place and only got one right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227
1 parent 7b6e2e2 commit a623142

6 files changed

Lines changed: 1087 additions & 244 deletions

File tree

package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@
101101
"regate-sort-key:check": "node --experimental-strip-types scripts/check-regate-sort-key.ts",
102102
"command-redelivery-guards:check": "node --experimental-strip-types scripts/check-command-redelivery-guards.ts",
103103
"dispatch-gate-reasons:check": "node --experimental-strip-types scripts/check-dispatch-gate-reasons.ts",
104+
"verdict-backoff-chokepoint:check": "node --experimental-strip-types scripts/check-verdict-backoff-chokepoint.ts",
104105
"replay-runner-manifest": "tsx scripts/replay-runner-image-manifest.ts",
105106
"replay-runner-manifest:write": "tsx scripts/replay-runner-image-manifest.ts --write",
106107
"replay-runner-manifest:check": "tsx scripts/replay-runner-image-manifest.ts --check",
@@ -149,7 +150,7 @@
149150
"test:smoke:browser:install": "playwright install chromium",
150151
"test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts",
151152
"pretest:ci": "npm run check-node-version",
152-
"test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build",
153+
"test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run verdict-backoff-chokepoint:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build",
153154
"test:release": "npm run test:ci && npm run changelog:check",
154155
"test:release:mcp": "npm run test:ci",
155156
"test:watch": "vitest",
Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
#!/usr/bin/env node
2+
// #10227: the verdict-stability backoff (#10184) must guard the verdict DERIVATION, not a pass entry point.
3+
//
4+
// The bug class this closes, stated once. #10204 put the skip at the entry of the whole publish-and-maintain
5+
// pass, so it suppressed everything the pass OWED -- bounded retry chains, one-shot markers, budgets -- and
6+
// not just the redundant re-derivation. Each bounded retry then needed an entry in an `explicitlyRequested`
7+
// allowlist, and a chain that forgot to join it was silently truncated the day it shipped. It is the shape of
8+
// rule whose correctness depends on every future author remembering an unwritten obligation, and the repo has
9+
// been burned by that shape enough times (#9860, #10061) to stop trusting it.
10+
//
11+
// The redesign removes the obligation instead of documenting it: one guard, at the single point a pass derives
12+
// a verdict, with everything the pass owes running above it. A new retry chain is then safe BY CONSTRUCTION.
13+
// That guarantee is only worth anything while the structure holds, so this checks the structure:
14+
//
15+
// 1. ONE call site. The guard is a choke point or it is nothing; two call sites means it is a per-entry-point
16+
// rule again, and the second one will drift.
17+
// 2. That call site is inside maybePublishPrPublicSurface -- the function both entry points reach a verdict
18+
// through.
19+
// 3. It sits ABOVE the derivation it is meant to skip (the pending gate check-run and the gate evaluation).
20+
// A guard that drifts below them still compiles, still passes every behavioural test that only asserts
21+
// "backed off", and saves nothing.
22+
// 4. Every publish-and-maintain entry point routes through it: a function that runs maybeRunAgentMaintenance
23+
// must get its gate from maybePublishPrPublicSurface. This is the one the issue asked for -- a NEW entry
24+
// point that derives a verdict some other way bypasses the choke point, and nothing else would notice.
25+
import { readFileSync } from "node:fs";
26+
import { fileURLToPath } from "node:url";
27+
28+
const PROCESSORS = "src/queue/processors.ts";
29+
30+
/** The guard, the function that must contain it, and the two derivation steps it must precede. */
31+
const GUARD_CALL = "await stableVerdictBackoffEngaged(env, {";
32+
const CHOKE_POINT_FUNCTION = "maybePublishPrPublicSurface";
33+
const DERIVATION_MARKERS = ["createOrUpdatePendingGateCheckRun(", "gateEvaluation = await withReviewPipelineSpan("] as const;
34+
35+
/** The call that makes a function a publish-and-maintain entry point, and the call that must accompany it. */
36+
const MAINTENANCE_CALL = "maybeRunAgentMaintenance(env, {";
37+
const PUBLISH_CALL = "maybePublishPrPublicSurface(";
38+
39+
export type Violation = { rule: string; detail: string };
40+
41+
/** Byte offsets of every occurrence of `needle` in `source`. */
42+
export function occurrences(source: string, needle: string): number[] {
43+
const found: number[] = [];
44+
for (let at = source.indexOf(needle); at !== -1; at = source.indexOf(needle, at + 1)) found.push(at);
45+
return found;
46+
}
47+
48+
/**
49+
* The `[start, end)` byte range of the top-level function named `name`.
50+
*
51+
* Deliberately brace-free: it runs from the declaration to the next TOP-LEVEL declaration, which is what
52+
* "column 0" identifies unambiguously in this file's formatting. A brace counter would have to understand
53+
* strings, regex literals and template literals to be correct over a 13k-line file, and being subtly wrong
54+
* about the range is worse here than not checking, because a checker that cries wolf gets muted.
55+
*/
56+
export function topLevelFunctionRange(source: string, name: string): { start: number; end: number } | null {
57+
const declaration = source.indexOf(`\nasync function ${name}(`);
58+
if (declaration === -1) return null;
59+
const start = declaration + 1;
60+
const next = source.slice(start + 1).search(/\n(?:export )?(?:async )?function /);
61+
return { start, end: next === -1 ? source.length : start + 1 + next };
62+
}
63+
64+
/**
65+
* The enclosing top-level function for a byte offset: the last `function` declaration at column 0 before it.
66+
* Used to answer "which entry point is this maybeRunAgentMaintenance call in", so a caller's own body -- not
67+
* the whole file -- is what gets searched for the matching publish call.
68+
*/
69+
export function enclosingFunction(source: string, offset: number): { name: string; start: number } | null {
70+
const before = source.slice(0, offset);
71+
const matches = [...before.matchAll(/\n(?:export )?(?:async )?function (\w+)\(/g)];
72+
const last = matches.at(-1);
73+
if (!last || last[1] === undefined || last.index === undefined) return null;
74+
return { name: last[1], start: last.index + 1 };
75+
}
76+
77+
/** Every violation of the four rules above, in the order they are numbered. */
78+
export function findViolations(source: string): Violation[] {
79+
const violations: Violation[] = [];
80+
const guards = occurrences(source, GUARD_CALL);
81+
82+
if (guards.length !== 1) {
83+
violations.push({
84+
rule: "one call site",
85+
detail: `stableVerdictBackoffEngaged is called ${guards.length} time(s); the choke point is exactly one.`,
86+
});
87+
// Rules 2 and 3 are about THE call site, so they cannot be evaluated without one.
88+
if (guards.length === 0) return violations;
89+
}
90+
91+
const chokePoint = topLevelFunctionRange(source, CHOKE_POINT_FUNCTION);
92+
if (chokePoint === null) {
93+
violations.push({ rule: "choke point exists", detail: `${CHOKE_POINT_FUNCTION} was renamed or removed; this checker needs updating with it.` });
94+
return violations;
95+
}
96+
97+
const guard = guards[0]!;
98+
if (guard < chokePoint.start || guard >= chokePoint.end) {
99+
violations.push({ rule: "guard is inside the choke point", detail: `the guard is outside ${CHOKE_POINT_FUNCTION}, so it is guarding a pass entry again.` });
100+
} else {
101+
for (const marker of DERIVATION_MARKERS) {
102+
const derivation = source.indexOf(marker, chokePoint.start);
103+
if (derivation !== -1 && derivation < chokePoint.end && derivation < guard) {
104+
violations.push({ rule: "guard precedes the derivation", detail: `\`${marker}\` runs BEFORE the guard, so a backed-off pass still pays for it.` });
105+
}
106+
}
107+
}
108+
109+
for (const call of occurrences(source, MAINTENANCE_CALL)) {
110+
const entry = enclosingFunction(source, call);
111+
if (entry === null) continue;
112+
if (!source.slice(entry.start, call).includes(PUBLISH_CALL)) {
113+
violations.push({
114+
rule: "entry points route through the choke point",
115+
detail: `${entry.name} runs the maintenance pass without deriving its gate through ${CHOKE_POINT_FUNCTION}, so it bypasses the backoff.`,
116+
});
117+
}
118+
}
119+
120+
return violations;
121+
}
122+
123+
function main(): void {
124+
const violations = findViolations(readFileSync(PROCESSORS, "utf8"));
125+
if (violations.length === 0) {
126+
console.log("check-verdict-backoff-chokepoint: the verdict-stability backoff guards exactly one derivation choke point.");
127+
return;
128+
}
129+
process.stderr.write(`check-verdict-backoff-chokepoint: ${violations.length} violation(s) of the #10227 choke-point invariant in ${PROCESSORS}:\n\n`);
130+
for (const violation of violations) process.stderr.write(` [${violation.rule}] ${violation.detail}\n`);
131+
process.stderr.write(
132+
"\nThe backoff must guard the point a pass DERIVES a verdict, not the entry of a pass. Guarding an entry\n" +
133+
"suppresses everything the pass owed -- bounded retry chains, one-shot markers, budgets -- and then every\n" +
134+
"one of them needs an exemption it has to remember to ask for (#10204 truncated #10061's recapture chain\n" +
135+
"from 5 attempts to 3 exactly this way). Put the work a pass OWES above the guard, and the verdict\n" +
136+
"derivation below it, and no future chain needs to know the guard exists.\n",
137+
);
138+
process.exit(1);
139+
}
140+
141+
if (process.argv[1] === fileURLToPath(import.meta.url)) main();

0 commit comments

Comments
 (0)