fix(bin): let a declared pause outrank a done or cancelled run - #1057
Closed
yomi202703 wants to merge 3 commits into
Closed
yomi202703 wants to merge 3 commits into
yomi202703 wants to merge 3 commits into
Conversation
When an attributed no-mistakes run is already terminal (done/failed, including cancelled) and the status log ends with paused:, report paused from the status log and keep the terminal run detail as secondary text. Active and parked runs still win over a pause line. This stops a stale historical validation record from permanently overruling a fresher self-declared external wait, without widening absorption for live or working crews.
…ep failures visible
Owner
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for firstmate#1057 at |
Owner
|
Automated reminder: this PR still looks blocked on a rebase or merge conflict fix. If you are still interested, please rebase onto the current base branch, resolve the conflict, and push. If I do not hear back, I may close this as inactive. |
Owner
|
Speaking as Kun's firstmate: closing this as stale. It has been waiting on a contributor update for 14+ days with no author push or comment. Reopen if you want to pick it back up. |
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
Let a crew's declared
paused:survive an attributed terminal historical no-mistakes validation run, so a correct pause is not permanently overruled by a stale run record.Correctness fix only (not a claimed 75-second stale loop): when
bin/fm-crew-state.shhasHAVE_RUN=1for a true terminal outcome of done or cancelled (not the mapped presentation state) and the status log's last verb ispaused:, emitstate: pausedwithsource: status-log, retaining the terminal run outcome as leading secondary detail. Mirror the existing CI-green status-log override shape for that emit style.What outranks a pause, by design:
cancelled) is a supervisor decision; overriding it with the crew's own pause declaration is sound. A failed run is the work itself failing and must surface whether or not any pause line is fresher. That restriction is the final accepted scope, not a temporary fallback.Rejected: changing only
crew_absorb_classorpause_state_class(dual-authority drift); stopping terminal attribution entirely; TTL on cancelled runs; teardown on abort. Residuals left by design: live-agent first surface, no-verb turn-end while paused, hourly forgotten-pause re-surface.Tests cover cancelled/done + trailing paused (override), failed + trailing paused (still failed), and negatives that working/parked/ci-green-monitoring still win. No claim this eliminates residual first-surface or signal spam.
What Changed
bin/fm-crew-state.shtracks the attributed run's own terminal outcome (RUN_TERMINAL: none/done/cancelled/failed) alongside the mappedRUN_STATE, set only from the run's reported outcome/status across the coarse, outcome, and bare-status paths (not from CI-green presentation mapping).doneorcancelledand the status log's last verb ispaused:, the crew is emitted asstate: paused · source: status-logwith the run outcome leading the detail and the pause reason following (and no doubled separator when the pause carries no reason).failedrun continues to reportfailedeven if the last status verb ispaused:, so a validation failure is not demoted to a quiet external wait. Active, parked, and CI-green-still-monitoring runs likewise outrank pause.tests/fm-crew-state.test.shadds cases for cancelled/done + paused, bare paused separator, failed-stays-failed, and active/parked/ci-green negatives;docs/architecture.mddocuments the second status-log exception andAGENTS.mdpoints at the script header as the owner of the run-step-to-state mapping instead of restating it.Risk Assessment
Low: Review-driven corrections gate the override on true terminal outcomes (so mapped CI-green done stays active authority), keep genuine failures visible by design, put terminal detail first for truncation, fix the empty-note separator, and document the exception. Scope is narrower than the first draft intent:
failedis intentionally not pause-overridable.Testing
Exercised the targeted crew-state suite (all cases pass, including the new pause-vs-terminal-run ones) and the watch-triage suite that consumes the verdict, then demonstrated the change end-to-end on a hermetic fleet fixture: with a cancelled run plus a declared pause the helper flips from
state: failed · source: run-step · run cancelledtostate: paused · source: status-log · run cancelled · holding for upstream maintainer merge, the watcher's absorb class flips fromnone(wake surfaces) topaused(absorbed), and the humanfm-fleet-view.shtable cell flips fromfailed / run-steptopaused / status-log; the done/checks-passed case behaves the same, while failed, active, and parked runs stay failed/working/parked with or without a trailing pause line. No screenshot applies - the affected surfaces are shell CLI output and a Markdown fleet table, both captured verbatim in the transcript artifact.Evidence: End-to-end before/after transcript (crew-state verdict, watcher absorb class, rendered fleet view)
SCENARIO: supervisor CANCELLED the validation run, crew declared a pause status log (tail): paused: holding for upstream maintainer merge BEFORE (c64ad1c) crew-state : state: failed · source: run-step · run cancelled BEFORE (c64ad1c) absorb : none AFTER (HEAD) crew-state : state: paused · source: status-log · run cancelled · holding for upstream maintainer merge AFTER (HEAD) absorb : paused SCENARIO: run finished DONE (checks green) , crew declared a pause BEFORE (c64ad1c) crew-state : state: done · source: run-step · checks green: PR ready for review AFTER (HEAD) crew-state : state: paused · source: status-log · checks green: PR ready for review · holding for upstream maintainer merge SCENARIO: run FAILED (must stay visible) + same pause line BEFORE (c64ad1c) crew-state : state: failed · source: run-step · run failed AFTER (HEAD) crew-state : state: failed · source: run-step · run failed SCENARIO: run still ACTIVE (working) + same pause line AFTER (HEAD) crew-state : state: working · source: run-step · validating (running) HUMAN FLEET VIEW (bin/fm-fleet-view.sh) - cancelled run + declared pause --- BEFORE (c64ad1c) --- | upstream-merge | failed / run-step | ship | alpha | tmux | present | https://github.com/o/r/pull/12 | ... | --- AFTER (HEAD) --- | upstream-merge | paused / status-log | ship | alpha | tmux | present | https://github.com/o/r/pull/12 | ... |Evidence: Reproduction driver for the end-to-end evidence (ROOT=<repo> OUT=<dir> bash e2e-pause-vs-terminal-run.sh)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-crew-state.sh:607- The new override keys on the mapped RUN_STATE (failed|done), not on whether the run actually reached a terminal outcome. RUN_STATE is also set todoneat bin/fm-crew-state.sh:557 for a run whose status is stillci/runningwhen the ci-step log tail reads green (detail: "checks green: PR ready for review (still monitoring for merge/close)") - that run is active, not historical. The intent states as required: "Active/working and parked runs must still outrank a declared pause", and the new header comment itself says the override applies "when the attributed run is already TERMINAL"; neither holds for this path. Failure scenario: a crew appendedpaused: awaiting maintainer mergewhile its run is still in the CI-monitor phase; checks go green, so RUN_STATE becomesdoneat line 557, the new case at line 607 matches, and the crew is reportedstate: paused · source: status-loginstead ofdone- the fleet/bearings view loses the "PR ready for review" signal for an actively-monitored run. Gating on the terminal branches themselves (outcome passed/checks-passed/failed/cancelled, coarse completed/failed/cancelled, or bare status completed/failed/cancelled) rather than on the post-override RUN_STATE would keep the ci-green case out.bin/fm-crew-state.sh:608- The override has no ordering or freshness signal: it only asks whether the status log's LAST verb ispaused:. It therefore cannot distinguish the motivating case (a pause declared AFTER a historical run ended) from a pause declared BEFORE or DURING a run that subsequently failed - the intent's own framing is "a crew's freshly declared paused:". Because crews append sparsely during validation (AGENTS.md status contract), a pause line stays last for the whole run. Failure scenario: crew appendspaused: waiting on vendor rate-limit resetwhile its no-mistakes run is in ci; CI goes red and the run endsoutcome: failed; crew-state now reportsstate: pausedwith the failure demoted to trailing detail, so the fleet snapshot/bearings surface a benign external wait and the real validation failure is only re-checked on the hourly pause cadence instead of surfacing asfailed. This masking is not among the residuals the intent lists as accepted (live-agent first surface, no-verb turn-end while paused, hourly forgotten-pause re-surface), so it needs an explicit call. Note TTL was rejected, but a status-file-mtime vs run-attribution ordering check is a different mechanism if you want one.bin/fm-crew-state.sh:609- The retained terminal outcome is appended LAST in the emitted detail (<pause note> · <run detail>), which makes it the first text dropped by downstream truncation. bin/fm-bearings-snapshot.sh:378-379 renderscurrent_state.detailthroughtrunc(90)for the in-flight rows. Failure scenario: a crew withpaused: holding until the upstream vendor publishes the 2.4 release and the mirror re-syncsplus a cancelled run emits detail of ~95+ chars, so bearings shows only the pause reason with an ellipsis and the "run cancelled" evidence the intent wants retained as secondary detail never reaches the reader. Putting the run outcome before the pause note, or shortening it, keeps the terminal fact inside the 90-char budget.bin/fm-crew-state.sh:609- When the pause line carries no reason,status_line_notereturns empty and the detail argument becomes" · <run detail>", producing a doubled separator in the canonical line. Failure scenario: status log's last line is a barepaused:with a cancelled run - the emitted line isstate: paused · source: status-log · · run cancelled, and bin/fm-fleet-snapshot.sh:219 parsesdetailas" · run cancelled"with a leading separator, which then renders verbatim in the bearingsdoingcolumn. Building the detail conditionally (use$RUN_DETAILalone when the note is empty) avoids it.docs/architecture.md:31- docs/architecture.md owns the crew-state mechanism (AGENTS.md:331) and now contradicts the code in two places. Line 31 states the helper "keeps that run-step authoritative even if the pane has closed" with only the ci-log-tail exception spelled out at lines 33-34; line 37 states "In that status-log fallback, a declared external wait reports the distinctpausedstate with its reason" - i.e. paused is described as reachable ONLY on the no-run fallback path. After this change a declared pause also overrides an attributed terminal run outside that fallback. Failure scenario: a reader (or a future change) follows line 31/37 and concludes an attributed failed/cancelled run always reportsfailed, then builds surfacing logic on that guarantee, which the new bin/fm-crew-state.sh:601-612 block breaks. The ci exception is documented at that level of detail; this second exception should be too.🔧 Fix: gate pause override on true terminal outcome, keep failures visible
2 infos still open:
bin/fm-crew-state.sh:628- Flagging per intent-conformance: the change now contradicts the literal User intent text on thefailedoutcome. The intent states as required: "when bin/fm-crew-state.sh has HAVE_RUN=1 for a terminal outcome (failed including cancelled, or done) and the status log's last verb is paused:, emit state paused with source status-log", and "Tests added for cancelled/failed/done + trailing paused". The contradicting hunk iscase "$RUN_TERMINAL" in done|cancelled)at bin/fm-crew-state.sh:627-628, which excludesfailed, plus tests/fm-crew-state.test.sh:735-751 where the formertest_terminal_failed_plus_pausedis replaced bytest_terminal_failed_plus_paused_surfaces_failure, now assertingstate: failed/source: run-stepandassert_not_contains "state: paused". This divergence is the author's own round-1 decision (key=review-gate: "If NO reliable freshness signal exists, fail SAFE ... Cancelled ... may still be overridden by pause; failed (work failed) must surface. Prefer surfacing."), so the code is almost certainly right and the resolution is to update the intent/PR description rather than the code. Failure scenario if left unresolved: the PR body and the recorded acceptance criteria claimfailed+ trailingpaused:reportspaused, while the shipped behavior and its test assert the opposite - a later reader or change re-introduces the failed-override believing it was the agreed contract.bin/fm-crew-state.sh:619- The load-bearing rationale comment states "no freshness signal exists to prove the pause came AFTER the failure (status-log lines carry no timestamp, andaxi statusreports no completion time)", mirrored in docs/architecture.md:36 ("neither the status log noraxi statustimestamps its events"). That is accurate for the primaryaxi statuspath and for status-log lines, but this same file documents at bin/fm-crew-state.sh:387-388 that the coarseno-mistakes runsrows are "newest-first, columns &fix(watcher): make check wakes lossless via watcher-side suppression #34;<status> <branch> <short-sha> <date> [<pr-url>]&fix(watcher): make check wakes lossless via watcher-side suppression #34;" - i.e. a date IS present on the coarse fallback path, and the status file's mtime is always readable. Failure scenario: a future change reads the comment as "no timestamp exists anywhere in this script's inputs" and rules out an ordering check that is in fact partially available. No code change needed - a coarse-only freshness rule would give the two attribution paths different pause semantics, which is exactly the dual-authority drift the intent rejects, and the fail-safe (surface failures) is the safer branch either way; only the absoluteness of the claim is worth narrowing to "no per-event timestamp on the attributed-run path".bin/fm-crew-state.sh:614- Intent text says the pause override should apply to "a terminal outcome (failed including cancelled, or done)", but the shipped behavior (head commit b96b202, "keep failures visible") deliberately excludes terminal failed: a failed run + trailingpaused:still reportsstate: failed · source: run-step. The added testtest_terminal_failed_plus_paused_surfaces_failureand the intent's rejected-alternatives section encode that narrower choice, so this reads as a conscious review-driven narrowing rather than a defect - flagged only so the reviewer confirms the narrower scope is what they want.bash tests/fm-crew-state.test.sh(whole file; includes the 7 new cases: cancelled+paused, failed+paused stays failed, cancelled+bare paused separator, done/checks-passed+paused, active outranks paused, parked outranks paused, ci-green monitoring outranks paused)bash tests/fm-watch-triage.test.sh(the only suite exercising crew_absorb_class / crew_is_paused, the downstream consumer of this verdict)Manual end-to-end: hermetic FM_HOME with a real git worktree onfm/upstream-merge, fakeno-mistakes axi statusandtmux(idle pane), status log endingpaused: holding for upstream maintainer merge; ranbin/fm-crew-state.sh upstream-mergeandcrew_absorb_class upstream-mergeagainst both the base (c64ad1c) and target (b96b202) scripts across cancelled / done / failed / active run fixturesManual end-to-end:bin/fm-fleet-view.shrendered over the same fixture with base vs targetbin/, showing the human fleet table cell change fromfailed / run-steptopaused / status-log✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.