fix(teardown): keep remote retirement working with unrelated pending replies - #69
Merged
Merged
Conversation
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
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
Fix the confirmed bug in bin/fm-teardown.sh remote_pending_replies_cleanup under set -eu where an AND-list returns status 1 for a pending-reply record belonging to another task and aborts remote secondmate retirement. Make non-matching records a successful no-op with an explicit conditional while preserving the existing directory identity check, regular-file and non-symlink checks, and every refusal path; genuine unsafe entries and removal failures must still fail cleanup. Audit remote_outbox_cleanup and sibling cleanup subshells in the same file for the same conditional-last-statement class, fixing only confirmed instances. Add a minimal colocated executable-interface regression test under tests/ proving cleanup succeeds with only unrelated records, deletes only the target task's records while preserving unrelated records, and refuses a non-regular entry; demonstrate red before the fix and green after it. Keep the diff minimal, shellcheck-clean, and do not change any other teardown behavior. The pristine-main baseline independently reproduces two unrelated existing failures, 'failing seed exited before remote provisioning' in fm-remote-secondmate-lifecycle-e2e.test.sh and 'herdr-preflight-missing-adapter' in fm-teardown.test.sh; record them as pre-existing, do not fix them in this task, and judge this change using the focused regression and suites that are green on the same baseline.
What Changed
remote_pending_replies_cleanupinbin/fm-teardown.shnow guards the record deletion with an explicitifinstead of an&& rmAND-list, so a trailing pending-reply record belonging to another task no longer becomes the subshell's exit status and aborts remote secondmate retirement underset -eu; the directory identity, regular-file and non-symlink refusals stay intact andrmfailures still exit 1.tests/fm-teardown-remote-pending-replies.test.shcovering cleanup succeeding with only unrelated records, deleting only the target task's records while preserving unrelated ones, and refusing a non-regular entry.docs/remote-secondmates.mdstates that retirement clears only the retiring secondmate's pending-reply records and lists the new test in the remote suite commands.Risk Assessment
✅ Low: Two-line, well-scoped fix to a confirmed set -e conditional-last-statement bug that preserves every existing refusal path, plus a colocated regression test that is genuinely red before and green after; the only remaining notes are test-coverage/coupling observations.
Testing
I drove the real fm-teardown.sh CLI end-to-end against a synthetic remote-secondmate home at both the baseline and the target commit: on the baseline, retiring a secondmate while another task's pending reply sat in state/pending-replies aborted with 'remote pending-reply cleanup failed; preserving the local route for retry' and left the route behind, while at HEAD the same run completes with 'teardown ... complete', removes only the target task's records, and preserves the unrelated one; the non-regular-entry case still refuses identically on both commits with the route and registry intact. The new colocated regression test fails on the pre-fix line and passes with the fix, tests/fm-teardown.test.sh shows no new failures versus baseline, and the two failures named in the intent (herdr-preflight-missing-adapter and the lifecycle e2e's failing-seed case) reproduce identically on the pristine baseline and are left alone. No UI surface is involved, so evidence is a CLI transcript rather than screenshots; temp clones and scratch files were removed and the worktree is clean.
Evidence: Before/after teardown CLI transcript (baseline vs fixed)
Source: Before/after teardown CLI transcript (baseline vs fixed)
############ BEFORE FIX (baseline c447db5) ############ === Scenario A: retire remote secondmate 'sm-alpha'; pending-replies holds only ANOTHER task's record === $ fm-teardown.sh sm-alpha error: remote pending-reply cleanup failed; preserving the local route for retry [exit status: 1] $ ls state/sm-alpha.meta # retired => gone .../sm-alpha/state/sm-alpha.meta <-- still there, retirement aborted ############ AFTER FIX (7aa5486) ############ === Scenario A === $ fm-teardown.sh sm-alpha teardown sm-alpha complete (remote remote.example:/srv/firstmate-homes/sm-alpha) [exit status: 0] $ ls state/pending-replies/ # sm-beta's record must survive reply-for-sm-beta $ ls state/sm-alpha.meta No such file or directory === Scenario B (mixed records), after fix === teardown sm-gamma complete (...) [exit status: 0] $ ls state/pending-replies/ -> zzz-sm-beta (only sm-gamma's record removed) === Scenario C (non-regular entry), identical before and after === REFUSED: pending-replies contains an unsafe recovery entry [exit status: 1] route + registry preservedEvidence: Regression red/green log and baseline comparison
Source: Regression red/green log and baseline comparison
RED (pre-fix line restored): not ok - remote teardown failed when no pending-reply records matched GREEN (7aa5486): 3/3 ok tests/fm-teardown.test.sh: baseline and HEAD both show only herdr-preflight-missing-adapter lifecycle e2e: baseline and HEAD both stop at 'failing seed exited before remote provisioning'Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
tests/fm-teardown-remote-pending-replies.test.sh:114- test_cleanup_refuses_non_regular_entry never reaches remote_pending_replies_cleanup: remote_recovery_paths_validate initial (bin/fm-teardown.sh:293-296) already refuses the non-regular pending-reply entry before any remote retirement, so the assertions on preserved meta/registry pass trivially and the cleanup subshell's own[ -f ] && [ ! -L ] || exit 1guard stays unexercised. The test is still a valid teardown-interface refusal test, but its name attributes the refusal to the cleanup step. Consider renaming it, or exercising the entry appearing only after validate (the race the cleanup guard actually defends).tests/fm-teardown-remote-pending-replies.test.sh:32- make_case hand-mirrors fm-teardown.sh's. "$SCRIPT_DIR/fm-*-lib.sh"list as 13 explicit symlinks into a fake bin dir. Adding a new sourced sibling to fm-teardown.sh will fail this test with an opaque "No such file or directory" rather than a behavioral failure. The repo already has a supported seam for this (FM_SSH_BIN/fm_fakebin fake-ssh + FM_FAKE_REMOTE_ENTRYPOINT, see tests/fm-remote-secondmate-lifecycle-e2e.test.sh:258) that runs against the real bin/ dir. Not blocking — the symlink farm is more hermetic — but it is a standing coupling to an implementation detail.✅ **Test** - passed
✅ No issues found.
bash tests/fm-teardown-remote-pending-replies.test.sh— green at HEAD (3/3 ok)Red demonstration: temporarily restored the pre-fix AND-list atbin/fm-teardown.sh:313, reran the same test — failed witherror: remote pending-reply cleanup failed; preserving the local route for retry; source restored,git status --porcelaincleanManual end-user CLI verification: ran realbin/fm-teardown.sh <id>against a syntheticFM_HOME/FM_ROOT_OVERRIDEremote-secondmate fixture for 3 scenarios (unrelated-record-only, mixed records, non-regular entry) at both baseline c447db5 and HEAD 7aa5486, capturing stdout/stderr, exit status, and resulting state/registry filesbash tests/fm-teardown.test.shat HEAD and at baseline c447db5 (temp clone) — identical single failureherdr-preflight-missing-adapter, no new failuresbash tests/fm-remote-secondmate-lifecycle-e2e.test.shat HEAD and at baseline c447db5 — identical pre-existing failurefailing seed exited before remote provisioningCode audit of sibling cleanup subshells:remote_outbox_cleanupends in an unconditionalrm -f, so it does not exhibit the conditional-last-statement defectdocs/fm-test-portable-shards.md:8- docs/fm-test-portable-shards.md and docs/fm-test-isolation-proof.{md,json} are dated (2026-08-20) point-in-time proofs of a 24-candidate portable set and do not list the new tests/fm-teardown-remote-pending-replies.test.sh. I treated them as maintainer-verification evidence of a specific run rather than a live inventory, so I did not hand-edit them; tests are glob-discovered by bin/fm-test-run.sh with a coverage guard, so no registration is required. If the project wants those artifacts current, they should be regenerated by bin/fm-test-isolation-proof.sh in a follow-up rather than hand-copied.✅ **Push** - passed
✅ No issues found.