From 54942e62c601db90e50d5e7cd4093cfb4a06618b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 25 Sep 2026 05:46:38 +0000 Subject: [PATCH] fix(hooks): the HOOKS check reads what Claude Code reads and prints what can be pasted back - Reads the union of .claude/settings.json and .claude/settings.local.json (what Claude Code merges; user-level settings are never read). Hooks wired only locally are `local-only`: named in the note, not failing --strict (status.ts now gates on hookWiringFails, not on the note's presence). An unreadable project file is never guessed around. The "no PHARN hook is wired" claim is gone. - A symlinked project settings.json FILE is read through, as Claude Code does; a symlinked .claude/ and any upstream symlink are still refused. - Every settings file is opened O_NONBLOCK: a FIFO is refused instead of blocking status/update forever (test runs in a child with a timeout). - Each missing hook prints as the hook's own JSON under `Event [matcher]`, so exec form (command + args) and shell form differ and a pasted line satisfies the check (round trip tested, and verified on the real upstream settings.json). C0/C1/Cf characters and U+2028/2029 become \u JSON escapes; each line is capped at 300 characters. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0199owRmYfskqYQVQrVP679o --- .dev/features/hook-wiring-truth/GRILL.md | 59 ++++ .dev/features/hook-wiring-truth/PLAN.md | 142 ++++++++++ .dev/features/hook-wiring-truth/REGRESSION.md | 41 +++ .dev/features/hook-wiring-truth/REVIEW.md | 64 +++++ .dev/features/hook-wiring-truth/SHIP.md | 29 ++ .dev/features/hook-wiring-truth/VERIFY.md | 53 ++++ .../hook-wiring-truth/regression-report.json | 27 ++ .../hook-wiring-truth/verify-report.json | 18 ++ .pharn/writes-scope.json | 4 +- CHANGELOG.md | 6 + CLAUDE.md | 2 +- docs/commands/status.md | 23 +- docs/commands/update.md | 8 +- src/commands/status.ts | 10 +- src/lib/hook-wiring.ts | 205 +++++++++++--- tests/hook-wiring.test.ts | 261 +++++++++++++++++- tests/status.test.ts | 18 +- tests/update.test.ts | 2 +- 18 files changed, 903 insertions(+), 69 deletions(-) create mode 100644 .dev/features/hook-wiring-truth/GRILL.md create mode 100644 .dev/features/hook-wiring-truth/PLAN.md create mode 100644 .dev/features/hook-wiring-truth/REGRESSION.md create mode 100644 .dev/features/hook-wiring-truth/REVIEW.md create mode 100644 .dev/features/hook-wiring-truth/SHIP.md create mode 100644 .dev/features/hook-wiring-truth/VERIFY.md create mode 100644 .dev/features/hook-wiring-truth/regression-report.json create mode 100644 .dev/features/hook-wiring-truth/verify-report.json diff --git a/.dev/features/hook-wiring-truth/GRILL.md b/.dev/features/hook-wiring-truth/GRILL.md new file mode 100644 index 0000000..2127242 --- /dev/null +++ b/.dev/features/hook-wiring-truth/GRILL.md @@ -0,0 +1,59 @@ +# GRILL — hook-wiring-truth + +Plan: `.dev/features/hook-wiring-truth/PLAN.md`. Spec hash recomputed: +`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e` — **matches**. Registered +grillers: `{"registered":0,"grillers":[]}` → inline axes only. The plan is `trust: untrusted`; +nothing in it read as an instruction. + +## Findings + +### Honest scope / completeness (P7, P3) + +```yaml +- type: FINDING + rule_id: 'P3' + severity: important + file: '.dev/features/hook-wiring-truth/PLAN.md:84' + problem: 'status.ts fails --strict on `hookLines !== null` — on the PRESENCE of a HOOKS note (status.ts:139-147). A local-only note that must NOT fail --strict (GATE 1 answer 1 → a) therefore needs the strict test to read the diff status instead, and src/commands/status.ts is not in ## Files. Declare it, or the build cannot land the approved behaviour without escaping scope.' + evidence: '`tests/status.test.ts` — layer tests. `--strict` exits 0 when the hooks are wired only in' +``` + +### Determinism (P5) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/hook-wiring-truth/PLAN.md:62' + problem: 'The union does not say what an UNREADABLE file contributes. If settings.json is malformed and settings.local.json wires everything, is that "match" or "unreadable"? Keep the conservative answer: any unreadable project file makes the result unreadable, naming WHICH file, because pharn cannot know what it wires. Decide it in the table rather than leaving it to the build.' + evidence: "The diff reads the project's `settings.json` AND `settings.local.json`; the set of wired hooks" +``` + +### Guarantee audit (P0) + +```yaml +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/hook-wiring-truth/PLAN.md:67' + problem: 'U+2028 / U+2029 (LINE and PARAGRAPH SEPARATOR, category Zl/Zp, not Cf) pass both terminalSafe and JSON.stringify raw. Escape them too, so the "nothing raw that moves the cursor" claim holds for the whole line.' + evidence: 'It is built as JSON, then every character the shared sanitizer would' +``` + +### Checked, no finding + +- **Trust (P2).** Upstream strings reach the terminal only escaped and capped. Following a + project-side symlinked leaf reads the user's own target as data. Its content is never printed: + only upstream entries are. +- **Eval coverage (P1).** Each finding has a failing-on-base case listed. The FIFO case needs no + child process: with `O_NONBLOCK` the open returns at once, and on the base it hangs past the + vitest timeout, which is the failure. + +## Summary + +Right idea, one scope gap. The approved "local-only counts as wired, with a note" cannot be built +without touching the strict gate in `status.ts`, so the plan must declare that file. The union also +needs an explicit rule for an unreadable file. The escape set should cover the two line separators. + +**ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 1 important, 2 minor) — for the human to +weigh before /pharn-dev-build.** diff --git a/.dev/features/hook-wiring-truth/PLAN.md b/.dev/features/hook-wiring-truth/PLAN.md new file mode 100644 index 0000000..2a0f217 --- /dev/null +++ b/.dev/features/hook-wiring-truth/PLAN.md @@ -0,0 +1,142 @@ +# PLAN — hook-wiring-truth (the HOOKS check reads what Claude Code reads, and prints what can be pasted back) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: `lib/hook-wiring.ts` changes in five ways. + 1. The project side also reads `.claude/settings.local.json` (F15). + 2. A symlinked `.claude/settings.json` LEAF on the project side is followed for reading (the + `status --strict` half of F8). + 3. Every file is opened non-blocking, so a FIFO is refused instead of hanging (F18). + 4. Each missing hook prints as the exact JSON object that satisfies the check, exec form and shell + form kept distinct (F17). + 5. That rendering is terminal-safe and length-capped through the shared sanitizer (F16). + + The note's heading only says what is true. + +- layer(s): the CLI itself (`src/lib/hook-wiring.ts`), docs +- constitution_refs: [P0, P1, P2, P5, P6] + +## Discovery — verified this run (P6) + +Read on HEAD `abb274a`. `hook-wiring.ts` is unchanged since the review range. + +- **F18.** `readSettings` (`:62-110`) calls `openSync(…, 'r')`, with no `O_NONBLOCK`. On a FIFO, + `open` blocks before `fstat` can refuse it. The hardened readers already use + `O_RDONLY | O_NONBLOCK` (`capability-index.ts:167-173`, `detect-archetype.ts:180-184`). +- **F8, status half.** `readSettings` walks `findSymlinkComponent`, and any symlinked component, + leaf included, makes it `unreadable` → a HOOKS note → `--strict` exit 1 (`status.ts:137-147`). That + happens even when the linked file wires every hook. The project-side content is never printed: + only UPSTREAM entries reach the terminal. The upstream side is a clone written by our own + extractor, and that side keeps its refusal. +- **F15.** Only `.claude/settings.json` is read. Claude Code merges hooks from `settings.json`, + `settings.local.json` (gitignored, per-user) and user-level `~/.claude/settings.json`. Hooks wired + in `settings.local.json` are reported missing, and with `settings.json` absent the heading says + "is absent — no PHARN hook is wired" (`:185-186`), which is false. None of this is documented + (`docs/commands/status.md:78-80`). +- **F16.** `displayHook` (`:170-174`) strips `[\x00-\x1f\x7f-\x9f]` with its own regex. It keeps + `\p{Cf}` (U+202E, U+2066, U+200B) and has no length cap. `terminalSafe` (`terminal-safe.ts:27-39`) + is the shared sanitizer and does both. +- **F17.** `displayHook` joins `[command, ...args]` with spaces. `entryKey` (`:143-145`) compares + `[event, matcher, command, args]`, so exec and shell forms are different keys. Upstream's real + `Stop` hook is exec form, and the other two are shell form: + + ```json + { + "type": "command", + "command": "node", + "args": ["${CLAUDE_PROJECT_DIR}/.claude/hooks/require-loop-record.cjs"], + "timeout": 10 + } + ``` + + (from a git-archive of pharn-oss). The exec-form hook prints as `Stop: node ${CLAUDE_PROJECT_DIR}/…`. + Pasted back as `command`, that is shell form, so it stays "missing". + +## Files + +- `src/lib/hook-wiring.ts` — layer CLI/lib. Changes: + - Every settings read opens with `O_RDONLY | O_NONBLOCK`, and the upstream side adds + `O_NOFOLLOW`. `fstat` still refuses anything but a regular file, so a FIFO is refused instead of + blocking forever (F18). + - Project side: a symlinked LEAF is followed; a symlinked intermediate directory is still refused + (GATE 1 answer 2 → a). Upstream side: the refusal stays as it is. + - The diff reads the project's `settings.json` AND `settings.local.json`; the set of wired hooks + is their union (F15). Each missing entry is still upstream data. The result names the upstream + hooks wired ONLY in `settings.local.json` (GATE 1 answer 1 → a: they count as wired, with a + note). An UNREADABLE project file makes the whole result unreadable, naming which file — pharn + cannot know what it wires (grill finding 2). + - The display line is ` []: {"type":"command","command":…,"args":[…]}`, args + only when present (F17). It is built as JSON, then every character the shared sanitizer would + strip is replaced by its backslash-u escape: still valid JSON, parsing back to the exact + upstream string, with nothing raw reaching the terminal (F16). U+2028 / U+2029 are escaped too + (grill finding 3). The event and matcher get the same escaping. Each line is capped at 300 characters with an ellipsis. + - Headings say "not wired in `.claude/settings.json` or `.claude/settings.local.json`". The + "absent" heading is used only when neither file exists, and makes no claim about other scopes. +- `tests/hook-wiring.test.ts` — layer tests. FAIL on base: + - FIFO at `settings.json` → unreadable, within a bounded runtime (on base it hangs). + - Hooks only in `settings.local.json` → match. + - Symlinked leaf to a file wiring every hook → match. + - U+202E / U+200B in an upstream command → the line holds their escape text and no raw Cf + character. + - A 200 000-character command → a line of at most 301 characters. + - Exec-form Stop hook → the printed JSON, parsed and pasted into a project `settings.json`, makes + the diff return match (the round trip F17 lacked). + - Guards: a symlinked `.claude` dir is still unreadable; the upstream-side symlink is still + refused. +- `src/commands/status.ts` — layer CLI/commands. `--strict` fails on the diff STATUS (missing or + unreadable), no longer on the mere presence of a HOOKS note, so the local-only note prints without + failing it (grill finding 1). +- `tests/status.test.ts` — layer tests. `--strict` exits 0 when the hooks are wired only in + `settings.local.json` (FAILS on base), and the note names them as local-only. +- `tests/update.test.ts` — layer tests. Only if a heading assertion there changes. +- `docs/commands/status.md` — layer docs. The HOOKS check reads `settings.json` + + `settings.local.json`, never user-level settings; it notes local-only wiring and prints + paste-ready JSON. +- `docs/commands/update.md` — layer docs. The same, for update's HOOKS note. +- `CLAUDE.md` — layer docs. The `lib/hook-wiring.ts` sentence ("both files size-capped + + symlink-refused…") becomes the new read rules. +- `CHANGELOG.md` — `[Unreleased]` → `### Fixed` (local settings, symlinked settings, paste-able + lines, FIFO) and `### Security` (format characters and the cap in the HOOKS note) + +## Contracts satisfied + +- PHARN-17's "one display sanitizer" (`terminal-safe.ts` header) now covers the HOOKS note, and + PHARN-14/15's non-blocking open rule now covers this reader too (cited, P4). + +## Evals to write (P1) + +- Listed under Files. Seven cases FAIL on the base. + +## Guarantee audit (P0) + +- "no Cf, C0 or C1 character from upstream reaches the HOOKS note" → floor: an escape pass over the + same `\p{Cf}`/C0/C1 class `terminalSafe` uses, plus a test. +- "a printed line satisfies the check when pasted" → floor: the round-trip test. Advisory beyond + the tested shapes. +- "a FIFO cannot hang `status`/`update`" → floor: `O_NONBLOCK` + `fstat`, plus a test. +- "local-only wiring passes `--strict`" → floor: a test. Whether that is the right policy is the + human's call (open question 1). + +## Trust audit (P2) + +- Upstream `settings.json` is untrusted. Its strings reach the terminal only escaped and capped. +- The project files are the user's own, read as data and never printed. Following their symlinked + leaf reveals nothing: a target that is not JSON is `unreadable`, and its content never reaches + output. + +## Determinism audit (P5) + +- Exact-string set membership, as today. Union is set union. No fallback guess. + +## Open questions (HALT) + +None open. Resolved at GATE 1 (human, 2026-09-25): every question below → **(a)**, the +recommended answer. Kept for the record: + +1. Hooks wired only in `settings.local.json`. (a) They count as wired, so `--strict` passes, and the + note says they apply to you only, not to teammates or CI — recommended. CI never has that file, + so CI's answer is unchanged. (b) They are shown as local-only but `--strict` still fails, keeping + local runs at parity with CI. (c) Keep reading `settings.json` only, and fix just the false + wording and the docs. +2. A symlinked project `.claude/settings.json`. (a) Follow a symlinked LEAF only (the dotfiles case + reported); a symlinked `.claude/` dir stays refused — recommended. (b) Follow any project-side + symlink, since this is a read-only path. (c) Keep refusing, and say so in the note. diff --git a/.dev/features/hook-wiring-truth/REGRESSION.md b/.dev/features/hook-wiring-truth/REGRESSION.md new file mode 100644 index 0000000..7cd6e42 --- /dev/null +++ b/.dev/features/hook-wiring-truth/REGRESSION.md @@ -0,0 +1,41 @@ +# REGRESSION — hook-wiring-truth + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `36ecf7003aa4fa9d3d9d755c73e15f20a0715f90` (`HEAD` — `origin/main` after #221; the + build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): + - `src/lib/hook-wiring.ts`, `src/commands/status.ts` + - `tests/hook-wiring.test.ts`, `tests/status.test.ts`, `tests/update.test.ts` + - `docs/commands/status.md`, `docs/commands/update.md` + - `CLAUDE.md`, `CHANGELOG.md` +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook + scratch) and this feature's own stage artifacts are not build output. +- **outside gates:** the 46 stdlib `*.test.mjs` / `*.test.cjs` files `scope` returned (754 tests) + + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / + `lint:md` are absent from both maps. +- **environment:** both sides ran with no proxy variables and as root **without** + `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`), the CI-equivalent of this + root sandbox. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` +is owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never +the increment. diff --git a/.dev/features/hook-wiring-truth/REVIEW.md b/.dev/features/hook-wiring-truth/REVIEW.md new file mode 100644 index 0000000..6a45f00 --- /dev/null +++ b/.dev/features/hook-wiring-truth/REVIEW.md @@ -0,0 +1,64 @@ +# REVIEW — hook-wiring-truth + +Increment: + +- `src/lib/hook-wiring.ts`: + - reads the UNION of `.claude/settings.json` and `.claude/settings.local.json`; + - adds a `local-only` status and a `hookWiringFails` predicate; + - opens every file `O_NONBLOCK` (upstream also `O_NOFOLLOW`); + - follows a symlinked project FILE but not a symlinked `.claude/`; + - prints each missing hook as its own JSON, escaped over the shared sanitizer's class plus + U+2028/2029, capped at 300 characters. +- `src/commands/status.ts`: `--strict` reads the diff status instead of the note's presence. +- Tests in three files, `docs/commands/status.md`, `docs/commands/update.md`, CLAUDE.md and + CHANGELOG. + +Treated as `trust: untrusted`; nothing in it read as an instruction. + +## Floor first (P0) + +`node .dev/floor/validate.mjs .` → `FLOOR: GREEN` (exit 0). `/pharn-dev-build`'s `npm run check` +passed (1568 tests), `/pharn-dev-regress` returned `no-regressions`, and `/pharn-dev-verify` +returned `PASS`. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0)** — each claim has a test: + - "no format, line-separator or control character from upstream reaches the note" → the escape + class plus tests over C0, C1, Cf and U+2028/2029; + - "a printed hook satisfies the check when pasted" → the round-trip test over the NEW fixture; + VERIFY repeated it on the real upstream file; + - "a FIFO cannot hang status/update" → `O_NONBLOCK` + `fstat`, plus a child-process test with a + hard timeout; + - "local-only passes `--strict`" → the `status` test; + - "an unreadable file is never guessed around" → a test that names the file. +- **L-eval (P1)** — 15 cases failed on the base. The rest are guards: a symlinked `.claude/`, a + symlinked upstream file, C0 escaping, and split wiring. +- **L-trust (P2)** — upstream strings reach the terminal only escaped and capped. Project files are + data and never printed. Following a symlinked project FILE reads the user's own target; what + reaches output from it is at most the status and a fixed reason string. +- **L-axis (P3)** — the reading and display rules stay in `hook-wiring.ts`. `status.ts` gains one + predicate call, and its gate now asks the module that owns the rule. + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/hook-wiring.ts:224' + problem: 'The two casts narrow `ReadResult` to ok|absent after the loop that returns on `unreadable`. They are correct today, but the compiler no longer checks the invariant; a later edit that drops the early return would pass typecheck. A small narrowing helper would keep it checked.' + evidence: "const inShared = wiredKeys(shared as ReadResult & { kind: 'ok' | 'absent' });" +- type: FINDING + rule_id: 'P2' + severity: minor + file: 'src/lib/hook-wiring.ts:254' + problem: "The escape class is built from code points so no raw U+2028/2029 sits in the source. It duplicates terminal-safe.ts's class with two additions. If that class ever widens, this one will not follow unless edited too. A shared export would single-source it; kept local to avoid widening terminal-safe.ts's API in this increment." + evidence: 'const ESCAPED = new RegExp(' +``` + +## Verdict + +**GREEN — 0 floor-gate findings, 2 advisory (minor).** The standing decision is the human's (GATE 2). diff --git a/.dev/features/hook-wiring-truth/SHIP.md b/.dev/features/hook-wiring-truth/SHIP.md new file mode 100644 index 0000000..72cbfc1 --- /dev/null +++ b/.dev/features/hook-wiring-truth/SHIP.md @@ -0,0 +1,29 @@ +# SHIP — hook-wiring-truth + +Stages run, in order: + +1. `/pharn-dev-plan` → GATE 1 (human: plans A–F accepted with every recommended answer). +2. `/pharn-dev-grill`. +3. The plan's `## Files` was rewritten for the writes-scope parser. Raw U+202E/U+200B characters had + crept in where escape text was meant, and they were removed. The grill findings were folded in: + - `src/commands/status.ts` declared, since the approved local-only behaviour needs the strict + gate to read the diff status; + - the unreadable-file rule; + - U+2028/2029 escaping. + + Intent unchanged. + +4. `/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) +- The run ended at **GATE 2**. The human's standing instruction for this batch: after each + increment, open a pull request and merge it once its checks are green, then start the next plan. + +chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good or +wise; that is the human's call at the post-review gate. diff --git a/.dev/features/hook-wiring-truth/VERIFY.md b/.dev/features/hook-wiring-truth/VERIFY.md new file mode 100644 index 0000000..777c366 --- /dev/null +++ b/.dev/features/hook-wiring-truth/VERIFY.md @@ -0,0 +1,53 @@ +# VERIFY — hook-wiring-truth + +## FLOOR layer (owns the verdict) + +The gates ran over the whole repo with the feature present, on node 22, with the session proxy +variables unset. They ran as root **without** `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / +`CAP_FOWNER` (`setpriv`), the CI-equivalent of this root sandbox. + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `test:floor` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +- `test` is vitest (1568 tests). It collects this increment's own cases in + `tests/hook-wiring.test.ts` and `tests/status.test.ts`. 15 of them failed on the base. +- The FIFO case runs in a child with a 15 s timeout. On the base, its synchronous `open(2)` blocked + until that timeout killed it, which is the failure; in-process, it hung the worker entirely. +- `test:floor` is floor.yml's `node --test` run (754 tests). +- There is no `structural:*` gate: the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +Outside the verdict, the real upstream `.claude/settings.json` (from a git-archive of pharn-oss) was +run through the new code in a scratch project: + +- **Empty project → `project-absent`, 3 hooks, fails `--strict`.** The exec-form `Stop` hook + prints its `args`: + `Stop: {"type":"command","command":"node","args":["${CLAUDE_PROJECT_DIR}/.claude/hooks/require-loop-record.cjs"]}`. +- **Paste round trip.** Pasting all three printed lines into `.claude/settings.local.json` gives + `local-only`, which does not fail `--strict`. The note says they run for you, not for teammates + or CI. +- **The CLI itself could not be driven end to end here:** the sandbox's egress proxy answers + codeload with 403. + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}`. No verifiers are +registered, so the verdict rests on the floor gates only. + +Residual (P0/P7): verified = the named gates passed; this is NOT a guarantee of correctness beyond +what those gates check — verifier concerns are advisory help, not assurance. + +Named limits: + +- Matching stays textual. +- User-level `~/.claude/settings.json` is not read, by design. +- A symlinked `settings.json` file is followed with `O_NONBLOCK` + `fstat`. Following a symlink + to a device or other special file is refused by the regular-file check. diff --git a/.dev/features/hook-wiring-truth/regression-report.json b/.dev/features/hook-wiring-truth/regression-report.json new file mode 100644 index 0000000..5f7d193 --- /dev/null +++ b/.dev/features/hook-wiring-truth/regression-report.json @@ -0,0 +1,27 @@ +{ + "base": "36ecf7003aa4fa9d3d9d755c73e15f20a0715f90", + "inside": [ + "CHANGELOG.md", + "CLAUDE.md", + "docs/commands/status.md", + "docs/commands/update.md", + "src/commands/status.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-truth/verify-report.json b/.dev/features/hook-wiring-truth/verify-report.json new file mode 100644 index 0000000..9a8f9e4 --- /dev/null +++ b/.dev/features/hook-wiring-truth/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "hook-wiring-truth", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "test:floor": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index 3ad6306..9b374ee 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/proxy-notice-truth/SHIP.md" + ".dev/features/hook-wiring-truth/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-25T05:32:54.033Z" + "set_at": "2026-09-25T05:46:20.332Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index aadd5d9..9d54a14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,9 +34,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 Only the notice changes; the network behaviour is Node's and is unchanged. - **The test suite no longer depends on the proxy settings of the machine running it.** With `NODE_USE_ENV_PROXY=1` set, 15 tests failed that pass in CI. With `HTTPS_PROXY` exported, 1 did. A setup file now clears the proxy variables before every test file. +- **`pharn status` and `pharn update` read the hooks Claude Code actually runs.** + - They now read both `.claude/settings.json` and your per-user `.claude/settings.local.json`, and compare their union with upstream. Hooks wired locally used to be reported missing, `status --strict` failed, and with no `settings.json` the note claimed "no PHARN hook is wired". A hook wired only in `settings.local.json` counts as wired: `--strict` passes, and the note says it reaches neither teammates nor CI. + - A symlinked `settings.json` file, managed from dotfiles for example, is now read through, as Claude Code does. It used to be treated as unreadable. A symlinked `.claude/` directory is still refused. + - A FIFO at a settings path no longer blocks `status` or `update` forever. +- **Each missing hook in the `HOOKS` note is now printed as the hook itself, in JSON, so you can paste it.** An exec-form hook (`command` with `args`) used to print as one shell-style line, and pasting that line in did not satisfy the check. ### Security +- **The `HOOKS` note no longer prints upstream text raw.** Hook commands from upstream's `settings.json` went through a control-character filter that let Unicode format characters through. A right-to-left override could make a path in the note read reversed, in a note that asks you to copy it by hand. Such characters, and the two Unicode line separators, are now shown as `\u` escapes. Each line is capped at 300 characters. - **The extractor's name check now judges the name it writes.** It checked one decoding of a tar entry's name and wrote another. A raw C1 byte (such as `0x9B`, the terminal CSI) passed the control-character check and landed on disk as that control character, where `pharn status` and `pharn update` then printed it raw. Every non-ASCII name was also written garbled. Entry names are now decoded once, as strict UTF-8, and the check, the path rules and the write all use that one string. A name that is not valid UTF-8, or that starts with a byte-order mark, is refused. - **Pax parsing is bounded per archive.** The 64 KiB cap applied to each global header, not to their number, so many headers each under the cap still cost seconds of CPU. The cap now covers all global headers in the archive together, and every header counts toward the entry limit. - **A malformed numeric header field no longer reaches the error output raw.** It is shown with non-printable bytes escaped, so an archive cannot add a line of its own to the fatal message. diff --git a/CLAUDE.md b/CLAUDE.md index ebd747f..2f5e164 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 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. +**`pharn status` (`commands/status.ts`) is strictly read-only** — it never writes, deletes, or overwrites (fixing is `update`/`add`). Two sections: a **version** check (installed `skillsVersion` vs the upstream `SKILLS_VERSION` file, plus an archetype + capability-count summary) and a **drift** check. Default clones `@main` once and reuses it for both; `--no-drift` skips the clone and uses `fetchRemoteSkillsVersion` for the version section only; `--strict` exits 1 on any outdated/modified/missing (CI gate, default exit 0). Cleanup runs in a `finally`, and every `process.exit` happens _after_ it. The pure (no I/O) engine is **`lib/diff.ts` → `diffInstalledCapabilities`**: it mirrors `installCapabilities` to derive the **expected** file set — the selected capability dirs + the fixed product surfaces, at the recorded `layout` — then `sha256`-compares each against the project root, returning `{modified, missing, okCount}`. Every read is `safeJoin`-guarded (from `lib/validate.ts`). `.claude/settings.json` is user-owned (preserved at install) and excluded from the file diff — but its `hooks` block is compared by `lib/hook-wiring.ts` (`diffHookWiring`: exact-string set difference over `Event · matcher · command · args`, against the UNION of the project's `.claude/settings.json` and `.claude/settings.local.json` — what Claude Code merges; user-level settings are never read. Every read is size-capped and opened `O_NONBLOCK` (a FIFO is refused, not waited on); upstream's file is symlink-refused at every component, while a project file may be a symlinked FILE (read through, as Claude Code does) but never sit under a symlinked `.claude/`. Only upstream entries are displayed, each as the hook's own JSON — exec vs shell form visible, paste-ready — with every `terminalSafe`-class character and U+2028/2029 turned into a `\u` JSON escape and each line capped at 300 chars); upstream hooks wired in neither file print a `HOOKS` note in `status` (and fail `--strict`, via `hookWiringFails`, as does an unreadable project file) and in `update` (report only — `update` still never writes the file); hooks wired only in `settings.local.json` are named as `local-only` and do NOT fail `--strict`; the copied-verbatim trusted docs (including `CONSTITUTION.md` / `pharn/CONSTITUTION.md`), hooks, contracts, and floor checkers ARE compared. Drift is derived live from the clone, always against `@main`, never the pinned `commit`. `status` does NOT read `pharn.records.json`, so it is a report, not a preview: a file it lists as differing may be cleanly upgraded OR skipped — only `update` can tell those apart. ## Testing diff --git a/docs/commands/status.md b/docs/commands/status.md index 31c1edf..6bc3bbb 100644 --- a/docs/commands/status.md +++ b/docs/commands/status.md @@ -75,9 +75,23 @@ a whole tree to relocate. ## What is intentionally excluded `.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`, +configuration, which the install preserves (never overwrites). Its `hooks` block is checked separately. +Claude Code merges the hooks of `.claude/settings.json` and your per-user `.claude/settings.local.json`, +so `status` reads both. It never reads user-level `~/.claude/settings.json`: the check is about the +project. The `HOOKS` section works like this: + +- **Missing.** It names every hook the upstream `settings.json` wires that neither project file does. + Extra hooks of your own never count, and matching is textual. `--strict` exits `1` while any is + missing, or while a project settings file cannot be read. +- **Paste-ready.** Each line is `Event [matcher]: `, so you can paste it into your + settings under that event. An exec-form hook (`command` plus `args`) prints its `args`, so it no + longer reads like a shell command. Characters that could move or hide text are shown as `\uXXXX` + JSON escapes, and a line is capped at 300 characters. +- **Local only.** A hook wired **only** in `settings.local.json` counts as wired, since it runs for + you, and `--strict` passes. The section still names it, because it reaches neither your teammates + nor CI (where that file does not exist). +- **Symlinks.** A symlinked `settings.json` file is read through, as Claude Code does. A symlinked + `.claude/` directory is refused. 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 @@ -88,7 +102,8 @@ 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, or an upstream hook is not wired in `.claude/settings.json` — useful as a CI gate. +unreadable, or an upstream hook is wired in neither `.claude/settings.json` nor +`.claude/settings.local.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 0f503e2..a24483c 100644 --- a/docs/commands/update.md +++ b/docs/commands/update.md @@ -305,9 +305,11 @@ command addresses it any more, and the update prints its own warning naming it. - `.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). 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. + compare the hooks of that file and `.claude/settings.local.json` with the fetched upstream ones. It + prints a `HOOKS` note naming every hook upstream wires that neither file does. Matching is textual, + so an equivalent hook you wrote differently is listed too. Each line is the hook as JSON, ready to + merge by hand. The note also names hooks wired only in `settings.local.json`, which run for you but + not for your teammates. See [status](status.md#what-is-intentionally-excluded) for the full rules. - `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 14c8b6a..23861fd 100644 --- a/src/commands/status.ts +++ b/src/commands/status.ts @@ -1,4 +1,8 @@ -import { diffHookWiring, hookWiringLines } from '../lib/hook-wiring.js'; +import { + diffHookWiring, + hookWiringFails, + 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'; @@ -143,7 +147,9 @@ async function runArchetypeStatus( result.modified.length || result.missing.length || result.unreadable.length || - hookLines !== null) + // The diff's STATUS, not the note's presence: a hook wired only in + // settings.local.json is noted, and is still wired. + hookWiringFails(hooks)) ) { exitCode = 1; } diff --git a/src/lib/hook-wiring.ts b/src/lib/hook-wiring.ts index 783d56d..ad959f5 100644 --- a/src/lib/hook-wiring.ts +++ b/src/lib/hook-wiring.ts @@ -1,11 +1,12 @@ -import { closeSync, fstatSync, openSync, readSync } from 'node:fs'; +import { closeSync, constants, fstatSync, openSync, readSync } from 'node:fs'; +import { dirname } from 'node:path'; 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. +// the project 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 @@ -15,10 +16,22 @@ import { isPlainObject, safeJoin } from './validate.js'; // 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`). +// WHAT "WIRED" MEANS is what Claude Code runs: it merges the hooks of the +// project's `.claude/settings.json` and the per-user, gitignored +// `.claude/settings.local.json`, so both are read and their union is compared. +// A hook found ONLY in the local file is wired for this user alone, which the +// report says (`local-only`) without calling it missing. User-level +// `~/.claude/settings.json` is deliberately NOT read: this is a report about +// the project. +// +// Trust (P2): upstream's file is untrusted remote content and the project's are +// local user content. All are read as DATA — size-capped, opened non-blocking +// (a FIFO is refused, not waited on), regular files only, JSON-parsed, never +// executed. Upstream's may not be a symlink at any component. A project file +// may be a symlinked FILE (a dotfiles-managed settings.json — Claude Code reads +// through it too); a symlinked `.claude/` directory is still refused. Project +// content is never printed: only UPSTREAM entries reach a terminal, escaped and +// capped by `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 @@ -26,6 +39,9 @@ import { isPlainObject, safeJoin } from './validate.js'; // Extra hooks the user added never count. // --------------------------------------------------------------------------- +/** The per-user settings file Claude Code merges over `settings.json`. */ +const LOCAL_SETTINGS_FILE = '.claude/settings.local.json'; + /** A single wired hook, normalized. */ export interface HookEntry { event: string; @@ -35,21 +51,26 @@ export interface HookEntry { } export type HookWiringStatus = - /** Every upstream hook is wired in the project (extra user hooks allowed). */ + /** Every upstream hook is wired in `settings.json` (extra user hooks allowed). */ | 'match' - /** At least one upstream hook is not wired in the project. */ + /** Every upstream hook is wired, some only in `settings.local.json`. */ + | 'local-only' + /** At least one upstream hook is wired in neither project file. */ | 'missing' /** Upstream ships no readable settings.json / hooks block — nothing to compare. */ | 'no-upstream' - /** The project has no settings.json at all. */ + /** Neither project settings file exists. */ | 'project-absent' - /** The project's settings.json exists but cannot be read as JSON hooks. */ + /** A project settings file 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`). */ + /** Upstream hooks wired only in `settings.local.json` (when there are any). */ + localOnly?: HookEntry[]; + /** Which project file is unreadable, and why (only for `unreadable`). */ + file?: string; reason?: string; } @@ -60,10 +81,24 @@ type ReadResult = | { kind: 'absent' } | { kind: 'unreadable'; reason: string }; -function readSettings(base: string): ReadResult { +// O_NONBLOCK keeps open(2) on a FIFO from blocking forever — the PHARN-14/15 +// rule for every reader of a path it does not control. POSIX-only constants are +// simply absent on win32, where the flag is not set. +const PROJECT_OPEN = constants.O_RDONLY | (constants.O_NONBLOCK ?? 0); +const UPSTREAM_OPEN = PROJECT_OPEN | (constants.O_NOFOLLOW ?? 0); + +/** + * Read one settings file under `base`. `followLeaf` is the project side: a + * symlinked FILE is followed, a symlinked directory on the way is not. + */ +function readSettings( + base: string, + rel: string, + followLeaf: boolean, +): ReadResult { let linked: string | null; try { - linked = findSymlinkComponent(base, CLAUDE_SETTINGS_FILE); + linked = findSymlinkComponent(base, followLeaf ? dirname(rel) : rel); } catch { return { kind: 'unreadable', @@ -74,11 +109,21 @@ function readSettings(base: string): ReadResult { return { kind: 'unreadable', reason: `${linked} is a symbolic link` }; let fd: number; try { - fd = openSync(safeJoin(base, CLAUDE_SETTINGS_FILE), 'r'); + fd = openSync( + safeJoin(base, rel), + followLeaf ? PROJECT_OPEN : UPSTREAM_OPEN, + ); } catch (err) { - return (err as NodeJS.ErrnoException).code === 'ENOENT' - ? { kind: 'absent' } - : { kind: 'unreadable', reason: 'it cannot be opened' }; + const code = (err as NodeJS.ErrnoException).code; + if (code === 'ENOENT') return { kind: 'absent' }; + // Only the project side reaches this with a non-directory component (the + // upstream walk above covers every component): `.claude` is a file. + if (code === 'ENOTDIR') + return { + kind: 'unreadable', + reason: 'a path component is not a directory', + }; + return { kind: 'unreadable', reason: 'it cannot be opened' }; } try { const st = fstatSync(fd); @@ -137,59 +182,139 @@ function entryKey(e: HookEntry): string { return JSON.stringify([e.event, e.matcher, e.command, e.args]); } +/** The entry keys a project file wires; an absent file wires nothing. */ +function wiredKeys( + result: ReadResult & { kind: 'ok' | 'absent' }, +): Set { + return new Set( + result.kind === 'ok' ? (hookEntries(result.value) ?? []).map(entryKey) : [], + ); +} + /** 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 upstream = readSettings(repoDir, CLAUDE_SETTINGS_FILE, false); 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') + const shared = readSettings(projectRoot, CLAUDE_SETTINGS_FILE, true); + const local = readSettings(projectRoot, LOCAL_SETTINGS_FILE, true); + // An unreadable file could wire anything: the answer is "unknown", never a + // guess from the other file. + for (const [file, result] of [ + [CLAUDE_SETTINGS_FILE, shared], + [LOCAL_SETTINGS_FILE, local], + ] as const) { + if (result.kind === 'unreadable') + return { + status: 'unreadable', + missing: upstreamEntries, + file, + reason: result.reason, + }; + } + if (shared.kind === 'absent' && local.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 }; + + const inShared = wiredKeys(shared as ReadResult & { kind: 'ok' | 'absent' }); + const inLocal = wiredKeys(local as ReadResult & { kind: 'ok' | 'absent' }); + const missing = upstreamEntries.filter( + (e) => !inShared.has(entryKey(e)) && !inLocal.has(entryKey(e)), + ); + const localOnly = upstreamEntries.filter( + (e) => !inShared.has(entryKey(e)) && inLocal.has(entryKey(e)), + ); + if (missing.length) return { status: 'missing', missing }; + if (localOnly.length) return { status: 'local-only', missing, localOnly }; + return { status: 'match', missing }; } -// eslint-disable-next-line no-control-regex -const CONTROL_RE = /[\x00-\x1f\x7f-\x9f]/g; +/** + * Does this diff fail `status --strict`? A hook wired only locally IS wired + * (it runs for this user), so `local-only` passes; in CI the local file does + * not exist, so CI's answer is the same as before. + */ +export function hookWiringFails(diff: HookWiringDiff): boolean { + return ( + diff.status === 'missing' || + diff.status === 'unreadable' || + diff.status === 'project-absent' + ); +} + +// What never reaches a terminal raw: the shared sanitizer's set (C0, DEL, C1, +// Unicode format characters — lib/terminal-safe.ts) plus the two Unicode line +// separators, which are not format characters but still move text. Built from +// code points so no such character sits in this source file. +const ESCAPED = new RegExp( + `[\\x00-\\x1f\\x7f-\\x9f\\p{Cf}${String.fromCodePoint(0x2028, 0x2029)}]`, + 'gu', +); +/** Longest line the note prints for one hook. */ +const MAX_LINE = 300; + +/** + * Each UTF-16 unit of each unsafe character as `\uXXXX`. Inside a JSON string + * these are JSON escapes, so the printed object parses back to the EXACT + * upstream string; outside one they are just visible text. + */ +function escapeUnsafe(text: string): string { + return text.replace(ESCAPED, (ch) => + [...Array(ch.length).keys()] + .map((i) => `\\u${ch.charCodeAt(i).toString(16).padStart(4, '0')}`) + .join(''), + ); +} -/** One display line for an entry, with control characters replaced. */ +/** + * One display line: `Event [matcher]: `. The JSON is the hook + * itself — `args` only when present — so an exec-form hook and a shell-form one + * print differently, and a printed line pasted into a settings file satisfies + * the check. Escaped (see above) and capped at MAX_LINE characters. + */ export function displayHook(e: HookEntry): string { - const cmd = [e.command, ...e.args].join(' '); + const hook = + e.args.length > 0 + ? { type: 'command', command: e.command, args: e.args } + : { type: 'command', command: e.command }; const where = e.matcher ? `${e.event} [${e.matcher}]` : e.event; - return `${where}: ${cmd}`.replace(CONTROL_RE, '?'); + const line = `${escapeUnsafe(where)}: ${escapeUnsafe(JSON.stringify(hook))}`; + return line.length > MAX_LINE ? `${line.slice(0, MAX_LINE)}…` : line; } +const FILES = `${CLAUDE_SETTINGS_FILE} or ${LOCAL_SETTINGS_FILE}`; + /** * 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; + if (diff.status === 'local-only') { + const only = diff.localOnly ?? []; + return [ + ` Every hook upstream wires is wired — ${only.length} of them only in ${LOCAL_SETTINGS_FILE}.`, + ' They run for you, not for teammates or CI:', + ...only.map((e) => ` ${displayHook(e)}`), + ]; + } const head = diff.status === 'project-absent' - ? ` ${CLAUDE_SETTINGS_FILE} is absent — no PHARN hook is wired. Upstream wires:` + ? ` Neither ${FILES} exists. 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:`; + ? ` ${diff.file ?? CLAUDE_SETTINGS_FILE} could not be read (${diff.reason ?? 'unknown'}). Upstream wires:` + : ` Upstream wires ${diff.missing.length} hook(s) that neither ${FILES} does:`; 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.', + ` pharn never writes ${CLAUDE_SETTINGS_FILE} after init — merge these by hand:`, + ' each line is one hook, as JSON, under its event [matcher]. 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 index b4eb094..8996d10 100644 --- a/tests/hook-wiring.test.ts +++ b/tests/hook-wiring.test.ts @@ -1,3 +1,4 @@ +import { execFileSync } from 'node:child_process'; import { mkdirSync, symlinkSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; @@ -6,6 +7,7 @@ import { diffHookWiring, displayHook, hookEntries, + hookWiringFails, hookWiringLines, } from '../src/lib/hook-wiring.js'; @@ -72,21 +74,23 @@ const NEW = { describe('diffHookWiring', () => { const tmp = useTmpDir(); - function setup(upstream: unknown, project: unknown | undefined) { + function setup( + upstream: unknown, + project: unknown | undefined, + local?: unknown, + ) { const repo = join(tmp.path(), 'repo'); const proj = join(tmp.path(), 'proj'); mkdirSync(join(repo, '.claude'), { recursive: true }); mkdirSync(join(proj, '.claude'), { recursive: true }); + const put = (file: string, v: unknown) => + writeFileSync(file, typeof v === 'string' ? v : JSON.stringify(v)); if (upstream !== undefined) - writeFileSync( - join(repo, '.claude/settings.json'), - typeof upstream === 'string' ? upstream : JSON.stringify(upstream), - ); + put(join(repo, '.claude/settings.json'), upstream); if (project !== undefined) - writeFileSync( - join(proj, '.claude/settings.json'), - typeof project === 'string' ? project : JSON.stringify(project), - ); + put(join(proj, '.claude/settings.json'), project); + if (local !== undefined) + put(join(proj, '.claude/settings.local.json'), local); return { repo, proj }; } @@ -94,10 +98,12 @@ describe('diffHookWiring', () => { const { repo, proj } = setup(NEW, OLD); const diff = diffHookWiring(repo, proj); expect(diff.status).toBe('missing'); + // Each line is the JSON of the hook as upstream wires it, so the SHELL + // form (command only) and the EXEC form (command + args) read differently. 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', + 'PreToolUse [Write|Edit|MultiEdit|NotebookEdit]: {"type":"command","command":"node \\"${CLAUDE_PROJECT_DIR}\\"/.claude/hooks/protect-trusted-paths.cjs"}', + 'PreToolUse [Write|Edit|MultiEdit|NotebookEdit]: {"type":"command","command":"node \\"${CLAUDE_PROJECT_DIR}\\"/.claude/hooks/enforce-writes-scope.cjs"}', + 'Stop: {"type":"command","command":"node","args":["${CLAUDE_PROJECT_DIR}/.claude/hooks/require-loop-record.cjs"]}', ]); }); @@ -126,11 +132,15 @@ describe('diffHookWiring', () => { expect(diff.missing[0]!.event).toBe('Stop'); }); - it('project settings.json absent → project-absent, listing all upstream hooks', () => { + it('neither project file present → 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); + // No claim about scopes pharn does not read (user-level settings). + const lines = hookWiringLines(diff)!.join('\n'); + expect(lines).not.toContain('no PHARN hook is wired'); + expect(lines).toContain('settings.local.json'); }); it.each([ @@ -169,17 +179,73 @@ describe('diffHookWiring', () => { }); }); - it('refuses to follow a symlinked project settings.json', () => { + // A dotfiles-managed settings.json is a symlinked FILE: Claude Code reads + // through it, so the check must too. It is read as data and never printed — + // only upstream entries reach the terminal. + it('follows a symlinked project settings.json (the file), as Claude Code does', () => { 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)).toEqual({ + status: 'match', + missing: [], + }); + }); + + it('still refuses a symlinked project .claude directory', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + const elsewhere = join(tmp.path(), 'elsewhere'); + mkdirSync(join(repo, '.claude'), { recursive: true }); + mkdirSync(elsewhere, { recursive: true }); + mkdirSync(proj, { recursive: true }); + writeFileSync(join(repo, '.claude/settings.json'), JSON.stringify(NEW)); + writeFileSync(join(elsewhere, 'settings.json'), JSON.stringify(NEW)); + symlinkSync(elsewhere, join(proj, '.claude')); expect(diffHookWiring(repo, proj)).toMatchObject({ status: 'unreadable', - reason: '.claude/settings.json is a symbolic link', + reason: '.claude is a symbolic link', }); }); + it('still refuses a symlinked UPSTREAM settings.json', () => { + const { repo, proj } = setup(undefined, NEW); + const outside = join(tmp.path(), 'outside-upstream.json'); + writeFileSync(outside, JSON.stringify(NEW)); + symlinkSync(outside, join(repo, '.claude/settings.json')); + expect(diffHookWiring(repo, proj).status).toBe('no-upstream'); + }); + + // A FIFO used to block `pharn status` / `update` forever in open(2) — and + // with fetchRepo's signal handlers installed, only SIGKILL ended it. The + // open is SYNCHRONOUS, so a regression would hang this test worker past any + // vitest timeout; the read therefore runs in a child with a hard timeout, + // and a hang fails here as a timeout instead of stalling CI. + it('a FIFO at the project settings.json is unreadable, not a hang', async () => { + const { repo, proj } = setup(NEW, undefined); + execFileSync('mkfifo', [join(proj, '.claude/settings.json')]); + const { spawnSync } = await import('node:child_process'); + const { fileURLToPath } = await import('node:url'); + const mod = fileURLToPath( + new URL('../src/lib/hook-wiring.ts', import.meta.url), + ); + const script = ` + const { diffHookWiring } = await import(${JSON.stringify(mod)}); + console.log(JSON.stringify(diffHookWiring(process.argv[1], process.argv[2]))); + `; + const r = spawnSync( + process.execPath, + ['--import', 'tsx', '--input-type=module', '-e', script, repo, proj], + { encoding: 'utf8', timeout: 15_000 }, + ); + expect(r.signal, 'the read hung and was killed').toBeNull(); + expect(JSON.parse(r.stdout)).toMatchObject({ + status: 'unreadable', + reason: 'it is not a regular file', + }); + }, 30_000); + it.each([[undefined], ['{ not json'], [{ hooks: 'nope' }], [{ hooks: {} }]])( 'no usable upstream hooks (%#) → no-upstream, nothing to report', (upstream) => { @@ -217,3 +283,168 @@ describe('diffHookWiring', () => { expect(lines).toContain('pharn never writes .claude/settings.json'); }); }); + +// Claude Code merges hooks from `.claude/settings.json` and the per-user, +// gitignored `.claude/settings.local.json`. The check read only the first, so +// hooks wired locally were reported missing — and with settings.json absent the +// note claimed "no PHARN hook is wired". (User-level ~/.claude settings are +// deliberately NOT read: status is about the project.) +describe('diffHookWiring — settings.local.json', () => { + const tmp = useTmpDir(); + + function setup(project: unknown | undefined, local: 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 }); + writeFileSync(join(repo, '.claude/settings.json'), JSON.stringify(NEW)); + const put = (file: string, v: unknown) => + writeFileSync(file, typeof v === 'string' ? v : JSON.stringify(v)); + if (project !== undefined) + put(join(proj, '.claude/settings.json'), project); + if (local !== undefined) + put(join(proj, '.claude/settings.local.json'), local); + return { repo, proj }; + } + + it('hooks wired only in settings.local.json count as wired, and are named', () => { + const { repo, proj } = setup({ permissions: { allow: [] } }, NEW); + const diff = diffHookWiring(repo, proj); + expect(diff.status).toBe('local-only'); + expect(diff.missing).toEqual([]); + expect(diff.localOnly).toHaveLength(3); + expect(hookWiringFails(diff)).toBe(false); + const lines = hookWiringLines(diff)!.join('\n'); + expect(lines).toContain('settings.local.json'); + expect(lines).toContain('not for teammates or CI'); + }); + + it('with no settings.json at all, a complete settings.local.json still matches', () => { + const { repo, proj } = setup(undefined, NEW); + expect(diffHookWiring(repo, proj).status).toBe('local-only'); + }); + + it('the union covers a split: some hooks in each file', () => { + const { repo, proj } = setup(OLD, { + hooks: { Stop: NEW.hooks.Stop }, + }); + const diff = diffHookWiring(repo, proj); + // OLD's two relative-path PreToolUse hooks are not upstream's anchored + // ones, so those two are still missing; the Stop hook is found locally. + expect(diff.status).toBe('missing'); + expect(diff.missing.map((e) => e.event)).toEqual([ + 'PreToolUse', + 'PreToolUse', + ]); + expect(hookWiringFails(diff)).toBe(true); + }); + + it('an unreadable settings.local.json makes the result unreadable, naming that file', () => { + const { repo, proj } = setup(NEW, '{ not json'); + const diff = diffHookWiring(repo, proj); + expect(diff).toMatchObject({ + status: 'unreadable', + file: '.claude/settings.local.json', + reason: 'it is not valid JSON', + }); + expect(hookWiringFails(diff)).toBe(true); + expect(hookWiringLines(diff)!.join('\n')).toContain( + '.claude/settings.local.json could not be read', + ); + }); + + it('everything in settings.json → match, with no note', () => { + const { repo, proj } = setup(NEW, { permissions: {} }); + const diff = diffHookWiring(repo, proj); + expect(diff).toEqual({ status: 'match', missing: [] }); + expect(hookWiringLines(diff)).toBeNull(); + expect(hookWiringFails(diff)).toBe(false); + }); +}); + +describe('displayHook — what the HOOKS note prints', () => { + const entry = (command: string, args: string[] = [], matcher = '') => ({ + event: 'Stop', + matcher, + command, + args, + }); + + // PHARN-17's one display sanitizer covers format characters too: a bidi + // override from upstream settings.json made a path in this note render + // reversed, in a note that asks the user to copy it by hand. + it('escapes format and line-separator characters instead of printing them', () => { + const line = displayHook( + entry('node \u202eevil\u2066.cjs\u200b', ['a\u2028b'], 'Write\u2029'), + ); + expect(line).not.toMatch(/[\p{Cf}\u2028\u2029]/u); + expect(line).toContain('\\u202e'); + expect(line).toContain('\\u200b'); + expect(line).toContain('\\u2028'); + }); + + it('keeps C0/C1 control characters out too, escaped', () => { + const line = displayHook(entry('node x\u001b]0;pwned\u0007\u009b')); + // eslint-disable-next-line no-control-regex + expect(line).not.toMatch(/[\x00-\x1f\x7f-\x9f]/); + }); + + it('caps a huge command', () => { + const line = displayHook(entry('x'.repeat(200_000))); + expect(line.length).toBeLessThanOrEqual(301); + expect(line.endsWith('…')).toBe(true); + }); + + // The escapes are JSON escapes inside a JSON string, so the printed object + // still parses back to the EXACT upstream strings. + it('prints JSON that parses back to the exact upstream strings', () => { + const command = 'node \u202e.cjs'; + const line = displayHook(entry(command, ['\u200b'])); + const json = JSON.parse(line.slice(line.indexOf('{'))) as { + command: string; + args: string[]; + }; + expect(json.command).toBe(command); + expect(json.args).toEqual(['\u200b']); + }); +}); + +// F17: the note printed `Stop: node ${CLAUDE_PROJECT_DIR}/…` for an EXEC-form +// hook, and pasting that into `command` produced a SHELL-form hook — a different +// entry, still reported missing, with --strict still red. The printed line is +// now the hook itself. +describe('HOOKS note — the printed hook satisfies the check when pasted', () => { + const tmp = useTmpDir(); + + it('round-trips every upstream hook through the note into a project that then matches', () => { + const repo = join(tmp.path(), 'repo'); + const proj = join(tmp.path(), 'proj'); + mkdirSync(join(repo, '.claude'), { recursive: true }); + mkdirSync(join(proj, '.claude'), { recursive: true }); + writeFileSync(join(repo, '.claude/settings.json'), JSON.stringify(NEW)); + + const missing = diffHookWiring(repo, proj).missing; + expect(missing).toHaveLength(3); + + // What a user does with the note: one group per printed line, the printed + // JSON as the hook, under the printed event and matcher. + const hooks: Record = {}; + for (const line of missing.map(displayHook)) { + const head = line.slice(0, line.indexOf(': {')); + const m = /^(\w+)(?: \[(.*)\])?$/.exec(head)!; + const hook: unknown = JSON.parse(line.slice(line.indexOf(': {') + 2)); + (hooks[m[1]!] ??= []).push( + m[2] ? { matcher: m[2], hooks: [hook] } : { hooks: [hook] }, + ); + } + writeFileSync( + join(proj, '.claude/settings.json'), + JSON.stringify({ hooks }), + ); + + expect(diffHookWiring(repo, proj)).toEqual({ + status: 'match', + missing: [], + }); + }); +}); diff --git a/tests/status.test.ts b/tests/status.test.ts index 4906dce..c380109 100644 --- a/tests/status.test.ts +++ b/tests/status.test.ts @@ -710,7 +710,9 @@ describe('runStatus — HOOKS (PHARN-04)', () => { 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('HOOKS')).toContain( + 'Stop: {"type":"command","command":"node","args":[".claude/hooks/b.cjs"]}', + ); expect(noteBody('DRIFT')).toContain('No drift'); }); @@ -726,4 +728,18 @@ describe('runStatus — HOOKS (PHARN-04)', () => { await runStatus({ strict: true }); expect(noteBody('HOOKS')).toBe(''); }); + + // Hooks wired in the per-user .claude/settings.local.json run for this user, + // so they are WIRED: --strict passes. The note still says they reach nobody + // else. (In CI that file does not exist, so CI's answer is unchanged.) + it('--strict exits 0 when the hooks are wired only in settings.local.json, and says so', async () => { + put(proj, { permissions: { allow: [] } }); + writeFileSync( + join(proj, '.claude/settings.local.json'), + JSON.stringify(newWiring), + ); + await runStatus({ strict: true }); + expect(noteBody('HOOKS')).toContain('settings.local.json'); + expect(noteBody('HOOKS')).toContain('not for teammates or CI'); + }); }); diff --git a/tests/update.test.ts b/tests/update.test.ts index d332461..622eed9 100644 --- a/tests/update.test.ts +++ b/tests/update.test.ts @@ -859,7 +859,7 @@ describe('runUpdate (drift-safe)', () => { .mocked(prompts.note) .mock.calls.find((c) => c[1] === 'HOOKS'); expect(String(hooksNote?.[0])).toContain( - 'Stop: node .claude/hooks/require-loop-record.cjs', + 'Stop: {"type":"command","command":"node","args":[".claude/hooks/require-loop-record.cjs"]}', ); expect(body('.claude/settings.json')).toBe(oldSettings); });