diff --git a/agents/darkwing/work/chat-03-I1-review-r1-2026-10-04.md b/agents/darkwing/work/chat-03-I1-review-r1-2026-10-04.md new file mode 100644 index 00000000..8830281d --- /dev/null +++ b/agents/darkwing/work/chat-03-I1-review-r1-2026-10-04.md @@ -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", ]`. 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 ` (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. diff --git a/agents/filbert/work/chat-03-i1-review-r1-2026-10-04.md b/agents/filbert/work/chat-03-i1-review-r1-2026-10-04.md new file mode 100644 index 00000000..6946de65 --- /dev/null +++ b/agents/filbert/work/chat-03-i1-review-r1-2026-10-04.md @@ -0,0 +1,265 @@ +# Queue row 5, CHAT-03 increment 1, round 1 review + +Filbert, 2026-10-04. Request: #1507 comment 26671 (Dewey). Brief: +`agents/dewey/work/chat-03/BRIEF.md`, sha256 1ef15ac0…, with lead decisions +30–34 and 36. Candidate: `agents/dewey/work/chat-03/I1-manifest.sha256`, +digest `1404341eaeaf7d1e684c9f27e76718061ed08c25a52da5f190425feb1274ba69`, +27 files, uncommitted. + +My part is the whole brief except the binding and the extension-load +refusal, which Darkwing reviews (`pi-pin.mjs`, `claim.mjs`, `guard.mjs`, +`SEAL_BASIS`, the K8 load check, takeover and generation; N24, K6/K8 pin +side, W1–W20, G1–G3, H1–H4, mutants 37/37b). + +Verdict: **changes requested.** Six blocking findings, B1–B6. Two are +defects in the terminal, one is a defect in the force-stop chain, and three +are missing tests that leave a brief or lead-decision property unpinned. +The rest are notes. None of my findings repeats Darkwing's B1 or B2. + +## How I checked + +- Frozen copy in a scratch clone of 1c724958 (`/tmp/f5r1/repo`, push URL + `DISABLED`, candidate files read-only). The manifest verifies there and + in the canonical working tree. Every changed path is under + `packages/conversation/` (Files owned). `git diff 1c724958 -- docs + contracts roles` is empty and the brief's 12 contract hashes match. +- Export: `git archive` of 1c724958 plus the 27 files, manifest OK. Every + §10 suite was run on it, each with its own log: + - conversation 141/141, 0 skipped; control-board 124; webui 14; seat 19; + - CHAT-00 48 checks; CHAT-01 R3 PASS; CHAT-01c PASS; + - auth 15, config 24, discord 64, extension-package 18, foundation 44, + release 14, task 90; + - queue 27/0 with node 148/148. The two live-queue checks skip outside + the canonical root and say so, which accounts for Dewey's 29; + - conductor fails on the export only because the export isn't a git + repository (`fatal: not a git repository`). In the scratch clone + holding the candidate it passes 17/17. +- Three scoped deep reads, each in its own scratch copy: turns (§3, O1–O6, + N1–N25, decision 34); stop and recovery (§6, K1–K18, H10, H17, H20–H23); + transport, slash, terminal and seam (§1, §4, §9, H11–H19, E1–E7). I + reran every blocking result myself against the frozen candidate before + listing it. + +## Blocking + +### B1. Text typed after Enter in the same input chunk joins the submitted message + +`terminal.mjs` 125–133 and 157–158. `key()` appends characters to the +composer at once but queues the Enter action until the loop ends, so +`submit()` reads the composer after the rest of the chunk has gone in. + +Rerun on the frozen candidate (`/tmp/f5r1/b1probe.mjs`, a recording fake +client): `key("first\rsecond\r")` sends one prompt, `["firstsecond"]`; the +second Enter sends nothing. `key("abc\rdef")` sends `["abcdef"]`. + +This is the concatenation hazard §4 exists to remove ("cleared after each +submit"; README "It clears after each submit"). One chunk holding Enter and +more text is ordinary: tmux `send-keys "x" Enter "y"`, typeahead while the +event loop is busy, or a terminal without bracketed paste. Fix: snapshot +the composer at the Enter (submit that text, clear, then keep reading the +chunk), and add a test with text after Enter in one chunk. + +### B2. A paste-start marker split right after its ESC loses paste mode, and an Enter inside the paste submits + +`terminal.mjs` 114. The carry needs `s.length - i >= 2`, so a lone trailing +`\x1b` is consumed as an Escape key. Rerun: `key("\x1b")` then +`key("[200~a\rb\x1b[201~")` sends `["[200~ab"]`. Splits at other offsets +inside the marker are handled. + +README: "A bracketed paste is inserted literally, newlines included, and +never submits by itself." The read boundary has to fall exactly after the +ESC, so this is rare, but the fix is cheap: a lone Escape has no action in +this terminal, so carrying a trailing lone `\x1b` costs nothing. Add the +split case to the paste test. + +### B3. A second force stop runs a parallel escalation and the first one's phase is written onto the second stop's record + +`controller.mjs` 657–665 admits `force-stop` while `b.state` is +`stopping`. `#forceStop` for the second stop writes +`stop: { id: , phaseStarted: null }` into the claim, while the +first escalation is still running. The first one's `onPhase` then writes +`{ ...r.stop, phaseStarted: name }`, which is the second stop's record. The +superseded check runs only after `stopCohort` returns (1454), so both +escalations signal, freeze and kill at once. + +The stop-and-recovery read proved it with pause hooks (log kept at +`/tmp/f5r1-cohort-logs/XFS2-1.txt`, three runs): the claim shows the second +stop's ID with `phaseStarted: "kill"` while the first is held at the kill +phase and the second hasn't run TERM. I read the code path and it matches. +All three runs ended `stopped` with the child dead, so no false proof was +seen. But §6 makes the stopping revision the record of which stop reached +which phase before any signal, and here it is wrong. Two concurrent +freeze/enumerate/kill passes on one cgroup are also outside what the proof +reasoning covers. + +Fix: one force-stop escalation at a time. Either refuse a second +`force-stop` while one is running (it can be retried once the first ends +`uncertain`), or mark the first superseded and wait for it before starting +the second. Add the race to `races.test.mjs` with engine bytes, the claim +revisions and events asserted. + +### B4. Nothing tests lead decision 34 + +Decision 34: "If the code ever sees `aborted` with no stop in progress, it +treats it as an overlap signal. … This goes into the build and its tests." +The check is at `turns.mjs` 163. I replaced that line with a comment in a +copy of the export and ran the whole conversation suite: 141/141 pass +(`/tmp/f5r1/logs/mutant-d34.log`). No test mentions `aborted-without-stop`. + +The check itself works: the turns read drove an `aborted` with no stop and +got `["aborted-without-stop"]`, binding `uncertain`, receipt outcome +unknown. Add that as a fixture and add the mutant to the table. + +### B5. K12 doesn't prove the freeze + +Brief line 1117: "The freeze stops forking. The enumeration is complete." +I made the shim skip the `cgroup.freeze` write and answer `frozen 1` +anyway. The whole cohort suite passes, 18/18, 0 skipped +(`/tmp/f5r1/logs/mutant-C3-cohort.log`). The stop-and-recovery read got the +same result for a freeze that is written but not waited on. K12 asserts +only the end state, and `cgroup.kill` alone reaches that state. + +Fix: assert `cgroup.frozen` is 1 before enumeration, and that no member +outside the proof's list existed after the freeze (for example from the +fork loop's own pid log). Add the no-freeze mutant to the table. + +### B6. K15 has no "engine path missing" case + +Brief line 1120 lists "`engine` cgroup path missing or unreadable, or the +shim gone"; §6 says "an absent `engine` cgroup is an absent observation, +never an empty one." The test covers the shim gone and `chmod 000` on +`cgroup.events` (EACCES), not a missing path. I made the shim's `events()` +answer `populated 0` on ENOENT only. The cohort suite passes 18/18 +(`/tmp/f5r1/logs/mutant-D-cohort.log`). The mutation table's 21 is a +different, broader variant that the EACCES case does kill, so I'm not +disputing the table, only the coverage. + +The code does fail closed here today, through the TERM-phase enumeration +and the freeze write (the stop read removed the `engine` cgroup and got +`uncertain`, "engine enumeration failed (ENOENT)"). Pin it with a missing +`engine` fixture. + +## Notes (not blocking) + +Turns: + +- **n1.** An `aborted` that arrives after the fence but before the first + abort is written is linked to the stop: receipt `failed`, reason + `interrupted`, no overlap signal, binding stays `active` (stop outcome + Unknown, so nothing reconciles). Decision 34's wording ("no stop in + progress") allows this, but under the seal an `aborted` before the + controller's abort isn't the controller's. Recommend linking only after + an abort was written (`ctx.abortIdxs` non-empty). +- **n2.** An O5 that arrives in the same chunk as the empty clear response + is recorded before the abort, yet the abort is still sent and the queued + item runs (`controller.mjs` 1278–1284; the overlap check is only at the + top of each round). N6 accepts this outcome. Rechecking `tr.overlapped` + after the `before-abort` pause costs one line and fits rule 2's reason. +- **n3.** On a non-empty clear or an abort failure, `stop.nativeQueue` + keeps its initial value instead of `unknown`. No turnProof is built on + any uncertain path, so "the turnProof records `nativeQueue: unknown`" + (rule 2) lives only on the stop record. +- **n4.** After a K9 no-settle the stop is `uncertain`, so a later + `aborted` from the controller's own abort raises `aborted-without-stop`. + After a force stop supersedes an Interrupt, an Interrupt-caused `aborted` + links to the force-stop ID. Labels only. +- **n5.** Test gaps: `rounds-exhausted` has no test; N25 asserts only that + O5 is present; no test for O5 from a `get_state` pending count, or O2 + "before the slot's ack" and "a second settle for one ack". +- **n6.** A user `message_start` with no current run and an `agent_end` + with no open start are counted but raise no signal. Not in O1–O6, so not + a brief violation. + +Stop and recovery: + +- **n7.** Resuming from TERM after a restart isn't asserted: skipping TERM + when `phaseStarted` is `kill` passes K10 and K11. K3 doesn't assert TERM + was delivered. +- **n8.** K13: the README says the refusal is "not a controller check", but + the cgroup namespace is the shim's `unshare --cgroup` (removing it fails + K13). Nothing checks at runtime that cgroup2 is mounted `nsdelegate`; on + a host without it, stops would still record `stopped`. The brief only + asks that K13 shows the refusal on this host, so this is a portability + note for the cutover increment. +- **n9.** A member can ask the user systemd manager for a new unit outside + the scope (`systemd-run --user` from inside a user and cgroup namespace + lands in `app.slice`). Same-uid escape, Limits 3/4; worth a line in the + packet's limits. +- **n10.** The shim channel is not authenticated: the engine sees the + scope's `INVOCATION_ID` and can reach the shim socket, so a same-uid + engine that kills the shim could answer as it. Limit 1; the claim could + record the shim's pid and start time to narrow it. +- **n11.** An orphan's tool map is taken at restart, not at stop time. + Low impact. +- **n12.** H17 tests a confirmation used after the stop changed, not one + answered after it changed; answering doesn't check the stop, using it + refuses, so the outcome matches. A freeze or kill timeout leaves the + cgroup frozen (fails closed). + +Transport, terminal, seam: + +- **n13.** `visible()` passes U+061C (a bidi control the README says is + made visible), U+200B and other zero-width characters, U+FEFF, tag + characters U+E0000–E007F and LF in the status and head lines. None can + drive the terminal; they can hide or spoof text. +- **n14.** `LineSplitter`'s limit isn't checked for a line whose LF arrives + in the same chunk: with `maxBytes` 100 a 590-byte line is accepted. The + overshoot is at most one chunk. +- **n15.** For I3: the 5 s ack timeout can be short for real Pi, whose + `prompt()` may refresh OAuth or run auto-compaction before it acks. That + fails closed (poison, force stop) but the fake doesn't model it. +- **n16.** Compaction summaries are rendered on pages but not streamed, and + a quiet cut doesn't re-read after `run-settled`, so a summary appears + only after a reconnect or Ctrl-O (E4 "exactly once"). +- **n17.** Two interrupts with one request ID in flight: the second gets + `fenced` instead of the first's outcome. The client has no + `onOverflow`, and the controller's socket writes have no backpressure. +- **n18.** Test gaps: no test that a socket directory that isn't 0700 or is + a symlink refuses; none for an overlong or split multibyte engine line; + the escaping test covers only ESC, BEL and U+202E; H12 and H13 don't + assert events (brief line 962: "Every race asserts the engine bytes, the + receipts and the events"). + +README: + +- **n19.** "The scope launcher stops the cgroup with SIGKILL, because the + shim ignores SIGTERM by design" contradicts the code, which runs the + brief's TERM phase on `engine` members and then freeze and `cgroup.kill`. + The shim ignoring SIGTERM only keeps the scope's anchor alive. +- **n20.** The pieces table says the shim "answers `proof`"; its ops are + hello, events, members, term, freeze, kill and release. + +## Checked and found correct + +- Rule 4 receipt table, rule 5 stop outcomes, rule 6 reopening only on + Interrupted with every reconcile condition required; O1–O6 present; clear + failure sends no abort; `no-turn` lifts only its own fence; N1–N25 each + have a test. +- Force stop: the stopping revision before any signal; the invocation ID + checked through systemd and the shim; TERM phase then freeze, enumerate, + `cgroup.kill`, `populated 0`; `membershipComplete` only after all of + them; unavailable evidence ends `uncertain`; the pgroup fallback never + reaches `stopped`; restart re-runs from TERM. +- Recover: verified proof, unchanged pins and leaf, consumed confirmation, + single-use eligibility bound to the incarnation, launch rechecks it + (K6, K7, K17, K18). Foreign host refused, boot proof verified. +- Framing splits bytes at LF only and survives multibyte splits; the socket + directory is 0700, lstat-checked, owned, before listen; connections start + as observers; `stale-incarnation` first; the client never resends; + dedup by actor, conversation and request ID with `conflicting-request`; + `busy` decided under the lock; three write outcomes with poisoning; no + `streamingBehavior`, steer or follow-up is ever sent. +- Slash policy matches pinned Pi (`rpc-mode.js` 298–318, + `agent-session.js` 821–948): only a leading `/`, `/skill:` and templates + are interpreted, Pi doesn't trim, so refusing after whitespace is + conservative; `!`, `!!`, `@` and later-line `/` are admitted correctly. +- The quiet-cut rule holds for runs: Pi persists `message_end` before + `agent_settled`, the page read is under the lock. +- `ControlRefusal`, the four new package exports and the cross-package + `journal.mjs` import (same pattern as `packages/queue`). + +## Round 2 + +I'll review the touched files and rerun the mutants above (decision 34, +no-freeze, ENOENT-as-empty) plus the B1–B3 probes against the round 2 +candidate. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index 5b2b9983..f82753ca 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -464,3 +464,5 @@ are never rewritten or removed; corrections are new entries. 2026-10-04T08:11:41Z | Dewey (T3 Claude Code) | CHAT-03 I1 build, row 5 (#1507) | candidate frozen: manifest agents/dewey/work/chat-03/I1-manifest.sha256 (1404341e, 27 files), packet BUILD-I1.md; all acceptance suites green, 41/41 mutants killed after 7 kill tests; row 5 in-review round 1, request comment 26671 (rev 52); reviewers darkwing, filbert; nothing committed or pushed 2026-10-04T18:45:43Z | Sage (T3 Claude Code, thread 1ef1e4f8) | foundation direction (Jason's KPI message) | proposal docs/plans/2026-10-04_foundation-direction.md: five questions answered, brain dump mapped against what exists, Vikunja integrate-not-annex, slice 1 order; row 32 park recommended; needs Jason's ratification before any goal moves; row 5 I1 reviews pinged to darkwing and filbert 2026-10-04T18:52:41Z | Sage (T3 Claude Code, thread 1ef1e4f8) | row 5 r1 routing (#1507) | pushed darkwing's queue commit 5a5013c4 (verify rev 53 ok); woke Dewey (84fb346c, stopped) with B1/B2 rework under decision 42, and Filbert's session restarted on my INFO; Sage-launched sessions running: 2 of 2 (dewey, filbert); one round 2 to both reviewers after rework +2026-10-04T19:00:33Z | Filbert (T3 Claude Code, thread 9cb9731e) | row 5 CHAT-03 I1 review round 1 (#1507) | candidate 1404341e (27 files) verified frozen in a scratch clone and an export; §10 suites green (conductor 17/17 needs a git clone, queue 27 + 2 canonical-only skips); changes requested: B1 terminal Enter concatenation, B2 split paste-start ESC, B3 parallel force-stop escalations corrupt claim phaseStarted, B4 no test kills decision 34, B5 K12 freeze unproven (no-freeze mutant 18/18), B6 K15 missing-path case absent (ENOENT mutant 18/18); 20 notes; comment 26683, recorded rev 54 (filbert-5-r1-changes), committed f0b13a2f via queue-commit.sh, not pushed; review file agents/filbert/work/chat-03-i1-review-r1-2026-10-04.md (0444, uncommitted); REVIEW-RESULT to Dewey, copy to Sage; Darkwing's 26681 is on #1508, not the round's #1507 (flagged to Sage); rev 55 (jason park row 32) left uncommitted +2026-10-04T19:01:37Z | Sage (T3 Claude Code, thread 1ef1e4f8) | foundation ratified, row 32 parked, row 5 r1 records | Jason ratified the foundation direction (lead decision 44; goals page superseded in part); row 32 parked on Jason's word (rev 55, 3270cc4e); row 5 r1 closed with both reviewers requesting changes (darkwing 26681, filbert 26683), both review files committed; pointer comment 26685 on #1507 for darkwing's verdict that landed on #1508; DEFERRED item for review record not checking the comment's issue diff --git a/docs/plans/DEFERRED.md b/docs/plans/DEFERRED.md index a66ba59b..03100a32 100644 --- a/docs/plans/DEFERRED.md +++ b/docs/plans/DEFERRED.md @@ -165,6 +165,15 @@ at every gate. Started 2026-09-12 during the control board MVP. no `timeout` on PATH the queue issue calls report "credential or Gitea request failure". Optional test and clearer message. Filbert, E r2. (2026-09-27, #1508) +- **`review record` doesn't check which issue the verdict comment is on.** + Row 5 round 1's request (26671) and Filbert's verdict (26683) are on + #1507. Darkwing's verdict 26681 went to #1508, and `review record` + accepted it, because only `review resolve` reads the comment back + (`checkComment` in `packages/queue/src/review.mjs`). The receipt + points at a real comment on the wrong issue. Sage posted a pointer on + #1507 (comment 26685). Fix: have `record` check the comment's issue and author the way + `resolve` does, or refuse a comment id it can't verify. Found by + Filbert. (2026-10-04, #1508) ## Queue