feat(beagle-analysis): four planning guardrails from issue-124 retention post-mortem

plugins/beagle-analysis/skills/brainstorm-beagle/SKILL.md

@@ -74,6 +74,7 @@ This step exists because of a specific, expensive failure: drafting a spec that
    - Say so plainly, citing the file(s) — "`agent/src/spill.rs` already implements head-tail capping with a sidecar, and it's tested."
    - Reframe the spec around the gap, not the whole feature: extend/fix/wire-up what exists rather than rebuild it. Often this shrinks the spec dramatically or dissolves the need for one.
    - Record the discovery as a Key Decision ("Build on existing `spill.rs` rather than a fresh truncation module") so the downstream planner doesn't re-litigate.
+5. **Composition check.** For each existing mechanism the sweep surfaced, ask: does it sit **upstream or downstream in the same data pipeline** as the feature, and does it transform (truncate, filter, buffer, reorder, dedupe) the data this feature depends on? If yes, "the mechanism exists" is not the end of the inquiry — its *composition* with the new feature is the load-bearing question. Record the interaction as a Key Decision ("bash already caps output to 50 KiB upstream; the spill must capture before that cap or it cannot deliver full output") AND tag that decision `needs-spike-before-planning`, adding to its Rationale: `**Spike required:** before plan-lock, verify the upstream/downstream data shape (what it truncates/filters/buffers/reorders/dedupes and whether this feature captures before or after) against this repo and revise this decision if the result diverges from the assumption.` A new mechanism that composes wrongly with an existing one in the same pipeline ships broken even though neither piece was reinvented.
 
 This is distinct from *Explore context* (step 2), which reads to understand the project. The prior art check is adversarial: its job is to disprove the assumption that the feature is new, before that assumption hardens into a spec.
 
@@ -228,6 +229,7 @@ After drafting the spec, review it for:
 5. **Missing rationale** — do constraints and out-of-scope items explain WHY? Add reasons.
 6. **Scope** — is this focused enough for a single planning cycle?
 7. **Reinvention (brownfield)** — does any must-have rebuild a capability that already exists in the codebase? If the prior art check wasn't run, run it now. Reframe duplicated requirements around the actual gap.
+8. **Consumer (brownfield)** — does any must-have introduce a new externally-facing capability (API surface, command, endpoint, exported contract) that nothing else in the spec consumes? If who/what consumes it isn't named, either name the consumer or move the item to *Future Considerations*. Unconsumed externally-facing surface is speculative — fix it before presenting.
 
 Fix issues inline. Then present to the user for review.
 
@@ -264,4 +266,5 @@ See `references/spec-reviewer.md` for the detailed review checklist.
 - **No implementation** — WHAT and WHY, never HOW
 - **Capture everything** — ideas outside scope go to Future Considerations, never lost
 - **Incremental validation** — confirm understanding before moving on
+- **Every capability has a consumer** — a must-have that introduces externally-facing surface names who/what consumes it, or it moves to Future Considerations; unconsumed surface is speculation
 - **The spec stands alone** — anyone should be able to read it and understand the project

plugins/beagle-analysis/skills/brainstorm-beagle/references/spec-reviewer.md

@@ -78,6 +78,28 @@ For a feature being added to an existing codebase, the most expensive miss is sp
 
 If the prior art check was skipped, run it now: `grep -riE '<capability synonyms>'` across all source roots before approving. One matching file means a requirement needs reframing from "build X" to "extend/fix/wire-up the existing X."
 
