fix(bin): attest existing Captain's Call references - #1987
Open
moneyteamameen wants to merge 4 commits into
Open
fix(bin): attest existing Captain's Call references#1987moneyteamameen wants to merge 4 commits into
moneyteamameen wants to merge 4 commits into
Conversation
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 the decision-hold completion lifecycle so a completed investigation or visual review can explicitly attest unresolved captain decisions already registered under earlier formal or legacy structured task identities, without duplicating, closing, or mutating those Captain's Call rows. Extend bin/fm-decision-hold.sh complete narrowly and fail closed with repeated explicit existing task identities; require live reviewing-origin metadata for durable, sorted, idempotent attestation; verify every newly referenced identity exists in the active authoritative backlog and is queued, unblocked, actively captain-held kind captain work; reject mixed invalid sets, missing or malformed identities, completed work, non-captain work, ordinary parked work, external holds, and blocked items with actionable errors before any attestation. Preserve referenced identity, title, body, dependencies, and open state, create no task, and preserve complete --none plus deterministic current-origin-key behavior. Keep verify and scout teardown compatible, retain the default tasks-axi and manual-backend contracts, and never weaken the rule that decisions must already be durable before completion. Cover a pre-existing formal hold, an arbitrary legacy structured hold, idempotent retry, multiple holds, every rejection class, absence of duplicate creation or mutation, unchanged existing flows, successful verify, and teardown gating. Keep exact syntax in the script header and help, policy only in the decision-hold skill, mechanism and regression evidence in the decision-hold document, do not change AGENTS.md, use one sentence per Markdown line and plain dashes, and add no agent co-author. The local focused, documentation, lint, coverage, and lifecycle suites are green; an unrelated Muse detection test fails only in the outer Codex process environment, so classify or correct it within pipeline isolation. Use branch fm/decision-hold-existing-reference, push and open a PR, wait for green checks, and do not merge.
What Changed
completeto accept repeated--existing <task-id>references alongside current-origin decision keys, recording sorted identities without creating or mutating referenced backlog items.Risk Assessment
✅ Low: The follow-up changes close all three previously identified source defects: blocker eligibility now follows raw dependency semantics, completion metadata is published by one same-directory atomic replacement, and the missing rejection and interrupted-publication regressions are covered without broadening behavior.
Testing
The focused lifecycle E2E suite and an independent manual-backend CLI walkthrough passed, covering help syntax, formal and legacy attestation, sorted idempotent metadata, preservation without duplication or mutation, all rejection classes, successful verify, and teardown gating; this is CLI-only behavior, so no visual artifact applies.
Evidence: Decision-hold existing-reference CLI walkthrough
Shows sorted formal/legacy attestation, unchanged referenced tasks and backlog, successful verification, idempotent retry, and atomic rejection of an external hold.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 3 issues found → auto-fixed ✅
bin/fm-decision-hold.sh:202- The required criterion says every new reference must be “queued, unblocked, actively captain-held” and blocked items must be rejected, but this trusts tasks-axi’s derivedblockedfield. That field treats a danglingblocked-byidentity as resolved, while Firstmate’s canonical fleet projection deliberately keeps missing blockers unresolved. A captain-held row withblocked-by: missingis therefore attested and teardown can proceed even though Bearings excludes it from Captain’s Call. Validate blocker readiness using the canonical Firstmate semantics and add the dangling-blocker regression.bin/fm-decision-hold.sh:390- The durable attestation is published in two appends:decisions_reviewed=1and keys first, thendecision_refs. If the second append fails or the process stops between them,verifysees a completed inventory with no references and teardown may remove the source. This contradicts the required “durable, sorted, idempotent attestation.” Publish keys, references, and the reviewed marker as one failure-atomic metadata update and cover an interrupted-write failure.tests/fm-decision-hold-lifecycle.test.sh:359- The required “every rejection class” coverage is incomplete: this fixture always creates live origin metadata and never exercises a kind-captain item that is inactive (held=no) or non-queued. Thus the new live-metadata, active-hold, and queued-state rejection branches have no regression evidence. Add those exact cases and assert both metadata and backlog remain unchanged.🔧 Fix: Fix atomic decision attestation and blocker validation
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-decision-hold-lifecycle.test.shbin/fm-decision-hold.sh --helpManual-backend CLI walkthrough usingcomplete demo-followup-review --existing demo-earlier-review-decision-route --existing legacy-demo-captain-call, followed byverify, duplicate/reordered retry, and mixed external-hold rejectionCompared backlog hashes and full referenced-task projections before and after completion; compared metadata hashes across retry and rejected completiongit status --shortafter testing✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix: Quote completed blocker state in lint-safe comparison
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.