diff --git a/.dev/features/review-cleanups/GRILL.md b/.dev/features/review-cleanups/GRILL.md new file mode 100644 index 0000000..235eb81 --- /dev/null +++ b/.dev/features/review-cleanups/GRILL.md @@ -0,0 +1,81 @@ +# GRILL — review-cleanups + +Plan: `.dev/features/review-cleanups/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 + +### Honest scope / completeness (P7, P3) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: important + file: '.dev/features/review-cleanups/PLAN.md:90' + problem: 'The F21 abort design says ONE stderr line names the part-way stop and the backup dir, yet also that the existing abort tests keep their stderr assertions. tests/update.test.ts:1814-1826 pins the LAST log.info AND the LAST log.warn of an aborted run on stderr, so one line cannot satisfy both. Decide it here: keep the part-way warning (log.warn) and the backup pointer (log.info) on stderr and drop only the repeated .gitignore hint — the duplicate F21 names — so both existing assertions hold.' + evidence: 'The aborted path prints one stderr line that names the part-way stop and the backup dir.' +- type: FINDING + rule_id: 'P4' + severity: important + file: '.dev/features/review-cleanups/PLAN.md:115' + problem: 'The backfill names two merged pairs, but at least ten PHARN commits were later extended or superseded by an entry already in [Unreleased] or by this plan: PHARN-02/16 by #223, PHARN-04/17 by #222, PHARN-06 by #219 (>=20.12.0 → >=20.13.0), PHARN-10 by #220, PHARN-11 by #216/#223, PHARN-12 by #221, PHARN-13 by #217, PHARN-14 and PHARN-15 by this plan (F23, F22), PHARN-18 by #218. Each needs ONE entry describing the net behavior since 0.5.0, or the section contradicts itself. The build should read each commit rather than its title.' + evidence: 'Changes that a later entry already describes (e.g. PHARN-11''s manual carry, extended by #216; PHARN-13, extended by #217) get a single merged entry.' +``` + +### Determinism / parity (P5) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: important + file: '.dev/features/review-cleanups/PLAN.md:77' + problem: 'Upstream''s parser takes the frontmatter as text.slice(3, end), so the REMAINDER of the opening line is content: `--- name: a11y` on line 1 supplies `name`. The plan''s line rule ("opens on a first line that starts with ---") does not say what happens to that remainder, and the differential test over "every fence shape" will disagree on it unless the port takes the same slice. Port upstream''s slice semantics literally, and include an opening-line-remainder shape in the differential set.' + evidence: 'The fence opens on a first line that starts with `---` and closes at the next line that starts with `---`, which is the upstream rule' +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/review-cleanups/PLAN.md:63' + problem: 'SKIP_DIRS matches the dir name case-insensitively (detect-archetype.ts:157). The plan does not say whether ECOSYSTEM_DIRS does, nor whether the markers (Cargo.toml, go.mod, …) are matched exactly. State both; the conservative reading is: the dir name case-insensitively (as today), the marker names exactly as each tool writes them.' + evidence: 'A new `ECOSYSTEM_DIRS` map skips a dir only when its marker holds:' +``` + +### Eval coverage (P1) + +```yaml +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/review-cleanups/PLAN.md:93' + problem: 'The spinner mock in tests/update.test.ts:33 returns fresh anonymous vi.fn()s per spinner, so no assertion can order a spinner stop against a log line today. The call-order case needs a recording spinner mock (one shared log of stop/info/warn calls); changing the shared mock touches every update test, so keep it behavior-compatible.' + evidence: 'The spinner is stopped before the notice prints (call-order assertion on the clack mocks).' +- type: FINDING + rule_id: 'P7' + severity: minor + file: '.dev/features/review-cleanups/PLAN.md:88' + problem: 'Stopping s2 with "Backed up N file(s)" and then printing the notice''s "Backed up N file(s) to …" says the same thing twice on consecutive lines. Stop the spinner with a neutral phrase (or with the pointer itself and no repeat).' + evidence: 'The backup callback first stops `s2` ("Backed up N file(s)"), prints the notice, then starts' +``` + +### Checked, no finding + +- **Trust (P2).** The fence change widens the untrusted parser to upstream's rule only; the field + reader, enums and duplicate-key refusal are unchanged. The ecosystem test reads only names already + listed plus one `lstat` inside the user's own tree. +- **Scope.** The declared paths parse to 20 entries; the subtree refactor's six call sites are all + declared (`install-records`, `install-manifest` ×2, `install-capabilities`, `update`, `remove`). + A fresh scan finds exactly the three raw characters the plan names in `src/` and `tests/`, which + hold only `.ts` files, so the hygiene test needs no binary exclusions. +- **Honest scope (P7).** The hygiene test answers a real, twice-repeated slip (#218 and plan D). + +## Summary + +Sound plan; three points must be decided before the build. The abort output must keep a stderr +`log.warn` and a stderr `log.info`, or an existing test breaks. The fence port must take upstream's +slice, opening-line remainder included, or its own differential test fails. The CHANGELOG backfill +must merge about ten pairs, not two. Smaller points: the case rule for the ecosystem dirs, a +recording spinner mock, and one duplicated line. + +**ADVISORY VERDICT: 6 concerns raised (0 blocking-severity, 3 important, 3 minor) — for the human to +weigh before /pharn-dev-build.** diff --git a/.dev/features/review-cleanups/PLAN.md b/.dev/features/review-cleanups/PLAN.md new file mode 100644 index 0000000..2d437a1 --- /dev/null +++ b/.dev/features/review-cleanups/PLAN.md @@ -0,0 +1,174 @@ +# PLAN — review-cleanups (detector skips, fence parity, update's backup notice, one subtree helper, CHANGELOG + CLAUDE.md catch-up, raw invisible characters) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: the small findings left from the PHARN-01..18 review, in one PR, as the human grouped + them (plan F): + - F22: the detector skips `vendor`/`target`/`venv`/`.venv` only where they are that ecosystem's + tree. + - F23: the frontmatter fence rule matches upstream's validator. + - F21: update's backup notice is printed with no spinner running, and once per stream. + - The role → subtree ternary lives in one place. + - F26: `CHANGELOG.md` gets its missing entries for PHARN-01..18. + - `CLAUDE.md` catches up with #215–#223. + - Amended after GATE 1 (same kind of cleanup, found while shipping A–E and B): raw invisible + characters in two test files become escapes, a test keeps `src/` and `tests/` free of them, and a + stale function name in comments is corrected. +- layer(s): the CLI itself (`src/lib/detect-archetype.ts`, `src/lib/capability-index.ts`, + `src/commands/update.ts`, `src/lib/layout.ts` + 5 callers, `src/lib/symlink-guard.ts` comments), + tests, docs +- constitution_refs: [P1, P3, P4, P5, P6] + +## Discovery — verified this run (P6), code read on HEAD after #223 + +- **F22.** `SKIP_DIRS` (`detect-archetype.ts:77-104`) is matched at every depth, case-insensitively + (`:157`). PHARN-15 (#209) added `.venv`, `venv`, `__pycache__`, `vendor`, `target`, `.yarn`, and + its comment says "package.json still backstops it". That holds for `ssr`/`spa`, not for `backend`, + whose signal is structural (`app/**/route.ts`, migrations, `.sql`). So a Next.js route at + `app/target/route.ts` or `app/vendor/route.ts` goes dark: `[ssr, backend]` → `[ssr]`, reproduced + by the review on the same tree. `__pycache__` and `.yarn` are never hand-authored JS. + `target`/`vendor`/`venv` are ordinary route or folder names. +- **F23.** `extractFrontmatter` (`capability-index.ts:232-241`) requires `lines[0] === '---'` and + closes at the first `/^---[ \t]*$/` line. This repo's copy of the upstream validator + (`.dev/floor/validate.mjs:69-73`) opens on `startsWith("---")` and closes at the first + `"\n---"`, which is any line STARTING with `---` (`----`, `--- note`). So a capability upstream's + own CI passes becomes `unknown` here. If the body later holds a column-0 field, the reason shown + is a misleading duplicate-key error. Both parsers handle the empty block `---\n---` the same way. +- **F21.** `update.ts:343` prints the backup notice (`printBackupNotice`) from `onBackup` while + spinner `s2` is still animating, so the note's `│` is glued to the spinner frame, and below ~50 + columns `stop` erases its last row (the review captured this with clack 1.8.1). On a later throw, + `:396` prints the same two lines again on stderr. `add` stops its spinner before its backup + (`add.ts:217-219`, then its backup). +- **Subtree ternary.** `role === 'griller' ? paths.grillers : paths.lenses` appears at + `install-records.ts:369`, `install-manifest.ts:142,285`, `install-capabilities.ts:129`, + `update.ts:601`, and `remove.ts:150` (`capabilityRelDir`): six copies. `LayoutPaths` lives in + `layout.ts:40`. +- **F26.** `[Unreleased]` has entries only for #213 onward. None of the 18 PHARN commits (#195–#212, + after the 0.5.0 release) added one. +- **CLAUDE.md** (not hook-protected; `protect-trusted-paths.cjs:58`): + - The init step-5 passage still describes init's carry-over as "keep manual entries the index + still has", from before #216. It does not mention the kept unparseable entries, the + `frozenCapabilities` carry, the named drop, the fingerprint re-check under the lock, or #215's + preserved user keys. + - The update paragraph never mentions `frozenCapabilities` (PHARN-13/#217): 0 hits. + - The `add` passage calls the drift scan `collectDestDrift`; it is `scanDest`. +- **Raw invisible characters (new).** `tests/tar-extract.test.ts:653` holds a raw U+FEFF (from + #218), `tests/terminal-safe.test.ts:8-9` a raw U+202E and U+200B. An editor or a diff view shows + nothing there, and the same slip reached plan D's files before it was caught. No check covers + `src/` or `tests/` for this. +- **Stale name (new).** `symlink-guard.ts:45,51` name `collectDestDrift` for what is `scanDest`. + +## Files + +- `src/lib/detect-archetype.ts` — layer CLI/lib. `SKIP_DIRS` keeps the always-skipped set (caches, + VCS, `__pycache__`, `.yarn`). A new `ECOSYSTEM_DIRS` map skips a dir only when its marker holds: + - target: a sibling Cargo.toml, pom.xml or build.sbt; + - vendor: a sibling go.mod, composer.json or Gemfile; + - venv and .venv: a pyvenv.cfg inside. + + The sibling test is a membership check over the parent's already-read `entries`, so it costs no + extra read. The venv test is one `lstat`. The dir name is matched case-insensitively, as + `SKIP_DIRS` is today; the marker names exactly as each tool writes them (grill). +- `tests/detect-archetype.test.ts` — layer tests: + - A route at app/target/route.ts with no sibling marker → `backend` detected. FAILS on base. + - A route at app/vendor/route.ts → the same. FAILS on base. + - A target/ dir next to Cargo.toml holding more entries than the budget → still skipped, no budget + spent (guard). + - A .venv/ dir holding pyvenv.cfg → skipped (guard). + - The existing pins adjust to the conditional members. +- `src/lib/capability-index.ts` — layer CLI/lib. The fence is upstream's slice, ported literally: + the text must start with `---`, and the frontmatter is everything from the fourth character up to + the first newline followed by `---` at or after it — so it closes at the next line that STARTS + with `---`, and the rest of the opening line counts as frontmatter (grill). The comment cites + `validate.mjs`. +- `tests/capability-index.test.ts` — layer tests: + - A closing `----` parses. FAILS on base. + - A closing `--- note` parses. FAILS on base. + - A body horizontal rule after a proper close changes nothing (guard). + - The empty block is still refused for missing fields (guard). + - A differential case runs every fence shape through a verbatim copy of upstream's + `parseFrontmatter` and requires the same accept/refuse verdict. The shapes include a field on + the opening line itself. +- `src/commands/update.ts` — layer CLI/commands: + - The backup callback first stops `s2` with a neutral phrase (the notice right after it names + the count and the directory), prints the notice, then starts `s3` for the writes + (`spinnerRef` follows it). + - The aborted path keeps the part-way warning (`log.warn`) and the backup pointer (`log.info`) on + stderr, and drops only the repeated `.gitignore` hint (grill: an existing test pins both + stderr levels). +- `tests/update.test.ts` — layer tests: + - The spinner is stopped before the notice prints: a recording spinner mock keeps one ordered + log of stop/info/warn calls, behavior-compatible for every other case. FAILS on base. + - An aborted `--force` run prints the `.gitignore` hint once, and the backup dir once on stdout + and once on stderr. FAILS on base. + - The two existing abort tests keep their stderr assertions. +- `src/lib/layout.ts` — layer CLI/lib. `capabilitySubtree(paths, role)`, the one role → subtree + mapping (refactor, no behavior change). Its callers follow. +- `src/lib/install-records.ts` — layer CLI/lib. Calls `capabilitySubtree`. +- `src/lib/install-manifest.ts` — layer CLI/lib. Calls `capabilitySubtree` (two sites). +- `src/lib/install-capabilities.ts` — layer CLI/lib. Calls `capabilitySubtree`. +- `src/commands/remove.ts` — layer CLI/commands. `capabilityRelDir` delegates to `capabilitySubtree`. +- `tests/layout.test.ts` — layer tests. `capabilitySubtree` for both roles × both layouts. +- `src/lib/symlink-guard.ts` — layer CLI/lib. Comments only: `collectDestDrift` → `scanDest`. +- `tests/tar-extract.test.ts` — layer tests. The raw U+FEFF becomes a `\uFEFF` escape; the bytes + under test are identical. +- `tests/terminal-safe.test.ts` — layer tests. The raw U+202E / U+200B become escapes; the strings + under test are identical. +- `tests/source-hygiene.test.ts` — layer tests. New: no file under `src/` or `tests/` contains a raw + C0 (other than tab, LF, CR), C1, Unicode format (Cf) or U+2028/2029 character. FAILS on base (the + three above). +- `CHANGELOG.md` — `[Unreleased]`: backfilled entries for PHARN-01..18, in the existing + Fixed/Security/Changed sections plus `### Added`, one user-facing sentence or two each, written + from each commit's diff rather than its title. A change that a later entry already describes, or + that this plan changes again, gets ONE entry for the net behavior since 0.5.0 (grill): PHARN-02 + and -16 with #223, -04 and -17 with #222, -06 with #219, -10 with #220, -11 with #216/#223, -12 + with #221, -13 with #217, -14 with F23, -15 with F22, -18 with #218. +- `CLAUDE.md` — the init step-5 carry-over sentence describes #216 and #215; a `frozenCapabilities` + sentence goes in the update paragraph; the `add` passage names `scanDest`; the detect-archetype, + frontmatter-fence and subtree-helper mentions match this change. +- `docs/commands/init.md` — layer docs. The skip list says `target`/`vendor`/`venv` are skipped only + beside their ecosystem's marker. +- `docs/troubleshooting.md` — layer docs. The same, in the monorepo paragraph. + +## Contracts satisfied + +- PHARN-14's "the CLI reads what upstream ships". The fence rule is now the one upstream's CI + enforces (cited, P4). +- PHARN-10's "the backup pointer is printed the moment it exists" is kept. Only its framing changes. + +## Evals to write (P1) + +- Listed under Files. Seven cases FAIL on the base. The refactor is covered by the existing suites + plus `layout.test.ts`. + +## Guarantee audit (P0) + +- "a hand-authored route under `target/`/`vendor/` is scanned" → floor: detector tests. +- "the CLI accepts every fence upstream's validator accepts" → floor: a differential test against a + pinned copy of upstream's parser. The residual is named: a future upstream parser change is not + seen until that copy is refreshed. +- "no log line is printed while a spinner animates in update's apply" → floor: a call-order test. +- "no raw invisible character in `src/` or `tests/`" → floor: the hygiene test. +- CHANGELOG / CLAUDE.md accuracy → advisory (a human reads it). markdownlint is the only floor. + +## Trust audit (P2) + +- The fence change widens what the untrusted-frontmatter parser accepts to exactly upstream's + rule. The field reader, the enums and the duplicate-key refusal are unchanged, and only + `name`/`role`/`applies` are read. + +## Determinism audit (P5) + +- Name membership (sibling markers), line-prefix tests, a code-point class. No fallback guessing. + +## 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. F22 rule. (a) Skip `target`/`vendor`/`venv`/`.venv` only beside their ecosystem's marker, at any + depth — recommended. This keeps PHARN-15's budget protection for nested Maven, PHP or Python + trees. (b) Skip those four only at the project root. That is simpler, but a nested PHP `vendor/` + exhausts the walk budget again. (c) Leave it and document the tradeoff. +2. F23 direction. (a) Match upstream's `startsWith('---')` rule — recommended. The CLI must accept + what upstream ships. (b) Stay strict and give a clearer reason. Upstream-valid capabilities are + then still skipped. diff --git a/.dev/features/review-cleanups/REGRESSION.md b/.dev/features/review-cleanups/REGRESSION.md new file mode 100644 index 0000000..a7c6f3f --- /dev/null +++ b/.dev/features/review-cleanups/REGRESSION.md @@ -0,0 +1,45 @@ +# REGRESSION — review-cleanups + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `8b53ba91cf278c270ecc2d0ddda3140f33477a1c` (`HEAD` — `origin/main` after #223; the + build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): + - `src/lib/detect-archetype.ts`, `src/lib/capability-index.ts`, `src/lib/layout.ts`, + `src/lib/install-records.ts`, `src/lib/install-manifest.ts`, `src/lib/install-capabilities.ts`, + `src/lib/symlink-guard.ts`, `src/commands/update.ts`, `src/commands/remove.ts` + - `tests/detect-archetype.test.ts`, `tests/capability-index.test.ts`, `tests/update.test.ts`, + `tests/layout.test.ts`, `tests/tar-extract.test.ts`, `tests/terminal-safe.test.ts`, + `tests/source-hygiene.test.ts` (new) + - `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/review-cleanups/REVIEW.md b/.dev/features/review-cleanups/REVIEW.md new file mode 100644 index 0000000..77a4617 --- /dev/null +++ b/.dev/features/review-cleanups/REVIEW.md @@ -0,0 +1,90 @@ +# REVIEW — review-cleanups + +Increment: + +- `src/lib/detect-archetype.ts`: `target`/`vendor`/`venv`/`.venv` leave `SKIP_DIRS` for an + `ECOSYSTEM_DIRS` map, skipped only beside their build file (checked over the parent's + already-read entries) or, for a virtualenv, holding `pyvenv.cfg` (one `lstat`). +- `src/lib/capability-index.ts`: the frontmatter fence is upstream's slice, ported literally. +- `src/commands/update.ts`: the backup callback stops the running spinner, prints the notice, and + starts a new one for the writes. An abort repeats only the warning and the pointer on stderr. +- `src/lib/layout.ts` `capabilitySubtree`, used at the six sites that carried the ternary. +- Comment fix in `src/lib/symlink-guard.ts`; raw invisible characters in two test files became + escapes; a new `tests/source-hygiene.test.ts`. +- CHANGELOG backfill for PHARN-01..18 (merged into net-since-0.5.0 entries), CLAUDE.md catch-up, + docs skip lists. + +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 (1649 tests), `/pharn-dev-regress` returned `no-regressions`, and `/pharn-dev-verify` +returned `PASS`. The chain ran twice: this review's first read found a misplaced comment (advisory +finding 1), fixed within the plan's `## Files` before the second run. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0)** — each claim has a test: + - "a hand-authored route under `target/`/`vendor/` is scanned" → the detector tests (and a base + probe: `[ssr]` there, `[ssr, backend]` now); + - "the CLI accepts every fence upstream's validator accepts" → the differential test, run + against the validator's own function extracted from `.dev/floor/validate.mjs`; + - "no log line is printed while a spinner animates in update's apply" → the recording-mock + order test; + - "no raw invisible character in `src/` or `tests/`" → the hygiene test, which also pins its own + class against each kind it names. + - CHANGELOG / CLAUDE.md accuracy stays advisory, as the plan labels it. +- **L-eval (P1)** — 16 cases fail on the base (VERIFY.md lists them); the guards pass on both. +- **L-trust (P2)** — the fence now accepts exactly what upstream's validator accepts, plus the + named CRLF leniency; the enums, the field reader and the duplicate-key refusal are unchanged. The + detector reads names it already listed and one `lstat` inside the user's own tree, never through + a symlink (symlinked entries are skipped before the check). +- **L-axis (P3)** — each change stays in the module that owns it. The subtree mapping moved to + `layout.ts`, which already owns `LayoutPaths`; `remove.ts` lost its private copy. + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P3' + severity: minor + file: 'src/lib/layout.ts:119' + problem: 'RESOLVED IN THIS INCREMENT. The first build inserted capabilitySubtree between configLayout and its leading comment, so that comment ("The layout an installed project was recorded with…") sat above the wrong function. Moved above the comment in the second iteration.' + evidence: 'export function capabilitySubtree(' +- type: FINDING + rule_id: 'P2' + severity: minor + file: 'tests/capability-index.test.ts:482' + problem: 'The differential test evaluates code with `new Function`. The text is this repo''s committed .dev/floor/validate.mjs, not anything fetched, and the extraction must match exactly one `function parseFrontmatter(text)` or the test fails — but a reader should know the test executes that file''s function body.' + evidence: 'return new Function(`${found![0]}; return parseFrontmatter;`)() as (' +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'tests/capability-index.test.ts:530' + problem: 'The one named difference from upstream: a CRLF block now parses here (the multiline field pattern matches before a CR) while upstream''s per-line pattern yields no field and its CI refuses the file. On the base both refused. The direction is lenient, never stricter, and pinned by a test; aligning it would mean changing readField, outside this plan.' + evidence: "it('is more lenient than upstream on CRLF, never less', () => {" +- type: FINDING + rule_id: 'P7' + severity: minor + file: 'tests/source-hygiene.test.ts:49' + problem: 'The hygiene check covers src/ and tests/ only. docs/, scripts/, .claude/ and .dev/ are not scanned; a raw character there would still pass. Widening it is cheap but was not in the approved scope.' + evidence: "it.each(['src', 'tests'])('%s/ holds none', (dir) => {" +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:10' + problem: 'Merging to net-since-0.5.0 entries rewrote several entries that earlier PRs in this batch had added (#216, #221, #222, #223), and dropped two that described only unreleased intermediate states (the manifest built five times; the notice "no longer" printing raw in a note that did not exist in 0.5.0). Every remaining fact was kept; a reviewer comparing against those PRs will see them reworded rather than appended.' + evidence: '### Added' +``` + +## Proposed lesson (candidate — NOT canon) + +- None new. The raw-invisible-character slip that this increment fixes now has a floor check. + +## Verdict + +**GREEN — 0 floor-gate findings, 5 advisory (minor; one resolved in this increment).** The standing +decision is the human's (GATE 2). diff --git a/.dev/features/review-cleanups/SHIP.md b/.dev/features/review-cleanups/SHIP.md new file mode 100644 index 0000000..1b0cf07 --- /dev/null +++ b/.dev/features/review-cleanups/SHIP.md @@ -0,0 +1,39 @@ +# SHIP — review-cleanups + +Stages run, in order: + +1. `/pharn-dev-plan` → GATE 1 (human: plans A–F accepted with every recommended answer). The plan + was parked while A–E and B shipped, then brought back on the current `main`. Four changes were + made on the way, with the same intent: + - its `## Files` was rewritten for the writes-scope parser: sub-bullets that began with a + backtick would have been read as paths, and the subtree helper's callers were undeclared; + - the discovery line numbers were refreshed; + - three raw invisible characters, a `source-hygiene` test and a stale function name in + comments were added, the same kind of cleanup found while shipping A–E and B; + - the grill findings were folded in (below). +2. `/pharn-dev-grill`. Its findings were folded into the plan before the build: + - the abort output keeps a stderr warning and a stderr pointer (an existing test pins both); + - the fence is ported as upstream's slice, opening-line remainder included; + - the CHANGELOG backfill merges about ten pairs into net-since-0.5.0 entries; + - the case rule for ecosystem dirs is stated; + - the order test uses a recording spinner mock; + - the spinner stops with a phrase the notice does not repeat. +3. `/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review`. +4. `/pharn-dev-review` found one defect (REVIEW.md advisory finding 1). It was fixed within the + plan's `## Files`, and build → regress → verify ran again. Everything below is from that second + run. +5. 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) +- 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. This is the last plan of + the batch. + +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/review-cleanups/VERIFY.md b/.dev/features/review-cleanups/VERIFY.md new file mode 100644 index 0000000..92336c0 --- /dev/null +++ b/.dev/features/review-cleanups/VERIFY.md @@ -0,0 +1,57 @@ +# VERIFY — review-cleanups + +## 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 (1649 tests). The increment's new and changed test files were copied into a base + worktree (`8b53ba9`), where 16 of their cases fail, each for the reason the plan names: + - F22: a route at `app/target/route.ts` / `app/vendor/route.ts` detects `[ssr]`, not + `[ssr, backend]`. The committed file cannot load on the base (it imports the new + `ECOSYSTEM_DIRS`), so these two ran as a standalone probe of the same cases; + - F23: a closing `----` / `--- note`, an opening `----` / `--- note` and a field on the opening + line are refused on the base, where upstream's own parser accepts them; and the CRLF pin + (base refuses, the port reads it); + - F21: the notice printed while the spinner ran; the `.gitignore` hint printed twice; + - the hygiene test: `tests/` held three raw invisible characters; + - `capabilitySubtree`: absent on the base (4 cases). + + The guard cases pass on both: a marked ecosystem tree is still skipped at zero budget, the empty + block is still refused, and a body rule after a proper close changes nothing. +- The fence's differential test builds the parser from `.dev/floor/validate.mjs` itself, so it + cannot drift from that copy; it asserts the extraction found exactly one such function. +- `test:floor` is floor.yml's `node --test` run (754 tests). +- `npm run test:coverage` passes its ratchet (97.63 / 93.04 / 98.33 / 98.46) 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: []`). + +## 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 fence parity is against this repo's copy of upstream's validator. A future upstream change is + seen only once that copy is refreshed. +- The CHANGELOG and CLAUDE.md text is advisory: markdownlint is its only floor. Each backfilled + entry was written from its commit's message and checked against the later entries it merges with. +- The spinner ordering is proven through a recording clack mock, not a real terminal: the sandbox's + egress proxy answers codeload with 403, so `pharn update` could not be driven end to end here. diff --git a/.dev/features/review-cleanups/regression-report.json b/.dev/features/review-cleanups/regression-report.json new file mode 100644 index 0000000..80e0a76 --- /dev/null +++ b/.dev/features/review-cleanups/regression-report.json @@ -0,0 +1,38 @@ +{ + "base": "8b53ba91cf278c270ecc2d0ddda3140f33477a1c", + "inside": [ + "CHANGELOG.md", + "CLAUDE.md", + "docs/commands/init.md", + "docs/troubleshooting.md", + "src/commands/remove.ts", + "src/commands/update.ts", + "src/lib/capability-index.ts", + "src/lib/detect-archetype.ts", + "src/lib/install-capabilities.ts", + "src/lib/install-manifest.ts", + "src/lib/install-records.ts", + "src/lib/layout.ts", + "src/lib/symlink-guard.ts", + "tests/capability-index.test.ts", + "tests/detect-archetype.test.ts", + "tests/layout.test.ts", + "tests/source-hygiene.test.ts", + "tests/tar-extract.test.ts", + "tests/terminal-safe.test.ts", + "tests/update.test.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/review-cleanups/verify-report.json b/.dev/features/review-cleanups/verify-report.json new file mode 100644 index 0000000..21ed29d --- /dev/null +++ b/.dev/features/review-cleanups/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "review-cleanups", + "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 93b8998..abb39ff 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/init-reinstall-safety/SHIP.md" + ".dev/features/review-cleanups/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-25T12:41:05.339Z" + "set_at": "2026-09-25T13:05:36.351Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index d1471bc..e8dc3d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,50 +7,62 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **`pharn status` and `pharn update` report the PHARN hooks upstream wires and your project does not.** Upstream re-wired its hooks twice (6.1.0 and 6.12.0). `update` installs the hook scripts but never writes `.claude/settings.json`, so an existing install could keep dead or broken wiring with no signal. `status` now prints a `HOOKS` section, and `status --strict` exits 1 while an upstream hook is unwired; `update` prints the same note and still never writes the file. Hooks you added yourself never count against you. The check reads what Claude Code reads: + - both `.claude/settings.json` and your per-user `.claude/settings.local.json`, as one union. A hook wired only in `settings.local.json` counts as wired: `--strict` passes, and the note says it reaches neither teammates nor CI; + - a symlinked `settings.json` file, managed from dotfiles for example, is read through, as Claude Code does. A symlinked `.claude/` directory is refused; + - a FIFO at a settings path cannot block `status` or `update`. + + Each missing hook is printed as the hook itself, in JSON (an exec-form hook with its `args`), so a pasted line satisfies the check. + ### Changed -- **Node 20.13.0 or newer is now required (`engines.node: ">=20.13.0"`).** On Node 20.12.x, cancelling a `pharn` confirmation (Esc or Ctrl-C), or a picker after selecting something, crashed with `ERR_INVALID_ARG_VALUE`, exit 1 and a stack trace, instead of "Cancelled" and exit 0. The prompt library passes `util.styleText` an array of formats, which Node accepts only from 20.13.0, although the library itself declares `>= 20.12.0`. No project was ever changed at that point, but 20.12.x never worked properly, so this drops no working setup. `tests/engines.test.ts` now also scans the runtime dependencies' shipped code for known version-gated Node APIs, rather than trusting their declared floor. The smoke job that starts the packed CLI on the floor Node is now named `Smoke (node floor)`. +- **Node 20.13.0 or newer is now required (`engines.node: ">=20.13.0"`; 0.5.0 declared `>=20`).** On Node 20.0–20.11 even `pharn --version` failed at load time with a `SyntaxError`, because the prompt library imports `util.styleText`. On 20.12.x, cancelling a `pharn` confirmation (Esc or Ctrl-C), or a picker after selecting something, crashed with `ERR_INVALID_ARG_VALUE`, exit 1 and a stack trace, instead of "Cancelled" and exit 0: the library passes `styleText` an array of formats, which Node accepts only from 20.13.0, although it declares `>= 20.12.0`. No project was ever changed at that point, so the new floor drops no working setup. `tests/engines.test.ts` fails if a runtime dependency declares a higher floor than ours, or if its shipped code calls a Node API newer than ours. A separate smoke job, `Smoke (node floor)`, starts the packed CLI on exactly the floor. ### Fixed - **Releases could not publish: the `Pack` step wrote into a directory nothing had created.** Since the release workflow was split into an unprivileged `build` job and a `publish` job, `build` ran `npm pack --pack-destination "$RUNNER_TEMP/pkg"` without creating `pkg/`, and npm does not create it (`ENOENT` on npm 10 and 11). Every Release run would have failed at `Pack` and never reached `publish`. The step now runs `mkdir -p` first, and a live test pins that every workflow's pack destination is created earlier in the job that packs. -- **A re-run `pharn init` no longer loses what `pharn update` would keep.** It applies `update`'s own rules to the config it replaces: - - A capability you added by hand stays `manual` even when your archetypes now select it too. It used to be recorded `auto`, so a later archetype change let `update` drop it. - - A capability upstream still ships but this pharn cannot read is left as it is, however it was added. Its config entry, files and records are untouched, and it is listed in `frozenCapabilities`. It used to be dropped from the config and its files orphaned. - - A hand-added capability upstream no longer ships is still dropped, but now named. +- **A re-run `pharn init` no longer destroys your edits.** It used to overwrite every file it installs with no copy anywhere, and its prompt listed 10 of about 400 paths. It now copies to `.pharn-backup//`, before the first write, every file `pharn update` would have skipped, names that directory as soon as it exists, and lists those files first in the prompt, marked: + - `(edited)`: the file changed since pharn wrote it; + - `(no pharn record)`: it differs from upstream and `pharn.records.json` has no entry for it; + - `(differs from upstream)`: there is no usable records file, so every difference counts. + + A file still exactly as pharn wrote it is a clean upgrade: not marked, not backed up. `PHARN-LICENSE` / `pharn/LICENSE` is compared with upstream's `LICENSE`, like every other file with its source. +- **A re-run `pharn init` keeps what you added by hand.** It used to rebuild `capabilities` from your archetypes alone, dropping every capability you added with `pharn add` while its files stayed. It now applies `update`'s own rules to the config it replaces: + - A capability you added by hand is installed again and stays `manual`, even when your archetypes now select it too. + - A capability upstream still ships but this pharn cannot read is left as it is, however it was added. Its config entry, files and records are untouched, and it is listed in `frozenCapabilities`. + - A hand-added capability upstream no longer ships is dropped from the config, and named; its files stay. - Carried entries show as "added by hand" in the install summary, not also under SKIPPED. -- **`pharn init` no longer overwrites a config another `pharn` command wrote while its prompts were open.** It now re-checks `pharn.config.json` under its lock, like `add`, `update` and `remove`. If the file changed after `init` read it, `init` refuses and writes nothing. - **A re-run `pharn init` dropped the `pharn.config.json` keys you added by hand.** Upstream PHARN reads top-level keys that users add themselves: `testResults` (without it `/pharn-loop` stops with `blocked: no-test-runner`) and `ship.requireAttestation`. `add`, `update` and `remove` kept them, but `init` rebuilt the config from its own fields alone. It now copies every top-level key pharn does not own across from the config it replaces, unchanged. It does so from any file that parses as a JSON object, including one the other commands refuse. Keys pharn owns are still written fresh. -- **`pharn update` could stop re-checking a capability whose files were still out of date.** When pharn cannot read a capability upstream, `update` keeps it, skips its files, and moves the skills version on. Once pharn could read it again, the next run removed it from `frozenCapabilities` even if one of its files had to be skipped, for example because you edited it. Every later run then said "Already up to date", and that file stayed at the old version even after you resolved your edit. The capability now stays listed until none of its files are skipped, so each run checks it again. `update` also no longer writes a `pendingSkillsVersion` equal to the recorded `skillsVersion`. -- **An interrupted run no longer leaves `.pharn.lock` behind.** The lock was released only on a normal return or a `process.exit`. A real signal ends the process without either, so `pharn update --yes` cancelled in CI, or stopped by `timeout` or `docker stop`, exited 130/143 with the lock still in the project. On the same machine the next run reclaimed it. A machine sharing the directory (a container bind mount) had to wait out the six-hour staleness window. `SIGINT` and `SIGTERM` now release the lock and remove the temp download first, print that the project may be partially updated, and still exit 130 or 143. A hangup (`SIGHUP`) is deliberately not handled: listening for it would override `nohup`, so a `nohup pharn update` would stop mid-write. +- **No `pharn` command overwrites a config another `pharn` command wrote while it waited.** `add`, `update`, `remove` and `init` read `pharn.config.json` before taking the project lock, and `update`, the `remove` picker and `init` prompt in between. A concurrent run could therefore be overwritten from a stale copy: a `remove` confirmed after a concurrent `add` dropped the added capability while its files stayed. Each now re-checks the file under the lock and refuses, writing nothing, if it changed. And a lock another run is still writing is no longer mistaken for a stale one and broken. +- **A project `init` cannot finish installing into is refused before the first write.** A directory where `init` writes a file, or a file where it needs a directory — `pharn.config.json` and `pharn.records.json` included — used to surface part-way through the copy, leaving hundreds of new files with no config and no records. `init` now checks every path first and refuses, naming each one. A refused install writes nothing, `.pharn-backup/` included. +- **`pharn add` no longer dead-ends after an `update` that kept your edits.** `update` withholds the `skillsVersion` bump while any file is skipped, so a user keeping one local edit could never finish the upgrade, and `add`'s version gate kept sending them back to `pharn update`. When the bump is withheld only because of your own kept edits (`modified` / `unrecorded`), `update` now records a `pendingSkillsVersion`, and `add` accepts a download at that version while keeping the config's own version. Other skip kinds still refuse, and the refusal names the real ways out. +- **`pharn update` keeps re-checking a capability it had to keep.** When pharn cannot read a capability upstream, `update` keeps it, skips its files, and moves the skills version on. The next run then stopped at "Already up to date" before fetching, so the capability was never checked again, and its files stayed stale even after a pharn upgrade that could read it. `update` now lists such capabilities in `frozenCapabilities` in `pharn.config.json` and fetches while any is listed. One stays listed until none of its files is skipped (for example because you edited it), so each run checks it again. +- **An interrupted run exits non-zero, releases the lock and names the backup.** Ctrl-C while a spinner was running exited 0 and ran no cleanup: `pharn update --force` said "Canceled", left `.pharn.lock` behind, and never named the `.pharn-backup/` it had already made. A real signal did the same (`pharn update --yes` cancelled in CI, or stopped by `timeout` or `docker stop`), and a machine sharing the directory (a container bind mount) had to wait out the six-hour staleness window. An interrupted run now exits 130 (143 for `SIGTERM`), releases the lock and removes the temporary download first, and says the project may be partially updated. `update` names its backup directory the moment it creates it, and again on stderr if the run then fails. A hangup (`SIGHUP`) is deliberately not handled: listening for it would override `nohup`, so a `nohup pharn update` would stop mid-write. +- **A slow download can no longer hang `pharn`.** On Node 20 and 22, a fetch whose time limit had passed could keep reading a slowly dripping response indefinitely, because the abort stopped reaching the body after a garbage collection. `init`, `add`, `update` or `status` could hang, `add` and `update` while holding the project lock. Every fetch now has a hard deadline that holds either way. - **A slow or failed GitHub API response no longer keeps `pharn` running after it has finished.** When the commit-SHA lookup got an error response (for example a 403 rate limit), or a success whose body was still arriving at the 8-second limit, the command carried on without the SHA as designed, but it left that response unread. The open connection kept the process alive until the server finished sending. It was measured at 30 seconds after `pharn status` had printed its last line. Such responses are now released at once. -- **The proxy notice now says what Node's `fetch` actually does.** It was wrong in several cases, each now measured on the Node releases that matter: - - On Node 24.0–24.4, `NODE_USE_ENV_PROXY` works but the notice said this Node had no support. - - `--use_env_proxy` (underscores) was not recognised. - - `--no-use-env-proxy` did not count as turning the opt-in off. - - A mixed-case name such as `Https_Proxy`, which Node never reads, was reported as the proxy in use. - - When both cases were set, the notice named `HTTPS_PROXY`, although Node uses `https_proxy`. - - With only `HTTP_PROXY` set, pharn said nothing, although Node sends https requests through it once the opt-in is on. - - Only the notice changes; the network behaviour is Node's and is unchanged. +- **A capability whose `.md` is not a regular file no longer stops every command.** A directory at that path made `init`, `add`, `update` and `status` abort, so one oddly shaped upstream capability broke every deployed CLI at once. It is now reported as a capability pharn cannot read, like any other. A symbolic link there is never followed, and a FIFO cannot block the read. +- **The capability index reads frontmatter exactly as upstream's validator does.** A closing line that merely starts with `---` (`----`, `--- note`) made pharn skip a capability upstream's own CI accepts, and an empty block (`---` then `---`) was read from the document body instead. A `role` or `applies` key given twice is now reported as unreadable rather than pharn picking one of them, since upstream keeps the last. +- **Archetype detection copes with unusual projects.** A FIFO at `package.json` hung `init`, a symlink to `/dev/zero` was read without bound, and a UTF-8 byte-order mark made the parse fail, so a Next.js app detected as `lib`. `package.json` is now read once, as a regular file of at most 4 MiB, with a leading byte-order mark ignored. Large non-JS trees no longer exhaust the walk before it reaches your source: `__pycache__/` and `.yarn/` are skipped, and so are these, where they are another ecosystem's tree: `target/` beside a `Cargo.toml`, `pom.xml` or `build.sbt`, `vendor/` beside a `go.mod`, `composer.json` or `Gemfile`, and `venv/` or `.venv/` holding a `pyvenv.cfg`. A folder of your own with one of those names, such as the route at `app/target/route.ts`, is still scanned. +- **The proxy notice now says what Node's `fetch` actually does.** It said pharn would never use a configured proxy, which is false once you opt in on a Node that supports it (measured on 22.22.2 and 24): `fetch` then goes through the proxy variables with `NODE_USE_ENV_PROXY=1` or `--use-env-proxy`. The notice now says whether this run uses the proxy, and names the opt-in where your Node has one. It follows Node's measured rules: + - on Node 24.0–24.4, any non-empty `NODE_USE_ENV_PROXY` turns it on; + - the `--use_env_proxy` spelling and `--no-use-env-proxy` are recognised; + - `https_proxy` wins over `HTTPS_PROXY`; + - a mixed-case name such as `Https_Proxy`, which Node never reads, is not reported; + - `HTTP_PROXY` alone carries https requests once the opt-in is on. + + Only the notice changes; the network behaviour is Node's. - **The test suite no longer depends on the proxy settings of the machine running it.** With `NODE_USE_ENV_PROXY=1` set, 15 tests failed that pass in CI. With `HTTPS_PROXY` exported, 1 did. A setup file now clears the proxy variables before every test file. -- **`pharn status` and `pharn update` read the hooks Claude Code actually runs.** - - They now read both `.claude/settings.json` and your per-user `.claude/settings.local.json`, and compare their union with upstream. Hooks wired locally used to be reported missing, `status --strict` failed, and with no `settings.json` the note claimed "no PHARN hook is wired". A hook wired only in `settings.local.json` counts as wired: `--strict` passes, and the note says it reaches neither teammates nor CI. - - 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 -- **The `HOOKS` note no longer prints upstream text raw.** Hook commands from upstream's `settings.json` went through a control-character filter that let Unicode format characters through. A right-to-left override could make a path in the note read reversed, in a note that asks you to copy it by hand. Such characters, and the two Unicode line separators, are now shown as `\u` escapes. Each line is capped at 300 characters. -- **The extractor's name check now judges the name it writes.** It checked one decoding of a tar entry's name and wrote another. A raw C1 byte (such as `0x9B`, the terminal CSI) passed the control-character check and landed on disk as that control character, where `pharn status` and `pharn update` then printed it raw. Every non-ASCII name was also written garbled. Entry names are now decoded once, as strict UTF-8, and the check, the path rules and the write all use that one string. A name that is not valid UTF-8, or that starts with a byte-order mark, is refused. -- **Pax parsing is bounded per archive.** The 64 KiB cap applied to each global header, not to their number, so many headers each under the cap still cost seconds of CPU. The cap now covers all global headers in the archive together, and every header counts toward the entry limit. +- **A hand-edited `pharn.config.json` can no longer make `pharn remove` delete outside a capability directory.** An entry such as `{"name":"../..","role":"lens"}` made `pharn remove ../..` delete the project root, `.git` included. Every `capabilities[]` entry is now validated when the config is read (a capability name and a known role), and a bad one is named, with exit 1. `remove` also deletes only a single directory directly under its role's subtree. +- **`init` and `remove` no longer write or delete through a symbolic link in your project.** With `.claude/commands` or `pharn/` linked to another directory, `init` wrote its files outside the project, overwriting what was there without a prompt. `pharn remove a11y` with `pharn -> ../shared` deleted `../shared/pharn-review/a11y`. Both now refuse, naming each linked component, before writing or deleting anything. A link at a file `init` writes is refused too, since the copy would replace it. `.claude/settings.json` is the exception while its link points at an existing file: `init` never writes an existing settings file, so the link is left alone. A link there that points at nothing is refused. +- **Only a release job that installs nothing can mint the npm publish token.** The release workflow ran `npm ci` and every build and test script in the same job that held `id-token: write`, so any of about 280 dev dependencies could have requested the token and published a malicious version with valid provenance. Releases now build and test in a job with no token, and publish the tested tarball from a job that checks out and installs nothing. +- **Text from upstream never reaches your terminal raw.** Extractor errors printed raw entry names and type bytes, which a hostile archive could use to fake output, erase lines or set the clipboard (OSC 52). The unknown-capability list let Unicode format characters, such as a right-to-left override, through. Every such message now goes through one sanitizer for control and format characters, and so does the fatal-error output. The `HOOKS` note, which asks you to copy hooks by hand, shows those characters and the two Unicode line separators as `\u` escapes, and caps each line at 300 characters. +- **The extractor refuses a tar entry name it cannot write faithfully.** Names are decoded once, as strict UTF-8, and the check, the path rules and the write all use that one string. A name that holds a control or format character, is not valid UTF-8, or starts with a byte-order mark is refused. A raw C1 byte (such as `0x9B`, the terminal CSI) used to land on disk as that control character, where `pharn status` and `pharn update` then printed it raw, and every non-ASCII name was written garbled. +- **A hostile archive can no longer exhaust memory or CPU, or be read leniently.** A 192 KB archive whose pax header inflated to about 128 MB took about 13 s and over 1 GB of memory to parse, and ran out of memory under a 512 MB heap, leaving the temporary download behind. Pax headers are now capped at 64 KiB each, and across all global headers in the archive together, and every header counts toward the entry limit. Framing is strict: a zero block mid-archive, a missing end marker, anything after it, a non-ustar header, and empty or `.` path segments are refused. - **A malformed numeric header field no longer reaches the error output raw.** It is shown with non-printable bytes escaped, so an archive cannot add a line of its own to the fatal message. ## [0.5.0] - 2026-09-10 diff --git a/CLAUDE.md b/CLAUDE.md index 9a35d46..ec10439 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,22 +48,22 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten **`commands/init.ts` is the archetype install flow** — the only init flow (the legacy module/wizard step pipeline `runInitLegacy`/`runInitV2` and its `steps/*` were removed). `runInit` calls `runInitArchetype` unconditionally: 1. `prereqs` (`runGitPrereq`) — hard-fails if `.git` is absent. -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`). +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`). The walk skips `SKIP_DIRS` (caches, VCS, build output, `__pycache__`, `.yarn`) at zero budget, and `ECOSYSTEM_DIRS` (`target`/`vendor`/`venv`/`.venv`) only where they are that ecosystem's tree — a sibling `Cargo.toml`/`pom.xml`/`build.sbt` or `go.mod`/`composer.json`/`Gemfile` in the parent's already-read entries, or a `pyvenv.cfg` inside — because those are ordinary folder names in a JS project too and the `backend` signal is structural (`app/**/route.ts`), with no `package.json` backstop. 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`). `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. +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; the fingerprint is taken FIRST and re-checked under the lock by `assertConfigFingerprintUnchanged`, which refuses, writing nothing, if another run wrote the file while the prompts were open) and carries it over by `update`'s own merge rules (`carryOver`): a `source: 'manual'` entry the index still has is installed again and recorded `manual` via `manualKeys`, even when the archetypes also select it; an entry of ANY source that upstream ships but this CLI cannot parse (`index.unknown`) is KEPT verbatim — config entry, files and records (`keptRecords`) untouched — and listed in `frozenCapabilities`; a manual entry upstream no longer ships is dropped and NAMED. Top-level config keys pharn does not own (`userOwnedConfigEntries` — upstream's `testResults` / `ship`) are copied across from any file that parses as a JSON object. 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, 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/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 `role`/`applies` via a strict field reader — the name is the directory's — inside a `---` fence that is upstream's own rule, ported literally from pharn-oss's validator (`.dev/floor/validate.mjs` → `parseFrontmatter`: the file starts with `---`, the block runs to the first newline followed by `---`, trimmed), and differential-tested against that function) + `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. **`lib/pharn-config.ts`** reads/writes `pharn.config.json` (`pharnVersion`, `skillsVersion`, `repo`, `commit`, `installedAt`, and — for an archetype install — `archetypes[]`, `capabilities[]` (`{name, role}`), `layout`, plus the `models`/`seam` blocks). Every `capabilities[]` entry is validated at ingest: it must be an object whose `name` matches `CAPABILITY_NAME_RE` (no control chars) and whose `role` is in `ROLE_VALUES`, else `CapabilityEntryError` — the config is committed and hand-editable, and `name` is path-joined by `remove`'s recursive delete (a `{"name":"../.."}` entry used to delete the project root). A present-but-invalid `capabilities[].source` (anything outside `{auto, manual}`; ABSENT is legal, P7) is rejected by `CapabilitySourceError`. Both join `isConfigValidationError`'s union. A present-but-invalid `models`/`seam` block is caught by its own validator — `lib/model-routing.ts` (`validateModelRouting`/`ModelRoutingError`) and `lib/seam-config.ts` (`validateSeamConfig`/`SeamConfigError`) — and `readPharnConfig` lets that named error PROPAGATE (never collapsing a bad hand-edit into the "run init" path); `isConfigValidationError` + `loadConfigOrExit` catch and report it, and the validated/stripped blocks replace the raw ones. Schema is additive — a legacy config's now-unused `modules[]`/`constitution`/`stackAnswers`/`installedSkills[]` still load (P7). `isArchetypeConfig` (= `Array.isArray(config.capabilities)`) is the deterministic discriminator; **`loadArchetypeConfigOrExit`** is the shared load-or-reject surface for `add`/`update`/`status`/`remove` (a pre-archetype config → `LEGACY_CONFIG_MESSAGE` + exit(1), never a fetch; `list` keeps its own json-aware check so `--json` stderr stays clean). `add`/`update`/`remove` update the config in place, and each calls `assertConfigUnchanged` as the FIRST step inside its `.pharn.lock` (`lib/project-lock.ts`): the config is loaded before the lock (and across `update`'s / the `remove` picker's confirm), so a concurrent run could otherwise be overwritten from a stale snapshot; a mismatch throws `ProjectChangedError` (a `ProjectLockedError`, so every catch site reports it as a named refusal, exit 1, nothing written). The lock itself treats an unparseable lock file younger than `MALFORMED_GRACE_MS` (10 s) as LIVE — `tryCreate` writes its payload after the `O_EXCL` create, so a live lock is briefly empty — and only breaks older ones. -**`pharn add` addressing** (`commands/add.ts` + `lib/capability-address.ts`). `add ` or `add :` (e.g. `add a11y`, `add lens:n-plus-one`) installs one capability into an archetype project — a manual override of archetype auto-selection. It clones pharn-oss (SHA-pinned), then applies **the version gate**: a local `versionGate` helper, called ONCE per command from INSIDE each path's existing `try` (so `readSkillsVersion`'s throw still reaches the `finally` that cleans up the clone), refuses when `readSkillsVersion(repo.dir) !== config.skillsVersion` — reusing the existing `{kind:'error'}` outcome → `exit(1)`. It fires on `!==` (never `<`, so a rollback reads the same) — except that a clone at the config's additive `pendingSkillsVersion` also passes: `update` records that field only when it withheld the bump SOLELY because of `modified`/`unrecorded` skips (the user's kept edits; every other file is at that version) and clears it on the next complete run; at a pending version `add` keeps the config's own `(skillsVersion, commit)` pair (`addStamp`) so the records stamp stays consistent, fires **gate-first** (before the already-installed no-op and before the picker's `all-installed`, and before `groupMultiselect` renders), and writes nothing. A sibling **layout gate** (`layoutGate`) sits immediately after it at both call sites, `??`-chained (`versionGate(…) ?? layoutGate(…)`) so the **version** refusal wins when both mismatch — by short-circuit evaluation, not statement order: it refuses when `detectLayout(repo.dir) !== configLayout(config)`, naming both resolved layouts and `pharn update --force`. `add` copies at the CLONE's layout (`installCapabilityDirs`' default) and records at it (`mergeCapabilityRecords`), while `remove`/`status`/`diff.ts` address the project at `configLayout` — so without the gate a mismatched add lands files nothing ever looks at, and the next `remove` drops the config entry reporting "its files were already gone", orphaning the dir. `add` must NEVER record the clone's layout the way `update` does: `update` may only because it rewrites the WHOLE tree at that layout, whereas `add` writes one capability, so stamping `layout` here would re-address every other already-placed file. Comparing `configLayout(config)` (not the raw `config.layout`) is the point — agreement with the readers is the invariant, and it makes a garbage hand-edited value resolve to `flat` and fail closed. This is what keeps `update`'s `config.skillsVersion === latest` early-return honest: `add` must never stamp a newer `skillsVersion` over unchanged old bytes. `add` therefore refreshes `commit` but NEVER `skillsVersion` (the legal same-version-different-commit case). Past the gate it resolves the arg against `parseCapabilityIndex`, and if it uniquely names a not-yet-installed capability, copies it via `installCapabilityDirs` and **appends** to `capabilities` with `source: 'manual'` (never touches `archetypes`). That tag is what makes the override survive `update`, and it is written at BOTH entry-construction sites — `resolveArchetypeAdd` and the picker's threaded `cfg` mirror, which the next pick spreads into its own config write. Already-installed → no-op; unknown/ambiguous → lists the valid `role:name` addresses. **Before the copy, `add` backs up destination drift**: `capabilityCloneFiles` (`lib/install-manifest.ts`) enumerates the capability dir in the CLONE, `collectDestDrift` (`lib/dest-drift.ts`) keeps the rels whose dest exists as a regular file and whose `sha256File` DIFFERS from the clone's, and a non-empty set goes to `createBackup` (`lib/backup.ts` — the same `.pharn-backup//` `update --force` writes) BEFORE the first `cpSync`, logged at creation so the pointer survives a later throw. `add` was the only write path with none of the product's three edit-protections (init's prompt, update's per-file skip, `--force`'s backup), and `update` itself manufactures the reachable sequence: a `dropped-unselected` capability's files are left on disk, the user edits them, and the later `add` is not a config no-op. Byte-identical is NOT drift (mirrors update's `identical → no-op`), so a drop-then-re-add of unedited files stays silent. The scan SKIPS an absent/symlinked/non-dir source rather than throwing, so `installCapabilityDirs`' curated `Capability "x" (griller) is missing at …` still wins — the same read-skips/write-throws split `install-manifest.ts`'s `addDir` already uses. `scanDest` returns a PARTITION, not just the drift set: a rel whose PROJECT-side path crosses a symlinked component is `unsafe`, and `add` **refuses the whole install** naming the component. That is measured, not defensive — on node v24.13.1 `cpSync` guards only the source, so a symlinked INTERMEDIATE dir under the capability dir is written straight THROUGH (bytes outside the project replaced), while a symlinked leaf is REPLACED and a symlinked capability root throws `ERR_FS_CP_DIR_TO_NON_DIR`. Skipping would be strictly worse than doing nothing (the copy writes through anyway while the backup omits the file), and backing up is impossible (`createBackup` refuses symlinked components; `copyFileSync` would save the link's TARGET). An ENOTDIR walk (a component below a regular file) is NOT unsafe — `cpSync` throws on that tree by itself — so it stays a skip, which is also what keeps a non-directory component out of the set `createBackup` consumes unwrapped. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one), keyed by that SAME clone-derived list — never a walk of the destination, which used to sweep a user's own file inside a leftover capability dir into the store as pharn-written and let a later `update` read it as cleanly upgradeable instead of `modified`; the hashes are still taken at the DEST (`buildRecords`), so a record can never disagree with what landed. `CONSTITUTION.md` is **not** touched — `add` installs capability dirs only. `pharn update` re-resolves the **recorded archetypes** against the latest index and re-copies — drift-safely, see below. +**`pharn add` addressing** (`commands/add.ts` + `lib/capability-address.ts`). `add ` or `add :` (e.g. `add a11y`, `add lens:n-plus-one`) installs one capability into an archetype project — a manual override of archetype auto-selection. It clones pharn-oss (SHA-pinned), then applies **the version gate**: a local `versionGate` helper, called ONCE per command from INSIDE each path's existing `try` (so `readSkillsVersion`'s throw still reaches the `finally` that cleans up the clone), refuses when `readSkillsVersion(repo.dir) !== config.skillsVersion` — reusing the existing `{kind:'error'}` outcome → `exit(1)`. It fires on `!==` (never `<`, so a rollback reads the same) — except that a clone at the config's additive `pendingSkillsVersion` also passes: `update` records that field only when it withheld the bump SOLELY because of `modified`/`unrecorded` skips (the user's kept edits; every other file is at that version) and clears it on the next complete run; at a pending version `add` keeps the config's own `(skillsVersion, commit)` pair (`addStamp`) so the records stamp stays consistent, fires **gate-first** (before the already-installed no-op and before the picker's `all-installed`, and before `groupMultiselect` renders), and writes nothing. A sibling **layout gate** (`layoutGate`) sits immediately after it at both call sites, `??`-chained (`versionGate(…) ?? layoutGate(…)`) so the **version** refusal wins when both mismatch — by short-circuit evaluation, not statement order: it refuses when `detectLayout(repo.dir) !== configLayout(config)`, naming both resolved layouts and `pharn update --force`. `add` copies at the CLONE's layout (`installCapabilityDirs`' default) and records at it (`mergeCapabilityRecords`), while `remove`/`status`/`diff.ts` address the project at `configLayout` — so without the gate a mismatched add lands files nothing ever looks at, and the next `remove` drops the config entry reporting "its files were already gone", orphaning the dir. `add` must NEVER record the clone's layout the way `update` does: `update` may only because it rewrites the WHOLE tree at that layout, whereas `add` writes one capability, so stamping `layout` here would re-address every other already-placed file. Comparing `configLayout(config)` (not the raw `config.layout`) is the point — agreement with the readers is the invariant, and it makes a garbage hand-edited value resolve to `flat` and fail closed. This is what keeps `update`'s `config.skillsVersion === latest` early-return honest: `add` must never stamp a newer `skillsVersion` over unchanged old bytes. `add` therefore refreshes `commit` but NEVER `skillsVersion` (the legal same-version-different-commit case). Past the gate it resolves the arg against `parseCapabilityIndex`, and if it uniquely names a not-yet-installed capability, copies it via `installCapabilityDirs` and **appends** to `capabilities` with `source: 'manual'` (never touches `archetypes`). That tag is what makes the override survive `update`, and it is written at BOTH entry-construction sites — `resolveArchetypeAdd` and the picker's threaded `cfg` mirror, which the next pick spreads into its own config write. Already-installed → no-op; unknown/ambiguous → lists the valid `role:name` addresses. **Before the copy, `add` backs up destination drift**: `capabilityCloneFiles` (`lib/install-manifest.ts`) enumerates the capability dir in the CLONE, `scanDest` (`lib/dest-drift.ts`) keeps the rels whose dest exists as a regular file and whose `sha256File` DIFFERS from the clone's, and a non-empty set goes to `createBackup` (`lib/backup.ts` — the same `.pharn-backup//` `update --force` writes) BEFORE the first `cpSync`, logged at creation so the pointer survives a later throw. `add` was the only write path with none of the product's three edit-protections (init's prompt, update's per-file skip, `--force`'s backup), and `update` itself manufactures the reachable sequence: a `dropped-unselected` capability's files are left on disk, the user edits them, and the later `add` is not a config no-op. Byte-identical is NOT drift (mirrors update's `identical → no-op`), so a drop-then-re-add of unedited files stays silent. The scan SKIPS an absent/symlinked/non-dir source rather than throwing, so `installCapabilityDirs`' curated `Capability "x" (griller) is missing at …` still wins — the same read-skips/write-throws split `install-manifest.ts`'s `addDir` already uses. `scanDest` returns a PARTITION, not just the drift set: a rel whose PROJECT-side path crosses a symlinked component is `unsafe`, and `add` **refuses the whole install** naming the component. That is measured, not defensive — on node v24.13.1 `cpSync` guards only the source, so a symlinked INTERMEDIATE dir under the capability dir is written straight THROUGH (bytes outside the project replaced), while a symlinked leaf is REPLACED and a symlinked capability root throws `ERR_FS_CP_DIR_TO_NON_DIR`. Skipping would be strictly worse than doing nothing (the copy writes through anyway while the backup omits the file), and backing up is impossible (`createBackup` refuses symlinked components; `copyFileSync` would save the link's TARGET). An ENOTDIR walk (a component below a regular file) is NOT unsafe — `cpSync` throws on that tree by itself — so it stays a skip, which is also what keeps a non-directory component out of the set `createBackup` consumes unwrapped. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one), keyed by that SAME clone-derived list — never a walk of the destination, which used to sweep a user's own file inside a leftover capability dir into the store as pharn-written and let a later `update` read it as cleanly upgradeable instead of `modified`; the hashes are still taken at the DEST (`buildRecords`), so a record can never disagree with what landed. `CONSTITUTION.md` is **not** touched — `add` installs capability dirs only. `pharn update` re-resolves the **recorded archetypes** against the latest index and re-copies — drift-safely, see below. -**`pharn remove` addressing** (`commands/remove.ts`) is the inverse of `add`. `remove ` / `remove :` (no arg → an interactive picker over the installed capabilities) deletes that one isolated capability dir — addressed at the project's recorded `layout` (flat `pharn-review` / `pharn-pipeline/grillers/`, OR the same under `pharn/`, via `configLayout` + `layoutPaths`) — and drops its `capabilities` entry. **No clone, no network** — everything is derivable from `config.capabilities` + the filesystem, so `remove.ts` imports no repo module at all; `archetypes` is never touched; `CONSTITUTION.md`/`memory-bank/` are **never** touched (they are not capability dirs). Removing an entry whose stored `source` is **literally** `'auto'` warns that the next `update` will reinstall it; an **absent** `source` warns NOTHING (absence means provenance-unknown, and a false warning on a legacy manual add is worse than silence) — derived from the stored field only, so `remove` stays zero-network. Not-installed → benign no-op listing the removable capabilities; a name installed in both roles → hard-fail (ambiguous). `remove` has no `--yes`, and the reason is per-path: the NAMED remove deletes without asking (no prompt to skip), while the bare picker's ONE confirm — listing the picks, default No — IS the destructive gate, so nothing may skip that either. **`remove`'s `ALLOWED_FLAGS` row is empty, so `pharn remove --yes` now exits 1** naming the flag. This SUPERSEDES the previous contract, which read: _"`--yes` stays a declared minimist boolean only because it is `update`'s flag; that is what keeps `pharn remove --yes` a harmless parse rather than an unknown-option refusal, and turning it into a refusal belongs to a per-command allowlist, not to the dispatch."_ That allowlist now exists (see the dispatch paragraph above), so the deferral is spent — but its reasoning still holds and is why the row is empty rather than `['yes']`: there is no confirm on the named path to skip, and the picker's one confirm may not be skipped. `--yes` is still declared globally, because the declaration list is DERIVED from the table and `update`'s row grants it. Before any delete (the picker: for the whole selection, before its confirm) `remove` refuses with exit 1 when a target dir's path crosses a symlinked component (`findSymlinkComponent`), since `rmSync` resolves ancestors. Every delete is a **strict child** of its role subtree — `safeChildJoin(safeJoin(cwd, subtree), name)` (`lib/validate.ts`), which, unlike `safeJoin`, never returns the base itself nor a deeper path — a second floor under the ingest-time name validation. `remove` also **prunes that capability's entries from `pharn.records.json`** (`pruneCapabilityRecords`), so it is no longer the one write path leaving records that describe bytes that are gone — the converse of the invariant `update` is pinned to. The prune is a **key-prefix filter over the store**, never an fs enumeration: a walk of the DEST dir finds nothing after the delete AND nothing before it on the "already gone" path — which is exactly where the stale keys are the only thing left to clean, so the store is the only place the answer still exists. It mirrors `add`'s `mergeCapabilityRecords` guard verbatim (`recordsBaseline(...) === null → return`): absent/corrupt/stale-stamped → the file is left byte-identical, **never** minted and never blessed (the baseline `note` is deliberately not surfaced — `update` is where an unusable store changes an outcome, and it is the command that names the reason). The trailing slash in `relDir + '/'` is load-bearing (`lenses/a11y` must not eat `lenses/a11y-extended`); nothing matched → the write is skipped entirely; and the `skillsVersion`/`commit` stamp is re-written from the CONFIG pair unchanged, since `remove` advances neither. Order is delete → prune → config (mirroring `add`'s records-before-config) — a benignity argument, not atomicity: each write is individually atomic (`lib/atomic-write.ts` — temp-then-`rename`), but the PAIR is not a transaction, so a failed prune leaves the old status quo and a failed config write leaves an entry the next `update` restores. The role→dir ternary is single-sourced in `capabilityRelDir`, shared with `deleteCapabilityDir`, so the delete and the prune address the same directory by construction. Both call sites prune — the named path with `[target]`, the picker ONCE with the full selection (one store write matching the one config write). +**`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→subtree mapping is single-sourced in `capabilitySubtree` (`lib/layout.ts`), which every reader and writer uses; `remove`'s `capabilityRelDir` builds on it and is 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`). +**`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`). A capability it KEPT (upstream ships it, this CLI cannot parse it) is recorded in the additive `frozenCapabilities` (`role:name`, sorted, omitted when empty), and while that list is non-empty the same-version early return is skipped, so every run re-fetches and re-checks it; an entry leaves the list only once none of its files was skipped. When a run withholds the `skillsVersion` bump SOLELY for `modified`/`unrecorded` skips it records `pendingSkillsVersion` (see `add` above). With `--force`, the backup notice prints the moment the backup exists, with its spinner stopped first (a line logged under a running clack spinner is glued to its frame and, in a narrow terminal, erased by its stop); a later failure repeats only the part-way warning and the pointer, on stderr. **`pharn status` (`commands/status.ts`) is strictly read-only** — it never writes, deletes, or overwrites (fixing is `update`/`add`). Two sections: a **version** check (installed `skillsVersion` vs the upstream `SKILLS_VERSION` file, plus an archetype + capability-count summary) and a **drift** check. Default clones `@main` once and reuses it for both; `--no-drift` skips the clone and uses `fetchRemoteSkillsVersion` for the version section only; `--strict` exits 1 on any outdated/modified/missing (CI gate, default exit 0). Cleanup runs in a `finally`, and every `process.exit` happens _after_ it. The pure (no I/O) engine is **`lib/diff.ts` → `diffInstalledCapabilities`**: it mirrors `installCapabilities` to derive the **expected** file set — the selected capability dirs + the fixed product surfaces, at the recorded `layout` — then `sha256`-compares each against the project root, returning `{modified, missing, okCount}`. Every read is `safeJoin`-guarded (from `lib/validate.ts`). `.claude/settings.json` is user-owned (preserved at install) and excluded from the file diff — but its `hooks` block is compared by `lib/hook-wiring.ts` (`diffHookWiring`: exact-string set difference over `Event · matcher · command · args`, against the UNION of the project's `.claude/settings.json` and `.claude/settings.local.json` — what Claude Code merges; user-level settings are never read. Every read is size-capped and opened `O_NONBLOCK` (a FIFO is refused, not waited on); upstream's file is symlink-refused at every component, while a project file may be a symlinked FILE (read through, as Claude Code does) but never sit under a symlinked `.claude/`. Only upstream entries are displayed, each as the hook's own JSON — exec vs shell form visible, paste-ready — with every `terminalSafe`-class character and U+2028/2029 turned into a `\u` JSON escape and each line capped at 300 chars); upstream hooks wired in neither file print a `HOOKS` note in `status` (and fail `--strict`, via `hookWiringFails`, as does an unreadable project file) and in `update` (report only — `update` still never writes the file); hooks wired only in `settings.local.json` are named as `local-only` and do NOT fail `--strict`; the copied-verbatim trusted docs (including `CONSTITUTION.md` / `pharn/CONSTITUTION.md`), hooks, contracts, and floor checkers ARE compared. Drift is derived live from the clone, always against `@main`, never the pinned `commit`. `status` does NOT read `pharn.records.json`, so it is a report, not a preview: a file it lists as differing may be cleanly upgraded OR skipped — only `update` can tell those apart. diff --git a/docs/commands/init.md b/docs/commands/init.md index 630e6cb..791b4f5 100644 --- a/docs/commands/init.md +++ b/docs/commands/init.md @@ -104,7 +104,7 @@ Shows the PHARN logo and CLI version. ### 3. Detect archetypes -Reads `package.json` dependency names and walks the project tree (bounded and symlink-safe, skipping dependencies, VCS metadata, and build/deploy caches — `node_modules`, `.git`, `dist`, `build`, `out`, `coverage`, `storybook-static`, `.next`, `.nuxt`, `.svelte-kit`, `.astro`, `.turbo`, `.vercel`, `.cache`, `.parcel-cache`, and non-JS dependency/build trees — `.venv`, `venv`, `__pycache__`, `vendor`, `target`, `.yarn`) for structural signals, then reduces both to an `Archetype[]`. Skipping those trees costs the walk nothing, so a large framework cache cannot exhaust its bound and hide your real source; the tradeoff is that a signal file you hand-authored inside one of those directories is not seen. The detected set is shown in a "Detected archetypes" note. Only names are tested against fixed in-code allowlists — no discovered file body is read (other than `package.json`, which must be a regular file of at most 4 MiB — a leading UTF-8 BOM is ignored; anything else counts as no `package.json`) and no untrusted value is executed, interpolated, or logged. +Reads `package.json` dependency names and walks the project tree (bounded and symlink-safe, skipping dependencies, VCS metadata, and build/deploy caches — `node_modules`, `.git`, `dist`, `build`, `out`, `coverage`, `storybook-static`, `.next`, `.nuxt`, `.svelte-kit`, `.astro`, `.turbo`, `.vercel`, `.cache`, `.parcel-cache`, and non-JS trees — `__pycache__`, `.yarn`) for structural signals, then reduces both to an `Archetype[]`. Three more names are skipped only where they are another ecosystem's tree, because they are ordinary folder names in a JS project too: `target/` beside a `Cargo.toml`, `pom.xml` or `build.sbt`; `vendor/` beside a `go.mod`, `composer.json` or `Gemfile`; `venv/` or `.venv/` holding a `pyvenv.cfg`. Anywhere else they are scanned like any folder, so a route at `app/target/route.ts` is still detected. Skipping those trees costs the walk nothing, so a large framework cache cannot exhaust its bound and hide your real source; the tradeoff is that a signal file you hand-authored inside one of those directories is not seen. The detected set is shown in a "Detected archetypes" note. Only names are tested against fixed in-code allowlists — no discovered file body is read (other than `package.json`, which must be a regular file of at most 4 MiB — a leading UTF-8 BOM is ignored; anything else counts as no `package.json`) and no untrusted value is executed, interpolated, or logged. ### 4. Fetch PHARN diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index ca3a289..0bf8efd 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -138,7 +138,7 @@ installs the **universal** capabilities — the ones that apply to any codebase. ### Monorepos / workspaces -`pharn init` checks the **current directory** for a `.git` directory, reads the `package.json` there and runs a bounded, symlink-safe file-tree scan from it for archetype detection, then installs into that directory. The scan skips heavy or generated trees (`node_modules`, `dist`, `build`, `.next`, `out`, `coverage`, framework caches, and non-JS trees such as `.venv`, `vendor` and `target`), so in a workspace it still sees `apps/` and `packages/`. It does not walk up to a workspace root or into workspace packages. In a monorepo, run it from the directory that contains both `.git` and the app's `package.json`. Split layouts (`.git` at the root, the app's `package.json` in `apps/web/`) are unsupported in v1. +`pharn init` checks the **current directory** for a `.git` directory, reads the `package.json` there and runs a bounded, symlink-safe file-tree scan from it for archetype detection, then installs into that directory. The scan skips heavy or generated trees (`node_modules`, `dist`, `build`, `.next`, `out`, `coverage`, framework caches, and non-JS trees such as a Python virtualenv, or `vendor`/`target` beside the build file that owns them — a `target/` or `vendor/` folder of your own is scanned), so in a workspace it still sees `apps/` and `packages/`. It does not walk up to a workspace root or into workspace packages. In a monorepo, run it from the directory that contains both `.git` and the app's `package.json`. Split layouts (`.git` at the root, the app's `package.json` in `apps/web/`) are unsupported in v1. ## Overwrite warnings diff --git a/src/commands/remove.ts b/src/commands/remove.ts index 4ee3868..1632bc7 100644 --- a/src/commands/remove.ts +++ b/src/commands/remove.ts @@ -9,7 +9,12 @@ import { } from '@clack/prompts'; import pc from 'picocolors'; import { cancelAndExit } from '../lib/confirm.js'; -import { configLayout, layoutPaths, type LayoutPaths } from '../lib/layout.js'; +import { + capabilitySubtree, + configLayout, + layoutPaths, + type LayoutPaths, +} from '../lib/layout.js'; import { parseCapabilityArg } from '../lib/capability-address.js'; import { logError } from '../lib/report-error.js'; import { @@ -94,7 +99,7 @@ function deleteCapabilityDir( target: InstalledCapability, ): boolean { const dir = safeChildJoin( - safeJoin(cwd, capabilitySubtree(paths, target)), + safeJoin(cwd, capabilitySubtree(paths, target.role)), target.name, ); const existed = existsSync(dir); @@ -132,22 +137,16 @@ function refuseSymlinkedTargets( } } -// One source for the role→dir mapping. The delete addresses the filesystem and +// One source for the capability dir. 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 +// sharing this (and, through it, layout.ts's capabilitySubtree — the one role → +// subtree mapping) makes that true by construction rather than by two copies // staying in sync. function capabilityRelDir( paths: LayoutPaths, capability: InstalledCapability, ): string { - return `${capabilitySubtree(paths, capability)}/${capability.name}`; -} - -function capabilitySubtree( - paths: LayoutPaths, - capability: InstalledCapability, -): string { - return capability.role === 'griller' ? paths.grillers : paths.lenses; + return `${capabilitySubtree(paths, capability.role)}/${capability.name}`; } // Drop the removed capabilities' entries from `pharn.records.json`, so `remove` diff --git a/src/commands/update.ts b/src/commands/update.ts index fb8c4e7..7d8c620 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -37,6 +37,7 @@ import { applyWrites, ApplyError, readDiskState } from '../lib/apply-update.js'; import { createBackup, BACKUP_DIR } from '../lib/backup.js'; import { sha256File } from '../lib/hash.js'; import { + capabilitySubtree, configLayout, detectLayout, layoutPaths, @@ -340,10 +341,21 @@ async function runArchetypeUpdate( // Named the moment it exists (as `add` does): everything after the // backup can throw or be interrupted, and this pointer is the // user's only route back to the pre-overwrite bytes. + // + // With NO spinner running: clack's spinner owns the line it + // animates, so a line logged under it is glued to its frame, and + // below ~50 columns its `stop` erases that line. So the spinner is + // stopped first — with a neutral phrase, the notice says the rest — + // and a new one is started for the writes (`spinnerRef` follows it, + // so every failure path stops the one actually running). + s2.stop('Backup written'); printBackupNotice(backup, { aborted: false }); + const s3 = spinner(); + spinnerRef.current = s3; + s3.start('Writing files'); }, ); - s2.stop( + spinnerRef.current.stop( applied.plan.writes.length ? 'Capabilities updated' : 'Nothing to write', @@ -598,7 +610,7 @@ async function applyUpdate( const paths = layoutPaths(layout); const hasSkippedFile = (cap: InstalledCapability): boolean => { // The same `//` prefix recordsUnderCapabilities keys on. - const subtree = cap.role === 'griller' ? paths.grillers : paths.lenses; + const subtree = capabilitySubtree(paths, cap.role); const prefix = `${subtree}/${cap.name}/`; return skippedRels.some((rel) => rel.startsWith(prefix)); }; @@ -797,10 +809,14 @@ function printBackupNotice(backup: Backup, opts: { aborted: boolean }): void { `Backed up ${backup.count} file(s) to ${backup.dir} before overwriting.`, { output }, ); - log.info( - `${BACKUP_DIR}/ is not gitignored — add it to .gitignore or delete it once you are happy.`, - { output }, - ); + // Advice, not a pointer: said once, when the backup is made. On an abort the + // pointer above is repeated on stderr (what `2> err.log` keeps); this is not. + if (!opts.aborted) { + log.info( + `${BACKUP_DIR}/ is not gitignored — add it to .gitignore or delete it once you are happy.`, + { output }, + ); + } } // The order change groups are reported in — additions first, then departures, diff --git a/src/lib/capability-index.ts b/src/lib/capability-index.ts index 0981076..d40fe64 100644 --- a/src/lib/capability-index.ts +++ b/src/lib/capability-index.ts @@ -224,20 +224,28 @@ function readCapabilityMarkdown( * never read. Hard-fails (naming the capability) when no frontmatter fence is * present (P5). * - * Line-based, not a lazy regex: the block opens on a first line that is exactly - * `---` and ends at the NEXT line that is `---` (trailing blanks allowed). A - * regex capture (`^---\n([\s\S]*?)\n---`) skips an empty block (`---\n---`) - * and reads the document BODY as frontmatter, up to the next `---`. + * The fence is UPSTREAM'S rule, ported literally from pharn-oss's own validator + * (this repo's copy: .dev/floor/validate.mjs → parseFrontmatter), so a + * capability upstream's CI passes is never skipped here for its fence (P4): + * - the file must START with `---`; + * - the block is everything from the fourth character up to the first newline + * followed by `---` — so it closes at the next line that STARTS with `---` + * (`---`, `----`, `--- note`), and the rest of the opening line belongs to + * the block; + * - the block is trimmed, as upstream trims it. + * An empty block (`---` then `---`) is '' — every field is missing, and both + * parsers refuse it rather than reading the BODY as frontmatter (PHARN-14). + * tests/capability-index.test.ts runs every fence shape through the validator's + * own function and requires the same verdict. */ function extractFrontmatter(content: string, name: string): string { - const lines = content.split('\n'); - const close = lines.findIndex((line, i) => i > 0 && /^---[ \t]*$/.test(line)); - if (lines[0] !== '---' || close === -1) { + const end = content.startsWith('---') ? content.indexOf('\n---', 3) : -1; + if (end === -1) { throw new ManifestValidationError( `Capability "${name}" is missing a "---"-fenced frontmatter block.`, ); } - return lines.slice(1, close).join('\n'); + return content.slice(3, end).trim(); } /** diff --git a/src/lib/detect-archetype.ts b/src/lib/detect-archetype.ts index e7ae491..e052295 100644 --- a/src/lib/detect-archetype.ts +++ b/src/lib/detect-archetype.ts @@ -2,6 +2,7 @@ import { closeSync, constants, fstatSync, + lstatSync, openSync, readSync, readdirSync, @@ -90,18 +91,67 @@ export const SKIP_DIRS: ReadonlySet = new Set([ '.cache', // generic tool cache (Parcel, Gatsby, …) '.parcel-cache', // Parcel cache 'storybook-static', // Storybook static build - // Non-JS dependency/build trees that share a repo with a JS app. Same LOST- - // signal tradeoff as above: hand-authored source under e.g. `vendor/` goes - // dark (package.json still backstops it); a 50k-file `.venv` no longer - // exhausts MAX_ENTRIES before the walk reaches `src/`. - '.venv', // Python virtualenv - 'venv', // Python virtualenv + // Non-JS trees that are never hand-authored JS source, whatever sits beside + // them. The ones that ARE ordinary folder names elsewhere (`target`, `vendor`, + // `venv`) are in ECOSYSTEM_DIRS below instead. '__pycache__', // Python bytecode cache - 'vendor', // Go / PHP (Composer) / Ruby vendored dependencies - 'target', // Rust / Maven build output '.yarn', // Yarn Berry cache / PnP store ]); +/** How to tell that a directory is its ecosystem's tree. */ +export type EcosystemMarker = + // A regular file with one of these exact names beside the directory. + | { siblings: readonly string[] } + // A regular file with this exact name inside the directory. + | { inside: string }; + +// Non-JS dependency/build trees that share a repo with a JS app — skipped, at +// zero budget, like SKIP_DIRS, but ONLY where they are that ecosystem's tree. +// Their names are ordinary folder names in a JS project too, and the backend +// signal is STRUCTURAL (`app/**/route.ts`), so no package.json dependency +// backstops it: skipping every `target/` sent a Next.js route at +// app/target/route.ts dark, and `[ssr, backend]` detected as `[ssr]`. +// +// The test is a membership check over the parent's already-read entries (the +// sibling build file), or one `lstat` (a virtualenv's pyvenv.cfg), so a real +// 50k-file tree still costs one check and zero budget — the protection PHARN-15 +// added, kept at any depth (a nested Maven module, a PHP `vendor/`). The dir +// name matches case-insensitively, as SKIP_DIRS does; each marker matches +// exactly as its tool writes it, and only as a regular file. +// +// Exported read-only for the same reason as SKIP_DIRS: the tests pin every +// member's behavior and its classification neutrality against this map. +export const ECOSYSTEM_DIRS: ReadonlyMap = new Map< + string, + EcosystemMarker +>([ + ['target', { siblings: ['Cargo.toml', 'pom.xml', 'build.sbt'] }], // Rust / Maven / sbt output + ['vendor', { siblings: ['go.mod', 'composer.json', 'Gemfile'] }], // Go / PHP / Ruby deps + ['venv', { inside: 'pyvenv.cfg' }], // Python virtualenv + ['.venv', { inside: 'pyvenv.cfg' }], // Python virtualenv +]); + +/** + * Is `name` (a directory inside `dir`) its ecosystem's tree? `siblingFiles` is + * the set of regular-file names in `dir`, from the entries already read. + */ +function isEcosystemTree( + dir: string, + name: string, + siblingFiles: ReadonlySet, +): boolean { + const marker = ECOSYSTEM_DIRS.get(name.toLowerCase()); + if (marker === undefined) return false; + if ('siblings' in marker) { + return marker.siblings.some((file) => siblingFiles.has(file)); + } + return ( + lstatSync(join(dir, name, marker.inside), { + throwIfNoEntry: false, + })?.isFile() === true + ); +} + // Bounded walk. These caps are a DEFENSIVE bound on a pathological tree, NOT a // perf-only knob: a signal that lies past a cap is silently UNDETECTED (a // completeness tradeoff — determinism is preserved, but a real signal could be @@ -148,6 +198,9 @@ export function scanFileTreeSignals(root: string): ArchetypeSignals { const entries = readEntries(dir).sort((a, b) => a.name < b.name ? -1 : a.name > b.name ? 1 : 0, ); + const siblingFiles = new Set( + entries.filter((e) => e.isFile()).map((e) => e.name), + ); for (const entry of entries) { if (budget <= 0 || allFound()) return; const name = entry.name; @@ -155,6 +208,7 @@ export function scanFileTreeSignals(root: string): ArchetypeSignals { if (entry.isSymbolicLink()) continue; const isDir = entry.isDirectory(); if (isDir && SKIP_DIRS.has(name.toLowerCase())) continue; + if (isDir && isEcosystemTree(dir, name, siblingFiles)) continue; if (!isDir && name.toLowerCase().startsWith('.env')) continue; if (!isDir && !entry.isFile()) continue; // sockets/fifos/etc.: not signals budget -= 1; diff --git a/src/lib/install-capabilities.ts b/src/lib/install-capabilities.ts index c9cc837..6f91250 100644 --- a/src/lib/install-capabilities.ts +++ b/src/lib/install-capabilities.ts @@ -18,6 +18,7 @@ import { PRODUCT_COMMAND_PREFIX, } from './constants.js'; import { + capabilitySubtree, detectLayout, layoutPaths, resolveFeaturesReadme, @@ -126,7 +127,7 @@ export function installCapabilityDirs( const planned = capabilities.map((cap) => { assertSafeString(cap.name, `capability "${cap.name}"`, CAPABILITY_NAME_RE); assertNoDotDot(cap.name, `capability "${cap.name}"`); - const subtree = cap.role === 'griller' ? paths.grillers : paths.lenses; + const subtree = capabilitySubtree(paths, cap.role); const from = safeJoin(repoDir, `${subtree}/${cap.name}`); if (!existsSync(from)) { throw new ManifestValidationError( diff --git a/src/lib/install-manifest.ts b/src/lib/install-manifest.ts index 4d48d3f..82f4d63 100644 --- a/src/lib/install-manifest.ts +++ b/src/lib/install-manifest.ts @@ -8,6 +8,7 @@ import { PRODUCT_COMMAND_PREFIX, } from './constants.js'; import { + capabilitySubtree, layoutPaths, resolveFeaturesReadme, type LayoutPaths, @@ -139,7 +140,7 @@ export function collectExpectedInstallPaths(params: { // Selected capabilities (whole dir, incl. evals) at the layout's subtree. for (const cap of capabilities) { - const subtree = cap.role === 'griller' ? paths.grillers : paths.lenses; + const subtree = capabilitySubtree(paths, cap.role); addDir(`${subtree}/${cap.name}`); } // Product commands: top-level non-dev pharn-*.md. @@ -282,7 +283,7 @@ export function capabilityCloneFiles( paths: LayoutPaths, capability: InstalledCapability, ): string[] { - const subtree = capability.role === 'griller' ? paths.grillers : paths.lenses; + const subtree = capabilitySubtree(paths, capability.role); const relDir = `${subtree}/${capability.name}`; if (findSymlinkComponent(repoDir, relDir) !== null) return []; const from = safeJoin(repoDir, relDir); diff --git a/src/lib/install-records.ts b/src/lib/install-records.ts index aa13a65..2f434ce 100644 --- a/src/lib/install-records.ts +++ b/src/lib/install-records.ts @@ -3,7 +3,7 @@ import { writeJsonAtomic } from './atomic-write.js'; import { resolve } from 'node:path'; import { sha256File } from './hash.js'; import { isPlainObject, safeJoin, toPosix } from './validate.js'; -import type { LayoutPaths } from './layout.js'; +import { capabilitySubtree, type LayoutPaths } from './layout.js'; import type { InstalledCapability } from '../types.js'; // --------------------------------------------------------------------------- @@ -366,7 +366,7 @@ export function recordsUnderCapabilities( capabilities: readonly InstalledCapability[], ): FileRecords { const prefixes = capabilities.map((cap) => { - const subtree = cap.role === 'griller' ? paths.grillers : paths.lenses; + const subtree = capabilitySubtree(paths, cap.role); return `${subtree}/${cap.name}/`; }); if (prefixes.length === 0) return {}; diff --git a/src/lib/layout.ts b/src/lib/layout.ts index dc5d90e..891c8fd 100644 --- a/src/lib/layout.ts +++ b/src/lib/layout.ts @@ -19,7 +19,7 @@ import { TRUSTED_DOCS, UPSTREAM_LICENSE, } from './constants.js'; -import type { Layout, PharnConfig } from '../types.js'; +import type { InstalledCapability, Layout, PharnConfig } from '../types.js'; // --------------------------------------------------------------------------- // Layout resolver — the ONE place that knows the two install layouts pharn-oss @@ -111,6 +111,18 @@ export function layoutPaths(layout: Layout): LayoutPaths { }; } +/** + * The subtree holding a capability of `role` at this layout — the ONE role → + * directory mapping (grillers or lenses). Every reader and writer that + * addresses a capability dir goes through it, so no two can disagree. + */ +export function capabilitySubtree( + paths: LayoutPaths, + role: InstalledCapability['role'], +): string { + return role === 'griller' ? paths.grillers : paths.lenses; +} + // The layout an installed project was recorded with. Enum-safe membership (P5): // exactly `'pharn'` → pharn; anything else (including a legacy config that omits // the field, or a hand-edited garbage value) → `flat`, the safe legacy default. diff --git a/src/lib/symlink-guard.ts b/src/lib/symlink-guard.ts index b3a59f3..9ee7d97 100644 --- a/src/lib/symlink-guard.ts +++ b/src/lib/symlink-guard.ts @@ -42,14 +42,14 @@ import { safeJoin, toPosix } from './validate.js'; * false` suppresses ENOENT ONLY, so a component below a REGULAR FILE raises * ENOTDIR straight out of the walk (as does `safeJoin`'s escape refusal). Where * each caller stands differs, and is stated rather than assumed: `readDiskState` - * wraps the call and turns it into its `unreadable` terminal; `collectDestDrift` + * wraps the call and turns it into its `unreadable` terminal; `scanDest` * wraps it too and skips the rel, which is what keeps such a path out of the set * it hands `createBackup`; `applyWrites` is already inside a try, so it becomes an * ApplyError carrying what was written; `backup.ts` is NOT wrapped, and would * surface it as a fatal error — reachable only if a path with a non-directory * component ever reached the backup set, which `readDiskState` (for `update`) and - * `collectDestDrift` (for `add`) each classify and skip first. Pinned by - * tests/symlink-guard.test.ts. + * `scanDest` (for `add` and a re-run `init`) each classify and skip first. + * Pinned by tests/symlink-guard.test.ts. */ export function findSymlinkComponent(base: string, rel: string): string | null { let current = ''; diff --git a/tests/capability-index.test.ts b/tests/capability-index.test.ts index 02bbb65..7e68120 100644 --- a/tests/capability-index.test.ts +++ b/tests/capability-index.test.ts @@ -1,5 +1,5 @@ import { execFileSync } from 'node:child_process'; -import { mkdirSync, symlinkSync, writeFileSync } from 'node:fs'; +import { mkdirSync, readFileSync, symlinkSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { useTmpDir } from './helpers.js'; @@ -427,3 +427,109 @@ describe('parseCapabilityIndex', () => { expect(index.capabilities.map((c) => c.name)).toEqual(['security']); }); }); + +// F23: the fence is upstream's own rule. pharn-oss's validator (this repo's copy: +// .dev/floor/validate.mjs → parseFrontmatter) opens on a file that STARTS with +// `---` and closes at the first newline followed by `---` — any line that +// starts with it (`----`, `--- note`). A capability upstream's CI passes must +// never be skipped here for its fence. +describe("parseCapabilityIndex — the frontmatter fence is upstream's", () => { + const tmp = useTmpDir(); + const FIELDS = 'role: griller\napplies: ["universal"]'; + + // Installs `body` as the one griller and reports whether it was ACCEPTED. + function accepted(body: string): boolean { + const repo = tmp.path(); + scaffold(repo); + writeCap(repo, GRILLERS, 'probe', body); + const index = parseCapabilityIndex(repo); + return index.capabilities.some((c) => c.name === 'probe'); + } + + it('accepts a closing ---- line', () => { + expect(accepted(`---\n${FIELDS}\n----\n# x\n`)).toBe(true); + }); + + it('accepts a closing "--- note" line', () => { + expect(accepted(`---\n${FIELDS}\n--- note\n# x\n`)).toBe(true); + }); + + it('closes at the FIRST such line — a later horizontal rule in the body is prose', () => { + expect( + accepted(`---\n${FIELDS}\n---\n# x\n\n---\n\nrole: lens\napplies: []\n`), + ).toBe(true); + }); + + it('still refuses an empty block whose fields sit only in the body', () => { + expect(accepted(`---\n---\n${FIELDS}\n---\n# x\n`)).toBe(false); + }); + + // The upstream parser is read out of this repo's copy of the validator, so + // there is no second copy to drift: the day its rule changes, this test runs + // the new one. The file runs its checks on import (it exits the process), so + // the one function is extracted by name and built on its own. + function upstreamParseFrontmatter(): (text: string) => { + fm: Record | null; + } { + const src = readFileSync( + join(import.meta.dirname, '..', '.dev', 'floor', 'validate.mjs'), + 'utf8', + ); + const found = src.match( + /^function parseFrontmatter\(text\) \{\n[\s\S]*?\n\}\n/gm, + ); + expect(found).toHaveLength(1); + return new Function(`${found![0]}; return parseFrontmatter;`)() as ( + text: string, + ) => { fm: Record | null }; + } + + // Upstream's verdict for the two fields this CLI reads: a block exists and + // carries a role and a non-empty applies. + function upstreamAccepts(body: string): boolean { + const { fm } = upstreamParseFrontmatter()(body); + return ( + fm !== null && + typeof fm.role === 'string' && + fm.role !== '' && + Array.isArray(fm.applies) && + fm.applies.length > 0 + ); + } + + const BOM = String.fromCharCode(0xfeff); + const SHAPES: [string, string][] = [ + ['the plain fence', `---\n${FIELDS}\n---\n# x\n`], + ['a closing fence with trailing blanks', `---\n${FIELDS}\n--- \t\n# x\n`], + ['a closing ----', `---\n${FIELDS}\n----\n# x\n`], + ['a closing "--- note"', `---\n${FIELDS}\n--- note\n# x\n`], + ['an opening ----', `----\n${FIELDS}\n---\n# x\n`], + ['an opening "--- note"', `--- note\n${FIELDS}\n---\n# x\n`], + [ + 'a field on the opening line', + `--- role: griller\napplies: ["universal"]\n---\n`, + ], + ['a body rule after the close', `---\n${FIELDS}\n---\n# x\n\n---\n`], + ['an empty block', `---\n---\n${FIELDS}\n---\n`], + ['an unterminated block', `---\n${FIELDS}\n# x\n`], + ['no fence at all', `# x\n${FIELDS}\n`], + ['a blank line before the fence', `\n---\n${FIELDS}\n---\n`], + ['a byte-order mark before the fence', `${BOM}---\n${FIELDS}\n---\n`], + ['a fence that is not at column 0', ` ---\n${FIELDS}\n---\n`], + ]; + + it.each(SHAPES)("gives upstream's verdict for %s", (_label, body) => { + expect(accepted(body)).toBe(upstreamAccepts(body)); + }); + + // The one named difference, and its direction. Upstream reads each line with + // `(.*)$` and no multiline flag, so a line ending in CR yields no field and + // its validator refuses the file — upstream never ships one. This CLI reads + // fields with a multiline pattern, where `$` also matches before a CR, so it + // reads them. More lenient, never less: nothing upstream ships is refused. + it('is more lenient than upstream on CRLF, never less', () => { + const crlf = `---\r\nrole: griller\r\napplies: ["universal"]\r\n---\r\n# x\r\n`; + expect(upstreamAccepts(crlf)).toBe(false); + expect(accepted(crlf)).toBe(true); + }); +}); diff --git a/tests/detect-archetype.test.ts b/tests/detect-archetype.test.ts index a2b3fcd..af173ad 100644 --- a/tests/detect-archetype.test.ts +++ b/tests/detect-archetype.test.ts @@ -3,6 +3,7 @@ import { mkdirSync, symlinkSync, writeFileSync } from 'node:fs'; import { dirname, join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { + ECOSYSTEM_DIRS, MAX_PACKAGE_JSON_BYTES, SKIP_DIRS, detectArchetypesFromProject, @@ -191,13 +192,95 @@ describe('detectArchetypesFromProject — package.json read hardening', () => { describe('SKIP_DIRS — non-JS dependency/build trees', () => { const tmp = useTmpDir(); - it.each(['.venv', 'venv', '__pycache__', 'vendor', 'target', '.yarn'])( - '%s/ is skipped', + it.each(['__pycache__', '.yarn'])('%s/ is skipped', (dir) => { + touch(tmp.path(), `${dir}/x/Page.tsx`); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(false); + }); +}); + +// `target`, `vendor`, `venv` and `.venv` are ORDINARY folder names in a JS +// project too — a Next.js route at app/target/route.ts is real source — so each +// is skipped only where it is that ecosystem's tree: beside the build file that +// owns it, or (a virtualenv) holding its pyvenv.cfg. The budget protection +// PHARN-15 added stays for the real trees; a hand-authored folder is scanned. +describe("ECOSYSTEM_DIRS — skipped only where they are that ecosystem's tree", () => { + const tmp = useTmpDir(); + + it.each([ + ['target', 'Cargo.toml'], + ['target', 'pom.xml'], + ['target', 'build.sbt'], + ['vendor', 'go.mod'], + ['vendor', 'composer.json'], + ['vendor', 'Gemfile'], + ])('%s/ beside %s is skipped', (dir, marker) => { + touch(tmp.path(), marker); + touch(tmp.path(), `${dir}/x/Page.tsx`); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(false); + }); + + it.each(['venv', '.venv'])('%s/ holding pyvenv.cfg is skipped', (dir) => { + touch(tmp.path(), `${dir}/pyvenv.cfg`); + touch(tmp.path(), `${dir}/x/Page.tsx`); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(false); + }); + + it.each(['target', 'vendor', 'venv', '.venv'])( + '%s/ with no marker is scanned like any folder', (dir) => { touch(tmp.path(), `${dir}/x/Page.tsx`); - expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(false); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(true); + }, + ); + + // THE REPRODUCTION. A Next.js App-Router route under a folder named `target` + // or `vendor`: the backend signal is structural (`app/**/route.ts`), so no + // package.json dependency can stand in for it. It went dark. + it.each(['target', 'vendor'])( + 'detects the route at app/%s/route.ts as backend', + (dir) => { + writePkg(tmp.path(), { dependencies: { next: '15' } }); + touch(tmp.path(), `app/${dir}/route.ts`); + expect(detectArchetypesFromProject(tmp.path()).archetypes).toEqual([ + 'ssr', + 'backend', + ]); }, ); + + it('matches the dir name case-insensitively and the marker exactly', () => { + touch(tmp.path(), 'Cargo.toml'); + touch(tmp.path(), 'TARGET/x/Page.tsx'); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(false); + }); + + it('does not take a differently-cased marker, or a directory, as the marker', () => { + touch(tmp.path(), 'cargo.toml'); + touch(tmp.path(), 'go.mod/keep'); + touch(tmp.path(), 'target/x/Page.tsx'); + touch(tmp.path(), 'vendor/y/Other.tsx'); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(true); + }); + + it('checks the marker in the SAME directory, at any depth', () => { + // A nested Rust crate keeps its skip; a sibling marker one level up does + // not reach into a subfolder. + touch(tmp.path(), 'crates/core/Cargo.toml'); + touch(tmp.path(), 'crates/core/target/x/Page.tsx'); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(false); + touch(tmp.path(), 'Cargo.toml'); + touch(tmp.path(), 'web/target/Page.tsx'); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(true); + }); + + it('a marked ecosystem tree costs ZERO budget, like any skipped dir', () => { + touch(tmp.path(), 'Cargo.toml'); + for (let i = 0; i < 200; i += 1) { + touch(tmp.path(), `target/debug/f${i}.rlib`); + } + touch(tmp.path(), 'src/Page.tsx'); + expect(scanFileTreeSignals(tmp.path()).clientUi).toBe(true); + }); }); describe('detectArchetypesFromProject — file-tree scanning', () => { @@ -675,7 +758,7 @@ describe('SKIP_DIRS — classification neutrality (classifyEntry)', () => { ['under db/ (a SQL_HOST_DIRS ancestor — the migrations trigger)', ['db']], ]; - describe.each([...SKIP_DIRS])('%s', (dir) => { + describe.each([...SKIP_DIRS, ...ECOSYSTEM_DIRS.keys()])('%s', (dir) => { it.each(CONTEXTS)('produces no signal %s', (_label, segments) => { expect(classifyEntry(dir, true, segments)).toEqual(NO_SIGNAL); }); diff --git a/tests/layout.test.ts b/tests/layout.test.ts index aa4335f..ca7ea3e 100644 --- a/tests/layout.test.ts +++ b/tests/layout.test.ts @@ -2,7 +2,12 @@ import { mkdirSync } from 'node:fs'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; import { useTmpDir } from './helpers.js'; -import { configLayout, detectLayout, layoutPaths } from '../src/lib/layout.js'; +import { + capabilitySubtree, + configLayout, + detectLayout, + layoutPaths, +} from '../src/lib/layout.js'; import type { PharnConfig } from '../src/types.js'; describe('detectLayout', () => { @@ -111,3 +116,17 @@ describe('configLayout', () => { expect(configLayout(base)).toBe('flat'); }); }); + +// The ONE role → subtree mapping. Six call sites carried their own ternary +// (install-records, install-manifest ×2, install-capabilities, update, remove); +// they all route through this now, so they cannot disagree. +describe('capabilitySubtree', () => { + it.each([ + ['flat', 'griller', 'pharn-pipeline/grillers'], + ['flat', 'lens', 'pharn-review'], + ['pharn', 'griller', 'pharn/pharn-pipeline/grillers'], + ['pharn', 'lens', 'pharn/pharn-review'], + ] as const)('%s layout, %s → %s', (layout, role, subtree) => { + expect(capabilitySubtree(layoutPaths(layout), role)).toBe(subtree); + }); +}); diff --git a/tests/source-hygiene.test.ts b/tests/source-hygiene.test.ts new file mode 100644 index 0000000..1f56e66 --- /dev/null +++ b/tests/source-hygiene.test.ts @@ -0,0 +1,80 @@ +import { readdirSync, readFileSync } from 'node:fs'; +import { join, relative } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +// --------------------------------------------------------------------------- +// No file under src/ or tests/ may hold a raw INVISIBLE character: a C0 control +// other than tab, LF and CR; DEL or a C1 control; a Unicode format character +// (Cf — bidi overrides, zero-width characters, the byte-order mark); or a line / +// paragraph separator. An editor and a diff view show nothing there, so the +// character a test is about (or an attack) cannot be told from no character at +// all. Tests spell such characters as escapes instead. Two slipped through +// before this check existed: a BOM in a tar-name test (#218) and a +// right-to-left override in plan D's files. +// +// The class is built from escapes and code points, so this file passes its own +// check — the same construction lib/hook-wiring.ts uses for its display escape. +// --------------------------------------------------------------------------- + +const RAW_INVISIBLE = new RegExp( + `[\\x00-\\x08\\x0b\\x0c\\x0e-\\x1f\\x7f-\\x9f\\p{Cf}${String.fromCodePoint(0x2028, 0x2029)}]`, + 'u', +); + +const ROOT = join(import.meta.dirname, '..'); + +function* filesUnder(dir: string): Generator { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const path = join(dir, entry.name); + if (entry.isDirectory()) yield* filesUnder(path); + else if (entry.isFile()) yield path; + } +} + +// Every raw invisible character in `text`, as `line:column U+XXXX`. +function offenders(text: string): string[] { + const found: string[] = []; + text.split('\n').forEach((line, i) => { + [...line].forEach((ch, j) => { + if (RAW_INVISIBLE.test(ch)) { + const hex = ch.codePointAt(0)!.toString(16).toUpperCase(); + found.push(`${i + 1}:${j + 1} U+${hex.padStart(4, '0')}`); + } + }); + }); + return found; +} + +describe('source hygiene — no raw invisible characters', () => { + it.each(['src', 'tests'])('%s/ holds none', (dir) => { + const hits = [...filesUnder(join(ROOT, dir))].flatMap((file) => + offenders(readFileSync(file, 'utf8')).map( + (at) => `${relative(ROOT, file)}:${at}`, + ), + ); + expect(hits).toEqual([]); + }); + + // The class itself, so a check that matched nothing could not pass: every + // kind it names is caught, and the ordinary whitespace it spares is spared. + it.each([ + ['NUL', 0x00], + ['BEL', 0x07], + ['ESC', 0x1b], + ['DEL', 0x7f], + ['CSI (C1)', 0x9b], + ['zero-width space', 0x200b], + ['right-to-left override', 0x202e], + ['byte-order mark', 0xfeff], + ['line separator', 0x2028], + ['paragraph separator', 0x2029], + ])('catches %s', (_label, cp) => { + expect(offenders(`ok${String.fromCodePoint(cp)}ok`)).toEqual([ + `1:3 U+${cp.toString(16).toUpperCase().padStart(4, '0')}`, + ]); + }); + + it('spares tab, LF, CR and ordinary non-ASCII text', () => { + expect(offenders('a\tb\r\nc — é → ✓ …')).toEqual([]); + }); +}); diff --git a/tests/tar-extract.test.ts b/tests/tar-extract.test.ts index bf8cfc4..f56a8ef 100644 --- a/tests/tar-extract.test.ts +++ b/tests/tar-extract.test.ts @@ -650,7 +650,7 @@ describe('extractTar', () => { extractTar( githubArchive([ { - name: utf8AsLatin1('f.md'), + name: utf8AsLatin1('\ufeff' + 'f.md'), // BOM, then `f.md` prefix: 'pharn-oss-abc1234', data: 'x', }, diff --git a/tests/terminal-safe.test.ts b/tests/terminal-safe.test.ts index 382fc93..ff1e874 100644 --- a/tests/terminal-safe.test.ts +++ b/tests/terminal-safe.test.ts @@ -5,8 +5,8 @@ import { hasUnsafeChars, terminalSafe } from '../src/lib/terminal-safe.js'; const ESC = '\u001b'; const BEL = '\u0007'; const CSI_C1 = '\u009b'; -const RLO = '‮'; -const ZWSP = '​'; +const RLO = '\u202e'; +const ZWSP = '\u200b'; describe('terminalSafe (PHARN-17)', () => { it.each([ diff --git a/tests/update.test.ts b/tests/update.test.ts index 622eed9..32d6611 100644 --- a/tests/update.test.ts +++ b/tests/update.test.ts @@ -23,14 +23,35 @@ import { import { LOCK_FILE } from '../src/lib/project-lock.js'; import type { PharnConfig } from '../src/types.js'; +// One ordered record of what a run showed — spinner starts/stops and info/warn +// lines together — so a test can assert ORDER: a line logged while a clack +// spinner animates is glued to its frame, and below ~50 columns the spinner's +// stop erases it. Hoisted, because the mock factory can run before this file's +// own top-level code. Emptied after every test with the mocks' calls. +const { shown } = vi.hoisted(() => ({ shown: [] as string[] })); vi.mock('@clack/prompts', () => ({ intro: vi.fn(), isCancel: (v: unknown) => v === CANCEL, confirm: vi.fn(), note: vi.fn(), - log: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + log: { + info: vi.fn((m: unknown) => { + shown.push(`info: ${String(m)}`); + }), + warn: vi.fn((m: unknown) => { + shown.push(`warn: ${String(m)}`); + }), + error: vi.fn(), + }, outro: vi.fn(), - spinner: () => ({ start: vi.fn(), stop: vi.fn() }), + spinner: () => ({ + start: vi.fn((m?: string) => { + shown.push(`start: ${m ?? ''}`); + }), + stop: vi.fn((m?: string) => { + shown.push(`stop: ${m ?? ''}`); + }), + }), })); const fetchRepo = vi.fn(); @@ -167,6 +188,7 @@ describe('runUpdate (drift-safe)', () => { }); afterEach(() => { vi.clearAllMocks(); + shown.length = 0; restoreTTY(); }); @@ -1764,6 +1786,31 @@ describe('runUpdate (drift-safe)', () => { expect(backupDirs()).toHaveLength(1); }); + // F21: the notice used to print while the apply's spinner still animated + // — glued to its frame, and erased by its stop in a narrow terminal. The + // spinner is stopped first, the notice printed, and a new one started for + // the writes. + it('prints the backup notice with no spinner running', async () => { + await installed(); + write(join(proj, DOC), 'MY LOCAL EDIT'); + + await runUpdate({ force: true }); + + const notice = shown.findIndex((l) => l.startsWith('info: Backed up')); + expect(notice).toBeGreaterThan(-1); + const before = shown + .slice(0, notice) + .filter((l) => l.startsWith('start:') || l.startsWith('stop:')); + // The last spinner event before the notice STOPPED a spinner… + expect(before.at(-1)).toMatch(/^stop: /); + // …without repeating what the notice says next. + expect(before.at(-1)).not.toContain('Backed up'); + // …and the writes run under a spinner started AFTER it. + const after = shown.slice(notice); + expect(after.findIndex((l) => l.startsWith('start:'))).toBeGreaterThan(0); + expect(after.at(-1)).toBe('stop: Capabilities updated'); + }); + // --- the pointer on the FAILURE path -------------------------------- // // createBackup runs BEFORE the first original is touched, but its path used @@ -1811,6 +1858,26 @@ describe('runUpdate (drift-safe)', () => { expect(printedLines()).toContain('stopped part-way'); }); + // F21: the abort used to repeat the whole notice. The pointer is printed + // once per stream (the stderr copy is what `2> err.log` keeps), and the + // .gitignore hint only once — it is advice, not a pointer. + it('prints the .gitignore hint once, and the backup dir once per stream', async () => { + await abortedForcedRun(); + + const infos = vi.mocked(prompts.log.info).mock.calls; + expect( + infos.filter((c) => String(c[0]).includes('not gitignored')), + ).toHaveLength(1); + const dir = backupDirs()[0]!; + const pointers = infos.filter((c) => + String(c[0]).includes(`${BACKUP_DIR}/${dir}`), + ); + expect(pointers.map((c) => c[1])).toEqual([ + { output: process.stdout }, + { output: process.stderr }, + ]); + }); + it('sends the aborted-run notice to stderr, with the rest of the failure', async () => { await abortedForcedRun();