From 4a00aaca1bbba2b0fb214768b783f991f8abe17b Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 11 Oct 2026 20:30:54 -0500 Subject: [PATCH] review(darkwing): row 67 round 1 review packet, approve (#1560) Co-Authored-By: Claude Opus 5.5 --- .../work/queue-67-review/r1/mutants.sh | 22 +++++ .../work/queue-67-review/r1/mutants.txt | 32 +++++++ .../queue-67-review/r1/tests-head-preview.txt | 14 +++ .../work/queue-67-review/r1/tests-light.txt | 8 ++ .../work/queue-67-review/review-r1.md | 94 +++++++++++++++++++ 5 files changed, 170 insertions(+) create mode 100755 agents/darkwing/work/queue-67-review/r1/mutants.sh create mode 100644 agents/darkwing/work/queue-67-review/r1/mutants.txt create mode 100644 agents/darkwing/work/queue-67-review/r1/tests-head-preview.txt create mode 100644 agents/darkwing/work/queue-67-review/r1/tests-light.txt create mode 100644 agents/darkwing/work/queue-67-review/review-r1.md diff --git a/agents/darkwing/work/queue-67-review/r1/mutants.sh b/agents/darkwing/work/queue-67-review/r1/mutants.sh new file mode 100755 index 000000000..5e41b2497 --- /dev/null +++ b/agents/darkwing/work/queue-67-review/r1/mutants.sh @@ -0,0 +1,22 @@ +#!/usr/bin/env bash +# Row 67 round 1, Darkwing's mutants. Usage: mutants.sh ; one edit, one test file, restore. +set -u +T=$1; M=$HOME/darkwing-scratch/rv67/mut.mjs +export TMPDIR=$HOME/darkwing-scratch/rv67/tmp +B=packages/bus/src/broker.mjs; S=packages/webui/src/serve.mjs +run() { # name file from to testfile + local name=$1 file=$2 from=$3 to=$4 tf=$5 + ( cd "$T" && node "$M" "$file" "$from" "$to" >/dev/null ) || { echo "$name: did not apply"; return; } + local out; out=$(cd "$T" && node --test "$tf" 2>&1) + local p f; p=$(grep -E '^ℹ pass' <<<"$out" | awk '{print $3}'); f=$(grep -E '^ℹ fail' <<<"$out" | awk '{print $3}') + echo "$name: pass $p fail $f"; grep -E '^✖ ' <<<"$out" | grep -v 'failing tests' | sed 's/ ([0-9.]*ms)//' | sort -u | sed 's/^/ /' + ( cd "$T" && git checkout -q -- "$file" ) +} +date -u +%FT%TZ +run view-business-null $B " blocking: !!d.blocking," " blocking: !!d.blocking, business: null," packages/bus/tests/decisions.test.mjs +run closed-drop-by $B 'SELECT * FROM decision_events WHERE decision=? AND op IN ${CLOSED} ORDER BY seq DESC LIMIT 1' 'SELECT seq, decision, op FROM decision_events WHERE decision=? AND op IN ${CLOSED} ORDER BY seq DESC LIMIT 1' packages/bus/tests/decisions.test.mjs +run recent-order-raise $B 'ORDER BY x.seq DESC LIMIT ${RECENT}' 'ORDER BY d.seq DESC LIMIT ${RECENT}' packages/bus/tests/decisions.test.mjs +run auth-target-task-ref $B 'target: context.target,' 'target: d.task_ref ?? context.target,' packages/bus/tests/decisions.test.mjs +run web-list-recent-only $S "const DECISION_LISTS = ['roles', 'recent'];" "const DECISION_LISTS = ['recent'];" packages/webui/tests/bus.test.mjs +date -u +%FT%TZ +( cd "$T" && git status --short packages/bus/src packages/webui/src ) diff --git a/agents/darkwing/work/queue-67-review/r1/mutants.txt b/agents/darkwing/work/queue-67-review/r1/mutants.txt new file mode 100644 index 000000000..9bef70119 --- /dev/null +++ b/agents/darkwing/work/queue-67-review/r1/mutants.txt @@ -0,0 +1,32 @@ +## candidate ~/darkwing-scratch/rv67/wt +2026-10-12T01:28:00Z +view-business-null: pass 5 fail 2 + ✖ decisions is a read: one business, strict arguments, nothing written + ✖ every row is what trail already gives the same session for that id +closed-drop-by: pass 4 fail 3 + ✖ every row is what trail already gives the same session for that id + ✖ recent gives the closing event as closed when the decision was seen first + ✖ recent lists closed decisions of every route, newest close first, with how each closed +recent-order-raise: pass 5 fail 2 + ✖ recent gives the closing event as closed when the decision was seen first + ✖ recent lists closed decisions of every route, newest close first, with how each closed +auth-target-task-ref: pass 6 fail 1 + ✖ every row is what trail already gives the same session for that id +web-list-recent-only: pass 6 fail 2 + ✖ bus refusals map to statuses and never carry a path or a stack + ✖ decisions route serves the roles and recent lists from the reader view +2026-10-12T01:28:11Z +M packages/bus/src/broker.mjs +## base 970ef729 +2026-10-12T01:28:11Z +view-business-null: pass 5 fail 1 + ✖ decisions is a read: one business, strict arguments, nothing written +closed-drop-by: pass 4 fail 2 + ✖ every row is what trail already gives the same session for that id + ✖ recent lists closed decisions of every route, newest close first, with how each closed +recent-order-raise: pass 5 fail 1 + ✖ recent lists closed decisions of every route, newest close first, with how each closed +auth-target-task-ref: pass 6 fail 0 +web-list-recent-only: pass 7 fail 1 + ✖ decisions route serves the roles and recent lists from the reader view +2026-10-12T01:28:39Z diff --git a/agents/darkwing/work/queue-67-review/r1/tests-head-preview.txt b/agents/darkwing/work/queue-67-review/r1/tests-head-preview.txt new file mode 100644 index 000000000..7155daac2 --- /dev/null +++ b/agents/darkwing/work/queue-67-review/r1/tests-head-preview.txt @@ -0,0 +1,14 @@ +2026-10-12T01:28:46Z +== packages/bus/tests/decisions.test.mjs +ℹ pass 7 +ℹ fail 0 +== packages/bus/tests/agents.test.mjs +ℹ pass 6 +ℹ fail 0 +== packages/bus/tests/credential-status.test.mjs +ℹ pass 4 +ℹ fail 0 +== packages/webui/tests/bus.test.mjs +ℹ pass 9 +ℹ fail 0 +2026-10-12T01:28:52Z diff --git a/agents/darkwing/work/queue-67-review/r1/tests-light.txt b/agents/darkwing/work/queue-67-review/r1/tests-light.txt new file mode 100644 index 000000000..e82aaf078 --- /dev/null +++ b/agents/darkwing/work/queue-67-review/r1/tests-light.txt @@ -0,0 +1,8 @@ +2026-10-12T01:27:11Z +== packages/bus/tests/decisions.test.mjs +ℹ pass 7 +ℹ fail 0 +== packages/webui/tests/bus.test.mjs +ℹ pass 8 +ℹ fail 0 +2026-10-12T01:27:28Z diff --git a/agents/darkwing/work/queue-67-review/review-r1.md b/agents/darkwing/work/queue-67-review/review-r1.md new file mode 100644 index 000000000..384359102 --- /dev/null +++ b/agents/darkwing/work/queue-67-review/review-r1.md @@ -0,0 +1,94 @@ +# Row 67, decisions read follow-up, round 1 review (Darkwing) + +Issue #1560, request comment 27385, packet `agents/filbert/work/queue-67/` +(commit `69761f73`). Brief: `docs/plans/2026-10-11_decisions-read-follow-up.md` +(blob 7674ff40). Base `970ef729`. Candidate manifest sha256 +`8959e615d63ce59b0a2eb02dd6549dcca4f77937c8e9d1439a04e9bbb9aa7369`, 4 +files, matching the request. Dewey approved in comment 27452. + +Verdict: **approve**. The row ships the brief's four items and nothing +else, and the only non-test edits are the broker comment and the README +sentence. Both now describe what `#decisionView` and `#closed` actually do. +Nothing blocks. The base has drifted since the request (below), so the row +needs a rebase round before it lands. + +## Method + +- A detached worktree at `970ef729` with `git apply --index build.patch`, + then `sha256sum -c` on the manifest: 4 OK, and again after the mutants. +- I read the patch, `#decisionView` (`broker.mjs` 284), `#closed` (300), + the `decisions` case and the `inbox` branch (827) that shares the view, + and the `busRead` checks in `serve.mjs` (lines 50 to 62). +- Two test files, then five mutants of my own on the candidate and on a + clean `970ef729` worktree (`r1/mutants.sh`). Each edits one site, runs + one test file and restores the file from git. +- A preview of the rebase: `build.patch` applied with `--3way` to a + worktree of `refactor` HEAD `a949b3b9`, with four test files run there. + +Node v26.8.1, `TMPDIR=~/darkwing-scratch/rv67/tmp`. + +## The comment and the README + +Each row is `#decisionView(d)`: the `decisions` row, `options` parsed, +`blocking` made a boolean, and `authorization` built from the first +`action.allowed` event whose body names `decision.raise` and this id. A +`recent` row adds `#closed(d.id)`, the newest `decision_events` row with a +closing op. `inbox` and the `decision.*` verbs return the same view, so +"inbox's view of the decision" is accurate. The new comment and README +sentence say this, and the whole-row compare in test 4 is what shows that +trail's row carries `options` already parsed. + +Dewey's point about the closing event holds: `decision.seen`, `resolve` and +`withdraw` refuse with `decision-closed` once a closing event exists +(`broker.mjs` 657), so a "last event of any op" join would be equivalent. + +## Tests and mutants + +On the candidate, 01:27:11Z to 01:27:28Z on 2026-10-12 +(`r1/tests-light.txt`): `packages/bus/tests/decisions.test.mjs` 7 pass, 0 +fail; `packages/webui/tests/bus.test.mjs` 8 pass, 0 fail. I didn't rerun +the full gate. Filbert's and Dewey's are green, and Sage reruns it. + +Filbert's nine mutants and Dewey's eleven cover the brief's sites. I aimed +mine at ones neither names (`r1/mutants.txt`, 01:28:00Z to 01:28:39Z): + +| Mutant | Edit | Base `970ef729` | Candidate | +|---|---|---|---| +| `view-business-null` | the view sets `business` to `null` | killed: the read test | killed: the read test, test 4 | +| `closed-drop-by` | `closed` has only `seq`, `decision`, `op` | killed: test 4, recent | killed: test 4, recent, seen-then-closed | +| `recent-order-raise` | `recent` ordered by raise, not close | killed: recent | killed: recent, seen-then-closed | +| `auth-target-task-ref` | `authorization.target` prefers `task_ref` | survives | killed: test 4 | +| `web-list-recent-only` | `serve.mjs` accepts only `?list=recent` | killed: the route test | killed: the route test, refusals | + +Five of five fail on the candidate. `auth-target-task-ref` is the one +that matters: the seed's human decision has target `release-1` and task +ref `vikunja:3/3`, and only the rewritten test 4 compares `authorization` +with the raise event, so the base lets it through. The other four were +already caught on the base, and the new tests add a second catch. + +## Notes + +**1. Test 4 doesn't say its new decisions are listed.** This is Dewey's N1, +and I agree. `view-supersedes-null` is caught only because the superseding +decision routes to `cto` and lands in `roles`. If `technical` routing +changed, the row would drop out and test 4 would still pass with less +coverage than its comment claims. One assertion that the superseding id is +in `roles` and the first id in `recent` would pin it. + +**2. The base has drifted.** `build.md` and Dewey's check were right when +written: no commit since `970ef729` touched these paths. Since then rows 58 +(`aaf87213`), 60 (`1771bf98`) and 59 (`a8512007`) have landed on the bus +README, `broker.mjs` and the WebUI bus tests. On HEAD `a949b3b9`, +`build.patch` fails a plain `git apply` at README line 205. With `--3way` +all four files merge cleanly. In that preview the README sentence sits at +line 238, after row 60's table rows, and reads correctly. The decisions, +agents, credential-status and WebUI bus tests all pass there (7, 6, 4 and +9, `r1/tests-head-preview.txt`). That preview isn't the candidate. Before +landing, the row needs a round on the new base with a new manifest, and +I'll re-approve its digest. + +## Filbert's question + +**1. Keep the `trail?subject=` line.** It's the same `busRead` path, in a +file this row owns, and Dewey's `web-trail-503-first` shows nothing else +pins it.