🤖 fix: gate bash monitor wake delivery on the settled shown-frontier

[ethanndickson]

Summary

A background bash monitor could wake the agent about output it had already been shown. In the reported case a monitor armed with /ALL DONE|FAIL|error/ fired a synthetic "monitor matched" wake for ALL DONE exit=0 after a concurrent task_await had already returned that exact line in its completed report. This moves the "don't re-report shown output" invariant from emit time to delivery time, where it can be enforced against a settled view of what the agent has seen.

Background

PR #3663 introduced the correct primitives — two absolute file offsets, proc.shownThroughOffset (end of the last complete line an unfiltered read delivered to the agent) and monitor.matchedThroughOffset (end of the last matched line) — but it compared them exactly once, inside emitMonitorMatch. The monitor tail loop and task_await's read run as independent async loops. On process exit the monitor flushes immediately (bypassing the cooldown), so that exit-flush can win the race against the read that advances shownThroughOffset: the comparison sees a stale frontier, emits the wake, and persists it. drainBashMonitorWakes then delivers it later — possibly seconds afterward, at idle — with no re-check. This is exactly the common terminal case: a process prints its final DONE/FAIL line and exits while a task_await is reading it.

Implementation

"A wake must not re-report shown output" is a property of delivery, not of matching, so the authoritative check now lives at the single point where a wake becomes a transcript message.

• The match's matchedThroughOffset travels through MonitorMatchPayload into the persisted BashMonitorWakeRecord (optional on the record/schema so older on-disk records keep parsing).
• BackgroundProcessManager.getSettledShownThroughOffset(processId, originNotAfterMs?) reports the shown-frontier after any in-flight unfiltered read settles. getOutput registers a settled-promise for unfiltered reads only — filtered reads never advance the frontier and a filtered long-poll can hold the lock for its full timeout, so gating on them would stall delivery for nothing.
• drainBashMonitorWakes re-checks each match record against that settled frontier and supersedes any record already shown, dropping it from the batch (and returning early if the batch empties). The emit-time check is retained as a cheap fast-path early-out.

Delivery-time correctness edge cases

Two ways the settled frontier could otherwise mislead the gate, both handled here:

• Filtered reads share the byte cursor. outputBytesRead is advanced by filtered reads too, which drop non-matching complete lines without showing them. A filtered read that consumed a matched line, followed by an unfiltered read with no new output, previously let shownThroughOffset jump the gap and falsely mark the dropped line as shown. getOutput now advances the frontier only when the read's processable region is contiguous with it; a gap pins the frontier low, which can only over-wake, never suppress genuinely-unshown output.
• Process IDs are reused across restarts. IDs are derived from display_name and reclaimed when a relaunched process lands on an empty manager, so a pending wake for a dead instance could otherwise be superseded by an unrelated newer process that shares the ID and has been read past the old match. Rather than persist a separate instance token, the gate binds the check to the record's own createdAt: the originating instance necessarily started before its first match created the record, so getSettledShownThroughOffset returns undefined (fail open) whenever the live process's startTime is after createdAt — i.e. it reused the ID. A cross-generation merge therefore needs no special handling: it simply takes the max offset, and the createdAt binding makes the whole record deliver against a newer instance, so a dead instance's undelivered lines are never dropped.

Fail-open is preserved everywhere: legacy records without matchedThroughOffset and managers that don't implement the query still deliver, matching prior behavior.

Downgrade safety

matchedThroughOffset is the only field this change adds to the persisted BashMonitorWakeRecord. To keep the record forward/backward tolerant, its Zod schema is relaxed from .strict() to .strip(), so this build never rejects a record written by a newer build that added a field. Downgrading to a build whose parser predates matchedThroughOffset drops an in-flight pending wake as malformed, but the file is not deleted, so re-upgrading recovers it; the loss is bounded to nightly builds mid-drain (stable v0.27.0 ships no wake store at all).

Risks

Low-to-moderate; scoped to background bash monitor wake delivery. The gate only ever suppresses a wake when the settled shown-frontier is at or past the match and the live process is the same instance that produced it (per the createdAt binding), so the worst-case failure mode is a missed wake rather than a spurious one — and that only triggers when the agent has provably already seen the output. All other paths (filtered-read gaps, reused IDs, legacy records, missing query) fail open.

---

Generated with mux • Model: anthropic:claude-opus-4-8 • Thinking: xhigh • Cost: $26.26

[ethanndickson]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eb9ddc5e31

ℹ️ 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".

[ethanndickson]

@codex review

