diff --git a/agents/darkwing/work/queue-60-review/r1/gate-summary.txt b/agents/darkwing/work/queue-60-review/r1/gate-summary.txt new file mode 100644 index 000000000..f7edaba10 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r1/gate-summary.txt @@ -0,0 +1,10 @@ +test-auth rc=0 selftest: 15 passed, 0 failed +test-conductor rc=0 selftest: 17 passed, 0 failed +test-config rc=0 selftest: 24 passed, 0 failed +test-discord rc=0 discord suite: 66 passed, 0 failed +test-extension-package rc=0 extension package selftest: 18 passed, 0 failed +test-foundation rc=0 selftest: 44 passed, 0 failed +test-queue rc=0 queue suite: 27 passed, 0 failed +test-release rc=0 selftest: 14 passed, 0 failed +test-task rc=0 selftest: 26 passed, 0 failed +DONE diff --git a/agents/darkwing/work/queue-60-review/r1/gate.sh b/agents/darkwing/work/queue-60-review/r1/gate.sh new file mode 100755 index 000000000..948a117d7 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r1/gate.sh @@ -0,0 +1,10 @@ +#!/bin/bash +# Every scripts/test-*.sh in the review worktree, one at a time; bus and webui ran in tests.sh. +export TMPDIR=~/darkwing-scratch/rv60/tmp; O=~/darkwing-scratch/rv60/out/gate; mkdir -p $O; rm -f $O/summary.txt; cd ~/darkwing-scratch/rv60/wt +[ -x node_modules/.bin/pi ] || { echo "node_modules not linked" >> $O/summary.txt; exit 1; } +for s in scripts/test-*.sh; do + n=$(basename $s .sh) + if [ $n = test-task ]; then DOCKER_HOST=unix:///nonexistent bash $s > $O/$n.log 2>&1; else bash $s > $O/$n.log 2>&1; fi + echo "$n rc=$? $(grep -iE '[0-9]+ (passed|pass)' $O/$n.log | tail -1)" >> $O/summary.txt +done +echo DONE >> $O/summary.txt diff --git a/agents/darkwing/work/queue-60-review/r1/mutants.sh b/agents/darkwing/work/queue-60-review/r1/mutants.sh new file mode 100755 index 000000000..1a3d08f16 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r1/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/rv60/tmp +cd ~/darkwing-scratch/rv60/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'), +" '' "$ALL" +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/r1/mutants.txt b/agents/darkwing/work/queue-60-review/r1/mutants.txt new file mode 100644 index 000000000..c13a7caa9 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r1/mutants.txt @@ -0,0 +1,59 @@ +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=0 ℹ pass 84 ℹ fail 0 +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 +views-no-verb (webui): 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: +restored diff --git a/agents/darkwing/work/queue-60-review/r1/tests.sh b/agents/darkwing/work/queue-60-review/r1/tests.sh new file mode 100755 index 000000000..7abe56133 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/r1/tests.sh @@ -0,0 +1,3 @@ +#!/bin/bash +export TMPDIR=~/darkwing-scratch/rv60/tmp; cd ~/darkwing-scratch/rv60/wt; O=~/darkwing-scratch/rv60/out +for s in bus webui; do env -u NODE_TEST_CONTEXT node --test "packages/$s/tests/*.test.mjs" > $O/$s-tests.log 2>&1; echo "$s rc=$? $(grep -E '^ℹ (pass|fail) ' $O/$s-tests.log | tr '\n' ' ')"; done diff --git a/agents/darkwing/work/queue-60-review/review-r1.md b/agents/darkwing/work/queue-60-review/review-r1.md new file mode 100644 index 000000000..59da0cb26 --- /dev/null +++ b/agents/darkwing/work/queue-60-review/review-r1.md @@ -0,0 +1,133 @@ +# Row 60, bus read: credential status on the broker, round 1 review (Darkwing) + +Issue #1549, request comment 27383, packet `agents/filbert/work/queue-60/`. +Brief: `docs/plans/2026-10-10_design-implementation.md`, "Bus read: +credential status on the broker". Base `4100fa68`. Candidate manifest sha256 +`99d67e44e226fdf15aeb850bfa2f36a61f3b7f940fad36249ff4bd101f24eedc`, 9 +files, matching the request. + +Verdict: **approve**. The read gives human and reader sessions service, +account, role, date kind, date and state for their own business, and nothing +that names a token, file or variable. A role session gets `human-required`, +and any argument gets `invalid-request`. The broker gets a status function, +never the `Credentials` object, so it has no path to `use()`. The read asks +the status on every request, so a date that crosses a boundary while the host +runs shows up without a restart. Nothing blocks. Two notes and the answers to +Filbert's two questions are below. + +## Method + +- I read the whole patch: the `credentials` case and the constructor option + in `broker.mjs`, the runtime wiring, `views.mjs`, the WebUI route, both + READMEs and the tests. +- A detached worktree at `4100fa68` with `build.patch` applied, then + `sha256sum -c` on the manifest: 9 OK. +- I checked the account rule against the business file loader + (`packages/business/src/business.mjs` lines 91 to 101): a role whose + definition uses Vikunja must have `tracker.bot`, and a role without + Vikunja can't have `tracker`. +- 16 mutants of my own (`r1/mutants.sh`, using + `agents/darkwing/work/queue-59-review/r1/mut.mjs`), each run against one + test file and restored from the worktree index. I aimed some at sites + Filbert's 16 name, to check the tests kill them for the right reason, and + the rest at sites they don't. + +Node v26.8.1, `TMPDIR=~/darkwing-scratch/rv60/tmp`. + +## Suites + +On the candidate, one at a time (`r1/tests.sh`, `r1/gate.sh`), from +23:48Z to 23:53:59Z on 2026-10-11: + +| Suite | Result | +|---|---| +| packages/bus | 84 pass, 0 fail | +| packages/webui | 41 pass, 0 fail | +| test-auth | 15 passed | +| test-conductor | 17 passed | +| test-config | 24 passed | +| test-discord | 66 passed | +| test-extension-package | 18 passed | +| test-foundation | 44 passed | +| test-queue | 27 passed | +| test-release | 14 passed | +| test-task | 26 passed | + +`test-task` ran with `DOCKER_HOST` set to a missing socket and skipped its +Docker cases. I didn't run `packages/control-board` or `packages/cli`; +Filbert's run has them green, and Sage's gate rerun covers landing. +`test-discord` passed first time for me, so I didn't see Filbert's timing +failure. + +## Mutants + +13 of 16 fail on the first run (`r1/mutants.txt`). Each kill failed the test +meant for it. + +| Mutant | Fails | +|---|---| +| `reader-refused`: `credentials` left off the reader list | all 4 credential tests | +| `role-allowed`: no human-or-reader check | the role and argument test, the process test | +| `args-accepted`: `keys(a, [], [])` removed | the role and argument test | +| `prefix-no-slash` | the shape test (`demo` sees `demox`), the process test | +| `no-filter` | the shape test, the process test | +| `sync-label-raw`: role `@sync` | the shape test, the process test | +| `sync-no-bot`: the sync account `null` | the shape test, the process test | +| `gitea-gets-bot`: Gitea takes `tracker.bot` | the shape test, the process test | +| `datekind-fixed`: always `expires` | the shape test, the process test | +| `leak-instance`: `instance` kept in the row | the shape test, the process test | +| `status-cached`: status read once at construction | the per-request test | +| `runtime-no-status`: the runtime passes no status | the process test | +| `serve-no-verb`: `credentials` off `BUS_VERBS` | 2 WebUI bus tests | +| `views-no-verb`: `credentials()` removed from `views` | 2 WebUI bus tests, on a rerun | +| `account-undefined`: `bot` for `bot ?? null` | survives, equivalent | +| `no-result-check`: no `#secretCheck(result)` | survives, base code | + +`views-no-verb` survived the first run because I pointed it at the bus suite. +The WebUI is the only caller of `views().credentials`, and the rerun against +`packages/webui/tests/bus.test.mjs` kills it. That was my mistake in the +script, not a gap in the tests. + +`account-undefined` is equivalent under a loaded business file. The only +row where `bot` could be `undefined` is a Vikunja credential on a role with +no `tracker.bot`, and the loader refuses that file. The broker tests build +their businesses by hand, so they'd accept one, but the runtime gets its +businesses from the loader. + +`no-result-check` removes a line the base already has for every read, so it +isn't this row's code. No credential row in the fixtures holds a known +token, so nothing reaches it. The process test covers the same leak from the +outside: it seeds a token in a file and in the environment and greps the +output, the log and every `bus.sqlite*` file. + +## Notes + +**1. The re-round meets two rows on four of these paths.** Row 74 +(`351464ce`) changed `packages/webui/README.md` and +`packages/webui/src/serve.mjs` after `4100fa68`. Row 59 edits +`broker.mjs` and the bus README, and Filbert's overlap note already shows +`git apply --3way` merging it cleanly. Row 58 touches the webui README too. +When this row re-rounds after 58, the `serve.mjs` merge is the one to read by +hand: row 74 put its preview checks ahead of routing in the same request +handler that reads `BUS_VERBS`. Nothing in this review depends on those +lines, so my findings carry over, and on the re-round I'll check the digest, +the merged handler and the WebUI bus and preview tests. + +**2. `test-release` is 14, not 4.** The gate table in `build.md` says +"release 4". My run has 14 passed, the same as the other runs today. A typo +in the packet; it doesn't touch the candidate. + +## Questions + +1. **The Business view.** Leave it alone in this row. The brief asks for the + endpoint, and the Business view working without a bus host is worth + keeping. The reason this read exists is the case the view can't show: a + token file rotated after startup, which the host doesn't reread until + restart. That only helps Jason if a page puts the two side by side. A + later row, after the cutover, could add a "host loaded" column to the + Business view when the bus answers and flag a row whose date or state + differs from the file's, with the file metadata alone when it doesn't. +2. **The missing `decisions` row.** Yes, add it. Since this row re-rounds + anyway and already edits that table, I'd put the one line in the + re-round rather than open a docs row for it. If Sage would rather keep + the re-round to the digest, a docs follow-up is fine.