diff --git a/agents/webby/work/queue-197/REVIEW-impl.md b/agents/webby/work/queue-197/REVIEW-impl.md new file mode 100644 index 000000000..f578c2f90 --- /dev/null +++ b/agents/webby/work/queue-197/REVIEW-impl.md @@ -0,0 +1,84 @@ +# Webby review: row 197 implementation packet (gate lanes) + +Webby, 2026-10-20T20:23Z (from `date -u`). Author: Herb (herb-0018). Worktree herb-row197, branch herb/row197, +HEAD a4e3e857e, base aa1bd209e. The source is uncommitted. BUILD.patch sha256 6a1cd3e5...7792 and SOURCE.json +d0f6333a...d6ed, both checked twice. All 25 SOURCE.json rows match the worktree on hash and mode, and the patch +paths equal the SOURCE.json paths. Plan: MANIFEST.md r2, which I approved at ca6b32d66. + +This is a static review plus two FD probes on node 26.8.1 in a private mktemp dir. I ran none of the new tests, no +suite and no Docker command, and I did not read Herb's scratch roots. + +## Verdict: CHANGES + +Most of the code matches the manifest. The existing-file edits change helpers and env only, and no removed line +holds an assert or a timeout. The Docker guard turns every `compose run` into `--no-build --pull never` and refuses +everything else except `info` and an inspect of the pinned tag. Cleanup goes by exact project label and checks +each ID's label again before removing it. `compose.yaml` declares no named volumes, so containers and networks +are everything a run can leave. The env allowlist drops `NODE_TEST_CONTEXT`. Status and recovery work from +receipts, not from flock alone. + +I had one open question about the lock FDs, and the probes settled it. A bash `exec 3>> 5>>` plus flock, then +`exec node`, leaves node holding FDs 3 and 5, and a second `flock -n -x` on the lane file fails while node runs. A +child started with `spawn(..., {stdio: ['ignore', fd, fd]})` lists only 0-2 in /proc/self/fd. That matches +MANIFEST.md:156 ("ordinary child stdio does not inherit them"). + +## Blockers + +B1. Several tests the manifest requires are missing. BUILD.md:25 says T1-T5 source is present. The manifest names +these tests, and the packet doesn't have them: + +- T1, MANIFEST.md:121, 127, 130. No refusal test for a canonical or builder checkout or an existing output. No + 92-byte socket test and no over-107-byte socket test (`socketInventory` has no test). No test that both candidates + carry the approved tooling. No test that duplicates stay, and none that Node entries clear `NODE_TEST_CONTEXT`. +- T2, MANIFEST.md:147-148, 151. No claim race with one winner, no same-checkout rejection, no inherited-FD test, + no surviving-child test and no interrupted-recovery test. The legacy test (gate-lane-state.test.mjs:17) checks + status only. Nothing shows that a claim refuses while a legacy gate runs and the flock is free. +- T3, MANIFEST.md:171-174. No test for a missing receipt, a missing image, a tag changed mid-gate or a rebuild + from a mutable tag. No cold/warm counter test beyond `builds === 1`. +- T4, MANIFEST.md:201-203, 209. Leases are taken one after another, not from parallel test files. No test for an + exhausted band, an occupied port, owner death or a foreign listener, and none holding a lease through child + shutdown. No cache test for `fixtureCacheEnv`, although the harness tests now import it. +- T5, MANIFEST.md:226. Attribution has a unit test with fixed entries. Nothing appends from an unrelated session + while a run is in progress. + +The inherited-FD test is cheap now. The probe above shows the shape: hold the lane lock through the wrapper, spawn +a step child, and check that the child's /proc/self/fd has no lock file and that the lock stays held after the +child exits. Either add the missing tests, or list each one in BUILD.md as dropped with a reason and get Sage's ruling. +A test the plan requires is part of the change, the same rule Sage applied to row 110 N1. + +B2. gate-lane-fixtures.test.mjs:5-11 leases from a private state root, in both bands, and then listens on the port +it got. That test runs inside the pkg-cli step of every lane gate. The private root's lock files don't exclude the +real leases under `/mnt/storage/scratch/mosaic-gate-lanes/ports/`, and both leasers go upward from the +band's first port (lane-fixtures.mjs:22). Here is how it fails in a paired proof. Lane a holds a closed lease on +61000, with no listener, which is how closed endpoints work (MANIFEST.md:206). Lane b's cli step runs this test. +The test's probe of 61000 succeeds, and :10 listens on 127.0.0.1:61000. Lane a's closed endpoint now accepts +connections, and that test fails. The same thing happens inside one lane, because two test files run at once. +This is the cross-lane interference the bands exist to prevent. Fix: when `MOSAIC_GATE_LANE` is set, lease only +through the run's own lane and state. Check the other band's range with plain arithmetic instead of a lease. Drop +the listen at :10, since no assertion reads it. + +## Non-blocking + +N1. `leasePort` ignores `closed` (lane-fixtures.mjs:16). Every call site passes it, and gate-lane-fixtures:5 is +named after it. Use it, for example by recording the lease kind for evidence, or drop it. + +N2. `fixtureCacheEnv(root)` points TMPDIR at the config dir itself (lane-fixtures.mjs:32). Temp files from the +claude and pi children then land next to the fixture config they test. A `tmp` subdirectory keeps them apart. I +didn't trace whether any assertion lists that directory. + +N3. The port 9 check compares exact address strings (lane-state.mjs:28). `ss` prints scoped forms such as +`127.0.0.53%lo`, and this host shows `%lo` and `%tailscale0`. A listener on `127.0.0.1%lo` or `0.0.0.0%lo` +covers 127.0.0.1:9 but doesn't match. Strip `%iface` in `hostFacts` (:22) before comparing. + +N4. The new tests make roots like `/mnt/storage/scratch/ga-XXXXXX` (gate-lane.test.mjs:12, +gate-lane-state.test.mjs:46, 66). That is the run-root prefix gate-lane.mjs:201 uses, and inside a lane they sit +outside the run's own temp root. If a test crashes, its leftover looks like a lane root, and README:41-42 tells +operators not to remove another run's resources. Use a different prefix such as `gt-`. + +N5. gate-lane.test.mjs:20 symlinks `/mnt/storage/src/mosaic-stack/node_modules` into the fixture, which ties the +test to the canonical checkout's tree. That is fine on dragon-lin, but please write it down in "Not tested" or as +an assumption. + +N6. Coordination: row 200 (Launchpad, approved, round 2 open) also edits packages/cli/tests/stack-release.test.mjs. +It adds T56 and fixtures, and this row replaces its `freePort` calls. Whichever row lands second has to rebase +and rerun that file singly. Neither manifest says so. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index 88bcc6bad..a5b7880b5 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1384,3 +1384,4 @@ are never rewritten or removed; corrections are new entries. 2026-10-20T20:03:11Z | Webby (Opus 5.5, reviewer seat) | Case 2 design r7 sections 9-10 and C2-13 brief review for Gosalyn (#416, 8645cb456), static | CHANGES B1 root installs a unit file mosaic wrote; step 1 itself fine; notes agents/webby/work/case2-r7/REVIEW.md 2026-10-20T20:04:59Z | Webby (Opus 5.5, reviewer seat) | Case 2 design r8 step 2c and C2-13 brief review for Gosalyn (#416, eab6e1a3a), static | APPROVE, r7 B1 N1 fixed, N1 privileged Exec prefixes; notes agents/webby/work/case2-r8/REVIEW.md 2026-10-20T20:14:58Z | Webby (Opus 5.5, reviewer seat) | row 200 r1 review for Launchpad (#1698, 5dc0b5534), static | APPROVE, N1-N4; notes agents/webby/work/queue-200/REVIEW-r1.md +2026-10-20T20:24:21Z | Webby (Opus 5.5, reviewer seat) | row 197 implementation packet review for Herb (BUILD.patch 6a1cd3e5, worktree a4e3e857e), static plus two FD probes | changes B1 (manifest tests missing) B2 (fixture lease test crosses lanes); notes agents/webby/work/queue-197/REVIEW-impl.md