From bdf6841ee885f1836efa4263fb2558a66fe4e90e Mon Sep 17 00:00:00 2001 From: Jacob Bassiri Date: Tue, 28 Jul 2026 17:22:55 -0400 Subject: [PATCH 1/2] fix(bin): resolve captain holds with multiple dependencies 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. --- bin/fm-decision-hold.sh | 16 ++++- tests/fm-decision-hold-lifecycle.test.sh | 84 ++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 3 deletions(-) diff --git a/bin/fm-decision-hold.sh b/bin/fm-decision-hold.sh index c770fe4772c..bdd36be1a55 100755 --- a/bin/fm-decision-hold.sh +++ b/bin/fm-decision-hold.sh @@ -111,8 +111,16 @@ task_show() { # } show_field() { # - local output=$1 field=$2 - printf '%s\n' "$output" | sed -n "s/^ $field: //p" | head -1 + # tasks-axi quotes a rendered value only when it needs to (a comma-joined + # blocked_by list, an escaped body, a placeholder "-"), so an unstripped value + # matches for a single dependency and never matches for two. Strip one + # surrounding pair here, once, rather than at each call site. + local output=$1 field=$2 value + value=$(printf '%s\n' "$output" | sed -n "s/^ $field: //p" | head -1) + case "$value" in + '"'*'"') value=${value#\"}; value=${value%\"} ;; + esac + printf '%s\n' "$value" } origin_exists_here() { # @@ -205,7 +213,9 @@ verify_hold_durable() { # verify_resolution_identity() { local id=$1 hold_body=$2 decision_digest=$3 routed_csv=$4 resolution_prefix resolution_fields recorded_digest recorded_routes - resolution_prefix='"Resolution recorded by fm-decision-hold.\nDecision digest: ' + # show_field strips tasks-axi's surrounding quotes, so this anchors on the body + # text itself; the escaped newlines are still literal in the rendered value. + resolution_prefix='Resolution recorded by fm-decision-hold.\nDecision digest: ' case "$hold_body" in "$resolution_prefix"*) resolution_fields=${hold_body#"$resolution_prefix"} ;; *) fail "captain hold $id has no retry identity record" ;; diff --git a/tests/fm-decision-hold-lifecycle.test.sh b/tests/fm-decision-hold-lifecycle.test.sh index 3bf9f4863b8..7687c270e84 100755 --- a/tests/fm-decision-hold-lifecycle.test.sh +++ b/tests/fm-decision-hold-lifecycle.test.sh @@ -410,6 +410,89 @@ test_terminal_single_owner_status_decision_does_not_block_empty_inventory() { pass "terminal single-owner stale status decisions do not block empty inventory" } +test_multi_dependency_hold_resolves_and_unblocks() { + local home id hold show single_dep multi_dep + home=$(make_home multi-dep-resolve) + id=sample-multi-review + mkdir -p "$home/data/$id" + tasks_in "$home" add "$id" "Investigate multi-dep sample" --kind scout --repo sample --start >/dev/null \ + || fail "could not create investigation fixture" + write_origin_meta "$home" "$id" + cat > "$home/state/$id.status" <<'EOF' +needs-decision [key=route]: choose route north or route south +done: report and visual review complete +EOF + printf '# Multi-dep sample review\n\nOne captain choice remains.\n' > "$home/data/$id/report.md" + + hold=$(run_decisions "$home" hold "$id" route \ + --title "Choose the sample route" --reason "captain route choice pending" --repo sample) \ + || fail "could not register the route hold" + run_decisions "$home" complete "$id" route >/dev/null \ + || fail "shared completion gate failed" + + # Dependent work: one task blocked ONLY by the hold (tasks-axi renders that + # blocked_by unquoted), one task blocked by the hold AND an unrelated blocker + # (a comma-joined list, which tasks-axi renders quoted). The quoted rendering is + # the case the resolve/unblock membership tests could never match. + tasks_in "$home" add sample-unrelated-blocker "Unrelated sample blocker" \ + --kind ship --repo sample >/dev/null \ + || fail "could not create unrelated blocker fixture" + tasks_in "$home" add sample-single-dep "Single-dependency dependent" \ + --kind ship --repo sample --blocked-by "$hold" >/dev/null \ + || fail "could not create single-dependency fixture" + tasks_in "$home" add sample-multi-dep "Multi-dependency dependent" \ + --kind ship --repo sample --blocked-by "$hold" >/dev/null \ + || fail "could not create multi-dependency fixture" + tasks_in "$home" block sample-multi-dep --by sample-unrelated-blocker >/dev/null \ + || fail "could not add the unrelated blocking edge" + + # Pin the rendered shapes the bug depends on: single-dep unquoted, multi-dep quoted. + single_dep=$(tasks_in "$home" show sample-single-dep --full | sed -n 's/^ blocked_by: //p' | head -1) + multi_dep=$(tasks_in "$home" show sample-multi-dep --full | sed -n 's/^ blocked_by: //p' | head -1) + [ "$single_dep" = "$hold" ] \ + || fail "single-dependency blocked_by did not render unquoted: $single_dep" + [ "$multi_dep" = "\"$hold,sample-unrelated-blocker\"" ] \ + || fail "multi-dependency blocked_by did not render quoted: $multi_dep" + + printf 'Use route north for the sample system.\n' > "$home/route-decision.txt" + + # Load-bearing case: the routed set includes the quoted multi-dep task. On unfixed + # code show_field keeps the surrounding quotes, so the membership test tests + # ,"a,b", and matches no id, and resolve exits non-zero ("not durably blocked"). + # The single-dep task passes by accident, so it is never the only routed task. + run_decisions "$home" resolve "$id" route --decision-file "$home/route-decision.txt" \ + --routed-to sample-single-dep --routed-to sample-multi-dep >/dev/null \ + || fail "resolve could not close a hold with a multi-dependency routed task" + + # The hold is durably resolved. + show=$(tasks_in "$home" show "$hold" --full) + assert_contains "$show" "state: done" "multi-dep resolution did not close the hold" + assert_contains "$show" "Resolution recorded by fm-decision-hold" \ + "resolved hold lost its durable decision record" + + # Unblock path: the hold edge is cleared from both dependents; the single-dep task + # is fully released, the multi-dep task retains ONLY the unrelated edge. This + # exercises the second membership test (the unblock loop) against the quoted render. + show=$(tasks_in "$home" show sample-single-dep --full) + assert_contains "$show" "blocked: no" "single-dependency task was not released" + show=$(tasks_in "$home" show sample-multi-dep --full) + assert_contains "$show" "blocked: yes" "multi-dependency task lost its unrelated blocker" + multi_dep=$(printf '%s\n' "$show" | sed -n 's/^ blocked_by: //p' | head -1) + [ "$multi_dep" = "sample-unrelated-blocker" ] \ + || fail "multi-dependency task did not retain ONLY the unrelated edge: $multi_dep" + + # Idempotent re-resolve exercises the coupled identity check: show_field now strips + # tasks-axi's surrounding quotes from the body, so verify_resolution_identity must + # anchor without the leading quote. A quote-strip that omits that coupled change + # would leave the body carrying a leading quote the prefix no longer expects and + # break this re-resolve path. + run_decisions "$home" resolve "$id" route --decision-file "$home/route-decision.txt" \ + --routed-to sample-single-dep --routed-to sample-multi-dep >/dev/null \ + || fail "identical multi-dep resolution retry was not idempotent" + + pass "multi-dependency captain holds resolve, unblock only the hold edge, and re-resolve idempotently" +} + test_secondmate_hold_stays_in_authoritative_home() { local parent mate origin hold json parent=$(make_home main-routing) @@ -462,4 +545,5 @@ test_origin_slug_validation_precedes_path_construction test_visual_review_uses_shared_completion_owner test_none_inventory_and_resolved_prose_do_not_create_holds test_terminal_single_owner_status_decision_does_not_block_empty_inventory +test_multi_dependency_hold_resolves_and_unblocks test_secondmate_hold_stays_in_authoritative_home From 9ab64286a8e767c3ef251481a6e5cf29950d5475 Mon Sep 17 00:00:00 2001 From: Jacob Bassiri Date: Tue, 28 Jul 2026 18:00:21 -0400 Subject: [PATCH 2/2] no-mistakes(document): docs: clarify resolve clears only the hold edge --- docs/decision-hold-lifecycle.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/decision-hold-lifecycle.md b/docs/decision-hold-lifecycle.md index b5ed682d6d9..e025ac5a33a 100644 --- a/docs/decision-hold-lifecycle.md +++ b/docs/decision-hold-lifecycle.md @@ -24,7 +24,8 @@ Scout teardown calls the script's read-only `verify` subcommand after checking f The `--force` path remains the explicit captain-approved discard escape hatch. The `resolve` subcommand requires a decision file and at least one existing dependent task whose structured `blocked-by` edge points to the hold. -It records the decision digest and routed task identities as a retry identity in the hold body, clears each dependency edge through tasks-axi, and marks the hold Done only after those writes succeed. +It records the decision digest and routed task identities as a retry identity in the hold body, clears each routed task's edge to the hold through tasks-axi, and marks the hold Done only after those writes succeed. +A routed task may carry other blockers; only its edge to the hold is cleared, so such a task stays blocked by whatever else it depends on. An exact retry can finish a partial routing operation, while a changed decision or routed-task set is rejected. A failed intermediate step leaves the hold open.