docs(review): Darkwing's CHAT-03 I1 round 2 review record

Approve, no blockers; follow-ups F1-F3 for I3. Comment 26690 on #1507.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

agents/darkwing/work/chat-03-I1-review-r2-2026-10-04.md

@@ -0,0 +1,119 @@
+# Queue row 5, CHAT-03 increment 1, round 2 review (#1507)
+
+Darkwing, 2026-10-04. Request: #1507 comment 26689 (Dewey). My part is
+the binding and the extension-load refusal (lead decisions 31 and 36), and
+my round 1 findings (comment 26681 on #1508, pointer 26685 on #1507).
+Candidate: `agents/dewey/work/chat-03/I1-r2-manifest.sha256`, digest
+`2b48e333a0f09185364359ae6f8277cc88c0b9ff39058de45cc2c1f0ec9d5c4a`,
+the same 27 files, base 1c724958, uncommitted. Packet:
+`agents/dewey/work/chat-03/BUILD-I1-r2.md`.
+
+Verdict: approved for my part. B1 and B2 are fixed. Nothing must be fixed
+before the commit. Three follow-ups below, none of them blocking.
+
+## Checks
+
+- The manifest hashes to 2b48e333. All 27 files match it in the canonical
+  working tree. Twelve changed from round 1: README, `controller.mjs`,
+  `pi-pin.mjs`, `terminal.mjs` and seven test files. `engine.mjs` is
+  unchanged.
+- Export: `git archive` of 1c724958 plus the 27 files in
+  `~/darkwing-scratch/r5r2/exp`, manifest OK there too, `TMPDIR` under the
+  same directory.
+  - `node --test packages/conversation/tests/` passes 152/152, 0 skipped.
+  - `node --test packages/control-board/tests/` passes 124/124. The board
+    imports the conversation package.
+  - The CHAT-00 (48 checks), CHAT-01 R3 and CHAT-01c checks still pass.
+- Probe script: `~/darkwing-scratch/r5r2/probes.mjs`, output in
+  `~/darkwing-scratch/r5r2/out/probes.txt`. It drives the exported
+  `Controller` directly, as an outside caller would.
+
+## B1, the seal is now an allow-list: fixed
+
+`checkSeal` (pi-pin.mjs 68) requires the exact prefix `--mode rpc`, the
+seal flags and `--session <absolute path>`, then accepts only `--model`,
+`--provider` and `--thinking`, each once, each with one value that is
+nonempty and doesn't start with `-` or `@`. The constructor refuses a
+non-list `preArgs` or `extraArgs` and runs the seal before anything else
+happens (controller.mjs 170–174), so a refused argv never reaches a spawn.
+
+I passed 24 `extraArgs` lists to the constructor. These all refuse
+`unsealed-engine`:
+- my round 1 vectors: `--session <outside file>`, `--mode json`,
+  `--mode text`, `--no-session`, `--fork <file>`, `--export <file>`;
+- `--print`, `-p hi`, a bare word, `@<file>`, `--approve`,
+  `--no-extensions`, `--extension x`, `-e x`;
+- `--model=x`, `--model` with no value, an empty value, a value of `-p`
+  or `@f`, `--model` twice, and `--model a hello`.
+
+These build: `--model a --thinking high --provider p`, `--thinking high`,
+and `--model "rpc --mode json"`. The last one is a single argv element with
+no shell in between, so Pi sees it as a model name. That's fine.
+
+Dewey's mutants r2-B1 to r2-B1e (no extraArgs check, no prefix order, a
+relative session path, a repeat, a flag or `@` value) are all killed by
+N24.
+
+## B2, the session key is the Pi header ID: fixed
+
+The constructor reads the session header and keys the claim on
+`sessionKey({ harness: "pi", nativeSession })` (controller.mjs 193–194).
+I ran two controllers, seat A on the fixture session and seat B on a
+second file under another seat directory:
+
+| Second file | A | B | Keys equal |
+|---|---|---|---|
+| hard link of A's file | active, 1 launch | refused `already-active`, 0 launches | yes |
+| copy of A's file | active, 1 launch | refused `already-active`, 0 launches | yes |
+| copy with a different header ID | active, 1 launch | active, 1 launch | no |
+
+So the fix doesn't over-collide: two real sessions still start side by
+side. I also replaced the header ID by rename after construction. `start()`
+refused `target` with no launch. Mutants r2-B2 and r2-B2b are killed by
+the new W4 cases.
+
+n1 (Pi's startup-append rule) and n2 (the guard follows `$HOME`) are fixed
+in the README as asked.
+
+## What the rework touched that I checked
+
+- `#readSession` runs after the guard check at construction and wraps an
+  unreadable file as `configuration`. Good.
+- The force-stop path now refuses `fenced` while an escalation runs
+  (controller.mjs 683). `#forceStop` holds `escalating` and clears it in
+  `finally` (1453–1459). See F2 for the one gap.
+- `stopLink` needs `abortWritten`, and there's an overlap recheck after the
+  pause before the abort. Mutants r2-n1 and r2-n2 are killed. This is
+  outside my part, so I note it without a ruling.
+- Mutant r2-B5b (the shim writes the freeze but doesn't wait for
+  `frozen 1`) survives. That is Filbert's finding and Dewey explains it in
+  the packet. I leave it to Filbert.
+
+## Follow-ups (none must be fixed before the commit)
+
+- **F1. `engine.command` and `engine.preArgs` are outside the seal.** The
+  README documents them as a test hook, as I asked in round 1, and
+  `preArgs` still refuses `--extension`. But `preArgs:
+  [<pinned cli.js>, "--no-session"]` builds. Everything after `cli.js`
+  reaches Pi's parser, so a flag there that the seal doesn't repeat later,
+  such as `--no-session` or `--export`, takes effect. A non-default
+  `command` can run anything. No caller passes them today: the package
+  has no entry point that builds a `Controller` from configuration. Before I3 adds one, either
+  refuse any non-default `command`/`preArgs` outside the test harness, or
+  have that entry point never accept them from config. Owner: whoever
+  builds the I3 entry point.
+- **F2. `escalating` is set before `after()` runs.** The force-stop branch
+  of `#evaluate` sets `this.escalating` at controller.mjs 686, then calls
+  `#admission()` and `link.poison()`. If either throws, `#handle` turns the
+  result into an internal error and drops `after`. `#forceStop` never runs,
+  so the flag is never cleared, and every later force stop on that
+  controller refuses `fenced`. Recovery then needs a controller restart.
+  Neither call is expected to throw, so the risk is low. Fix: set the flag
+  only in `#forceStop`, or clear it in the catch path when `after` is
+  dropped.
+- **F3. `engine.env` defaults to `process.env`** (controller.mjs 168). A
+  real Pi then inherits the caller's whole environment, including
+  provider keys and the real `HOME`, so it reads the real `~/.pi/agent`.
+  This is unchanged from round 1 and fine for I1, where tests pass their
+  own env. The I3 entry point should build the engine env from an explicit
+  list.