From c45710a4c2f492e0f8e76676c4ade1f9f1acbbb3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 08:17:21 +0000 Subject: [PATCH] feat(status,update): report upstream hooks missing from settings.json (PHARN-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream re-wired its hooks twice (6.1.0 anchored both guards on CLAUDE_PROJECT_DIR because the relative form was silently off after a cd; 6.12.0 added an exec-form Stop hook). `update` installed the new hook scripts but never looks at settings.json (user-owned, never overwritten), and `status --strict` stayed green — so existing installs kept dead or broken wiring with no signal at all. New lib/hook-wiring.ts compares the `hooks` block of the project's settings.json with the fetched upstream one (exact Event/matcher/command/args set difference; both files size-capped, symlink-refused, parsed as data): - `status` prints a HOOKS section and `--strict` exits 1 while an upstream hook is unwired (extra user hooks never count); - `update` prints the same HOOKS note — and still never writes the file. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc --- .dev/features/hook-wiring-drift/GRILL.md | 33 +++ .dev/features/hook-wiring-drift/PLAN.md | 71 +++++++ .dev/features/hook-wiring-drift/REGRESSION.md | 30 +++ .dev/features/hook-wiring-drift/REVIEW.md | 45 ++++ .dev/features/hook-wiring-drift/SHIP.md | 17 ++ .dev/features/hook-wiring-drift/VERIFY.md | 27 +++ .../hook-wiring-drift/regression-report.json | 27 +++ .../hook-wiring-drift/verify-report.json | 17 ++ .pharn/writes-scope.json | 4 +- CLAUDE.md | 2 +- docs/commands/status.md | 10 +- docs/commands/update.md | 5 +- src/commands/status.ts | 12 +- src/commands/update.ts | 15 +- src/lib/hook-wiring.ts | 195 +++++++++++++++++ tests/hook-wiring.test.ts | 197 ++++++++++++++++++ tests/status.test.ts | 71 ++++++- tests/update.test.ts | 53 +++++ 18 files changed, 819 insertions(+), 12 deletions(-) create mode 100644 .dev/features/hook-wiring-drift/GRILL.md create mode 100644 .dev/features/hook-wiring-drift/PLAN.md create mode 100644 .dev/features/hook-wiring-drift/REGRESSION.md create mode 100644 .dev/features/hook-wiring-drift/REVIEW.md create mode 100644 .dev/features/hook-wiring-drift/SHIP.md create mode 100644 .dev/features/hook-wiring-drift/VERIFY.md create mode 100644 .dev/features/hook-wiring-drift/regression-report.json create mode 100644 .dev/features/hook-wiring-drift/verify-report.json create mode 100644 src/lib/hook-wiring.ts create mode 100644 tests/hook-wiring.test.ts diff --git a/.dev/features/hook-wiring-drift/GRILL.md b/.dev/features/hook-wiring-drift/GRILL.md new file mode 100644 index 00000000..deedcbba --- /dev/null +++ b/.dev/features/hook-wiring-drift/GRILL.md @@ -0,0 +1,33 @@ +# GRILL — hook-wiring-drift + +Plan: `.dev/features/hook-wiring-drift/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live +`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +```yaml +- type: FINDING + rule_id: 'P5' + severity: important + file: '.dev/features/hook-wiring-drift/PLAN.md:25' + problem: 'Upstream 6.12.0 ships an EXEC-form hook (`command` + `args[]`, no shell). A key built from `command` alone would make every exec-form hook with the same binary (`node`) collide; the normalized entry must include `args`.' + evidence: 'normalizes `hooks.[].{matcher, hooks[].command}`' +- type: FINDING + rule_id: 'P7' + severity: important + file: '.dev/features/hook-wiring-drift/PLAN.md:31' + problem: 'Making `status --strict` exit 1 on missing upstream hooks turns currently-green CI gates red for every install predating 6.1.0/6.12.0 until the user edits settings.json by hand — intended by the finding, but a user-visible behavior change that belongs in the docs and CHANGELOG.' + evidence: '`--strict` exits 1 when upstream hooks are missing' +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/hook-wiring-drift/PLAN.md:57' + problem: 'The equality is purely textual: a user who rewrote the same hook with different quoting is reported as missing. The plan labels this advisory; the note text should say so.' + evidence: 'a differing command string is reported as missing — advisory to the human' +``` + +## Summary + +Report-only design keeps the settings.json invariant. Key normalization must cover exec-form `args`. + +ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 3 advisory) — for the human to weigh before /pharn-dev-build. diff --git a/.dev/features/hook-wiring-drift/PLAN.md b/.dev/features/hook-wiring-drift/PLAN.md new file mode 100644 index 00000000..3c01a0e4 --- /dev/null +++ b/.dev/features/hook-wiring-drift/PLAN.md @@ -0,0 +1,71 @@ +# PLAN — hook-wiring-drift (PHARN-04: upstream hook-wiring changes must be reported, not silent) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: a pure `diffHookWiring` compares the `hooks` block of the project's `.claude/settings.json` + with the fetched upstream one and returns the upstream hook entries (event + matcher + command) the + project lacks; `update` prints a `HOOKS` note naming them, and `status` prints a `HOOKS` section and + counts missing upstream hooks toward `--strict`. `settings.json` itself is still NEVER written. +- layer(s): the CLI itself (`src/lib/hook-wiring.ts` new, `src/commands/{update,status}.ts`) +- constitution_refs: [P0, P1, P2, P3, P4, P5, P7] + +## Discovery — verified this run (P6) + +Reproduced in the review (`repro/hook-wiring`): `init` at upstream 6.0.0 then `update --yes` to 6.17.1 → +"updated 35 · restored 33 · skipped 0", `.claude/settings.json` byte-identical (relative commands, no +`Stop` hook although `require-loop-record.cjs` was installed), and `status --strict` exit 0 "No drift". +Upstream changed the wiring twice (6.1.0 `CLAUDE_PROJECT_DIR`-anchored commands; 6.12.0 new `Stop` hook). +Live code: `install-capabilities.ts` copies `settings.json` only when absent; `install-manifest.ts` +excludes it, so neither `update` nor `status` ever looks at it. Not overwriting it is documented and stays. + +## Files + +- `src/lib/hook-wiring.ts` — `diffHookWiring(repoDir, projectRoot)`: reads both `.claude/settings.json` + (`safeJoin` + `findSymlinkComponent` + regular-file + 256 KB cap, JSON parse, all failures → a named + `unreadable`/`absent` status, never a throw); normalizes `hooks.[].{matcher, hooks[].command}` into + `Event · matcher · command` entries; returns `{ status: 'match' | 'missing' | 'no-upstream' | +'project-absent' | 'unreadable', missing: HookEntry[] }`. Command strings are display-sanitized + (control characters replaced). Pure of `process`/console — layer CLI/lib +- `src/commands/update.ts` — after a successful apply, compute it against the clone and print a + `HOOKS` note when `missing` is non-empty (or the project file is unreadable), naming the entries and + that `settings.json` is never written by pharn — layer CLI/command +- `src/commands/status.ts` — `HOOKS` note in the drift path; `--strict` exits 1 when upstream hooks are + missing (extra user hooks never count) — layer CLI/command +- `tests/hook-wiring.test.ts` — match; missing Stop hook; changed command string counts as missing; + extra user hooks ignored; project absent / unparseable / symlinked / oversized; upstream absent; + control characters sanitized +- `tests/update.test.ts` — old wiring → HOOKS note printed, settings.json byte-identical +- `tests/status.test.ts` — old wiring → HOOKS section; `--strict` exits 1; matching wiring → exit 0 +- `docs/commands/update.md` — the protected-files section: `update` reports hook-wiring differences (P4) +- `docs/commands/status.md` — HOOKS section + `--strict` semantics (P4) +- `CLAUDE.md` — status/update paragraphs (P4) + +## Contracts satisfied + +- `docs/commands/update.md` "What is protected by default": `settings.json` stays user-owned (unchanged); + the defect was the silence, now closed by a report. + +## Evals to write (P1) + +- per test file above; the lib cases and the `status --strict` exit must fail on the base source. + +## Guarantee audit (P0) + +- "a missing upstream hook is reported by `status` and fails `--strict`" → floor: set membership over + normalized `Event·matcher·command` strings (exact string compare). +- "pharn never writes `.claude/settings.json` on update" → unchanged; asserted byte-identical in tests. +- What the wiring MEANS (whether a user's differing command is equivalent) → not judged; a differing + command string is reported as missing — advisory to the human, named in the note text. + +## Trust audit (P2) + +- Upstream `settings.json` is untrusted remote content; the project's is local user content. Both are + parsed as DATA only (never executed), size-capped, symlink-refused, and only command strings reach the + terminal — sanitized of control characters. + +## Determinism audit (P5) + +- Exact-string set difference; an unparseable file is a named status, never a guess. + +## Open questions (HALT) + +- none diff --git a/.dev/features/hook-wiring-drift/REGRESSION.md b/.dev/features/hook-wiring-drift/REGRESSION.md new file mode 100644 index 00000000..2672e7e6 --- /dev/null +++ b/.dev/features/hook-wiring-drift/REGRESSION.md @@ -0,0 +1,30 @@ +# REGRESSION — hook-wiring-drift + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `71bc375143fb9666ea6b1725ecbd7612fe774f9e` (`origin/main` at build time; the build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `CLAUDE.md`, `docs/commands/status.md`, `docs/commands/update.md`, `src/commands/status.ts`, `src/commands/update.ts`, `src/lib/hook-wiring.ts`, `tests/hook-wiring.test.ts`, `tests/status.test.ts`, `tests/update.test.ts`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook scratch) and this + feature's own stage artifacts are not build output. +- **outside gates:** the stdlib `*.test.mjs` / `*.test.cjs` files + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / `lint:md` are absent from both maps. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` is +owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never the increment. diff --git a/.dev/features/hook-wiring-drift/REVIEW.md b/.dev/features/hook-wiring-drift/REVIEW.md new file mode 100644 index 00000000..5f381c00 --- /dev/null +++ b/.dev/features/hook-wiring-drift/REVIEW.md @@ -0,0 +1,45 @@ +# REVIEW — hook-wiring-drift + +Floor first: `node .dev/floor/validate.mjs .` → exit 0 (GREEN). Everything below is **advisory**. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0):** "a missing upstream hook is reported and fails `status --strict`" reduces to an + exact-string set difference; "update never writes settings.json" is asserted byte-identical in a test. + Textual equality is labeled in the user-facing note. +- **L-eval (P1):** 13 lib cases (6.0.0→6.17.1 wiring, exec-form `args`, extra user hooks, absent / + invalid JSON / oversized / symlinked project file, no usable upstream, control-char sanitizing, note + text) + 3 `status` cases + 2 `update` cases; the command cases fail on the base source. +- **L-trust (P2):** both files are size-capped (256 KB), symlink-refused, parsed as data; only command + strings reach the terminal, with C0/C1 control characters replaced. +- **L-axis (P3):** one new lib module with one axis (hook-wiring comparison); `status`/`update` only + consume it. + +## Advisory findings + +```yaml +- type: FINDING + rule_id: 'P7' + severity: important + file: 'src/commands/status.ts' + problem: "`status --strict` now exits 1 for every install whose settings.json predates upstream's 6.1.0/6.12.0 re-wiring, until the user merges the hooks by hand — the intended fix, but CI pipelines using --strict will turn red on upgrade. Needs a CHANGELOG line." + evidence: 'hookLines !== null' +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/commands/update.ts' + problem: "`update`'s same-version early return never clones, so the HOOKS note only appears on runs that actually fetch; `status` is the continuous check." + evidence: 'return { ...applied, hookWiring: diffHookWiring(repo.dir, cwd) };' +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:8' + problem: "No CHANGELOG `[Unreleased]` entry (not in the plan's `## Files`)." + evidence: '## [Unreleased]' +``` + +## Verdict + +**GREEN** — 0 floor-gate findings, 3 advisory findings. No lesson proposed for canon. diff --git a/.dev/features/hook-wiring-drift/SHIP.md b/.dev/features/hook-wiring-drift/SHIP.md new file mode 100644 index 00000000..11c1f55d --- /dev/null +++ b/.dev/features/hook-wiring-drift/SHIP.md @@ -0,0 +1,17 @@ +# SHIP — hook-wiring-drift + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: **Approve as written**) → `/pharn-dev-grill` → +`/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review` → GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- Run ended at **GATE 2**. The human's standing instruction for this batch: open a PR and merge it only if + its CI checks are green. + +chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good or +wise; that is the human's call at the post-review gate. diff --git a/.dev/features/hook-wiring-drift/VERIFY.md b/.dev/features/hook-wiring-drift/VERIFY.md new file mode 100644 index 00000000..6824c03e --- /dev/null +++ b/.dev/features/hook-wiring-drift/VERIFY.md @@ -0,0 +1,27 @@ +# VERIFY — hook-wiring-drift + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, as a non-root user on node 22 with the session +proxy variables unset (as root with the proxy set, 5 pre-existing tests in `init.test.ts` / +`update.test.ts` fail for environmental reasons, identically at the baseline). + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +No `structural:*` gate — the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — floor gates only. + +Residual (P0/P7): "verified" means the named gates passed — not that the feature is correct in any sense +the suite does not encode. diff --git a/.dev/features/hook-wiring-drift/regression-report.json b/.dev/features/hook-wiring-drift/regression-report.json new file mode 100644 index 00000000..11c1a051 --- /dev/null +++ b/.dev/features/hook-wiring-drift/regression-report.json @@ -0,0 +1,27 @@ +{ + "base": "71bc375143fb9666ea6b1725ecbd7612fe774f9e", + "inside": [ + "CLAUDE.md", + "docs/commands/status.md", + "docs/commands/update.md", + "src/commands/status.ts", + "src/commands/update.ts", + "src/lib/hook-wiring.ts", + "tests/hook-wiring.test.ts", + "tests/status.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/hook-wiring-drift/verify-report.json b/.dev/features/hook-wiring-drift/verify-report.json new file mode 100644 index 00000000..da46b1a4 --- /dev/null +++ b/.dev/features/hook-wiring-drift/verify-report.json @@ -0,0 +1,17 @@ +{ + "feature": "hook-wiring-drift", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index c774326e..c7ad7c95 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/lock-read-modify-write/SHIP.md" + ".dev/features/hook-wiring-drift/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T08:06:32.625Z" + "set_at": "2026-09-24T08:17:20.630Z" } diff --git a/CLAUDE.md b/CLAUDE.md index a7efbfd9..2cdedaff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -65,7 +65,7 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten **`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 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; 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. +**`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`, both files size-capped + symlink-refused, command strings control-char-sanitized for display); upstream hooks the project does not wire print a `HOOKS` note in `status` (and fail `--strict`) and in `update` (report only — `update` still never writes the file); 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. ## Testing diff --git a/docs/commands/status.md b/docs/commands/status.md index 388590b8..31c1edf5 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, differing, missing, or unreadable (for CI) +pharn status --strict # exit 1 if outdated, differing, missing, unreadable, or a hook is unwired (for CI) ``` `status` is the read side of [`update`](update.md): it reports PHARN-owned files at your **recorded** @@ -74,8 +74,10 @@ a whole tree to relocate. ## What is intentionally excluded -`.claude/settings.json` is **never** flagged — it is your Claude Code configuration, which the install -preserves (never overwrites). The copied-verbatim trusted docs, `.cjs` hooks, `pharn/features/README.md`, +`.claude/settings.json` is **never** flagged as a differing file — it is your Claude Code +configuration, which the install preserves (never overwrites). Its `hooks` block is checked separately: +a `HOOKS` section names every hook the upstream `settings.json` wires that yours does not (extra hooks +of your own never count; matching is textual), and `--strict` exits `1` while any is missing. The copied-verbatim trusted docs, `.cjs` hooks, `pharn/features/README.md`, pharn's `LICENSE` copy, and the contracts, `pharn-core` and floor dirs at your recorded layout (`pharn/pharn-contracts/`, `pharn/pharn-core/`, `pharn/floor/`, or `pharn-contracts/`, `pharn-core/`, `.dev/floor/` when it is flat) **are** compared, so an edit to any of those surfaces shows up as @@ -86,7 +88,7 @@ never compared. 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. +unreadable, or an upstream hook is not wired in `.claude/settings.json` — useful as a CI gate. `--strict` governs **findings**, not failures. `status` still exits `1` without it when it cannot produce a report at all: no `pharn.config.json` (or a pre-archetype one), a clone it cannot fetch, or diff --git a/docs/commands/update.md b/docs/commands/update.md index 5bc4a22e..9afefd68 100644 --- a/docs/commands/update.md +++ b/docs/commands/update.md @@ -300,7 +300,10 @@ command addresses it any more, and the update prints its own warning naming it. ## What is protected by default - `.claude/settings.json` — your Claude Code configuration. `init` writes it only when absent; `update` - **never** touches it at all (not even with `--force` — it is not in the install manifest). + **never** touches it at all (not even with `--force` — it is not in the install manifest). It does + compare the file's `hooks` block with the fetched upstream one and prints a `HOOKS` note naming every + hook upstream wires that yours does not (matching is textual, so an equivalent hook you wrote + differently is listed too); merge those by hand. - `pharn/CONSTITUTION.md` (flat: `CONSTITUTION.md`) — protected like every other manifest path: if you have edited it, it is a `modified` skip by default. `--force` overwrites it too, after copying the current bytes to `.pharn-backup/`. (Before 0.4.0 `update` silently overwrote a hand-edited constitution despite docs claiming otherwise — diff --git a/src/commands/status.ts b/src/commands/status.ts index f8b5147f..14c8b6aa 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -1,3 +1,4 @@ +import { diffHookWiring, hookWiringLines } from '../lib/hook-wiring.js'; import { intro, log, note, outro, spinner } from '@clack/prompts'; import pc from 'picocolors'; import { REPO, REPO_BRANCH } from '../lib/constants.js'; @@ -30,7 +31,8 @@ const REF = `${REPO}@${REPO_BRANCH}`; * * Default clones the repo once and reuses it for both sections. `--no-drift` * skips the clone and only checks the version (via the lightweight SKILLS_VERSION - * fetch). `--strict` exits 1 when anything is outdated, modified, or missing. + * fetch). `--strict` exits 1 when anything is outdated, modified, or missing, + * or when a hook upstream wires is not wired in `.claude/settings.json`. */ export async function runStatus( opts: { strict?: boolean; drift?: boolean } = {}, @@ -130,12 +132,18 @@ async function runArchetypeStatus( layout: configLayout(config), }); printDriftSection(result); + // HOOKS: settings.json is user-owned and never compared as a file above; + // what IS checked is whether every hook upstream wires is wired here. + const hooks = diffHookWiring(repo.dir, cwd); + const hookLines = hookWiringLines(hooks); + if (hookLines) note(hookLines.join('\n'), 'HOOKS'); if ( strict && (outdated || result.modified.length || result.missing.length || - result.unreadable.length) + result.unreadable.length || + hookLines !== null) ) { exitCode = 1; } diff --git a/src/commands/update.ts b/src/commands/update.ts index 29ecb6ed..a491264a 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -1,3 +1,8 @@ +import { + diffHookWiring, + hookWiringLines, + type HookWiringDiff, +} from '../lib/hook-wiring.js'; import { confirm, intro, @@ -136,6 +141,9 @@ interface UpdateOutcome { versionWithheld: boolean; abandonedLayout: Layout | null; featuresReadmeRelocation: boolean; + // Upstream hooks the project's settings.json does not wire. Reported only — + // update never writes settings.json. Absent on paths that never saw a clone. + hookWiring?: HookWiringDiff; } async function runArchetypeUpdate( @@ -332,7 +340,7 @@ async function runArchetypeUpdate( ? 'Capabilities updated' : 'Nothing to write', ); - return applied; + return { ...applied, hookWiring: diffHookWiring(repo.dir, cwd) }; } finally { repo.cleanup(); } @@ -611,6 +619,11 @@ function reportOutcome(outcome: UpdateOutcome, force: boolean): void { reportCapabilityChanges(outcome.capabilityChanges); + const hookLines = outcome.hookWiring + ? hookWiringLines(outcome.hookWiring) + : null; + if (hookLines) note(hookLines.join('\n'), 'HOOKS'); + // An unhashable destination is skipped BEFORE the decision table and `force` // is not an input to that branch (lib/update-decision.ts) — deliberately, since // there is no hash to compare and guessing one is what P5 forbids. So the diff --git a/src/lib/hook-wiring.ts b/src/lib/hook-wiring.ts new file mode 100644 index 00000000..783d56dd --- /dev/null +++ b/src/lib/hook-wiring.ts @@ -0,0 +1,195 @@ +import { closeSync, fstatSync, openSync, readSync } from 'node:fs'; +import { CLAUDE_SETTINGS_FILE } from './constants.js'; +import { findSymlinkComponent } from './symlink-guard.js'; +import { isPlainObject, safeJoin } from './validate.js'; + +// --------------------------------------------------------------------------- +// Hook-wiring drift: which hooks upstream's `.claude/settings.json` wires that +// the project's does not. +// +// `settings.json` is user-owned — `init` writes it only when absent and nothing +// ever overwrites it (install-capabilities.ts). That stays. What was wrong was +// the SILENCE: upstream re-wired its hooks (6.1.0 anchored both guards on +// CLAUDE_PROJECT_DIR because the relative form was silently off after a `cd`; +// 6.12.0 added a `Stop` hook), `update` installed the new hook SCRIPTS, and the +// project kept the old wiring with `status --strict` green. This module is the +// report: a pure set difference, never a write. +// +// Trust (P2): upstream's file is untrusted remote content and the project's is +// local user content. Both are read as DATA — size-capped, symlink-refused, +// JSON-parsed, never executed — and only the command strings reach a terminal, +// with control characters replaced (`displayHook`). +// +// Determinism (P5): an entry is `Event · matcher · command · args` compared as +// an exact string. Equality is TEXTUAL — a user's equivalent rewrite of a hook +// reads as missing; the message says so, and the human decides (advisory). +// Extra hooks the user added never count. +// --------------------------------------------------------------------------- + +/** A single wired hook, normalized. */ +export interface HookEntry { + event: string; + matcher: string; + command: string; + args: string[]; +} + +export type HookWiringStatus = + /** Every upstream hook is wired in the project (extra user hooks allowed). */ + | 'match' + /** At least one upstream hook is not wired in the project. */ + | 'missing' + /** Upstream ships no readable settings.json / hooks block — nothing to compare. */ + | 'no-upstream' + /** The project has no settings.json at all. */ + | 'project-absent' + /** The project's settings.json exists but cannot be read as JSON hooks. */ + | 'unreadable'; + +export interface HookWiringDiff { + status: HookWiringStatus; + missing: HookEntry[]; + /** Why the project file is unreadable (only for `unreadable`). */ + reason?: string; +} + +const MAX_SETTINGS_BYTES = 256 * 1024; + +type ReadResult = + | { kind: 'ok'; value: unknown } + | { kind: 'absent' } + | { kind: 'unreadable'; reason: string }; + +function readSettings(base: string): ReadResult { + let linked: string | null; + try { + linked = findSymlinkComponent(base, CLAUDE_SETTINGS_FILE); + } catch { + return { + kind: 'unreadable', + reason: 'a path component is not a directory', + }; + } + if (linked !== null) + return { kind: 'unreadable', reason: `${linked} is a symbolic link` }; + let fd: number; + try { + fd = openSync(safeJoin(base, CLAUDE_SETTINGS_FILE), 'r'); + } catch (err) { + return (err as NodeJS.ErrnoException).code === 'ENOENT' + ? { kind: 'absent' } + : { kind: 'unreadable', reason: 'it cannot be opened' }; + } + try { + const st = fstatSync(fd); + if (!st.isFile()) + return { kind: 'unreadable', reason: 'it is not a regular file' }; + if (st.size > MAX_SETTINGS_BYTES) + return { kind: 'unreadable', reason: 'it is larger than 256 KB' }; + const buf = Buffer.alloc(st.size); + let off = 0; + while (off < buf.length) { + const n = readSync(fd, buf, off, buf.length - off, off); + if (n === 0) break; + off += n; + } + try { + return { + kind: 'ok', + value: JSON.parse(buf.subarray(0, off).toString('utf8')), + }; + } catch { + return { kind: 'unreadable', reason: 'it is not valid JSON' }; + } + } catch { + return { kind: 'unreadable', reason: 'it cannot be read' }; + } finally { + closeSync(fd); + } +} + +/** + * Normalize a settings object's `hooks` block into entries, or `null` when there + * is no usable `hooks` object. Malformed members are skipped, not thrown on: a + * shape this reader does not understand is simply not an entry. + */ +export function hookEntries(settings: unknown): HookEntry[] | null { + if (!isPlainObject(settings) || !isPlainObject(settings.hooks)) return null; + const out: HookEntry[] = []; + for (const [event, groups] of Object.entries(settings.hooks)) { + if (!Array.isArray(groups)) continue; + for (const group of groups) { + if (!isPlainObject(group) || !Array.isArray(group.hooks)) continue; + const matcher = typeof group.matcher === 'string' ? group.matcher : ''; + for (const hook of group.hooks) { + if (!isPlainObject(hook) || typeof hook.command !== 'string') continue; + const args = Array.isArray(hook.args) + ? hook.args.filter((a): a is string => typeof a === 'string') + : []; + out.push({ event, matcher, command: hook.command, args }); + } + } + } + return out; +} + +function entryKey(e: HookEntry): string { + return JSON.stringify([e.event, e.matcher, e.command, e.args]); +} + +/** Compare the project's hook wiring with the fetched upstream clone's. */ +export function diffHookWiring( + repoDir: string, + projectRoot: string, +): HookWiringDiff { + const upstream = readSettings(repoDir); + const upstreamEntries = + upstream.kind === 'ok' ? hookEntries(upstream.value) : null; + if (upstreamEntries === null || upstreamEntries.length === 0) + return { status: 'no-upstream', missing: [] }; + + const project = readSettings(projectRoot); + if (project.kind === 'absent') + return { status: 'project-absent', missing: upstreamEntries }; + if (project.kind === 'unreadable') + return { + status: 'unreadable', + missing: upstreamEntries, + reason: project.reason, + }; + const have = new Set((hookEntries(project.value) ?? []).map(entryKey)); + const missing = upstreamEntries.filter((e) => !have.has(entryKey(e))); + return { status: missing.length ? 'missing' : 'match', missing }; +} + +// eslint-disable-next-line no-control-regex +const CONTROL_RE = /[\x00-\x1f\x7f-\x9f]/g; + +/** One display line for an entry, with control characters replaced. */ +export function displayHook(e: HookEntry): string { + const cmd = [e.command, ...e.args].join(' '); + const where = e.matcher ? `${e.event} [${e.matcher}]` : e.event; + return `${where}: ${cmd}`.replace(CONTROL_RE, '?'); +} + +/** + * The note body both `update` and `status` print, or `null` when there is + * nothing to say (`match` / `no-upstream`). + */ +export function hookWiringLines(diff: HookWiringDiff): string[] | null { + if (diff.status === 'match' || diff.status === 'no-upstream') return null; + const head = + diff.status === 'project-absent' + ? ` ${CLAUDE_SETTINGS_FILE} is absent — no PHARN hook is wired. Upstream wires:` + : diff.status === 'unreadable' + ? ` ${CLAUDE_SETTINGS_FILE} could not be read (${diff.reason ?? 'unknown'}). Upstream wires:` + : ` Upstream wires ${diff.missing.length} hook(s) your ${CLAUDE_SETTINGS_FILE} does not:`; + return [ + head, + ...diff.missing.map((e) => ` ${displayHook(e)}`), + '', + ` pharn never writes ${CLAUDE_SETTINGS_FILE} after init — merge these by hand`, + ' (compare with pharn-oss .claude/settings.json). Matching is textual: an', + ' equivalent hook you wrote differently is listed too.', + ]; +} diff --git a/tests/hook-wiring.test.ts b/tests/hook-wiring.test.ts new file mode 100644 index 00000000..95678984 --- /dev/null +++ b/tests/hook-wiring.test.ts @@ -0,0 +1,197 @@ +import { mkdirSync, symlinkSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { useTmpDir } from './helpers.js'; +import { + diffHookWiring, + displayHook, + hookEntries, + hookWiringLines, +} from '../src/lib/hook-wiring.js'; + +// PHARN-04: upstream re-wired its hooks twice (6.1.0 CLAUDE_PROJECT_DIR anchoring, +// 6.12.0 a new exec-form Stop hook) and existing installs never heard of it. + +const OLD = { + hooks: { + PreToolUse: [ + { + matcher: 'Write|Edit|MultiEdit|NotebookEdit', + hooks: [ + { + type: 'command', + command: 'node .claude/hooks/protect-trusted-paths.cjs', + }, + { + type: 'command', + command: 'node .claude/hooks/enforce-writes-scope.cjs', + }, + ], + }, + ], + }, +}; + +const NEW = { + _comment: 'upstream 6.17.1 shape', + hooks: { + PreToolUse: [ + { + matcher: 'Write|Edit|MultiEdit|NotebookEdit', + hooks: [ + { + type: 'command', + command: + 'node "${CLAUDE_PROJECT_DIR}"/.claude/hooks/protect-trusted-paths.cjs', + }, + { + type: 'command', + command: + 'node "${CLAUDE_PROJECT_DIR}"/.claude/hooks/enforce-writes-scope.cjs', + }, + ], + }, + ], + Stop: [ + { + hooks: [ + { + type: 'command', + command: 'node', + args: [ + '${CLAUDE_PROJECT_DIR}/.claude/hooks/require-loop-record.cjs', + ], + timeout: 10, + }, + ], + }, + ], + }, +}; + +describe('diffHookWiring', () => { + const tmp = useTmpDir(); + + function setup(upstream: unknown, project: unknown | undefined) { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + mkdirSync(join(repo, '.claude'), { recursive: true }); + mkdirSync(join(proj, '.claude'), { recursive: true }); + if (upstream !== undefined) + writeFileSync( + join(repo, '.claude/settings.json'), + typeof upstream === 'string' ? upstream : JSON.stringify(upstream), + ); + if (project !== undefined) + writeFileSync( + join(proj, '.claude/settings.json'), + typeof project === 'string' ? project : JSON.stringify(project), + ); + return { repo, proj }; + } + + it('reports every re-wired and new upstream hook for a 6.0.0-era wiring', () => { + const { repo, proj } = setup(NEW, OLD); + const diff = diffHookWiring(repo, proj); + expect(diff.status).toBe('missing'); + expect(diff.missing.map(displayHook)).toEqual([ + 'PreToolUse [Write|Edit|MultiEdit|NotebookEdit]: node "${CLAUDE_PROJECT_DIR}"/.claude/hooks/protect-trusted-paths.cjs', + 'PreToolUse [Write|Edit|MultiEdit|NotebookEdit]: node "${CLAUDE_PROJECT_DIR}"/.claude/hooks/enforce-writes-scope.cjs', + 'Stop: node ${CLAUDE_PROJECT_DIR}/.claude/hooks/require-loop-record.cjs', + ]); + }); + + it('matches when the project wires everything upstream does, ignoring its own extra hooks and _comment', () => { + const project = structuredClone(NEW) as typeof NEW & { + hooks: Record; + }; + delete (project as { _comment?: string })._comment; + project.hooks.PostToolUse = [ + { hooks: [{ type: 'command', command: 'echo mine' }] }, + ]; + const { repo, proj } = setup(NEW, project); + expect(diffHookWiring(repo, proj)).toEqual({ + status: 'match', + missing: [], + }); + }); + + it('distinguishes exec-form hooks by their args, not just the binary', () => { + const project = structuredClone(NEW); + project.hooks.Stop[0]!.hooks[0]!.args = ['something-else.cjs']; + const { repo, proj } = setup(NEW, project); + const diff = diffHookWiring(repo, proj); + expect(diff.status).toBe('missing'); + expect(diff.missing).toHaveLength(1); + expect(diff.missing[0]!.event).toBe('Stop'); + }); + + it('project settings.json absent → project-absent, listing all upstream hooks', () => { + const { repo, proj } = setup(NEW, undefined); + const diff = diffHookWiring(repo, proj); + expect(diff.status).toBe('project-absent'); + expect(diff.missing).toHaveLength(3); + }); + + it.each([ + ['{ not json', 'it is not valid JSON'], + ['x'.repeat(256 * 1024 + 1), 'it is larger than 256 KB'], + ])( + 'project settings.json unreadable (%#) → unreadable with a reason', + (raw, reason) => { + const { repo, proj } = setup(NEW, raw); + expect(diffHookWiring(repo, proj)).toMatchObject({ + status: 'unreadable', + reason, + }); + }, + ); + + it('refuses to follow a symlinked project settings.json', () => { + const { repo, proj } = setup(NEW, undefined); + const outside = join(tmp.path(), 'outside.json'); + writeFileSync(outside, JSON.stringify(NEW)); + symlinkSync(outside, join(proj, '.claude/settings.json')); + expect(diffHookWiring(repo, proj)).toMatchObject({ + status: 'unreadable', + reason: '.claude/settings.json is a symbolic link', + }); + }); + + it.each([[undefined], ['{ not json'], [{ hooks: 'nope' }], [{ hooks: {} }]])( + 'no usable upstream hooks (%#) → no-upstream, nothing to report', + (upstream) => { + const { repo, proj } = setup(upstream, OLD); + expect(diffHookWiring(repo, proj)).toEqual({ + status: 'no-upstream', + missing: [], + }); + expect( + hookWiringLines({ status: 'no-upstream', missing: [] }), + ).toBeNull(); + }, + ); + + it('replaces control characters from either file before display', () => { + const evil = { + hooks: { + Stop: [ + { + hooks: [{ type: 'command', command: 'node x\u001b]0;pwned\u0007' }], + }, + ], + }, + }; + const entries = hookEntries(evil)!; + // eslint-disable-next-line no-control-regex + expect(displayHook(entries[0]!)).not.toMatch(/[\x00-\x1f]/); + }); + + it('the note names the entries and that pharn never writes settings.json', () => { + const { repo, proj } = setup(NEW, OLD); + const lines = hookWiringLines(diffHookWiring(repo, proj))!.join('\n'); + expect(lines).toContain('Upstream wires 3 hook(s)'); + expect(lines).toContain('require-loop-record.cjs'); + expect(lines).toContain('pharn never writes .claude/settings.json'); + }); +}); diff --git a/tests/status.test.ts b/tests/status.test.ts index 3a3c60d4..4906dce3 100644 --- a/tests/status.test.ts +++ b/tests/status.test.ts @@ -1,5 +1,7 @@ +import { mkdirSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { ProcessExit, stubProcessExit } from './helpers.js'; +import { ProcessExit, stubProcessExit, useTmpDir } from './helpers.js'; import type { CapabilityIndex, PharnConfig } from '../src/types.js'; vi.mock('@clack/prompts', () => ({ @@ -658,3 +660,70 @@ describe('runStatus — frozen capabilities', () => { expect(fetchRepo).not.toHaveBeenCalled(); }); }); + +// PHARN-04: settings.json is never compared as a file, but whether every hook +// upstream wires is wired in the project IS checked — and fails --strict. +describe('runStatus — HOOKS (PHARN-04)', () => { + stubProcessExit(); + const tmp = useTmpDir(); + let repo = ''; + let proj = ''; + const oldWiring = { + hooks: { + PreToolUse: [ + { + matcher: 'Write|Edit', + hooks: [{ type: 'command', command: 'node .claude/hooks/a.cjs' }], + }, + ], + }, + }; + const newWiring = { + hooks: { + ...oldWiring.hooks, + Stop: [ + { + hooks: [ + { type: 'command', command: 'node', args: ['.claude/hooks/b.cjs'] }, + ], + }, + ], + }, + }; + const put = (dir: string, v: unknown) => { + mkdirSync(join(dir, '.claude'), { recursive: true }); + writeFileSync(join(dir, '.claude/settings.json'), JSON.stringify(v)); + }; + + beforeEach(() => { + repo = join(tmp.path(), 'repo'); + proj = join(tmp.path(), 'proj'); + put(repo, newWiring); + vi.spyOn(process, 'cwd').mockReturnValue(proj); + loadArchetypeConfigOrExit.mockReturnValue(config()); + fetchRepo.mockResolvedValue({ dir: repo, cleanup: vi.fn() }); + readSkillsVersion.mockReturnValue('1.0.0'); + diffInstalledCapabilities.mockReturnValue(CLEAN); + }); + afterEach(() => vi.clearAllMocks()); + + it('names an upstream hook the project does not wire, and exits 0 without --strict', async () => { + put(proj, oldWiring); + await runStatus({}); + expect(noteBody('HOOKS')).toContain('Stop: node .claude/hooks/b.cjs'); + expect(noteBody('DRIFT')).toContain('No drift'); + }); + + it('--strict exits 1 when an upstream hook is not wired', async () => { + put(proj, oldWiring); + await expect(runStatus({ strict: true })).rejects.toMatchObject( + new ProcessExit(1), + ); + }); + + it('--strict exits 0 and prints no HOOKS note when the wiring matches', async () => { + put(proj, newWiring); + await runStatus({ strict: true }); + expect(noteBody('HOOKS')).toBe(''); + }); +}); diff --git a/tests/update.test.ts b/tests/update.test.ts index 384f26a6..0ba30c19 100644 --- a/tests/update.test.ts +++ b/tests/update.test.ts @@ -819,6 +819,59 @@ describe('runUpdate (drift-safe)', () => { expect(cleanup).toHaveBeenCalled(); }); + // PHARN-04: new upstream hook wiring is REPORTED, and settings.json is never + // written — the old silence let the 6.1.0 / 6.12.0 re-wiring go unnoticed. + it('reports upstream hooks missing from settings.json and leaves the file byte-identical', async () => { + await installed(); + const oldSettings = JSON.stringify({ + hooks: { + PreToolUse: [ + { + matcher: 'Write', + hooks: [{ type: 'command', command: 'node .claude/hooks/x.cjs' }], + }, + ], + }, + }); + write(join(proj, '.claude/settings.json'), oldSettings); + write( + join(repo, '.claude/settings.json'), + JSON.stringify({ + hooks: { + Stop: [ + { + hooks: [ + { + type: 'command', + command: 'node', + args: ['.claude/hooks/require-loop-record.cjs'], + }, + ], + }, + ], + }, + }), + ); + + await runUpdate(); + + const hooksNote = vi + .mocked(prompts.note) + .mock.calls.find((c) => c[1] === 'HOOKS'); + expect(String(hooksNote?.[0])).toContain( + 'Stop: node .claude/hooks/require-loop-record.cjs', + ); + expect(body('.claude/settings.json')).toBe(oldSettings); + }); + + it('prints no HOOKS note when upstream ships no settings.json', async () => { + await installed(); + await runUpdate(); + expect( + vi.mocked(prompts.note).mock.calls.some((c) => c[1] === 'HOOKS'), + ).toBe(false); + }); + it('re-resolves the RECORDED archetypes against the fresh index, and UNIONS the result with the manual entries', async () => { // This test used to pin only the re-resolve call — which was true of the // wholesale-replace bug too. The re-resolve still happens; what it now also