fix(bin): refuse PR overwrite, add main-health check, fold pause state, detect foreground-process wedges - #2867
Open
pramendra wants to merge 6 commits into
Open
Conversation
added 4 commits
August 23, 2026 20:47
Four instances of the same defect class from 2026-08-22: a claim about state was stored apart from the state, nothing forced them to agree, and the stale copy kept being believed. - fm-pr-check.sh: refuse to overwrite a task's recorded pr= with a different PR instead of silently re-aiming its merge watch. - fm-main-health.sh (new): a fast, live GitHub Actions read for a project's default branch, so a worker can rule out a red baseline before blaming its own branch, instead of a slow manual reproduction. - fm-watch.sh: derive wedge liveness from the pane's actual foreground process (ps -t), not just screen quiescence - a worker was flagged "possible wedge" while its gate-response command had been running 19 minutes, visible immediately in the process table. - fm-classify-lib.sh: fold the whole status stream for a task's current pause/captain-held declaration instead of trusting the literal last line, which an unrelated keyed "resolved" line (fm-send's --resolve-key answering some OTHER decision) could silently clear. The fifth concrete item (recorded terminal identity blocking spawning) did not hold against current code: the herdr launcher-pane path already resolves live at every spawn and was fixed pre-emptively in commit f0d7cbe, three weeks before the incident report.
…CI status, not one run
…reground-wedge, main-health, PR-refuse
Confidence Score: 4/5The PR is not yet safe to merge because the new health command cannot determine status for documented self-hosted GitHub Enterprise origins. The provider branch recognizes GitHub only when the host is exactly github.com, causing private GitHub Enterprise origins to be sent through glab and a GitLab API endpoint. Files Needing Attention: bin/fm-main-health.sh Reviews (3): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile |
| FM_MAIN_HEALTH_TIMEOUT=${FM_MAIN_HEALTH_TIMEOUT:-20} | ||
| case "$FM_MAIN_HEALTH_TIMEOUT" in ''|*[!0-9]*|0) FM_MAIN_HEALTH_TIMEOUT=20 ;; esac | ||
|
|
||
| if [ "$HOST" = github.com ]; then |
There was a problem hiding this comment.
This was referenced Aug 23, 2026
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
Stop storing claims the system can derive: terminal identity, PR state, main health.
SYSTEMIC fix, not an instance fix. Seven separate incidents on 2026-08-22 share one
shape: a claim about state is stored apart from the state, nothing forces them to
agree, and the stale copy keeps being believed.
The rule: derive at the point of use, or make the stored copy fail loudly when it
drifts. Never store a claim and simply trust it. Do NOT fix these by adding
synchronisation between copies. Prefer deleting the cached copy.
Concrete scope, smallest first:
cannot be resolved, say so in those words instead of reporting an ambiguous
placement error.
already has one.
rediscovered per branch.
record the sha/turn it was declared at.
from one blocked on a long foreground command. Measured 2026-08-22: a worker
was flagged 'possible wedge, escalation 1' while its gate-response command had
been running 19 minutes - visible immediately in the process table. Derive
liveness from the actual running process, not from screen quiescence. Same
defect class as the rest: an observed proxy standing in for the state itself.
Accepted supersession from investigation (verify-before-build, per the standing
crew contract): item 1's premise does not hold against current code. No code path
reads a stale/cached terminal identity from a file and uses it as a blocking
precondition for spawning across all tasks. The closest analog (Herdr launcher-pane
identity) already resolves fresh at every use and was fixed pre-emptively in commit
f0d7cbe, three weeks before the incident report. Item 1 is therefore excluded from
the delivered fix; only items 2-5 are implemented.
Acceptance criteria: the described work is done and shown working, not asserted.
Any check added must be provably able to fail (mutate what it guards and show it go
red). Do not build more than the description asks for - no control planes, no
speculative generality. Claim only what was measured; state plainly what was not
fixed.
What Changed
fm-pr-check.shnow refuses (rather than silently overwrites) recording a second, differentpr=against a task that already has one recorded, with the check enforced both pre-lock and again under the meta lock to close the race window.fm-main-health.sh, which resolves the default branch's HEAD commit live viagh apiand aggregates all check runs on it (not just the latest-started run) into a singleGREEN/PENDING/REDverdict with distinct exit codes, so a red baseline is detected directly instead of rediscovered per branch;FM_MAIN_HEALTH_TIMEOUTbounds each API call.fm-classify-lib.shaddslast_general_status_line/_fm_pause_fold_line, which fold the whole status-line stream to find the crew's current pause/captain-held state instead of trusting the literal last line, so an unrelatedresolved [key=...]entry appended after a pause no longer reads as the pause ending;fm-supervise-daemon.sh,fm-watch.sh, andfm-push-transition-lib.share updated to use it wherever pause/captain-held state is checked.fm-watch.shandfm-tmux-lib.shadd foreground-process-based wedge detection (fm_tmux_pane_foreground_pid,crew_foreground_process_running_since,wedge_defer_foreground): a tmux pane whose foreground process PID is unchanged and alive since the stale timer opened is deferred as busy-on-a-long-command instead of escalated as a possible wedge, mirroring the existing worktree-write deferral..gitignore(ignore.serena/),AGENTS.md, anddocs/architecture.md/configuration.md/scripts.mdto document the new marker files, timeout variable, and script.Risk Assessment
✅ Low: Both prior-round findings (PR-overwrite TOCTOU race, single-workflow CI check masking failures, startup_failure misclassification, .serena artifact leak) are correctly and verifiably fixed with genuine behavioral tests (including a real concurrent-process race test and a real-tmux foreground-process test), and the remaining items 3-5 of the intent (main-health check, pause-state fold independent of status-line ordering, process-table-based wedge liveness) are implemented consistently with existing codebase conventions, well-documented, and exercised by non-source-content-matching tests.
Testing
placeholder
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (2) ✅
bin/fm-pr-check.sh:48- The new 'refuse to overwrite an existing pr=' guard (item 2 of the intent: 'Refuse, do not overwrite, when a second PR is recorded against a task that already has one') reads META and compares EXISTING_PR to URL BEFORE fm_lock_acquire_wait is taken (lock acquired later, around line 104), and the actual write path (lines ~121-129) unconditionally strips any pr=/pr_head= lines and appends the new URL without re-checking EXISTING_PR under the lock. Concrete race: task has no pr= yet; process A (URL_A) and process B (URL_B) both run fm-pr-check.sh for the same task at nearly the same time. Both read META before either has written, so both see EXISTING_PR empty and pass the guard. There is a long window between the guard and the lock (fm_pr_poll_retirement_recover_one, the fm-pr-check-migrate.sh call, and for GitHub a livegh pr viewnetwork call to resolve pr_head) during which either process can interleave. A acquires the lock first, writes pr=URL_A, releases. B then acquires the lock and, because the write logic never re-verifies against the now-different on-disk pr=, strips URL_A and records pr=URL_B - exactly the silent overwrite item 2 says must be refused, just reached through concurrent invocation instead of a single sequential call. The added test (test_refuses_to_overwrite_recorded_pr_with_a_different_one) only exercises two fully-sequential calls and does not cover this window. Fix: re-read META and re-check EXISTING_PR against URL at the point the lock is actually held (immediately before constructing META_TMP), i.e. derive/verify the invariant at the same boundary where the mutation happens, rather than trusting the earlier unlocked read.bin/fm-main-health.sh:51-gh run list --repo "$REPO" --branch "$DEFAULT" --limit 1 --json conclusion,status,headSha,urlreturns the single most recently-started run on the default branch across ALL workflows in the repo, not the run of whichever workflow is actually the project's CI/build-and-test gate. Concrete scenario: repo hasci.yml(push-triggered build/test) and an unrelatednightly.yml(schedule-triggered housekeeping).ci.yml's latest run on main failed;nightly.ymlran later and succeeded. This check reports GREEN (the nightly run is the most recent one), even though the branch's actual CI is red - the opposite of a rediscovered-per-branch false negative the intent is trying to eliminate, now reproduced as a false GREEN that could send a worker chasing a phantom bug in their own branch. The reverse is also possible: an unrelated workflow's failure reporting RED for an otherwise-healthy branch. No test exercises a multi-workflow repo, and the script's docstring doesn't disclose this 'latest run of any workflow' scope, so callers can't tell the limitation applies.🔧 Fix: Close PR-overwrite TOCTOU race; check aggregate CI status, not one run
2 issues (1 error, 1 warning) still open:
.serena/project.yml:1- This fix-round commit (9211a65) tracks .serena/.gitignore and .serena/project.yml — unrelated Serena MCP tooling config auto-written into the worktree, not intentional content. This is a known recurring leak in this exact pipeline: a sibling branch (fm/fleet-ws5-learning-loop) has roughly a dozen prior commits titled things like 'Drop accidental .serena tooling files' / 'chore: untrack accidental Serena MCP artifacts', and one of them adds a root .gitignore entry for .serena/ specifically to stop this from recurring. That fix never landed on main/this branch, so it happened again here. These files have nothing to do with the PR-overwrite race or CI-aggregation fix and should not be part of this commit.bin/fm-main-health.sh:68- The VERDICT jq filter's failing set is {failure, timed_out, cancelled, action_required}; it omits GitHub'sstartup_failureconclusion (emitted when a check run's job never actually started, e.g. due to a runner/concurrency problem). A check-runs response containing one{"status":"completed","conclusion":"startup_failure"}alongside other completed/successful entries falls through to the finalelse passing, so this reports GREEN for a commit where a check effectively never ran — the same 'false GREEN hides a real problem' failure mode the aggregate-check rewrite was meant to eliminate, just via an unhandled conclusion value instead of an unhandled workflow.🔧 Fix: Untrack .serena artifacts; classify startup_failure as red
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
placeholder✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.