Skip to content

Commit ac6210b

Browse files
committed
Never fold two findings that both name no rule
Found by the review pass the plan schedules after this cluster. The fold compared rule codes for equality, and two absent codes are equal, so a deletions-stage finding citing no rule could be folded into an unrelated adversarial finding that happened to overlap it in the same file. Every duplicate actually observed carried a rule code, so this was a latent path rather than a live defect. It is also the one direction this fold must never take: a surviving duplicate is noise a person can dismiss, and a folded finding is gone with nothing to notice. Two findings at one line that both cite nothing are indistinguishable to this code, so folding them is a guess, and the guess now goes toward keeping both. Removing the guard fails the new test by name.
1 parent 5686d81 commit ac6210b

3 files changed

Lines changed: 55 additions & 9 deletions

File tree

docs/DECISIONS.md

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1636,3 +1636,22 @@ verified evidence, in writing, here.
16361636
minimum is S1's risk rationale, which disposes of nothing and refuses
16371637
nothing; it stays as it is rather than gaining a constraint that could fail
16381638
a run over a cosmetic field.
1639+
- 2026-08-04 REVIEW ROUND (after the WP-A to WP-F cluster, per the plan's
1640+
cadence): a pass over the day's eight behaviour-changing commits. One
1641+
finding, fixed in the same pass.
1642+
Finding 1: the D-59 fold matched on rule code equality, and two findings
1643+
that both name no rule compared equal, so a deletions-stage finding citing
1644+
nothing could be folded into an unrelated adversarial one at the same line.
1645+
The observed duplication always carried rule codes, so this was a latent
1646+
path rather than an active defect, but it is the one direction this fold
1647+
must never take. A finding naming no rule is now never folded; the test
1648+
fails by name when the guard is removed.
1649+
Checked and sound: the literal-pattern fallback never changes a valid
1650+
regex's meaning; the fold cannot merge across repositories, since ledger
1651+
paths carry their slug; the deleted-module finding, which S4 raises by
1652+
design, is not foldable because no adversarial finding exists in a file
1653+
outside the change set; the rule-code reader strips a word and its
1654+
punctuation, never digits, so "Rule 1a" against a ruleset holding "1" is
1655+
kept rather than snapped; and the private-material scan over untracked
1656+
files did not grow the file count, so .gitignore is doing the excluding.
1657+
`./verify.sh --build --e2e` exited 0 over the whole cluster.

src/server/review/pipeline.ts

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -489,21 +489,28 @@ export async function runReviewPipeline(input: PipelineInput): Promise<PipelineR
489489
// candidates, each one twice, and the person deciding them would have
490490
// decided every finding in the review twice (D-59).
491491
//
492-
// The match is deliberately narrow: same file, same rule, and line ranges
493-
// that overlap. Folding on position alone would merge two genuinely
492+
// The match is deliberately narrow: same file, same named rule, and line
493+
// ranges that overlap. Folding on position alone would merge two genuinely
494494
// different findings that happen to cite one line, and losing a real
495495
// finding silently is the one failure this pipeline exists to prevent.
496496
// A duplicate that survives because the two stages spelled the rule
497497
// differently is visible noise; a fold that ate a finding is not.
498+
//
499+
// A finding that names no rule is never folded. Two such findings at one
500+
// line are indistinguishable to this code, so folding them would be a
501+
// guess, and the direction to guess in is the one that keeps both.
498502
const foldedIntoEarlier: string[] = [];
499503
for (const finding of deletions.findings) {
500-
const alreadyRaised = candidates.find(
501-
(existing) =>
502-
existing.filePath === finding.path &&
503-
existing.ruleCode === finding.ruleCode &&
504-
existing.lineStart <= finding.lineEnd &&
505-
finding.lineStart <= existing.lineEnd,
506-
);
504+
const alreadyRaised =
505+
finding.ruleCode === null
506+
? undefined
507+
: candidates.find(
508+
(existing) =>
509+
existing.filePath === finding.path &&
510+
existing.ruleCode === finding.ruleCode &&
511+
existing.lineStart <= finding.lineEnd &&
512+
finding.lineStart <= existing.lineEnd,
513+
);
507514
if (alreadyRaised) {
508515
foldedIntoEarlier.push(`${finding.path}:${finding.lineStart}`);
509516
continue;

tests/server/review/pipeline.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,26 @@ describe("a finding two stages both report", () => {
311311
expect(result.candidatesRaised).toBe(2);
312312
});
313313

314+
it("keeps a finding that names no rule, rather than guessing it is the same one", async () => {
315+
// Two findings at one line that both cite nothing are indistinguishable
316+
// here, so folding them would be a guess. The direction to guess in is
317+
// the one that keeps both: a duplicate is noise, a lost finding is not.
318+
const result = await run({
319+
...BASE_ANSWERS,
320+
s3_adversarial: {
321+
...COMPLETE_ADVERSARIAL,
322+
findings: [{ ...COMPLETE_ADVERSARIAL.findings[0]!, ruleCode: null }],
323+
},
324+
s4_deletions: {
325+
findings: [restated({ ruleCode: null, issue: "Also unattributed, also line 2." })],
326+
reviewedDeletions: [],
327+
},
328+
s5_verification: { verdicts: [] },
329+
});
330+
331+
expect(result.candidatesRaised).toBe(2);
332+
});
333+
314334
it("keeps a finding at a line the earlier one does not cover", async () => {
315335
const result = await run({
316336
...BASE_ANSWERS,

0 commit comments

Comments
 (0)