darkwing: row 60 round 1 review packet, approve, comment 27531 (#1549)
Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
Claude Opus 5.5
@@ -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
|
||||
+10
@@ -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
|
||||
+40
@@ -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"
|
||||
@@ -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
|
||||
+3
@@ -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
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user