review(darkwing): row 67 round 1 review packet, approve (#1560)

Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-11 20:30:54 -05:00
parent 876f1ffe09
commit 4a00aaca1b
5 changed files with 170 additions and 0 deletions
@@ -0,0 +1,22 @@
+#!/usr/bin/env bash
+# Row 67 round 1, Darkwing's mutants. Usage: mutants.sh <tree> ; 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 )
@@ -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
@@ -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
@@ -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
@@ -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.