fix(bin): contain test lane execution, add sync-axi and agy adapters, and harden crew supervision - #2639
Open
BohnBawerick wants to merge 67 commits into
Open
Conversation
Kimi 0.36.0 prompts Trust this folder? on every untrusted worktree path, with Don't trust preselected. The readiness gate waited only for Welcome or an empty composer, so spawn aborted and the pane died. Detect that exact dialog, send Up then Enter, and keep the existing readiness and delivery checks. Do not write ~/.kimi-code/workspace-trust/.
Checked kimi-cli and kimi-code CLI/config docs plus kimi --help. --auto and --yolo do not skip Trust this folder?, so spawn still accepts the dialog with Up then Enter.
Applied locally at the captain's explicit instruction so kimi is dispatchable in this home today. The same change is open upstream as PR kunchenguid#2328, held by GitHub pending first-time-contributor workflow approval. This diverges the primary from origin/main until that PR lands. Reconcile by dropping this merge once upstream carries the change.
Verify Antigravity CLI as a crewmate/scout adapter. Pin the launch to gemini-3.1-pro-high, omit --effort (it conflicts with *-high model ids), accept the workspace trust dialog, and install a gated global Stop hook. Secondmate, primary turn-end, and watcher-arm remain unverified.
A separated greater-than pair is unknown on zellij, cmux, and orca because agy is not verified there. Identity-capable backends keep the existing probe-then-shortcut path.
Verify Antigravity CLI as a crewmate/scout adapter. Pin the launch to gemini-3.1-pro-high, omit --effort (it conflicts with *-high model ids), accept the workspace trust dialog, and install a gated global Stop hook. Secondmate, primary turn-end, and watcher-arm remain unverified.
A separated greater-than pair is unknown on zellij, cmux, and orca because agy is not verified there. Identity-capable backends keep the existing probe-then-shortcut path.
…derive teardown pointers
Applies the same pattern as the Kimi 0.36.0 workspace-trust fix: the adapter works and the runtime is installed and funded, so the running copy gets it now rather than waiting on the PR to land. Reconcile by dropping this merge once the upstream PR merges. Adopted at fm/hz-agy-adapter e474f5a; the review fix round still in flight is not included and will arrive with the PR. # Conflicts: # bin/fm-spawn.sh # docs/configuration.md
- The mode=no-mistakes ship scaffold told the worker to invoke the /no-mistakes skill three times. A crewmate runs in a project worktree, not the firstmate home, so that skill is unreachable and the worker stalls exactly where it has just finished implementing. Overnight 2026-08-17/18 three workers each burned a supervisor round trip there and reported done: for a commit with no PR. - The scaffold now names the interface the worker actually has: the no-mistakes CLI on PATH, with the concrete run and respond commands, and it notes that firstmate's trigger may still be worded as a skill invocation so either wording lands on the same command. - Harden the failure mode the defect lands on: completion for this mode is now stated as a green PR, the implementation handoff line says in its own text that nothing has shipped yet, and a run that cannot start is routed to blocked: rather than done:. - direct-PR refused "/no-mistakes"; it now refuses the pipeline by name. local-only, scout and the secondmate charter carried no such premise. - Extend tests/fm-brief.test.sh: every variant is generated and checked for skill-invocation instructions, and the no-mistakes definition of done is pinned to the CLI commands and the PR-bound done: gate.
…e affirmatively reports working
- Add an away supervision model to bin/fm-wake-lib.sh: while state/.afk exists the away daemon owns supervision for every primary harness and runs the watcher one cycle at a time, so an unheld watcher lock is the healthy state and the daemon itself is what must be tested. - fm_turnend_supervision_healthy keeps the PID-strict watcher check everywhere except away mode, where fm_away_daemon_healthy requires a live identity-matched daemon plus a turning loop; a dead, recycled-pid or wedged daemon still blocks the turn end. - Bound daemon-tick freshness with FM_AWAY_TICK_GRACE (180s), derived from the daemon's housekeeping cadence and crash backoff rather than guessed, and read the freshest of its housekeeping tick, its watcher child's beat, and its startup stamp. - Give the pull guard the same answer, and name the daemon in both banners instead of telling the session to arm a watcher. - Let bin/fm-afk-start.sh delegate its already-running check to the same predicate so entering away mode and guarding it cannot disagree. - Away mode outranks a pinned FM_SUPERVISION_MODEL harness model, which bin/fm-spawn.sh bakes into every secondmate launch.
Suppress the possible-wedge escalation when the busy source affirmatively reports a pane as working. Away-mode housekeeping was aging a stale marker into a wedge alarm without ever asking whether the worker was busy, which produced repeated false alarms on panes that were provably mid-task. Landed locally on the captain's word so the fleet gets the fix now; upstream PR 2554 stays open on its own at kunchenguid/firstmate.
Stop the turn-end guard raising TURN WOULD END BLIND on a healthy fleet under away mode. The guard demanded a live long-lived watcher process, which away mode deliberately does not run - the away daemon owns supervision and runs the watcher one-shot. The supervision-model vocabulary in fm-wake-lib.sh now covers that fourth shape, so a healthy away fleet stops quietly while a dead or stalled daemon still blocks. Measured 3/200 false blocks before, 0/200 after. Landed locally on the captain's word so the fleet gets the fix now; upstream PR 2557 stays open on its own at kunchenguid/firstmate.
A run's recorded head is the head the PIPELINE has advanced to as it applies its own fix commits, and those commits are pushed to the configured target rather than fetched into the crew's worktree - so during a run the head does not resolve in that repository at all. The attribution rule treated an unresolvable head as a mismatch, rejected the branch's own live row, and matched the newest stale row underneath it: a failed run recorded at the very head the worktree still held. A healthy validation therefore read as `failed`, which routes firstmate into recovery and can restart a run that is working. - Code identity is now ternary: match, mismatch, and unverified for a real run head this copy cannot resolve. The strict predicate is unchanged, so teardown's abort authority still requires a verified binding. - An unverified run is bound by its submitted head - the head it was launched against - read from `axi sync --check`, the one read-only surface that reports it on the installed v1.48.0. - When nothing binds the run it may still report work under way, but never a terminal verdict: `failed` and `done` become `unknown`, which is not itself an instruction to act. - The coarse runs list stops at the branch's newest row, so a superseded run can no longer answer for the current one.
… attribution rules
Generated ship briefs told the worker to invoke the /no-mistakes skill, which a worker cannot reach: the skill lives in the firstmate home and loads for a firstmate session, while a worker runs in an isolated project copy. Every brief variant now names the CLI path instead. Also narrows the accompanying "never report done: for work that has no PR" line to the case where a validation run cannot start, resolving the contradiction with the section 7 handoff. The captain approved that scoping (option a) and separately approved removing the intermediate handoff altogether, filed as fm-validation-self-start. Landed locally on the captain's word; upstream PR 2566 stays open at kunchenguid/firstmate with all 13 checks green.
fm-crew-state.sh reported a live, healthy validation run as failed. It matched runs to a task by comparing the run's head against the worktree head, but a running pipeline commits its fix rounds to the gate repository, so that head is absent from the worktree's object store. The live row was rejected and an older failed row at the unchanged worktree head answered instead - a confident wrong verdict that routes firstmate into recovery and risks restarting healthy work. Now uses ternary code identity bound to the run's submitted head, withholds a terminal verdict when identity cannot be proven, and takes the newest matching row. 7 new regression assertions. Landed locally on the captain's word. Upstream PR 2569 stays open at kunchenguid/firstmate; its checks are held pending the repository owner's approval of outside-contributor workflow runs, which never arrived. Our own pipeline completed review, test, document and lint with no findings.
… main Firstmate's delivery path on a fork repository left tasks unable to land where the running fleet actually executes. For ordinary projects, no-mistakes tasks merge remotely via PR and sync down via fleet-sync. For Firstmate's own repository, local main is authoritative for the fleet while upstream PRs remain open outward contributions. Because fm-merge-local.sh refused mode!=local-only tasks and fm-fleet-sync.sh diverged on upstream origin/main, changes were stranded. - Teach bin/fm-merge-local.sh to fast-forward local main for Firstmate's own repository tasks (where project is FM_ROOT/FM_HOME) in any mode. - Teach bin/fm-pr-merge.sh to fast-forward local main when merging a Firstmate repository task remotely. - Teach bin/fm-fleet-sync.sh to skip FM_ROOT/FM_HOME gracefully rather than falsely alarming STUCK on upstream origin/main. - Teach bin/fm-teardown.sh to recognize work landed in the local default branch. - Add docs/verification/fork-reconciliation.md with the empirical evidence and reconciliation plan for fork/main. - Add tests/fm-merge-local.test.sh and expand fleet-sync, pr-merge, and teardown test suites with full RED-GREEN coverage.
Away-mode housekeeping treated every failed capture as a gone pane and dropped the marker with no escalation. A redraw, timeout, or backend hiccup then silently stopped watching a worker that was still there, which is the failure this path exists to prevent. Both the stale-wedge and pause-resurface sites now share stale_window_recheck: retry the capture twice (0.4s apart) before verdict, then ask fm_backend_agent_state. Only an authoritatively missing endpoint is gone. A present dead shell is ordinary idle. Every other state, including an unreadable or unverified probe, escalates and keeps the marker on the same cadence because the watcher cannot recapture an unreadable pane. target_exists is not used as a gone proof: tmux can fall back to the active window, and Orca's check is itself a capture. Tests cover gone, unreadable-present (alive/unreadable/unverified), retry-then-ordinary, and dead-is-not-gone at both call sites.
Remove the intermediate pre-PR done: handoff. The implementation worker starts its own no-mistakes CLI run immediately after the commit, reports working: when that run starts, and reports done: only with a PR.
Firstmate's own tasks could not land where the running fleet executes. On this fork, local main is authoritative and upstream PRs are an outward courtesy, but the local merge path refused any task that was not local-only and fleet sync falsely alarmed on upstream origin/main, so finished work was stranded. Teaches the local merge path, the PR merge path, fleet sync, and teardown to recognize firstmate's own repository and its authoritative local main. Adds docs/verification/fork-reconciliation.md plus a new merge-local test suite and expanded fleet-sync, pr-merge and teardown coverage. Landed locally on the captain's explicit word. Our own pipeline completed review, test, document and lint with no findings. The upstream PR 2597 check failure is the known fm-watcher-lock fork/execve flake, tracked separately as fm-racy-watcher-lock-test.
A worker that finished writing code stopped and waited for firstmate to tell it to start validation. That handoff is a message that can fail to land: one worker sat finished and idle for eighty minutes today because the start message was never submitted, and every task paid the round trip even when it worked. The worker now starts its own no-mistakes run immediately after the implementation commit, announces the start with a nonterminal working line, and must report failed or blocked if the run dies mid-pipeline, so firstmate still learns start and failure without a handoff. It may no longer claim done before a PR exists. Review, tests, gates, ask-user escalation and merge authority are untouched: the pipeline's own separate agent still does the reviewing, the author still cannot answer its own ask-user finding, and --yes remains banned. Landed locally on the captain's explicit word.
Task ids become free again after cleanup, but the previous task's data directory was left on disk. Reusing the id then handed the new worker the old task's brief, silently, and it would build the wrong thing. Scaffolding now refuses a reused id while that directory still exists, rather than writing over or reading through it. Landed locally on the captain's explicit word.
While the captain is away, the background supervisor treated every failed screen capture as proof the worker was gone, dropped its marker and silently stopped watching it. A redraw, timeout or backend hiccup was enough. That is precisely the failure the away-mode supervisor exists to prevent. Both the stale-wedge and pause-resurface sites now retry the capture twice before deciding, then ask the backend whether the endpoint actually exists. Only an authoritatively missing endpoint counts as gone; a present dead shell is ordinary idle; every other state, including unreadable or unverified, escalates and keeps the marker. Endpoint presence alone is not accepted as proof, because tmux can fall back to the active window and Orca's own check is a capture. Landed locally on the captain's explicit word.
Session start printed data/captain.md and data/learnings.md whole, so the startup memory surface grew with no read path that could refuse it: 35,526 estimated tokens against a 7,500-token budget on the reference home. - bin/fm-memory-compile.sh compiles the bundle session start injects: a standing core, a catalog of every note, and the notes whose triggers match live fleet work, capped against config/startup-memory-budget. Core is never dropped, the catalog outranks every note, and a note that does not fit is skipped rather than ending selection. - bin/fm-memory-migrate.sh splits a home's data/learnings.md into one atomic cited note per heading, publishes the catalog, and freezes plus archives the original before removing it. - fm-session-start.sh injects the compiled bundle when data/memory/ exists and keeps the whole-file print when it does not, or when the compile fails. - Two session-start fixtures forced a MISSING diagnostic by removing node from the fake bin, which proves nothing on a host that also ships /usr/bin/node. They now shadow gh-axi, which cannot exist outside the fake bin. On the reference home the surface goes from 35,526 tokens to 7,490, and to 7,480 with four hot notes once the core is trimmed to its target size.
- Each compiler mode now accepts only its own flags, so `catalog --context` or `compile --dry-run` is a usage error instead of a silently ignored option that reads as a compile which simply matched nothing. - Session start says so when it cannot create the temporary file for the compiled bundle, rather than falling back to the whole-file print with no word about why.
Trigger derivation stemmed a trailing `s` off every proper noun, so a heading about `Windows` produced the trigger `window` - which then matched nothing, because trigger matching is whole-token. The stopword test now tries the singular while the trigger keeps its original spelling. The possessive-stripping `\b` is a GNU sed extension that does nothing on BSD sed. It was also redundant: the following substitution already splits `Firstmate's` into `Firstmate` and a one-character `s` the length filter drops.
The network-partition fixture removed node from the fake bin to produce a local-half diagnostic, which proves nothing on a host that also ships /usr/bin/node: the base PATH still satisfies `command -v`, no MISSING line is emitted, and the partition assertion fails for a reason that has nothing to do with the partition. gh-axi cannot exist outside the fake bin, so the assertion now means the same thing on every host. Same change as the two session-start fixtures in this branch's first commit.
BohnBawerick
force-pushed
the
fm/fm-racy-watcher-lock-test
branch
from
August 19, 2026 20:43
cf6266e to
d2e889d
Compare
Three defects in the same family made the behavior suite untrustworthy: a lane could hang indefinitely with no diagnostic, tests leaked the processes they started, and one check went red at random. - bin/fm-test-run.sh runs every script contained: its own process group, a private output file instead of a share of the lane's own stdout pipe, a per-script wall-clock budget (--script-timeout, default 1800s), and a reap of whatever the script leaves behind. A leak is reported as FM_TEST_LEAK and the lane keeps going; a script that stops making progress is terminated and named with exit=124. Reaping is always by PID or by the group id the lane itself created, never by matching command lines. - tests/lib.sh gains a spawned-process registry: fm_test_track_pid makes the reap unconditional, so a process survives neither a passing test nor one that fails an assertion before its own reap runs. Every reap escalates TERM to KILL after a bounded grace, because a shell whose trap action fails to parse at delivery swallows the signal and keeps running. - tests/lib.sh gains fm_test_wait_exec_settled, and the watcher-lock test waits for a backgrounded child to finish becoming itself before sampling its pid identity. Sampling inside the fork-to-exec window reads the forking shell's own command line, which is why that check passed locally and failed on a cold runner. Adds four lane-safety regressions to tests/fm-test-run.test.sh, a non-vacuous execve-settling regression to tests/fm-watcher-lock.test.sh, and the evidence record in docs/verification/test-lane-safety.md.
BohnBawerick
force-pushed
the
fm/fm-racy-watcher-lock-test
branch
from
August 19, 2026 21:00
d2e889d to
1f7e2b5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Make the Firstmate test lane safe: stop it hanging forever, stop it leaking processes, and fix the test that goes red at random. This is the piece that makes our pipeline trustworthy, so nothing else about it can be taken on faith.
Three defects, one family.
The random red check. tests/fm-watcher-lock.test.sh::test_pid_identity_is_locale_invariant is racy by construction: it launches "sleep 300 &", captures the pid, then samples /proc//cmdline twice and compares. The first sample can land inside the fork window, before execve has replaced the image, so it reads the forking shell's own command line, while the second, milliseconds later, reads the exec'd image. It passes locally and fails on a cold runner that widens the fork window. Fix: do not sample the child until execve has completed - wait for the exec'd image to settle before taking the first sample, with a bounded timeout that fails loudly rather than sampling early. Do not paper over it by comparing only the locale-formatted prefix; the point of the test is the pid identity string. Prove it by artificially widening the window (sampling immediately after the fork with no settle wait), showing red, then showing green with the settle wait, repeated enough times to mean something.
The leaked process, reproduced from two different test files, so it is a family problem needing a shared fix. A lane runner (bin/fm-test-run.sh --lane portable-serial-3of4) was still alive after 7h23m with its output frozen, holding a test-started watcher that never exits. The lane's last output before it froze was a diagnostic from bin/fm-watch.sh: "trap: line 2: unexpected EOF while looking for matching )". That trap diagnostic is the load-bearing clue and must be explained, not worked around. Separately, tests/fm-watch-triage.test.sh left an orphan reparented to init with its scratch state directory already deleted, so its cleanup had run and only the process outlived it; treat that as possibly a second, different cause and diagnose it on its own evidence rather than assuming one root cause covers both.
The wedge mechanism, the worst of the three. The orphan's file descriptor 2 was still the lane's stdout pipe, the same pipe the lane's tee read from, confirmed by identical pipe inodes. So tee never saw end of file, the lane runner blocked on tee forever, and the suite made no further progress with no diagnostic at all. One orphan is enough to hang a whole lane silently and indefinitely.
Required, and none of these substitutes for another:
Scope. This change owns bin/fm-test-run.sh's lane safety (reaping, pipe inheritance, per-script timeout) and the two watcher test files. A sibling task owns the other half of the suite problem: triaging the scripts that fail on this machine under load, and making individual slow scripts faster. Findings that clearly belong to that sibling are written into the PR body rather than fixed here.
Constraints. Do not weaken an assertion to make a test green; a test that no longer proves its contract is worse than a slow one. Colocate tests with the existing pattern in tests/, name them .test.sh, and extend an existing script rather than inventing a new runner. Tests must exercise behaviour through an executable interface, never assert implementation-source bytes. Never pattern-kill: no pkill -f, no killall, because other lanes share this machine's process table and a pattern kill has already reached into a sibling lane; reap by pid. bin/fm-lint.sh must pass. One full sentence per line in tracked Markdown, plain dash and never an em dash, and no agent co-author on commits.
What Changed
bin/fm-test-run.shwith process group isolation, private file output redirection, per-script timeouts, and automatic process tracking and reaping intests/lib.sh(fm_test_track_pid,fm_test_wait_exec_settled).agy) harness adapter support with turn-end hook integration inbin/fm-agy-turnend-hook.shand introduced the/sync-axisynchronization tool and skill (bin/fm-sync-axi.sh,.agents/skills/sync-axi/SKILL.md).bin/fm-spawn.sh, corrected crew validation state reporting inbin/fm-crew-state.sh, and updated local branch landing workflows inbin/fm-merge-local.sh.Risk Assessment
✅ Low: The test lane containment, per-script timeout, process group reaping, and execve settlement changes are well-bounded, thoroughly tested, and all prior review findings have been resolved.
Testing
Executed the full set of targeted test suites (
tests/fm-test-run.test.sh,tests/fm-watcher-lock.test.sh,tests/fm-watch-triage.test.sh) and generated empirical verification artifacts covering fork/exec settling invariance, process containment without pipe leaks, per-script timeouts, and test-helper cleanup, with all tests passing cleanly.Evidence: Fork/Exec Settling Comparison
Source: Fork/Exec Settling Comparison
=== Demonstration: Fork-to-exec settling window === Widening fork-to-exec window with bash -c 'sleep 0.25; exec sleep 300' Running 20 iterations with IMMEDIATE sampling (no settle wait): iter 1: sample1 != sample2 (diverged mid-exec) iter 2: sample1 != sample2 (diverged mid-exec) iter 3: sample1 != sample2 (diverged mid-exec) Immediate sampling divergence: 20 / 20 iterations (FAILED / RED) Running 20 iterations with fm_test_wait_exec_settled (with settle wait): Settled sampling divergence: 0 / 20 iterations (PASSED / GREEN)Evidence: Lane Leak Containment & Output Follower Evidence
Source: Lane Leak Containment & Output Follower Evidence
=== Demonstration: Lane containment of leaked processes === Running leaky.test.sh (leaves background sleep 600 holding stdout) followed by after.test.sh FM_TEST_BEGIN 2026-08-19T18:03:07Z /tmp/demo-lane-leak.bZoP4b/leaky.test.sh family=unclassified expected_gate_skip=none ok - leaky test started ok - leaky test finished its assertions fm-test-run: reaping processes /tmp/demo-lane-leak.bZoP4b/leaky.test.sh left behind (group 49791) FM_TEST_LEAK /tmp/demo-lane-leak.bZoP4b/leaky.test.sh pgid=49791 FM_TEST_END 2026-08-19T18:03:13Z /tmp/demo-lane-leak.bZoP4b/leaky.test.sh exit=0 duration_ms=5476 gate_skip=false FM_TEST_BEGIN 2026-08-19T18:03:13Z /tmp/demo-lane-leak.bZoP4b/after.test.sh family=unclassified expected_gate_skip=none ok - after script executed successfully FM_TEST_END 2026-08-19T18:03:13Z /tmp/demo-lane-leak.bZoP4b/after.test.sh exit=0 duration_ms=160 gate_skip=false FM_TEST_SUMMARY total=2 failed=0 skipped_gate=0 duration_ms=5813Evidence: Lane Per-Script Timeout Enforcement Evidence
Source: Lane Per-Script Timeout Enforcement Evidence
=== Demonstration: Lane per-script timeout enforcement === Running hang.test.sh under --script-timeout 3 followed by after.test.sh FM_TEST_BEGIN 2026-08-19T18:03:19Z /tmp/demo-lane-timeout.vyjOLM/hang.test.sh family=unclassified expected_gate_skip=none ok - script started and is now hanging... fm-test-run: per-script budget of 3s exceeded, terminating: /tmp/demo-lane-timeout.vyjOLM/hang.test.sh not ok - /tmp/demo-lane-timeout.vyjOLM/hang.test.sh exceeded the per-script budget of 3s and was terminated fm-test-run: reaping processes /tmp/demo-lane-timeout.vyjOLM/hang.test.sh left behind (group 54590) FM_TEST_LEAK /tmp/demo-lane-timeout.vyjOLM/hang.test.sh pgid=54590 FM_TEST_END 2026-08-19T18:03:22Z /tmp/demo-lane-timeout.vyjOLM/hang.test.sh exit=124 duration_ms=2917 gate_skip=false FM_TEST_BEGIN 2026-08-19T18:03:22Z /tmp/demo-lane-timeout.vyjOLM/after.test.sh family=unclassified expected_gate_skip=none ok - lane successfully proceeded to subsequent script FM_TEST_END 2026-08-19T18:03:22Z /tmp/demo-lane-timeout.vyjOLM/after.test.sh exit=0 duration_ms=167 gate_skip=false FM_TEST_SUMMARY total=2 failed=1 skipped_gate=0 duration_ms=3305Evidence: Test Helper Automatic Reaping on Failure Evidence
Source: Test Helper Automatic Reaping on Failure Evidence
=== Demonstration: Test helper automatic reaping on early failure === Executing test script that spawns background child and fails early... Spawning background child that ignores SIGTERM... Failing assertion deliberately before explicit reap... not ok - assertion failed as part of demonstration Checking if tracked child PID 57530 is still running: SUCCESS: Child process 57530 was cleanly reaped by tests/lib.sh EXIT trap.Evidence: Targeted Test Suite Run Log
Source: Targeted Test Suite Run Log
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
⏭️ **Rebase** - skipped
.agents/skills/afk/SKILL.md- branch carries 59 commit(s) that exist on your local main branch but were never pushed to origin/main; rebasing would bundle this unrelated work (93 file(s)) into the PR:Push main to origin, or rebase your branch onto origin/main, before gating.
🔧 **Review** - 3 issues found → auto-fixed (2) ✅
bin/fm-test-run.sh:1757- The serial path startsstream_growing_file "$out" "$stop" &as a lane-owned background job that inherits the lane's stdout, and it exits only when the stop flag file appears under $RUN_TMP. The lane installs no INT/TERM trap; its only trap istrap 'rm -rf "$RUN_TMP"' EXIT(bin/fm-test-run.sh:1662). Failing sequence: runbin/fm-test-run.sh --lane portable-serial-3of4 | tee lane.logand send SIGINT to the foreground group (Ctrl-C, or a supervisor). Job control is off in the lane's main shell at that point (probe_group_reaping restoresset +m, and run_script_contained'sset -mis confined to a command-substitution subshell), so per POSIX bash sets SIGINT to SIG_IGN for the asynchronous follower: the follower survives. The lane exits, its EXIT trap deletes $RUN_TMP, so$stopcan never be created and[ -e "$stop" ]is false forever. The follower loops onsleep 0.05indefinitely while still holding the lane's stdout pipe, so the outerteenever sees EOF - the exact silent indefinite hang this change exists to remove, with the lane's own child in place of a test's orphan. Separately, becauseset -mputs the running test script in its own process group, the same SIGINT does not reach it either, so it is orphaned too. Fix: extend the trap to INT and TERM, kill the follower by the PID the lane already holds and reap the in-flight script's process group before removing $RUN_TMP, and make stream_growing_file return when its input file no longer exists.tests/fm-watcher-lock.test.sh:1079- test_pid_identity_sampling_waits_for_execve widens the fork-to-exec window with a wall-clock timer (bash -c 'sleep 0.3; exec sleep 300' &, line 1059) and then demands[ "$diverged" -eq 5 ]- every one of 5 iterations must differ. Theearlysample is not free: it forks a fresh bash, sources bin/fm-wake-lib.sh (which itself forksunameat line 15 and runsmkdir -p "$STATE"at line 16), then forksodto hex the cmdline. Failing sequence: on a cold or heavily loaded runner - the same machine class the intent describes, where a lane sat for 7h23m - one iteration's early sample takes longer than 0.3s, lands after execve, soearlyequalssettled,divergedbecomes 4, and the case fails with "the widened exec window stopped diverging". That is a new random red in the change whose stated purpose is to remove random reds. A deterministic widening keeps the 5/5 strictness: have the child block on a gate file (bash -c 'while [ ! -e "$1" ]; do sleep 0.05; done; exec sleep 300' _ "$gate" &) and have the test create that gate only after taking the early sample, so the pre-exec window is closed by the test rather than by a timer.tests/lib.sh:265- fm_test_reap_tracked_pids only reaps a registered PID that is also in this shell's job table (jobs -rp/jobs -spwritten to $live, thengrep -qx). The comment presents that solely as a PID-recycling guard, but it also discards every legitimately registered PID that is not one of the calling shell's own jobs - for example a watcher started by bin/fm-watch-arm.sh, which is a grandchild of the test shell and is exactly the process type in the reported leak (see the arm-startedwatcher_pidat tests/fm-watcher-lock.test.sh:955). Today every fm_test_track_pid caller in the two changed test files passes a direct$!job, so nothing regresses now, and the grandchild case happens to be covered by fm_test_reap_pid's descendant snapshot while the arm is still alive. The risk is the contract: CONTRIBUTING.md now instructs contributors to register PIDs with fm_test_track_pid, and a contributor who registers a grandchild PID gets a silent no-op with no diagnostic. Either document the job-table restriction at fm_test_track_pid, or keep the recycling guard while also accepting a PID that is still a descendant of this shell in the live ps parent/child graph (fm_test_descendant_pids already computes that), which carries no recycling risk.🔧 Fix: reap the lane's follower and script on interrupt
5 issues (2 warnings, 3 infos) still open:
bin/fm-test-run.sh:312-inflight="$LANE_INFLIGHT_DIR/$BASHPID"reads BASHPID unguarded while the script runs underset -eu(bin/fm-test-run.sh:80). BASHPID does not exist before Bash 4.0. Every other reference in this repo guards it - bin/fm-watch.sh:790, bin/fm-watch-arm.sh:86, bin/fm-spawn.sh:2793, bin/fm-remote-job-worker.sh:73 and ~15 more all write${BASHPID:-$$}- and .github/workflows/ci.yml:350 maintains an explicitmacos-stock-bashjob pinned to Bash 3.2.57, so 3.2 is a live compatibility target rather than a hypothetical. Failing sequence: on a macOS box whereenv bashresolves to /bin/bash 3.2,bin/fm-test-run.sh tests/anything.test.shreaches run_script_contained, expands$BASHPID, and dies with "BASHPID: unbound variable" before the first script's status is written - the runner is unusable, not degraded. Nothing else in bin/fm-test-run.sh requires Bash 4 (nodeclare -A,mapfile,${var,,}), so this single expansion is what drops support. Fix by matching the repo convention, but not naively:$$is identical in every parallel worker subshell, so${BASHPID:-$$}alone would make all --jobs>1 workers write the same inflight file and lane_abort would reap only the last one. Key the file on the started script's PID (available as$pidimmediately after the&), which is unique per worker on every Bash version.tests/fm-watcher-lock.test.sh:711- The user intent marks as REQUIRED: "Do not leak the process. Every watcher a test starts is reaped by that test before it exits, verified by pid." Three cases in tests/fm-watcher-lock.test.sh - one of the two files the intent puts in scope - start a watcher through bin/fm-watch-arm.sh and neither register it with fm_test_track_pid nor verify it is gone. This is precisely the process shape in the reported leak: docs/verification/test-lane-safety.md records the frozen tree as lane -> test script -> fm-watch-arm.sh -> fm-watch.sh -> sleep.Line 711
kill "$armpid" "$lock_pid" 2>/dev/null || truein test_arm_starts_and_self_heals sends a single unescalated TERM to$lock_pid, the watcher whose liveness the test just asserted at line 710, then waits only on$armpid. The same doc's central finding is that a watcher's TERM can be swallowed when its trap action fails to re-parse at delivery, which is why every other reap in this change escalates to KILL.Line 966
kill -TERM "$watcher_pid"in test_stopped_watcher_is_live_but_stale_then_exit_is_classified has the same shape.test_arm_self_eviction_is_loud_without_successor captures
watcher_pidat line 563 and never signals or verifies it at all, relying entirely on the watcher self-evicting.Why the registry does not cover them:
$lock_pidand$watcher_pidare grandchildren of the test shell and are never passed to fm_test_track_pid, so fm_test_reap_tracked_pids never iterates them. They would only be caught as a snapshot descendant inside fm_test_reap_pid("$armpid"), and in all three cases the arm has already exited by then, so the snapshot is empty and a surviving watcher is an orphan of init that nothing reaps.This is survivable, not a hang: the lane's process-group reap in run_script_contained catches it and prints FM_TEST_LEAK. But the intent states the four requirements explicitly do not substitute for each other, so requirement 2 covering requirement 1 is not acceptance. Raising this rather than fixing it because deciding whether these three cases should register the lock pid, assert it exited, or are deliberately left to the lane backstop is the author's scope call.
tests/fm-test-run.test.sh:762- test_lane_reaps_a_leaked_child_and_keeps_going assertsgrep -Fq 'exit=0' "$out"with the message "the leaking script's own result was not preserved", but$outholds FM_TEST_END markers for both fixtures and the second one,$tmp/after.test.sh, always exits 0. Failing sequence: change run_script_contained so a reported leak overwrites the script's own status (for examplerc=1alongside the FM_TEST_LEAK line), and the leaky script's marker becomesexit=1while after.test.sh still emitsexit=0- the grep still matches and the case still passes, so it cannot detect the regression it names. This is the one behavior in that test not pinned by another assertion; the FM_TEST_LEAK grep on line 760 is already correctly scoped to the script path. Anchor this one the same way, e.g.grep -Eq "FM_TEST_END .* ${tmp}/leaky\\.test\\.sh exit=0 ".bin/fm-test-run.sh:323- kill_pid_hard (bin/fm-test-run.sh:283) sends SIGKILL as its last statement and returns without confirming the process died, and the budget path at line 323 then does an unboundedwait "$pid". The comment on REAP_GRACE_TICKS at line 111 says the grace applies "between TERM and KILL, and again after KILL", but only reap_process_group implements the second wait; kill_pid_hard does not. The sibling helper in this same change, fm_test_signal_pid_hard (tests/lib.sh:210), does implement the post-KILL wait and returns whether the process is actually gone - so the two reapers in one change disagree on the same contract. Concretely: a script wedged in uninterruptible sleep (D state on a stalled mount) survives SIGKILL until the I/O returns, and the lane blocks inwaitwith no bound and no diagnostic - the failure mode the whole change exists to eliminate, now inside the timeout handler itself. This is the least reachable of the four findings and the budget already fires first, so it is hardening rather than a live defect: give kill_pid_hard the post-KILL confirmation loop it documents, and have run_script_contained report "could not reap" and move on instead of waiting when it comes back false.tests/lib.sh:165- fm_test_reap_tracked_pids only ever runs from fm_test_cleanup, which is reached through thetrap fm_test_cleanup EXITarmed at tests/lib.sh:294. Roughly 40 test files replace that trap with their own EXIT handler and never call fm_test_cleanup - tests/fm-control.test.sh, tests/fm-kimi-harness.test.sh, tests/fm-agy-harness.test.sh, tests/fm-cursor-harness.test.sh and tests/fm-procevent.test.sh among them. In any of those, fm_test_track_pid appends to the registry and nothing ever reads it: no reap, no warning, no diagnostic. Nothing regresses today, because only the two watcher files call fm_test_track_pid and neither overrides the trap (I checked). The new exposure is the contract: CONTRIBUTING.md:100 now tells every contributor to register PIDs with fm_test_track_pid, and a contributor who adds a background process to one of those ~40 files gets a silent no-op and reintroduces exactly the orphan this change removed. The library comment at tests/lib.sh:57 already names the rule ("define its own EXIT trap and call fm_test_cleanup from inside it") but fm_test_track_pid's own doc block does not repeat it and the CONTRIBUTING sentence does not mention it at all. Cheapest close: state the dependency at fm_test_track_pid and in the CONTRIBUTING sentence that introduces it.🔧 Fix: drop BASHPID, bound the post-KILL wait, reap arm watchers
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-test-run.test.shbin/fm-test-run.sh tests/fm-watcher-lock.test.shbin/fm-test-run.sh tests/fm-watch-triage.test.shbin/fm-test-run.sh tests/fm-test-run.test.sh tests/fm-watcher-lock.test.sh tests/fm-watch-triage.test.shEmpirical fork-to-exec settling demonstration comparing immediate sampling (20/20 failed) againstfm_test_wait_exec_settled(20/20 passed)Empirical lane leak containment demonstration verifying process-group reaping, follower decoupling,FM_TEST_LEAKreporting, and unblocked lane progressionEmpirical per-script timeout demonstration verifying termination of hung scripts, explicit naming,exit=124reporting, and continued suite progressEmpirical test helper reap verification ensuringtests/lib.shPID tracking unconditionally cleans up background processes on early assertion failures✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.