Skip to content

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
kunchenguid:mainfrom
pramendra:fm/fleet-derive-dont-store
Open

fix(bin): refuse PR overwrite, add main-health check, fold pause state, detect foreground-process wedges#2867
pramendra wants to merge 6 commits into
kunchenguid:mainfrom
pramendra:fm/fleet-derive-dont-store

Conversation

@pramendra

Copy link
Copy Markdown

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:

  1. Resolve the terminal identity at use rather than reading a cached value; if it
    cannot be resolved, say so in those words instead of reporting an ambiguous
    placement error.
  2. Refuse, do not overwrite, when a second PR is recorded against a task that
    already has one.
  3. Add a main-health check so a red baseline is detected once rather than
    rediscovered per branch.
  4. Make the pause state independent of status-line ordering, or have the pause
    record the sha/turn it was declared at.
  5. WEDGE DETECTION reads pane idleness, which cannot distinguish a wedged worker
    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.sh now refuses (rather than silently overwrites) recording a second, different pr= 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.
  • Added fm-main-health.sh, which resolves the default branch's HEAD commit live via gh api and aggregates all check runs on it (not just the latest-started run) into a single GREEN/PENDING/RED verdict with distinct exit codes, so a red baseline is detected directly instead of rediscovered per branch; FM_MAIN_HEALTH_TIMEOUT bounds each API call.
  • fm-classify-lib.sh adds last_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 unrelated resolved [key=...] entry appended after a pause no longer reads as the pause ending; fm-supervise-daemon.sh, fm-watch.sh, and fm-push-transition-lib.sh are updated to use it wherever pause/captain-held state is checked.
  • fm-watch.sh and fm-tmux-lib.sh add 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.
  • Updates .gitignore (ignore .serena/), AGENTS.md, and docs/architecture.md/configuration.md/scripts.md to 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 live gh pr view network 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,url returns 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 has ci.yml (push-triggered build/test) and an unrelated nightly.yml (schedule-triggered housekeeping). ci.yml's latest run on main failed; nightly.yml ran 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's startup_failure conclusion (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 final else 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.

Your Name 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.
@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The 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

Comment thread bin/fm-classify-lib.sh
Comment thread bin/fm-watch.sh Outdated
Comment thread bin/fm-main-health.sh
Comment thread bin/fm-main-health.sh
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Enterprise GitHub routes through GitLab

When an origin uses a self-hosted GitHub Enterprise domain, the exact github.com comparison sends it into the GitLab branch, which invokes glab against a GitLab-shaped endpoint and exits without determining main health.

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