Conversation
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up review scope. No blocking failure remains. Reviews (2): Last reviewed commit: "no-mistakes(document): Document liveness..." | Re-trigger Greptile |
|
Hi maintainers, this PR has been refreshed onto the current upstream main. GitHub is holding its workflow runs for maintainer approval. When convenient, could you please approve them so CI can run?\n\n- CI: https://github.com/kunchenguid/firstmate/actions/runs/32524342928\n- Required validation: https://github.com/kunchenguid/firstmate/actions/runs/32524342946 |
|
Speaking as Kun's firstmate: VISION verdict: align. A confirmed-dead harness must not keep reporting working from a frozen busy hook or a stale status line. That is honest interface under load, and it reuses the existing recovery-grade classifier instead of mixing a new heuristic into scripts. Class: corrective. Reviewed the full diff vs base. Security: no. Waiting on CI including no-mistakes, not on the captain. Not merge-eligible yet. |
…fallback Consult the existing recovery-grade fm_backend_agent_state classifier before trusting a frozen busy-hook record or a stale status-log line, and broaden the fleet-snapshot heartbeat liveness check beyond secondmates. Only a confident dead/missing verdict changes behavior; every inconclusive verdict and every backend without a classifier falls through unchanged.
…d panes The raw event-stream drain in fm_backend_herdr_wait_transition forwards whatever pane_id the reader delivers with no cross-check against the caller's own subscribed window list. A torn-down task's former pane, or its surviving husk shell, could therefore keep delivering agent-status edges long after teardown removed its state/<id>.meta, each one waking firstmate with nothing actionable to act on. handle_push_transition now requires the window to match a currently-recorded task before treating an edge as actionable; an unrecorded pane's transition is committed (so it is never re-evaluated as fresh) and absorbed silently. A live, recorded task's detection is untouched.
…ws fallback Broadening fm-fleet-snapshot.sh's agent liveness check (previous commit) now calls the recovery-grade tmux classifier for every kind, not just secondmates. This fixture's fake tmux had no list-windows handler, so the classifier's session-inventory read silently succeeded with empty output and was read as a confident "missing" endpoint, turning an expected endpoint.status of "unknown" into a false "dead". Fail list-windows generically instead, matching real tmux's behavior for an unconfigured session, so the classifier reads unreadable and the existing assertion holds.
handle_push_transition now also skips a pane no task currently records, not just secondmate endpoints and declared pauses - keep the exemption list here accurate to match.
…ate pre-check before busy state
…ntory The review-fix hardening of fm_backend_target_exists routes the labelled tmux presence read through `tmux list-windows` instead of `display-message`, because real tmux answers an absent named target from the client's active window and so cannot prove a window exists. fm_backend_tmux_agent_state already read presence through that same call. This fixture's fake tmux only modelled its presence rule in the display-message arm; its list-windows arm failed generically, which after the hardening reported every fixture endpoint absent and broke the child-endpoint freshness assertion. Model the rule once, in the inventory the production code actually consults: list the windows the fixture homes record for the requested session, minus the dead-* targets the display-message arm already reports absent. An omitted window is then honestly absent, so the dead-* endpoints stay unhealthy.
Intent
Stop firstmate from reporting stale liveness for a crew whose harness process has died but whose pane or shell still answers, so a dead worker surfaces as unknown instead of reading 'working' forever (upstream issue #3402: the session-start fleet digest printed 'endpoint: alive' for tmux windows that did not exist).
In bin/fm-crew-state.sh's no-run fallback, consult the recovery-grade fm_backend_agent_state classifier (owned by bin/fm-backend.sh) ahead of both the busy-hook read and the status-log fallback. Only a CONFIDENT dead/missing verdict changes behavior, reporting 'unknown / agent-state'; unverified, unreadable, and ambiguous verdicts must fall through byte-identical to the previous behavior, because a false 'crew is dead' would trigger recovery against a live worker, which is worse than the silent-working gap being closed. Reuse the existing single-owner classifier rather than forking a second one.
A TERMINAL done/failed status-log record is exempt from that dead verdict and is still reported as that terminal state, because a recorded outcome is not a liveness claim a dead harness can withdraw. Without the exemption, a finished single-owner task whose harness exited while its pane survived would lose its completion signal and have its already-answered keyed decision re-opened by bin/fm-fleet-snapshot.sh's lifecycle reconciliation. Only non-terminal readings (working/needs-decision/blocked/paused) are the stale-liveness case this check exists to suppress.
Also populate endpoint.agent_alive for every task with a recorded target in the fleet snapshot, and stop the herdr push path from waking firstmate for retired panes that no task currently records, committing that transition so the dedupe marker still advances and then absorbing it rather than misrouting it to the wrong crew.
Delivery constraints: this finishes the EXISTING open PR #2501 from its preserved, custody-returned branch fm/fm-liveness. Preserve every commit already on this branch, including the earlier accepted review corrections and the two recovered review fixes (the terminal-record exemption and its crew-state contract-header documentation); never reset, rebase away, replace, squash, or drop any preserved commit. Rebase onto exactly upstream kunchenguid/firstmate main 4ad8cba, which the primary local default branch and the SimTet fork default branch have both already been fast-forwarded to. Two files overlap that upstream range and need conflict resolution that keeps BOTH sides' intent rather than dropping preserved fixes: docs/architecture.md and tests/fm-session-start.test.sh. Update ONLY the existing PR 2501 and its existing fork branch. Never merge the PR, never force-push, never push to the kunchenguid upstream, never create a duplicate PR, and never discard preserved work.
Repo conventions that apply: bin/*.sh must pass bin/fm-lint.sh (pinned shellcheck), tracked Markdown uses one full sentence per line with plain dashes and no em dash, tests are colocated in tests/ as .test.sh and must exercise behavior through an executable interface rather than asserting implementation source bytes, and no agent name may be added as a commit co-author. Note that tests/fm-session-start.test.sh has one pre-existing environment-dependent failure ('MISSING diagnostic did not appear at all') that also fails on the base commit and is not caused by this branch.
What Changed
Risk Assessment
✅ Low: The behavior change is narrowly gated - only a confident dead/missing verdict acts, every other verdict falls through untouched, and the tmux primitive's stricter path is opt-in via an expected label that matches the window convention fm-spawn.sh enforces - with direct regression coverage including byte-identical fallthrough proofs, and the only findings are informational.
Testing
Baseline checks reproduced the stale working verdict and previously isolated the session-start fixture mismatch; all focused crew-state, fleet, Bearings, supervision-event, and node-free session-start tests now pass, and the CLI transcript confirms dead workers become unknown, terminal completion survives, endpoint liveness is populated, and retired Herdr panes are absorbed without waking firstmate while advancing dedupe.
Evidence: End-to-end firstmate liveness CLI transcript
Source: End-to-end firstmate liveness CLI transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-backend.sh:831- fm_backend_target_exists' contract header still describes the tmux arm as adisplay-messageprobe that "Mirrors fm-crew-state.sh's pane_readable check". That is now only true for the label-less form: with an expected label the tmux arm reads the session inventory (list-windows -F '#{window_name}') and, when the target's window part is not exactly the label, returns absent without querying tmux at all. Since this header is the shared primitive's contract for callers that pass a label (fm-session-start.sh:825, fm-fleet-snapshot.sh:556, fm-control.sh:418) and for those that deliberately do not (fm-supervise-daemon.sh, fm-send.sh), document the label-gated rule and the tmux active-window fallback it exists to defeat.bin/fm-fleet-snapshot.sh:561- Every task with a target now pays two endpoint probes per snapshot: fm_backend_target_exists plus fm_backend_agent_alive. For tmux that is twolist-windowsinventory reads; for herdr it is three CLI round-trips (pane get, then pane get + agent get) instead of one. fm_backend_agent_state'smissingverdict already subsumes presence, but collapsing the two is not behavior-preserving (agent_state'sunreadablewould have to map to endpoint_exists=null, not false), so this is a cost note rather than a required change. Note also that, unlike the remote-secondmate arm above, this call is not wrapped in fm_run_timed - though fm_backend_target_exists already carried the same unbounded exposure for every task.bin/fm-push-transition-lib.sh:158- The retired-pane absorb commits the herdr dedupe marker, so a blocked edge that arrives in the window between endpoint creation (fm-spawn.sh:2248) and the state/<id>.meta write (fm-spawn.sh:2856) is absorbed and never re-delivered on the push path; the marker clears only on a laterworkingedge. The poll loop is unaffected (it uses .stale-<key>, not .herdr-escalated-<key>) and remains the documented permanent backstop, and committing the transition is exactly what the stated intent requires, so this is recorded as a tradeoff rather than a defect.🔧 **Test** - 1 issue found → auto-fixed ✅
tests/fm-session-start.test.sh:1320- tests/fm-session-start.test.sh::test_endpoint_liveness_tmux failed on the branch (base 4ad8cba passes it). The fixture recordedwindow=fm-sess:live-windowfor task idtask-live, but the branch's new label form of fm_backend_target_exists requires the tmux window name to be the expectedfm-<id>label, so the live endpoint was reported dead. It was masked locally because this machine has /usr/bin/node, so the file aborts earlier at the pre-existing environment-dependentMISSING: nodeassertion; run with the suite's own FM_TEST_BASE_PATH knob pointed at a node-free PATH (the shape CI has), base reaches the test and passes 49/49 while the branch failed. Fixed by giving the fixture production naming (fm-sess:fm-task-live / fm-sess:fm-task-dead), matching bin/fm-spawn.sh's W="fm-$ID" and fm_backend_validate_task_endpoint's stated tmux record rule; assertions otherwise unchanged. The file now passes 49/49 on the branch, and the same fixed fixture also passes 49/49 against base code.bin/fm-test-run.sh tests/fm-crew-state.test.sh tests/fm-supervision-events.test.sh tests/fm-fleet-snapshot-view.test.sh(all pass, including the seven new agent-state cases and the terminal-record exemption)bin/fm-test-run.sh tests/fm-backend.test.sh tests/fm-tmux-agent-liveness.test.sh tests/fm-bearings-snapshot.test.sh tests/fm-secondmate-liveness.test.sh tests/fm-session-start.test.shbin/fm-test-run.sh tests/fm-busy-state.test.sh tests/fm-backend-orca.test.sh tests/fm-teardown-endpoint-safety.test.sh tests/fm-daemon.test.sh(the other fm_backend_target_exists callers)FM_TEST_BASE_PATH=<node-free mirror of /usr/bin:/bin:/usr/sbin:/sbin> bash tests/fm-session-start.test.shon base 4ad8cba (49 ok), on branch 379b683 (failed at test_endpoint_liveness_tmux), on branch after the fixture fix (49 ok), and base code with the fixed fixture (49 ok)Confirmed theMISSING diagnostic did not appear at allfailure is pre-existing by running tests/fm-session-start.test.sh from a pristinegit archive 4ad8cbacheckout (same failure, same line)Manual end-to-end: real tmux 3.2a on an isolated socket with windows fm-live-crew (real verified-harness process), fm-dead-crew and fm-done-crew (husk shell only), plus ghost-crew whose recorded window does not exist; ranbin/fm-crew-state.sh <id>for all four on base and branchManual end-to-end:bin/fm-fleet-view.shagainst the same FM_HOME on base and branch (Under Way table, Current and Endpoint columns)Manual end-to-end: fullbin/fm-session-start.shrun on base and branch against the same FM_HOME and live tmux server, comparing the FLEET STATEendpoint:lines (issue #3402 reproduction)Manual: drovehandle_push_transition herdr default <blocked record>for a pane with no recorded task on base and branch, plus a live-recorded-task control, capturing the wake queue, watcher triage log and herdr dedupe markerProbedfm_backend_target_exists tmux fm-sess:live-window fm-task-liveon base vs branch to isolate the label-strictness behaviour change🔧 Fix: use production fm-<id> window names in session-start tmux fixture
✅ Re-checked - no issues remain.
Against base4ad8cba, extracted the tree withgit archiveand ran onlytest_fast_path_confirmed_dead_agent_overrides_frozen_busy_hook; it reproduced the stalestate: working · source: paneregression.bash tests/fm-crew-state.test.shbash tests/fm-fleet-snapshot-view.test.shbash tests/fm-supervision-events.test.shbash tests/fm-bearings-snapshot.test.shFM_TEST_BASE_PATH="$PWD/.no-mistakes-test-bin" bash tests/fm-session-start.test.shusing a node-free PATH to avoid the documented environment-dependent failure.Manually invokedfm-crew-state.sh,fm-fleet-view.sh, andfm-fleet-snapshot.sh --json, then exercised a retired Herdr push transition and recorded its wake, dedupe, and triage state.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.