+### 8. Consumer (brownfield only)
+
+A must-have that introduces new externally-facing surface with no named consumer is speculative — it can't be planned or verified.
+
+| Check | What to look for |
+|-------|-----------------|
+| Surface has a consumer | A must-have introduces new externally-facing surface (API surface, command, endpoint, exported contract) but nothing else in the spec consumes it |
+| Consumer is named | Where such surface exists, the spec names who/what consumes it — not left implicit |
+
+If who/what consumes the surface isn't named, either name the consumer or move the item to *Future Considerations* before approving.
+
+### 9. Composition (brownfield only)
+
+For an existing mechanism the prior-art sweep surfaced that sits upstream or downstream in the same data pipeline, "the mechanism exists" is not the end — its composition with the new feature is the load-bearing question.
+
+| Check | What to look for |
+|-------|-----------------|
+| Pipeline interaction surfaced | A surfaced mechanism sits upstream/downstream in the same data pipeline and transforms (truncate, filter, buffer, reorder, dedupe) the data the feature depends on |
+| Composition recorded | The interaction is recorded as a Key Decision tagged `needs-spike-before-planning`, not left as an unexamined assumption |
+
+A new mechanism that composes wrongly with an existing one in the same pipeline ships broken even though neither piece was reinvented.
+
 ## Calibration
 
 **Only fix issues that would cause real problems downstream.**

plugins/beagle-analysis/skills/resolve-beagle/SKILL.md

@@ -69,6 +69,8 @@ Two categories count as gaps:
 | Contradiction | Requirement conflicts with another requirement, with a constraint, or with Out of Scope |
 | Untestable success | No observable way to verify the requirement was met |
 | Implementation leakage | A requirement prescribes HOW instead of describing WHAT |
+| Unconsumed surface | A must-have introduces new externally-facing surface (API surface, command, endpoint, exported contract) that nothing else in the spec consumes |
+| Unresolved composition | An existing mechanism sits upstream/downstream in the same data pipeline and transforms (truncate, filter, buffer, reorder, dedupe) the data the feature depends on, but its composition with the feature is left unexamined |
 
 The reason to treat latent gaps as first-class: a spec that says "fast" or "good UX" hasn't been answered just because nothing was explicitly flagged. Planning will trip over those same words. Close them here.
 
@@ -143,6 +145,8 @@ Run the checks from `../brainstorm-beagle/references/spec-reviewer.md`:
 - No implementation leakage
 - All requirements testable
 - Constraints and Out-of-Scope items have rationale
+- No new externally-facing surface (API surface, command, endpoint, exported contract) without a named consumer — name it or move it to Future Considerations
+- Any existing upstream/downstream pipeline mechanism that transforms the feature's data is recorded as a Key Decision tagged `needs-spike-before-planning`, not left unexamined
 
 Fix anything that surfaces inline before handing back to the user. If new gaps appear during the rewrite (they sometimes do), add them to a "new gaps surfaced" list and ask the user whether to resolve them now or leave for a later pass.
 

plugins/beagle-analysis/skills/write-plan/SKILL.md

@@ -98,6 +98,7 @@ Task 0 is non-optional when the spec's Key Decisions rest on tool behavior the t
 - "Library Y's default test attribute uses the same pool config production uses."
 - "CLI Z's introspection covers every query shape we'll write."
 - "Migration framework W handles concurrent migrators against a fresh DB idempotently."
