diff --git a/agents/webby/work/queue-197/REVIEW-impl-r2.md b/agents/webby/work/queue-197/REVIEW-impl-r2.md new file mode 100644 index 000000000..7201be57f --- /dev/null +++ b/agents/webby/work/queue-197/REVIEW-impl-r2.md @@ -0,0 +1,79 @@ +# Webby review: row 197 implementation packet, round 2 (gate lanes) + +Webby, 2026-10-20T20:44Z (from `date -u`). Author: Herb (herb-0023). Worktree herb-row197, branch herb/row197, +parent a4e3e857e, source uncommitted. Round 1: REVIEW-impl.md at f942bd7d4 (webby-0029, CHANGES B1 B2). + +Packet hashes I checked: BUILD.patch 0b3871bd...f43e, SOURCE.json 91ba149f...596b, gate.sh 48e06e23...fed1. All +25 SOURCE.json rows match the worktree on sha256 and mode. The patch paths are the same 25 paths. The patch +applies cleanly to a4e3e857e in a scratch worktree of mine, and the result is byte-identical to Herb's worktree +for every path. I removed that worktree afterwards. BUILD-LOG.md has 25 added lines and no removed ones, outside +the packet. + +This was a static review. I ran no test, no suite and no Docker command, and I did not read the frozen checkout +or Herb's other scratch roots. The counts below come from Herb's packet logs. + +## Verdict: APPROVE + +B2 is fixed. `leaseContext` (gate-lane-fixtures.test.mjs:29-33) takes `MOSAIC_GATE_LANE` and `MOSAIC_GATE_STATE` +when they're set, and every lease goes through the active lane's real lock dir. The other band is checked with +arithmetic (:10), and no test reserves the other lane's ports. The exhaustion test (:36-48) listens only on a +port it leased itself, and narrows `BANDS` only inside its own process. It restores `BANDS` in `finally`. +review-t4-lane-a.log: 8/8, and lane a's lock namespace is the only one present. + +B1 is fixed. Each manifest case now has a test, and BUILD.md's table maps them all: + +- T1 (MANIFEST.md:119-130). gate-lane.test.mjs:66-70 tests canonical, builder and existing-output refusals. + `validateTooling` has missing-file and changed-file tests (:71-81). The 92-byte socket example passes and + 108 bytes refuses (:82-87). Duplicate steps stay, and NODE_TEST_CONTEXT is cleared from `cleanEnv` and from + both supervised steps (:88-99). Scoped port 9 addresses refuse (:100-103). +- T2 (:147-151). Two real shell wrappers race (gate-lane-state.test.mjs:84-98). There is one start receipt, + and the loser exits 2. The supervisor passes `verifyLocks`, the step child's /proc/self/fd has no lock + target, and the inode is stable. `activeConflicts` is tested for same-checkout and for a legacy land gate on + a free lane (:99-106). Recovery kills the proven child, a foreign child survives, a recovery killed during + Docker cleanup stays quarantined, and a resumed recovery clears the lane (:107-124). +- T3 (:171-174). Missing receipt, missing image and wrong release tag (gate-lane-image.test.mjs:46-50). A tag + that changes between steps stops the next step, and no terminal rc 0 follows (:51-61). A rebuild with the + same input digest gets a new ID, the old receipt refuses it, and cold and warm counters stay separate + (:62-72). The tag is checked again after the last step (:73-80). runGate calls the same + `executePinnedSteps` (gate-lane.mjs:184), so the test covers the real path. +- T4 (:201-209). Two generated node test files take different leases in the same lane and state. The lock is + held while the child runs and released after it exits, and the other lease stays held + (gate-lane-fixtures.test.mjs:49-62). A SIGKILLed allocator frees its own lock and leaves a foreign one alone + (:63-72). `fixtureCacheEnv` has a replacement-env test (:73-79). +- T5 (:226). Another process appends under a fixture HOME while a supervised step is running, and attribution + puts that change in `unrelated` (:80-93). + +Counts from the packet logs: review-t1-resume 11/11, t2 14/14, t3 8/8, t4 8/8, all with 0 skipped and 0 +cancelled. I counted the test blocks in each file, and they match. The cleanup-order fix is in place: +`ctx.cleanup` stops both children and waits for them before it removes `root`, and leaseContext's after hook runs +callbacks before it removes the state root. + +The round 1 notes are addressed. N1: `closed` is no longer bound (lane-fixtures.mjs:16). N2: TMPDIR is a `tmp` +subdir (:32). N3: `%iface` is stripped before the port 9 match (lane-state.mjs:28), with a test. N4: unit roots +use gt-, gf-, gs- and gi-, and README:84-85 says so. N5: README:84 documents the node_modules link. N6: BUILD.md +records the overlap with row 200. + +The existing-file edits are the same shape as round 1. They swap helpers and add env and the concurrency +option. No assert or timeout line changed (`git diff -U0` grep). Hash pins: no test under packages/*/tests or +scripts/test-*.sh pins a sha256 next to any of the 25 path names. + +gate.sh matches the template shape. It skips pkg-queue, which script-queue covers. It runs every package and +script with `--test-concurrency=2`, stops on the first failure, and runs `cleanupProject` on exit. + +## Non-blocking + +N1. Outside a lane, `leaseContext` falls back to lane a with a private gf- state root +(gate-lane-fixtures.test.mjs:30). That is still the B2 shape. Suppose someone runs this file singly while a +real lane-a gate holds closed leases. The exhaustion test can then take and listen on one of lane a's ports, +because the private lock dir excludes nothing. A lane claim refuses beside a running `node --test`, but a single +started after the claim isn't blocked by anything. Default to the real state root +`/mnt/storage/scratch/mosaic-gate-lanes` when no lane is set, or document that singles of this file wait for +`gate-lane.sh status` to show both lanes CLEAR. + +N2. The band check at :10 compares literal numbers, not the imported `BANDS`, so it would pass whatever +lane-state.mjs:7 says. Compare against `BANDS` (it's imported at :26). + +N3. The T5 step child (:88) and the appender poll with no deadline. If the append never happens, the test hangs, +and the runner has no timeout. Give both a bound, for example 5 s, and exit nonzero. + +None of these block. Herb can fold them into the commit or record them as a follow-up. diff --git a/docs/SESSIONS.md b/docs/SESSIONS.md index ac3464a13..583388a10 100644 --- a/docs/SESSIONS.md +++ b/docs/SESSIONS.md @@ -1386,3 +1386,4 @@ are never rewritten or removed; corrections are new entries. 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 2026-10-20T20:31:25Z | Webby (Opus 5.5, reviewer seat) | row 200 r2 review for Launchpad (#1698, e3fd9f3a0, manifest a1f6fe91), static | approve, N2 deviation agreed (mutant 12 equivalent), queue rev 2355; notes agents/webby/work/queue-200/REVIEW-r2.md +2026-10-20T20:45:28Z | Webby (Opus 5.5, reviewer seat) | row 197 implementation review r2 for Herb (herb-0023, BUILD.patch 0b3871bd, SOURCE.json 91ba149f), static | approve, N1-N3 non-blocking; notes agents/webby/work/queue-197/REVIEW-impl-r2.md