rfc(fabrika): deterministic diff filtering, scope preview, and path-scoped rules in review gate

[creosB]

Summary

Examining production review pipelines (notably Alibaba's open-code-review), three deterministic mechanisms fit fabrika's contract-first architecture and address concrete noise, token overhead, and path-specific precision in fabrika:review:

1. Deterministic Diff Filtering (Noise Exclusion)

   • Problem: Full diffs pass directly to review models, burning tokens on lockfiles (pnpm-lock.yaml), snapshot files, and generated artifacts.
   • Proposal: Route an exclude filter through path derivation so consumers stay consistent.
   • Shipped defaults: Lockfiles (pnpm-lock.yaml, lockfile churn), test snapshots (__snapshots__/**), and generated schema/build outputs. Repositories can extend freely via .fabrika.jsonc; removals of defaults are permitted but explicitly enumerated in scope output (un-excluded: pnpm-lock.yaml by repo config) to maintain visibility.
   • Consumer Split vs. Trigger Safety:
     • Requirement-deriving consumers (touchesGovernanceRoot, lane prove namespace derivation, portability-guard triggers, ship scope) must read the raw path list so filtering cannot dodge gates (e.g. packages/fabrika-cli/src/ is a portability trigger but not a governedRoot; filtering packages/** must never bypass the guard).
     • Content-delivery consumers (review diff, preview) read the filtered path list.
     • As defense-in-depth, the CLI will refuse any exclude pattern that intersects governedRoots ∪ guard trigger trees.
     • (Note: other changed-path consumers like build's leak/prose surfaces exist; while out of scope here, the hook design shouldn't preclude applying this same split there later.)
   • Dep-Bump PRs & Filter Placement Fork:
     • Dep-bumps often touch only pnpm-lock.yaml. How they resolve hinges on placement:
       • Branch A (filter before namespace derivation): Zero namespaces are owed on lockfile-only diffs; scope outputs the exclusion enumeration and no reviewer is spawned. Safety rests entirely on the non-excludable trigger set.
       • Branch B (filter after namespace derivation): A code namespace is derived, but content is empty. Rather than stalling in UNKNOWN/park or inventing a new terminal state, scope emits an excluded row following the existing routed pattern.
   • Filter vs. Truncation Distinction: review diff must clearly distinguish deliberately excluded paths from accidentally truncated reads via a machine-readable excluded-paths header.

2. Pre-Flight Inspection (fabrika review preview)

   • Proposal: A dedicated, token-free CLI verb sharing the filtered path-derivation logic with phoenix product MVP — sozluk + pano + profile, view layer + outbox + Workflows #1, pinning the target tree with zero LLM turns.
   • Parity & Scope: Supports both single PR ($pr --sha) and range arms (--base/--tip) for parity with existing review verbs.
   • Sequencing Note: In Phase 1, preview reports matched and excluded files; rule fragment output remains empty until d1-direct: defer DOs and workflows #3 lands.

3. Path-to-Rule Subsystem Matching

   • Problem: Artifact class rubrics exist, but paths within a class are evaluated against uniform prompts (e.g. apps/web/worker/** Effect/runtime vs apps/web/client/** React).
   • Proposal: Map file globs in .fabrika.jsonc (via generated fragments in packages/fabrika-cli/src/config/keys/, noting this will fire the governance skill). review scope resolves matching fragments deterministically alongside namespace rows.
   • Composition: Subsystem fragments serve strictly as additive constraints layered onto base class rubrics (code, doc, skill, ui).

(Related, out-of-scope note: A prompt reflection pass re-quoting offending lines before review post is high-ROI precision work, but lives purely in skill text without requiring a CLI contract.)

Measured locally (ocr-port benchmark, 2026-09-13)

Measured motivation. We classified every merged PR from the last six months by changed-file paths (n=2,852). Exactly two are dependency-only; zero touch only pnpm-lock.yaml; the default exclusion set matches content on 1 of 9 sampled PRs. With the exclusion scaffold trimmed (zero-exclusion runs drop the scope-rows block), the filter is strictly non-negative: unchanged (a fixed +43-token header, under half a percent of prompt even on the smallest PR) on the eight PRs with nothing to exclude, -804 tokens on the one PR that carried a lockfile hunk -- -460 tokens net across the nine-PR sample (o200k_base, exact counts). The token case is roughly neutral and we are not claiming it.

The case that survives is semantic, not economic. (a) The one excludable hunk was real noise the reviewer read as code context. (b) The dependency-PR question is real regardless of token counts: both dep-only PRs in the frame have no verifiable acceptance-criteria block, and the before/after fork decides what a contract-less, all-excluded diff means to the merge gate -- zero namespaces (nothing to review) versus namespace rows beside a deliberate-exclusion enumeration (something was there, and was excluded on purpose, distinct from truncation).

Verdicts are proxies from a simulated reviewer (skill text + diff, no CI/governance arms). They are single samples with no error bars; on eight of nine PRs the reviewed input is byte-identical across arms, so any verdict cell there is harness variance, not filter effect (observed: one clean PRs defect flag flipped between two runs on identical input). Verdicts stay out of the table below; the one filter-attributable data point (#535: defect flagged under both placements) is stated in prose.

PR	raw diff B	tokens base	tokens A (delta)	tokens B (delta)	excluded paths
#9193	16,575	14,691	14,734 (+43)	14,734 (+43)	0
#9190	11,038	11,964	12,007 (+43)	12,007 (+43)	0
#9170	10,014	11,733	11,776 (+43)	11,776 (+43)	0
#9162	61,481	25,750	25,793 (+43)	25,793 (+43)	0
#4015	47,167	21,895	21,938 (+43)	21,938 (+43)	0
#535	40,327	20,795	19,991 (-804)	19,992 (-803)	1 -- pnpm-lock.yaml
#7375	2,888	9,677	9,720 (+43)	9,720 (+43)	0
#7040	25,908	16,110	16,153 (+43)	16,153 (+43)	0
#6835	3,558	10,041	10,084 (+43)	10,084 (+43)	0
net			-460	-459	1 PR of 9

Sample stated plainly: n=9, confirmed-defect n=2, dep-only n=0. The safety claim is "zero new false negatives" and it is weakly powered -- on eight PRs the inputs are unchanged so no filter effect is possible; on #535 the input changed and the defect flag held in both placements.

One consistency note: on the eight zero-exclusion PRs the A and B token columns are identical, whereas an earlier untrimmed run differed by one token everywhere. Expected -- the only content difference between placements is the placement marker inside the exclusion scaffold, and that scaffold is dropped when nothing is excluded; the markers one-token difference reappears on #535, where the scaffold is present.

---

Key Decisions for Maintainers

1. Filter Placement: Do we filter before namespace derivation (Branch A: dep-bumps cleanly bypass reviewer invocation -- zero reviewer runs on an all-excluded diff; measured below) or after (Branch B: reviewer invoked with empty diff, emits an excluded row)?
2. Consumer Split: Consensus on requirement-derivers reading raw paths while content-delivery reads filtered paths.
3. excluded Scope Row: Conditional on Branch B being preferred above; confirmation on adopting the routed-style excluded row over introducing any new terminal state.
4. Narrow the refusal union to governedRoots only. The draft asserted the filter must refuse any exclusion pattern intersecting governedRoots ∪ guard trigger trees. The benchmark measured that union misfiring on its own motivating population: on real dep-only PR fix(epic-ledger): drop publishConfig.provenance:true (redundant under OIDC, breaks non-CI publish) #399, excluding a package.json glob refused at exit 17 because it grazed catalog-guards corpus -- blocking a legitimate policy choice about review scope, on the exact PR class the filter exists to serve. To be explicit: the package.json glob was a probe of the refusal path, not a proposed default -- the proposed default excludes remain lockfiles, snapshots, and generated artifacts. Guards dont need this protection: they read the raw path list (the consumer split), so a review-side exclusion cannot blind them, and narrowing therefore adds no risk. We recommend the union narrow to governedRoots only, with the guard trees kept documented and drift-loud by a golden test. The narrowed union also makes the fork observable on a real PR: fix(epic-ledger): drop publishConfig.provenance:true (redundant under OIDC, breaks non-CI publish) #399 with the pattern accepted becomes all-excluded, and the two placements demonstrably diverge -- Branch A derives zero namespaces, Branch B keeps namespace review-code beside excluded 1 -- packages/epic-ledger/package.json.

[creosB]

ADR draft is up for the four questions above:

#9476

• ADR 0401 — Review diff filtering is deterministic, enumerated, and never blinds a gate (status: proposed).
• Fields 2–4 carry the RFC recommendations as posted above; filter placement (Q1) stays an open ruling in the ADR until someone picks a branch.
• Benchmark artifacts + the claim ledger land under reports/ with the epic's first child; implementation follows through the epic's children — this PR records the decision only.