webby: row 197 implementation review, changes B1 B2, and session line

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/<lane>`, 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.

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