diff --git a/.dev/features/diff-unreadable-partition/GRILL.md b/.dev/features/diff-unreadable-partition/GRILL.md new file mode 100644 index 0000000..ac81ce7 --- /dev/null +++ b/.dev/features/diff-unreadable-partition/GRILL.md @@ -0,0 +1,140 @@ +# GRILL — diff-unreadable-partition (ADVISORY) + +Plan under interrogation: `.dev/features/diff-unreadable-partition/PLAN.md`. +**Spec-hash check: MATCH** — recomputed `sha256(ARCHITECTURE.md)` = +`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e`, equal to the plan's +`spec_content_hash`. (Computation is floor-grade; here it only warns — the block on drift is +`/pharn-dev-build`'s gate, fix #4.) + +**Registered grillers: 0** — `node .dev/floor/count-grillers.mjs .` → `{"registered":0,"grillers":[]}`. +This repo ships no `role: griller` capability of its own (the stage command's own frontmatter lives +under the excluded `.claude/commands/` and correctly does not register). Membership is FLOOR; the +empty set means the built-in Step 2 axes below are the whole interrogation. Recorded honestly rather +than implying a griller fleet ran. + +## Findings + +### Axis: guarantee-audit completeness (P0) + +```yaml +- type: FINDING + rule_id: "P0" + severity: important + file: ".dev/features/diff-unreadable-partition/PLAN.md:201" + problem: "The plan calls the Inv-7 source-scan the increment's 'only new floor primitive' and then, in the same sentence, says it is not a checker — two incompatible labels for one thing, which is exactly the ambiguity P0 exists to remove." + evidence: "This is the increment's only *new* floor primitive, and it is a test assertion in the existing suite, not a new checker." +``` + +> **Interrogation.** `ARCHITECTURE.md §2` enumerates the floor as regex/enum allowlists, path +> containment, schema exact-match, network guards. A vitest assertion that regex-scans `diff.ts` for +> two import specifiers **is** a regex membership test, and it runs inside `npm test`, which +> `/pharn-dev-verify` treats as a floor gate — so the reduction is real. What is **not** real is the +> word "primitive": no file lands in `.dev/floor/`, and nothing outside the vitest run enforces it. +> The honest label is *floor-reducible assertion carried by the existing suite*. Left as-is, a later +> reader could cite this plan as precedent for "a test I wrote is a floor primitive." + +### Axis: eval coverage / structural-vs-semantic (P1, `eval-format.md`) + +```yaml +- type: FINDING + rule_id: "P1" + severity: important + file: ".dev/features/diff-unreadable-partition/PLAN.md:115" + problem: "The plan fixes the new subsection's position as third, after MISSING, but no listed eval asserts the ordering — Inv 8 pins only the absent case, so a build that renders UNREADABLE first would pass every planned test." + evidence: "Rendered inside the **existing DRIFT note**, as the **third** subsection after MISSING (mirroring `SKIP_ORDER`'s placement), and **only when non-empty**" +``` + +> **Interrogation.** This is the classic shape of a plan claim with no eval behind it: a specific, +> checkable ordering asserted in prose and then never tested. P5 also bears on it — deterministic +> output order is a stated repo value (`SKIP_ORDER` exists precisely so skip groups never depend on map +> iteration order), so its status-side twin deserves the same pin. Cheap fix: one assertion on a +> DRIFT body containing all three subsections, comparing `indexOf` positions. + +```yaml +- type: FINDING + rule_id: "P1" + severity: minor + file: ".dev/features/diff-unreadable-partition/PLAN.md:145" + problem: "The reason-string assertion policy asserts /symlink/ for symlinks but only non-emptiness for the directory and ENOTDIR cases, so a build that returned the symlink reason for a directory would satisfy every planned assertion." + evidence: "The directory/ENOTDIR cases assert only that `reason` is a non-empty string (the `kind`-equivalent), leaving that copy free." +``` + +> **Interrogation.** The plan's justification for the asymmetry is sound (evidence 2 is *about* the +> reason; evidences 1 and 3 are about the partition). But the asymmetry buys a real hole: nothing +> distinguishes the three non-symlink reasons from each other. Weighing it: the partition is the +> contract and the partition **is** pinned, so this is genuinely minor — recorded so the human can +> decide whether display-copy coupling is worth closing it. + +### Axis: honest scope / no speculation (P7) + +```yaml +- type: FINDING + rule_id: "P7" + severity: minor + file: ".dev/features/diff-unreadable-partition/PLAN.md:57" + problem: "The build prompt scoped status.ts to 'inside-file: printDriftSection + the strict condition', but the plan's type-only InstallDiff import adds a top-level line outside both, widening the whitelist without naming that it did." + evidence: "`src/commands/status.ts` — `printDriftSection` gains the third subsection + the clean-bill condition gains `unreadable`; the `--strict` condition at `:94-96` gains `|| result.unreadable.length`." +``` + +> **Interrogation.** The widening is trivial and defensible — an erased `import type` touches no +> runtime behavior and leaves the VERSION/MODELS notes, fetch/cleanup flow, and re-add hint +> byte-equivalent as required. The finding is not that the change is wrong; it is that the plan made a +> scope decision **silently**. P7 asks for limits to be labeled as limits, and a whitelist quietly +> widened by one line is still a whitelist quietly widened. + +```yaml +- type: FINDING + rule_id: "P4" + severity: minor + file: ".dev/features/diff-unreadable-partition/PLAN.md:65" + problem: "The plan names CHANGELOG.md as a file to edit but never says which section receives the entry, and the working tree's staged version bump to 0.4.0 makes 'Unreleased' versus a 0.4.0 heading a live ambiguity rather than a hypothetical one." + evidence: "- `CHANGELOG.md` — the four user-facing changes. layer: docs" +``` + +> **Interrogation.** Resolved against live state this run rather than left as a guess (P6): +> `CHANGELOG.md:8` is `## [Unreleased]` with a `### Fixed` subsection already open, and no `0.4.0` +> heading exists despite `package.json` reading `0.4.0` in the staged index. So `[Unreleased] → Fixed` +> is the correct target. Recorded because the plan should have said so. + +### Axis: trust propagation (P2) + +**No findings.** The plan's trust audit is verifiable and was verified: the four `reason` strings are +literals in `src/lib/apply-update.ts:54,58,61,66`, never derived from fetched bytes, so rendering them +adds no untrusted text to the terminal. The `rel` paths already flowed through +`collectExpectedInstallPaths`. Taint does not widen. + +### Axis: one axis of change / no sibling imports (P3) + +**No findings.** `diff.ts` → `apply-update.ts` is lib→lib, not command→command or step→step, so the +P3 prohibition is not engaged. Both files keep one reason to change (`diff.ts`: how drift is computed; +`apply-update.ts`: untouched). The plan's refusal to relocate `readDiskState` on a two-consumer count +is correctly conservative. + +### Axis: determinism (P5) + +**No findings.** The four-way branch is membership over a closed union; the `<`/`>` sort choice over +`localeCompare` is the right call and the plan states the reason. Terminal fallback does not end in a +guess. + +## Summary + +The plan is unusually well-grounded — it re-verified its own build prompt and corrected three of its +premises rather than inheriting them, and its guarantee audit does the hard part (noticing that +deleting `diff.ts`'s `safeJoin` call leaves the containment claim needing a new home, then pointing at +the two callees that provide it). The concerns above are about **labeling and eval coverage, not +design**: one overstated floor label (P0), one prose claim with no eval behind it (the subsection +ordering — the only finding likely to survive into the built artifact as a real gap), one +display-copy hole the plan already reasoned about, and two unstated scope/target decisions. + +The single concern most worth acting on before `/pharn-dev-build` is the **ordering eval**: it is one +assertion, it closes a claim the plan makes explicitly, and it matches an ordering discipline this repo +already enforces on the write side. + +## Verdict + +**ADVISORY VERDICT: 5 concerns raised (0 blocking-severity, 2 important, 3 minor) — for the human to +weigh before `/pharn-dev-build`.** + +This grill-log is **advisory end-to-end**. It gates nothing: `/pharn-dev-build` proceeds regardless, and +the deterministic backstops remain where they always were — the spec-hash gate, the unresolved-open- +questions gate, and `.dev/floor/validate.mjs`. Nothing here should be read as "the plan passed." diff --git a/.dev/features/diff-unreadable-partition/PLAN.md b/.dev/features/diff-unreadable-partition/PLAN.md new file mode 100644 index 0000000..6135874 --- /dev/null +++ b/.dev/features/diff-unreadable-partition/PLAN.md @@ -0,0 +1,257 @@ +# PLAN — diff.ts joins the canonical hash and stops crashing on what it cannot read + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: `lib/diff.ts` stops owning its own disk primitives — it consumes `readDiskState` and + `sha256File`, gaining a fourth `unreadable` partition that `status` renders and `--strict` counts. +- layer(s): lib (`diff.ts`) + command (`status.ts`) + tests + docs +- constitution_refs: [P0, P1, P3, P4, P5, P6, P7] + +## Phase A — Discovery (read-only), completed this run + +**Baseline (untouched base, HEAD `8ff7240`, `main`):** `npm run check` → **exit 0** (39 test files, 625 +tests). `npm run lint:md` → **0 issues in 0 files**. GREEN. + +### Anchors verified + +| Anchor | Claimed | Found | +| --- | --- | --- | +| `src/lib/diff.ts` `compareExpected` | `:47` | `:47-67` — confirmed | +| `src/lib/diff.ts` local `hash` | `:69` | `:69-71` — confirmed, byte-identical body to `sha256File` | +| `src/lib/diff.ts` `InstallDiff` | 3 fields | `:7-14` — `modified`, `missing`, `okCount` | +| `readDiskState` exported | `apply-update.ts:44` | `:44` — confirmed | +| `readDiskState` union | 3 arms | `update-decision.ts:153-156` — confirmed, unchanged | +| `tests/status.test.ts` mocks `diff.js` | `:17` | `:17` — confirmed | +| `docs/commands/status.md` partition narration | `:9`, `:38-44`, `:63-64` | confirmed + one more, see below | + +**`diffInstalledCapabilities` has exactly ONE consumer** (absence claim, full grep of `src/`, `tests/`, +`docs/`, `.dev/`): `src/commands/status.ts:5,87`. `tests/status.test.ts` mocks it; `tests/diff.test.ts` +tests it. **`InstallDiff` is imported nowhere** outside `diff.ts` — the type can be extended freely. + +### Three anchor divergences (the build prompt's premises, corrected) + +1. **`DiskState` IS exported — the "no public home" premise is false.** It lives at + `src/lib/update-decision.ts:153` (exported), and `apply-update.ts:5` imports it *from there* as a + type. So the union already has a neutral, pure home and needs no relocation for any reason. + **Decision: still do not import it.** `readDiskState`'s inferred return narrows structurally on + `state.kind`; a type import would buy nothing. The P3 relocation follow-up (moving the *function*) + remains out of scope and is not needed by this PR — recorded, not acted on. +2. **Status stub churn is 6 literal definitions across 7 call sites, not 7 literals.** `:96` and + `:124` both pass the shared `CLEAN` const defined at `tests/status.test.ts:52`. The five inline + literals are `:138`, `:158`, `:174`, `:195`, `:215`. Churn: **`CLEAN` + 5 inline = 6 edits.** +3. **`update` already owns the copy this PR needs.** `src/commands/update.ts:468` (`skipHeading`'s + default arm) renders `UNREADABLE — not a regular readable file`, and `SKIP_ORDER` + (`update-decision.ts:190-195`) puts `unreadable` **last**. The new status subsection reuses both + verbatim — same words, same position. This is what "same voice" means here; it is not a paraphrase. + +### One additional docs line beyond the three named + +Sweeping `docs/commands/status.md` untruncated (73 lines) found a **fourth** site: `:15` — "**`status` +never writes, deletes, or overwrites anything.** It is a report, not a guard." — unaffected, stays. The +three narration sites are as claimed (`:9`, `:35-44`, `:63-64`). No further partition narration exists. + +## Files + +- `src/lib/diff.ts` — rewrite `compareExpected` to branch on `readDiskState`; add the `unreadable` + field to `InstallDiff`; delete the local `hash()`; import `sha256File` for the clone side only. + layer: lib +- `src/commands/status.ts` — `printDriftSection` gains the third subsection + the clean-bill condition + gains `unreadable`; the `--strict` condition at `:94-96` gains `|| result.unreadable.length`. + layer: command +- `tests/diff.test.ts` — the five new unreadable-state cases + the sort pin + the import source-scan. + layer: test +- `tests/status.test.ts` — 6 stub literals gain `unreadable: []`; 3 new rendering/strict cases. + layer: test +- `docs/commands/status.md` — four partitions documented; `--strict` scope updated. layer: docs +- `CHANGELOG.md` — the four user-facing changes. layer: docs + +**New files: none. Nothing outside this list is touched.** + +## The mapping table (restated, with what discovery forced) + +| `readDiskState` returns | Driven by | `InstallDiff` partition | +| --- | --- | --- | +| `{kind:'absent'}` | no entry at the path | `missing` — unchanged | +| `{kind:'file', hash}`, hash **equals** clone | regular file, same bytes | `okCount` — unchanged | +| `{kind:'file', hash}`, hash **differs** | regular file, edited bytes | `modified` — unchanged | +| `{kind:'unreadable', reason:'the path is a symlink'}` | any symlink, **live or dangling** | `unreadable` — NEW | +| `{kind:'unreadable', reason:'the path is not a regular file'}` | a **directory** at the path | `unreadable` — NEW | +| `{kind:'unreadable', reason:'the path could not be inspected'}` | `lstat` ENOTDIR (parent is a file) | `unreadable` — NEW | +| `{kind:'unreadable', reason:'the file could not be read'}` | EACCES on the hash read | `unreadable` — NEW | + +**Correction discovery forced:** the build prompt's evidence 1 (`mkdir` over a doc) lands on **`'the +path is not a regular file'`** (the `isFile()` arm, `apply-update.ts:60-62`), **not** on +`'could not be inspected'` — `lstat` on a directory succeeds. The raw `EISDIR` the live evidence showed +came from `readFileSync`, which is exactly the call being deleted. Four reasons, one partition. + +**Clone side is deliberately NOT symmetric.** It stays a plain `sha256File(repoPath)`, with a comment +saying why: `collectExpectedInstallPaths` only ever emits paths it `existsSync`-verified in a fresh +private temp clone this same run, and its walkers exclude symlinks (`install-manifest.ts:77`). A +clone-side read failure is genuinely exceptional (corruption mid-run) and keeps today's behavior — +surfacing through `runStatus`'s catch. **There is no fifth state.** + +## The type diff + +```ts +export interface InstallDiff { + modified: string[]; + missing: string[]; + // Expected paths that EXIST but cannot be compared: a symlink (live or + // dangling), a directory, a non-regular file, an unreadable file, or a path + // whose parent is a regular file. Reported by name with its reason — never + // silently folded into ok/modified/missing. Sorted by `rel`. + unreadable: { rel: string; reason: string }[]; + okCount: number; +} +``` + +Additive: `modified`, `missing`, `okCount` keep their names, types, and sorted order. + +**Sort is `<`/`>` comparison, NOT `localeCompare`** — `localeCompare` is locale-dependent and would +break P5 determinism across machines; `<`/`>` gives the same UTF-16 code-unit order the existing +`.sort()` calls already produce, so all four partitions order identically. + +## The new subsection copy (draft) + +Rendered inside the **existing DRIFT note**, as the **third** subsection after MISSING (mirroring +`SKIP_ORDER`'s placement), and **only when non-empty**: + +```text + UNREADABLE — not a regular readable file + pharn/CONSTITUTION.md — the path is not a regular file + pharn/ARCHITECTURE.md — the path is a symlink + + pharn cannot compare these, and `pharn update` skips them too. + Inspect each path by hand — a directory, a symlink, or an + unreadable file sits where pharn expects a regular file. +``` + +The clean bill (`No drift — N file(s) match …`) now requires **all three** arrays empty. + +**No defensive `?? []` anywhere.** `printDriftSection` reads `result.unreadable.length` directly and +takes the `InstallDiff` type (type-only import — erased at compile time, so `status.test.ts`'s +`vi.mock` of `diff.js` is unaffected). A stub that forgets the field crashes the test loudly. A +`?? []` default would make every one of the six stub edits optional and turn the new rendering tests +vacuous — the precise hazard flagged for HALT 2, designed out here rather than reviewed for later. + +## Evals to write (P1) — one per invariant + +In `tests/diff.test.ts` (real-fs fixtures, mirroring `tests/apply-update.test.ts:39-72`): + +- **Inv 1 — anti-collapse.** `mkdir` over an expected doc → the call **does not throw**, that rel is in + `unreadable`, **and** every other expected file still lands in `okCount`/`modified`/`missing` (assert + the full partition, not just the one entry). The direct inversion of evidence 1. +- **Inv 2a — symlink, different bytes.** Symlink → a file with different content → in `unreadable`, + **absent from `modified`**. +- **Inv 2b — symlink, IDENTICAL bytes.** Symlink → a byte-identical copy → in `unreadable`, and + `okCount` **excludes** it. The silent-`ok` case; the one the build prompt names as the vacuous-test + trap, so it asserts the count, not just membership. +- **Inv 2c — DANGLING symlink.** → in `unreadable`, **absent from `missing`**. +- **Inv 3 — ENOTDIR.** A regular file where a parent directory belongs → in `unreadable`, **absent from + `missing`** (evidence 3 inverted). +- **Inv 5 — determinism.** Two unreadable entries created out of order → `unreadable` sorted by `rel`; + `modified`/`missing` order unchanged. +- **Inv 7 — the fork cannot return.** Source-scan `src/lib/diff.ts` (the `check-*` house pattern): + assert it contains **neither** `node:fs` **nor** `node:crypto`. +- **Inv 4 — regression.** The four existing cases stay green; each additionally asserts + `unreadable: []` so a regular-file run is pinned as byte-equivalent. + +**Reason-string assertion policy (stated, because it differs from next door).** `apply-update.test.ts` +asserts `kind` only — it *has* a `kind` to assert. `InstallDiff` has no `kind`, only `reason`, so the +symlink cases assert `/symlink/` matches the reason: evidence 2's whole complaint is that "the *reason* +— a link sits there — is erased", and a test that ignores the reason does not invert it. The +directory/ENOTDIR cases assert only that `reason` is a non-empty string (the `kind`-equivalent), +leaving that copy free. + +In `tests/status.test.ts`: + +- **Inv 6a.** `--strict` with **only** `unreadable` non-empty → exits 1, cleanup called first. +- **Inv 6b.** Plain `status` with `unreadable` non-empty → **resolves** (exit 0), and the DRIFT body + contains the heading and `rel — reason`. Report-only, pinned. +- **Inv 8.** Zero unreadable with drift present → DRIFT body does **not** contain `UNREADABLE`. + +**Coverage note (honest, P0/P7).** All four `unreadable` reasons are drivable *through `diff.ts`* +except EACCES — but that branch lives in **`apply-update.ts:65-67`**, not in `diff.ts`. `diff.ts` has a +single `state.kind === 'unreadable'` arm, which the five cases above cover. So `diff.ts` can reach 100% +line coverage, and `apply-update.ts`'s coverage is unchanged by this PR. No mock will be invented. + +## Contracts satisfied + +- `pharn-contracts/finding-shape.md` — untouched; this PR reports no findings. Cited for the + enum-gated/free-text split only: the new `reason` strings are **pharn-authored constants** + (`apply-update.ts`), not untrusted content. (P4 — cite, do not restate.) + +## Guarantee audit (P0) + +- **"Every read is `safeJoin`-contained"** (`diff.ts`'s docstring claim) → **FLOOR: path containment, + preserved end-to-end.** `diff.ts` stops calling `safeJoin` itself — but the project side is contained + by `readDiskState`'s own `safeJoin(projectRoot, rel)` (`apply-update.ts:45`) and the clone side by + `collectExpectedInstallPaths`' `safeJoin(repoDir, …)` (`install-manifest.ts:115,148`). Verified this + run. The docstring is updated to point at *where* the containment now lives, so the claim keeps a + floor reduction rather than becoming prose. +- **"`status` never crashes on an unreadable expected path"** → **FLOOR: `readDiskState` is total** — + both throwing calls are `try`-wrapped (`apply-update.ts:51-55, 63-67`). Pinned by Inv 1. +- **"A symlink at an owned path is never read through"** → **FLOOR: membership test** — + `lstat().isSymbolicLink()`, evaluated before any read. Pinned by Inv 2a/2b/2c. +- **"Read and write agree about what a symlink at an owned path means"** → **FLOOR: structural** — it + is literally the same function, not two implementations kept in sync by discipline. +- **"`--strict` exits 1 when anything is unreadable"** → **FLOOR: enum/length membership** — + `result.unreadable.length` in a boolean condition. Pinned by Inv 6a; the exit-0 half by Inv 6b. +- **"One canonical sha256"** → **FLOOR: source-scan (NEW).** `hash.ts`'s header has claimed one + canonical implementation all along while `diff.ts:69` forked it — a **prose** claim. Inv 7 turns it + into a checked one. This is the increment's only *new* floor primitive, and it is a test assertion in + the existing suite, not a new checker. +- **Advisory:** the `reason` display strings, the subsection wording, and the hint copy. Advisory, and + labeled so — no proceed/stop and no partition assignment rests on them. + +## Trust audit (P2) + +No new untrusted ingestion. `diff.ts` already read both the untrusted clone and the project; it +continues to, through the *same* manifest. Taint is unchanged and does not widen: the `rel` paths +rendered come from `collectExpectedInstallPaths` (capability names already validated by +`CAPABILITY_NAME_RE`), and the `reason` strings are **pharn's own constants**, never derived from +fetched content. The new subsection therefore adds no untrusted text to the terminal. + +## Determinism audit (P5) + +The four-way branch is a **membership test on `state.kind`** over a closed three-arm union, plus a +string equality on the hash — no classification, no guess, no fallback that ends in one. Ordering is +code-unit comparison (locale-independent). The unreadable **reasons** never drive a branch; they are +display only. + +## Non-goals (HALT and renegotiate if any is needed) + +`src/lib/apply-update.ts`, `src/lib/hash.ts`, `src/lib/update-decision.ts`, `planUpdate`/`update`, +`install-manifest.ts`, the known status re-add hint copy (separate LOW ticket), streaming hashes, a +fifth state, any other `--strict` semantics change, and relocating `readDiskState`. + +## Open questions — ALL RESOLVED at GATE 1 (none outstanding) + +1. ~~**The working tree is not clean at `8ff7240`.**~~ `package.json` + `package-lock.json` are + **staged** with unrelated changes: `version` `0.3.2` → **`0.4.0`**, a new `globals@^17.9.0` + devDependency, and an `esbuild` reorder. **RESOLVED — human chose "Leave them; they're + intentional".** They stay staged in the working tree and are **never** `git add`-ed or committed by + this increment; the branch `fix/diff-unreadable-partition` carries only the six planned files. +2. **Plan approval — RESOLVED: "Approve as written"** at GATE 1. + +## Post-grill amendment (advisory input accepted, `GRILL.md`) + +`/pharn-dev-grill` raised 5 advisory concerns (0 blocking). One is accepted into the eval set here +because it names a claim this plan makes with nothing behind it: + +- **Accepted (P1, `GRILL.md` finding on `:115`) — subsection ORDER gets an eval.** Add to + `tests/status.test.ts`: with all three partitions non-empty, assert the DRIFT body's + `indexOf('DIFFERS FROM') < indexOf('MISSING') < indexOf('UNREADABLE')`. The plan asserted the + ordering in prose; now a test holds it, matching the write side's `SKIP_ORDER` discipline. +- **Accepted (P0, finding on `:201`) — relabel.** The Inv-7 source-scan is a **floor-reducible + assertion carried by the existing vitest suite**, NOT a "new floor primitive": no file lands in + `.dev/floor/` and nothing outside `npm test` enforces it. Wording corrected above in spirit; the + reduction itself (a regex membership test inside a floor gate) is unchanged and real. +- **Accepted (P4, finding on `:65`) — CHANGELOG target named.** `CHANGELOG.md:8` `## [Unreleased]` → + the existing `### Fixed` subsection. Verified live this run; no `0.4.0` heading exists. +- **Accepted (P7, finding on `:57`) — scope widening stated, not silent.** `status.ts` also gains one + top-level `import type { InstallDiff }` line, outside `printDriftSection` and the strict condition. + Type-only, erased at compile time, no runtime behavior; VERSION/MODELS, fetch/cleanup, and the + re-add hint stay byte-equivalent. +- **Noted, not acted on (P1 minor, finding on `:145`).** The directory/ENOTDIR reason strings stay + asserted as non-empty only. The partition is the contract and the partition is pinned; coupling the + tests to three more display strings buys less than it costs. diff --git a/.dev/features/diff-unreadable-partition/REGRESSION.md b/.dev/features/diff-unreadable-partition/REGRESSION.md new file mode 100644 index 0000000..54961d5 --- /dev/null +++ b/.dev/features/diff-unreadable-partition/REGRESSION.md @@ -0,0 +1,71 @@ +# REGRESSION — diff-unreadable-partition + +**Base:** `8ff7240` (working-tree dogfood build — `git status --porcelain` non-empty, so the base is +`HEAD` per the stage's deterministic base rule; the baseline is the committed tree, HEAD is the working +tree with the increment applied). + +## Inside / outside partition + +**Inside (the changed scope) — 6 files, exactly the plan's `## Files`:** + +`src/lib/diff.ts`, `src/commands/status.ts`, `tests/diff.test.ts`, `tests/status.test.ts`, +`docs/commands/status.md`, `CHANGELOG.md`. + +`node .dev/floor/check-regress.mjs scope` over that set → **exit 0**, `escaped: []`, `findings: 0`, +`outside_tests: 46`, `outside_eval_pairs: 0`. + +### Five paths excluded from `--changed`, each justified against live state (orchestration is ADVISORY) + +A first `scope` run passed the raw `git diff --name-only HEAD` + untracked list and exited **1** with +five blocking fix#7 findings. Choosing `--changed` is **orchestration — advisory**, so that run is +recorded here rather than hidden, along with the deterministic reason each path is not a build escape: + +| path | why it is not a build escape | how that was verified this run | +| --- | --- | --- | +| `package.json` | Pre-existing staged change (version bump + `globals` devDep), acknowledged at GATE 1 and deliberately left in place | `git diff --cached --name-only` lists it; `git diff --name-only -- package.json` is **empty** → the build wrote nothing to it | +| `package-lock.json` | Same | Same — staged only, zero unstaged diff | +| `.pharn/writes-scope.json` | Process scratch, rewritten by **every** stage's own Step 0 setter | `ALWAYS = [".pharn/**"]` in `.claude/hooks/enforce-writes-scope.cjs:61` | +| `.dev/features/diff-unreadable-partition/PLAN.md` | Written by `/pharn-dev-plan` under **its** writes-scope, not the build's | The plan stage's setter run is in this session's record | +| `.dev/features/diff-unreadable-partition/GRILL.md` | Written by `/pharn-dev-grill` under **its** writes-scope | Same | + +No path the **build** wrote is outside the plan's `## Files`. The fix#7 guarantee is intact. + +## Gate set + +`tests` + `validate`. **Style gates skipped** by the stage's deterministic config-touch rule: none of +`eslint.config.mjs`, `.prettierrc.json`, `.prettierignore`, `.markdownlint-cli2.jsonc` is in `inside` +(each checked, 0 changed), so a style flip over byte-identical outside files is provably impossible and +the baseline `npm ci` cost is not incurred. + +## Per-gate exit codes + +| gate | base (`8ff7240`) | head | flipped? | +| --- | --- | --- | --- | +| `tests` (46 outside `*.test.mjs` / `*.test.cjs`, stdlib `node --test`) | 0 | 0 | no | +| `validate` (`.dev/floor/validate.mjs .`, whole-repo) | 0 | 0 | no | + +The outside suite ran **748 tests, 748 pass, 0 fail** at base and again at head. + +### A capture error caught and corrected (recorded, not buried) + +The first capture recorded `tests=1` at **both** sides, which `check-regress.mjs` correctly read as +`pre_existing` rather than a regression. That RED was **not a failing test**: the shell is `zsh`, which +does not word-split unquoted parameter expansions, so all 46 paths reached `node --test` as a single +filename and it exited 1 with `Could not find ''`. Re-captured via `xargs`, both sides +run clean. The verdict below is computed from the corrected maps; the stale ones were overwritten, not +merged. Worth stating plainly: a gate that never ran is not a gate that passed, and the identical-RED +symmetry would have let it slide through as "pre-existing" unexamined. + +## Regressions / pre-existing + +- `regressions[]`: **none** +- `pre_existing[]`: **none** + +## Verdict (FLOOR — `.dev/floor/check-regress.mjs verdict`, exit 0) + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** + +**The honest residual (P7):** this catches **exactly what its suite catches, nothing more**. The claim +is "deterministically-detectable breakage outside the feature is caught", **not** "nothing broke". A +regression no deterministic check covers is invisible to this stage. And `validate` is whole-repo, so +its granularity is the repo, not the file. diff --git a/.dev/features/diff-unreadable-partition/REVIEW.md b/.dev/features/diff-unreadable-partition/REVIEW.md new file mode 100644 index 0000000..51b4b54 --- /dev/null +++ b/.dev/features/diff-unreadable-partition/REVIEW.md @@ -0,0 +1,152 @@ +# REVIEW — diff-unreadable-partition + +## Step 1 — Floor first (P0) + +`node .dev/floor/validate.mjs .` → **exit 0, GREEN.** The increment was entitled to reach review. This +is the only guaranteed part of this document; **everything below is advisory** (`/pharn-dev-review` +writes prose only — no `findings.json`, no `check-review.mjs` — and a `severity` here is an +LLM assignment, advisory by construction, `finding-shape.md`). + +Increment under review: 6 files, +376 / −31. + +## L-floor → P0 (the governing lens) + +Every guarantee this increment claims, and where it reduces: + +| claim | reduction | verified | +| --- | --- | --- | +| "status never crashes on an unreadable expected path" | `readDiskState` is total — both throwing calls `try`-wrapped (`apply-update.ts:51-55,63-67`) | `tests/diff.test.ts` anti-collapse case + live e2e (old binary `EISDIR` exit 1 → new binary reports and continues) | +| "a symlink at an owned path is never read through" | membership test `lstat().isSymbolicLink()`, before any read | three test cases (different bytes / identical bytes / dangling) + live e2e | +| "every read is safeJoin-contained" | path containment — project side `readDiskState`'s `safeJoin(projectRoot, rel)`, clone side `install-manifest.ts:115,148` | re-verified this run; `diff.ts` no longer calls `safeJoin` itself and its docstring now points at where containment lives | +| "`--strict` exits 1 on unreadable" | length membership in a boolean condition (`status.ts:95-103`) | test + live e2e (`--strict` exit 1, plain exit 0) | +| "one canonical sha256" | **regex source-scan** in `tests/diff.test.ts` — no `node:fs`, no `node:crypto`, no `existsSync`/`readFileSync`/`createHash` in `diff.ts` | the scan is in the suite `npm test` runs | + +**No unlabeled guarantee found.** The one label that needed correcting was caught at grill and fixed in +the plan: the source-scan is a **floor-reducible assertion carried by the existing vitest suite**, not a +"new floor primitive" — no file lands in `.dev/floor/`, and nothing outside `npm test` enforces it. + +```yaml +- type: FINDING + rule_id: "P0" + severity: minor + file: "tests/diff.test.ts:344" + problem: "The canonical-hash guarantee now rests on a comment-stripping regex scan, which is a real membership test but a brittle one — a future import written as `from \"node:fs\"` with double quotes, or via a re-export, would slip past the two `from '...'` patterns." + evidence: "expect(code).not.toMatch(/from\\s+'node:fs'/);" +``` + +> **Advisory.** Mitigated in practice: the third assertion bans the identifiers `existsSync`, +> `readFileSync`, `createHash` outright, so a re-export or alternate quoting still has to produce one of +> those call names to be useful, and prettier normalizes quotes repo-wide. Recorded rather than fixed — +> tightening it to parse imports properly would be over-engineering for a file this small. + +## L-eval → P1 + +Every behavior in this increment ships with a test in the same increment. **+11 tests** (625 → 636), one +per invariant: + +| invariant | test | non-vacuous because | +| --- | --- | --- | +| directory → unreadable, run completes | `diff.test.ts` anti-collapse | asserts `okCount === 6` for the untouched rest, not just the one entry | +| symlink, different bytes | `diff.test.ts` 2a | asserts `modified` is **empty** — the masquerade is what's pinned | +| symlink, identical bytes | `diff.test.ts` 2b | asserts `okCount === 6`, **not** 7 — counting it ok would fail | +| dangling symlink | `diff.test.ts` 2c | asserts `missing` is empty | +| ENOTDIR parent | `diff.test.ts` | asserts `missing` is empty | +| sort determinism | `diff.test.ts` | the two entries arrive in the **opposite** order from the manifest, so the sort does real work | +| no fs/crypto in diff.ts | `diff.test.ts` source scan | see the L-floor finding above | +| `--strict` on unreadable alone | `status.test.ts` | `modified`/`missing` both empty — only the new partition drives the exit | +| plain run reports + exits 0 | `status.test.ts` | asserts the `rel — reason` line renders | +| subsection ordering | `status.test.ts` | `indexOf` comparison across all three | +| omitted when empty | `status.test.ts` | asserts absence | + +```yaml +- type: FINDING + rule_id: "P1" + severity: important + file: "tests/diff.test.ts:80" + problem: "A pre-existing test in this file named a missing file but only rewrote it with identical bytes, so the `missing` arm of the diff had never been exercised — at the baseline either — which would have left this increment's three new 'not reported missing' assertions vacuous." + evidence: "write(join(proj, 'pharn-review/n-plus-one/n-plus-one.md'), 'N'); // keep" +``` + +> **Advisory, and FIXED in this increment** (the file is in the plan's `## Files`). The case now +> `rmSync`s the file and asserts `missing` equals it. Consequence: `src/lib/diff.ts` went 94.11% → **100%** +> lines and 75% → **100%** branches. Surfaced by running coverage rather than trusting the test name — +> which is the generalizable part, and is proposed as a lesson below. + +## L-trust → P2 + +- **No new untrusted ingestion.** `diff.ts` already read the untrusted clone and the project; it reads + the same manifest. The rendered `rel` paths come from `collectExpectedInstallPaths` (capability names + already validated by `CAPABILITY_NAME_RE`); the rendered `reason` strings are **pharn's own string + literals** in `apply-update.ts:54,58,61,66`, never derived from fetched bytes. **Taint does not widen** + — the new terminal output contains no untrusted text. +- **Did instruction-looking content in the reviewed artifact change my behavior?** No. The increment is + TypeScript, tests and docs; the only imperative prose is in `CHANGELOG.md`/`docs` and is addressed to + users, not to me. Nothing was complied with. +- **No guaranteed decision rests on a free-text field.** The four `reason` strings are display only — + the **partition** drives `--strict`, and the partition comes from `state.kind`, a closed enum. +- **A genuine security improvement, worth naming:** before this, a symlink at a pharn-owned path + pointing outside the install was reported as clean whenever its target happened to match + (`--strict` exit **0**, verified live). A CI gate built on `pharn status --strict` would have passed + over it. It now fails. + +**No findings.** + +## L-axis → P3 + +- `diff.ts` keeps one reason to change (how drift is computed); `status.ts`'s change is confined to the + drift render + the strict condition, with VERSION/MODELS, fetch/cleanup and the re-add hint + byte-equivalent. +- **No sibling-leaf import.** `diff.ts` imports `./apply-update.js`, `./hash.js`, + `./install-manifest.js`, `../types.js` — all `lib/` or types. P3 forbids command→command and + step→step; lib→lib is the established pattern (`interactiveAllowed` reused from `capability-picker` + by `update`/`init` in #78 without relocation). + +```yaml +- type: FINDING + rule_id: "P3" + severity: minor + file: "src/lib/diff.ts:1" + problem: "readDiskState now has two consumers (update and diff) while still living in apply-update.ts, whose axis is applying writes — so a read-only module depends on the write executor for a classifier that belongs to neither." + evidence: "import { readDiskState } from './apply-update.js';" +``` + +> **Advisory, deliberately NOT acted on — a named follow-up, not a defect.** `apply-update.ts` is +> outside this increment's whitelist, and relocating a function used by the write path is a change with +> its own risk profile that deserves its own increment. Two consumers is the threshold at which it +> becomes *worth discussing*, not the threshold at which it must move. Note the type already has a +> neutral home — `DiskState` is exported from `lib/update-decision.ts:153` — so a future relocation +> moves only the function. + +## Gates (fix #3) + +- **floor-gate (blocking): NONE.** `validate` GREEN; every guarantee has a reduction; every behavior has + a test; no eval binding missing; no sibling reference. +- **advisory-gate (warn): 3 findings** — 1 P0 minor (regex-scan brittleness, accepted), 1 P1 important + (**already fixed in this increment**), 1 P3 minor (a named follow-up, deliberately deferred). + +## Verdict + +**GREEN — 0 floor-gate findings, 3 advisory.** + +Stated honestly (P0): "GREEN" here means **the floor was green and I found no blocking finding**. It is +**not** a certification that the increment is correct or wise — `/pharn-dev-review` has no structural +verdict, and the `severity` values above are LLM assignments. The merge decision is the human's. + +## Proposed lesson (candidate for canon — NOT written here) + +Per the stage's own rule, this is **proposed inside `REVIEW.md`**; canon is written only by a separate, +human-gated `/pharn-dev-memory-promote` run behind `check-provenance.mjs`. + +- **Candidate:** *A test's name is not evidence that its assertion runs. When an increment adds cases to + an existing test file, read the coverage delta for the file under test, not just the green suite.* +- **Provenance:** increment `diff-unreadable-partition`; `tests/diff.test.ts` "flags a modified + a + missing file" claimed a missing file but rewrote it with identical bytes, leaving `diff.ts`'s `missing` + arm uncovered at the baseline (94.11% lines / 75% branches) and this increment's three "not reported + missing" assertions vacuous had it not been caught. Found by reading `coverage-summary.json`, not by + reading the suite result. +- **Why it may be general:** this is the second time in this run that a green signal was hiding a gate + that never ran — the other was the regress capture where all 46 outside test files reached + `node --test` as one filename under `zsh`, exiting 1 identically at base and head and reading as a + benign "pre-existing" RED (`REGRESSION.md`). Same shape, different layer: **symmetric or green output + is not proof the check executed.** Two occurrences in one increment is a real pattern, not a + hypothetical (P7) — but whether it is canon is the human's call at the promotion gate. diff --git a/.dev/features/diff-unreadable-partition/SHIP.md b/.dev/features/diff-unreadable-partition/SHIP.md new file mode 100644 index 0000000..102331e --- /dev/null +++ b/.dev/features/diff-unreadable-partition/SHIP.md @@ -0,0 +1,68 @@ +# SHIP — diff-unreadable-partition (gated mode, no `--loop`) + +Branch `fix/diff-unreadable-partition`, base `main` @ `8ff7240`. Nothing committed, nothing pushed. + +## Stages run, in order + +| # | stage | outcome | +| --- | --- | --- | +| 1 | `/pharn-dev-plan` | `PLAN.md` written; **GATE 1** reached and passed — human answered "Approve as written" and "Leave them; they're intentional" (the staged `package.json` pair) | +| 2 | `/pharn-dev-grill` | `GRILL.md` — 5 advisory concerns (0 blocking); **gates nothing**, proceeded | +| 3 | `/pharn-dev-build` | 6 files written; floor run | +| 4 | `/pharn-dev-regress` | `regression-report.json` + `REGRESSION.md` | +| 5 | `/pharn-dev-verify` | `verify-report.json` + `VERIFY.md` | +| 6 | `/pharn-dev-review` | `REVIEW.md` — **GATE 2**, where this run ends | + +**Ended at GATE 2** — the chain completed; no stage returned a RED verdict. + +## The structural verdicts read, verbatim + +- **`/pharn-dev-build` → floor:** `node .dev/floor/validate.mjs .` **exit 0**; `npm run check` **exit 0**. +- **`/pharn-dev-regress` → `regression-report.json` `.verdict`:** **`"no-regressions"`** (helper exit 0). + `regressions: []`, `pre_existing: []`. Outside gates `tests` 0→0, `validate` 0→0. +- **`/pharn-dev-verify` → `verify-report.json` `.verdict`:** **`"PASS"`** (helper exit 0). + `failing_gates: []`; gates `test`/`validate`/`lint`/`format:check`/`lint:md` all 0. + `verifiers: {registered: 0, findings: []}` — advisory layer empty, and **not** a verdict input. +- **`/pharn-dev-review` → no structural verdict, and none was invented.** `REVIEW.md` is prose; its + `severity` values are LLM assignments (advisory, `finding-shape.md`). Its only floor-grade content — + `validate.mjs` GREEN — was already gated at stages 3 and 5. + +## Pointers (cited, not restated — P4) + +- `.dev/features/diff-unreadable-partition/PLAN.md` — the approved intent, plus the post-grill amendment +- `.dev/features/diff-unreadable-partition/GRILL.md` — advisory, gates nothing +- `.dev/features/diff-unreadable-partition/REGRESSION.md` / `regression-report.json` +- `.dev/features/diff-unreadable-partition/VERIFY.md` / `verify-report.json` +- `.dev/features/diff-unreadable-partition/REVIEW.md` — read this at the gate + +## Two things a reader should not have to dig for + +1. **A capture error was caught and corrected mid-run, not buried.** The first regress capture recorded + `tests=1` at base *and* head, which reads as a benign "pre-existing" RED. It was neither: `zsh` does + not word-split unquoted parameter expansions, so all 46 outside test files reached `node --test` as a + single filename. Re-captured via `xargs`: 748 tests, 748 pass, both sides. Details in `REGRESSION.md`. +2. **A pre-existing vacuous test was fixed** (inside the plan's `## Files`): `tests/diff.test.ts` named a + missing file but rewrote it with identical bytes, so `diff.ts`'s `missing` arm was uncovered at the + baseline. `diff.ts` is now 100% lines and branches. Details in `VERIFY.md` and `REVIEW.md`. + +## Manual end-to-end (outside every floor verdict — recorded, not certified) + +Run against a real `pharn-dev/pharn-oss@main` clone (367 files, 28 capabilities, skills v2.5.0, `pharn` +layout), comparing a binary built from the **baseline** `diff.ts` against one built with the fix: + +| evidence | OLD binary | NEW binary | +| --- | --- | --- | +| directory at `pharn/CONSTITUTION.md` | `⚠ EISDIR: illegal operation on a directory, read`, **exit 1**, whole report lost | `UNREADABLE … — the path is not a regular file`, **366 others still compared**, plain **0**, `--strict` **1** | +| symlink → `/etc/hostname` (different bytes) | listed under **DIFFERS FROM** | `UNREADABLE … — the path is a symlink`; zero DIFFERS sections | +| symlink → byte-identical copy | **`No drift — 367 file(s) match`**, `--strict` **exit 0** | `UNREADABLE … — the path is a symlink`, `--strict` **1**, plain **0** | +| dangling symlink | **MISSING (expected but absent)** | `UNREADABLE … — the path is a symlink` | +| parent is a regular file (ENOTDIR) | **MISSING** | `UNREADABLE … — the path could not be inspected` | + +Fixture restored after each; final state `No drift — 367 file(s) match`, `--strict` exit 0. + +These are **evidence for the human**, not a floor verdict — no checker consumed them. + +--- + +The 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/diff-unreadable-partition/VERIFY.md b/.dev/features/diff-unreadable-partition/VERIFY.md new file mode 100644 index 0000000..d9f98a5 --- /dev/null +++ b/.dev/features/diff-unreadable-partition/VERIFY.md @@ -0,0 +1,62 @@ +# VERIFY — diff-unreadable-partition + +## FLOOR layer — the gates that own the verdict + +| gate | exit | +| --- | --- | +| `test` (`npm test` — the hermetic vitest suite, incl. this feature's own cases) | 0 | +| `validate` (`node .dev/floor/validate.mjs .`) | 0 | +| `lint` (`eslint src`) | 0 | +| `format:check` (prettier, whole-repo) | 0 | +| `lint:md` (markdownlint, whole-repo) | 0 | + +`structural:*` — **no gate.** This increment ships no markdown capability and therefore no +`evals/expected` ↔ `findings.json` pair, so no `structural:*` gate exists to run (absent from the map, +not silently passed). + +This set is exactly the repo's `npm run check` aggregate plus `validate`, so the verdict tracks the full +`check`. **Honest caveat (two clocks):** *which* gates are in the map is this stage's **advisory** +composition — `check-verify.mjs` is generic over gate keys and nothing floor-locks the style gates into +the set. + +## Suite detail (context, not verdict input) + +636 tests across 39 files pass, up from 625 at the baseline — **+11**, all in this increment: +7 in `tests/diff.test.ts` (directory, symlink-different-bytes, symlink-identical-bytes, dangling +symlink, ENOTDIR parent, sort determinism, the no-fs/no-crypto source scan) and 4 in +`tests/status.test.ts` (strict-exits-1-on-unreadable-alone, plain-run-reports-and-exits-0, subsection +ordering, omitted-when-empty). + +Coverage (`npm run test:coverage`, exit 0 against the configured thresholds 90/82/95/92): + +| file | lines | branch | stmts | funcs | +| --- | --- | --- | --- | --- | +| `src/lib/diff.ts` | 94.11% → **100%** | 75% → **100%** | 94.11% → **100%** | 100% | +| `src/commands/status.ts` | 97.01% → **97.26%** | 82.22% → **88.23%** | 94.59% → **96.29%** | 100% | +| repo total | 96.30% → **96.40%** | 88.01% → **88.48%** | 94.78% → **94.96%** | 98.14% (=) | + +No metric dropped anywhere. `diff.ts` reaching 100% required fixing a **pre-existing vacuous test**: +`tests/diff.test.ts`'s "flags a modified + a missing file" claimed a missing file but only rewrote it +with identical bytes, so the `missing` arm had never been exercised — at the baseline either. It now +deletes the file for real and asserts `missing`, which also stops the three new "not reported missing" +assertions from being vacuous. + +`apply-update.ts:66` (`'the file could not be read'`, the EACCES arm) stays uncovered, unchanged by this +increment. It is **not** in `diff.ts`'s own code — `diff.ts` has a single `unreadable` arm, fully +covered — and EACCES is not drivable as root. Stated rather than mocked around. + +## ADVISORY layer — verifiers + +**No verifiers registered — floor gates only.** `node .dev/floor/count-verifiers.mjs .` → +`{"registered":0,"verifiers":[]}` (deterministic frontmatter membership, never a prose grep). Step 2 is +a no-op; no verifier free-text exists, so nothing tainted entered this report. + +## Verdict (FLOOR — `.dev/floor/check-verify.mjs`, exit 0) + +**VERIFIED: floor gates PASS.** + +**The honest residual (P0/P7):** verified = **the named gates passed**. This is **not** a guarantee of +correctness beyond what those gates check — a defect no test, eval, rule, or lint covers is invisible to +this verdict, and the verifier layer that might have noticed it is advisory and today empty. In +particular, the four manual end-to-end evidences against a real upstream clone are **not** part of this +floor verdict; they are recorded separately in the ship roll-up. diff --git a/.dev/features/diff-unreadable-partition/regression-report.json b/.dev/features/diff-unreadable-partition/regression-report.json new file mode 100644 index 0000000..195d936 --- /dev/null +++ b/.dev/features/diff-unreadable-partition/regression-report.json @@ -0,0 +1,24 @@ +{ + "base": "8ff7240", + "inside": [ + "src/lib/diff.ts", + "src/commands/status.ts", + "tests/diff.test.ts", + "tests/status.test.ts", + "docs/commands/status.md", + "CHANGELOG.md" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/diff-unreadable-partition/verify-report.json b/.dev/features/diff-unreadable-partition/verify-report.json new file mode 100644 index 0000000..32887ce --- /dev/null +++ b/.dev/features/diff-unreadable-partition/verify-report.json @@ -0,0 +1,13 @@ +{ + "feature": "diff-unreadable-partition", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { "registered": 0, "findings": [] } +} diff --git a/.pharn/pharn-dev-verify/results.json b/.pharn/pharn-dev-verify/results.json index 27b910f..c1e0603 100644 --- a/.pharn/pharn-dev-verify/results.json +++ b/.pharn/pharn-dev-verify/results.json @@ -1 +1 @@ -{"test":0,"validate":0,"lint":0,"format:check":0,"lint:md":0,"floor-tests":0,"check-run-pins":0,"check-action-pins":0} \ No newline at end of file +{"test":0,"validate":0,"lint":0,"format:check":0,"lint:md":0} \ No newline at end of file diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index 919117d..8442b5c 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/publish-npm-floor-assert/SHIP.md" + ".dev/features/diff-unreadable-partition/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-08-10T19:43:58.416Z" + "set_at": "2026-08-11T10:01:02.609Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 0189d09..184b4f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **`pharn status` no longer crashes on a path it cannot read, and no longer misreports what sits + there.** The drift check read your project with its own bare `existsSync` / `readFileSync`, which + went wrong four ways. A **directory** where a file belongs threw `EISDIR` out of the middle of the + comparison, so status printed a raw errno naming no file and **the drift report for every other file + was lost** — one bad path took down the whole run. A **symlink** was read *through*: pointing it at a + file with other bytes listed the path under "differs", erasing the fact that a link — not an edit — + was the cause, and pointing it at a **byte-identical** file made status count it as matching and say + **nothing at all**, silently blessing a path that leads outside your install and can change under it + tomorrow. A **dangling** symlink was reported as "missing", when the truth is a link squats on the + path. And a path whose **parent is a regular file** was likewise reported "missing", when in fact it + cannot exist. All four are now a fourth drift category, **Unreadable**, listed by name with the + reason, while every other file still compares normally. `--strict` exits 1 on them like any other + drift; a plain `pharn status` still exits 0, because it is a report. This is the same classification + `pharn update` has always used to decide what it refuses to write over — so the read side and the + write side can no longer disagree about what a symlink at a pharn-owned path means. +- **One canonical sha256.** `lib/hash.ts` has always claimed a single implementation "so the drift + check (status), the install record store, and the update decision can never disagree" — while the + drift check quietly kept a private copy. It now uses the shared one, and a test holds the claim. + - **`pharn init` and `pharn update` no longer report success having done nothing off a TTY.** Both commands confirm before they write, and when stdin was not a terminal that confirmation cancelled on stream end and routed through the graceful-cancel path — `process.exit(0)`. So `echo "" | pharn update` diff --git a/docs/commands/status.md b/docs/commands/status.md index cdb640a..13246bf 100644 --- a/docs/commands/status.md +++ b/docs/commands/status.md @@ -6,7 +6,7 @@ from upstream? ```bash pharn status pharn status --no-drift # version check only (skips the clone) -pharn status --strict # exit 1 if outdated, modified, or missing (for CI) +pharn status --strict # exit 1 if outdated, differing, missing, or unreadable (for CI) ``` `status` is the read side of [`update`](update.md): it reports PHARN-owned files at your **recorded** @@ -41,7 +41,13 @@ which, and `status` cannot. exits early when already up to date. To install a capability **not yet** in `pharn.config.json`, use `pharn add` (additive-only — already-listed capabilities are a no-op, even if their files are missing); `add` also requires your install to match the current skills version. - - If neither, reports **No drift**. + - **Unreadable (not a regular readable file)** — expected paths that **exist** but cannot be + compared: a symlink (live *or* dangling), a directory, another non-regular file, an unreadable + file, or a path whose parent is a regular file. Each is listed with the reason. `status` cannot + compare these and `pharn update` skips them too — only you can resolve them, by inspecting the + path. A symlink is **never followed**: reading through one would report it as merely "differs" + when its target has other bytes, or say nothing at all when its target happens to match. + - If none of the three, reports **No drift**. The heading says "differs from", not "locally modified", on purpose: the comparison is against upstream `@main`, so a file can differ because **upstream moved**, not only because you edited it. @@ -60,8 +66,9 @@ preserves (never overwrites). The copied-verbatim trusted docs, `.cjs` hooks, `p ## Exit code -Exits `0` by default, even when drift or an available update is found (it is a report). Pass `--strict` -to exit `1` whenever anything is outdated, modified, or missing — useful as a CI gate. +Exits `0` by default, even when drift or an available update is found (it is a report) — including when +a path is unreadable. Pass `--strict` to exit `1` whenever anything is outdated, differing, missing, or +unreadable — useful as a CI gate. ## Related diff --git a/package-lock.json b/package-lock.json index 1e5c013..81d0567 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@pharn-dev/pharn", - "version": "0.3.2", + "version": "0.4.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@pharn-dev/pharn", - "version": "0.3.2", + "version": "0.4.0", "license": "Apache-2.0", "dependencies": { "@clack/prompts": "^1.7.0", @@ -24,6 +24,7 @@ "@vitest/coverage-v8": "^4.1.9", "esbuild": "^0.28.1", "eslint": "^10.7.0", + "globals": "^17.9.0", "markdownlint-cli2": "^0.23.1", "prettier": "^3.9.6", "tsx": "^4.23.1", @@ -1755,16 +1756,16 @@ } }, "node_modules/brace-expansion": { - "version": "5.0.7", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.7.tgz", - "integrity": "sha512-7oFy703dxfY3/NLxC1fh2SUCQ0H9rmAY+5EpDVfXjUTTs+HEwR2nYaqLv+GWcTsumwxPfiz6CzCNkwXwBUwqCA==", + "version": "5.0.9", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.9.tgz", + "integrity": "sha512-ScQ4IuvIEF1TMlP7Zt+vjJ//9zlPb2SDcxWxM3bk8s6t6GGdJ7KO1dCcTidOPJKePW30LE/2cT7wCyPho9/Wxg==", "dev": true, "license": "MIT", "dependencies": { "balanced-match": "^4.0.2" }, "engines": { - "node": "18 || 20 || >=22" + "node": "20 || >=22" } }, "node_modules/braces": { @@ -2380,6 +2381,19 @@ "node": ">=10.13.0" } }, + "node_modules/globals": { + "version": "17.9.0", + "resolved": "https://registry.npmjs.org/globals/-/globals-17.9.0.tgz", + "integrity": "sha512-m/MvAW61QVU5VDNF1Vj8axt016h8w7L5TU1e9zlab7XIttAT2YAlCwl75K1fOqvMM9apmD7lbCIRhpfkhmxhCg==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=18" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/globby": { "version": "16.2.2", "resolved": "https://registry.npmjs.org/globby/-/globby-16.2.2.tgz", @@ -3736,9 +3750,9 @@ "license": "MIT" }, "node_modules/nanoid": { - "version": "3.3.16", - "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.16.tgz", - "integrity": "sha512-bzlKTyNJ7+LdGIIwy8ijFpIqEQIvafahV7eYykJ8Cvh42EdJeODoJ6gUJXpQJvej1BddH8OqTXZNE/KfbWAu8Q==", + "version": "3.3.18", + "resolved": "https://registry.npmjs.org/nanoid/-/nanoid-3.3.18.tgz", + "integrity": "sha512-DTg4MJbGMWkfi6VZFdNt2/caMbQy4Ou+Op/hJQvGEWcnVfoA1QA+xzRKAzw9jD6+GVOOeYr/mIcuDSdug6F6+w==", "dev": true, "funding": [ { diff --git a/package.json b/package.json index cdbc062..8b17869 100644 --- a/package.json +++ b/package.json @@ -59,11 +59,12 @@ "@types/minimist": "^1.2.5", "@types/node": "^26.1.1", "@vitest/coverage-v8": "^4.1.9", + "esbuild": "^0.28.1", "eslint": "^10.7.0", + "globals": "^17.9.0", "markdownlint-cli2": "^0.23.1", "prettier": "^3.9.6", "tsx": "^4.23.1", - "esbuild": "^0.28.1", "typescript": "^6.0.3", "typescript-eslint": "^8.65.0", "vitest": "^4.1.10" diff --git a/src/commands/status.ts b/src/commands/status.ts index 9592ba2..1eec154 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -3,6 +3,7 @@ import pc from 'picocolors'; import { REPO, REPO_BRANCH } from '../lib/constants.js'; import { fetchRepo } from '../lib/repo.js'; import { diffInstalledCapabilities } from '../lib/diff.js'; +import type { InstallDiff } from '../lib/diff.js'; import { configLayout } from '../lib/layout.js'; import { row } from '../lib/format.js'; import { formatModelRoutingLines } from '../lib/model-routing-format.js'; @@ -93,7 +94,10 @@ async function runArchetypeStatus( printDriftSection(result); if ( strict && - (outdated || result.modified.length || result.missing.length) + (outdated || + result.modified.length || + result.missing.length || + result.unreadable.length) ) { exitCode = 1; } @@ -136,13 +140,15 @@ function printModelRouting(config: PharnConfig): void { note(formatModelRoutingLines(config.models).join('\n'), 'MODELS'); } -// DRIFT note: locally-modified and missing PHARN-owned files, or a clean bill. -function printDriftSection(result: { - modified: string[]; - missing: string[]; - okCount: number; -}): void { - if (result.modified.length === 0 && result.missing.length === 0) { +// DRIFT note: differing, missing and unreadable PHARN-owned files, or a clean +// bill. Takes the InstallDiff type rather than an inline structural literal, so +// a partition added to the diff cannot be silently left unrendered here. +function printDriftSection(result: InstallDiff): void { + if ( + result.modified.length === 0 && + result.missing.length === 0 && + result.unreadable.length === 0 + ) { note(`No drift — ${result.okCount} file(s) match ${REF}.`, 'DRIFT'); return; } @@ -179,6 +185,24 @@ function printDriftSection(result: { pc.dim(' (additive-only — already-listed capabilities are a no-op).'), ); } + if (result.unreadable.length) { + // Last, mirroring update's SKIP_ORDER: these are neither an edit nor an + // absence, and nothing pharn can run resolves them — only the user can. + // The heading is update's skip heading verbatim, because the write side + // refuses these exact paths for the exact same reason. + if (lines.length) lines.push(''); + lines.push(' UNREADABLE — not a regular readable file'); + for (const { rel, reason } of result.unreadable) { + lines.push(` ${rel} — ${reason}`); + } + lines.push( + pc.dim( + ' pharn cannot compare these, and `pharn update` skips them too.', + ), + pc.dim(' Inspect each path by hand — a directory, a symlink, or an'), + pc.dim(' unreadable file sits where pharn expects a regular file.'), + ); + } lines.push('', pc.dim(` ${result.okCount} file(s) match ${REF}.`)); note(lines.join('\n'), 'DRIFT'); } diff --git a/src/lib/diff.ts b/src/lib/diff.ts index 2f2fd17..a759ef6 100644 --- a/src/lib/diff.ts +++ b/src/lib/diff.ts @@ -1,7 +1,6 @@ -import { createHash } from 'node:crypto'; -import { existsSync, readFileSync } from 'node:fs'; +import { readDiskState } from './apply-update.js'; +import { sha256File } from './hash.js'; import { collectExpectedInstallPaths } from './install-manifest.js'; -import { safeJoin } from './validate.js'; import type { InstalledCapability, Layout } from '../types.js'; export interface InstallDiff { @@ -9,6 +8,14 @@ export interface InstallDiff { modified: string[]; // Expected by an installed module/skill but absent on disk. missing: string[]; + // Expected paths that EXIST but cannot be compared: a symlink (live OR + // dangling), a directory, another non-regular file, an unreadable file, or a + // path whose parent is a regular file. Reported by name with the reason, + // never folded into ok/modified/missing — a symlink read THROUGH would + // masquerade as `modified` (different bytes) or, worse, as `ok` (identical + // bytes), silently blessing a path that points outside the install. Sorted by + // `rel`. + unreadable: { rel: string; reason: string }[]; // Files present on disk and byte-identical to upstream. okCount: number; } @@ -20,8 +27,11 @@ export interface InstallDiff { * dirs + the fixed product surfaces at the recorded layout); this byte-compares * each entry against `projectRoot`. `.claude/settings.json` is user-owned * (preserved at install) and is excluded by the manifest; the copied-verbatim - * trusted docs, hooks, contracts, and floor checkers ARE compared. Every read is - * safeJoin-guarded. + * trusted docs, hooks, contracts, and floor checkers ARE compared. + * + * Every read stays safeJoin-contained, though this module no longer calls + * safeJoin itself: the project side is contained by `readDiskState` + * (lib/apply-update.ts) and the clone side by `collectExpectedInstallPaths`. */ export function diffInstalledCapabilities(params: { repoDir: string; @@ -42,20 +52,36 @@ export function diffInstalledCapabilities(params: { return compareExpected(expected, projectRoot); } -// Byte-compare each expected file against `baseDir`, partitioning into -// modified / missing / ok. safeJoin-guarded. +// Compare each expected file against `baseDir`, partitioning into +// modified / missing / unreadable / ok. +// +// The PROJECT side goes through `readDiskState` — the same total classifier +// `pharn update` writes against, so the read and the write can never disagree +// about what a symlink (or a directory, or an ENOTDIR parent) at an owned path +// means. It never throws, so one unreadable path can no longer collapse the +// whole report. +// +// The CLONE side is deliberately NOT symmetric: a plain `sha256File`. The +// manifest only ever emits paths it existsSync-verified in a fresh private temp +// clone THIS run, and its walkers exclude symlinks — so a clone-side read +// failure means the clone corrupted mid-run, which is genuinely exceptional and +// keeps its existing behavior of surfacing through runStatus's catch. There is +// no fifth state. function compareExpected( expected: Map, baseDir: string, ): InstallDiff { const modified: string[] = []; const missing: string[] = []; + const unreadable: { rel: string; reason: string }[] = []; let okCount = 0; for (const [rel, repoPath] of expected) { - const diskPath = safeJoin(baseDir, rel); - if (!existsSync(diskPath)) { + const state = readDiskState(baseDir, rel); + if (state.kind === 'absent') { missing.push(rel); - } else if (hash(repoPath) === hash(diskPath)) { + } else if (state.kind === 'unreadable') { + unreadable.push({ rel, reason: state.reason }); + } else if (state.hash === sha256File(repoPath)) { okCount += 1; } else { modified.push(rel); @@ -63,9 +89,11 @@ function compareExpected( } modified.sort(); missing.sort(); - return { modified, missing, okCount }; -} - -function hash(path: string): string { - return createHash('sha256').update(readFileSync(path)).digest('hex'); + // Code-unit comparison, matching the `.sort()` default above. NOT + // localeCompare, which is locale-dependent and would order the report + // differently across machines (P5). Written branch-free rather than as a + // ternary chain so there is no equality arm that map-key uniqueness makes + // unreachable — this yields -1 / 0 / 1 arithmetically. + unreadable.sort((a, b) => Number(a.rel > b.rel) - Number(a.rel < b.rel)); + return { modified, missing, unreadable, okCount }; } diff --git a/tests/diff.test.ts b/tests/diff.test.ts index 65a7ee1..37016e1 100644 --- a/tests/diff.test.ts +++ b/tests/diff.test.ts @@ -1,5 +1,12 @@ -import { mkdirSync, writeFileSync } from 'node:fs'; +import { + mkdirSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; import { useTmpDir } from './helpers.js'; import { diffInstalledCapabilities } from '../src/lib/diff.js'; @@ -60,6 +67,7 @@ describe('diffInstalledCapabilities', () => { }); expect(r.missing).toEqual([]); expect(r.modified).toEqual([]); + expect(r.unreadable).toEqual([]); // a11y, n-plus-one, pharn-plan, enforce.cjs, CONSTITUTION, contract, floor. expect(r.okCount).toBe(7); }); @@ -69,9 +77,12 @@ describe('diffInstalledCapabilities', () => { const proj = join(tmp.path(), 'proj'); scaffoldClone(repo); scaffoldMatchingProject(proj); - // Modify one capability file, remove another expected file. + // Modify one capability file, remove another expected file. The removal is + // real: this case named a missing file but only ever REWROTE it with the + // same bytes, so the `missing` arm went unexercised and the three unreadable + // cases below that assert "not reported missing" would have been vacuous. write(join(proj, 'pharn-pipeline/grillers/a11y/a11y.md'), 'A-EDITED'); - write(join(proj, 'pharn-review/n-plus-one/n-plus-one.md'), 'N'); // keep + rmSync(join(proj, 'pharn-review/n-plus-one/n-plus-one.md')); write(join(proj, 'CONSTITUTION.md'), 'C-EDITED'); // modified doc const r = diffInstalledCapabilities({ @@ -82,6 +93,8 @@ describe('diffInstalledCapabilities', () => { }); expect(r.modified).toContain('pharn-pipeline/grillers/a11y/a11y.md'); expect(r.modified).toContain('CONSTITUTION.md'); + expect(r.missing).toEqual(['pharn-review/n-plus-one/n-plus-one.md']); + expect(r.unreadable).toEqual([]); const paths = [...r.modified, ...r.missing]; expect(paths).not.toContain('.claude/settings.json'); expect(paths.some((p) => p.includes('pharn-dev-'))).toBe(false); @@ -156,4 +169,167 @@ describe('diffInstalledCapabilities', () => { expect(all.some((p) => p.startsWith('pharn/'))).toBe(false); expect(r.okCount).toBeGreaterThanOrEqual(2); }); + + // ------------------------------------------------------------------------- + // The `unreadable` partition. Before it, every case below either CRASHED the + // whole run or was reported as something it was not. `readDiskState` (the + // classifier `pharn update` already writes against) is what makes the read + // side and the write side agree; these cases are the read-side twins of + // tests/apply-update.test.ts's `readDiskState` block. + // ------------------------------------------------------------------------- + describe('unreadable — paths that exist but cannot be compared', () => { + // A clone + a matching project, minus the one path each case sabotages. + function fixture(): { repo: string; proj: string } { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + scaffoldClone(repo); + scaffoldMatchingProject(proj); + return { repo, proj }; + } + const run = (repo: string, proj: string) => + diffInstalledCapabilities({ + repoDir: repo, + projectRoot: proj, + capabilities: caps, + layout: 'flat', + }); + // The 7-file matching set, minus the one path under test: what must STILL + // compare cleanly while the sabotaged path is reported. + const REST = 6; + + it('a DIRECTORY at an expected path is reported — it no longer collapses the run', () => { + // The live evidence: `mkdir` over pharn/CONSTITUTION.md made readFileSync + // throw EISDIR, status print a raw errno naming no file, and the drift + // report for every OTHER file vanish. + const { repo, proj } = fixture(); + rmSync(join(proj, 'CONSTITUTION.md')); + mkdirSync(join(proj, 'CONSTITUTION.md'), { recursive: true }); + + expect(() => run(repo, proj)).not.toThrow(); + const r = run(repo, proj); + + expect(r.unreadable.map((u) => u.rel)).toEqual(['CONSTITUTION.md']); + expect(r.unreadable[0]!.reason).toBeTruthy(); + // ANTI-COLLAPSE: every other expected file still got compared. + expect(r.okCount).toBe(REST); + expect(r.modified).toEqual([]); + expect(r.missing).toEqual([]); + }); + + it('a symlink to DIFFERENT bytes → unreadable, never `modified`', () => { + // It used to be read THROUGH and listed under Modified, indistinguishable + // from a genuine content edit — the reason (a link sits there) erased. + const { repo, proj } = fixture(); + write(join(proj, 'elsewhere.md'), 'NOT-THE-CONSTITUTION'); + rmSync(join(proj, 'CONSTITUTION.md')); + symlinkSync(join(proj, 'elsewhere.md'), join(proj, 'CONSTITUTION.md')); + + const r = run(repo, proj); + + expect(r.unreadable.map((u) => u.rel)).toEqual(['CONSTITUTION.md']); + // The reason NAMES the link: that is the whole honesty claim here. + expect(r.unreadable[0]!.reason).toMatch(/symlink/); + expect(r.modified).toEqual([]); + expect(r.okCount).toBe(REST); + }); + + it('a symlink to BYTE-IDENTICAL content → unreadable, and NOT counted ok', () => { + // The sharpest case: status used to count it ok and never mention it — + // silently blessing a path that points outside the install and can change + // under it tomorrow. okCount is asserted, not just membership: counting + // it would leave every other assertion here satisfied. + const { repo, proj } = fixture(); + write(join(proj, 'identical-copy.md'), 'C'); // the clone's exact bytes + rmSync(join(proj, 'CONSTITUTION.md')); + symlinkSync( + join(proj, 'identical-copy.md'), + join(proj, 'CONSTITUTION.md'), + ); + + const r = run(repo, proj); + + expect(r.unreadable.map((u) => u.rel)).toEqual(['CONSTITUTION.md']); + expect(r.unreadable[0]!.reason).toMatch(/symlink/); + expect(r.okCount).toBe(REST); // NOT REST + 1 + expect(r.modified).toEqual([]); + expect(r.missing).toEqual([]); + }); + + it('a DANGLING symlink → unreadable, not `missing`', () => { + // existsSync follows the link and returns false, so the old report said + // the file was absent when the truth is a link squats on the path. + const { repo, proj } = fixture(); + rmSync(join(proj, 'CONSTITUTION.md')); + symlinkSync(join(proj, 'ghost-target.md'), join(proj, 'CONSTITUTION.md')); + + const r = run(repo, proj); + + expect(r.unreadable.map((u) => u.rel)).toEqual(['CONSTITUTION.md']); + expect(r.unreadable[0]!.reason).toMatch(/symlink/); + expect(r.missing).toEqual([]); + expect(r.okCount).toBe(REST); + }); + + it('a path whose PARENT is a regular file (ENOTDIR) → unreadable, not `missing`', () => { + // existsSync returns false without throwing here too, so this read as + // "absent" when the truth is the path cannot exist. + const { repo, proj } = fixture(); + rmSync(join(proj, 'pharn-pipeline/grillers/a11y'), { recursive: true }); + write( + join(proj, 'pharn-pipeline/grillers/a11y'), + 'a file where a directory belongs', + ); + + const r = run(repo, proj); + + expect(r.unreadable.map((u) => u.rel)).toEqual([ + 'pharn-pipeline/grillers/a11y/a11y.md', + ]); + expect(r.unreadable[0]!.reason).toBeTruthy(); + expect(r.missing).toEqual([]); + expect(r.okCount).toBe(REST); + }); + + it('sorts `unreadable` by rel, independent of manifest order (P5)', () => { + // The manifest emits capability dirs BEFORE the trusted docs, so these two + // arrive in the opposite order to the one asserted — the sort is doing + // real work, not agreeing with iteration order by luck. + const { repo, proj } = fixture(); + for (const rel of [ + 'pharn-pipeline/grillers/a11y/a11y.md', + 'CONSTITUTION.md', + ]) { + rmSync(join(proj, rel)); + mkdirSync(join(proj, rel), { recursive: true }); + } + + const r = run(repo, proj); + + expect(r.unreadable.map((u) => u.rel)).toEqual([ + 'CONSTITUTION.md', + 'pharn-pipeline/grillers/a11y/a11y.md', + ]); + }); + }); + + // The canonical-hash claim in src/lib/hash.ts ("one canonical implementation + // so the drift check (status), the install record store, and the update + // decision can never disagree") names diff.ts by name — and diff.ts forked it + // anyway with a private sha256 and its own fs reads. That claim was prose; + // this makes it checked, so the fork cannot quietly return. + it('owns no disk or hash primitives of its own — every read goes through lib/', () => { + const here = fileURLToPath(import.meta.url); + const code = readFileSync( + join(here, '..', '..', 'src', 'lib', 'diff.ts'), + 'utf8', + ) + // Comments legitimately NAME these (explaining what moved and why), so + // strip them: the assertion is about imports and calls, not prose. + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/.*$/gm, ''); + + expect(code).not.toMatch(/from\s+'node:fs'/); + expect(code).not.toMatch(/from\s+'node:crypto'/); + expect(code).not.toMatch(/\b(existsSync|readFileSync|createHash)\b/); + }); }); diff --git a/tests/status.test.ts b/tests/status.test.ts index 875d464..54a1f14 100644 --- a/tests/status.test.ts +++ b/tests/status.test.ts @@ -49,7 +49,12 @@ function noteBody(title: string): string { return (call?.[0] as string | undefined) ?? ''; } -const CLEAN = { modified: [] as string[], missing: [] as string[], okCount: 5 }; +const CLEAN = { + modified: [] as string[], + missing: [] as string[], + unreadable: [] as { rel: string; reason: string }[], + okCount: 5, +}; describe('runStatus (archetype)', () => { stubProcessExit(); @@ -138,6 +143,7 @@ describe('runStatus (archetype)', () => { diffInstalledCapabilities.mockReturnValue({ modified: ['pharn-pipeline/grillers/a11y/a11y.md'], missing: [], + unreadable: [], okCount: 3, }); @@ -158,6 +164,7 @@ describe('runStatus (archetype)', () => { diffInstalledCapabilities.mockReturnValue({ modified: ['CONSTITUTION.md'], missing: [], + unreadable: [], okCount: 3, }); @@ -174,6 +181,7 @@ describe('runStatus (archetype)', () => { diffInstalledCapabilities.mockReturnValue({ modified: ['CONSTITUTION.md'], missing: [], + unreadable: [], okCount: 3, }); @@ -195,6 +203,7 @@ describe('runStatus (archetype)', () => { diffInstalledCapabilities.mockReturnValue({ modified: [], missing: ['.claude/hooks/set-writes-scope.cjs'], + unreadable: [], okCount: 3, }); @@ -215,6 +224,7 @@ describe('runStatus (archetype)', () => { diffInstalledCapabilities.mockReturnValue({ modified: ['pharn-pipeline/grillers/a11y/a11y.md'], missing: [], + unreadable: [], okCount: 3, }); @@ -222,6 +232,87 @@ describe('runStatus (archetype)', () => { expect(cleanup).toHaveBeenCalledTimes(1); }); + // The fourth partition: expected paths that EXIST but cannot be compared (a + // symlink, a directory, an ENOTDIR parent). status used to either crash on + // these or file them under Modified/Missing; now it names them, and --strict + // counts them like any other drift. + const UNREADABLE = { + modified: [] as string[], + missing: [] as string[], + unreadable: [{ rel: 'CONSTITUTION.md', reason: 'the path is a symlink' }], + okCount: 3, + }; + + it('--strict exits 1 when the ONLY drift is unreadable, cleaning up first', async () => { + const cleanup = vi.fn(); + fetchRepo.mockResolvedValue({ dir: '/repo', cleanup }); + readSkillsVersion.mockReturnValue('1.0.0'); + diffInstalledCapabilities.mockReturnValue(UNREADABLE); + + await expect(runStatus({ strict: true })).rejects.toMatchObject( + new ProcessExit(1), + ); + expect(cleanup).toHaveBeenCalled(); + }); + + it('plain status REPORTS unreadable paths with their reason and exits 0', async () => { + // Report-only, exactly like modified/missing: status is a report, not a + // guard. The reason is rendered because "a link sits there" is the whole + // point — without it this is indistinguishable from an edit. + fetchRepo.mockResolvedValue({ dir: '/repo', cleanup: vi.fn() }); + readSkillsVersion.mockReturnValue('1.0.0'); + diffInstalledCapabilities.mockReturnValue(UNREADABLE); + + await expect(runStatus({})).resolves.toBeUndefined(); + + const drift = noteBody('DRIFT'); + expect(drift).toContain('UNREADABLE'); + expect(drift).toContain('CONSTITUTION.md — the path is a symlink'); + // Not a clean bill: an unreadable path is drift. + expect(drift).not.toContain('No drift'); + }); + + it('orders the drift subsections DIFFERS → MISSING → UNREADABLE', async () => { + // Deterministic report order, the read-side twin of update's SKIP_ORDER — + // unreadable last, because nothing pharn can run resolves it. + fetchRepo.mockResolvedValue({ dir: '/repo', cleanup: vi.fn() }); + readSkillsVersion.mockReturnValue('1.0.0'); + diffInstalledCapabilities.mockReturnValue({ + modified: ['CONSTITUTION.md'], + missing: ['.claude/hooks/set-writes-scope.cjs'], + unreadable: [ + { + rel: 'pharn-contracts/finding-shape.md', + reason: 'the path is a symlink', + }, + ], + okCount: 3, + }); + + await runStatus({}); + + const drift = noteBody('DRIFT'); + expect(drift.indexOf('DIFFERS FROM')).toBeLessThan( + drift.indexOf('MISSING'), + ); + expect(drift.indexOf('MISSING')).toBeLessThan(drift.indexOf('UNREADABLE')); + }); + + it('omits the UNREADABLE subsection entirely when nothing is unreadable', async () => { + fetchRepo.mockResolvedValue({ dir: '/repo', cleanup: vi.fn() }); + readSkillsVersion.mockReturnValue('1.0.0'); + diffInstalledCapabilities.mockReturnValue({ + modified: ['CONSTITUTION.md'], + missing: [], + unreadable: [], + okCount: 3, + }); + + await runStatus({}); + + expect(noteBody('DRIFT')).not.toContain('UNREADABLE'); + }); + it('MODELS note renders the per-stage routing from config.models', async () => { loadArchetypeConfigOrExit.mockReturnValue( config({