Skip to content

feat(review): merge findings across producers and mark consumption explicitly (ADR 0010) #2678

Description

@kyle-sexton

Phase 1 of the boris-routines-adoption plan. Implements ADR 0010 — this issue does not re-decide it.

Blocks every other phase. review:fanout is findings producer #1, so the first detector of any kind makes producer #2, and the fix action consumes exactly one file.

Why this is a correctness gate

plugins/review/skills/fanout/context/fix-pass-mode.md:3 — consume "the newest persisted findings file for the CURRENT branch". :7 takes the newest *.md by filename sort and stops. No merge, union, or multi-file handling exists anywhere in the fanout tree.

Two conforming producers on one branch means the later timestamp silently wins and the earlier producer's findings are never applied — no error, no warning, run reports success. A detector running after a full review shadows the entire review. This is the green-with-hidden-findings class docs/conventions/liveness-assertion/README.md:58 exists to prevent, and it fails silently rather than loudly.

Two changes, not one

1. The applied-plan record becomes a consumption ledger. fix-pass-mode.md:76 writes it only under --yes in a non-interactive session — "Interactive and headless-stop paths write no record". Any bound anchored on it is a no-op on the interactive path, so the merge set would grow without limit and re-inject findings that :95's required post-fix re-review already resolved. Write it on every apply path; source-findings: carries the full consumed set.

2. The locator becomes a merge set. All type: review-findings files whose branch: equals the current branch exactly, minus every file named by any source-findings: of a record whose own branch: also matches exactly. The branch filter binds both sides — :7 calls it load-bearing because the directory slug is lossy, and an unfiltered record from a slug-collided branch silently truncates the set.

Work items

  • Pre-flight consumer check (FIRST). grep -rn "review-findings\|fix-pass-record\|source-findings" --include='*.md' --include='*.sh' --include='*.py' . — enumerate every parse path inside and outside plugins/review/, record them before editing.
  • Change 1 — unconditional record, consumed-set source-findings:.
  • Change 2 — merge set by consumption marking, exact-branch filter on both sides.
  • Dedup is presence-only: collapse rows sharing identical Location and identical Finding text; everything else stays distinct with its producer named in Surface(s). State in the doc why this is narrower than Stage 3's key — findings-normalization.md:77 puts dedup at "Stage 3 Sonnet (semantic merge)" and :66 orders "Minimize FALSE-MERGE over FALSE-SPLIT". The fix action runs no LLM stage, and the ±3-line bucket would merge distinct defects at foo.ts:42 and foo.ts:44, dropping one remediation.
  • Union ## Unparsed by concatenation; union ## Surfaces with each producer named; report each consumed file's tier: rather than picking one. default-mode.md:77 declares these required "to keep the report honest about coverage".
  • Step 3 plan header names the consumed file set, not a single path.
  • Document the degenerate cases: empty set keeps today's clean STOP; a single file reduces byte-for-byte to today's behavior.
  • Tolerate the conditional > DEGRADED: blockquote above ## Findings (run-everything-mode.md:163-168).
  • One sentence on the shared-directory case: a memory_dir resolving outside the worktree serves several worktrees on different branches; the exact-branch: filter on both sides is what keeps that correct.
  • Version bump plugins/review/.claude-plugin/plugin.json (0.19.0 to 0.20.0) and the matching CHANGELOG entry in the same PR.

Sanity checks

  • scripts/check-changelog-parity.sh --check-bump origin/main exits 0. (Bare invocation exits 2 — usage.)
  • scripts/check-changed-skills.sh origin/main exits 0. Editing context/fix-pass-mode.md makes fanout a changed skill — check-changed-skills.sh:52-56 maps any path under plugins/<p>/skills/<s>/ to the skill dir — so the full static gate runs; only --require-evals is SKILL.md-scoped.
  • grep -c "fix-pass-record" plugins/review/skills/fanout/context/fix-pass-mode.md >= 3.
  • Two conforming fixtures, different timestamps, one branch directory: the plan lists findings from both and names both files in its header.
  • After an interactive apply against fixture A, a re-run with A and a newer B present lists only B — proving the ledger is written on the interactive path.
  • A record whose branch: differs from the current branch excludes nothing.
  • Two rows at foo.ts:42 and foo.ts:44 with different Finding text both survive (false-merge guard).

Related

  • ADR 0010 (the decision this implements)
  • Blocks: the detector-findings convention stub, and every detector phase

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageNot yet classified. Floor until a type and one priority tier are set.status: readyTriaged, unblocked, and fully specified; eligible to pick up.work-class: structuralRefactors, migrations, contract changes; cross-cutting and hard to reverse.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions