docs(records): row 5 round 1 review files, record-issue gap in DEFERRED
Both reviewers requested changes (26681, 26683). Darkwing's verdict landed on #1508; pointer 26685 on #1507. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -0,0 +1,160 @@
|
||||
# Queue row 5, CHAT-03 increment 1, round 1 review (#1508)
|
||||
|
||||
Darkwing, 2026-10-04. Request: #1508 comment 26671. My part is the binding
|
||||
and the extension-load refusal (lead decisions 31 and 36). Brief:
|
||||
`agents/dewey/work/chat-03/BRIEF.md` at 1ef15ac0. Candidate:
|
||||
`agents/dewey/work/chat-03/I1-manifest.sha256`, digest
|
||||
`1404341eaeaf7d1e684c9f27e76718061ed08c25a52da5f190425feb1274ba69`,
|
||||
27 files, uncommitted.
|
||||
|
||||
Verdict: changes requested. Two blocking findings, B1 and B2.
|
||||
|
||||
## Checks
|
||||
|
||||
- The manifest hashes to 1404341e. All 27 files match it in the canonical
|
||||
working tree.
|
||||
- Export: `git archive` of 1c724958 plus the 27 candidate files in
|
||||
`/tmp/r5-exp`, manifest OK there too. `node --test
|
||||
packages/conversation/tests/` passes 141/141, 0 skipped.
|
||||
`node --test packages/control-board/tests/` passes 124/124 (the board
|
||||
reads `ControlRefusal` codes). The CHAT-00, CHAT-01 and CHAT-01c checks
|
||||
still pass.
|
||||
|
||||
## B1 (blocking). The seal is a deny-list over an argv the caller builds
|
||||
|
||||
`checkSeal` (pi-pin.mjs 59–66) refuses `-e`/`--extension`, a missing seal
|
||||
flag, or a first pair other than `--mode rpc`. Everything else in
|
||||
`engine.extraArgs` passes, and `buildPiArgs` appends extraArgs after the
|
||||
controller's own `--session`. Pi's parser keeps the last `--mode` and the
|
||||
last `--session`. The controller is a public export (`./controller` in
|
||||
package.json), so this is reachable without touching the source.
|
||||
|
||||
(a) `extraArgs: ["--session", <file outside the fixture root>]`. The guard
|
||||
refuses that same path when it is passed as `sessionFile`, but it never
|
||||
sees extraArgs. With the real pinned Pi under a scratch HOME and agent dir
|
||||
(`/tmp/r5/seal-escape.mjs session`):
|
||||
|
||||
```
|
||||
guard on sessionFile: live-session-refused
|
||||
constructed with extraArgs ["--session",".../outside/proj/.pi/state/other/sessions/s1.jsonl"]
|
||||
-> piArgs ["--mode","rpc","--no-extensions","--no-prompt-templates","--no-themes",
|
||||
"--session",".../fx/.../fixture-seat/sessions/s1.jsonl","--session",".../outside/..."]
|
||||
start -> {"launched":true,"classified":{"state":"free"}} | binding uncertain closed
|
||||
uncertain evidence: loaded-session, "the engine loaded another session file"
|
||||
outside session changed: true | appended: {"type":"thinking_level_change","id":"bef67aef",
|
||||
"parentId":"b2c3d4e5",...,"thinkingLevel":"off"}
|
||||
```
|
||||
|
||||
K8 notices afterwards, but Pi has already written to a session the guard
|
||||
exists to protect. That breaks §2 "no writes to sessions" and the
|
||||
fixture-only rule in code.
|
||||
|
||||
(b) `extraArgs: ["--mode", "json"]` (or `text`). checkSeal passes because
|
||||
it looks only at args[0] and args[1]. Pi starts in print mode, reads stdin
|
||||
to EOF and treats it as the prompt. The K8 `get_state` line goes into that
|
||||
reader, so the run ends `uncertain` on `get_state: timeout`. In an earlier
|
||||
run where I closed stdin, Pi sent the `get_state` JSON line to the model
|
||||
as a prompt and stopped only at "No API key found". The default
|
||||
`engine.env` is `process.env`, so with real auth present that becomes a
|
||||
paid model call outside RPC. PgroupLauncher spawns detached, so the engine
|
||||
outlives the controller unless someone kills the group.
|
||||
|
||||
Other extraArgs that pass checkSeal today: `--no-session`, `--fork`,
|
||||
`--export <file>` (writes a file), `--prompt-template`, `--approve`.
|
||||
`engine.command` and `engine.preArgs` can also run any script, or node
|
||||
with `--import`. `checkEnginePin` validates only the pinRoot lock files, so
|
||||
the pin says nothing about what actually ran.
|
||||
|
||||
Suggested fix:
|
||||
- Allow-list extraArgs. The only non-extension use in the suites is
|
||||
`["--model", "other"]` (claim.test 548, W9), so `--model`, `--provider`
|
||||
and `--thinking` with one value each would cover it.
|
||||
- Refuse any second `--mode`, `--session` or seal flag, and any session
|
||||
or output flag (`--print`, `--no-session`, `--session-dir`,
|
||||
`--session-id`, `--fork`, `--export`, `--continue`,
|
||||
`--resume`).
|
||||
- Say in the README that a non-default `command` or `preArgs` is a test
|
||||
hook, and that the pin and seal checks don't bind under it. Or refuse it
|
||||
outside tests.
|
||||
- N24 cases for `--session`, `--mode json` and `--no-session` in
|
||||
extraArgs.
|
||||
|
||||
## B2 (blocking). The claim's session key is the conversation ID
|
||||
|
||||
controller.mjs 187:
|
||||
`this.sessionK = sessionKey({ harness: "pi", conversation: this.conversation })`.
|
||||
`conversation` is `"pi-" + sha256(projectRoot, seat, name)` (reader.mjs
|
||||
72). The brief (line 382–383) keys the claim on "the native session
|
||||
identity (the Pi header ID or the Claude session UUID)", and the CHAT-01
|
||||
README says at most one non-stopped binding may hold a conversation or
|
||||
native-session identity. Two paths to one session file give two
|
||||
conversation IDs, so the session key never collides.
|
||||
|
||||
Repro, `/tmp/r5/hardlink.mjs`: one session hard-linked into
|
||||
`.pi/state/fixture-seat/sessions/s1.jsonl` and
|
||||
`.pi/state/seat-b/sessions/s1.jsonl`, two controllers via the test harness
|
||||
with the fake engine.
|
||||
|
||||
```
|
||||
same inode: true
|
||||
A conversation pi-b5901a69... | B conversation pi-f21f1a75...
|
||||
A start: {"launched":true,...} state active
|
||||
B start: {"launched":true,...} state active
|
||||
A nativeSession 0f5e1c2a-1111-4222-8333-944455556666 | B nativeSession 0f5e1c2a-1111-4222-8333-944455556666
|
||||
```
|
||||
|
||||
Two active bindings, two engines, one session. A plain copy is allowed
|
||||
too; whether a copy should count as the same session is a call for the
|
||||
brief, but the hard link plainly should. W4 (claim.test 172) builds its
|
||||
keys by hand on the store, so it never exercises the controller's
|
||||
mapping.
|
||||
|
||||
Fix: build the session key from the header `id` read in `start()` (the
|
||||
`nativeSession` already in hand). Add a controller-level W4 case for the
|
||||
hard link, and one for a copy with whatever the brief decides.
|
||||
|
||||
## n1 (non-blocking). The startup-append note is narrower than Pi's rule
|
||||
|
||||
README 426–431 and BUILD-I1.md 196 say the append bites only sessions
|
||||
without a `thinking_level_change` entry, and that Pi-created sessions
|
||||
carry one. Pi's rule is sdk.js 82:
|
||||
`hasExistingSession = existingSession.messages.length > 0`. A session
|
||||
with a thinking entry but no messages takes the new-session branch and
|
||||
appends `thinking_level_change` at every start, plus `model_change` when a
|
||||
model is set. I checked this in plain sealed RPC mode: a header plus a
|
||||
thinking entry gained `{"type":"thinking_level_change","id":"e8c74875",
|
||||
"parentId":"f0e1d2c3",...}`. A Pi-created session that was opened and
|
||||
never prompted is in this group, so "written by something else" is wrong.
|
||||
Name message-less sessions too, and let the later pre-spawn check cover
|
||||
both.
|
||||
|
||||
## n2 (minor). The guard follows $HOME
|
||||
|
||||
`LiveSessionGuard` protects `~/.pi`, `~/.claude` and `~/.mosaic-dev` via
|
||||
`os.homedir()`. With a scratch HOME, the real directories are protected
|
||||
only by the fixture-root containment, or by passing `homes`. That
|
||||
containment holds today, so I'm noting it, not blocking on it.
|
||||
|
||||
## What holds in the binding
|
||||
|
||||
- H1 to H3. `#dispatch` holds `this.lock` across `#recheck` and the
|
||||
write. Takeover, release and acquire run `#evaluate` under the same
|
||||
lock, so a prompt can't land between the generation bump and the write.
|
||||
- The generation check comes first in `#evaluateOp` and again in
|
||||
`#recheck`.
|
||||
- Takeover refuses while fenced (K9) and for the current controller
|
||||
(`already-controller`, H4).
|
||||
- Disconnect never moves control (H11).
|
||||
- Confirmations are single-use: `#checkConfirmation` marks them
|
||||
`consumed`.
|
||||
- Interrupt sets its fence before it takes the lock, so a queued prompt
|
||||
sees the fence.
|
||||
- K8 catches a wrong session or leaf after launch, as in B1(a). B1 is that
|
||||
the write happens before K8 can run.
|
||||
- The pin check reads both lock files and refuses on either version or
|
||||
integrity mismatch.
|
||||
|
||||
Scratch scripts and outputs are in `/tmp/r5/`: `seal-escape.mjs`,
|
||||
`hardlink.mjs`, `seal-session.txt`, `seal-json.txt`, `hardlink.txt`,
|
||||
`conv-suite.txt`, `board-suite.txt`. All scratch engines were killed by
|
||||
their process group.
|
||||
Reference in New Issue
Block a user