feat(review): merge findings across producers and mark consumption explicitly - #2690
Merged
Conversation
…plicitly Implements ADR 0010. The findings-file shape is the whole integration contract -- nothing authenticates the writer -- so any component that persists a conforming file reaches the apply relay. That made a second producer a silent data-loss bug: the fix action took the newest *.md declaring type: review-findings and merged nothing, so a detector running after a full review shadowed the entire review with no error, no warning, and a green run. This is the failure class the liveness-assertion convention exists to prevent, and it is settled before the second producer ships rather than after. The fix action now consumes the set of conforming files for the exact current branch and unions the coverage fields, because reporting one producer's ## Surfaces line would hide a surface that ran and returned nothing -- moving the hidden-findings failure up a layer instead of closing it. Dedup is presence-only: identical Location and identical Finding text. The tempting key, normalised path plus a plus-or-minus-three-line bucket, sits behind a Sonnet semantic stage the fix action does not run, and adopting the bucket without the semantics inverts the pipeline's own minimise-false-merge rule -- two distinct defects at foo.ts:42 and foo.ts:44 would collapse, and since each row carries its own Action, one producer's remediation would be discarded with no trace. A false split adds a row an operator can see; a false merge drops a finding. The applied-plan record becomes the consumption ledger and is written on every path that applies. It was headless---yes-only, so a bound anchored on it was a no-op on the dominant interactive path and the merge set would have grown without limit, re-injecting findings the required post-fix re-review had already resolved. source-findings is pinned as a YAML block sequence of consumed file names -- always a sequence, never a bare scalar, because a writer emitting a scalar where the reader expects a sequence under-matches silently and re-consumes what the record was written to retire. Names rather than paths, because the resolved memory_dir can differ between the session that wrote the findings and the session that applies them, while the branch findings directory is a single home with unique timestamped names. The exact-branch filter binds records as well as candidates, so a slug-collided branch's record cannot truncate the set. Consumption is per file, not per row: rows surfaced by the low-confidence fence or narrowed out by the operator are named in the record body and recovered by re-running the review, never by re-consumption. Documented rather than left for a reader to discover, since a reader implementing "exclude the consumed file" would otherwise reasonably conclude those rows vanish. A one-file set reduces to the previous behaviour byte-for-byte and an empty set keeps the clean STOP; eval 23 flips (an interactive apply now writes a record) and cases 24-27 pin the two-producer merge, the ledger subtraction on the interactive path, the both-sides branch filter, and the false-merge guard. Closes #2678 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 3m 15s —— View job Review in progress
|
|
Claude finished @kyle-sexton's task in 56s —— View job Security review —
|
|
Last security-reviewed head: |
|
Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94b0f68ca9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A fresh-context reviewer audited the diff against ADR 0010 with the rationale withheld and found four blocking defects. All four are prose-contract bugs: this skill is read and executed by an agent, so an instruction that can be read two ways is a real defect. Recovery from a deferred row claimed more than it delivers. Step 5 said rows surfaced or narrowed out are recovered by re-running the review, but re-running this skill re-fans-out this skill's own reviewers -- it regenerates nothing a script detector produced. Since consumption is per file, an operator narrowing to "only the correctness ones" retired a detector's cleanup rows permanently, and pre-change that loss was impossible on the interactive path because no record was written at all. Recovery now names the row's own producer as the thing to re-run, and the record body must attribute every deferred row to the file it came from, which is what tells the operator which producer that is. The ## Surfaces union was computed and then discarded. Step 2 ordered the union, but neither the plan template, the report bullets, nor the record body had a slot to print it -- reproducing the exact harm ADR 0010 Decision 1 names, one layer further along. All three now carry it. default-mode.md still told producers "the fix action parses only ## Findings and ## Unparsed" while fix-pass-mode.md now requires ## Surfaces and tier: from every consumed file. A producer reading the writer contract and a consumer reading the consumer contract would execute differently. The clause is narrowed rather than deleted -- presentation-additive stays true of ## By dimension alone. Records written by 0.19.0 carry source-findings as a bare scalar and survive the upgrade untouched, because the findings directory is gitignored local state. The new reader expected a sequence, so a legacy record would have subtracted nothing and re-injected findings already applied. The reader now tolerates a scalar as a one-entry sequence compared by base name, and the CHANGELOG carries the migration note. Also from the same pass: the collapsed row had no rule for Tier, Confidence, or Action (now MAX tier, MAX confidence, every distinct Action retained) and merged Rank collided at 1 across files (now renumbered, which also fixes the apply order); "identical" is defined as byte-for-byte after unescaping; an abnormally terminated apply writes no record, since re-consuming an applied fix is recoverable and a silently retired row is not; a minimally conforming producer that omits tier: or ## Surfaces is consumed with tier: unstated rather than skipped or guessed at; the DEGRADED blockquote is collapsed to its first line; the names-not-paths rationale is replaced -- it claimed a cross-directory property the design does not have, when the true reason is that both sides are always read from the same single directory; and the empty-set message no longer says "run the review first" to an operator who already did. Stale single-file language cleared from the SKILL.md description and the plugin README. Eval 22 gains the sequence assertion, 25 trades a restated expectation for an orthogonal one, 27 covers the collapsed-row fields and renumbering, and 28 pins the legacy-scalar path. Refs #2678 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 39s —— View job Security review —
|
|
Claude finished @kyle-sexton's task in 4m 25s —— View job Code review —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
Author
|
Handoff from PR #2686 (ADR 0010) — one implementation-side finding for this PR's Context: a P1 review thread on #2686 raised that marking a whole consumed FILE as retired drops rows the operator narrowed out or the fixer surfaced instead of applying. Step 5 of The prose states the requirement:
The template renders it on only one of the three body lines:
Why it matters beyond tidiness: recovery depends on the operator knowing which producer to re-run, and Step 5 itself says naming the source file is what tells them. A bare count does not. ADR 0010's new Consequences bullet deliberately phrases this as an obligation the decision places on the record format rather than a property the record already has — so the template is the surface that has to meet it. Suggested shape (owner's call): render the correctness-class No action needed on #2686 — its thread is resolved and the ADR change is independent of this. |
kyle-sexton
added a commit
that referenced
this pull request
Aug 15, 2026
… reviewer-burden term (#2694) Closes #2682 Closes #2683 Phases 5 and 6 of the boris-routines-adoption plan, in **one PR by design** — one autonomy version bump, one CHANGELOG entry. Splitting them either duplicates the bump or strands one without its changelog line, and `check-changelog-parity.sh` fails on both. Independent of the detector chain (#2690 / #2692); nothing here blocks on those merging. ## Why rows for classes we are not building A catalog that lists only what shipped cannot be reasoned from. A reader asking *"why is there no clone unifier?"* finds silence, and silence reads as an oversight rather than a decision. **Nine rows added, one existing row amended**, and every derivation run **through the mapping rules** (`routines.md` "Mapping rules (catalog to matrix)"), never by hand — that is this phase's whole discipline. ## Existing-row sweep (first work item) Run against `routines.md` itself, not the research record. Per candidate class, "no row" or the existing row's identity: | Candidate | Sweep result | |---|---| | dead code | **Row exists** — `dead-code-sweep`, `DET detect \| DC (review-gated PR) \| repo \| n/a — no agent session \| not-a-routine`. → **amended**, not duplicated. | | clone work | **No row.** The nearest row, `coverage-mutation-watch`, is a **different observable** — coverage and mutation score, not clone density — so it is no match and was not amended. → new row. | | the other eight | **No row.** → new rows. | `clone-trend-gate` ships a **byte-identical cell signature** to `coverage-mutation-watch` (`DET | R (digest/gate) | repo | n/a — no agent session | not-a-routine`). The rows are genuinely distinct — clone density and its trend versus coverage and mutation score — and because nothing in the six cells says so, a class parameter now states it. Two rows that read as a copy-paste duplicate need the difference written down somewhere. **The issue's line citations had drifted.** It cites `dead-code-sweep` at `:192` and `coverage-mutation-watch` at `:195`; they are at `:191` and `:193` on `main`. The row *identities* and their full cell text match exactly, so the sweep result stands — but the offsets are not uniform (1 and 2), so the numbers were re-resolved rather than adjusted. ## ADR-0004 incumbent gate — the search record #2682: *"ADR-0004's incumbent gate applies to rows here, not only to detectors."* ADR-0004 D-1 requires the search carry `path:line` evidence. It is recorded here rather than in `routines.md` because that contract is deployment-agnostic — an adopting org does not ship this marketplace, and a binding parameter grounded in what *this* repository happens to contain is the same defect as grounding one in this fleet's hardware. What the search *changed* in the contract is in the contract; the evidence trail is here. | # | Class | Verdict | Evidence | |---|---|---|---| | 1 | `formal-logic-modeling` | **No incumbent** | Searched `TLA+`, `Alloy`, model-checking, invariant-verification, design-by-contract vocabulary across `plugins/`. No hits that were not false positives. | | 2 | `cant-fail-test-repair` | **Partial** | `plugins/mutation-testing/skills/audit/SKILL.md:126` — surviving-mutant disposition surfaces tests that cannot fail a given mutant. Remediation (`:201-209`) authors **new** killing tests via `/testing:write`; it does not repair the existing tautological assertion, which is this class's whole content. | | 3 | `layering-enforcement` | **Incumbent** | `plugins/review/agents/architecture-guardian.md:3` — "Reviews code for dependency-direction violations, layer boundary breaches…"; `:33` checks "inner layers must not reference outer layers; follow the project's stated layer rules." Close match, same inform-human posture. **Already reflected in the row**: its join trigger requires "a recurring manual pattern the incumbent reviewer does not already cover", and its parameter requires clearing the incumbent gate against the existing architecture-review surface. | | 4 | `clone-trend-gate` | **No incumbent** | Searched clone-detection tool names, `clone densit`, `copy-paste detector`, `code clone`. `plugins/review/agents/code-reviewer.md:46` flags "Duplicated Code" ad hoc during diff review — not a density trend or a gate. | | 5 | `stale-flag-removal` | **Partial (weak)** | `plugins/code-tidying/skills/tidy/templates/host-wiring-lane.template.md:16` lists "stale feature-flag branches" as one example under the Dead Code tidying. A bare example phrase inside a glob-scoped lane; no flag-age tracking, no staleness detector, no single-variation-everywhere trigger. | | 6 | `logic-simplification-sweep` | **No incumbent, at either altitude** | Above expression level: `tidyings.md` has no cross-function control-flow restructuring; Beck's Guard Clauses (`plugins/code-tidying/skills/tidy/reference/tidyings.md:15`) restructures within one method only. At expression level: searched `boolean expression`, De Morgan, `redundant condition`, `simplif.*expression` across `plugins/code-tidying/` — **zero matches**. `plugins/code-tidying/skills/batch-simplify/SKILL.md:140` delegates to an external agent not present in this repo, with instructions naming no altitude. | | 7 | `abstraction-flattening` | **Partial — the closest call in the set** | `plugins/architecture/skills/improve/research/deepening/scan-briefing.md:49-51` (two-adapter rule → "speculative indirection") and `research/deepening/vocabulary.md:28` (the deletion test: "If complexity vanishes, it was a pass-through") detect the same smells this class targets. But the lens's remedy is *deepening*, it applies no fix itself, and it routes through a human interview and planning handoff (`SKILL.md:63`) — there is no autonomous change path. | | 8 | `ant-only-shipper` | **No incumbent** | Searched champion/challenger, canary, shadow deploy, A/B, promote-to-production, competing-implementation vocabulary. `plugins/prototype/skills/explore-directions/SKILL.md` is adjacent — throwaway UI variations picked by a human in-browser — not an evidence-based promotion recommender. | | 9 | `gui-crash-fuzzing` | **No incumbent** | Searched `fuzz`, monkey-test, random-click, GUI-crash, UI-stress vocabulary. All hits false positives. | | 10 | `dead-code-sweep` (amended) | **Partial** | `plugins/code-tidying/skills/tidy/reference/tidyings.md:21-25` — Beck #2 Dead Code covers detection and verified deletion inside a scoped lane. It has **no quarantine-then-judge staging**, which is the entire normative content of the amendment. | ### What the search changed - **`abstraction-flattening`'s "no validated detector" was under-qualified.** Heuristic detectors for exactly these smells ship — including in this marketplace. The parameter now says what is actually true and what the join trigger actually names: *a scanner is not the join trigger; a published validated detector is*, and none surveyed is validated against a fault-outcome ground truth. - **`logic-simplification-sweep`'s "already covered by structure-only tidying surfaces" was false**, and the search is what established that — there is no incumbent at either altitude. The claim is removed rather than restated. - **No verdict removed a row.** Two partials (`cant-fail-test-repair`, `dead-code-sweep`) are "detection exists, the remediation shape does not", which is a capability gap rather than a duplicate. `layering-enforcement`'s real incumbent was already gated in the row before this search. ## Rows added **Tier 1 — join triggers** | Class | Judgment | Output | Access | Derived | Status | |---|---|---|---|---|---| | `formal-logic-modeling` | AGT | R | repo | `C1` | join: a stated invariant or specification artifact exists to model against | | `cant-fail-test-repair` | hybrid: DET detect; AGT repair judgment is the routine | DC (PR) | repo | `C3` | join: proven recurring manual pattern | | `layering-enforcement` | AGT/HUM | R | repo | `C1`; disposition human-gated | join: layering rules stated as text, and a recurring manual pattern the incumbent reviewer does not already cover | **Tier 2** | Class | Note | |---|---| | `dead-code-sweep` | **Amended.** Was `DET detect` only; the quarantine-exit judgment makes it hybrid, and that portion derives `C3` — liveness is *not* mechanically checkable, since reflection, dynamic dispatch, and out-of-tree callers each defeat the build that would otherwise prove it. Window floor **30–90 days with staged quarantine, never one day**, and not tunable downward by an org binding. | | `clone-trend-gate` | `DET \| R (digest/gate) \| repo \| n/a \| not-a-routine`. Detection and trend gating **only**. | | `stale-flag-removal` | hybrid; removal portion derives **`C4`** — a flag definition is a configuration surface, and the structural-blast-radius rule composes above `C2`/`C3`. Disposition human-gated. | **Tier 3 — recording why not** | Class | Why not | |---|---| | `logic-simplification-sweep` | `C3`; `join (external)`: published effectiveness evidence exists. | | `abstraction-flattening` | `C4` (structural surface); `join (external)`: a validated detector is published. The fault data also runs *backwards* — Speculative Generality and Middle Man are in some studies associated with **fewer** faults. | | `ant-only-shipper` | `AGT/HUM`, `C1`, human-gated. The promotion decision is a product call. | | `gui-crash-fuzzing` | `not-a-routine` (DET). Reported crash-replay reproducibility is low enough that filing every crash would degrade the governed queue rather than feed it, so its `WI` output is replay-gated. The row also carries the isolation consequence the catalog had skipped: unattended GUI actuation requires `L3`. | ## Exclusions are exclusions, not deferrals Recorded as such, because a deferral invites a future PR to "finish" the class: - **`clone-trend-gate`** — the unify *decision* is excluded, not a deferred posture. No surveyed clone-detection tool automates deciding which clones to unify, across a detection literature the survey found spanning two decades. On that record there is nothing to defer *to*, so adding a unify posture re-opens the class rather than extending it. - **`gui-crash-fuzzing`** — judging a filed crash beyond replay is excluded the same way. A filed crash is ordinary queue intake, owned by the issue-lifecycle classes. Without this the row's `DET → not-a-routine` exit would be skipping a judgment portion, which is precisely the misreading `routines.md` §"What a routine is" is worded to prevent. - **`stale-flag-removal` / `ant-only-shipper`** — the disposition never automates. Which branch survives, and whether to promote, are product calls. That is why both rows are `AGT/HUM`. - **`layering-enforcement`** — inform-human posture only; a direct-change posture derives `C4` and no surveyed precedent supports one. ## The contract is now three tiers, explicitly The new `Class parameters` section carries normative detail six table cells cannot hold — stated as **binding**, so a leaf contradicting one is non-conforming. That created a contradiction with three statements in the same file saying definition depth exists only for the `v1` classes, while `clone-trend-gate` and `gui-crash-fuzzing` — neither of which will ever gain a leaf — carry parameters. **Resolved by widening, not collapsing.** The hub now declares three tiers: catalog plus mapping rules; class parameters, binding any class leaf or not; leaf-level depth for the ten `v1` classes. Collapsing the parameters back into row cells was the alternative and was rejected — the section exists precisely because the cells cannot carry the detail, and several parameters bind `v1` and hybrid rows generally rather than one deferred class. ## Four general derivation rules now live in the mapping rules `## Mapping rules` exists, in its own words, *"so an adopting org can classify a **novel** routine class end-to-end… without a contract change."* Three general rules were filed under `### Class parameters` instead — two of them titled with a class token while generalizing in their last sentence (*"The same reading applies to every `AGT/HUM` row"*; the structural axis *"keys on blast radius"*). An org reading the mapping rules would not have found them. All three moved: the hybrid portion-split rule onto the hybrid bullet under *Judgment and output*; the `AGT/HUM` clarification onto its existing bullet; the target-not-file rule onto *Structural blast radius*. A **fourth** was stranded the same way inside a `dead-code-sweep` note and is now stated generally: a risk-raising axis evaluates **per item** as well as class-wide, so a class whose axis fires on only some items derives the lower class and records the escalation rather than deriving the higher one wholesale. **One honest limitation now stated in the rules themselves.** The structural axis keys on the change's *target*, and **no catalog column records a target** — which is why `logic-simplification-sweep` and `abstraction-flattening` carry byte-identical axis cells (`AGT | DC (PR) | repo`) and derive `C3` and `C4`. Rather than leave a reader to conclude the three axis columns are sufficient when they are not, the rule now says the target comes from the class's own definition and that a row turning on it says so in its `Derived row` cell. ## A second `join` semantic, separated Every pre-existing `join:` trigger is a condition the adopting **org** can satisfy: connect a telemetry surface, write the layering rules down, accumulate a manual pattern. `published effectiveness evidence exists` and `a validated detector is published` are world state no adopter can act on. One token carrying both semantics made those two rows read as backlog items. A `join (external): …` legend row now separates them. Rewording the two triggers into adopter-actionable form was the alternative and would have been dishonest — the point is that the org *cannot* fire them. An `excluded:` status was also rejected: these classes are genuinely deferred pending evidence, unlike the unify decision, which is excluded outright. Every other `join:` row in the table was re-checked against the new definition; none belongs under the new one. ## #2683 — the predicate side - **What may never enter a predicate.** An acceptance or merge rate is never a promotion input and is not an efficacy signal, in either role, at any cell, at any threshold. - **Two shipped terms sit close to that line, and both are now distinguished.** `0 human-reverted merges` is a *correctness* signal — a human asserting the change was wrong after it landed — not an acceptance rate. And `≥ 20 autonomous C2 merges over ≥ 14 days` is a **volume floor**, not a rate: a ratio rises when its denominator shrinks, so attempting less — or attempting only what is certain to land — raises it with no change in the work. A count has no denominator to shrink. Selectivity leaves it flat. - **The term inventory is now complete.** It previously enumerated four term types and the table has **seven** — merge counts, advisory-review counts, and missed-blocking-finding counts were all missing. All three are correctness- or volume-side, so nothing in the argument moved. - **Reviewer-burden term: deferred with an explicit trigger, not omitted.** It needs a denominator, and a denominator needs three org-scale things this contract does not have — a population to divide by, a non-merge outcome signal, and a lookback window with a demotion rule. Without them the term moves with *volume* rather than trustworthiness, which rewards a cell for producing less. Trigger: the volume **and** a non-merge outcome signal; volume alone is not the trigger. - **Standing constraint on any future tuner** — its signal set stays **disjoint** from promotion evidence. Overlap is a self-dealing loop: a tuner optimizing a signal that also promotes a cell can raise that signal to reduce the scrutiny applied to the tuner's own output. Binds the tuner's inputs, not its intent, and binds whether or not the reviewer-burden term is ever activated. ## Two claims in #2682 that did not verify — and one of the replacements did not either **#2682 says logic simplification above expression level is "excluded by name in `tidyings.md`".** It is not: `plugins/code-tidying/skills/tidy/reference/tidyings.md` contains **zero** occurrences of `simplif`, case-insensitive. **The first replacement for that claim was also false.** An earlier revision of this description said expression-level simplification is "already covered by structure-only tidying surfaces" and that simplification above that level "changes behavior in the general case". Both fail: - The incumbent search found **no incumbent at either altitude** — no cross-function control-flow restructuring in `tidyings.md`, and zero expression-simplification matches anywhere in `plugins/code-tidying/`. - "Changes behavior in the general case" is contradicted by `plugins/code-tidying/skills/batch-simplify/SKILL.md:174` — *"Simplification is behavior-preserving"* — which treats behavior alteration as a **regression** its verification exists to catch. It also contradicted the row's own cell, which claims only that equivalence above expression level is not mechanically checkable: weaker, and correct. A checkable-and-false claim had been replaced by an uncheckable one. The parameter now uses the row cell's own wording and drops the coverage comparison entirely; the `path:line` evidence lives in the incumbent table above, where it cannot rot a deployment-agnostic contract. ## Deployment-specific fact removed from normative text A binding parameter read: *"The fleet ships no GUI to fuzz, so the class has no observable here at all"* — inside a section declaring *"A parameter here binds the class; it is not commentary"*, in a contract whose own closing line is *"The contract assumes no machine, org size, or budget"* and whose hosting stance makes substrate a deployment-owned binding. An adopting org that ships GUIs would read a binding parameter grounded in **this** fleet's inventory. Struck. The deployment-independent half stays, and the class's `not-a-routine` derivation never depended on the inventory in the first place — it follows from Judgment `DET`, which holds for any deployment. **On #2682's present-observable admission check** (*"Drop any candidate anchored to nothing"*): the observable here is crashes surfaced by actuating a GUI — a real, nameable artifact class, not nothing. A deployment without a GUI simply never schedules the class, which is true of every access-gated row in the catalog and is not an admission question. The row is admitted; what was wrong was grounding its *parameter* in one deployment's inventory, not the row's presence. ## Empirical claims, hedged to the register the evidence supports The supporting research record is gitignored, so no in-tree reader can check any empirical claim in this contract. Several were stated at a confidence the reader has no way to audit: | Was | Is | |---|---| | "Published practice at scale runs an order of magnitude longer than a day" — ~10 days, which does not reach the 30-day floor it was offered to support | The floor derives from what the window must **out-last**: 30 days is the shortest window spanning a monthly invocation cadence at all, 90 spans a quarterly one, and a one-day window spans nothing | | "twenty years of clone-detection tooling produced no production automation" — a universal negative stated as fact | "no **surveyed** clone-detection tool automates the choice…, across a detection literature the survey found spanning two decades" | | "The flag-lifecycle tools that lead this space" | "The flag-lifecycle tooling **surveyed**" | | "No published effectiveness evidence supports automating it" — universal negative | "No effectiveness evidence… **surfaced in the surveyed literature**" | | "a direct-change posture … has no evidence behind it" | "**no surveyed precedent** supports one" | | "reproducibility around 36.6%" — two significant figures, no citation | "low enough that filing every crash would degrade the queue" — and the population shift is fixed too: the figure measured replay of *crashes*, and was being applied to *filed work items* | | "weakest precedent of the catalog" — a superlative over all forty-nine rows | "among the weakest precedent the survey found" | | "Three independent lines of evidence — peer-reviewed observational work, large-N regression, and a randomized trial", citing exactly one | The rule now rests on the one finding **verified at primary source** (Lenarduzzi et al., quoted verbatim); the other two design families are named as survey context, explicitly not checked | The target register is the one already-well-calibrated claim in the section — *"in some studies associated with fewer faults"* — which is left as it stands. ## Fresh-context verification, and the two contradictions it caught The edits above were checked by a **separate fresh-context agent** with the authoring session's reasoning withheld, briefed to judge whether each finding actually no longer holds, whether any fix introduced a new in-tree contradiction, whether the remaining empirical claims are calibrated, and whether every row still derives correctly. It re-derived six rows from the mapping rules and axis cells alone before reading the `Derived row` column: **six of six matched.** It found two real contradictions, both now closed: - **`gui-crash-fuzzing`'s Output cell (`R + WI`) contradicted its own binding parameter**, which said filing would degrade the governed queue. The previous revision masked this — *"the fleet ships no GUI to fuzz"* made the whole row moot, so the Output cell never had to agree with the parameter. Removing that escape exposed it. The `WI` output is now admission-constrained: an item is filed only where re-running the recorded input sequence reproduces the crash, and the rest stay in the `R` half. That gate is a re-run rather than a judgment, so it adds no `AGT` portion and the `DET` exit stays complete. - **`work-classes.md` deferred the reviewer-burden term on "the org-scale trust-path requirements this contract already defers"** — a phrase with **zero** other occurrences anywhere in the repository. A back-reference leaked from the gitignored research record into normative text, asserting a prior decision no in-tree reader can locate. The three requirements were already enumerated inline, so the phantom reference is dropped rather than manufactured. It also caught the stranded per-item rule, the missing-target-column limitation, the four over-claimed hedges above, and one SSOT gap outside the three files in play: **`guardrails/isolation-ladder.md` scoped `L3` to untrusted-provenance (`C5`) work alone**, while the catalog's access rule has always also required it for unattended GUI actuation — a demand that reaches classes deriving no work class, so it cannot travel through the matrix's min-isolation column. That leaf is the contract's source of truth for when a level applies, and was incomplete against its own charter. Fixed at the root rather than papered over in `routines.md`. ## Other corrections in this pass - **The posture-qualified-identity mandate was unsatisfiable for seven of the nine hybrid rows.** The new parameter made it mandatory for every hybrid row, while *Routine identity* says posture tokens are owned by the class's definition leaf — and seven hybrid rows are `join:`-deferred with no leaf. Scoped: tokens are minted with the leaf, so the obligation binds at leaf time; until then a deferred class records its split in its Judgment cell and binds nothing. - **The `L3` on `gui-crash-fuzzing` does not come from the matrix.** The guardrail matrix's min-isolation column is indexed by work class (`C1`–`C5`); a row deriving no class cannot reach it. The floor comes from the GUI-actuation mapping rule directly, and the row and its parameter now say so rather than implying a matrix lookup that would not resolve. - **`## Precedent pointers`' scope line claimed coverage it did not have.** It declared itself pointers "for the deferred and deterministic rows"; all nine new rows are deferred or deterministic and none got one. Narrowed to the rows where a shipped pattern was surveyed, with an explicit note that absence is not a claim that no pattern exists — rather than inventing nine pointers this pass has no evidence for. - **`dead-code-sweep`'s precedent pointer was stale against the amended row.** It described deletion pipelines with no quarantine stage while the row's whole normative content is staged quarantine — so it read as precedent for the unstaged form. It now says the staged quarantine is the row's own normative content, not a property read off those pipelines. - **`### Reviewer-burden term` sits under a preamble that did not cover it.** The preamble said "the threshold values below are suggested defaults the org binds"; a deliberately-not-live term with no threshold is not a suggested default. Promoting it to a sibling `##` was the alternative and was **rejected** — #2683 names §"Suggested default predicates" as its placement. The preamble was narrowed to the table instead, and now states that the two subsections after it are not defaults and carry no bindable threshold. - **CHANGELOG bullets re-filed per Keep a Changelog**, which the file's own header cites. The `dead-code-sweep` amendment and the derivation-rule bullet change existing content and moved to `### Changed`. The row count is now exact: **nine added, one amended** — class rows go 40 → 49, and "ten rows added" double-counted the amendment. (Commit `21fce8eb`'s body carries the same off-by-one; history is not rewritten, the durable surface is fixed.) ## Sanity checks — actual output, re-run at `c0795c93` - **No two rows share a class token** — `awk -F'|' '/^\| [a-z]/ {print $2}' … | sort | uniq -d` returns **empty**. Verified non-vacuous: the same pipeline yields **51** distinct tokens, against **41** on `origin/main`. The delta is **+10, not +9**: nine new class rows plus the new `join (external): …` status-legend row, which the pipeline also matches because legend rows begin `| join…`. Class rows alone go 40 → 49. - `grep -n "388\|180 merged" plugins/autonomy/reference/routines.md` → **empty**. - `grep -ci "never a promotion input\|no acceptance-rate\|not an efficacy signal" …work-classes.md` → **1**. - `grep -Eci "predicate .*(merge|acceptance) rate|(merge|acceptance) rate .*(threshold|>=)" …work-classes.md` → **0**. - `scripts/check-changelog-parity.sh --check-bump origin/main` → **exit 0** (autonomy 0.16.12 → 0.17.0). - `scripts/check-changed-skills.sh origin/main` → "No changed skills under `plugins/*/skills/`". - `markdownlint-cli2` on all four changed files → **0 issues**. ## Declared human gate — evidence recorded, disposition open #2682: *"a reviewer who did not author the rows re-derives three of them from the mapping rules and the row's own axis cells alone. No command self-clears this."* **A fresh-context agent performed the re-derivation. No human re-derived these rows, and this PR does not claim the gate is cleared.** An earlier revision of this description said CLEARED; that was the authoring session grading its own homework, and a fresh-context same-vendor agent is the middle rung of the independence hierarchy, not the reviewer #2682 names. The full record — the withheld-answer protocol, the agent's unprompted provenance statement, the three derivations, and this same caveat — is posted as a durable comment on **both** #2682 and this PR, so it does not live only in a commit body and a PR description written by the session that produced the rows. | Class | Independently derived | Authored | Match | |---|---|---|---| | `dead-code-sweep` | `C3` | `C3` | yes | | `stale-flag-removal` | `C4` | `C4` | yes | | `abstraction-flattening` | `C4` | `C4` | yes | Three of three. **The run predates three subsequent commits**, so it was performed against a superseded revision — but the later verification pass re-derived all three classes against the current text and reached the same three answers, so the result survives the edits rather than merely predating them. More useful than the match: the derivation showed the mapping rules *imply* three things they never *stated*, each of which the agent had to reason out to avoid a wrong answer. All three are now stated in `## Mapping rules` itself, alongside the fourth the verification pass found stranded. **What a human still owes this gate:** deciding whether an agent re-derivation satisfies it. The record exists so that call can be made against the actual protocol rather than against a summary. ## Related Independent of #2690 / #2692. Blocks the can't-fail test detector's row amendment (#2684). --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
9 tasks
The fix action's merge-set ledger keyed on file names, and names are not unique: the timestamp has second resolution, the topic is producer-chosen, and nothing requires a producer to put a timestamp in a name at all. Two defects followed. Write side — the consumption record was `<UTC-timestamp>-fix-pass-applied.md`, so two applies on one branch finishing in the same UTC second wrote the same path and the second clobbered the first. That was harmless while the record was audit-only prose; this branch made it the ledger Step 1 subtracts by, so a lost record leaves its files unsubtracted and re-injects already-applied findings — the failure Step 5 exists to prevent. The record is now staged through `mktemp` outside the findings directory, digested, and moved in as `<UTC-timestamp>-fix-pass-applied-<sha256-12>.md`. A content digest rather than a nonce because the only case that still collides is two byte-identical records, which name the same consumed set, so the overwrite is a no-op. Read side — `source-findings:` held bare names and matching was by name, so a producer writing a different file under a name an old record already named was subtracted unread. Entries are now `name:` + `sha256:` mappings, and Step 1 computes each candidate's digest at read time from the candidate's own bytes. A candidate is subtracted only when an entry matches BOTH halves. An entry with no digest matches by name alone — the whole of the 0.19.0 bare-scalar tolerance — and is bounded twice: an entry that has a digest never falls back, and a digest-less entry is honored only when the candidate's `date:` is not newer than the record's `date:`. Declared frontmatter instants, never filesystem mtimes, which a copied or restored memory tier rewrites and whose `stat` format flag differs between GNU and BSD userland. The second bound is load-bearing because a conforming detector may write one fixed name it overwrites every run, and without it a single stale legacy record would retire every future version of that file silently and forever. A candidate with no readable `date:` fails the test and stays in the set: `date:` is not part of the admission test, so the case must be decided, and it is decided toward a recoverable re-application over an unrecoverable drop. The producer-facing findings-file shape is unchanged — the identity is computed by the consumer, so a conforming detector needs no edit. `default-mode.md`'s writer contract gains a never-overwrite rule for a producer's own path, stated as hygiene rather than a second identity mechanism. The same under-specification reached the directory itself. Both skills glossed the findings home as `<memory_dir>/reviews/<branch-slug>/` unconditionally, but only two of the binding's five rungs compose that segment; a producer and a consumer disagreeing about it land in different directories, and the symptom is a clean empty-set STOP an operator cannot tell from "no findings". Both `SKILL.md` "Shared inputs" bullets, fanout's Step 1, and the README now cite `reference/topic-docs.md` as the authority instead of restating a path shape, and that binding states which rungs compose the segment. The binding also now cites the topic-docs convention's "Non-interactive / forked mode" section, which is contract-owned and which it previously neither cited nor covered — leaving the headless `--yes` path with no stated behavior at the rungs that need a user. The record body gains a rendered `## Not applied` table — location, finding, why, and source file — so every row that did not land is individually recoverable. Step 5 already required that attribution; the template rendered it on one of three lines, reporting surfaced rows as a bare count and giving operator-narrowed rows no home at all. Recovery means re-running the producer that found the row, and a count cannot say which one that is. Also corrects the CHANGELOG's stale names-not-paths rationale, which cited a cross-`memory_dir` property the design does not have; scopes `default-mode.md`'s "required" coverage sections to fanout's own writer, which contradicted Step 1's minimal admission test; pins `## Surfaces` attribution to the consumed file's name, the only identifier the shape carries; and softens the one-file "byte-for-byte" claim to the applied set, which is what is actually preserved. Evals: 19, 22, 28 and 30 updated; 29, 31, 32, 33 and 34 added to pin the read-side collision, the attribution table, the fixed-name legacy producer, the undated candidate, and the minimally conforming detector. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…oked
An independent verifier found the not-newer test had reintroduced, through
its tiebreak, the failure it was added to prevent.
`date:` is producer-DECLARED, not machine-observed. `default-mode.md` declared
it as a bare `date: <ISO-8601 UTC>` with no stated semantics, so a detector
deriving it from the commit under review, a scan date, or a template constant
is making an ordinary implementation choice. With a constant `date:`, EQUALITY
IS THE NORMAL STATE — and subtract-on-equal then let one pre-0.20.0 record
retire every future version of a fixed-name file, silently and forever.
The tiebreak now fails open: a digest-less entry is honored only for a
candidate strictly OLDER than the record, and equal keeps the candidate. That
is the direction every other clause in the comparison already takes — an
unreadable `date:` keeps the candidate, a missing digest narrows rather than
widens — because re-application is recoverable and silent retirement is not.
The old justification ("preserving the behavior the record was written under")
preserved nothing: 0.19.0 ran no date comparison at all.
The producer-facing shape now states what `date:` MEANS — the instant the file
is written, not a commit date, scan date, or constant — and that the file name
must end in `.md`, since the consumer's scan is the only way a file is ever
seen. Both bind fanout's own writer; the consumer still assumes neither.
Comparison is now specified: normalize to UTC and compare as instants, accept
an explicit `Z` or numeric offset, treat date-only or designator-less values as
unreadable. The string-comparison shortcut is withdrawn — it inverts on
fractional seconds, where `…01.123Z` sorts before `…01Z` while being later.
The empty-set STOP now prints the resolved directory and the rung that resolved
it, and on a non-interactive run says the asking and persisting rungs were
skipped. Step 1 diagnosed a wrong-directory resolution as indistinguishable
from "no findings", then printed nothing an operator could tell them apart
with; this is also the skill's half of the binding's cited non-interactive rule,
which requires surfacing the assumption rather than silently taking a default.
Smaller items from the same review: `date` added to the shape's required list,
so the parity claim between the shape and the admission test is true for the
field this change turns on; a cell-escaping rule for the `## Not applied` table,
whose rows carry producer text and whose loss costs the source attribution that
makes a row recoverable; the `/simplify` staleness note scoped to files that
declare a readable `date:`, since a fixed-name producer has no age to read; and
`SKILL.md`'s inlined timestamp spec replaced by the binding it says owns it.
The pattern behind the finding is the reason for the last fix here: this doc
had twice stated an optimistic bound as a property of the system when the
admission test does not guarantee it. A pass over the remaining absolutes found
one more — the candidate scan assumes `*.md` — now stated as a requirement of
the shape rather than left as an inference about producer naming.
Evals: 28 and 32 updated for the flipped tiebreak; 35 added to pin the
constant-`date:` producer, the case the old tiebreak silently retired.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
The `hygiene` job failed on the `typos` step: `unparseable` should be `unparsable`, twice -- `context/fix-pass-mode.md` Step 1's date-readability rule and eval 33's expectation for the same rule. Both are prose written in this PR. Worth noting where the gates did not catch it: `check-changed-skills.sh` and `markdownlint-cli2` both passed on these files, because neither is a spell checker. `typos` runs only in CI, so a misspelling in newly authored skill prose is invisible locally no matter how carefully the skill gates are run. The third occurrence in the tree, `plugins/source-control/.../babysit_checks.py`, is pre-existing and was not flagged -- the run emitted exactly two warnings, both in this plugin. Left alone rather than swept up: it is outside this PR's scope and touching it would put an unrelated plugin in this diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 25s —— View job Security review —
|
|
Claude finished @kyle-sexton's task in 4m 48s —— View job Code review —
|
Step 1's compact bolded rule still said a digest-less entry matches "only if the candidate is not newer than the record" -- a <= condition that includes equal -- while the rule six lines below, default-mode.md's writer contract, the CHANGELOG migration note and eval 35 all specify strictly older, equal keeps the candidate. The summary sentence is the one an implementer skimming for "the rule" extracts, and read at face value it authorises exactly the failure the strictly-older test was added in this same PR to prevent: a producer with a constant date: retired permanently by one pre-0.20.0 record. Self-inflicted and worth naming: the tiebreak was flipped from not-newer to strictly-older in an earlier commit, and every downstream statement was updated while the upstream summary was not. Swept the plugin for the old phrasing -- no other occurrence survives, so the class is closed rather than the instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 38s —— View job Security review —
|
|
Claude finished @kyle-sexton's task in 1m 58s —— View job Code review —
|
kyle-sexton
added a commit
that referenced
this pull request
Aug 15, 2026
#2690 squash-merged, so main carries its changes as one commit while this branch carries the same content as real history. Git sees divergent ancestry for identical content, which is why three files conflicted despite nothing actually disagreeing. All three resolved to this branch's side, because it already absorbed #2690's final state -- including the typo fix and the strictly-older tie-case correction -- through the parent merges before the squash landed: - plugin.json: 0.20.1, which is this PR's bump on top of #2690's 0.20.0. - CHANGELOG.md: keeps the 0.20.1 entry above the 0.20.0 one main just gained. - default-mode.md: keeps the union resolved earlier -- "by content digest, not by file name" from #2690, plus the pointer to the detector-findings convention from this branch. Verified after resolution rather than assumed: no conflict markers survive anywhere under plugins/ or docs/, the version reads 0.20.1, and both changelog headings are present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 15, 2026
This doc said "The two texts state different rung counts -- SKILL.md inlines the rungs it operates on -- and the non-interactive rule below is what makes them coincide." True when written. Not true now. #2690 rewrote SKILL.md "Shared inputs" to delegate the ladder entirely: "Resolve the home; never assume its shape ... Read the binding rather than working from the default's shape." It inlines no rung count at all. So there are no longer two texts to reconcile, and the non-interactive rule is not what reconciles them -- verified against SKILL.md:32 on this branch rather than assumed from the commit message. The failure mode is worth naming, because a stack invites it: this branch described a sibling PR's pre-fix state, the sibling then fixed it, and the merge brought the fix in without touching the sentence describing the problem. Nobody edited a wrong line -- the line went wrong when a different file got better. And it landed in the one section this doc points readers at for authoritative resolution behaviour, which is the citation-accuracy bar the doc holds everything else to. Replaced with what is now true: SKILL.md does not restate the ladder, so a producer and the consumer read one text rather than two. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 15, 2026
Closes #2679 Phase 2 of the boris-routines-adoption plan. > **Stacked on [#2690](#2690 (phase 1, > #2678). Base branch is `feat/merge-findings-across-producers`, not `main` — this PR edits > `plugins/review/skills/fanout/context/default-mode.md`, which #2690 also edits, and it bumps the > `review` plugin to 0.20.1 on top of #2690's 0.20.0. Branching off `main` instead would have staged > 0.20.0 twice and collided on `check-changelog-parity.sh --check-order`. GitHub retargets this PR to > `main` when #2690 merges. **Merge #2690 first.** ## Why a stub now rather than the full doc later `docs/PLUGIN-PHILOSOPHY.md` "Convention registry" — *"A new cross-plugin convention lands in an owner doc **before a second plugin adopts it**"* — is a deadline, not a licence to author it late. The detector pilot makes a second plugin the adopter of the multi-producer rule, so the owner doc must precede it. Depth then trails the pilot, because the pilot is what produces the evidence to harden against. Both halves are load-bearing: the deadline is why the stub cannot wait, and the evidence is why the stub is a stub. This is also **a hard merge barrier**: a plugin installs standalone and cannot resolve a repo-relative `docs/conventions/` path, so the citation form is a raw URL to `main` (`plugins/architecture/reference/topic-docs.md:6` is the repo's exemplar). Nothing can cite this doc until it is on `main`. ## What the doc owns — and deliberately does not Owns: why the contract is format-only, the four producer-owned fields, coexistence between producers, consumption semantics as they bind a producer, minimal conformance, the liveness relationship, enforceability, and adopters. Does **not** own, and points at instead: - **The findings-file schema** → `plugins/review/skills/fanout/context/default-mode.md` "Findings-file shape". Pointer, never a copy — a second statement of a table is a second thing to drift. `! grep -q "^| Rank | Tier | Confidence"` passes. - **The consumer algorithm** → `context/fix-pass-mode.md` "Step 1: Build the merge set". - **Normalization and ranking** → `context/findings-normalization.md`. A detector emits final values, not pipeline inputs. ## The four producer-owned fields A detector has no severity crosswalk, no confidence filter, and no normalization stage behind it, so it computes these itself — and each fails silently: 1. **`Tier` is machine-computed, never guessed** — derived from the rule that fired, identically for every finding of that class, or rank order stops meaning anything across runs. 2. **`Confidence` is `high` or OMITTED — never `low`.** This is the rule a first detector author will get wrong, because it inverts the intuition: the rank order is `high` > `medium` > `unscored` > `low` (`findings-normalization.md:72`) and absent confidence resolves to `unscored`, which ranks **above** `low`. `:62` states it outright — *"Absent confidence ≠ low."* Emitting `low` to express uncertainty buries the finding beneath the one you never wrote. Both line references verified against the file, not recalled. 3. **`Location` is a repo-relative `file:line`** — the fix action fences each remediation to it, and an absolute path is not portable across the checkout that applies the fix. 4. **Cell escaping is the producer's job** — detector output routinely contains pipes (shell pipelines, type unions, regex alternation), and an unescaped one splits the row into phantom columns that parse *wrong* rather than not at all. ## Moving the multi-producer rule out of the review plugin #2690 (0.20.0) stated the multi-producer rule in `context/default-mode.md`. That rule binds every component that writes a conforming file, and `PLUGIN-PHILOSOPHY.md` "Convention registry" is one owner doc per shared concern — a rule binding three plugins cannot live inside one of them. This PR therefore also: - moves the general rules into the convention, - leaves `default-mode.md` recording only what fanout's own writer does, plus a raw-URL pointer, - bumps `review` to **0.20.1** with a matching CHANGELOG entry. **No behavior change.** This is the #2679 work item *"Own the multi-producer rule here, not in a review-plugin context file — #2678's file points here"*, executed rather than deferred. ## Sanity checks All four of the issue's checks, plus the gates: - `ls docs/conventions/detector-findings/README.md docs/conventions/detector-findings/CHANGELOG.md` → exit 0. - `grep -c "detector-findings" docs/PLUGIN-PHILOSOPHY.md` → 1 (the registry row). - `! grep -q "^| Rank | Tier | Confidence" docs/conventions/detector-findings/README.md` → passes. Phrased as a negated `grep -q` rather than checking `grep -c` for 0, because `grep -c` **exits 1** on a zero count — a check written as "returns 0" fails on success and passes on the condition it was meant to catch. - `gh pr view --json state -q .state` returns `MERGED` **before the pilot phase opens** — the one check this PR cannot satisfy itself. It is why the PR exists. - `scripts/check-changelog-parity.sh --check-bump origin/main` → exit 0. - `scripts/check-changed-skills.sh origin/main` → `CHECK-SKILL fanout: PASS — 0 errors, 1 warning(s)` (the pre-existing no-Gotchas-surface warning). - `markdownlint-cli2` on all five changed/added markdown files → 0 issues. - Every relative link in the new README resolved against the filesystem — 6/6 OK. ## Shape Follows the majority sibling shape (12 of 20 convention directories carry a CHANGELOG): `README.md` + `CHANGELOG.md`, SemVer with a stated bump policy, a Boundary section, an Enforceability table classified per `melodic-software/standards` `enforceability-tiers.md`, and an Adopters table. Enforcement is deferred with **event triggers rather than dates** — a conformance gate has nothing to run against until the first detector reaches `main`, and the doc's own depth waits on the pilot or a second adopter, whichever comes first. The adopters table lists only `review:fanout`, because tabling a planned adopter would assert what a reader cannot rely on. ## Independent review A fresh-context reviewer audited the diff against #2679 with the authoring rationale withheld. Its factual-accuracy lane came back clean (every cited line reference and quoted phrase resolves in the real file — nothing asserted from memory), links 6/6 correct, the raw-URL form matching the exemplar character-for-character, and sibling-shape conformance strong. Its structural lanes did not, and all findings are fixed in `53252eed`: 1. **The doc could not be acted on.** Its one normative instruction is "write a conforming file into the current branch's findings directory" — and it never said how to resolve that directory. The existing chain dead-ends for exactly this audience: `default-mode.md` points at the review `SKILL.md`, which resolves through a `${CLAUDE_PLUGIN_ROOT}`-relative path only that plugin can expand. A **"Where the file goes"** section now routes tier resolution to the `topic-docs` convention and states the three producer-facing specifics that pointer does not carry: the `<memory_dir>/reviews/<branch-slug>/` sub-path, the lossy slug rule, and the **self-ignore guard** — whose omission would commit findings meant to stay checkout-local. 2. **`Tier` never named its vocabulary or its owner.** `plugins/review/context/severity.md` "Severity tiers" owns CRITICAL/IMPORTANT/SUGGESTION *and* a consumer-precedence rule that binds a producer too — where the consuming project defines its own vocabulary, map to that instead. A detector emitting `P1` would have been non-conforming with nothing here to say so. 3. **`Confidence` cited the wrong owner** — `findings-normalization.md` is fanout's internal pipeline, which this doc's own Boundary excludes from a producer's concern. The definition now cites `severity.md` "Confidence axis"; the rank order stays only as the *consequence* that makes `low` worse than omission. Added: `Confidence` is confidence-of-*realness*, not confidence in the fix. 4. **Pointer-not-copy violations.** The doc claimed "This doc never restates it" and then restated merge-set construction, consumption subtraction, per-file consumption, the presence-only dedup rationale with its borrowed FALSE-MERGE quote, the cell-escaping characters, and the path-relativization rule. Duplication runs one direction here (convention copying plugin), so each is reduced to the producer-facing consequence plus a pointer — and line 10's claim becomes true. 5. **The adopters table contradicted the doc's scope** — title says "from outside `review:fanout`", sole tabled adopter was `review:fanout`. It ships **empty**, with `review:fanout` named in prose as the reference writer. That is the honest state of a stub. Also fixed: `date:` had no stated status for a producer and is now required; the admission test is cited rather than restated; `default-mode.md`'s closing sentence no longer overstates the split; `severity.md`, `topic-docs`, and `enforceability-tiers.md` join External authority (matching both siblings); and the review CHANGELOG's "No behavior change" is replaced with what actually did and did not change, since instruction text an agent reads is not nothing. Not taken: folding 0.20.1 into 0.20.0 while the stack is unmerged. Per-PR bumps are this repo's convention and rewriting the parent branch's entry from a stacked branch buys nothing. ## Related Depends on #2678 / #2690. Blocks the Pattern-C pilot (#2680) and the hardening phase (#2681). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 15, 2026
…le behind a flag (#2715) Closes #2680. Phase 3 of the boris-routines-adoption plan — the **integration slice**: the smallest end-to-end demonstration that a non-fanout producer reaches the apply relay, run against a detector that already existed rather than one invented alongside the contract. Stacked on #2692 → #2690 → `main`. Base is `docs/detector-findings-convention`. ## What this does `/mutation-testing:audit --persist-findings` writes the run's survivors as a findings file the `review:fanout` `fix` action consumes. Opt-in; bare invocation still reports and stops. No fanout edit, no registration, no dispatch wiring — the producer writes one conforming file and that is the whole integration. Mechanics live in the new spoke `plugins/mutation-testing/skills/audit/context/persist-findings.md`, which reads the producer contract for this plugin and points at it rather than restating it. ## The open question the issue handed us: zero survivors **Settled: the discriminator is whether the run *examined* anything, not whether it *found* anything.** - **≥1 mutant examined, no rows to emit** → **write**, with the `## Findings` header row and no data rows. The payload is `## Surfaces`. The consumer unions `## Surfaces` across producers precisely so "this surface ran and returned nothing" is not lost; a merged report saying only "the reviewers found three things" reads differently from one that also says the mutation surface ran over these files and found nothing, and the second is the true one. An empty table still meets the admission test, so the file is consumed and its coverage reaches the plan. - **No mutant examined** — empty scope, everything dropped by coverage/suppression/cap, or a Phase 0 refusal → **write nothing.** There is no coverage to report, and a `## Surfaces` line claiming the surface ran would assert coverage never attempted. The reasoning is recorded in the skill's own text, not only here. ## Severity is machine-computed, and two classes emit nothing | Phase 4 class | Row | `Tier` | |---|---|---| | Productive | emitted | IMPORTANT | | Unclassified (equivalence claimed, no demonstration) | emitted | IMPORTANT | | Arid | **none** | — | | Equivalent | **none** | — | Productive is IMPORTANT rather than CRITICAL because a survivor demonstrates that *the suite* fails to detect a change — it establishes no input, caller, or future edit that produces a wrong result, which is what all three of CRITICAL's limbs require. **Arid emits no row** because its only remediation is a suppression entry the user must accept, and handing a consent-gated write to an apply relay would launder that gate; the human still sees it in the Phase 5 report. **Equivalent emits no row** because it is not a defect. The map is deliberately flat — every emitted row makes the same claim, so manufacturing a tier spread would mean re-deriving tier from prose, which is what a class-keyed map exists to prevent. `Confidence` is `high` on every emitted row (Phase 3 executed the mutant) and **never `low`**, which ranks below omitting the field. ## Sanity checks | # | Check | Result | |---|---|---| | 1 | `check-changed-skills.sh origin/main` exits 0 | **PASS** — evals present, all 5 base-ref trigger phrases preserved verbatim | | 2 | `branch:` equals `git branch --show-current`; filename matches the timestamp pattern | **PASS** — verified mechanically on both specimens | | 3 | With a fanout findings file also present, `fix` plans findings from **both** producers and names both files | **PASS** — executed, see below | | 4 | `git status --porcelain` empty after a persist run except the gitignored write | **PASS** — clean tree; `git check-ignore` confirms the specimens ignored | ## Sanity check 3, executed A fresh-context agent read `fix-pass-mode.md` and executed Steps 1–3 against two real files in the resolved findings directory, with my reasoning withheld. It resolved the destination by running the rung order itself, admitted both files, merged, and hit the non-interactive gate (no `--yes`) — **STOP after the plan, mutate nothing, write no record**, which is why the demonstration cannot damage the tree. ```text Fix-pass plan — consumed 2 findings file(s), 4 findings after merge - 20260815T143000Z-review.md (tier: medium) - 20260815T144500Z-mutation-survivors.md (tier: unstated) - Surfaces (union) — ran: [code-reviewer, doc-drift-detector, architecture-guardian (20260815T143000Z-review.md), mutation-testing:audit (20260815T144500Z-mutation-survivors.md)]; returned no result: [architecture-guardian (20260815T143000Z-review.md) — no module or layer structure touched] - Cleanup-class (1) → /simplify - Correctness-class (2) → sequential scope-fenced fix - Surface-only (1, need human judgment / unparsed) ``` Both producers are consumed and named, both tiers reported — the detector's absent `tier:` renders as `unstated` rather than being invented — and `## Surfaces` is unioned with each producer's line attributed to it. Merged ranks: 1 `SKILL.md:216` (fanout), 2 `check-skill-precompute-compose.sh:30` (**detector**), 3 `skill-leaf-name-registry.txt:55` (fanout), 4 `README.md:11` (fanout). **The plan block evidences the merge, not row authenticity** — it would look the same for invented rows. Authenticity is proved separately, below. ## Row authenticity — measured, not asserted An earlier round of this work had hand-authored rows citing *fabricated* code at real `file:line` values. That is the failure this contract's `Location` rule exists to prevent, and it was caught in review. Both specimens were rebuilt from measurement. The detector row comes from an executed mutation run in a scratch copy (no tracked file touched; restore verified byte-identical): ``` === BASELINE === exit=0, 5 passed, 0 failed ROR line30 (-gt 0 -> -ge 0) SURVIVED while [[ $# -ge 0 ]]; do ROR line47 (-eq 0 -> -ne 0) KILLED ROR line86 (line_count > 1 -> >= 1) KILLED LCR line86 (&& -> ||) KILLED ROR line86 (git_count == 1 -> != 1) KILLED ROR line126 (scanned == 0 -> != 0) KILLED ROR line134 (STRICT == 1 -> != 1) KILLED SBR line89 (drop return 1) KILLED RESTORE: scratch script byte-identical to pristine ``` The survivor is genuine and its cause is precise: every test in that suite enters through the `-- | --all | --paths)` branch, which `break`s out before `$#` reaches 0, so no test drains arguments through the `*)` fall-through and the loop's termination condition is unasserted. A differential run of the mutant from a real checkout differs on **5 of 6** invocation shapes (dies on `case "$1"` with an unbound variable under `set -u`); only `--all` is identical. **That oracle gap is a real defect in this repo's tooling and is filed separately as #2708** — it is not part of this PR. Every `Location` cell in both specimens was then swept mechanically against tracked files; all four resolve to a real tracked line. One miscite (`skill-leaf-name-registry.txt:52`, actually `:55`) was caught by review and fixed. ## The second survivor: equivalent, and therefore absent from the table `scripts/check-docs-only.sh:76` (`break` removal) survived and is classified **equivalent**, so it emits no row and is reported in `## Surfaces` instead — the specimen demonstrates the omission rule on real data. Evidence: 8 differential inputs identical on stdout, exit code and stderr; `$prefix` dead after the loop; no iteration counter; `matched=1` idempotent. **Scope note, stated plainly:** the 8 differential inputs were executed by me, not re-executed by the independent verifier. The verifier confirmed the *structural* argument (dead variable, no counter, idempotent assignment) and said explicitly that it did not re-run the inputs. Read that limb as single-sourced. The equivalence probe itself was wrong twice before it was right — the allowlist was committed *with* the change (so the flag was decided before the mutated loop was reached), and the script `cd`s to its own parent, so invoking it from outside a checkout resolved a different repo. Both produced an all-`IDENTICAL` result that proved nothing. The harness now asserts that both `docs_only=true` and `false` appear, so a degenerate run announces itself. ## Defects found in review and fixed - **The tree self-check was vacuous.** It compared `git status --porcelain` before and after — but a `.gitignore` containing `*` matches *itself*, so a memory root inside tracked space leaves porcelain byte-identical whether the write was ignored or not. Replaced with a positive `git check-ignore -q` proof before any write, plus the three states it catches that reasoning about the guard alone does not. - **"The only write it makes"** — there are two; the self-ignore guard may create a `.gitignore`. Both are now subject to the same proof. - **The spoke restated contract premises** under a no-restatement banner, including one the parent branch had just deleted. Rewritten to point. - **The CRITICAL dismissal addressed 1 of 3 limbs.** Now addresses all three. - **"A wording correction, not a loosening"** was false — the invariant *did* widen. The CHANGELOG now discloses it as a widening. - **Contract-unfetchable path** added: report and stop, never invent a destination. ## Invariant amendment `SKILL.md` and `scripts/skill-leaf-name-registry.txt` now read **read-only with respect to tracked source**. Mutants are still applied, measured and reverted, and tracked source is still byte-identical when the run ends. This is a real widening of what the skill may do, disclosed as one — what did not change is the skill's standing under the naming doctrine, whose verb table already permits mutation behind an explicit user override. ## Contract tables updated here The `Adopters` row lands in the commit that makes it true, per the contract's own rule that a row is tabled only once a producer actually conforms — adding it in #2692 would have asserted what a reader could not rely on. The conformance-gate recheck trigger it named ("the first detector reaches `main`") is recorded as **fired**, and the two enforceability rows reading "**Not built**: no producer exists yet" now read as buildable. No gate is invented. Convention CHANGELOG 1.0.0 → 1.1.0. ## Known limitation, routed not solved A mutation finding's remediation lands in the covering test, not at its `Location`, so a consumer fencing each fix to `Location` cannot reach the target. `Location` is **not** retargeted (that would destroy the row's cross-producer dedup key) and **no column is invented**. Naming the test file in `Action` makes the conflict explicit rather than resolving it. Disposition belongs to #2681. ## Deliberately out of scope - **A mechanical conformance gate.** The contract defers it; this PR makes it buildable and says so, rather than shipping one unasked. - **#2708.** A real defect in this repo's own test coverage, found by this pilot, filed separately so it is fixed on its merits. - **A structured field for "equivalent".** An equivalence disposition currently lives in free prose and cannot be machine-checked. That is a crosswalk/vocabulary question routed to #2681, not a field to invent here. - **Consumed-file identity.** The text says nothing about how the consumer identifies what it consumed, so it is correct under both name-based and content-digest ledgers. ## Related - #2692 — the detector-findings owner doc this PR is the first adopter of. **Direct base of this PR.** Its `Adopters` and enforceability tables are updated here rather than there, because this is the commit that makes the row true. - #2690 — merge findings across producers and mark consumption explicitly. Grandparent in the stack; its gate is what this PR proves in situ rather than in a fixture. - #2681 — findings crosswalk. Carries two dispositions this pilot surfaced and deliberately did not solve: a producer whose remediation site is not its `Location`, and the absence of any structured field for an "equivalent" disposition. - #2708 — `check-skill-precompute-compose.sh`'s argument parser is never exercised by its test suite. A real defect in this repo's tooling, found by this pilot's measured mutation run and filed separately so it is fixed on its own merits. Not closed by this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 15, 2026
…ity crosswalk (#2737) Phase 4 of the boris-routines-adoption plan. Closes #2681. Builds on merged #2715 → #2692 → #2690; rebased onto `main`. ## The trap this closes `plugins/review/context/severity.md` defines tiers by **tests**, not examples — "The test decides the tier". CRITICAL is "you can name a concrete input, caller, or subsequent otherwise-correct change that the defect makes produce a wrong result". A bare threshold cannot evaluate that predicate, so a threshold-to-tier table with no test in it is nominal closure. **The argument is the row.** Every crosswalk row carries the test its mapping asserts, and a rule whose tier cannot be argued from the test is **not admitted** — its detector reports to a human, which the contract's boundary already places outside this convention. The crosswalk is seeded with the four rules the first adopter evaluates, two of which argue a *non-emission* rather than a tier. ## The three judgment calls the issue handed over **1. Shared emitter — three implementations accepted, no registry cluster declared.** Decided on what the mechanism can actually detect, not on preference. `scripts/check-cross-plugin-source-drift.sh` clusters files by path-within-plugin under `plugins/*/` and compares hashes, so a `docs/` convention can never be a cluster; and a registered path that is not a live byte-identical 2+-plugin cluster fails as `REGISTRY STALE` — verified by adding one such line and running `--check`. There is also no emitter *code* to share: both existing emitters are prose a model executes. What prevents drift is one owner per mechanic, reached by pointer. The revisit trigger fires itself — the first emitter code copied across two plugins is reported `UNREGISTERED` by that same script. Nothing mechanical catches producer drift today and Enforceability says so rather than dressing it up. **2. The scope fence (comment 1) — a real disposition, not a column.** `Location` still names the detection site and is never retargeted; the producer names the remediation target in `Action`. `fix-pass-mode.md` Step 2 routes such a row to surface-only and Step 4 surfaces it under a fourth named trigger. Deciding it at classification rather than apply time is what keeps Step 3's counts honest — the correctness count is what Step 4 will attempt. A remediation-target column is recorded as considered and rejected: what it would enable is an unattended two-file apply, which is the one thing the fence exists to forbid, and the existing contained-fix criterion already reaches that verdict without new structure. **3. The equivalence disposition (comment 2) — a property of the rule, not of the finding.** Three outcomes, three homes. A declined candidate is coverage, reported as a count per rule id in `## Surfaces`. A real finding an operator accepted belongs to `finding-suppression/` — and binding an automatic decline there would launder a consent gate, so a producer *proposes* an entry and never writes one. A not-a-defect claim without the rule's stated evidence emits a row. Because disposition belongs to the rule, it is declared once in the crosswalk and only the count is per run — no field is added to the 7-column shape and no new file type with no reader is invented. ## Also in scope - **Rule and threshold vocabulary.** Every emitted row leads its `Finding` cell with the rule that fired and the threshold it crossed in the run's own values. A rule id is `rule-<slug>`, qualified `<plugin>/<skill>/rule-<slug>` — exactly the `check:` constituent `finding-suppression` hashes into a `finding_id`, so it is one vocabulary rather than two. - **`REVIEW.md` cited as the consumer-precedence override**, with the reason it is not decorative: this repository's own vocabulary folds Critical and Important onto one marker, so the tier name does not survive the fold and the row's argued test is what lets a reader re-derive which side of it a finding sat on. - **Auto-applicability settled per rule at contract time.** Cross-file and architectural-judgment rules are never auto-applicable — layering, abstraction, and coupling detectors are *designed to inform a human*, which is the intent of the route rather than a limitation in it. Shaping a rule to look auto-applicable, by narrowing `Location` or lowering `Confidence`, is named as the failure it is. - **The tier argument moved rather than being copied.** Why a productive survivor is IMPORTANT and not CRITICAL is a rule-to-tier argument every consumer of the rule needs; it now appears in exactly one file, and `persist-findings.md` keeps only the class-to-rule map a mutation run owns. - **Enforceability re-rated.** `Tier` is machine-computed moves from reasoning-only to detect-then-judge, because a rule id in every row gives a gate something to check. Three rows added, including one recording honestly that a declined-candidate count is greppable but no gate can know what a run examined. ## Versions `review` 0.20.1 → 0.21.0, `mutation-testing` 0.2.0 → 0.3.0, detector-findings convention 1.1.0 → **2.0.0** (major under the contract's own rule: three producer-owned obligations are added, and a producer ignoring any of them stops conforming). ## Sanity checks (issue #2681) ```text grep -c "finding-suppression" docs/conventions/detector-findings/README.md -> 3 grep -c "REVIEW.md" docs/conventions/detector-findings/README.md -> 2 scripts/check-cross-plugin-source-drift.sh --check -> exit 0 scripts/check-detector-findings-crosswalk.sh --check -> exit 0 "Crosswalk OK: 4 rule row(s), every disposition argued from a stated test." ``` ### One stated divergence from the issue's literal check The issue specifies `awk -F'|' '/^\| rule-/ …'` and `grep -c "^| rule-"`, both of which presume a **bare** rule id at the start of each row. Independent verification found that bare ids are exactly what makes the table unsafe: it is a shared cross-producer registry, and the gate it enables resolves an emitted id against a row by exact match, so two producers sharing a slug would resolve to the wrong row. Rule ids are now fully qualified, and `^| rule-` matches zero rows. **The bar is unchanged and now stronger; only the pattern that measures it moved.** `scripts/check-detector-findings-crosswalk.sh` enforces the same two properties the issue's checks were proxies for — every row has a non-empty test cell, and the row set is exactly the rule set — plus four the greps could not reach: a prose-free cell (an em dash passes a non-empty test while arguing nothing), an unqualified or duplicated id, a row whose cells an unescaped pipe has shifted, and a restatement of the findings-file table. It locates the table by its exact header rather than a row prefix, so a neighbouring table can neither satisfy it nor be dragged into it. It ships with 13 discriminating self-tests and runs in CI as `detector-findings-crosswalk-gate`, so the bar outlives the issue that stated it — which is what the Enforceability row previously claimed falsely. Also green after re-syncing from the parent (which moved three times, once with a conflict): `check-changelog-parity.sh --check / --check-order / --check-bump`, `check-changed-skills.sh`, `check-contract-slice-prune.sh --check`, `check-shell-portability.sh`, `shellcheck`, `markdownlint-cli2` (1127 files, 0 issues), `typos` (0). Pointer-not-copy guard `! grep -q "^| Rank | Tier | Confidence" docs/conventions/detector-findings/README.md` still passes, and is now also enforced by the crosswalk gate. ## What independent verification changed A fresh-context verifier (rationale withheld) returned 2 blocking, 4 should-fix and 7 nits. All were applied; the two blocking ones were real defects in the first commit: - **The crosswalk falsified itself.** A bare-id table sat three lines below prose claiming a bare id "is only ever read inside one producer's own file". Fixed by deleting the short form rather than repairing the clause. - **`rule id == finding-suppression check:` was false.** `suppression.md` keys its `check:` to the mutation *operator*, because a suppression retires per mutant while a rule classifies a disposition. The pilot's file was right and this doc was wrong; the claim is downgraded to shape-compatibility and `suppression.md` is untouched. - **A consumption loop.** Every rule this producer emits is off-site, so Step 2 routed every row to surface-only, so nothing applied, so no record was written, so Step 1 never subtracted the file — re-merged and re-surfaced forever. See below. - Both non-emitting crosswalk rows argued from `severity.md`'s **Action** column, not its **Test** column, failing the admission test they sit under. - Two pointers landed on the wrong heading (`Findings-file shape` vs `Findings-writer contract`), including one pre-existing instance. ## The consumption-record repair The record's trigger was "the action applied anything". That conflated a gate the operator **declined** with a pass that **ran to completion and surfaced every row** — only the first is what the no-record rule was for. The trigger is now a **consented gate followed by a pass that ran to completion**, whether or not it mutated the tree. A declined gate and the non-interactive STOP still write nothing: both emit a plan and process nothing, and a record there would retire files the action never opened — a worse silent drop than the loop being closed. `Consumption is per FILE, not per row` is **extended** to zero-applied rather than merely applied to it, and the PR says so: its wording covered a *partly* surfaced file, which presupposes a non-empty applied set, so the zero case was silent rather than decided. Two zero shapes are argued separately because the recoverability argument is vacuous for the second — a file whose rows were all surfaced is retired because each is rendered in the "Not applied" table with its producer; a **coverage-only** file (no data rows, the ordinary output of a clean detector run) is retired because it carries coverage rather than findings and has nothing to recover. The trade is stated rather than sold: re-running the producer is the only route back, and for a mutation run that is a full re-audit. ## Deliberately out of scope - **A mechanical conformance gate.** The contract still defers it and now records four detect-then-judge verdicts naming what a gate could check. Shipping one unasked would be a different PR. - **A remediation-target column and a disposition field.** Both considered, both rejected in the doc with the reasoning recorded so they are not silently re-litigated. - **Rules for detectors that do not exist.** The crosswalk is seeded with the four rules the one live adopter actually evaluates; inventing rows for a hypothetical layering detector would be fabrication, and the admission test is what the next detector brings its rules to. ## Related - #2715 — the mutation-testing pilot (**merged**). Its two pilot observations are this issue's spec, and its `persist-findings.md` is edited here so the Adopters row stays true. - #2692 — the detector-findings owner doc this PR hardens. Parent in the stack. - #2690 — merge findings across producers and mark consumption explicitly. Grandparent; its Step 2 collapse key and Step 5 record are what the off-site disposition composes with. - #2684 — the can't-fail test detector, named in the pilot's first comment as the next producer to land on the off-site problem. Not closed by this PR; it is the first consumer of the disposition added here. - ADR 0010 — the findings coexistence decision the merge set rests on. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 17, 2026
…sub-threshold miss (#2905) Closes #2865. ## What this fixes `scripts/silent-revert-incidents.txt` pins a `clean` row that is supposed to be the closest a non-incident got to the 200-line threshold without crossing it — the row that breaks first if a threshold change starts taxing ordinary development. The pinned commit was not that, and its note named a number that is not a pull request. **Defect 1 (the re-pin).** Measured over the file's own 500-commit corpus (`7b47d2253~500..7b47d22`) at the pinned invocations (#2843, `GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_NOSYSTEM=1`), the pinned `c8470efd0` scores **136** blamed lines — sixth-closest of the eight commits in the 100–199 band. The true closest miss is **`9a2307c43` (#2189) at 195 lines** from `3584ae1fa` (#2183) — a margin of 5 lines, not the ~71 the old row implied. `9a2307c43` is now the lead `clean` row. **Defect 2 (the wrong PR number).** The old note credited the deleted content to #2679, which is a closed issue in this repository, not a pull request (`gh pr view 2679` cannot resolve it). The blamed lines trace to `6370a44e7`, the squash commit that landed #2715 — `gh api .../commits/6370a44e7/pulls` returns only #2715, and the commit's own body says `Builds on merged #2715 → #2692 → #2690`. The row is kept as a second guard with its note corrected, rather than dropped: it is still a verified-legitimate quiet commit, and keeping it costs a few lines of prose. **Defect 3 (the "once a month" rate)** was already fixed by #2847, which removed the rate claim from `scripts/check-silent-revert.sh` entirely. Nothing in this PR touches it. ## What this does NOT do Neither clean-row figure is CI-asserted, before or after this change. `clean` rows carry no bracketed attribution field (#2879) — their assertion is the absence of findings, which has no per-culprit count to pin — so the 195 and 136 are hand-measured prose, not watched numbers. The old row's recorded 129 drifting to a measured 136 under the pinned invocations without anything going red is exactly that gap, and the section comment now states it so a reader does not mistake the re-pin for an assertion. Both counts are written as floors ("no fewer than") because `attribute_file` drops `git blame`'s stderr (#2880), so any line blame fails on is silently not counted. ## Verification - Spot-checked both figures against PR #2843's pinned invocations before editing: `9a2307c43` reproduces **195** (75 lines `song-forms-examples.md`, 67 `box-model.md`, culprit `3584ae1fa`), `c8470efd0` reproduces **136** (109 lines `persist-findings.md`, culprit `6370a44e7`). - `bash scripts/check-silent-revert.test.sh`: **101 passed, 0 failed** on this branch. - `scripts/check-silent-revert.sh --verify-known-incidents`: exit 0 — all three `fires` rows reproduce their attributions exactly, and both `clean` rows stay quiet. - `scripts/check-silent-revert.sh --verify-restoration`: exit 0 — all 5 markers present. - A fresh-context verifier independently swept all 500 corpus commits twice (complete coverage: 487 `ok` + 2 `acknowledged` + 11 finding commits = 500) and reproduced every figure in the file. Its verdict: **195 at `9a2307c43` is the highest sub-threshold score** — the next highest is 188 (`3d69448cb`) — so the lead `clean` row pins the true closest miss. The two acknowledged commits were re-run with the ack file disabled and score 447 and 323, both above the threshold, so neither could displace it. ## Related - #2843, #2847, #2873 — the three PRs that reshaped the canary and this file ahead of this change; the figures here are measured under #2843's pinned invocations. - #2879 — records that `clean` rows carry no bracketed attribution field, which is why neither figure in this PR is CI-asserted. - #2880 — records that `attribute_file` drops `git blame`'s stderr, which is why both counts are written as floors. - #2831 / #2832 — the same wrong-PR-number defect shape, corrected earlier on the `fires` rows. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01LwdkpWf6bptu3AqTMoeg2H --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Closes #2678
Phase 1 of the boris-routines-adoption plan, implementing
ADR 0010
rather than re-deciding it. The ADR itself lands in #2686; this PR is independent of that merge and
cites the decision by URL rather than by in-tree path, so neither PR blocks the other.
Summary
review:fanout's findings-file shape is the whole integration contract — nothing authenticates thewriter — so any component of any shape that persists a conforming file reaches the apply relay
without a fanout edit. That is the property the detector work depends on, and it made a second
producer a silent data-loss bug:
fixtook the newest*.mddeclaringtype: review-findingsandmerged nothing, so a detector running after a full review shadowed the entire review with no error,
no warning, and a green run. This is the green-with-hidden-findings class
docs/conventions/liveness-assertion/README.mdexists to prevent, and it fails silently rather thanloudly — which is why it is settled before the second producer ships.
This blocks every other phase of the plan.
Changes
1.
fixconsumes a merge set, not the newest file (context/fix-pass-mode.mdStep 1).Candidates are every conforming file whose
branch:equals the current branch exactly; the set isthen reduced by what a
fix-pass-recordalready names as consumed. Coverage fields are unioned,not picked —
## Unparsedconcatenated,## Surfacesattributed per producer, every consumedfile's
tier:reported — because reporting one producer's## Surfacesline would hide a surfacethat ran and returned nothing, moving the hidden-findings failure up a layer instead of closing it.
The Step 3 plan header names the consumed set, one line per file.
2. Dedup is presence-only, deliberately narrower than Stage 3. Identical
Locationandidentical
Findingtext. The tempting key — normalized path plus a ±3-line bucket — sits behind aSonnet semantic stage the fix action does not run (
findings-normalization.md), and adopting thebucket without the semantics inverts the pipeline's own rule: "Minimize FALSE-MERGE over
FALSE-SPLIT — a false merge silently drops a real issue." Two distinct defects at
foo.ts:42andfoo.ts:44would collapse, and since Step 4 applies oneActionper row, one producer'sremediation would be discarded with no trace. Duplicate rows are possible and accepted.
3. The applied-plan record becomes the consumption ledger, written on every apply path
(Step 5). It was headless-
--yes-only — "Interactive and headless-stop paths write no record" —so a bound anchored on it was a no-op on the dominant interactive path and the merge set would have
grown without limit, re-injecting findings the required post-fix re-review had already resolved.
The one path that applies nothing still writes nothing.
Two things the issue did not specify, decided here
source-findings:serialization is pinned. Always a YAML block sequence of consumed filenames — one entry even for a single file, never a bare scalar. The exclusion is a match
against these values, so a writer emitting a scalar where the reader expects a sequence
under-matches silently and re-consumes exactly what the record was written to retire. Names
rather than repo-relative paths because the resolved
memory_dircan differ between the sessionthat wrote the findings and the session that applies them (a second checkout, or a
memory_diroutside the worktree), while the branch findings directory is a single home whose
<UTC-timestamp>-<topic>.mdnames are already unique within it. Comparing paths would failexactly where comparing names holds — which is the same interaction as the shared-directory case,
so the two are decided together.
Step 4's low-confidence fence, or narrowed out by an operator ("only the correctness ones"), are
still inside a file marked consumed. They are named in the record body and recovered by re-running
the review — a fresh pass re-finds anything still present and persists it as a NEW file, which
enters the next merge set as a fresh candidate. Stated explicitly because a reader implementing
"exclude the consumed file" would otherwise reasonably conclude those rows vanish. Row-level
ledgering was not adopted: ADR 0010 Decision 3 settles the ledger at file granularity.
Migration safety
all identities on one input.
.work/reviews/<branch-slug>/will see records where previously only headless
--yesruns produced them.Verification
scripts/check-changelog-parity.sh --check-bump origin/main→ exit 0.scripts/check-changed-skills.sh origin/main→CHECK-SKILL fanout: PASS — 0 errors, 1 warning(s)(the pre-existing no-Gotchas-surface warning). Editingcontext/makesfanoutachanged skill, so the full static gate ran.
markdownlint-cli2on all four changed markdown files → 0 issues.evals.jsonparses; 27 cases, no duplicate ids or names.review-findings/fix-pass-record/source-findingsacross*.md,*.sh,*.py,*.json,*.yml,*.txt: no parse path outsideplugins/review/—only the ADR, two historical CHANGELOG lines, and this skill's own files and evals.
Not executed, and stated as such: four of the issue's seven sanity checks are behavioral
(two-producer merge, the interactive-ledger re-run, the foreign-branch record, the
foo.ts:42/foo.ts:44guard). This skill is a prose contract with no executable surface, so thosefour are pinned as model-graded eval cases 24–27, not as executed fixtures.
evals.jsoncarries nofixture files for them.
Independent review
A fresh-context reviewer audited the diff against ADR 0010 and #2678 with the authoring rationale
withheld. It confirmed all three ADR decisions present and every issue work item implemented,
independently re-ran the three mechanical sanity checks, and returned SHIP-WITH-FIXES with four
blocking findings. All four are fixed in
c00f874d, along with every MEDIUM and LOW it raised:/review:fanoutregenerates only fanout's ownrows, so a detector's row deferred by operator narrowing was retired permanently — a new silent
drop inside the change that exists to close silent drops. Recovery now names the row's own
producer, and the record body must attribute each deferred row to its source file.
## Surfacesunion had no output slot — computed in Step 2 and printed nowhere,reproducing ADR Decision 1's harm one layer along. Now in the plan template, the report, and the
record body.
default-mode.md:77still said the fix action parses only## Findingsand## Unparsed—contradicting the new Step 2 for anyone reading the producer-facing contract. Narrowed, not
deleted:
## By dimensionalone stays presentation-additive.source-findings:as a bare scalar andthose records survive the upgrade (gitignored local state), so a legacy record would have
subtracted nothing and re-injected already-applied findings. The reader now tolerates a scalar as
a one-entry sequence compared by base name; CHANGELOG carries the migration note; eval 28 pins it.
Also fixed from the same pass: collapsed-row field rule (MAX tier, MAX confidence, all distinct
Actions retained) andRankrenumbering after merge, which also fixes the apply order; "identical"defined as byte-for-byte after unescaping; an abnormally terminated apply writes no record; a
minimally conforming producer missing
tier:or## Surfacesis consumed withtier: unstatedrather than skipped or guessed at; the
> DEGRADED:blockquote collapses to its first line; thenames-not-paths rationale replaced (it claimed a cross-directory property the design lacks — the
true reason is that both sides are always read from one directory); the empty-set message no longer
tells an operator who already ran the review to run it first; stale single-file language cleared
from the SKILL.md description and the plugin README.
Eval changes
--yesapply now writes the record; the old expectation assertedthe opposite.
--yespath is unchanged.Surfaces (union)line, theledger subtraction proving the record is written on the interactive path, the both-sides
exact-
branch:filter, the false-merge guard atfoo.ts:42/foo.ts:44with the collapsed-rowfield rule and
Rankrenumbering, and the legacy-scalarsource-findings:tolerance.Related
ADR 0010, #2686 (the ADR's PR), #2679 (detector-findings convention stub — unblocked by this)
Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com