feat(bin): close exited ship/scout panes as ordinary cleanup - #2970
feat(bin): close exited ship/scout panes as ordinary cleanup#2970nbost130 wants to merge 9 commits into
Conversation
A finished worker that has already quit left an empty pane until merge teardown. Close that pane as ordinary cleanup once the agent is confirmed gone, keep the isolated copy, and skip stale/missing-endpoint recovery until landed teardown returns the copy.
Confidence Score: 4/5The PR is not yet safe to merge because sufficiently long wrapped pending input can still be mistaken for an empty prompt and discarded when its pane closes. The joined-capture fix only examines the final 20 physical rows, allowing a longer command to lose its original prompt glyph at the capture boundary and leave a tail that passes the pane-close emptiness proof. Files Needing Attention: bin/fm-teardown.sh, bin/fm-composer-lib.sh, tests/fm-teardown-close-pane.test.sh Reviews (8): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Speaking as Kun's firstmate: Scheduled 11:10am PT 8/24 pass. VISION.md read in full from current main VISION (inspected Class: opt-in. Watcher auto-close is gated. Security: none. No workflow-file / secret / injection. Herdr close refuses the captain's active tab and unverified backends. Composer pending/unproven and classifier failure block close (fail-closed). Greptile still flags Herdr wrapped-row capture (tmux got Overlap / HOLD: CI / NM: HEAD Workflows approved this pass: CI 32757973617, Require no-mistakes 32757973668. Not green at comment time. Land-eligible rec: NO (standing teardown-hold overlap; NM attestation mismatch; CI not yet green). Captain-flag NOW: no. Waiting-on-author to restamp no-mistakes for THIS HEAD. Not a captain-decision hold on class (opt-in is fine). Standing holds still prevent auto-merge. |
|
Speaking as Kun's firstmate: first look. First-time fork CI reviewed and approved this pass (no class=opt-in. VISION.md: authority aligns (explicit grant). Honest interface aligns (empty leftover panes are not news). Scripts align. Restart aligns. Spine aligns. Vendor mixed (composer empty-proof). Scope aligns. This HEAD: Hold files: Waiting on author for a HEAD-matching attestation. The file-overlap hold is coordinator, not a captain ping. |
| # time and could pass it wrongly (Greptile P1: task fm-close-exited-panes | ||
| # review - Herdr reaches this same fallback through an unjoined capture). | ||
| fm_backend_capture_joined_reliable "$BACKEND" || return 0 | ||
| cap=$(fm_backend_capture_joined "$BACKEND" "$T" "${FM_COMPOSER_CAPTURE_LINES:-20}" 2>/dev/null) || return 0 |
There was a problem hiding this comment.
When an exited worker has an unsubmitted command spanning more than 20 physical terminal rows, the bounded joined capture omits the original prompt and can leave a tail ending in one shell glyph. The fallback then treats that truncated tail as an empty prompt and closes the pane, discarding the unsubmitted command.
2 participants
Intent
Close a finished worker's pane as ordinary cleanup once the agent has actually exited, even if the PR is still open.
Today cleanup stops the agent and, after a merge, returns the copy. It does not close a finished worker that already quit while its PR is still open. The empty pane keeps alarming. Current leftover panes were already closed by hand; this ship makes pane-close ordinary going forward.
Acceptance:
What Changed
bin/fm-teardown.shgains a--close-panemode: ordinary cleanup that closes a finished (PR recorded,done:/failed:status, or scoutreport.md) ship or scout worker's backend pane once the recovery-grade agent-state classifier confirms the agent has actually exited. It refuses on a live or unreadable agent state, a pending composer, secondmate tasks, unverified backends (e.g. orca), or a Herdr pane that is the captain's active tab; on success it recordspane_closed=1on the task meta without returning the isolated copy or discarding any work, and--close-pane/--forcecannot be combined.bin/fm-watch.sh's watch loop now auto-invokesfm-teardown.sh --close-panefor finished ship/scout windows whose agent state isdead/missing, skipping the normal stale-window handling for them; bothfm-watch.sh(recorded_windows) andbin/fm-session-start.shtreat panes withpane_closed=1as already closed instead of probing them or raising stale/missing-endpoint alarms.tests/fm-teardown-close-pane.test.sh, updatestests/fm-session-start.test.shandbin/fm-test-run.shtest routing, and updates docs (AGENTS.md,docs/agent-control.md,docs/architecture.md,docs/configuration.md,docs/scripts.md,.agents/skills/stuck-crewmate-recovery/SKILL.md) to describe the new cleanup path.Risk Assessment
Testing
The new fm-teardown.sh --close-pane behavior is directly and thoroughly demonstrated end-to-end via real tmux panes in tests/fm-teardown-close-pane.test.sh, all 5 cases passing and matching every acceptance criterion (exited+PR-open closes/retains copy, live agent keeps pane, unfinished exit keeps pane, teardown-after-merge still returns copy, watcher skips closed panes). While validating the companion fm-session-start.test.sh coverage, I found the new test_endpoint_liveness_closed_pane itself was broken (missing required argument to a helper, crashing under set -u) and fixed it with a one-line argument addition; it now passes both in isolation and as part of the full file, alongside all other session-start tests once two unrelated pre-existing environment failures (verified present on the base commit too) are set aside.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-teardown.sh:2445- teardown_composer_blocks_pane_close only refuses to close the pane when fm_backend_composer_state returns pending or pending-unproven, letting anunknownverdict fall through and permit the close. Everywhere else this classifier is consumed (fm_pane_input_pending in bin/fm-tmux-lib.sh:194-196, and the classifier's own contract comment 'positive proof required for empty ... unrecognized future verdicts failing safe'),unknownis treated as not-provably-empty and blocks the action. fm_tmux_composer_state legitimately returnsunknownon a transient cursor-row or capture read failure (bin/fm-tmux-lib.sh:141-143), which can coincide with an agent_state=dead pane (agent process exited, shell still up) — exactly the situation --close-pane targets. In that case the pane closes without ever proving the composer was empty, which can discard unsubmitted composer text and contradicts the stated acceptance criterion 'Do not close a pane that ... has a pending composer that is not empty.' Recommend failing closed: block the close on any verdict other than the provenempty(i.e.case "$verdict" in empty) return 1 ;; *) return 0 ;; esac), matching the fail-safe convention already established for this same classifier elsewhere in the codebase.tests/fm-session-start.test.sh:1352- Fixed a real test bug in the newly added test_endpoint_liveness_closed_pane (tests/fm-session-start.test.sh): it called make_fake_tmux "$fakebin" with only one argument, but that helper requires a second (live-target) argument, and the file runs underset -u, so the test crashed with "$2: unbound variable" before any assertions ran. Fixed by passing a placeholder live-target string; harmless since pane_closed=1 short-circuits before tmux liveness is ever checked. Verified the fix in isolation and against the full file with two confirmed pre-existing/environment-only failures (test_output_ordering_diagnostics_lead, test_composition_invokes_real_scripts's node-detection line) skipped, reproducing identically on the base commit — confirming they predate this change.bash tests/fm-teardown-close-pane.test.sh (real tmux-backed behavioral coverage: exited+PR-open closes pane/retains copy, live agent keeps pane, unfinished exit keeps pane, teardown-after-close still returns copy, recorded_windows skips closed panes)isolated invocation of test_endpoint_liveness_closed_pane from tests/fm-session-start.test.sh (endpoint: closed reporting for a pane_closed meta) before and after the test-bug fixfull tests/fm-session-start.test.sh with two confirmed pre-existing/environment-only failing tests skipped, to confirm all other 99 tests (including the new one) pass cleanlycross-check against base commit 038d0f7 to confirm the two skipped failures (test_output_ordering_diagnostics_lead, node-detection line in test_composition_invokes_real_scripts) are pre-existing and unrelated to this changedocs/herdr-backend.md:124- docs/herdr-backend.md documents that ordinary (non-projected) full teardown performs a plain close even on the captain's active tab, while the new --close-pane path refuses to close on the captain's active tab unconditionally. This isn't a contradiction (herdr-backend.md describes only the pre-existing full-teardown/projected-cleanup paths, which are unchanged), and the new invariant is already fully owned by fm-teardown.sh's header comment per the repo's mechanics-ownership tier. Flagging only as a judgment call in case a future consolidation of Herdr close-contract documentation is wanted.✅ **Push** - passed
✅ No issues found.