diff --git a/.dev/features/add-after-withheld-update/GRILL.md b/.dev/features/add-after-withheld-update/GRILL.md new file mode 100644 index 0000000..f3e186c --- /dev/null +++ b/.dev/features/add-after-withheld-update/GRILL.md @@ -0,0 +1,33 @@ +# GRILL — add-after-withheld-update + +Plan: `.dev/features/add-after-withheld-update/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/add-after-withheld-update/PLAN.md:12' + problem: "`add` also merges records stamped with the config pair and refreshes `commit`; at a pending version the clone's commit differs from the recorded one. The plan must confirm that `add` writing the clone's commit does not invalidate the records stamp the withheld update left (stamp = config's old skillsVersion/commit)." + evidence: "`add`'s version gate accepts a clone at `skillsVersion` OR `pendingSkillsVersion`" +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/add-after-withheld-update/PLAN.md:33' + problem: "The `unreadable` label is not in FORCEABLE_SKIPS but is a skip label; the set test must use exactly {modified, unrecorded}, not 'forceable'." + evidence: 'whose skip labels are all in `{modified, unrecorded}`' +- type: FINDING + rule_id: 'P7' + severity: minor + file: '.dev/features/add-after-withheld-update/PLAN.md:31' + problem: "A new config field must round-trip through every writer that spreads `...config` (add/remove) and be dropped by init's fresh config; confirm no writer re-serializes a stale pending value after a complete update." + evidence: '`PharnConfig.pendingSkillsVersion?: string` (additive, P7)' +``` + +## Summary + +The key risk is the `commit`/records-stamp interaction in `add` at a pending version — verify in build. + +ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 3 advisory) — for the human to weigh before /pharn-dev-build. diff --git a/.dev/features/add-after-withheld-update/PLAN.md b/.dev/features/add-after-withheld-update/PLAN.md new file mode 100644 index 0000000..8ac7fd8 --- /dev/null +++ b/.dev/features/add-after-withheld-update/PLAN.md @@ -0,0 +1,75 @@ +# PLAN — add-after-withheld-update (PHARN-05: one kept local edit must not dead-end `pharn add`) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: when `update` withholds the version bump ONLY because of `modified` / `unrecorded` skips + (the user's own kept edits), it records the version it DID apply as an additive + `pendingSkillsVersion` in `pharn.config.json` (cleared by the next complete run); `add`'s version + gate accepts a clone at `skillsVersion` OR `pendingSkillsVersion`. When the gate still refuses, the + message names the real ways out instead of looping on "run `pharn update`". +- layer(s): the CLI itself (`src/types.ts`, `src/lib/pharn-config.ts`, `src/commands/{update,add}.ts`) +- constitution_refs: [P0, P1, P4, P5, P7] + +## Discovery — verified this run (P6) + +Reproduced (`repro/add-deadlock`): `init` 6.11.1 → one local edit in `.claude/commands/pharn-plan.md` → +`update --yes` to 6.17.1 twice, each "skipped 1", config stays 6.11.1 → `pharn add a11y` exit 1 "Skills +version mismatch … run `pharn update` first". Loop: `update` can never finish while the edit is kept. +Code: `update.ts` `versionWithheld = plan.counts.skipped > 0` keeps `skillsVersion`/`commit`; +`add.ts` `versionGate` compares `readSkillsVersion(clone) !== config.skillsVersion`; +`docs/commands/add.md` "`pharn update` is the only resolution". + +Why restricting to `modified`/`unrecorded`: those are files the user owns and chose to keep — everything +else was upgraded, so the tree IS at the new version apart from them. `unverifiable` (no usable +records baseline → every present differing file skipped) and `unreadable` can leave most of the tree at +the old version, so they do NOT set `pendingSkillsVersion` and the gate keeps refusing. + +## Files + +- `src/types.ts` — `PharnConfig.pendingSkillsVersion?: string` (additive, P7) — layer CLI +- `src/lib/pharn-config.ts` — ingest: a present `pendingSkillsVersion` failing `VERSION_RE` is dropped + (like a garbage `layout`), so a hand-edit can only fail closed — layer CLI/lib +- `src/commands/update.ts` — on a withheld run whose skip labels are all in `{modified, unrecorded}`, + write `pendingSkillsVersion: installedVersion`; on a complete run, delete the field; any other + withheld run leaves it absent — layer CLI/command +- `src/commands/add.ts` — `versionGate` passes when the clone's version equals `skillsVersion` or + `pendingSkillsVersion`; its refusal names `pharn update --force` (backs up edits) and reverting the + edits as the ways out when a pending version exists but upstream moved again — layer CLI/command +- `tests/update.test.ts` — withheld-by-`modified` → `pendingSkillsVersion` set; withheld-by- + `unverifiable` → not set; complete run → cleared +- `tests/add.test.ts` — clone at pending version → add proceeds and never touches `skillsVersion`; + clone at a third version → refused with the new message +- `tests/pharn-config.test.ts` — valid pending round-trips; garbage pending dropped; absent is legal +- `docs/commands/add.md` — "Version mismatch" section: the kept-edits case now works (P4) +- `docs/reference/pharn-config.md` — document `pendingSkillsVersion` (P4) +- `CLAUDE.md` — add/update paragraphs (P4) + +## Contracts satisfied + +- CLAUDE.md "`add` must never stamp a newer `skillsVersion` over unchanged old bytes" — preserved: `add` + still never writes `skillsVersion`, and `pendingSkillsVersion` exists only when every non-user file + was upgraded. + +## Evals to write (P1) + +- per test file above; the add-at-pending and update-sets-pending cases fail on the base source. + +## Guarantee audit (P0) + +- "`add` installs only at a version the project's non-user-owned files are at" → floor: exact version + string membership in `{skillsVersion, pendingSkillsVersion}`, where `pendingSkillsVersion` is written + only when every skip label ∈ `{modified, unrecorded}` (enum membership). +- A hand-edited `pendingSkillsVersion` is validated by `VERSION_RE`; a well-formed forged one could let + `add` install at that version — the same trust level as a hand-edited `skillsVersion` today (advisory, + named). + +## Trust audit (P2) + +- No new remote input; the new config field is local, hand-editable, regex-validated at ingest. + +## Determinism audit (P5) + +- Enum/string membership only; terminal = the existing named refusal. + +## Open questions (HALT) + +- none diff --git a/.dev/features/add-after-withheld-update/REGRESSION.md b/.dev/features/add-after-withheld-update/REGRESSION.md new file mode 100644 index 0000000..2289e2c --- /dev/null +++ b/.dev/features/add-after-withheld-update/REGRESSION.md @@ -0,0 +1,30 @@ +# REGRESSION — add-after-withheld-update + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `a6c55bc7400903236b1f1ee36432a9d74b15e1d0` (`origin/main` at build time; the build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `CLAUDE.md`, `docs/commands/add.md`, `docs/reference/pharn-config.md`, `src/commands/add.ts`, `src/commands/update.ts`, `src/lib/pharn-config.ts`, `src/types.ts`, `tests/add.test.ts`, `tests/pharn-config.test.ts`, `tests/update.test.ts`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook scratch) and this + feature's own stage artifacts are not build output. +- **outside gates:** the stdlib `*.test.mjs` / `*.test.cjs` files + 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. + +## 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/add-after-withheld-update/REVIEW.md b/.dev/features/add-after-withheld-update/REVIEW.md new file mode 100644 index 0000000..c14278a --- /dev/null +++ b/.dev/features/add-after-withheld-update/REVIEW.md @@ -0,0 +1,46 @@ +# REVIEW — add-after-withheld-update + +Floor first: `node .dev/floor/validate.mjs .` → exit 0 (GREEN). Everything below is **advisory**. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0):** the gate passes on exact membership in `{skillsVersion, pendingSkillsVersion}`; + `pendingSkillsVersion` is written only when every skip label ∈ `{modified, unrecorded}` (enum set) and + is `VERSION_RE`-validated at ingest. Grill concern #1 was real and is fixed in build: at a pending + version `add` keeps the config's `(skillsVersion, commit)` pair (`addStamp`), so it never stamps a + version the kept edits did not receive and the records stamp stays consistent (tested). +- **L-eval (P1):** 8 new cases fail on the base source: 3 in `update` (set by user-edit skips, not set by + `unverifiable`, cleared by a complete run), 2 in `add` (proceeds at pending without advancing the pair; + third version refused with the new message), 3 ingest cases. +- **L-trust (P2):** no new remote input; the field is local and regex-validated. +- **L-axis (P3):** type in `types.ts`, ingest in `pharn-config.ts`, writer in `update.ts`, reader in + `add.ts` — each file keeps its own axis. + +## Advisory findings + +```yaml +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'docs/reference/pharn-config.md:15' + problem: 'The field table at the top of the reference does not list `pendingSkillsVersion`; it is described in the prose note below it.' + evidence: "| `skillsVersion` | string | The repo's `SKILLS_VERSION` at the installed commit |" +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/commands/status.ts' + problem: '`status` still reports the install as outdated (skillsVersion is honest) without mentioning that a pending version exists; a user may not realize `add` works now.' + evidence: 'printArchetypeVersion(config, latest)' +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:8' + problem: "No CHANGELOG `[Unreleased]` entry (not in the plan's `## Files`)." + evidence: '## [Unreleased]' +``` + +## Verdict + +**GREEN** — 0 floor-gate findings, 3 advisory findings (minor). No lesson proposed for canon. diff --git a/.dev/features/add-after-withheld-update/SHIP.md b/.dev/features/add-after-withheld-update/SHIP.md new file mode 100644 index 0000000..e14faef --- /dev/null +++ b/.dev/features/add-after-withheld-update/SHIP.md @@ -0,0 +1,17 @@ +# SHIP — add-after-withheld-update + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: **Approve as written**) → `/pharn-dev-grill` → +`/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: open a PR and merge it only if + its CI checks are green. + +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/add-after-withheld-update/VERIFY.md b/.dev/features/add-after-withheld-update/VERIFY.md new file mode 100644 index 0000000..32744f7 --- /dev/null +++ b/.dev/features/add-after-withheld-update/VERIFY.md @@ -0,0 +1,27 @@ +# VERIFY — add-after-withheld-update + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, as a non-root user on node 22 with the session +proxy variables unset (as root with the proxy set, 5 pre-existing tests in `init.test.ts` / +`update.test.ts` fail for environmental reasons, identically at the baseline). + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +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":[]}` — floor gates only. + +Residual (P0/P7): "verified" means the named gates passed — not that the feature is correct in any sense +the suite does not encode. diff --git a/.dev/features/add-after-withheld-update/regression-report.json b/.dev/features/add-after-withheld-update/regression-report.json new file mode 100644 index 0000000..2937788 --- /dev/null +++ b/.dev/features/add-after-withheld-update/regression-report.json @@ -0,0 +1,28 @@ +{ + "base": "a6c55bc7400903236b1f1ee36432a9d74b15e1d0", + "inside": [ + "CLAUDE.md", + "docs/commands/add.md", + "docs/reference/pharn-config.md", + "src/commands/add.ts", + "src/commands/update.ts", + "src/lib/pharn-config.ts", + "src/types.ts", + "tests/add.test.ts", + "tests/pharn-config.test.ts", + "tests/update.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/add-after-withheld-update/verify-report.json b/.dev/features/add-after-withheld-update/verify-report.json new file mode 100644 index 0000000..ae852f9 --- /dev/null +++ b/.dev/features/add-after-withheld-update/verify-report.json @@ -0,0 +1,17 @@ +{ + "feature": "add-after-withheld-update", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 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 c7ad7c9..c8c1e59 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/hook-wiring-drift/SHIP.md" + ".dev/features/add-after-withheld-update/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T08:17:20.630Z" + "set_at": "2026-09-24T08:22:49.900Z" } diff --git a/CLAUDE.md b/CLAUDE.md index 2cdedaf..36deef9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -59,7 +59,7 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten **`lib/pharn-config.ts`** reads/writes `pharn.config.json` (`pharnVersion`, `skillsVersion`, `repo`, `commit`, `installedAt`, and — for an archetype install — `archetypes[]`, `capabilities[]` (`{name, role}`), `layout`, plus the `models`/`seam` blocks). Every `capabilities[]` entry is validated at ingest: it must be an object whose `name` matches `CAPABILITY_NAME_RE` (no control chars) and whose `role` is in `ROLE_VALUES`, else `CapabilityEntryError` — the config is committed and hand-editable, and `name` is path-joined by `remove`'s recursive delete (a `{"name":"../.."}` entry used to delete the project root). A present-but-invalid `capabilities[].source` (anything outside `{auto, manual}`; ABSENT is legal, P7) is rejected by `CapabilitySourceError`. Both join `isConfigValidationError`'s union. A present-but-invalid `models`/`seam` block is caught by its own validator — `lib/model-routing.ts` (`validateModelRouting`/`ModelRoutingError`) and `lib/seam-config.ts` (`validateSeamConfig`/`SeamConfigError`) — and `readPharnConfig` lets that named error PROPAGATE (never collapsing a bad hand-edit into the "run init" path); `isConfigValidationError` + `loadConfigOrExit` catch and report it, and the validated/stripped blocks replace the raw ones. Schema is additive — a legacy config's now-unused `modules[]`/`constitution`/`stackAnswers`/`installedSkills[]` still load (P7). `isArchetypeConfig` (= `Array.isArray(config.capabilities)`) is the deterministic discriminator; **`loadArchetypeConfigOrExit`** is the shared load-or-reject surface for `add`/`update`/`status`/`remove` (a pre-archetype config → `LEGACY_CONFIG_MESSAGE` + exit(1), never a fetch; `list` keeps its own json-aware check so `--json` stderr stays clean). `add`/`update`/`remove` update the config in place, and each calls `assertConfigUnchanged` as the FIRST step inside its `.pharn.lock` (`lib/project-lock.ts`): the config is loaded before the lock (and across `update`'s / the `remove` picker's confirm), so a concurrent run could otherwise be overwritten from a stale snapshot; a mismatch throws `ProjectChangedError` (a `ProjectLockedError`, so every catch site reports it as a named refusal, exit 1, nothing written). The lock itself treats an unparseable lock file younger than `MALFORMED_GRACE_MS` (10 s) as LIVE — `tryCreate` writes its payload after the `O_EXCL` create, so a live lock is briefly empty — and only breaks older ones. -**`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. **Before the copy, `add` backs up destination drift**: `capabilityCloneFiles` (`lib/install-manifest.ts`) enumerates the capability dir in the CLONE, `collectDestDrift` (`lib/dest-drift.ts`) keeps the rels whose dest exists as a regular file and whose `sha256File` DIFFERS from the clone's, and a non-empty set goes to `createBackup` (`lib/backup.ts` — the same `.pharn-backup//` `update --force` writes) BEFORE the first `cpSync`, logged at creation so the pointer survives a later throw. `add` was the only write path with none of the product's three edit-protections (init's prompt, update's per-file skip, `--force`'s backup), and `update` itself manufactures the reachable sequence: a `dropped-unselected` capability's files are left on disk, the user edits them, and the later `add` is not a config no-op. Byte-identical is NOT drift (mirrors update's `identical → no-op`), so a drop-then-re-add of unedited files stays silent. The scan SKIPS an absent/symlinked/non-dir source rather than throwing, so `installCapabilityDirs`' curated `Capability "x" (griller) is missing at …` still wins — the same read-skips/write-throws split `install-manifest.ts`'s `addDir` already uses. `scanDest` returns a PARTITION, not just the drift set: a rel whose PROJECT-side path crosses a symlinked component is `unsafe`, and `add` **refuses the whole install** naming the component. That is measured, not defensive — on node v24.13.1 `cpSync` guards only the source, so a symlinked INTERMEDIATE dir under the capability dir is written straight THROUGH (bytes outside the project replaced), while a symlinked leaf is REPLACED and a symlinked capability root throws `ERR_FS_CP_DIR_TO_NON_DIR`. Skipping would be strictly worse than doing nothing (the copy writes through anyway while the backup omits the file), and backing up is impossible (`createBackup` refuses symlinked components; `copyFileSync` would save the link's TARGET). An ENOTDIR walk (a component below a regular file) is NOT unsafe — `cpSync` throws on that tree by itself — so it stays a skip, which is also what keeps a non-directory component out of the set `createBackup` consumes unwrapped. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one), keyed by that SAME clone-derived list — never a walk of the destination, which used to sweep a user's own file inside a leftover capability dir into the store as pharn-written and let a later `update` read it as cleanly upgradeable instead of `modified`; the hashes are still taken at the DEST (`buildRecords`), so a record can never disagree with what landed. `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 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) — except that a clone at the config's additive `pendingSkillsVersion` also passes: `update` records that field only when it withheld the bump SOLELY because of `modified`/`unrecorded` skips (the user's kept edits; every other file is at that version) and clears it on the next complete run; at a pending version `add` keeps the config's own `(skillsVersion, commit)` pair (`addStamp`) so the records stamp stays consistent, 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. **Before the copy, `add` backs up destination drift**: `capabilityCloneFiles` (`lib/install-manifest.ts`) enumerates the capability dir in the CLONE, `collectDestDrift` (`lib/dest-drift.ts`) keeps the rels whose dest exists as a regular file and whose `sha256File` DIFFERS from the clone's, and a non-empty set goes to `createBackup` (`lib/backup.ts` — the same `.pharn-backup//` `update --force` writes) BEFORE the first `cpSync`, logged at creation so the pointer survives a later throw. `add` was the only write path with none of the product's three edit-protections (init's prompt, update's per-file skip, `--force`'s backup), and `update` itself manufactures the reachable sequence: a `dropped-unselected` capability's files are left on disk, the user edits them, and the later `add` is not a config no-op. Byte-identical is NOT drift (mirrors update's `identical → no-op`), so a drop-then-re-add of unedited files stays silent. The scan SKIPS an absent/symlinked/non-dir source rather than throwing, so `installCapabilityDirs`' curated `Capability "x" (griller) is missing at …` still wins — the same read-skips/write-throws split `install-manifest.ts`'s `addDir` already uses. `scanDest` returns a PARTITION, not just the drift set: a rel whose PROJECT-side path crosses a symlinked component is `unsafe`, and `add` **refuses the whole install** naming the component. That is measured, not defensive — on node v24.13.1 `cpSync` guards only the source, so a symlinked INTERMEDIATE dir under the capability dir is written straight THROUGH (bytes outside the project replaced), while a symlinked leaf is REPLACED and a symlinked capability root throws `ERR_FS_CP_DIR_TO_NON_DIR`. Skipping would be strictly worse than doing nothing (the copy writes through anyway while the backup omits the file), and backing up is impossible (`createBackup` refuses symlinked components; `copyFileSync` would save the link's TARGET). An ENOTDIR walk (a component below a regular file) is NOT unsafe — `cpSync` throws on that tree by itself — so it stays a skip, which is also what keeps a non-directory component out of the set `createBackup` consumes unwrapped. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one), keyed by that SAME clone-derived list — never a walk of the destination, which used to sweep a user's own file inside a leftover capability dir into the store as pharn-written and let a later `update` read it as cleanly upgradeable instead of `modified`; the hashes are still taken at the DEST (`buildRecords`), so a record can never disagree with what landed. `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). `remove` has no `--yes`, and the reason is per-path: the NAMED remove deletes without asking (no prompt to skip), while the bare picker's ONE confirm — listing the picks, default No — IS the destructive gate, so nothing may skip that either. **`remove`'s `ALLOWED_FLAGS` row is empty, so `pharn remove --yes` now exits 1** naming the flag. This SUPERSEDES the previous contract, which read: _"`--yes` stays a declared minimist boolean only because it is `update`'s flag; that is what keeps `pharn remove --yes` a harmless parse rather than an unknown-option refusal, and turning it into a refusal belongs to a per-command allowlist, not to the dispatch."_ That allowlist now exists (see the dispatch paragraph above), so the deferral is spent — but its reasoning still holds and is why the row is empty rather than `['yes']`: there is no confirm on the named path to skip, and the picker's one confirm may not be skipped. `--yes` is still declared globally, because the declaration list is DERIVED from the table and `update`'s row grants it. Before any delete (the picker: for the whole selection, before its confirm) `remove` refuses with exit 1 when a target dir's path crosses a symlinked component (`findSymlinkComponent`), since `rmSync` resolves ancestors. Every delete is a **strict child** of its role subtree — `safeChildJoin(safeJoin(cwd, subtree), name)` (`lib/validate.ts`), which, unlike `safeJoin`, never returns the base itself nor a deeper path — a second floor under the ingest-time name validation. `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: a walk of the DEST dir finds 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, so the store is the only place the answer still exists. 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: each write is individually atomic (`lib/atomic-write.ts` — temp-then-`rename`), but the PAIR is not a transaction, so a failed prune leaves the old status quo and 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). diff --git a/docs/commands/add.md b/docs/commands/add.md index 931df80..dbb3d4c 100644 --- a/docs/commands/add.md +++ b/docs/commands/add.md @@ -86,6 +86,13 @@ itself has already been fetched by then — that is where the version being comp Run [`pharn update`](update.md) to bring your install to the current version, then re-run `pharn add`. +**Kept local edits.** If `pharn update` skipped files **only because you edited them** +(`modified` / `unrecorded`), it leaves `skillsVersion` at the old value but records the version it +applied as `pendingSkillsVersion` in `pharn.config.json` — every other file is already at that version. +`add` accepts a clone at that version too, and still does not advance `skillsVersion` or `commit`. If +upstream has moved on again, the refusal names the ways out: run `pharn update`, and if it keeps +skipping your edits, revert them or run `pharn update --force` (which backs them up first). + **Known limit.** There is no way to add a capability to a deliberately-pinned older install — `add` has no `--force`, and `pharn update` is the only resolution. Matching versions is the condition under which `add` can promise anything about the tree it is adding to. diff --git a/docs/reference/pharn-config.md b/docs/reference/pharn-config.md index 7647182..cd8bdb4 100644 --- a/docs/reference/pharn-config.md +++ b/docs/reference/pharn-config.md @@ -69,6 +69,10 @@ The hash map lives there rather than here so this file stays small and hand-edit Note that `skillsVersion` / `commit` describe the last **complete** install: a `pharn update` that skipped any file deliberately leaves them at their previous values (see [update](../commands/update.md)). +When the only skipped files were ones you edited (`modified` / `unrecorded`), it also writes +`pendingSkillsVersion` — the version it applied to everything else — which lets +[`pharn add`](../commands/add.md) run at that version; the next complete `pharn update` removes it. A +value that is not a `x.y.z` version is ignored. ## Example diff --git a/src/commands/add.ts b/src/commands/add.ts index 78220f5..9dccfb6 100644 --- a/src/commands/add.ts +++ b/src/commands/add.ts @@ -90,10 +90,35 @@ export async function runAdd(capabilityArg: string | undefined): Promise { // INSIDE each path's existing try — not after fetchRepo — because readSkillsVersion // throws on a missing/invalid SKILLS_VERSION, and only inside the try does that // throw still reach the finally that cleans the clone up (P0: cleanup before exit). +// +// A `pendingSkillsVersion` (types.ts) also passes: `update` records it only when +// the bump was withheld SOLELY by the user's kept edits, so every other file is +// at that version. Without it, one kept edit dead-ended `add` for good — `update` +// could never finish while the edit stayed, and this message sent the user back +// to `update` in a loop. function versionGate(repoDir: string, config: PharnConfig): string | null { const fetched = readSkillsVersion(repoDir); if (fetched === config.skillsVersion) return null; - return `Skills version mismatch: pharn.config.json records v${config.skillsVersion}, but the fetched ${REPO_URL} is at v${fetched}. \`pharn add\` installs only at the version your project is already on — run \`pharn update\` first, then re-run \`pharn add\`.`; + if (fetched === config.pendingSkillsVersion) return null; + const base = `Skills version mismatch: pharn.config.json records v${config.skillsVersion}, but the fetched ${REPO_URL} is at v${fetched}. \`pharn add\` installs only at the version your project is already on`; + return config.pendingSkillsVersion === undefined + ? `${base} — run \`pharn update\` first, then re-run \`pharn add\`.` + : `${base} (v${config.pendingSkillsVersion} apart from files you edited). Run \`pharn update\` first; if it keeps skipping your edited files, either revert those edits or run \`pharn update --force\` (it backs them up to .pharn-backup/ first), then re-run \`pharn add\`.`; +} + +// The (skillsVersion, commit) pair `add` writes to the config and stamps the +// records with. Normally the clone's — equal to the recorded version by the gate. +// At a PENDING version the pair must stay the config's own: the records store is +// stamped with it, and writing the newer version would claim a complete upgrade +// the kept edits never received (`update` would early-return "up to date"). +function addStamp( + config: PharnConfig, + version: string, + sha: string | null, +): { skillsVersion: string; commit: string | null } { + return version === config.skillsVersion + ? { skillsVersion: version, commit: sha } + : { skillsVersion: config.skillsVersion, commit: config.commit }; } // THE LAYOUT GATE — the sibling of versionGate, and the same shape for the same @@ -440,8 +465,7 @@ async function resolveAddPicker( // the next pick's stamp check fail and silently drop its records.) cfg = { ...cfg, - skillsVersion: result.version, - commit: sha, + ...result.stamp, // Mirrors the entry resolveArchetypeAdd just persisted — INCLUDING its // `source: 'manual'`. This is the second entry-construction site, and it // must not diverge: the next pick spreads THIS array into its own config @@ -471,7 +495,12 @@ function plural(n: number): string { } type AddResult = - | { kind: 'added'; name: string; version: string } + | { + kind: 'added'; + name: string; + version: string; + stamp: { skillsVersion: string; commit: string | null }; + } | { kind: 'noop'; name: string } // See PickerAddOutcome: `cause` present ⇔ this came from an exception. | { kind: 'error'; message: string; cause?: FatalCause }; @@ -588,8 +617,9 @@ async function resolveArchetypeAdd( installCapabilityDirs(repoDir, cwd, [{ name: cap.name, role: cap.role }]); const version = readSkillsVersion(repoDir); // The SHA the tree was pinned to (recorded == fetched, or null when the branch - // was floated — LIMITS.md §3b); threaded from fetchRepo, no separate fetch. - const commit = sha; + // was floated — LIMITS.md §3b); threaded from fetchRepo, no separate fetch — + // unless the clone is at a pending version (see addStamp). + const stamp = addStamp(config, version, sha); // `source: 'manual'` — the user asked for this capability BY NAME, so it is // theirs, not archetype resolution's. `pharn update` reads that tag and // PRESERVES the entry instead of replacing it with the re-resolved auto set @@ -611,15 +641,20 @@ async function resolveArchetypeAdd( // shipped a file at that path, record==dest would make `update` read the user's // file as cleanly upgradeable instead of `modified`. The hashes are still taken // at the DEST (buildRecords), so a record can never disagree with what landed. - await mergeCapabilityRecords(cwd, config, cloneRels, version, commit); + await mergeCapabilityRecords( + cwd, + config, + cloneRels, + stamp.skillsVersion, + stamp.commit, + ); await writePharnConfig(cwd, { ...config, - skillsVersion: version, - commit, + ...stamp, capabilities, installedAt: new Date().toISOString(), }); - return { kind: 'added', name: cap.name, version }; + return { kind: 'added', name: cap.name, version, stamp }; } // Extend `pharn.records.json` with one just-installed capability's files. The diff --git a/src/commands/update.ts b/src/commands/update.ts index a491264..7b330a9 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -500,6 +500,15 @@ async function applyUpdate( ? config.skillsVersion : installedVersion; const nextCommit = versionWithheld ? config.commit : sha; + // Withheld ONLY by the user's own kept edits → every other file IS at + // `installedVersion`, so record that as pending (types.ts) and let `add` + // install at it. Any other skip (`unverifiable`, `unreadable`) can leave most + // of the tree at the old version, so nothing is recorded; a complete run + // clears it. Exact label membership, not "forceable" (P5). + const pendingSkillsVersion = + versionWithheld && plan.skipped.every((g) => USER_EDIT_SKIPS.has(g.label)) + ? installedVersion + : undefined; let written: string[]; try { @@ -564,10 +573,12 @@ async function applyUpdate( ...buildRecords(cwd, written), }, }); + const { pendingSkillsVersion: _previousPending, ...rest } = config; await writePharnConfig(cwd, { - ...config, + ...rest, skillsVersion: nextSkillsVersion, commit: nextCommit, + ...(pendingSkillsVersion !== undefined ? { pendingSkillsVersion } : {}), capabilities: configCapabilities, layout, installedAt: new Date().toISOString(), @@ -602,6 +613,13 @@ async function applyUpdate( // check (there is no shared enum and no test pinning the two lists): a new skip // label has to be added HERE as well as there, or the heading will say UNREADABLE // while the advice offers `--force`. +// The skips that are the user's own kept edits — the only ones that may leave +// a `pendingSkillsVersion` behind (applyUpdate). +const USER_EDIT_SKIPS = new Set([ + 'modified', + 'unrecorded', +]); + const FORCEABLE_SKIPS = new Set([ 'modified', 'unrecorded', diff --git a/src/lib/pharn-config.ts b/src/lib/pharn-config.ts index b7ccd1e..2c956a6 100644 --- a/src/lib/pharn-config.ts +++ b/src/lib/pharn-config.ts @@ -7,6 +7,7 @@ import { CAPABILITY_NAME_RE, isPlainObject, ROLE_VALUES, + VERSION_RE, } from './validate.js'; import { ProjectChangedError } from './project-lock.js'; import { validateModelRouting, ModelRoutingError } from './model-routing.js'; @@ -280,6 +281,16 @@ export function readPharnConfig(cwd: string): PharnConfig | null { if (config.layout !== 'pharn' && config.layout !== 'flat') { delete config.layout; } + // Additive `pendingSkillsVersion` (types.ts): only a VERSION_RE-shaped string + // survives; anything else is dropped, so a garbage hand-edit fails closed — + // `add`'s version gate then compares against `skillsVersion` alone. + if ( + config.pendingSkillsVersion !== undefined && + (typeof config.pendingSkillsVersion !== 'string' || + !VERSION_RE.test(config.pendingSkillsVersion)) + ) { + delete config.pendingSkillsVersion; + } return config; } diff --git a/src/types.ts b/src/types.ts index ea4d4c5..e97d035 100644 --- a/src/types.ts +++ b/src/types.ts @@ -116,6 +116,11 @@ export interface SeamConfig { export interface PharnConfig { pharnVersion: string; skillsVersion: string; + // Additive (P7). The upstream version a `pharn update` APPLIED while + // withholding the `skillsVersion` bump solely because of the user's own kept + // edits (`modified` / `unrecorded` skips): every other file is at this + // version. `add` accepts a clone at it; the next complete update clears it. + pendingSkillsVersion?: string; repo: string; commit: string | null; // Legacy (module/wizard) installs record the chosen constitution variant. diff --git a/tests/add.test.ts b/tests/add.test.ts index 0258d56..93f4d85 100644 --- a/tests/add.test.ts +++ b/tests/add.test.ts @@ -822,6 +822,45 @@ describe('runAdd — pharn.records.json', () => { }); }); + // PHARN-05: after an update withheld ONLY by the user's kept edits, the clone + // is at `pendingSkillsVersion`. `add` proceeds — and keeps the config's own + // (skillsVersion, commit) pair, so the store stamp stays consistent and the + // withheld state is not papered over with a version the edits never got. + it('adds at a pendingSkillsVersion without advancing skillsVersion or commit', async () => { + await seedStore(); + loadArchetypeConfigOrExit.mockReturnValue({ + ...config(), + pendingSkillsVersion: '1.1.0', + }); + readSkillsVersion.mockReturnValue('1.1.0'); + + await runAdd('a11y'); + + const [, written] = writePharnConfig.mock.calls.at(-1)!; + expect((written as PharnConfig).skillsVersion).toBe('1.0.0'); + expect((written as PharnConfig).commit).toBeNull(); + expect((written as PharnConfig).pendingSkillsVersion).toBe('1.1.0'); + expect(store()!.skillsVersion).toBe('1.0.0'); + expect(store()!.commit).toBeNull(); + expect(store()!.files[CAP_FILE]).toBe(sha256File(join(proj, CAP_FILE))); + }); + + it('still refuses a clone at a THIRD version, naming the real ways out', async () => { + loadArchetypeConfigOrExit.mockReturnValue({ + ...config(), + pendingSkillsVersion: '1.1.0', + }); + readSkillsVersion.mockReturnValue('1.2.0'); + + await expect(runAdd('a11y')).rejects.toMatchObject(new ProcessExit(1)); + + const msg = vi.mocked(prompts.log.error).mock.calls.at(-1)![0] as string; + expect(msg).toContain('v1.2.0'); + expect(msg).toContain('pharn update --force'); + expect(msg).toContain('revert'); + expect(writePharnConfig).not.toHaveBeenCalled(); + }); + it('re-stamps the store to match the config written beside it', async () => { await seedStore(); await runAdd('a11y'); diff --git a/tests/pharn-config.test.ts b/tests/pharn-config.test.ts index 3378ea6..f26f590 100644 --- a/tests/pharn-config.test.ts +++ b/tests/pharn-config.test.ts @@ -787,3 +787,40 @@ describe('assertConfigUnchanged', () => { ); }); }); + +// 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', () => { + const tmp = useTmpDir(); + const base = { + pharnVersion: '0.5.0', + skillsVersion: '1.0.0', + repo: 'pharn-dev/pharn-oss', + commit: null, + modules: [], + installedAt: '2026-09-24T00:00:00.000Z', + archetypes: ['ssr'], + capabilities: [], + }; + const put = (v: unknown): void => + writeFileSync(join(tmp.path(), 'pharn.config.json'), JSON.stringify(v)); + + it('round-trips a VERSION_RE-shaped value', () => { + put({ ...base, pendingSkillsVersion: '1.1.0' }); + expect(readPharnConfig(tmp.path())!.pendingSkillsVersion).toBe('1.1.0'); + }); + + it.each([['latest'], [''], [7], ['1.1']])('drops %j', (v) => { + put({ ...base, pendingSkillsVersion: v }); + expect(readPharnConfig(tmp.path())).not.toHaveProperty( + 'pendingSkillsVersion', + ); + }); + + it('absent is legal (P7)', () => { + put(base); + expect(readPharnConfig(tmp.path())).not.toHaveProperty( + 'pendingSkillsVersion', + ); + }); +}); diff --git a/tests/update.test.ts b/tests/update.test.ts index 0ba30c1..1add33c 100644 --- a/tests/update.test.ts +++ b/tests/update.test.ts @@ -872,6 +872,42 @@ describe('runUpdate (drift-safe)', () => { ).toBe(false); }); + // PHARN-05: a bump withheld ONLY by the user's own kept edits records the + // version it did apply, so `pharn add` is not dead-ended by one kept edit. + it("records pendingSkillsVersion when only the user's edits were skipped", async () => { + await installed(); + write(join(proj, DOC), 'MY LOCAL EDIT'); + + await runUpdate(); + + const config = readPharnConfig(proj)!; + expect(config.skillsVersion).toBe('1.0.0'); + expect(config.pendingSkillsVersion).toBe('1.1.0'); + expect(body(CAP_FILE)).toBe('a11y v2'); + }); + + it('records NO pendingSkillsVersion when a skip is not a user edit (no usable records)', async () => { + await installed(); + rmSync(join(proj, RECORDS_FILE)); + write(join(proj, DOC), 'MY LOCAL EDIT'); + + await runUpdate(); + + const config = readPharnConfig(proj)!; + expect(config.skillsVersion).toBe('1.0.0'); + expect(config.pendingSkillsVersion).toBeUndefined(); + }); + + it('a complete run clears a previous pendingSkillsVersion', async () => { + await installed({ pendingSkillsVersion: '1.0.5' }); + + await runUpdate(); + + const config = readPharnConfig(proj)!; + expect(config.skillsVersion).toBe('1.1.0'); + expect(config.pendingSkillsVersion).toBeUndefined(); + }); + it('re-resolves the RECORDED archetypes against the fresh index, and UNIONS the result with the manual entries', async () => { // This test used to pin only the re-resolve call — which was true of the // wholesale-replace bug too. The re-resolve still happens; what it now also