pharn-dev · PrzemekGalarowicz · Sep 25, 2026 · Sep 25, 2026
@@ -0,0 +1,62 @@
+# GRILL — signal-lock-release
+
+Plan: `.dev/features/signal-lock-release/PLAN.md`. Spec hash recomputed:
+`bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e` — **matches**. Registered
+grillers: `{"registered":0,"grillers":[]}`, so only the inline axes run. The plan is
+`trust: untrusted`, and nothing in it read as an instruction.
+
+## Findings
+
+### Guarantee audit (P0)
+
+```yaml
+- type: FINDING
+  rule_id: 'P0'
+  severity: important
+  file: '.dev/features/signal-lock-release/PLAN.md:47'
+  problem: 'The re-raise must never leave the process running. SIGHUP (added at GATE 1) has no default-kill emulation on win32, where Node emulates only SIGINT/SIGTERM/SIGKILL for process.kill, and a kill that throws inside the handler would leave the process alive with its listeners removed. Wrap the re-raise: if process.kill throws, exit with 128 + the signal number.'
+  evidence: '4. Re-raises `sig`, so the exit status stays 130/143/129.'
+- type: FINDING
+  rule_id: 'P0'
+  severity: minor
+  file: '.dev/features/signal-lock-release/PLAN.md:59'
+  problem: "The interrupted-while-writing line printed from the signal path must obey the same rule as the exit path: a stderr write failure (EPIPE on a closed terminal, which is exactly SIGHUP's case) must be swallowed, or it replaces the signal exit with an exception."
+  evidence: '`withProjectLock` registers `release(cwd)` plus the same "interrupted while writing" stderr line'
+```
+
+### Eval coverage (P1)
+
+```yaml
+- type: FINDING
+  rule_id: 'P1'
+  severity: minor
+  file: '.dev/features/signal-lock-release/PLAN.md:76'
+  problem: 'The deadline case needs fake timers AND a body stream whose cancel is observable. Otherwise "the reader was cancelled" is inferred from the null result, which the base code already returns, so the test would pass on the base and prove nothing. Assert on the recorded cancel call.'
+  evidence: 'A 2xx body that never ends → at the 8 s deadline the reader was cancelled and the result is'
+- type: FINDING
+  rule_id: 'P1'
+  severity: minor
+  file: '.dev/features/signal-lock-release/PLAN.md:69'
+  problem: 'The "leaves alone a lock another process created" case has to create that foreign lock between the release and the signal, in the child. Say so in the test, or the case collapses into the plain release case.'
+  evidence: 'After a normal release, a later SIGTERM leaves alone a lock file another process created'
+```
+
+### Checked, no finding
+
+- Axis (P3): the signal logic leaves `repo.ts` for a module whose one reason to change is fatal-signal
+  handling. `project-lock.ts` and `repo.ts` both depend on it, and neither on the other.
+- Determinism (P5): a fixed signal list and set membership. Handlers are installed lazily, on the
+  first registration, so a run that never registers keeps the default disposition: the same
+  behaviour as today for the prompt-only phases.
+- Trust (P2): response bodies are cancelled, never read further. There is no new output from
+  untrusted data.
+
+## Summary
+
+The design is sound. The one important gap is the win32 re-raise: once SIGHUP joins the list, the
+handler must end the process even where `process.kill` cannot emulate the signal. The rest is test
+sharpness: cancels must be observable rather than inferred, and the foreign-lock case needs an
+explicit setup.
+
+**ADVISORY VERDICT: 4 concerns raised (0 blocking-severity, 1 important, 3 minor) — for the human to
+weigh before /pharn-dev-build.**
@@ -0,0 +1,130 @@
+# PLAN — signal-lock-release (a real SIGINT/SIGTERM releases the lock; no response body outlives its command)
+
+- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4
+- increment: (1) a SIGINT or SIGTERM received while `.pharn.lock` is held releases the lock before
+  the process dies. A small shared fatal-signal registry does this, used by both the temp-clone
+  cleanup and the lock. (2) `fetchCommitSha` cancels a non-2xx body, and reads a 2xx body through
+  its own reader, cancelled on the deadline, so no body keeps the process alive after the command
+  has finished. `downloadArchive`'s non-2xx throw cancels its body too.
+- layer(s): the CLI itself (`src/lib/fatal-signal.ts` new, `src/lib/repo.ts`,
+  `src/lib/project-lock.ts`)
+- constitution_refs: [P0, P1, P3, P5, P6]
+
+## Discovery — verified this run (P6)
+
+- F13 reproduced on HEAD `abb274a` with a tsx probe (scratchpad `plan-C/probe13.mts`). It holds
+  `withProjectLock`, runs `fetchRepo` (stubbed fetch) and signals itself. SIGTERM exits 143 and
+  SIGINT exits 130, and in both cases `.pharn.lock` is left behind.
+  - Cause: `repo.ts:66-78` handles SIGINT/SIGTERM by removing the clones, calling
+    `process.removeAllListeners(sig)`, and re-raising with the default action. The default action
+    ends the process without emitting `exit`. The lock is released only from an `exit` listener
+    (`project-lock.ts:502-515`) or its `finally`, so neither runs.
+  - Without a fetch (`remove`, or `add`/`update` before `fetchRepo`) there is no handler at all.
+    The default action strands the lock there too.
+  - The next run on the same host reclaims the lock via the dead-pid check. A different host (a bind
+    mount) waits `STALE_MS` = 6 h (`project-lock.ts:84`).
+- F12, read on HEAD (`repo.ts:239-270`): on `!res.ok`, `fetchCommitSha` returns `null` without
+  touching the body. On 2xx it calls `res.json()`, a read with no reader of its own, so nothing
+  cancels it at the 8 s deadline. `withDeadline` answers the caller, but the socket keeps the event
+  loop alive (`deadline.ts:1-20` documents why the abort alone is not enough on Node 20/22).
+  - A null SHA is a degraded mode, not an exit, so the command finishes normally. `index.ts` calls
+    `process.exit` only on failure, so the process lives until the body ends: 30 s in the review's
+    403-drip measurement.
+  - `downloadArchive` (`:192-194`) also throws on `!res.ok` with the body untouched. That is benign
+    today only because every caller then exits 1.
+- Only `tests/repo.test.ts` (6) and `tests/repo-signals.test.ts` (2) stub the SHA response with
+  `json:`. The command tests mock `repo.js` wholesale.
+- The docs say a `SIGKILL`ed run cannot release (`docs/troubleshooting.md:369`). They make no claim
+  either way about SIGINT/SIGTERM.
+
+## Files
+
+- `src/lib/fatal-signal.ts` (new) — layer CLI/lib. `onFatalSignal(fn): () => void`, a
+  once-installed SIGINT/SIGTERM handler (SIGHUP deliberately NOT handled — see the amendment under
+  Open questions) that works in four steps:
+  1. Runs every registered cleanup synchronously, last-registered first, each in its own try/catch.
+  2. Clears the set.
+  3. Calls `removeAllListeners(sig)`.
+  4. Re-raises `sig`, so the exit status stays 130/143.
+
+  The re-raise moves here from `repo.ts` unchanged. One axis: dying cleanly on a fatal signal.
+
+- `src/lib/repo.ts` — layer CLI/lib. The clone cleanup registers through `onFatalSignal`; its
+  `exit` listener stays as it is. `fetchCommitSha` gets two fixes:
+  - On a non-2xx status it calls `res.body?.cancel()` (errors ignored) and returns `null`.
+  - On 2xx it reads the body through its own reader, with an abort listener that calls
+    `reader.cancel()` (the `downloadArchive` pattern), then parses the JSON.
+
+  `downloadArchive` cancels the body before its non-2xx throw.
+
+- `src/lib/project-lock.ts` — layer CLI/lib. `withProjectLock` registers `release(cwd)` plus the
+  same "interrupted while writing" stderr line as the `exit` path, for the lock's lifetime, and
+  deregisters in its `finally`.
+- `tests/project-lock.test.ts` — layer tests. Child-process cases (the `tsx` pattern already used
+  there):
+  - The lock is held with no fetch, and the process sends itself SIGTERM. It exits 143, the lock is
+    gone, and stderr names the interruption. FAILS on base.
+  - The same with SIGINT after a stubbed `fetchRepo`. It exits 130, and both the lock and the clone
+    are gone. FAILS on base.
+  - After a normal release, a later SIGTERM leaves alone a lock file another process created (the
+    `release` re-read, plus the deregistration).
+- `tests/fatal-signal.test.ts` (new) — layer tests. In-process, with `process.kill` stubbed:
+  cleanups run in reverse order, a throwing cleanup does not stop the rest, a deregistered cleanup
+  does not run, and the handlers install once.
+- `tests/repo.test.ts` — layer tests:
+  - A 403 whose body stream records `cancel` → the cancel was called. FAILS on base.
+  - A 2xx body that never ends → at the 8 s deadline the reader was cancelled and the result is
+    `null`. FAILS on base.
+  - A `downloadArchive` 500 → the body was cancelled. FAILS on base.
+  - The existing `json:` stubs become stream bodies.
+- `tests/repo-signals.test.ts` — layer tests. Its SHA stub becomes a stream body. The clone-removal
+  and exit-130 assertions stay as they are.
+- `docs/troubleshooting.md` — layer docs. The lock section: Ctrl-C, `timeout` and `docker stop`
+  (SIGINT/SIGTERM) release the lock and exit 130/143. SIGKILL, power loss and a hangup (SIGHUP)
+  still cannot release.
+- `CHANGELOG.md` — `[Unreleased]` → `### Fixed`, two entries
+
+## Contracts satisfied
+
+- The lock's contract in `docs/reference/pharn-records.md` ("a second pharn process refuses instead
+  of interleaving"). It stops costing a 6 h wait after an ordinary interrupt (cited, P4).
+- `deadline.ts`'s stated rule, "callers additionally cancel their body reader on abort". It now
+  holds for every fetch in `repo.ts`.
+
+## Evals to write (P1)
+
+- Listed under Files. Five cases FAIL on the base.
+
+## Guarantee audit (P0)
+
+- "a SIGINT/SIGTERM delivered while the lock is held releases it" → floor: child-process tests
+  observing the exit status and the lock file. The residual is named: SIGKILL, power loss, and a
+  signal that arrives before `withProjectLock` registers.
+- "a non-2xx or timed-out SHA response cannot keep the process alive" → floor: cancel-spy tests over
+  both paths. Wall-clock exit timing is not asserted (flaky). The mechanism is.
+- "the exit status stays truthful (130/143)" → floor: the existing `repo-signals` test plus the
+  new ones.
+
+## Trust audit (P2)
+
+- The response bodies are untrusted, and they are now consumed less: cancelled, never drained. No
+  new data reaches output.
+
+## Determinism audit (P5)
+
+- A fixed signal list and set membership. No fallback path.
+
+## 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. SIGHUP (terminal closed while `update` holds the lock): (a) handle it too, re-raised as exit 129 —
+   recommended. The failure is the same stranded lock. (b) SIGINT/SIGTERM only, as measured.
+
+**Amendment during build (human, 2026-09-25): question 1 → (b).** Measured before review: a
+`process.on('SIGHUP')` listener overrides the `SIG_IGN` that `nohup` sets. Node 22 ran the handler
+even under `trap '' HUP`. So handling SIGHUP would make a hangup interrupt a `nohup pharn update`
+mid-write, where today it keeps running. Asked again with that finding, the human chose
+SIGINT/SIGTERM only. A hangup's stale lock is still reclaimed automatically on the same machine
+(dead-pid check).
@@ -0,0 +1,40 @@
+# REGRESSION — signal-lock-release
+
+The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment.
+
+## Base and partition
+
+- **base:** `cf6582e92eaae15e3dd431824107101da741b9b8` (`HEAD` — `origin/main` after #219; the
+  build is an uncommitted working tree on top of it).
+- **inside** (each declared in `PLAN.md` `## Files`): `src/lib/fatal-signal.ts` (new),
+  `src/lib/repo.ts`, `src/lib/project-lock.ts`, `tests/fatal-signal.test.ts` (new),
+  `tests/project-lock.test.ts`, `tests/repo.test.ts`, `tests/repo-signals.test.ts`,
+  `docs/troubleshooting.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.
@@ -0,0 +1,65 @@
+# REVIEW — signal-lock-release
+
+Increment:
+
+- `src/lib/fatal-signal.ts` (new): `onFatalSignal`, a lazily installed SIGINT/SIGTERM handler. It runs
+  the registered cleanups newest first, removes every listener, re-raises, and falls back to
+  `exit(128 + n)`. SIGHUP is deliberately excluded (the plan amendment: `nohup`).
+- `src/lib/repo.ts`: the clone cleanup registers through `onFatalSignal`. `fetchCommitSha` discards a
+  non-2xx body and reads a 2xx body through its own abort-cancelled reader. `downloadArchive`
+  discards a non-2xx body.
+- `src/lib/project-lock.ts`: `withProjectLock` registers the release plus the interrupted line for
+  the lock's lifetime.
+- Tests in four files, `docs/troubleshooting.md`, and two CHANGELOG entries.
+
+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 (1530 tests), `/pharn-dev-regress` returned `no-regressions`, and `/pharn-dev-verify`
+returned `PASS`.
+
+## Floor-gate findings (blocking)
+
+None.
+
+- **L-floor (P0)**
+  - "a SIGINT/SIGTERM while the lock is held releases it and still dies by the signal" → child
+    processes observing the exit status and the lock file.
+  - "the clone goes too" → the same child.
+  - "a released lock's cleanup can never delete another process's lock" → a child writes an
+    empty (mid-create) lock after release and signals itself. The lock survives.
+  - "no SIGHUP listener" → a membership test.
+  - "an unread SHA body is released" → observable-cancel streams, per the grill.
+  - What the floor does not reach is named in VERIFY.md: SIGKILL, power loss, a hangup, a signal
+    before registration, and the stub-only fallback.
+- **L-eval (P1)** — 14 of the new cases fail when run against the base `src/`, checked in a
+  worktree. The rest are guards: a leftover lock, the in-process clone cleanup, and the unchanged
+  deadline answer.
+- **L-trust (P2)** — untrusted response bodies are now consumed less (cancelled, never drained).
+  Nothing new reaches output.
+- **L-axis (P3)** — `fatal-signal.ts` has one reason to change. `repo.ts` and `project-lock.ts` each
+  depend on it and not on each other. The body helpers stay in `repo.ts`, the fetch boundary's
+  own module.
+
+## Advisory findings (warn — severity is this reviewer's judgment, fix #3)
+
+```yaml
+- type: FINDING
+  rule_id: 'P0'
+  severity: minor
+  file: 'src/lib/repo.ts:294'
+  problem: "readText still has no byte cap, the same as the res.json() it replaces, so a hostile or broken API response can grow memory for up to 8 s. skills-version.ts caps its body at 256 KB. A cap needs a size above real commit responses, which carry file lists; that is out of this increment's scope, so it is named, not added."
+  evidence: 'async function readText(res: Response, signal: AbortSignal): Promise<string> {'
+- type: FINDING
+  rule_id: 'P5'
+  severity: minor
+  file: 'src/lib/fatal-signal.ts:83'
+  problem: "removeAllListeners(sig) removes every listener on the signal, not only clack's, before the re-raise. That is the pre-existing repo.ts behavior, moved unchanged, and correct for a process that is about to die. A future listener meant to observe a signal and let the process live would be removed too; the header comment says so."
+  evidence: 'process.removeAllListeners(sig);'
+```
+
+## Verdict
+
+**GREEN — 0 floor-gate findings, 2 advisory (minor).** The standing decision is the human's (GATE 2).
@@ -0,0 +1,26 @@
+# SHIP — signal-lock-release
+
+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 reworded so the writes-scope parser reads every path (formatting only).
+4. `/pharn-dev-build`.
+5. **Plan amended by the human mid-build.** Before review, a measurement showed that handling SIGHUP
+   overrides `nohup`. Asked again, the human chose SIGINT/SIGTERM only (open question 1 → b). The
+   build was changed to match.
+6. `/pharn-dev-build` floor re-run → `/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.