fix(backend): detach Herdr server from caller lifecycle - #4
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
Harden fm_backend_herdr_server_ensure against caller tmux-pane death using a verified setsid-based detached lifecycle while keeping the adapter as the single Herdr lifecycle owner; add honest regression coverage and empirical lab documentation; repair the merge-main conformance baseline resolver exposed by PR #2 post-merge CI; validate, push, and open the fix PR, but do not merge it without fresh Captain authorization.
What Changed
nohupand double-forkedsetsidpath so they survive the initiating tmux pane while the backend adapter remains the lifecycle owner.nohupandperlas Herdr prerequisites, provide actionable bootstrap guidance, and document the detached lifecycle with empirical verification.Risk Assessment
✅ Low: The changes are well-bounded, preserve lifecycle ownership, register the new dependencies, and correctly gate baseline fallback on verified main-at-HEAD state.
Testing
The already-green full shell suite, focused Herdr lifecycle/baseline/bootstrap regressions, and a real Herdr 0.7.3 tmux-pane-death flow all passed after correcting evidence-harness-only setup issues; the transcript proves the compatible server and working pane survived, followed by exact cleanup.
Evidence: Herdr pane-death end-to-end transcript
POST_STATUS running=true compatible=true POST_PANE pane=w1:p1 reachable=true agent_status=working PASS: real Herdr server and working pane survived caller tmux-pane death CLEANUP: isolated session stopped/deleted and transient configuration removedPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (2) ✅
tests/fm-backend.test.sh:98- When CI runs after a squash, rebase, fast-forward, or ordinary single-parent push to main, every main ref equals HEAD and is skipped, while this fallback only accepts merge commits. BASE_REF resolution then aborts the suite despite HEAD^ being available. Handle verified main-at-HEAD single-parent commits as well.bin/backends/herdr.sh:142- The launcher makes nohup and perl mandatory, but the canonical fm_backend_required_tools registry still declares only herdr, jq, and treehouse. Bootstrap can report a healthy Herdr environment before spawn or recovery fails. Register and document both dependencies, or use the already-required python3 runtime.🔧 Fix: Fix Herdr preflight and single-parent baseline resolution
2 warnings still open:
tests/fm-backend.test.sh:107- The merge fallback returns HEAD^1 whenever HEAD has multiple parents, even if no candidate main ref equals HEAD. With absent or stale main refs—or an arbitrary topic-branch merge—this silently selects an unrelated baseline. Gate this fallback on verified main-at-HEAD state or otherwise prove HEAD is integrated main.bin/fm-backend.sh:319- Registering nohup and perl routes missing dependencies through bootstrap, but neither tool has an install_cmd or manual_install_url entry. Bootstrap therefore emitsMISSING: <tool> (install: ), andfm-bootstrap.sh install <tool>reports it as unknown. Add actionable install/manual mappings or use the already-required python3 runtime.🔧 Fix: Gate baseline fallback and guide Herdr dependency recovery
✅ Re-checked - no issues remain.
🔧 **Test** - 1 issue found → auto-fixed ✅
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"🔧 Fix: Fix baseline fixture helper dependencies
✅ Re-checked - no issues remain.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"Baseline supplied as successful:command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"bash tests/fm-backend-herdr.test.shFM_TEST_FOCUSED=review-round-35 bash tests/fm-backend.test.shFM_TEST_FOCUSED=review-round-35 bash tests/fm-bootstrap.test.shManual real-stack check: invoked productionfm_backend_herdr_container_ensurefrom a disposable tmux pane, destroyed it withtmux kill-session, then verified survival using scopedherdr status --json,herdr pane get, andherdr agent getcallsVerified exact isolated-session cleanup, absence of test processes/configuration, and a cleangit status --short✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: Captain: fix detached Herdr lint findings
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.