Skip to content

fix: preserve crew run attribution during validation - #1635

Closed
HelloWorldSungin wants to merge 7 commits into
kunchenguid:mainfrom
HelloWorldSungin:fm/fm-crew-state-blind-during-fix-round
Closed

HelloWorldSungin wants to merge 7 commits into
kunchenguid:mainfrom
HelloWorldSungin:fm/fm-crew-state-blind-during-fix-round

Conversation

@HelloWorldSungin

Copy link
Copy Markdown

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):

  1. Distinguish "no run exists for this branch" from "the run lookup could not complete". A lookup failure must not read the same as an absence. Degrade to the last known run-step, or to an explicit degraded marker - never to "unknown".
  2. A fix round that advances the tip must still attribute to its own run. The skip rule is correct for a stale or rewritten run and wrong for the run that is currently authoring those commits.
  3. Neither change may weaken genuine wedge detection for a crew that really has stopped. This is the constraint that makes the task non-trivial: an over-broad "assume still working" degrades into never detecting a real wedge.
  4. Required approach: reproduce both causes deterministically in tests BEFORE fixing - a lookup that times out, and a run whose recorded sha is behind a tip the run itself advanced. Then fix both, then prove a genuinely stopped crew is still detected as wedged. That third test is the one that stops this fix from becoming a worse bug.
  5. Acceptance criteria: a timed-out or failed run lookup yields a degraded-but-known state, not unknown; a run that advanced the tip via its own fix commits still attributes to that run; a genuinely stopped crew is still detected, covered by an explicit test; tests fail against the original code and pass after.

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:

  • The reported premise ("the branch is entirely ABSENT from no-mistakes runs --limit 200, which returned only 13 rows") was WRONG. In the crew's own worktree the listing returned 35 rows and the branch was the FIRST row. The 13-row figure came from a different repository; "no-mistakes runs" lists per-repository and the helper runs it inside the crew's worktree. The row is not keyed on anything other than branch.
  • But the underlying finding was RIGHT and is a genuine third mechanism, distinct from the two above. Verified live on task 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 the head it reported (1c17405c) is NOT an object in that worktree at all ("git rev-parse --verify" fails with "malformed object name"), because the pipeline commits in a copy the worktree has never fetched. The lookup completed in under a second and the tip had not moved, so this is neither the timeout cause nor the code-identity-skip cause.
  • Accepted requirement: a head we cannot RESOLVE is different evidence from a head that DIVERGED, and must be handled, still without weakening wedge detection.

SCOPE DISCIPLINE (accepted exclusions):

  • fm-parked-decision-stale-noise is a DIFFERENT mechanism with the same visible symptom and is explicitly NOT in scope; fixing one will not fix the other. The diff is kept to source-resolution and attribution logic.
  • tests/fm-watcher-lock.test.sh belongs to another concurrent branch and must not be edited.
  • Tests must run only inside the task worktree against a temporary home, never against the primary checkout's live state/.

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):

  • Nothing is degraded for a crew never seen validating: no record, no replay.
  • The degrade is age-bounded by a new FM_CREW_STATE_DEGRADED_MAX_AGE knob (default 900s; 0 disables the degrade entirely), so a permanently unreachable daemon stops absorbing wedge suspicion instead of hiding a real wedge forever.
  • Placement is a safety property, not a style choice: the degraded answer sits BELOW the endpoint checks and the exact busy verdict, so live positive evidence (a gone endpoint proving the crew stopped, a busy harness proving it is working now) always outranks a remembered step; and ABOVE the unreadable-harness and status-log fallbacks, because an observed run-step beats an append-only event log. This is why the busy-verdict "unknown" emit was deferred into a BUSY_STATE variable rather than emitted inline - it still outranks the status-log fallback exactly as before.
  • A completed lookup that finds no run never degrades: absence stays absence.
  • run-step-degraded is deliberately EXCLUDED from bin/fm-fleet-snapshot.sh's decision-clearing live-activity sources: it is good enough to keep a crew provably working for wedge triage, but never good enough to clear a captain decision. Only a comment was added there to make that deliberate exclusion explicit.

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

  • Distinguish failed run lookups from confirmed absence and replay recent observed run steps through an age-bounded run-step-degraded source without overriding stopped or dead crew evidence.
  • Preserve attribution for actively authoring fix rounds and current pipeline-owned heads unavailable in the worktree, while rejecting terminal, diverged, or unsupported historical runs.
  • Integrate degraded run steps with watcher classification, teardown, configuration, and documentation, with regression coverage for attribution failures and wedge-detection safeguards.

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
SCENARIO 1 - real bounded timeout after an observed validating step
state: working · source: run-step-degraded · validating (running) · run lookup unavailable (last known 1s ago)
watcher triage: absorbed stale (provably working)

