diff --git a/proposals/0002-validate-warning-dead-renames.md b/proposals/0002-validate-warning-dead-renames.md new file mode 100644 index 0000000..31a2e9b --- /dev/null +++ b/proposals/0002-validate-warning-dead-renames.md @@ -0,0 +1,146 @@ +# 0002 — `validate` warning for dead `renames` overlay entries + +| | | +|---|---| +| **Status** | Accepted | +| **Tracking issue** | #19 | +| **Principles touched** | P4 | + +## Problem + +A manifest `renames:` entry whose `from` names no rule the active set actually renamed is +**silently ignored** at materialization. The overlay maps a pack's published rule id to a local +id (`{pack, kind: rule, from, to}`, decision 0007, D38 at `archive/pre-github`); the mapping is +consulted only through `materialize.local_rule` while walking selected methods +(`src/ea_core/materialize.py`). An entry that no selected method reaches is never applied and +never reported — the closure just doesn't call `local_rule` with that key. + +So a user who mistypes `from` (or `pack`), or whose target rule left the active set after a +`method remove`/`migrate`, gets **no signal**. The overlay entry is dead weight, indistinguishable +in the manifest from a live one. The Phase-26 note flagged "a `validate` warning surfacing dead +rename entries" as a later nicety; this is that nicety. + +The gap is exactly the one `policy.dangling` already fills for the sibling overlay: a +method-deprecation policy naming an inactive method is warned, not enforced +(`validate._policy_checks`, D23/D25). `renames` is the only org overlay without such a mirror — +`policies` has `policy.dangling`, `relationships` has `relationship.overlay_dangling` +(`model.py`), `renames` has nothing. + +## Design + +Core, read-only, non-blocking. `validate` emits one **`warning`** diagnostic, code +`renames.dangling`, `path` = the manifest, for each `renames` entry that resolves to nothing +against the active materialized set. No exit-3 path, no schema change, no new write. + +### Placement + +A new `_renames_checks(model, diags)` in `src/ea_core/validate.py`, called from +`validate_semantic` beside `_policy_checks` and `_shadow_checks`. It reads +`model.manifest.raw["renames"]` (the overlay is carried on the loaded manifest) and the active +rule provenance, and appends warnings sorted deterministically by `(pack, from)` (P1). + +Message, mirroring `policy.dangling`'s register: + +``` +rename overlay entry {pack}:{from} → {to} applies to no active rule +``` + +### What "resolves to nothing" means — provenance, not id presence + +A **live** `kind: rule` rename produces exactly one materialized rule: the file is written under +its local `to` id, and materialization already stamps `source.original_id = from` / +`source.pack = pack` on it (`materialize.py` `_claim`; the `original_id` breadcrumb is written +*only* when `local != rid`). So the set of applied renames is recoverable from the store: + +``` +applied = { (rule.source.pack, rule.source.original_id) + for each active rule whose source carries original_id } +``` + +An entry `{pack, kind: rule, from, to}` is **dead** iff `(pack, from) ∉ applied`. This is the +precise reading of the issue's "`from` matches no active rule": post-rename the active rule is +keyed by `to`, so presence of the *`to`* id is not the test — the provenance breadcrumb is, +and it already exists on disk for exactly this purpose. + +Only `kind: rule` entries are checked (the whole enum today; extending the enum is #8, which +would extend this check in lockstep). + +### One additive model touch + +`load_definitions` currently discards the store's `source` block — `_ruledef_from_doc` keeps +only `id`/`kind`/`applies_to`/`params`, and `defs.methods` keeps only the inner method doc. So +the provenance the check needs is on disk but not in the loaded `Model`. The implementation +threads it in minimally: give `RuleDef` an additive `source: dict = {}` field populated from the +rule doc's `source`. Additive, provenance-only, and independently useful (the model today throws +away lineage the materializer deliberately recorded). `validate` reads +`model.definitions.rules[*].source`; nothing else changes. + +### Forward-looking entries are warned too — deliberately + +`manifest rename add` accepts a `from` for a **not-yet-active** pack as *forward-looking* +(manifest.md; essential because the clashing `method add` hasn't run yet). A forward-looking +entry is, by construction, dead *right now* — and it is **indistinguishable from a typo'd +`pack`/`from` from repo state alone**. This proposal warns on it anyway, for parity with +`policy.dangling`, which already warns on a forward-looking policy naming a not-yet-active +method. The register makes this safe: it is a non-blocking heads-up ("this entry isn't doing +anything yet"), never a red repo, so a green forward-looking manifest stays green. The noise cost +is real but bounded and matches an accepted precedent; the alternative of suppressing it silently +loses the most common real bug (a mistyped pack name looks exactly like forward-looking). See +forks for the tiered variant. + +## Forks considered + +- **Do nothing** — dead entries are harmless (ignored, never *mis*-applied); the only cost is a + silent typo. But the same argument was rejected for `policy.dangling`: a silently-ineffective + governance/tailoring entry is a latent surprise, and the overlay-consistency mirror is cheap. +- **Refuse at `manifest rename add`** (usage error) instead of warning at `validate` — breaks the + forward-looking case manifest.md explicitly supports (a rename staged before its clashing + `method add`); a hard refuse there would make legitimate staging impossible. A warning is the + right register — it surfaces the dead entry without forbidding the forward-looking one. +- **`to`-id-presence heuristic** (dead iff `to ∉ active rules`) — needs no model change at all, + but a *native* rule coincidentally named `to` masks a genuinely-dead entry (false negative), + and the collision guard runs on post-rename ids so a live entry always leaves `to` present + anyway. Provenance is only one additive field more and is exact; the heuristic trades + correctness for that field. +- **Suppress forward-looking (pack-not-active) entries; warn only when the pack is active but + `from` is unmatched** — quieter, and the pack-active case is the unambiguous-typo tier. But + "pack active" is itself only knowable from source breadcrumbs, a mistyped `pack` name is then + never caught (it reads as forward-looking), and it diverges from the `policy.dangling` + register for no schema saving. Recorded as the tiered alternative; the flat parity design is + simpler and catches the common typo. Revisit if the forward-looking noise proves annoying in + practice. +- **Fold into the rename-kind-extension issue (#8)** — kept separate: this applies to today's + `kind: rule` renaming and is independent of adding new kinds. #8, when it lands, extends the + same check to the new kinds rather than replacing it. + +## Compatibility & migration + +Additive. A new advisory diagnostic **code** (`renames.dangling`) joins the explicitly +non-exhaustive dotted-code set cli.md documents ("e.g. …"); consumers match codes, so a new +warning code is additive by the contract's own design. No exit-code change (warnings never move +exit status), no `schema_version` bump, no manifest-shape change. The `RuleDef.source` field is +an additive dataclass member with an empty default. Existing repos are unaffected: a repo with no +dead entries sees no new diagnostics; a repo with a dead entry that was silently green stays +green, now with a warning. This does not move a binding decision — it is the advisory consequence +of decision 0007's overlay, mirroring how `policy.dangling` rode alongside decision 0008 without a +decision-record change. + +## Proof + +**Objective half** (implementation PR): + +- A scenario e2e case (extending the manifest-overlay validate suite): a manifest with a live + rename (materialized, no warning) plus a dead entry whose `from` matches no applied rename — + `validate` stays exit 0 and emits one `renames.dangling` warning naming `pack`/`from`/`to`; + a second dead entry produces a second warning, and the two are ordered by `(pack, from)` + (byte-stability). A forward-looking entry (pack not yet active) warns identically — asserting + the deliberate parity choice. A repo with only live renames emits none. +- Unit coverage for the `applied`-set derivation from rule provenance (renamed vs. un-renamed + rules) and the additive `RuleDef.source` population. +- Docs in the same PR: `docs/reference/cli.md` (add `renames.dangling` to the diagnostic-code + examples) and `docs/reference/manifest.md` (a line under the `renames` section noting a dead + entry is warned, not enforced, and pointing at `validate`). + +**Agent-facing half:** none required — this is a core diagnostic with no surface/skill change and +no new judgment. The plugins already teach `validate` as the repo-health verb; the new warning +rides that verb with no prompt edit.