fix(bin): stop dropping captain answers and churning on declared pauses - #2749
fix(bin): stop dropping captain answers and churning on declared pauses#2749Tiraso12 wants to merge 3 commits into
Conversation
|
Speaking as Kun's firstmate: Corrective — two named defects: (1) a live crew's declared VISION: aligns (obligations close by records; a captain answer must not fall through the cracks; idle declared waits must not spend tokens). Security: no. Overlap (do not land together): This is waiting on the author, not the captain. Fork CI is approved (runs 32508738381 CI now in_progress; 32508738229 Require no-mistakes). no-mistakes is currently red: the body has the signature line but is missing the structured |
|
Speaking as Kun's firstmate: Two stacked fixes: (1) absorb a live crew's declared pause on the long cadence; (2) render the fold's decision key and deliver a captain answer even when a named key is not open (nonzero, close nothing). Class: (1) is corrective. (2) is a default send-path change — delivery no longer cancels on a missing key. That is waiting on CI, not on a captain decision. VISION: pause absorb aligns (token efficiency, not always-on noise). Send-key honesty aligns (never drop a captain answer). Cannot yet land: no-mistakes/CI must finish, and this overlaps #2748 on Waiting on CI/author rebase after the overlap lands — not waiting on the captain. |
A crew that declares `paused:` is idling on a known external wait, so its stale pane must be absorbed and rechecked only on the long pause cadence. `pause_state_class` instead re-read agent liveness on every distinct stale hash and let a live or unreadable agent override the authoritative paused verdict, returning `none`. Because a held dev server repaints, every repaint was a fresh first sighting, so the stale path queued another contentless `stale: <window>` wake every minute or two while `.watch-triage.log` simultaneously logged the same pane as absorbed. The authoritative crew-state read now decides first, and liveness only breaks the remaining tie: a `paused:` or `captain-held` status whose crew `fm-crew-state.sh` can no longer confirm still surfaces once unless the agent is confirmed dead, so a captain hold over a crew that never declared the wait itself cannot silence a live decision gate. Once the bounded cadence is granted, the cheap `.paused-<key>` path holds it without re-deciding per stale hash until its recheck marker ages out. Regression tests cover the three boundaries: a live declared pause absorbs every churny stale hash with no queued wake, a live idle crew without a declared pause still surfaces on the unchanged schedule, and a declared pause past the bounded window still re-surfaces once as a labeled recheck. The existing live-gate case moves to `captain-held`, where the surface-once-when-unconfirmable rule actually applies.
The OPEN DECISIONS section printed no key at all whenever the fold landed on the shared "default" bucket. A decision whose note text still carried a "[key=...]" token therefore advertised that token as its key, and the section's single generic hint invited answering with it. fm-send refused that key - and because the refusal cancelled the send, the captain's answer was dropped rather than delivered, leaving a real decision open until someone noticed and resent it by hand. Every entry now renders the key the fold decided, "default" included, and carries its own ready-to-run close command underneath, so no key has to be inferred from note prose; a "[key=...]" inside a note stays quoted verbatim as the worker's own words. Note and command are charged to the byte cap together, so an entry is dropped whole rather than listed with no way to close it, and a task id outside the plain-slug charset gets a pointer instead of a string an agent would paste into a shell. fm-send now delivers the answer even when a named key is not open. That is a deliberate change to the safety property, not a convenience relaxation. The mis-state the flag exists to prevent is a decision that looks answered while its record stays open, and cancelling the send never prevented it - it produced the worse version, where the decision stayed open AND no worker ever received the answer the captain believed was given. Delivering the text is no more dangerous than the plain steer that is always allowed, because the flag's power is the close, and that is exactly what an unmatched key still withholds. "Delivered, not closed" is also the outcome the post-delivery append-failure path already treats as the safe direction, since an open decision re-surfaces on every later presentation. The diagnostic stays loud: it exits nonzero, names the key, and says not to resend, while any other named key that is open still closes. Malformed usage - a key outside the slug charset, a duplicate key, --key, an explicit backend target, an empty message - is still refused before anything is sent, because those are argument errors with no answer to deliver. The generated briefs now show the keyed form explicitly, so what a worker is told to write matches what the fold reads. The parse itself already honors the inline marker after kunchenguid#2202; the regression tests cover the inline-marker form and the keyless fallback, and prove the agreement end to end by replaying the drain's own printed commands through the real fm-send.
…nmatched-key behavior
47eaace to
a94147d
Compare
2 participants
Intent
Rebuild two finished-but-never-published firstmate fixes on top of the latest origin/main and ship them as ONE pull request.
The two fixes, as two separate commits with their original commit messages preserved verbatim:
This is a faithful rebuild on a new base, NOT a redesign. Do not simplify, re-litigate, weaken or "improve" either fix; both were already reviewed and finished. In particular do not restore the old refuse-before-sending behavior in fm-send, and do not revert the OPEN DECISIONS per-entry close command.
Upstream had moved 6 commits ahead and now touches all four scripts (including the rename of the decision-hold concept to captain-held tasks). Conflicts were resolved by keeping upstream new content AND the fix additions, never dropping either side; the fm-send comment and error text were adapted to upstream captain-held naming and to upstream fm_send_hold_resolved_id.
One necessary completion beyond the original commits: tests/fm-wake-drain-unread-status.test.sh asserted the OLD generic OPEN DECISIONS hint text, which fix 2 deliberately replaces with a per-entry close command. That stale assertion was updated to assert the new per-entry command for the same decision. Keep that; do not restore the old hint string.
Acceptance: branch based on latest origin/main carrying both fixes as two commits with original messages intact, behavior preserved, their regression tests passing, full test suite passing, and ONE PR containing both fixes with CI green. Do not split the fixes into separate PRs and do not merge the PR.
Publishing constraint: the captain does not own kunchenguid/firstmate, so branches publish from the Tiraso12/firstmate fork (already registered as the no-mistakes fork push target) and the PR opens from that fork into kunchenguid/firstmate. Do not rewrite the origin remote.
Known unrelated pre-existing failure, not to be fixed here: tests/fm-backend-herdr-focus-flash-e2e.test.sh fails locally against installed herdr 0.7.5 (a known upstream herdr bug the test documents). It references none of the changed files and skips cleanly on CI where herdr is absent.
What Changed
bin/fm-watch.sh:pause_state_classnow lets the authoritative crew-state read decide first and uses agent liveness only to break the remaining tie, so a live crew that declaredpaused:stays absorbed on the long pause cadence instead of queuing a contentlessstale:wake on every repaint; a paused or captain-held status whose crew can no longer be confirmed still surfaces once unless the agent is confirmed dead, and the cheap.paused-<key>path no longer re-decides per stale hash.bin/fm-wake-drain.sh: every OPEN DECISIONS entry now prints the key the fold decided (including the shareddefaultbucket) plus its own ready-to-run close command, charged to the byte cap together with the note so an entry is dropped whole rather than listed with no way to close it; the generic section hint is replaced by one stating a[key=...]inside note text is never the key, and a task id outside the plain-slug charset gets a pointer instead of a pasteable string.bin/fm-send.sh:--resolve-keywith a key that is open in neither ledger now still delivers the answer and closes any other open named keys, exiting nonzero with a loud stderr diagnostic that names the key and says not to resend, instead of cancelling the send and dropping the answer; malformed usage (non-slug key, duplicate key,--key, explicit backend target, empty message) is still refused before anything is sent.bin/fm-brief.shnow teaches the keyedneeds-decision [key=<slug>]:form, anddocs/architecture.mdis aligned with the new delivery behavior.Regression coverage was extended across
tests/fm-watch-triage.test.sh,tests/fm-send-resolve-key.test.sh, andtests/fm-wake-drain-open-decisions.test.sh(which replays the drain's own printed close commands through the realfm-send);tests/fm-wake-drain-unread-status.test.shwas updated to assert the new per-entry close command in place of the removed generic hint. Review raised two informational notes only: the per-entry command consumes part of the unchanged 4000-byte listing budget (omissions are still disclosed and re-surface), and a non-slug--resolve-keyvalue remains refused before delivery by design.Risk Assessment
✅ Low: The branch is a verified faithful rebuild of two already-reviewed fixes onto the exact origin/main head, with commit messages intact, conflict resolutions that kept both upstream and fix content, and new regression tests covering each behavioral boundary; the two deliberate safety-property changes are explicitly authorized by the stated intent and no contradicting behavior was introduced.
Testing
Ran the four regression suites the two commits touch plus four adjacent owners of the changed scripts (fm-brief, fm-send-strict, fm-classify-decision-key, fm-captain-hold-lifecycle) - all green - then proved both fixes end-to-end the way a captain experiences them, by building a base-commit copy of the four changed scripts and running the same scenario against base and branch. The decision transcript reproduces the reported bug on base (answer refused, pane log empty, decision still open) and shows the branch rendering the folded key with a per-entry close command that delivers and closes, plus the deliberate unmatched-key behavior (delivered, nothing closed, exit 1). The watcher transcript shows 4 contentless stale wakes on base versus 0 on the branch across 8 dev-server repaints, with both counter-guardrails still surfacing. This is a shell/CLI product with no rendered UI surface, so the reviewer-visible evidence is CLI transcripts and persisted state (status ledger, wake queue, triage log) rather than screenshots. Per the phase boundary I did not run the full suite (CI owns that) and did not run tests/fm-backend-herdr-focus-flash-e2e.test.sh, the known unrelated pre-existing local failure. No actionable issues found.
Evidence: Captain flow before/after: OPEN DECISIONS key + close command, and fm-send delivery
BEFORE (base 4d2cb0c) $ bin/fm-wake-drain.sh totals-pool needs-decision: totals pool separation - display-only or engine bug? [key=totals-pool-separation] OPEN DECISIONS: close one by answering it: bin/fm-send.sh <task> --resolve-key <key> '<answer>' $ bin/fm-send.sh totals-pool --resolve-key totals-pool-separation '...' error: --resolve-key 'totals-pool-separation': no open decision ... nothing was sent. [exit 1] $ cat send.log -> (EMPTY - the captain's answer was never delivered) $ bin/fm-wake-drain.sh -> decision STILL OPEN AFTER (518ed18) $ bin/fm-wake-drain.sh totals-pool [key=default] needs-decision: totals pool separation - display-only or engine bug? [key=totals-pool-separation] close it: bin/fm-send.sh totals-pool --resolve-key default '<answer>' OPEN DECISIONS: answer each one with the close command printed under it; the key that closes a decision is the bracketed key before its verb, never a [key=...] inside the note text. $ bin/fm-send.sh totals-pool --resolve-key default '...' [exit 0] $ cat send.log -> display-only - fix the view, leave the engine alone $ cat state/totals-pool.status -> resolved [key=default]: answered: display-only - fix the view, leave the engine alone $ bin/fm-wake-drain.sh -> (nothing - no decision left open) AFTER, captain types the WRONG key (the token inside the note) warning: --resolve-key 'totals-pool-separation': ... The answer is still being delivered; nothing will be closed for that key. error: the answer was delivered to sess:fm-totals-pool, but no decision was closed ... Do not resend the answer. [exit 1] $ cat send.log -> display-only - fix the view, leave the engine alone (delivered) $ bin/fm-wake-drain.sh -> decision still open, with its real close commandEvidence: Watcher before/after: a live crew's declared pause across 8 dev-server repaints
status line: paused: holding the dev server while the captain reviews BEFORE (base 4d2cb0c) repaint 1 -> absorbed repaint 2 -> WATCHER EXITED and woke firstmate with: 'stale: test:fm-held' repaint 3 -> absorbed repaint 4 -> WATCHER EXITED and woke firstmate with: 'stale: test:fm-held' repaint 5 -> absorbed repaint 6 -> WATCHER EXITED and woke firstmate with: 'stale: test:fm-held' repaint 7 -> absorbed repaint 8 -> WATCHER EXITED and woke firstmate with: 'stale: test:fm-held' supervision turns this quiescent captain review cost: 4 AFTER (4d98859) repaint 1..8 -> absorbed (no wake; pause cadence marker: present) supervision turns this quiescent captain review cost: 0 $ tail -3 state/.watch-triage.log absorbed stale (paused, awaiting external, age 9s): test:fm-held absorbed stale (paused, awaiting external, age 10s): test:fm-held absorbed stale (paused, awaiting external, age 11s): test:fm-heldEvidence: Watcher guardrails on the branch (the absorb does not silence real waits)
--- live idle crew, NO declared pause (must still surface) status line: working: wiring the dev server watcher outcome: WOKE firstmate -> 'stale: test:fm-case' --- declared pause older than the bounded window (must re-surface once, labeled) status line: paused: holding the dev server while the captain reviews watcher outcome: WOKE firstmate -> 'stale: test:fm-case (paused 502s, awaiting external - declared pause, rechecked on a long cadence not a wedge; confirm the wait still holds)'Evidence: Generated crewmate brief now teaches the keyed needs-decision form
bin/fm-brief.sh demo-task acme --mode no-mistakes -> 6. If a decision belongs above the implementation worker (product choices, destructive actions, ask-user findings), appendneeds-decision: {summary of options}and stop. Firstmate will apply the configured authority and reply with the decision. Give it a key when more than one decision or blocker can be open at once:needs-decision [key=<slug>]: {summary}.Evidence: Targeted test runs (regression suites + adjacent owners of the changed scripts)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-wake-drain.sh:176- Each entry now charges its close command (~55-80 bytes) against the unchanged 4000-byte global budget, so a presentation lists roughly 20-25% fewer open decisions before hitting the cap. Nothing is silently lost - the 'N more omitted (byte cap)' disclosure still prints and omitted decisions re-surface once earlier ones close - but if decision pressure matters, global_bytes was not raised to absorb the new per-entry cost. Charging note+command together is itself correct and deliberate (an entry is dropped whole rather than listed with no way to close it).bin/fm-send.sh:351- The authorized failure (a refusal cancelling the send and dropping the captain's answer) is fixed for every ledger-state mismatch, but one residual path remains: a --resolve-key value outside the slug charset is still refused before delivery, so an agent that copies a non-slug key-shaped token out of note prose (e.g. a note reading 'pick a [key=red theme]') still loses the answer entirely. This is the boundary the commit message deliberately draws ('Malformed usage IS refused before anything is sent ... argument errors with no answer to deliver'), and fix 2 removes the inference that produced it by printing an exact per-entry close command and stating that a [key=...] inside note text is never the key. Noting the residual path only; no change requested.✅ **Test** - passed
✅ No issues found.
bin/fm-test-run.sh tests/fm-watch-triage.test.sh tests/fm-send-resolve-key.test.sh tests/fm-wake-drain-open-decisions.test.sh tests/fm-wake-drain-unread-status.test.sh- the four suites the two commits touch, all pass (includes the new live-declared-pause churn/guardrail cases, the unmatched-key delivery case, and the drain-command replay through the real fm-send)bin/fm-test-run.sh tests/fm-brief.test.sh tests/fm-send-strict.test.sh tests/fm-classify-decision-key.test.sh tests/fm-captain-hold-lifecycle.test.sh- adjacent owners of the four changed scripts (brief scaffold, fm-send argument refusals, the decision-key fold, captain-held resolution), all passManual E2E captain flow, before vs after: realbin/fm-wake-drain.sh+bin/fm-send.sh(stubbed tmux pane, FM_GATE_REFUSE_BYPASS=1 as the suite's own helpers do) over a keylessneeds-decision:whose note carries a[key=...]token, run against a base-commit copy of the four changed scripts and against the branch; asserted the printed listing, the pane's received text, the status ledger, and whether the decision still lists as openManual E2E unmatched-key leg on the branch:bin/fm-send.sh totals-pool --resolve-key totals-pool-separation '<answer>'- answer delivered to the pane, loud warning + error on stderr, exit 1, decision still listed open with its real close commandManual E2E watcher churn, before vs after: realbin/fm-watch.shdriven over 8 repaints of a pane held by a crew that declaredpaused:, draining and acking each wake like a captain session would, counting queuedstale:wakes and supervision turnsManual E2E watcher guardrails on the branch: live idle crew with no declared pause still surfacesstale: test:fm-case; a declared pause aged past FM_PAUSE_RESURFACE_SECS re-surfaces once as... (paused 502s, awaiting external - declared pause, rechecked on a long cadence not a wedge...)bin/fm-brief.sh demo-task acme --mode no-mistakes- generated a real brief and confirmed rule 6 now teachesneeds-decision [key=<slug>]: {summary}git status --porcelain- worktree clean after testing; demo scratch trees under /tmp removed✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.