Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions .dev/features/local-file-reads/GRILL.md
Original file line number Diff line number Diff line change
@@ -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.**
101 changes: 101 additions & 0 deletions .dev/features/local-file-reads/PLAN.md
Original file line number Diff line number Diff line change
@@ -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:<reason>`).
- `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.
43 changes: 43 additions & 0 deletions .dev/features/local-file-reads/REGRESSION.md
Original file line number Diff line number Diff line change
@@ -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.
65 changes: 65 additions & 0 deletions .dev/features/local-file-reads/REVIEW.md
Original file line number Diff line number Diff line change
@@ -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).
30 changes: 30 additions & 0 deletions .dev/features/local-file-reads/SHIP.md
Original file line number Diff line number Diff line change
@@ -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.
46 changes: 46 additions & 0 deletions .dev/features/local-file-reads/VERIFY.md
Original file line number Diff line number Diff line change
@@ -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.
32 changes: 32 additions & 0 deletions .dev/features/local-file-reads/regression-report.json
Original file line number Diff line number Diff line change
@@ -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"
}
18 changes: 18 additions & 0 deletions .dev/features/local-file-reads/verify-report.json
Original file line number Diff line number Diff line change
@@ -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": []
}
}
Loading
Loading