Addressed the P2 (Preserve wakes for output only consumed by filtered reads) in f1ceabfcf.

Root cause: outputBytesRead is a shared cursor that filtered reads also advance. A filtered getOutput could consume a matched complete line (advancing the cursor past it) without advancing shownThroughOffset; a later unfiltered read with no new output then computed the frontier from that already-advanced cursor, jumping the gap and marking the filtered-out line as shown. The drain gate would then supersede the only wake for output the agent never saw.

Fix: added a contiguity guard in getOutput — shownThroughOffset only advances when this read's processable region begins at or before the current frontier (shownRegionStart <= shownThroughOffset). A gap left by a prior filtered read pins the frontier low, which is safe: it can only over-wake, never suppress a genuinely-unshown line. Added a regression test in backgroundProcessManager.test.ts (verified failing without the guard — frontier jumps to 14 — and passing with it).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f1ceabfcfa

ℹ️ 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".

[ethanndickson]

@codex review

Addressed the P2 (Avoid suppressing wakes from reused process IDs) in 55ab56827.

Process IDs are derived from display_name and reclaimed after a restart (a relaunched process lands on an empty manager and takes the same ID), so a pending wake for a dead instance could be superseded by an unrelated newer process that shared the ID and had been read past the old match's matchedThroughOffset.

Fix: the emitting process's startTime now rides along on MonitorMatchPayload and is persisted on the wake record (processStartTime, optional for legacy records). The drain gate passes it to getSettledShownThroughOffset(processId, expectedStartTime), which only vouches for the shown-frontier when the currently-registered process is that same instance; a mismatched (or absent) instance returns undefined, so the drain fails open and delivers the dead instance's undelivered output. Added a manager-level test (matching instance returns the frontier, reused-ID returns undefined) and a drain-level regression test (a stale-instance wake still delivers even when the reused ID was read past the match — verified failing without the instance-token forwarding).

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55ab568275

ℹ️ 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".

[ethanndickson]

@codex review

Addressed the P2 (Do not merge wakes across process generations) in 39f21f170.

The match-merge branch previously kept the pending record's old lines but overwrote its processStartTime with the new payload's, so a live instance's match merging into a dead instance's still-pending record (after a restart reused the display-name-derived ID) could let the drain supersede the whole record against the live instance's frontier and silently drop the dead instance's undelivered lines.

Fix: a record is drain-suppressible only while its lines belong to one instance. The merge now carries the offset and instance token forward only when both sides share the same processStartTime; a cross-generation merge keeps both instances' lines but clears matchedThroughOffset/processStartTime so the record always delivers (fail open). Added wake-store tests for both same-instance (offset grows, token kept) and cross-generation (both lines kept, offset+token cleared — verified failing without the guard).

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Chef's kiss.

Reviewed commit: 39f21f1701

ℹ️ 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".

[ethanndickson]

@codex review

Rebased onto latest origin/main (clean, zero conflicts). The diff is byte-identical to the previously approved version — no logic changed, only the base commit. Re-requesting review to clear the dismissed approval.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0f603894b6

ℹ️ 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".

[ethanndickson]

Addressed the downgrade finding (thread PRRT_kwDOPxxmWM6Pdb4g) by removing the second new key entirely rather than persisting it more carefully.

The delivery gate needed two things per wake: the matched offset, and the identity of the process instance that produced it (IDs are reclaimed across restarts). The earlier revision persisted a processStartTime token for identity — that was the second new key. It turned out redundant: the record's existing createdAt already encodes instance identity, since the originating instance necessarily started before its first match created the record. So the gate now binds via createdAt (a live process whose startTime is after createdAt reused the ID → fail open), and processStartTime is gone from the payload, record, and schema — which also let me delete the whole cross-generation merge branch.

That leaves matchedThroughOffset as the only new persisted key. I can't drop it (a live read can't replace it without re-reporting on the merge race), so I made the record tolerant instead: the schema is relaxed from .strict() to .strip(), so this build never rejects a newer record, and an older build that predates the field drops the in-flight wake as malformed but does not delete the file — re-upgrading recovers it. The exposure is bounded to nightly builds mid-drain; stable v0.27.0 ships no wake store at all.

[ethanndickson]

@codex review

Reworked per the downgrade finding: removed the processStartTime key (instance identity now derives from the record's createdAt), deleted the cross-generation merge branch, and relaxed the wake-record schema from .strict() to .strip(). matchedThroughOffset remains the only added persisted key. See the updated PR body and the reply above for the full rationale.

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: f1ccfbc64d

ℹ️ 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".