stack/agents/darkwing/work/queue-56-review/review-r2.md

Row 56, runs and releases reader, round 2 review (Darkwing)

Issue #1545, request comment 27120, queue revs 350-354. Base 75ab1646. Candidate manifest sha256 2727198f7e50b935205546a60ea97d8d4f20d87cad2a88e10477148c8d3dc42e, build.patch sha256 d2fa5fb7…. Both match comment 27120, and the packet manifest checks clean. This is the delta review Sage asked for, against round 1 (ed3c5392, my approval in comment 27115).

Verdict: approve. src/ and scripts/mosaic-task.mjs are the round 1 bytes apart from one comment in state.mjs. D01 (Rocko's M31), D17, D20 and D22 now die on assertions in the new tests. Every suite is green. The README fixes for my notes 4b and 4c are right. The fix for 4a is half right: it covers a state/ link to nothing, and the sentence above it still says a state/ link out refuses, which isn't what happens when the link points at a directory that exists. That is wording, nothing outside is read, and I don't think it needs a round 3.

Method

• Detached worktrees at 75ab1646: base, cand and mutwt, the last two with git apply --index build.patch, then sha256sum -c on the manifest, 15 OK each. mutwt still checks 15 OK after the mutant runs (r2/mut/manifest-after.txt).
• A fourth worktree at dc96f87b with round 1's build.patch applied (15 OK against ed3c5392). diff -r of packages/runs/src and cmp of scripts/mosaic-task.mjs against the round 2 candidate (r2/out/src-diff.txt, r2/out/src-against-r1.txt).
• diff -u of the three test files, the README and delta-check.sh against round 1. The result matches out/round2-delta.diff.
• r2/gate.sh: the packages/runs suite, then every scripts/test-*.sh with DOCKER_HOST=unix:///nonexistent.sock, 17:12:07Z to 17:14:22Z.
• Round 1's 21 mutants, unchanged (r2/mut/mutate.py, r2/mut/run.sh), 17:12:08Z to 17:12:34Z in mutwt, alongside the gate.
• Rocko's byte-check.sh and delta-check.sh from the candidate, base against candidate, work directories in my scratch.
• Four probes of the README's new link wording (r2/probes.sh, output in r2/out/probes.txt).
• test-release with Docker, 17:14:45Z to 17:15:08Z.

Node v26.8.1, TMPDIR=~/darkwing-scratch/r56b/tmp.

src/ against round 1

Five of the six files are byte-identical. state.mjs differs only in the comment above readActivationLog:

// The activation log as { entries, malformed }, oldest first. A line that
// isn't JSON, or is JSON but not the entry shape release.sh writes, is
// counted in malformed and skipped. This is stricter than release.sh
// rollback, which skips only lines that aren't JSON. A missing log is empty.
// With last, only the newest last well-formed entries are returned.

That matches release.sh lines 91-99: rollback drops empty lines, skips lines JSON.parse rejects, and uses the newest activate or rollback entry whose imageTag is truthy and differs from the current one.

Suites

Suite	Result
packages/runs (node)	41/0
test-auth	15/0
test-conductor	17/0
test-config	24/0
test-discord	65/1 in the gate, 66/0 alone (below)
test-extension-package	18/0
test-foundation	44/0
test-queue	27/0
test-release	4/0 without Docker, 14/0 with it
test-task, Docker unreachable	26/0
byte-check.sh	16 cases identical, data root unchanged
delta-check.sh	13 cases, same as Rocko's apart from scratch paths

The test-discord failure in the gate was "engine: when pi has not started a timed-out turn by the end of the abort grace, the engine stops pi and fails held prompts", a timing test that ran while the mutants and checks were loading the machine. The candidate changes nothing under packages/discord. A rerun on its own at 17:14:31Z gave 66/0 (r2/out/test-discord-r2.txt).

I didn't run test-task with real Docker, because it makes live model calls. Round 2 doesn't touch mosaic-task.mjs, and Rocko's run gave 98/0.

Mutants

20 of 21 killed (r2/mut/summary.txt). Only D10 survives, which I said in round 1 needs no test.

Mutant	Round 1	Round 2, killed by
D01 (M31)	survived	"show refuses an invalid id before reading the config": exit 3, configuration problem
D17	survived	"listRunRecords lists an r- name that isn't a valid run id, as before" and the CLI list test: exit 4, invalid run id: "r-.bad"
D20	survived	the same two tests: the list comes back empty
D22	survived	"the log reads oldest first and counts malformed lines": the entry with no imageTag shows up in entries

Each is an AssertionError from the new assertions, not a load or reference error.

Wording

Note	Fixed?
4b, the "like rollback" comparison (Filbert's N1)	yes, in the README and the state.mjs comment, and both now match release.sh
4c, no delta row for an unreadable run directory	yes: a README row and a delta-check.sh case. Base prints two lines and crashes, exit 1; the candidate refuses with EACCES, exit 4
Filbert's N2, release as a string	yes: { ...good[1], release: 5 } is now in the malformed lines
4a, state/ linked out	half: see below

The README still says "runs/ or state/ resolving outside refuses with a RunsError". The new paragraph after the list says a state/ link out "to a path that doesn't exist" reads as no release and an empty log, and the new test covers that case. My round 1 probe P5 linked state/ to a directory that exists, and that case still contradicts the bullet:

Probe	Candidate
Q1 runs/ linked out to nothing	list prints nothing, exit 0; show says run not found, exit 4. Base does the same
Q2 runs/ linked out to an empty directory that exists	list refuses, exit 4 (base: nothing, exit 0)
Q3 state/ linked out to nothing	pointer null, log empty
Q4 state/ linked out to an empty directory that exists	pointer null, log empty, no refusal
Q4b the same, with active.json there	readActivePointer refuses, exit 4

The module never resolves state/ itself, only state/active.json and state/activation-log.jsonl. So a state/ link out refuses only once one of those files exists behind it. A sentence that says so would fix it, for example: "runs/ resolving outside refuses. state/ isn't resolved on its own; active.json or activation-log.jsonl resolving outside refuses, and while neither exists a state/ link out reads as no release and an empty log." The invariant holds either way, because nothing outside the data root is read.

Notes (not blocking)

1. The 4a wording above. It can ride with the padEnd follow-up or any later change to this README. I don't want a round 3 for it.
2. BUILD.md describes rollback as acting on "any parsed activate or rollback entry with a truthy imageTag". It also skips the entry whose imageTag equals the current one. The README and the comment don't make that claim, so only the packet is affected.
3. Correction to my round 1 review: it said "17 of 22 killed". mutate.py defines 21 mutants (there is no D02 or D12), and 16 of them were killed in round 1. The five survivors I listed were right.

Files

• r2/candidate-manifest.sha256: copy of Rocko's.
• r2/gate.sh, r2/out/: suite runs, summary.txt, the test-discord rerun, byte and delta checks, test-release with Docker, the src/ comparison with round 1, and the probe output.
• r2/mut/: round 1's mutant definitions and runner, diffs, outputs, summary.txt and the manifest check after the runs.
• r2/probes.sh: Q1 to Q4b.