fix(bin): correct false answered-decision and failed-run supervision reports - #3265
tknguyen29032002 wants to merge 21 commits into
Conversation
Two defects made a stopped worker read as a moving one. A steer answering a keyed decision closed that decision at enqueue time, but the local doorbell is deliberately skipped whenever the composer holds pending text, so the answer could sit unread while every durable record reported it answered. The closure is now parked beside its inbox record and committed at the worker's acknowledgement, so the decision stays open until it is actually read. The typed plane still closes on confirmed submit and the remote plane on remote enqueue, where a pending-reply expectation already backstops it. fm-crew-state walked back to an older run for the same branch when the newest one could not be bound to the worktree's code. A live run's review commits live in no-mistakes' own repo mirror, so they often do not resolve in the crew worktree, while the previous failed run's head frequently is the worktree's HEAD and binds perfectly. Only a branch's newest run may describe its current state; an unbindable newest run now attributes nothing and falls through to the pane. Regression tests for both fail on reintroduction.
Two defects made a stopped worker read as a moving one. A steer answering a keyed decision closed that decision at enqueue time, but the local doorbell is deliberately skipped whenever the composer holds pending text, so the answer could sit unread while every durable record reported it answered. The closure is now parked beside its inbox record and committed at the worker's acknowledgement, so the decision stays open until it is actually read. The typed plane still closes on confirmed submit and the remote plane on remote enqueue, where a pending-reply expectation already backstops it. fm-crew-state walked back to an older run for the same branch when the newest one could not be bound to the worktree's code. A live run's review commits live in no-mistakes' own repo mirror, so they often do not resolve in the crew worktree, while the previous failed run's head frequently is the worktree's HEAD and binds perfectly. Only a branch's newest run may describe its current state; an unbindable newest run now attributes nothing and falls through to the pane. Regression tests for both fail on reintroduction.
Two defects made a stopped worker read as a moving one. A steer answering a keyed decision closed that decision at enqueue time, but the local doorbell is deliberately skipped whenever the composer holds pending text, so the answer could sit unread while every durable record reported it answered. The closure is now parked beside its inbox record and committed at the worker's acknowledgement, so the decision stays open until it is actually read. The typed plane still closes on confirmed submit and the remote plane on remote enqueue, where a pending-reply expectation already backstops it. Recovery no longer waits on a spent attempt budget alone: a proven composer-blocked skip and an absolute unhandled-age bound each escalate on their own stated schedule, so an unacknowledged instruction cannot sit silently behind a busy pane. fm-crew-state walked back to an older run for the same branch when the newest one could not be bound to the worktree's code. Rebasing onto main picked up PR kunchenguid#3194's fix for most of this shape (an unresolvable newest row now stops the coarse scan instead of walking onto an older terminal row, and an active pipeline-owned run binds without head equality). Reconciling against it found one gap it left open: when axi status already answers for the crew's own branch but the head does not bind and the run is not pipeline-owned-active, the code still fell through to the coarse runs list, which can independently rediscover an older superseded row for that same branch. Closed by never consulting the coarse list once axi status has already answered for this branch - it can only re-find the same run or an older, superseded one, never a better one. Regression tests for both fail on reintroduction, confirmed by reverting each fix in isolation.
Round 2 of the review's three ask-user findings, applied by hand after the pipeline agent that was mid-fix died (recovered from the gate mirror's last committed head, 7739dd9, per the documented recovery procedure). The commit-idempotence check deduped a status key's closing line against the WHOLE status log's text, so a decision legitimately reopened under the same key and answered identically a second time was silently skipped as already-closed and stayed open forever. Scoped to a per-sidecar committed- identity marker instead, so idempotence protects only a retry of the same sidecar's own earlier append. A worker that rm's an acknowledged record instead of moving it into handled/ (a contract violation, but not literally prevented) left its sidecar bound to a record in neither place - a third state the ladder's .msg-based scan can never see, so the closure never committed and never escalated. The ladder now detects this and escalates once, independent of pane busy state, naming the violation and the record. Corrected the blocked-bound timing doc to what the code actually does (~1 grace period plus one poll interval, not ~2 grace periods); the timing itself is unchanged, since escalating on the very next poll after the first skipped attempt is the intended loud-over-quiet behavior. Regression tests for all three fail on reintroduction, confirmed by reverting each fix in isolation.
An orphaned decision closure (its inbox record removed instead of moved to handled/) is now set aside under handled/orphaned/ once its escalation marker is durable, so it stops reading as a pending answer and cannot annotate a later, genuine reopening of the same key as already delivered. The watcher's orphan escalation names the orphan's own keys as something to close by hand and no longer appends the wait-for-acknowledgement suffix that contradicted it. A committed closure that cannot be filed under handled/ now respects the already-surfaced quiet flag like the other two commit failure paths, so a permanently unwritable handled/ surfaces once instead of re-waking firstmate on every poll. The inbox layout inventory documents the per-sidecar committed ledger, the orphan escalation marker, and the orphaned quarantine.
…eardown - fm_task_inbox_commit_resolutions hands back exactly the closure names it failed on for the first time (.commit-failed), and fm_task_inbox_record_commit_escalated folds only those into .commit-escalated, so a closure acknowledged after the pass is never marked surfaced before its own first attempt. - _fm_task_inbox_orphaned_sidecar skips names already in .orphan-escalated, so a surfaced orphan whose retirement failed cannot shadow a newer one. - fm-teardown.sh runs the same closure-commit pass before removing the task inbox: an acknowledged answer lands its resolved line first, and an unacknowledged one is named as undelivered at cleanup without refusing. - Regression tests for all three, each verified to fail on the pre-fix code.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (5): Last reviewed commit: "no-mistakes(document): name teardown and..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 971b1d4419
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fm_cap_line_var "resolved [key=$k]: answered: $note" | ||
| line=$FM_LINE_CAP_LINE | ||
| append_rc=0 | ||
| fm_wake_status_append_self_announced "$state" "$status_file" "$line" || append_rc=$? |
There was a problem hiding this comment.
Captain, if the watcher exits after this status append succeeds but before the .committed receipt is written on line 707, the next pass treats the closure as uncommitted and appends it again. More seriously, if the worker reopens the same key during that interval, the retried stale resolved line appears after the new needs-decision and incorrectly closes the new decision; persist a transaction identity with the status event or otherwise make recovery able to recognize that this sidecar's append already landed.
Useful? React with 👍 / 👎.
|
Speaking as Kun's firstmate: first-time fork CI approved after a diff review (no This replaces the specified local-inbox close-at-enqueue path with acknowledgement-gated close, and adds always-on escalation bounds (including a 900s unhandled-age fire while busy). That is a new unconfigured default, not a restore of a broken specified path. Pipeline tests were skipped; twelve known findings are parked for captain judgment. Not merge-ready. Not a captain-decision flag yet (CI still settling; author still owns attestation and the parked findings). Waiting on CI, then the author, not the captain. VISION.md per-rule:
contract-class: new-default (replaces specified close-at-enqueue; new always-on 900s busy escalation). No auto-merge. |
…osure fm-gotmp.test.sh's fake FM_HOME never symlinked fm-task-inbox-lib.sh (and the fm-line-cap-lib.sh it sources), a new sibling fm-teardown.sh now sources unconditionally to commit or name deferred decision closures at cleanup; without it the fixture's teardown subprocess failed to source and exited non-zero. fm-captain-hold-lifecycle.test.sh's chat-channel test asserted the old close-at-send-time contract on fm-send.sh's local steering-inbox plane. That contract is what this branch's fix corrects: the closure now parks beside the durable record and only commits once the worker acknowledges it. Updated the test to assert the decision stays open immediately after send, simulate the worker's acknowledgement and the watcher's commit pass, then assert the close - matching the corrected provenance wording the deferred commit path records.
…iability # Conflicts: # docs/captain-hold-lifecycle.md
tknguyen29032002
changed the title
|
Speaking as Kun's firstmate: newer-activity re-triage after HEAD moved. Fresh fork runs for HEAD Attestation MATCH ( VISION.md per-rule (inspected HEAD vs main headers +
contract-class: new-default. Main's |
This reverts commit 4bdde97. That commit was produced autonomously by the pipeline's CI auto-fix step. Its diagnosis was correct - on a synchronize event the gate judges the event payload's PR body, which is a snapshot taken before the pr step writes the head-bound attestation into it - but the change does not belong on this branch: - This branch's scope is two supervision reliability defects in bin/. A change to .github/workflows/no-mistakes-required.yml, a new bin/fm-pr-body-settled.sh, and CONTRIBUTING.md are none of them. - It edits the verification surface that judges every pull request in this repo, from a fork pull request, which is a shape that needs deliberate maintainer review rather than arriving inside an unrelated change. - The maintainer's approval on this PR was given explicitly on the basis of no .github/workflows writes. The race it found is real and is being tracked as its own piece of work so it gets reviewed on its own merits.
…iability # Conflicts: # docs/scripts.md
tknguyen29032002
changed the title
2 participants
Intent
Fix two supervision reliability defects in firstmate's own shared tracked material, both about how firstmate reads a worker's true state, treated as one coherent piece of work.
Defect 1 - a silently undelivered instruction reads as answered. The doorbell-skip fail-safe in bin/fm-send.sh (composer holds pending text) must stay exactly as-is; failing safe there is correct. The bug was that --resolve-key wrote the decision's closing "resolved [key=...]" line to the task status log at SEND time, so the OPEN DECISIONS fold and the session-start digest reported a decision as answered while the worker had never seen it. Fixed by parking the closure as a dot-prefixed sidecar beside the durable inbox record; it commits only when the worker acknowledges the record, either on the supervision poll or at teardown as a last commit while the status log is still live, naming any closure never acknowledged as undelivered. The typed and remote planes still close immediately since they prove delivery directly. The steering-inbox escalation ladder gained bounded causes beyond the spent delivery-attempt budget: a provably composer-blocked skip, an absolute unhandled-age bound that fires even while the pane reads busy, and orphaned-sidecar detection, quarantine and retry.
Defect 2 - a healthy waiting job reported as failed. bin/fm-crew-state.sh matched a stale or superseded no-mistakes run instead of the one belonging to the branch and current code, producing false "failed" wakes for healthy validating crews. Fixed by binding run attribution to the branch's newest run row or attributing nothing.
Constraints: do not weaken the doorbell-skip fail-safe; tests colocated and demonstrated to fail on reintroduction; shellcheck clean; where something cannot be made certain make it loud rather than quiet. The separately-filed fm-turnend-guard-readonly defect is explicitly out of scope.
Regression coverage is colocated in tests/fm-send-resolve-key.test.sh, tests/fm-task-inbox.test.sh, tests/fm-crew-state.test.sh, tests/fm-wake-drain-open-decisions.test.sh and tests/fm-teardown.test.sh, each confirmed to fail when only its own fix is reverted. An earlier review round in this same effort fixed two further real defects in the new code: the overdue escalation was not deduplicated and would have re-woken firstmate on every poll forever, and the orphan sidecar scan accepted a non-file and could starve the ladder.
IMPORTANT SCOPE RULE FOR THIS RUN: this branch must contain ZERO changes under .github/. A previous CI auto-fix attempt on this branch autonomously committed 4bdde97, which edited the required-no-mistakes workflow, added a new PR-body helper script under bin/, and edited CONTRIBUTING.md. That commit has been reverted in full by explicit decision and must NOT be reintroduced. Its diagnosis was correct - on a synchronize event the required-no-mistakes gate judges the event payload's PR body, which is a snapshot taken before the pr step writes the head-bound attestation, so that run fails while the later edited-event run on the same head passes - but changing the repo-wide verification surface from a fork pull request is out of scope here, was explicitly excluded by the maintainer's approval condition of no workflow writes, and is being tracked as its own separate piece of work. Do not edit, re-add, or work around anything under .github/, and do not modify any gate to make a check pass. If a check is red because of that known stale superseded synchronize run, that is expected and is for a maintainer to re-run; it is not a defect to fix here.
Also merged origin/main (commit c7fdef9) to clear a merge conflict, resolving one conflict in docs/scripts.md by keeping both sides' independent row edits.
Never add an agent name as a commit co-author. Do not merge the PR.
What Changed
--resolve-keyno longer writes the closingresolved [key=...]line at send time on the local inbox plane.bin/fm-send.shnow parks the closure as a dot-prefixed sidecar beside the durable inbox record, andbin/fm-task-inbox-lib.shcommits it only once the worker acknowledges that record: on the watcher's supervision poll (bin/fm-watch.sh), or as a last commit at cleanup while the status log is still live (bin/fm-teardown.sh), which names any closure never acknowledged as undelivered. The typed and remote planes still close immediately, andbin/fm-wake-drain.shmarks an open decision whose answer is delivered but still unread.FM_TASK_INBOX_BLOCKED_MAX), an absolute unhandled-age bound that fires even while the pane reads busy (FM_TASK_INBOX_UNHANDLED_MAX_SECS, default 900s), plus detection, quarantine underhandled/orphaned/, and retry of orphaned sidecars. Each escalation is recorded once per record so a permanently failing closure cannot re-wake firstmate on every poll. The doorbell-skip fail-safe itself is unchanged.bin/fm-crew-state.shnow binds no-mistakes run attribution to the branch's newest run row or attributes nothing, instead of walking past an unbindable newest row onto an older superseded one and reporting a healthy validating crew as failed; the now-unusedfm_nm_head_resolvablehelper is dropped frombin/fm-nm-run-lib.sh. Regression coverage is colocated intests/fm-send-resolve-key.test.sh,tests/fm-task-inbox.test.sh,tests/fm-crew-state.test.sh,tests/fm-wake-drain-open-decisions.test.sh, andtests/fm-teardown.test.sh, with the docs,AGENTS.md, and skill files updated to describe acknowledgement-gated closure.Risk Assessment
✅ Low: The change is large but well-bounded to its stated intent, satisfies every source-verifiable required constraint including the zero-.github scope rule, and carries colocated behavioral regression coverage for each fixed defect; nothing survived this pass except two mechanical documentation/test-selection findings that cannot change runtime behavior.
Testing
Ran the colocated regression suites the intent names, then demonstrated both defects end-to-end through the surfaces an operator actually reads, reproducing each on the base commit and showing it fixed on the target. For defect 1 that meant driving the real fm-send with a visibly occupied composer (so the doorbell fail-safe skips, unchanged) and reading fm-wake-drain's OPEN DECISIONS fold: base silently reports the decision answered, target keeps it open and marks the answer as delivered-but-unread until the worker's acknowledgement commits the closure. For defect 2, the real fm-crew-state over real git repos and real-shaped run rows reports a live validating crew as failed on base and as working on target, in both false-failure shapes. I also captured the three new escalation wakes, the teardown cleanup transcript naming an undelivered closure, and the last commit's changed-file selection newly picking up the captain-hold suite, and confirmed each fix's own suite fails when only that fix is reverted. Three suites fail on this machine - fm-teardown, fm-captain-hold-lifecycle and fm-gotmp - but all three fail identically on an unmodified origin/main checkout here, so none is a regression from this change. Root cause: stock macOS bash 3.2 kills the shell with status 0 on a failed dot-builtin under set -e even when it is guarded, so the preflight never reports the missing adapter. CI's authoritative lanes are ubuntu-latest and never run these three suites; the macos-stock-bash job deliberately runs only bash -n plus two snapshot suites. Tracked separately as fm-preexisting-test-failures. This change's own new cases inside those suites pass when run in isolation. The worktree is clean and the branch has no .github/ changes.
Evidence: Defect 1 before/after: an undelivered answer no longer reads as resolved (real fm-send + fm-wake-drain transcript)
Source: Defect 1 before/after: an undelivered answer no longer reads as resolved (real fm-send + fm-wake-drain transcript)
BASE c7fdef9: ----- 3. the task status log right after the answer was sent ----- 1 needs-decision [key=api-shape]: pick REST or RPC for the mapping endpoint 2 working: kept busy on an unrelated stream 3 resolved [key=api-shape]: answered: go with REST ----- 4. what the supervisor now reads (OPEN DECISIONS fold, fm-wake-drain.sh) ----- (empty) >>> VERDICT: decision reads as ANSWERED although the worker never saw it. THE DEFECT. TARGET fefdf60: ----- 3. the task status log right after the answer was sent ----- 1 needs-decision [key=api-shape]: pick REST or RPC for the mapping endpoint 2 working: kept busy on an unrelated stream ----- 4. what the supervisor now reads (OPEN DECISIONS fold, fm-wake-drain.sh) ----- OPEN DECISIONS (still open, folded from the durable status logs - not just the latest line): t1 [key=api-shape] needs-decision: pick REST or RPC for the mapping endpoint (answer already delivered, still unread by the worker) >>> VERDICT: decision still OPEN and marked as delivered-but-unread. CORRECT. ----- 5/6/7. worker acknowledges -> commit_resolutions: ok -> 'resolved [key=api-shape]: answered: go with REST' appended -> fold empty >>> VERDICT: the decision closed exactly when the worker acknowledged it. CORRECT.Evidence: Defect 2 before/after: a healthy validating crew no longer reads as failed (real fm-crew-state output)
Source: Defect 2 before/after: a healthy validating crew no longer reads as failed (real fm-crew-state output)
SCENARIO A - crew committed past the live run's head; older FAILED row binds at the worktree HEAD SCENARIO B - this branch's own live run at head 305b0969 that only the no-mistakes mirror holds BASE c7fdef9: $ bin/fm-crew-state.sh moved -> state: failed · source: run-step · run failed $ bin/fm-crew-state.sh mismatch -> state: failed · source: run-step · run failed >>> a live, busy, validating crew is reported FAILED off a superseded run. THE DEFECT. TARGET fefdf60: $ bin/fm-crew-state.sh moved -> state: working · source: pane · harness busy (fm-spawn) $ bin/fm-crew-state.sh mismatch -> state: working · source: pane · harness busy (fm-spawn) >>> the live crew reads as working, sourced from the pane. CORRECT.Evidence: The three new bounded escalation causes, as firstmate reads them (real fm-watch.sh -> wake queue)
Source: The three new bounded escalation causes, as firstmate reads them (real fm-watch.sh -> wake queue)
provably composer-blocked doorbell: stale: sess:fm-t1 (unread firstmate instruction: .../t1.inbox/001.msg cannot be delivered because the composer visibly holds pending text, so every doorbell is being skipped; clear the composer, then re-ring - it carries the answer to decision key(s) api-shape, which stay OPEN until the worker acknowledges the record) absolute unhandled bound, busy pane: stale: sess:fm-t1 (unread firstmate instruction: .../t1.inbox/001.msg has been unhandled for over 1s without an acknowledgement while the pane reads busy - the worker may be inside one long tool call; inspect it before treating it as stopped) orphaned closure (record removed instead of acknowledged): stale: sess:fm-t1 (steering-inbox contract violation: .../t1.inbox/.001.resolve is an answered decision's closure with no bound record in the inbox or handled/ - the worker likely removed its record instead of moving it into handled/, so the closure can never commit on its own; it is set aside under .../handled/orphaned/ and the answer it carries to decision key(s) api-shape - which the worker most likely did read - must be closed by hand)Evidence: Teardown as the last commit point: acknowledged closure committed, unacknowledged one named undelivered
Source: Teardown as the last commit point: acknowledged closure committed, unacknowledged one named undelivered
$ bin/fm-teardown.sh task-x1 closed the acknowledged answer at cleanup: resolved [key=api-shape]: answered: go with REST undelivered at cleanup: task-x1 never acknowledged the answer to decision key(s) deploy-window (record 002.msg); that decision stays open teardown task-x1 complete (window firstmate:fm-task-x1, worktree ...) Backlog: task-x1 just finished. Run tasks-axi done task-x1 --note "local main", ... Backlog: include each 'closed the acknowledged answer at cleanup' line above in that done note - its closing line went with the task's status log, so the note is the only record of that answer after cleanup.Evidence: Changed-file test selection before/after (fm-test-run.sh --list --changed over an edit confined to bin/fm-task-inbox-lib.sh)
Source: Changed-file test selection before/after (fm-test-run.sh --list --changed over an edit confined to bin/fm-task-inbox-lib.sh)
BASE c7fdef9 -> captain-hold suite selected? NO TARGET fefdf60 -> captain-hold suite selected? YES newly selected by the target that the base did not select: tests/fm-captain-hold-lifecycle.test.sh tests/fm-pr-check-security.test.sh tests/fm-pr-merge.test.sh tests/fm-review-diff.test.sh tests/fm-teardown.test.sh tests/fm-wake-drain-open-decisions.test.sh tests/fm-x-mode.test.shEvidence: Regression coverage proof: each colocated suite fails when only its own fix is reverted
Source: Regression coverage proof: each colocated suite fails when only its own fix is reverted
Defect 1 revert (local inbox plane closes at ENQUEUE again): $ bash tests/fm-send-resolve-key.test.sh not ok - an unacknowledged answer must not read as resolved exit=1 Defect 2 revert (coarse list may walk past the newest unbindable row again): $ bash tests/fm-crew-state.test.sh not ok - a resolvable-but-mismatched newest run must not be resolved by an older run that binds (unexpected: 'state: failed') --- output --- state: failed · source: run-step · run failedEvidence: Targeted suite results and the pre-existing macOS bash-3.2 failures (with a reduced repro)
Source: Targeted suite results and the pre-existing macOS bash-3.2 failures (with a reduced repro)
FM_TEST_END tests/fm-task-inbox.test.sh exit=0 FM_TEST_END tests/fm-crew-state.test.sh exit=0 FM_TEST_END tests/fm-wake-drain-open-decisions.test.sh exit=0 FM_TEST_END tests/fm-send-resolve-key.test.sh exit=0 FM_TEST_END tests/fm-remote-transport-lanes.test.sh exit=0 FM_TEST_END tests/fm-teardown.test.sh exit=1 <- also fails on origin/main here FM_TEST_END tests/fm-captain-hold-lifecycle.test.sh exit=1 <- also fails on origin/main here tests/fm-gotmp.test.sh exit=1 <- also fails on origin/main here Reduced root cause (GNU bash 3.2.57, macOS /bin/bash): $ bash -c 'set -eu; src(){ . /nonexistent/x.sh || return 1; echo sourced-ok; }; req(){ echo "req entered"; if ! src; then echo "src FAILED"; return 1; fi; }; if req; then echo ok0; else echo nonzero; fi; echo "reached end"' req entered bash: line 1: /nonexistent/x.sh: No such file or directory (dies right there with status 0; on bash 4+/CI Ubuntu it prints "src FAILED" and continues)Evidence: Reproduction script for the defect 1 transcript
Source: Reproduction script for the defect 1 transcript
Evidence: Reproduction script for the defect 2 transcript
Source: Reproduction script for the defect 2 transcript
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-test-run.sh:1201- This change givestests/fm-captain-hold-lifecycle.test.sha brand-new direct dependency onbin/fm-task-inbox-lib.sh(the newrun_ack_and_commit_resolutionshelper sources the library and callsfm_task_inbox_commit_resolutions, andtest_chat_channel_feeds_the_same_keyed_answer_intakenow assertsstate: queuedbefore the ack andstate: doneafter), but thebin/fm-task-inbox-lib.sharm offamilies_for_changed_path- edited in this same change - was not extended to select it. That suite's family ispure-contract-unit, which this arm does not emit, and thefamilies_for_unmapped_binfallback does not apply because this path is explicitly mapped. Concretely: a later edit confined tofm_task_inbox_defer_resolutionorfm_task_inbox_commit_resolutionsthat breaks the deferral (for example closing at enqueue again) would leave the selected suites green whilefm-captain-hold-lifecycle.test.sh- the suite that pins that exact contract for the captain-hold channel - is never run. Fix is the same mechanical one already accepted for the wake-drain suite last round: addprintf '%s\n' __script__:fm-captain-hold-lifecycle.test.shbeside the existing__script__entry, rather than widening to the wholepure-contract-unitfamily.bin/fm-teardown.sh:241-teardown_inbox_closuresderives its undelivered list fromfm_task_inbox_pending_resolutions, which reports a sidecar as pending whenever its record is in neither the inbox root norhandled/. That is exactly the orphan case (the workerrm'd its record instead of moving it). If teardown runs before the watcher's next poll surfaces that orphan, the sidecar is still unsurfaced, so it lands in this loop and teardown printsundelivered at cleanup: <id> never acknowledged the answer to decision key(s) <k> (record 001.msg); that decision stays open- naming a record that no longer exists and telling firstmate the worker never saw an answer it almost certainly did read and act on.bin/fm-watch.sh's own orphan wake says the opposite for the identical state ("which the worker most likely did read"). Trace: write 001.msg +.001.resolvefor keyk,rm001.msg, run teardown with no intervening watcher poll -> the message above. The two states are distinguishable here without new machinery:fm_task_inbox_resolution_recordgives the bound record, and "still present in the inbox root" means genuinely undelivered while "absent from both root and handled/" means the orphan case. Flagging rather than patching because the wording is user-facing supervision text and the captain has stopped fix rounds on this machinery.🔧 Fix: select captain-hold suite on task-inbox library changes
2 infos still open:
bin/fm-task-inbox-lib.sh:111- The file header's commit contract says the idempotence check is against the status log's text - "a status key whose exact closing line is already in the status log is not appended again" - but the implementation deliberately does the opposite._fm_task_inbox_resolution_is_committed(line 495) greps the per-sidecar ledger<sidecar>.committed, and both of the more detailed doc blocks in this same file explicitly forbid the status-log-text reading:_fm_task_inbox_resolution_committed_path's header (lines 484-489) says "Scoped to THIS sidecar, never to the status log's text ... a global 'does this exact line already exist anywhere in the status log' check cannot tell the two apart and silently orphans the reopened decision", andfm_task_inbox_commit_resolutions's own block (lines 668-670) repeats it.tests/fm-task-inbox.test.sh'stest_reopened_key_with_identical_answer_closes_againexists precisely to fail if anyone implements what the header describes: a key legitimately reopened and re-answered with identical wording (routine for a short answer like "fix them all" on a repeated step key) would never close again. Since this file declares itself the ONE owner of the closure contract, a maintainer reading the header first is being pointed at the exact bug the test forbids. Fix is the same one-line kind already accepted forcommitted-ledger-comment-overstates: state that the dedupe is per-sidecar identity, not status-log text.bin/fm-test-run.sh:1112- This change adds new behavior tobin/fm-wake-drain.sh- the delivered-but-unread annotation inprint_open_decisions_section(bin/fm-wake-drain.sh:270-289) - whose only coverage is the three cases added totests/fm-wake-drain-open-decisions.test.sh. Butbin/fm-wake-drain.shmatches thebin/fm-watch*|bin/fm-wake*|...arm here, which emits onlywatcher-wake-lock, and that family (family_for_basename lines 218-226) listsfm-wake-drain-unread-status.test.shbut notfm-wake-drain-open-decisions.test.sh; the latter is unmapped, sofamily_for_basenamereturnsunclassifiedand it is never selected. Concretely: a later edit confined tobin/fm-wake-drain.shthat broke the annotation (for example dropping theunread_keyslookup, or capping the note away) would leave the selected suites green while the only suite pinning that behavior never runs. The same change already recognized this dependency in the other direction, adding__script__:fm-wake-drain-open-decisions.test.shto thebin/fm-task-inbox-lib.sharm at line 1203. Fix is the same mechanical one: add that__script__entry to this arm too, rather than wideningwatcher-wake-lock.tests/fm-teardown.test.sh:1696- Three suites fail locally on this macOS machine, and all three fail IDENTICALLY on the base commit c7fdef9 (origin/main) with unmodified code, so none is a regression from this change: tests/fm-teardown.test.sh (herdr-preflight-missing-adapter: teardown continued without its required preflight), tests/fm-captain-hold-lifecycle.test.sh (aborts at test_bound_channel_answers_close_at_answer_time withthe fixture channel captured no result to feed), and tests/fm-gotmp.test.sh (teardown did not remove the tasktmp dir). Root cause of the teardown one, reduced: macOS system /bin/bash is 3.2.57, where underset -ea failed.(source) builtin terminates the shell with status 0 even when guarded by|| return 1inside anifcondition, so teardown_herdr_require_prerequisites never reaches its error path. The repository's authoritative CI lanes run on ubuntu-latest (bash 5); .github/workflows/ci.yml's macos-stock-bash job deliberately runs onlybash -nparsing plus two snapshot suites under bash 3.2, not these. Because the suites abort at the first failure, this change's own new cases inside them never ran in the full-suite pass; I ran each in isolation and all pass. Flagging so you can decide whether the local bash-3.2 gap is worth its own piece of work - it is out of scope here (the change touches none of bin/fm-backend.sh, bin/backends/*, or the herdr preflight code path).bash bin/fm-test-run.sh tests/fm-send-resolve-key.test.sh- 16/16 pass, includingtest_undelivered_answer_never_reads_as_resolvedbash bin/fm-test-run.sh tests/fm-task-inbox.test.sh tests/fm-crew-state.test.sh tests/fm-wake-drain-open-decisions.test.sh tests/fm-teardown.test.sh tests/fm-captain-hold-lifecycle.test.sh- task-inbox, crew-state and wake-drain-open-decisions all exit 0; teardown and captain-hold exit 1 on pre-existing local failuresbash bin/fm-test-run.sh tests/fm-gotmp.test.sh tests/fm-remote-transport-lanes.test.sh- remote-transport-lanes exit 0; gotmp exit 1 on a pre-existing local failureManual E2E, defect 1: realbin/fm-send.sh t1 --resolve-key api-shape 'go with REST'underFM_FAKE_TMUX_COMPOSER=pending, then realbin/fm-wake-drain.sh, run against both the base tree (c7fdef9) and the worktree (fefdf60), asserting the status log, the inbox record,handled/, and the OPEN DECISIONS fold before and after the worker's acknowledgementManual E2E, defect 2: realbin/fm-crew-state.sh <id>over real throwaway git repos, a fakeno-mistakesserving real-shapedaxi status/runs --limitrows and a busy pane, for both false-failure shapes, run against base and targetManual E2E, test selection:bash bin/fm-test-run.sh --list --changed --base HEADover a one-line edit confined tobin/fm-task-inbox-lib.sh, in isolated git snapshots of base and targetReversion check: reverted onlyfm_send_defer_resolved_keysback tofm_send_close_resolved_keys/fm_send_feed_resolved_holdsin an isolated copy, thenbash tests/fm-send-resolve-key.test.sh->not ok - an unacknowledged answer must not read as resolvedReversion check: reverted onlynm_runs_status_for_branchand the own-branch bind inbin/fm-crew-state.shin an isolated copy, thenbash tests/fm-crew-state.test.sh->not ok - a resolvable-but-mismatched newest run must not be resolved by an older run that binds (unexpected: 'state: failed')Evidence capture: realbin/fm-watch.shpolls producing the wake-queue text for the composer-blocked, absolute-unhandled-bound and orphaned-closure escalation causesEvidence capture: realbin/fm-teardown.sh task-x1stdout with one acknowledged and one unacknowledged closure parked in the inboxPre-existing-failure isolation:bash /tmp/nm-base-tree/tests/fm-teardown.test.sh,.../fm-captain-hold-lifecycle.test.sh,.../fm-gotmp.test.shat base commit c7fdef9 - identical failuresIsolated re-runs of the change's own new cases in the blocked suites:test_teardown_commits_acknowledged_closure_and_names_undelivered_one,test_teardown_names_a_quietly_retried_closure_it_cannot_commit,test_chat_channel_feeds_the_same_keyed_answer_intake,test_teardown_skips_gracefully_without_tasktmp,test_teardown_skips_gracefully_when_dir_missing- all passScope check:git diff --name-only c7fdef9..fefdf60 | grep '^\.github/'returns nothing, and none of reverted 4bdde97's paths (.github/workflows/no-mistakes-required.yml, CONTRIBUTING.md, bin/fm-pr-body-settled.sh) appear in the net diff✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.