fix(crew-state): stop claiming a PR outcome the reader never checked #5

Merged
john merged 1 commit from fm/fm-crew-state-pr-claim into main 2026-08-08 05:40:38 +00:00
Owner

Fixes the line in firstmate's own state reader that asserted a pull-request outcome it never checked. It is the root cause of seven false "done" reports in a single day.

What was wrong

passed)        RUN_STATE="done"; RUN_DETAIL="run passed: PR merged/closed" ;;
checks-passed) RUN_STATE="done"; RUN_DETAIL="checks green: PR ready for review" ;;

A validation-pipeline outcome of passed means the pipeline finished. It does not mean a pull request was opened, and certainly not merged or closed. The run's pr step is skipped routinely — no-mistakes pr+ci skipped due to mid-run target_changed was observed repeatedly — and the outcome is still passed.

So firstmate asserted a merged pull request that may never have existed. The string is generated locally from a state file: no worker is involved and no forge is ever contacted. Confirmed by two tasks on the same day printing identical text, where one genuinely had a pull request and the other had a pushed branch and none at all.

The cost was not cosmetic. It was believed, reported to the captain, and the mistake was attributed to workers and to "leftover run records in reused workspaces" — twice written into the fleet's own notes as a worker defect when it was firstmate's own line all along.

What changed

The detail now says only what is known, and names a pull request only when local task metadata records one:

passed)        RUN_DETAIL=$(run_detail_with_recorded_pr "run passed: pipeline complete") ;;
checks-passed) RUN_DETAIL=$(run_detail_with_recorded_pr "checks passed: pipeline complete") ;;

run_detail_with_recorded_pr appends recorded PR: <url> — and the recorded head when present — from state/<id>.meta. With no pr=, it appends nothing rather than implying one exists.

The header comment now states the invariant directly: a terminal pipeline outcome proves no PR lifecycle state.

Constraints honoured

  • No network call added. Verified: zero curl, wget, gh or API references introduced. This script runs constantly, inside loops and guards; a forge call here would be a latency and failure-mode disaster.
  • The state vocabulary callers depend on — done, failed, working, parked, unknown — is unchanged. This is a wording and honesty fix, not a state-machine change.
  • 44 lines of new tests cover both the recorded-PR and no-recorded-PR cases.

Test environment note

This machine produces several failures unrelated to any branch, each verified today against clean main and in some cases pure upstream/main: the herdr presentation suite (these sessions run inside a real herdr), the MISSING-tool checks in bootstrap and session-start (/usr/bin/node exists here), fm-on and fm-remote-doctor (herdr, tasks-axi, treehouse and harness are all installed), and the calm-extension and documentation-audience suites. Two more can fail under concurrency and pass alone.

Also worth recording: run the suite with FM_HOME scoped to the worktree under test or unset. Pointing it at the primary home while testing a worktree makes fm-arm-pretool-check case A13 fail falsely — a trap that cost a full diagnostic pass today.

Fixes the line in firstmate's own state reader that asserted a pull-request outcome it never checked. It is the root cause of **seven false "done" reports in a single day**. ## What was wrong ```bash passed) RUN_STATE="done"; RUN_DETAIL="run passed: PR merged/closed" ;; checks-passed) RUN_STATE="done"; RUN_DETAIL="checks green: PR ready for review" ;; ``` A validation-pipeline outcome of `passed` means **the pipeline finished**. It does not mean a pull request was opened, and certainly not merged or closed. The run's `pr` step is skipped routinely — `no-mistakes pr+ci skipped due to mid-run target_changed` was observed repeatedly — and the outcome is still `passed`. So firstmate asserted a merged pull request that may never have existed. The string is generated locally from a state file: **no worker is involved and no forge is ever contacted.** Confirmed by two tasks on the same day printing identical text, where one genuinely had a pull request and the other had a pushed branch and none at all. The cost was not cosmetic. It was believed, reported to the captain, and the mistake was attributed to workers and to "leftover run records in reused workspaces" — twice written into the fleet's own notes as a worker defect when it was firstmate's own line all along. ## What changed The detail now says only what is known, and names a pull request **only when local task metadata records one**: ```bash passed) RUN_DETAIL=$(run_detail_with_recorded_pr "run passed: pipeline complete") ;; checks-passed) RUN_DETAIL=$(run_detail_with_recorded_pr "checks passed: pipeline complete") ;; ``` `run_detail_with_recorded_pr` appends `recorded PR: <url>` — and the recorded head when present — from `state/<id>.meta`. With no `pr=`, it appends nothing rather than implying one exists. The header comment now states the invariant directly: *a terminal pipeline outcome proves no PR lifecycle state.* ## Constraints honoured - **No network call added.** Verified: zero `curl`, `wget`, `gh` or API references introduced. This script runs constantly, inside loops and guards; a forge call here would be a latency and failure-mode disaster. - The state vocabulary callers depend on — `done`, `failed`, `working`, `parked`, `unknown` — is unchanged. This is a wording and honesty fix, not a state-machine change. - 44 lines of new tests cover both the recorded-PR and no-recorded-PR cases. ## Test environment note This machine produces several failures unrelated to any branch, each verified today against clean `main` and in some cases pure `upstream/main`: the herdr presentation suite (these sessions run inside a real herdr), the `MISSING`-tool checks in bootstrap and session-start (`/usr/bin/node` exists here), `fm-on` and `fm-remote-doctor` (herdr, tasks-axi, treehouse and harness are all installed), and the calm-extension and documentation-audience suites. Two more can fail under concurrency and pass alone. Also worth recording: run the suite with `FM_HOME` scoped to the worktree under test or unset. Pointing it at the primary home while testing a worktree makes `fm-arm-pretool-check` case A13 fail falsely — a trap that cost a full diagnostic pass today.
fix(state): avoid inferring PR lifecycle from validation
Some checks failed
CI / Stock macOS Bash snapshot compatibility (pull_request) Has been cancelled
CI / Repo invariants (pull_request) Has been cancelled
Require no-mistakes / PR must be raised via no-mistakes (pull_request) Has been cancelled
CI / Behavior portable parallel 2 (pull_request) Has been cancelled
CI / Lint shell scripts (pull_request) Has been cancelled
CI / Test coverage guard (pull_request) Has been cancelled
CI / Behavior portable parallel 1 (pull_request) Has been cancelled
CI / Behavior portable serial 1 (pull_request) Has been cancelled
CI / Behavior portable serial 2 (pull_request) Has been cancelled
CI / Behavior portable serial 3 (pull_request) Has been cancelled
CI / Behavior portable serial 4 (pull_request) Has been cancelled
CI / Behavior tests (Herdr) (pull_request) Has been cancelled
CI / Behavior timing aggregate (pull_request) Has been cancelled
8435fd5fe8
john merged commit fc648343c1 into main 2026-08-08 05:40:38 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
john/firstmate!5
No description provided.