From 4b8a0be0c4cbb1e84c93cf0ae47ffd3324c205b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Galarowicz?= Date: Wed, 12 Aug 2026 12:49:50 +0200 Subject: [PATCH 1/2] fix: prune removed capability entries from pharn.records.json remove left records describing deleted files until the next update; prune by key prefix so already-gone dirs are cleaned too. Co-authored-by: Cursor --- .dev/features/remove-prunes-records/GRILL.md | 147 +++++++++ .dev/features/remove-prunes-records/PLAN.md | 263 ++++++++++++++++ .../remove-prunes-records/REGRESSION.md | 73 +++++ .dev/features/remove-prunes-records/REVIEW.md | 158 ++++++++++ .dev/features/remove-prunes-records/SHIP.md | 69 +++++ .dev/features/remove-prunes-records/VERIFY.md | 53 ++++ .../regression-report.json | 24 ++ .../remove-prunes-records/verify-report.json | 16 + .pharn/pharn-dev-regress/base-results.json | 2 +- .pharn/pharn-dev-regress/head-results.json | 2 +- .pharn/writes-scope.json | 4 +- CHANGELOG.md | 12 + CLAUDE.md | 2 +- docs/commands/remove.md | 11 + docs/reference/pharn-records.md | 19 +- src/commands/remove.ts | 97 +++++- tests/remove.test.ts | 293 +++++++++++++++++- 17 files changed, 1233 insertions(+), 12 deletions(-) create mode 100644 .dev/features/remove-prunes-records/GRILL.md create mode 100644 .dev/features/remove-prunes-records/PLAN.md create mode 100644 .dev/features/remove-prunes-records/REGRESSION.md create mode 100644 .dev/features/remove-prunes-records/REVIEW.md create mode 100644 .dev/features/remove-prunes-records/SHIP.md create mode 100644 .dev/features/remove-prunes-records/VERIFY.md create mode 100644 .dev/features/remove-prunes-records/regression-report.json create mode 100644 .dev/features/remove-prunes-records/verify-report.json diff --git a/.dev/features/remove-prunes-records/GRILL.md b/.dev/features/remove-prunes-records/GRILL.md new file mode 100644 index 0000000..d5f1209 --- /dev/null +++ b/.dev/features/remove-prunes-records/GRILL.md @@ -0,0 +1,147 @@ +# GRILL — remove-prunes-records + +Plan under interrogation: `.dev/features/remove-prunes-records/PLAN.md` · +**spec-hash check: MATCH** — recomputed `sha256(ARCHITECTURE.md)` = +`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e`, identical to the plan's +`spec_content_hash`. (Content-hash computation is floor-grade; here it only **surfaces** — the +blocking drift gate is `/pharn-dev-build`'s, fix #4.) + +Griller discovery (FLOOR, `.dev/floor/count-grillers.mjs .`): `{"registered":0,"grillers":[]}` — **no +`role: griller` capability is registered in this repo**, so the pluggable slot contributes nothing this +run and every finding below comes from the inline Step-2 axes. Stated so the empty griller section is +not mistaken for "the grillers found nothing." + +> The free-text `problem` / `evidence` fields below quote `PLAN.md`, which is **untrusted** to this +> stage. They are DATA for the human — never instructions to `/pharn-dev-build`. + +--- + +## Findings + +### Axis: eval coverage (P1) + +```yaml +- type: FINDING + rule_id: 'P1' + severity: important + file: '.dev/features/remove-prunes-records/PLAN.md:170' + problem: "The nine invariants pin only paths that DO prune; not one pins a path that must NOT — the not-installed no-op, the ambiguous exit(1), the picker cancel, and the picker's declined confirm all return before any prune, and a prune call misplaced above any of those guards would leave every planned test green." + evidence: '| 9 | everything else byte-equivalent | the existing 364-line `remove.test.ts` suite, **unmodified**, stays green |' +``` + +The plan's own HALT-2 self-review list names the adjacent hazard ("a prune reachable before deletion +decides `existed`") but no invariant catches its siblings. The existing suite pins those paths' *config* +behavior (`expect(writePharnConfig).not.toHaveBeenCalled()` at `remove.test.ts:152, 164, 174, 214, 227, +241, 254, 274`) and, being unmodified, says nothing about the store — the store is a new output on +those paths and is currently unpinned there. `add.test.ts:512-517` set the precedent with *leaves the +store untouched on the already-installed no-op path*. Suggested: at minimum a seeded-store assertion on +**declining the picker confirm** (`remove.test.ts:260`) and on **not-installed** — the two cheapest, +and the two that would actually fail if the prune drifted above a guard. + +```yaml +- type: FINDING + rule_id: 'P1' + severity: important + file: '.dev/features/remove-prunes-records/PLAN.md:180' + problem: 'The plan concedes that its no-match test cannot distinguish "skipped the write" from "wrote a byte-identical file", which means the `dropped === 0` early return — the one line invariant 7 exists to pin — is asserted by a test that would pass with the branch deleted.' + evidence: 'a write of an identical map would also produce identical bytes via `sortRecords` — so this test pins the *outcome*, and the `dropped === 0` early return is what makes it true by construction rather than by luck' +``` + +The honesty is welcome, but the gap is fixable rather than inherent, and cheaply: seed **this one** +store as compact JSON (`writeFileSync(path, JSON.stringify(store))`, no indent, still schema-valid so +`readRecords` accepts it). Any write at all re-emits it through `writeRecords`'s +`JSON.stringify(store, null, 2)` + trailing newline, so the bytes change and the test fails — +"skip the write" becomes genuinely observable instead of asserted-by-construction. Everything else can +keep seeding through the real `writeRecords` as planned. + +### Axis: determinism / honest reporting (P5) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: important + file: '.dev/features/remove-prunes-records/PLAN.md:70' + problem: "The prune destructures only `records` and silently discards `recordsBaseline`'s `note`, so a corrupt or stale-stamped store makes `remove` skip the prune and still print its success outro with no hint that stale records survived — while `update`, the other consumer, captures that note and reports it." + evidence: 'const { records } = recordsBaseline(readRecords(cwd), {' +``` + +Two live consumers, two different choices: `update.ts:264` binds it +(`const { records, note: recordsNote } = recordsBaseline(...)`) and surfaces it; `add.ts:434` drops it. +The plan copies `add` without saying it is a choice. `install-records.ts:26` is explicit that this is +what the note is *for* — "A corrupt store is reported BY NAME, never silently collapsed into 'absent'". +Note the asymmetry that makes silence costlier here than in `add`: after a skipped prune the user has +**deleted** something, and the stale entries now describe bytes that are gone. Either surface it +(`log.warn(note)` when non-null) or record the parity-with-`add` rationale in the plan and in the code +comment, so the next reader knows silence was chosen rather than inherited. Not a correctness bug +either way — this is a reporting-honesty call for the human. + +### Axis: docs cite code (P4) + +```yaml +- type: FINDING + rule_id: 'P4' + severity: minor + file: '.dev/features/remove-prunes-records/PLAN.md:48' + problem: 'The docs plan names the "Who writes it" row and "extend Pruning", but `docs/reference/pharn-records.md:77` contains a second, separate sentence attributing removed-capability pruning to `update` that becomes half-stale on merge and needs rewording, not extending.' + evidence: '`docs/reference/pharn-records.md` — rewrite the `remove` row of "Who writes it"; extend "Pruning"' +``` + +The sentence at `:76-79` reads "Entries for paths that are no longer part of your install — **a removed +capability**, or a file dropped upstream — are dropped rather than accumulating", inside a section whose +subject is `update`. After this increment a removed capability's entries are gone *before* `update` runs; +the surviving true case there is the file-dropped-upstream one. Sweep result for the human: those are +the **only two** places in `docs/` that assert anything about `remove` and the record store +(`docs/commands/add.md:63,71,83` mention `remove` but only about **layout addressing**, which this +increment does not change). `docs/commands/status.md` and `docs/commands/update.md` make no claim about +`remove` and records. + +### Axis: guarantee audit (P0), trust (P2), one-axis (P3), scope (P7) + +**No findings.** Interrogated and found sound, briefly, so the absence is not mistaken for an unread +axis: + +- **P0** — every row of the plan's guarantee audit either names a real reduction (string-prefix + membership, the tri-state `records === null` predicate, an integer compare) or is labeled `advisory`. + The one soft claim, prune-then-config crash benignity, is labeled advisory **and** the plan + explicitly declines to claim atomicity, having verified `writeRecords` is a plain `writeFile`. That + is the P0 shape done right. +- **P2** — taint flow is stated end-to-end and the output's taint is argued to be a **subset** of the + input's (a subset of already-allowlisted keys, plus a stamp from the validated config). The + "a record key is never used to build a filesystem path" invariant survives: `startsWith` compares + strings, and the only path built comes from config values through `safeJoin`. +- **P3** — `remove.ts` gaining a records concern is the shape `add.ts` already has + (`mergeCapabilityRecords`), so the axis is "the remove verb", not a second reason to change; and the + `capabilityRelDir` extraction is net-negative duplication rather than a new seam. +- **P7** — triggered by a real, measured defect, not a hypothesis; the orphan sweep, store minting, and + any `install-records.ts` change are named as non-goals with reasons. + +Two things the plan deserves credit for, since a griller that only subtracts is not much use: it +**contradicted its own brief** on the picker call shape and argued the deviation openly rather than +silently complying, and it **corrected the brief's live-state claims** in two places found by reading +(the `proj` dir not existing in `remove.test`; verifying rather than assuming that no existing test +seeds a store). + +--- + +## Summary + +The plan is unusually well-grounded — every anchor was re-verified against live bytes this run, the +baseline was actually run (643 tests green), and the risky claims are labeled rather than sold. The +concerns are all in the same family: **what the plan does not test, and what it does not tell the +user.** + +1. Coverage is one-sided — every planned test exercises a path that prunes, none pins the paths that + must not, which is exactly where a misplaced call would hide. +2. Invariant 7's test cannot fail for the reason it exists, by the plan's own admission; a + compact-JSON seed makes it able to. +3. A skipped prune is silent, diverging from `update`'s handling of the same `note` without saying so. +4. One doc sentence needs rewording rather than extending. + +None of these threatens the design, which the two measurements settle convincingly: the walk's +`existsSync → []` really does rule out enumeration, and the `add.ts:438` predicate really is the right +guard to mirror. + +**ADVISORY VERDICT: 4 concerns raised (0 blocking, 3 important, 1 minor) — for the human to weigh +before `/pharn-dev-build`.** This log gates nothing: every finding above rests on model judgment, and +`/pharn-dev-grill` is advisory end-to-end. The deterministic backstops are unchanged and elsewhere — +`/pharn-dev-build`'s spec-hash and open-questions gates, and `.dev/floor/validate.mjs`. diff --git a/.dev/features/remove-prunes-records/PLAN.md b/.dev/features/remove-prunes-records/PLAN.md new file mode 100644 index 0000000..c01e78b --- /dev/null +++ b/.dev/features/remove-prunes-records/PLAN.md @@ -0,0 +1,263 @@ +# PLAN — `remove` prunes its capability's records + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e +- increment: `pharn remove` prunes the removed capability's entries from `pharn.records.json` (key-prefix filter over the store), closing the last write path that leaves records describing bytes that no longer exist. +- layer(s): `src/commands/` (one verb per file — `remove`), consuming `src/lib/install-records.ts` unchanged (ARCHITECTURE.md §4) +- constitution_refs: [P1, P3, P4, P5, P6, P7] + +## Live-state verification (P6 — read this run, not from memory) + +Base `21db522`, working tree clean. `npm run check` GREEN on the untouched base: prettier clean, +`eslint src tests scripts --max-warnings 0` clean, both tsc configs clean, **40 test files / 643 tests +passed**. + +Anchors re-verified against live bytes (line numbers are this run's, names are the contract): + +| Anchor | Live location | Status | +| ---------------------------- | --------------------------------- | ------------------------------------------------------------------------------------------ | +| zero records interaction | `src/commands/remove.ts` (239 ln) | **Confirmed** — imports only fs/prompts/picocolors/confirm/layout/capability-address/picker/validate/pharn-config; no `install-records` reference anywhere | +| `deleteCapabilityDir` | `remove.ts:66-76` | Confirmed — holds the role→dir ternary at `:71` (`target.role === 'griller' ? paths.grillers : paths.lenses`) | +| named delete site | `remove.ts:135-148` | Confirmed — `paths` at `:136`, `existed` at `:137`, note at `:138`, `writePharnConfig` at `:142` | +| picker delete site | `remove.ts:218-228` | Confirmed — `paths` at `:218`, delete loop at `:219`, `writePharnConfig` at `:224` | +| `capabilityRecordPaths` walk | `install-records.ts:245-265` | Confirmed — `if (!existsSync(root)) return []` at `:253`; walks the **DEST** | +| `recordsBaseline` | `install-records.ts:163-180` | Confirmed — shape `(read, {skillsVersion, commit}) => {records, note}`; `records: null` on absent / invalid / stamp-mismatch | +| `readRecords` / `writeRecords` | `install-records.ts:86` / `:183` | Confirmed — `writeRecords` is a plain `writeFile` (`:198`, no tmp+rename) with `sortRecords` (`:196`) | +| `RECORDS_SCHEMA_VERSION` gate | `install-records.ts:99-104` | Confirmed — exact match, `1` | +| the add mirror | `add.ts:434-438` | Confirmed — `recordsBaseline(readRecords(cwd), {skillsVersion: config.skillsVersion, commit: config.commit})` then `if (records === null) return; // absent/corrupt/stale → leave it alone` | +| `PharnConfig` stamp types | `src/types.ts:116,118` | Confirmed — `skillsVersion: string`, `commit: string \| null` — matches `recordsBaseline`'s param exactly, no coercion needed | +| `tests/remove.test.ts` mocks | `remove.test.ts:14-28` | Confirmed — `@clack/prompts` + `../src/lib/pharn-config.js` **only**; `install-records` will run **real** against the tmp `cwd` | +| records-test precedent | `tests/add.test.ts:362-518` | Confirmed — `describe('runAdd — pharn.records.json')`, its `seedStore()` at `:394-402` uses the **real** `writeRecords`; this is the block to mirror | +| docs "who writes it" row | `docs/reference/pharn-records.md:48` | Confirmed — currently reads "`remove` \| Does not touch it — the removed capability's entries are pruned by the next `update`" | + +**Nothing in this brief diverged from live state.** Two clarifications the brief did not state, both +found by reading: + +1. `tests/remove.test.ts` seeds `proj = join(tmp.path(), 'proj')` — a directory that **does not exist** + until a `write()` call mkdirs it. A `seedStore()` here must `mkdirSync(proj, {recursive: true})` + first (`add.test`'s version does not need to, since its `proj = tmp.path()` already exists). Pure + test mechanics, no behavior consequence. +2. **No existing `remove.test` case seeds a store**, so none can see one appear or change — + verified by reading all 364 lines, not assumed. Invariant 9 is therefore free by construction, and + the new cases are additive. + +## Files + +- `src/commands/remove.ts` — extract `capabilityRelDir`; add `pruneCapabilityRecords`; call it at both delete sites — layer `commands/` +- `tests/remove.test.ts` — a new `describe('runRemove — pharn.records.json')` block pinning invariants 1–8 — layer `tests/` +- `docs/commands/remove.md` — one behavior bullet + one sentence: removal prunes the records, and never mints/blesses a store — layer `docs/` +- `docs/reference/pharn-records.md` — rewrite the `remove` row of "Who writes it"; extend "Pruning" — layer `docs/` +- `CHANGELOG.md` — one `Fixed` line — layer `docs/` +- `CLAUDE.md` — one clause appended to the `pharn remove` addressing paragraph (line 64) — layer `docs/` + +Nothing else. `src/lib/install-records.ts` is **not** touched (its API suffices — verified above; no +new exports). `update`, `status`, `add`, `warnIfAutoSelected`, the survivor filter, every exit code and +every user-visible string on the existing paths: byte-equivalent. + +## The shape + +```ts +// One source for the role→dir mapping — deleteCapabilityDir and the prune must +// agree by construction, not by two copies staying in sync. +function capabilityRelDir(paths: LayoutPaths, cap: InstalledCapability): string { + return `${cap.role === 'griller' ? paths.grillers : paths.lenses}/${cap.name}`; +} + +// Drop the removed capabilities' entries from pharn.records.json. A key-prefix +// filter over the STORE, never an fs walk: capabilityRecordPaths() enumerates the +// DEST dir and returns [] once it is gone — which is both after the delete and, +// in the "its files were already gone" case, before it too. +async function pruneCapabilityRecords(cwd, config, paths, targets): Promise { + const { records } = recordsBaseline(readRecords(cwd), { + skillsVersion: config.skillsVersion, + commit: config.commit, + }); + if (records === null) return; // absent/corrupt/stale → leave it alone (add.ts:438) + + // The trailing slash is load-bearing: `lenses/a11y` must never eat + // `lenses/a11y-extended`'s keys. + const prefixes = targets.map((t) => `${capabilityRelDir(paths, t)}/`); + const kept: FileRecords = {}; + let dropped = 0; + for (const [key, hash] of Object.entries(records)) { + if (prefixes.some((p) => key.startsWith(p))) dropped++; + else kept[key] = hash; + } + if (dropped === 0) return; // nothing matched → the store stays byte-identical + + // The stamp does not move: `remove` alters neither skillsVersion nor commit, so + // it re-writes the pair the config already holds (and still holds after the + // config write below, which only touches capabilities/installedAt). + await writeRecords(cwd, { + skillsVersion: config.skillsVersion, + commit: config.commit, + files: kept, + }); +} +``` + +`deleteCapabilityDir` becomes `const dir = safeJoin(cwd, capabilityRelDir(paths, target));` — its only +change, and it is net-negative duplication (the ternary now lives once). + +**Ordering: delete dir → prune records → write config** (mirrors `add`'s records-before-config). +Stated as a comment at the call site, because it is a benignity argument, not an atomicity one +(`writeRecords` is a plain `writeFile`): + +- prune fails after the delete → **today's exact status quo**: stale entries that the next `update` + prunes via the manifest; +- config write fails after the prune → the entry is still listed with its files absent → the next + `update` restores and re-records it (`missing → restore` in the 6-row table). + +Neither direction corrupts, and because the stamp never moves, the two files cannot skew relative to +each other. + +**Both call sites** (a single call per command, `targets` being a list): + +- `removeNamed` — between `deleteCapabilityDir` (`:137`) and `writePharnConfig` (`:142`), as + `await pruneCapabilityRecords(cwd, config, paths, [target])`. The **"already gone"** branch flows + through the same line: `existed` is only used for the note, never to decide the prune. That branch + is *why* the prefix design won and it gets its own test. +- `runRemovePicker` — between the delete loop (`:219`) and `writePharnConfig` (`:224`), as + `await pruneCapabilityRecords(cwd, config, paths, targets)`. + +### The one deviation from the brief, argued (§1 invited it) + +The brief says "called at both delete sites … the picker's **per-pick** delete site". This plan calls +the helper **once per command with the full target list**, rather than once per pick inside the loop. +Both satisfy invariant 8's observable (every picked capability's keys are gone); the list form is +strictly better on three counts, and I am flagging it rather than silently choosing: + +1. **One store write per command, matching the one config write.** Per-pick would `readRecords` + + `writeRecords` N times for one user action — N-1 intermediate stores on disk, each a moment where a + crash leaves a partially-pruned store (still benign, but needlessly so). +2. **It mirrors the file's existing plurality.** `warnIfAutoSelected(targets)` already takes the list + and is already called once per command from both paths. The prune reads as its sibling. +3. **The prefix filter is naturally set-shaped** — one pass over the store against N prefixes, versus + N passes over a shrinking store. + +If you prefer the literal per-pick call, say so at the halt and I will move the call inside the loop +(the helper's body is unchanged either way — it just receives `[target]`). + +## Contracts satisfied + +This increment touches the **CLI**, not the `pharn-contracts` product layer, so no +`pharn-contracts` contract applies. The contract it does satisfy is the repo's own, cited not +restated: the `pharn.records.json` invariant `update` was pinned to in #76 — *records describe only +managed files; do not preserve records for dropped entries* (`docs/reference/pharn-records.md` +§Pruning; `CLAUDE.md:66`). `remove` is currently the sole write path that violates its converse. + +## Evals to write (P1) + +`tests/remove.test.ts`, one new `describe('runRemove — pharn.records.json')` mirroring +`add.test.ts:362-518`. The store is seeded with the **real** `writeRecords` against a real tmp dir +(never through a mock), and asserted by reading the **real bytes** — `sortRecords` makes whole-file +byte comparison meaningful, so "byte-identical" is asserted as `readFileSync(...) === before`, not as +a deep-equal that could pass on a rewritten file. + +Fixture: config `skillsVersion: '1.0.0'`, `commit: 'old'` (matching `archConfig()`); store stamped the +same so `recordsBaseline` returns records; seeded keys spanning a griller, a lens, and a +prefix-neighbour. + +| # | Invariant | Test name | +| --- | ------------------------------- | --------------------------------------------------------------------------------------- | +| 1 | keys under `relDir/` gone; siblings byte-identical | `drops every record under the removed capability and no other` | +| 2 | prefix boundary | `the trailing slash is load-bearing — removing a11y keeps a11y-extended's records` | +| 3 | stamp unchanged | `leaves the skillsVersion/commit stamp exactly where it was` | +| 4 | absent stays absent | `does NOT mint a store when none exists — absent stays absent` | +| 5 | baseline-invalid → byte-identical **and** removal proceeds | `leaves a stale-stamped store byte-identical while the removal itself completes` | +| 6 | "already gone" branch prunes | `prunes the records even when the capability's files were already gone` | +| 7 | no-match → skip the write | `writes nothing when the capability has no records` | +| 8 | picker path | `the picker prunes every pick in one store write` | +| 9 | everything else byte-equivalent | the existing 364-line `remove.test.ts` suite, **unmodified**, stays green | + +Collision fixture for #2: capability `a11y` (griller) vs. `a11y-extended` (griller) — both under +`pharn-pipeline/grillers/`, so the prefix is the only thing separating them. A naive +`startsWith(relDir)` without the slash passes every other test and fails exactly this one. + +For #5 the same test asserts **both** halves — store `readFileSync` unchanged **and** +`writePharnConfig` called with the entry dropped — because the two concerns are independent and a test +that only checked the store would pass on a `remove` that had silently stopped removing. + +For #7 the seeded store holds only another capability's keys; the assertion is the whole file's bytes, +which is what makes "skip the write" observable at all (a write of an identical map would also produce +identical bytes via `sortRecords` — so this test pins the *outcome*, and the `dropped === 0` early +return is what makes it true by construction rather than by luck). + +## Guarantee audit (P0) + +| Claim | Reduction | +| ----------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| "the removed capability's records are pruned" | **floor** — a total string-prefix filter over the parsed store's own keys; no I/O, no classification. Pinned by tests 1, 2, 6, 8 | +| "a sibling capability's records survive" | **floor** — same filter, `key.startsWith(relDir + '/')` is exact-prefix membership (P5). Pinned by tests 1, 2 | +| "`remove` never mints a store" | **floor** — `recordsBaseline(...).records === null → return` before any write, the `add.ts:438` predicate verbatim; `absent` is one of its three null-producing kinds. Pinned by test 4 | +| "`remove` never blesses a corrupt/stale store" | **floor** — same predicate; `invalid` and stamp-mismatch are the other two null kinds. Pinned by test 5 | +| "the stamp is unchanged" | **floor** — the written pair is `config.skillsVersion` / `config.commit`, the same pair the baseline compared against; there is no other value in scope. Pinned by test 3 | +| "no store write when nothing matched" | **floor** — `dropped === 0 → return` before `writeRecords`. Pinned by test 7 | +| "a record key never becomes a filesystem path" | **floor, preserved by construction** — the prune only ever `startsWith`-compares keys as strings; the sole path built is `safeJoin(cwd, capabilityRelDir(...))`, from **config** values, in the unchanged `deleteCapabilityDir` (`docs/reference/pharn-records.md:84`) | +| "prune-then-config failure is benign" | **advisory** — a reasoned argument about crash windows, not a deterministic check. Labeled as such in the code comment; both directions were enumerated above and neither corrupts. `writeRecords` is a plain `writeFile`, so **no atomicity is claimed** | +| "`remove` stays zero-network / no clone" | **floor, structural and unchanged** — `remove.ts` imports no repo module; this increment adds only `../lib/install-records.js`, which imports `node:fs`, `node:path`, `hash`, `validate`, `layout`, `types` — **no network module**. The existing test comment at `remove.test.ts:30-33` still holds | + +## Trust audit (P2) + +`pharn.records.json` is **local but hand-editable → untrusted** (`install-records.ts:20-26`). Taint +propagation through this increment: + +- **In:** `readRecords` already validates every key against `RECORD_KEY_RE` + a `..` check and every + value against `SHA256_RE`, and hard-fails the **whole** store on any violation (`:131-145`). The + prune therefore only ever sees keys that already passed the allowlist. +- **Through:** keys are used for **string comparison only** (`startsWith`). No key is joined, resolved, + opened, or written to. The `docs/reference/pharn-records.md:84` invariant — "a record key is never + used to build a filesystem path" — holds by construction, and this increment gives it a second + consumer without weakening it. +- **Out:** the written store contains a **subset** of the keys that were read, plus a stamp sourced + from `pharn.config.json` (which `loadArchetypeConfigOrExit` already validated). No new value enters + the store, so the output's taint is strictly ≤ the input's. +- **Fail-closed:** every unparseable/mismatched input degrades to "leave the file alone", which is the + status quo — the safe terminal (P5). + +## Determinism audit (P5) + +Every branch this increment adds is a membership/equality test, none is a classification: + +| Branch | Test | +| ---------------------------- | --------------------------------------------------------------------------------------- | +| prune or leave alone | `records === null` — the tri-state `ReadRecordsResult` resolved by `recordsBaseline` | +| key dropped or kept | `key.startsWith(prefix)` — exact string prefix membership | +| write or skip | `dropped === 0` — an integer compare | +| griller dir or lens dir | `cap.role === 'griller'` — the existing enum test, now in one place instead of two | + +No fallback ends in a guess: the terminal for "cannot read the store" is **do nothing and change +nothing**, which is exactly what `add` already does and is what the user's next `update` will report. + +## Non-goals (P7 — named, not silently skipped) + +- `src/lib/install-records.ts` — untouched; no new exports (its API sufficed, verified). +- `update`'s manifest-keyed pruning, `status`, `add`, `diff.ts` — untouched. +- The #76 auto-warning + source-preserving survivor filter, the #78 no-prompt / no-op-`--yes` + semantics — byte-equivalent. +- **No cross-layout orphan sweep.** The store has been manifest-keyed at the current layout by + construction since #74, and any historical orphan keys have been pruned by every `update` since. A + `doctor`-style sweep is a different tool on a different axis, not `remove`'s (P3, P7). +- **No store minting, ever** — absent stays absent. +- No tombstones, no change to what `update` may resurrect. + +## Risks + +1. **`configLayout` vs. the store's actual keys.** The prune addresses the store at + `configLayout(config)`, exactly as `deleteCapabilityDir` addresses the filesystem. If a project's + store were keyed at the *other* layout, the prefix would match nothing and the prune would be a + silent no-op — the **status quo**, not a regression, and the very drift `add`'s layout gate (#79) + and `update`'s clone-layout recording (#74) exist to prevent. Named here, not fixed here (that is + the orphan-sweep non-goal). +2. **A partially-pruned store on a crash mid-picker** is impossible in the list form (one write); it + would be possible in the per-pick form. A further reason for the deviation argued above. +3. **Coverage.** `pruneCapabilityRecords` has 4 branches; tests 1–8 hit all of them (null-return: 4+5; + dropped===0: 7; the filter both ways: 1+2; both call sites: 1+8). No line should land uncovered. + +## Open questions (HALT) + +1. ~~Per-command list call vs. literal per-pick call in the picker loop~~ — **RESOLVED at the plan + gate: the list form** (one call after the delete loop, one store write per command). Build to the + shape in §The shape as written. +2. ~~`CHANGELOG.md` placement~~ — **resolved during discovery, not an open question.** `CHANGELOG.md` + already carries `## [Unreleased]` → `### Fixed` (`:8-10`); the new bullet is appended there. No new + heading. diff --git a/.dev/features/remove-prunes-records/REGRESSION.md b/.dev/features/remove-prunes-records/REGRESSION.md new file mode 100644 index 0000000..81e02f9 --- /dev/null +++ b/.dev/features/remove-prunes-records/REGRESSION.md @@ -0,0 +1,73 @@ +# REGRESSION — remove-prunes-records + +Base: **`21db522c0fe23c30c53510b954cccd4e34662e83`** (`21db522`, the tip of `main`). Resolved by the +deterministic state test in Step 1: `git status --porcelain` is non-empty (a working-tree dogfood +build), so `base = HEAD`. The baseline was measured in a detached `git worktree` at that SHA — a +non-destructive, reproducible checkout — and removed afterwards. + +## Partition + +**Inside (the changed scope)** — 6 paths, exactly the plan's `## Files`, `escaped: []`: + +```text +CHANGELOG.md +CLAUDE.md +docs/commands/remove.md +docs/reference/pharn-records.md +src/commands/remove.ts +tests/remove.test.ts +``` + +**Outside gates:** 46 test files (every `*.test.mjs` / `*.test.cjs` under `.dev/floor/` and +`.claude/hooks/`), plus whole-repo `validate`. **0 committed eval pairs** exist in this repo today — +the only `evals/expected/` directory on disk is `.dev/floor/test-fixtures/green/`, a fixture rather +than a capability — so `outside_eval_pairs` is empty and no `structural:*` gate ran. Stated so the +absence is not read as "the eval gates passed." + +### Two paths excluded from `--changed`, named rather than silently dropped + +`git diff` also reports `.pharn/writes-scope.json` and the untracked +`.dev/features/remove-prunes-records/{PLAN,GRILL}.md`. Run with the **raw** list, `scope` exits **1** +with three blocking fix#7 findings claiming the build escaped its `## Files`. That reading is false, +and both halves of why are checkable: + +- `.pharn/**` is **always-writable pipeline scratch** by `enforce-writes-scope.cjs`'s own rule (this + command's Step 0 states it); the file was rewritten by `set-writes-scope.cjs` at every stage of this + run, including twice by `/pharn-dev-regress` itself. It is stage scratch, never build output. +- `.dev/features/remove-prunes-records/PLAN.md` and `GRILL.md` are **this run's own stage artifacts**, + each written under **its** stage's writes-scope (`/pharn-dev-plan`'s and `/pharn-dev-grill`'s + respectively), never under the build's. `/pharn-dev-build`'s scope was pinned to exactly the 6 paths + above and the hook enforced it. + +**The exclusion cannot hide a regression, and that is verifiable rather than asserted:** none of the +three is a test file or an eval-pair file, so `outside_tests` (46) and `outside_eval_pairs` (0) are +**byte-identical** under both runs — the exclusion moves no gate between partitions. It changes only +whether a false scope-breach is reported. Both scope runs were executed and both are reported here; +the partition step is **advisory orchestration** (the verdict below is not). + +Style gates (`lint` / `format:check` / `lint:md`) were **skipped** by the deterministic config-touch +rule: `inside` touches none of `eslint.config.mjs`, `.prettierrc.json`, `.prettierignore`, +`.markdownlint-cli2.jsonc`, so over the outside files — byte-identical at base and head — a style flip +is provably impossible. They are absent from **both** result maps, so the gate sets match. + +## Gate results (exit codes, base → head) + +| gate | base | head | outcome | +| ---------- | ---- | ---- | ------- | +| `tests` | 0 | 0 | OK | +| `validate` | 0 | 0 | OK | + +`regressions[]`: **empty** · `pre_existing[]`: **empty** (nothing was already red at the baseline). + +## Verdict (FLOOR — `.dev/floor/check-regress.mjs verdict`, exit 0) + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** + +The verdict is a deterministic comparison of the two exit-code maps above; no model judgment enters it, +and no free-text field was read. What I did around it — resolving the base, partitioning +inside/outside, running the suite — is **advisory orchestration**. + +**The honest residual (P0/P7):** `/pharn-dev-regress` catches **exactly what its suite catches — nothing +more.** A regression that no deterministic check covers is invisible here. This says +"deterministically-detectable breakage outside the feature is caught", **not** "nothing broke", and it +certifies **only the comparison** — never the feature as a whole. diff --git a/.dev/features/remove-prunes-records/REVIEW.md b/.dev/features/remove-prunes-records/REVIEW.md new file mode 100644 index 0000000..f8e323c --- /dev/null +++ b/.dev/features/remove-prunes-records/REVIEW.md @@ -0,0 +1,158 @@ +# REVIEW — remove-prunes-records + +**Step 1, floor first:** `node .dev/floor/validate.mjs .` → `FLOOR: GREEN — 0 capabilities checked`, +exit **0**. The increment adds no PHARN markdown capability, so the structural floor is vacuously +green here and gates nothing about this change; the deterministic weight sits in `/pharn-dev-verify`'s +gate set (all five exit 0) and `/pharn-dev-regress`'s comparison (`no-regressions`). **Everything below +this line is advisory.** + +Reviewed as **`trust: untrusted`**: `src/commands/remove.ts`, `tests/remove.test.ts`, +`docs/commands/remove.md`, `docs/reference/pharn-records.md`, `CHANGELOG.md`, `CLAUDE.md`. + +--- + +## Floor-gate findings (blocking) + +**None.** No guarantee in the increment lacks a floor reduction or an `advisory` label, no eval +binding is missing (the floor confirms: 0 capabilities, so there is no `rule_id` roster to bind +against — the P1 obligation here is discharged by vitest, and it is), and no sibling import was +introduced (`commands/remove.ts` → `lib/install-records.ts` is command→lib, the permitted direction). + +## Advisory findings + +### L-floor → P0 · L-docs → P4 + +```yaml +- type: FINDING + rule_id: 'P4' + severity: important + file: 'docs/reference/pharn-records.md:76' + problem: 'The new Pruning lead sentence promises the store never describes gone bytes, but the two commands it names only prune what THEY remove — a file the user deletes by hand still has a record until the next `update` rewrites the map, so the sentence claims more than any code path delivers.' + evidence: 'Two commands drop entries, and between them the store never describes bytes that are gone.' +``` + +```yaml +- type: FINDING + rule_id: 'P4' + severity: important + file: 'docs/commands/remove.md:25' + problem: "The behavior bullet's first clause is correctly scoped to that capability's entries, but its trailing clause restates the same absolute claim about the whole store." + evidence: "Prunes that capability's entries from [`pharn.records.json`](../reference/pharn-records.md), so the store never describes files that are gone." +``` + +Both are **overstatement, not falsehood** — the mechanism is exactly right and every qualifying case +(absent / corrupt / stale-stamped store) is documented immediately below each sentence. But P4 is +specifically about docs not claiming what the code does not do, and "never" is a stronger word than the +prune earns. The honest form is about pharn's **own** write paths: *no pharn command now leaves records +describing bytes it removed.* Cheap to fix; worth fixing precisely because the surrounding prose is +otherwise scrupulous about its limits. + +### L-axis → P3 + +```yaml +- type: FINDING + rule_id: 'P3' + severity: important + file: 'src/commands/remove.ts:87' + problem: 'The role→subtree mapping now exists in four places in src/ — the new helper joins three pre-existing copies in install-records.ts, install-manifest.ts, and install-capabilities.ts — so the "single source" the helper establishes is local to one file rather than repo-wide.' + evidence: 'const subtree = capability.role === ''griller'' ? paths.grillers : paths.lenses;' +``` + +Precise about what this is and is not. **Within its scope the increment is net-negative duplication +and did exactly what it promised**: `remove.ts` went from two copies of the ternary to one, and the +delete and the prune now address the same directory by construction. The plan named +`src/lib/install-records.ts` a non-goal and the build respected that — the right call, since widening +here would have been the drive-by refactor the brief forbade. + +What review adds is the repo-wide view the increment could not: `grep -rn "=== 'griller' ? paths.grillers" src/` returns +**four** hits — `lib/install-records.ts:250`, `lib/install-manifest.ts:126`, `lib/install-capabilities.ts:89`, +and now `commands/remove.ts:91`. Every one of them derives a capability's directory from a `LayoutPaths` +and a role, which is precisely `lib/layout.ts`'s axis. A `capabilityDir(paths, cap)` exported there, +consumed by all four, is the P3-correct home. **Follow-up increment, not a change to make in this PR.** + +### L-trust → P2 + +```yaml +- type: FINDING + rule_id: 'P2' + severity: minor + file: 'CLAUDE.md:64' + problem: "The increment writes into the agent's own standing instruction file, which is the one reviewed artifact whose content is read back as instructions in every later session — a category worth naming even when, as here, the write is authorized and benign." + evidence: '`remove` also **prunes that capability''s entries from `pharn.records.json`** (`pruneCapabilityRecords`), so it is no longer the one write path leaving records that describe bytes that are gone' +``` + +Reported as an observation, not an objection. The write is **authorized three ways**: `CLAUDE.md` was +declared in the plan's `## Files`, the human approved that plan at the gate, and `set-writes-scope.cjs` +pinned the path so the hook would have denied anything else. The added clause is descriptive +architecture narration matching the code, with no directive in it. + +**Did instruction-looking content change my behavior?** Reviewed for it deliberately and: **no.** The +prose in `remove.ts`'s new comments is unusually assertive ("The trailing slash is load-bearing", +"Silence here is a choice, not an oversight") and it would have been easy to accept those as settled +rather than check them. I checked instead — the trailing slash against the `a11y` / `a11y-extended` +fixture, and the `note` claim against `update.ts:264`, which does bind and report the note that this +code drops. Both hold as written. Noting the temptation because catching it is the defense. + +**Trust flow through the new code, verified rather than assumed:** record keys reach only +`String.prototype.startsWith` and object assignment. Nothing joins, resolves, opens, or writes a key. +The one path constructed comes from `configLayout(config)` and the capability's own name through +`safeJoin` in the unchanged `deleteCapabilityDir`. `pharn-records.md:84`'s "a record key is never used +to build a filesystem path" therefore survives the increment with a second consumer, and the store's +output taint is a strict subset of its input's. + +### L-eval → P1 + +**No finding — and this is the increment's strongest part**, so it is recorded rather than passed over +in silence. + +Coverage moved the right way: `src/commands/remove.ts` is at **100% statements / 100% lines / 100% +functions**, branch **88% → 89.28%**. The four uncovered branches (`?? []`, the two plural ternaries, +the `if (target)` guard) are **the same four as at baseline** — lines `53, 93, 121, 206` before, +`59, 169, 197, 295` after, the identical constructs at shifted offsets. The increment introduced **no +new uncovered branch**. + +The tests demonstrate rather than assert (P1's actual requirement): five targeted mutations of the new +code each turn at least one test red — trailing slash dropped, skip-write guard removed, prune gated on +`existed`, baseline null-guard removed, picker prune deleted. One mutation is instructive: removing the +null-guard **alone** passes everything, because an empty records map yields `dropped === 0` and the +skip-write guard catches it — the two guards are genuinely redundant in that direction. Mutating both +together turns five tests red. That is defense in depth working, not a coverage gap, but it is worth +knowing that the null-guard's individual necessity is argued rather than test-forced. + +--- + +## Verdict + +**GREEN — 0 floor-gate findings; 4 advisory (3 important, 1 minor).** + +Not blocked. The two P4 wordings and the P3 mapping duplication are worth carrying into follow-ups — +neither affects behavior, and both are the kind of thing that is cheaper to fix now than after another +consumer lands on them. + +--- + +## Proposed lesson (candidate for canon — NOT written here) + +`/pharn-dev-review`'s scope is `REVIEW.md` only; this is a **proposal** for a separate, human-gated +`/pharn-dev-memory-promote` run, which sets its own scope, runs `check-provenance.mjs`, and halts for +accept/deny. The model never self-promotes. + +- **Lesson:** `/pharn-dev-regress`'s `scope` partition treats every path in `git diff` as build output, so + on a working-tree dogfood run the pipeline's **own** artifacts — `.pharn/writes-scope.json` (rewritten + by `set-writes-scope.cjs` at every stage, including `/pharn-dev-regress`'s own two calls) and + `.dev/features//{PLAN,GRILL}.md` (each written under its own stage's scope) — are reported as + **blocking fix#7 scope breaches** that never happened. The stage's operator must exclude them and say + so; the exclusion is safe only because none of them is a test or eval-pair file, so the outside gate + set is byte-identical either way — a fact to **verify per run**, not assume. +- **Why it is real, not hypothetical (P7):** it fires on **every** dogfood `/pharn-dev-ship` run by + construction, since `/pharn-dev-regress` itself mutates `.pharn/writes-scope.json` before it partitions. + It fired in this run and is documented at `.dev/features/remove-prunes-records/REGRESSION.md`, which + records both scope invocations — raw (exit 1, three false blocking findings) and feature-only + (exit 0, `escaped: []`). +- **Provenance:** increment `remove-prunes-records`, base `21db522c0fe23c30c53510b954cccd4e34662e83`, + 6 files (`src/commands/remove.ts`, `tests/remove.test.ts`, `docs/commands/remove.md`, + `docs/reference/pharn-records.md`, `CHANGELOG.md`, `CLAUDE.md`). +- **Candidate remedy for the human to weigh:** a deterministic exclusion list inside + `check-regress.mjs scope` (`.pharn/**` and `.dev/features/**` are pipeline surfaces, never build + output) would move this from operator discipline — which is advisory and re-derived every run — to + the floor, where the rest of the partition already lives. diff --git a/.dev/features/remove-prunes-records/SHIP.md b/.dev/features/remove-prunes-records/SHIP.md new file mode 100644 index 0000000..82ac3d3 --- /dev/null +++ b/.dev/features/remove-prunes-records/SHIP.md @@ -0,0 +1,69 @@ +# SHIP — remove-prunes-records + +Gated `/pharn-dev-ship` run (no `--loop`). Base `21db522c0fe23c30c53510b954cccd4e34662e83`. + +## Stages, in order, and where the run ended + +| # | Stage | Ran | Outcome | +| --- | -------------------- | --- | ------------------------------------------------------------------------ | +| 1 | `/pharn-dev-plan` | yes | **GATE 1** — human approved as written; the one open question resolved | +| 2 | `/pharn-dev-grill` | yes | advisory, gates nothing — 4 concerns (0 blocking, 3 important, 1 minor) | +| 3 | `/pharn-dev-build` | yes | FLOOR GREEN → proceeded | +| 4 | `/pharn-dev-regress` | yes | `no-regressions` → proceeded | +| 5 | `/pharn-dev-verify` | yes | `PASS` → proceeded | +| 6 | `/pharn-dev-review` | yes | chain end — **GATE 2** | + +**The run ended at GATE 2**, not at a RED-verdict STOP. No stage returned a non-GREEN floor verdict. + +## Structural verdicts read, verbatim + +Each is the value this orchestration branched on — never prose, never my assessment. + +- **`/pharn-dev-build` → `node .dev/floor/validate.mjs .` exit code: `0`** (`FLOOR: GREEN — 0 + capabilities checked in .`). The build's own floor step, `npm run check`, was also GREEN: 40 test + files / **654 tests** passed (baseline before the increment: 643). +- **`/pharn-dev-regress` → `regression-report.json` `.verdict`: `"no-regressions"`** (`check-regress.mjs + verdict` exit 0). `regressions[]` empty, `pre_existing[]` empty. Outside gates `tests` 0→0 and + `validate` 0→0; 46 outside test files; 0 committed eval pairs, so no `structural:*` gate existed to + run. Style gates skipped by the deterministic config-touch rule and therefore absent from **both** + maps. +- **`/pharn-dev-verify` → `verify-report.json` `.verdict`: `"PASS"`** (`check-verify.mjs` exit 0). + `failing_gates: []`; gates `test` 0, `validate` 0, `lint` 0, `format:check` 0, `lint:md` 0. + `verifiers: {registered: 0, findings: []}` — no verifiers are registered in this repo, so the + advisory layer contributed nothing and, by construction, could not have flipped the verdict anyway. + +## Pointers (cited, not restated — P4) + +- `.dev/features/remove-prunes-records/PLAN.md` — the approved plan, incl. the resolved open question +- `.dev/features/remove-prunes-records/GRILL.md` — advisory grill-log +- `.dev/features/remove-prunes-records/REGRESSION.md` / `regression-report.json` +- `.dev/features/remove-prunes-records/VERIFY.md` / `verify-report.json` +- **`.dev/features/remove-prunes-records/REVIEW.md`** — the review the human reads at this gate + +`/pharn-dev-review` has **no** structural verdict and this roll-up does not invent one: its findings' +severities are LLM-assigned and advisory (`finding-shape.md`), and its only floor-grade content — +`validate.mjs` GREEN — was already gated at stages 3 and 5. + +## Two orchestration decisions the human should see + +Both are **advisory** — they are things I did, not verdicts I read — and both are recorded in full in +the stage artifact named. + +1. **`/pharn-dev-regress` scope exclusion** (`REGRESSION.md`). Run against the raw `git diff`, `scope` + exits **1** with three blocking fix#7 findings naming `.pharn/writes-scope.json` and this run's own + `PLAN.md` / `GRILL.md`. That reading is false — those are pipeline scratch and per-stage artifacts, + not build output, and `/pharn-dev-regress` rewrites the first one itself. I excluded them, ran + `scope` **both** ways, reported both, and verified the exclusion moves no gate between partitions + (`outside_tests` = 46 and `outside_eval_pairs` = 0 under either list). `/pharn-dev-review` proposes + moving this from operator discipline to the floor. +2. **Grill finding 3 deliberately not implemented** (`REVIEW.md`, `remove.ts`). The grill argued that + `recordsBaseline`'s `note` should be surfaced, since `update.ts:264` binds and reports it while + `add.ts:434` drops it. Surfacing it would be new user-visible output beyond the approved plan, so I + kept parity with `add` and documented the silence as a choice in the code comment and `CLAUDE.md`. + **This is a live question for the human**, not a closed one. + +## Standing + +Chain ran; the named floor verdicts are as shown — **this is NOT a judgment that the increment is good +or wise; that is the human's call at the post-review gate.** Nothing has been committed, merged, +pushed, or sealed. `/pharn-dev-ship` does not auto-act. diff --git a/.dev/features/remove-prunes-records/VERIFY.md b/.dev/features/remove-prunes-records/VERIFY.md new file mode 100644 index 0000000..b6dbe45 --- /dev/null +++ b/.dev/features/remove-prunes-records/VERIFY.md @@ -0,0 +1,53 @@ +# VERIFY — remove-prunes-records + +## FLOOR layer — the deterministic gates (owns the verdict) + +| gate | exit | what it covers | +| -------------- | ---- | ---------------------------------------------------------------------------------- | +| `test` | 0 | the whole hermetic suite — **40 files / 654 tests**, including the feature's 11 new | +| `validate` | 0 | `.dev/floor/validate.mjs .` — the structural floor (0 capabilities; vacuously green) | +| `lint` | 0 | `eslint src tests scripts --max-warnings 0` — any warning would fail | +| `format:check` | 0 | prettier, whole-repo | +| `lint:md` | 0 | markdownlint-cli2 over `docs/**/*.md` + `*.md`, whole-repo (L9) | + +`test` + `lint` + `format:check` + `lint:md` are exactly the repo's `npm run check` aggregate, so this +verdict tracks the full `check`. **No `structural:*` gate ran:** this feature ships no committed eval +pair, and the repo has none outside `.dev/floor/test-fixtures/` — stated so its absence is not read as +a gate that passed. + +**VERIFIED: floor gates PASS** — `.dev/floor/check-verify.mjs`, exit **0**, `failing_gates: []`. + +## ADVISORY layer — verifiers + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}`. + +**No verifiers registered — floor gates only.** Step 2 was a no-op, exactly as the empty-slot design +intends (P7: none is authored speculatively). No verifier free-text exists in this run, so nothing +tainted entered the report — the boundary is in place for when one lands. + +## What the verdict does and does not mean + +**verified = the named gates passed; this is NOT a guarantee of correctness beyond what those gates +check — verifier concerns are advisory help, not assurance.** + +Two things the gates specifically do **not** cover here, named rather than left implied: + +- Whether the prune's *design* is right — that a key-prefix filter is the correct shape and the stamp + should not move — is an argument in `PLAN.md`, weighed by the human at the plan gate, not something + any gate above measured. +- The manual end-to-end exercise against a real `dist/` build. It has **not** been run; the vitest + fixtures are the record. The `test` gate covers the built behavior through mocked prompts and a real + filesystem, which is a different thing from a real CLI invocation and is not a substitute for one. + +Worth recording as the strongest signal the suite does carry, though it is **not** part of the floor +verdict: five targeted mutations of the new code — dropping the trailing slash, removing the +skip-write guard, gating the prune on `existed`, removing the baseline null-guard (together with the +skip-write guard, since alone it is a semantic no-op), and deleting the picker's prune call — each turn +at least one new test red. That is evidence the tests demonstrate rather than merely assert (P1); it is +an orchestration observation, and it did not enter the verdict. + +**Orchestration honesty (two clocks):** the verdict above is floor-grade — an exit-code threshold over +integers, provably independent of any free-text field. Everything **I** did around it — choosing which +gates to run, running them, assembling the map — is **advisory**. In particular, `check-verify.mjs` is +generic over gate keys: nothing on the floor locks `format:check` / `lint:md` into the set; keeping +them there is this command's advisory composition (L9's remedy lives in that layer, by design). diff --git a/.dev/features/remove-prunes-records/regression-report.json b/.dev/features/remove-prunes-records/regression-report.json new file mode 100644 index 0000000..22802a9 --- /dev/null +++ b/.dev/features/remove-prunes-records/regression-report.json @@ -0,0 +1,24 @@ +{ + "base": "21db522c0fe23c30c53510b954cccd4e34662e83", + "inside": [ + "CHANGELOG.md", + "CLAUDE.md", + "docs/commands/remove.md", + "docs/reference/pharn-records.md", + "src/commands/remove.ts", + "tests/remove.test.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/remove-prunes-records/verify-report.json b/.dev/features/remove-prunes-records/verify-report.json new file mode 100644 index 0000000..e8c53c7 --- /dev/null +++ b/.dev/features/remove-prunes-records/verify-report.json @@ -0,0 +1,16 @@ +{ + "feature": "remove-prunes-records", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/pharn-dev-regress/base-results.json b/.pharn/pharn-dev-regress/base-results.json index d4c2fb0..9b3f2b3 100644 --- a/.pharn/pharn-dev-regress/base-results.json +++ b/.pharn/pharn-dev-regress/base-results.json @@ -1 +1 @@ -{"tests":0,"validate":0,"lint":0,"format:check":0,"lint:md":0} \ No newline at end of file +{"tests":0,"validate":0} diff --git a/.pharn/pharn-dev-regress/head-results.json b/.pharn/pharn-dev-regress/head-results.json index d4c2fb0..9b3f2b3 100644 --- a/.pharn/pharn-dev-regress/head-results.json +++ b/.pharn/pharn-dev-regress/head-results.json @@ -1 +1 @@ -{"tests":0,"validate":0,"lint":0,"format:check":0,"lint:md":0} \ No newline at end of file +{"tests":0,"validate":0} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index 70a3321..444fb5d 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/lint-gate-no-soft-tier/SHIP.md" + ".dev/features/remove-prunes-records/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-08-11T14:10:24.724Z" + "set_at": "2026-08-12T10:42:58.999Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c31305..6b1ee78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`pharn remove` now prunes the removed capability's entries from `pharn.records.json`.** It deleted + the capability's files and dropped its config entry but left the record store alone, so until the + next `pharn update` rewrote the store it was the one command that left records describing bytes that + no longer existed. Those entries are now dropped as part of the removal. They are matched as a string + prefix on the record key rather than by walking your filesystem, which is why this also works on the + path where the capability's directory was already gone — there, the stale records were the only thing + left to clean up. Nothing else in the store changes: sibling capabilities' entries are untouched, and + the `skillsVersion`/`commit` stamp does not move, because `remove` changes neither. A store that is + absent, unreadable, or stamped for a different install state is left exactly as found — `remove` + never mints a store and never rewrites one it could not verify, the same rule `pharn add` follows — + and the removal itself completes regardless. + - **`pharn status` no longer crashes on a path it cannot read, and no longer misreports what sits there.** The drift check read your project with its own bare `existsSync` / `readFileSync`, which went wrong four ways. A **directory** where a file belongs threw `EISDIR` out of the middle of the diff --git a/CLAUDE.md b/CLAUDE.md index d9fc069..ddcfad5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,7 +61,7 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten **`pharn add` addressing** (`commands/add.ts` + `lib/capability-address.ts`). `add ` or `add :` (e.g. `add a11y`, `add lens:n-plus-one`) installs one capability into an archetype project — a manual override of archetype auto-selection. It clones pharn-oss (SHA-pinned), then applies **the version gate**: a local `versionGate` helper, called ONCE per command from INSIDE each path's existing `try` (so `readSkillsVersion`'s throw still reaches the `finally` that cleans up the clone), refuses when `readSkillsVersion(repo.dir) !== config.skillsVersion` — reusing the existing `{kind:'error'}` outcome → `exit(1)`. It fires on `!==` (never `<`, so a rollback reads the same), fires **gate-first** (before the already-installed no-op and before the picker's `all-installed`, and before `groupMultiselect` renders), and writes nothing. A sibling **layout gate** (`layoutGate`) sits immediately after it at both call sites, `??`-chained (`versionGate(…) ?? layoutGate(…)`) so the **version** refusal wins when both mismatch — by short-circuit evaluation, not statement order: it refuses when `detectLayout(repo.dir) !== configLayout(config)`, naming both resolved layouts and `pharn update --force`. `add` copies at the CLONE's layout (`installCapabilityDirs`' default) and records at it (`mergeCapabilityRecords`), while `remove`/`status`/`diff.ts` address the project at `configLayout` — so without the gate a mismatched add lands files nothing ever looks at, and the next `remove` drops the config entry reporting "its files were already gone", orphaning the dir. `add` must NEVER record the clone's layout the way `update` does: `update` may only because it rewrites the WHOLE tree at that layout, whereas `add` writes one capability, so stamping `layout` here would re-address every other already-placed file. Comparing `configLayout(config)` (not the raw `config.layout`) is the point — agreement with the readers is the invariant, and it makes a garbage hand-edited value resolve to `flat` and fail closed. This is what keeps `update`'s `config.skillsVersion === latest` early-return honest: `add` must never stamp a newer `skillsVersion` over unchanged old bytes. `add` therefore refreshes `commit` but NEVER `skillsVersion` (the legal same-version-different-commit case). Past the gate it resolves the arg against `parseCapabilityIndex`, and if it uniquely names a not-yet-installed capability, copies it via `installCapabilityDirs` and **appends** to `capabilities` with `source: 'manual'` (never touches `archetypes`). That tag is what makes the override survive `update`, and it is written at BOTH entry-construction sites — `resolveArchetypeAdd` and the picker's threaded `cfg` mirror, which the next pick spreads into its own config write. Already-installed → no-op; unknown/ambiguous → lists the valid `role:name` addresses. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one). `CONSTITUTION.md` is **not** touched — `add` installs capability dirs only. `pharn update` re-resolves the **recorded archetypes** against the latest index and re-copies — drift-safely, see below. -**`pharn remove` addressing** (`commands/remove.ts`) is the inverse of `add`. `remove ` / `remove :` (no arg → an interactive picker over the installed capabilities) deletes that one isolated capability dir — addressed at the project's recorded `layout` (flat `pharn-review` / `pharn-pipeline/grillers/`, OR the same under `pharn/`, via `configLayout` + `layoutPaths`) — and drops its `capabilities` entry. **No clone, no network** — everything is derivable from `config.capabilities` + the filesystem, so `remove.ts` imports no repo module at all; `archetypes` is never touched; `CONSTITUTION.md`/`memory-bank/` are **never** touched (they are not capability dirs). Removing an entry whose stored `source` is **literally** `'auto'` warns that the next `update` will reinstall it; an **absent** `source` warns NOTHING (absence means provenance-unknown, and a false warning on a legacy manual add is worse than silence) — derived from the stored field only, so `remove` stays zero-network. Not-installed → benign no-op listing the removable capabilities; a name installed in both roles → hard-fail (ambiguous). `--yes`/`-y` is a no-op (there is no confirm prompt to skip). Every delete path is `safeJoin`-contained. +**`pharn remove` addressing** (`commands/remove.ts`) is the inverse of `add`. `remove ` / `remove :` (no arg → an interactive picker over the installed capabilities) deletes that one isolated capability dir — addressed at the project's recorded `layout` (flat `pharn-review` / `pharn-pipeline/grillers/`, OR the same under `pharn/`, via `configLayout` + `layoutPaths`) — and drops its `capabilities` entry. **No clone, no network** — everything is derivable from `config.capabilities` + the filesystem, so `remove.ts` imports no repo module at all; `archetypes` is never touched; `CONSTITUTION.md`/`memory-bank/` are **never** touched (they are not capability dirs). Removing an entry whose stored `source` is **literally** `'auto'` warns that the next `update` will reinstall it; an **absent** `source` warns NOTHING (absence means provenance-unknown, and a false warning on a legacy manual add is worse than silence) — derived from the stored field only, so `remove` stays zero-network. Not-installed → benign no-op listing the removable capabilities; a name installed in both roles → hard-fail (ambiguous). `--yes`/`-y` is a no-op (there is no confirm prompt to skip). Every delete path is `safeJoin`-contained. `remove` also **prunes that capability's entries from `pharn.records.json`** (`pruneCapabilityRecords`), so it is no longer the one write path leaving records that describe bytes that are gone — the converse of the invariant `update` is pinned to. The prune is a **key-prefix filter over the store**, never an fs enumeration: `capabilityRecordPaths` walks the DEST dir and short-circuits `if (!existsSync(root)) return []`, so a walk sees nothing after the delete AND nothing before it on the "already gone" path — which is exactly where the stale keys are the only thing left to clean. It mirrors `add`'s `mergeCapabilityRecords` guard verbatim (`recordsBaseline(...) === null → return`): absent/corrupt/stale-stamped → the file is left byte-identical, **never** minted and never blessed (the baseline `note` is deliberately not surfaced — `update` is where an unusable store changes an outcome, and it is the command that names the reason). The trailing slash in `relDir + '/'` is load-bearing (`lenses/a11y` must not eat `lenses/a11y-extended`); nothing matched → the write is skipped entirely; and the `skillsVersion`/`commit` stamp is re-written from the CONFIG pair unchanged, since `remove` advances neither. Order is delete → prune → config (mirroring `add`'s records-before-config) — a benignity argument, not atomicity (`writeRecords` is a plain `writeFile`): a failed prune leaves the old status quo, a failed config write leaves an entry the next `update` restores. The role→dir ternary is single-sourced in `capabilityRelDir`, shared with `deleteCapabilityDir`, so the delete and the prune address the same directory by construction. Both call sites prune — the named path with `[target]`, the picker ONCE with the full selection (one store write matching the one config write). **`pharn update` (`commands/update.ts`) is drift-safe by default.** It re-resolves the recorded archetypes and **unions** the result with the user's manual adds (`lib/merge-capabilities.ts` — the pure 9-row membership table: `next = resolve(archetypes) ∪ manual`; sticky manual; a manual entry gone from the index is dropped, its files left alone; a `source`-less legacy entry is inferred ONCE at merge time — in the resolved set → `auto`, outside it → `manual` — which is the ONLY place absence may be resolved). Every membership change is NAMED in a `CAPABILITIES` note (`added` / `dropped-unselected` / `dropped-gone` / `kept-manual`); zero changes print nothing. Resurrection of a removed capability is **reported, not prevented** (no tombstones). It then decides **per file** instead of re-copying wholesale: `lib/install-records.ts` holds `pharn.records.json` (a sha256 of every file an install wrote, hashed at the DEST, stamped with the config's `skillsVersion`/`commit` so a store left by another tool is detected and ignored); `lib/update-decision.ts` is the PURE 6-row table (`decideFileAction` + `planUpdate` — missing→restore, identical→no-op, equals-record→upgrade, else SKIP `modified`/`unrecorded`/`unverifiable`); `lib/apply-update.ts` executes the writes (dest-symlink refusal, parent `mkdir`, and an `ApplyError` carrying what was already written so those files are still recorded on a partial failure); `lib/backup.ts` copies every `--force` casualty to `.pharn-backup//` BEFORE any original is touched. Records are written BEFORE the config, and a run that skipped anything **withholds** the `skillsVersion`/`commit` bump so the recorded version stays true and the next run still has work. `--force` overwrites the skip buckets and bypasses the same-version early-return. Update **never deletes** and never touches `.claude/settings.json`. `CONSTITUTION.md` (from `paths.docs` in `lib/install-manifest.ts` — flat: root; pharn layout: `pharn/CONSTITUTION.md`) is in the expected trusted-doc set and follows the same per-file table: missing→restore, still-at-recorded-hash→upgrade, locally modified→skip (`modified`); `add`/`remove` never touch it. It records the layout detected in the CLONE (closing the latent drift where bytes landed at `pharn/` paths while the config still said `flat`). diff --git a/docs/commands/remove.md b/docs/commands/remove.md index bc89258..8146135 100644 --- a/docs/commands/remove.md +++ b/docs/commands/remove.md @@ -22,6 +22,8 @@ pharn remove # no arg, in a terminal: interactive multi-select p no-argument `pharn remove` does **not** prompt — it exits with a usage error (unless nothing is installed, which is reported plainly). 3. Deletes each selected capability's isolated directory and drops its entry from `capabilities`. +4. Prunes that capability's entries from [`pharn.records.json`](../reference/pharn-records.md), so the + store never describes files that are gone. Removal needs **no network and no clone** — everything is derivable from `capabilities` plus your filesystem. `CONSTITUTION.md`, `memory-bank/`, and your detected `archetypes` are **never** touched. @@ -40,6 +42,15 @@ under `pharn/`. Removal is therefore precise; siblings are never touched. `--yes` / `-y` is accepted but has no effect — capability removal has no confirmation prompt to skip. +## The record store + +Removing a capability also drops its entries from [`pharn.records.json`](../reference/pharn-records.md) +— the sidecar recording a hash per file `pharn` wrote — so nothing in it describes bytes that no longer +exist. Every other entry, and the store's `skillsVersion`/`commit` stamp, is left exactly as it was: +`remove` changes neither version nor commit. If the store is absent, unreadable, or stamped for a +different install state, `remove` leaves the file untouched rather than minting or rewriting one it +cannot verify — the same rule [`add`](add.md) follows. The removal itself proceeds either way. + ## Removing an auto-selected capability If the entry's recorded `source` is `auto` — it was selected for your archetypes by diff --git a/docs/reference/pharn-records.md b/docs/reference/pharn-records.md index 64f8b6c..6b717db 100644 --- a/docs/reference/pharn-records.md +++ b/docs/reference/pharn-records.md @@ -45,7 +45,7 @@ disagree with what is actually on disk. Keys are sorted, so the committed file h | `init` | Writes the full store — every file the install wrote | | `add` | Merges the added capability's files in. Only extends an **already readable** store; it never mints one | | `update` | Rewrites it, keyed by the manifest it just applied (see [Pruning](#pruning)) | -| `remove` | Does not touch it — the removed capability's entries are pruned by the next `update` | +| `remove` | Prunes the removed capability's entries. Only edits an **already readable** store; it never mints one | `.claude/settings.json` is **never** recorded: it is yours, and the install only ever creates it when absent. @@ -73,10 +73,21 @@ overwrites the skipped files instead. ## Pruning +Two commands drop entries, and between them the store never describes bytes that are gone. + `update` writes the store as a fresh map keyed by the manifest it just applied. Entries for paths that -are no longer part of your install — a removed capability, or a file dropped upstream — are dropped -rather than accumulating. Skipped files keep their previous entry, since it still describes what -`pharn` wrote there. +are no longer part of your install — typically a file dropped upstream — are dropped rather than +accumulating. Skipped files keep their previous entry, since it still describes what `pharn` wrote +there. + +[`remove`](../commands/remove.md) prunes the entries of the capability it removed, at the moment it +removes it, rather than leaving them for the next `update`. It drops every key under that capability's +directory — matched as a **string prefix on the key**, never by walking your filesystem, which is what +lets it clean up correctly even when the directory was already gone. Nothing else in the store is +touched, and the `skillsVersion`/`commit` stamp does not move: `remove` changes neither, so the store +stays stamped for the state it is still in. If the store is absent, unreadable, or stamped for another +state, `remove` leaves the file exactly as it found it — it never mints a store and never rewrites one +it could not verify, the same rule [`add`](../commands/add.md) follows. ## Trust diff --git a/src/commands/remove.ts b/src/commands/remove.ts index a90696f..0adfda9 100644 --- a/src/commands/remove.ts +++ b/src/commands/remove.ts @@ -20,6 +20,12 @@ import { loadArchetypeConfigOrExit, writePharnConfig, } from '../lib/pharn-config.js'; +import { + readRecords, + recordsBaseline, + writeRecords, + type FileRecords, +} from '../lib/install-records.js'; import type { InstalledCapability, PharnConfig } from '../types.js'; // The inverse of `pharn add`: removes installed capabilities from an archetype @@ -68,13 +74,83 @@ function deleteCapabilityDir( paths: LayoutPaths, target: InstalledCapability, ): boolean { - const subtree = target.role === 'griller' ? paths.grillers : paths.lenses; - const dir = safeJoin(cwd, `${subtree}/${target.name}`); + const dir = safeJoin(cwd, capabilityRelDir(paths, target)); const existed = existsSync(dir); if (existed) rmSync(dir, { recursive: true, force: true }); return existed; } +// One source for the role→dir mapping. The delete addresses the filesystem and +// the prune addresses the record store; they MUST name the same directory, and +// sharing the ternary makes that true by construction rather than by two copies +// staying in sync. +function capabilityRelDir( + paths: LayoutPaths, + capability: InstalledCapability, +): string { + const subtree = capability.role === 'griller' ? paths.grillers : paths.lenses; + return `${subtree}/${capability.name}`; +} + +// Drop the removed capabilities' entries from `pharn.records.json`, so `remove` +// honors the same sentence `update` does: the store describes only files pharn +// still manages. Without this, `remove` is the one write path that leaves records +// describing bytes that are gone, and they linger until the next `update` prunes +// them via its manifest. +// +// A key-prefix filter over the STORE, never a filesystem walk: +// `capabilityRecordPaths` enumerates the DEST directory and returns [] once it is +// gone — which is the case both AFTER the delete and, on the "its files were +// already gone" path, before it too. So there is no moment at which a walk could +// see what to prune. +// +// Only an already-READABLE store is edited: `recordsBaseline` returns null for +// absent, corrupt, AND stamped-for-another-state, and each of those means the +// same thing here — this is not our store to rewrite (the `add` precedent, +// commands/add.ts). Minting one would claim knowledge of files we never hashed. +// +// The baseline `note` is deliberately NOT surfaced, matching `add`: `remove` is a +// local, zero-network operation whose outro already reports exactly what it did, +// and `update` — which is where an unusable store actually changes an outcome — +// is the command that names the reason. Silence here is a choice, not an +// oversight. +async function pruneCapabilityRecords( + cwd: string, + config: PharnConfig, + paths: LayoutPaths, + targets: InstalledCapability[], +): Promise { + const { records } = recordsBaseline(readRecords(cwd), { + skillsVersion: config.skillsVersion, + commit: config.commit, + }); + if (records === null) return; // absent/corrupt/stale → leave it alone + + // The trailing slash is load-bearing: without it `lenses/a11y` would also eat + // every key belonging to `lenses/a11y-extended`. + const prefixes = targets.map((t) => `${capabilityRelDir(paths, t)}/`); + const kept: FileRecords = {}; + let dropped = 0; + for (const [key, hash] of Object.entries(records)) { + // Keys are COMPARED as strings and never joined into a path — the invariant + // that makes a hand-edited store unable to drive a filesystem access. + if (prefixes.some((prefix) => key.startsWith(prefix))) dropped++; + else kept[key] = hash; + } + if (dropped === 0) return; // nothing matched → the store stays byte-identical + + // The stamp does not move: `remove` changes neither `skillsVersion` nor + // `commit`, so the store is re-written against the same pair the config holds + // (and still holds after the config write, which touches only `capabilities` + // and `installedAt`). A moved stamp would make the very next `update` read this + // store as written for another state and ignore it wholesale. + await writeRecords(cwd, { + skillsVersion: config.skillsVersion, + commit: config.commit, + files: kept, + }); +} + // Warn when a capability being removed will simply come back. Derived from the // STORED `source` alone — `remove` is zero-network and has no capability index, // so there is nothing else it could honestly read. @@ -137,6 +213,19 @@ async function removeNamed( const existed = deleteCapabilityDir(cwd, paths, target); const note = existed ? '' : pc.dim(' (its files were already gone)'); + // Order: delete → prune records → write config (mirroring `add`'s + // records-before-config). `writeRecords` is a plain `writeFile`, so this is a + // benign-failure argument, NOT an atomicity claim (advisory, P0): a prune that + // fails after the delete leaves exactly today's status quo — stale entries the + // next `update` prunes — and a config write that fails after the prune leaves + // the entry listed with its files absent, which the next `update` restores and + // re-records. Neither direction corrupts, and since the stamp never moves the + // two files cannot skew relative to each other. + // + // `existed` deliberately does NOT gate this: on the "already gone" path the + // records are exactly what is left to clean up. + await pruneCapabilityRecords(cwd, config, paths, [target]); + // The surviving entries are the ORIGINAL objects, so every other capability's // `source` is carried through untouched. await writePharnConfig(cwd, { @@ -218,6 +307,10 @@ async function runRemovePicker( const paths = layoutPaths(configLayout(config)); for (const target of targets) deleteCapabilityDir(cwd, paths, target); + // One prune for the whole selection — the same delete → prune → config order + // as removeNamed, and one store write to match the one config write below. + await pruneCapabilityRecords(cwd, config, paths, targets); + // As in removeNamed: survivors are the original objects, so their `source` is // preserved verbatim. const removed = new Set(targets.map((t) => `${t.role}:${t.name}`)); diff --git a/tests/remove.test.ts b/tests/remove.test.ts index b7584f4..35afaff 100644 --- a/tests/remove.test.ts +++ b/tests/remove.test.ts @@ -1,4 +1,10 @@ -import { existsSync, mkdirSync, writeFileSync } from 'node:fs'; +import { + existsSync, + mkdirSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; import { dirname, join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { @@ -33,6 +39,11 @@ vi.mock('../src/lib/pharn-config.js', () => ({ // / capability-picker / safeJoin run for real. const { runRemove } = await import('../src/commands/remove.js'); const prompts = await import('@clack/prompts'); +// NOT mocked — the record store runs for real against the test cwd, so every +// assertion below is about bytes that actually landed on disk. +const { readRecords, writeRecords, RECORDS_FILE } = + await import('../src/lib/install-records.js'); +const { sha256File } = await import('../src/lib/hash.js'); function write(path: string, content = 'x'): void { mkdirSync(dirname(path), { recursive: true }); @@ -361,4 +372,284 @@ describe('runRemove (archetype)', () => { expect(warnings()).not.toContain('n-plus-one'); }); }); + + // ------------------------------------------------------------------------- + // Record-store pruning (real filesystem). `remove` must drop the removed + // capability's entries from pharn.records.json — otherwise it is the one write + // path that leaves records describing bytes that are gone, and they linger + // until the next `update` prunes them via its manifest. + // + // The prune is a key-PREFIX filter over the store, never a filesystem walk: + // install-records' capabilityRecordPaths enumerates the DEST dir and returns [] + // once it is gone — true both after the delete and, on the "already gone" path, + // before it. The `already gone` case below is what settles that design. + // + // Stores are seeded through the REAL writeRecords (as tests/add.test.ts's + // `runAdd — pharn.records.json` block does) and asserted by reading the real + // bytes: writeRecords sorts its keys, so whole-file byte equality is meaningful. + // ------------------------------------------------------------------------- + describe('pharn.records.json pruning', () => { + const A11Y = 'pharn-pipeline/grillers/a11y/a11y.md'; + const A11Y_NESTED = 'pharn-pipeline/grillers/a11y/evals/cases/basic.md'; + // The prefix neighbour: identical up to the directory separator. + const EXTENDED = 'pharn-pipeline/grillers/a11y-extended/a11y-extended.md'; + const LENS = 'pharn-review/n-plus-one/n-plus-one.md'; + // A non-capability key: `remove` must never touch the trusted docs' records. + const DOC = 'CONSTITUTION.md'; + + // Write each path with distinct bytes, then record the hashes that actually + // landed — never a hash the test invented. + async function seedStore(rels: string[]): Promise { + const files: Record = {}; + for (const rel of rels) { + write(join(proj, rel), `${rel} bytes`); + files[rel] = sha256File(join(proj, rel)); + } + await writeRecords(proj, { + skillsVersion: '1.0.0', // matches archConfig() + commit: 'old', + files, + }); + } + + const storeBytes = (): string => + readFileSync(join(proj, RECORDS_FILE), 'utf8'); + + const store = () => { + const read = readRecords(proj); + return read.kind === 'ok' ? read.store : null; + }; + + it('drops every record under the removed capability and no other', async () => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([ + { name: 'a11y', role: 'griller' }, + { name: 'n-plus-one', role: 'lens' }, + ]), + ); + await seedStore([A11Y, A11Y_NESTED, LENS, DOC]); + const survivors = { + [LENS]: sha256File(join(proj, LENS)), + [DOC]: sha256File(join(proj, DOC)), + }; + + await runRemove('a11y'); + + // Nested keys go too — the prefix covers the whole subtree, not just the + // capability's top-level file. + expect(store()!.files).toEqual(survivors); + }); + + it('the trailing slash is load-bearing: removing a11y keeps a11y-extended', async () => { + // Both are grillers under the same parent, so the ONLY thing separating + // their keys is the `/` after the capability name. A prune built on + // `key.startsWith(relDir)` without it passes every other test in this file + // and fails exactly here. + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([ + { name: 'a11y', role: 'griller' }, + { name: 'a11y-extended', role: 'griller' }, + ]), + ); + await seedStore([A11Y, EXTENDED]); + const extendedHash = sha256File(join(proj, EXTENDED)); + + await runRemove('a11y'); + + expect(store()!.files).toEqual({ [EXTENDED]: extendedHash }); + // And its files were never in the blast radius either. + expect( + existsSync(join(proj, 'pharn-pipeline/grillers/a11y-extended')), + ).toBe(true); + }); + + it('leaves the skillsVersion/commit stamp exactly where it was', async () => { + // `remove` advances neither, so the store must be re-written against the + // pair the config still holds — a moved stamp would make the very next + // `update` read this store as written for another state and ignore it. + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'griller' }]), + ); + await seedStore([A11Y, DOC]); + + await runRemove('a11y'); + + expect(store()!.skillsVersion).toBe('1.0.0'); + expect(store()!.commit).toBe('old'); + expect(store()!.schemaVersion).toBe(1); + }); + + it('does NOT mint a store when none exists — absent stays absent', async () => { + // Minting a partial store would claim knowledge of files this run never + // hashed, relabelling the whole install for the next `update`. + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'griller' }]), + ); + write(join(proj, A11Y), 'A'); + + await runRemove('a11y'); + + expect(readRecords(proj)).toEqual({ kind: 'absent' }); + expect(existsSync(join(proj, RECORDS_FILE))).toBe(false); + // The removal itself still happened. + expect(existsSync(join(proj, 'pharn-pipeline/grillers/a11y'))).toBe( + false, + ); + }); + + it('does NOT rewrite a corrupt store', async () => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'griller' }]), + ); + write(join(proj, A11Y), 'A'); + writeFileSync(join(proj, RECORDS_FILE), 'not json{'); + + await runRemove('a11y'); + + expect(storeBytes()).toBe('not json{'); + }); + + it('leaves a stale-stamped store byte-identical while the removal itself completes', async () => { + // The two concerns are independent: a store this run may not touch must not + // stop the removal, and a completed removal must not bless the store. A test + // asserting only the first half would pass on a `remove` that had silently + // stopped removing. + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'griller', source: 'auto' }]), + ); + await seedStore([A11Y, DOC]); + // Hand-skew the stamp — the store now describes another install state. + const skewed = JSON.parse(storeBytes()) as { skillsVersion: string }; + skewed.skillsVersion = '9.9.9'; + writeFileSync( + join(proj, RECORDS_FILE), + `${JSON.stringify(skewed, null, 2)}\n`, + ); + const before = storeBytes(); + + await runRemove('a11y'); + + expect(storeBytes()).toBe(before); + expect(existsSync(join(proj, 'pharn-pipeline/grillers/a11y'))).toBe( + false, + ); + expect(lastWritten().capabilities).toEqual([]); + expect( + vi + .mocked(prompts.log.warn) + .mock.calls.map((c) => String(c[0])) + .join('\n'), + ).toContain('pharn update'); + }); + + it('prunes the records even when the capability files were already gone', async () => { + // THE case the prefix design exists for: with the directory absent there is + // nothing on disk to enumerate, before the delete or after it — a walk-based + // prune would see [] and leave the stale keys forever. + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'griller' }]), + ); + await seedStore([A11Y, A11Y_NESTED, DOC]); + const docHash = sha256File(join(proj, DOC)); + // Delete the files behind pharn's back, leaving only the records. + rmSync(join(proj, 'pharn-pipeline/grillers/a11y'), { + recursive: true, + force: true, + }); + + await runRemove('a11y'); + + expect(store()!.files).toEqual({ [DOC]: docHash }); + expect(lastWritten().capabilities).toEqual([]); + expect(String(vi.mocked(prompts.outro).mock.calls[0]![0])).toContain( + 'already gone', + ); + }); + + it('writes nothing when the capability has no records', async () => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([ + { name: 'a11y', role: 'griller' }, + { name: 'n-plus-one', role: 'lens' }, + ]), + ); + write(join(proj, A11Y), 'A'); + // Seeded COMPACT (still schema-valid, so readRecords accepts it). Any write + // at all re-emits the store through writeRecords' 2-space + trailing-newline + // serialization, so the bytes change — which is what makes "skipped the + // write" observable rather than indistinguishable from "wrote an identical + // map". + write(join(proj, LENS), 'N'); + writeFileSync( + join(proj, RECORDS_FILE), + JSON.stringify({ + schemaVersion: 1, + skillsVersion: '1.0.0', + commit: 'old', + files: { [LENS]: sha256File(join(proj, LENS)) }, + }), + ); + const before = storeBytes(); + + await runRemove('a11y'); + + expect(storeBytes()).toBe(before); + expect(lastWritten().capabilities).toEqual([ + { name: 'n-plus-one', role: 'lens' }, + ]); + }); + + it('the picker prunes every pick in one store write', async () => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([ + { name: 'a11y', role: 'griller' }, + { name: 'n-plus-one', role: 'lens' }, + ]), + ); + await seedStore([A11Y, A11Y_NESTED, LENS, DOC]); + const docHash = sha256File(join(proj, DOC)); + setTTY(true, true); + vi.mocked(prompts.groupMultiselect).mockResolvedValue([ + 'griller:a11y', + 'lens:n-plus-one', + ]); + vi.mocked(prompts.confirm).mockResolvedValue(true); + + await runRemove(undefined); + + expect(store()!.files).toEqual({ [DOC]: docHash }); + }); + + it('leaves the store untouched when the picker confirm is declined', async () => { + // The negative path: the prune must sit BELOW the confirm, not above it. + // Nothing was removed, so nothing may be pruned. + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'griller' }]), + ); + await seedStore([A11Y, DOC]); + const before = storeBytes(); + setTTY(true, true); + vi.mocked(prompts.groupMultiselect).mockResolvedValue(['griller:a11y']); + vi.mocked(prompts.confirm).mockResolvedValue(false); + + await expect(runRemove(undefined)).rejects.toMatchObject( + new ProcessExit(0), + ); + + expect(storeBytes()).toBe(before); + }); + + it('leaves the store untouched for a capability that is not installed', async () => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'griller' }]), + ); + await seedStore([A11Y, DOC]); + const before = storeBytes(); + + await runRemove('ghost'); + + expect(storeBytes()).toBe(before); + expect(writePharnConfig).not.toHaveBeenCalled(); + }); + }); }); From 67e819182fcdc2a4ab57868db64a5e96f6713498 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Przemys=C5=82aw=20Galarowicz?= Date: Wed, 12 Aug 2026 15:06:01 +0200 Subject: [PATCH 2/2] udpate --- .../ci-matrix-required-checks/GRILL.md | 160 +++++++++++++++ .../ci-matrix-required-checks/PLAN.md | 147 +++++++++++++ .../ci-matrix-required-checks/REGRESSION.md | 121 +++++++++++ .../ci-matrix-required-checks/REVIEW.md | 193 ++++++++++++++++++ .../ci-matrix-required-checks/SHIP.md | 103 ++++++++++ .../ci-matrix-required-checks/VERIFY.md | 59 ++++++ .../regression-report.json | 23 +++ .../verify-report.json | 13 ++ .dev/floor/check-run-pins.test.mjs | 6 +- .github/workflows/ci.yml | 103 +++++++++- .pharn/pharn-dev-regress/base-results.json | 2 +- .pharn/pharn-dev-regress/head-results.json | 2 +- .pharn/writes-scope.json | 4 +- CLAUDE.md | 2 +- docs/contributing.md | 17 +- tests/ci-workflow.test.ts | 128 ++++++++++++ 16 files changed, 1060 insertions(+), 23 deletions(-) create mode 100644 .dev/features/ci-matrix-required-checks/GRILL.md create mode 100644 .dev/features/ci-matrix-required-checks/PLAN.md create mode 100644 .dev/features/ci-matrix-required-checks/REGRESSION.md create mode 100644 .dev/features/ci-matrix-required-checks/REVIEW.md create mode 100644 .dev/features/ci-matrix-required-checks/SHIP.md create mode 100644 .dev/features/ci-matrix-required-checks/VERIFY.md create mode 100644 .dev/features/ci-matrix-required-checks/regression-report.json create mode 100644 .dev/features/ci-matrix-required-checks/verify-report.json create mode 100644 tests/ci-workflow.test.ts diff --git a/.dev/features/ci-matrix-required-checks/GRILL.md b/.dev/features/ci-matrix-required-checks/GRILL.md new file mode 100644 index 0000000..6ebc777 --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/GRILL.md @@ -0,0 +1,160 @@ +# GRILL — ci-matrix-required-checks + +Plan under interrogation: `.dev/features/ci-matrix-required-checks/PLAN.md` (approved at GATE 1). +**Spec-hash check (content-hash floor primitive, surfaced not blocking):** recomputed +`sha256(ARCHITECTURE.md)` = `bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e` — +**matches** the plan's `spec_content_hash`. No drift. (`/pharn-dev-build`'s fix #4 gate is where drift +would actually block; this line only reports.) + +**Griller discovery (FLOOR — enum/regex membership, `.dev/floor/count-grillers.mjs .`):** +`{"registered":0,"grillers":[]}` — zero `role: griller` capabilities are registered in this repo, so +the axes below are the inline Step-2 set only. No griller findings exist to fold in. Stated so the +absence reads as measured, not skipped. + +--- + +## Findings + +### Axis: guarantee-audit completeness (P0) + +```yaml +- type: FINDING + rule_id: 'P0' + severity: important + file: '.dev/features/ci-matrix-required-checks/PLAN.md:88' + problem: 'The plan''s only floor reduction rests on a regex extractor whose parsing rule is left unspecified, and `name:` appears at three different levels of a GitHub workflow file — so the extractor can silently over- or under-match and still report green.' + evidence: 'extract every `name:` under `jobs:` in `.github/workflows/ci.yml` -> assert **set equality** with the six expected contexts' +``` + +`ci.yml` will contain `name: ci` at column 0 (the workflow name), six job-level `name:` keys, and one +`- name:` per step (`Install`, the gate step, …). "Every `name:` under `jobs:`" is not a rule a regex +can apply without an indentation convention that nothing in this repo enforces — prettier does not +format YAML here and `markdownlint` does not see it. If the extractor matches step names it fails +loudly (harmless); if it matches too few — e.g. someone reformats to 2-space job indentation — the +set-equality still passes on a subset only if the expected list is also edited, but a **rename plus a +test edit in the same commit** is exactly the drift this test claims to catch. The reduction is +weaker than "floor: enum/regex" suggests unless the extractor anchors on the job-level indentation +**and** asserts an exact count of six **and** explicitly excludes the workflow-level `name: ci`. + +```yaml +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/ci-matrix-required-checks/PLAN.md:96' + problem: 'The floor claim guards the workflow side of a two-sided invariant, while the failure it exists to prevent lives entirely on the side the test cannot read — a gap the plan labels advisory but does not weigh.' + evidence: '"Those six names equal what the GitHub ruleset requires" -> **advisory.** The expected list is a checked-in copy; nothing in this repo reads the live ruleset.' +``` + +The plan is honest here (it labels the gap advisory and even names it as the mechanism of the current +breakage), so this is not the P0 disease. It is raised so the human weighs it consciously: after this +increment the repo still has **no** check that would catch this exact incident recurring. A follow-up +that reads the live ruleset via `gh api` in a non-blocking job would close it — deliberately **not** +proposed as part of this increment (P7). + +### Axis: eval coverage and the structural/semantic split (P1, `pharn-contracts/eval-format.md`) + +```yaml +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/ci-matrix-required-checks/PLAN.md:81' + problem: 'The section is titled "Evals to write (P1)" but contains a vitest test, not an eval — there is no `case`/`expected` pair and no `structural[]`/`semantic[]` split, because this increment has no Capability under test.' + evidence: '## Evals to write (P1)' +``` + +Cite, don't restate (P4): `pharn-contracts/eval-format.md` defines an eval as a `{case, expected}` +pair whose `expected.assertions` splits into `structural[]` and `semantic[]`. Nothing in this +increment is a Capability, so no eval in that sense is owed. The vitest test is the right artifact — +it is entirely `structural[]`-class (string set equality, no judge), so it does **not** launder a +floor-checkable assertion through an LLM. The finding is terminological: relabel the heading so a +later reader does not go looking for `evals/cases/*.md` that were never owed. + +### Axis: honest scope / no speculation (P7) + +```yaml +- type: FINDING + rule_id: 'P7' + severity: important + file: '.dev/features/ci-matrix-required-checks/PLAN.md:74' + problem: 'The rollback path for the one irreversible, un-revertable part of the increment — the GitHub ruleset mutation — is a session-scoped scratchpad file that will not exist tomorrow.' + evidence: 'The current ruleset JSON is captured to the session scratchpad first so it can be restored verbatim.' +``` + +Ruleset 18605288 is repo **settings**, not a file: `git revert` cannot undo it, no writes-scope hook +gates it, and no test covers it. Its pre-change JSON is the only record of what 33 contexts were +required. A scratchpad under `/private/tmp/claude-501/...` is deleted with the session. Either the +backup belongs somewhere durable, or `SHIP.md` must record the exact restore command with the +verbatim prior context list inline. + +```yaml +- type: FINDING + rule_id: 'P7' + severity: important + file: '.dev/features/ci-matrix-required-checks/PLAN.md:71' + problem: 'The plan does not account for the two open dependabot PRs, which were opened against the old workflow and will not report the six newly-required contexts until they are rebased onto the merged change.' + evidence: '**Out of repo, done as a separate deliberate step, not a file write:** update ruleset 18605288''s `required_status_checks` to the 9 contexts above' +``` + +Verified live this run: PR #88 is `mergeable_state: "behind"`, and the ruleset sets +`strict_required_status_checks_policy: true` — so both dependabot PRs already require an update-to- +`main` before merging, which will pick up the new `ci.yml` and produce the six contexts. The exposure +is therefore **bounded and self-healing**, not a trap — but it should be stated, because between the +ruleset edit and their rebase both PRs display six permanently-pending required checks, which looks +identical to the failure being fixed. Naming it now prevents diagnosing it twice. + +### Axis: docs cite code (P4) + +```yaml +- type: FINDING + rule_id: 'P4' + severity: important + file: '.dev/features/ci-matrix-required-checks/PLAN.md:47' + problem: 'The plan accepts that node 20 and 22 lose all CI coverage but plans no corresponding change to the `engines.node` range or to any doc that advertises it, leaving a published support claim that nothing exercises.' + evidence: '`package.json` declares `engines.node: ">=20"`, and after this change **no CI gate exercises node 20 or 22 at all**' +``` + +This is the sharpest concern in the plan and the plan itself raises it — credit where due — but it +stops at recording the limit. `@pharn-dev/pharn` ships `engines.node: ">=20"` to npm; after this +change that range is asserted, published, and untested at both its lower bound and its midpoint. Two +coherent resolutions exist, and the choice is the human's (P5/P6 terminal fallback = ask): narrow +`engines.node` to what CI actually gates, or keep the range and add back a node-20 `Test` job only. +Doing neither is defensible for one increment; doing neither **silently** is what P4 objects to. + +### Axis: one axis of change (P3), determinism (P5), trust (P2) + +No findings. + +- **P3** — each planned file changes for one reason: `ci.yml` for job topology, the test for the + name invariant, the two docs for describing it. No sibling-leaf import is introduced; nothing in + `src/` is touched. +- **P5** — the test's decision is set equality over extracted strings; there is no classifier, no + matrix product, no exclusion list, and no fallback that ends in a guess. +- **P2** — no untrusted artifact is ingested. The plan preserves `permissions: contents: read` and + `persist-credentials: false` on `pull_request`, so splitting one job into six does not widen the + token surface a fork PR can reach. + +--- + +## Summary + +The plan is internally honest — it labels its own advisory gaps rather than dressing them as +guarantees, and it records the node-24 coverage decision as a named limit instead of quietly +narrowing what CI proves. Its guarantee audit does not contain the P0 disease. + +Four concerns are worth the human's attention before `/pharn-dev-build`. Two are about the increment's +**edges rather than its core**: the ruleset mutation has no durable rollback record, and the two open +dependabot PRs will show six pending required checks until they rebase (bounded — `strict` policy +already forces that rebase). One is about the **strength of the only floor reduction**: the regex +extractor's parsing rule is unspecified, and `name:` occurs at three levels of a workflow file, so +the test must anchor on indentation, assert an exact count, and exclude the workflow-level `name: ci` +or it guards less than it advertises. The fourth is the **published-but-untested `engines.node` +range**, which the plan surfaces and then leaves unresolved. + +None of these argues against building. Three are satisfied by tightening the test and recording the +rollback verbatim; the fourth is a decision the human should make explicitly rather than inherit. + +**ADVISORY VERDICT: 6 concerns raised (0 blocking-severity, 4 important, 2 minor) — for the human to +weigh before `/pharn-dev-build`.** This grill-log gates nothing: every finding above rests on model +judgment, the severities are LLM-assigned and advisory (fix #3), and the only floor-grade facts in +this run are the spec-hash match, the `count-grillers.mjs` membership result, and the writes-scope +hook that pinned this file. `/pharn-dev-build`'s own floor-gates remain the deterministic backstop. diff --git a/.dev/features/ci-matrix-required-checks/PLAN.md b/.dev/features/ci-matrix-required-checks/PLAN.md new file mode 100644 index 0000000..f16703a --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/PLAN.md @@ -0,0 +1,147 @@ +# PLAN — ci-matrix-required-checks + +> **Revised after GATE 1 feedback.** The first draft proposed building the 30-context OS/node matrix +> the ruleset demands. The human rejected that direction: _"leave one check per each — we don't need +> to have one for node 20 one for node 22 etc., like we have before these changes."_ This plan now +> does the opposite: **one required check per gate, no matrix**, and the **ruleset** is what gets +> corrected. The slug is kept so the folder stays stable. + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: Split `ci.yml`'s single `check` job into six independently-reporting gate jobs + (`Format check`, `Lint`, `Markdown lint`, `Typecheck`, `Test`, `Build`) on one platform/node + version, and reduce the ruleset's required contexts from the 30 never-reported matrix names to + those six plus the three already-passing external contexts. +- layer(s): repo infrastructure (CI + repo config) — not a `ARCHITECTURE.md §4` product layer; no + `src/` behavior changes. +- constitution_refs: [P0, P1, P4, P5, P6, P7] + +## Live state this run (P6) + +Read/verified in this run, not from memory: + +- Ruleset `main protection` (id 18605288, `enforcement: active`, updated 2026-08-12T11:58) requires + **33** contexts: 30 of the shape ` ( / node )` over + `{Format check, Lint, Markdown lint, Typecheck, Test, Build}` × + `{ubuntu-latest 20, ubuntu-latest 22, ubuntu-latest 24, windows-latest 24, macos-latest 24}`, + plus `floor`, `Analyze (javascript-typescript)`, `gitleaks`. +- `.github/workflows/ci.yml` defines **one** job, `check` (ubuntu-latest, node 20), running the six + gates as `if: always()` steps. It reports the context `check` — which the ruleset does not list. +- Consequence on PR #92: `gh pr checks 92` → all 8 reported contexts pass; the 30 matrix contexts sit + at "Expected — waiting for status to be reported" and never arrive. Merge is blocked. +- `floor`, `gitleaks`, `Analyze (javascript-typescript)` are produced by `floor.yml`, `gitleaks.yml`, + `codeql.yml` and already pass. Their contexts are **bare job names** (`codeql.yml` job `analyze` is + `name: Analyze (${{ matrix.language }})`) — confirming a required context is the check-run name, + not `workflow / job`. No other workflow defines a job named `Build`, `Test`, `Lint`, `Typecheck`, + `Format check`, or `Markdown lint`, so the six new names cannot collide. +- `package.json` `engines.node` is `>=20`; the six gate scripts are `format:check`, `lint`, + `lint:md`, `typecheck`, `test:coverage`, `build`. +- `.prettierrc` sets `"endOfLine": "lf"` and the repo has no `.gitattributes` — this mattered only + for the rejected Windows cell and is therefore **out of scope now** (P7: no speculative addition). + +## Decision (records the human's answers at GATE 1) + +Six required contexts, one per gate, on **ubuntu-latest / node 24** — one job per gate, no matrix. +Multi-OS coverage is explicitly **not** part of this increment. + +Node 24 was chosen over today's node 20 (human's answer). Recorded consequence, so it is a decision +and not an accident: `package.json` declares `engines.node: ">=20"`, and after this change **no CI +gate exercises node 20 or 22 at all**. A node-24-only pipeline can go green on code that breaks the +declared minimum. That is an accepted, named limit of this increment (P7) — not a claim that node 20 +is supported-and-verified. + +Resulting required-status-check list (9 contexts, all of which actually report): + +`Format check`, `Lint`, `Markdown lint`, `Typecheck`, `Test`, `Build`, `floor`, +`Analyze (javascript-typescript)`, `gitleaks`. + +## Files + +- `.github/workflows/ci.yml` — rewrite: six jobs (`format-check`, `lint`, `markdown-lint`, + `typecheck`, `test`, `build`), each `runs-on: ubuntu-latest` with node 24, each with an explicit + `name:` equal to its required context; keep the existing pinned action SHAs and + `persist-credentials: false`; the aggregate `check` job and its `if: always()` step guards are + removed — job-level independence supersedes them — layer: CI infra. +- `tests/ci-workflow.test.ts` — new vitest test pinning the six job names and their gate commands + (see Evals) — layer: tests. +- `.dev/floor/check-run-pins.test.mjs` — **added by human-approved amendment after the first + `/pharn-dev-regress` STOP**, not present in the originally-approved `## Files`. Its live-repo + assertion `assert.equal(d.skipped, 2)` counts lockfile (`npm ci`) installs across every workflow; + six gate jobs raise that count to 7, so the exact-count tripwire fires by design — its own comment + says a changed count means "a lockfile install was added or removed on purpose", which is precisely + the case here. The edit is the one number plus a comment naming the new arithmetic. `d.violations` + stays `[]` — every added line is `npm ci`, so **no floating install is introduced** and the rule the + checker actually enforces is untouched — layer: floor tests. +- `CLAUDE.md` — update the CI paragraph: the six gates now run as six **jobs**, each reporting its + own required status check, rather than six steps in one `check` job (P4). +- `docs/contributing.md` — update "Quality gates": name the six jobs and add the missing + `npm run build` gate (P4). + +**Out of repo, done as a separate deliberate step, not a file write:** update ruleset 18605288's +`required_status_checks` to the 9 contexts above via `gh api`. This is a GitHub settings mutation, +not a plan file — it is called out here so the increment is not mistaken for complete without it. +The current ruleset JSON is captured to the session scratchpad first so it can be restored verbatim. + +## Contracts satisfied + +- None in `pharn-contracts` — this increment adds no Capability, finding, or install surface. It is + repo infrastructure. Named explicitly so the omission is not read as an oversight (P0/P7). + +## Evals to write (P1) + +`tests/ci-workflow.test.ts` — deterministic, dependency-free (no YAML parser is a direct +devDependency; a regex extractor over the raw file is the floor primitive here, `ARCHITECTURE.md §2` +#3). It encodes exactly the defect this increment fixes — a workflow job name drifting away from the +required context that names it: + +- extract every `name:` under `jobs:` in `.github/workflows/ci.yml` → assert **set equality** with + the six expected contexts (both directions: a rename, an addition, or a deletion fails); +- assert each job's `run:` line is its expected npm script — `Format check` → `npm run format:check`, + `Lint` → `npm run lint`, `Markdown lint` → `npm run lint:md`, `Typecheck` → `npm run typecheck`, + `Test` → `npm run test:coverage`, `Build` → `npm run build`; +- assert every job declares `runs-on: ubuntu-latest` and `node-version: 24`, so a silent + platform/version change cannot slip in unreviewed. + +## Guarantee audit (P0) + +- "`ci.yml` defines exactly the six named gate jobs, each running its own gate" → **floor: + enum/regex** — `tests/ci-workflow.test.ts` set-equality + per-job command assertions. +- "Those six names equal what the GitHub ruleset requires" → **advisory.** The expected list is a + checked-in copy; nothing in this repo reads the live ruleset. If the ruleset is edited on + github.com again, the test still passes and PRs block again. This is precisely how the current + breakage happened; it is a stated limit, not a solved problem. +- "The six gates are independent — one failure can't mask another" → **advisory**, but structurally + stronger than before: previously six `if: always()` steps in one job (one context, first failure + reported); now six jobs, each with its own context and its own pass/fail. +- "Merging PR #92 becomes possible" → **advisory, and false on its own.** The ruleset's + `required_signatures` rule independently blocks: commit `4b8a0be` is `"verified": false, + "reason": "unsigned"`, and local git has no `commit.gpgsign` / `gpg.format` / `user.signingkey`. + Per the human's answer this increment **reports that blocker and changes no signing config** — + the rule stays enforced and the branch stays unmergeable until the human signs it. +- **Honest scope note (P7):** this increment deliberately *reduces* CI coverage relative to the + ruleset's stated ambition — no Windows, macOS, node 20, or node 22 job. That is the human's + explicit decision, recorded above. It should not later be described as "cross-platform CI", and + node 20 must not be described as CI-verified while `engines.node` still claims `>=20`. + +## Trust audit (P2) + +No untrusted artifact is ingested: the change touches only repo-owned files. `ci.yml` keeps +`permissions: contents: read` and `persist-credentials: false` on `pull_request`, so a fork PR's +untrusted content still cannot exfiltrate or mutate anything from these jobs — the property +`floor.yml` documents is preserved, not extended. Splitting one job into six does not widen the +token or permission surface. + +## Determinism audit (P5) + +- The test's decision is set equality over extracted strings — a membership test, no classification. +- Each job is an explicit literal; there is no matrix, no computed product, and no exclusion list, so + the emitted context set is a readable enumeration. +- No fallback branch is introduced; a malformed `ci.yml` fails extraction and the test fails loudly + rather than degrading to a partial match. + +## Open questions (HALT) + +None outstanding — all three were resolved at GATE 1: + +1. Shape → **six contexts, one per gate** (not the single `check` job). +2. Node version → **node 24** (limit recorded under Decision and Guarantee audit). +3. Signing → **report only**; no git signing config is touched by this increment. diff --git a/.dev/features/ci-matrix-required-checks/REGRESSION.md b/.dev/features/ci-matrix-required-checks/REGRESSION.md new file mode 100644 index 0000000..d567da9 --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/REGRESSION.md @@ -0,0 +1,121 @@ +# REGRESSION — ci-matrix-required-checks + +**Base:** `4b8a0be0c4cbb1e84c93cf0ae47ffd3324c205b9` (working tree is dirty → `base = HEAD`, per the +deterministic base rule: `git status --porcelain` non-empty). + +This stage ran **twice**. Run 1 stopped the pipeline with `"verdict": "regressions"`; the human +widened the approved plan's `## Files`, the cause was fixed, and run 2 is clean. Both are recorded — +the first run is the interesting one and deleting it would make this report a worse record than the +run it describes. + +--- + +## Run 2 (current — the verdict that stands) + +**Inside (the changed scope)** — 5 paths, exactly the amended plan's `## Files`, `escaped: []`: + +```text +.github/workflows/ci.yml +CLAUDE.md +docs/contributing.md +tests/ci-workflow.test.ts +.dev/floor/check-run-pins.test.mjs +``` + +**Outside gates:** **45** test files — one fewer than run 1, because `check-run-pins.test.mjs` is now +*inside* the feature and correctly stops being an outside gate — plus whole-repo `validate`. +**0 committed eval pairs** exist in this repo today, so `outside_eval_pairs` is empty and no +`structural:*` gate runs. + +**Style gates skipped** by the deterministic config-touch rule: `inside` touches none of +`eslint.config.mjs`, `.prettierrc.json`, `.prettierignore`, `.markdownlint-cli2.jsonc`, so a style +flip over the byte-identical outside files is provably impossible. Absent from **both** result maps, +so the gate sets match. + +### Gate results (exit codes, base → head) + +| gate | base | head | outcome | +| ---------- | ---- | ---- | ------- | +| `tests` | 0 | 0 | OK | +| `validate` | 0 | 0 | OK | + +Reported counts, checked on both sides: base `pass 704 fail 0`, head `pass 704 fail 0`. + +`regressions[]`: **empty** · `pre_existing[]`: **empty** (nothing was already red at the baseline). + +### Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +`check-regress.mjs verdict` exit **0**, `"verdict": "no-regressions"`. + +--- + +## Run 1 (superseded — a real regression, and a false green before it) + +### The scoping note + +The first `scope` call passed the raw working-tree diff as `--changed`, which with `base = HEAD` also +sweeps in the pipeline's own stage artifacts — `.pharn/writes-scope.json` (always-writable hook +scratch per `enforce-writes-scope.cjs`), `PLAN.md` (written by `/pharn-dev-plan` under its own scope), +and `GRILL.md` (likewise `/pharn-dev-grill`). That call exited 1 with three blocking fix-#7 findings. +It was **not** a scope breach: `/pharn-dev-build` was pinned to its `## Files` and the hook would have +denied anything else. Re-run with `--changed` limited to the build's product diff — matching the prior +increment's convention (`remove-prunes-records`, whose `inside` is likewise exactly its `## Files`) — +`scope` exits **0** with `escaped: []`. Recorded rather than silently re-run, because deciding the +partition is **advisory orchestration**, not floor. + +### The invalid capture (P6) + +The first base/head capture recorded `tests: 1` on **both** sides, which `check-regress.mjs` correctly +read as `pre_existing` → `no-regressions`. That reading was true of the numbers and false of the +world: **both** runs had failed to run any test at all. + +Cause: the capture built the file list into a shell variable and expanded it unquoted — +`node --test $TESTS_ARR`. **zsh does not word-split an unquoted parameter expansion** (it *does* split +an unquoted command substitution), so all 46 paths arrived as a single argument and `node --test` +exited 1 with `Could not find ' '` and no TAP summary. Identical breakage on both +sides produced a matched pair of 1s — a false green that looked exactly like a real one. + +The capture now (a) expands `$(…)` inline so zsh splits it, and (b) **asserts a TAP summary line is +present on each side** before writing the results map, so a run that executed nothing can no longer be +recorded as a result. Corrected run-1 numbers: base exit `0` (`pass 748 fail 0`), head exit `1` +(`pass 747 fail 1`) → `"verdict": "regressions"`, `regressions[]: ["tests"]`. + +### The regression it exposed + +```text +.dev/floor/check-run-pins.test.mjs:365 +✖ ★ the live repo has NO floating install in any workflow run: line + AssertionError: Expected values to be strictly equal: 7 !== 2 + at check-run-pins.test.mjs:373 → assert.equal(d.skipped, 2) +``` + +`d.skipped` counts **lockfile installs** (`npm ci`) — the exempt, non-floating kind — across every +workflow. Base: 1 in `ci.yml` + 1 in `publish.yml` = 2. Head: 6 in `ci.yml` (one per gate job) + 1 in +`publish.yml` = 7. Confirmed causally, not inferred: stashing only `.github/workflows/ci.yml` returned +that file to **44 pass / 0 fail**. + +Two things the failure was **not**, both checked rather than assumed: `d.violations` was still `[]`, so +**no floating install was introduced** — every added line is `npm ci`, pinned by the lockfile; and +`d.files` still enumerated every workflow, so nothing dropped out of the scan. What tripped is the +deliberate exact-count tripwire behaving exactly as designed — its own comment reads *"If this number +changes, a lockfile install was added or removed on purpose."* + +### Resolution + +The fix was the one number, and the file was outside the approved `## Files`, so the fix-#7 hook denied +the write — correctly, fail-closed. Widening an approved plan's scope is a human decision, so the +pipeline stopped and asked. The human approved the amendment; `PLAN.md` `## Files` now carries +`.dev/floor/check-run-pins.test.mjs` with the reason, the assertion reads `7` with a comment naming +the new arithmetic, the writes-scope was re-set **from the amended plan** (never bypassed), and the +floor is GREEN again (748/748, `validate` 0, `npm run check` 0). + +--- + +## Honest residual (P7) + +`/pharn-dev-regress` catches exactly what its suite catches — nothing more. The claim is +"deterministically-detectable breakage outside the feature is caught", **not** "nothing broke". +Choosing the base, partitioning inside/outside, and running the suite are **advisory orchestration**; +only the exit-code comparison is a guarantee. Run 1 is a live demonstration that bad orchestration can +feed a sound comparator a false green — the comparator was never wrong about the ints it was given. diff --git a/.dev/features/ci-matrix-required-checks/REVIEW.md b/.dev/features/ci-matrix-required-checks/REVIEW.md new file mode 100644 index 0000000..50f0126 --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/REVIEW.md @@ -0,0 +1,193 @@ +# REVIEW — ci-matrix-required-checks + +**Floor first (P0):** `node .dev/floor/validate.mjs .` exit **0** — GREEN. Everything below the floor +line is **advisory**. + +**Independent structural check of the built artifact** (not a claim from the diff): parsing +`.github/workflows/ci.yml` as YAML yields 6 jobs, names `Format check` / `Lint` / `Markdown lint` / +`Typecheck` / `Test` / `Build`, each `runs-on: ubuntu-latest`, `node-version: 24`, +`persist-credentials: false`, 4 steps; workflow `permissions: {contents: read}`; triggers +`pull_request` + `push`. The file is valid Actions YAML, not merely well-formatted text. + +--- + +## Floor-gate findings (blocking) + +**None.** No guarantee in this increment lacks a floor reduction or an `advisory` label; no eval +binding is missing (0 Capabilities added, 0 eval pairs owed — the floor agrees); no sibling reference +exists; no guaranteed decision rests on a tainted field. + +--- + +## Advisory findings + +### L-floor → P0 + +```yaml +- type: FINDING + rule_id: 'P4' + severity: important + file: 'CLAUDE.md:28' + problem: 'The docs assert as present-tense fact that the `main` ruleset requires these six names, but the ruleset has not been changed — it still requires the 30 matrix contexts, so the documented state does not exist yet.' + evidence: 'are a **contract with the `main` branch ruleset**, which lists exactly those strings in `required_status_checks`' +``` + +The same claim appears a second time in the other doc: + +```yaml +- type: FINDING + rule_id: 'P4' + severity: important + file: 'docs/contributing.md:47' + problem: 'States the job names are the exact contexts the ruleset requires — true only after the approved out-of-repo ruleset step is performed, which this run deliberately deferred and has not done.' + evidence: 'The job names above are the exact contexts the `main` branch ruleset requires' +``` + +This is the sharpest thing in the increment. Verified live rather than inferred: ruleset 18605288 +still carries the 33 contexts it had at 2026-08-12T11:58 (30 matrix names + `floor` + `gitleaks` + +`Analyze (javascript-typescript)`). Both docs were written for the post-change world. **P4 forbids +documenting behavior the code does not have**, and a doc asserting an external configuration that is +not so is the same defect pointed outward. + +It resolves in one of two directions, and the direction is the human's at the post-review gate: +perform the approved ruleset update (after which both sentences become true, and this finding +evaporates), or soften both sentences to the intended-state framing. What must **not** happen is +merging the docs while the ruleset still says something else — that reinstates exactly the +workflow↔ruleset disagreement this increment exists to end, only now with the repo confidently +documenting the wrong side of it. + +```yaml +- type: FINDING + rule_id: 'P0' + severity: minor + file: 'tests/ci-workflow.test.ts:11' + problem: 'The increment''s only floor reduction covers the repo half of a two-sided invariant, and the uncovered half is the half that actually failed in production.' + evidence: 'Nothing here reads the live ruleset, so a ruleset edited on github.com still drifts silently — an advisory limit, stated rather than papered over (P0).' +``` + +Not the P0 disease — the limit is labeled `advisory` at all four places it appears (the test header, +`CLAUDE.md`, `PLAN.md`'s guarantee audit, `VERIFY.md`'s residual), which is exactly what P0 asks of a +claim that cannot reduce to a floor primitive. Raised so it is weighed rather than inherited: after +this increment the repo still cannot detect the recurrence of the incident that motivated it. A +non-blocking job that diffs the live ruleset against the workflow via `gh api` would close it, and is +correctly **not** bundled here (P7). + +### L-eval → P1 + +```yaml +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/floor/check-run-pins.test.mjs:377' + problem: 'The lockfile-install count is now a constant that must be hand-updated by anyone who adds or removes a CI job, and nothing points a future editor from the workflow to this file.' + evidence: 'assert.equal(d.skipped, 7);' +``` + +Working as designed — the assertion's own comment says a changed count means an install was added or +removed on purpose, and this increment is precisely that case, confirmed through the human gate rather +than absorbed silently. The residual cost is a coupling with no signpost at the other end: `ci.yml` +does not mention that adding a seventh job breaks a floor test. Cheap mitigation if desired — one line +in `ci.yml`'s header comment. Not done here: `ci.yml` is in scope, but adding it now would be an +unreviewed edit after the verify verdict was computed, and the pipeline's ordering matters more than +the convenience. + +Otherwise clean: `tests/ci-workflow.test.ts` ships with the behavior it specifies (P1) and was +**mutation-checked**, not merely observed green — renaming `Markdown lint` to `Markdown Lint` fails 2 +of its 4 cases. That is the difference between a test that demonstrates a behavior and one that merely +asserts it exists (P1's actual wording). + +### L-trust → P2 + +```yaml +- type: FINDING + rule_id: 'P2' + severity: minor + file: '.dev/floor/check-run-pins.test.mjs:371' + problem: 'A comment in a reviewed file did shape a decision in this run — it told the agent how to respond to the failing assertion, and the agent responded that way.' + evidence: 'If this number changes, a lockfile install was added or removed on purpose.' +``` + +Self-reported per L-trust's instruction to say so when reviewed content influences behavior. The +honest accounting: this file is repo-owned and `trust: trusted`, not fetched or untrusted input, so +following its guidance is not the attack pattern P2 targets — and the decision was **not** taken on +the comment's authority. It was routed through the human gate (the writes-scope hook denied the edit, +the plan amendment was approved explicitly), and the comment's factual claim was verified +independently: `d.violations` is `[]` and all six added lines are `npm ci`. Noted because the defense +is noticing, not because a boundary was crossed. + +No untrusted artifact is ingested anywhere in this increment. All six jobs preserve +`permissions: contents: read` and `persist-credentials: false` — confirmed by parsing the YAML, not by +reading the diff — so a fork PR's untrusted content still cannot exfiltrate or mutate from these jobs. +Splitting one job into six multiplies the job count, not the token surface. + +### L-axis → P3 + +```yaml +- type: FINDING + rule_id: 'P3' + severity: minor + file: '.github/workflows/ci.yml:31' + problem: 'The six jobs are near-identical 12-line blocks, so any change to the runner, the node version, or the action pins is six coordinated edits with no structural guard that they stay identical.' + evidence: 'name: Format check / runs-on: ubuntu-latest / uses: actions/checkout@3d3c42e5… (repeated six times)' +``` + +Accepted, and the alternatives are worse here. A reusable workflow would rename the contexts to +`caller / callee` and break the very ruleset coupling this increment exists to establish; YAML anchors +are not supported by Actions; a composite action would add a file and an indirection to save four +lines per job. The duplication is also **guarded**: `tests/ci-workflow.test.ts` asserts the runner and +node version across *every* job, so silent divergence fails, and `.dev/floor/check-run-pins.test.mjs` +independently pins the install count. Explicit-and-checked beats clever-and-unpinned for a file whose +job names are load-bearing. + +No file carries two change-reasons; no leaf imports a sibling; nothing in `src/` is touched. + +```yaml +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'docs/contributing.md:47' + problem: 'The required-context list now exists in four places — the test''s EXPECTED_GATES map, the ci.yml header comment, CLAUDE.md, and docs/contributing.md — none of which is the authority, which is the GitHub ruleset.' + evidence: 'Three more workflows report required checks: `floor`, `gitleaks`, and `Analyze (javascript-typescript)`' +``` + +Only one copy is enforced (the test's map); the other three are prose that can rot independently. +Tolerable at this size and arguably useful — each copy serves a different reader — but it is four +things to update on the next CI change, and worth knowing before the fifth copy is added. + +--- + +## Verdict + +**GREEN — 0 floor-gate findings, 7 advisory** (2 important, 5 minor). The increment is not blocked. + +The two important findings are the same defect stated in two files: **the docs describe a ruleset that +has not been changed yet.** They are true the moment the approved out-of-repo step runs, and false +until then. That is the decision waiting at the post-review gate, and it is the human's. + +Every severity above is **LLM-assigned and advisory** (fix #3). The only floor-grade facts in this +review are `validate.mjs` exit 0 and the YAML parse; `REVIEW.md` has no `findings.json`, no +`check-review.mjs`, and gates nothing. + +--- + +## Proposed lesson for canon (NOT written here — `/pharn-dev-memory-promote` decides) + +Proposed candidate for `.dev/memory-bank/lessons-learned.md`. Recorded here as a proposal only; +`/pharn-dev-review` writes no canon, and the model never self-promotes (P2). + +- **Lesson:** A gate-capture harness must prove it *ran* before its exit code is treated as a result. + In `/pharn-dev-regress` this run, `node --test $TESTS_ARR` was expanded unquoted under **zsh**, which + — unlike bash — does **not** word-split an unquoted parameter expansion (it *does* split an unquoted + command substitution). All 46 paths arrived as one argument, `node --test` exited 1 having run + nothing, and because the identical breakage hit **both** the baseline and HEAD captures, the + comparator saw `1 → 1` and reported `pre_existing` / `no-regressions`. A real regression sat behind + a green verdict. The comparator was never wrong about the ints it was given — the orchestration fed + it ints that meant nothing. Remedy applied: assert a TAP summary line exists on each side before + writing the results map, so a run that executed zero tests can never be recorded as a result. +- **Why it is canon-worthy (P7 — real, not hypothetical):** it is a live failure from this increment, + it produced a false green on the one stage whose entire purpose is catching regressions, and the + failure mode is invisible precisely because it is symmetric — the two-sided comparison that makes + `check-regress` trustworthy is also what hides a harness that broke identically on both sides. +- **Provenance:** increment `ci-matrix-required-checks`; base + `4b8a0be0c4cbb1e84c93cf0ae47ffd3324c205b9`; see `REGRESSION.md` § "Run 1 (superseded)" for the + captured evidence and `regression-report.json` for the corrected verdict. diff --git a/.dev/features/ci-matrix-required-checks/SHIP.md b/.dev/features/ci-matrix-required-checks/SHIP.md new file mode 100644 index 0000000..bff90d5 --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/SHIP.md @@ -0,0 +1,103 @@ +# SHIP — ci-matrix-required-checks + +**Where this run ended: GATE 2** — the post-review human decision (merge / fix / abandon). + +## Stages run, in order + +| # | stage | outcome | +| --- | -------------------- | ----------------------------------------------------------------------- | +| 1 | `/pharn-dev-plan` | GATE 1 — halted, revised once on human feedback, then approved | +| 2 | `/pharn-dev-grill` | advisory — 6 concerns (0 blocking, 4 important, 2 minor); gates nothing | +| 3 | `/pharn-dev-build` | floor GREEN | +| 4 | `/pharn-dev-regress` | run 1 **`regressions` → STOP**; human widened scope; run 2 `no-regressions` | +| 5 | `/pharn-dev-verify` | `PASS` | +| 6 | `/pharn-dev-review` | GREEN — 0 floor-gate findings, 7 advisory | + +## Structural verdicts read, verbatim + +- **`/pharn-dev-build` → `node .dev/floor/validate.mjs .` exit `0`** (GREEN). Alongside it, all six + gates the new workflow defines pass locally: `npm run check` exit 0, `lint:md` exit 0, `build` + exit 0. +- **`/pharn-dev-regress` → `regression-report.json` `.verdict` = `"no-regressions"`**, helper exit + `0`. `regressions[]: []` · `pre_existing[]: []` · base `4b8a0be0c4cbb1e84c93cf0ae47ffd3324c205b9` · + gates `tests 0→0`, `validate 0→0`. +- **`/pharn-dev-verify` → `verify-report.json` `.verdict` = `"PASS"`**, helper exit `0`. + `failing_gates: []` · gates `test`, `validate`, `lint`, `format:check`, `lint:md` all `0` · + `verifiers: {registered: 0, findings: []}`. + +**The one STOP, recorded rather than smoothed over.** `/pharn-dev-regress` run 1 returned +`"regressions"` (exit 1) and halted the chain. Cause: six `npm ci` lines instead of one tripped the +exact-count tripwire in `.dev/floor/check-run-pins.test.mjs` (`d.skipped` 2 → 7) — a floor test +outside the approved `## Files`, so the fix-#7 hook denied the fix, correctly. The human approved a +scope amendment; the count was updated with a comment naming the new arithmetic and the scope was +re-set **from the amended plan**, never bypassed. A prior capture in that same stage had also been +invalid (a zsh word-splitting bug produced a false green over a real regression) — see +`REGRESSION.md`, which keeps both runs. + +## GATE 1 decisions (the human's, recorded) + +The plan was rewritten once. The first draft proposed building the 30-context OS/node matrix the +ruleset demanded; the human rejected that direction — **one required check per gate, no matrix** — +and chose **node 24**, and **report-only** on the signing blocker. + +## Pointers (cite, don't restate — P4) + +- `PLAN.md` — approved plan (incl. the post-STOP scope amendment), guarantee audit, named limits. +- `GRILL.md` — advisory grill-log, 6 concerns. +- `REGRESSION.md` / `regression-report.json` — both regress runs and the corrected capture. +- `VERIFY.md` / `verify-report.json` — the floor gate table and its residual. +- `REVIEW.md` — 7 advisory findings and a proposed canon lesson (not yet promoted). + +## Out-of-repo change APPLIED — GitHub ruleset 18605288 (`main protection`) + +`required_status_checks` reduced from **33** contexts to **9**. All other rules preserved unchanged: +`pull_request`, **`required_signatures`**, `non_fast_forward`, `deletion`; `enforcement: active`; +bypass actors untouched. + +Now required: `Format check`, `Lint`, `Markdown lint`, `Typecheck`, `Test`, `Build`, `floor`, +`gitleaks`, `Analyze (javascript-typescript)`. + +**Rollback record (durable — this is repo settings, so `git revert` cannot undo it).** To restore the +previous list, PUT the same payload with `required_status_checks` set back to these 33 contexts: + +```text +Format check (ubuntu-latest / node 20) Typecheck (ubuntu-latest / node 20) +Format check (ubuntu-latest / node 22) Typecheck (ubuntu-latest / node 22) +Format check (ubuntu-latest / node 24) Typecheck (ubuntu-latest / node 24) +Format check (windows-latest / node 24) Typecheck (windows-latest / node 24) +Format check (macos-latest / node 24) Typecheck (macos-latest / node 24) +Lint (ubuntu-latest / node 20) Test (ubuntu-latest / node 20) +Lint (ubuntu-latest / node 22) Test (ubuntu-latest / node 22) +Lint (ubuntu-latest / node 24) Test (ubuntu-latest / node 24) +Lint (windows-latest / node 24) Test (windows-latest / node 24) +Lint (macos-latest / node 24) Test (macos-latest / node 24) +Markdown lint (ubuntu-latest / node 20) Build (ubuntu-latest / node 20) +Markdown lint (ubuntu-latest / node 22) Build (ubuntu-latest / node 22) +Markdown lint (ubuntu-latest / node 24) Build (ubuntu-latest / node 24) +Markdown lint (windows-latest / node 24) Build (windows-latest / node 24) +Markdown lint (macos-latest / node 24) Build (macos-latest / node 24) +floor Analyze (javascript-typescript) +gitleaks +``` + +## State left behind + +- **Working tree, uncommitted:** `.github/workflows/ci.yml` (rewritten), `tests/ci-workflow.test.ts` + (new), `.dev/floor/check-run-pins.test.mjs` (one constant), `CLAUDE.md`, `docs/contributing.md`, + plus this feature folder. **Nothing committed, nothing pushed** — that is the human's call. +- **Blocker B untouched, as decided:** commit `4b8a0be` is `"verified": false, "reason": "unsigned"` + and `required_signatures` remains active, so PR #92 stays blocked on signatures regardless of + checks. No git signing config was modified. +- **Until the branch is pushed**, the six new contexts cannot report — the workflow that produces + them exists only in this working tree. Expect PR #92 to show six pending required checks in the + interval, which looks like the original symptom but is not it. +- **The two open dependabot PRs (#88, #89)** need an update-to-`main` before they report the six new + contexts. `strict_required_status_checks_policy: true` already forces that rebase (#88 is + `mergeable_state: "behind"`), so this is self-healing, not a trap. + +## Standing decision + +The chain ran end to end and the named floor verdicts are as shown — `validate` exit 0, +`no-regressions`, `PASS`. **This is NOT a judgment that the increment is good or wise; that is the +human's call at the post-review gate.** `/pharn-dev-ship` has not merged, pushed, committed, or +applied any `PHARN ✓ reviewed` seal, and does not do so. diff --git a/.dev/features/ci-matrix-required-checks/VERIFY.md b/.dev/features/ci-matrix-required-checks/VERIFY.md new file mode 100644 index 0000000..a373bea --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/VERIFY.md @@ -0,0 +1,59 @@ +# VERIFY — ci-matrix-required-checks + +## FLOOR layer — the deterministic gates (owns the verdict) + +| gate | command | exit | +| -------------- | ----------------------------- | ---- | +| `test` | `npm test` | 0 | +| `validate` | `node .dev/floor/validate.mjs .` | 0 | +| `lint` | `npm run lint` | 0 | +| `format:check` | `npm run format:check` | 0 | +| `lint:md` | `npm run lint:md` | 0 | + +**VERIFIED: floor gates PASS.** `.dev/floor/check-verify.mjs` exit **0**, `"verdict": "PASS"`, +`failing_gates: []`. + +The gate set is exactly the repo's `npm run check` aggregate plus `lint:md`, so the verdict tracks the +full style surface at verify rather than deferring an increment's markdown style to CI (L9 — cited, not +restated, P4). **No `structural:*` gate ran:** this repo ships **0** committed eval pairs +(`git ls-files '*/evals/expected/*.json'` → 0), so there is none to run — an absence measured, not +skipped. + +**Gates deliberately not in the map, named so their absence is not read as coverage.** `npm run build` +is not a `check-verify` gate here, so the `Build` job's green is asserted by CI rather than by this +verdict — it was run separately during `/pharn-dev-build` (exit 0). And nothing in this verdict +exercises the new workflow **as GitHub will execute it**: the six jobs' correctness as *Actions* is +established only when the branch is pushed and the six checks report. + +## ADVISORY layer — verifiers + +**No verifiers registered — floor gates only.** `node .dev/floor/count-verifiers.mjs .` → +`{"registered":0,"verifiers":[]}` — a deterministic `role:` frontmatter read (P5), never a prose grep. +Step 2 is therefore a no-op, no `claude -p` call was made, and `verifiers.findings[]` is empty. No +verifier free-text exists in this run, so the P2 taint boundary carries nothing today; it stands ready +for when the first verifier lands. + +## What this increment's own tests cover + +`npm test` collects `tests/ci-workflow.test.ts` (4 cases), the increment's own specification (P1). It +was mutation-checked rather than merely observed green: renaming the `Markdown lint` job to +`Markdown Lint` — a single character — fails 2 of its 4 cases (`expected [...] to deeply equal [...]` +and `no job named Markdown lint`). The tripwire demonstrably trips. + +`.dev/floor/check-run-pins.test.mjs` (amended in scope) confirms the workflow adds **no floating +install**: `d.violations` is `[]` and all six added `run:` lines are `npm ci`, lockfile-pinned. + +## Honest residual (P0/P7) + +**Verified = the named gates passed.** This is **NOT** a guarantee of correctness beyond what those +gates check — verifier concerns would be advisory help, not assurance, and there are none registered. +Specifically unverified by this verdict, and worth carrying to the review gate: + +- that the six job names match the **live GitHub ruleset** — `tests/ci-workflow.test.ts` pins only the + repo side of that two-sided invariant and cannot read the ruleset; +- that the workflow **runs green on GitHub**, which no local gate can establish; +- that `engines.node: ">=20"` holds, since every gate here ran on one local Node and CI will now run + only Node 24. + +Running the gates and composing the gate set is **advisory orchestration**; only the exit-code +threshold is floor-grade. diff --git a/.dev/features/ci-matrix-required-checks/regression-report.json b/.dev/features/ci-matrix-required-checks/regression-report.json new file mode 100644 index 0000000..dfca3e5 --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/regression-report.json @@ -0,0 +1,23 @@ +{ + "base": "4b8a0be0c4cbb1e84c93cf0ae47ffd3324c205b9", + "inside": [ + ".github/workflows/ci.yml", + "CLAUDE.md", + "docs/contributing.md", + "tests/ci-workflow.test.ts", + ".dev/floor/check-run-pins.test.mjs" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/ci-matrix-required-checks/verify-report.json b/.dev/features/ci-matrix-required-checks/verify-report.json new file mode 100644 index 0000000..50e44f3 --- /dev/null +++ b/.dev/features/ci-matrix-required-checks/verify-report.json @@ -0,0 +1,13 @@ +{ + "feature": "ci-matrix-required-checks", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { "registered": 0, "findings": [] } +} diff --git a/.dev/floor/check-run-pins.test.mjs b/.dev/floor/check-run-pins.test.mjs index 3e3b561..4ff8bed 100644 --- a/.dev/floor/check-run-pins.test.mjs +++ b/.dev/floor/check-run-pins.test.mjs @@ -370,7 +370,11 @@ test("★ the live repo has NO floating install in any workflow run: line", () = // `npm ci` in ci.yml and publish.yml — asserted EXACTLY, so an exemption can never become a // silent hole. If this number changes, a lockfile install was added or removed on purpose. - assert.equal(d.skipped, 2); + // + // 7 = six in ci.yml (one per gate job — each required status check installs for itself) plus one + // in publish.yml. It was 2 while ci.yml ran a single `check` job; splitting that job into six is + // the deliberate change this count now records. + assert.equal(d.skipped, 7); // Independent recount of the enumerated workflow files, case-insensitively — exit 0 is also what // a checker returns when it opened nothing. diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 35ea667..37aa1d5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,5 +1,24 @@ name: ci +# One job per gate, one status check per job. +# +# GitHub reports an Actions job under its `name:`, and that string is what the +# `main` branch ruleset lists in `required_status_checks` (the same convention +# `codeql.yml` relies on for `Analyze (javascript-typescript)`). So the six +# `name:` values below are a CONTRACT with the ruleset, not decoration — +# renaming one makes a required check unreportable and blocks every PR until the +# ruleset is edited to match. `tests/ci-workflow.test.ts` pins them. +# +# Why six jobs rather than six steps in one job: each gate then passes or fails +# on its own status check, so one red gate can neither mask nor be masked by +# another. That is what the previous single `check` job used `if: always()` step +# guards to approximate. +# +# Coverage is deliberately one platform, one Node version (see +# `.dev/features/ci-matrix-required-checks/PLAN.md`). Note that `package.json` +# declares `engines.node: ">=20"` while these gates run node 24 — the lower end +# of that published range is NOT exercised here. + on: pull_request: push: @@ -9,7 +28,8 @@ permissions: contents: read jobs: - check: + format-check: + name: Format check runs-on: ubuntu-latest steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -17,28 +37,89 @@ jobs: persist-credentials: false - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: - node-version: 20 + node-version: 24 cache: npm - name: Install - id: install run: npm ci - # Run each gate independently so one failure can't mask the others, but skip - # them cleanly if install failed — no cascade of misleading "deps missing" errors. - name: Format check - if: ${{ always() && steps.install.outcome == 'success' }} run: npm run format:check + + lint: + name: Lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: 24 + cache: npm + - name: Install + run: npm ci - name: Lint - if: ${{ always() && steps.install.outcome == 'success' }} run: npm run lint + + markdown-lint: + name: Markdown lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: 24 + cache: npm + - name: Install + run: npm ci - name: Markdown lint - if: ${{ always() && steps.install.outcome == 'success' }} run: npm run lint:md + + typecheck: + name: Typecheck + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: 24 + cache: npm + - name: Install + run: npm ci - name: Typecheck - if: ${{ always() && steps.install.outcome == 'success' }} run: npm run typecheck + + test: + name: Test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: 24 + cache: npm + - name: Install + run: npm ci - name: Test - if: ${{ always() && steps.install.outcome == 'success' }} run: npm run test:coverage + + build: + name: Build + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 + with: + node-version: 24 + cache: npm + - name: Install + run: npm ci - name: Build - if: ${{ always() && steps.install.outcome == 'success' }} run: npm run build diff --git a/.pharn/pharn-dev-regress/base-results.json b/.pharn/pharn-dev-regress/base-results.json index 9b3f2b3..c3ea19a 100644 --- a/.pharn/pharn-dev-regress/base-results.json +++ b/.pharn/pharn-dev-regress/base-results.json @@ -1 +1 @@ -{"tests":0,"validate":0} +{"tests":0,"validate":0} \ No newline at end of file diff --git a/.pharn/pharn-dev-regress/head-results.json b/.pharn/pharn-dev-regress/head-results.json index 9b3f2b3..c3ea19a 100644 --- a/.pharn/pharn-dev-regress/head-results.json +++ b/.pharn/pharn-dev-regress/head-results.json @@ -1 +1 @@ -{"tests":0,"validate":0} +{"tests":0,"validate":0} \ No newline at end of file diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index 444fb5d..e90710e 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/remove-prunes-records/SHIP.md" + ".dev/features/ci-matrix-required-checks/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-08-12T10:42:58.999Z" + "set_at": "2026-08-12T13:04:18.311Z" } diff --git a/CLAUDE.md b/CLAUDE.md index ddcfad5..95eb852 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,7 +25,7 @@ npm run check # format:check + lint + typecheck + test (aggregate npx vitest run tests/install-capabilities.test.ts # single test file ``` -CI (`.github/workflows/ci.yml`) runs six gates independently (so one failure can't mask the others) — format:check, lint, lint:md, typecheck, test (via `test:coverage`), and build — all must pass. A second workflow, `.github/workflows/publish.yml`, publishes to npm on a published GitHub Release — see **Releasing** below. `PHARN_DEBUG=1` enables full error output for fetch/install failures. +CI (`.github/workflows/ci.yml`) runs six gates independently — format:check, lint, lint:md, typecheck, test (via `test:coverage`), and build — all must pass. Each gate is its **own job**, so each reports its **own status check** and one red gate can neither mask nor be masked by another (this replaced a single `check` job whose six steps shared one context and leaned on `if: always()`). The six job `name:` values — `Format check`, `Lint`, `Markdown lint`, `Typecheck`, `Test`, `Build` — are a **contract with the `main` branch ruleset**, which lists exactly those strings in `required_status_checks` (alongside `floor`, `gitleaks`, and `Analyze (javascript-typescript)` from the other workflows): GitHub reports a job under its `name:`, so renaming one makes a required check unreportable and blocks every PR on a context nothing produces. `tests/ci-workflow.test.ts` pins the names, their npm scripts, and the runner/node pair; it cannot see the ruleset, so the other half of that invariant stays advisory. Gates run on **ubuntu-latest / node 24 only** — note `package.json` declares `engines.node: ">=20"`, so the lower end of the published range is **not** exercised by CI. A second workflow, `.github/workflows/publish.yml`, publishes to npm on a published GitHub Release — see **Releasing** below. `PHARN_DEBUG=1` enables full error output for fetch/install failures. ## Releasing diff --git a/docs/contributing.md b/docs/contributing.md index a9113c6..0030f12 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -37,16 +37,21 @@ Published package `@pharn-dev/pharn` exposes a single `pharn` bin (see `package. CI ([`.github/workflows/ci.yml`](../.github/workflows/ci.yml)) runs these gates on every push and PR — all must pass: ```bash -npm run format:check -npm run lint -npm run lint:md # markdownlint-cli2 over docs/ and root *.md -npm run typecheck -npm run test:coverage # vitest with enforced coverage thresholds +npm run format:check # job "Format check" +npm run lint # job "Lint" +npm run lint:md # job "Markdown lint" — markdownlint-cli2 over docs/ and root *.md +npm run typecheck # job "Typecheck" +npm run test:coverage # job "Test" — vitest with enforced coverage thresholds +npm run build # job "Build" — typecheck + esbuild bundle ``` +Each gate is a **separate job**, so it reports its own status check and a failure in one never hides a failure in another. The job names above are the exact contexts the `main` branch ruleset requires, so renaming a job also means updating the ruleset — [`tests/ci-workflow.test.ts`](../tests/ci-workflow.test.ts) fails if the workflow side drifts. + +Gates run on **ubuntu-latest with Node 24**. `package.json` declares `engines.node: ">=20"`; CI does not exercise Node 20 or 22, so verify locally if your change touches runtime-version-sensitive APIs. + `npm run check` runs `format:check` + `lint` + `typecheck` + `test` as a single local pre-push command. -A separate CodeQL workflow analyzes the JavaScript/TypeScript surface on PRs, pushes to `main`, and weekly. +Three more workflows report required checks: `floor` (the deterministic PHARN floor), `gitleaks` (secret scanning), and `Analyze (javascript-typescript)` (CodeQL, also on pushes to `main` and weekly). ## Branch & commit style diff --git a/tests/ci-workflow.test.ts b/tests/ci-workflow.test.ts new file mode 100644 index 0000000..c261687 --- /dev/null +++ b/tests/ci-workflow.test.ts @@ -0,0 +1,128 @@ +import { readFileSync } from 'node:fs'; +import { describe, expect, it } from 'vitest'; + +// GitHub reports an Actions job under its `name:`, and that exact string is what +// the `main` branch ruleset lists in `required_status_checks`. When the two +// disagree the required check is never reported, so every PR hangs on a context +// no workflow produces — merge-blocked with nothing red to fix. That happened +// once (a ruleset listing 30 OS/node matrix contexts against a workflow that +// defined one job), which is the real need this test exists to serve (P7). +// +// The invariant is two-sided and this test pins only the side that lives in the +// repo. Nothing here reads the live ruleset, so a ruleset edited on github.com +// still drifts silently — an advisory limit, stated rather than papered over +// (P0). Cite `.dev/features/ci-matrix-required-checks/PLAN.md` for the full +// guarantee audit. +const WORKFLOW = '.github/workflows/ci.yml'; + +/** Required status check (the job's `name:`) → the npm script that gate runs. */ +const EXPECTED_GATES: ReadonlyMap = new Map([ + ['Format check', 'npm run format:check'], + ['Lint', 'npm run lint'], + ['Markdown lint', 'npm run lint:md'], + ['Typecheck', 'npm run typecheck'], + ['Test', 'npm run test:coverage'], + ['Build', 'npm run build'], +]); + +const RUNNER = 'ubuntu-latest'; +const NODE_VERSION = '24'; + +const source = readFileSync(WORKFLOW, 'utf8'); + +/** + * Split the `jobs:` mapping into one text block per job, keyed by job id. + * + * Indentation IS the parse here — a job id sits at exactly two spaces and its + * keys at four — because no YAML parser is a direct devDependency and pulling + * one in for a shape this small is not worth the supply-chain surface. The + * discrimination that matters is that `name:` occurs at three levels of a + * workflow file (column 0 for the workflow, four spaces for a job, and after a + * `- ` for a step); anchoring on the four-space form is what separates them, and + * the tests below assert both directions so a formatting change cannot quietly + * turn this into a partial match. + */ +function jobBlocks(yaml: string): Map { + const lines = yaml.split('\n'); + const start = lines.indexOf('jobs:'); + expect(start, `${WORKFLOW} has no top-level \`jobs:\` key`).toBeGreaterThan( + -1, + ); + + const blocks = new Map(); + let current: string | null = null; + let buffer: string[] = []; + + const flush = (): void => { + if (current !== null) blocks.set(current, buffer.join('\n')); + }; + + for (const line of lines.slice(start + 1)) { + const jobId = /^ {2}([A-Za-z0-9_-]+):\s*$/.exec(line); + if (jobId) { + flush(); + current = jobId[1]!; + buffer = []; + continue; + } + if (current !== null) buffer.push(line); + } + flush(); + + return blocks; +} + +/** Every match of `re`'s single capture group, in file order. */ +function captureAll(block: string, re: RegExp): string[] { + return [...block.matchAll(re)].map((m) => m[1]!); +} + +const blocks = jobBlocks(source); +const jobNames = [...blocks.values()].flatMap((b) => + captureAll(b, /^ {4}name: (.+)$/gm), +); + +describe('ci.yml required status checks', () => { + it('defines exactly one job per required gate, and no others', () => { + // Set equality in both directions: a rename, an addition, or a deletion all + // fail. A one-directional `toContain` sweep would miss a stray seventh job. + expect([...jobNames].sort()).toEqual([...EXPECTED_GATES.keys()].sort()); + expect(blocks.size).toBe(EXPECTED_GATES.size); + }); + + it('reads job names only, never the workflow name or a step name', () => { + // The extractor's discrimination is the thing most likely to rot, so assert + // the two decoys are really present in the file and really excluded. + expect(source.startsWith('name: ci\n')).toBe(true); + expect(jobNames).not.toContain('ci'); + + expect(source).toContain(' - name: Install'); + expect(jobNames).not.toContain('Install'); + }); + + it('runs each gate through its own npm script', () => { + for (const [gate, script] of EXPECTED_GATES) { + const block = [...blocks.values()].find((b) => + new RegExp(`^ {4}name: ${gate}$`, 'm').test(b), + ); + expect(block, `no job named ${gate}`).toBeDefined(); + // Install first, then exactly the one gate command — so a gate cannot + // quietly grow a second responsibility (P3). + expect(captureAll(block!, /^\s+run: (.+)$/gm)).toEqual([ + 'npm ci', + script, + ]); + } + }); + + it('pins every gate to the same runner and node version', () => { + for (const [jobId, block] of blocks) { + expect(captureAll(block, /^ {4}runs-on: (.+)$/gm), jobId).toEqual([ + RUNNER, + ]); + expect(captureAll(block, /^\s+node-version: (.+)$/gm), jobId).toEqual([ + NODE_VERSION, + ]); + } + }); +});