From 69761f73a25d3cdb746371208996e95942bfd3b1 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 11 Oct 2026 17:11:46 -0500 Subject: [PATCH] review(filbert): row 67 round 1 packet, decisions read follow-up tests (#1560) Manifest pins 4 files on base 970ef729; candidate source is not committed. Co-Authored-By: Claude Opus 5.5 --- agents/filbert/work/queue-67/build.md | 119 +++++++++++++ agents/filbert/work/queue-67/build.patch | 160 ++++++++++++++++++ .../work/queue-67/candidate-manifest.sha256 | 4 + 3 files changed, 283 insertions(+) create mode 100644 agents/filbert/work/queue-67/build.md create mode 100644 agents/filbert/work/queue-67/build.patch create mode 100644 agents/filbert/work/queue-67/candidate-manifest.sha256 diff --git a/agents/filbert/work/queue-67/build.md b/agents/filbert/work/queue-67/build.md new file mode 100644 index 000000000..11ea446a2 --- /dev/null +++ b/agents/filbert/work/queue-67/build.md @@ -0,0 +1,119 @@ +# Row 67 (#1560): decisions read follow-up, round 1 + +Filbert, 2026-10-11. Brief: `docs/plans/2026-10-11_decisions-read-follow-up.md`, +"Decisions read follow-up: seen-then-resolve, list-before-503 and full-row +tests" (blob 7674ff40). Darkwing and Dewey review. Sage commits after the +gate; nothing is committed, staged or pushed yet. + +Base 970ef729. `build.patch` applies there with `git apply --index`; +`candidate-manifest.sha256` pins all 4 files after the patch. + +No behaviour changes. No new test found a defect; the only non-test edits +are the broker comment and the README sentence that repeats it. + +## What ships + +- **Seen, then closed** (`decisions.test.mjs`, new test after `recent`). The + human marks the human-routed decision seen and resolves it; `cto` does the + same for its decision; `pm` marks its decision seen and the raiser + withdraws it. For each, the store holds exactly `seen` then the closing + op, and the `recent` row's `closed` has that op, the closing event's `seq` + and its `by`. The test also pins the `recent` order by close. Mutant + `recent-closed-first-event` fails it. +- **List before 503** (`webui/tests/bus.test.mjs`, the no-bus server in + "bus refusals map to statuses…"). With no bus configured, + `/api/bus/decisions?list=bad` answers 400 `invalid-request`, and + `?list=roles` answers 503 `not-configured`, so a good list still reaches + the bus check. Trail checks `subject` the same way, so the same loop pins + `/api/bus/trail?subject=%3C` at 400. Mutant `web-503-first` fails it. +- **The whole row** (test 4, "every row is what trail already gives…"). For + every `roles` and `recent` row, for human and reader sessions, the row + minus `authorization` and `closed` deep-equals trail's `decisions` row for + that id minus `table`, with `blocking` as a boolean. `authorization` + deep-equals `{action, target, approvalChoice}` from trail's one + `action.allowed` `decision.raise` event for the id. `closed` is present on + `recent` rows only, and equals trail's `decision_events` row as before. + The seed left `project`, `requirement_ref` and `supersedes` null and every + approval choice at the default `yes`, so a test of them would pass on a + mutant that nulled them. Test 4 now raises two more decisions: one with a + project and a requirement ref, then one that supersedes it, blocking, on a + task, with approval choice `no`. The first closes as `withdrawn` by the + supersede, so it lands in `recent`; the second is in `roles`. +- **The comment** above the `decisions` case in `broker.mjs` now says which + part each row equals: inbox's view of the decision, which is trail's + `decisions` row with `blocking` as a boolean plus `authorization` from the + raise's `action.allowed` event (trail gives that too); a `recent` row adds + `closed`, the closing `decision_events` row. +- **README** (`packages/bus/README.md`): the sentence "Every row is the + decision as `trail` gives…" said the same imprecise thing, so it now says + what the comment says. The brief allows this edit only if the comment fix + changes what the README says; it does. + +Out of scope, unchanged: no `LIMIT` on `roles`; `roles` order stays `seq`. + +## Files + +- `packages/bus/src/broker.mjs`: comment only. +- `packages/bus/README.md`: one sentence. +- `packages/bus/tests/decisions.test.mjs`: one new test; test 4 rewritten. +- `packages/webui/tests/bus.test.mjs`: three requests in the no-bus case. + +## Mutants + +`mut/run.mjs` (not shipped): one edit per mutant in the worktree, then the +owning suite (`packages/bus/tests/*.test.mjs`, or +`packages/webui/tests/bus.test.mjs` for `web-503-first`), then a restore. +`git diff` was unchanged after each run. + +| Mutant | Edit | Base 970ef729 | Candidate | +|---|---|---|---| +| `recent-closed-first-event` | `closed` is the first event for the id | survives | killed: seen-then-closed | +| `recent-join-any-event` | `recent` joins any event, not only closing ops | survives | killed: seen-then-closed | +| `web-503-first` | `busRead` answers 503 before checking the request | survives | killed: refusals test | +| `view-requirement-null` | row drops `requirement_ref` | survives | killed: test 4 | +| `view-supersedes-null` | row drops `supersedes` | survives | killed: test 4 | +| `view-project-null` | row drops `project` | survives | killed: test 4 | +| `view-task-ref-null` | row drops `task_ref` | survives | killed: test 4 | +| `view-approval-fixed` | `approvalChoice` always `yes` | survives | killed: test 4 | +| `view-blocking-raw` | `blocking` left as 0/1 | killed: tests 1, 2 | killed: tests 1, 2, 4 | + +Candidate: 9 of 9 killed, each by the test this row adds or rewrites. +Base: 8 of 9 survive. Each mutant fails only the tests named above; no +other test moved. + +## Gate + +`gate.sh` (not shipped) on the candidate worktree, sequentially, with +`TMPDIR` in scratch and `DOCKER_HOST` pointed at a missing socket: + +| Suite | Result | +|---|---| +| `node --test packages/bus/tests/*.test.mjs` | 81 pass, 0 fail | +| `node --test packages/control-board/tests/*.test.mjs` | 124 pass, 0 fail | +| `node --test packages/webui/tests/*.test.mjs` | 40 pass, 0 fail | +| `node --test packages/cli/tests/*.test.mjs` | 112 pass, 0 fail | +| `scripts/test-auth.sh` | 15 passed, 0 failed | +| `scripts/test-conductor.sh` | 17 passed, 0 failed | +| `scripts/test-config.sh` | 24 passed, 0 failed | +| `scripts/test-discord.sh` | 66 passed, 0 failed | +| `scripts/test-extension-package.sh` | 18 passed, 0 failed | +| `scripts/test-foundation.sh` | 44 passed, 0 failed | +| `scripts/test-queue.sh` | 27 passed, 0 failed | +| `scripts/test-release.sh` | 4 passed, 0 failed | +| `scripts/test-task.sh` | 26 passed, 0 failed | + +Every suite exited 0. `sha256sum -c candidate-manifest.sha256` passes in the +worktree, and `git diff 970ef729` there hashes the same as `build.patch`. +No commit since 970ef729 touches `packages/`. + +Rows 59 and 60, both in review, also edit `packages/bus`. On a fresh +970ef729 worktree, `build.patch` applies after row 59's patch with plain +`git apply`, and after row 60's (alone, or after 59 then 60) with `--3way` +and no conflicts. Whichever lands first, this one re-bases cleanly; the +gate above is for this patch alone. + +## Questions for reviewers + +1. The webui check also pins `trail?subject=` before the 503. The brief + names only `list`; the order is the same code path one branch up, and + the line is in a file this row owns. Keep it or drop it? diff --git a/agents/filbert/work/queue-67/build.patch b/agents/filbert/work/queue-67/build.patch new file mode 100644 index 000000000..33e397ec9 --- /dev/null +++ b/agents/filbert/work/queue-67/build.patch @@ -0,0 +1,160 @@ +diff --git a/packages/bus/README.md b/packages/bus/README.md +index 5b57b7fc..fcd0f69e 100644 +--- a/packages/bus/README.md ++++ b/packages/bus/README.md +@@ -205,7 +205,9 @@ decisions. It takes `{list}`, nothing else. `roles` lists the business's open + decisions routed to a role, never to the human, oldest first. `recent` lists + its last `RECENT` (50) closed decisions of every route, newest close first, + each with `closed`, the closing `decision_events` row. Every row is the +-decision as `trail` gives the same session for its id, so the lists add no ++`inbox` view of the decision: the `decisions` row `trail` gives the same ++session for its id, with `blocking` as a boolean and `authorization` from the ++raise's `action.allowed` event, which `trail` also gives. So the lists add no + data a reader couldn't already read one id at a time; they add the listing. + + ## Shared reads and adapter boundary +diff --git a/packages/bus/src/broker.mjs b/packages/bus/src/broker.mjs +index 84db8799..07f35114 100644 +--- a/packages/bus/src/broker.mjs ++++ b/packages/bus/src/broker.mjs +@@ -837,7 +837,10 @@ export class Broker { + return this.#trail(s, identifier(a.subject)); + } + // Row 57 (#1546): the Inbox's "Routed to roles" and "Recent" lists, for human and +- // reader sessions only. Each row is what trail gives the same session for that id. ++ // reader sessions only. Each row is inbox's view of the decision: the `decisions` row trail ++ // gives the same session for that id, with blocking as a boolean, plus `authorization` from ++ // the raise's `action.allowed` event, which trail also gives. A `recent` row adds `closed`, ++ // the decision's closing `decision_events` row, as trail gives it. + case 'decisions': { + if (!s.human && !s.reader) fail('human-required'); + keys(a, ['list'], ['list']); +diff --git a/packages/bus/tests/decisions.test.mjs b/packages/bus/tests/decisions.test.mjs +index 6626460b..069e2ef4 100644 +--- a/packages/bus/tests/decisions.test.mjs ++++ b/packages/bus/tests/decisions.test.mjs +@@ -109,6 +109,39 @@ test('recent lists closed decisions of every route, newest close first, with how + } + }); + ++// Row 67 (#1560): seen, then closed, the usual order. `closed` is the closing event, not the first. ++test('recent gives the closing event as closed when the decision was seen first', (t) => { ++ const { b, store, human, reader, coder, cto, pm, toCto, toPm, toHuman, within } = seed(t); ++ call(b, human, 'decision.seen', { id: toHuman.id }); ++ call(b, human, 'decision.resolve', { id: toHuman.id, choice: 'yes' }); ++ call(b, cto, 'decision.seen', { id: toCto.id }); ++ call(b, cto, 'decision.resolve', { id: toCto.id, choice: 'no' }); ++ call(b, pm, 'decision.seen', { id: toPm.id }); ++ call(b, coder, 'decision.withdraw', { id: toPm.id }); ++ const events = (id) => store.all('SELECT seq, op FROM decision_events WHERE decision=? ORDER BY seq', id); ++ for (const cap of [human, reader]) { ++ const rows = list(b, cap, 'recent'); ++ assert.deepEqual( ++ rows.map((r) => r.id), ++ [toPm.id, toCto.id, toHuman.id, within.id], ++ ); ++ for (const [d, op, by] of [ ++ [toHuman, 'resolved', 'jason'], ++ [toCto, 'resolved', 'demo-cto-run'], ++ [toPm, 'withdrawn', 'demo-coder-run'], ++ ]) { ++ const ev = events(d.id); ++ assert.deepEqual( ++ ev.map((e) => e.op), ++ ['seen', op], ++ 'seen, then closed', ++ ); ++ const { closed } = rows.find((r) => r.id === d.id); ++ assert.deepEqual([closed.op, closed.seq, closed.by], [op, ev[1].seq, by]); ++ } ++ } ++}); ++ + test('recent stops at RECENT rows', (t) => { + const { b, reader, coder } = seed(t); + const ids = []; +@@ -122,22 +155,55 @@ test('recent stops at RECENT rows', (t) => { + }); + + test('every row is what trail already gives the same session for that id', (t) => { +- const { b, human, reader, toHuman } = seed(t); ++ const { b, human, reader, coder, toHuman } = seed(t); + call(b, human, 'decision.resolve', { id: toHuman.id, choice: 'yes' }); ++ // Row 67 (#1560): fill the columns the seed leaves empty: project, requirement_ref, supersedes, ++ // and an approval choice other than the default. ++ const first = raise(b, coder, 'task.scope.change', { ++ domain: 'technical', ++ target: 'vikunja:3/5', ++ project: 'demo-web', ++ requirement_ref: 'REQ-DEC-1', ++ }); ++ raise(b, coder, 'task.scope.change', { ++ domain: 'technical', ++ target: 'vikunja:3/5', ++ task_ref: 'vikunja:3/5', ++ blocking: true, ++ supersedes: first.id, ++ approvalChoice: 'no', ++ }); + for (const cap of [human, reader]) +- for (const row of [...list(b, cap, 'roles'), ...list(b, cap, 'recent')]) { +- const trail = call(b, cap, 'trail', { subject: row.id }); +- const d = trail.find((r) => r.table === 'decisions' && r.id === row.id); +- assert.ok(d, row.id); +- for (const k of ['route_to', 'class', 'action', 'question', 'recommendation', 'raised_by_role', 'raised_by_run']) +- assert.equal(d[k], row[k], k); +- assert.deepEqual(d.options, row.options); +- if (row.closed) { +- const { table, ...closed } = trail.find((r) => r.table === 'decision_events' && r.seq === row.closed.seq); +- assert.equal(table, 'decision_events'); +- assert.deepEqual(closed, { ...row.closed }); ++ for (const which of ['roles', 'recent']) ++ for (const row of list(b, cap, which)) { ++ const trail = call(b, cap, 'trail', { subject: row.id }); ++ const found = trail.find((r) => r.table === 'decisions' && r.id === row.id); ++ assert.ok(found, row.id); ++ // The whole row: the view adds authorization (and closed on recent) and makes blocking ++ // a boolean. Every other column is trail's. ++ const { table, ...d } = found; ++ const { authorization, closed, ...rest } = row; ++ assert.deepEqual(rest, { ...d, blocking: !!d.blocking }, row.id); ++ const raised = trail.filter( ++ (r) => ++ r.table === 'events' && ++ r.kind === 'action.allowed' && ++ r.body.operation === 'decision.raise' && ++ r.body.decision === row.id, ++ ); ++ assert.equal(raised.length, 1, row.id); ++ assert.deepEqual(authorization, { ++ action: d.action, ++ target: raised[0].body.target, ++ approvalChoice: raised[0].body.approvalChoice, ++ }); ++ assert.equal(closed !== undefined, which === 'recent', row.id); ++ if (closed) { ++ const { table, ...event } = trail.find((r) => r.table === 'decision_events' && r.seq === closed.seq); ++ assert.equal(table, 'decision_events'); ++ assert.deepEqual(event, { ...closed }); ++ } + } +- } + }); + + test("a role session can't list decisions, so it can't read another role's open decisions", (t) => { +diff --git a/packages/webui/tests/bus.test.mjs b/packages/webui/tests/bus.test.mjs +index 2d9ca6ba..86fae610 100644 +--- a/packages/webui/tests/bus.test.mjs ++++ b/packages/webui/tests/bus.test.mjs +@@ -163,6 +163,11 @@ test('bus refusals map to statuses and never carry a path or a stack', async t = + assert.deepEqual([shape.status, (await shape.json()).error], [502, 'invalid-response']); + const unset = await fetch(`http://127.0.0.1:${none.address().port}/api/bus/agents`); + assert.deepEqual([unset.status, (await unset.json()).error], [503, 'not-configured']); ++ // Row 67 (#1560): with no bus, a bad argument is still a 400; the request is checked first. ++ for (const [query, status, error] of [['decisions?list=bad', 400, 'invalid-request'], ['decisions?list=roles', 503, 'not-configured'], ['trail?subject=%3C', 400, 'invalid-request']]) { ++ const r = await fetch(`http://127.0.0.1:${none.address().port}/api/bus/${query}`); ++ assert.deepEqual([query, r.status, (await r.json()).error], [query, status, error]); ++ } + }); + + // A fake human CLI: it reads the request, then acts on args.mode. diff --git a/agents/filbert/work/queue-67/candidate-manifest.sha256 b/agents/filbert/work/queue-67/candidate-manifest.sha256 new file mode 100644 index 000000000..b9f0ce43f --- /dev/null +++ b/agents/filbert/work/queue-67/candidate-manifest.sha256 @@ -0,0 +1,4 @@ +1d46e3f0fd0ccdb3c5a9cb6159b0f1a7031757983297d612f78209dd20740c93 packages/bus/README.md +9fecd3d888ce86c113b6176eb86902bdd5f606965f1c0b5c90d59016f4f6cd92 packages/bus/src/broker.mjs +9477cfa7411fc46b4be4c960942b9f5c912bb050b1119a635ab161d321b77b2a packages/bus/tests/decisions.test.mjs +10bc41d8a332027aa902da836ad7644cdc09ef8be26a2b8bab106c5f365e3da1 packages/webui/tests/bus.test.mjs