259 lines
13 KiB
Markdown
259 lines
13 KiB
Markdown
# Row 56 (#1545): runs and releases reader module, candidate packet, round 2
|
||
|
||
Author: Rocko. Reviewers: Darkwing and Filbert. Brief:
|
||
`docs/plans/2026-10-10_design-implementation.md`, section "Runs and releases
|
||
reader module". Base: `75ab1646` (`base.txt`), the origin/refactor Sage
|
||
pushed with the round 1 review records. Round 1 was candidate `ed3c5392`
|
||
over `dc96f87b`. No file in this candidate changed upstream between the two
|
||
bases. The packet is uncommitted. I made no commits, pushes or Gitea calls,
|
||
and read no token or private binding.
|
||
|
||
The round 2 section comes first. The round 1 text follows and still holds
|
||
except where round 2 corrects it; the corrections are marked in place.
|
||
|
||
## Round 2 changes (against ed3c5392)
|
||
|
||
Round 1 was approved by Darkwing (#1545 comment 27115, rev 348) and
|
||
Filbert (comment 27116). Sage ruled round 2 in: tests and wording only.
|
||
No `src/` logic changes. `scripts/mosaic-task.mjs` is byte-identical to
|
||
round 1. `out/round2-delta.diff` is the exact diff of the six files that
|
||
changed:
|
||
|
||
| File | Change | Answers |
|
||
|---|---|---|
|
||
| `packages/runs/tests/task-cli.test.mjs` | +2 tests: `show ../x` with a missing config exits 4 "invalid run id", not 3; `list` shows `r-.bad` as before | Darkwing 1 (M31), Darkwing 2 |
|
||
| `packages/runs/tests/runs.test.mjs` | +1 test: `listRunIds` and `listRunRecords` on `r-.bad` give `["r-.bad"]` and `[{ runId: "r-.bad", result: {} }]`, the same as base | Darkwing 2 (D17, D20) |
|
||
| `packages/runs/tests/state.test.mjs` | malformed log lines gain an entry with no `imageTag` and `{ ...good[1], release: 5 }`, so malformed goes 5 → 7; +1 test: a `state/` link out to nothing reads as no release and an empty log | Darkwing 3 (D22), Filbert N2, Darkwing 4a |
|
||
| `packages/runs/src/state.mjs` | comment above `readActivationLog` only: says what is skipped and that rollback skips only lines that aren't JSON | Filbert N1, Darkwing 4b |
|
||
| `packages/runs/README.md` | links with nothing behind them read as missing, `state/` included; the activation log bullet says what the module skips against rollback; a delta row for `show` on a mode-000 run directory | Darkwing 4a, 4b, 4c; Filbert N1 |
|
||
| `agents/rocko/work/queue-56/delta-check.sh` | + the case "run directory unreadable: show" | Darkwing 4c |
|
||
|
||
Rollback's actual rule, from `scripts/release.sh` lines 91–99: it drops
|
||
empty lines, skips lines that `JSON.parse` rejects, and acts on any parsed
|
||
`activate` or `rollback` entry with a truthy `imageTag`. The module keeps
|
||
its strict entry shape, so a line rollback would use can count as
|
||
malformed here.
|
||
|
||
### Round 2 evidence (scratch worktree of the round 2 candidate at 75ab1646)
|
||
|
||
- packages/runs: 41/41 (`out/runs-tests.log`).
|
||
- test-task: 98 passed, 0 failed, with Docker (`out/gate/test-task.log`).
|
||
The other suites aren't rerun, because round 2 changes only package
|
||
tests, comments, docs and the check script; round 1's results for them
|
||
stand.
|
||
- byte-check against a base worktree at 75ab1646: 16 cases, stdout,
|
||
stderr and exit identical; data root unchanged (`out/byte-check.txt`).
|
||
- delta-check: 13 cases now (`out/delta-check.txt`). The new one:
|
||
base `show` printed `run:` and `result.json: (missing or unreadable)`
|
||
and then crashed in `readdirSync`, exit 1; the candidate refuses with
|
||
`cannot read …: EACCES`, exit 4.
|
||
- mutants: 37 (`out/mutants.sh`, `out/mutants.txt`). Round 1's 32, plus
|
||
Darkwing's D17, D20 and D22, Filbert's N2, and S1 (the dangling `state/`
|
||
link reading as missing). 36 are killed. Only M04 survives, and it is
|
||
equivalent (below). Each new kill comes from an assertion in the new
|
||
tests, not a syntax or reference error.
|
||
- `build.patch`: `git diff --cached --binary 75ab1646`, 15 files,
|
||
+1068/−27. It applies to 75ab1646 and the manifest matches
|
||
(`out/apply-75ab1646.txt`).
|
||
|
||
### Correction: M31 was never equivalent
|
||
|
||
Round 1 called M31 equivalent: "`readRunRecord` throws the same message".
|
||
That is wrong. The `isRunId` check in `showRun` is the only id check that
|
||
runs before `loadConfig`. Without it, `show ../x` with a missing config
|
||
exits 3 with a configuration problem instead of 4 "invalid run id"
|
||
(Darkwing's probe P1). The round 1 paragraph even said the check stays "so
|
||
that an invalid id refuses before the config loads", which contradicted
|
||
its own verdict. No test covered that case. The new CLI test does, and
|
||
M31 is killed.
|
||
|
||
# Round 1 (candidate ed3c5392 over dc96f87b)
|
||
|
||
Round 1 evidence is under `out/round1/`. Paths below that start `out/` mean `out/round1/` in this packet.
|
||
The round 1 packet files are in `r1/`, unchanged, so the posted hashes still check:
|
||
`r1/build.patch` dd7b469b…, `r1/candidate-manifest.sha256` ed3c5392… and
|
||
`r1/packet-manifest.sha256` e0497ad1…. That manifest lists the round 1
|
||
paths, so it checks against the round 1 tree, not this directory.
|
||
|
||
## Files (`files.txt`, 15)
|
||
|
||
| File | Change |
|
||
|---|---|
|
||
| `packages/runs/package.json` | new: `@mosaic/runs`, private, no dependencies |
|
||
| `packages/runs/src/errors.mjs` | new: `RunsError(message, exitCode = 4)` |
|
||
| `packages/runs/src/paths.mjs` | new: `resolveInside` containment, `readJsonObject` |
|
||
| `packages/runs/src/runs.mjs` | new: `isRunId`, `listRunIds`, `listRunRecords`, `readRunDocument`, `readRunRecord` |
|
||
| `packages/runs/src/state.mjs` | new: `readActivePointer`, `readActivationLog` |
|
||
| `packages/runs/src/index.mjs` | new: re-exports |
|
||
| `packages/runs/README.md` | new: what it reads, the limits, the deliberate deltas |
|
||
| `packages/runs/tests/*.mjs` | new: 37 tests in round 1 (runs 22, state 10, task-cli 5) plus helpers; 41 in round 2 (runs 23, state 11, task-cli 7) |
|
||
| `scripts/mosaic-task.mjs` | `list` and `show` read through the module, +18/−27 |
|
||
| `agents/rocko/work/queue-56/{seed,byte-check,delta-check}.sh` | the byte-identity and delta checks |
|
||
|
||
`build.patch` is `git diff --cached --binary dc96f87b` over those files:
|
||
+1021/−27 in round 1. Round 2's `build.patch` is over 75ab1646, +1068/−27. `candidate-manifest.sha256` hashes the 15 files.
|
||
|
||
Q14 freeze: nothing under `packages/bus`, `packages/tasks`, `packages/cli`,
|
||
`scripts/bus-service.sh` or `scripts/mosaic` changes. Apart from the import,
|
||
`scripts/mosaic-task.mjs` is the only file under `scripts/` that changes. The
|
||
module covers the runs view's needs, but the view itself, `release.sh` and
|
||
pruning are untouched.
|
||
|
||
## Design
|
||
|
||
- **Containment.** `resolveInside` runs `realpathSync` on the data root and
|
||
the target, then applies a `path.relative` check:
|
||
- `..`, `../…` or an absolute relative path counts as outside;
|
||
- the configured data root is trusted even when it is itself a link;
|
||
- links that stay inside it are followed.
|
||
- **Outside paths.**
|
||
- `runs/`, `state/`, a run directory read by `readRunRecord`, `active.json`
|
||
and the activation log refuse when they resolve outside.
|
||
- A run document that resolves outside reads as `null`, the way an
|
||
unreadable one always has.
|
||
- **Missing versus broken.** ENOENT and ENOTDIR read as missing. Anything
|
||
else (ELOOP, EACCES) refuses with the error code instead of reading as
|
||
empty.
|
||
- **Documents.** A run document is a JSON object or `null`. Run records are
|
||
not validated further, because older records carry older shapes.
|
||
- **Release pointer.** `active.json` is strict: exactly the version 1 shape
|
||
`release.sh` writes. Bad data exits 2, and a read error exits 4.
|
||
- **Activation log.** The log is lenient per line, like `release.sh
|
||
rollback`. It returns `{ entries, malformed }`, and `last` keeps the newest
|
||
entries.
|
||
**Corrected in round 2:** not "like rollback". Rollback skips only
|
||
lines that aren't JSON; this module also counts wrong-shaped entries.
|
||
- **Errors.** Every refusal is a `RunsError` with exit code 2 or 4.
|
||
`mosaic-task.mjs` maps it to `fail(exitCode, message)` through
|
||
`readRuns`.
|
||
- **Read-only.** No function writes. The "readers write nothing" test and
|
||
byte-check's before/after snapshot of the data root both check this.
|
||
|
||
## Gate (`out/gate/`, sequential, scratch worktree of the candidate)
|
||
|
||
| Suite | Result |
|
||
|---|---|
|
||
| packages/runs tests | 34/34 at gate time; 37/37 after the mutant round (`out/runs-tests-final.log`) |
|
||
| test-auth | 15 passed, 0 failed |
|
||
| test-conductor | 17 passed, 0 failed |
|
||
| test-config | 24 passed, 0 failed |
|
||
| test-discord | first run 57 passed, 1 failed; rerun 66 passed, 0 failed (below) |
|
||
| test-extension-package | 18 passed, 0 failed |
|
||
| test-foundation | 44 passed, 0 failed |
|
||
| test-queue | 27 passed, 0 failed |
|
||
| test-release | 14 passed, 0 failed |
|
||
| test-task | 98 passed, 0 failed |
|
||
|
||
**test-discord's first-run failure was environmental.** The failing case was
|
||
"pi binary present at node_modules/.bin/pi for the extension checks". The
|
||
scratch worktree has no `node_modules`, and the base worktree lacks it too.
|
||
I symlinked the checkout's `node_modules` into the scratch tree and reran
|
||
the suite: 66 passed, 0 failed. The extra 8 cases are the extension checks
|
||
that the missing binary had skipped. Then I removed the symlink
|
||
(`out/gate/test-discord-r2.log`). Nothing in this candidate touches
|
||
packages/discord. Sage's rerun in a tree that has `node_modules` should see
|
||
66.
|
||
|
||
After the gate, the only changes were tests, the README table row and the
|
||
check scripts. No `src/` or `scripts/mosaic-task.mjs` change came after it.
|
||
|
||
## Byte identity (`out/byte-check.txt`)
|
||
|
||
`byte-check.sh BASE CAND WORK` seeds a data root with `seed.sh`. The seed
|
||
holds:
|
||
|
||
- runs a–f;
|
||
- a run that is a file;
|
||
- an internal link and a dangling link;
|
||
- `r-zz.weird_name-1`;
|
||
- a non-`r-` name, `.pruned.log` and a stray file.
|
||
|
||
It runs 16 cases from both trees: `list` on seeded, empty and absent roots,
|
||
and `show` on each run, a missing run, `../escape`, no argument and an
|
||
absent root. It diffs stdout, stderr and exit codes.
|
||
|
||
```
|
||
byte-check: 16 cases, stdout, stderr and exit identical
|
||
byte-check: data root unchanged
|
||
```
|
||
|
||
**Correction to my own harness.** My first rerun in this round passed a
|
||
relative WORK_DIR. Every redirect inside the `cd` subshell failed, both trees
|
||
recorded exit 1, and the diff still reported "identical": a false pass.
|
||
Earlier runs used absolute paths and were not affected. Both scripts now
|
||
make WORK_DIR absolute before doing anything else. `byte-check.sh` also
|
||
exits 4 if any case leaves no stdout or stderr file. The output above
|
||
comes from the fixed script.
|
||
|
||
## Deliberate deltas (`out/delta-check.txt`)
|
||
|
||
These cases fall outside the byte-identity domain: a link out of the data
|
||
root, a document that is JSON but not an object, a run that is a file, a
|
||
link loop, and an unreadable directory. Each one changes on purpose. The
|
||
README table lists them, and `delta-check.sh` prints base against candidate:
|
||
|
||
- run directory linked out: `list` shows `unknown`; `show` refuses, exit 4.
|
||
- `result.json` linked out: `list` shows `unknown`; `show` prints
|
||
`(missing or unreadable)`.
|
||
- `runs/` linked out: `list` and `show` refuse, exit 4.
|
||
- `result.json` is `5`: base `list` crashed (exit 1); base `show` printed
|
||
`undefined` fields. The candidate gives `unknown` and
|
||
`(missing or unreadable)`.
|
||
- `task.json` is `[]`: base printed the snapshot line; the candidate
|
||
omits it.
|
||
- run is a regular file: base `show` crashed in `readdirSync`; the
|
||
candidate gives `run not found`, exit 4.
|
||
- link loop: base said `run not found`; the candidate refuses with ELOOP,
|
||
exit 4.
|
||
- `runs/` unreadable: base `list` printed nothing, exit 0; the candidate
|
||
refuses with EACCES, exit 4.
|
||
|
||
## Mutants (`out/mutants.sh`, `out/mutants.txt`)
|
||
|
||
I ran 32 single-line mutants of `src/` and `scripts/mosaic-task.mjs`
|
||
against the packages/runs tests.
|
||
|
||
First round: 26 killed, 4 survived and 2 didn't apply. Three of the
|
||
survivors were real gaps, and I added a test for each:
|
||
|
||
- **M02** dropped the exact `..` check. A link to the data root's parent got
|
||
through. New test: "a link to the data root's parent is outside it".
|
||
- **M09** dropped the sort. Node returned this directory already sorted,
|
||
so no fixture could catch it. New test: it mocks `fs.readdirSync` to
|
||
return the names reversed.
|
||
- **M15** read every readdir error on a run directory as missing. New
|
||
test: "an unreadable run directory refuses instead of reading as
|
||
missing".
|
||
|
||
Final round: 30 killed, 2 survived. Both survivors are equivalent:
|
||
|
||
- **M04** dropped `!path.isAbsolute(relative)`. On POSIX, `path.relative`
|
||
between two absolute paths is never absolute, so the branch is
|
||
unreachable on Linux. I kept it as a guard for other platforms.
|
||
- **M31** dropped the CLI's `isRunId` pre-check in `show`. `readRunRecord`
|
||
throws the same message with exit 4, and `readRuns` maps it to the same
|
||
`fail`. The pre-check stays so that an invalid id refuses before the
|
||
config loads, as it did before.
|
||
|
||
**Corrected in round 2:** M31 was not equivalent. See "Correction: M31
|
||
was never equivalent" above; it is killed in round 2.
|
||
|
||
## Follow-up (not fixed, out of scope)
|
||
|
||
This defect predates the candidate and is unchanged: `list` crashes with
|
||
a `padEnd` TypeError when `result.json` is an object without a string
|
||
`status` or `taskId`. Base and candidate behave the same here, so
|
||
byte-identity holds. A fix would change `list` output and belongs in its
|
||
own row.
|
||
|
||
## Reproduce
|
||
|
||
```sh
|
||
git worktree add --detach /tmp/r56-base 75ab1646
|
||
git worktree add --detach /tmp/r56-cand 75ab1646
|
||
(cd /tmp/r56-cand && git apply --index <packet>/build.patch && sha256sum -c <packet>/candidate-manifest.sha256)
|
||
env -u NODE_TEST_CONTEXT node --test '/tmp/r56-cand/packages/runs/tests/*.test.mjs'
|
||
bash /tmp/r56-cand/agents/rocko/work/queue-56/byte-check.sh /tmp/r56-base /tmp/r56-cand /tmp/r56-bc
|
||
bash /tmp/r56-cand/agents/rocko/work/queue-56/delta-check.sh /tmp/r56-base /tmp/r56-cand /tmp/r56-dc
|
||
```
|