From 0f56106ef9ab46858e8e5117dc470482a1d4e49c Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 19:09:29 +0000 Subject: [PATCH] fix(init): a re-run init keeps what update would keep, and re-checks the config under its lock A re-run `pharn init` carried over hand-added capabilities, but by a narrower rule than `pharn update`'s merge table: - a `manual` entry the archetypes ALSO selected was re-recorded `auto`, so a later archetype change let `update` drop a capability the user asked for by name (merge row 3 keeps it sticky); - an entry upstream still ships but this CLI cannot parse was dropped from the config and its files orphaned, where `update` keeps it (row 0); - a manual entry upstream no longer ships was dropped silently. init now applies the same rules. Manual entries stay manual. Unparseable entries of any source are written back verbatim, listed in `frozenCapabilities`, and keep their records from a stamp-valid store. Dropped manual entries are named. Carried entries show as "added by hand" in the summary instead of also appearing under SKIPPED. init also reads the config before its prompts and takes the lock after them, but never re-checked it (PHARN-03's contract for add/update/remove), so a concurrent `pharn add` during the confirm was lost. It now takes a fingerprint of pharn.config.json (absent / unreadable / sha256 of the bytes) BEFORE the tolerant parse, and refuses under the lock, writing nothing, if it changed. Docs: docs/reference/pharn-config.md and the init.md config table no longer say a re-run init resets every entry to `auto`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o --- .dev/features/init-manual-carry/GRILL.md | 45 ++++ .dev/features/init-manual-carry/PLAN.md | 121 ++++++++++ .dev/features/init-manual-carry/REGRESSION.md | 39 ++++ .dev/features/init-manual-carry/REVIEW.md | 73 ++++++ .dev/features/init-manual-carry/SHIP.md | 19 ++ .dev/features/init-manual-carry/VERIFY.md | 32 +++ .../init-manual-carry/regression-report.json | 30 +++ .../init-manual-carry/verify-report.json | 18 ++ .pharn/writes-scope.json | 4 +- CHANGELOG.md | 6 + docs/commands/init.md | 13 +- docs/reference/pharn-config.md | 14 +- src/commands/init.ts | 172 ++++++++++---- src/lib/pharn-config.ts | 44 +++- src/steps/archetype-summary.ts | 8 +- src/steps/install-archetype.ts | 101 ++++++-- src/types.ts | 8 +- tests/archetype-summary.test.ts | 10 + tests/init-archetype.test.ts | 147 +++++++++++- tests/init.test.ts | 219 ++++++++++++++++-- tests/pharn-config.test.ts | 51 ++++ 21 files changed, 1072 insertions(+), 102 deletions(-) create mode 100644 .dev/features/init-manual-carry/GRILL.md create mode 100644 .dev/features/init-manual-carry/PLAN.md create mode 100644 .dev/features/init-manual-carry/REGRESSION.md create mode 100644 .dev/features/init-manual-carry/REVIEW.md create mode 100644 .dev/features/init-manual-carry/SHIP.md create mode 100644 .dev/features/init-manual-carry/VERIFY.md create mode 100644 .dev/features/init-manual-carry/regression-report.json create mode 100644 .dev/features/init-manual-carry/verify-report.json diff --git a/.dev/features/init-manual-carry/GRILL.md b/.dev/features/init-manual-carry/GRILL.md new file mode 100644 index 00000000..a187e24a --- /dev/null +++ b/.dev/features/init-manual-carry/GRILL.md @@ -0,0 +1,45 @@ +# GRILL — init-manual-carry + +Plan: `.dev/features/init-manual-carry/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live +`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +```yaml +- type: FINDING + rule_id: 'P0' + severity: important + file: '.dev/features/init-manual-carry/PLAN.md:52' + problem: 'The fingerprint and the parse are two separate reads. If the parse runs first and the file changes before the fingerprint is taken, the carry set comes from the OLD bytes while the fingerprint (and so the under-lock check) blesses the NEW ones — the concurrent write is lost with no refusal. Take the fingerprint BEFORE the parse, or derive both from one read, so any change in between fails closed.' + evidence: 'the previous `(skillsVersion, commit)` stamp, and a config fingerprint' +- type: FINDING + rule_id: 'P5' + severity: important + file: '.dev/features/init-manual-carry/PLAN.md:6' + problem: 'update''s row 0 keeps ANY frozen entry (`frozen | any source → KEEP VERBATIM`), but the plan keeps only `source: manual` ones; a frozen AUTO entry (update kept it earlier) is still dropped by a re-run init and its files orphaned. Either extend the kept set to any source or name the gap — this is a question for the human, since GATE 1 approved the manual-only wording.' + evidence: 'A manual entry whose capability upstream ships but this CLI cannot parse is KEPT as-is' +- type: FINDING + rule_id: 'P3' + severity: minor + file: '.dev/features/init-manual-carry/PLAN.md:48' + problem: 'The carry computation (manualKeys / extra / kept / gone) is pure set logic over the previous config and the index; kept inside the command it is testable only through the fully mocked runInit. Acceptable, but keep it a pure function with no I/O besides the one tolerant read so the tests can pin every bucket.' + evidence: 'replace `carriedManualCapabilities` with one tolerant read of the previous config returning' +- type: FINDING + rule_id: 'P6' + severity: minor + file: '.dev/features/init-manual-carry/PLAN.md:59' + problem: 'Records for a kept entry are selected by prefix at the NEW clone layout; if the re-run init also moves the install from flat to `pharn/`, the kept capability''s files and records sit at the old paths and nothing is carried. update behaves the same, so this is parity, but it should be named rather than implied away.' + evidence: 'merge their records from the previous store (`recordsBaseline` + `recordsUnderCapabilities`, the pair `update` uses)' +``` + +## Summary + +The plan closes a reproduced provenance bug (manual → auto) and a reproduced lost-write race, and brings +init into line with update's merge table for hand-added capabilities. The one soundness concern is the +order of the fingerprint and the parse: only fingerprint-first (or a single read) makes the new lock +check fail closed. The open design question is whether frozen AUTO entries get the same treatment as +frozen manual ones — update keeps both; the approved plan covers manual only. The other two are scope +notes. + +ADVISORY VERDICT: 4 concerns raised (0 blocking-severity, 4 advisory — 2 important) — for the human to +weigh before /pharn-dev-build. diff --git a/.dev/features/init-manual-carry/PLAN.md b/.dev/features/init-manual-carry/PLAN.md new file mode 100644 index 00000000..da35f7ac --- /dev/null +++ b/.dev/features/init-manual-carry/PLAN.md @@ -0,0 +1,121 @@ +# PLAN — init-manual-carry (a re-run init keeps `pharn add`s the way `update` does) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: PHARN-11's carry-over is made consistent with `update`'s merge table and with PHARN-03's + lock contract. (1) A `source: "manual"` entry stays `manual` even when archetype resolution also + selects it (merge row 3, sticky). (2) An entry — of ANY `source`, manual or auto (amended at + grill, see Open questions) — whose capability upstream ships but this CLI cannot parse is KEPT + as-is — config entry VERBATIM, files and records untouched, listed in `frozenCapabilities` so + `update` re-checks it — instead of silently dropped (merge row 0, `frozen | any source`). (3) A + manual entry upstream no longer ships is still dropped, but NAMED (merge row 7's `dropped-gone`). + (4) Carried entries render as "added by hand" in the summary and no longer also appear under + SKIPPED. (5) Inside its lock, init refuses (nothing written) if `pharn.config.json` changed after + init read it for the carry-over. (6) The two docs that still say re-init resets every entry to + `auto` are corrected. +- layer(s): the CLI (`src/commands/init.ts`, `src/steps/`, `src/lib/pharn-config.ts`, `src/types.ts`) + + user docs +- constitution_refs: [P1, P4, P5, P6, P7] + +## Discovery — verified this run (P6) + +- `init.ts:243-266` `carriedManualCapabilities` keeps a previous entry only if + `source === 'manual' && inIndex && !selected`, and ONLY the carried keys become `manualKeys` + (`init.ts:156`). `install-archetype.ts:135-140` stamps everything else `auto`. So a manual entry + that resolution ALSO selects is re-recorded `auto` — reproduced by review: `[spa]` + `add + griller:a11y`, project gains ssr, re-run init → a11y `auto`; a later `update` then reports + "REMOVED — no longer selected" and drops a capability the user asked for by name. update's own + table keeps it `manual` (`merge-capabilities.ts:200-205`, row 3). +- `inIndex = index.capabilities` only (`init.ts:255`). A manual entry in `index.unknown` (upstream + ships it, this CLI cannot parse it) is dropped from the config and its files orphaned; `update` + KEEPS the same entry (row 0, `kept-frozen`) with its records (`update.ts:558-580`) and + `frozenCapabilities` (`update.ts:584-600`). Reproduced by review (add lens:perf → upstream gives + perf an unparseable `applies` → `update` KEPT, re-init drops it). +- A manual entry gone from the index is dropped silently; `update` names it (`dropped-gone`). +- Display: carried entries are appended with `matched: []` (`init.ts:151`) → empty reason + (`archetype-summary.ts:16-17`), while `...resolved` (`init.ts:144`) still lists the same + capability under SKIPPED ("applies to [ssr]; detected [spa]"). +- Lock: init reads the config (`init.ts:137` → `:249`) before its prompts and takes the lock only + after them (`:197`), but never calls `assertConfigUnchanged` — `add.ts`, `update.ts:308` and + `remove.ts` all do (PHARN-03). Reproduced by review: a concurrent `pharn add lens:perf` during + init's confirm is lost when init writes its config. `assertConfigUnchanged` compares PARSED configs + and throws when the current one is unreadable (`pharn-config.ts:371-387`); init reads TOLERANTLY + (absent/corrupt → carry nothing), so it needs a comparison that also covers absent/unreadable. +- Docs: `docs/reference/pharn-config.md:58-60` ("every entry becomes `auto` and previous `manual` + tags are lost") and `docs/commands/init.md:150` ("only `pharn add` writes `manual`") contradict + `docs/commands/init.md:137-138` — a P4 contradiction. + +## Files + +- `src/commands/init.ts` — replace `carriedManualCapabilities` with one tolerant read of the + previous config returning: `manualKeys` (every manual entry the index still has, selected or not), + `extra` (those not selected — appended with `matched: 'manual'` and REMOVED from `skipped`), + `kept` (entries of any `source` in `index.unknown`), `gone` (manual entries in neither — named in a + warning), the previous `(skillsVersion, commit)` stamp, and a config fingerprint taken BEFORE the + parse (grill finding 1 — a change between the two reads must fail closed); inside + `withProjectLock`, assert the fingerprint is unchanged BEFORE `runInstallArchetype` — layer + CLI/commands +- `src/lib/pharn-config.ts` — `configFingerprint(cwd)` (`absent` | `unreadable` | sha256 of the + bytes; never throws) + `assertConfigFingerprintUnchanged(cwd, fingerprint, command)` throwing the + existing `ProjectChangedError` message — layer CLI/lib +- `src/steps/install-archetype.ts` — take a `carry` argument (`manualKeys`, `kept`, previous stamp) + in place of bare `manualKeys`; append `kept` entries verbatim to `capabilities`, write + `frozenCapabilities` (sorted keys) when non-empty, and merge their records from the previous store + (`recordsBaseline` + `recordsUnderCapabilities`, the pair `update` uses) into the new store — layer + CLI/steps +- `src/steps/archetype-summary.ts` — `describeMatched('manual')` → "added by hand" — layer CLI/steps +- `src/types.ts` — `SelectedCapability.matched: 'universal' | 'manual' | Archetype[]` — layer types +- `tests/init.test.ts` — manual+resolved → in `manualKeys` (FAILS on base); carried entry absent + from `skipped`, `matched: 'manual'`; unparseable entry (manual AND auto) → passed as `kept` + verbatim, named; gone manual → + named warning; config changed during the prompts → refusal, `runInstallArchetype` never called, + exit 1 (FAILS on base); config unchanged/absent/corrupt → proceeds +- `tests/init-archetype.test.ts` — `kept` entry written verbatim + `frozenCapabilities`, its records + carried from a stamp-valid store, dropped from a stale/absent one (never minted) +- `tests/archetype-summary.test.ts` — a `'manual'` entry renders "added by hand" +- `tests/pharn-config.test.ts` — fingerprint: absent / bytes / unreadable (directory); assert throws + `ProjectChangedError` on any change, including absent→present +- `docs/commands/init.md` — the re-run paragraph + the config table row describe (1)-(5) +- `docs/reference/pharn-config.md` — replace the "start-over … every entry becomes `auto`" note +- `CHANGELOG.md` — `[Unreleased]` → `### Fixed` entries + +## Contracts satisfied + +- `lib/merge-capabilities.ts` decision table rows 0, 3, 7 — init now agrees with them (cited, not + restated, P4). Legacy `source`-absent entries are still NOT carried: the merge is "the ONLY place + absence may be resolved" (CLAUDE.md), and `docs/commands/init.md` already limits the carry to + `source: "manual"`. +- PHARN-03's "re-check the config under the lock" — extended to init. + +## Evals to write (P1) + +- listed under Files; the manual+resolved and config-changed cases fail on 0ca29b7. + +## Guarantee audit (P0) + +- "a manual entry the index still has is recorded manual" → floor: set membership over `role:name`. +- "init writes nothing if the config changed after it was read" → floor: sha256 content-hash + compare of the file bytes (or the absent/unreadable sentinel) inside the lock, before the first + write (the backup included). +- "a kept entry's records are carried" → floor: `recordsBaseline`'s stamp compare; a stale/absent + store carries nothing (never minted, never blessed). + +## Trust audit (P2) + +- The previous config and records are local, user-editable input, already validated at ingest + (`readPharnConfig`, `readRecords`); no new field is trusted. Upstream names in `index.unknown` are + only used as set keys against config entries that passed `CAPABILITY_NAME_RE`. + +## Determinism audit (P5) + +- Every branch is a `role:name` set lookup or an exact enum compare; the lock check is a string + equality over fingerprints. + +## Open questions (HALT) + +- none. RESOLVED at GATE 1 (2026-09-24, by the human): (2) a hand-added capability that upstream + still ships but this CLI cannot parse is KEPT as-is, like `update` — "Leave it alone", the + recommended option — not dropped with a warning. The plan was accepted as written. +- AMENDED after `/pharn-dev-grill` (2026-09-24, by the human): grill finding 2 asked whether an + AUTOMATIC (`source: auto`, or absent) entry that upstream ships but this CLI cannot parse gets the + same treatment; the human chose "Yes, leave them alone too" — so (2) keeps a frozen entry of ANY + source verbatim, exactly as `update`'s row 0 does. Files unchanged. diff --git a/.dev/features/init-manual-carry/REGRESSION.md b/.dev/features/init-manual-carry/REGRESSION.md new file mode 100644 index 00000000..5bda36b0 --- /dev/null +++ b/.dev/features/init-manual-carry/REGRESSION.md @@ -0,0 +1,39 @@ +# REGRESSION — init-manual-carry + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `5cf6ce3451697f83589d057bc3783ea364b08b94` (`HEAD` — `origin/main` after #213; the build is an + uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `src/commands/init.ts`, `src/lib/pharn-config.ts`, + `src/steps/install-archetype.ts`, `src/steps/archetype-summary.ts`, `src/types.ts`, `tests/init.test.ts`, + `tests/init-archetype.test.ts`, `tests/archetype-summary.test.ts`, `tests/pharn-config.test.ts`, + `docs/commands/init.md`, `docs/reference/pharn-config.md`, `CHANGELOG.md`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook scratch) and + stage artifacts are not build output — this feature's own `PLAN.md` / `GRILL.md`, and the untracked + `PLAN.md` files of the two increments still queued (`update-frozen-recheck`, `tar-entry-names`). +- **outside gates:** the 46 stdlib `*.test.mjs` / `*.test.cjs` files `scope` returned (754 tests) + + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / `lint:md` are + absent from both maps. +- **environment:** both sides ran with no proxy variables and as root **without** `CAP_DAC_OVERRIDE` / + `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`), the CI-equivalent of this root sandbox. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` is +owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never the increment. diff --git a/.dev/features/init-manual-carry/REVIEW.md b/.dev/features/init-manual-carry/REVIEW.md new file mode 100644 index 00000000..81ba35e5 --- /dev/null +++ b/.dev/features/init-manual-carry/REVIEW.md @@ -0,0 +1,73 @@ +# REVIEW — init-manual-carry + +Increment: `src/commands/init.ts` (`readPreviousConfig` — fingerprint first, then a tolerant parse; +pure `carryOver`; the under-lock `assertConfigFingerprintUnchanged`), `src/lib/pharn-config.ts` +(`configFingerprint`, `assertConfigFingerprintUnchanged`, shared `configChangedError`), +`src/steps/install-archetype.ts` (`InstallCarry`; kept entries verbatim, `frozenCapabilities`, carried +records), `src/steps/archetype-summary.ts` + `src/types.ts` (`matched: 'manual'` → "added by hand"), +four test files, `docs/commands/init.md`, `docs/reference/pharn-config.md`, `CHANGELOG.md`. Treated as +`trust: untrusted`; nothing in it read as an instruction. + +## Floor first (P0) + +`node .dev/floor/validate.mjs .` → `FLOOR: GREEN` (exit 0). `/pharn-dev-build`'s `npm run check` exit 0 +(1486 tests), `/pharn-dev-regress` `no-regressions`, `/pharn-dev-verify` `PASS`. + +## Floor-gate findings (blocking) + +None. + +- L-floor (P0): each claim reduces to a floor primitive. "A manual entry the index still has is recorded + `manual`" → `role:name` set membership (`carryOver`). "init writes nothing if the config changed after + it was read" → a sha256 content-hash compare (or the `absent` / `unreadable:` sentinel) inside the + lock, before `runInstallArchetype` and so before the backup. The fingerprint is taken before the parse, + so a change between the two reads fails closed (grill finding 1). "A kept entry's records are carried" + → `recordsBaseline`'s stamp compare, and a stale or absent store carries nothing. The only unclaimed + residual is an edit made DURING the install, inside the lock — the same window every other writer has. +- L-eval (P1): every behavior has a test that fails on the base source (8 in `tests/init.test.ts`, checked + against the stashed base `src/`): sticky manual, the carried entry shown once, kept entries of every + source passed verbatim, the gone warning, the corrupt-config carry, and both refusal paths + (rewritten; appeared). The fixture tests pin the written config, `frozenCapabilities` and the three + record-carry cases; `tests/pharn-config.test.ts` pins the fingerprint's sentinels. +- L-trust (P2): kept entries come from the local config, already validated at ingest + (`CAPABILITY_NAME_RE`, `ROLE_VALUES`), and are written back verbatim. Upstream names from + `index.unknown` are only set keys, matched against those validated entries. Every printed key comes + from a validated entry. +- L-axis (P3): no sibling reference. The command imports a type from the step it already calls (the + allowed direction). + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CLAUDE.md:54' + problem: 'CLAUDE.md still describes the re-run carry as manual entries only, via `manualKeys`; it now also keeps unparseable entries of any source (frozenCapabilities + records) and re-checks the config fingerprint under the lock. CLAUDE.md is outside this plan''s Files, so it needs a follow-up edit.' + evidence: 'to keep its `source: ''manual''` entries that the index still has — installed again and recorded `manual` via `manualKeys`' +- type: FINDING + rule_id: 'P3' + severity: minor + file: 'src/commands/init.ts:311' + problem: 'carryOver is pure membership logic sitting in a command, so it is tested only through the fully mocked runInit; a lib module beside merge-capabilities would allow direct table tests. Accepted at grill (finding 3); noted for when the table grows.' + evidence: 'function carryOver(' +- type: FINDING + rule_id: 'P1' + severity: minor + file: 'src/commands/init.ts:326' + problem: 'The first-entry-wins rule for a duplicated `role:name` in the previous config has no test; it mirrors the merge, but only the merge''s copy of the rule is pinned.' + evidence: 'if (seen.has(k)) continue;' +``` + +## Proposed lesson for canon (NOT written — for a human-gated `/pharn-dev-memory-promote`) + +- **Candidate:** "A command that reads shared state before a prompt and writes it after must re-check + that state under the lock (PHARN-03). Adding a pre-prompt read to a command that had none (PHARN-11 gave + `init` a config read for its carry-over) silently re-opens the lost-write race." +- **Provenance:** increment `init-manual-carry`; the gap entered with PHARN-11 (8d11906, #205), a few + commits after PHARN-03 (71bc375, #197) set the contract for `add`/`update`/`remove`; found by the + 18-commit review on 2026-09-24; closed by this diff (`src/commands/init.ts:224`). + +## Verdict + +**GREEN — 0 floor-gate findings, 3 advisory (minor).** The standing decision is the human's (GATE 2). diff --git a/.dev/features/init-manual-carry/SHIP.md b/.dev/features/init-manual-carry/SHIP.md new file mode 100644 index 00000000..c8fd3418 --- /dev/null +++ b/.dev/features/init-manual-carry/SHIP.md @@ -0,0 +1,19 @@ +# SHIP — init-manual-carry + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: accepted — "deliver all of them one by one using +pharn-dev-ship"; open question answered "Leave it alone") → `/pharn-dev-grill` → plan amended by the human +(grill finding 2: "Yes, leave them alone too" — keep unparseable entries of ANY source) → +`/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review` → GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- Run ended at **GATE 2**. The human's standing instruction for this batch: after each increment, open a + pull request and merge it once its checks are green, then start the next plan. + +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. diff --git a/.dev/features/init-manual-carry/VERIFY.md b/.dev/features/init-manual-carry/VERIFY.md new file mode 100644 index 00000000..c7ee4362 --- /dev/null +++ b/.dev/features/init-manual-carry/VERIFY.md @@ -0,0 +1,32 @@ +# VERIFY — init-manual-carry + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, on node 22 with the session proxy variables unset +and as root **without** `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`) — the +CI-equivalent of this root sandbox. + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `test:floor` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +`test` is vitest (1486 tests), which collects this increment's own tests (`tests/init.test.ts`, +`tests/init-archetype.test.ts`, `tests/archetype-summary.test.ts`, `tests/pharn-config.test.ts`). +`test:floor` is floor.yml's `node --test` run (754 tests), kept in the map for parity with the previous +increment. No `structural:*` gate — the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — no verifiers registered, floor +gates only. + +Residual (P0/P7): verified = the named gates passed; this is NOT a guarantee of correctness beyond what those +gates check — verifier concerns are advisory help, not assurance. diff --git a/.dev/features/init-manual-carry/regression-report.json b/.dev/features/init-manual-carry/regression-report.json new file mode 100644 index 00000000..f0dadcd2 --- /dev/null +++ b/.dev/features/init-manual-carry/regression-report.json @@ -0,0 +1,30 @@ +{ + "base": "5cf6ce3451697f83589d057bc3783ea364b08b94", + "inside": [ + "CHANGELOG.md", + "docs/commands/init.md", + "docs/reference/pharn-config.md", + "src/commands/init.ts", + "src/lib/pharn-config.ts", + "src/steps/archetype-summary.ts", + "src/steps/install-archetype.ts", + "src/types.ts", + "tests/archetype-summary.test.ts", + "tests/init-archetype.test.ts", + "tests/init.test.ts", + "tests/pharn-config.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/init-manual-carry/verify-report.json b/.dev/features/init-manual-carry/verify-report.json new file mode 100644 index 00000000..14a1855c --- /dev/null +++ b/.dev/features/init-manual-carry/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "init-manual-carry", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "test:floor": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index dc6569c7..8f0eb51a 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/publish-pack-destination/SHIP.md" + ".dev/features/init-manual-carry/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T18:52:09.219Z" + "set_at": "2026-09-24T19:08:58.340Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index ba76948a..264e5f71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Releases could not publish: the `Pack` step wrote into a directory nothing had created.** Since the release workflow was split into an unprivileged `build` job and a `publish` job, `build` ran `npm pack --pack-destination "$RUNNER_TEMP/pkg"` without creating `pkg/`, and npm does not create it (`ENOENT` on npm 10 and 11). Every Release run would have failed at `Pack` and never reached `publish`. The step now runs `mkdir -p` first, and a live test pins that every workflow's pack destination is created earlier in the job that packs. +- **A re-run `pharn init` no longer loses what `pharn update` would keep.** It applies `update`'s own rules to the config it replaces: + - A capability you added by hand stays `manual` even when your archetypes now select it too. It used to be recorded `auto`, so a later archetype change let `update` drop it. + - A capability upstream still ships but this pharn cannot read is left as it is, however it was added. Its config entry, files and records are untouched, and it is listed in `frozenCapabilities`. It used to be dropped from the config and its files orphaned. + - A hand-added capability upstream no longer ships is still dropped, but now named. + - Carried entries show as "added by hand" in the install summary, not also under SKIPPED. +- **`pharn init` no longer overwrites a config another `pharn` command wrote while its prompts were open.** It now re-checks `pharn.config.json` under its lock, like `add`, `update` and `remove`. If the file changed after `init` read it, `init` refuses and writes nothing. ## [0.5.0] - 2026-09-10 diff --git a/docs/commands/init.md b/docs/commands/init.md index 0d83be73..046d7a77 100644 --- a/docs/commands/init.md +++ b/docs/commands/init.md @@ -135,8 +135,15 @@ edits — are listed **first** and marked `(edited)`, and the prompt says how ma continue, `init` copies each of them to `.pharn-backup//` **before** the first write and prints that directory as soon as it is created (byte-identical files are not edits and are not backed up). Capabilities you added by hand with `pharn add` (`source: "manual"` in `pharn.config.json`) are **kept**: -`init` installs them again and records them as `manual`, as long as upstream still ships them. An -unreadable or invalid existing config never blocks `init` — nothing is carried over from it. The target set is derived from the fetched clone's layout + your resolved selection (`lib/install-manifest.ts`), so it is exact — not a git-history heuristic. +`init` installs them again and records them as `manual` — also when your archetypes now select them +too — as long as upstream still ships them. The summary lists them as "added by hand". One that upstream +no longer ships is dropped from `pharn.config.json` and named (its files stay on disk). A capability +upstream still ships but this pharn [cannot read](update.md#when-pharn-cannot-read-a-capability-upstream) +is **left as it is**, however it was added: its config entry, files and records are untouched, and it is +listed in `frozenCapabilities` so the next [`pharn update`](update.md) re-checks it. An unreadable or +invalid existing config never blocks `init` — nothing is carried over from it. If `pharn.config.json` +changes while `init` waits at its prompts (another `pharn` command wrote it), `init` refuses and writes +nothing; re-run it. The target set is derived from the fetched clone's layout + your resolved selection (`lib/install-manifest.ts`), so it is exact — not a git-history heuristic. ### 7. Install @@ -147,7 +154,7 @@ unreadable or invalid existing config never blocks `init` — nothing is carried | Preserve settings | An existing `.claude/settings.json` is **never** overwritten (a note tells you to wire the hooks by hand if needed) | | Mirror the layout | Whichever layout the fetched clone uses is mirrored verbatim; the CLI never rewrites copied file contents. Today that is `pharn/pharn-contracts/`, `pharn/pharn-core/`, `pharn/floor/`; the legacy flat layout is `pharn-contracts/`, `pharn-core/`, `.dev/floor/` | | Pin commit SHA | Best-effort (the SHA the tree was pinned to; `null` if unavailable) | -| Write `pharn.config.json` | `pharnVersion`, `skillsVersion` (from the repo's `SKILLS_VERSION`), `repo`, `commit`, `installedAt`, `archetypes`, `capabilities` (each stamped `source: "auto"` — only `pharn add` writes `manual`), `layout`, `models`, `seam`, `modules: []` | +| Write `pharn.config.json` | `pharnVersion`, `skillsVersion` (from `SKILLS_VERSION`), `repo`, `commit`, `installedAt`, `archetypes`, `capabilities` (`source: "auto"`, or `"manual"` for a kept `pharn add`), `layout`, `models`, `seam`, `modules: []`, `frozenCapabilities` (see above) | | Write `pharn.records.json` | A sha256 of every file the install wrote, so [`pharn update`](update.md) can keep your later edits ([reference](../reference/pharn-records.md)) | The install also copies pharn-oss's Apache-2.0 `LICENSE` — to `pharn/LICENSE`, or `PHARN-LICENSE` at diff --git a/docs/reference/pharn-config.md b/docs/reference/pharn-config.md index 15690d89..2118ebca 100644 --- a/docs/reference/pharn-config.md +++ b/docs/reference/pharn-config.md @@ -55,10 +55,14 @@ A `source` present but outside `{auto, manual}` is a hand-edit error: `pharn` re (`capabilities[2].source`) and exits, rather than falling back to "run `pharn init`". Deleting the field is a valid fix — the next update sets it. -> Re-running `pharn init` on an existing project is an explicit **start-over**: it rewrites -> `capabilities` from scratch, so every entry becomes `auto` and previous `manual` tags are lost. -> `init` warns before overwriting `pharn.config.json` and defaults to **No**. Use `pharn update` to -> refresh an existing install; `init` is for installing one. +> Re-running `pharn init` on an existing project rebuilds `capabilities` from your archetypes, with +> two things carried over by `pharn update`'s own rules: a `manual` entry upstream still ships stays +> `manual` (and is installed again), and an entry of any `source` whose capability upstream ships but +> this pharn cannot read is kept exactly as it was (and listed in `frozenCapabilities`). A `manual` +> entry upstream no longer ships is dropped and named. An entry with **no** `source` is not carried — +> it is re-resolved from the archetypes like an `auto` one. `init` warns before overwriting +> `pharn.config.json` and defaults to **No**. Use `pharn update` to refresh an existing install; +> `init` is for installing one. A sibling file, [`pharn.records.json`](pharn-records.md), holds a sha256 per installed file. It is written by the same operations that write this config and is **stamped** with this file's @@ -75,7 +79,7 @@ When the only skipped files were ones you edited (`modified` / `unrecorded`), it value that is not a `x.y.z` version is ignored. `frozenCapabilities` lists (`role:name`, sorted) the installed capabilities the last `pharn update` -kept because it could not read them upstream. While it is non-empty, `pharn update` re-fetches even +(or re-run `pharn init`) kept because it could not read them upstream. While it is non-empty, `pharn update` re-fetches even at the same skills version, so they are re-checked on every run; the field is removed once none are left. If you `pharn remove` one, the next update drops it from the list. A value that is not a list of `role:name` keys is ignored. diff --git a/src/commands/init.ts b/src/commands/init.ts index 701c8af5..4ef9bd3d 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -22,11 +22,19 @@ import { proxyNoticeMessage } from '../lib/proxy-env-format.js'; import { runGitPrereq } from '../steps/prereqs.js'; import { confirmWriteTargets } from '../steps/overwrite-check.js'; import { runArchetypeSummary } from '../steps/archetype-summary.js'; -import { runInstallArchetype } from '../steps/install-archetype.js'; -import { readPharnConfig } from '../lib/pharn-config.js'; +import { + runInstallArchetype, + type InstallCarry, +} from '../steps/install-archetype.js'; +import { + assertConfigFingerprintUnchanged, + configFingerprint, + readPharnConfig, +} from '../lib/pharn-config.js'; import type { CapabilityIndex, InstalledCapability, + PharnConfig, Selection, } from '../types.js'; @@ -131,29 +139,43 @@ async function runInitArchetype(): Promise { const unknownWarning = unknownCapabilitiesWarning(index.unknown); if (unknownWarning) log.warn(unknownWarning); const resolved = resolveCapabilities(archetypes, index); - // A re-run init must not silently drop what the user added by hand - // (`pharn add` → `source: 'manual'`): carry those entries over, install - // them with the rest, and keep their provenance. - const manual = carriedManualCapabilities(cwd, index, resolved); - if (manual.length) { + // A re-run init must not silently drop what the user added by hand, nor + // what this CLI merely could not READ upstream: the config it replaces is + // carried over by `update`'s own merge rules (carryOver, below). + const previous = readPreviousConfig(cwd); + const carry = carryOver(previous.config, index, resolved); + if (carry.extra.length) { + log.info( + `Keeping ${carry.extra.length} capabilit${carry.extra.length === 1 ? 'y' : 'ies'} you added by hand: ${carry.extra.map(capabilityKey).join(', ')}.`, + ); + } + if (carry.kept.length) { log.info( - `Keeping ${manual.length} capabilit${manual.length === 1 ? 'y' : 'ies'} you added by hand: ${manual.map((c) => `${c.role}:${c.name}`).join(', ')}.`, + `Keeping ${carry.kept.length} capabilit${carry.kept.length === 1 ? 'y' : 'ies'} as installed — upstream still ships ${carry.kept.length === 1 ? 'it' : 'them'} but this pharn cannot read ${carry.kept.length === 1 ? 'it' : 'them'} (see above): ${carry.kept.map(capabilityKey).join(', ')}. Files and config entries are left as they are; \`pharn update\` re-checks them.`, + ); + } + if (carry.gone.length) { + log.warn( + `Not keeping ${carry.gone.length} capabilit${carry.gone.length === 1 ? 'y' : 'ies'} you added by hand that upstream no longer ships: ${carry.gone.map(capabilityKey).join(', ')}. ${carry.gone.length === 1 ? 'It is' : 'They are'} dropped from pharn.config.json; the files stay on disk.`, ); } - const selection: Selection = manual.length + const extraKeys = new Set(carry.extra.map(capabilityKey)); + const selection: Selection = carry.extra.length ? { - ...resolved, selected: [ ...resolved.selected, - ...manual.map((c) => ({ + ...carry.extra.map((c) => ({ name: c.name, role: c.role, - matched: [], + matched: 'manual' as const, })), ], + // Installed now, so no longer "skipped" by the archetypes. + skipped: resolved.skipped.filter( + (s) => !extraKeys.has(capabilityKey(s)), + ), } : resolved; - const manualKeys = new Set(manual.map((c) => `${c.role}:${c.name}`)); const action = await runArchetypeSummary(archetypes, selection); // Both prompts return a VALUE and neither exits, so `outcome` stays @@ -194,16 +216,21 @@ async function runInitArchetype(): Promise { // still pays the full download before being refused. Accepted — init is // the bootstrap command, run once, interactively, and it is where a // concurrent-writer collision is least likely. - await withProjectLock(cwd, 'init', () => - runInstallArchetype( + await withProjectLock(cwd, 'init', () => { + // PHARN-03's re-check, for init. The carry-over was computed from the + // config as it was BEFORE both prompts; a pharn run that wrote it since + // (a concurrent `pharn add`) would be silently overwritten. Refuse + // instead — before the first write, the backup included. + assertConfigFingerprintUnchanged(cwd, previous.fingerprint, 'init'); + return runInstallArchetype( repo.dir, cwd, archetypes, selection, commit, - manualKeys, - ), - ); + carry, + ); + }); outcome = 'installed'; } } @@ -230,38 +257,93 @@ async function runInitArchetype(): Promise { if (outcome === 'cancelled') cancelAndExit(); } +/** A capability's identity everywhere in this CLI: the `role:name` pair. */ +function capabilityKey(cap: { name: string; role: string }): string { + return `${cap.role}:${cap.name}`; +} + /** - * The `source: 'manual'` capabilities of the config a re-run init is about to - * replace that the fetched index still has and archetype resolution did not - * already select. Read TOLERANTLY: init is the command every other one points - * at for recovery, so an absent, unreadable, invalid or pre-archetype config - * simply means "nothing to carry over" — never a refusal. A manual entry the - * index no longer has is not resurrected (the `dropped-gone` rule `update` - * applies in lib/merge-capabilities.ts). + * The config a re-run init is about to replace, read TOLERANTLY: init is the + * command every other one points at for recovery, so an absent, unreadable, + * invalid or pre-archetype config simply means "nothing to carry over" — never + * a refusal. The fingerprint is taken FIRST, so a write that lands between the + * two reads fails the under-lock check (closed) instead of being blessed by it. */ -function carriedManualCapabilities( - cwd: string, - index: CapabilityIndex, - resolved: Selection, -): InstalledCapability[] { - let previous: InstalledCapability[]; +function readPreviousConfig(cwd: string): { + fingerprint: string; + config: PharnConfig | null; +} { + const fingerprint = configFingerprint(cwd); + let config: PharnConfig | null; try { - previous = readPharnConfig(cwd)?.capabilities ?? []; + config = readPharnConfig(cwd); } catch { - return []; + config = null; } - const key = (c: { name: string; role: string }): string => - `${c.role}:${c.name}`; - const inIndex = new Set(index.capabilities.map(key)); - const selected = new Set(resolved.selected.map(key)); + return { fingerprint, config }; +} + +interface Carry extends InstallCarry { + // Manual entries the index has but the archetypes did not select — added to + // the selection so they are installed again (row 6). + extra: InstalledCapability[]; + // Manual entries upstream no longer ships — dropped and NAMED (row 7). + gone: InstalledCapability[]; +} + +/** + * What a re-run init carries over from the previous config, by `update`'s own + * membership table (lib/merge-capabilities.ts — cited, not restated, P4). Pure: + * every branch is a `role:name` set lookup or an exact `source` compare (P5). + * + * - Row 0: an entry of ANY source whose capability the fetch could not PARSE + * (`index.unknown`) is KEPT verbatim — a parse failure is evidence about + * upstream's bytes, not about who asked for it. + * - Rows 3 and 6: a `manual` entry the index still has stays `manual`, whether + * or not the archetypes also select it (sticky). + * - Row 7: a `manual` entry the index no longer has is dropped — and named. + * - Everything else (`auto`, or a legacy entry with no `source`) is simply + * re-resolved from the archetypes: init starts over. Resolving an ABSENT + * source is the merge's job alone, so it is not inferred here. + * + * The first entry wins for a duplicated key, as in the merge. + */ +function carryOver( + previous: PharnConfig | null, + index: CapabilityIndex, + resolved: Selection, +): Carry { + const inIndex = new Set(index.capabilities.map(capabilityKey)); + const frozen = new Set(index.unknown.map(capabilityKey)); + const selected = new Set(resolved.selected.map(capabilityKey)); + const manualKeys = new Set(); + const kept: InstalledCapability[] = []; + const extra: InstalledCapability[] = []; + const gone: InstalledCapability[] = []; const seen = new Set(); - return previous.filter((c) => { - const k = key(c); - if (c.source !== 'manual' || !inIndex.has(k) || selected.has(k)) { - return false; - } - if (seen.has(k)) return false; + for (const cap of previous?.capabilities ?? []) { + const k = capabilityKey(cap); + if (seen.has(k)) continue; seen.add(k); - return true; - }); + if (frozen.has(k)) { + kept.push({ ...cap }); + continue; + } + if (cap.source !== 'manual') continue; + if (!inIndex.has(k)) { + gone.push(cap); + continue; + } + manualKeys.add(k); + if (!selected.has(k)) extra.push(cap); + } + return { + manualKeys, + kept, + extra, + gone, + previousStamp: previous + ? { skillsVersion: previous.skillsVersion, commit: previous.commit } + : null, + }; } diff --git a/src/lib/pharn-config.ts b/src/lib/pharn-config.ts index a3d12fc1..ea19adfc 100644 --- a/src/lib/pharn-config.ts +++ b/src/lib/pharn-config.ts @@ -1,3 +1,4 @@ +import { createHash } from 'node:crypto'; import { existsSync, readFileSync } from 'node:fs'; import { writeJsonAtomic } from './atomic-write.js'; import { resolve } from 'node:path'; @@ -381,11 +382,52 @@ export function assertConfigUnchanged( } if (current !== null && JSON.stringify(current) === JSON.stringify(snapshot)) return; - throw new ProjectChangedError( + throw configChangedError(command); +} + +function configChangedError(command: string): ProjectChangedError { + return new ProjectChangedError( `${CONFIG_FILENAME} changed while \`pharn ${command}\` was running — another pharn process (or an edit) wrote it after this command read it. Nothing was written. Re-run \`pharn ${command}\` to act on the current state.`, ); } +/** + * What `pharn.config.json` is on disk right now, as one comparable string: + * `absent`, `unreadable:`, or `sha256:` of the raw bytes. + * Never throws. + * + * `assertConfigUnchanged` compares two PARSES and counts an unreadable file as + * changed, which is right for `add`/`update`/`remove` (they refuse an unreadable + * config up front). `init` is the one command that reads the config it replaces + * TOLERANTLY — absent, corrupt and unreadable all mean "carry nothing over" — so + * "what init read" must be expressible for every one of those states, and a + * content hash of the bytes is (P0: content-hash, `ARCHITECTURE.md §2`). + */ +export function configFingerprint(cwd: string): string { + let bytes: Buffer; + try { + bytes = readFileSync(configPath(cwd)); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + return code === 'ENOENT' ? 'absent' : `unreadable:${code ?? 'unknown'}`; + } + return `sha256:${createHash('sha256').update(bytes).digest('hex')}`; +} + +/** + * `init`'s under-the-lock re-check: refuse, with the same `ProjectChangedError` + * the other writers raise, when `pharn.config.json` is no longer what init saw + * before its prompts (`configFingerprint`, taken BEFORE the tolerant parse, so a + * change between the two reads fails closed rather than being blessed). + */ +export function assertConfigFingerprintUnchanged( + cwd: string, + fingerprint: string, + command: string, +): void { + if (configFingerprint(cwd) !== fingerprint) throw configChangedError(command); +} + /** * Is `err` a present-but-invalid-config error — a file that IS there and that * the user has to fix (unparseable JSON, a hand-edited `models`/`seam` block, or diff --git a/src/steps/archetype-summary.ts b/src/steps/archetype-summary.ts index c80aee4f..ec37e91d 100644 --- a/src/steps/archetype-summary.ts +++ b/src/steps/archetype-summary.ts @@ -13,8 +13,12 @@ import type { Archetype, Selection } from '../types.js'; export type ArchetypeSummaryAction = 'install' | 'cancel'; -function describeMatched(matched: 'universal' | Archetype[]): string { - return matched === 'universal' ? 'universal' : matched.join(', '); +function describeMatched( + matched: 'universal' | 'manual' | Archetype[], +): string { + if (matched === 'universal') return 'universal'; + if (matched === 'manual') return 'added by hand'; + return matched.join(', '); } export async function runArchetypeSummary( diff --git a/src/steps/install-archetype.ts b/src/steps/install-archetype.ts index 1841ded8..426fc978 100644 --- a/src/steps/install-archetype.ts +++ b/src/steps/install-archetype.ts @@ -6,7 +6,14 @@ import { collectExpectedInstallPaths } from '../lib/install-manifest.js'; import { detectLayout, layoutPaths } from '../lib/layout.js'; import { scanDest } from '../lib/dest-drift.js'; import { createBackup } from '../lib/backup.js'; -import { buildRecords, writeRecords } from '../lib/install-records.js'; +import { + buildRecords, + readRecords, + recordsBaseline, + recordsUnderCapabilities, + writeRecords, + type FileRecords, +} from '../lib/install-records.js'; import { DEFAULT_MODEL_ROUTING } from '../lib/model-routing.js'; import { formatModelRoutingLines } from '../lib/model-routing-format.js'; import { DEFAULT_SEAM_CONFIG } from '../lib/seam-config.js'; @@ -21,6 +28,33 @@ import type { Selection, } from '../types.js'; +/** + * What a re-run `init` carries over from the config it replaces (built by + * commands/init.ts, which reads that config tolerantly). Empty for a first + * install. + */ +export interface InstallCarry { + // `role:name` of every capability the user added by hand (`pharn add` → + // `source: 'manual'`) that the fetched index still has — selected by the + // archetypes or not. Recorded `manual`, so `pharn update` keeps them (its merge + // table, rows 3 and 6); every other installed entry is `auto`. + manualKeys: ReadonlySet; + // Previous entries, of ANY source, whose capability upstream still ships but + // this CLI could not PARSE (`index.unknown`) — `update`'s row 0. Nothing is + // copied for them: each is written back VERBATIM, listed in + // `frozenCapabilities` so `update` re-checks it, and keeps its records. + kept: readonly InstalledCapability[]; + // The (skillsVersion, commit) of the config being replaced — the stamp the + // records store must carry for a kept entry's records to be trusted. + previousStamp: { skillsVersion: string; commit: string | null } | null; +} + +const NO_CARRY: InstallCarry = { + manualKeys: new Set(), + kept: [], + previousStamp: null, +}; + // The archetype-install apply stage (pharn init --archetype). Copies the // resolved capabilities + product surfaces from the already-fetched `repoDir`, // then writes pharn.config.json. Network-free (the commit SHA is passed in by @@ -32,10 +66,7 @@ export async function runInstallArchetype( archetypes: Archetype[], selection: Selection, commit: string | null, - // `role:name` keys of capabilities the user added by hand in the config this - // install replaces (commands/init.ts carries them over). Recorded `manual`, - // so `pharn update` keeps them — every other entry is `auto`. - manualKeys: ReadonlySet = new Set(), + carry: InstallCarry = NO_CARRY, ): Promise { const startedAt = Date.now(); @@ -112,6 +143,9 @@ export async function runInstallArchetype( ); } + const frozenCapabilities = [ + ...new Set(carry.kept.map((c) => `${c.role}:${c.name}`)), + ].sort(); const config: PharnConfig = { pharnVersion: PHARN_VERSION, skillsVersion, @@ -129,18 +163,26 @@ export async function runInstallArchetype( // An entry from archetype resolution is `auto` — `pharn update` owns it and // may drop it when the archetypes stop selecting it. `manual` is what // `pharn add` writes, and what a re-run init carries over from the config it - // replaces (`manualKeys`), so a hand-added capability survives. Tagged at + // replaces (`carry.manualKeys` — sticky even when the archetypes ALSO select + // it, as in update's merge), so a hand-added capability survives. Tagged at // the WRITE site so lib/install-capabilities.ts (the copy routine) stays - // unaware of provenance, which is not its axis (P3). - capabilities: capabilities.map((c) => ({ - ...c, - source: manualKeys.has(`${c.role}:${c.name}`) - ? ('manual' as const) - : ('auto' as const), - })), + // unaware of provenance, which is not its axis (P3). Kept (frozen) entries + // follow VERBATIM — a parse failure upstream is not evidence of provenance. + capabilities: [ + ...capabilities.map((c) => ({ + ...c, + source: carry.manualKeys.has(`${c.role}:${c.name}`) + ? ('manual' as const) + : ('auto' as const), + })), + ...carry.kept.map((c) => ({ ...c })), + ], // The layout mirrored from the fetched clone (flat OR pharn/) — status/remove // read this back to address the project the same way (lib/layout.ts). layout, + // While non-empty, `pharn update` re-fetches even at the same skills version, + // so a kept capability is re-checked once this CLI can read it (types.ts). + ...(frozenCapabilities.length > 0 ? { frozenCapabilities } : {}), }; // Record sha256 of every file this install just wrote (hashed at the DEST, so // the record cannot disagree with what landed) BEFORE the config, and stamped @@ -150,10 +192,13 @@ export async function runInstallArchetype( await writeRecords(cwd, { skillsVersion, commit, - files: buildRecords( - cwd, - collectExpectedInstallPaths({ repoDir, capabilities, layout }).keys(), - ), + files: { + ...keptRecords(cwd, carry, layout), + ...buildRecords( + cwd, + collectExpectedInstallPaths({ repoDir, capabilities, layout }).keys(), + ), + }, }); await writePharnConfig(cwd, config); @@ -198,3 +243,25 @@ export async function runInstallArchetype( ].join('\n'), ); } + +/** + * The records of every kept (frozen) capability, carried from the store this + * install replaces — the pair `update` uses for its frozen entries + * (`recordsBaseline` + `recordsUnderCapabilities`). A store that is absent, + * unreadable, or stamped for a different install state carries NOTHING: never + * minted, never blessed. Nothing under a kept capability was touched, so the + * hashes it does carry are still true; dropping them would make the next + * `update` read those files as `unrecorded` and skip them. Keyed at the clone's + * layout, as in `update`. + */ +function keptRecords( + cwd: string, + carry: InstallCarry, + layout: Layout, +): FileRecords { + if (carry.kept.length === 0 || carry.previousStamp === null) return {}; + const { records } = recordsBaseline(readRecords(cwd), carry.previousStamp); + return records === null + ? {} + : recordsUnderCapabilities(records, layoutPaths(layout), carry.kept); +} diff --git a/src/types.ts b/src/types.ts index e6558156..2f53ddd3 100644 --- a/src/types.ts +++ b/src/types.ts @@ -237,12 +237,14 @@ export interface CapabilityIndex { unknown: UnknownCapability[]; } -// A capability chosen for install. `matched` is why: 'universal', or the -// detected archetypes that intersected its `applies` set. +// A capability chosen for install. `matched` is why: 'universal', the detected +// archetypes that intersected its `applies` set, or 'manual' — a re-run `init` +// carrying over a capability the user added by hand (`pharn add`) that the +// archetypes did not select. export interface SelectedCapability { name: string; role: 'griller' | 'lens'; - matched: 'universal' | Archetype[]; + matched: 'universal' | 'manual' | Archetype[]; } // A capability left out, with a deterministic, human-readable reason. diff --git a/tests/archetype-summary.test.ts b/tests/archetype-summary.test.ts index 0a1a8bf9..f489044b 100644 --- a/tests/archetype-summary.test.ts +++ b/tests/archetype-summary.test.ts @@ -59,6 +59,16 @@ describe('runArchetypeSummary', () => { expect(note).toContain('migrations (griller)'); }); + it('says why a carried-over capability is selected: added by hand', async () => { + vi.mocked(prompts.select).mockResolvedValue('install'); + await runArchetypeSummary(archetypes, { + selected: [{ name: 'perf', role: 'lens', matched: 'manual' }], + skipped: [], + }); + const note = vi.mocked(prompts.note).mock.calls.at(-1)![0] as string; + expect(note).toMatch(/perf \(lens\)\s+added by hand/); + }); + it('omits the skipped block when nothing was skipped', async () => { vi.mocked(prompts.select).mockResolvedValue('install'); await runArchetypeSummary(archetypes, { diff --git a/tests/init-archetype.test.ts b/tests/init-archetype.test.ts index 166c0e1c..a0a021ec 100644 --- a/tests/init-archetype.test.ts +++ b/tests/init-archetype.test.ts @@ -27,7 +27,8 @@ const { runInstallArchetype } = await import('../src/steps/install-archetype.js'); const { readPharnConfig } = await import('../src/lib/pharn-config.js'); const { DEFAULT_MODEL_ROUTING } = await import('../src/lib/model-routing.js'); -const { readRecords } = await import('../src/lib/install-records.js'); +const { readRecords, writeRecords } = + await import('../src/lib/install-records.js'); const { collectExpectedInstallPaths } = await import('../src/lib/install-manifest.js'); const { sha256File } = await import('../src/lib/hash.js'); @@ -458,18 +459,19 @@ describe('re-running init over an existing install (PHARN-11)', () => { ...selection, selected: [ ...selection.selected, - { name: 'path-traversal', role: 'lens' as const, matched: [] }, + { + name: 'path-traversal', + role: 'lens' as const, + matched: 'manual' as const, + }, ], }; - await runInstallArchetype( - repo, - proj, - archetypes, - withManual, - 'sha123', - new Set(['lens:path-traversal']), - ); + await runInstallArchetype(repo, proj, archetypes, withManual, 'sha123', { + manualKeys: new Set(['lens:path-traversal']), + kept: [], + previousStamp: null, + }); const caps = readPharnConfig(proj)!.capabilities!; expect(caps).toContainEqual({ @@ -482,4 +484,129 @@ describe('re-running init over an existing install (PHARN-11)', () => { existsSync(join(proj, 'pharn-review/path-traversal/path-traversal.md')), ).toBe(true); }); + + // update's merge row 3 (sticky manual): an entry the user asked for by name + // stays manual while the archetypes ALSO select it, so a later archetype + // change cannot quietly drop it. + it('keeps a manual capability `manual` when the archetypes also select it', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const { archetypes, selection } = await firstInstall(repo, proj); + + await runInstallArchetype(repo, proj, archetypes, selection, 'sha123', { + manualKeys: new Set(['griller:a11y']), + kept: [], + previousStamp: null, + }); + + const caps = readPharnConfig(proj)!.capabilities!; + expect(caps.filter((c) => c.name === 'a11y')).toEqual([ + { name: 'a11y', role: 'griller', source: 'manual' }, + ]); + expect(caps.filter((c) => c.source === 'manual')).toHaveLength(1); + }); + + describe('a kept capability — upstream ships it, this CLI cannot parse it', () => { + const PERF_FILE = 'pharn-review/perf/perf.md'; + const kept = [ + { name: 'perf', role: 'lens' as const, source: 'manual' as const }, + // No `source` at all: kept VERBATIM, never re-tagged. + { name: 'legacy', role: 'griller' as const }, + ]; + + // A first install, plus perf's file on disk and in the store — as if added + // by `pharn add` at the same install state. + async function withPerfInstalled(repo: string, proj: string) { + const first = await firstInstall(repo, proj); + write(join(proj, PERF_FILE), 'perf as installed'); + const read = readRecords(proj); + if (read.kind !== 'ok') throw new Error('fixture: records unreadable'); + await writeRecords(proj, { + ...read.store, + files: { + ...read.store.files, + [PERF_FILE]: sha256File(join(proj, PERF_FILE)), + }, + }); + return first; + } + + it('writes the entries back verbatim, lists them in frozenCapabilities, and carries their records', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const { archetypes, selection } = await withPerfInstalled(repo, proj); + const perfHash = sha256File(join(proj, PERF_FILE)); + + await runInstallArchetype(repo, proj, archetypes, selection, 'sha456', { + manualKeys: new Set(), + kept, + previousStamp: { skillsVersion: '1.0.0', commit: 'sha123' }, + }); + + const config = readPharnConfig(proj)!; + expect(config.capabilities).toContainEqual(kept[0]); + expect(config.capabilities).toContainEqual(kept[1]); + expect( + config.capabilities!.find((c) => c.name === 'legacy'), + ).not.toHaveProperty('source'); + expect(config.frozenCapabilities).toEqual([ + 'griller:legacy', + 'lens:perf', + ]); + // Nothing under it was touched, so its record is still true — and kept. + expect(readFileSync(join(proj, PERF_FILE), 'utf8')).toBe( + 'perf as installed', + ); + const read = readRecords(proj); + if (read.kind !== 'ok') throw new Error('records unreadable'); + expect(read.store.commit).toBe('sha456'); + expect(read.store.files[PERF_FILE]).toBe(perfHash); + }); + + it('carries NO record from a store stamped for a different install state', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const { archetypes, selection } = await withPerfInstalled(repo, proj); + + await runInstallArchetype(repo, proj, archetypes, selection, 'sha456', { + manualKeys: new Set(), + kept, + previousStamp: { skillsVersion: '0.9.0', commit: 'older' }, + }); + + const read = readRecords(proj); + if (read.kind !== 'ok') throw new Error('records unreadable'); + expect(read.store.files).not.toHaveProperty([PERF_FILE]); + // The entry itself is still kept — only the unverifiable record is not. + expect(readPharnConfig(proj)!.frozenCapabilities).toEqual([ + 'griller:legacy', + 'lens:perf', + ]); + }); + + it('never mints a record for it when there was no store', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const { archetypes, selection } = await withPerfInstalled(repo, proj); + rmSync(join(proj, 'pharn.records.json')); + + await runInstallArchetype(repo, proj, archetypes, selection, 'sha456', { + manualKeys: new Set(), + kept, + previousStamp: { skillsVersion: '1.0.0', commit: 'sha123' }, + }); + + const read = readRecords(proj); + if (read.kind !== 'ok') throw new Error('records unreadable'); + expect(read.store.files).not.toHaveProperty([PERF_FILE]); + }); + + it('writes no frozenCapabilities when nothing is kept', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + await firstInstall(repo, proj); + + expect(readPharnConfig(proj)!).not.toHaveProperty('frozenCapabilities'); + }); + }); }); diff --git a/tests/init.test.ts b/tests/init.test.ts index 60eea11e..331ef8d0 100644 --- a/tests/init.test.ts +++ b/tests/init.test.ts @@ -139,6 +139,35 @@ describe('runInit (archetype default)', () => { vi.mocked(process.cwd).mockRestore(); }); + // What runInstallArchetype received: the selection it installs and the + // carry-over it records. + const installed = () => { + const call = runInstallArchetype.mock.calls[0] as unknown as [ + unknown, + unknown, + unknown, + { + selected: { name: string; role: string; matched: unknown }[]; + skipped: { name: string; role: string }[]; + }, + unknown, + { + manualKeys: Set; + kept: { name: string; role: string; source?: string }[]; + previousStamp: { + skillsVersion: string; + commit: string | null; + } | null; + }, + ]; + return { selection: call[3], carry: call[5] }; + }; + const warned = (): string => + vi + .mocked(log.warn) + .mock.calls.map((c) => String(c[0])) + .join('\n'); + it('installs and flags a manual entry that the index still has', async () => { const dir = tmp.path(); vi.spyOn(process, 'cwd').mockReturnValue(dir); @@ -153,21 +182,126 @@ describe('runInit (archetype default)', () => { await runInit(); - const [, , , selection, , manualKeys] = runInstallArchetype.mock - .calls[0] as unknown as [ - unknown, - unknown, - unknown, - { selected: { name: string }[] }, - unknown, - Set, - ]; + const { selection, carry } = installed(); expect(selection.selected.map((c) => c.name)).toEqual([ 'a11y', 'path-traversal', ]); - expect([...manualKeys]).toEqual(['lens:path-traversal']); + expect(selection.selected[1]!.matched).toBe('manual'); + expect([...carry.manualKeys]).toEqual(['lens:path-traversal']); expect(informed()).toContain('lens:path-traversal'); + // Row 7: gone upstream → dropped, but NAMED (never silently). + expect(warned()).toContain('lens:gone-now'); + expect(carry.kept).toEqual([]); + }); + + // update's merge row 3: sticky manual. This used to be recorded `auto`, so a + // later archetype change let `update` drop what the user asked for by name. + it('keeps a manual entry manual when the archetypes ALSO select it', async () => { + const dir = tmp.path(); + vi.spyOn(process, 'cwd').mockReturnValue(dir); + writeConfig(dir, { + ...baseConfig, + capabilities: [{ name: 'a11y', role: 'griller', source: 'manual' }], + }); + + await runInit(); + + const { selection, carry } = installed(); + expect([...carry.manualKeys]).toEqual(['griller:a11y']); + // Installed once, as the archetypes selected it — not appended again. + expect(selection.selected.map((c) => c.name)).toEqual(['a11y']); + expect(informed()).not.toContain('you added by hand'); + }); + + it('lists a carried entry once: selected as added by hand, no longer skipped', async () => { + const dir = tmp.path(); + vi.spyOn(process, 'cwd').mockReturnValue(dir); + resolveCapabilities.mockReturnValue({ + selected: [{ name: 'a11y', role: 'griller', matched: 'universal' }], + skipped: [ + { + name: 'path-traversal', + role: 'lens', + reason: 'applies to [backend]; detected [ssr]', + }, + ], + } as never); + writeConfig(dir, { + ...baseConfig, + capabilities: [ + { name: 'path-traversal', role: 'lens', source: 'manual' }, + ], + }); + + await runInit(); + + const { selection } = installed(); + expect(selection.selected).toContainEqual({ + name: 'path-traversal', + role: 'lens', + matched: 'manual', + }); + expect(selection.skipped).toEqual([]); + }); + + // update's merge row 0: a capability the fetch could not PARSE is kept + // verbatim, whoever added it — it used to be dropped and its files orphaned. + it('keeps an entry upstream ships but this CLI cannot parse — manual AND auto — verbatim', async () => { + const dir = tmp.path(); + vi.spyOn(process, 'cwd').mockReturnValue(dir); + parseCapabilityIndex.mockReturnValue({ + capabilities: [{ name: 'a11y', role: 'griller', applies: 'universal' }], + unknown: [ + { + name: 'perf', + role: 'lens', + subtree: 'pharn-review', + reason: 'unknown applies token', + }, + { + name: 'old-auto', + role: 'griller', + subtree: 'pharn-pipeline/grillers', + reason: 'unknown applies token', + }, + { + name: 'legacy', + role: 'griller', + subtree: 'pharn-pipeline/grillers', + reason: 'unknown applies token', + }, + ], + }); + writeConfig(dir, { + ...baseConfig, + commit: 'sha000', + capabilities: [ + { name: 'perf', role: 'lens', source: 'manual' }, + { name: 'old-auto', role: 'griller', source: 'auto' }, + { name: 'legacy', role: 'griller' }, + ], + }); + + await runInit(); + + const { selection, carry } = installed(); + expect(carry.kept).toEqual([ + { name: 'perf', role: 'lens', source: 'manual' }, + { name: 'old-auto', role: 'griller', source: 'auto' }, + { name: 'legacy', role: 'griller' }, + ]); + expect(carry.previousStamp).toEqual({ + skillsVersion: '1.0.0', + commit: 'sha000', + }); + // Kept, not installed: nothing of theirs is in the selection. + expect(selection.selected.map((c) => c.name)).toEqual(['a11y']); + expect(informed()).toContain('lens:perf'); + expect(informed()).toContain('griller:old-auto'); + // Named as unreadable upstream (the unknown-capability warning), never as + // dropped. + expect(warned()).not.toContain('Not keeping'); }); it('a corrupt existing config never blocks init — nothing is carried over', async () => { @@ -178,9 +312,61 @@ describe('runInit (archetype default)', () => { await runInit(); expect(runInstallArchetype).toHaveBeenCalledTimes(1); - const manualKeys = runInstallArchetype.mock.calls[0]![5 as never] as - Set | undefined; - expect([...(manualKeys ?? [])]).toEqual([]); + const { carry } = installed(); + expect([...carry.manualKeys]).toEqual([]); + expect(carry.kept).toEqual([]); + expect(carry.previousStamp).toBeNull(); + }); + + // PHARN-03's re-check, extended to init: the carry-over was computed before + // both prompts, so a pharn run that wrote the config meanwhile must refuse + // the install rather than be silently overwritten by it. + describe('the config changed while the prompts were open', () => { + const concurrentAdd = (dir: string, before: unknown) => { + confirmWriteTargets.mockImplementationOnce(async () => { + writeConfig(dir, before); + return 'proceed'; + }); + }; + + it('refuses — exit 1, nothing installed — when another run rewrote it', async () => { + const dir = tmp.path(); + vi.spyOn(process, 'cwd').mockReturnValue(dir); + const config = { + ...baseConfig, + capabilities: [{ name: 'a11y', role: 'griller', source: 'auto' }], + }; + writeConfig(dir, config); + concurrentAdd(dir, { + ...config, + capabilities: [ + ...config.capabilities, + { name: 'path-traversal', role: 'lens', source: 'manual' }, + ], + }); + + await expect(runInit()).rejects.toMatchObject(new ProcessExit(1)); + + expect(runInstallArchetype).not.toHaveBeenCalled(); + expect(cleanup).toHaveBeenCalledTimes(1); + expect(String(vi.mocked(log.error).mock.calls.at(-1)![0])).toContain( + 'pharn.config.json changed while `pharn init` was running', + ); + // The concurrent write survives untouched. + expect( + JSON.parse(readFileSync(join(dir, 'pharn.config.json'), 'utf8')) + .capabilities, + ).toHaveLength(2); + }); + + it('refuses when a config APPEARED where there was none', async () => { + const dir = tmp.path(); + vi.spyOn(process, 'cwd').mockReturnValue(dir); + concurrentAdd(dir, { ...baseConfig, capabilities: [] }); + + await expect(runInit()).rejects.toMatchObject(new ProcessExit(1)); + expect(runInstallArchetype).not.toHaveBeenCalled(); + }); }); }); @@ -211,7 +397,12 @@ describe('runInit (archetype default)', () => { ['ssr'], { selected: [], skipped: [] }, 'sha123', - new Set(), + // No previous config → nothing carried over. + expect.objectContaining({ + manualKeys: new Set(), + kept: [], + previousStamp: null, + }), ); expect(cleanup).toHaveBeenCalledTimes(1); }); diff --git a/tests/pharn-config.test.ts b/tests/pharn-config.test.ts index fdac175d..5bc318b5 100644 --- a/tests/pharn-config.test.ts +++ b/tests/pharn-config.test.ts @@ -11,7 +11,9 @@ import { log } from '@clack/prompts'; import { ProcessExit, stubProcessExit, useTmpDir } from './helpers.js'; import { ProjectChangedError } from '../src/lib/project-lock.js'; import { + assertConfigFingerprintUnchanged, assertConfigUnchanged, + configFingerprint, readPharnConfig, loadConfigOrExit, loadArchetypeConfigOrExit, @@ -788,6 +790,55 @@ describe('assertConfigUnchanged', () => { }); }); +// init-manual-carry: init reads the config it replaces TOLERANTLY, so its +// under-the-lock re-check compares a fingerprint that also covers absent and +// unreadable files — never a parse. +describe('configFingerprint / assertConfigFingerprintUnchanged', () => { + const tmp = useTmpDir(); + const file = (): string => join(tmp.path(), 'pharn.config.json'); + + it('is `absent` for no file, a sha256 of the bytes for a file, `unreadable:*` otherwise', () => { + expect(configFingerprint(tmp.path())).toBe('absent'); + writeFileSync(file(), '{ not json'); + const corrupt = configFingerprint(tmp.path()); + expect(corrupt).toMatch(/^sha256:[0-9a-f]{64}$/); + writeFileSync(file(), '{ not json '); + expect(configFingerprint(tmp.path())).not.toBe(corrupt); + rmSync(file()); + mkdirSync(file()); + expect(configFingerprint(tmp.path())).toBe('unreadable:EISDIR'); + }); + + it('passes while nothing changed — absent, corrupt or valid alike', () => { + const absent = configFingerprint(tmp.path()); + expect(() => + assertConfigFingerprintUnchanged(tmp.path(), absent, 'init'), + ).not.toThrow(); + writeFileSync(file(), '{ not json'); + const corrupt = configFingerprint(tmp.path()); + expect(() => + assertConfigFingerprintUnchanged(tmp.path(), corrupt, 'init'), + ).not.toThrow(); + }); + + it('refuses with ProjectChangedError on any change — including absent → present', () => { + const absent = configFingerprint(tmp.path()); + writeFileSync(file(), '{}'); + expect(() => + assertConfigFingerprintUnchanged(tmp.path(), absent, 'init'), + ).toThrow(ProjectChangedError); + const present = configFingerprint(tmp.path()); + writeFileSync(file(), '{ }'); + expect(() => + assertConfigFingerprintUnchanged(tmp.path(), present, 'init'), + ).toThrow(/pharn\.config\.json changed while `pharn init` was running/); + rmSync(file()); + expect(() => + assertConfigFingerprintUnchanged(tmp.path(), present, 'init'), + ).toThrow(ProjectChangedError); + }); +}); + // PHARN-05: the additive pendingSkillsVersion round-trips when well-formed and is // dropped otherwise, so a hand-edit can only fail closed. describe('pendingSkillsVersion ingest', () => {