fix(bin): recover legacy endpoint bindings instead of refusing lifecycle actions - #2675
Closed
doitdigital0495 wants to merge 4 commits into
Closed
doitdigital0495 wants to merge 4 commits into
doitdigital0495 wants to merge 4 commits into
Conversation
A task record written before endpoint_task_id existed (bin/fm-spawn.sh gained it in kunchenguid#1171) was refused outright by fm_backend_validate_task_endpoint on every opaque-id backend. For a live secondmate on herdr that meant both supported lifecycle verbs refused, and the only way to restart the agent was a hand-rolled kill and respawn - exactly the improvised lifecycle handling the control plane exists to remove. The refusal was right that such a record proves nothing offline: herdr, zellij, and cmux record opaque runtime ids that carry no task label, so nothing in the record binds it to its task. It was wrong to leave no recoverable outcome. What those backends do have is a label. fm-spawn creates each task's tab or workspace as fm-<id> and nothing renames it, so the binding is readable from live state even when the record predates the metadata field - the same fact tmux and Orca get for free from their recorded window name, read from the runtime instead of from the record. - fm_backend_validate_task_endpoint now returns 2, not 1, for the one recoverable shape: a record consistent in every other respect whose only missing fact is the binding. Every existing caller treats nonzero as a refusal and is unaffected. - fm_backend_endpoint_label_provable proves the whole recorded chain against live state, not just the label. For herdr that is workspace -> pane -> owning tab -> label, so a re-parented pane, a relabeled tab, a duplicated label, a missing pane, or an unreachable server all refuse. Zellij reuses its existing tab matcher; cmux gets an unambiguous workspace matcher. - fm_backend_resolve_task_endpoint records a proven binding and revalidates, so the next call takes the ordinary offline path. It never rewrites an existing binding, right or wrong. - fm-control.sh and fm-teardown.sh use the resolver. The runtime read is read-only and happens before any mutation. - Orca no longer requires the binding at all: its recorded window is already fm-<id>, so a legacy Orca record was never ambiguous. Identity guarantee: unchanged in strength. Acceptance still requires the recorded endpoint to carry the task's own fm-<id> label; the only change is that on these backends the label is read from the live runtime rather than from the record. That is the same test the already-accepted legacy tmux records pass, against a live source, and it has the same residual gap - two homes sharing one session and one task id.
…or legacy re-derivation
Author
|
Closing: firstmate fixes stay in the fork, not upstream. A PR here cannot run its checks (pull-only rights), so this was reopened against doitdigital0495/firstmate main instead. No review action needed. |
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
Let a second mate with legacy endpoint metadata be relaunched, instead of refusing.
WHAT HAPPENED (2026-08-20). A live second mate (rentmax-mate, Herdr backend) needed restarting. Both supported lifecycle verbs refused: 'bin/fm-control.sh rentmax-mate relaunch' and 'bin/fm-control.sh rentmax-mate exit' both printed 'REFUSED: legacy Herdr endpoint metadata for task rentmax-mate lacks an exact task binding; preserving task state.' Its state/rentmax-mate.meta carried window=personal:wW:p2 plus herdr_session / herdr_workspace_id / herdr_tab_id / herdr_pane_id, but not the newer binding the control plane requires. The refusal is correct in spirit - the plane must not act on an endpoint it cannot prove is the right one - but the outcome was that firstmate had to stop the agent by hand with kill and respawn it, which is exactly the improvised lifecycle handling the control plane exists to remove.
WHAT WAS ASKED FOR. Make a legacy-metadata direct report relaunchable through the supported path, WITHOUT weakening the identity guarantee that makes the refusal correct. Establish first, from the code rather than assumption: what exact binding the plane requires today and which producer writes it; what a legacy record has instead; and whether the legacy fields identify the endpoint UNAMBIGUOUSLY or only probably. Then pick the honest fix: if the legacy fields DO identify the endpoint unambiguously, accept them - ideally upgrading the record to the current shape as it goes, so the next call takes the normal path; if they genuinely do not, then a blanket refusal is wrong in a different way because it leaves the operator with no supported action at all, so give the plane a way to re-derive or re-bind the identity, and refuse only when even that cannot be proven. Explicit constraint: do NOT weaken the check merely to make the error go away - a plane that acts on the wrong pane is far worse than one that refuses. Cover it with a test that reproduces the exact refusal against a legacy-shaped record and proves the new path both works and still refuses a genuinely unidentifiable endpoint. Also consider whether the same gap exists for the tmux backend and for ordinary crewmates, not just Herdr second mates, and report what is found either way. Definition of done includes a clear statement of what identity guarantee still holds.
WHAT THE INVESTIGATION ESTABLISHED FROM THE CODE. endpoint_task_id is written by bin/fm-spawn.sh as the self-referential line endpoint_task_id=$ID (fm-spawn.sh lines 612 and 2643), added in commit fbece9c (#1171, 2026-07-28). Its only real guarantee is detecting a record whose CONTENT was authored for a different task - a copied or renamed meta file - since fm_backend_validate_task_endpoint refuses when the binding names another task. tmux never needed it because its window field is session:fm-, so the record carries the task label itself and a foreign record cannot pass. Orca is structurally identical (window=fm-) yet still demanded the binding, which was redundant. Herdr, Zellij and cmux record opaque runtime ids that carry no task label, so a legacy record there genuinely has NOTHING offline that binds it to its task. Conclusion: the legacy fields do NOT identify the endpoint unambiguously offline, so the second branch of the brief applies - give the plane a way to re-derive the identity. The gap is purely temporal and applies to ordinary crewmates and scouts exactly as much as to second mates, because fm-spawn writes the binding unconditionally for every kind; only records written before #1171 on a non-tmux backend are affected.
WHAT WAS BUILT AND WHY. bin/fm-spawn.sh creates every task's tab or workspace with the caller-facing label fm- (W="fm-$ID" at fm-spawn.sh:1848, passed to each backend's create_task and to fm_backend_herdr_projection_create_task) and nothing ever renames it, so the binding is readable from LIVE state even when the record predates the metadata field. That is the same fact tmux and Orca get for free from their recorded window name, read from the runtime instead of from the record. Changes: (1) fm_backend_validate_task_endpoint now returns 2 rather than 1 for the one recoverable shape - a record internally consistent in every other respect whose only missing fact is the binding - and the per-backend binding check moved after the field-consistency checks so code 2 means only that; every existing caller treats any nonzero as a refusal and is unaffected. (2) New fm_backend_endpoint_label_provable proves the whole recorded chain against live state, not just the label: for Herdr, workspace -> pane -> owning tab -> label, via new fm_backend_herdr_tab_matches_label, so a re-parented pane, a relabeled tab, a duplicated label, a missing pane, or an unreachable server all refuse; Zellij reuses its existing fm_backend_zellij_tab_matches_label, which already handles the ambiguous legacy bare title; cmux gets a new fm_backend_cmux_workspace_matches_label that requires exactly one live workspace to carry the title and it to be the recorded one, deliberately unlike fm_backend_cmux_workspace_id_for_label which adopts the first match. (3) New fm_backend_bind_task_endpoint records a proven binding through a temp file in the same directory at mode 0600, and NEVER rewrites an existing binding, right or wrong, because rewriting one is how a wrong endpoint would get laundered into a right-looking record. (4) New fm_backend_resolve_task_endpoint composes those: on code 2 it proves, records, and revalidates from the upgraded record, so the next call takes the ordinary offline path; on failure the original refusal stands. (5) bin/fm-control.sh and bin/fm-teardown.sh use the resolver; the runtime read is read-only and happens before any mutation. Locking is deliberately the caller's responsibility because bin/fm-teardown.sh already holds the meta lock across its whole validate-and-clean sequence and locking inside the resolver would deadlock it; fm-control acquires and releases the meta lock around the call only, so it does not hold it across the fm-spawn --relaunch it later delegates to. (6) Orca no longer requires the binding at all, since its recorded window is already fm- and a legacy Orca record was never ambiguous.
DELIBERATE SCOPE DECISIONS a reviewer would otherwise question. bin/fm-spawn.sh's own --relaunch validation and bin/fm-remote-secondmate-control.sh were deliberately NOT wired to the resolver: fm-spawn --relaunch is reached through fm-control, which has already resolved and recorded the binding by then, and wiring it would require adding new lock churn around a validation that currently runs before fm-spawn takes the meta lock; fm-remote-secondmate-control validates a REMOTE endpoint record for which no local runtime exists to query, so it must keep refusing. The validator was deliberately kept offline-by-default rather than making it call the runtime itself, because bin/fm-teardown.sh depends on it deciding before any runtime command runs; the live read was put in a separate resolver instead.
IDENTITY GUARANTEE THAT STILL HOLDS, as required by the definition of done. Unchanged in strength. Acceptance still requires the recorded endpoint to carry the task's own fm- label. The only change is that on the opaque-id backends the label is read from the live runtime rather than from the record. That is the same test the already-accepted legacy tmux records pass, against a live source, and it has the same residual gap - two homes sharing one session AND one task id. An inconclusive read never licenses a rebind: an unreachable runtime, an unparseable response, a missing, moved or duplicated label, and a backend with no label to read all refuse.
TESTS. tests/fm-teardown-endpoint-safety.test.sh gained a case covering both directions: it reproduces the exact reported refusal against a legacy-shaped Herdr record (now code 2 with the same message), proves a legacy Orca record validates offline with no binding and no live read, proves recovery works and is durable (a second resolve makes zero runtime calls), proves five distinct unprovable live shapes - relabeled tab, re-parented pane, duplicated label, missing pane, unreachable server - each refuse with no binding written, and proves a record bound to ANOTHER task stays a terminal refusal with no live read attempted and its conflicting binding left exactly as found. tests/fm-backend-herdr-smoke.test.sh gained a real-binary case exercising the same proof against a live herdr server, because a canned fake can only confirm the shape the fake already assumes.
KNOWN LIMITATION, deliberately accepted and documented rather than worked around. That live smoke case has NOT been executed yet. It runs only inside a named non-default isolated Herdr lab via bin/fm-herdr-lab.sh, and on this machine that helper's fleet-state tripwire hard-requires exactly one RUNNING default session, while this machine's fleet runs named sessions (geris, personal) with default stopped. The helper failed closed, created no lab session, and left the fleet untouched. Starting the default session or relaxing the tripwire were both refused as out of scope and as weakening shared safety infrastructure. docs/verification/runtime-backends.md states honestly that the row has not been refreshed and names the live guard as the command that refreshes it.
DOCS. docs/configuration.md, docs/agent-control.md and docs/verification/runtime-backends.md were updated at their existing owning passages rather than by adding new sections, per the repo's one-owner and size-discipline rules in the firstmate-coding-guidelines skill. bin/fm-doc-audience-check.sh passes.
PRE-EXISTING FAILURES, verified by stashing the branch and re-running against clean HEAD, so they are NOT caused by this change and were deliberately left alone under firstmate's scope discipline: tests/fm-backend-cmux.test.sh 'send_text_submit should report send-failed when the target is absent, got unknown', and tests/fm-teardown.test.sh 'herdr-child-preflight: refusal did not explain its non-mutating boundary'.
What Changed
fm_backend_validate_task_endpointnow returns2(instead of1) for the one recoverable shape - a Herdr/Zellij/cmux record that is consistent in every other respect and only lacks theendpoint_task_idbinding - with the per-backend binding check moved after the field-consistency checks; Orca no longer requires the binding at all, since its recorded window is alreadyfm-<id>. Existing callers treat any nonzero as a refusal and are unaffected.bin/fm-backend.sh:fm_backend_endpoint_label_provableproves the recorded endpoint against live state (Herdr proves workspace -> pane -> owning tab -> label via the newfm_backend_herdr_tab_matches_label; cmux via the newfm_backend_cmux_workspace_matches_label, which demands exactly one live workspace carrying the title; Zellij reuses its existing tab matcher),fm_backend_bind_task_endpointrecords a proven binding through a same-directory temp file at mode 0600 and never rewrites an existing binding, andfm_backend_resolve_task_endpointcomposes them and revalidates offline from the upgraded record. Any inconclusive read - unreachable server, missing, moved, or duplicated label - still refuses.bin/fm-control.shandbin/fm-teardown.shnow call the resolver (fm-control takes and releases the meta lock around the call; fm-teardown already holds it), the live read stays read-only and precedes any mutation, anddocs/configuration.md,docs/agent-control.md, anddocs/verification/runtime-backends.mdwere updated at their existing owning passages, including a note that the new live Herdr smoke case has not been executed yet.Risk Assessment
Testing
Reproduced the reported incident verbatim through the real bin/fm-control.sh entrypoint on base code, then demonstrated the fix end-to-end on the same sandbox record: the legacy Herdr endpoint's identity is re-derived from the live fm-<id> label, recorded, and both lifecycle verbs get past the refusal, while relabeled-tab and unreachable-server shapes still refuse without writing a binding. Also verified through the CLI that the gap is kind-independent and that tmux was never affected, that fm-teardown.sh recovers on the same path, that the new unit case genuinely fails on base and passes on target, and that the related control/backend/teardown suites are green apart from two failures that reproduce identically on base. This is a shell control-plane change with no rendered user surface, so the reviewer-visible evidence is CLI transcripts and the resulting persisted task-record state rather than screenshots. One gap remains: the new live-binary Herdr smoke case cannot execute on this machine because the isolated-lab helper fails closed on its fleet-state tripwire, identically on base and target.Evidence: Before/after CLI transcript: legacy second mate relaunch, plus the unprovable shapes and the teardown path
Source: Before/after CLI transcript: legacy second mate relaunch, plus the unprovable shapes and the teardown path
Evidence: Scope evidence: gap is kind-independent, tmux unaffected
Source: Scope evidence: gap is kind-independent, tmux unaffected
Evidence: Core before/after: the exact reported refusal, then recovery
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed (2) ✅
bin/backends/cmux.sh:352- fm_backend_cmux_workspace_matches_label readsworkspace list --jsonwith no--window, which this same file documents as CURRENT-window-scoped and verified live (fm_backend_cmux_window_of_workspace comment, bin/backends/cmux.sh:605). Two consequences: (a) the recovery this change adds does not work for a cmux task whose workspace is not in the focused window - the recorded workspace is simply absent from the list, so the proof refuses and the operator is still stranded, which is the exact gap the intent says cmux is covered for; (b) the ambiguity guard ('exactly one live workspace carries the title') cannot see a same-titled workspace in another window, so a duplicated legacy bare title is invisible and the proof can accept a workspace it should have refused as ambiguous. Suggested fix: enumerate windows vialist-windows --jsonand per-windowworkspace list --window <id>(the pattern already used by fm_backend_cmux_window_of_workspace) before deciding both membership and uniqueness.bin/fm-backend.sh:622- fm_backend_bind_task_endpoint composes the new record ascat "$meta" && printf 'endpoint_task_id=%s\n'. If the record has no trailing newline, the binding is concatenated onto the last line (e.g.herdr_pane_id=w1:p2endpoint_task_id=<id>), which both loses the last field and fails to record a binding - the record is left permanently corrupt and every later validation refuses it. The repo's existing meta rewriter (bin/fm-pr-check.sh:108-114) avoids this by reading line-by-line withwhile IFS= read -r line || [ -n "$line" ]and re-emitting each line newline-terminated. Fix: use the same read-loop form, or append a newline when the file does not end with one.bin/fm-backend.sh:565- The comment states the proof covers 'the whole recorded chain, not just the label'. That holds only for herdr (workspace -> pane -> owning tab -> label). The zellij branch calls fm_backend_zellij_tab_matches_label, which proves only that the recorded tab id carries the label and never that the recorded zellij_pane_id still belongs to that tab; the cmux branch proves only the workspace title and never that the recorded cmux_surface_id is still in that workspace. The target subsequently acted on by fm-control/fm-teardown is the pane/surface, not the tab/workspace, so the residual gap is larger on those two backends than the comment and docs/configuration.md claim. Either bind the pane/surface to the proven container (herdr already shows the shape) or narrow the wording.bin/fm-control.sh:297- CONTROL_META_LOCK is acquired at line 297 and released at line 300 with no coverage from the existingcontrol_cleanupEXIT trap, unlike CONTROL_LOCK. A signal delivered while fm_backend_resolve_task_endpoint runs (it can block on a live runtime query) leaves the meta lock dir behind. It is self-healing via fm_lock_try_acquire's stale-owner steal, so impact is low, but the established pattern (bin/fm-pr-check.sh:100 with META_LOCK_HELD plus trap) is to register it. Fix: track a held flag and release it in control_cleanup.🔧 Fix: fix binder newline handling, meta lock trap, narrow proof claims
3 issues (1 warning, 2 infos) still open:
bin/fm-teardown.sh:2167- preflight_firstmate_home_herdr_children (bin/fm-teardown.sh:2167) and validate_firstmate_home_children_removal (bin/fm-teardown.sh:2024) still call the bare offline fm_backend_validate_task_endpoint, so a CHILD task record inside a secondmate's home that predates fix: adapt Grok Stop continuation and harden endpoint cleanup #1171 now returns 2 and both loops|| return 1. Concrete path: tearing down the very secondmate the intent names (a home whose state/ was written in the same pre-2026-07-28 era as its own legacy record) refuses on the first legacy child meta, with the same 'lacks an exact task binding' message and no supported action on that teardown. The intent lists exactly two deliberately-unwired paths (fm-spawn --relaunch and fm-remote-secondmate-control) and this is neither, so the authorized failure is still reachable one path over. The earliest shared boundary is these two call sites, but wiring the resolver there needs the CHILD's meta lock (the loop only holds the parent's), which is presumably why it was skipped. Mitigation that does exist: resolving each child individually via fm-control/fm-teardown binds it and unblocks the parent. Worth a decision rather than a silent gap.bin/fm-teardown.sh:441- The teardown comment and the intent both describe the re-derivation as 'a read-only runtime query ... before any mutation', but fm_backend_resolve_task_endpoint also calls fm_backend_bind_task_endpoint, which rewrites state/<id>.meta in place before the authorization check concludes. That write is safe (proof-gated, meta-lock held, whole-file temp+mv, 0600, never overwrites an existing binding) and only ever appends the one missing field, so nothing is actually laundered - but the header of this hunk dropped 'It is metadata-only' while the remaining wording still reads as if the gate never writes. Narrowing that one sentence would keep the doc honest.bin/backends/cmux.sh:352- tests/fm-teardown-endpoint-safety.test.sh covers the herdr recovery in both directions thoroughly (provable, durable, five unprovable shapes, foreign binding, missing trailing newline), but the newly added fm_backend_cmux_workspace_matches_label and the zellij resolve branch have no executable coverage at all. Their refusal-on-unprovable behavior - the one property the intent says must never weaken - is currently established by code reading only, including the scoped-title-then-bare-title fallback and the absent-workspace refusal that the accepted limitation depends on.🔧 Fix: document teardown child-loop and resolver-write limitations
✅ Re-checked - no issues remain.
tests/fm-backend-herdr-smoke.test.sh:104- The new live-binary case in tests/fm-backend-herdr-smoke.test.sh could not be executed on this machine, so the label proof has never run against a real herdr server. The suite fails closed at lab setup ('fm-herdr-lab: fleet-state tripwire requires exactly one running default session') before reaching the new case; base and target both exit 1 with the same single 'not ok - could not prepare isolated Herdr lab session', so this is a pre-existing environment limitation and not a regression. All Herdr proof evidence I gathered therefore comes from a stub herdr CLI, which can only confirm the response shape the stub itself assumes. The change documents this honestly in docs/verification/runtime-backends.md. You need to decide whether to refresh that row on a machine with a running default Herdr session before merging, or accept the documented gap.bash tests/fm-teardown-endpoint-safety.test.shon target - all cases pass, including the new 'cleanup identity: a legacy non-tmux record is recoverable through a live label proof, and refuses without one'Regression proof: same test file run against basebin/(checked out from 1cb900c into an isolated copy) - fails withnot ok - a legacy Herdr record should refuse offline as recoverable (code 2), got 'rc=1'Manual E2E:bin/fm-control.sh rentmax-mate relaunchandbin/fm-control.sh rentmax-mate exitagainst a sandbox FM_HOME holding a legacy-shaped Herdr record (window/herdr_session/workspace/tab/pane, no endpoint_task_id) plus a stub herdr CLI, run on both base 1cb900c and target 8dc03ccManual E2E negative direction: same CLI invocations with the live tab relabeled tofm-someone-else, and with the herdr server unreachable - both refuse and write no bindingManual E2E scope check: same legacy record withkind=ship,kind=scout,kind=secondmate, and a legacybackend=tmuxrecord on both base and targetManual E2E:bin/fm-teardown.sh rentmax-mate --forceon a legacy Herdr record, base vs targetbash tests/fm-control.test.sh,bash tests/fm-control-relaunch.test.sh,bash tests/fm-backend-herdr.test.sh,bash tests/fm-backend.test.sh,bash tests/fm-backend-orca.test.sh- all pass on targetbash tests/fm-teardown.test.shandbash tests/fm-backend-cmux.test.shrun against BOTH base and target to confirm their failures are pre-existing and identicalbash tests/fm-backend-herdr-smoke.test.shon base and target - both fail closed at Herdr lab setup, identically✅ **Document** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.