review(59): Darkwing round 1 packet, approve (#1548 comment 27487)

Candidate 90d5bddc on f2f2861b. bus 86/0, webui 40/0, every scripts/test-*.sh green 23:23:30Z-23:26:25Z; 20 of 21 mutants killed, any-role-msg equivalent. Notes: a claim that outlives its runner, a stranded delivered message (bus follow-up to Sage), overlap with row 58. Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-11 18:32:46 -05:00
parent 60c8dcf970
commit 3417d21483
9 changed files with 288 additions and 0 deletions
@@ -0,0 +1,10 @@
+test-auth rc=0 selftest: 15 passed, 0 failed
+test-conductor rc=0 selftest: 17 passed, 0 failed
+test-config rc=0 selftest: 24 passed, 0 failed
+test-discord rc=0 discord suite: 66 passed, 0 failed
+test-extension-package rc=0 extension package selftest: 18 passed, 0 failed
+test-foundation rc=0 selftest: 44 passed, 0 failed
+test-queue rc=0 queue suite: 27 passed, 0 failed
+test-release rc=0 selftest: 14 passed, 0 failed
+test-task rc=0 selftest: 26 passed, 0 failed
+DONE
@@ -0,0 +1,10 @@
+#!/bin/bash
+# Every scripts/test-*.sh in the review worktree, one at a time; bus and webui ran in tests.sh.
+export TMPDIR=~/darkwing-scratch/rv59/tmp; O=~/darkwing-scratch/rv59/out/gate; mkdir -p $O; rm -f $O/summary.txt; cd ~/darkwing-scratch/rv59/wt
+[ -x node_modules/.bin/pi ] || { echo "node_modules not linked" >> $O/summary.txt; exit 1; }
+for s in scripts/test-*.sh; do
+  n=$(basename $s .sh)
+  if [ $n = test-task ]; then DOCKER_HOST=unix:///nonexistent bash $s > $O/$n.log 2>&1; else bash $s > $O/$n.log 2>&1; fi
+  echo "$n rc=$? $(grep -iE '[0-9]+ (passed|pass)' $O/$n.log | tail -1)" >> $O/summary.txt
+done
+echo DONE >> $O/summary.txt
@@ -0,0 +1,15 @@
+#!/bin/bash
+export TMPDIR=~/darkwing-scratch/rv59/tmp; cd ~/darkwing-scratch/rv59/wt; M=~/darkwing-scratch/rv59/mut.mjs
+B=packages/bus/src/broker.mjs; W=packages/webui/src/public/bus.js; BT=packages/bus/tests/agents.test.mjs; WB=packages/webui/tests/bus-browser.test.mjs
+run() { # name file from to test-file
+  node $M "$2" "$3" "$4" || { echo "$1: MUTATION DID NOT APPLY"; return; }
+  env -u NODE_TEST_CONTEXT node --test --test-timeout=60000 "$5" > $TMPDIR/mut-$1.log 2>&1
+  echo "$1: rc=$? $(grep -E '^ℹ (pass|fail) ' $TMPDIR/mut-$1.log | tr '\n' ' ')"
+  grep -E '^✖ ' $TMPDIR/mut-$1.log | sed 's/ ([0-9.]*ms)$//; s/^/    /' | sort -u | head -4
+  git checkout -q -- "$2"
+}
+run seq-order $B "route_to='human' AND NOT EXISTS(SELECT 1 FROM decision_events x WHERE x.decision=d.id AND x.op IN \${CLOSED}) ORDER BY CASE class WHEN 'gated' THEN 0 ELSE 1 END,seq" "route_to='human' AND NOT EXISTS(SELECT 1 FROM decision_events x WHERE x.decision=d.id AND x.op IN \${CLOSED}) ORDER BY seq" $BT
+run blocking-raw $B "class: d.class, blocking: !!d.blocking, task_ref" "class: d.class, blocking: d.blocking, task_ref" $BT
+run no-blocking-chip $W "short(d.id))}</span>\${d.blocking ? \` \${blockChip('blocking')}\` : ''}" "short(d.id))}</span>" $WB
+run question-raw $W 'Input needed: <a href="#/inbox/${enc(d.id)}">${txt(first(d.question))}</a>' 'Input needed: <a href="#/inbox/${enc(d.id)}">${first(d.question)}</a>' $WB
+git diff --quiet -- $B $W && echo "restored: sources match the candidate index" || echo "NOT RESTORED"
@@ -0,0 +1,6 @@
+// node mut.mjs <file> <from> <to>: replace exactly one occurrence, else exit 3.
+import { readFileSync, writeFileSync } from 'node:fs';
+const [f, a, b] = process.argv.slice(2);
+const s = readFileSync(f, 'utf8'); const n = s.split(a).length - 1;
+if (n !== 1) { console.error(`matches=${n}`); process.exit(3); }
+writeFileSync(f, s.replace(a, () => b));
@@ -0,0 +1,13 @@
+seq-order: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: input needed is the role's open decisions routed to the human, gated first, then oldest
+    ✖ failing tests:
+blocking-raw: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: input needed is the role's open decisions routed to the human, gated first, then oldest
+    ✖ failing tests:
+no-blocking-chip: rc=1 ℹ pass 0 ℹ fail 1 
+    ✖ failing tests:
+    ✖ S5 pages in a browser: real broker rows, inert text, Copy, routes, failures, layout and no writes
+question-raw: rc=1 ℹ pass 0 ℹ fail 1 
+    ✖ failing tests:
+    ✖ S5 pages in a browser: real broker rows, inert text, Copy, routes, failures, layout and no writes
+restored: sources match the candidate index
@@ -0,0 +1,37 @@
+#!/bin/bash
+# Darkwing's own row 59 mutants. Each edits one source file in the review worktree, runs one
+# test file, then restores the file from the worktree index, which holds the candidate.
+export TMPDIR=~/darkwing-scratch/rv59/tmp
+cd ~/darkwing-scratch/rv59/wt
+M=~/darkwing-scratch/rv59/mut.mjs
+B=packages/bus/src/broker.mjs; W=packages/webui/src/public/bus.js
+BT=packages/bus/tests/agents.test.mjs; WE=packages/webui/tests/bus.test.mjs; WB=packages/webui/tests/bus-browser.test.mjs
+run() { # name file from to test-file
+  node $M "$2" "$3" "$4" || { echo "$1: MUTATION DID NOT APPLY"; return; }
+  env -u NODE_TEST_CONTEXT node --test --test-timeout=60000 "$5" > $TMPDIR/mut-$1.log 2>&1
+  echo "$1: rc=$? $(grep -E '^ℹ (pass|fail) ' $TMPDIR/mut-$1.log | tr '\n' ' ')"
+  grep -E '^✖ ' $TMPDIR/mut-$1.log | sed 's/ ([0-9.]*ms)$//; s/^/    /' | sort -u | head -4
+  git checkout -q -- "$2"
+}
+run any-holder $B 'AND d.holder_run=? AND' 'AND (d.holder_run=? OR 1) AND' $BT
+run read-ignored $B " AND NOT EXISTS(SELECT 1 FROM deliveries r WHERE r.message=m.id AND r.op='read')\"" '"' $BT
+run any-business-msg $B 'WHERE m.business=? AND m.to_role=?' 'WHERE (m.business=? OR 1) AND m.to_role=?' $BT
+run any-role-msg $B 'AND m.to_role=? AND' 'AND (m.to_role=? OR 1) AND' $BT
+run since-max $B 'min(d.at) AS since' 'max(d.at) AS since' $BT
+run any-route $B "raised_by_role=? AND route_to='human' AND" 'raised_by_role=? AND' $BT
+run closed-kept $B "AND route_to='human' AND NOT EXISTS(SELECT 1 FROM decision_events x WHERE x.decision=d.id AND x.op IN \${CLOSED}) ORDER" "AND route_to='human' ORDER" $BT
+run seq-order $B "ORDER BY CASE class WHEN 'gated' THEN 0 ELSE 1 END,seq" 'ORDER BY seq' $BT
+run any-raiser $B 'AND raised_by_role=? AND' 'AND (raised_by_role=? OR 1) AND' $BT
+run waiting-wins $B "const state = working ? 'working' : inputNeeded.length ? 'waiting' : 'idle';" "const state = inputNeeded.length ? 'waiting' : working ? 'working' : 'idle';" $BT
+run role-gets-state $B 'return s.human || s.reader ? claims.map((c) => this.#agentState(c)) : claims;' 'return claims.map((c) => this.#agentState(c));' $BT
+run reader-plain $B 'return s.human || s.reader ? claims.map' 'return s.human ? claims.map' $BT
+run blocking-raw $B 'blocking: !!d.blocking' 'blocking: d.blocking' $BT
+run working-zero $B 'const working = busy.messages ? { messages: busy.messages, since: busy.since } : null;' 'const working = { messages: busy.messages, since: busy.since };' $BT
+run endpoint-plain $B 'return s.human || s.reader ? claims.map((c) => this.#agentState(c)) : claims;' 'return claims;' $WE
+run last-ask $W 'const d = asks[0];' 'const d = asks[asks.length - 1];' $WB
+run no-more-link $W "\${asks.length > 1 ? \` <a href=\"#/inbox\">+\${esc(asks.length - 1)} more</a>\` : ''}" '' $WB
+run no-working-detail $W "\${c.working ? \`<span class=\"source\">\${esc(plural(c.working.messages, 'message'))} since \${when(c.working.since)}</span>\` : ''}" '' $WB
+run no-blocking-chip $W "\${d.blocking ? \` \${blockChip('blocking')}\` : ''}" '' $WB
+run question-raw $W '${txt(first(d.question))}</a>' '${first(d.question)}</a>' $WB
+run no-state-col $W "['Role', 'State', 'Attention', 'Holder'" "['Role', 'Attention', 'Holder'" $WB
+git diff --quiet -- $B $W && echo "restored: sources match the candidate index" || echo "NOT RESTORED"
@@ -0,0 +1,62 @@
+any-holder: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: a new holder doesn't inherit what the last one left unread; decisions stay with the role
+    ✖ failing tests:
+read-ignored: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: working counts what the holder received and has not read; routed alone is not working
+    ✖ failing tests:
+any-business-msg: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: state reads only its own business, even for a run name both use
+    ✖ failing tests:
+any-role-msg: rc=0 ℹ pass 6 ℹ fail 0 
+since-max: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: working counts what the holder received and has not read; routed alone is not working
+    ✖ failing tests:
+any-route: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: input needed is the role's open decisions routed to the human, gated first, then oldest
+    ✖ failing tests:
+closed-kept: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: input needed is the role's open decisions routed to the human, gated first, then oldest
+    ✖ failing tests:
+matches=2
+seq-order: MUTATION DID NOT APPLY
+any-raiser: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: input needed is the role's open decisions routed to the human, gated first, then oldest
+    ✖ failing tests:
+waiting-wins: rc=1 ℹ pass 3 ℹ fail 3 
+    ✖ agents: a new holder doesn't inherit what the last one left unread; decisions stay with the role
+    ✖ agents: state reads only its own business, even for a run name both use
+    ✖ agents: working wins over waiting, and Input needed still shows
+    ✖ failing tests:
+role-gets-state: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: a held role with nothing on the bus is idle; a role session gets the claim rows unchanged
+    ✖ failing tests:
+reader-plain: rc=1 ℹ pass 5 ℹ fail 1 
+    ✖ agents: a held role with nothing on the bus is idle; a role session gets the claim rows unchanged
+    ✖ failing tests:
+matches=2
+blocking-raw: MUTATION DID NOT APPLY
+working-zero: rc=1 ℹ pass 1 ℹ fail 5 
+    ✖ agents: a held role with nothing on the bus is idle; a role session gets the claim rows unchanged
+    ✖ agents: a new holder doesn't inherit what the last one left unread; decisions stay with the role
+    ✖ agents: input needed is the role's open decisions routed to the human, gated first, then oldest
+    ✖ agents: state reads only its own business, even for a run name both use
+endpoint-plain: rc=1 ℹ pass 7 ℹ fail 1 
+    ✖ bus routes serve the four reads from the reader view, unescaped JSON for the page to escape
+    ✖ failing tests:
+last-ask: rc=1 ℹ pass 0 ℹ fail 1 
+    ✖ failing tests:
+    ✖ S5 pages in a browser: real broker rows, inert text, Copy, routes, failures, layout and no writes
+no-more-link: rc=1 ℹ pass 0 ℹ fail 1 
+    ✖ failing tests:
+    ✖ S5 pages in a browser: real broker rows, inert text, Copy, routes, failures, layout and no writes
+no-working-detail: rc=1 ℹ pass 0 ℹ fail 1 
+    ✖ failing tests:
+    ✖ S5 pages in a browser: real broker rows, inert text, Copy, routes, failures, layout and no writes
+matches=2
+no-blocking-chip: MUTATION DID NOT APPLY
+matches=2
+question-raw: MUTATION DID NOT APPLY
+no-state-col: rc=1 ℹ pass 0 ℹ fail 1 
+    ✖ failing tests:
+    ✖ S5 pages in a browser: real broker rows, inert text, Copy, routes, failures, layout and no writes
+restored: sources match the candidate index
@@ -0,0 +1,3 @@
+#!/bin/bash
+export TMPDIR=~/darkwing-scratch/rv59/tmp; cd ~/darkwing-scratch/rv59/wt; O=~/darkwing-scratch/rv59/out
+for s in bus webui; do env -u NODE_TEST_CONTEXT node --test "packages/$s/tests/*.test.mjs" > $O/$s-tests.log 2>&1; echo "$s rc=$? $(grep -E '^ℹ (pass|fail) ' $O/$s-tests.log | tr '\n' ' ')"; done
@@ -0,0 +1,132 @@
+# Row 59, bus read: agent state per role, round 1 review (Darkwing)
+
+Issue #1548, request comment 27371, packet `agents/filbert/work/queue-59/`
+(commit `e82f7c32`). Brief: `docs/plans/2026-10-10_design-implementation.md`,
+section "Bus read: agent state per role". Base `f2f2861b`. Candidate
+manifest sha256
+`90d5bddc545e429091bf43202ef70cb55eeb1ce964964f440a0c753decb82f71`, 8
+files, matching the request.
+
+Verdict: **approve**. The broker derives each held role's state from the
+claim, delivery and decision rows it already keeps, writes nothing and
+changes no schema. A role session still gets the plain claim rows. The
+Agents view escapes everything it prints. Nothing blocks. I have two notes
+on the README's limits, one bus follow-up that predates this row, and
+answers to both questions.
+
+## Method
+
+- I read the whole patch: `#agentState` and the `agents` branch in
+  `broker.mjs`, the webui `bus.js` and `bus.css` changes, both READMEs and
+  the three test files. I checked the working query against `receive`
+  (which skips delivered messages, so a message can't count twice) and the
+  `inputNeeded` order against the Inbox query in the same file. I checked
+  the runner's claim in `packages/harness/src/runner.mjs`, lines 311 to 350:
+  it receives a batch, takes a turn per message, sends the result and marks
+  the message read, and its catch only exits on `SESSION_GONE`.
+- A detached worktree at `f2f2861b` with `git apply --index build.patch`,
+  then `sha256sum -c` on the manifest: 8 OK.
+- 21 mutants of my own (`r1/mutants.sh`, `r1/m2.sh`, and `r1/mut.mjs`,
+  which replaces text that must match exactly once). Each ran one test
+  file and restored the source from the worktree index. Four of the first
+  pass matched twice and didn't apply; `m2.sh` reran them with longer
+  matches.
+
+Node v26.8.1, `TMPDIR=~/darkwing-scratch/rv59/tmp`.
+
+## Suites
+
+On the candidate tree (`f2f2861b` plus `build.patch`), one at a time.
+The two package runs were at 22:36Z (`r1/tests.sh`); the scripts ran
+23:23:30Z to 23:26:25Z (`r1/gate.sh`, `r1/gate-summary.txt`). `test-task`
+ran with `DOCKER_HOST=unix:///nonexistent`.
+
+| Suite | Result |
+|---|---|
+| bus `node --test` | 86 pass, 0 fail |
+| webui `node --test` | 40 pass, 0 fail |
+| test-auth | 15 passed, 0 failed |
+| test-conductor | 17 passed, 0 failed |
+| test-config | 24 passed, 0 failed |
+| test-discord | 66 passed, 0 failed |
+| test-extension-package | 18 passed, 0 failed |
+| test-foundation | 44 passed, 0 failed |
+| test-queue | 27 passed, 0 failed |
+| test-release | 14 passed, 0 failed |
+| test-task | 26 passed, 0 failed |
+
+None of the 8 files changed between `f2f2861b` and `HEAD` (`d7548697`)
+at the time of review.
+
+## Mutants
+
+20 of 21 fail (`r1/mutants.txt`, `r1/mutants-2.txt`). I checked each
+browser kill's assertion; each failed on the check meant for it.
+`question-raw` fails the injected-markup count, not a layout check.
+
+| Mutant | Fails |
+|---|---|
+| `any-holder`: deliveries to any holder count | agents 5 (new holder) |
+| `read-ignored`: read deliveries still count | agents 2 |
+| `any-business-msg`: messages from every business | agents 6 |
+| `any-role-msg`: `m.to_role` dropped | survives, equivalent |
+| `since-max`: newest delivery as `since` | agents 2 |
+| `any-route`: decisions routed anywhere | agents 3 |
+| `closed-kept`: closed decisions still listed | agents 3 |
+| `seq-order`: no gated-first order | agents 3 |
+| `any-raiser`: decisions raised by any role | agents 3 |
+| `waiting-wins`: waiting over working | agents 4, 5, 6 |
+| `role-gets-state`: role sessions get state too | agents 1 |
+| `reader-plain`: readers get plain rows | agents 1 |
+| `blocking-raw`: `blocking` not coerced to a boolean | agents 3 |
+| `working-zero`: `working` set with no messages | agents 1, 3, 5, 6 |
+| `endpoint-plain`: no state for anyone | webui endpoint test |
+| `last-ask`: the last decision shown first | browser |
+| `no-more-link`: no "+N more" | browser |
+| `no-working-detail`: no message count or since | browser |
+| `no-blocking-chip` | browser |
+| `question-raw`: the question printed unescaped | browser |
+| `no-state-col`: no State column | browser |
+
+`any-role-msg` is equivalent, as the packet says. `bindLaunch` refuses a
+second launch record for a run name with another role
+(`launch-record-mismatch`), so within a business a holder run belongs to
+one role, and its deliveries are for that role's messages.
+
+## Notes
+
+**A. A runner whose claim outlives it.** The README's limits say a reply
+that fails leaves its message unread. There is a second case. The
+launcher calls `endLaunch` when a run's process exits, and `endLaunch`
+releases the claim, so a crashed or killed runner normally clears. If the
+launcher itself dies, or `endLaunch` is refused (the launcher logs
+`endLaunch refused` and goes on), the claim stays and the role shows
+working, with a `since` that keeps growing, or idle, until someone
+revokes it. One clause in the limits paragraph would cover it. Not
+blocking.
+
+**B. A delivered message can be stranded (predates this row).** `receive`
+skips messages that already have a delivery. If a holder receives a
+message and releases the role without marking it read, the next holder
+never gets it. Row 59 shows this correctly: the new holder is idle and
+doesn't inherit the old holder's count, as test 5 pins. But the message
+sits there unanswered with nothing pointing at it. That belongs in a bus
+row, not this one; I'm sending it to Sage.
+
+**C. Overlap with row 58.** Rows 58 and 59 both change `Broker` in
+`broker.mjs` and the webui `bus.js`: row 58 changes `#taskRows` and the
+Tasks view, and row 59 changes the `agents` branch, `#agentState` and the
+Agents view. The hunks don't touch each other. Whichever row lands second
+should rebase and rerun the browser test.
+
+## Questions
+
+1. **Working signal.** Received and not read is the right signal. The
+   runner marks a message read only after its turn's reply, and the bus
+   has no per-turn record, so it is the only signal available. The other
+   option, the claim's own activity, would need a new write. Note A is
+   where this signal gets it wrong.
+2. **Attention.** The first decision and a count is right. The Inbox
+   already lists every decision in the same order, and one line per role
+   keeps the table readable. "+N more" links to the Inbox, so the rest is
+   one click away.