fix: fail teardown on adapter load errors and premature exits - #3568
fix: fail teardown on adapter load errors and premature exits#3568kuan0808 wants to merge 9 commits into
Conversation
…k sibling tests/fm-gotmp.test.sh has failed since cf95112 (kunchenguid#1577), which made bin/backends/tmux.sh source fm-session-lock-lib.sh at load time. The test builds a fake FM_HOME by symlinking only the bin/ siblings teardown needs, and that hand-curated list never gained the new sibling. The teardown kill step then dies on the failed source; the message is hidden behind `2>/dev/null || true`, the EXIT trap returns Bash's post-fatal `$?` of 0, and the test's "did not remove the tasktmp dir" assertion is the first one that notices. Production teardown is unaffected: a real bin/ always has the sibling, and tasktmp removal works. This is a fixture fix only. - Symlink fm-session-lock-lib.sh into the fixture next to fm-tmux-lib.sh. - Stub fm-remote-job-reap-orphans.sh like fm-guard.sh and fm-fleet-sync.sh; teardown calls it best-effort, and the real sweep signals processes, so it must never be symlinked into a fixture. - Let make_fake_root omit the tasktmp= line when called with one argument, and drop the second, duplicated copy of the fixture so the sibling list lives in one place. bin/fm-teardown.sh and bin/fm-spawn.sh are untouched.
bin/fm-teardown.sh ends through an EXIT trap that returns `$status`, and Bash reports `$?` as 0 inside that trap after a fatal shell error such as a failed `source` of a missing sibling. A missing sibling in bin/ therefore made teardown exit 0 silently while the task's meta, tasktmp, endpoint, and state were all left in place, and every caller read that as success. - In the EXIT trap, treat exit 0 with the task record still present as an aborted teardown: report it on the stderr saved before the trap (the abort may happen inside a call whose own stderr is discarded) and exit 1. Every legitimate exit-0 path has already removed the record, so no refusal path or message changes. - In fm_backend_source, check the adapter file is readable before sourcing it, so a missing adapter is the ordinary failed return its callers already handle instead of a Bash 3.2 fatal error that skips their own refusal. This restores teardown's designed "nothing was changed" herdr preflight refusal on stock macOS Bash. - Regression in tests/fm-gotmp.test.sh: remove one sourced sibling from the fixture and assert non-zero exit, the stderr report, and that the task record and tasktmp survive.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (3): Last reviewed commit: "no-mistakes(document): Clarify teardown ..." | Re-trigger Greptile |
|
Speaking as Kun's firstmate: HEAD Attestation: MISSING — no head-bound no-mistakes Contract-class: restore — teardown already promised a loud non-zero abort that leaves durable records in place ( VISION (per rule, evidence =
Overlap with #3556: this PR's first commit is the gotmp fixture sibling list ( This is waiting on you (the author), not a captain decision. Please workflow-approvals this pass: 33687571873, 33687571845 |
…260905/pr-3568-clean-r1
kuan0808
changed the title
|
Updated for published head The earlier missing-attestation request is addressed: the genuine no-mistakes-generated PR-body attestation matches the published head. Review, Test, Document, Lint, Push and PR completed; the pipeline remains at CI awaiting maintainer approval. The pinned upstream verifier passed against this body and head during the publication review; that local verification does not mean the hosted workflow ran. The historical missing-sibling assertion was replaced with explicit POSIX-mode and default-Bash zero-exit cases. Required child adapters are preflighted before forced parent cleanup. The refresh also records successful metadata removal under the lock, preventing a new same-ID record from turning successful cleanup into a false abort. The canonical changed-file baseline passed all 23 scripts with zero failures and zero gate skips on Please approve the fork workflows for current head |
kuan0808
changed the title
2 participants
Intent
Complete the authorized refresh of existing upstream Firstmate PR #3568 (#3568), preserving its accepted intent: fatal backend/sibling sourcing and early teardown exits must fail nonzero rather than report successful cleanup. A missing/unreadable backend adapter must return an ordinary failed source result so the caller can emit its own refusal. A teardown that exits with status zero before removing its task record must return failure and report the abort on saved original stderr, even if the triggering call discards stderr. Preserve task metadata and task scratch on that abort, retain the normal successful cleanup, absent-tasktmp compatibility, and already-missing-tasktmp paths. Do not weaken refusal or unlanded-work safety.
Fix the actual cross-shell assertion that fails in hosted CI run 33687571873: modern Bash does not necessarily abort on a missing sourced sibling inside an OR-protected best-effort function. Integrate upstream #3644's hermetic tmux adapter fixture, never restore its real host tmux dependency. The revised regression enables POSIX mode inside the stub adapter after ordinary Bash startup and proves that the missing sibling was reached, separately exercises a deterministic zero-status exit through a stub adapter under default Bash to pin the EXIT guard and saved stderr, and checks that a missing adapter reaches the caller's refusal. These use the actual teardown/backend interfaces, not source-text assertions. The shared make_fake_root retains the remote orphan-reaper stub so tests cannot signal live workers. Preserve stock Bash 3.2 compatibility and verify the Linux/modern Bash assertion honestly; this host has only Bash 3.2 and Docker is not running, so no local modern-Bash result is claimed. Do not install tools or start host services.
This is one independent PR within an authorized refresh of eleven existing public PRs. Only #3568 is this run's implementation/publication scope. The other ten PRs, unpublished cadence item 13, withdrawn 09 and closed 901 are tracked separately; do not implement or publish their patches in this PR. #3560 has now been closed as equivalent to merged #3681 after its original regression passed on current main. #3556/#3557 retain their existing current-head attestations and parked CI runs. Do not alter those runs or their refs. The separate adoption worker owns the local installed-version candidate; exclude that bridge, private LF-21/TP-1/home content, and old composite validation stacks from every public patch. No main/default-branch push, PR merge, branch deletion, unrelated public messages, credential change, tool install, daemon restart, reset, stash, discard, or unguarded Herdr lifecycle is authorized.
The submitted candidate is 004fad0, an ordinary merge whose parents are exact public/pipeline cf96867 and freshly verified public upstream d6660d7. Firstmate explicitly authorized synchronizing all published fixes, ending the now-obsolete CI-parked run01M1R1ZK6HZ06WH69RHRXSW0QP because upstream introduced a conflict, and starting this fresh ordinary-history reconciliation. The old run is terminal cancelled and all receipts remain preserved; its hosted approval was not waived. Current native home explicitly offers this new pipeline for the clean local-ahead branch. No recovery mutation was needed. The only conflict was the old duplicated no-tasktmp test fixture versus upstream's new tasks-axi stub declarations: retain the shared make_fake_root with both upstream declarations (FM_TASKS_AXI_MIN and fm_tasks_axi_backend) and retain its hermetic backend/orphan-reaper stubs. Upstream's legacy-record teardown acceptance/refusal and stamp rollback behavior remains intact. The final public diff still contains only bin/fm-backend.sh, bin/fm-teardown.sh and tests/fm-gotmp.test.sh. Public and upstream heads are actual ancestors. Delivery remains on the separate independent clone's fm/up-10-teardown-source-failure branch; no shared origin/main or primary ref is moved, and no private adoption head is imported. Rebase is intentionally skipped because exact current upstream is already an ancestor and ordinary fast-forward history is required. Review, Test, Document, Lint, Push, PR and CI must all run genuinely. Never rewrite this history, force-push, overwrite a newer remote head, or drop any prior pipeline fix; return a newly conflicting base or preservation mismatch to Firstmate.
Contribution requirements: follow current CONTRIBUTING, AGENTS, .agents/skills/firstmate-coding-guidelines/SKILL.md, and trusted .no-mistakes.yaml; keep shell scripts ShellCheck 0.11.0/actionlint 1.7.12 clean, one sentence per Markdown line, documentation with one authoritative owner, no agent co-author. The pinned upstream attestation action at 32d396ac0f29135daf7fcb9964aba9d5f4e796d6 requires the actual pipeline-generated signature plus current-head attestation with completed review/test/document. Do not handcraft/transplant attestation or mark an absent/approval-gated check green. Correct the existing PR title/body so it describes the final fixture and actual cross-shell results, with accurate Intent, What Changed, Risk Assessment, Testing and Pipeline evidence. The previous genuine publication already corrected the obsolete real-adapter fixture description; keep that correction and update only evidence that changes for this new head. Preserve genuine hosted failures/limitations in evidence. Request maintainer workflow approval only when it is the remaining blocker, never impersonate a maintainer or approve protected workflows.
Use Codex GPT-6 Astra/high, as configured globally. This lane owns broad/no-mistakes validation one PR at a time. Use the current canonical changed-file baseline excluding real-herdr-gated and focused additional intent evidence, not an all-tests walk. Installed no-mistakes 1.60.2 has a 30-minute per-agent invocation cap: reuse baseline evidence, do not blindly repeat timed-out full suites or change global timeout settings. The exact committed-head canonical selection against d6660d7 contains23 scripts. Let the configured baseline run once under its canonical runner bounds (changed selection has per-script900s); reuse that fresh baseline in the subsequent evidence-agent invocation, then run only focused interface cases needed for intent. Installed v1.60.2 Test source was verified by Git blob318996d04fe1710bcecd5a8ed55af88cdfd962ee: the configured shell baseline runs separately before the30-minute evidence-agent context. There is no supported per-run timeout flag in current axi run/respond help. This bounded evidence-reuse approach needs no config change or dropped coverage. If findings cannot all fit one supported round, return the concrete bounded plan to Firstmate: do not silently leave unselected findings as user-declined or treat partial fixes as all accepted. All active-run fixes belong to the pipeline; the outer worker will respond to gates, never hand-edit while custody is active. Every ask-user finding must be returned to Firstmate before any gate response; never pass --yes.
Accepted Firstmate R1 decision: fatal required child-adapter sourcing must fail nonzero before removing parent or child task records in forced secondmate cleanup. Use the existing recursive parent preflight to load required child adapters before destructive cleanup. Preserve best-effort endpoint kills after successful required loading, including a failed kill that still permits normal cleanup. The regression must use isolated fake tmux/Zellij adapters and orphan-reaper stubs, pin unchanged parent/child metadata, work and scratch on missing sibling, missing adapter and zero-status exit, and retain success/best-effort kill-failure controls. No real fleet teardown or Herdr lifecycle is authorized. The accepted two-file R1 fix was reproduced, passed 11 Bash3.2 cases and re-reviewed without findings in the old run; that historical evidence does not replace this successor's fresh pipeline attestation.
Publication history exclusion: the cancelled old native run 01M1QREG6QZFX15MB2BJ5FHTS3 accidentally committed local Graft artifacts at f550c3a5e66ac2036a2ffc9a72766431392b31a8 and 1ce0f59058ad98fa8f306261dd2eb1c531053fe6. Firstmate explicitly authorized lossless native custody return and exact clean R1 reconstruction. Both contaminated heads and complete objects remain privately preserved, but neither may be an ancestor of any public push. Do not cherry-pick, merge, rebase onto or resurrect those commits or any generated graph/cache/card objects. The R1 reconstruction's accepted two source/test blobs match the approved fix exactly and its other paths match the clean submitted parent; the subsequent ordinary upstream merge preserves those blobs and current public/upstream ancestry.
A tested task-local Graft boundary in this dedicated gate seeds an ignored .env with only GRAFT_NO_GITIGNORE=1 and GRAFT_NO_IGNORE=1 before agent startup, and privately excludes graft/, .graft/, .ignore and .env from Git staging. These private adapter files, the graph, preservation bundle and reports must never enter the public patch or commit history. Leave the task-owned startup hook and .env intact. Do not run graft init, change global hooks/config/daemon/installed tools, add tool-specific tracked ignore stanzas, or weaken canonical test mapping to admit generated artifacts. Actual installed Graft startup/post-edit/Stop/query plus add-A passed twice in an isolated fixture; conflicting initial or repeated startup fails closed before native agent launch. Any newly observed artifact or custody/head mismatch is a genuine gate to return to Firstmate, not a reason to auto-approve, skip validation or publish.
Fresh pre-pipeline checks on this reconciled tree: all11 existing fm-gotmp cases pass on stock Bash3.2, including forced-child failure retention and best-effort kill controls; ensure-agents is idempotent and diff whitespace passes. These focused checks are context, not a replacement for this new exact-head native baseline, Review/Test/Document attestation or real hosted checks. Preserve upstream current lint owner changes and its current trusted configuration. Do not run other PR baselines concurrently. Stop at the actual external approval gate or CI-ready return and report accurately.
What Changed
Risk Assessment
✅ Low: The bounded changes preserve teardown safety checks and correctly distinguish completed cleanup from early aborts, including replacement metadata.
Testing
The supplied baseline and focused Bash 3.2 checks passed; captured CLI evidence demonstrates abort diagnostics, parent/child record and scratch preservation, replacement-record safety, and successful/best-effort cleanup. Pre-fix controls reproduced both regressions. Linux/modern Bash remains for hosted CI; no live fleet operations ran. Graft: one query; savings unavailable in truncated output.
Evidence: CLI diagnostics, preserved state, successful cleanup, and pre-fix failure reproductions
Source: CLI diagnostics, preserved state, successful cleanup, and pre-fix failure reproductions
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
⏭️ **Rebase** - skipped
Step was skipped.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-teardown.sh:310- The EXIT guard can mistake a replacement task for an aborted teardown. Successful cleanup removes the original metadata and releases its metadata lock at line 3156, then runs fleet-sync and summary helpers. A fresh dispatch of the same ID can publish new metadata during that interval; this guard then returns failure and incorrectly claims all records were retained. Record successful removal in a process-local flag while holding the metadata lock, covering local and remote completion paths, and use that completion fact in the guard.🔧 Fix: Track teardown completion before replacement metadata can appear
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh --changed --exclude-family real-herdr-gatedbin/fm-test-run.sh --changed --exclude-family real-herdr-gated— reused the successful baseline supplied by the executor.bin/fm-test-run.sh tests/fm-gotmp.test.sh— passed on Bash 3.2.57.python3 ~/.no-mistakes/evidence/01M1SSYMJXN442GWS2CHEEYZWE/capture-teardown.py "$PWD"— captured executable-interface output and preserved state; additionally verified unreadable-adapter refusal.Executed replacement-record regression against004fad0and zero-status-abort regression againstd6660d75; both failed as expected before their fixes.git status --shortandgit diff --exit-code— confirmed cleanup left the worktree unchanged.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.