From 0f328bded260b06e2d5adab9ce72e1f1508a6946 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 12:41:41 +0000 Subject: [PATCH] fix(init): a re-install backs up only what update would keep; refusals come before the backup - Edited = changed since pharn wrote it. init's prompt and its backup now classify each existing file through update's own decideFileAction, against pharn.records.json (stamp-checked against the config being replaced). A file still at its recorded hash is a clean upgrade: not marked, not backed up. Marks: (edited), (no pharn record), and (differs from upstream) when there is no usable store. - Each file is compared with its REAL source, so an edited PHARN-LICENSE / pharn/LICENSE (source: upstream LICENSE) is marked and backed up instead of silently overwritten. - The destination pre-flight (prepareInstall, exported) runs before the backup: a refused install writes nothing, .pharn-backup/ included. - The type pre-flight also refuses a directory at pharn.config.json or pharn.records.json (was: every file copied, then EISDIR, leaving no records and no config). - A live .claude/settings.json symlink is accepted (init never writes an existing settings file); a dangling one is refused with a file-specific message. Linked files and linked directories are named apart. Measured on Node 20.13 / 22 / 24: cpSync replaces a dangling leaf link, it never writes through. - The install manifest is built once per run and shared by the prompt, the pre-flight, the backup scan, the copy and the records (was: 5x). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o --- .dev/features/init-reinstall-safety/GRILL.md | 55 +++ .dev/features/init-reinstall-safety/PLAN.md | 167 +++++++ .../init-reinstall-safety/REGRESSION.md | 43 ++ .dev/features/init-reinstall-safety/REVIEW.md | 121 +++++ .dev/features/init-reinstall-safety/SHIP.md | 38 ++ .dev/features/init-reinstall-safety/VERIFY.md | 71 +++ .../regression-report.json | 33 ++ .../init-reinstall-safety/verify-report.json | 18 + .pharn/writes-scope.json | 4 +- CHANGELOG.md | 6 + CLAUDE.md | 4 +- docs/commands/init.md | 36 +- docs/troubleshooting.md | 30 +- src/commands/init.ts | 22 +- src/lib/dest-drift.ts | 144 ++++-- src/lib/install-capabilities.ts | 195 +++++--- src/lib/install-manifest.ts | 16 +- src/steps/install-archetype.ts | 127 ++++-- src/steps/overwrite-check.ts | 115 ++++- tests/dest-drift.test.ts | 194 +++++++- tests/init-archetype.test.ts | 421 +++++++++++++++++- tests/init.test.ts | 43 +- tests/install-capabilities.test.ts | 164 ++++++- tests/overwrite-check.test.ts | 141 +++++- 24 files changed, 2038 insertions(+), 170 deletions(-) create mode 100644 .dev/features/init-reinstall-safety/GRILL.md create mode 100644 .dev/features/init-reinstall-safety/PLAN.md create mode 100644 .dev/features/init-reinstall-safety/REGRESSION.md create mode 100644 .dev/features/init-reinstall-safety/REVIEW.md create mode 100644 .dev/features/init-reinstall-safety/SHIP.md create mode 100644 .dev/features/init-reinstall-safety/VERIFY.md create mode 100644 .dev/features/init-reinstall-safety/regression-report.json create mode 100644 .dev/features/init-reinstall-safety/verify-report.json diff --git a/.dev/features/init-reinstall-safety/GRILL.md b/.dev/features/init-reinstall-safety/GRILL.md new file mode 100644 index 00000000..c2b75e1a --- /dev/null +++ b/.dev/features/init-reinstall-safety/GRILL.md @@ -0,0 +1,55 @@ +# GRILL — init-reinstall-safety + +Plan: `.dev/features/init-reinstall-safety/PLAN.md`. Spec hash recomputed: +`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e` — **matches**. Registered +grillers: `{"registered":0,"grillers":[]}` → inline axes only. The plan is `trust: untrusted`; +nothing in it read as an instruction. + +## Findings + +### Eval coverage (P1) + +```yaml +- type: FINDING + rule_id: 'P1' + severity: important + file: '.dev/features/init-reinstall-safety/PLAN.md:104' + problem: 'tests/init.test.ts mocks the install step and the prompt, so a spy there only sees init.ts''s own call. The "once per init" claim spans init → prompt → install → pre-flight → records, which only a test running the REAL steps can count. That is init-archetype.test.ts, with the manifest module partially mocked to pass through while counting. Otherwise the F28 test passes while four of the five calls survive below it.' + evidence: '`tests/init.test.ts` — layer tests. The manifest builder runs once per init (spy;' +``` + +### Guarantee audit (P0) + +```yaml +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/init-reinstall-safety/PLAN.md:83' + problem: 'The records store is keyed at the layout of the install that wrote it. A re-install whose clone changed layout (flat → pharn/) finds no record at the new paths, so every differing file is labelled "pharn has no record of them" and backed up. That is conservative and correct, but name it in the docs, so a layout migration''s long backup list is not read as a bug.' + evidence: 'records baseline (`carry.previousStamp`) → backup → copy (F20). The records keys' +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/init-reinstall-safety/PLAN.md:86' + problem: 'The prompt classifies before the lock and the backup re-classifies inside it, so the two can disagree if the records store changes in between. The lock and init''s config-fingerprint re-check make that rare, and the backup is the one that acts: state that the backup scan is authoritative and the prompt''s labels are advisory.' + evidence: 'from the same classifier, worded per label: "changed since pharn wrote them"; "pharn' +``` + +### Checked, no finding + +- Determinism (P5): classification is `decideFileAction`, already table-tested for update. The + fallback with no usable store is "back it up", never a guess. +- Trust (P2): source paths come from the manifest, already `safeJoin`-contained. Records are local, + stamp-checked data. +- Axis (P3): the scan widens in `dest-drift.ts` (its owner), the pre-flight moves within + `install-capabilities.ts`, and the ordering changes in the step that owns it. + +## Summary + +The design follows update's decision table and closes each finding with a failing-on-base case. +The important gap is test placement: the F28 count must be taken where the real steps run, or it +proves nothing. Two documentation points follow from the design and should be written down: the +layout-migration backups, and which scan is authoritative. + +**ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 1 important, 2 minor) — for the human to +weigh before /pharn-dev-build.** diff --git a/.dev/features/init-reinstall-safety/PLAN.md b/.dev/features/init-reinstall-safety/PLAN.md new file mode 100644 index 00000000..83bb58d7 --- /dev/null +++ b/.dev/features/init-reinstall-safety/PLAN.md @@ -0,0 +1,167 @@ +# PLAN — init-reinstall-safety (a re-run init calls only YOUR edits "edited", backs up every one of them, refuses before it backs up, and computes its manifest once) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: `init`'s install path, six findings. + - **Edited = changed since pharn wrote it (F9).** A re-install calls a file "edited", and backs it + up, only when `update` would have skipped it: by `pharn.records.json`, through update's own + `decideFileAction`, not "differs from upstream". + - **Scan the real source (F10).** The scan compares each destination with its real SOURCE, so + `PHARN-LICENSE` / `pharn/LICENSE` ← `LICENSE` is covered. + - **Refuse before the backup (F20).** Every pre-flight runs before the backup. A refused install + creates no `.pharn-backup/`. + - **Config and records paths (F19).** The type pre-flight also covers `pharn.config.json` and + `pharn.records.json`. + - **A live symlinked settings.json is fine (F8, init half).** init never writes an existing file + there, so only a dangling link is refused. + - **One manifest per run (F28).** It is computed once and passed down. +- layer(s): the CLI itself (`src/commands/init.ts`, `src/steps/overwrite-check.ts`, + `src/steps/install-archetype.ts`, `src/lib/install-capabilities.ts`, `src/lib/dest-drift.ts`), + docs +- constitution_refs: [P0, P1, P2, P5, P6] + +## Discovery — verified this run (P6), code read on HEAD `abb274a` + +- **F9.** `confirmWriteTargets` (`overwrite-check.ts:108-155`) labels `(edited)` the conflicts + whose bytes differ from the NEW upstream (`scanDest`). It says "N of them differ from upstream + (your edits)". `runInstallArchetype` (`install-archetype.ts:83-101`) backs up the same set. So + after any upstream bump, untouched pharn files are called your edits and copied to + `.pharn-backup/`. The review reproduced 14 of them with zero edits. + - `update` already has the right table: `decideFileAction` (`update-decision.ts:73+`). A disk + hash equal to the record is a clean upgrade with no backup. A modified, unrecorded or + unverifiable file gets `backup: true` under `force`, which is init's position, since init always + overwrites. + - init already reads the previous config's `(skillsVersion, commit)` as `carry.previousStamp` + (`install-archetype.ts:54`, `init.ts:345`). +- **F10.** `collectExpectedInstallPaths` returns `Map`, and LICENSE is its one + entry where they differ (`install-manifest.ts:172-181`). Both scans pass only `.keys()`, and + `scanDest` (`dest-drift.ts`) joins the same `rel` on both sides. The clone has no `PHARN-LICENSE`, + so an edited copy is never compared, never backed up, and silently overwritten (reproduced by the + review, both layouts). +- **F20.** The backup (`install-archetype.ts:83-101`) runs before `installCapabilities`. That + function's pre-flights (`install-capabilities.ts:160-171`: symlinked destinations, type + collisions) then refuse with "Nothing was written". The user sees "Backed up…", and each retry + adds `.pharn-backup/-2`, `-3`. +- **F19.** `assertDestinationTypes` (`:384-412`) walks only manifest paths. A DIRECTORY at + `pharn.records.json` passes every check, with no prompt (records are not a conflict), so every file + is copied and then the records rename fails (EISDIR): no records and no config. + `pharn.config.json/` does the same after the confirm. +- **F8, init half.** `assertDestinationsInProject` (`:342-375`) walks `.claude/settings.json` with + `findSymlinkComponent`, so a symlinked LEAF refuses the whole install. Its advice ("replace it + with a real directory") is also wrong for a file. Yet `settingsPreserved = existsSync(settingsTo)` + (`:196`) follows a live link, so an existing settings file is never written. Only a DANGLING leaf + would be written through (`cpSync` follows it and creates the target outside the project). +- **F28.** `collectExpectedInstallPaths` runs 5× per init: + 1. `conflictingWriteTargets` (`install-manifest.ts:228`); + 2. the backup scan (`install-archetype.ts:88`); + 3. `assertDestinationsInProject` (`install-capabilities.ts:349`); + 4. `assertDestinationTypes` (`:392`); + 5. the records keys (`install-archetype.ts:203`). + + Every call returns the same map: it depends only on the clone, the selection and the layout, and + none of those change during a run. The re-scan HASHING (prompt time, then again before the copy) + is deliberate, because it catches an edit made while the prompt was open, and it stays. + +## Files + +- `src/lib/dest-drift.ts` — layer CLI/lib. Changes: + - The scan takes `(dest → source)` pairs instead of plain rels; `add` passes identity pairs, so + its behaviour stays as it is (F10). + - An optional records baseline: a differing file is "drifted" exactly when + `decideFileAction({…, force: true}).backup` holds — update's own table (F9). With no baseline, + today's rule (differs from upstream) stands. + - Each drifted file carries its label: modified, unrecorded, or unverifiable. +- `src/lib/install-capabilities.ts` — layer CLI/lib. An exported pre-flight wraps the symlink and + type walks over a precomputed manifest: + - The type walk also covers `pharn.config.json` and `pharn.records.json`, which must never be a + directory (F19). + - The settings leaf is refused only when it is a DANGLING link (F8, GATE 1 answer 2 → a). A + symlinked `.claude/` is still refused, and the message distinguishes a linked file from a + linked directory. + - The install function (`installCapabilities`) accepts the precomputed manifest and still runs + the pre-flight itself. +- `src/lib/install-manifest.ts` — layer CLI/lib. `conflictingWriteTargets` accepts an optional + precomputed manifest, so the prompt reuses init's one computation instead of building its own + (F28; amendment during build — the alternative was duplicating its logic in the prompt). +- `src/steps/install-archetype.ts` — layer CLI/steps. Order becomes pre-flight → scan with the + records baseline (`carry.previousStamp`) → backup → copy (F20). The records keys come from the + same manifest (F28). +- `src/steps/overwrite-check.ts` — layer CLI/steps. The `(edited)` marker and the edits line come + from the same classifier, worded per label: "changed since pharn wrote them"; "pharn has no + record of them"; and, with no usable store, "differ from upstream — pharn has no record to tell + your edits from upstream changes". A file equal to its record is a clean upgrade: never marked, + never backed up (GATE 1 answer 1 → a). +- `src/commands/init.ts` — layer CLI/commands. Computes the manifest ONCE after resolution and + passes it to the prompt and the install (F28). +- `tests/dest-drift.test.ts` — layer tests. A file equal to its record but not to upstream → not + drifted (FAILS on base); an unrecorded differing file → drifted, unrecorded; a dest≠src pair → + compared with its source (FAILS on base). +- `tests/init-archetype.test.ts` — layer tests. An upstream bump with no edits → no `(edited)`, no + backup (FAILS on base); one real edit → exactly that file listed and backed up; an edited + `PHARN-LICENSE` (flat) or `pharn/LICENSE` → marked and backed up (FAILS on base); an edited file + plus a type collision → refused, no `.pharn-backup/`, no "Backed up" line (FAILS on base). The manifest builder runs + once per init, counted through the REAL steps with a pass-through spy (FAILS on base with 5; + grill finding 1). +- `tests/install-capabilities.test.ts` — layer tests. A directory at `pharn.records.json` or + `pharn.config.json` → refused before the first write (FAILS on base); a live + `.claude/settings.json` symlink → the install proceeds and the link target stays untouched (FAILS + on base); a dangling one → refused, target never created (guard); a symlinked `.claude/` → + still refused (guard). +- `tests/init.test.ts` — layer tests. Only the call-shape assertions change (the prompt and the + install now receive the manifest). The once-per-init count lives in init-archetype.test.ts, + where the real steps run (grill finding 1). +- `tests/overwrite-check.test.ts` — layer tests. The three wordings. +- `docs/commands/init.md` — layer docs. The re-install paragraph (what "edited" means, the + no-records fallback, LICENSE included) and the refusals (config/records directories; the + dangling `settings.json` link). +- `docs/troubleshooting.md` — layer docs. A symlinked `.claude/settings.json` is fine and a dangling + one is refused; the directory-at-config/records refusal. +- `CLAUDE.md` — layer docs. The init step-5 passage (prompt / scan / backup order) and the + `installCapabilities` DESTINATION-guard sentence (settings.json rule, config/records in the type + walk, manifest passed in). +- `CHANGELOG.md` — `[Unreleased]` → `### Fixed`, one entry per finding + +## Contracts satisfied + +- `update`'s decision table (`lib/update-decision.ts`) becomes the single definition of "your + edit" for both commands (cited, P4). +- PHARN-16's "no half-install": now true for the two CLI-owned files too. + +## Evals to write (P1) + +- Listed under Files. Eight cases FAIL on the base. + +## Guarantee audit (P0) + +- "a pristine pharn file is never called your edit when records are usable" → floor: + `decideFileAction` (already tested) plus the scan tests. Without usable records → advisory, + conservative (every difference is backed up), and the prompt says so. +- "every file init overwrites that update would have skipped is backed up first, LICENSE included" + → floor: the pair scan plus tests. +- "a refused install writes nothing, not even a backup" → floor: pre-flight before backup, plus a + test. Residual: a filesystem change between pre-flight and copy (TOCTOU), the same residual + `update` names. +- "the manifest is computed once" → floor: a spy test (a performance claim, not a safety one). + +## Trust audit (P2) + +- The clone is untrusted. The scan joins source paths from the manifest, which are already + `safeJoin`-contained. Records are local, stamp-checked data (a stale or foreign store is ignored + → the conservative path). Nothing new is printed except CLI-owned wording and manifest paths, + as today. + +## Determinism audit (P5) + +- Hash equality and set membership through the existing decision table. The fallback with no usable + store is the conservative "back it up", never a guess. + +## Open questions (HALT) + +None open. Resolved at GATE 1 (human, 2026-09-25): every question below → **(a)**, the +recommended answer. Kept for the record: + +1. A file that equals its record but differs from upstream (pharn's own bytes, just outdated). + (a) Treat it as a clean upgrade: no `(edited)`, no backup, exactly as `update` does — + recommended. (b) Keep backing it up, noisy but zero-risk. +2. `.claude/settings.json` as a symlink during init. (a) A live link is allowed (it is preserved, as + today) and a dangling one is refused with a file-specific message — recommended. (b) Allow a + dangling link too, by skipping the settings write and warning. diff --git a/.dev/features/init-reinstall-safety/REGRESSION.md b/.dev/features/init-reinstall-safety/REGRESSION.md new file mode 100644 index 00000000..7350160a --- /dev/null +++ b/.dev/features/init-reinstall-safety/REGRESSION.md @@ -0,0 +1,43 @@ +# REGRESSION — init-reinstall-safety + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `5b63e313ed74c5d875039685227c68ea76c1ea67` (`HEAD` — `origin/main` after #222; the + build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): + - `src/commands/init.ts`, `src/lib/dest-drift.ts`, `src/lib/install-capabilities.ts`, + `src/lib/install-manifest.ts`, `src/steps/install-archetype.ts`, `src/steps/overwrite-check.ts` + - `tests/dest-drift.test.ts`, `tests/init-archetype.test.ts`, `tests/init.test.ts`, + `tests/install-capabilities.test.ts`, `tests/overwrite-check.test.ts` + - `docs/commands/init.md`, `docs/troubleshooting.md` + - `CLAUDE.md`, `CHANGELOG.md` +- **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 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-reinstall-safety/REVIEW.md b/.dev/features/init-reinstall-safety/REVIEW.md new file mode 100644 index 00000000..21013b72 --- /dev/null +++ b/.dev/features/init-reinstall-safety/REVIEW.md @@ -0,0 +1,121 @@ +# REVIEW — init-reinstall-safety + +Increment: + +- `src/lib/dest-drift.ts`: `scanDest` takes an optional `sources` map (dest → clone path) and an + optional records baseline. A differing file is drifted exactly when update's `decideFileAction` + (`force: true`) sets `backup`, and carries its label. `manifestSources` converts the install + manifest's absolute sources. +- `src/lib/install-capabilities.ts`: an exported pre-flight, `prepareInstall`, over a precomputed or + computed manifest: + - linked directories and linked files are named apart; + - a live `.claude/settings.json` link is allowed, a dangling one is refused; + - a directory at `pharn.config.json` / `pharn.records.json` is refused. + + `installCapabilities` accepts the manifest and still runs the pre-flight itself. +- `src/lib/install-manifest.ts`: `conflictingWriteTargets` accepts a precomputed manifest. +- `src/steps/install-archetype.ts`: `installManifest` and `reinstallBaseline`; the order is + pre-flight → scan with the baseline → backup → copy; the records keys come from the same + manifest. +- `src/steps/overwrite-check.ts`: the markers and count lines come from the scan's labels. +- `src/commands/init.ts`: builds the manifest once and passes it, with the baseline. +- Tests in five files, `docs/commands/init.md`, `docs/troubleshooting.md`, CLAUDE.md and CHANGELOG. + +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` +passed (1605 tests), `/pharn-dev-regress` returned `no-regressions`, and `/pharn-dev-verify` +returned `PASS`. The chain ran twice: this review's first read found two false claims in the +increment's own docs (advisory findings 1 and 2), which were fixed within the plan's `## Files` +before the second run. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0)** — each claim has a test: + - "a file pharn wrote is never called your edit when the records are usable" → `decideFileAction` + (already table-tested) plus the scan, prompt and step tests, and two whole-`runInit` runs; + - "every file init overwrites that update would have skipped is backed up first, LICENSE + included" → the pair scan plus flat and `pharn/` tests; + - "a refused install writes nothing, not even a backup" → the pre-flight now precedes the backup, + plus a test. Residual: a change between the pre-flight and the copy (TOCTOU), as `update` names; + - "a directory at the config or records path is refused before the first write" → two tests; + - "a live `settings.json` link is kept, a dangling one refused, the link untouched" → two tests; + - "the manifest is built once per run" → a pass-through spy across a whole `runInit`, plus the + identity assertions in `tests/init.test.ts` (a performance claim, not a safety one). +- **L-eval (P1)** — 35 cases fail on the base, each for the reason the plan names (VERIFY.md + lists them). The guard cases pass on both. +- **L-trust (P2)** — `sources` values come from the manifest (inside the clone) and are + re-`safeJoin`ed under the clone before any read, and a test pins an escaping value as a throw. + The records are local, stamp-checked data; they can only take a file OUT of the backup set, and + only one whose bytes are what pharn recorded writing. The prompt prints manifest paths (as + before) and CLI-owned wording only. +- **L-axis (P3)** — the classification stays in `dest-drift.ts`, the pre-flight in + `install-capabilities.ts`, the order in the install step, the wording in the prompt step. + `init.ts` only orchestrates. `installManifest` is a thin wrapper in the install step, so `init.ts` + imports no `*manifest.js` module: `tests/init.test.ts` has a static guard against re-introducing + the deleted module manifest, and this keeps it as it is rather than loosening it. + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P6' + severity: minor + file: '.dev/features/init-reinstall-safety/PLAN.md:51' + problem: 'RESOLVED IN THIS INCREMENT. The plan, and the test it replaced, said cpSync writes THROUGH a dangling settings.json link and creates its target outside the project. Measured on Node 20.13.0, 22.22.2 and 24.21.0 (absolute and relative targets): cpSync REPLACES the link with a regular file and writes nothing outside. The approved decision (refuse a dangling link) stands on the true reason — the link would be lost silently — and the code comment, docs and test title now say that.' + evidence: 'Only a DANGLING leaf would be written through (`cpSync` follows it and creates the target outside the project).' +- type: FINDING + rule_id: 'P0' + severity: minor + file: 'docs/commands/init.md:147' + problem: 'RESOLVED IN THIS INCREMENT (second iteration). The first build repeated the grill premise that a flat → pharn/ layout change backs up every differing file, marked "no pharn record". It does not: the new paths do not exist in a flat project, and the old files are left in place, so nothing extra is backed up. Only a file of the user own already at a new path is. The docs and the reinstallBaseline comment now say so, and a new case in tests/init-archetype.test.ts pins it.' + evidence: 'a re-install that moves your project from the flat layout to `pharn/` backs up every file there that differs from upstream' +- type: FINDING + rule_id: 'P2' + severity: minor + file: 'src/steps/install-archetype.ts:331' + problem: 'PRE-EXISTING, NOT FIXED HERE. readRecords reads pharn.records.json with readFileSync, which blocks forever on a FIFO; update already reads it that way. init now reads it on every re-install (reinstallBaseline) rather than only when a kept capability needs its records, so a FIFO at that path now also hangs init. Only a local actor can plant one, a threat THREAT-MODEL.md does not model. The fix belongs in lib/install-records.ts (an O_NONBLOCK + fstat read, as the hook-wiring reader does), outside this plan.' + evidence: ': recordsBaseline(readRecords(cwd), previousStamp).records;' +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/dest-drift.ts:147' + problem: 'The dest list and its sources are two parameters. A dest missing from `sources` is read at its own path in the clone, which is right for add and wrong only for a mapped entry. Both init call sites build `sources` from the same manifest their dest list comes from, so they cannot disagree today; a caller that forgot `sources` would silently lose the LICENSE comparison again.' + evidence: 'const srcRel = sources?.get(rel) ?? rel;' +- type: FINDING + rule_id: 'P1' + severity: minor + file: 'tests/init-archetype.test.ts:37' + problem: 'A module mock counts only calls that cross a module boundary. On the base the spy sees 4 builds, not the 5 the plan counts: the fifth ran inside install-manifest.ts. The fix removes that one too (conflictingWriteTargets now receives the manifest), which the "uses the manifest it is GIVEN" test and the identity assertions in tests/init.test.ts show; the spy alone could not.' + evidence: 'A module mock sees only calls that cross a module boundary' +- type: FINDING + rule_id: 'P3' + severity: minor + file: 'src/lib/symlink-guard.ts:45' + problem: 'PRE-EXISTING. Two comments here and the add passage in CLAUDE.md call the drift scan `collectDestDrift`; it is `scanDest`. Queued for plan F (review-cleanups) rather than widened into this increment.' + evidence: 'wraps the call and turns it into its `unreadable` terminal; `collectDestDrift`' +- type: FINDING + rule_id: 'P7' + severity: minor + file: 'src/steps/install-archetype.ts:108' + problem: 'The pre-flight runs after the overwrite prompt, so a user can confirm "Continue and overwrite?" and then be refused. Running it before the prompt as well would refuse first. Not in this plan, which orders pre-flight before BACKUP; noted as a possible follow-up.' + evidence: 'const prepared = prepareInstall(repoDir, cwd, selection.selected, manifest);' +``` + +## Proposed lesson (candidate — NOT canon; promotion is a separate, human-gated run) + +- **Re-measure a stated filesystem behavior on the CI Node before encoding it.** Provenance: this + increment. Two claims about Node/`fs` behavior reached the build as fact, one from the plan + (a dangling link is "written through") and one from the grill (a layout change "backs up every + differing file"). Both were false when measured, and the second had already been written into the + docs. Candidate rule: every behavior claim a plan or grill states about `fs` / `cpSync` / Node is + re-measured at build time, on the CI Node version, before it is encoded in a comment, doc or test. + +## Verdict + +**GREEN — 0 floor-gate findings, 7 advisory (minor; two resolved in this increment).** The standing +decision is the human's (GATE 2). diff --git a/.dev/features/init-reinstall-safety/SHIP.md b/.dev/features/init-reinstall-safety/SHIP.md new file mode 100644 index 00000000..323aff23 --- /dev/null +++ b/.dev/features/init-reinstall-safety/SHIP.md @@ -0,0 +1,38 @@ +# SHIP — init-reinstall-safety + +Stages run, in order: + +1. `/pharn-dev-plan` → GATE 1 (human: plans A–F accepted with every recommended answer). +2. `/pharn-dev-grill`. The plan was amended with its findings before the build: the manifest-once + count moved to `tests/init-archetype.test.ts`, where the real steps run, and the docs were to say + which scan is authoritative. +3. `/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review`. +4. `/pharn-dev-review` found two false claims in the increment's own docs (REVIEW.md advisory + findings 1–2). The scope was re-set from the plan, both were fixed within its `## Files` (plus one + test pinning the corrected layout-change behavior), and build → regress → verify ran again. + Everything below is from that second run. +5. GATE 2. + +Deviations from the plan, all inside its `## Files`: + +- `init.ts` gets the manifest from a thin `installManifest` in the install step, not from + `install-manifest.ts` directly: a static guard in `tests/init.test.ts` forbids `init.ts` importing + any `*manifest.js`, and the guard was kept rather than loosened. +- `tests/init.test.ts` gained one case beyond the call-shape changes: a cancelled summary builds no + manifest and reads no records. +- The plan's reason for refusing a dangling `settings.json` link ("written through") was false when + measured on Node 20.13 / 22 / 24; the approved decision stands on the true reason (REVIEW.md + finding 1). + +| 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) +- The 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-reinstall-safety/VERIFY.md b/.dev/features/init-reinstall-safety/VERIFY.md new file mode 100644 index 00000000..f635ae0e --- /dev/null +++ b/.dev/features/init-reinstall-safety/VERIFY.md @@ -0,0 +1,71 @@ +# VERIFY — init-reinstall-safety + +## FLOOR layer (owns the verdict) + +The gates ran over the whole repo with the feature present, on node 22, with the session proxy +variables unset. They ran 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 (1605 tests). It collects this increment's own cases in five files. 34 of them + failed on the base (`5b63e31`, the new test files copied into a base worktree), each for the + reason the plan names: + - an upstream bump alone → `.pharn-backup/` created; + - one real edit → two files backed up instead of one; + - an edited `PHARN-LICENSE` / `pharn/LICENSE` → nothing backed up; + - an edit plus a type collision → a backup made before the refusal; + - a directory at `pharn.records.json` → every file copied, then `EISDIR`; + - a whole `runInit` → the manifest builder called 4 times where the spy can see it (the fifth + ran inside `install-manifest.ts`, invisible to a module mock); 1 after the fix. + + The guard cases (no records, a foreign-stamped store, a symlinked `.claude/`) pass on both. A + 35th case, added in the second iteration, pins that a flat → `pharn/` layout change backs up only + a file of the user's own at a new path; it also fails on the base (`expected [] to deeply equal + [ 'pharn/LICENSE' ]` — the LICENSE-source defect). +- `test:floor` is floor.yml's `node --test` run (754 tests). +- `npm run test:coverage` passes its ratchet (97.61 / 92.93 / 98.32 / 98.45) and `npm run build` + exits 0; neither is a verdict gate, both are CI gates. +- There is no `structural:*` gate: the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +Outside the verdict: + +- **Two whole `runInit` runs through the real steps** (`tests/init-archetype.test.ts`, a fixture + clone standing in for the network fetch): after an upstream bump alone, the prompt marks nothing + and nothing is backed up; with one real edit, the prompt marks exactly that file `(edited)` and + the backup holds exactly that file. The prompt and the backup agree. +- **The dangling-link rationale was re-measured, and the plan's claim did not hold.** On Node + 20.13.0, 22.22.2 and 24.21.0, `cpSync` onto a dangling `.claude/settings.json` link (absolute or + relative, target directory present or not) **replaces the link with a regular file**; it never + writes through. The approved decision (refuse a dangling link) stands on the true reason — the + user's link would be lost silently — and the code comment, docs and tests say that, not + "written through". +- **The CLI itself could not be driven end to end here:** the sandbox's egress proxy answers + codeload with 403. + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}`. No verifiers are +registered, so the verdict rests on the 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. + +Named limits: + +- The prompt's labels are advisory: they are computed before the lock, and the scan under the lock + decides the backup. +- The pre-flight and the copy are a check, not a lock: a link created between them is not seen (the + TOCTOU residual `update` also names). +- Without a usable `pharn.records.json`, "your edit" cannot be told from an upstream change, so + every difference is backed up and labelled that way. diff --git a/.dev/features/init-reinstall-safety/regression-report.json b/.dev/features/init-reinstall-safety/regression-report.json new file mode 100644 index 00000000..917d80f9 --- /dev/null +++ b/.dev/features/init-reinstall-safety/regression-report.json @@ -0,0 +1,33 @@ +{ + "base": "5b63e313ed74c5d875039685227c68ea76c1ea67", + "inside": [ + "CHANGELOG.md", + "CLAUDE.md", + "docs/commands/init.md", + "docs/troubleshooting.md", + "src/commands/init.ts", + "src/lib/dest-drift.ts", + "src/lib/install-capabilities.ts", + "src/lib/install-manifest.ts", + "src/steps/install-archetype.ts", + "src/steps/overwrite-check.ts", + "tests/dest-drift.test.ts", + "tests/init-archetype.test.ts", + "tests/init.test.ts", + "tests/install-capabilities.test.ts", + "tests/overwrite-check.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-reinstall-safety/verify-report.json b/.dev/features/init-reinstall-safety/verify-report.json new file mode 100644 index 00000000..1ba2dda8 --- /dev/null +++ b/.dev/features/init-reinstall-safety/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "init-reinstall-safety", + "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 9b374ee9..93b8998e 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/hook-wiring-truth/SHIP.md" + ".dev/features/init-reinstall-safety/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-25T05:46:20.332Z" + "set_at": "2026-09-25T12:41:05.339Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d54a14d..d1471bc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A symlinked `settings.json` file, managed from dotfiles for example, is now read through, as Claude Code does. It used to be treated as unreadable. A symlinked `.claude/` directory is still refused. - A FIFO at a settings path no longer blocks `status` or `update` forever. - **Each missing hook in the `HOOKS` note is now printed as the hook itself, in JSON, so you can paste it.** An exec-form hook (`command` with `args`) used to print as one shell-style line, and pasting that line in did not satisfy the check. +- **A re-run `pharn init` called pharn's own files "your edits" and backed them up.** After any upstream update, every installed file that upstream had changed was marked `(edited)` and copied to `.pharn-backup/`, although you had not touched it. `init` now decides what to back up the way `pharn update` decides what to skip, using the hashes in `pharn.records.json`: a file that changed since pharn wrote it is marked `(edited)`, a differing file pharn has no record of is marked `(no pharn record)`, and both are backed up. A file still exactly as pharn wrote it is a clean upgrade: not marked, not backed up. Without a usable records file, every file that differs from upstream is still backed up, and the prompt now says it cannot tell your edits from upstream changes (`(differs from upstream)`). +- **An edited `PHARN-LICENSE` or `pharn/LICENSE` was overwritten without a backup.** It was compared with a file of the same name in the download, which does not exist; its source is upstream's `LICENSE`. It is now compared with that file, and backed up and marked like any other. +- **A refused `pharn init` could still create a backup.** The backup ran before the checks that refuse an install (a symbolic link or a wrong kind of entry on the way), so you saw "Backed up …", nothing was installed, and every retry added another `.pharn-backup/` directory. The checks now run first; a refused install writes nothing. +- **A directory named `pharn.config.json` or `pharn.records.json` left a half-installed project.** Every file was copied, then writing the records failed, leaving no records and no config. `init` now refuses such a project before the first write, like any other entry in the way. +- **`pharn init` refused a symbolic-link `.claude/settings.json` that it would never write.** `init` never overwrites an existing settings file, so a link to an existing file (a settings file kept in a dotfiles repository) is now accepted and left alone. A link that points at nothing is still refused, now with a message about the file: `init` would create its settings file in the link's place. A symbolic link at any other file `init` writes is now reported as a file, with "replace it with a regular file", instead of the directory advice. +- **`pharn init` built its list of files to install five times per run.** It now builds it once and uses it for the prompt, the checks, the backup, the copy and the records. ### Security diff --git a/CLAUDE.md b/CLAUDE.md index 2f5e164b..9a35d46f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -51,9 +51,9 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten 2. `detectArchetypesFromProject` (`lib/detect-archetype.ts`) — merges `package.json` dependency names + a bounded, symlink-safe file-tree walk into an `Archetype[]` (`ssr`/`backend`/`spa`/`lib`). 3. `fetchRepo` (`lib/repo.ts`) — ONE SHA resolve, then download pharn-oss's tarball from codeload and extract it with `lib/tar-extract.ts` into a temp dir (cleaned up in a `finally`; every `process.exit`/`cancelAndExit` happens AFTER it). No fetch dependency, no `git` binary, no cache. 4. `parseCapabilityIndex` (`lib/capability-index.ts`) + `resolveCapabilities` (`lib/resolve-capabilities.ts`) — select capabilities whose `applies` is `universal` or intersects the detected archetypes; skip the rest with a reason. -5. `runArchetypeSummary` (selected + skipped) → `install` / `cancel`; on `install`, `confirmWriteTargets` (`steps/overwrite-check.ts`) warns if any of the install's actual write targets already exist in the cwd — listing them (capped), default No, silent when none; it subsumes the old `confirmOverwriteIfExists` (its set includes `pharn.config.json`) and derives the target set from `lib/install-manifest.ts` (the shared install manifest, also used by `status`'s `diff.ts`). On a re-install it lists the files that differ from upstream (`scanDest`) first, marked `(edited)`; `runInstallArchetype` re-scans immediately before the copy and backs those up with `createBackup` (the same pair `add` uses), naming the directory at creation; and `init` reads the config it replaces TOLERANTLY (any failure → nothing carried over) to keep its `source: 'manual'` entries that the index still has — installed again and recorded `manual` via `manualKeys`. Then `runInstallArchetype` (`steps/install-archetype.ts` → `lib/install-capabilities.ts`) copies the capabilities + fixed product surfaces (including the layout-resolved product-loop boundary contract (`resolveFeaturesReadme`)) into the mirrored layout (flat OR `pharn/`) and writes the archetype `pharn.config.json` (`archetypes`, `capabilities`, `layout`, `skillsVersion` read from the fetched `SKILLS_VERSION` file via `readSkillsVersion`, `modules: []`, plus `models`/`seam` defaults; canonical `CONSTITUTION.md` copied verbatim). The `--archetype` CLI flag is a retained no-op alias for one release. +5. `runArchetypeSummary` (selected + skipped) → `install` / `cancel`; on `install`, `confirmWriteTargets` (`steps/overwrite-check.ts`) warns if any of the install's actual write targets already exist in the cwd — listing them (capped), default No, silent when none; it subsumes the old `confirmOverwriteIfExists` (its set includes `pharn.config.json`) and derives the target set from `lib/install-manifest.ts` (the shared install manifest, also used by `status`'s `diff.ts`). `init` computes the install manifest ONCE (`installManifest`, `steps/install-archetype.ts`) after the summary and hands that same map to the prompt and to `runInstallArchetype` — pre-flight, backup scan, copy and records keys all share it (it depends only on the clone, the selection and the layout; it used to be built five times). On a re-install the prompt lists first, and marks, exactly the files `update` would have SKIPPED: `scanDest` (`lib/dest-drift.ts`) compares each dest with its REAL source (`manifestSources` — the mapped `LICENSE` → `PHARN-LICENSE`/`pharn/LICENSE` included) and classifies a differing file through update's own `decideFileAction` (`force: true` → its `backup` bit) against the records baseline (`reinstallBaseline` = `recordsBaseline` against the replaced config's stamp): `(edited)` = `modified`, `(no pharn record)` = `unrecorded`, `(differs from upstream)` = `unverifiable` (no usable store — every difference is backed up); a file still at its recorded hash is a clean upgrade — unmarked, never backed up. The prompt's labels are ADVISORY: `runInstallArchetype` runs, in order, the destination pre-flight (`prepareInstall`) → the same scan under the lock → `createBackup` (named at creation) → the copy, so a refused install writes nothing, `.pharn-backup/` included, and an edit made while the prompt was open is still saved. `init` also reads the config it replaces TOLERANTLY (any failure → nothing carried over) to keep its `source: 'manual'` entries that the index still has — installed again and recorded `manual` via `manualKeys`. Then `runInstallArchetype` (`steps/install-archetype.ts` → `lib/install-capabilities.ts`) copies the capabilities + fixed product surfaces (including the layout-resolved product-loop boundary contract (`resolveFeaturesReadme`)) into the mirrored layout (flat OR `pharn/`) and writes the archetype `pharn.config.json` (`archetypes`, `capabilities`, `layout`, `skillsVersion` read from the fetched `SKILLS_VERSION` file via `readSkillsVersion`, `modules: []`, plus `models`/`seam` defaults; canonical `CONSTITUTION.md` copied verbatim). The `--archetype` CLI flag is a retained no-op alias for one release. -**`lib/install-capabilities.ts`** is the shared capability copy core — `installCapabilityDirs` (`add`) and `installCapabilities` (`init`); `update` applies the manifest per file instead. `installCapabilityDirs(repoDir, projectRoot, capabilities, paths?)` pre-flights **every** selected capability source (validated name via `CAPABILITY_NAME_RE` + `safeJoin` + existence + symlink rejection) before any write — no partial installs — then copies each griller/lens dir into the mirrored layout. `installCapabilities` additionally copies the fixed product surfaces: product `pharn-*` commands (excluding `pharn-dev-*`), `.cjs` hooks (excluding `*.test.cjs`), `settings.json` (**never** overwritten), the trusted docs, the layout-resolved product-loop boundary contract (`resolveFeaturesReadme`) (layout-invariant; guarded by `findSymlinkComponent`, not just the leaf `isSymlink`, since it is the one root-relative copy with an intermediate directory), `pharn-contracts/`, and `.dev/floor/` minus test files and its `test-fixtures/` subtree (a floor-RELATIVE segment match, single-sourced as `FLOOR_TEST_FIXTURES_DIR` and anchored at the floor root on both sides — an unanchored absolute-path match would prune the whole floor copy under an ancestor of that name). Copying from the untrusted clone is symlink-guarded (`isSymlink` reject / `noSymlinks` filter) and `safeJoin`-contained; file contents are copied verbatim, never executed. The DESTINATION is guarded too: before the first write, `installCapabilities` walks every path the install manifest (`collectExpectedInstallPaths`) says it writes, plus `.claude/settings.json`, with `findSymlinkComponent` and refuses the whole install naming each symlinked component (`cpSync` follows a symlinked `.claude/commands` or `pharn/` out of the project, and `safeJoin` is lexical). The install set is resolved by `lib/capability-index.ts` (`parseCapabilityIndex` — the untrusted-frontmatter → typed `CapabilityIndex` fetch boundary, reading only `name`/`role`/`applies` via a strict field reader) + `lib/resolve-capabilities.ts` (select where `applies` is `universal` or intersects the detected archetypes). The commit SHA is threaded from `fetchRepo` (`repo.sha`) — no separate GitHub fetch (closes the resolve/fetch TOCTOU). +**`lib/install-capabilities.ts`** is the shared capability copy core — `installCapabilityDirs` (`add`) and `installCapabilities` (`init`); `update` applies the manifest per file instead. `installCapabilityDirs(repoDir, projectRoot, capabilities, paths?)` pre-flights **every** selected capability source (validated name via `CAPABILITY_NAME_RE` + `safeJoin` + existence + symlink rejection) before any write — no partial installs — then copies each griller/lens dir into the mirrored layout. `installCapabilities` additionally copies the fixed product surfaces: product `pharn-*` commands (excluding `pharn-dev-*`), `.cjs` hooks (excluding `*.test.cjs`), `settings.json` (**never** overwritten), the trusted docs, the layout-resolved product-loop boundary contract (`resolveFeaturesReadme`) (layout-invariant; guarded by `findSymlinkComponent`, not just the leaf `isSymlink`, since it is the one root-relative copy with an intermediate directory), `pharn-contracts/`, and `.dev/floor/` minus test files and its `test-fixtures/` subtree (a floor-RELATIVE segment match, single-sourced as `FLOOR_TEST_FIXTURES_DIR` and anchored at the floor root on both sides — an unanchored absolute-path match would prune the whole floor copy under an ancestor of that name). Copying from the untrusted clone is symlink-guarded (`isSymlink` reject / `noSymlinks` filter) and `safeJoin`-contained; file contents are copied verbatim, never executed. The DESTINATION is guarded too, by `prepareInstall` — exported so `init` runs it BEFORE its backup, and run again by `installCapabilities` itself, over the manifest passed in (or computed when absent): it walks every path the install manifest (`collectExpectedInstallPaths`) says it writes, plus `.claude/settings.json`, with `findSymlinkComponent` and refuses the whole install, naming linked DIRECTORIES apart from linked FILES (`cpSync` follows a symlinked `.claude/commands` or `pharn/` out of the project, and `safeJoin` is lexical; at a symlinked LEAF it replaces the link — measured on Node 20.13/22/24, nothing lands outside, the user's link is lost). A LIVE `.claude/settings.json` link is allowed — `existsSync` follows it, so the file is preserved and never written — while a DANGLING one is refused. Its type walk (`findTypeCollision`) also refuses a DIRECTORY at `pharn.config.json` / `pharn.records.json`, the two files `init` writes beside the copy (their atomic `rename` fails EISDIR only on a directory). The install set is resolved by `lib/capability-index.ts` (`parseCapabilityIndex` — the untrusted-frontmatter → typed `CapabilityIndex` fetch boundary, reading only `name`/`role`/`applies` via a strict field reader) + `lib/resolve-capabilities.ts` (select where `applies` is `universal` or intersects the detected archetypes). The commit SHA is threaded from `fetchRepo` (`repo.sha`) — no separate GitHub fetch (closes the resolve/fetch TOCTOU). **`lib/validate.ts` is security-sensitive.** Untrusted names, versions, paths, and capability frontmatter are validated against strict regex/enum allowlists (`CAPABILITY_NAME_RE`, `VERSION_RE`, `COPY_FILENAME_RE`, `COMMIT_RE`, the `role`/`applies` enums), checked for `..`, and rejected on control chars. **`safeJoin` lives here** (relocated from the deleted `install-modules.ts`) — the lexical path-containment gate that `install-capabilities.ts`, `diff.ts`, `capability-index.ts`, `layout.ts`, `skills-version.ts`, and `remove.ts` all guard their fs access with, so nothing escapes its base dir (its strict-child sibling `safeChildJoin` — exactly one segment below the base, never the base — guards `remove`'s recursive delete) (`install-capabilities.ts` adds a symlink-aware backstop at the write sites). `toPosix` (the other purely lexical primitive — separator normalization + trailing-slash strip, relocated here once `install-manifest.ts` and `symlink-guard.ts` both needed it) sits beside it. The **physical** counterpart lives in **`lib/symlink-guard.ts`** — `findSymlinkComponent(base, rel)`, the single component walk that `backup.ts` (read side), `apply-update.ts` (write side), and `install-manifest.ts` (twice, skip side) all reach for; it returns the first symlinked component below `base` or `null`, and each **caller owns its failure shape** (backup and apply throw their own `ManifestValidationError` messages, the manifest skips), which is why the core returns a value instead of throwing. Nonexistent components deliberately pass — `applyWrites` creates parents _after_ the walk. That lexical/physical split is the point: `safeJoin` contains the path _string_, `findSymlinkComponent` refuses the path _on disk_. Remote fetches (`skills-version.ts`) use `redirect: 'error'`, an 8s timeout, and a 256KB body cap. Preserve these invariants. diff --git a/docs/commands/init.md b/docs/commands/init.md index 84d55587..630e6cbf 100644 --- a/docs/commands/init.md +++ b/docs/commands/init.md @@ -130,10 +130,28 @@ Lists the **selected** capabilities (name, role, and why — `universal` or the After you choose **install**, `init` checks which of its **actual write targets** (the selected capability dirs, product `pharn-*` commands, `.cjs` hooks, `pharn/features/README.md`, the contracts, core and floor dirs, the trusted docs, pharn's `LICENSE` copy, and `pharn.config.json`) already exist in your project. If any do, it lists them (capped at 10, then "…and N more") and asks you to confirm before overwriting — default **no**. When `pharn.config.json` is one of them, the warning also names the `skillsVersion` your existing config records, so you can see which version you are about to replace (read locally, never fetched; the clause is simply omitted if that file cannot be read). If none do, there is no prompt (zero friction). `.claude/settings.json` is never overwritten, so it is excluded from the check. -**Re-running `init` over an existing install.** Existing files whose bytes **differ from upstream** — your -edits — are listed **first** and marked `(edited)`, and the prompt says how many there are. If you -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). +**Re-running `init` over an existing install.** `init` overwrites every file it installs, so before it +does, it copies the ones you could lose to `.pharn-backup//` and prints that directory as soon +as it is created. Which files those are is decided the way [`pharn update`](update.md) decides what to +skip: against the hashes in `pharn.records.json` that the previous install wrote. The prompt lists these +files **first**, marked, and says how many there are of each kind: + +| Marked | Meaning | Backed up | +| ------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------ | --------- | +| `(edited)` | The file changed since pharn wrote it — your edit | Yes | +| `(no pharn record)` | It differs from upstream and the records have no entry for it | Yes | +| `(differs from upstream)` | It differs from upstream, and there is no usable `pharn.records.json` to tell your edits from upstream changes, so every difference counts | Yes | +| _(not marked)_ | Byte-identical to upstream, or still exactly what pharn wrote (upstream simply moved on) — a clean upgrade | No | + +`pharn.records.json` counts as usable only when it matches the `skillsVersion` and `commit` of the +config being replaced, as for `update`. Records are kept by path, so a file of yours at a path the +previous install did not write — for example your own file at a path a newer pharn now installs to — +has no record and is backed up, marked `(no pharn record)`. A change of layout (flat → `pharn/`) +installs to new paths and leaves the old copies where they are (`init` never deletes), so it backs up +nothing extra. pharn's own `LICENSE` copy (`PHARN-LICENSE` / `pharn/LICENSE`) is compared with +upstream's `LICENSE`, like every other file with its source. The marks are a preview: `init` checks again just before it copies, +under the project lock, and that check decides what is backed up — so an edit you make while the prompt +is open is still saved. 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` — 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 @@ -148,6 +166,16 @@ config the other commands would accept, and your own keys from any config that p a config that is not valid JSON carries nothing over. 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. +**When `init` refuses to install.** Before anything is written — the backup included — `init` checks +every path it is about to write and stops, naming each problem, if the copy could not finish: a +symbolic link on the way (writing through a linked directory would put files outside your project, and +a linked file would be replaced by pharn's copy), or an entry of the wrong kind (a directory where +pharn writes a file, or a file where it needs a directory — `pharn.config.json` and +`pharn.records.json` included). A refused install writes nothing: no files, no config, no records and +no `.pharn-backup/`. A symbolic link at `.claude/settings.json` is fine while it points at an existing +file — `init` never writes an existing settings file — and is refused only when its target does not +exist. See [Something in your project is in the way](../troubleshooting.md#something-in-your-project-is-in-the-way). + ### 7. Install | Action | Behavior | diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 17f79a9a..ca3a2893 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -150,7 +150,11 @@ or cancel to exit cleanly (code 0); the default is **no**. - If **nothing** conflicts, there is no prompt at all. - `.claude/settings.json` is never overwritten, so it never triggers the warning. -- On a re-install the list is long, so it is capped (first 10 shown, then "…and N more"). +- On a re-install the list is long, so it is capped (first 10 shown, then "…and N more"). The files + `init` backs up before overwriting are listed first, marked `(edited)`, `(no pharn record)` or + `(differs from upstream)` — see [Re-running `init`](commands/init.md#6-summary) for what each + means. With a usable `pharn.records.json`, a file pharn wrote that upstream has since changed is + not marked: overwriting it loses nothing. ## Capabilities could not be fetched @@ -236,9 +240,27 @@ a directory, or a directory where you have a file. Nothing was written. Before its first write, `init` checks every path it is about to install. If one of them exists in your project as the wrong kind of entry (a directory where pharn writes a file, or a file where it needs a -directory), `init` stops and names each one (up to five, then a count). Your project is left exactly as -it was. Move or rename the named entries and re-run `pharn init`. The optional `features/README.md` is -the one exception: a collision there is skipped rather than refused. +directory), `init` stops and names each one (up to five, then a count). That includes a **directory** +named `pharn.config.json` or `pharn.records.json`, the two files `init` writes beside the copy. Your +project is left exactly as it was — no backup directory either. Move or rename the named entries and +re-run `pharn init`. The optional `features/README.md` is the one exception: a collision there is +skipped rather than refused. + +```text +Refusing to install: pharn-review is a symbolic link inside the project, so writing through it would +put files OUTSIDE the project. Replace it with a real directory (or remove it). Nothing was written; … +``` + +A **symbolic link** on the way is refused the same way, before anything is written: + +- A linked **directory** (`.claude/`, `.claude/commands`, `pharn/`, a capability directory, …): the + copy would follow it and write outside your project. Replace it with a real directory. +- A linked **file** that pharn writes (`… is a symbolic link where pharn writes a file`): the copy + would replace your link with pharn's file. Replace it with a regular file, or remove it. +- `.claude/settings.json` as a link is **fine** while it points at an existing file — `init` never + writes over an existing settings file, so a settings file kept in a dotfiles repository is left alone. + It is refused only when the link points at nothing, because `init` would then create the file in the + link's place. Create the target, or remove the link, and re-run. ### When the `PHARN_DEBUG` hint appears diff --git a/src/commands/init.ts b/src/commands/init.ts index 4ef9bd3d..e38d96e1 100644 --- a/src/commands/init.ts +++ b/src/commands/init.ts @@ -23,6 +23,8 @@ import { runGitPrereq } from '../steps/prereqs.js'; import { confirmWriteTargets } from '../steps/overwrite-check.js'; import { runArchetypeSummary } from '../steps/archetype-summary.js'; import { + installManifest, + reinstallBaseline, runInstallArchetype, type InstallCarry, } from '../steps/install-archetype.js'; @@ -183,11 +185,24 @@ async function runInitArchetype(): Promise { // that disposes of the clone. 'decline' and 'cancel' are indistinguishable // to the user here — same message, same exit 0 — but only one of them used // to leak the clone. + // + // The install manifest — every file this install writes, dest → source — + // computed ONCE, here, and handed to the prompt and to the install (its + // pre-flight, backup scan, copy and records). It depends only on the + // clone, the selection and the clone's layout, and none of those change + // during the run; it used to be rebuilt five times. Only the HASHING is + // repeated, deliberately: the install re-scans under the lock, so an edit + // made while the prompt was open is still backed up. + const manifest = + action === 'install' ? installManifest(repo.dir, selection) : null; const overwrite = - action === 'install' - ? await confirmWriteTargets(repo.dir, cwd, selection) + manifest !== null + ? await confirmWriteTargets(repo.dir, cwd, selection, { + manifest, + records: reinstallBaseline(cwd, carry.previousStamp), + }) : 'cancel'; - if (overwrite === 'proceed') { + if (manifest !== null && overwrite === 'proceed') { // Reuse the SHA the tree was pinned to (recorded == fetched, or null when // the branch was floated — LIMITS.md §3b); no separate fetch (TOCTOU). const commit = repo.sha; @@ -229,6 +244,7 @@ async function runInitArchetype(): Promise { selection, commit, carry, + manifest, ); }); outcome = 'installed'; diff --git a/src/lib/dest-drift.ts b/src/lib/dest-drift.ts index ad2f6156..2526e21a 100644 --- a/src/lib/dest-drift.ts +++ b/src/lib/dest-drift.ts @@ -1,24 +1,46 @@ import { lstatSync } from 'node:fs'; +import { relative } from 'node:path'; import { sha256File } from './hash.js'; +import type { FileRecords } from './install-records.js'; import { findSymlinkComponent } from './symlink-guard.js'; -import { safeJoin } from './validate.js'; +import { decideFileAction, type FileDecision } from './update-decision.js'; +import { safeJoin, toPosix } from './validate.js'; // --------------------------------------------------------------------------- // The destination scan — what a capability copy is about to destroy. It answers // two questions in ONE walk, because they are two readings of the same fact: // -// drifted[] — the dest exists as a regular file whose bytes DIFFER from the -// clone's. `pharn add` backs exactly these up (lib/backup.ts) -// before its first write, so an overwrite stops being final. +// drifted[] — the dest exists as a regular file that `update` would have +// SKIPPED: its bytes differ from the clone's and pharn cannot +// show they are its own (below). The caller backs exactly these +// up (lib/backup.ts) before its first write, so an overwrite +// stops being final. `labels` says why, per file. // unsafe[] — the dest path crosses a SYMLINKED component. These must not be // copied at all: see below. `add` refuses the whole install. // -// Why `add` needs this and `init`/`update` do not: `init` asks first -// (steps/overwrite-check.ts) and `update` decides per file against -// pharn.records.json (lib/update-decision.ts). `add` did neither — and `update` -// itself manufactures the sequence that makes that reachable: a dropped +// Two callers. `pharn add` — which had no edit protection at all, while `update` +// itself manufactures the sequence that makes one necessary: a dropped // capability's files are LEFT on disk (update never deletes), the user edits -// them, and a later `add` of that capability is not a config no-op. +// them, and a later `add` of that capability is not a config no-op. And a +// re-run `pharn init`, which overwrites everything it installs: its prompt marks +// these files (steps/overwrite-check.ts) and its install step backs them up +// (steps/install-archetype.ts). +// +// "Drifted" is update's definition, not a second one (P4): a differing file is +// drifted exactly when update's decision table (lib/update-decision.ts → +// decideFileAction) would SKIP it — so a forced write backs it up. With a +// usable records baseline that is `modified` (changed since pharn wrote it) or +// `unrecorded` (pharn has no record of it); a file still at its recorded hash is +// pharn's own bytes, merely outdated — a clean upgrade, never backed up. With +// no usable baseline (`add`, a first install, an absent / corrupt / foreign +// store) every differing file is `unverifiable` and backed up: conservative, +// never a guess. +// +// Each rel is compared with its REAL source. The install manifest maps one dest +// to a different clone path — upstream's `LICENSE` lands at `PHARN-LICENSE` / +// `pharn/LICENSE` (lib/layout.ts) — so `sources` carries that pairing; a rel +// absent from it is read at the same path in the clone, which is every path +// `add` scans. // // WHY `unsafe` IS A REFUSAL AND NOT A SKIP — measured, not assumed. Node's // `cpSync(from, to, { recursive: true, force: true, filter: noSymlinks })` guards @@ -50,14 +72,17 @@ import { safeJoin } from './validate.js'; // `identical → no-op`. A drop-then-re-add of unedited files must stay silent, or // every such add would litter `.pharn-backup/` with copies of bytes nobody lost. // -// Determinism (P5): every branch is a type check or a byte comparison — a -// symlink test, `isFile()`, `sha256 !==`. No judgment, no third outcome. +// Determinism (P5): every branch is a type check, a byte comparison or a row of +// update's table — a symlink test, `isFile()`, a sha256 equality. No judgment. // // Trust (P2): `repoDir` is an untrusted tree — a codeload tarball fetched and -// extracted into a temp dir (lib/repo.ts, lib/tar-extract.ts) — and `rels` are -// names read from it. Both `safeJoin`s run BEFORE either walk, so a rel that -// escapes either base is refused LOUDLY and no catch below can bury it. File -// contents are hashed, never parsed or executed. +// extracted into a temp dir (lib/repo.ts, lib/tar-extract.ts) — and `rels` and +// `sources` are names read from it. Both `safeJoin`s run BEFORE either walk, so +// a path that escapes either base is refused LOUDLY and no catch below can bury +// it. File contents are hashed, never parsed or executed. `records` is local, +// stamp-checked data (lib/install-records.ts), and it can only take a file OUT +// of the backup set: one whose bytes are still exactly what pharn recorded +// writing — the files update upgrades without asking. // // NAMED RESIDUAL — this scan is a CHECK, not a LOCK (P0/P7, and stated rather // than implied). It reads the destination at time T; `installCapabilityDirs` @@ -83,32 +108,47 @@ export interface UnsafeDest { link: string; } +/** + * Why a drifted file is backed up — update's SKIP label for it + * (lib/update-decision.ts): `modified` changed since pharn wrote it, + * `unrecorded` has no record, `unverifiable` had no usable records to ask. + */ +export type DriftLabel = 'modified' | 'unrecorded' | 'unverifiable'; + export interface DestScan { - /** Existing dest files whose bytes differ from the clone's — back these up. */ + /** Existing dest files update would have skipped — back these up. */ drifted: string[]; + /** Why each `drifted` rel is in the set. */ + labels: ReadonlyMap; /** Paths the copy would write through a symlink — refuse the install. */ unsafe: UnsafeDest[]; } /** - * Scan the destination for `rels` (project-root-relative posix paths, which are - * also their paths inside the mirrored clone). Both lists are sorted; nothing is - * written. + * Scan the destination for `rels` (project-root-relative posix paths). Each is + * compared with `sources.get(rel)` in the clone, or with the same path there + * when `sources` has no entry for it. `records` is the validated baseline, or + * null when there is none to trust. Both lists are sorted; nothing is written. */ export function scanDest(params: { repoDir: string; projectRoot: string; rels: readonly string[]; + sources?: ReadonlyMap; + records?: FileRecords | null; }): DestScan { - const { repoDir, projectRoot, rels } = params; + const { repoDir, projectRoot, rels, sources } = params; + const records = params.records ?? null; const drifted: string[] = []; + const labels = new Map(); const unsafe: UnsafeDest[] = []; for (const rel of rels) { - // Containment first, on BOTH sides: an escaping rel throws here rather than + const srcRel = sources?.get(rel) ?? rel; + // Containment first, on BOTH sides: an escaping path throws here rather than // being swallowed by the unreadable-catch below (P2 is never a silent skip). const dest = safeJoin(projectRoot, rel); - const src = safeJoin(repoDir, rel); + const src = safeJoin(repoDir, srcRel); const destLink = symlinkComponent(projectRoot, rel); if (destLink === UNREADABLE) continue; // ENOTDIR — see symlinkComponent @@ -125,21 +165,73 @@ export function scanDest(params: { // is defense in depth for a function that is exported and takes a plain // string[], the same reasoning install-records.ts records for shape-checking // a key it never path-joins. - if (symlinkComponent(repoDir, rel) !== null) continue; + if (symlinkComponent(repoDir, srcRel) !== null) continue; if (!lstatSync(src, { throwIfNoEntry: false })?.isFile()) continue; // sha256File's failure on a path that IS a regular file (EACCES) deliberately - // PROPAGATES: it aborts `add` before any write, with the clone cleaned up. - // Treating an unreadable file as "no drift" would clobber it. - if (sha256File(dest) !== sha256File(src)) drifted.push(rel); + // PROPAGATES: it aborts the caller before any write, with the clone cleaned + // up. Treating an unreadable file as "no drift" would clobber it. + // + // `force: true` because both callers overwrite regardless: the question is + // only which overwrites need a backup, and that is exactly the table's + // `backup` bit. Byte-identical is row 2 (no-op), never drift. + const label = driftLabel( + decideFileAction({ + diskHash: sha256File(dest), + latestHash: sha256File(src), + recordedHash: + records !== null && Object.hasOwn(records, rel) + ? (records[rel] ?? null) + : null, + recordsAvailable: records !== null, + force: true, + }), + ); + if (label !== null) { + drifted.push(rel); + labels.set(rel, label); + } } return { drifted: drifted.sort(), + labels, unsafe: unsafe.sort((a, b) => (a.rel < b.rel ? -1 : a.rel > b.rel ? 1 : 0)), }; } +/** + * The install manifest (lib/install-manifest.ts → collectExpectedInstallPaths, + * `dest → ABSOLUTE clone source`) as `scanDest`'s `sources`: dest → clone- + * RELATIVE source. Every entry is converted, so the one mapped pair (LICENSE) + * needs no special case here; `scanDest` re-contains each value under the + * clone before reading it. + */ +export function manifestSources( + repoDir: string, + manifest: ReadonlyMap, +): Map { + const sources = new Map(); + for (const [dest, from] of manifest) { + sources.set(dest, toPosix(relative(repoDir, from))); + } + return sources; +} + +/** + * The skip label a forced write backs up for, or null when it needs none. + * `backup` is the whole decision: any decision that asks for one is backed up. + * The label only explains it, and a label outside the two the records can prove + * reads as `unverifiable` — "pharn cannot tell" — never as "no backup". + */ +function driftLabel(decision: FileDecision): DriftLabel | null { + if (!decision.backup) return null; + const { label } = decision; + return label === 'modified' || label === 'unrecorded' + ? label + : 'unverifiable'; +} + /** `symlinkComponent`'s third outcome: the walk could not be completed. */ const UNREADABLE = Symbol('unreadable'); diff --git a/src/lib/install-capabilities.ts b/src/lib/install-capabilities.ts index 1b429dc5..c9cc8377 100644 --- a/src/lib/install-capabilities.ts +++ b/src/lib/install-capabilities.ts @@ -23,7 +23,11 @@ import { resolveFeaturesReadme, type LayoutPaths, } from './layout.js'; -import { collectExpectedInstallPaths } from './install-manifest.js'; +import { + collectExpectedInstallPaths, + PHARN_CONFIG_FILE, +} from './install-manifest.js'; +import { RECORDS_FILE } from './install-records.js'; import { findSymlinkComponent, findTypeCollision } from './symlink-guard.js'; import type { InstalledCapability, Layout, Selection } from '../types.js'; @@ -146,29 +150,73 @@ export function installCapabilityDirs( return installed; } -export function installCapabilities( +/** What the destination pre-flight checked, for the copy that follows it. */ +export interface PreparedInstall { + // The layout mirrored from the clone (flat OR pharn/). + paths: LayoutPaths; + // Where this clone keeps the optional features README (resolveFeaturesReadme). + featuresRel: string; + // Every file the install writes, dest → source (collectExpectedInstallPaths). + manifest: ReadonlyMap; +} + +/** + * The install's destination pre-flight: refuse, before anything is written, + * every project the copy could not finish in — a path through a symlink (see + * assertDestinationsInProject) or a type in the way (see assertDestinationTypes). + * Read-only; throws ManifestValidationError, or returns what it checked. + * + * Exported so `init` can run it BEFORE its backup (steps/install-archetype.ts): + * a refused install then writes nothing at all — no `.pharn-backup/` either. + * `installCapabilities` still runs it itself, so no caller can copy without it. + * + * `manifest` is the install manifest for these capabilities at the clone's + * layout, when the caller already computed it (init does, once per run); + * absent → computed here. + */ +export function prepareInstall( repoDir: string, projectRoot: string, - selection: Selection, -): InstallCapabilitiesResult { + capabilities: InstalledCapability[], + manifest?: ReadonlyMap, +): PreparedInstall { // Mirror whichever layout the fetched clone has (flat OR the relocated pharn/). // The resolved relative paths are the clone SOURCE and the project DEST at once — // the CLI never rewrites copied file contents (lib/layout.ts). const paths = layoutPaths(detectLayout(repoDir)); const featuresRel = resolveFeaturesReadme(repoDir, paths.layout); + const expected = + manifest ?? + collectExpectedInstallPaths({ + repoDir, + capabilities, + layout: paths.layout, + }); - // Destination pre-flight, before the FIRST write: nothing below may follow a - // symlinked project directory out of the project (see assertDestinationsInProject). - assertDestinationsInProject(repoDir, projectRoot, selection.selected, paths); + // Nothing below may follow a symlinked project directory out of the project + // (see assertDestinationsInProject)... + assertDestinationsInProject(projectRoot, expected); // ...and nothing may start writing into a tree whose TYPES it cannot write: // a collision found by `cpSync` part-way leaves a half-installed project with // no config and no records (see assertDestinationTypes). - assertDestinationTypes( + assertDestinationTypes(projectRoot, expected, featuresRel); + + return { paths, featuresRel, manifest: expected }; +} + +export function installCapabilities( + repoDir: string, + projectRoot: string, + selection: Selection, + // The install manifest, when the caller already computed it (see + // prepareInstall). The pre-flight runs here either way, before the first write. + manifest?: ReadonlyMap, +): InstallCapabilitiesResult { + const { paths, featuresRel } = prepareInstall( repoDir, projectRoot, selection.selected, - paths, - featuresRel, + manifest, ); // Copy the selected capability dirs (pre-flighted; no partial installs). @@ -193,6 +241,8 @@ export function installCapabilities( }); // --- settings.json: NEVER overwrite the user's existing one (grill F1) ----- + // `existsSync` FOLLOWS a symlink, so a live link here counts as existing and + // is never written; a dangling one was refused by the pre-flight. const settingsFrom = safeJoin(repoDir, CLAUDE_SETTINGS_FILE); const settingsTo = safeJoin(projectRoot, CLAUDE_SETTINGS_FILE); const settingsPreserved = existsSync(settingsTo); @@ -335,49 +385,89 @@ export function installCapabilities( * take, applied once to the full write set: every file the install manifest * says this install writes, plus the user-owned `.claude/settings.json`. * + * A symlinked LEAF is refused too, for a different reason, and the message says + * which: `cpSync` does not follow a link at the file it writes — measured on + * Node 20.13, 22 and 24, it REPLACES the link with a regular file, so nothing + * lands outside the project, but the user's link is gone without a word. + * + * `.claude/settings.json` is the one leaf exempt while its link is LIVE: + * `installCapabilities` never writes an existing settings file (`existsSync` + * follows the link), so there is nothing to refuse. A DANGLING link there does + * not exist to `existsSync`, so the install would write upstream's settings in + * its place — replacing the link — and is refused like any other leaf. A + * symlinked `.claude/` is an intermediate component, refused as ever. + * * ENOTDIR from the walk (a component below a REGULAR file) is not a symlink and - * is left to the copy itself, which fails on that tree as before. + * is left to assertDestinationTypes, which names it. * * Residual (advisory): a link created between this walk and the copy (TOCTOU) * is not covered — the same residual `update` names. */ function assertDestinationsInProject( - repoDir: string, projectRoot: string, - capabilities: InstalledCapability[], - paths: LayoutPaths, + manifest: ReadonlyMap, ): void { - const rels = [ - ...collectExpectedInstallPaths({ - repoDir, - capabilities, - layout: paths.layout, - }).keys(), - CLAUDE_SETTINGS_FILE, - ]; - const linked = new Set(); - for (const rel of rels) { - let hit: string | null; - try { - hit = findSymlinkComponent(projectRoot, rel); - } catch { - hit = null; + const dirs = new Set(); + const files = new Set(); + for (const rel of manifest.keys()) { + const hit = symlinkedComponent(projectRoot, rel); + if (hit !== null) (hit === rel ? files : dirs).add(hit); + } + const settingsHit = symlinkedComponent(projectRoot, CLAUDE_SETTINGS_FILE); + if (settingsHit === CLAUDE_SETTINGS_FILE) { + if (!existsSync(safeJoin(projectRoot, CLAUDE_SETTINGS_FILE))) { + files.add(settingsHit); + } + } else if (settingsHit !== null) { + dirs.add(settingsHit); + } + if (dirs.size === 0 && files.size === 0) return; + + const sentences: string[] = []; + if (dirs.size > 0) { + const one = dirs.size === 1; + sentences.push( + `${shownList(dirs)} ${one ? 'is a symbolic link' : 'are symbolic links'} inside the project, so writing through ${one ? 'it' : 'them'} would put files OUTSIDE the project. Replace ${one ? 'it' : 'each'} with a real directory (or remove ${one ? 'it' : 'them'}).`, + ); + } + if (files.size > 0) { + const one = files.size === 1; + sentences.push( + `${shownList(files)} ${one ? 'is a symbolic link' : 'are symbolic links'} where pharn writes a file, so the install would replace ${one ? 'it' : 'them'} with pharn's copy and the link would be lost. Replace ${one ? 'it' : 'each'} with a regular file (or remove ${one ? 'it' : 'them'}).`, + ); + if (files.has(CLAUDE_SETTINGS_FILE)) { + sentences.push( + `A link at ${CLAUDE_SETTINGS_FILE} is fine while it points at an existing file — pharn never writes over an existing one — but this one points at nothing.`, + ); } - if (hit !== null) linked.add(hit); } - if (linked.size === 0) return; - const shown = [...linked].sort(); - const list = - shown.length > MAX_LINKED_SHOWN - ? `${shown.slice(0, MAX_LINKED_SHOWN).join(', ')} and ${shown.length - MAX_LINKED_SHOWN} more` - : shown.join(', '); throw new ManifestValidationError( - `Refusing to install: ${list} ${shown.length === 1 ? 'is a symbolic link' : 'are symbolic links'} inside the project, so writing through ${shown.length === 1 ? 'it' : 'them'} would put files OUTSIDE the project. Nothing was written. Replace ${shown.length === 1 ? 'it' : 'each'} with a real directory (or remove ${shown.length === 1 ? 'it' : 'them'}) and re-run \`pharn init\`.`, + `Refusing to install: ${sentences.join(' ')} Nothing was written; re-run \`pharn init\` once that is done.`, ); } +/** + * `findSymlinkComponent`, with its ENOTDIR (a component below a regular file) + * read as "no symlink here" — assertDestinationTypes reports that path. + */ +function symlinkedComponent(projectRoot: string, rel: string): string | null { + try { + return findSymlinkComponent(projectRoot, rel); + } catch { + return null; + } +} + const MAX_LINKED_SHOWN = 5; +/** A sorted, capped, comma-joined list of project paths for a refusal. */ +function shownList(items: ReadonlySet): string { + const shown = [...items].sort(); + return shown.length > MAX_LINKED_SHOWN + ? `${shown.slice(0, MAX_LINKED_SHOWN).join(', ')} and ${shown.length - MAX_LINKED_SHOWN} more` + : shown.join(', '); +} + /** * Refuse the whole install when any path it would write collides by TYPE with * what is already in the project: an existing component on the way that is not @@ -387,36 +477,39 @@ const MAX_LINKED_SHOWN = 5; * and no records. Runs AFTER assertDestinationsInProject, so a symlink is still * reported as a symlink. * + * The two files `init` writes BESIDE the copy are walked too: a DIRECTORY at + * `pharn.config.json` or `pharn.records.json` passed every check before, so the + * whole tree was copied and then the atomic write's `rename` failed (EISDIR), + * leaving no records and no config. Only a directory blocks that rename — it + * replaces a file, a FIFO or a symlink (even one to a directory) in place — so + * a directory is the one type refused there. + * * Out of the walk: `.claude/settings.json` (never overwritten — `existsSync` * skips it) and the optional features README (`destAcceptsWrite` skips it on a * collision, by contract). Each path's walk stops at its FIRST non-directory * component: nothing below it can be lstat-ed. */ function assertDestinationTypes( - repoDir: string, projectRoot: string, - capabilities: InstalledCapability[], - paths: LayoutPaths, + manifest: ReadonlyMap, featuresRel: string, ): void { const colliding = new Set(); - for (const rel of collectExpectedInstallPaths({ - repoDir, - capabilities, - layout: paths.layout, - }).keys()) { + for (const rel of manifest.keys()) { if (rel === featuresRel) continue; const hit = findTypeCollision(projectRoot, rel); if (hit !== null) colliding.add(hit); } + for (const rel of [PHARN_CONFIG_FILE, RECORDS_FILE]) { + const stat = lstatSync(safeJoin(projectRoot, rel), { + throwIfNoEntry: false, + }); + if (stat?.isDirectory()) colliding.add(rel); + } if (colliding.size === 0) return; - const shown = [...colliding].sort(); - const list = - shown.length > MAX_LINKED_SHOWN - ? `${shown.slice(0, MAX_LINKED_SHOWN).join(', ')} and ${shown.length - MAX_LINKED_SHOWN} more` - : shown.join(', '); + const one = colliding.size === 1; throw new ManifestValidationError( - `Refusing to install: ${list} ${shown.length === 1 ? 'is in the way' : 'are in the way'} — pharn needs a file where you have a directory, or a directory where you have a file. Nothing was written. Move or rename ${shown.length === 1 ? 'it' : 'them'} and re-run \`pharn init\`.`, + `Refusing to install: ${shownList(colliding)} ${one ? 'is in the way' : 'are in the way'} — pharn needs a file where you have a directory, or a directory where you have a file. Nothing was written. Move or rename ${one ? 'it' : 'them'} and re-run \`pharn init\`.`, ); } diff --git a/src/lib/install-manifest.ts b/src/lib/install-manifest.ts index 886801fe..4d48d3fc 100644 --- a/src/lib/install-manifest.ts +++ b/src/lib/install-manifest.ts @@ -237,13 +237,19 @@ export function conflictingWriteTargets(params: { projectRoot: string; capabilities: InstalledCapability[]; layout: Layout; + // The manifest for exactly these inputs, when the caller already has it: + // `init` computes it ONCE per run and hands the same map to this prompt and + // to the install (commands/init.ts). Absent → computed here. + expected?: ReadonlyMap; }): string[] { const { repoDir, projectRoot, capabilities, layout } = params; - const expected = collectExpectedInstallPaths({ - repoDir, - capabilities, - layout, - }); + const expected = + params.expected ?? + collectExpectedInstallPaths({ + repoDir, + capabilities, + layout, + }); const candidates = new Set(expected.keys()); candidates.add(PHARN_CONFIG_FILE); const conflicts: string[] = []; diff --git a/src/steps/install-archetype.ts b/src/steps/install-archetype.ts index 9518aaef..16537467 100644 --- a/src/steps/install-archetype.ts +++ b/src/steps/install-archetype.ts @@ -2,10 +2,13 @@ import { readFileSync } from 'node:fs'; import { log, outro, spinner } from '@clack/prompts'; import pc from 'picocolors'; import { FIRST_FEATURE_COMMAND, REPO_URL } from '../lib/constants.js'; -import { installCapabilities } from '../lib/install-capabilities.js'; +import { + installCapabilities, + prepareInstall, +} from '../lib/install-capabilities.js'; import { collectExpectedInstallPaths } from '../lib/install-manifest.js'; import { detectLayout, layoutPaths } from '../lib/layout.js'; -import { scanDest } from '../lib/dest-drift.js'; +import { manifestSources, scanDest } from '../lib/dest-drift.js'; import { createBackup } from '../lib/backup.js'; import { buildRecords, @@ -55,6 +58,23 @@ export interface InstallCarry { previousStamp: { skillsVersion: string; commit: string | null } | null; } +/** + * Every file installing `selection` from `repoDir` writes, dest → source, at + * the clone's own layout (lib/install-manifest.ts). init computes it ONCE per + * run, before its overwrite prompt, and passes the same map to the prompt and + * to runInstallArchetype. + */ +export function installManifest( + repoDir: string, + selection: Selection, +): Map { + return collectExpectedInstallPaths({ + repoDir, + capabilities: selection.selected, + layout: detectLayout(repoDir), + }); +} + const NO_CARRY: InstallCarry = { manualKeys: new Set(), kept: [], @@ -73,32 +93,49 @@ export async function runInstallArchetype( selection: Selection, commit: string | null, carry: InstallCarry = NO_CARRY, + // The install manifest for this selection, when the caller already computed + // it — init computes it ONCE per run and passes the same map to its prompt + // and here. Absent → computed by the pre-flight below. + manifest?: ReadonlyMap, ): Promise { const startedAt = Date.now(); - // Back up every existing file this install is about to overwrite whose bytes - // DIFFER from upstream — a re-run `init` used to discard local edits with no - // copy anywhere. The same scan + backup `pharn add` uses (lib/dest-drift.ts, - // lib/backup.ts), taken HERE, immediately before the copy, not at the prompt: - // an edit made while the prompt was open must be covered too. Byte-identical - // files are not edits. A path through a symlinked directory is left out — the - // install's own pre-flight refuses the whole run over it (install-capabilities.ts). + // 1. The destination pre-flight, FIRST: a project the copy cannot finish in + // (a symlink on the way, a type in the way) is refused before anything is + // written — the backup below included, so a refused re-install leaves no + // `.pharn-backup/` behind and prints no "Backed up" line for a copy that + // never happens. installCapabilities repeats it immediately before copying. + const prepared = prepareInstall(repoDir, cwd, selection.selected, manifest); + + // 2. Back up every existing file this install is about to overwrite that + // `update` would have SKIPPED — a re-run `init` used to discard local edits + // with no copy anywhere, then over-corrected into backing up every file that + // differed from upstream, calling pharn's own outdated bytes "your edits". + // The records baseline decides (lib/dest-drift.ts, through update's own + // table): a file still at the hash pharn recorded writing is a clean upgrade; + // one that changed since, or that pharn has no record of, is backed up; with + // no usable store every differing file is. Each file is compared with its + // REAL source, so the mapped LICENSE is covered. + // + // Taken HERE, under init's lock and immediately before the copy, not at the + // prompt: an edit made while the prompt was open must be covered too, so this + // scan is the authoritative one — the prompt's labels are advisory. + const baseline = reinstallBaseline(cwd, carry.previousStamp); const scan = scanDest({ repoDir, projectRoot: cwd, - rels: [ - ...collectExpectedInstallPaths({ - repoDir, - capabilities: selection.selected, - layout: detectLayout(repoDir), - }).keys(), - ], + rels: [...prepared.manifest.keys()], + sources: manifestSources(repoDir, prepared.manifest), + records: baseline, }); + // `unsafe` is empty after a passing pre-flight; non-empty means a link + // appeared since, and installCapabilities' own pre-flight refuses the run — + // so no backup is made for a copy that will not happen. if (scan.drifted.length > 0 && scan.unsafe.length === 0) { const backupDir = createBackup(cwd, scan.drifted); // Named at creation, so the pointer survives anything that fails after it. log.info( - `Backed up ${scan.drifted.length} edited file(s) to ${backupDir} before overwriting.`, + `Backed up ${scan.drifted.length} file(s) to ${backupDir} before overwriting.`, ); } @@ -110,7 +147,12 @@ export async function runInstallArchetype( let layout: Layout; let docsWritten: string[]; try { - const result = installCapabilities(repoDir, cwd, selection); + const result = installCapabilities( + repoDir, + cwd, + selection, + prepared.manifest, + ); capabilities = result.capabilities; settingsPreserved = result.settingsPreserved; layout = result.layout; @@ -195,15 +237,15 @@ export async function runInstallArchetype( // with the config values written beside it. This is the baseline `pharn update` // compares against so it can tell pharn's bytes from the user's edits // (lib/install-records.ts). Without it every later update is degraded. + // + // Keyed by the SAME manifest the copy just applied — it depends only on the + // clone, the selection and the layout, none of which change during a run. await writeRecords(cwd, { skillsVersion, commit, files: { - ...keptRecords(cwd, carry, layout), - ...buildRecords( - cwd, - collectExpectedInstallPaths({ repoDir, capabilities, layout }).keys(), - ), + ...keptRecords(baseline, carry, layout), + ...buildRecords(cwd, prepared.manifest.keys()), }, }); // The config this install replaces may hold keys pharn does not own — @@ -262,6 +304,33 @@ export async function runInstallArchetype( ); } +/** + * The records a re-install tells pharn's bytes from the user's by: the store + * the install replaces, through `update`'s own gate (`recordsBaseline`) against + * the stamp of the config it replaces. `null` — a first install, or a store + * that is absent, corrupt or stamped for another install state — means every + * file that differs from upstream is `unverifiable`, and so backed up: never + * minted, never blessed, never guessed about. + * + * Keyed by PATH, at the layout of the install that wrote it. A re-install + * whose clone changed layout (flat → pharn/) writes to new paths, which the + * project does not have yet, and leaves the old files in place (init never + * deletes) — so it backs up nothing extra. Only a file of the user's own + * already at one of the new paths has no record there, and is backed up + * (`unrecorded`). Pinned by tests/init-archetype.test.ts. + * + * Exported for init's prompt (commands/init.ts), which labels the same files + * before the lock; the scan above, under the lock, is the one that acts. + */ +export function reinstallBaseline( + cwd: string, + previousStamp: InstallCarry['previousStamp'], +): FileRecords | null { + return previousStamp === null + ? null + : recordsBaseline(readRecords(cwd), previousStamp).records; +} + /** * The records of every kept (frozen) capability, carried from the store this * install replaces — the pair `update` uses for its frozen entries @@ -270,18 +339,16 @@ export async function runInstallArchetype( * 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`. + * layout, as in `update`. `baseline` is reinstallBaseline's, read before the + * copy — the store exactly as this install found it. */ function keptRecords( - cwd: string, + baseline: FileRecords | null, 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); + if (carry.kept.length === 0 || baseline === null) return {}; + return recordsUnderCapabilities(baseline, layoutPaths(layout), carry.kept); } /** diff --git a/src/steps/overwrite-check.ts b/src/steps/overwrite-check.ts index 6ef48f42..5ed3f0dc 100644 --- a/src/steps/overwrite-check.ts +++ b/src/steps/overwrite-check.ts @@ -1,12 +1,18 @@ import { readFileSync } from 'node:fs'; import { confirm, isCancel, log } from '@clack/prompts'; import { + collectExpectedInstallPaths, conflictingWriteTargets, PHARN_CONFIG_FILE, } from '../lib/install-manifest.js'; import { detectLayout } from '../lib/layout.js'; -import { scanDest } from '../lib/dest-drift.js'; +import { + manifestSources, + scanDest, + type DriftLabel, +} from '../lib/dest-drift.js'; import { BACKUP_DIR } from '../lib/backup.js'; +import type { FileRecords } from '../lib/install-records.js'; import { safeJoin, VERSION_RE } from '../lib/validate.js'; import type { Selection } from '../types.js'; @@ -90,45 +96,114 @@ function recordedSkillsVersion(cwd: string): string | null { */ export type WriteTargetsAction = 'proceed' | 'decline' | 'cancel'; +// How each backed-up file is marked in the list, and in what order the groups +// come — update's skip order, most actionable first (lib/update-decision.ts). +const MARKERS: Record = { + modified: '(edited)', + unrecorded: '(no pharn record)', + unverifiable: '(differs from upstream)', +}; +const LABEL_ORDER: readonly DriftLabel[] = [ + 'modified', + 'unrecorded', + 'unverifiable', +]; + +// One line per group: WHY these files are backed up. "Your edits" is claimed +// only where pharn can show it — a file that changed since pharn recorded +// writing it; without a usable records store the line says so instead. +function labelLine(label: DriftLabel, n: number): string { + const one = n === 1; + switch (label) { + case 'modified': + return `${n} of them changed since pharn wrote ${one ? 'it' : 'them'} (your edits).`; + case 'unrecorded': + return `${n} of them ${one ? 'differs' : 'differ'} from upstream, and pharn has no record of ${one ? 'it' : 'them'}.`; + case 'unverifiable': + return `${n} of them ${one ? 'differs' : 'differ'} from upstream — pharn has no record to tell your edits from upstream changes.`; + } +} + +/** + * What init already knows when it asks: the install manifest it computed once + * for this run (commands/init.ts), and the records baseline of the install it + * replaces (steps/install-archetype.ts → reinstallBaseline). Each is optional: + * no manifest → computed here; no records → every differing file is + * `unverifiable`. + */ +export interface WriteTargetsContext { + manifest?: ReadonlyMap; + records?: FileRecords | null; +} + // 'proceed' when there is nothing to overwrite or the user confirmed; // 'decline' on a No; 'cancel' on Ctrl+C. Never exits. export async function confirmWriteTargets( repoDir: string, cwd: string, selection: Selection, + context: WriteTargetsContext = {}, ): Promise { + const layout = detectLayout(repoDir); + const manifest = + context.manifest ?? + collectExpectedInstallPaths({ + repoDir, + capabilities: selection.selected, + layout, + }); const conflicts = conflictingWriteTargets({ repoDir, projectRoot: cwd, capabilities: selection.selected, - layout: detectLayout(repoDir), + layout, + expected: manifest, }); if (conflicts.length === 0) return 'proceed'; // zero friction — nothing to overwrite - // Which of those differ from upstream — the user's EDITS. They are listed - // first (a 400-path list capped at 10 used to hide them) and named as backed - // up: runInstallArchetype copies them to .pharn-backup/ before the first - // write, re-scanning then rather than trusting this snapshot. - const drifted = new Set( - scanDest({ - repoDir, - projectRoot: cwd, - rels: conflicts.filter((p) => p !== PHARN_CONFIG_FILE), - }).drifted, - ); - const ordered = [ - ...conflicts.filter((p) => drifted.has(p)), - ...conflicts.filter((p) => !drifted.has(p)), - ]; + // Which of those the install will back up — the files `update` would have + // SKIPPED (lib/dest-drift.ts, through update's own table): changed since + // pharn wrote them, not recorded, or not verifiable. A file still at the hash + // pharn recorded is pharn's own bytes, merely outdated: not marked, not + // backed up. Each is compared with its real source (the mapped LICENSE). + // They are listed first (a 400-path list capped at 10 used to hide them). + // + // ADVISORY: runInstallArchetype re-scans under the lock, immediately before + // the copy, and that scan is the one that backs files up — this snapshot + // only labels the list. `pharn.config.json` is outside the manifest (init + // regenerates it), so it is never scanned. + const { labels } = scanDest({ + repoDir, + projectRoot: cwd, + rels: conflicts.filter((p) => manifest.has(p)), + sources: manifestSources(repoDir, manifest), + records: context.records ?? null, + }); + const rank = (p: string): number => { + const label = labels.get(p); + return label === undefined + ? LABEL_ORDER.length + : LABEL_ORDER.indexOf(label); + }; + // A stable sort over the already-sorted conflicts: grouped by label, then by + // path within a group (P5 — deterministic output). + const ordered = [...conflicts].sort((a, b) => rank(a) - rank(b)); const shown = ordered.slice(0, MAX_LISTED); const more = ordered.length - shown.length; const list = shown - .map((p) => (drifted.has(p) ? ` • ${p} (edited)` : ` • ${p}`)) + .map((p) => { + const label = labels.get(p); + return label === undefined ? ` • ${p}` : ` • ${p} ${MARKERS[label]}`; + }) .join('\n'); const tail = more > 0 ? `\n …and ${more} more` : ''; + const counts = LABEL_ORDER.map( + (label) => + [label, [...labels.values()].filter((l) => l === label).length] as const, + ).filter(([, n]) => n > 0); const editsLine = - drifted.size > 0 - ? `\n${drifted.size} of them differ from upstream (your edits) and will be copied to ${BACKUP_DIR}/ before being overwritten.` + labels.size > 0 + ? `\n${counts.map(([label, n]) => labelLine(label, n)).join('\n')}\n${labels.size === 1 ? 'It' : `All ${labels.size}`} will be copied to ${BACKUP_DIR}/ before being overwritten.` : ''; // Only when the config itself is at risk — i.e. a re-install over an existing // one — and only AFTER the zero-conflict return above, so a conflict-free diff --git a/tests/dest-drift.test.ts b/tests/dest-drift.test.ts index 3bdfd553..aea806b0 100644 --- a/tests/dest-drift.test.ts +++ b/tests/dest-drift.test.ts @@ -2,7 +2,8 @@ import { mkdirSync, symlinkSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { useTmpDir } from './helpers.js'; -import { scanDest } from '../src/lib/dest-drift.js'; +import { manifestSources, scanDest } from '../src/lib/dest-drift.js'; +import { sha256File } from '../src/lib/hash.js'; // --------------------------------------------------------------------------- // The destination-drift set — the files `pharn add`'s copy is about to overwrite @@ -244,7 +245,198 @@ describe('scanDest', () => { const { repo, proj } = trees(); expect(scanDest({ repoDir: repo, projectRoot: proj, rels: [] })).toEqual({ drifted: [], + labels: new Map(), unsafe: [], }); }); + + it('labels every drifted file `unverifiable` when there are no records', () => { + // `add`'s case, and a first install's: nothing to tell pharn's bytes from + // the user's, so every difference is backed up — the set is unchanged. + const { repo, proj } = trees(); + write(join(repo, REL), 'upstream'); + write(join(proj, REL), 'MY EDIT'); + + const scan = scanDest({ repoDir: repo, projectRoot: proj, rels: [REL] }); + expect(scan.drifted).toEqual([REL]); + expect(scan.labels).toEqual(new Map([[REL, 'unverifiable']])); + }); +}); + +// A re-run `init` overwrites every file it installs, so "what must be backed up +// first" is exactly what `update` would have SKIPPED — its own decision table, +// read through the records baseline. A file still at the hash pharn recorded is +// pharn's bytes, merely outdated: a clean upgrade, not the user's edit. +describe("scanDest — the records baseline (update's table)", () => { + const tmp = useTmpDir(); + const REL = 'pharn-review/a11y/a11y.md'; + + function trees(): { repo: string; proj: string } { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + mkdirSync(repo, { recursive: true }); + mkdirSync(proj, { recursive: true }); + return { repo, proj }; + } + + function hashOf(content: string): string { + const p = join(tmp.path(), 'hash-me'); + writeFileSync(p, content); + return sha256File(p); + } + + it('does NOT report a file still at its recorded hash — an upstream bump is not an edit', () => { + const { repo, proj } = trees(); + write(join(repo, REL), 'upstream v2'); + write(join(proj, REL), 'upstream v1'); + + const scan = scanDest({ + repoDir: repo, + projectRoot: proj, + rels: [REL], + records: { [REL]: hashOf('upstream v1') }, + }); + expect(scan.drifted).toEqual([]); + expect(scan.labels.size).toBe(0); + }); + + it('reports a file that changed since pharn wrote it as `modified`', () => { + const { repo, proj } = trees(); + write(join(repo, REL), 'upstream v2'); + write(join(proj, REL), 'MY EDIT'); + + const scan = scanDest({ + repoDir: repo, + projectRoot: proj, + rels: [REL], + records: { [REL]: hashOf('upstream v1') }, + }); + expect(scan.drifted).toEqual([REL]); + expect(scan.labels.get(REL)).toBe('modified'); + }); + + it('reports a differing file the records do not cover as `unrecorded`', () => { + const { repo, proj } = trees(); + write(join(repo, REL), 'upstream'); + write(join(proj, REL), 'somebody put this here'); + + const scan = scanDest({ + repoDir: repo, + projectRoot: proj, + rels: [REL], + records: {}, + }); + expect(scan.drifted).toEqual([REL]); + expect(scan.labels.get(REL)).toBe('unrecorded'); + }); + + it('never reports a byte-identical file, whatever the record says', () => { + // Row 2 precedes every record row: identical is never a skip. + const { repo, proj } = trees(); + write(join(repo, REL), 'same'); + write(join(proj, REL), 'same'); + + const scan = scanDest({ + repoDir: repo, + projectRoot: proj, + rels: [REL], + records: { [REL]: hashOf('something else entirely') }, + }); + expect(scan.drifted).toEqual([]); + }); + + it("reads a record only as the file's OWN key (no prototype lookups)", () => { + const { repo, proj } = trees(); + write(join(repo, 'constructor'), 'upstream'); + write(join(proj, 'constructor'), 'MY EDIT'); + + const scan = scanDest({ + repoDir: repo, + projectRoot: proj, + rels: ['constructor'], + records: {}, + }); + expect(scan.labels.get('constructor')).toBe('unrecorded'); + }); +}); + +// The install manifest maps ONE dest to a different clone path: upstream's +// `LICENSE` lands at `PHARN-LICENSE` (flat) / `pharn/LICENSE` (pharn layout). +// Scanning the dest path on both sides found no `PHARN-LICENSE` in the clone, +// so an edited copy was never compared, never backed up, and overwritten. +describe('scanDest — a dest compared with its REAL source', () => { + const tmp = useTmpDir(); + + function trees(): { repo: string; proj: string } { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + mkdirSync(repo, { recursive: true }); + mkdirSync(proj, { recursive: true }); + return { repo, proj }; + } + + it.each(['PHARN-LICENSE', 'pharn/LICENSE'])( + "reports an edited %s against the clone's LICENSE", + (dest) => { + const { repo, proj } = trees(); + write(join(repo, 'LICENSE'), 'Apache-2.0'); + write(join(proj, dest), 'MY EDITED LICENSE'); + + const scan = scanDest({ + repoDir: repo, + projectRoot: proj, + rels: [dest], + sources: new Map([[dest, 'LICENSE']]), + }); + expect(scan.drifted).toEqual([dest]); + }, + ); + + it('does not report the mapped dest when it matches its source', () => { + const { repo, proj } = trees(); + write(join(repo, 'LICENSE'), 'Apache-2.0'); + write(join(proj, 'PHARN-LICENSE'), 'Apache-2.0'); + + const scan = scanDest({ + repoDir: repo, + projectRoot: proj, + rels: ['PHARN-LICENSE'], + sources: new Map([['PHARN-LICENSE', 'LICENSE']]), + }); + expect(scan.drifted).toEqual([]); + }); + + it('refuses a source that escapes the clone (safeJoin containment)', () => { + const { repo, proj } = trees(); + write(join(proj, 'PHARN-LICENSE'), 'x'); + expect(() => + scanDest({ + repoDir: repo, + projectRoot: proj, + rels: ['PHARN-LICENSE'], + sources: new Map([['PHARN-LICENSE', '../outside']]), + }), + ).toThrow(); + }); + + it("manifestSources turns the manifest's absolute sources into clone paths", () => { + const { repo } = trees(); + expect( + manifestSources( + repo, + new Map([ + ['PHARN-LICENSE', join(repo, 'LICENSE')], + [ + 'pharn-review/a11y/a11y.md', + join(repo, 'pharn-review/a11y/a11y.md'), + ], + ]), + ), + ).toEqual( + new Map([ + ['PHARN-LICENSE', 'LICENSE'], + ['pharn-review/a11y/a11y.md', 'pharn-review/a11y/a11y.md'], + ]), + ); + }); }); diff --git a/tests/init-archetype.test.ts b/tests/init-archetype.test.ts index 1d11c8ca..fba7a63a 100644 --- a/tests/init-archetype.test.ts +++ b/tests/init-archetype.test.ts @@ -7,17 +7,44 @@ import { writeFileSync, } from 'node:fs'; import { join } from 'node:path'; -import { describe, expect, it, vi } from 'vitest'; -import { useTmpDir } from './helpers.js'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { restoreTTY, setTTY, stubProcessExit, useTmpDir } from './helpers.js'; // runInstallArchetype uses clack for progress UI only — mock it so the fixture // e2e exercises the copy + config-write apply path without a real terminal. +// The prompt members serve the whole-`init` cases at the end of the file: the +// summary answers "install" and the overwrite confirm answers yes. vi.mock('@clack/prompts', () => ({ spinner: () => ({ start: vi.fn(), stop: vi.fn(), message: vi.fn() }), outro: vi.fn(), + intro: vi.fn(), + note: vi.fn(), + cancel: vi.fn(), + select: vi.fn(async () => 'install'), + confirm: vi.fn(async () => true), + isCancel: () => false, log: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, })); +// The whole-`init` cases run the REAL steps; only what leaves the machine or +// the test is stubbed: the network fetch (a fixture clone stands in) and the +// banner. +const fetchRepo = vi.fn(); +vi.mock('../src/lib/repo.js', () => ({ fetchRepo })); +vi.mock('../src/lib/banner.js', () => ({ showBanner: vi.fn() })); + +// A pass-through spy on the manifest builder, so a run can COUNT how often it +// is computed. A module mock sees only calls that cross a module boundary — +// every caller of it after this fix does. +vi.mock('../src/lib/install-manifest.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + collectExpectedInstallPaths: vi.fn(actual.collectExpectedInstallPaths), + }; +}); + const { detectArchetypesFromProject } = await import('../src/lib/detect-archetype.js'); const { parseCapabilityIndex } = await import('../src/lib/capability-index.js'); @@ -32,6 +59,7 @@ const { readRecords, writeRecords } = const { collectExpectedInstallPaths } = await import('../src/lib/install-manifest.js'); const { sha256File } = await import('../src/lib/hash.js'); +const { runInit } = await import('../src/commands/init.js'); const prompts = await import('@clack/prompts'); // The single string runInstallArchetype passed to outro() — the same @@ -846,3 +874,392 @@ describe('re-running init keeps the config keys pharn does not own', () => { expect(({} as Record).polluted).toBeUndefined(); }); }); + +// Every file under the ONE backup directory a run created, relative to it — +// what the user can get back. +function backedUp(proj: string): string[] { + const root = join(proj, '.pharn-backup'); + if (!existsSync(root)) return []; + const dirs = readdirSync(root); + expect(dirs).toHaveLength(1); + const dir = join(root, dirs[0]!); + return ( + readdirSync(dir, { recursive: true, withFileTypes: true }) as { + isFile(): boolean; + name: string; + parentPath: string; + }[] + ) + .filter((e) => e.isFile()) + .map((e) => + join(e.parentPath, e.name) + .slice(dir.length + 1) + .split('\\') + .join('/'), + ) + .sort(); +} + +// A pharn-layout clone (pharn-oss >= 5): capabilities, contracts and the +// constitution under pharn/, the LICENSE still at the root. +function scaffoldPharnRepo(repo: string): void { + write( + join(repo, 'pharn/pharn-pipeline/grillers/a11y/a11y.md'), + cap('griller', '["ssr"]'), + ); + write(join(repo, 'pharn/pharn-contracts/finding-shape.md'), 'fs'); + write(join(repo, 'pharn/CONSTITUTION.md'), 'C'); + write(join(repo, '.claude/commands/pharn-plan.md'), 'plan'); + write(join(repo, '.claude/settings.json'), '{"hooks":{}}'); + write(join(repo, 'LICENSE'), 'Apache-2.0'); + write(join(repo, 'SKILLS_VERSION'), '1.0.0\n'); +} + +// A re-run `init` overwrites every file it installs. What it backs up first is +// what `update` would have SKIPPED — its own table, through the records the +// previous install wrote — so an upstream bump alone is not "your edits". +describe('re-running init after an upstream bump — the records decide what is yours', () => { + const tmp = useTmpDir(); + // The stamp of the config a re-run replaces: the first install's. + const STAMP = { skillsVersion: '1.0.0', commit: 'sha123' }; + const carry = ( + previousStamp: { skillsVersion: string; commit: string | null } | null, + ) => ({ manualKeys: new Set(), kept: [], previousStamp }); + + async function installed() { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(repo); + write(join(repo, 'LICENSE'), 'Apache-2.0'); + write( + join(proj, 'package.json'), + JSON.stringify({ dependencies: { next: '14.0.0' } }), + ); + const { archetypes } = detectArchetypesFromProject(proj); + const selection = resolveCapabilities( + archetypes, + parseCapabilityIndex(repo), + ); + await runInstallArchetype(repo, proj, archetypes, selection, 'sha123'); + vi.mocked(prompts.log.info).mockClear(); + return { repo, proj, archetypes, selection }; + } + + // Upstream moves on: two installed files change, and the version with them. + function bumpUpstream(repo: string): void { + write(join(repo, '.claude/commands/pharn-plan.md'), 'plan v2'); + write(join(repo, 'CONSTITUTION.md'), 'C v2'); + write(join(repo, 'SKILLS_VERSION'), '1.1.0\n'); + } + + const informed = (): string => + vi + .mocked(prompts.log.info) + .mock.calls.map((c) => String(c[0])) + .join('\n'); + + it("backs up NOTHING after an upstream bump alone — pharn's own bytes are a clean upgrade", async () => { + const { repo, proj, archetypes, selection } = await installed(); + bumpUpstream(repo); + + await runInstallArchetype( + repo, + proj, + archetypes, + selection, + 'sha456', + carry(STAMP), + ); + + expect(existsSync(join(proj, '.pharn-backup'))).toBe(false); + expect(informed()).not.toContain('Backed up'); + expect( + readFileSync(join(proj, '.claude/commands/pharn-plan.md'), 'utf8'), + ).toBe('plan v2'); + }); + + it('backs up exactly the ONE file the user changed', async () => { + const { repo, proj, archetypes, selection } = await installed(); + bumpUpstream(repo); + write(join(proj, 'CONSTITUTION.md'), 'MY CONSTITUTION'); + + await runInstallArchetype( + repo, + proj, + archetypes, + selection, + 'sha456', + carry(STAMP), + ); + + expect(backedUp(proj)).toEqual(['CONSTITUTION.md']); + const [dir] = readdirSync(join(proj, '.pharn-backup')); + expect( + readFileSync( + join(proj, '.pharn-backup', dir!, 'CONSTITUTION.md'), + 'utf8', + ), + ).toBe('MY CONSTITUTION'); + expect(readFileSync(join(proj, 'CONSTITUTION.md'), 'utf8')).toBe('C v2'); + }); + + it.each<[string, { skillsVersion: string; commit: string | null } | null]>([ + ['there is no previous config', null], + [ + 'the store was stamped for another install state', + { + skillsVersion: '0.9.0', + commit: 'other', + }, + ], + ])( + 'backs up EVERY differing file when %s — conservative, never a guess', + async (_label, stamp) => { + const { repo, proj, archetypes, selection } = await installed(); + bumpUpstream(repo); + + await runInstallArchetype( + repo, + proj, + archetypes, + selection, + 'sha456', + carry(stamp), + ); + + expect(backedUp(proj)).toEqual([ + '.claude/commands/pharn-plan.md', + 'CONSTITUTION.md', + ]); + }, + ); + + it("backs up an edited PHARN-LICENSE, compared with upstream's LICENSE (flat layout)", async () => { + const { repo, proj, archetypes, selection } = await installed(); + write(join(proj, 'PHARN-LICENSE'), 'MY LICENSE NOTES'); + + await runInstallArchetype( + repo, + proj, + archetypes, + selection, + 'sha123', + carry(STAMP), + ); + + expect(backedUp(proj)).toEqual(['PHARN-LICENSE']); + expect(readFileSync(join(proj, 'PHARN-LICENSE'), 'utf8')).toBe( + 'Apache-2.0', + ); + }); + + it('backs up an edited pharn/LICENSE (pharn layout)', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldPharnRepo(repo); + mkdirSync(proj, { recursive: true }); + const selection = { + selected: [ + { name: 'a11y', role: 'griller' as const, matched: ['ssr' as const] }, + ], + skipped: [], + }; + await runInstallArchetype(repo, proj, ['ssr'], selection, 'sha123'); + expect(readFileSync(join(proj, 'pharn/LICENSE'), 'utf8')).toBe( + 'Apache-2.0', + ); + write(join(proj, 'pharn/LICENSE'), 'MY LICENSE NOTES'); + + await runInstallArchetype( + repo, + proj, + ['ssr'], + selection, + 'sha123', + carry(STAMP), + ); + + expect(backedUp(proj)).toEqual(['pharn/LICENSE']); + }); + + // The records are keyed by PATH. A layout change (flat → pharn/) writes to + // new paths, which a flat project does not have yet, and leaves the old files + // where they are (init never deletes) — so it backs up nothing extra. Only a + // file of the user's own already sitting at one of the new paths has no + // record there, and that one IS backed up. + it('a layout change (flat → pharn/) backs up only a file of yours at a new path', async () => { + const flat = join(tmp.path(), 'flat'); + const relocated = join(tmp.path(), 'relocated'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(flat); + write(join(flat, 'LICENSE'), 'Apache-2.0'); + scaffoldPharnRepo(relocated); + mkdirSync(proj, { recursive: true }); + const selection = { + selected: [ + { name: 'a11y', role: 'griller' as const, matched: ['ssr' as const] }, + ], + skipped: [], + }; + await runInstallArchetype(flat, proj, ['ssr'], selection, 'sha123'); + write(join(proj, 'pharn/LICENSE'), 'MY OWN NOTES'); + + await runInstallArchetype( + relocated, + proj, + ['ssr'], + selection, + 'sha456', + carry(STAMP), + ); + + expect(backedUp(proj)).toEqual(['pharn/LICENSE']); + expect(readFileSync(join(proj, 'pharn/CONSTITUTION.md'), 'utf8')).toBe('C'); + // The flat copies are left in place — init never deletes. + expect(existsSync(join(proj, 'CONSTITUTION.md'))).toBe(true); + expect(existsSync(join(proj, 'PHARN-LICENSE'))).toBe(true); + }); + + it('refuses a project it cannot install into BEFORE backing anything up', async () => { + // An edit (which would be backed up) AND a type collision (which the + // install refuses). The refusal used to come after the backup: the user + // read "Backed up…", nothing was installed, and every retry added another + // .pharn-backup/ directory. + const { proj, repo, archetypes, selection } = await installed(); + write(join(proj, '.claude/commands/pharn-plan.md'), 'MY EDIT'); + rmSync(join(proj, 'pharn-contracts'), { recursive: true, force: true }); + write(join(proj, 'pharn-contracts'), 'a FILE where a directory goes'); + + await expect( + runInstallArchetype( + repo, + proj, + archetypes, + selection, + 'sha123', + carry(STAMP), + ), + ).rejects.toThrow(/Refusing to install: pharn-contracts is in the way/); + + expect(existsSync(join(proj, '.pharn-backup'))).toBe(false); + expect(informed()).not.toContain('Backed up'); + expect( + readFileSync(join(proj, '.claude/commands/pharn-plan.md'), 'utf8'), + ).toBe('MY EDIT'); + }); + + it('refuses a directory at pharn.records.json before copying anything — no half-install', async () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(repo); + write(join(proj, 'package.json'), '{}'); + mkdirSync(join(proj, 'pharn.records.json')); + const { archetypes } = detectArchetypesFromProject(proj); + const selection = resolveCapabilities( + archetypes, + parseCapabilityIndex(repo), + ); + + await expect( + runInstallArchetype(repo, proj, archetypes, selection, 'sha123'), + ).rejects.toThrow( + /Refusing to install: pharn\.records\.json is in the way/, + ); + + expect(existsSync(join(proj, '.claude'))).toBe(false); + expect(existsSync(join(proj, 'pharn.config.json'))).toBe(false); + }); +}); + +// The same guarantees through `runInit` itself — detect, fetch (a fixture +// clone), both prompts, the lock and the install — so the prompt and the +// backup are seen to agree, and the manifest builder can be counted across the +// whole run rather than inside one step. +describe('`pharn init` run twice, through the real steps', () => { + const tmp = useTmpDir(); + stubProcessExit(); + beforeEach(() => setTTY(true, true)); + afterEach(() => restoreTTY()); + + function fixture(): { repo: string; proj: string } { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(repo); + write(join(repo, 'LICENSE'), 'Apache-2.0'); + write( + join(proj, 'package.json'), + JSON.stringify({ dependencies: { next: '14.0.0' } }), + ); + mkdirSync(join(proj, '.git')); + return { repo, proj }; + } + + async function init(repo: string, proj: string, sha: string): Promise { + fetchRepo.mockResolvedValue({ dir: repo, sha, cleanup: vi.fn() }); + const cwd = vi.spyOn(process, 'cwd').mockReturnValue(proj); + try { + await runInit(); + } finally { + cwd.mockRestore(); + } + } + + function bumpUpstream(repo: string): void { + write(join(repo, '.claude/commands/pharn-plan.md'), 'plan v2'); + write(join(repo, 'CONSTITUTION.md'), 'C v2'); + write(join(repo, 'SKILLS_VERSION'), '1.1.0\n'); + } + + // The overwrite prompt's warning (the only one that lists paths). + function overwriteWarning(): string { + return ( + vi + .mocked(prompts.log.warn) + .mock.calls.map((c) => String(c[0])) + .find((w) => w.includes('already exist and may be overwritten')) ?? '' + ); + } + + it('computes the install manifest ONCE per run, and marks and backs up nothing after an upstream bump alone', async () => { + const { repo, proj } = fixture(); + await init(repo, proj, 'sha123'); + bumpUpstream(repo); + vi.mocked(collectExpectedInstallPaths).mockClear(); + vi.mocked(prompts.log.warn).mockClear(); + + await init(repo, proj, 'sha456'); + + // Prompt, pre-flight (twice), backup scan, copy and records all share it. + expect(collectExpectedInstallPaths).toHaveBeenCalledTimes(1); + const warning = overwriteWarning(); + expect(warning).toContain('.claude/commands/pharn-plan.md'); + expect(warning).not.toContain('(edited)'); + expect(warning).not.toContain('(differs from upstream)'); + expect(warning).not.toContain('.pharn-backup/'); + expect(existsSync(join(proj, '.pharn-backup'))).toBe(false); + const config = readPharnConfig(proj)!; + expect(config.skillsVersion).toBe('1.1.0'); + expect(config.commit).toBe('sha456'); + }); + + it('marks exactly the file the user changed, and backs up exactly that file', async () => { + const { repo, proj } = fixture(); + await init(repo, proj, 'sha123'); + bumpUpstream(repo); + write(join(proj, 'CONSTITUTION.md'), 'MY CONSTITUTION'); + vi.mocked(prompts.log.warn).mockClear(); + + await init(repo, proj, 'sha456'); + + const warning = overwriteWarning(); + const bullets = warning + .split('\n') + .filter((l) => l.trimStart().startsWith('•')); + expect(bullets[0]).toContain('CONSTITUTION.md (edited)'); + expect(bullets.filter((b) => b.includes('('))).toHaveLength(1); + expect(warning).toContain( + '1 of them changed since pharn wrote it (your edits).', + ); + expect(backedUp(proj)).toEqual(['CONSTITUTION.md']); + }); +}); diff --git a/tests/init.test.ts b/tests/init.test.ts index 331ef8d0..0b1056dc 100644 --- a/tests/init.test.ts +++ b/tests/init.test.ts @@ -67,7 +67,15 @@ const runArchetypeSummary = vi.fn( vi.mock('../src/steps/archetype-summary.js', () => ({ runArchetypeSummary })); const runInstallArchetype = vi.fn(async () => undefined); -vi.mock('../src/steps/install-archetype.js', () => ({ runInstallArchetype })); +// The install manifest init computes ONCE per run and hands to both the prompt +// and the install; the records baseline the prompt labels files by. +const installManifest = vi.fn(() => new Map()); +const reinstallBaseline = vi.fn(() => null); +vi.mock('../src/steps/install-archetype.js', () => ({ + runInstallArchetype, + installManifest, + reinstallBaseline, +})); // The pre-install write-target conflict check (steps/overwrite-check.ts). Default: // no conflicts → true → install proceeds; overridden per-test to exercise decline. @@ -383,12 +391,29 @@ describe('runInit (archetype default)', () => { expect(parseCapabilityIndex).toHaveBeenCalledWith('/fake/repo'); expect(resolveCapabilities).toHaveBeenCalledTimes(1); expect(runArchetypeSummary).toHaveBeenCalledTimes(1); - // The write-target conflict check gates the install (repo dir, cwd, selection). + // The manifest is built ONCE, and that same map reaches the prompt and the + // install — neither rebuilds it. + expect(installManifest).toHaveBeenCalledTimes(1); + expect(installManifest).toHaveBeenCalledWith('/fake/repo', { + selected: [], + skipped: [], + }); + const manifest = installManifest.mock.results[0]!.value as Map< + string, + string + >; + // The write-target conflict check gates the install (repo dir, cwd, + // selection, and what init already knows: the manifest + the records). expect(confirmWriteTargets).toHaveBeenCalledWith( '/fake/repo', expect.any(String), { selected: [], skipped: [] }, + { manifest, records: null }, ); + const promptContext = ( + confirmWriteTargets.mock.calls[0] as unknown as unknown[] + )[3] as { manifest: unknown }; + expect(promptContext.manifest).toBe(manifest); // Install ran with the pinned SHA; the temp clone was cleaned up. expect(runInstallArchetype).toHaveBeenCalledTimes(1); expect(runInstallArchetype).toHaveBeenCalledWith( @@ -403,10 +428,24 @@ describe('runInit (archetype default)', () => { kept: [], previousStamp: null, }), + manifest, + ); + expect((runInstallArchetype.mock.calls[0] as unknown as unknown[])[6]).toBe( + manifest, ); expect(cleanup).toHaveBeenCalledTimes(1); }); + it('builds no manifest and reads no records when the summary is cancelled', async () => { + runArchetypeSummary.mockResolvedValue('cancel'); + + await expect(runInit()).rejects.toMatchObject(new ProcessExit(0)); + + expect(installManifest).not.toHaveBeenCalled(); + expect(reinstallBaseline).not.toHaveBeenCalled(); + expect(confirmWriteTargets).not.toHaveBeenCalled(); + }); + it('cancels from the summary without installing', async () => { runArchetypeSummary.mockResolvedValue('cancel'); diff --git a/tests/install-capabilities.test.ts b/tests/install-capabilities.test.ts index 48fa95a8..89c78643 100644 --- a/tests/install-capabilities.test.ts +++ b/tests/install-capabilities.test.ts @@ -14,6 +14,7 @@ import { useTmpDir } from './helpers.js'; import { installCapabilities, installCapabilityDirs, + prepareInstall, } from '../src/lib/install-capabilities.js'; import { collectExpectedInstallPaths } from '../src/lib/install-manifest.js'; import { ManifestValidationError } from '../src/lib/validate.js'; @@ -970,7 +971,11 @@ describe('installCapabilities — symlinked destination pre-flight (PHARN-02)', }, ); - it('refuses a symlinked .claude/settings.json leaf (dangling link would be written through)', () => { + // A DANGLING link does not exist to `existsSync`, so the install would write + // upstream's settings there. Measured on Node 20.13, 22 and 24: cpSync then + // REPLACES the link with a regular file (nothing lands outside the project), + // so the user's link would be lost without a word. Refused, link untouched. + it('refuses a DANGLING .claude/settings.json link, leaving the link as it was', () => { const repo = join(tmp.path(), 'repo'); const proj = join(tmp.path(), 'proj'); const outside = join(tmp.path(), 'outside'); @@ -983,9 +988,85 @@ describe('installCapabilities — symlinked destination pre-flight (PHARN-02)', ); expect(() => installCapabilities(repo, proj, selection())).toThrow( - /\.claude\/settings\.json is a symbolic link/, + /\.claude\/settings\.json is a symbolic link where pharn writes a file.*points at nothing/, ); expect(existsSync(join(outside, 'settings.json'))).toBe(false); + expect( + lstatSync(join(proj, '.claude/settings.json')).isSymbolicLink(), + ).toBe(true); + expect(existsSync(join(proj, '.claude/commands'))).toBe(false); + }); + + // A LIVE link: the install never writes an existing settings.json (it is + // preserved), so there is nothing to refuse — refusing sent users who keep + // their Claude settings in a dotfiles repo away with wrong advice. + it('installs through a LIVE .claude/settings.json link and leaves its target alone', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const dotfiles = join(tmp.path(), 'dotfiles'); + scaffoldRepo(repo); + write(join(dotfiles, 'settings.json'), '{"mine":true}'); + mkdirSync(join(proj, '.claude'), { recursive: true }); + symlinkSync( + join(dotfiles, 'settings.json'), + join(proj, '.claude/settings.json'), + ); + + const result = installCapabilities(repo, proj, selection()); + + expect(result.settingsPreserved).toBe(true); + expect(readFileSync(join(dotfiles, 'settings.json'), 'utf8')).toBe( + '{"mine":true}', + ); + expect( + lstatSync(join(proj, '.claude/settings.json')).isSymbolicLink(), + ).toBe(true); + expect(existsSync(join(proj, '.claude/commands/pharn-plan.md'))).toBe(true); + }); + + it('says "regular file", not "real directory", for a symlinked file it would write', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const outside = join(tmp.path(), 'outside'); + scaffoldRepo(repo); + write(join(outside, 'plan.md'), 'MY PLAN'); + mkdirSync(join(proj, '.claude/commands'), { recursive: true }); + symlinkSync( + join(outside, 'plan.md'), + join(proj, '.claude/commands/pharn-plan.md'), + ); + + let message = ''; + try { + installCapabilities(repo, proj, selection()); + } catch (err) { + message = (err as Error).message; + } + expect(message).toContain( + '.claude/commands/pharn-plan.md is a symbolic link where pharn writes a file', + ); + expect(message).toContain('Replace it with a regular file'); + expect(message).not.toContain('real directory'); + expect(readFileSync(join(outside, 'plan.md'), 'utf8')).toBe('MY PLAN'); + }); + + it('names directory links and file links apart in one refusal', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const outside = join(tmp.path(), 'outside'); + scaffoldRepo(repo); + mkdirSync(join(outside, 'lenses'), { recursive: true }); + write(join(outside, 'plan.md'), 'MY PLAN'); + mkdirSync(join(proj, '.claude/commands'), { recursive: true }); + symlinkSync(join(outside, 'lenses'), join(proj, 'pharn-review')); + symlinkSync( + join(outside, 'plan.md'), + join(proj, '.claude/commands/pharn-plan.md'), + ); + + expect(() => installCapabilities(repo, proj, selection())).toThrow( + /pharn-review is a symbolic link inside the project.*real directory.*\.claude\/commands\/pharn-plan\.md is a symbolic link where pharn writes a file/, + ); }); it('names every symlinked component, sorted', () => { @@ -1106,6 +1187,36 @@ describe('installCapabilities — destination type pre-flight (PHARN-16)', () => expect(() => installCapabilities(repo, proj, selection())).not.toThrow(); }); + // The two files init writes BESIDE the copy. A directory at either passed + // every check, so the whole tree was copied and then the atomic write's + // rename failed (EISDIR): no records, no config — the half-install this + // pre-flight exists to prevent. + it.each(['pharn.config.json', 'pharn.records.json'])( + 'refuses a directory at %s before the first write', + (rel) => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(repo); + write(join(proj, rel, 'keep.txt'), 'USER'); + const before = tree(proj); + + expect(() => installCapabilities(repo, proj, selection())).toThrow( + `Refusing to install: ${rel} is in the way`, + ); + expect(tree(proj)).toEqual(before); + }, + ); + + it('accepts a regular file at pharn.config.json and pharn.records.json', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(repo); + write(join(proj, 'pharn.config.json'), '{}'); + write(join(proj, 'pharn.records.json'), '{}'); + + expect(() => installCapabilities(repo, proj, selection())).not.toThrow(); + }); + it('skips (does not refuse) a directory at the optional features/README.md', () => { const repo = join(tmp.path(), 'repo'); const proj = join(tmp.path(), 'proj'); @@ -1119,3 +1230,52 @@ describe('installCapabilities — destination type pre-flight (PHARN-16)', () => expect(existsSync(join(proj, 'CONSTITUTION.md'))).toBe(true); }); }); + +// init runs the pre-flight BEFORE its backup (steps/install-archetype.ts), over +// the manifest it computed once for the run. +describe('prepareInstall', () => { + const tmp = useTmpDir(); + + it('writes nothing and returns what it checked', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(repo); + mkdirSync(proj, { recursive: true }); + + const prepared = prepareInstall(repo, proj, selection().selected); + + expect(readdirSync(proj)).toEqual([]); + expect(prepared.paths.layout).toBe('flat'); + expect(prepared.featuresRel).toBe('features/README.md'); + expect([...prepared.manifest.keys()].sort()).toEqual( + [ + ...collectExpectedInstallPaths({ + repoDir: repo, + capabilities: selection().selected, + layout: 'flat', + }).keys(), + ].sort(), + ); + }); + + it('checks the manifest it is GIVEN — the same map, not a rebuilt one', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const outside = join(tmp.path(), 'outside'); + scaffoldRepo(repo); + mkdirSync(outside, { recursive: true }); + mkdirSync(proj, { recursive: true }); + // Only the given map names `extra/`, so only a pre-flight over THAT map + // can see its symlink. + symlinkSync(outside, join(proj, 'extra')); + const given = new Map([['extra/file.md', join(repo, 'CONSTITUTION.md')]]); + + expect(() => + prepareInstall(repo, proj, selection().selected, given), + ).toThrow(/Refusing to install: extra is a symbolic link/); + const ok = new Map([['CONSTITUTION.md', join(repo, 'CONSTITUTION.md')]]); + expect(prepareInstall(repo, proj, selection().selected, ok).manifest).toBe( + ok, + ); + }); +}); diff --git a/tests/overwrite-check.test.ts b/tests/overwrite-check.test.ts index c098cd43..7e1d98f8 100644 --- a/tests/overwrite-check.test.ts +++ b/tests/overwrite-check.test.ts @@ -19,6 +19,7 @@ const { installCapabilities } = type Selection = import('../src/types.js').Selection; const { readPharnConfig } = await import('../src/lib/pharn-config.js'); const { ModelRoutingError } = await import('../src/lib/model-routing.js'); +const { sha256File } = await import('../src/lib/hash.js'); // ESC built from its code point, so no literal control character lives in this // file and no editor can silently eat it. @@ -270,6 +271,11 @@ describe('confirmWriteTargets', () => { // PHARN-11: with hundreds of existing paths capped at MAX_LISTED, the user's // EDITS were buried in "…and N more". They are listed first, marked, and the // prompt says they will be backed up before being overwritten. +// +// What counts as backed up is `update`'s definition (lib/dest-drift.ts): only +// "your edits" when the records show the file changed since pharn wrote it; a +// file still at its recorded hash is a clean upgrade and is not marked at all; +// with no usable records the prompt says it cannot tell. describe('confirmWriteTargets — edited files first (PHARN-11)', () => { const tmp = useTmpDir(); const sel: Selection = { @@ -279,40 +285,143 @@ describe('confirmWriteTargets — edited files first (PHARN-11)', () => { ], skipped: [], }; + const NPO = 'pharn-review/n-plus-one/n-plus-one.md'; - it('lists the edited file first, marked, and announces the backup', async () => { + function firstListed(warning: string): string | undefined { + return warning.split('\n').find((l) => l.trimStart().startsWith('•')); + } + + // Everything installed from `repo` into `proj`, and the records a real + // install would have written for it (every file at its installed hash). + function installed(): { + repo: string; + proj: string; + records: Record; + } { const repo = join(tmp.path(), 'repo'); const proj = join(tmp.path(), 'proj'); scaffoldRepo(repo); - // Everything already installed, byte-identical… installCapabilities(repo, proj, sel); - // …except one file the user edited. `z…` would sort LAST alphabetically. - write(join(proj, 'pharn-review/n-plus-one/n-plus-one.md'), 'MY EDIT'); + const records: Record = {}; + for (const rel of [ + NPO, + 'pharn-pipeline/grillers/a11y/a11y.md', + 'CONSTITUTION.md', + ]) { + records[rel] = sha256File(join(proj, rel)); + } vi.mocked(prompts.confirm).mockResolvedValueOnce(false); + return { repo, proj, records }; + } + + it('with NO records: lists the differing file first and says it cannot tell whose change it is', async () => { + const { repo, proj } = installed(); + // `z…` would sort LAST alphabetically. + write(join(proj, NPO), 'MY EDIT'); await confirmWriteTargets(repo, proj, sel); const warning = lastWarning(); - const firstListed = warning + expect(firstListed(warning)).toContain(`${NPO} (differs from upstream)`); + expect(warning).toContain( + '1 of them differs from upstream — pharn has no record to tell your edits from upstream changes.', + ); + expect(warning).toContain('It will be copied to .pharn-backup/'); + expect(warning).not.toContain('(edited)'); + }); + + it('with records: a file changed since pharn wrote it is "(edited)"', async () => { + const { repo, proj, records } = installed(); + write(join(proj, NPO), 'MY EDIT'); + + await confirmWriteTargets(repo, proj, sel, { records }); + + const warning = lastWarning(); + expect(firstListed(warning)).toContain(`${NPO} (edited)`); + expect(warning).toContain( + '1 of them changed since pharn wrote it (your edits).', + ); + }); + + it('with records: a differing file with no record is "(no pharn record)"', async () => { + const { repo, proj, records } = installed(); + write(join(proj, 'CONSTITUTION.md'), 'SOMEONE ELSES CONSTITUTION'); + const { 'CONSTITUTION.md': _dropped, ...withoutIt } = records; + + await confirmWriteTargets(repo, proj, sel, { records: withoutIt }); + + const warning = lastWarning(); + expect(firstListed(warning)).toContain('CONSTITUTION.md (no pharn record)'); + expect(warning).toContain( + '1 of them differs from upstream, and pharn has no record of it.', + ); + }); + + it('with records: an upstream change to a file still at its record is a clean upgrade — not marked, no backup line', async () => { + const { repo, proj, records } = installed(); + // Upstream moved on; the project still holds exactly what pharn wrote. + write(join(repo, NPO), 'upstream v2'); + + await confirmWriteTargets(repo, proj, sel, { records }); + + // NPO differs from upstream, so any label would put it FIRST, marked. + const warning = lastWarning(); + expect(firstListed(warning)).not.toContain(NPO); + expect(warning).not.toContain('(edited)'); + expect(warning).not.toContain('(differs from upstream)'); + expect(warning).not.toContain('.pharn-backup/'); + }); + + it("groups the labels in update's order and counts every backed-up file", async () => { + const { repo, proj, records } = installed(); + write(join(proj, NPO), 'MY EDIT'); + write(join(proj, 'CONSTITUTION.md'), 'SOMEONE ELSES CONSTITUTION'); + const { 'CONSTITUTION.md': _dropped, ...withoutIt } = records; + + await confirmWriteTargets(repo, proj, sel, { records: withoutIt }); + + const warning = lastWarning(); + const bullets = warning .split('\n') - .find((l) => l.trimStart().startsWith('•')); - expect(firstListed).toContain( - 'pharn-review/n-plus-one/n-plus-one.md (edited)', + .filter((l) => l.trimStart().startsWith('•')); + expect(bullets[0]).toContain(`${NPO} (edited)`); + expect(bullets[1]).toContain('CONSTITUTION.md (no pharn record)'); + expect(warning).toContain('All 2 will be copied to .pharn-backup/'); + }); + + it("compares the mapped PHARN-LICENSE with the clone's LICENSE", async () => { + const { repo, proj } = installed(); + write(join(repo, 'LICENSE'), 'APACHE-2.0'); + write(join(proj, 'PHARN-LICENSE'), 'MY EDITED LICENSE'); + + await confirmWriteTargets(repo, proj, sel); + + expect(firstListed(lastWarning())).toContain( + 'PHARN-LICENSE (differs from upstream)', ); - expect(warning).toContain('1 of them differ from upstream (your edits)'); - expect(warning).toContain('.pharn-backup/'); + }); + + it('uses the manifest it is GIVEN instead of building its own', async () => { + const { repo, proj } = installed(); + // Only the given map names `extra.md`, so only a prompt over THAT map + // lists it. + write(join(proj, 'extra.md'), 'MINE'); + const manifest = new Map([['extra.md', join(repo, 'CONSTITUTION.md')]]); + + await confirmWriteTargets(repo, proj, sel, { manifest }); + + const warning = lastWarning(); + expect(firstListed(warning)).toContain('extra.md (differs from upstream)'); + expect(warning).not.toContain(NPO); }); it('calls nothing an edit when every existing file is byte-identical', async () => { - const repo = join(tmp.path(), 'repo'); - const proj = join(tmp.path(), 'proj'); - scaffoldRepo(repo); - installCapabilities(repo, proj, sel); - vi.mocked(prompts.confirm).mockResolvedValueOnce(false); + const { repo, proj } = installed(); await confirmWriteTargets(repo, proj, sel); expect(lastWarning()).not.toContain('(edited)'); - expect(lastWarning()).not.toContain('your edits'); + expect(lastWarning()).not.toContain('(differs from upstream)'); + expect(lastWarning()).not.toContain('.pharn-backup/'); }); });