fix(gate): never accept a finding's disappearance as a fix - #696
Conversation
Observed on Webu-PRO/lifted.sk-storefront#440: round 1 blocked on a HIGH
in store.tsx, round 2 blocked on a different HIGH in
product-card-compact.tsx ("drops the set-total price line, so a SET
product shows a per-kg figure with no total"), the fix commit touched
only store.tsx, and round 3 simply did not report the price bug -- so
the gate printed "Merge gate: pass" with the defect still in the branch.
Verified at head 7221fbf: product-card-compact.tsx:182-192 renders the
primary line plus a compare-at line and no set total, while
store.tsx:2019-2035 renders exactly that second line.
Review providers are nondeterministic. A blocking finding that fails to
reappear is not a fixed finding.
- Track files that ever held a blocking finding, and files a repair
round actually edited.
- resolveCarriedFindings() returns files whose disappearance is
unexplained: never edited, and absent from the latest review at any
severity. A clean final verdict with unexplained files now throws.
- Contract templates (AGENTS.multiagent-safety.md + .min.md) name the
gated ship --gate-review --gate-autofix as the default completion
path, explain each gate flag, and state that posting a review is not
merging -- gx pr-review posts and exits, only gx branch finish merges.
File-level granularity is deliberate: lines shift after a repair and
provider severity varies run to run (product-card-compact.tsx:93 came
back LOW, then MEDIUM, then MEDIUM in the same PR), so a line or message
fingerprint would not survive a round. Being coarse errs toward blocking.
Only active with --gate-autofix; strictly stricter, it can turn a merge
into a block and never the reverse.
Verified: npm test failing set byte-identical to main (41 pre-existing),
biome 1.9.4 clean, openspec change + specs valid.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NagyVikt
left a comment
NagyVikt
left a comment
There was a problem hiding this comment.
Code review — PR #696
Decision: APPROVE with comments — 0 CRITICAL, 0 HIGH, 2 MEDIUM, 0 LOW.
Validation
| Check | Result |
|---|---|
| Lint (biome 1.9.4, pinned) | Pass — 189 files |
Tests (npm test) |
812/854 pass, 41 fail — failing set byte-identical to main |
node --check bin/multiagent-safety.js |
Pass |
bash scripts/check-script-symlinks.sh |
Pass |
openspec validate <change> --type change --strict |
Pass |
openspec validate --specs |
Pass — 133/133 |
The bug this closes is confirmed, not hypothetical
Verified against Webu-PRO/lifted.sk-storefront at head 7221fbf:
product-card-compact.tsx:182-192 renders the primary price line and a
compare-at line, with no set-total; store.tsx:2019-2035 renders exactly that
missing second line. The HIGH was real, the fix commit touched a different file,
and round 3's silence flipped the gate to pass. Test 5 in
gate-carry-forward.test.js reproduces that sequence and now blocks.
Correctness properties verified
- Cannot loosen the gate. The check only ever adds a throw after an
already-clean verdict. There is no path where it converts a block into a merge. - Inert without
--gate-autofix.maxFixRoundsis 0, the loop breaks on the
first round,blockedPathsis populated but never compared against a later
review. Pinned by a test. - Malformed input fails toward blocking. A null/garbage findings list yields
an emptystillReported, so every carried path reads as unexplained.
MEDIUM
-
A cross-file repair now reads as unexplained. If the correct fix for a
finding inA.tsxis an edit toB.tsx, the gate blocks even though the work
was done properly. That is the deliberate trade — the alternative is trusting
silence, which is what produced #440 — but it will produce false blocks on
legitimate indirect fixes, and the error message should probably say so more
explicitly than "re-run the review, fix by hand, or bypass". -
File-level granularity can mask a second finding. Two blocking findings in
the same file, where the repair fixes only one: the file lands in
repairedPaths, so the survivor's disappearance would still be accepted. The
round's own re-review is the backstop there (it would report the survivor), so
this only bites when the provider drops both — but the guarantee is
per-file, not per-finding, and that limit is worth stating in the docs.
Note on the template change
AGENTS.multiagent-safety.md is marker-managed and propagates into every repo
gx claude install touches, so this edit changes agent behavior fleet-wide, not
just here. The added text is instructional only — no command is executed by the
contract block itself — and it makes the review-vs-merge distinction explicit,
which is the confusion that left #445 and #440 sitting open with clean reviews.
1 participant
Summary
Test plan