Conversation
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior endpoint-confirmation issue is resolved by counting consecutive poll observations instead of expiring confirmation according to wall-clock time. Reviews (3): Last reviewed commit: "no-mistakes(review): exempt secondmate w..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2edffc8440
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # supervisor has actually looked at the last thing it said about this pane, and it | ||
| # is the only input that separates "nobody is listening" from "already handled". | ||
| wedge_escalation_unacknowledged() { # <window> | ||
| fm_wake_queued_keys stale 2>/dev/null | grep -Fxq -- "$1" |
There was a problem hiding this comment.
Captain, when another stale record for this window remains queued—such as the new endpoint-gone alarm—this check returns true even if the prior wedge escalation was already acknowledged, because fm_wake_queued_keys exposes only the shared kind/key and not the payload or escalation identity. wedge_escalation_due consequently retains the short cadence and advances the wedge counter toward demand-deep-inspection based on an unrelated wake, defeating the acknowledged-escalation backoff this change introduces; track the specific wedge escalation rather than treating every stale row for the window as that escalation.
Useful? React with 👍 / 👎.
Violated invariant: pane-idle supervision must escalate on evidence the supervisor has not already seen. The crew's own latest declaration sets the recheck cadence, an escalation firstmate has already handled does not repeat on the detection cadence, and a lost endpoint is covered by neither. Three ways the watcher broke it, each reproduced against a real watcher poll loop over a scratch home with a real tmux endpoint before any change. A live declared wait earns one prompt surface so an external-decision gate is never hidden behind the pause cadence, but that allowance was keyed to the pane HASH. A quiet pane that redraws - a progress tick, a token counter, a resized footer - produces a new hash every few seconds, and each one re-earned the same immediate wake. A crew that declared a 40-60 minute wait was surfaced 19 times in 6 minutes and never once reached the long cadence its declaration buys. The allowance is now keyed to the declaration's own status-log signature, so a redraw under a wait firstmate already knows about is absorbed and only a fresh append re-arms it. Measured on the same fixture: 19 stale wakes to 1. wedge_timer_check deleted its own timer when it escalated, so the next poll restarted the same short interval against an unchanged pane while the escalation counter compounded. Nothing consumed the fact that firstmate had drained the previous escalation, inspected the pane and acknowledged it, so a live crew was alarmed every STALE_ESCALATE_SECS indefinitely, reaching demand-deep-inspection with every wake already handled. The interval is now evidence-driven: an escalation still queued unacknowledged keeps the detection cadence, so a ladder nobody has read still climbs; an acknowledged repeat on an unchanged pane doubles the required quiet interval up to WEDGE_ACKED_BACKOFF_MAX_SECS. The durable queue is the acknowledgement authority, read only inside the narrow band where it can change the answer, never once per poll per pane. A recorded window absent from a successful backend inventory produced no wake at all: the stale loop skipped it on the pane-capture failure, so a crew that died mid-wait was silent indefinitely with its own paused: line explaining the silence. Such an endpoint now surfaces once on its own evidence, before and independently of any declared wait, and re-arms if the endpoint returns. Only a `missing` verdict qualifies; every other unreadable answer keeps the previous silence so a backend hiccup cannot manufacture a wake. blocked: and needs-decision: are unaffected - only the paused and captain-held verbs are declared waits - and the ladder still reaches demand-deep-inspection for an undeclared quiet pane. Regression coverage drives the real fm-watch.sh poll path against scratch-home fixtures: the declared-wait cadence across redraws plus its blocked: exclusion, the acknowledged-repeat backoff, the unchanged ladder, and the lost-endpoint alarm under a standing pause. The two existing ladder fixtures now pin the backoff cap so they measure the ladder rather than its spacing. Real-tmux evidence for the lost-endpoint capture failure is recorded under docs/verification/.
FM_WEDGE_ACKED_BACKOFF_MAX_SECS belongs in the same operator block as FM_STALE_ESCALATE_SECS and FM_WEDGE_DEMAND_INSPECT_COUNT, since it is the third value that decides when a quiet pane alarms.
The lost-endpoint alarm this branch added fired on a single `missing` verdict, which is also the normal shape of an ordinary cleanup. bin/fm-teardown.sh closes the runtime endpoint before it removes the task record, and the watcher enumerates windows from that record without taking the metadata lock, so a poll landing in that gap sees a window that is genuinely absent and genuinely still recorded, and alarmed "inspect and recover" for a task that was completing normally. That is the same class of false wake the branch exists to remove. The alarm now requires the missing verdict on two consecutive polls of the same window. A worker that is not coming back pays one poll interval of detection latency, which is irrelevant against a process that is gone; a completing task pays nothing, because its record disappears well before the next poll. Only an unbroken run counts: any other verdict, including the transient `unreadable` a backend hiccup produces, drops the pending confirmation, so two hiccups a minute apart can never add up to an alarm. The pending confirmation is evidence about one specific poll pair, so it expires. Nothing clears per-window markers for a window that stops being polled at all, which is exactly what cleanup does, and a record left behind by a retired task would otherwise let a single missing verdict alarm instantly the next time that window name is recorded. A record older than ENDPOINT_GONE_CONFIRM_POLLS intervals starts a fresh pair instead of completing a stale one. The fake tmux endpoint-absence knob becomes a flag file rather than a boolean, so a fixture can take an endpoint away and give it back mid-run, and it now drives the inventory as well as the capture - the pair real tmux actually presents for an absent target. watch_bg's documented extra-env arguments went through as expanded words, which bash does not treat as assignments, so they became the command name; they now go through env and the helper honors what its comment promised. Regression coverage runs the real poll path over a scratch home: a first missing poll records only a pending confirmation, the task record is then removed as cleanup finishes, and no alarm is queued. The existing fixture still proves an endpoint that stays gone alarms once under a standing declared wait.
…on multiply The two-consecutive-polls confirmation window scaled the poll interval with integer arithmetic ($(( POLL * ENDPOINT_GONE_CONFIRM_POLLS ))). FM_POLL is a whole-second cadence by contract, but the watcher tests drive it fractionally (as low as 0.2), and Bash integer arithmetic on a decimal is a syntax error that aborts the enclosing window loop under set -u - so a lost endpoint under a fractional cadence would silently kill supervision at the exact moment it is needed instead of alarming. Floor POLL to a whole second with a 1s minimum before the multiply. The window is integer seconds either way, so the confirmation semantics are unchanged for the documented integer cadence and a fractional cadence now alarms on the second missing poll rather than crashing on it. Adds a regression test that drives the endpoint-gone path with FM_POLL=0.2 and asserts the alarm still fires; it reports `not ok` against the pre-fix arithmetic. Reported by the Codex PR reviewer (P1) on kunchenguid#3214.
2c02fe6 to
3ea8ca6
Compare
|
Speaking as Kun's firstmate: Reviewed HEAD Class: corrective. Honors the declared- VISION.md per-rule:
Fork CI approved this pass: |
|
Closed as superseded — this work already landed on main via #4586. — Kun's Firstmate |
2 participants
Intent
Fix the watcher's stale-alarm churn against tasks that are legitimately quiet: a task whose latest declared status is
paused:must go on a long recheck cadence, and repeated identical staleness the supervisor has already handled must back off instead of escalating. This is firstmate's own repo; follow .agents/skills/firstmate-coding-guidelines/SKILL.md.The defect, with two dated reproductions. (1) 2026-08-25, task flp-mcp-bench: crew declared
paused: benchmark batch running, ~40-60 min expected; 4 stale wakes fired in ~10 min anyway. (2) 2026-08-26, task qwenglm-bench, the decisive one: stale wakes fired at ~250s pane-idle, escalations 1 through 7 over ~35 min, cadence unchanged by the primary draining, handling and acking every one; the crew was MID-TURN continuously for over an hour (long foreground wait loops on a benchmark run) with ZERO crew turn-ends in the window, so any hypothesis that the crew's own turn-ends reset pause/stale bookkeeping cannot explain it;paused: glm-5.3-flash arm running (3 memos, ~45-60m each), ETA ~150mwas appended between escalations 6 and 7 and escalation 7 fired ~4 min later, so the declared-paused long-cadence contract (AGENTS.md section 8) was not honored at all; the escalation counter compounded to demand-deep-inspection even though every prior stale had been handled and acked with the pane verified healthy each time.Investigation contract: reproduce before reading code - stand up a scratch home (never the live state/), with a fake crew pane that stays mid-turn and quiet, append a
paused:status line, and run the real watcher against it; confirm the alarm cadence and the paused-state non-honoring on the real invocation path before any fix. Then localize: the stale predicate and its bookkeeping live in bin/fm-watch.sh (state/.paused-, .stale-, .stale-since-, .wedge-escalations- are its private records) and status classification lives in bin/fm-classify-lib.sh. Establish as fact, from code and the reproduction together: (a) whether the stale path consults the task's latest declared status at all; (b) what actually advances the escalation counter and what, if anything, was supposed to reset it; (c) whether the primary's own turn-ends disturb per-task pause records. Name the violated invariant explicitly in the fix commit.Required behavior after the fix: a task whose latest status line is
paused:gets a long recheck cadence for pane-idle staleness, defined as a named constant with a comment, on the order of 20-30 min rather than 250s, and a subsequent non-paused status line restores the normal cadence; ablocked:orneeds-decision:line is NOT paused and keeps the normal cadence; after the primary acknowledges a stale wake for a pane, an unchanged-still-idle pane does not re-fire on the same short cadence with an incremented escalation but backs off, while new evidence (a status append, a pane content change followed by fresh idleness) legitimately re-arms; the escalation ladder still exists and still reaches demand-deep-inspection for a pane stale WITHOUT a paused declaration and WITHOUT prior handled acks, so genuine wedge detection is not blunted; and a genuinely dead endpoint (process gone) must still alarm promptly regardless of a stale paused: line.Accepted decision on the dead-endpoint alarm: the endpoint-gone check must require the missing verdict on TWO consecutive polls before waking, which kills the teardown-race false alarm (bin/fm-teardown.sh closes the runtime endpoint before it removes the task record, and the watcher enumerates windows from that record without taking the metadata lock, so a poll landing in that gap sees a window genuinely absent and genuinely still recorded) while keeping the dead-endpoint alarm.
Scope fence: only the watcher's stale/pause/escalation bookkeeping and whatever classification helper it needs. Do not restructure fm-watch.sh's arming, wake-queue, PR-poll, procevent, or Relay paths; do not change status-line syntax or invent new status states; do not touch supervision protocol docs beyond the minimal accurate update if behavior wording changes. No new configuration knobs unless the fix is impossible without one.
Tests: regression tests must run the real watcher poll path against a scratch home fixture, not a reimplementation - one proving a declared-paused quiet pane does not fire on the short cadence; one proving the post-ack backoff; one proving a non-paused quiet pane still escalates; one proving a dead endpoint still alarms despite a paused line. Wire them where this repo's existing script tests live so CI runs them.
Delivery: this ships as an OUTSIDE CONTRIBUTION. kunchenguid/firstmate is the upstream template we do not own; the captain's account is shadohead, an outside contributor with a fork at https://github.com/shadohead/firstmate. Push the branch to the shadohead fork and open the PR against kunchenguid:main. The PR body must reference upstream issue #3206, which reports this exact wedge-escalation-on-declared-paused bug, and must summarize the root cause, the two-consecutive-polls decision, and the validation evidence. State plainly in the PR body that this addresses only the watcher-triage half of #3206 and does NOT touch the Grok busy-footer matching that issue also asks for, and that it is independent of the alternative patch referenced there (josh-padnick#10). Do not merge; merge authority stays with the upstream maintainer.
What Changed
declared_wait_already_surfaced) instead of the pane hash, so apaused:/captain-heldpane that keeps redrawing while idle stays on the longFM_PAUSE_RESURFACE_SECScadence and only a fresh status append re-arms an immediate surface; the stable-hash and pause-reclassification branches now route a still-declared pane throughhandle_paused_stalerather than tearing down its pause bookkeeping every tick.wedge_escalation_due/wedge_backed_off_interval: an escalation the first mate has drained and acknowledged (read from the durable wake queue) doubles the required quiet interval per acknowledged repeat up to the newFM_WEDGE_ACKED_BACKOFF_MAX_SECS(default 1800s) ceiling, while one still queued unacknowledged keeps the shortFM_STALE_ESCALATE_SECScadence so an unread ladder still reachesdemand-deep-inspection.endpoint_gone_checkplus a persistedPOLL_SEQpoll counter so a recorded window absent from a successful backend inventory alarms once, on its own evidence, independently of any declared wait - but only after themissingverdict repeats on two immediately consecutive polls, which suppresses the teardown-race false alarm wherefm-teardown.shcloses the endpoint before removing the (still-enumerated) task record.AGENTS.md,docs/architecture.md,docs/configuration.md, a runtime-backends lost-endpoint verification note) and added scratch-home regression tests driving the real watcher poll path for the paused-quiet, post-ack backoff, non-paused escalation, and dead-endpoint cases.Risk Assessment
✅ Low: The re-reviewed fix-round change is a minimal, correct one-line secondmate exemption that aligns the watcher with the existing never-read-mate-liveness invariant (pause_state_class, docs, and the real secondmate_liveness_sweep owner), and the surrounding branch logic, two-poll confirmation, and behavior-based tests all check out against the authoritative intent.
Testing
Ran the full tests/fm-watch-triage.test.sh suite (exit code 0) and a focused re-run of the six new cases, all against the real bin/fm-watch.sh poll path over scratch-home fixtures rather than a reimplementation. The tests demonstrate every required behavior end-to-end as the watcher experiences it: declared-paused quiet panes stay on the long recheck cadence and never wedge-escalate (even across redraws), handled/acked repeats back off,
blocked:stays on the normal cadence, an unpaused unacked wedge still climbs to demand-deep-inspection, and a dead endpoint still alarms under a paused line while the two-consecutive-poll rule suppresses the teardown-race false alarm. This is a CLI/watcher behavior change with no UI surface, so evidence is the CLI test transcript (no screenshot applies). Overall: all targeted and regression tests green, worktree clean.Evidence: Watcher pause-churn behavioral evidence (intent-to-test mapping, real fm-watch.sh poll path)
Source: Watcher pause-churn behavioral evidence (intent-to-test mapping, real fm-watch.sh poll path)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-watch.sh:1578- endpoint_gone_check (bin/fm-watch.sh:599, called at line 1578) reads fm_backend_agent_state for a secondmate window when a paused/captain-held mate's capture fails. The secondmate gate at line 1574 only skips a mate that is NOT paused/held, so a paused or captain-held mate falls through to the capture and, on failure, into endpoint_gone_check. That helper consults the mate's agent_state (missing) and, on two consecutive polls, alarms 'endpoint gone'. This is a new behavior for mates: previously a capture failure was a silent continue, and both pause_state_class and docs/architecture.md:42 (context in this diff) state a secondmate's endpoint liveness is deliberately never read. It only fires when the mate's window is genuinely absent for two polls (so it is not a false wake per se and may even be desirable), but it does read secondmate endpoint liveness the surrounding design says it never does. Decide whether endpoint_gone_check should exempt secondmate windows like pause_state_class does, or whether surfacing a genuinely vanished mate is intended and the 'never read' wording should be qualified.🔧 Fix: exempt secondmate windows from endpoint_gone_check
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-watch-triage.test.sh(full suite, exit code 0)Focused run of the 6 new cases:test_acknowledged_wedge_escalation_backs_off,test_declared_pause_survives_a_redrawing_pane,test_endpoint_gone_alarms_despite_a_declared_pause,test_endpoint_gone_survives_a_fractional_poll_interval,test_endpoint_gone_confirms_across_a_slow_poll_cycle,test_teardown_race_does_not_alarm_as_a_lost_endpoint- all passConfirmed adjacent regressions pass:test_wedge_escalation_marks_demand_deep_inspection_after_threshold,test_wedge_escalation_resets_when_pane_becomes_active,test_nonterminal_stale_paused_absorbed_then_resurfaced,test_busy_declared_pause_is_rechecked_not_wedge_escalatedConfirmed implementation reuses PAUSE_RESURFACE_SECS (default 3600s, long cadence) and adds WEDGE_ACKED_BACKOFF_MAX_SECS plus poll-counted endpoint confirmation with secondmate exemption in bin/fm-watch.sh✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.