SCENARIO 2 - active fix round after its own commit advanced the branch tip
recorded run head: cbf65580a934d17eed40b88c2997f770ce3a6a17
advanced worktree tip: 737d8713664a2f6d72efa8613e0d07e26e4d3f4d
state: working · source: run-step · validating (fixing)

SCENARIO 3 - live branch-scoped run with pipeline-owned unresolved head
worktree resolution: head 1c17405cdeadbeefdeadbeefdeadbeefdeadbeef is unavailable locally
state: parked · source: run-step · parked at review: 3 finding(s)

SCENARIO 4 - completed lookup finds no run and the crew is idle
state: unknown · source: none · no current-state source available
watcher triage: possible wedge surfaced (not provably working)

SCENARIO 5 - remembered validation expires during continued lookup failure
state: unknown · source: none · no current-state source available
watcher triage: possible wedge surfaced (not provably working)
Evidence: Five regressions reproduced against original code
TARGET TESTS AGAINST ORIGINAL CODE (base 1e247571aa75e00b00c7a01c4830025ecd44dc61)

test_lookup_timeout_degrades_to_last_known_run_step
not ok - a timed-out lookup must not read as unknown (unexpected: 'state: unknown')
--- output ---
state: unknown · source: none · no current-state source available
result: expected baseline failure (exit 1)

test_real_timeout_bound_degrades_to_last_known_run_step
not ok - a genuinely killed query degrades (missing: 'source: run-step-degraded')
--- output ---
state: unknown · source: none · no current-state source available
result: expected baseline failure (exit 1)

test_missing_run_head_with_failed_fallback_degrades
not ok - missing run head preserves the recorded step (missing: 'source: run-step-degraded')
--- output ---
state: working · source: status-log · current stage still in progress
result: expected baseline failure (exit 1)

test_fix_round_advanced_tip_attributes_via_axi_status
not ok - an active run behind its own fix commit keeps attribution (missing: 'source: run-step')
--- output ---
state: unknown · source: none · no current-state source available
result: expected baseline failure (exit 1)

test_pipeline_owned_unresolvable_head_attributes
not ok - a live run whose head this worktree cannot see still attributes (missing: 'source: run-step')
--- output ---
state: unknown · source: none · no current-state source available
result: expected baseline failure (exit 1)

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 emits run-step-degraded only when a record exists; otherwise it falls through to unknown, which the new test_lookup_timeout_without_known_run_step_still_surfaces explicitly 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-step intact. 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 and crew_is_provably_working absorbs 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-scoped axi status complete for this same branch with a terminal unresolvable head, then let runs time out; this branch sets LOOKUP_FAILED without 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 existing missing head 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 for missing; 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 records working, a later matching run plus done: ... checks green emits done at 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 historical running row as authoring. Reachable path: axi status returns another branch, a parked run’s coarse row is running, local work advances the tip, and authoring=1 accepts run-behind, causing the stopped crew to remain provably working. Decide whether coarse rows must reject run-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.sh
  • Ran test_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, and test_pipeline_owned_unresolvable_head_attributes against base commit 1e247571aa75e00b00c7a01c4830025ecd44dc61
  • Exercised the real bin/fm-crew-state.sh reader and crew_is_provably_working classifier across timeout, fix-round, unresolved-head, completed-absence, and expired-record scenarios
  • Ran test_nonterminal_stale_not_working_surfaced from tests/fm-watch-triage.test.sh against a real fm-watch.sh subprocess
  • Ran test_nonterminal_stale_provably_working_absorbed_then_escalated from tests/fm-watch-triage.test.sh against a real fm-watch.sh subprocess
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

Sungin Kim added 7 commits August 1, 2026 21:08
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

Copy link
Copy Markdown
Author

Re-landed on the fork: HelloWorldSungin#25 (this account cannot merge upstream). Closing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant