fix(panel): key the verdict cache by the full review request (P4) - #209
Merged
Conversation
…ss-request reuse (P4)
Observable-gates plan P4. The panel's verdict cache keyed only on
{treeHash, panelHash, rubricVersion, cacheVersion}, so a cached verdict was reused for
a DIFFERENT review request on the same tree: a different --base (different diff), a
different --intent (different context), or --quick (a REDUCED 1-reviewer roster, whose
panelHash is the FULL panel's) all hit the same key. A quick 1-reviewer PASS could thus
satisfy a full review — a panel-side false-green.
- verdictCacheKey now also mixes in base, intent, and mode (quick|full), so any change in
the review request MISSES the cache and forces a fresh review.
- The key is now JSON-serialized (unforgeable — escapes + array delimiting) instead of a
space-join, so a value containing a space can't slide across a field boundary and forge
a collision (same lesson as the db:push fingerprint).
NOT re-validating on a cache hit: only a real verdict is cached (⟹ validate passed), and
an identical treeHash ⟹ identical code ⟹ same validate result — re-running it would just
defeat the cache without adding safety.
Tests: base/intent/mode each change the key; unforgeable-key (a slid space can't collide
two distinct requests); existing tree-hash + cacheVersion coverage updated. Full suite green.
…raw flags (P4 round-2) Panel BLOCK (4/0, critical): keying on args.base/args.intent was still unsound — an omitted --base resolves to `merge-base main HEAD` and a named ref like `main` is resolved at review time, and an omitted --intent comes from the commit subject. So the SAME flags denote a DIFFERENT diff after main moves / a rebase, or a DIFFERENT intent after an amend — all with an unchanged treeHash → a stale PASS still shared one key and false-hit. New resolveReviewInputs(git, base, intent) resolves base→concrete sha (rev-parse, so a moved ref changes it) and intent→text (commit subject when omitted, so an amend changes it). The cache key uses those resolved values, and the SAME resolved base/intent are handed to runHarnessReview so the key and the review can never diverge. Tests: a moved base ref → different baseSha → different key; omitted intent → commit subject (amend → different key); plus the earlier base/intent/mode + unforgeable-key coverage. Full suite green.
…n-CI review path (P4 round-3)
Panel round-2 found the base-sha key still can't capture a three-dot merge-base
diff: a rebase shifts merge-base(base,HEAD) while the ref/tree/subject stay fixed,
so the reviewed diff changes but the key doesn't. It also found the --ci branch
still reviewed raw args while persisting under the resolved key, and a git-failure
fallback to the movable ref.
- resolveReviewInputs now hashes the ACTUAL reviewed diff (${base}...HEAD); a git
failure returns diffHash=null so the caller skips the cache (never falls back to
a movable ref).
- verdictCacheKey keys on diffHash (+intent+mode+panel+versions), dropping treeHash+base.
- new reviewPlan() binds the cache key AND the review request to ONE resolved object,
unifying the --ci and interactive paths so key and review can't diverge; --ci just
never reads the cache.
- tests: rebase-changes-diff, git-failure→null, reviewPlan CI-parity/quick/null-intent.
…ble orchestration (P4 round-4)
Panel round-3 found dropping treeHash removed the live-state invalidation: keying
purely on the committed ${base}...HEAD diff let a cache hit reuse a green verdict
even when the current worktree would fail validate. P4's actual requirement was to
re-run validate before trusting a cached verdict.
- new resolveVerdict() orchestrator: runs validate FRESH (caller), trusts a cached
verdict ONLY when the current gate is green; the summary is threaded into the
review (IGatherOptions.validateSummary) so validate runs exactly once. The cache
short-circuits only the expensive model panel, never the gate.
- extracted from the CLI so the wiring is unit-tested (cache-hit reuse, keyed write,
--ci writes-but-never-reads, red-validate-skips-cache, null-key no-op) — the
integration coverage the panel required.
- resolveBase now pins the base to merge-base(ref, HEAD) as an immutable SHA, so the
fingerprinted bytes and the reviewed bytes are one snapshot even if the ref moves.
- tests: resolveVerdict scenarios + gatherChange reuses a provided summary. Full suite 3178/0.
agjs
force-pushed
the
fix/panel-verdict-cache-integrity
branch
from
July 27, 2026 22:02
da93d7c to
b5b868b
Compare
…ew request (P4 round-6) Panel round-5 (full run, 4/4) converged on one root flaw: the key must fingerprint the exact request reviewers judge — diff AND full contextFiles AND the effective roster — computed from the same bytes the review uses. Incremental keys (diffHash, then base, then treeHash) each missed a dimension. Redesign — gather-then-key-then-cache: - gatherChange runs validate FRESH and builds the request first; a cache hit is only reached when the gate currently passes (P4's re-validate requirement, by construction). It now BLOCKS on a git-diff exit≠0 or an empty diff instead of building a vacuous green review (the ignored-exit false-green the panel flagged). - reviewRequestKey hashes the gathered request (diff + contextFiles + intent + rubric) + panelIdentityHash(EFFECTIVE roster + builder) + mode + CACHE_VERSION(→3). The key and the review hash the SAME object — no second git read to diverge from. This kills the contextFiles-omission, effective-vs-cfg-roster, and fingerprint/payload findings. - reviewRequest (review an already-gathered request) + decideVerdict (cache read/write, --ci writes-not-reads) extracted and unit-tested; runHarnessReview reuses them. - resolveBase pins base to merge-base(ref,HEAD); fallback + git-exit guards tested. - removed the as-cast in the test. Full suite 3173/0, typecheck+lint+format clean.
…pty-content diff (P4 round-7) Round-6 findings (all real): - reviewRequestKey omitted validateSummary though it IS in the request reviewers read (validateRunner can return passed:true with non-empty firstErrors) → false reuse across differing diagnostics. Now hashes the ENTIRE request object (same bytes reviewRequest gets), no field selection — key ≡ request literally. - gatherChange only blocked an empty FILE LIST, not an empty CONTENT diff (files listed, git diff exit 0, empty stdout — rename/mode-only). Now blocks the empty diff too. - restored the CACHE_VERSION-in-key pin test (deleted in round-6) via a recompute-mirror. - fixed a stale resolveBase comment referencing the removed diffHash. Full suite 3174/0, typecheck+lint+format clean.
…cal key (P4 round-8) Round-7 (4/4) findings, all addressed: - BLOCK: the CLI wiring had no test proving gather-before-cache / validate-fail-prevents- reuse. Extracted runReviewFlow (injectable) — gather runs BEFORE any cache access and a gather block never reads/writes the cache; the key is derived from the gathered request. Unit-tested for block-never-caches, gather-before-cache, hit/miss, --ci write-not-read. - panelIdentityHash now hashes the FULL resolved reviewer objects (model/endpoint/binary argv/timeout), not just ids — retargeting an id to a different implementation no longer reuses a verdict. Tested. - empty-content-diff guard kept but its rename justification corrected (real renames aren't empty; it's defense-in-depth) in both comment and test. - removed dead runHarnessReview + IRunDeps (CLI no longer calls them). - added a direct reviewRequest success-path test. - reviewRequestKey serializes via canonicalJson (recursive key-sort) so equal content can't thrash/diverge on object key order. - restored the CACHE_VERSION-in-key pin (recompute-mirror) + fixed stale tree-hash comments. Full suite 3177/0; typecheck+lint+format clean.
…und-9) Round-8 (4/4) findings, all addressed: - TOCTOU: pin HEAD to a SHA once (resolveHead) and read the file list, diff, subject, and context all against that snapshot — a HEAD move mid-gather can no longer mix commits into one request. resolveBase/resolveIntent/gatherContext take the pinned head. - CLI wiring now has a mirrored test: extracted buildReviewFlowDeps (in harness-review-mode) and unit-tested it — rosterHash derives from the EFFECTIVE panel, review targets it, mode/ci map from args, gather validates fresh + diffs the pinned base…HEAD, persist carries the effective rosterHash. The runReviewFlow test now asserts real call ORDER (gather→read→review→persist) and the exact key readCache/persist receive. - panelIdentityHash typed to require full ResolvedReviewer[] (no id-only caller) — the compile-time reintroduction of the false-reuse bug is closed. - added the resolveBase merge-base SUCCESS-path test (diffs MBSHA...HEADSHA). - added a canonicalJson order-independence test (same content, reordered keys → same digest). Full suite 3184/0; typecheck+lint+format clean.
…ening (P4 round-10) Round-9 (4/4) findings, all addressed: - CRITICAL: pre-push reviewed HEAD, not the pushed commit — an explicit refspec (feature:main) could push harness code the panel never saw. Added a --head <commitish> target to harness-review (default HEAD); the pre-push hook now reads git's stdin and reviews each pushed local OID with --head, skipping deletes (falls back to HEAD for manual runs). - the review target is now pinned to a SHA BEFORE validate (not after), and rev-parse's exit code is honored: a failed pin BLOCKS rather than soft-falling-back to the movable HEAD ref (which would silently re-open the TOCTOU). - house rule: removed the forbidden `as` casts from the CLI wiring test (typed ILog). - added a rev-parse-failure block test; strengthened the buildReviewFlowDeps gather test so a dropped base or frozen intent is actually caught (merge-base echoes its ref; intent asserted). Full suite 3185/0; typecheck+lint+format clean; hook: bash -n OK + executable.
…ting feature (round-11) Round-10 showed the round-10 --head/pushed-OID feature was a scope error: reviewing an arbitrary pushed commit requires validating THAT commit's tree (not the working tree) — a checkout/worktree-level change with its own base=remote_oid, artifact-tree, and hook robustness concerns. That is review-TARGETING integrity, a separate feature from P4's verdict-cache integrity, and can't be bolted on correctly here. - reverted the --head flag (harness-review + parse + buildReviewFlowDeps) and restored the pre-push hook to its original HEAD review (now identical to main → off the review surface). - KEPT the in-scope, cache-relevant HEAD-snapshot fix: gatherChange pins HEAD to a SHA once (rev-parse, exit-code-guarded → blocks on failure) and reads the file list/diff/subject/ context all against it, so the fingerprint is of one consistent commit (the round-8 TOCTOU). - KEPT all cache-integrity work: whole-request key (canonical), effective-roster hash (typed to ResolvedReviewer), gather-before-cache runReviewFlow, git-exit/empty-diff guards, and the CLI-wiring + order + merge-base-success + canonical-order tests. Review-targeting integrity (validate the pushed commit's tree; review the pushed OID, not HEAD) is recorded as a separate follow-up. Full suite 3185/0; typecheck+lint+format clean.
… round-11 advisory) Round-11 PASSED but 6 unanimous advisory findings converged on one real gap: the pin soft-fell-back to a MOVABLE ref (rev-parse empty-on-success → 'HEAD'; merge-base failure → the ref / HEAD~1), which the code's own comments say re-opens the mid-gather TOCTOU. Closed it: - new resolveSha(): a pin must be a real SHA — rev-parse with a nonzero exit OR empty stdout yields null. gatherChange blocks on a null HEAD (no soft fallback to the movable ref). - resolveBase honors merge-base's exit code and, on failure, pins the fallback ref to a SHA via resolveSha (or blocks) — the diff never runs against a movable ref, so name-only/content observe ONE snapshot. - moved the quick-mode roster slice INTO buildReviewFlowDeps so the effective-roster wiring (rosterHash + review target the sliced roster) is unit-tested. - tests: pinned-fallback range, unpinnable-fallback blocks, empty-rev-parse blocks, quick-slice roster hash; git test helper defaults rev-parse/merge-base to SHAs. Full suite 3188/0; typecheck+lint+format clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Observable-gates plan, P4 (final control). Closes the panel-side false-green.
The panel's verdict cache keyed only on
{treeHash, panelHash, rubricVersion, cacheVersion}, so a cached verdict was reused for a different review request on the same tree:--base(different diff),--intent(different context),--quick(a reduced 1-reviewer roster, whosepanelHashis still the full panel's).So a quick 1-reviewer PASS could satisfy a full review — a panel-side false-green (the reviewer inheriting a verdict it never actually produced).
verdictCacheKeynow mixes in base, intent, and mode (quick|full) → any change in the request MISSES the cache and forces a fresh review.Not re-validating on a cache hit: only a real verdict caches (⟹ validate passed), and identical
treeHash⟹ identical code ⟹ same validate result — re-running would just defeat the cache without adding safety.Tests: base/intent/mode each change the key; unforgeable-key (a slid space can't collide two distinct requests); existing tree-hash + cacheVersion coverage updated. Full suite 3164/0; typecheck + lint clean.