|
| 1 | +--- |
| 2 | +name: spec-property-retirement |
| 3 | +description: > |
| 4 | + Internal playbook for retiring an authorable `packages/spec` property under |
| 5 | + ADR-0049 enforce-or-remove — choosing the removal route, the liveness-ledger |
| 6 | + discipline each route implies, the ADR-0087 conversion, the generated |
| 7 | + baselines, forms, docs and pin tests a removal needs, and the gates that fail |
| 8 | + when any of it is missing. Use when a metadata key is declared-but-unenforced |
| 9 | + and the job is to REMOVE it, or when a ledger `dead` verdict needs confirming |
| 10 | + before you act on it: "retire this property", "enforce-or-remove", "the ledger |
| 11 | + says dead", "remove the inert key", "close out the liveness worklist". NOT a |
| 12 | + customer-published skill — internal agent tooling (lives in .claude/, never in |
| 13 | + the published `skills/` dir). |
| 14 | +metadata: |
| 15 | + # Hides this skill from interactive `npx skills add objectstack-ai/objectstack` |
| 16 | + # discovery, same as dogfood-verification. Enforced by |
| 17 | + # packages/create-objectstack/src/template-consistency.test.ts. |
| 18 | + internal: true |
| 19 | +--- |
| 20 | + |
| 21 | +# Spec property retirement (ADR-0049 enforce-or-remove) |
| 22 | + |
| 23 | +A parsed-but-unenforced property is a silent no-op; for a security or capability |
| 24 | +property it is **false compliance** — `tool.permissions` promised an invocation |
| 25 | +gate nothing enforced, `flow.active: false` never stopped a flow. ADR-0049 says |
| 26 | +such a property must be **enforced**, marked **`experimental`**, or **absent**. |
| 27 | +This skill is the third path: what a removal actually costs, and in what order. |
| 28 | + |
| 29 | +Removing the key is maybe 5% of the work. The other 95% is that a key is |
| 30 | +authorable in ~14 places, and **every gate that guards one of them fails |
| 31 | +separately and sequentially** — one stale surface masks the rest, so you get one |
| 32 | +red build per surface instead of one for all of them. |
| 33 | + |
| 34 | +Read the verification side first: `packages/spec/liveness/README.md` (how a |
| 35 | +verdict is reached, `verifiedAt`, why a preview renderer is not a consumer) and |
| 36 | +AGENTS.md §"Touched `packages/spec`?" (the eight generated artifacts). This file |
| 37 | +does not repeat them. |
| 38 | + |
| 39 | +## 0. Before you remove: is removal the right disposition? |
| 40 | + |
| 41 | +- [ ] **Is it security/capability-shaped?** Then ADR-0049 binds and inertness is |
| 42 | + a defect, not debt. `rls.enabled` was "live with wrong evidence" and |
| 43 | + actually UNREAD — a disabled policy kept contributing its grant. That one |
| 44 | + got **enforced**, not removed. Enforcement wins when the feature exists. |
| 45 | +- [ ] **Is it docs-shaped?** `hook.label`, `hook.description`, `flow.description` |
| 46 | + have no runtime consumer and are **deliberately KEPT** — they document |
| 47 | + intent for the next reader (per ADR-0033, often a model). Record the |
| 48 | + exemption in the ledger `note` so the next audit doesn't re-litigate it. |
| 49 | + Benign display metadata (`description`, `tags`, `icon`) is never |
| 50 | + "misleading"; do not mark it `authorWarn` and do not retire it. |
| 51 | +- [ ] **Is there a committed roadmap?** Then `experimental` + a |
| 52 | + `[EXPERIMENTAL — not enforced]` `.describe()` marker, not removal. |
| 53 | +- [ ] **Same-major bookkeeping.** If an earlier conversion in the *same |
| 54 | + unreleased major* renames a key you are now deleting, **absorb** it: fold |
| 55 | + the rename into the removal and delete the rename entry. Composed, its |
| 56 | + effect is unobservable, and the conversion table's fixture-disjointness |
| 57 | + contract (§3) will fail if you stack them. Precedent: `agent.knowledge` |
| 58 | + swallowed the `topics`→`sources` rename pre-release. |
| 59 | + |
| 60 | +## 1. The build is the referee — not the ledger |
| 61 | + |
| 62 | +A ledger `dead` verdict is an **input** to removal, not a substitute for the |
| 63 | +build's own proof. It is a claim with a timestamp, and code moves under it in |
| 64 | +both directions (`flow.status` and `action.undoable` were both *understated*). |
| 65 | + |
| 66 | +So: **attempt the removal, then let the build rule on it.** In the #3896 |
| 67 | +close-out, `view.form.data` was on the worklist as dead ("no form-path reader in |
| 68 | +either repo") and the removal broke `gen:schema` — `defineForm` writes |
| 69 | +`data: { provider: 'schema', schemaId }` onto every `*.form.ts`, and |
| 70 | +`metadata-protocol` serves it to the metadata-admin pipeline. The right response |
| 71 | +is **not** to force the removal: correct the ledger entry to `live` with the real |
| 72 | +evidence and a `verifiedAt`, narrow the conversion, and pin the non-warn. One of |
| 73 | +fourteen keys was refuted this way — budget for it. |
| 74 | + |
| 75 | +Two corollaries: |
| 76 | + |
| 77 | +- **`tsc` and the gates are your best sweepers.** A `retiredKey()` tombstone |
| 78 | + types the key `never`, so *every* authoring site in the monorepo fails to |
| 79 | + compile. Two of three `template: true` sites in `examples/app-showcase` were |
| 80 | + found by the tombstone after a grep missed them. Let the tombstone find the |
| 81 | + callers before you go hunting. |
| 82 | +- **A grep can only prove presence.** To prove absence, close the call graph by |
| 83 | + hand (declaration → registration → accessor → *caller*) or author the property |
| 84 | + and boot the app. See the README's "How to verify a claim without fooling |
| 85 | + yourself" — including that `git grep -E` does not honour `\b` on macOS. |
| 86 | + |
| 87 | +## 2. The fork: which removal route (decides everything downstream) |
| 88 | + |
| 89 | +| Schema | Route | Mechanism | |
| 90 | +|---|---|---| |
| 91 | +| **not `.strict()`** | `retiredKey()` tombstone | `retiredKey(guidance)` in `packages/spec/src/shared/retired-key.ts` — `z.never({ error: () => guidance }).optional()`. Two channels: `tsc` (input type `never`) and the parse (the prescription itself, not "unrecognized key"). | |
| 92 | +| **`.strict()`** | delete the key + guidance map | Delete from the shape; add an entry to a `*_RETIRED_KEY_GUIDANCE` record consumed by a `z.core.$ZodErrorMap` passed as `z.object(shape, { error: … }).strict()`. Reference: `packages/spec/src/ai/tool.zod.ts:29-93,180`. Also `object.zod.ts`'s `UNKNOWN_KEY_GUIDANCE` for object top-level keys. | |
| 93 | +| **nothing parses it** | neither | A prescription nobody can receive is noise. Drop the baseline lines deliberately and say so in the changeset — precedent `packages/spec/src/kernel/plugin-runtime.zod.ts:243-248`. | |
| 94 | + |
| 95 | +Never plain-delete a key from a non-strict schema: zod strips it silently and |
| 96 | +you have replaced one silent no-op with another (the #2169 "Mark Done does |
| 97 | +nothing" shape). |
| 98 | + |
| 99 | +### ⚠ The ledger discipline is OPPOSITE per route — and the two failures are not symmetric |
| 100 | + |
| 101 | +The liveness gate walks the **schema's shape** and looks up each property in |
| 102 | +`packages/spec/liveness/<type>.json`. A `retiredKey()` is still a property in |
| 103 | +that shape. Therefore: |
| 104 | + |
| 105 | +| Route | Key still in the walked shape? | Its ledger entry | |
| 106 | +|---|---|---| |
| 107 | +| `retiredKey()` tombstone | **YES** (`z.never()` is a property) | **STAYS** — `status: "dead"`, a `verifiedAt`, and a `note` saying REMOVED + why the entry remains | |
| 108 | +| strict removal | no | **DELETED**, along with any CLI advisory-lint expectation | |
| 109 | + |
| 110 | +Get this backwards and: deleting a tombstoned key's entry reports it |
| 111 | +**UNCLASSIFIED and fails CI** (14 at once, in the #3896 sweep — the mistake this |
| 112 | +section exists to prevent). The other direction is worse in the long run: the |
| 113 | +gate never walks entries looking for absent schema keys, so a **leftover entry |
| 114 | +after a strict removal rots invisibly** — report `aria`/`performance` sat stale |
| 115 | +for a release before a human deleted them as hygiene. One direction fails |
| 116 | +loudly; the other never fails at all. |
| 117 | + |
| 118 | +Note template for a tombstone entry (verbatim house style, e.g. |
| 119 | +`liveness/action.json`): |
| 120 | + |
| 121 | +> `REMOVED <date> (#<issue>) — tombstoned at the schema (retiredKey carries the prescription; authoring it is a tsc error and a parse error) and stripped from sources by the protocol-<N> conversion. The entry stays because retiredKey keeps the key in the walked shape (the rls.priority precedent); <what to do instead>.` |
| 122 | +
|
| 123 | +### Writing the guidance string |
| 124 | + |
| 125 | +Five conventions, obeyed by all ~28 tombstones in tree: |
| 126 | + |
| 127 | +1. Backticked **fully-qualified** key first — `` `flow.errorHandling.fallbackNodeId` ``, not the bare tail. |
| 128 | +2. `was removed in @objectstack/spec <version> (#issue[, ADR-XXXX Dn])`. |
| 129 | +3. An em-dash clause on **why it was inert or wrong** — "it never had an effect", "no renderer ever read it". |
| 130 | +4. The imperative fix: for a rename, "use `<replacement>`" + "Rename the key; the value (…) is unchanged."; for a removal, "Delete the key." + **what the live mechanism actually is**. |
| 131 | +5. ``Run `os migrate meta --from <N-1>` to rewrite it automatically.`` — **only** when a conversion rewrites sources. No message names a conversion id; the conversion is referenced by the CLI command. |
| 132 | + |
| 133 | +This string *is* the migration doc for whoever hits it, including someone |
| 134 | +jumping several majors at once, whom the load-path conversion no longer covers. |
| 135 | + |
| 136 | +## 3. Register the surface (ADR-0087 D2/D3) — or the gate stops you |
| 137 | + |
| 138 | +`scripts/build-schemas.ts` gate (b) fails any newly-tombstoned key with no |
| 139 | +registered migration surface: the tombstone is audible only to whoever *hits* |
| 140 | +it, while `spec-changes.json`, the generated upgrade guide and the |
| 141 | +`spec_changes` MCP tool are the primary channel and would stay empty. |
| 142 | + |
| 143 | +- [ ] **A `MetadataConversion`** in `packages/spec/src/conversions/registry.ts`: |
| 144 | + kebab-case `id` ending `-removed`, `toMajor`, one |
| 145 | + `emit({ from, to: '(removed)', path })` per key (use the shared `stripKeys` |
| 146 | + helper), and a `fixture` whose `expectedNotices` equals the **key** count, |
| 147 | + not the item count. Walkers (`mapCollection`, `mapFlowNodes`, `renameKey`) |
| 148 | + live in `conversions/walk.ts` and are copy-on-write — return the input |
| 149 | + reference untouched when nothing matched. |
| 150 | +- [ ] **`surface` must end with the bare key.** The matcher is |
| 151 | + `surfaces.some((s) => s.endsWith('.' + key))` after |
| 152 | + `.flatMap((s) => s.split(' / '))`. Multi-key conversions join clauses with |
| 153 | + exactly `' / '` (house style since the tool sweep) and **each clause must |
| 154 | + end with its own key**. Caveat: only the last dotted segment is compared, |
| 155 | + so the schema name is never checked — `dashboard.aria` would satisfy |
| 156 | + `ui/FormView:aria`. Don't lean on the gate for attribution. |
| 157 | +- [ ] **`retiredFromLoadPath: true`** — for a retirement, always. Two distinct |
| 158 | + justifications, and they are not interchangeable: for a *rename* it means |
| 159 | + "no alias window, deliberately" (the tombstone owns the refusal; the entry |
| 160 | + exists so `spec-changes.json` and `os migrate meta` still carry it); for a |
| 161 | + **default flip** it is load-bearing for correctness — a loader that |
| 162 | + auto-applied `field-required-notnull-explicit` would stamp NOT NULL onto |
| 163 | + 17-authored `required: true`, silently restoring the tri-binding ADR-0113 |
| 164 | + removed. Only `migrate meta --from <old>` may apply a flip, where "this |
| 165 | + source predates the split" is a fact rather than a guess. |
| 166 | +- [ ] **A D3 chain step** in `packages/spec/src/migrations/registry.ts` — add the |
| 167 | + id to `MIGRATIONS_BY_MAJOR[N].conversionIds` and extend that step's |
| 168 | + `rationale`. `conversion.toMajor` **must equal** the step's major. |
| 169 | + ⚠ Nothing asserts "every conversion is wired into a step" directly, and a |
| 170 | + typo'd id is **silently skipped at replay**; the chain-replay test catches |
| 171 | + it only because an unwired fixture never reaches its `after`. So read that |
| 172 | + test's failure as "not wired", not "transform broken". |
| 173 | +- [ ] **Fixtures must be DISJOINT — twice over.** Every fixture is replayed |
| 174 | + through the *whole* table and must equal exactly its own `after`, with |
| 175 | + every notice attributed to its own id. Keep `before` minimal and avoid |
| 176 | + other entries' keys (a new `objects[].fields` fixture must not carry a bare |
| 177 | + `required: true`, or the notNull conversion fires on it). Second |
| 178 | + constraint, easy to miss: a `retiredFromLoadPath` fixture must ALSO be |
| 179 | + untouched by every *live-window* conversion, since a separate test asserts |
| 180 | + it passes through the default load path with zero notices. This |
| 181 | + disjointness contract is what forces same-major absorption (§0). |
| 182 | +- [ ] **Idempotence is by construction, not by test.** No test replays a |
| 183 | + conversion twice. A `stripKeys` deletion is idempotent (`if (!(key in |
| 184 | + next)) continue`) and `renameKey` refuses to clobber an existing canonical |
| 185 | + value; a default flip is **not** idempotent-safe and relies on its own |
| 186 | + guard plus `retiredFromLoadPath`. If your transform is none of those |
| 187 | + shapes, prove idempotence yourself — the CLI e2e |
| 188 | + (`packages/cli/test/migrate-meta.e2e.test.ts`) replays the migrated |
| 189 | + snapshot and asserts `applied` is empty. |
| 190 | +- [ ] **Response-surface keys with no source to rewrite** register as a |
| 191 | + `SemanticMigration` (D3 `semantic[]`) with non-empty `reason` and |
| 192 | + `acceptanceCriteria` instead — `EnhancedApiError.fieldErrors` is the |
| 193 | + worked example. |
| 194 | + |
| 195 | +## 4. The surface checklist |
| 196 | + |
| 197 | +Work top to bottom; each line has a gate behind it. |
| 198 | + |
| 199 | +- [ ] **Schema** — tombstone or strict removal (§2), plus the in-schema comment |
| 200 | + saying what was removed and what the live mechanism is. |
| 201 | +- [ ] **Orphaned value schemas** — a key's `XxxConfigSchema` with no other |
| 202 | + consumer goes with it (`PerformanceConfigSchema`, `AIKnowledgeSchema`, |
| 203 | + `ToolCategorySchema`). An exported schema with no consumer is read as a |
| 204 | + capability by whoever finds it (#3950 precedent). This — and *only* this — |
| 205 | + moves `api-surface.json`: that snapshot prints type *references*, not |
| 206 | + expanded shapes, so it is blind to key-level narrowing (#3883 removed three |
| 207 | + keys from `defineAction`'s input and the snapshot did not change). Its gate |
| 208 | + also lives in a different workflow (`TypeScript Type Check`, not |
| 209 | + `Check Generated Artifacts`) and reads the built `dist/*.d.ts`. |
| 210 | +- [ ] **Conversion + chain step** (§3). |
| 211 | +- [ ] **Liveness ledger** — per §2's route table, with `verifiedAt`. Update the |
| 212 | + README's per-type row **and its counts** (that table has drifted badly |
| 213 | + once; regenerate the counts with the python snippet in the README rather |
| 214 | + than hand-editing). |
| 215 | +- [ ] **Generated baselines** — `pnpm --filter @objectstack/spec gen:schema` |
| 216 | + moves `authorable-surface.json` (tombstone → a new `… [RETIRED]` line; |
| 217 | + strict removal → the line **vanishes**, which is gate (a)'s trip wire, so |
| 218 | + delete it in the same PR deliberately) and `json-schema.manifest.json`. |
| 219 | + Then `gen:spec-changes`, `gen:upgrade-guide`, `gen:api-surface`, |
| 220 | + `gen:docs`. See AGENTS.md for the you-changed-X → regenerate-Y table. |
| 221 | +- [ ] **Forms** — prune the `{ field: '<key>' }` input from |
| 222 | + `packages/spec/src/**/*.form.ts`. A form input for an unenforced capability |
| 223 | + is the UI half of false compliance. Leave a one-line comment where it was. |
| 224 | +- [ ] **i18n bundles** — pruning a form input changes the extracted labels: |
| 225 | + `pnpm i18n:extract` regenerates |
| 226 | + `packages/platform-objects/src/apps/translations/*.metadata-forms.generated.ts` |
| 227 | + (merge mode; a retirement is a pure deletion). Gated by `pnpm check:i18n`. |
| 228 | +- [ ] **CLI advisory lint** — `packages/cli/src/utils/lint-liveness-properties.ts` |
| 229 | + is ledger-driven, so a retired key stops warning by itself; update its |
| 230 | + **test** to assert the non-warn ("the strict parse owns them now"). |
| 231 | +- [ ] **Pin tests** — one negative asserting the prescription itself |
| 232 | + (``.toThrow(/<key>.*removed.*use `<replacement>`/s)`` — the `s` flag is |
| 233 | + house style, since the message spans lines) and one positive asserting |
| 234 | + `not.toHaveProperty(key)` for the non-strict strip path. Reference |
| 235 | + `packages/spec/src/ai/agent.test.ts:69-95`. |
| 236 | +- [ ] **Examples** — `examples/app-showcase/**` must stop authoring the key. |
| 237 | + `tsc` finds these for you on the tombstone route. |
| 238 | +- [ ] **Published skills** — `skills/*/SKILL.md` teaching the key (tables, |
| 239 | + `defineX` examples) — gated by `check:skill-examples` and `check:skill-refs`. |
| 240 | +- [ ] **Docs** — `content/docs/**` prose, tables and code blocks. Grep the key, |
| 241 | + then read the surrounding files: a removed key hides in a `defineFlow` |
| 242 | + example three sections from the reference table. |
| 243 | +- [ ] **Release notes** — the `### Dead spec clusters removed` table in |
| 244 | + `content/docs/releases/v<major>.mdx` **plus** the upgrade checklist. |
| 245 | +- [ ] **Changeset** — `major` for `@objectstack/spec`. AGENTS.md: a breaking |
| 246 | + changeset must carry the FROM → TO mapping and the one-line fix; it ships |
| 247 | + as `CHANGELOG.md` in the npm package and is what an upgrading agent greps |
| 248 | + after the tombstone error. `.changeset/tool-inert-keys-removed.md` is the |
| 249 | + model — its "The retirement kit:" section is the template to copy. |
| 250 | + |
| 251 | +**A correction (§1) must propagate to every one of these lines too.** When |
| 252 | +`form.data` flipped back to `live`, the ledger, the conversion and the tests were |
| 253 | +all corrected — but the release-notes row and the ledger README row kept listing |
| 254 | +it as removed, telling authors to delete a key `defineForm` writes. Found and |
| 255 | +fixed a PR later. The correction path is the one nobody has a checklist for; use |
| 256 | +this one. |
| 257 | + |
| 258 | +## 5. Run the gates so they can actually fail |
| 259 | + |
| 260 | +```bash |
| 261 | +cd packages/spec && pnpm build # REQUIRED first — see the dist trap below |
| 262 | +for c in check:liveness check:empty-state check:authorable-surface check:docs \ |
| 263 | + check:api-surface check:spec-changes check:upgrade-guide \ |
| 264 | + check:skill-refs check:skill-docs check:skill-examples; do |
| 265 | + pnpm -s "$c" >/dev/null 2>&1; e=$? # capture BEFORE anything else runs |
| 266 | + printf '%-28s %s\n' "$c" "$( [ $e -eq 0 ] && echo PASS || echo FAIL )" |
| 267 | +done |
| 268 | +cd ../.. && pnpm check:i18n && pnpm --filter @objectstack/spec test |
| 269 | +``` |
| 270 | + |
| 271 | +`check:liveness`, `check:empty-state`, `check:skill-examples` have no generator — |
| 272 | +a failure there is a real finding, not a stale artifact. |
| 273 | + |
| 274 | +## 6. Traps that have each cost a red build |
| 275 | + |
| 276 | +- **Stale `dist` (5+ false alarms in one line of work).** Packages load from |
| 277 | + `dist`. A local suite failing after you edited `src` usually means the dist is |
| 278 | + old, not that you broke it — `check:api-surface` reads `dist/*.d.ts` and will |
| 279 | + report phantom "breaking removals". `pnpm turbo run build --filter=<pkg>...` |
| 280 | + before you believe any local red, and before filing a bug about `main`. |
| 281 | +- **`| tail -1` masks the exit code.** Piping a gate to `tail` reports the |
| 282 | + pipeline's status, so a failing gate reads as green. Capture `exit=$?` |
| 283 | + explicitly (as the loop above does). A liveness failure hid behind this. |
| 284 | +- **Truncated greps miss authors.** `| head -8` hid the `SKILL.md` |
| 285 | + `defineSkill` example; `examples/` had three `template: true` sites, not one. |
| 286 | + Search files-by-context, then grep the key inside them — and treat `tsc` and |
| 287 | + the gates as the authoritative sweepers. |
| 288 | +- **Sequential gates mask each other.** `Check Generated Artifacts` and |
| 289 | + `TypeScript Type Check` each run their gates in order and stop at the first |
| 290 | + failure. Regenerate everything up front; don't iterate one red build at a time. |
| 291 | +- **A green CI can mean a dormant gate.** `check-generated` runs behind a |
| 292 | + `paths` filter in `ci.yml`; a path missing from it makes the gate silent on |
| 293 | + exactly the PRs that break it (the reason the filter carries |
| 294 | + `packages/spec/src/**` wholesale). If you add a generated artifact or a new |
| 295 | + input to one, add its paths in the same PR. |
| 296 | +- **Editing only a conversion's `summary` still goes stale.** That string is |
| 297 | + copied verbatim into `spec-changes.json`'s `to` field and into the upgrade |
| 298 | + guide's table row, so a prose-only touch needs `gen:spec-changes` + |
| 299 | + `gen:upgrade-guide` like any other change. |
| 300 | +- **`--check` mode is the gate; bare mode rewrites.** `gen:*` fixes the file, |
| 301 | + `check:*` is the same script asserting it was committed. Never "fix" a |
| 302 | + `check:*` failure by editing the generated file by hand. |
0 commit comments