fix: preserve crew run attribution during validation - #25
Merged
Merged
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
HelloWorldSungin
force-pushed
the
fm/fm-crew-state-blind-during-fix-round
branch
from
August 4, 2026 04:07
6c5b095 to
1598341
Compare
added 7 commits
August 4, 2026 05:03
fm-crew-state.sh reported `unknown / none / no current-state source
available` for crews that were demonstrably mid-validation. Because
fm-classify-lib.sh's crew_is_provably_working() reads the same line, a
working crew stopped being provably working, so fm-watch.sh could not take
its absorbed-stale path and every idle repaint raised a fresh possible-wedge
escalation - precisely during the longest phase of a run.
Three distinct mechanisms produced that one symptom. All three are fixed.
1. A lookup FAILURE read as a run ABSENCE. nm_run swallowed the exit status
with `|| true`, so a `no-mistakes` call killed by its own timeout under a
saturated daemon was indistinguishable from "this branch has no run".
The status is now propagated (124 on timeout, 127 when the call cannot be
bounded at all), and a failure degrades to the crew's last observed
run-step under a new source, `run-step-degraded`, recorded in
state/<id>.run-step. Empty stdout counts as failure: verified against the
installed CLI that `axi status` answers a branch with no run of its own
with some other branch's run, so it has no empty "nothing to report".
2. The code-identity skip fired during a fix round. When a review finding is
answered `--action fix` the pipeline commits the fix, the tip advances past
the sha the run recorded, and the run that was authoring those very commits
stopped matching its own worktree. An actively-executing run now keeps
attribution across that advance; a parked run commits nothing, so a tip that
moved past it is still local work outside the run and still invalidates.
3. The run's head was not an object in the worktree at all. Verified live on
arkrh-corpus-types-unbound-unconstructible: `axi status` in the crew's own
worktree returned that crew's own branch, status running, parked at
fix_review with 3 findings and branch_sync.state pipeline_owned, but its
head 1c17405c failed `git rev-parse --verify` ("malformed object name")
because the pipeline commits in a copy the worktree never fetched. The
lookup completed in under a second and the tip had not moved, so this is
neither mechanism above. nm_head_relation now separates `unresolved` from
`diverged`, and a LIVE run answered for this branch may be attributed
despite an unresolvable head.
Against that live task the shipped reader could not attribute at all and fell
through to the pane; this branch reports `working / run-step / validating
(running)`. Read-only check against a temp state dir, never the live home.
Not weakening wedge detection is the constraint that shapes all three, and
every widening is paired with a guard test that must keep passing:
- Nothing is degraded for a crew never seen validating: no record, no replay.
- The degrade is age-bounded (FM_CREW_STATE_DEGRADED_MAX_AGE, default 900s,
0 disables), so a permanently unreachable daemon stops absorbing suspicion.
- A gone endpoint and an exact busy verdict both outrank a replayed record;
live evidence always beats memory.
- A completed lookup that finds no run never degrades - absence stays absence.
- Terminal runs are refused in every widened case, and the historical runs
listing never earns the pipeline-owned benefit that only a branch-scoped
`axi status` answer does.
- run-step-degraded is deliberately excluded from fm-fleet-snapshot.sh's
decision-clearing sources: good enough for wedge triage, never good enough
to clear a captain decision.
Tests: 5 reproductions fail on the current code and pass here; 7 safety tests
pass on both sides, which is what makes them guards. Verified per-test in both
directions rather than only as a suite.
Pre-existing and out of scope, each already queued separately and confirmed
failing on the base branch with these changes stashed:
tests/fm-calm-pi-extension.test.sh, tests/fm-session-start.test.sh,
tests/fm-test-run.test.sh. The rest of the non-e2e suite passes (76 files).
HelloWorldSungin
force-pushed
the
fm/fm-crew-state-blind-during-fix-round
branch
from
August 4, 2026 05:28
1598341 to
f0e80b3
Compare
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.
Re-landed on the fork from the already-validated branch (previously opened as kunchenguid#1635, which this account cannot merge upstream). Code unchanged.
Intent
Fix bin/fm-crew-state.sh losing its authoritative source mid-validation, which blinds supervision exactly when a run is busiest.
SYMPTOM. fm-crew-state.sh reported "working / run-step / validating" then flipped to "unknown / none / no current-state source available" and stayed there across ~10 minutes while the crew's pane was unmistakably alive. Because fm-classify-lib.sh's crew_is_provably_working() reads the same source, a working crew stops being provably-working, so bin/fm-watch.sh cannot take its absorbed-stale path and every idle repaint raises a fresh "possible wedge" escalation.
WHAT WAS WANTED (the accepted requirements):
LATER ACCEPTED ADDITION - A THIRD MECHANISM. Mid-task, a third distinct mechanism was reported from a live run and I was asked to verify it myself rather than take it on faith, reproduce it before designing around it, and say so if the runs row turned out to be keyed on something other than branch. Verification result, which is part of the accepted scope:
SCOPE DISCIPLINE (accepted exclusions):
WHAT I IMPLEMENTED, and the design decisions a reviewer reading only the diff would not know:
Mechanism 1 - lookup failure vs absence. nm_run swallowed the exit status with "|| true", so a no-mistakes call killed by its own timeout was indistinguishable from "this branch has no run". The status is now propagated (124 on timeout, and a deliberate 127 when the call cannot be bounded at all because no timeout/gtimeout/perl exists). A failure degrades to the crew's last observed run-step under a NEW source token, "run-step-degraded", recorded in a new state/.run-step file. Deliberate decision: empty stdout counts as a failure, not an absence - verified empirically against the installed CLI that "axi status" answers a branch with no run of its own with some OTHER branch's run as informational display, so it has no empty "nothing to report" answer to confuse this with.
Mechanism 2 - fix-round tip advance. An actively-executing run (running/fixing/ci) now keeps attribution when the tip advanced past the sha it recorded, because it authored that advance. Deliberate narrowing: a PARKED run commits nothing while waiting on a response, so a tip that moved past it is still local work outside the run and must still invalidate. That narrowing is what keeps the pre-existing test "local work advanced past run head invalidates attribution" passing rather than being loosened.
Mechanism 3 - pipeline-owned head. nm_head_relation now separates "unresolved" (sha is not an object here) from "diverged" (resolvable but on neither side of HEAD). A LIVE run answered for THIS branch may be attributed despite an unresolvable head. Deliberate asymmetry: the historical runs listing never earns that benefit, because it has no notion of "current" - only the branch-scoped "axi status" answer does. Terminal runs are refused in every widened case.
SAFETY DESIGN (the third acceptance criterion, deliberately load-bearing):
Also: bin/fm-classify-lib.sh's crew_absorb_class accepts run-step-degraded as working evidence (that is the whole point - it is what the watcher's absorb path consumes); bin/fm-teardown.sh removes the new state/.run-step record for both a task and a secondmate's children; AGENTS.md documents the new state file in its layout listing; docs/architecture.md and docs/configuration.md document the invariant and the new knob.
fm-crew-state.sh's header previously claimed "Read-only and side-effect free". That claim is now deliberately narrowed in the header: it writes exactly one thing, the run-step record, which is what makes degrading to a last known step possible at all. Verified that no doc or test depended on the old claim.
EVIDENCE: 5 reproduction tests fail against the original code and pass after; 7 safety/guard tests pass on BOTH sides, which is exactly what makes them guards rather than reproductions. Verified per-test in both directions, not only as a whole suite. Verified against the live arkrh task read-only using a temp state dir: the shipped reader could not attribute and fell through to the pane, while this branch reports "working / run-step / validating (running)".
KNOWN PRE-EXISTING FAILURES, deliberately out of scope and each already queued separately: tests/fm-calm-pi-extension.test.sh, tests/fm-session-start.test.sh, tests/fm-test-run.test.sh. All three were confirmed failing on the base branch with these changes stashed. The rest of the non-e2e suite (76 files) passes, plus fm-lint.sh and fm-doc-audience-check.sh.
What Changed
run-step-degradedsource without overriding stopped or dead crew evidence.Risk Assessment
✅ Low: The latest change removes unsupported authoring inference from coarse historical rows while retaining detailed current-run attribution, and the bounded cache lifecycle now preserves genuine wedge detection.
Testing
Alongside the supplied baseline context, this phase directly reproduced all five acceptance regressions on the original code, passed the focused target suite, captured end-to-end CLI evidence, and confirmed real watcher subprocesses still surface stopped crews and escalate genuine wedges. No source changes were made, and transient test data was removed.
Evidence: End-to-end crew-state and watcher-triage transcript
Evidence: Five regressions reproduced against original code
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (5) ✅
bin/fm-crew-state.sh:790- The accepted criterion says a failed lookup must use the last-known step or “an explicit degraded marker - never unknown,” but this branch emitsrun-step-degradedonly when a record exists; otherwise it falls through tounknown, which the newtest_lookup_timeout_without_known_run_step_still_surfacesexplicitly expects. Decide whether to add a non-working explicit degraded state/source that preserves wedge detection, or explicitly amend the requirement to permit unknown when no record exists.bin/fm-crew-state.sh:615- A completed lookup with no attributable run leaves<id>.run-stepintact. The reachable sequence is: observe a working run and write the record, complete a later lookup that finds no run, then have the next lookup fail within 900 seconds; the old working record is replayed andcrew_is_provably_workingabsorbs the stopped crew. Invalidate the record at the completed-no-run boundary and add a seed -> absence -> timeout regression test.🔧 Fix: Invalidate stale run-step records after confirmed absence
1 error still open:
bin/fm-crew-state.sh:618- The new invalidation still depends on the historical listing completing. Reachable sequence: seed a working record, let branch-scopedaxi statuscomplete for this same branch with a terminal unresolvable head, then letrunstime out; this branch setsLOOKUP_FAILEDwithout clearing the record, so the old working step is replayed and the terminal crew remains provably working. Clear the record as soon as a current-branch answer provides non-attributable/terminal evidence, independently of the fallback call, and cover seed -> terminal same-branch answer -> fallback failure.🔧 Fix: Invalidate cached run-step on same-branch rejection
1 error still open:
bin/fm-crew-state.sh:610- This early clear conflates disproving evidence with the existingmissinghead relation. Reachable sequence: seed a working record, receive a successful same-branch status response with its head omitted, then let the runs-list fallback fail; line 610 deletes the record before the failure is known, so an idle crew returns to unknown instead of its degraded known step. Preserve the record and classify the lookup as failed formissing; clear only when terminal status or a concrete relation such as diverged/non-authoring run-behind invalidates attribution, and extend the existing missing-head test with seed -> fallback failure.🔧 Fix: Preserve cached run-step for inconclusive missing heads
1 error still open:
bin/fm-crew-state.sh:769- The cache update occurs after two terminal early exits. Reachable sequence: a running read recordsworking, a later matching run plusdone: ... checks greenemitsdoneat lines 736/747 without updating or clearing the record, then a failed lookup replays the stale working step and makes the finished crew provably working again. Update or invalidate the cached step before every run-derived terminal emit, and cover seed -> CI-ready status-log verdict -> timeout.🔧 Fix: Persist CI-ready verdicts before terminal emits
1 error still open:
bin/fm-crew-state.sh:464- The accepted criterion says, “A PARKED run commits nothing…so a tip that moved past it…must still invalidate,” but this hunk treats every coarse historicalrunningrow as authoring. Reachable path:axi statusreturns another branch, a parked run’s coarse row isrunning, local work advances the tip, andauthoring=1acceptsrun-behind, causing the stopped crew to remain provably working. Decide whether coarse rows must rejectrun-behind, or add a detailed source that can prove the run is actively authoring before widening attribution.🔧 Fix: Reject coarse run-behind rows without authoring proof
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-crew-state.test.shRantest_lookup_timeout_degrades_to_last_known_run_step,test_real_timeout_bound_degrades_to_last_known_run_step,test_missing_run_head_with_failed_fallback_degrades,test_fix_round_advanced_tip_attributes_via_axi_status, andtest_pipeline_owned_unresolvable_head_attributesagainst base commit1e247571aa75e00b00c7a01c4830025ecd44dc61Exercised the realbin/fm-crew-state.shreader andcrew_is_provably_workingclassifier across timeout, fix-round, unresolved-head, completed-absence, and expired-record scenariosRantest_nonterminal_stale_not_working_surfacedfromtests/fm-watch-triage.test.shagainst a realfm-watch.shsubprocessRantest_nonterminal_stale_provably_working_absorbed_then_escalatedfromtests/fm-watch-triage.test.shagainst a realfm-watch.shsubprocess✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.