diff --git a/.dev/features/local-file-reads/GRILL.md b/.dev/features/local-file-reads/GRILL.md new file mode 100644 index 0000000..d781941 --- /dev/null +++ b/.dev/features/local-file-reads/GRILL.md @@ -0,0 +1,55 @@ +# GRILL — local-file-reads + +Plan: `.dev/features/local-file-reads/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 + +### Determinism (P5) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/local-file-reads/PLAN.md:56' + problem: 'The lock case says only "does not hang". The lock already has a rule for an unreadable file: presumed live while younger than MALFORMED_GRACE_MS (10 s), then broken (project-lock.ts isStale). State that a FIFO follows it — a young one refused as a live lock, an old one broken and replaced — and test both, or the test proves only the absence of a hang.' + evidence: 'A FIFO at `.pharn.lock` does not hang acquisition' +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/local-file-reads/PLAN.md:37' + problem: 'The reasons are named but not fixed. They reach messages (the records `invalid` note, the fingerprint) and the fingerprint is compared across two reads, so fix the vocabulary: "is not a regular file", "is larger than 16 MiB", and the errno code for any other open failure.' + evidence: 'the bytes, absent, or unusable with a named reason' +``` + +### Eval coverage (P1) + +```yaml +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/local-file-reads/PLAN.md:50' + problem: 'Only the reader''s own FIFO case is placed in a child process. The records, config and lock FIFO cases need the same: on the base they block in open(2) and would hang the vitest worker rather than fail (tests/hook-wiring.test.ts records the same lesson).' + evidence: 'a FIFO (in' +``` + +### Checked, no finding + +- **Scope.** Every plain read of the three files is declared (`readRecords`, `readPharnConfig`, + `configFingerprint`, `recordedSkillsVersion`, `readCarriedEntries`, `readRawAt`). The other + `readFileSync` sites read the clone (`skills-version.ts`: the extractor writes only regular files + and directories) or project files already type-checked first (`hash.ts` through `scanDest` / + `readDiskState`). +- **Trust (P2).** No new output beyond fixed reason strings; the bytes stay data. +- **Guarantee audit (P0).** Both claims reduce to `O_NONBLOCK` + `fstat` + a size compare in one + function, with tests. + +## Summary + +Small, well-scoped plan. Pin the lock case to its existing grace rule, fix the reason strings, and +run every FIFO case in a child process. + +**ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 0 important, 3 minor) — for the human to +weigh before /pharn-dev-build.** diff --git a/.dev/features/local-file-reads/PLAN.md b/.dev/features/local-file-reads/PLAN.md new file mode 100644 index 0000000..b6a7bda --- /dev/null +++ b/.dev/features/local-file-reads/PLAN.md @@ -0,0 +1,101 @@ +# PLAN — local-file-reads (no pharn-owned project file can hang a command) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: every file pharn itself keeps in the project — `pharn.records.json`, + `pharn.config.json`, `.pharn.lock` — is read through one bounded, non-blocking reader. A FIFO, a + directory, a device or an oversize file there is reported as unreadable, the same as any other + unreadable file today, instead of hanging the command or reading without bound. +- layer(s): the CLI itself (`src/lib/`, `src/steps/`), docs +- constitution_refs: [P0, P2, P5, P6] + +## Discovery — verified this run (P6), code read on HEAD `9d2d574` + +- **The hang.** Each of these reads with `readFileSync`, which on a FIFO blocks in `open(2)` until a + writer appears, i.e. forever: + - `readRecords` (`install-records.ts:125`, after an `existsSync`): read by `update`, `add`, + `remove`, and since #223 by every re-run `init` (`reinstallBaseline`), before its prompt and + again under the project lock; + - `readPharnConfig` (`pharn-config.ts:310`) and `configFingerprint` (`:483`): read by every + command, `status` and `list` included; + - `recordedSkillsVersion` (`overwrite-check.ts:70`) and `readCarriedEntries` + (`install-archetype.ts:373`), init's two tolerant config reads; + - `readRawAt` (`project-lock.ts:175`): the lock payload, read whenever a lock already exists. + + A hang while holding `.pharn.lock` also refuses every other `pharn` command in the project until + the lock goes stale. +- **No bound.** The same reads have no size cap: a symlink to `/dev/zero` at any of these paths + reads until memory runs out. +- **Precedent.** `hook-wiring.ts` (`readSettings`, #222) and `detect-archetype.ts` + (`readPackageJsonText`, PHARN-15) already read user-adjacent files through one descriptor opened + `O_NONBLOCK`, `fstat`-checked as a regular file and size-capped. These three files never got it. +- **Threat model.** Only a local actor can plant such a file; `THREAT-MODEL.md` models remote + content. This is hardening against a hang and unbounded memory, not a remote-attack fix. + +## Files + +- `src/lib/bounded-read.ts` — layer CLI/lib. New: `readBoundedFile(path, maxBytes)` returns + the bytes, absent, or unusable with a named reason. One descriptor, opened `O_RDONLY` + + `O_NONBLOCK` (absent on win32, so `?? 0`); `fstat` must show a regular file within the cap; the + bytes are read from that same descriptor. A symlinked FILE is still followed, as today. Never + throws. +- `src/lib/install-records.ts` — layer CLI/lib. `readRecords` reads through it; unusable → the + existing `invalid` outcome, with the reason named. +- `src/lib/pharn-config.ts` — layer CLI/lib. `readPharnConfig` and `configFingerprint` read through + it; unusable → the same results an unreadable file gives today (`null`, and + `unreadable:`). +- `src/steps/overwrite-check.ts` — layer CLI/steps. `recordedSkillsVersion` reads through it. +- `src/steps/install-archetype.ts` — layer CLI/steps. `readCarriedEntries` reads through it. +- `src/lib/project-lock.ts` — layer CLI/lib. `readRawAt` reads through it; unusable → `null`, the + existing never-wedge rule. +- `tests/bounded-read.test.ts` — layer tests. New: a regular file, absent, a directory, a FIFO (in + a child process with a hard timeout), over the cap. +- `tests/install-records.test.ts` — layer tests. A FIFO at `pharn.records.json` → `invalid`, not a + hang (child process + timeout; FAILS on base by hanging); a directory → `invalid`, naming it. +- `tests/pharn-config.test.ts` — layer tests. A FIFO at `pharn.config.json` → `readPharnConfig` + returns `null` and `configFingerprint` an `unreadable:` value, not a hang (FAILS on base). +- `tests/project-lock.test.ts` — layer tests. A FIFO at `.pharn.lock` does not hang acquisition + (FAILS on base). +- `docs/reference/pharn-records.md` — layer docs. The unreadable list names a non-regular or + oversize file. +- `docs/reference/pharn-config.md` — layer docs. The same, for the config, where it names an + unreadable file. +- `docs/troubleshooting.md` — layer docs. The "cannot be read at all" list for the config gains a + FIFO, a device and a file over 16 MiB (amended during build: that sentence is where the config's + unreadable cases are listed). +- `CLAUDE.md` — layer docs. The `pharn-config` / `install-records` / `project-lock` descriptions. +- `CHANGELOG.md` — `[Unreleased]` → `### Security`, one entry. + +## Contracts satisfied + +- The read-side posture `hook-wiring.ts` and `detect-archetype.ts` already hold, extended to the + three files pharn owns (cited, P4). + +## Evals to write (P1) + +- Listed under Files. The three FIFO cases FAIL on the base by hanging (killed by the timeout). + +## Guarantee audit (P0) + +- "no pharn-owned project file can block a read" → floor: `O_NONBLOCK` + `fstat` in one reader, + plus the FIFO tests. +- "no such read is unbounded" → floor: the size cap, plus a test. + +## Trust audit (P2) + +- These files are local and hand-editable; their bytes are data. Nothing new is printed except a + fixed reason string. + +## Determinism audit (P5) + +- A type check and a size compare; every failure is a named outcome the callers already handle. + +## Open questions (HALT) + +None open. Resolved at GATE 1 (human, 2026-09-25): both questions below → **(a)**, the recommended +answer. Kept for the record: + +1. Scope. (a) All three pharn-owned files (records, config, lock), through one reader — + recommended: fixing only the records leaves the identical hang one file over. (b) Only + `pharn.records.json`, as first reported. +2. The cap. (a) 16 MiB for all three — recommended: records for ~160k files fit, the other two are + a few KB, and one number is easier to reason about. (b) A cap per file. diff --git a/.dev/features/local-file-reads/REGRESSION.md b/.dev/features/local-file-reads/REGRESSION.md new file mode 100644 index 0000000..dca6fb2 --- /dev/null +++ b/.dev/features/local-file-reads/REGRESSION.md @@ -0,0 +1,43 @@ +# REGRESSION — local-file-reads + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `9d2d574f4e22df1f796ad02e9c4209b9f088e7ff` (`HEAD` — `origin/main` after #224; the + build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): + - `src/lib/bounded-read.ts` (new), `src/lib/install-records.ts`, `src/lib/pharn-config.ts`, + `src/lib/project-lock.ts`, `src/steps/overwrite-check.ts`, `src/steps/install-archetype.ts` + - `tests/bounded-read.test.ts` (new), `tests/install-records.test.ts`, + `tests/pharn-config.test.ts`, `tests/project-lock.test.ts` + - `docs/reference/pharn-records.md`, `docs/troubleshooting.md` + - `CLAUDE.md`, `CHANGELOG.md` +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook + scratch) and this feature's own stage artifacts are not build output. +- **outside gates:** the 46 stdlib `*.test.mjs` / `*.test.cjs` files `scope` returned (754 tests) + + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / + `lint:md` are absent from both maps. +- **environment:** both sides ran with no proxy variables and as root **without** + `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`), the CI-equivalent of this + root sandbox. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` +is owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never +the increment. diff --git a/.dev/features/local-file-reads/REVIEW.md b/.dev/features/local-file-reads/REVIEW.md new file mode 100644 index 0000000..2edfeb1 --- /dev/null +++ b/.dev/features/local-file-reads/REVIEW.md @@ -0,0 +1,65 @@ +# REVIEW — local-file-reads + +Increment: a new `src/lib/bounded-read.ts` (`readBoundedFile`, `MAX_LOCAL_FILE_BYTES`). It is one +`O_NONBLOCK` descriptor, `fstat`-checked as a regular file of at most 16 MiB, read from that same +descriptor, and it never throws. Six call sites now read through it: + +- `readRecords`; +- `readPharnConfig` and `configFingerprint`; +- `recordedSkillsVersion` and `readCarriedEntries`, init's two tolerant config reads; +- `readRawAt`, the lock. + +Tests in four files, two docs, 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 (1664 tests), `/pharn-dev-regress` returned `no-regressions`, and `/pharn-dev-verify` +returned `PASS`. The chain ran twice: this review's first read found a platform inconsistency +(advisory finding 1), fixed within the plan's `## Files` before the second run. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0)** — "no pharn-owned project file can block a read" reduces to `O_NONBLOCK` + + `fstat` in one function, tested per file: records, config, and a young and an old lock, each in a + child with a hard timeout. "No such read is unbounded" reduces to the size compare, tested at the + cap, one byte over it, and past it mid-read (`/proc/self/status`, whose `fstat` says 0 bytes). +- **L-eval (P1)** — the five caller cases failed on the unchanged code: four hung until killed, and + one mislabeled a directory as "not valid JSON". The reader's own cases cannot load on the base + (new module). +- **L-trust (P2)** — the bytes are returned as data; only the fixed reason strings reach a message. + A symlinked file is followed as before, and its target is held to the same type and size checks. +- **L-axis (P3)** — one new module with one job. Each caller swaps its read and keeps its own + failure shape. + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/bounded-read.ts:64' + problem: 'RESOLVED IN THIS INCREMENT. Windows refuses to open a directory (EISDIR at open) where POSIX opens it and fstat says so, so the same directory would have been reported as "could not be read (EISDIR)" on one platform and "is not a regular file" on the other. Both now give the same reason.' + evidence: "if (code === 'EISDIR') {" +- type: FINDING + rule_id: 'P7' + severity: minor + file: 'src/lib/pharn-config.ts:309' + problem: 'A FIFO (or a config over 16 MiB) at pharn.config.json now reads as an unreadable config, which every command other than init answers with "No pharn.config.json found. Run `pharn init` first." That wording was already imprecise for a directory or a permissions problem, and the plan keeps the existing outcome on purpose; the troubleshooting page names these cases. A message that says "cannot be read" would be clearer, but it changes the documented contract of every command, so it is left for its own increment.' + evidence: "if (read.kind !== 'ok') return null;" +- type: FINDING + rule_id: 'P0' + severity: minor + file: 'src/lib/bounded-read.ts:48' + problem: 'On win32 O_NONBLOCK is absent and the flag is not set. Windows has no filesystem FIFOs, so there is nothing to block on, but the Windows path is untested in CI, as every win32 branch in this repo is.' + evidence: 'const OPEN_FLAGS = constants.O_RDONLY | (constants.O_NONBLOCK ?? 0);' +``` + +## Verdict + +**GREEN — 0 floor-gate findings, 3 advisory (minor; one resolved in this increment).** The standing +decision is the human's (GATE 2). diff --git a/.dev/features/local-file-reads/SHIP.md b/.dev/features/local-file-reads/SHIP.md new file mode 100644 index 0000000..d3c74a0 --- /dev/null +++ b/.dev/features/local-file-reads/SHIP.md @@ -0,0 +1,30 @@ +# SHIP — local-file-reads + +Stages run, in order: + +1. `/pharn-dev-plan` → GATE 1 (human: "fix all 3", then "Approve both (Recommended)": all three + pharn-owned files through one reader, a 16 MiB cap). +2. `/pharn-dev-grill`. Its three minor findings were folded into the build: + - the lock case is pinned to its existing grace rule, with a young and an old case; + - the reason strings are fixed; + - every FIFO case runs in a child process. +3. `/pharn-dev-build` → `/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review`. During the + build, `docs/troubleshooting.md` was added to the plan's `## Files`: it is where the config's + unreadable cases are listed. +4. `/pharn-dev-review` found one platform inconsistency (REVIEW.md advisory finding 1). It was fixed + within the plan's `## Files`, and build → regress → verify ran again. Everything below is from + that second run. +5. GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- The run ended at **GATE 2**. The human's standing instruction: open a pull request and merge it + once its checks are green, then start the next plan (init-preflight-first). + +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/local-file-reads/VERIFY.md b/.dev/features/local-file-reads/VERIFY.md new file mode 100644 index 0000000..855c998 --- /dev/null +++ b/.dev/features/local-file-reads/VERIFY.md @@ -0,0 +1,46 @@ +# VERIFY — local-file-reads + +## 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 (1664 tests). The four FIFO cases (records, config, a young lock, an old lock) + were run against the unchanged code first: each read hung in `open(2)` until its 15 s child + timeout killed it, and a directory at `pharn.records.json` was misreported as "not valid JSON". + With the reader, the FIFO cases return at once. +- The lock follows its existing rule for an unreadable payload: a young FIFO is refused as a live + lock, an old one is broken and replaced, and the command runs. +- `npm run test:coverage` passes its ratchet (97.48 / 92.81 / 98.34 / 98.35) and `npm run build` + exits 0; neither is a verdict gate, both are CI gates. +- `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: []`). + +## 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: + +- Win32 has no `O_NONBLOCK`; the flag is simply not set there, and Windows has no filesystem FIFOs. + The Windows path is not run in CI (as `CLAUDE.md` records for every `win32` branch). +- The reader closes the hang for the three files pharn owns. Files it reads through other routes + (the clone, the settings files, `package.json`, project files under `scanDest` / `readDiskState`) + keep their own, already-checked readers. diff --git a/.dev/features/local-file-reads/regression-report.json b/.dev/features/local-file-reads/regression-report.json new file mode 100644 index 0000000..e0b7353 --- /dev/null +++ b/.dev/features/local-file-reads/regression-report.json @@ -0,0 +1,32 @@ +{ + "base": "9d2d574f4e22df1f796ad02e9c4209b9f088e7ff", + "inside": [ + "CHANGELOG.md", + "CLAUDE.md", + "docs/reference/pharn-records.md", + "docs/troubleshooting.md", + "src/lib/bounded-read.ts", + "src/lib/install-records.ts", + "src/lib/pharn-config.ts", + "src/lib/project-lock.ts", + "src/steps/install-archetype.ts", + "src/steps/overwrite-check.ts", + "tests/bounded-read.test.ts", + "tests/install-records.test.ts", + "tests/pharn-config.test.ts", + "tests/project-lock.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/local-file-reads/verify-report.json b/.dev/features/local-file-reads/verify-report.json new file mode 100644 index 0000000..e8fdf46 --- /dev/null +++ b/.dev/features/local-file-reads/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "local-file-reads", + "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 abb39ff..791a96c 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/review-cleanups/SHIP.md" + ".dev/features/local-file-reads/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-25T13:05:36.351Z" + "set_at": "2026-09-25T13:32:23.481Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index e8dc3d1..411256a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **The extractor refuses a tar entry name it cannot write faithfully.** Names are decoded once, as strict UTF-8, and the check, the path rules and the write all use that one string. A name that holds a control or format character, is not valid UTF-8, or starts with a byte-order mark is refused. A raw C1 byte (such as `0x9B`, the terminal CSI) used to land on disk as that control character, where `pharn status` and `pharn update` then printed it raw, and every non-ASCII name was written garbled. - **A hostile archive can no longer exhaust memory or CPU, or be read leniently.** A 192 KB archive whose pax header inflated to about 128 MB took about 13 s and over 1 GB of memory to parse, and ran out of memory under a 512 MB heap, leaving the temporary download behind. Pax headers are now capped at 64 KiB each, and across all global headers in the archive together, and every header counts toward the entry limit. Framing is strict: a zero block mid-archive, a missing end marker, anything after it, a non-ustar header, and empty or `.` path segments are refused. - **A malformed numeric header field no longer reaches the error output raw.** It is shown with non-printable bytes escaped, so an archive cannot add a line of its own to the fatal message. +- **A FIFO or a device in place of a pharn file can no longer hang `pharn` or exhaust its memory.** A FIFO at `pharn.config.json`, `pharn.records.json` or `.pharn.lock` made every command that read it wait forever — `pharn update` and a re-run `pharn init` while holding the project lock, so every other `pharn` command in the project was refused too — and a symlink to `/dev/zero` there was read until memory ran out. These files are now read through one bounded, non-blocking reader: anything other than a regular file of at most 16 MiB is treated as unreadable, the way a directory or a permissions problem already was. ## [0.5.0] - 2026-09-10 diff --git a/CLAUDE.md b/CLAUDE.md index ec10439..3ffa00f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,7 +57,7 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten **`lib/validate.ts` is security-sensitive.** Untrusted names, versions, paths, and capability frontmatter are validated against strict regex/enum allowlists (`CAPABILITY_NAME_RE`, `VERSION_RE`, `COPY_FILENAME_RE`, `COMMIT_RE`, the `role`/`applies` enums), checked for `..`, and rejected on control chars. **`safeJoin` lives here** (relocated from the deleted `install-modules.ts`) — the lexical path-containment gate that `install-capabilities.ts`, `diff.ts`, `capability-index.ts`, `layout.ts`, `skills-version.ts`, and `remove.ts` all guard their fs access with, so nothing escapes its base dir (its strict-child sibling `safeChildJoin` — exactly one segment below the base, never the base — guards `remove`'s recursive delete) (`install-capabilities.ts` adds a symlink-aware backstop at the write sites). `toPosix` (the other purely lexical primitive — separator normalization + trailing-slash strip, relocated here once `install-manifest.ts` and `symlink-guard.ts` both needed it) sits beside it. The **physical** counterpart lives in **`lib/symlink-guard.ts`** — `findSymlinkComponent(base, rel)`, the single component walk that `backup.ts` (read side), `apply-update.ts` (write side), and `install-manifest.ts` (twice, skip side) all reach for; it returns the first symlinked component below `base` or `null`, and each **caller owns its failure shape** (backup and apply throw their own `ManifestValidationError` messages, the manifest skips), which is why the core returns a value instead of throwing. Nonexistent components deliberately pass — `applyWrites` creates parents _after_ the walk. That lexical/physical split is the point: `safeJoin` contains the path _string_, `findSymlinkComponent` refuses the path _on disk_. Remote fetches (`skills-version.ts`) use `redirect: 'error'`, an 8s timeout, and a 256KB body cap. Preserve these invariants. -**`lib/pharn-config.ts`** reads/writes `pharn.config.json` (`pharnVersion`, `skillsVersion`, `repo`, `commit`, `installedAt`, and — for an archetype install — `archetypes[]`, `capabilities[]` (`{name, role}`), `layout`, plus the `models`/`seam` blocks). Every `capabilities[]` entry is validated at ingest: it must be an object whose `name` matches `CAPABILITY_NAME_RE` (no control chars) and whose `role` is in `ROLE_VALUES`, else `CapabilityEntryError` — the config is committed and hand-editable, and `name` is path-joined by `remove`'s recursive delete (a `{"name":"../.."}` entry used to delete the project root). A present-but-invalid `capabilities[].source` (anything outside `{auto, manual}`; ABSENT is legal, P7) is rejected by `CapabilitySourceError`. Both join `isConfigValidationError`'s union. A present-but-invalid `models`/`seam` block is caught by its own validator — `lib/model-routing.ts` (`validateModelRouting`/`ModelRoutingError`) and `lib/seam-config.ts` (`validateSeamConfig`/`SeamConfigError`) — and `readPharnConfig` lets that named error PROPAGATE (never collapsing a bad hand-edit into the "run init" path); `isConfigValidationError` + `loadConfigOrExit` catch and report it, and the validated/stripped blocks replace the raw ones. Schema is additive — a legacy config's now-unused `modules[]`/`constitution`/`stackAnswers`/`installedSkills[]` still load (P7). `isArchetypeConfig` (= `Array.isArray(config.capabilities)`) is the deterministic discriminator; **`loadArchetypeConfigOrExit`** is the shared load-or-reject surface for `add`/`update`/`status`/`remove` (a pre-archetype config → `LEGACY_CONFIG_MESSAGE` + exit(1), never a fetch; `list` keeps its own json-aware check so `--json` stderr stays clean). `add`/`update`/`remove` update the config in place, and each calls `assertConfigUnchanged` as the FIRST step inside its `.pharn.lock` (`lib/project-lock.ts`): the config is loaded before the lock (and across `update`'s / the `remove` picker's confirm), so a concurrent run could otherwise be overwritten from a stale snapshot; a mismatch throws `ProjectChangedError` (a `ProjectLockedError`, so every catch site reports it as a named refusal, exit 1, nothing written). The lock itself treats an unparseable lock file younger than `MALFORMED_GRACE_MS` (10 s) as LIVE — `tryCreate` writes its payload after the `O_EXCL` create, so a live lock is briefly empty — and only breaks older ones. +**`lib/pharn-config.ts`** reads/writes `pharn.config.json` (`pharnVersion`, `skillsVersion`, `repo`, `commit`, `installedAt`, and — for an archetype install — `archetypes[]`, `capabilities[]` (`{name, role}`), `layout`, plus the `models`/`seam` blocks). Every `capabilities[]` entry is validated at ingest: it must be an object whose `name` matches `CAPABILITY_NAME_RE` (no control chars) and whose `role` is in `ROLE_VALUES`, else `CapabilityEntryError` — the config is committed and hand-editable, and `name` is path-joined by `remove`'s recursive delete (a `{"name":"../.."}` entry used to delete the project root). A present-but-invalid `capabilities[].source` (anything outside `{auto, manual}`; ABSENT is legal, P7) is rejected by `CapabilitySourceError`. Both join `isConfigValidationError`'s union. A present-but-invalid `models`/`seam` block is caught by its own validator — `lib/model-routing.ts` (`validateModelRouting`/`ModelRoutingError`) and `lib/seam-config.ts` (`validateSeamConfig`/`SeamConfigError`) — and `readPharnConfig` lets that named error PROPAGATE (never collapsing a bad hand-edit into the "run init" path); `isConfigValidationError` + `loadConfigOrExit` catch and report it, and the validated/stripped blocks replace the raw ones. Schema is additive — a legacy config's now-unused `modules[]`/`constitution`/`stackAnswers`/`installedSkills[]` still load (P7). `isArchetypeConfig` (= `Array.isArray(config.capabilities)`) is the deterministic discriminator; **`loadArchetypeConfigOrExit`** is the shared load-or-reject surface for `add`/`update`/`status`/`remove` (a pre-archetype config → `LEGACY_CONFIG_MESSAGE` + exit(1), never a fetch; `list` keeps its own json-aware check so `--json` stderr stays clean). `add`/`update`/`remove` update the config in place, and each calls `assertConfigUnchanged` as the FIRST step inside its `.pharn.lock` (`lib/project-lock.ts`): the config is loaded before the lock (and across `update`'s / the `remove` picker's confirm), so a concurrent run could otherwise be overwritten from a stale snapshot; a mismatch throws `ProjectChangedError` (a `ProjectLockedError`, so every catch site reports it as a named refusal, exit 1, nothing written). The lock itself treats an unparseable lock file younger than `MALFORMED_GRACE_MS` (10 s) as LIVE — `tryCreate` writes its payload after the `O_EXCL` create, so a live lock is briefly empty — and only breaks older ones. All three files pharn keeps in a project — `pharn.config.json` (`readPharnConfig`, `configFingerprint`, and init's two tolerant reads), `pharn.records.json` (`readRecords`) and `.pharn.lock` (`readRawAt`) — are read through **`lib/bounded-read.ts`** (`readBoundedFile`): one descriptor opened `O_NONBLOCK`, `fstat`-checked as a regular file of at most `MAX_LOCAL_FILE_BYTES` (16 MiB), read from that same descriptor, never throwing. A FIFO, a device or a directory there is therefore each caller's existing "unreadable" outcome (config → `null` / `unreadable:`, records → a named `invalid`, lock → presumed live, then broken), never a hang in `open(2)` and never an unbounded read — the posture `hook-wiring.ts` and `detect-archetype.ts` already held for the files they read. **`pharn add` addressing** (`commands/add.ts` + `lib/capability-address.ts`). `add ` or `add :` (e.g. `add a11y`, `add lens:n-plus-one`) installs one capability into an archetype project — a manual override of archetype auto-selection. It clones pharn-oss (SHA-pinned), then applies **the version gate**: a local `versionGate` helper, called ONCE per command from INSIDE each path's existing `try` (so `readSkillsVersion`'s throw still reaches the `finally` that cleans up the clone), refuses when `readSkillsVersion(repo.dir) !== config.skillsVersion` — reusing the existing `{kind:'error'}` outcome → `exit(1)`. It fires on `!==` (never `<`, so a rollback reads the same) — except that a clone at the config's additive `pendingSkillsVersion` also passes: `update` records that field only when it withheld the bump SOLELY because of `modified`/`unrecorded` skips (the user's kept edits; every other file is at that version) and clears it on the next complete run; at a pending version `add` keeps the config's own `(skillsVersion, commit)` pair (`addStamp`) so the records stamp stays consistent, fires **gate-first** (before the already-installed no-op and before the picker's `all-installed`, and before `groupMultiselect` renders), and writes nothing. A sibling **layout gate** (`layoutGate`) sits immediately after it at both call sites, `??`-chained (`versionGate(…) ?? layoutGate(…)`) so the **version** refusal wins when both mismatch — by short-circuit evaluation, not statement order: it refuses when `detectLayout(repo.dir) !== configLayout(config)`, naming both resolved layouts and `pharn update --force`. `add` copies at the CLONE's layout (`installCapabilityDirs`' default) and records at it (`mergeCapabilityRecords`), while `remove`/`status`/`diff.ts` address the project at `configLayout` — so without the gate a mismatched add lands files nothing ever looks at, and the next `remove` drops the config entry reporting "its files were already gone", orphaning the dir. `add` must NEVER record the clone's layout the way `update` does: `update` may only because it rewrites the WHOLE tree at that layout, whereas `add` writes one capability, so stamping `layout` here would re-address every other already-placed file. Comparing `configLayout(config)` (not the raw `config.layout`) is the point — agreement with the readers is the invariant, and it makes a garbage hand-edited value resolve to `flat` and fail closed. This is what keeps `update`'s `config.skillsVersion === latest` early-return honest: `add` must never stamp a newer `skillsVersion` over unchanged old bytes. `add` therefore refreshes `commit` but NEVER `skillsVersion` (the legal same-version-different-commit case). Past the gate it resolves the arg against `parseCapabilityIndex`, and if it uniquely names a not-yet-installed capability, copies it via `installCapabilityDirs` and **appends** to `capabilities` with `source: 'manual'` (never touches `archetypes`). That tag is what makes the override survive `update`, and it is written at BOTH entry-construction sites — `resolveArchetypeAdd` and the picker's threaded `cfg` mirror, which the next pick spreads into its own config write. Already-installed → no-op; unknown/ambiguous → lists the valid `role:name` addresses. **Before the copy, `add` backs up destination drift**: `capabilityCloneFiles` (`lib/install-manifest.ts`) enumerates the capability dir in the CLONE, `scanDest` (`lib/dest-drift.ts`) keeps the rels whose dest exists as a regular file and whose `sha256File` DIFFERS from the clone's, and a non-empty set goes to `createBackup` (`lib/backup.ts` — the same `.pharn-backup//` `update --force` writes) BEFORE the first `cpSync`, logged at creation so the pointer survives a later throw. `add` was the only write path with none of the product's three edit-protections (init's prompt, update's per-file skip, `--force`'s backup), and `update` itself manufactures the reachable sequence: a `dropped-unselected` capability's files are left on disk, the user edits them, and the later `add` is not a config no-op. Byte-identical is NOT drift (mirrors update's `identical → no-op`), so a drop-then-re-add of unedited files stays silent. The scan SKIPS an absent/symlinked/non-dir source rather than throwing, so `installCapabilityDirs`' curated `Capability "x" (griller) is missing at …` still wins — the same read-skips/write-throws split `install-manifest.ts`'s `addDir` already uses. `scanDest` returns a PARTITION, not just the drift set: a rel whose PROJECT-side path crosses a symlinked component is `unsafe`, and `add` **refuses the whole install** naming the component. That is measured, not defensive — on node v24.13.1 `cpSync` guards only the source, so a symlinked INTERMEDIATE dir under the capability dir is written straight THROUGH (bytes outside the project replaced), while a symlinked leaf is REPLACED and a symlinked capability root throws `ERR_FS_CP_DIR_TO_NON_DIR`. Skipping would be strictly worse than doing nothing (the copy writes through anyway while the backup omits the file), and backing up is impossible (`createBackup` refuses symlinked components; `copyFileSync` would save the link's TARGET). An ENOTDIR walk (a component below a regular file) is NOT unsafe — `cpSync` throws on that tree by itself — so it stays a skip, which is also what keeps a non-directory component out of the set `createBackup` consumes unwrapped. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one), keyed by that SAME clone-derived list — never a walk of the destination, which used to sweep a user's own file inside a leftover capability dir into the store as pharn-written and let a later `update` read it as cleanly upgradeable instead of `modified`; the hashes are still taken at the DEST (`buildRecords`), so a record can never disagree with what landed. `CONSTITUTION.md` is **not** touched — `add` installs capability dirs only. `pharn update` re-resolves the **recorded archetypes** against the latest index and re-copies — drift-safely, see below. diff --git a/docs/reference/pharn-records.md b/docs/reference/pharn-records.md index 5a8ae44..b73c7d3 100644 --- a/docs/reference/pharn-records.md +++ b/docs/reference/pharn-records.md @@ -56,9 +56,10 @@ absent. differs from upstream, labelling them `unverifiable` — whenever it is: - **absent** (an install created before `pharn` 0.4.0); -- **unreadable or malformed** — invalid JSON, not an object, a missing `files` object, a non-sha256 - hash, or a path key that is empty, absolute, or has a `..` or `.` path **segment**. The key rule is - a segment rule, not a substring ban: an ordinary filename that merely _contains_ `..`, such as +- **unreadable or malformed** — not a regular file (a directory, a FIFO or a device) or larger than + 16 MiB, invalid JSON, not an object, a missing `files` object, a non-sha256 hash, or a path key + that is empty, absolute, or has a `..` or `.` path **segment**. The key rule is a segment rule, + not a substring ban: an ordinary filename that merely _contains_ `..`, such as `migration..v2.md`, is valid, because a key here is only ever compared against the install manifest — never used to build a path. Any one of these invalidates the **whole** store rather than a single entry, and the reason is reported by name so a fixable JSON error is not mistaken for a diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 0bf8efd..483d19d 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -487,7 +487,8 @@ had installed manually with `pharn add `. Two neighbouring cases are deliberately **not** reported this way, because the file is not readable in the first place: a config that is **absent**, or one that exists but cannot be read at all (a -permissions problem, or a directory sitting at that path), still says +permissions problem, a directory, a FIFO or a device sitting at that path, or a file over 16 MiB), +still says [`No pharn.config.json found`](#add--update-say-to-run-init-first). ## Unknown command diff --git a/src/lib/bounded-read.ts b/src/lib/bounded-read.ts new file mode 100644 index 0000000..5c5b15f --- /dev/null +++ b/src/lib/bounded-read.ts @@ -0,0 +1,107 @@ +import { closeSync, constants, fstatSync, openSync, readSync } from 'node:fs'; + +// --------------------------------------------------------------------------- +// The one reader for the files pharn keeps in a project — pharn.records.json, +// pharn.config.json and .pharn.lock. It returns their bytes, `absent`, or +// `unusable` with a fixed reason, and it never hangs and never reads without +// bound. +// +// Why not `readFileSync`: on a FIFO, a plain open(2) waits for a writer that +// never comes, so one planted at any of these paths hung every command that +// read it — `update` and a re-run `init` while holding the project lock, which +// in turn refused every other pharn command in the project. On a device (a +// symlink to /dev/zero) it read until memory ran out. The readers of +// user-adjacent files already had this shape (lib/hook-wiring.ts for the +// settings files, lib/detect-archetype.ts for package.json); these three never +// got it. +// +// The shape, the same as theirs: ONE descriptor, opened `O_NONBLOCK` (absent on +// win32, where the flag is simply not set), so opening a FIFO returns at once; +// `fstat` on that descriptor must show a regular file within the cap; the bytes +// are read from that same descriptor, so the file checked is the file read. A +// symlinked FILE is followed, as a plain read follows it — these are the user's +// own files — and its target is held to the same checks. +// +// Every failure is a value, never a throw: callers already have an "unreadable" +// outcome (records → `invalid`, config → `null`, lock → presumed live, then +// broken), and this only routes more cases into it. Determinism (P5): a type +// check and a size compare. Trust (P2): bytes are returned as data; only the +// fixed reason strings below ever reach a message. +// --------------------------------------------------------------------------- + +/** The most bytes pharn reads from one of its own project files. */ +export const MAX_LOCAL_FILE_BYTES = 16 * 1024 * 1024; + +export type BoundedRead = + | { kind: 'ok'; bytes: Buffer } + | { kind: 'absent' } + | { + kind: 'unusable'; + // An errno, or `not-a-file` / `too-large`. A directory reports `EISDIR`, + // the errno a plain read gives it, so a value recorded before this reader + // existed (a config fingerprint) keeps its meaning. + code: string; + // Completes " …" in a message: fixed text, never file content. + reason: string; + }; + +const OPEN_FLAGS = constants.O_RDONLY | (constants.O_NONBLOCK ?? 0); +const NOT_A_FILE = 'is not a regular file'; + +/** Read `path` if it is a regular file of at most `maxBytes` bytes. */ +export function readBoundedFile( + path: string, + maxBytes: number = MAX_LOCAL_FILE_BYTES, +): BoundedRead { + let fd: number; + try { + fd = openSync(path, OPEN_FLAGS); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code ?? 'EUNKNOWN'; + if (code === 'ENOENT') return { kind: 'absent' }; + // Windows refuses to open a directory at all (EISDIR), where POSIX opens it + // and the fstat below says so: same file, same reason, either way. + if (code === 'EISDIR') { + return { kind: 'unusable', code, reason: NOT_A_FILE }; + } + return { kind: 'unusable', code, reason: `could not be read (${code})` }; + } + try { + const stat = fstatSync(fd); + if (!stat.isFile()) { + return { + kind: 'unusable', + code: stat.isDirectory() ? 'EISDIR' : 'not-a-file', + reason: NOT_A_FILE, + }; + } + const tooLarge: BoundedRead = { + kind: 'unusable', + code: 'too-large', + reason: `is larger than ${formatBytes(maxBytes)}`, + }; + if (stat.size > maxBytes) return tooLarge; + // Read to EOF, never past the cap: the file can grow after the fstat. + const chunks: Buffer[] = []; + let total = 0; + const chunk = Buffer.alloc(Math.min(64 * 1024, maxBytes + 1)); + for (;;) { + const n = readSync(fd, chunk, 0, chunk.length, null); + if (n === 0) break; + total += n; + if (total > maxBytes) return tooLarge; + chunks.push(Buffer.from(chunk.subarray(0, n))); + } + return { kind: 'ok', bytes: Buffer.concat(chunks, total) }; + } catch (err) { + const code = (err as NodeJS.ErrnoException).code ?? 'EUNKNOWN'; + return { kind: 'unusable', code, reason: `could not be read (${code})` }; + } finally { + closeSync(fd); + } +} + +function formatBytes(n: number): string { + const mib = 1024 * 1024; + return n % mib === 0 && n >= mib ? `${n / mib} MiB` : `${n} bytes`; +} diff --git a/src/lib/install-records.ts b/src/lib/install-records.ts index 2f434ce..e0751c9 100644 --- a/src/lib/install-records.ts +++ b/src/lib/install-records.ts @@ -1,5 +1,6 @@ -import { existsSync, lstatSync, readFileSync } from 'node:fs'; +import { lstatSync } from 'node:fs'; import { writeJsonAtomic } from './atomic-write.js'; +import { readBoundedFile } from './bounded-read.js'; import { resolve } from 'node:path'; import { sha256File } from './hash.js'; import { isPlainObject, safeJoin, toPosix } from './validate.js'; @@ -123,12 +124,17 @@ export function recordsPath(cwd: string): string { * safe terminal (P5). */ export function readRecords(cwd: string): ReadRecordsResult { - const path = recordsPath(cwd); - if (!existsSync(path)) return { kind: 'absent' }; + // Through the bounded, non-blocking reader (lib/bounded-read.ts): a FIFO, a + // device or a directory at this path is a named `invalid`, never a hang. + const read = readBoundedFile(recordsPath(cwd)); + if (read.kind === 'absent') return { kind: 'absent' }; + if (read.kind === 'unusable') { + return { kind: 'invalid', message: `${RECORDS_FILE} ${read.reason}` }; + } let raw: unknown; try { - raw = JSON.parse(readFileSync(path, 'utf8')); + raw = JSON.parse(read.bytes.toString('utf8')); } catch { return { kind: 'invalid', message: `${RECORDS_FILE} is not valid JSON` }; } diff --git a/src/lib/pharn-config.ts b/src/lib/pharn-config.ts index 5b49a33..e781178 100644 --- a/src/lib/pharn-config.ts +++ b/src/lib/pharn-config.ts @@ -1,6 +1,6 @@ import { createHash } from 'node:crypto'; -import { existsSync, readFileSync } from 'node:fs'; import { writeJsonAtomic } from './atomic-write.js'; +import { readBoundedFile } from './bounded-read.js'; import { resolve } from 'node:path'; import { errorMessage, logError } from './report-error.js'; import { @@ -299,18 +299,15 @@ export function userOwnedConfigEntries( */ export function readPharnConfig(cwd: string): PharnConfig | null { const path = configPath(cwd); - if (!existsSync(path)) return null; - // The read and the parse are DELIBERATELY separate tries. An unreadable file - // (EACCES, or a directory planted at this path → EISDIR) keeps the OLD - // behaviour — `null`, the caller's "run init" — because that is a different, - // pre-existing failure, and folding it in here would newly mislabel a - // permissions problem as a syntax error. Narrow on purpose (P7). - let text: string; - try { - text = readFileSync(path, 'utf8'); - } catch { - return null; - } + // The read and the parse are DELIBERATELY separate steps. An unreadable file + // (EACCES, a directory planted at this path, a FIFO or a device — read through + // lib/bounded-read.ts, so none of them can hang or read without bound) keeps + // the OLD behaviour — `null`, the caller's "run init" — because that is a + // different, pre-existing failure, and folding it in here would newly + // mislabel a permissions problem as a syntax error. Narrow on purpose (P7). + const read = readBoundedFile(path); + if (read.kind !== 'ok') return null; + const text = read.bytes.toString('utf8'); // Present + readable + not JSON → the named throw. This try does NOT wrap the // validators below — that is the whole point (BUG 1). let raw: unknown; @@ -478,14 +475,10 @@ function configChangedError(command: string): ProjectChangedError { * content hash of the bytes is (P0: content-hash, `ARCHITECTURE.md §2`). */ export function configFingerprint(cwd: string): string { - let bytes: Buffer; - try { - bytes = readFileSync(configPath(cwd)); - } catch (err) { - const code = (err as NodeJS.ErrnoException).code; - return code === 'ENOENT' ? 'absent' : `unreadable:${code ?? 'unknown'}`; - } - return `sha256:${createHash('sha256').update(bytes).digest('hex')}`; + const read = readBoundedFile(configPath(cwd)); + if (read.kind === 'absent') return 'absent'; + if (read.kind === 'unusable') return `unreadable:${read.code}`; + return `sha256:${createHash('sha256').update(read.bytes).digest('hex')}`; } /** diff --git a/src/lib/project-lock.ts b/src/lib/project-lock.ts index d6ba32e..5c04cfc 100644 --- a/src/lib/project-lock.ts +++ b/src/lib/project-lock.ts @@ -3,7 +3,6 @@ import { closeSync, linkSync, openSync, - readFileSync, readdirSync, renameSync, rmSync, @@ -11,6 +10,7 @@ import { writeSync, } from 'node:fs'; import { hostname } from 'node:os'; +import { readBoundedFile } from './bounded-read.js'; import { onFatalSignal } from './fatal-signal.js'; import { safeJoin } from './validate.js'; @@ -171,11 +171,11 @@ function parsePayload(raw: string): LockPayload | null { * `null` is the same never-wedge rule `parsePayload` follows. */ function readRawAt(path: string): string | null { - try { - return readFileSync(path, 'utf8'); - } catch { - return null; - } + // Bounded and non-blocking (lib/bounded-read.ts): a FIFO at the lock path + // reads as unreadable — presumed live, then broken, like any malformed lock — + // instead of hanging every writer in open(2). + const read = readBoundedFile(path); + return read.kind === 'ok' ? read.bytes.toString('utf8') : null; } /** Read the lock file, or `null` when it is absent or unreadable. */ diff --git a/src/steps/install-archetype.ts b/src/steps/install-archetype.ts index 1653746..e6978b9 100644 --- a/src/steps/install-archetype.ts +++ b/src/steps/install-archetype.ts @@ -1,4 +1,3 @@ -import { readFileSync } from 'node:fs'; import { log, outro, spinner } from '@clack/prompts'; import pc from 'picocolors'; import { FIRST_FEATURE_COMMAND, REPO_URL } from '../lib/constants.js'; @@ -10,6 +9,7 @@ import { collectExpectedInstallPaths } from '../lib/install-manifest.js'; import { detectLayout, layoutPaths } from '../lib/layout.js'; import { manifestSources, scanDest } from '../lib/dest-drift.js'; import { createBackup } from '../lib/backup.js'; +import { readBoundedFile } from '../lib/bounded-read.js'; import { buildRecords, readRecords, @@ -369,8 +369,11 @@ function keptRecords( * importable from the module whose point is that it throws. */ function readCarriedEntries(cwd: string): Record { + // Bounded and non-blocking (lib/bounded-read.ts), like every read of this file. + const read = readBoundedFile(configPath(cwd)); + if (read.kind !== 'ok') return {}; try { - const raw: unknown = JSON.parse(readFileSync(configPath(cwd), 'utf8')); + const raw: unknown = JSON.parse(read.bytes.toString('utf8')); return isPlainObject(raw) ? userOwnedConfigEntries(raw) : {}; } catch { return {}; diff --git a/src/steps/overwrite-check.ts b/src/steps/overwrite-check.ts index 5ed3f0d..655bedb 100644 --- a/src/steps/overwrite-check.ts +++ b/src/steps/overwrite-check.ts @@ -1,4 +1,3 @@ -import { readFileSync } from 'node:fs'; import { confirm, isCancel, log } from '@clack/prompts'; import { collectExpectedInstallPaths, @@ -12,6 +11,7 @@ import { type DriftLabel, } from '../lib/dest-drift.js'; import { BACKUP_DIR } from '../lib/backup.js'; +import { readBoundedFile } from '../lib/bounded-read.js'; import type { FileRecords } from '../lib/install-records.js'; import { safeJoin, VERSION_RE } from '../lib/validate.js'; import type { Selection } from '../types.js'; @@ -66,9 +66,12 @@ export const MAX_LISTED = 10; // even though PHARN_CONFIG_FILE is a compile-time constant. Zero network: this // is the local config's number, never upstream's. function recordedSkillsVersion(cwd: string): string | null { + // Bounded and non-blocking (lib/bounded-read.ts): a FIFO here must not hang + // the prompt it only decorates. + const read = readBoundedFile(safeJoin(cwd, PHARN_CONFIG_FILE)); + if (read.kind !== 'ok') return null; try { - const raw = readFileSync(safeJoin(cwd, PHARN_CONFIG_FILE), 'utf8'); - const parsed: unknown = JSON.parse(raw); + const parsed: unknown = JSON.parse(read.bytes.toString('utf8')); if (typeof parsed !== 'object' || parsed === null) return null; const version = (parsed as { skillsVersion?: unknown }).skillsVersion; return typeof version === 'string' && VERSION_RE.test(version) diff --git a/tests/bounded-read.test.ts b/tests/bounded-read.test.ts new file mode 100644 index 0000000..4ca030b --- /dev/null +++ b/tests/bounded-read.test.ts @@ -0,0 +1,136 @@ +import { execFileSync, spawnSync } from 'node:child_process'; +import { chmodSync, mkdirSync, symlinkSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { useTmpDir } from './helpers.js'; +import { + MAX_LOCAL_FILE_BYTES, + readBoundedFile, +} from '../src/lib/bounded-read.js'; + +// --------------------------------------------------------------------------- +// The one reader for the files pharn keeps in a project: bytes, absent, or +// unusable with a fixed reason — never a hang, never an unbounded read. +// --------------------------------------------------------------------------- + +describe('readBoundedFile', () => { + const tmp = useTmpDir(); + const at = (name: string): string => join(tmp.path(), name); + + it('reads a regular file', () => { + writeFileSync(at('f.json'), '{"a":1}'); + expect(readBoundedFile(at('f.json'))).toEqual({ + kind: 'ok', + bytes: Buffer.from('{"a":1}'), + }); + }); + + it('reports an absent file as absent', () => { + expect(readBoundedFile(at('nope.json'))).toEqual({ kind: 'absent' }); + }); + + it('refuses a directory, with the errno a plain read would have given', () => { + mkdirSync(at('d.json')); + expect(readBoundedFile(at('d.json'))).toEqual({ + kind: 'unusable', + code: 'EISDIR', + reason: 'is not a regular file', + }); + }); + + it('refuses a file over the cap, and reads one exactly at it', () => { + writeFileSync(at('big'), 'x'.repeat(9)); + expect(readBoundedFile(at('big'), 8)).toEqual({ + kind: 'unusable', + code: 'too-large', + reason: 'is larger than 8 bytes', + }); + writeFileSync(at('edge'), 'x'.repeat(8)); + expect(readBoundedFile(at('edge'), 8)).toMatchObject({ kind: 'ok' }); + }); + + it('names the default cap in MiB', () => { + expect(MAX_LOCAL_FILE_BYTES).toBe(16 * 1024 * 1024); + writeFileSync(at('huge'), Buffer.alloc(MAX_LOCAL_FILE_BYTES + 1)); + expect(readBoundedFile(at('huge'))).toMatchObject({ + kind: 'unusable', + reason: 'is larger than 16 MiB', + }); + }); + + // The size check alone is not the bound: a file can grow after the fstat. + // procfs is a deterministic stand-in — its files are regular, fstat says 0 + // bytes, and a read returns more than that. + it.runIf(process.platform === 'linux')( + 'stops at the cap while reading, not only at the fstat size', + () => { + expect(readBoundedFile('/proc/self/status', 8)).toEqual({ + kind: 'unusable', + code: 'too-large', + reason: 'is larger than 8 bytes', + }); + }, + ); + + it('reports any other open failure by its errno', () => { + writeFileSync(at('locked.json'), '{}'); + chmodSync(at('locked.json'), 0o000); + try { + // (Inert for uid 0 with CAP_DAC_OVERRIDE — this suite must not run as + // root, like the other permission cases in it.) + expect(readBoundedFile(at('locked.json'))).toEqual({ + kind: 'unusable', + code: 'EACCES', + reason: 'could not be read (EACCES)', + }); + } finally { + chmodSync(at('locked.json'), 0o644); + } + }); + + it('follows a symlink to a regular file, as a plain read does', () => { + writeFileSync(at('real.json'), 'ok'); + symlinkSync(at('real.json'), at('link.json')); + expect(readBoundedFile(at('link.json'))).toEqual({ + kind: 'ok', + bytes: Buffer.from('ok'), + }); + }); + + // A device reads forever. It is refused on its type, before any byte. + it('refuses a symlink to /dev/zero without reading it', () => { + symlinkSync('/dev/zero', at('zero.json')); + expect(readBoundedFile(at('zero.json'))).toEqual({ + kind: 'unusable', + code: 'not-a-file', + reason: 'is not a regular file', + }); + }); + + // A blocking open(2) on a FIFO waits for a writer that never comes. The read + // runs in a child with a hard timeout, so a regression fails here as a + // timeout instead of hanging the test worker (as tests/hook-wiring.test.ts + // does for its settings files). + it('refuses a FIFO without waiting on it', () => { + execFileSync('mkfifo', [at('pipe.json')]); + const mod = fileURLToPath( + new URL('../src/lib/bounded-read.ts', import.meta.url), + ); + const script = ` + const { readBoundedFile } = await import(${JSON.stringify(mod)}); + console.log(JSON.stringify(readBoundedFile(process.argv[1]))); + `; + const r = spawnSync( + process.execPath, + ['--import', 'tsx', '--input-type=module', '-e', script, at('pipe.json')], + { encoding: 'utf8', timeout: 15_000 }, + ); + expect(r.signal, 'the read hung and was killed').toBeNull(); + expect(JSON.parse(r.stdout)).toEqual({ + kind: 'unusable', + code: 'not-a-file', + reason: 'is not a regular file', + }); + }, 30_000); +}); diff --git a/tests/install-records.test.ts b/tests/install-records.test.ts index 775823d..d18426b 100644 --- a/tests/install-records.test.ts +++ b/tests/install-records.test.ts @@ -1,3 +1,4 @@ +import { execFileSync, spawnSync } from 'node:child_process'; import { createHash } from 'node:crypto'; import { mkdirSync, @@ -7,6 +8,7 @@ import { writeFileSync, } from 'node:fs'; import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { describe, expect, it } from 'vitest'; import { useTmpDir } from './helpers.js'; import { @@ -534,3 +536,41 @@ describe('buildRecords — an unstatable dest is skipped, not fatal', () => { expect(Object.keys(files)).toEqual(['CONSTITUTION.md']); }); }); + +// A store that is not a regular file is `invalid`, named — never a hang and +// never an unbounded read. `update` reads it on every run and a re-run `init` +// before its prompt and under the lock, so a FIFO here used to hang both (in +// open(2), waiting for a writer). That case runs in a child with a hard timeout: +// a regression would hang the test worker instead of failing. +describe('readRecords — a store that is not a regular file', () => { + const tmp = useTmpDir(); + + it('names a directory as not a regular file', () => { + mkdirSync(join(tmp.path(), RECORDS_FILE)); + expect(readRecords(tmp.path())).toEqual({ + kind: 'invalid', + message: `${RECORDS_FILE} is not a regular file`, + }); + }); + + it('reads a FIFO as invalid instead of waiting on it', () => { + execFileSync('mkfifo', [join(tmp.path(), RECORDS_FILE)]); + const mod = fileURLToPath( + new URL('../src/lib/install-records.ts', import.meta.url), + ); + const script = ` + const { readRecords } = await import(${JSON.stringify(mod)}); + console.log(JSON.stringify(readRecords(process.argv[1]))); + `; + const r = spawnSync( + process.execPath, + ['--import', 'tsx', '--input-type=module', '-e', script, tmp.path()], + { encoding: 'utf8', timeout: 15_000 }, + ); + expect(r.signal, 'the read hung and was killed').toBeNull(); + expect(JSON.parse(r.stdout)).toEqual({ + kind: 'invalid', + message: `${RECORDS_FILE} is not a regular file`, + }); + }, 30_000); +}); diff --git a/tests/pharn-config.test.ts b/tests/pharn-config.test.ts index 5bc318b..2c595b2 100644 --- a/tests/pharn-config.test.ts +++ b/tests/pharn-config.test.ts @@ -1,3 +1,4 @@ +import { execFileSync, spawnSync } from 'node:child_process'; import { mkdirSync, readFileSync, @@ -6,6 +7,7 @@ import { writeFileSync, } from 'node:fs'; import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { afterEach, describe, expect, it, vi } from 'vitest'; import { log } from '@clack/prompts'; import { ProcessExit, stubProcessExit, useTmpDir } from './helpers.js'; @@ -922,3 +924,36 @@ describe('frozenCapabilities ingest', () => { ); }); }); + +// Every command reads pharn.config.json, so a FIFO there used to hang all of +// them in open(2). It is now an unreadable config: `readPharnConfig` gives the +// same `null` an unreadable file always gave, and the fingerprint names it. +// Run in a child with a hard timeout — a regression would hang the worker. +describe('pharn.config.json that is not a regular file', () => { + const tmp = useTmpDir(); + + it('reads a FIFO as unreadable instead of waiting on it', () => { + execFileSync('mkfifo', [join(tmp.path(), 'pharn.config.json')]); + const mod = fileURLToPath( + new URL('../src/lib/pharn-config.ts', import.meta.url), + ); + const script = ` + const m = await import(${JSON.stringify(mod)}); + const dir = process.argv[1]; + console.log(JSON.stringify({ + config: m.readPharnConfig(dir), + fingerprint: m.configFingerprint(dir), + })); + `; + const r = spawnSync( + process.execPath, + ['--import', 'tsx', '--input-type=module', '-e', script, tmp.path()], + { encoding: 'utf8', timeout: 15_000 }, + ); + expect(r.signal, 'the read hung and was killed').toBeNull(); + expect(JSON.parse(r.stdout)).toEqual({ + config: null, + fingerprint: 'unreadable:not-a-file', + }); + }, 30_000); +}); diff --git a/tests/project-lock.test.ts b/tests/project-lock.test.ts index 7979707..1910da3 100644 --- a/tests/project-lock.test.ts +++ b/tests/project-lock.test.ts @@ -1,3 +1,4 @@ +import { execFileSync, spawnSync } from 'node:child_process'; import { existsSync, mkdirSync, @@ -10,6 +11,7 @@ import { } from 'node:fs'; import { hostname } from 'node:os'; import { join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { describe, expect, it, vi } from 'vitest'; import { useTmpDir } from './helpers.js'; import { @@ -634,3 +636,50 @@ describe('withProjectLock — a fatal signal while held', () => { expect(r.stderr).not.toContain('interrupted while writing'); }, 40_000); }); + +// A FIFO at .pharn.lock used to hang every writer in open(2), reading the +// payload of a lock that could never be written. It now follows the rule for +// any lock whose payload cannot be read: presumed live while younger than +// MALFORMED_GRACE_MS (a holder mid-create looks the same), broken once older. +// In a child with a hard timeout — a regression would hang the worker. +describe('withProjectLock — a FIFO at the lock path', () => { + const tmp = useTmpDir(); + + function acquireInChild(dir: string): { signal: string | null; out: string } { + const mod = fileURLToPath( + new URL('../src/lib/project-lock.ts', import.meta.url), + ); + const script = ` + const { withProjectLock } = await import(${JSON.stringify(mod)}); + try { + await withProjectLock(process.argv[1], 'update', async () => undefined); + console.log('ran'); + } catch (err) { + console.log(err.name); + } + `; + const r = spawnSync( + process.execPath, + ['--import', 'tsx', '--input-type=module', '-e', script, dir], + { encoding: 'utf8', timeout: 15_000 }, + ); + return { signal: r.signal, out: r.stdout.trim() }; + } + + it('refuses while the FIFO is young, as for any unreadable lock', () => { + execFileSync('mkfifo', [lockPath(tmp.path())]); + const { signal, out } = acquireInChild(tmp.path()); + expect(signal, 'the read hung and was killed').toBeNull(); + expect(out).toBe('ProjectLockedError'); + }, 30_000); + + it('breaks it once it is older than the grace window, and runs', () => { + execFileSync('mkfifo', [lockPath(tmp.path())]); + const old = (Date.now() - MALFORMED_GRACE_MS - 60_000) / 1000; + utimesSync(lockPath(tmp.path()), old, old); + const { signal, out } = acquireInChild(tmp.path()); + expect(signal, 'the read hung and was killed').toBeNull(); + expect(out).toBe('ran'); + expect(existsSync(lockPath(tmp.path()))).toBe(false); + }, 30_000); +});