Repository navigation
fix: prevent failed captain holds from reassociating origins - #6777
cloud-practitioner wants to merge 5 commits into
Conversation
…ete hold move A hold moved to a new origin wrote its stamp and origin before the backend hold. A process stopped between those writes, or a refused hold whose rollback also failed, left the task associated with the new origin while it carried only the earlier call's answer, and complete and verify accepted that answer. Verification now treats an open, unheld task that leads with a hold-set stamp above a recorded answer as a re-hold that never completed, so the answer no longer satisfies the new origin however the move was interrupted. A refused hold also restores the body the attempt started from, and the origin lookup runs before any write, so a clean failure leaves the earlier answer valid. Closes kunchenguid#6461
…nd regression coverage
|
Speaking as Kun's firstmate: thanks, @cloud-practitioner, for filing #6461 and coming back with the fix. I read the whole diff of head What I checked:
The no-mistakes attestation matches this head, and "PR must be raised via no-mistakes" is green. The main CI run is still in progress, and the merge state is UNSTABLE while it runs. Contract-class: restore. The specified default path is the captain-hold completion gate: VISION.md, rule by rule
Next step: this is waiting on CI. If CI goes fully green on |
2 participants
Intent
Offer a pull request that fixes #6461 and closes it, with a regression test.
The issue:
bin/fm-captain-hold.sh hold <task> --origin <new-origin>records the new origin on the task before the backend hold succeeds.If the command is interrupted between those two writes, or the hold fails and the rollback write also fails, the task is left associated with the new origin while still carrying only the answer recorded for its previous origin.
verify_hold_durableaccepts any recorded answer viabody_has_resolution_recordwithout checking that it belongs to the current origin or postdates the current hold, andverify_entry_durablecompares only the stored origin with the requested origin, socompleteandverifyaccept the previous origin's answer as if the captain had answered the new origin's call.Expected: completion and verification refuse, because the task was never successfully held for the new origin and that call was never answered.
The issue's suggested fixes: make the verification boundary reject an incomplete reassociation instead of relying on write order (for example accept a recorded answer only if it is newer than the current hold-set stamp or carries the origin it answered, and treat a task whose stored origin has no matching live captain hold and no matching answer as not durable), or write the new origin only after the backend hold succeeds, or record a durable in-progress marker that
completeandverifyrefuse while present.What Changed
--originonly after the backend captain hold succeeds, is verified, and any secondmate parent notification is published; attempt to restore the previous body on backend failure.completeandverifyreject an earlier answer beneath a hold-set stamp on an open task without captain-hold annotations.Closes #6461
Risk Assessment
✅ Low: The changes are bounded to hold-origin safety, preserve parent publication before origin-write failures, and introduce no substantiated material defects.
Testing
All twelve live CLI scenarios passed, with persisted task state, refusal diagnostics, and parent notifications captured. The targeted captain-hold suite completed in bounded batches after its initial time cap and a corrected continuation-launch setup error; existing Beads migration cases skipped on this markdown-only host. The pre-fix failure reproduced successfully. No visual UI changed, so CLI transcripts provide the product evidence.
Evidence: Live hold-origin CLI transcript
Source: Live hold-origin CLI transcript
Evidence: Pre-fix false-acceptance reproduction
Source: Pre-fix false-acceptance reproduction
Evidence: Validation summary and evidence index
Source: Validation summary and evidence index
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
bin/fm-captain-hold.sh:542- The durable fix still permits an incomplete origin move to pass completion and verification. Starting from the regression's state—A's released answer, B's stored origin and leading stamp, and a failed backend hold/rollback—retryinganswer --releasewith A's original decision reaches bin/fm-captain-hold.sh:1244 and removes the only evidence the new guard checks.complete Bandverify Bthen accept A's answer without B ever being held or answered. Two sibling paths violate the same invariant: moving an already-active or date-expired A hold to B and interrupting before the backend operation bypasses verification at bin/fm-captain-hold.sh:538 because A's captain annotations remain; ordinarily completing released work after the failed move bypasses the stamp check at bin/fm-captain-hold.sh:542 because the task is now done. The shared cause remains bin/fm-captain-hold.sh:1017 publishing B before backend success, with bin/fm-captain-hold.sh:1031 allowing failed restoration; the origin comparison at bin/fm-captain-hold.sh:912 cannot distinguish these states. Both consumers, bin/fm-captain-hold.sh:1796 and bin/fm-captain-hold.sh:1863, inherit the false acceptance. Commit the origin only after backend hold success, as explicitly permitted by the intent, rather than treating removable body ordering as proof of provenance. Cover active/expired moves, old-answer replay, and normal work completion in the behavioral regression, and correct the guarantees at docs/captain-hold-lifecycle.md:69 and docs/captain-hold-lifecycle.md:71.🔧 Fix applied.
2 issues (1 error, 1 warning) still open:
bin/fm-captain-hold.sh:542- The durable fix still permits an incomplete origin move to pass completion and verification. Starting from the regression's state—A's released answer, B's stored origin and leading stamp, and a failed backend hold/rollback—retryinganswer --releasewith A's original decision reaches bin/fm-captain-hold.sh:1244 and removes the only evidence the new guard checks.complete Bandverify Bthen accept A's answer without B ever being held or answered. Two sibling paths violate the same invariant: moving an already-active or date-expired A hold to B and interrupting before the backend operation bypasses verification at bin/fm-captain-hold.sh:538 because A's captain annotations remain; ordinarily completing released work after the failed move bypasses the stamp check at bin/fm-captain-hold.sh:542 because the task is now done. The shared cause remains bin/fm-captain-hold.sh:1017 publishing B before backend success, with bin/fm-captain-hold.sh:1031 allowing failed restoration; the origin comparison at bin/fm-captain-hold.sh:912 cannot distinguish these states. Both consumers, bin/fm-captain-hold.sh:1796 and bin/fm-captain-hold.sh:1863, inherit the false acceptance. Commit the origin only after backend hold success, as explicitly permitted by the intent, rather than treating removable body ordering as proof of provenance. Cover active/expired moves, old-answer replay, and normal work completion in the behavioral regression, and correct the guarantees at docs/captain-hold-lifecycle.md:69 and docs/captain-hold-lifecycle.md:71.bin/fm-captain-hold.sh:1033- Round 1's fixer commit (bbd2fcf) introduced a post-hold exit that skips parent-channel delivery. In a secondmate home, let the backend hold succeed but refuse the subsequent origin-body update: the task remains captain-held, yet this exit bypasses publish_parent_hold at bin/fm-captain-hold.sh:1036, so no needs-decision event reaches the parent. This violates the existing script-owned delivery contract in docs/secondmate-parent-channel.md:21–28. New holds and re-holds share this path; origin staging failures at bin/fm-captain-hold.sh:846 and bin/fm-captain-hold.sh:849 and the update failure at bin/fm-captain-hold.sh:854 likewise bypass publication. Publish the successfully verified hold before attempting origin publication, while retaining the nonzero result when that publication fails. Exercise the existing origin-write failure case in a secondmate fixture and assert the parent event.🔧 Fix applied.
✅ Re-checked - no issues remain.
🔧 **Test** - 2 issues found → no changes applied ✅
git -C ~/.no-mistakes/worktrees/5e28e613310e/01M4B5GBJ9SPZSFZEVT47MEJ31 statusandgit -C ~/.no-mistakes/worktrees/5e28e613310e/01M4B5GBJ9SPZSFZEVT47MEJ31 diff). Respond with fix to validate it, or abort.🔧 No changes applied.
✅ Re-checked - no issues remain.
rm -rf -- .local-test-tmpremoved the explicitly identified untracked scratch.timeout 600 env TMPDIR=/tmp bash tests/fm-captain-hold-lifecycle.test.shcompleted the origin-ordering test, expanded interrupted-origin regression, and suite prefix before reaching the time cap.timeout 600 python3 ~/.no-mistakes/evidence/01M4B5GBJ9SPZSFZEVT47MEJ31/run-captain-hold-remaining.pycompleted the remaining captain-hold cases with exit 0 after fixing the continuation launcher's argument-size error.python3 ~/.no-mistakes/evidence/01M4B5GBJ9SPZSFZEVT47MEJ31/live-hold-origin.pydrove twelve real CLI scenarios using tasks-axi, process termination, filesystem permission failures, and local parent-channel delivery.python3 ~/.no-mistakes/evidence/01M4B5GBJ9SPZSFZEVT47MEJ31/reproduce-before-fix.pyreproduced incorrect completion and verification on base commit 47aff866dbe0612bd43df66d8fa76576e06a2b3e.git status --shortconfirmed no remaining worktree changes; disposable validation homes were removed.✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.