diff --git a/.dev/features/dest-symlink-guard-init-remove/GRILL.md b/.dev/features/dest-symlink-guard-init-remove/GRILL.md new file mode 100644 index 0000000..423efc1 --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/GRILL.md @@ -0,0 +1,34 @@ +# GRILL — dest-symlink-guard-init-remove + +Plan: `.dev/features/dest-symlink-guard-init-remove/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live +`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +```yaml +- type: FINDING + rule_id: 'P0' + severity: important + file: '.dev/features/dest-symlink-guard-init-remove/PLAN.md:62' + problem: "The pre-flight walks the FILE list, so a symlinked leaf that is itself a to-be-created directory is covered only through the files beneath it; an empty upstream dir would not be walked. Acceptable because cpSync only creates dirs for files it copies, but the claim should say 'every written file path'." + evidence: 'for every rel in `collectExpectedInstallPaths(...)` ∪ `{.claude/settings.json}`' +- type: FINDING + rule_id: 'P3' + severity: minor + file: '.dev/features/dest-symlink-guard-init-remove/PLAN.md:30' + problem: 'install-capabilities.ts will import install-manifest.ts (the mirror it is mirrored by); no cycle exists today (install-manifest imports constants/layout/symlink-guard/validate only), but the dependency direction should be noted.' + evidence: '`installCapabilities` pre-flight: for every rel in `collectExpectedInstallPaths(...)`' +- type: FINDING + rule_id: 'P4' + severity: minor + file: '.dev/features/dest-symlink-guard-init-remove/PLAN.md:44' + problem: "SECURITY.md's path-traversal scope line is not in `## Files`; README + CLAUDE.md cover the user-facing claim, so this is optional." + evidence: '`README.md` — "Safety model"' +``` + +## Summary + +Closes the two remaining write/delete sites with the existing shared walk; the TOCTOU residual is +named. Concerns are wording/coverage, not design. + +ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 3 advisory) — for the human to weigh before /pharn-dev-build. diff --git a/.dev/features/dest-symlink-guard-init-remove/PLAN.md b/.dev/features/dest-symlink-guard-init-remove/PLAN.md new file mode 100644 index 0000000..876b66a --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/PLAN.md @@ -0,0 +1,80 @@ +# PLAN — dest-symlink-guard-init-remove (PHARN-02: `init` / `remove` must not write or delete through a symlinked project directory) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: before any write, `init` walks every destination path it will write (the install manifest + + `.claude/settings.json`) with `findSymlinkComponent` and refuses the whole install, naming the + symlinked component; `remove` refuses (before any delete, for the whole selection) when a + capability dir's path crosses a symlinked component — the same posture `update` and `add` already take. +- layer(s): the CLI itself (`src/lib/install-capabilities.ts`, `src/commands/remove.ts`) +- constitution_refs: [P0, P1, P2, P3, P5, P7] + +## Discovery — verified this run (P6) + +Base = `main` @ `2b74a74` (PHARN-01 merged, #195). Reproduced in the review (`repro/init-symlink`, +`repro/init-symlink2`, `repro/remove-symlink`): + +- `.claude/commands -> ../../dotfiles/…` → `init` wrote 11 `pharn-*.md` outside the project, no prompt. +- `pharn -> ../team-shared/pharn` → `init` overwrote an external file and created 421 files outside. +- `pharn -> ../shared` → `pharn remove a11y` deleted `../shared/pharn-review/a11y` incl. a user file. + +Live code: `installCapabilityDirs` (`cpSync` at `safeJoin(projectRoot, …)`), `copyFilteredDir`, the +docs/license/contracts/core/floor copies and `settings.json` all check only the SOURCE; only +`features/README.md` walks the destination (`destAcceptsWrite`). `deleteCapabilityDir` walks nothing. +`findSymlinkComponent` (`lib/symlink-guard.ts`) is the shared physical walk; `collectExpectedInstallPaths` +(`lib/install-manifest.ts`) enumerates every file `installCapabilities` writes except the user-owned +`.claude/settings.json`. + +## Files + +- `src/lib/install-capabilities.ts` — `installCapabilities` pre-flight: for every rel in + `collectExpectedInstallPaths(...)` ∪ `{.claude/settings.json}`, `findSymlinkComponent(projectRoot, rel)`; + any hit → throw `ManifestValidationError` naming the component(s) (sorted, capped) before the first + write. ENOTDIR from the walk is not a symlink hit (left to the existing copy behaviour) — layer CLI/lib +- `src/commands/remove.ts` — before deleting (named path, and the picker for the WHOLE selection + before the first delete), `findSymlinkComponent(cwd, capabilityRelDir(...))`; a hit → `logError` + naming the component + exit 1, nothing deleted, config/records untouched — layer CLI/command +- `tests/install-capabilities.test.ts` — symlinked `.claude`, `.claude/commands`, `.claude/hooks`, + `pharn`, `pharn/pharn-review`, capability leaf dir, `.claude/settings.json` → throw, external dir + snapshot unchanged, project unchanged (no partial write) +- `tests/remove.test.ts` — symlinked `pharn` / `pharn/pharn-review` / capability dir → exit 1, external + dir unchanged, config write skipped; picker with one unsafe pick → nothing deleted +- `README.md` — "Safety model": `init` and `remove` also refuse symlinked destination paths (P4) +- `CLAUDE.md` — `install-capabilities` / `remove` paragraphs: destination walk (P4) + +## Contracts satisfied + +- README "Safety model" / `SECURITY.md` path-traversal scope / `LIMITS.md` containment — now true for + `init` and `remove`, not only `update`/`add`. +- CLAUDE.md "lexical/physical split": `safeJoin` contains the string, `findSymlinkComponent` refuses the + path on disk — applied at the two remaining write/delete sites. + +## Evals to write (P1) + +- init pre-flight: each symlinked root above → `ManifestValidationError` naming that component; the + link target dir byte-identical; nothing written under the project +- init with a regular (non-symlink) existing tree → unchanged behaviour (existing tests stay green) +- remove named: `pharn` symlink → exit 1, `../shared/pharn-review/a11y/NOTES.md` still present, + `writePharnConfig` not called, records untouched +- remove picker: two picks, one crossing a symlink → exit 1 before ANY delete (the safe one survives too) +- remove of a normal capability → unchanged + +## Guarantee audit (P0) + +- "`init` writes nothing through a symlinked destination component" → floor: `findSymlinkComponent` + over every manifest rel before the first write (lstat walk). Residual (advisory, named): a symlink + created concurrently between pre-flight and copy (TOCTOU) is not covered — same residual as `update`. +- "`remove` deletes nothing through a symlinked component" → floor: same walk before `rmSync`. + +## Trust audit (P2) + +- The project tree is local, user-controlled state; the walk reads only `lstat` metadata and emits a path + string (DATA) into the error message. No untrusted remote content drives a new branch. + +## Determinism audit (P5) + +- Branch = `findSymlinkComponent(...) !== null` (membership over lstat results); the terminal is a named + hard-fail, never a silent skip. + +## Open questions (HALT) + +- none diff --git a/.dev/features/dest-symlink-guard-init-remove/REGRESSION.md b/.dev/features/dest-symlink-guard-init-remove/REGRESSION.md new file mode 100644 index 0000000..c2aa6b5 --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/REGRESSION.md @@ -0,0 +1,30 @@ +# REGRESSION — dest-symlink-guard-init-remove + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `2b74a74df5f024a638ebc464181ea0bf971b13bd` (`origin/main` at build time; the build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `CLAUDE.md`, `README.md`, `src/commands/remove.ts`, `src/lib/install-capabilities.ts`, `tests/install-capabilities.test.ts`, `tests/remove.test.ts`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook scratch) and this + feature's own stage artifacts are not build output. +- **outside gates:** the stdlib `*.test.mjs` / `*.test.cjs` files + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / `lint:md` are absent from both maps. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` is +owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never the increment. diff --git a/.dev/features/dest-symlink-guard-init-remove/REVIEW.md b/.dev/features/dest-symlink-guard-init-remove/REVIEW.md new file mode 100644 index 0000000..66527af --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/REVIEW.md @@ -0,0 +1,47 @@ +# REVIEW — dest-symlink-guard-init-remove + +Floor first: `node .dev/floor/validate.mjs .` → exit 0 (GREEN). Everything below is **advisory**. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0):** both claims reduce to the existing physical walk `findSymlinkComponent` (lstat per + component) run before the first write/delete; the TOCTOU window is named as an advisory residual in + the code comment, matching `update`. +- **L-eval (P1):** 15 new cases fail on the base source and pass after: 7 symlinked destination roots + + the settings leaf + multi-component naming + pharn-layout root for `init`; 3 symlink positions + the + whole-selection picker refusal for `remove`; a non-symlink control case. The pre-existing + `features/` symlink test was updated from "silently skipped" to "whole install refused". +- **L-trust (P2):** only lstat metadata and a path string (DATA) reach the message; capability names in + that string are validated at ingest (PHARN-01). +- **L-axis (P3):** `install-capabilities.ts` now imports `install-manifest.ts` (no cycle — the manifest + imports only constants/layout/symlink-guard/validate); `remove.ts` imports the shared + `symlink-guard.ts`. No leaf→leaf import. + +## Advisory findings + +```yaml +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/install-capabilities.ts:156' + problem: "The refusal fires after the overwrite-confirm prompt (confirmWriteTargets), so a user may answer 'yes' and then be refused; nothing is written either way." + evidence: 'assertDestinationsInProject(repoDir, projectRoot, selection.selected, paths);' +- type: FINDING + rule_id: 'P7' + severity: minor + file: 'src/lib/install-capabilities.ts' + problem: 'Behavior change: a project whose features/ dir is a symlink used to install everything except features/README.md; it is now refused as a whole. Intentional (consistent posture), but user-visible.' + evidence: 'does NOT write through a symlinked features/ in the PROJECT (no escape)' +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:8' + problem: "No CHANGELOG `[Unreleased]` entry for the new refusals (not in the plan's `## Files`)." + evidence: '## [Unreleased]' +``` + +## Verdict + +**GREEN** — 0 floor-gate findings, 3 advisory findings (minor). No lesson proposed for canon. diff --git a/.dev/features/dest-symlink-guard-init-remove/SHIP.md b/.dev/features/dest-symlink-guard-init-remove/SHIP.md new file mode 100644 index 0000000..cab4ba3 --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/SHIP.md @@ -0,0 +1,17 @@ +# SHIP — dest-symlink-guard-init-remove + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: **Approve as written**) → `/pharn-dev-grill` → +`/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review` → GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- Run ended at **GATE 2**. The human's standing instruction for this batch: open a PR and merge it only if + its CI checks are green. + +chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good or +wise; that is the human's call at the post-review gate. diff --git a/.dev/features/dest-symlink-guard-init-remove/VERIFY.md b/.dev/features/dest-symlink-guard-init-remove/VERIFY.md new file mode 100644 index 0000000..421e317 --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/VERIFY.md @@ -0,0 +1,27 @@ +# VERIFY — dest-symlink-guard-init-remove + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, as a non-root user on node 22 with the session +proxy variables unset (as root with the proxy set, 5 pre-existing tests in `init.test.ts` / +`update.test.ts` fail for environmental reasons, identically at the baseline). + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +No `structural:*` gate — the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — floor gates only. + +Residual (P0/P7): "verified" means the named gates passed — not that the feature is correct in any sense +the suite does not encode. diff --git a/.dev/features/dest-symlink-guard-init-remove/regression-report.json b/.dev/features/dest-symlink-guard-init-remove/regression-report.json new file mode 100644 index 0000000..e251c4a --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/regression-report.json @@ -0,0 +1,24 @@ +{ + "base": "2b74a74df5f024a638ebc464181ea0bf971b13bd", + "inside": [ + "CLAUDE.md", + "README.md", + "src/commands/remove.ts", + "src/lib/install-capabilities.ts", + "tests/install-capabilities.test.ts", + "tests/remove.test.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/dest-symlink-guard-init-remove/verify-report.json b/.dev/features/dest-symlink-guard-init-remove/verify-report.json new file mode 100644 index 0000000..8b9d261 --- /dev/null +++ b/.dev/features/dest-symlink-guard-init-remove/verify-report.json @@ -0,0 +1,17 @@ +{ + "feature": "dest-symlink-guard-init-remove", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index b8609ad..4d7e496 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,12 @@ { "scope": [ - ".dev/features/config-capability-name-validation/SHIP.md" + "src/lib/install-capabilities.ts", + "src/commands/remove.ts", + "tests/install-capabilities.test.ts", + "tests/remove.test.ts", + "README.md", + "CLAUDE.md" ], - "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T07:36:53.788Z" + "set_by": ".dev/features/dest-symlink-guard-init-remove/PLAN.md", + "set_at": "2026-09-24T07:58:14.028Z" } diff --git a/CLAUDE.md b/CLAUDE.md index 3506823..aa3a5a6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -53,7 +53,7 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten 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`). 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 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: 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/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. @@ -61,7 +61,7 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten **`pharn add` addressing** (`commands/add.ts` + `lib/capability-address.ts`). `add ` or `add :` (e.g. `add a11y`, `add lens:n-plus-one`) installs one capability into an archetype project — a manual override of archetype auto-selection. It clones pharn-oss (SHA-pinned), then applies **the version gate**: a local `versionGate` helper, called ONCE per command from INSIDE each path's existing `try` (so `readSkillsVersion`'s throw still reaches the `finally` that cleans up the clone), refuses when `readSkillsVersion(repo.dir) !== config.skillsVersion` — reusing the existing `{kind:'error'}` outcome → `exit(1)`. It fires on `!==` (never `<`, so a rollback reads the same), fires **gate-first** (before the already-installed no-op and before the picker's `all-installed`, and before `groupMultiselect` renders), and writes nothing. A sibling **layout gate** (`layoutGate`) sits immediately after it at both call sites, `??`-chained (`versionGate(…) ?? layoutGate(…)`) so the **version** refusal wins when both mismatch — by short-circuit evaluation, not statement order: it refuses when `detectLayout(repo.dir) !== configLayout(config)`, naming both resolved layouts and `pharn update --force`. `add` copies at the CLONE's layout (`installCapabilityDirs`' default) and records at it (`mergeCapabilityRecords`), while `remove`/`status`/`diff.ts` address the project at `configLayout` — so without the gate a mismatched add lands files nothing ever looks at, and the next `remove` drops the config entry reporting "its files were already gone", orphaning the dir. `add` must NEVER record the clone's layout the way `update` does: `update` may only because it rewrites the WHOLE tree at that layout, whereas `add` writes one capability, so stamping `layout` here would re-address every other already-placed file. Comparing `configLayout(config)` (not the raw `config.layout`) is the point — agreement with the readers is the invariant, and it makes a garbage hand-edited value resolve to `flat` and fail closed. This is what keeps `update`'s `config.skillsVersion === latest` early-return honest: `add` must never stamp a newer `skillsVersion` over unchanged old bytes. `add` therefore refreshes `commit` but NEVER `skillsVersion` (the legal same-version-different-commit case). Past the gate it resolves the arg against `parseCapabilityIndex`, and if it uniquely names a not-yet-installed capability, copies it via `installCapabilityDirs` and **appends** to `capabilities` with `source: 'manual'` (never touches `archetypes`). That tag is what makes the override survive `update`, and it is written at BOTH entry-construction sites — `resolveArchetypeAdd` and the picker's threaded `cfg` mirror, which the next pick spreads into its own config write. Already-installed → no-op; unknown/ambiguous → lists the valid `role:name` addresses. **Before the copy, `add` backs up destination drift**: `capabilityCloneFiles` (`lib/install-manifest.ts`) enumerates the capability dir in the CLONE, `collectDestDrift` (`lib/dest-drift.ts`) keeps the rels whose dest exists as a regular file and whose `sha256File` DIFFERS from the clone's, and a non-empty set goes to `createBackup` (`lib/backup.ts` — the same `.pharn-backup//` `update --force` writes) BEFORE the first `cpSync`, logged at creation so the pointer survives a later throw. `add` was the only write path with none of the product's three edit-protections (init's prompt, update's per-file skip, `--force`'s backup), and `update` itself manufactures the reachable sequence: a `dropped-unselected` capability's files are left on disk, the user edits them, and the later `add` is not a config no-op. Byte-identical is NOT drift (mirrors update's `identical → no-op`), so a drop-then-re-add of unedited files stays silent. The scan SKIPS an absent/symlinked/non-dir source rather than throwing, so `installCapabilityDirs`' curated `Capability "x" (griller) is missing at …` still wins — the same read-skips/write-throws split `install-manifest.ts`'s `addDir` already uses. `scanDest` returns a PARTITION, not just the drift set: a rel whose PROJECT-side path crosses a symlinked component is `unsafe`, and `add` **refuses the whole install** naming the component. That is measured, not defensive — on node v24.13.1 `cpSync` guards only the source, so a symlinked INTERMEDIATE dir under the capability dir is written straight THROUGH (bytes outside the project replaced), while a symlinked leaf is REPLACED and a symlinked capability root throws `ERR_FS_CP_DIR_TO_NON_DIR`. Skipping would be strictly worse than doing nothing (the copy writes through anyway while the backup omits the file), and backing up is impossible (`createBackup` refuses symlinked components; `copyFileSync` would save the link's TARGET). An ENOTDIR walk (a component below a regular file) is NOT unsafe — `cpSync` throws on that tree by itself — so it stays a skip, which is also what keeps a non-directory component out of the set `createBackup` consumes unwrapped. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one), keyed by that SAME clone-derived list — never a walk of the destination, which used to sweep a user's own file inside a leftover capability dir into the store as pharn-written and let a later `update` read it as cleanly upgradeable instead of `modified`; the hashes are still taken at the DEST (`buildRecords`), so a record can never disagree with what landed. `CONSTITUTION.md` is **not** touched — `add` installs capability dirs only. `pharn update` re-resolves the **recorded archetypes** against the latest index and re-copies — drift-safely, see below. -**`pharn remove` addressing** (`commands/remove.ts`) is the inverse of `add`. `remove ` / `remove :` (no arg → an interactive picker over the installed capabilities) deletes that one isolated capability dir — addressed at the project's recorded `layout` (flat `pharn-review` / `pharn-pipeline/grillers/`, OR the same under `pharn/`, via `configLayout` + `layoutPaths`) — and drops its `capabilities` entry. **No clone, no network** — everything is derivable from `config.capabilities` + the filesystem, so `remove.ts` imports no repo module at all; `archetypes` is never touched; `CONSTITUTION.md`/`memory-bank/` are **never** touched (they are not capability dirs). Removing an entry whose stored `source` is **literally** `'auto'` warns that the next `update` will reinstall it; an **absent** `source` warns NOTHING (absence means provenance-unknown, and a false warning on a legacy manual add is worse than silence) — derived from the stored field only, so `remove` stays zero-network. Not-installed → benign no-op listing the removable capabilities; a name installed in both roles → hard-fail (ambiguous). `remove` has no `--yes`, and the reason is per-path: the NAMED remove deletes without asking (no prompt to skip), while the bare picker's ONE confirm — listing the picks, default No — IS the destructive gate, so nothing may skip that either. **`remove`'s `ALLOWED_FLAGS` row is empty, so `pharn remove --yes` now exits 1** naming the flag. This SUPERSEDES the previous contract, which read: _"`--yes` stays a declared minimist boolean only because it is `update`'s flag; that is what keeps `pharn remove --yes` a harmless parse rather than an unknown-option refusal, and turning it into a refusal belongs to a per-command allowlist, not to the dispatch."_ That allowlist now exists (see the dispatch paragraph above), so the deferral is spent — but its reasoning still holds and is why the row is empty rather than `['yes']`: there is no confirm on the named path to skip, and the picker's one confirm may not be skipped. `--yes` is still declared globally, because the declaration list is DERIVED from the table and `update`'s row grants it. Every delete is a **strict child** of its role subtree — `safeChildJoin(safeJoin(cwd, subtree), name)` (`lib/validate.ts`), which, unlike `safeJoin`, never returns the base itself nor a deeper path — a second floor under the ingest-time name validation. `remove` also **prunes that capability's entries from `pharn.records.json`** (`pruneCapabilityRecords`), so it is no longer the one write path leaving records that describe bytes that are gone — the converse of the invariant `update` is pinned to. The prune is a **key-prefix filter over the store**, never an fs enumeration: a walk of the DEST dir finds nothing after the delete AND nothing before it on the "already gone" path — which is exactly where the stale keys are the only thing left to clean, so the store is the only place the answer still exists. It mirrors `add`'s `mergeCapabilityRecords` guard verbatim (`recordsBaseline(...) === null → return`): absent/corrupt/stale-stamped → the file is left byte-identical, **never** minted and never blessed (the baseline `note` is deliberately not surfaced — `update` is where an unusable store changes an outcome, and it is the command that names the reason). The trailing slash in `relDir + '/'` is load-bearing (`lenses/a11y` must not eat `lenses/a11y-extended`); nothing matched → the write is skipped entirely; and the `skillsVersion`/`commit` stamp is re-written from the CONFIG pair unchanged, since `remove` advances neither. Order is delete → prune → config (mirroring `add`'s records-before-config) — a benignity argument, not atomicity: each write is individually atomic (`lib/atomic-write.ts` — temp-then-`rename`), but the PAIR is not a transaction, so a failed prune leaves the old status quo and a failed config write leaves an entry the next `update` restores. The role→dir ternary is single-sourced in `capabilityRelDir`, shared with `deleteCapabilityDir`, so the delete and the prune address the same directory by construction. Both call sites prune — the named path with `[target]`, the picker ONCE with the full selection (one store write matching the one config write). +**`pharn remove` addressing** (`commands/remove.ts`) is the inverse of `add`. `remove ` / `remove :` (no arg → an interactive picker over the installed capabilities) deletes that one isolated capability dir — addressed at the project's recorded `layout` (flat `pharn-review` / `pharn-pipeline/grillers/`, OR the same under `pharn/`, via `configLayout` + `layoutPaths`) — and drops its `capabilities` entry. **No clone, no network** — everything is derivable from `config.capabilities` + the filesystem, so `remove.ts` imports no repo module at all; `archetypes` is never touched; `CONSTITUTION.md`/`memory-bank/` are **never** touched (they are not capability dirs). Removing an entry whose stored `source` is **literally** `'auto'` warns that the next `update` will reinstall it; an **absent** `source` warns NOTHING (absence means provenance-unknown, and a false warning on a legacy manual add is worse than silence) — derived from the stored field only, so `remove` stays zero-network. Not-installed → benign no-op listing the removable capabilities; a name installed in both roles → hard-fail (ambiguous). `remove` has no `--yes`, and the reason is per-path: the NAMED remove deletes without asking (no prompt to skip), while the bare picker's ONE confirm — listing the picks, default No — IS the destructive gate, so nothing may skip that either. **`remove`'s `ALLOWED_FLAGS` row is empty, so `pharn remove --yes` now exits 1** naming the flag. This SUPERSEDES the previous contract, which read: _"`--yes` stays a declared minimist boolean only because it is `update`'s flag; that is what keeps `pharn remove --yes` a harmless parse rather than an unknown-option refusal, and turning it into a refusal belongs to a per-command allowlist, not to the dispatch."_ That allowlist now exists (see the dispatch paragraph above), so the deferral is spent — but its reasoning still holds and is why the row is empty rather than `['yes']`: there is no confirm on the named path to skip, and the picker's one confirm may not be skipped. `--yes` is still declared globally, because the declaration list is DERIVED from the table and `update`'s row grants it. Before any delete (the picker: for the whole selection, before its confirm) `remove` refuses with exit 1 when a target dir's path crosses a symlinked component (`findSymlinkComponent`), since `rmSync` resolves ancestors. Every delete is a **strict child** of its role subtree — `safeChildJoin(safeJoin(cwd, subtree), name)` (`lib/validate.ts`), which, unlike `safeJoin`, never returns the base itself nor a deeper path — a second floor under the ingest-time name validation. `remove` also **prunes that capability's entries from `pharn.records.json`** (`pruneCapabilityRecords`), so it is no longer the one write path leaving records that describe bytes that are gone — the converse of the invariant `update` is pinned to. The prune is a **key-prefix filter over the store**, never an fs enumeration: a walk of the DEST dir finds nothing after the delete AND nothing before it on the "already gone" path — which is exactly where the stale keys are the only thing left to clean, so the store is the only place the answer still exists. It mirrors `add`'s `mergeCapabilityRecords` guard verbatim (`recordsBaseline(...) === null → return`): absent/corrupt/stale-stamped → the file is left byte-identical, **never** minted and never blessed (the baseline `note` is deliberately not surfaced — `update` is where an unusable store changes an outcome, and it is the command that names the reason). The trailing slash in `relDir + '/'` is load-bearing (`lenses/a11y` must not eat `lenses/a11y-extended`); nothing matched → the write is skipped entirely; and the `skillsVersion`/`commit` stamp is re-written from the CONFIG pair unchanged, since `remove` advances neither. Order is delete → prune → config (mirroring `add`'s records-before-config) — a benignity argument, not atomicity: each write is individually atomic (`lib/atomic-write.ts` — temp-then-`rename`), but the PAIR is not a transaction, so a failed prune leaves the old status quo and a failed config write leaves an entry the next `update` restores. The role→dir ternary is single-sourced in `capabilityRelDir`, shared with `deleteCapabilityDir`, so the delete and the prune address the same directory by construction. Both call sites prune — the named path with `[target]`, the picker ONCE with the full selection (one store write matching the one config write). **`pharn update` (`commands/update.ts`) is drift-safe by default.** It re-resolves the recorded archetypes and **unions** the result with the user's manual adds (`lib/merge-capabilities.ts` — the pure 9-row membership table: `next = resolve(archetypes) ∪ manual`; sticky manual; a manual entry gone from the index is dropped, its files left alone; a `source`-less legacy entry is inferred ONCE at merge time — in the resolved set → `auto`, outside it → `manual` — which is the ONLY place absence may be resolved). Every membership change is NAMED in a `CAPABILITIES` note (`added` / `dropped-unselected` / `dropped-gone` / `kept-manual`); zero changes print nothing. Resurrection of a removed capability is **reported, not prevented** (no tombstones). It then decides **per file** instead of re-copying wholesale: `lib/install-records.ts` holds `pharn.records.json` (a sha256 of every file an install wrote, hashed at the DEST, stamped with the config's `skillsVersion`/`commit` so a store left by another tool is detected and ignored); `lib/update-decision.ts` is the PURE 6-row table (`decideFileAction` + `planUpdate` — missing→restore, identical→no-op, equals-record→upgrade, else SKIP `modified`/`unrecorded`/`unverifiable`); `lib/apply-update.ts` executes the writes (dest-symlink refusal, parent `mkdir`, and an `ApplyError` carrying what was already written so those files are still recorded on a partial failure); `lib/backup.ts` copies every `--force` casualty to `.pharn-backup//` BEFORE any original is touched. Records are written BEFORE the config, and a run that skipped anything **withholds** the `skillsVersion`/`commit` bump so the recorded version stays true and the next run still has work. `--force` overwrites the skip buckets and bypasses the same-version early-return. Update **never deletes** and never touches `.claude/settings.json`. `CONSTITUTION.md` (from `paths.docs` in `lib/install-manifest.ts` — flat: root; pharn layout: `pharn/CONSTITUTION.md`) is in the expected trusted-doc set and follows the same per-file table: missing→restore, still-at-recorded-hash→upgrade, locally modified→skip (`modified`); `add`/`remove` never touch it. It records the layout detected in the CLONE (closing the latent drift where bytes landed at `pharn/` paths while the config still said `flat`). diff --git a/README.md b/README.md index c651a28..4915aeb 100644 --- a/README.md +++ b/README.md @@ -249,7 +249,9 @@ Three behaviours matter if you script the CLI: PHARN is intentionally conservative about writes: -- `init` checks for existing install targets and asks before overwriting them. +- `init` checks for existing install targets and asks before overwriting them, + and refuses the whole install — writing nothing — when a destination path + crosses a symlinked directory. - `update` uses `pharn.records.json` to skip files it cannot prove are untouched. - `update --force` backs up overwritten files under `.pharn-backup//` @@ -259,7 +261,7 @@ PHARN is intentionally conservative about writes: - `status` and `list` are read-only. - `remove` deletes only the selected capability directory and prunes its records; it never touches `CONSTITUTION.md`, `memory-bank/`, or your detected - archetypes. + archetypes, and refuses when that directory's path crosses a symlink. PHARN can also refuse to install at all: if upstream declares a minimum CLI version newer than yours, `init`/`add`/`update` stop with a named error before diff --git a/src/commands/remove.ts b/src/commands/remove.ts index 77b328a..6bc59f4 100644 --- a/src/commands/remove.ts +++ b/src/commands/remove.ts @@ -17,6 +17,7 @@ import { interactiveAllowed, } from '../lib/capability-picker.js'; import { safeChildJoin, safeJoin } from '../lib/validate.js'; +import { findSymlinkComponent } from '../lib/symlink-guard.js'; import { ProjectLockedError, withProjectLock } from '../lib/project-lock.js'; import { loadArchetypeConfigOrExit, @@ -100,6 +101,36 @@ function deleteCapabilityDir( return existed; } +// Refuse (exit 1, nothing deleted) when any target's directory path crosses a +// symlinked component below the project root. `rmSync` on `pharn/pharn-review/x` +// resolves every ancestor, so with `pharn -> ../shared` it deletes +// `../shared/pharn-review/x` — outside the project — and `safeJoin` cannot see +// it (lexical). Checked for the WHOLE selection before the first delete (and, +// on the picker path, before the confirm), with no lock held, so the exit +// leaves nothing behind. The same physical walk `update`/`add` use. ENOTDIR (a +// component below a regular file) is not a link: that dir does not exist, and +// the delete's own existsSync already treats it as gone. +function refuseSymlinkedTargets( + cwd: string, + paths: LayoutPaths, + targets: InstalledCapability[], +): void { + for (const target of targets) { + let linked: string | null; + try { + linked = findSymlinkComponent(cwd, capabilityRelDir(paths, target)); + } catch { + linked = null; + } + if (linked !== null) { + logError( + `Refusing to remove ${target.name} (${target.role}): ${linked} is a symbolic link, so deleting through it would delete files OUTSIDE the project. Nothing was removed. Delete the directory yourself if that is what you want.`, + ); + process.exit(1); + } + } +} + // One source for the role→dir mapping. The delete addresses the filesystem and // the prune addresses the record store; they MUST name the same directory, and // sharing the ternary makes that true by construction rather than by two copies @@ -236,6 +267,7 @@ async function removeNamed( const target = matches[0]!; const paths = layoutPaths(configLayout(config)); + refuseSymlinkedTargets(cwd, paths, [target]); // The single-writer lock spans delete → prune → config: the three writes are // individually atomic but the sequence is not, so another process must not // land between them. `remove` has no prompt on this path and no network, so @@ -328,6 +360,11 @@ async function runRemovePicker( if (target) targets.push(target); } + const paths = layoutPaths(configLayout(config)); + // Before the confirm: a selection that would be refused must not be asked + // about first. + refuseSymlinkedTargets(cwd, paths, targets); + // ONE confirm listing what will be removed (default No — destructive). const ok = await confirm({ message: `Remove ${targets.length} ${plural(targets.length)}: ${targets @@ -337,7 +374,6 @@ async function runRemovePicker( }); if (isCancel(ok) || ok !== true) cancelAndExit(); - const paths = layoutPaths(configLayout(config)); // Taken AFTER the confirm — holding it across an unanswered destructive // prompt would block an agent hook for as long as a human takes to answer — // and spanning the whole selection, not one lock per capability. diff --git a/src/lib/install-capabilities.ts b/src/lib/install-capabilities.ts index 0bd20f5..6a73087 100644 --- a/src/lib/install-capabilities.ts +++ b/src/lib/install-capabilities.ts @@ -23,6 +23,7 @@ import { resolveFeaturesReadme, type LayoutPaths, } from './layout.js'; +import { collectExpectedInstallPaths } from './install-manifest.js'; import { findSymlinkComponent } from './symlink-guard.js'; import type { InstalledCapability, Layout, Selection } from '../types.js'; @@ -156,6 +157,10 @@ export function installCapabilities( const paths = layoutPaths(detectLayout(repoDir)); const featuresRel = resolveFeaturesReadme(repoDir, 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); + // Copy the selected capability dirs (pre-flighted; no partial installs). const capabilities = installCapabilityDirs( repoDir, @@ -309,6 +314,60 @@ export function installCapabilities( }; } +/** + * Refuse the whole install when any path it would write crosses a symlinked + * component below `projectRoot`. Every copy here is `safeJoin`-contained, but + * that check is LEXICAL: measured, a project whose `.claude/commands` or `pharn/` + * is a symlink to an external directory takes `cpSync` straight THROUGH it, + * creating and overwriting files outside the project with no prompt (the + * overwrite check's `existsSync` sees no conflict inside an empty target). This + * is the posture `update` (apply-update.ts) and `add` (dest-drift.ts) already + * take, applied once to the full write set: every file the install manifest + * says this install writes, plus the user-owned `.claude/settings.json`. + * + * 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. + * + * 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, +): 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; + } + 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\`.`, + ); +} + +const MAX_LINKED_SHOWN = 5; + /** * May the install write `rel` under `projectRoot`? False when any component * below the root is a symlink — following one writes OUTSIDE the project, which diff --git a/tests/install-capabilities.test.ts b/tests/install-capabilities.test.ts index a6353f6..001054c 100644 --- a/tests/install-capabilities.test.ts +++ b/tests/install-capabilities.test.ts @@ -2,12 +2,13 @@ import { existsSync, lstatSync, mkdirSync, + readdirSync, readFileSync, rmSync, symlinkSync, writeFileSync, } from 'node:fs'; -import { join } from 'node:path'; +import { join, relative } from 'node:path'; import { describe, expect, it } from 'vitest'; import { useTmpDir } from './helpers.js'; import { @@ -372,6 +373,8 @@ describe('installCapabilities', () => { // through it, writing outside the project root — and the pre-install overwrite // check never warns, because existsSync on the absent leaf inside that link is // false. safeJoin is lexical and cannot see it. + // Since PHARN-02 a symlinked destination component refuses the WHOLE install + // (the pre-flight), rather than silently skipping this one surface. it('does NOT write through a symlinked features/ in the PROJECT (no escape)', () => { const repo = join(tmp.path(), 'destlink-repo'); const proj = join(tmp.path(), 'destlink-proj'); @@ -381,9 +384,12 @@ describe('installCapabilities', () => { scaffoldRepo(repo); symlinkSync(outside, join(proj, 'features')); - installCapabilities(repo, proj, selection()); + expect(() => installCapabilities(repo, proj, selection())).toThrow( + /Refusing to install: features is a symbolic link/, + ); expect(existsSync(join(outside, 'README.md'))).toBe(false); + expect(existsSync(join(proj, 'CONSTITUTION.md'))).toBe(false); }); // ORDER: the file is OPTIONAL, so a clone without it must never reach the @@ -911,3 +917,130 @@ describe('installCapabilities — pharn/ layout (mirrors PR #86)', () => { expect(existsSync(join(proj, '.claude/settings.json'))).toBe(true); }); }); + +// PHARN-02: every install destination crossing a symlinked project directory is +// refused BEFORE the first write — `safeJoin` is lexical and `cpSync` follows a +// link, so without this a symlinked `.claude/commands` or `pharn/` received the +// install outside the project. +describe('installCapabilities — symlinked destination pre-flight (PHARN-02)', () => { + const tmp = useTmpDir(); + + function tree(root: string): string[] { + if (!existsSync(root)) return []; + const out: string[] = []; + const walk = (dir: string): void => { + for (const e of readdirSync(dir, { withFileTypes: true })) { + const p = join(dir, e.name); + out.push(relative(root, p)); + if (e.isDirectory()) walk(p); + } + }; + walk(root); + return out.sort(); + } + + it.each([ + ['.claude'], + ['.claude/commands'], + ['.claude/hooks'], + ['pharn-review'], + ['pharn-pipeline/grillers/a11y'], + ['pharn-contracts'], + ['.dev/floor'], + ])( + 'refuses when %s is a symlink out of the project; nothing written anywhere', + (link) => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const outside = join(tmp.path(), 'outside'); + scaffoldRepo(repo); + write(join(outside, 'mine.md'), 'USER FILE'); + mkdirSync(join(proj, link, '..'), { recursive: true }); + symlinkSync(outside, join(proj, link)); + const outsideBefore = tree(outside); + const projBefore = tree(proj); + + expect(() => installCapabilities(repo, proj, selection())).toThrow( + `Refusing to install: ${link} is a symbolic link`, + ); + + expect(tree(outside)).toEqual(outsideBefore); + expect(readFileSync(join(outside, 'mine.md'), 'utf8')).toBe('USER FILE'); + expect(tree(proj)).toEqual(projBefore); + }, + ); + + it('refuses a symlinked .claude/settings.json leaf (dangling link would be written through)', () => { + 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(join(proj, '.claude'), { recursive: true }); + symlinkSync( + join(outside, 'settings.json'), + join(proj, '.claude/settings.json'), + ); + + expect(() => installCapabilities(repo, proj, selection())).toThrow( + /\.claude\/settings\.json is a symbolic link/, + ); + expect(existsSync(join(outside, 'settings.json'))).toBe(false); + }); + + it('names every symlinked component, sorted', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const outside = join(tmp.path(), 'outside'); + scaffoldRepo(repo); + mkdirSync(join(outside, 'a'), { recursive: true }); + mkdirSync(join(outside, 'b'), { recursive: true }); + mkdirSync(join(proj, '.claude'), { recursive: true }); + symlinkSync(join(outside, 'a'), join(proj, 'pharn-review')); + symlinkSync(join(outside, 'b'), join(proj, '.claude/hooks')); + + expect(() => installCapabilities(repo, proj, selection())).toThrow( + /\.claude\/hooks, pharn-review are symbolic links/, + ); + }); + + it('refuses a symlinked pharn/ root in a pharn-layout install', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const outside = join(tmp.path(), 'shared'); + write(join(repo, 'pharn/pharn-pipeline/grillers/a11y/a11y.md'), 'a11y'); + write(join(repo, 'pharn/pharn-review/n-plus-one/n-plus-one.md'), 'npo'); + write(join(repo, 'pharn/pharn-contracts/finding-shape.md'), 'fs'); + write(join(repo, 'pharn/CONSTITUTION.md'), 'C'); + write(join(outside, 'pharn-pipeline/grillers/a11y/a11y.md'), 'TEAM COPY'); + mkdirSync(proj, { recursive: true }); + symlinkSync(outside, join(proj, 'pharn')); + const before = tree(outside); + + expect(() => installCapabilities(repo, proj, selection())).toThrow( + /Refusing to install: pharn is a symbolic link/, + ); + expect(tree(outside)).toEqual(before); + expect( + readFileSync( + join(outside, 'pharn-pipeline/grillers/a11y/a11y.md'), + 'utf8', + ), + ).toBe('TEAM COPY'); + }); + + it('a project with real (non-symlink) directories installs as before', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldRepo(repo); + mkdirSync(join(proj, '.claude/commands'), { recursive: true }); + mkdirSync(join(proj, 'pharn-review'), { recursive: true }); + + installCapabilities(repo, proj, selection()); + + expect(existsSync(join(proj, '.claude/commands/pharn-plan.md'))).toBe(true); + expect( + existsSync(join(proj, 'pharn-review/n-plus-one/n-plus-one.md')), + ).toBe(true); + }); +}); diff --git a/tests/remove.test.ts b/tests/remove.test.ts index df45ec7..68ceadf 100644 --- a/tests/remove.test.ts +++ b/tests/remove.test.ts @@ -4,6 +4,7 @@ import { readdirSync, readFileSync, rmSync, + symlinkSync, writeFileSync, } from 'node:fs'; import { dirname, join, relative } from 'node:path'; @@ -293,6 +294,62 @@ describe('runRemove (archetype)', () => { expect(existsSync(join(proj, '.git/HEAD'))).toBe(true); }); + // --- PHARN-02: never delete through a symlinked project directory ----------- + + it.each([['pharn'], ['pharn/pharn-review'], ['pharn/pharn-review/a11y']])( + 'refuses (exit 1) when %s is a symlink out of the project; outside files survive', + async (link) => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: 'a11y', role: 'lens' }], { layout: 'pharn' }), + ); + const shared = join(tmp.path(), 'shared'); + // The shared tree mirrors what `link` points at. + const target = join(shared, 'x'); + const rest = relative(link, 'pharn/pharn-review/a11y'); + write(join(target, rest, 'NOTES.md'), 'USER NOTES'); + mkdirSync(join(proj, link, '..'), { recursive: true }); + symlinkSync(target, join(proj, link)); + const before = snapshot(shared); + + await expect(runRemove('a11y')).rejects.toMatchObject(new ProcessExit(1)); + + expect(snapshot(shared)).toEqual(before); + expect(writePharnConfig).not.toHaveBeenCalled(); + expect( + vi.mocked(prompts.log.error).mock.calls.map(String).join(), + ).toContain(`${link} is a symbolic link`); + }, + ); + + it('picker: one unsafe pick refuses the WHOLE selection before the confirm', async () => { + setTTY(true, true); + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([ + { name: 'a11y', role: 'lens' }, + { name: 'a11y', role: 'griller' }, + ]), + ); + write(join(proj, 'pharn-pipeline/grillers/a11y/a11y.md'), 'SAFE'); + const shared = join(tmp.path(), 'shared'); + write(join(shared, 'a11y/NOTES.md'), 'USER NOTES'); + symlinkSync(shared, join(proj, 'pharn-review')); + vi.mocked(prompts.groupMultiselect).mockResolvedValueOnce([ + 'griller:a11y', + 'lens:a11y', + ]); + + await expect(runRemove(undefined)).rejects.toMatchObject( + new ProcessExit(1), + ); + + expect(prompts.confirm).not.toHaveBeenCalled(); + expect(existsSync(join(proj, 'pharn-pipeline/grillers/a11y/a11y.md'))).toBe( + true, + ); + expect(existsSync(join(shared, 'a11y/NOTES.md'))).toBe(true); + expect(writePharnConfig).not.toHaveBeenCalled(); + }); + // --- bare `pharn remove` (no arg): multi-select picker / non-TTY guard ------- it('no-arg in a TTY multi-selects, confirms, deletes each + one config write', async () => {