From 29124395f4fa4170499c42c9b43efe7b4a213bc2 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 11 Oct 2026 20:23:26 -0500 Subject: [PATCH] review(darkwing): row 76 round 1 review packet, approve (#1570) Co-Authored-By: Claude Opus 5.5 --- agents/darkwing/work/queue-76-review/r1/m2.sh | 20 ++ .../darkwing/work/queue-76-review/r1/m2.txt | 48 +++++ .../work/queue-76-review/r1/mutants.sh | 31 +++ .../work/queue-76-review/r1/mutants.txt | 80 ++++++++ .../work/queue-76-review/r1/tests-light.txt | 20 ++ .../work/queue-76-review/review-r1.md | 180 ++++++++++++++++++ 6 files changed, 379 insertions(+) create mode 100644 agents/darkwing/work/queue-76-review/r1/m2.sh create mode 100644 agents/darkwing/work/queue-76-review/r1/m2.txt create mode 100644 agents/darkwing/work/queue-76-review/r1/mutants.sh create mode 100644 agents/darkwing/work/queue-76-review/r1/mutants.txt create mode 100644 agents/darkwing/work/queue-76-review/r1/tests-light.txt create mode 100644 agents/darkwing/work/queue-76-review/review-r1.md diff --git a/agents/darkwing/work/queue-76-review/r1/m2.sh b/agents/darkwing/work/queue-76-review/r1/m2.sh new file mode 100644 index 000000000..787e692dc --- /dev/null +++ b/agents/darkwing/work/queue-76-review/r1/m2.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash +# The four survivors of mutants.sh, rerun against the other test files that could catch them. +set -u +M=$1 +W=packages/webui +P=$W/src/public/bus.js A=$W/src/public/app.js +run() { # name file from to testfile... + local name=$1 f=$2 from=$3 to=$4; shift 4 + for t in "$@"; do + echo "== $name on $t" + if ! node "$M" "$f" "$from" "$to"; then echo "$name: DID NOT APPLY"; return; fi + node --test "$W/tests/$t.test.mjs" 2>&1 | grep -E '^ℹ (pass|fail)|^✖ ' | grep -v '^✖ failing tests' | sort -u | sed "s/^/$name: /" + git checkout -q -- "$f"; echo "restored" + done +} +run page-caret-lost "$P" " if (k.caret && el.tagName === 'TEXTAREA') { el.setSelectionRange(k.caret[0], k.caret[1]); el.scrollTop = k.caret[2]; }" '' bus-browser +run page-no-config-gate "$P" "if (c?.send === true) {" "if (c) {" send-browser shell +run page-composer-always "$P" " if (!canSend) return '';" '' send-browser shell +run app-reply-4xx-unknown "$A" 'const refused = err.status >= 400 && err.status < 500;' 'const refused = false;' return-flow browser-edge history-return-flow shell +run app-reply-5xx-refused "$A" 'const refused = err.status >= 400 && err.status < 500;' 'const refused = err.status >= 400;' return-flow browser-edge diff --git a/agents/darkwing/work/queue-76-review/r1/m2.txt b/agents/darkwing/work/queue-76-review/r1/m2.txt new file mode 100644 index 000000000..021338c74 --- /dev/null +++ b/agents/darkwing/work/queue-76-review/r1/m2.txt @@ -0,0 +1,48 @@ +start 2026-10-12T01:19:14Z +== page-caret-lost on bus-browser +page-caret-lost: ℹ fail 0 +page-caret-lost: ℹ pass 1 +restored +== page-no-config-gate on send-browser +page-no-config-gate: ℹ fail 1 +page-no-config-gate: ℹ pass 0 +page-no-config-gate: ✖ the send box: held roles, the draft across a refresh, each receipt, one POST per Send, and the trail link (3517.666605ms) +restored +== page-no-config-gate on shell +page-no-config-gate: ℹ fail 0 +page-no-config-gate: ℹ pass 5 +restored +== page-composer-always on send-browser +page-composer-always: ℹ fail 1 +page-composer-always: ℹ pass 0 +page-composer-always: ✖ the send box: held roles, the draft across a refresh, each receipt, one POST per Send, and the trail link (1171.730778ms) +restored +== page-composer-always on shell +page-composer-always: ℹ fail 0 +page-composer-always: ℹ pass 5 +restored +== app-reply-4xx-unknown on return-flow +app-reply-4xx-unknown: ℹ fail 0 +app-reply-4xx-unknown: ℹ pass 1 +restored +== app-reply-4xx-unknown on browser-edge +app-reply-4xx-unknown: ℹ fail 0 +app-reply-4xx-unknown: ℹ pass 1 +restored +== app-reply-4xx-unknown on history-return-flow +app-reply-4xx-unknown: ℹ fail 0 +app-reply-4xx-unknown: ℹ pass 1 +restored +== app-reply-4xx-unknown on shell +app-reply-4xx-unknown: ℹ fail 0 +app-reply-4xx-unknown: ℹ pass 5 +restored +== app-reply-5xx-refused on return-flow +app-reply-5xx-refused: ℹ fail 0 +app-reply-5xx-refused: ℹ pass 1 +restored +== app-reply-5xx-refused on browser-edge +app-reply-5xx-refused: ℹ fail 0 +app-reply-5xx-refused: ℹ pass 1 +restored +end 2026-10-12T01:22:00Z diff --git a/agents/darkwing/work/queue-76-review/r1/mutants.sh b/agents/darkwing/work/queue-76-review/r1/mutants.sh new file mode 100644 index 000000000..836f8f6f7 --- /dev/null +++ b/agents/darkwing/work/queue-76-review/r1/mutants.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# Row 76 round 1, Darkwing's mutants. Run from the candidate worktree root: +# bash mutants.sh . Each mutant edits one site, runs one test file, +# and restores the file from the worktree index. +set -u +M=$1 +W=packages/webui +S=$W/src/serve.mjs B=$W/src/bus.mjs C=$W/src/cli.mjs P=$W/src/public/bus.js A=$W/src/public/app.js +run() { # name file from to testfile + local name=$1 f=$2 from=$3 to=$4 t=$5 + echo "== $name" + if ! node "$M" "$f" "$from" "$to"; then echo "$name: DID NOT APPLY"; return; fi + node --test "$W/tests/$t.test.mjs" 2>&1 | grep -E '^ℹ (pass|fail)|^✖ ' | grep -v '^✖ failing tests' | sort -u | sed "s/^/$name: /" + git checkout -q -- "$f"; echo "restored" +} +run cli-no-bus-ok "$C" ' if (sendOn) throw new Error(`--send needs the bus configured (${why})`);' '' send +run start-send-nonfunction "$S" "(preview === null || typeof send !== 'function')" '(preview === null)' send +run open-no-invalid-response "$B" "const OPEN = ['outcome-unknown', 'invalid-response', 'response-too-large'];" "const OPEN = ['outcome-unknown', 'response-too-large'];" send +run open-no-too-large "$B" "const OPEN = ['outcome-unknown', 'invalid-response', 'response-too-large'];" "const OPEN = ['outcome-unknown', 'invalid-response'];" send +run nonbus-definite "$B" "err instanceof BusReadError ? err.code : 'outcome-unknown'" "err?.code ?? 'invalid-request'" send +run missing-to-passes "$S" " || !('to' in value)" '' send +run route-any-method "$S" " if (req.method !== 'POST') return json(res, 405, { error: 'method not allowed' });" '' send +run status-mnf-502 "$B" "'message-not-found': 400, " '' send +run success-unlogged "$S" ' line(`id=${result.id}`);' '' send +run page-never-clears "$P" "if (draft.body === text) draft.body = '';" '' send-browser +run page-caret-lost "$P" " if (k.caret && el.tagName === 'TEXTAREA') { el.setSelectionRange(k.caret[0], k.caret[1]); el.scrollTop = k.caret[2]; }" '' send-browser +run page-sentfalse-ignored "$P" '((res.status >= 400 && res.status < 500) || body?.sent === false)' '(res.status >= 400 && res.status < 500)' send-browser +run page-no-config-gate "$P" "if (c?.send === true) {" "if (c) {" bus-browser +run page-composer-always "$P" " if (!canSend) return '';" '' bus-browser +run app-reply-4xx-unknown "$A" 'const refused = err.status >= 400 && err.status < 500;' 'const refused = false;' browser +run app-reply-5xx-refused "$A" 'const refused = err.status >= 400 && err.status < 500;' 'const refused = err.status >= 400;' preview diff --git a/agents/darkwing/work/queue-76-review/r1/mutants.txt b/agents/darkwing/work/queue-76-review/r1/mutants.txt new file mode 100644 index 000000000..8f3b74ca7 --- /dev/null +++ b/agents/darkwing/work/queue-76-review/r1/mutants.txt @@ -0,0 +1,80 @@ +start 2026-10-12T01:17:37Z +== cli-no-bus-ok +cli-no-bus-ok: ℹ fail 1 +cli-no-bus-ok: ℹ pass 12 +cli-no-bus-ok: ✖ the CLI refuses --send without the preview flags, and without a configured bus (10200.769622ms) +restored +== start-send-nonfunction +start-send-nonfunction: ℹ fail 1 +start-send-nonfunction: ℹ pass 12 +start-send-nonfunction: ✖ startServer takes send only as a function and only with the preview mode (11.492433ms) +restored +== open-no-invalid-response +open-no-invalid-response: ✖ a bus refusal is not sent, with its status; an outcome the bus could not confirm is unknown and never retried (7.441283ms) +open-no-invalid-response: ℹ fail 2 +open-no-invalid-response: ℹ pass 11 +open-no-invalid-response: ✖ through the human transport the request is exactly the business, message.send and the keys (117.993747ms) +restored +== open-no-too-large +open-no-too-large: ✖ a bus refusal is not sent, with its status; an outcome the bus could not confirm is unknown and never retried (8.767124ms) +open-no-too-large: ℹ fail 1 +open-no-too-large: ℹ pass 12 +restored +== nonbus-definite +nonbus-definite: ✖ a bus refusal is not sent, with its status; an outcome the bus could not confirm is unknown and never retried (8.725677ms) +nonbus-definite: ℹ fail 1 +nonbus-definite: ℹ pass 12 +restored +== missing-to-passes +missing-to-passes: ✖ each request check refuses before anything is sent, and logs the refusal without the body (16.621682ms) +missing-to-passes: ℹ fail 1 +missing-to-passes: ℹ pass 12 +restored +== route-any-method +route-any-method: ✖ each request check refuses before anything is sent, and logs the refusal without the body (28.066332ms) +route-any-method: ℹ fail 1 +route-any-method: ℹ pass 12 +restored +== status-mnf-502 +status-mnf-502: ✖ a bus refusal is not sent, with its status; an outcome the bus could not confirm is unknown and never retried (6.018709ms) +status-mnf-502: ✖ against a real broker: the message is written once as a human send, and its refusals map (97.093713ms) +status-mnf-502: ℹ fail 2 +status-mnf-502: ℹ pass 11 +restored +== success-unlogged +success-unlogged: ✖ a send passes exactly to, body and in_reply_to, answers the id, and logs it (4.473721ms) +success-unlogged: ℹ fail 2 +success-unlogged: ℹ pass 11 +success-unlogged: ✖ through the human transport the request is exactly the business, message.send and the keys (135.764088ms) +restored +== page-never-clears +page-never-clears: ℹ fail 1 +page-never-clears: ℹ pass 0 +page-never-clears: ✖ the send box: held roles, the draft across a refresh, each receipt, one POST per Send, and the trail link (7056.220257ms) +restored +== page-caret-lost +page-caret-lost: ℹ fail 0 +page-caret-lost: ℹ pass 1 +restored +== page-sentfalse-ignored +page-sentfalse-ignored: ℹ fail 1 +page-sentfalse-ignored: ℹ pass 0 +page-sentfalse-ignored: ✖ the send box: held roles, the draft across a refresh, each receipt, one POST per Send, and the trail link (6137.191886ms) +restored +== page-no-config-gate +page-no-config-gate: ℹ fail 0 +page-no-config-gate: ℹ pass 1 +restored +== page-composer-always +page-composer-always: ℹ fail 0 +page-composer-always: ℹ pass 1 +restored +== app-reply-4xx-unknown +app-reply-4xx-unknown: ℹ fail 0 +app-reply-4xx-unknown: ℹ pass 1 +restored +== app-reply-5xx-refused +app-reply-5xx-refused: ℹ fail 0 +app-reply-5xx-refused: ℹ pass 11 +restored +end 2026-10-12T01:18:51Z diff --git a/agents/darkwing/work/queue-76-review/r1/tests-light.txt b/agents/darkwing/work/queue-76-review/r1/tests-light.txt new file mode 100644 index 000000000..209e934e4 --- /dev/null +++ b/agents/darkwing/work/queue-76-review/r1/tests-light.txt @@ -0,0 +1,20 @@ +start 2026-10-12T01:16:27Z +== send +ℹ pass 13 +ℹ fail 0 +== send-browser +ℹ pass 1 +ℹ fail 0 +== shell +ℹ pass 5 +ℹ fail 0 +== preview +ℹ pass 11 +ℹ fail 0 +== bus +ℹ pass 9 +ℹ fail 0 +== serve +ℹ pass 4 +ℹ fail 0 +end 2026-10-12T01:16:44Z diff --git a/agents/darkwing/work/queue-76-review/review-r1.md b/agents/darkwing/work/queue-76-review/review-r1.md new file mode 100644 index 000000000..116ce16e4 --- /dev/null +++ b/agents/darkwing/work/queue-76-review/review-r1.md @@ -0,0 +1,180 @@ +# Row 76, Console sends a message to a role, round 1 review (Darkwing) + +Issue #1570, request comment 27594, packet `agents/dewey/work/queue-76/` +(commit `1deb6143`). Brief: `docs/plans/2026-10-11_dogfood-cutover.md`, +"Console sends a message to a role". Base `3d298920`. Candidate manifest +sha256 `ca8703145aed98142c1fdf0553ab846a9b9a808cd58c35f7cbb4fa35d7f279a2`, +9 files, matching the request. Sage asked for this round on content now; +the row re-rounds after row 59's landing, which changes the README and +`bus.js` digests. + +Verdict: **approve**. The route refuses everything the brief lists before +it calls the bus, and logs one line per send without the body. Dewey's +decision 5, that a coded bus answer means nothing was written, holds on +every path I traced from the socket to the page. The page sends once per +press and never resends. Nothing blocks. Five notes and my answers to the +two questions are below. + +## Method + +- A detached worktree at `3d298920` with `git apply --index build.patch`, + then `sha256sum -c` on the manifest: 9 OK, and again after the mutants. + None of the 9 paths changed from `3d298920` to `1ab6e181` at the start. +- I read the whole patch, and the bus code the send passes through: + `Broker.request` and the `message.send` case, `Store.transaction`, the + socket server, `Client.call`, `human-cli.mjs` and `humanCall`. +- Six WebUI test files, then 16 mutants of my own (`r1/mutants.sh`) and a + rerun of the four survivors against other files (`r1/m2.sh`). Each mutant + edits one site, runs one test file and restores from the worktree index, + using `agents/darkwing/work/queue-59-review/r1/mut.mjs`. + +Node v26.8.1, `TMPDIR=~/darkwing-scratch/rv76/tmp`. + +While I was reviewing, Dewey's working copy of the manifest and +`build.patch` changed (uncommitted, the row 59 re-round). Everything here +is against the committed `ca870314`, which still checks 9 OK on my tree. + +## Is a coded answer really "not written"? + +This is what the page's "Not sent" rests on, so I traced each way an error +code can reach `sendOutcome` (`packages/webui/src/bus.mjs:78`). + +- The broker runs `#dispatch` and the result's secret check inside one + `BEGIN IMMEDIATE` transaction (`broker.mjs` `request`, `store.mjs` + `transaction`). Anything thrown there, a failed `COMMIT` included, rolls + back. `#refused` then writes its `action.refused` event in a separate + transaction, or answers `storage-unavailable` if it can't. Both come + after the rollback. +- The socket server (`server.mjs`) calls `broker.request` synchronously for + `message.send`, so its `request-timeout` (line 27) can only fire before + the request is handled. `invalid-envelope`, `incomplete-request` and + `request-too-large` also come before the broker. +- `Client.call` turns every socket error, end or timeout into + `outcome-unknown`, and an answer it can't parse into `invalid-response`. + It refuses an oversized request before connecting. +- `human-cli.mjs` writes `invalid-request` for an uncoded error (line 37). + Inside its `try`, the only steps that can throw without a code run + before `client.call`. A failure writing the result to stdout surfaces as + an `error` event, not in that `catch`, and the parent reads that as + `invalid-response`. +- `humanCall` maps a kill, a timeout and a missing code to + `outcome-unknown` or `invalid-response`. + +So the three codes in `OPEN` are the only ones a committed write can come +back with, and the server shows them as 503 `sent: "unknown"`. + +I also checked `/api/config`, the route order and the log line. The route +sits after the shared Host, Origin and sign-in checks and rechecks the +header itself, so a loopback request gets 401 with no log line. `to` is +JSON-quoted and cut to 64 characters before it's logged. + +Row 59 has landed (`a8512007`). Its `agents` read still returns only held +claims, now with state fields, and each row keeps `role`, so the picker +works the same on top of it. + +## Tests and mutants + +On the candidate, 01:16:27Z to 01:16:44Z on 2026-10-12 +(`r1/tests-light.txt`): + +| File | Result | +|---|---| +| `send.test.mjs` | 13 pass, 0 fail | +| `send-browser.test.mjs` | 1 pass, 0 fail | +| `shell.test.mjs` | 5 pass, 0 fail | +| `preview.test.mjs` | 11 pass, 0 fail | +| `bus.test.mjs` | 9 pass, 0 fail | +| `serve.test.mjs` | 4 pass, 0 fail | + +I didn't run the full gate. Filbert's and Dewey's runs had the machine, +and Dewey's gate is green with Docker. + +Dewey's 21 mutants cover the request checks. I aimed mine at the sites +they don't name (`r1/mutants.txt`, 01:17:37Z to 01:18:51Z; `r1/m2.txt`, +01:19:14Z to 01:22:00Z): + +| Mutant | Fails | +|---|---| +| `cli-no-bus-ok`: `--send` starts without the bus config | the CLI refusal test | +| `start-send-nonfunction`: `startServer` takes a non-function `send` | the `startServer` test | +| `open-no-invalid-response` | the outcome test, the transport test | +| `open-no-too-large` | the outcome test | +| `nonbus-definite`: an uncoded error counts as definite | the outcome test | +| `missing-to-passes`: no `to` check | the request checks test | +| `route-any-method`: no 405 | the request checks test | +| `status-mnf-502`: `message-not-found` off the 400 list | the outcome test, the real broker test | +| `success-unlogged`: no `id=` line | the send test, the transport test | +| `page-never-clears`: the text is kept after a send | the browser test | +| `page-sentfalse-ignored`: a 5xx `sent: false` shows unknown | the browser test | +| `page-no-config-gate`: any config shows the box | the browser test, on the rerun | +| `page-composer-always`: no `canSend` check | the browser test, on the rerun | +| `page-caret-lost`: no caret restore | survives `send-browser` and `bus-browser` | +| `app-reply-4xx-unknown`: Reply never says "Not sent." | survives 6 files | +| `app-reply-5xx-refused`: Reply says "Not sent." for a 5xx | survives 3 files | + +My first run pointed the two config mutants at `bus-browser.test.mjs`, +which never turns `send` on. That was my mistake: `send-browser` kills +both. The last three are notes 1 and 2. + +## Notes + +**1. The Reply change in `app.js` has no test.** Line 378 now says "Not +sent." for a 4xx. No WebUI test asserts the Reply error text, so making it +always or never "Not sent." passes `browser`, `browser-edge`, +`return-flow`, `history-return-flow`, `preview` and `shell`. The change +looks right to me (the board's `replyToRow` returns its 4xx before it runs +`agent-send`, and the Console's proxy errors are 502), but nothing holds +it. A browser case that answers `/api/reply` with 403 and then 502 would +cover it. + +**2. The caret restore isn't tested.** `build.md` says focus and the caret +survive the refresh. Focus is checked; the caret line (`bus.js:594`) can +go and `send-browser` still passes. `browser.test.mjs:55` does this for +the board's Reply box and could be copied. + +**3. "Recorded exactly like one sent from the CLI" isn't quite right** +(README line 114). `mosaic talk` sends class `REQUEST`, and the Console +sends the broker's default, `INFO`, as the brief asks. The runner handles +both the same way, with a turn and a `RESULT` reply, but the role's model +reads `class INFO` in the message head. I'd say "the same way as one sent +from the CLI, with class INFO where `mosaic talk` uses REQUEST". Sage may +want `REQUEST` here later; that's a brief change, not this row. + +**4. The talk sentence** (README line 185). The broker records each +message delivered when `message.receive` returns it; talk then marks it +read. "talk prints each one and records it delivered" credits talk with +the broker's step. The effect it states is right. + +**5. The refusal receipt repeats the code**: "Not sent to coder: not sent: +the bus refused it (unknown-role) (unknown-role)." The server's message +already names the code, and the page adds it again. Cosmetic. + +Notes 1 to 5 can go into the row 59 re-round or a follow-up, as Sage +prefers. + +## Questions + +**1. Is it safe to show a proxy 4xx as "not sent"?** Yes, under the +README's proxy contract, with one line added to it. Every 4xx the Console +returns on this route comes before `send` is called. An edge proxy's own +4xx (sign-in, 413, 400, 431, 429) comes before it forwards. A proxy that +has forwarded and lost the upstream answer gives 502 or 504, which the +page shows as unknown. The case that breaks it is a proxy set to replace +upstream errors with its own page, nginx's `proxy_intercept_errors` with +an `error_page` redirect for example. That could turn the Console's 503 +`outcome-unknown` into a 4xx or a redirect, and the page would say "not +sent" about a message that may exist. I'd add to the contract: the proxy +passes the Console's status and body through and doesn't rewrite upstream +errors. + +**2. Should `--send` refuse to start without the bus config?** Yes. The +operator asked for sending. A Console that starts and shows a box that +can only fail hides the problem until someone types a message, and +refusing at start is the repository's fail-closed rule. A read-only +Console still starts without the config, as before. + +## Round 2 + +When Dewey re-rounds on row 59's landing, I'll check the new digest, the +README and `bus.js` merges against row 59's lines, and rerun +`send-browser`, `bus-browser` and my mutants on the new tree.