Skip to content

fix(teardown): keep remote retirement working with unrelated pending replies - #69

Merged
withally merged 2 commits into
mainfrom
fm/fm-teardown-pending-cleanup-f1
Aug 24, 2026
Merged

fix(teardown): keep remote retirement working with unrelated pending replies#69
withally merged 2 commits into
mainfrom
fm/fm-teardown-pending-cleanup-f1

Conversation

@withally

Copy link
Copy Markdown
Owner

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_cleanup in bin/fm-teardown.sh now guards the record deletion with an explicit if instead of an && rm AND-list, so a trailing pending-reply record belonging to another task no longer becomes the subshell's exit status and aborts remote secondmate retirement under set -eu; the directory identity, regular-file and non-symlink refusals stay intact and rm failures still exit 1.
  • Added tests/fm-teardown-remote-pending-replies.test.sh covering 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.md states 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 preserved

############ BEFORE FIX (baseline c447db5) ############

=== Scenario A: retire remote secondmate 'sm-alpha'; pending-replies holds only ANOTHER task's record ===
$ ls state/pending-replies/
reply-for-sm-beta
$ fm-teardown.sh sm-alpha
  error: remote pending-reply cleanup failed; preserving the local route for retry
  [exit status: 1]
$ ls state/pending-replies/   # sm-beta's record must survive
  reply-for-sm-beta
$ ls state/sm-alpha.meta      # retired => gone
  /var/folders/3n/3wfcplrn3clf44hjjkfgq8t00000gn/T//fm-evidence.K6Qtta/sm-alpha/state/sm-alpha.meta

=== Scenario B: mixed records - sm-gamma's own record plus an unrelated one ===
$ ls state/pending-replies/
  001-sm-gamma
  zzz-sm-beta
$ fm-teardown.sh sm-gamma
  error: remote pending-reply cleanup failed; preserving the local route for retry
  [exit status: 1]
$ ls state/pending-replies/   # only sm-gamma's record removed
  zzz-sm-beta

=== Scenario C: unsafe (non-regular) pending-reply entry must still refuse ===
$ fm-teardown.sh sm-delta
  REFUSED: pending-replies contains an unsafe recovery entry
  [exit status: 1]
$ ls state/sm-delta.meta      # route preserved for retry
  /var/folders/3n/3wfcplrn3clf44hjjkfgq8t00000gn/T//fm-evidence.K6Qtta/sm-delta/state/sm-delta.meta
$ grep sm-delta data/secondmates.md
  matches: 1

############ AFTER FIX (7aa5486) ############

=== Scenario A: retire remote secondmate 'sm-alpha'; pending-replies holds only ANOTHER task's record ===
$ ls state/pending-replies/
reply-for-sm-beta
$ 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      # retired => gone
  ls: /var/folders/3n/3wfcplrn3clf44hjjkfgq8t00000gn/T//fm-evidence.RGXZ7D/sm-alpha/state/sm-alpha.meta: No such file or directory

=== Scenario B: mixed records - sm-gamma's own record plus an unrelated one ===
$ ls state/pending-replies/
  001-sm-gamma
  zzz-sm-beta
$ fm-teardown.sh sm-gamma
  teardown sm-gamma complete (remote remote.example:/srv/firstmate-homes/sm-gamma)
  [exit status: 0]
$ ls state/pending-replies/   # only sm-gamma's record removed
  zzz-sm-beta

=== Scenario C: unsafe (non-regular) pending-reply entry must still refuse ===
$ fm-teardown.sh sm-delta
  REFUSED: pending-replies contains an unsafe recovery entry
  [exit status: 1]
$ ls state/sm-delta.meta      # route preserved for retry
  /var/folders/3n/3wfcplrn3clf44hjjkfgq8t00000gn/T//fm-evidence.RGXZ7D/sm-delta/state/sm-delta.meta
$ grep sm-delta data/secondmates.md
  matches: 1
Evidence: 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'

Regression test: tests/fm-teardown-remote-pending-replies.test.sh

--- RED (pre-fix AND-list restored in bin/fm-teardown.sh) ---
$ bash tests/fm-teardown-remote-pending-replies.test.sh
not ok - remote teardown failed when no pending-reply records matched: error: remote pending-reply cleanup failed; preserving the local route for retry
EXIT=1

--- GREEN (fix as committed at 7aa5486) ---
$ bash tests/fm-teardown-remote-pending-replies.test.sh
ok - remote pending-reply cleanup succeeds when only unrelated records exist
ok - remote pending-reply cleanup deletes only target task records
ok - remote pending-reply cleanup refuses a non-regular entry
EXIT=0

--- Existing suite tests/fm-teardown.test.sh ---
baseline c447db5 : 1 failure  -> not ok - herdr-preflight-missing-adapter: teardown continued without its required preflight
HEAD     7aa5486 : same 1 failure, no new failures

--- tests/fm-remote-secondmate-lifecycle-e2e.test.sh ---
baseline c447db5 : not ok - failing seed exited before remote provisioning
HEAD     7aa5486 : same failure at the same point (pre-existing, suite aborts there)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 2 infos
  • ℹ️ 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 ] &amp;&amp; [ ! -L ] || exit 1 guard 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 . &#34;$SCRIPT_DIR/fm-*-lib.sh&#34; 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 at bin/fm-teardown.sh:313, reran the same test — failed with error: remote pending-reply cleanup failed; preserving the local route for retry; source restored, git status --porcelain clean
  • Manual end-user CLI verification: ran real bin/fm-teardown.sh &lt;id&gt; against a synthetic FM_HOME/FM_ROOT_OVERRIDE remote-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 files
  • bash tests/fm-teardown.test.sh at HEAD and at baseline c447db5 (temp clone) — identical single failure herdr-preflight-missing-adapter, no new failures
  • bash tests/fm-remote-secondmate-lifecycle-e2e.test.sh at HEAD and at baseline c447db5 — identical pre-existing failure failing seed exited before remote provisioning
  • Code audit of sibling cleanup subshells: remote_outbox_cleanup ends in an unconditional rm -f, so it does not exhibit the conditional-last-statement defect
⚠️ **Document** - 1 info
  • ℹ️ docs/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.
⚠️ **Lint** - 1 warning
  • ⚠️ linter found issues (exit code 1)
✅ **Push** - passed

✅ No issues found.

@withally
withally merged commit 7e4fada into main Aug 24, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant