From a030b895fc606189d3389997487a419cbf2d2a3b Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Sun, 11 Oct 2026 19:36:08 -0500 Subject: [PATCH] darkwing: row 60 round 2 review, approve, comment 27575 (#1549) Co-Authored-By: Claude Opus 5.5 --- .../work/queue-60-review/r2/mutants.sh | 40 +++++++++ .../work/queue-60-review/r2/mutants.txt | 57 ++++++++++++ .../work/queue-60-review/r2/tests-light.txt | 2 + .../work/queue-60-review/review-r2.md | 89 +++++++++++++++++++ 4 files changed, 188 insertions(+) create mode 100644 agents/darkwing/work/queue-60-review/r2/mutants.sh create mode 100644 agents/darkwing/work/queue-60-review/r2/mutants.txt create mode 100644 agents/darkwing/work/queue-60-review/r2/tests-light.txt create mode 100644 agents/darkwing/work/queue-60-review/review-r2.md diff --git a/agents/darkwing/work/queue-60-review/r2/mutants.sh b/agents/darkwing/work/queue-60-review/r2/mutants.sh new file mode 100644 index 000000000..374653faa --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r2/mutants.sh @@ -0,0 +1,40 @@ +#!/bin/bash +# Darkwing's own row 60 mutants. Each edits one source file in the review worktree, runs one +# test file (or the bus suite), then restores the file from the worktree index (the candidate). +export TMPDIR=~/darkwing-scratch/rv60r2/tmp +cd ~/darkwing-scratch/rv60r2/wt +M=~/darkwing-scratch/rv59/mut.mjs +B=packages/bus/src/broker.mjs; R=packages/bus/src/runtime.mjs; V=packages/bus/src/views.mjs; S=packages/webui/src/serve.mjs +BT=packages/bus/tests/credential-status.test.mjs; WE=packages/webui/tests/bus.test.mjs; ALL='packages/bus/tests/*.test.mjs' +run() { # name file from to test + 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 reader-refused $B "'trail', 'decisions', 'credentials'].includes" "'trail', 'decisions'].includes" $BT +run role-allowed $B "if (!s.human && !s.reader) fail('human-required'); + keys(a, [], []);" "keys(a, [], []);" $BT +run args-accepted $B "fail('human-required'); + keys(a, [], []);" "fail('human-required');" $BT +run prefix-no-slash $B "const prefix = s.business + '/';" "const prefix = s.business;" $BT +run no-filter $B '.filter((c) => c.instance.startsWith(prefix))' '.filter(() => true)' $BT +run sync-label-raw $B "role: sync ? 'tracker sync' : role," "role," $BT +run sync-no-bot $B 'sync ? b.tracker?.sync?.bot :' 'sync ? null :' $BT +run gitea-gets-bot $B "service === 'vikunja' ? b.roles?.[role]?.tracker?.bot : null" 'b.roles?.[role]?.tracker?.bot' $BT +run datekind-fixed $B "dateKind: service === 'gitea' ? 'rotateBy' : 'expires'," "dateKind: 'expires'," $BT +run account-undefined $B 'account: bot ?? null,' 'account: bot,' $BT +run leak-instance $B " return { + service," " return { + instance, + service," $BT +run status-cached $B 'this.#credentialStatus = credentialStatus;' 'const cached = credentialStatus(); this.#credentialStatus = () => cached;' $BT +run no-result-check $B "const result = this.#dispatch(s, request.verb, args); + this.#secretCheck(result);" "const result = this.#dispatch(s, request.verb, args);" $BT +run runtime-no-status $R " credentialStatus: () => credentials.status(), +" '' $BT +run views-no-verb $V " credentials: () => client.call('credentials'), +" '' $WE +run serve-no-verb $S "'decisions', 'credentials'];" "'decisions'];" $WE +git diff --quiet -- $B $R $V $S && echo "restored: sources match the candidate index" || echo "NOT RESTORED" diff --git a/agents/darkwing/work/queue-60-review/r2/mutants.txt b/agents/darkwing/work/queue-60-review/r2/mutants.txt new file mode 100644 index 000000000..58cedf38a --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r2/mutants.txt @@ -0,0 +1,57 @@ +reader-refused: rc=1 ℹ pass 0 ℹ fail 4 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ credentials reads the status on each request, and a broker without one lists none + ✖ credentials refuses a role session and any argument + ✖ failing tests: +role-allowed: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials refuses a role session and any argument + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +args-accepted: rc=1 ℹ pass 3 ℹ fail 1 + ✖ credentials refuses a role session and any argument + ✖ failing tests: +prefix-no-slash: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +no-filter: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +sync-label-raw: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +sync-no-bot: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +gitea-gets-bot: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +datekind-fixed: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +account-undefined: rc=0 ℹ pass 4 ℹ fail 0 +leak-instance: rc=1 ℹ pass 2 ℹ fail 2 + ✖ credentials gives human and reader sessions service, account, role, date and state, and nothing else + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +status-cached: rc=1 ℹ pass 3 ℹ fail 1 + ✖ credentials reads the status on each request, and a broker without one lists none + ✖ failing tests: +no-result-check: rc=0 ℹ pass 4 ℹ fail 0 +runtime-no-status: rc=1 ℹ pass 3 ℹ fail 1 + ✖ failing tests: + ✖ the broker process answers credentials without a token, path or variable in its output, log or store +views-no-verb: rc=1 ℹ pass 7 ℹ fail 2 + ✖ busReader takes --business, else the live bus host, else refuses + ✖ credentials route serves the broker credential status from the reader view + ✖ failing tests: +serve-no-verb: rc=1 ℹ pass 7 ℹ fail 2 + ✖ bus routes are GET-only, have no write verb, and keep the same-origin checks + ✖ credentials route serves the broker credential status from the reader view + ✖ failing tests: +restored: sources match the candidate index diff --git a/agents/darkwing/work/queue-60-review/r2/tests-light.txt b/agents/darkwing/work/queue-60-review/r2/tests-light.txt new file mode 100644 index 000000000..9992e59f3 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r2/tests-light.txt @@ -0,0 +1,2 @@ +packages/bus/tests/credential-status.test.mjs rc=0 ℹ pass 4 ℹ fail 0 +packages/webui/tests/bus.test.mjs rc=0 ℹ pass 9 ℹ fail 0 diff --git a/agents/darkwing/work/queue-60-review/review-r2.md b/agents/darkwing/work/queue-60-review/review-r2.md new file mode 100644 index 000000000..c83d5eac8 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/review-r2.md @@ -0,0 +1,89 @@ +# Row 60, bus read: credential status on the broker, round 2 review (Darkwing) + +Issue #1549, request comment 27553, packet `agents/filbert/work/queue-60/` +(commit `5fc7635c`). Base `bbd22f1b`. Candidate manifest sha256 +`13541657c830ed59281f101fc3566924454fd96d1be01df844100544b10cbf93`, 9 +files, matching the request. Scope: Sage's rev 569. + +Verdict: **approve**. The rebase changes no code. The `decisions` row is +right, and Dewey's N1 sentence is right where my round 1 answer to question +1 was wrong; the correction is below. Nothing blocks. + +## Method + +- A detached worktree at `bbd22f1b` with `git apply --index build.patch`, + then `sha256sum -c` on the manifest: 9 OK. None of the 9 paths changed + from `bbd22f1b` to `HEAD` at the time of review. +- I compared the added and removed lines of round 1's `build.patch` + (`5fc7635c^`) with round 2's. They match except for the two + `packages/bus/README.md` edits: the new `decisions` table row and the + rewritten last sentences of the `credentials` paragraph. +- I read the merged files where the patch meets rows 58 and 74. +- The credential and WebUI bus test files, then my 16 round 1 mutants on the new tree + (`r2/mutants.sh`, one test file each), with `views-no-verb` pointed at the WebUI bus tests + this time. + +Node v26.8.1, `TMPDIR=~/darkwing-scratch/rv60r2/tmp`. + +## Tests and mutants + +On the candidate, 00:35:31Z to 00:35:46Z on 2026-10-12: +`packages/bus/tests/credential-status.test.mjs` 4 pass, 0 fail; +`packages/webui/tests/bus.test.mjs` 9 pass, 0 fail (`r2/tests-light.txt`). +I didn't run the full bus and WebUI suites. Another seat's mutant run held +the machine, the code is round 1's, and Sage's gate rerun is running the +full set now. + +The same 16 mutants as round 1 (`r2/mutants.txt`): 14 fail, each on the +test meant for it, and the same two survive for round 1's reasons +(`account-undefined` is equivalent under the business loader, +`no-result-check` removes a base line). `views-no-verb` fails 2 WebUI bus +tests on the first run now that the script points at them. + +## The merged hunks + +- `serve.mjs`: the patch changes only the `BUS_VERBS` line and its comment. + Row 74's checks run in the request handler at lines 107 to 135, and the + bus route lookup is at line 160, after them. So in preview mode + `/api/bus/credentials` gets the same Host, Origin, sign-in and read-only + checks as every other bus read. A GET from the operator's loopback path + reaches it as before. +- `broker.mjs`: the `credentials` case sits before `default:`. Row 58's + change is in the `tasks` branch, and the reader allowlist and the + constructor are lines row 58 doesn't touch. +- `webui/tests/bus.test.mjs` and `webui/README.md`: the round 1 additions + sit after row 58's and row 74's, sharing no lines. + +## The two README edits + +**The `decisions` row.** It matches the broker. `keys(a, ['list'], +['list'])` requires `list`. `roles` returns open decisions not routed to +`human`, and `recent` returns the last `RECENT` closed ones, which is 50. +The row says human and reader sessions only, which is what the broker +checks. + +**Dewey's N1.** The new sentences are correct. `Credentials.status()` maps +over the tokens the host loaded at startup, and the date comes from that +load; only `state` is computed at the request. The broker takes `account` +from its businesses, also loaded at startup. So the read and the Business +view differ only when the business file changes after startup. + +## Correction to my round 1 review + +In round 1 I answered question 1 by saying this read shows "a token file +rotated after startup, which the host doesn't reread until restart". That +was wrong. A token rotation without a date edit changes neither view, which +is what the README now says. The case a side-by-side view would catch is a +business file edited after startup: a new date, account or credential that +the host won't load until it restarts. My answer itself stands, to leave the +Business view alone in this row, and a later row could still compare the +two. The reason is that case, not rotation. + +I also take back round 1's note 2. `test-release` prints 4 without Docker +and 14 with it. My round 1 gate had Docker for `test-release`, and +Filbert's didn't. `build.md` now says so. + +## Dewey's N2 + +Decision 98 rules it: the read ships as built behind the one-user preview. +I have nothing to add.