From 26009c274b6d053e6aa2de8ab7d335015d225731 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Fri, 7 Aug 2026 09:50:19 +0100 Subject: [PATCH] docs(decisions): a judge's PASS is cached at the altitude of the question it answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records the ruling behind #360, which shipped without its why. New axis judge-verdict-cache-scope: completeness judges the commit MESSAGE's claims, so its confident PASS is keyed on branch + normalised message + brief rather than the staged bytes — a retry that reshapes the diff for a DIFFERENT reviewer is not re-judged. The Negative is on the record: a retry that guts claimed functionality under an unchanged message is not re-caught, and the message is the only guard. Filed as its own axis rather than a note under ship-gates-converge-not-restart: that axis rules that retries converge, this one rules what a verdict is scoped TO. Cross-linked both ways. Also notes the prewarm on review-gate-in-chain (placement, not scope): completeness now starts in parallel with the fleet at pre-commit on the ship path, and commit-msg re-judges from cache. Co-Authored-By: Claude Opus 5 (1M context) --- docs/decisions/INDEX.md | 1 + docs/decisions/judge-verdict-cache-scope.md | 21 +++++++++++++++++++++ docs/decisions/review-gate-in-chain.md | 1 + 3 files changed, 23 insertions(+) create mode 100644 docs/decisions/judge-verdict-cache-scope.md diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index fa3a59a9..02920f2f 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -36,6 +36,7 @@ timeline. New rationale lives in the per-axis file. | [gate-opt-out-is-visible-and-detectable](gate-opt-out-is-visible-and-detectable.md) | Fail-open stays the default; the SILENCE around it does not. Three layers. (1) DETECT: a doctor check (cli/lib/doctor/guard-config-checks.mts) reports DRIFT when the resolved indexPath is null AND either .search-code/index.db is on disk OR .devkit/config.json recorded searchCode. It is gated on the dup guard specifically (the only gate that reads the index), skipped when guard.config.json is itself MISSING/unparseable (same root cause, one line), and treats an EXPLICIT null indexPath value as a declared opt-out — resolveGuardConfig collapses absent and explicit-null, so the raw file is the one place they differ, which yields an escape hatch with zero new config surface. SEARCH_CODE_DB is checked directly because it is matcher-only and never reaches resolveGuardConfig. The fixable flag is CONDITIONAL on the recorded selection, since selectionFlags emits --search-code only for a repo that already has it — a warning promising a repair init cannot perform would never clear. It is a CheckResult in default mode and an ADVISORY line in overlay/self-host, which short-circuit before collectResults; without that the check would be unreachable in the devkit repo itself. (2) REPORT: runDeterministic collects fail-open gates into a skipped list, names them on GREEN runs as loudly as red, and emits one gate_result with status could_not_run per skipped gate so the rate becomes measurable at all. (3) BLOCK: GUARD_DETERMINISTIC_STRICT=1 turns an opt-out into label(could-not-run) and exit 1. Strict does NOT flip failOpen2: that flag is a property of the GATE (exit 2 is an opt-out for this one), strict is a property of the RUN (what we do about an opt-out), which is what keeps an --extra command fatal exit 2 at (unexpected:2) instead of relabelling it as a chosen skip. prefixCacheScope salts on strict for the same anti-laundering reason the coverage salt exists — a non-strict all-green key would otherwise be HIT by a later strict run of the identical tree, skipping every gate. | The co-occurrence matcher fails open when guard.config.json has no … | 2026-08-05 | | [gate-telemetry-self-describing](gate-telemetry-self-describing.md) | Gate telemetry is self-describing. (1) Every emitted event identifies its origin: the ship envelope stamps repo/branch from DEVKIT_SHIP_REPO/DEVKIT_SHIP_BRANCH exported by the ship script, degrading to empty strings rather than guessing. (2) Every judgement outcome emits — INCLUDING the non-outcomes: a cache hit emits cache_hit, a lost cache write emits cache_write_failed. (3) Those cheap outcomes get their OWN event type, never a synthetic row on an existing one, and carry the SAME judge label judge_exec uses — so hit rate is cache_hit/(cache_hit+judge_exec) grouped by judge, with no join and no inference. | Story sc-1239 was filed, triaged and nearly implemented against num… | 2026-07-25 | | [gate-verdict-attribution](gate-verdict-attribution.md) | A verdict-producing gate emits its INPUTS and its non-runs, not only its outcomes. (1) Every SELECTED reviewer emits review_scope BEFORE the judge runs — files, diff_sha256, file count and hash, and cached — so a cache hit is attributable on the same footing as a live run. (2) Every reviewer that did NOT run emits review_skipped with the reason (gate_disabled, no_llm, GUARD_REVIEW_SKIP, not_selected), so non-selection is a row and never an absence. (3) Every verdict carries prompt_identity: the same hash formula as the review-mode packaged preflight, computed over the SYNCED consumer assets on the commit/ship path, so a production rate and a bench baseline are comparable when the bytes match. (4) The per-lens checklist vector rides on review_result INCLUDING the passes, with each failing lens attributed to what the gate did with it (blocking, waived, dropped_out_of_charter). Absent items means no artifact — a different fact from an artifact with zero failures. | Reviewer telemetry recorded verdicts but never what produced them, … | 2026-07-27 | +| [judge-verdict-cache-scope](judge-verdict-cache-scope.md) | A confident PASS is cached at the altitude of the question the judge answers, not uniformly on the evidence bytes. Completeness judges the commit MESSAGE's claims against what the change delivers, so its PASS is additionally keyed on branch + normalised message + reviewer brief (version-salted), beside the byte-exact key. A retry that reshapes the diff to satisfy a DIFFERENT reviewer, on the same branch under the same message, is not re-judged. A FAIL is never sticky. Message normalisation mirrors git --cleanup=whitespace so the ship's composed temp file and git's COMMIT_EDITMSG compute one identical key across the two hooks. | The completeness gate is straight opus (mean 263s, max 1803s over 1… | 2026-08-07 | | [new-optional-component-offer](new-optional-component-offer.md) | A single registry, OPTIONAL_COMPONENTS in components.mts, drives a generic upgrade step (3c) that offers every optional component a repo has never been asked about. 'Never asked' is detected by the ABSENCE of the recorded key, not a falsy value: applyInit writes every component key on every run, so a repo that answered — yes OR no — carries the key and is never asked again, while a repo predating the component has no key at all. No per-repo 'offers made' state. Nothing is ever auto-added: non-TTY REPORTS only, matching the step-3 gates policy, because an opt-in component arriving because someone ran upgrade in CI is a defect. Correspondingly, a run where nobody was actually asked (non-TTY, or a cancelled prompt) passes those ids to applyInit as InitPlan.undecided, and EVERY config writer — applyInit and applyOverlay alike — runs dropUndecided, so their keys stay absent; otherwise step 4's broad refresh records the normalized 'false' as a decision nobody made and suppresses the offer permanently. The interactive wizard seeds an already-installed optional component into its defaults for the same reason: accepting the defaults on a re-run must not silently drop one. | devkit ships opt-in components after most consumer repos are alread… | 2026-07-29 | | [non-devkit-asset-collision-preserve](non-devkit-asset-collision-preserve.md) | a sync treats a name as the CONSUMER's (preserve, never clobber) iff it (1) exists under a target surface, (2) is NOT recorded in devkit's prior manifest, AND (3) its on-disk bytes DIVERGE from the bundle. Default everywhere is PRESERVE. `devkit init` interactive offers a per-asset `multiselect` (keyed `${kind}:${name}`) to adopt specific collisions; `--force` (package/standalone/overlay) and the standalone `sync-skills`/`sync-agents --force` adopt all. A preserved name is left off the manifest (devkit never claims a file it didn't write). devkit's OWN copies — manifest-owned, or unmanifested-but-byte-identical to the bundle — keep overwriting, so version-bump propagation and self-dogfood are intact. `clean`'s no-manifest fallback gains the same content/tracked guard so it never deletes a preserved untracked user asset. | the sync step (syncSkills/syncAgents/syncHookScripts) hardcoded `wr… | 2026-06-30 | | [open-ended-reviewer-gold-slots](open-ended-reviewer-gold-slots.md) | The completeness bench scores the reviewer against per-case GOLD SLOTS (gaps that must surface, each with target severity) plus DECOYS (recorded decisions / out-of-scope items it must not flag), mapped by an LLM matcher that asks one forced-choice question per slot (never one holistic list-to-list call), votes majority-of-K, and is itself audited (committed labels, Cohen's kappa >= 0.7 to be trusted) and hashed into the baseline (matcherHash) so a matcher edit invalidates comparisons exactly like a gate edit. Headline metrics are gap recall (hard floor) and decoy false-flag rate (hard ceiling); severity calibration is warn-tier; the flip gate clusters by CASE because slots within a case share one reviewer transcript. | A prompt/model edit to the 18KB feature-completeness-reviewer brief… | 2026-07-05 | diff --git a/docs/decisions/judge-verdict-cache-scope.md b/docs/decisions/judge-verdict-cache-scope.md new file mode 100644 index 00000000..bc9068c0 --- /dev/null +++ b/docs/decisions/judge-verdict-cache-scope.md @@ -0,0 +1,21 @@ +--- +slug: judge-verdict-cache-scope +created: 2026-08-07 +--- + +# judge-verdict-cache-scope + +## Target · 2026-08-07 — A judge's PASS is cached at the altitude of the question that judge answers + +**Context:** The completeness gate is straight opus (mean 263s, max 1803s over 198 calls, 1-6 Aug) and its PASS was keyed on the exact staged bytes, so ANY retry re-judged from scratch. Ship telemetry shows ~4.5 attempts land one ship and 143 of 150 review-gate runs recorded cache_state=none, so a single landed change paid the gap-finder up to five times over for a claim that never changed once. It was the largest single opus line item in the gate chain (~14h of opus in six days) and the cost was pure repetition: the reviewer whose finding forced the retry was never completeness. +**Ruling:** A confident PASS is cached at the altitude of the question the judge answers, not uniformly on the evidence bytes. Completeness judges the commit MESSAGE's claims against what the change delivers, so its PASS is additionally keyed on branch + normalised message + reviewer brief (version-salted), beside the byte-exact key. A retry that reshapes the diff to satisfy a DIFFERENT reviewer, on the same branch under the same message, is not re-judged. A FAIL is never sticky. Message normalisation mirrors git --cleanup=whitespace so the ship's composed temp file and git's COMMIT_EDITMSG compute one identical key across the two hooks. +**Consequences:** +- Positive: Opus completeness is paid once per branch+message across an entire retry chain instead of once per attempt, so the gap-finder's cost stops scaling with how many times an unrelated reviewer blocks the commit. +- Negative: A retry that GUTS claimed functionality while keeping the same commit message is not re-caught on that branch: the message is the only guard. Byte-exact re-judging would catch it and we knowingly gave that up. Amending the message, switching branch, or editing the brief re-opens the gate, and a FAIL always re-judges. +**Vision-fit:** n/a — internal tooling (devkit gate chain). +**Researched:** Measured from ~/.devkit/telemetry/gate-events.jsonl: per-gate makespan (review 260s mean vs deterministic 11s), 198 completeness judge_exec rows 1-6 Aug, cache_state distribution across 150 review-gate runs, and 468 ship attempts against 103 successes. +**Rejected:** (a) Keep byte-exact keying — SAFE but it IS the status quo whose cost forced this: every retry re-pays ~4min of opus for an unchanged claim. (b) Key on the diff minus the failing reviewer's files — UNSOUND: a fix routinely edits files outside that reviewer's domain, so the key still churns while quietly dropping real changes out of the judgement. (c) Run completeness only on the final attempt — IMPOSSIBLE: which attempt is final is unknowable in advance, and a blocked attempt is exactly when a gap-finder earns its keep. +**Anchored-bet:** [BET] +**Revisit-when:** A same-message retry is observed shipping a gap completeness had already passed, or per-judge cost instrumentation (sc-1527) shows completeness is no longer a material share of ship spend. +**Scope:** gate-engine/review/completeness.mts +**Source:** collab · PR #360 diff --git a/docs/decisions/review-gate-in-chain.md b/docs/decisions/review-gate-in-chain.md index 9fd27d6a..c080876e 100644 --- a/docs/decisions/review-gate-in-chain.md +++ b/docs/decisions/review-gate-in-chain.md @@ -53,3 +53,4 @@ created: 2026-07-02 - 2026-08-04 — sc-1474: renderTargets measures its cap in UTF-8 bytes (Buffer.byteLength), not UTF-16 chars — the 8KB cap guards argv size and this repo's em-dash-dense rulings under-counted by ~3x, silently exceeding the documented cap with no OMITTED note. - 2026-08-04 — sc-1476: checklist-contract recovery stays REVIEW-ONLY — only its SCHEDULING changed. Root cause (reproduced): haiku judge compliance with the mechanical checklist workflow degrades under concurrent judge load; the inline retry re-ran inside the same contended wave and re-failed. The one recovery attempt per reviewer now runs in a serial post-wave phase (recovery/settle.mts), bounded by the remaining SHIP_COMMIT_TIMEOUT ceiling with a NAMED budget skip; a parked reviewer is not marked completed until its recovery settles (kill mid-phase converges); recovered outcomes carry retried/retry_phase in review_result and per-part in lens_parts so the field rate stays measurable (gate-telemetry-self-describing), never in any cache key. Commit/ship paths never had a checklist retry and keep none. - 2026-08-05 — sc-1317: an autonomous report claiming completeness "hangs indefinitely" (90s of silence before an impatient interrupt) does NOT reproduce — that's inside the documented ~7min typical uncached-run latency, nowhere near DEEP_JUDGE_TIMEOUT_MS. But chasing it surfaced real, separate gaps in the shared exec helper. (1) execJudge/execJudgeAsync's timeout relied on execFile/execFileSync's default killSignal (SIGTERM), which a child can trap or ignore — sc-1227 single-sourced the cap's VALUE but never guaranteed its ENFORCEMENT. Both now pass killSignal: 'SIGKILL' (mirrors the existing co-occurrence/index-refresh.mts precedent); isJudgeTimeout's err.killed === true check is unaffected (Node sets it on any timeout-kill regardless of signal), so outage classification and the timeout/sync/outage remedy split are unchanged. (2) execJudgeAsync had no try/catch around its own execFile() call, unlike its sync execJudge twin — a synchronous throw (e.g. an out-of-range timeout value, reproduced with timeout: -1) escaped as a REJECTED promise, breaking the function's own documented 'never throws, always resolves null' contract for any unguarded awaiter (completeness.mts's own await exec(...) call sits outside its try/catch). Fixed by wrapping the executor body in try/catch, routed through the same outage-classification path the callback error branch already used. (3) ExecJudgeOpts.timeout flips from optional to required — Node treats an omitted/0 timeout as 'no cap enforced at all', which would silently defeat the whole SIGKILL guarantee; every real caller already supplied one, so this closes the gap at the type level with no behavior change. Also documented (test-only, no code change — no real caller is exposed): timeout: 0 disables the cap rather than killing instantly, and timeout: Number.MAX_SAFE_INTEGER silently overflows Node's 32-bit timer to ~1ms and kills almost immediately, the opposite of a caller's likely intent. Follow-up not in scope here: whether a claude -p judge's own MCP-subprocess children could survive killing just the immediate process (would need detached + process-group kill to close, unverified whether it's a real risk on this path). +- 2026-08-07 — PR #360 — on the ship path completeness no longer waits for commit-msg: the pre-commit review fragment starts it in PARALLEL with the reviewer fleet (armed only by the sc-1442 message file, so interactive commits and devkit review are unchanged) and applies the identical exit contract there. commit-msg then re-judges it from cache. Its PASS scope is ruled by [[judge-verdict-cache-scope]]. The cancelled-judge path had to background a SIMPLE command, not the __dk_no_git_env function: backgrounding a function forks a subshell, so $! was a wrapper and the judge survived kill+wait as an orphan still holding git's output pipe.