fix(bin): preserve hold reasons and reject invalid completion inventories - #6331
kunchenguid merged 7 commits into
Conversation
…mplete hold now stores a reason with parentheses, line breaks, or percent signs through a reversible percent encoding that every reader decodes, instead of refusing it. hold --origin records the origin on the held task, and complete refuses the origin as its own inventory entry and an entry held for a different origin; holds with no recorded origin are accepted and flagged.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sub decode_reason { | ||
| my ($value) = @_; | ||
| return $value unless defined($value) && $value =~ s/^fm-hold-v1://; | ||
| return decode("UTF-8", decode_base64($value), FB_CROAK); |
There was a problem hiding this comment.
An older, literal hold reason beginning with fm-hold-v1: is treated as encoded data even if this encoder never wrote it. For example, fm-hold-v1:bm9ydGg= displays as north; if the suffix decodes to invalid UTF-8, public task reads fail and the fleet snapshot can fail to load.
…us origin, and invalid base64/UTF-8 reasons remain verbatim. Added regressions and documented valid-literal ambiguity. Both failures were reproduced before fixes. Verification: 54 lifecycle tests and 9 wrapper tests passed; 7 Beads-specific cases skipped because tasks-axi is markdown-only. Focused lint and diff checks passed. No pipeline or publication actions performed
|
Speaking as Kun's firstmate: Verdict: Tip vs main 8f756bb reviewed. Body links #5712 but has no Closes #5712 / no GitHub closing reference — does NOT close an issue (add a closing keyword if that is the intent). Tip encodes hold reasons, records Captain hold origin, refuses self-inventory/wrong-origin on complete, restores previous origin on failed backend re-hold — restore matching ready-for-pr #5712. |
|
Reply to the review comment "Historical reasons can break reads" on A stored reason that starts with The remaining case, an older literal reason that happens to be a valid encoding such as |
|
Speaking as Kun's firstmate: Verdict: Tip vs main VISION.md (each rule): all align — authority via recorded origin; scripts own hold/complete mechanics; no new default-on captain surface. Greptile: P1 failed-re-hold thread resolved on tip. Historical-reason thread outdated; tip returns invalid base64/UTF-8 payloads verbatim (covered by tests); documented residual for hand-typed valid Attestation: MATCH. CI/NM: SUCCESS CLEAN. mergeable: MERGEABLE. Merging squash now. Firstmate flag: no. Security tip: none. |
|
Speaking as Kun's firstmate: this is merged. Thank you @mremond — really appreciate you taking the time on this. |
* test: preserve Pi calm transcript captures with Pi 1.0 (kunchenguid#6338) * test(calm): pin Pi's regular TUI mode where pane assertions read scrollback Pi 1.0.0 defaults its TUI to a fullscreen alternate-screen mode whose scrollable transcript is application-owned, so rows that leave the viewport never enter terminal scrollback and tmux capture-pane -S can no longer see them. The Pi Calm e2e launches now pass --tui-mode regular wherever the flag exists so the transcript assertions keep reading real scrollback on both the Pi 1.0.0 line and earlier Pi lines, which have no such flag and render regular-only anyway. * no-mistakes(document): Correct Pi TUI documentation and scrollback rationale * fix(bin): preserve hold reasons and reject invalid completion inventories (kunchenguid#6331) * fix(bin): encode captain-hold reasons and reject self-inventory in complete hold now stores a reason with parentheses, line breaks, or percent signs through a reversible percent encoding that every reader decodes, instead of refusing it. hold --origin records the origin on the held task, and complete refuses the origin as its own inventory entry and an entry held for a different origin; holds with no recorded origin are accepted and flagged. * fix(review): Decode marked hold reasons consistently across readers * fix(review): Remove unnecessary lifecycle test dispatch * fix(review): Correct hold origin identity and inventory recovery * fix(review): Record origins before placing backend holds * fix(document): Clarify captain-hold validation and reason reader documentation * fix(ci): Fixed both findings: failed backend holds restore the previous origin, and invalid base64/UTF-8 reasons remain verbatim. Added regressions and documented valid-literal ambiguity. Both failures were reproduced before fixes. Verification: 54 lifecycle tests and 9 wrapper tests passed; 7 Beads-specific cases skipped because tasks-axi is markdown-only. Focused lint and diff checks passed. No pipeline or publication actions performed * fix: reclaim orphaned watcher arms on the next park (kunchenguid#6335) * fix(bin): take over the watcher cycle a main-only pass-through leaves An attended main-only pass-through leaves a successor watcher cycle running through main's handling turn. The session's next park attached to that cycle instead of owning it, so the successor's arm, orphaned by its host's exit, kept owning the watcher while the new park's arm polled it twice a second until the next close or the park boundary, hours later in a quiet second mate. A second-mate restart hit this every time, since its persist request is a main-only close. The host now records the successor it leaves for main, and the next host's first cycle runs bin/fm-watch-arm.sh --take-over on it: when that arm still owns the healthy watcher, the new arm stops it, reports a reason the cycle delivered first, and otherwise owns a fresh cycle. The stop's own downtime publication is undone over an acknowledged episode when no wake was appended in between, so the handover wakes nobody. * no-mistakes(review): Keep left-arm record until the orphaned arm is gone * no-mistakes(review): Relinquish successor arm only after durably recording it * no-mistakes(review): Relinquish successor only after its record reads back * no-mistakes(document): Clarify successor takeover guarantees and authoritative documentation * no-mistakes(ci): Fixed ci-1: acknowledgement restore now requires the exact taken-over arm/watcher ledger row with signal=TERM, awaited within a short bound. Otherwise takeover proceeds without erasing downtime. Added a self-exit regression confirmed failing before the fix and passing afterward; ordinary takeover tests and the full watcher-arm suite pass. Updated Generation reuse documentation. Syntax, diff checks, and ShellCheck pass with existing SC1091/SC2034 warnings excluded. ci-2 remains unchanged * no-mistakes(document): Clarify watcher take-over recovery and restart limits * fix(bin): restore downtime on supervision-host hand-back when the successor already closed (kunchenguid#6355) * fix(bin): restore supervision host hand-back continuity * no-mistakes(review): Scope host hand-back failure fallback to lost pending:handling * no-mistakes(review): Remove stray scratch test copy tests/.tmp-rest.test.sh * no-mistakes(test): Initialise successor globals so early hand-back survives set -u * no-mistakes(document): Document host hand-back downtime failure and Claude lost-handback notice * no-mistakes(ci): I fixed the Greptile finding. The rule that must hold: when the supervision host hands back an actionable wake, its rewake is refused, and no watcher is healthy, the hand-back still has to reach main as a delivered notice. That must be true whether the recovery marker is `pending:handling` or `announced:handling`. Only one place applies this check: the lost hand-back fallback in `bin/fm-claude-stop-autoarm.sh`. **Fix:** that check now accepts both `pending:handling:*` and `announced:handling:*` tokens (a one-line change). Nothing else in the fallback changed: - Refusals on any other marker, such as an already acknowledged one, still exit 0 silently and open no failure episode. - The notice is still sent once per episode, and repeats are recorded as `failed-suppressed`. **Tests:** - `tests/fm-claude-stop-autoarm.test.sh`: the lost hand-back test now runs as a shared helper with two variants, one writing a `pending:handling` marker and a new one writing `announced:handling` (`test_host_lost_announced_handback_notifies_once_per_episode`). - `tests/fm-supervision-host.test.sh`: the end-to-end test where downtime restoration fails is now a shared helper too, with a new `announced` variant (`test_claude_stop_hook_notifies_when_closed_announced_successor_downtime_restore_fails`). It moves the handling episode to `announced` before the host hands back. Without the fix the hook would exit 0 here; the test requires exit 2, `outcome=failed` and a delivered failure notice. **Verification (all under nice -n 10):** - The full `tests/fm-claude-stop-autoarm.test.sh` suite passed (rc=0), including both lost hand-back variants and the benign-refusal test. - In `tests/fm-supervision-host.test.sh` I ran only the four hand-back test functions, all passing (rc=0). The suite can't run single functions, so I used a temporary copy with a trimmed test list and deleted it afterwards; `git status` shows only the 3 intended files changed. - shellcheck is clean on all three changed files. I did not run `bin/fm-lint.sh`. - I did not run the new tests against the unfixed code; the claim that they fail without the fix comes from reading the old check --------- Co-authored-by: Kun Chen <3233006+kunchenguid@users.noreply.github.com> Co-authored-by: Mickaël Rémond <mremond@process-one.net> Co-authored-by: Tiago <tiagop@hey.com>
2 participants
Intent
Launch upstream ready-for-pr issue #5712 as its own ship from current origin/main, through the usual upstream contribution path; the maintainer merges. Keep it scoped to its issue.
Substance of the issue and the maintainer's triage (contract-class restore):
bin/fm-captain-hold.sh hold --reasonrefuses any reason containing parentheses with "reason must not contain parentheses (tasks-axi hold contract)" and names no remedy, although parentheses are ordinary prose in a decision reason. Separately,bin/fm-captain-hold.sh complete <origin> <origin>accepts the origin task as its own captain-call inventory whenever the origin row looks durable, so a refused hold immediately before it (for example in a chain with pipefail off) goes unnoticed and the completion gate looks satisfied with no hold recorded. The maintainer left the issue open for a PR along the issue author's refined fix:holdstores the origin it was recorded for on the held task;completerefuses an inventory entry equal to the origin, and refuses an entry whose stored origin differs from the origin being completed, with older records that carry no stored origin falling back to the current check and flagged in the output; reason text is encoded where tasks-axi stores it instead of banning characters, so a reason containing parentheses, semicolons, quotes and a newline is stored and read back unchanged. Tests: a failed hold followed bycomplete <o> <o>exits non-zero; an entry held for a different origin is refused; a reason containing ( ) ; " and a newline round-trips unchanged. An optionalhold --completethat records hold and completion in one locked step is welcome but not required.What Changed
--originbefore applying a hold; makecompleteandverifyreject self-references and mismatched origins using backend identities, while completion flags legacy entries without recorded origins.Risk Assessment
✅ Low: The changes are bounded and follow the latest recorded decisions; the full static review identified no additional material defects.
Testing
Baseline defects reproduced; all targeted live Markdown CLI scenarios passed with tasks-axi 0.2.6. Corrected driver setup errors were re-tested. CLI transcripts and persisted output were captured; labs were removed. No full suite, linters, publication or CI phases were run.
Evidence: Live CLI transcripts
Source: Live CLI transcripts
Evidence: Fleet output with decoded reasons
Source: Fleet output with decoded reasons
Evidence: Validation results and cleanup record
Source: Validation results and cleanup record
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 5 issues found → auto-fixed (4) ✅
bin/fm-captain-hold.sh:956- A failed re-hold can certify the wrong origin. Hold task h for A, release it with a recorded answer, then re-hold h for B. This writes B before the backend hold succeeds; if that operation fails, complete B h accepts A's old answer with B's new association. Publish the association only with a successfully established hold. Related sites: bin/fm-captain-hold.sh:831 (origin write), :959 and :962 (fallible hold operations), :862 (durability/origin check), :1727 and :1794 (complete/verify consumers).bin/fm-captain-hold.sh:894- The required reason text must be “stored and read back unchanged,” but the diff passes --reason "$stored_reason" to tasks-axi while the routine public reader, bin/fm-tasks-axi.sh:140, still directly executes tasks-axi. Holding with reason '(north)\nNext' therefore exposes '%28north%29%0ANext' through show/list. Restore decoding at the shared public read boundary. Related changed sites: bin/fm-captain-hold.sh:959 and :962 (both writers), docs/captain-hold-lifecycle.md:67 (decoding promise), tests/fm-captain-hold-lifecycle.test.sh:4252 (round-trip assertion covers only fleet JSON).bin/fm-hold-reason-lib.sh:47- The blanket decoder changes text that this encoder never wrote. A task titled 'Investigate literal %28' becomes 'Investigate literal (' in startup output; an older hold containing a URL with %28 likewise changes in fleet JSON. No intent requirement calls for interpreting unrelated fields or legacy plain text as encoded reasons. Remove that blanket acceptance and restrict decoding to identifiable encoded reason fields. Related sites: bin/fm-session-start.sh:555 (entire listing), bin/fm-afk-return.sh:597 (entire rows), bin/fm-fleet-snapshot.sh:432 (all historical reasons), bin/fm-hold-reason-lib.sh:35 (parallel decoder).bin/fm-captain-hold.sh:849- The advertised recovery does not work for existing self-inventories. Before this change, complete o o persisted decision_keys=o. After upgrading, creating separate task c and following this message with complete o c still unions o into the inventory and refuses forever; --none also retains it. Distinguish this historical-record case and provide workable repair instructions while preserving self-inventory rejection. Related sites: bin/fm-captain-hold.sh:859 (resolved identity check), :1727 (verification of the union), :1794 (verify also blocks cleanup).bin/fm-hold-reason-lib.sh:33- Simplification: fm_hold_reason_decode has no callers. Production readers independently implement decoding in the stream filter and fleet jq expression, so this adds an unused parallel definition without satisfying an additional intent requirement. Remove the unused function from the final implementation.🔧 Fix applied.
5 issues (3 errors, 2 warnings) still open:
bin/fm-captain-hold.sh:956- A failed re-hold can certify the wrong origin. Hold task h for A, release it with a recorded answer, then re-hold h for B. This writes B before the backend hold succeeds; if that operation fails, complete B h accepts A's old answer with B's new association. Publish the association only with a successfully established hold. Related sites: bin/fm-captain-hold.sh:831 (origin write), :959 and :962 (fallible hold operations), :862 (durability/origin check), :1727 and :1794 (complete/verify consumers).bin/fm-captain-hold.sh:894- The required reason text must be “stored and read back unchanged,” but the diff passes --reason "$stored_reason" to tasks-axi while the routine public reader, bin/fm-tasks-axi.sh:140, still directly executes tasks-axi. Holding with reason '(north)\nNext' therefore exposes '%28north%29%0ANext' through show/list. Restore decoding at the shared public read boundary. Related changed sites: bin/fm-captain-hold.sh:959 and :962 (both writers), docs/captain-hold-lifecycle.md:67 (decoding promise), tests/fm-captain-hold-lifecycle.test.sh:4252 (round-trip assertion covers only fleet JSON).bin/fm-captain-hold.sh:849- The advertised recovery does not work for existing self-inventories. Before this change, complete o o persisted decision_keys=o. After upgrading, creating separate task c and following this message with complete o c still unions o into the inventory and refuses forever; --none also retains it. Distinguish this historical-record case and provide workable repair instructions while preserving self-inventory rejection. Related sites: bin/fm-captain-hold.sh:859 (resolved identity check), :1727 (verification of the union), :1794 (verify also blocks cleanup).bin/fm-captain-hold.sh:859- Self-inventory remains reachable through supported Beads aliases. With captain-held origin fm-o and no recorded origin,complete fm-o osucceeds: resolve_entry returns the requested spellingoeven when tasks-axi resolves it to fm-o. Both comparisons miss the identical row, and verify also accepts it. Compare backend-returned identities at verify_entry_durable. Related sites: bin/fm-captain-hold.sh:848 (initial comparison), :857 (requested identity), :865 (stored-origin comparison can also falsely reject equivalent spellings), :956 (origin storage), :1727 (complete), and :1794 (verify).tests/fm-captain-hold-lifecycle.test.sh:4361- Simplification: the Round 1 fixer introduced FM_TEST_ONLY, a new command-dispatch and early-exit mode. Neither issue fm-captain-hold: parentheses in --reason are refused without a remedy, and complete accepts the origin as its own inventory #5712 nor the approved R2/R3/R5 fixes require another test-selection option; the normal suite already invokes the regressions. Remove this added dispatch block to keep the fix round within the requested scope.🔧 Fix applied.
3 issues (2 errors, 1 warning) still open:
bin/fm-captain-hold.sh:956- A failed re-hold can certify the wrong origin. Hold task h for A, release it with a recorded answer, then re-hold h for B. This writes B before the backend hold succeeds; if that operation fails, complete B h accepts A's old answer with B's new association. Publish the association only with a successfully established hold. Related sites: bin/fm-captain-hold.sh:831 (origin write), :959 and :962 (fallible hold operations), :862 (durability/origin check), :1727 and :1794 (complete/verify consumers).bin/fm-captain-hold.sh:849- The advertised recovery does not work for existing self-inventories. Before this change, complete o o persisted decision_keys=o. After upgrading, creating separate task c and following this message with complete o c still unions o into the inventory and refuses forever; --none also retains it. Distinguish this historical-record case and provide workable repair instructions while preserving self-inventory rejection. Related sites: bin/fm-captain-hold.sh:859 (resolved identity check), :1727 (verification of the union), :1794 (verify also blocks cleanup).bin/fm-captain-hold.sh:859- Self-inventory remains reachable through supported Beads aliases. With captain-held origin fm-o and no recorded origin,complete fm-o osucceeds: resolve_entry returns the requested spellingoeven when tasks-axi resolves it to fm-o. Both comparisons miss the identical row, and verify also accepts it. Compare backend-returned identities at verify_entry_durable. Related sites: bin/fm-captain-hold.sh:848 (initial comparison), :857 (requested identity), :865 (stored-origin comparison can also falsely reject equivalent spellings), :956 (origin storage), :1727 (complete), and :1794 (verify).🔧 Fix applied.
4 issues (3 errors, 1 warning) still open:
bin/fm-captain-hold.sh:956- A failed re-hold can certify the wrong origin. Hold task h for A, release it with a recorded answer, then re-hold h for B. This writes B before the backend hold succeeds; if that operation fails, complete B h accepts A's old answer with B's new association. Publish the association only with a successfully established hold. Related sites: bin/fm-captain-hold.sh:831 (origin write), :959 and :962 (fallible hold operations), :862 (durability/origin check), :1727 and :1794 (complete/verify consumers).bin/fm-captain-hold.sh:849- The advertised recovery does not work for existing self-inventories. Before this change, complete o o persisted decision_keys=o. After upgrading, creating separate task c and following this message with complete o c still unions o into the inventory and refuses forever; --none also retains it. Distinguish this historical-record case and provide workable repair instructions while preserving self-inventory rejection. Related sites: bin/fm-captain-hold.sh:859 (resolved identity check), :1727 (verification of the union), :1794 (verify also blocks cleanup).bin/fm-captain-hold.sh:859- Self-inventory remains reachable through supported Beads aliases. With captain-held origin fm-o and no recorded origin,complete fm-o osucceeds: resolve_entry returns the requested spellingoeven when tasks-axi resolves it to fm-o. Both comparisons miss the identical row, and verify also accepts it. Compare backend-returned identities at verify_entry_durable. Related sites: bin/fm-captain-hold.sh:848 (initial comparison), :857 (requested identity), :865 (stored-origin comparison can also falsely reject equivalent spellings), :956 (origin storage), :1727 (complete), and :1794 (verify).bin/fm-captain-hold.sh:997- Round 3 moved R1's partial-write defect. For a newhold h --origin B, the backend hold can succeed and the subsequent origin lookup or body update fail. The remaining captain-held row has no recorded origin, socomplete A haccepts it through the legacy fallback despite A being unrelated. Re-holds can similarly retain a stale association. Related sites: bin/fm-captain-hold.sh:983 and bin/fm-captain-hold.sh:986 (hold writes), bin/fm-captain-hold.sh:825 and bin/fm-captain-hold.sh:843 (subsequent failures), bin/fm-captain-hold.sh:882 and bin/fm-captain-hold.sh:888 (origin checks), bin/fm-captain-hold.sh:1754 and bin/fm-captain-hold.sh:1821 (complete/verify consumers). Make incomplete origin-bearing holds distinguishable and reject them at verify_entry_durable until finalized. The remedy needs approval because reliably distinguishing interrupted writes requires durable transition state or an atomic storage change.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
python3 ~/.no-mistakes/evidence/01M3W33QQYV7FCM041WC2819ZB/live-validation.py, with targeted reruns after correcting driver setup.git archive 8f756bbc287c5bdfacc64a7cc09e8516c64fc919 bin .tasks.toml: executed the baseline commands and reproduced both original defects.Realfm-captain-hold.sh hold,answer,completeandverify, including process-localRLIMIT_FSIZEfailure injection.Realfm-tasks-axi.sh show/view/list,fm-fleet-snapshot.sh --json,fm-session-start.shandfm-afk-return.sh checkagainst disposable Markdown backlogs.git status --porcelainand workspace-local lab cleanup verification.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Closes #5712