+- **Input-shape assumption:** the spec assumes upstream input arrives in shape X — whole / sorted / deduped / complete / already-validated — but nobody verified that shape holds in *this* repo. (The retention spec assumed "tool output arrives whole"; for streaming tools it doesn't — a rolling buffer had already evicted the prefix upstream before the new layer saw it.) A load-bearing assumption about upstream data shape is a spike candidate, not a given.
 
 If the spike fails or surfaces caveats, **stop and revise the spec** — do not paper over the discovery with extra plan tasks. A spec that locks a Key Decision on a tool that does not behave as assumed is a spec that needs another brainstorm pass, not a plan that needs more workarounds.
 
@@ -282,6 +283,8 @@ Four rules for the test code you write into Step 1 of each task.
 
 **Pin the spec, not every conceivable edge case.** A test exists to prove a specific spec requirement is met or a specific failure mode (named in the spec or in your Assumption Audit) is closed. One precise test per behavior the spec calls out, plus one test per named bug class, is the target. If you find yourself writing the 5th boundary test for the same function "just in case," stop — the marginal coverage is probably negative once you count maintenance cost and the noise it adds to the suite. Speculative input-space exhaustion belongs in property-based tests or fuzz harnesses if the project has them; otherwise it's overengineering. YAGNI applies to tests, not just impl.
 
+**Exception — payload-preservation invariants.** When a test pins a preserve/recover/transform invariant (output survives truncation, data round-trips, ordering holds), the set of structurally-distinct producers and the corrupted region are NOT speculative edge cases — they ARE the spec. Enumerate every producer whose data path differs (streaming-with-eviction vs. whole-string) and assert the sentinel in the damaged region for each. YAGNI does not license collapsing these to one happy-path producer — that is exactly how a recovery test passes against the one tool without the bug.
+
 ## Behavior Contract Discipline
 
 Two rules for the behavior contract under each implementation step.
@@ -366,6 +369,8 @@ After drafting the complete plan, look at the spec with fresh eyes and check the
 | **Spike candidates** | Re-read the Assumptions block and the spec's Key Decisions. For every claim of the form "tool X does Y" where neither this repo nor the team has a working example, is there a Task 0 spike? If not, add one or revise the spec. |
 | **Parallel-implementation gate** | Does the plan add a second backend/platform/adapter behind an existing trait or interface? If yes, is there a final task that runs the canonical contract suite against BOTH implementations and asserts byte-identical observable behavior? If not, add it. |
 | **Failure-propagation contracts** | For every task that introduces a new fallible operation (serialize/parse/convert/open/connect), does its behavior contract name the propagation policy? `.unwrap_or(<plausible fallback>)` without explicit contract rationale is a bug class — fix the contract. |
+| **Consumer check** | Every new public API surface this plan introduces (trait method, exported fn, public field, endpoint, CLI flag) has at least one named **production consumer** — a caller on a non-test path — *in this same plan*. A contract/unit test is NOT a consumer. If the only caller is a test, either name the production consumer or cut the surface (tagging it deferred with the consuming work as a numbered follow-up). Fail the plan otherwise — a primitive that exists only to satisfy a contract test is dead surface forcing dead impls. |
+| **Discriminating assertion** | For each test, describe a plausible broken/no-op impl that still passes its assertion. If one exists, the assertion is on the wrong target — move it to the region/shape the bug would corrupt. For preserve/recover/transform invariants: assert a sentinel in the region the bug *damages* (the dropped middle, never the surviving head/tail), exercised through **every structurally-distinct producer** (a streaming producer whose buffer evicts is not the same path as a whole-string producer). Fail the plan if any test admits a false-pass impl. |
 | **Per-task suite green** | Does every task's Step 4 specify both the single-test command AND the broader-scope suite command? Single-test-only passes hide cross-task regressions. |
 | **Pattern application audit** | If any Pattern applies to many sites, is the final Audit task present (grep + production-config divergence enumeration + 3-site sample-verify)? |
 | **Project conventions** | Does the plan respect the project conventions you read (e.g. AGENTS.md or CLAUDE.md)? (e.g., real-path test coverage, comment policy, commit format) |
@@ -438,7 +443,7 @@ The plan is a handoff document, not an instruction to execute. After writing, wr
 - **References point, they do not paste** — `file.ext:line-line` is the reference; inline code blocks under "Reference:" rot the moment the underlying code shifts
 - **DRY repetition with Patterns** — when a transformation applies to N sites, name the pattern once; each task still owns its own files, test, and commit
 - **Tests reuse existing scaffolding** — grep before inventing; new helpers in the plan need a named existing one that didn't fit
-- **YAGNI for tests too** — pin the spec and named bug classes, not every conceivable edge case; speculative input-space exhaustion is overengineering
+- **YAGNI for tests too — except payload-preservation invariants** — pin the spec and named bug classes, not every conceivable edge case; speculative input-space exhaustion is overengineering. But for preserve/recover/transform invariants, the structurally-distinct producers and the corrupted region *are* the spec — enumerate every producer and assert the sentinel in the damaged region
 - **Sweep on the way out** — every task that modifies a file ends by removing orphaned comments, unused imports, dead params/fields/helpers in that file; describe the sweep targets in plain language, not line numbers — the executor greps
 - **Surface assumptions** — bake them into the plan visibly, not silently into tasks; re-read the files the spec names rather than trusting the spec's characterization of them
 - **Respect project conventions** — every project has its own test commands, commit conventions, comment policy, and test-tier rules; read project conventions (e.g. AGENTS.md or CLAUDE.md, and equivalents like `CONTRIBUTING.md`) and let those shape the plan

plugins/beagle-analysis/skills/write-plan/references/plan-reviewer.md

@@ -25,6 +25,8 @@ Reviewer brief (dispatch as a subagent if supported, otherwise run inline):
     |----------|------------------|
     | Spec coverage | Every must-have requirement from the spec has a task that implements it. List any gaps. |
     | Test discipline | Every behavior-changing task has a failing-test step before the implementation step. Flag tasks that implement without a test. |
+    | Consumer check | Every new public API surface (trait method, exported fn, public field, endpoint, CLI flag) has a named production consumer — a caller on a non-test path — in this same plan. A contract/unit test is NOT a consumer. Flag any surface whose only caller is a test; it must be cut or tagged deferred with a numbered follow-up. |
+    | Discriminating assertion | For each test, a plausible broken/no-op impl that still passes the assertion means the assertion is on the wrong target. For preserve/recover/transform invariants: the sentinel must be asserted in the region the bug damages (the dropped middle, never the surviving head/tail), through every structurally-distinct producer (streaming-with-eviction vs whole-string). Flag any test a no-op impl would still pass. |
     | Assertion quality | Tests assert observable consequences (DB rows, files written, user-visible output), not dispatch ("the handler was called"). Flag dispatch-only assertions. |
     | Placeholders | TBD, TODO, "implement later", "similar to Task N", `unimplemented!()`, vague verbs without code. |
     | Type consistency | Function names, type names, and signatures match across tasks. `clearLayers()` in Task 3 vs `clearFullLayers()` in Task 7 is a bug. |
@@ -43,6 +45,8 @@ Reviewer brief (dispatch as a subagent if supported, otherwise run inline):
     - Contradictory or out-of-order steps
     - Placeholder content where real code/commands are required
     - Tests that assert dispatch instead of consequence
+    - A new public surface with no production consumer
+    - A test whose assertion a no-op impl would still pass / a payload-preservation test that checks only the surviving head-tail
     - Tasks so vague they can't be acted on
 
     ## Output Format

plugins/beagle-analysis/skills/write-plan/references/plan-template.md

@@ -123,6 +123,8 @@ git commit -m "<type>(<scope>): <imperative one-line summary>"
 - **Spec coverage:** [confirmed / list of gaps closed]
 - **Placeholders:** [none / list of fixes]
 - **Type consistency:** [verified across tasks]
+- **Consumer check:** [every new public surface has a named production consumer in this plan / list of surfaces cut or tagged deferred]
+- **Discriminating assertion:** [no test admits a false-pass impl; payload-preservation tests assert the sentinel in the damaged region through every structurally-distinct producer]
 - **Project conventions:** [list the specific project rules followed — testing tier rules, comment policy, commit conventions]
 
 ````
@@ -155,7 +157,7 @@ git commit -m "<type>(<scope>): <imperative one-line summary>"
 - **Recoverability rule** — after drafting each step, ask "can the executor recover this by reading the referenced file?" If yes, delete it. Verbosity is not specificity.
 - **Test step bodies show assertions + call site, not setup ceremony** — ~15 lines or less; the seed loop is recovered from existing types and helpers
 - **Tests reuse existing scaffolding** — grep for an existing helper/fixture/mock before inventing one in the plan; if no existing helper fits, name a specific one that was considered and why it didn't
-- **YAGNI for tests** — one test per spec requirement and per named bug class; no speculative input-space exhaustion
+- **YAGNI for tests — except payload-preservation invariants** — one test per spec requirement and per named bug class; no speculative input-space exhaustion. For preserve/recover/transform invariants, enumerate every structurally-distinct producer and assert the sentinel in the damaged region — those producers and the corrupted region *are* the spec, not speculative edge cases
 - **Behavior contracts: 3-5 bullets, past 5 replace with reference** — the contract is the new/changed behavior, not a re-derivation of the impl
 - **References point, they do not paste** — `file.ext:line-line` is the reference; do not inline the cited code
 - **Modify-file tasks include a sweep step** — remove orphaned comments, unused imports, dead helpers in the modified files; describe targets in plain language, not line numbers