Conversation
fm-decision-hold.sh resolve could never close a captain decision whose routed task carried more than one blocker. tasks-axi quotes a rendered value only when it needs to, so blocked_by renders unquoted for a single dependency and quoted for two; show_field kept the surrounding quotes, so the resolve and unblock membership tests tested ,"a,b", and matched no id, failing with "not durably blocked" against a genuinely present edge. Such a hold exited 1 and re-surfaced forever; a single-dependency hold passed only by accident, which is why this was not caught. show_field now strips one surrounding quote pair, once, for every field, fixing both the resolve path and the unblock loop in one place. verify_resolution_identity's resolution_prefix loses its now-stripped leading quote to match, since the same strip applies to the escaped body field; both changes are required together or the idempotent re-resolve identity path breaks. Regression coverage in tests/fm-decision-hold-lifecycle.test.sh: a hold routed to both a single-dependency task (unquoted render) and a multi-dependency task (quoted render) plus an unrelated blocker; the resolve of the multi-dependency routed task was seen failing against the unfixed code. Post-fix the hold reaches state done with its durable decision record, the single-dependency task is released, the multi-dependency task retains only the unrelated edge, and an identical re-resolve is idempotent.
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
Fix bin/fm-decision-hold.sh so a captain-decision hold with MULTIPLE dependencies can be resolved. Today such a hold can never close, so it re-surfaces forever and re-pesters the captain with decisions already made. Root cause: tasks-axi quotes a rendered value only when it needs to, so blocked_by renders unquoted for one dependency and quoted ("a,b") for two; show_field kept the surrounding quotes, so the resolve membership test (case ",$blocked,") and the unblock loop tested ,"a,b", and matched no id, failing with 'not durably blocked' against a genuinely present edge. A single-dependency hold passed only by accident, which is why the bug went unnoticed.
Chosen fix (deliberate): the centralized shape - strip one surrounding quote pair once inside show_field itself, which fixes BOTH the resolve path and the unblock loop in one place. This is coupled with a mandatory second change: verify_resolution_identity's resolution_prefix loses its now-stripped leading quote, because the same strip also applies to the escaped body field. Both hunks are required together or the idempotent re-resolve identity path breaks. I deliberately did NOT use the narrower shape that strips at the two command_resolve call sites only, because that leaves the unblock-loop path broken. Scope is strictly the two decision-hold files; salvage commit 011ce5a carried the same fix but also killed turn-end-guard work that was explicitly deliberately excluded.
Test discipline (required by this work): tests/fm-decision-hold-lifecycle.test.sh gains a failing-first regression that was observed RED against the unfixed code (error: 'routed task sample-multi-dep is not durably blocked by ...') before the fix. It builds a single-dep hold (unquoted render), a multi-dep hold (quoted render), and an unrelated blocker; asserts the rendered shapes; resolves the multi-dep routed task; then asserts the hold reaches state done with 'Resolution recorded by fm-decision-hold', the single-dep task is blocked:no, the multi-dep task still blocked:yes carrying ONLY the unrelated edge, and an identical re-resolve is idempotent (exercising the coupled identity path).
What Changed
show_fieldinbin/fm-decision-hold.shnow strips one surrounding quote pair from a rendered tasks-axi value. tasks-axi quotes a value only when it needs to, soblocked_byrendered unquoted for one dependency and quoted ("a,b") for two; the resolve membership test (case ",$blocked,") and the unblock loop matched no id against the quoted form and failed with "not durably blocked" against a genuinely present edge, so a multi-dependency hold could never close. Stripping centrally fixes both the resolve path and the unblock loop in one place.verify_resolution_identity'sresolution_prefixdrops its now-stripped leading quote, since the same strip applies to the escaped body field. This hunk is required together with the first one, or the idempotent re-resolve identity path breaks; the escaped newlines stay literal in the rendered value.tests/fm-decision-hold-lifecycle.test.shgains a regression case that builds a single-dep hold (unquoted render), a multi-dep hold (quoted render), and an unrelated blocker; it pins both rendered shapes, resolves the multi-dep routed task, and asserts the hold reachesdonewith the resolution record, the single-dep task isblocked: no, the multi-dep task still carries only the unrelated edge, and an identical re-resolve is idempotent.docs/decision-hold-lifecycle.mdclarifies that resolve clears only each routed task's edge to the hold, leaving other blockers intact.Risk Assessment
✅ Low: The change is a small, well-bounded two-hunk fix whose safety is provable from tasks-axi's TOON quoting rules (a value containing a quote is always quoted, so the strip cannot over-strip), both coupled hunks are present and consistent, scope matches the stated intent exactly with no unrelated work removed, and it ships a targeted regression test covering the quoted-render resolve, selective unblock, and idempotent re-resolve paths.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-decision-hold.sh:121- show_field strips tasks-axi's surrounding quotes but does not unescape, so command_hold's title equality check at line 268 still mismatches for any title containing a double quote or backslash. tasks-axi quotes AND escapes such values (title: "Choose the &fix(watcher): make check wakes lossless via watcher-side suppression kunchenguid/firstmate#34;north&fix(watcher): make check wakes lossless via watcher-side suppression kunchenguid/firstmate#34; route"), so after the strip the value is Choose the &fix(watcher): make check wakes lossless via watcher-side suppression kunchenguid/firstmate#34;north&fix(watcher): make check wakes lossless via watcher-side suppression kunchenguid/firstmate#34; route and never equals the caller's Choose the "north" route. The idempotentholdretry then fails permanently with 'existing captain hold <id> has a different title' - the same never-closes failure class this change fixes for blocked_by. Not a regression (pre-fix every quoted title mismatched, including ones merely containing a comma or colon; the strip shrinks the failing set), and it is outside the intent's declared resolve/unblock scope, so flagging rather than fixing. Options: unescape only at the title call site, or normalize the comparison. Escapes must stay literal for the body, which verify_resolution_identity parses via literal \n.⏭️ **Test** - skipped
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"docs/decision-hold-lifecycle.md:40- docs/decision-hold-lifecycle.md's "Verification record" (dated 2026-07-14) hand-copies the per-testok -transcript of tests/fm-decision-hold-lifecycle.test.sh. It was already incomplete at the base commit (it omits "terminal single-owner stale status decisions do not block empty inventory", which landed in the same commit cd218f2 that wrote the record) and this change adds a ninth case, so it now lags by two lines. I did not edit it: it is presented as the exact output of one dated run, so appending new lines under the old date would falsify it, and re-dating it would require re-running the full 71-script suite plus lint - an out-of-scope rewrite. Follow-up worth considering: either regenerate the block from a fresh run with a new date, or reduce it to the commands plus a pointer to the test files so it stops drifting on every added case.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.