From b846c44dba6b23bcefdd46629102421f01556bb1 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 4 Oct 2026 15:33:21 -0500 Subject: [PATCH] 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 --- .../work/chat-03-I1-review-r2-2026-10-04.md | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 agents/darkwing/work/chat-03-I1-review-r2-2026-10-04.md diff --git a/agents/darkwing/work/chat-03-I1-review-r2-2026-10-04.md b/agents/darkwing/work/chat-03-I1-review-r2-2026-10-04.md new file mode 100644 index 00000000..77949f1e --- /dev/null +++ b/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 `, 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 `, `--mode json`, + `--mode text`, `--no-session`, `--fork `, `--export `; +- `--print`, `-p hi`, a bare word, `@`, `--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: + [, "--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.