🤖 fix: stop background monitor from re-waking on already-shown output#3663
Conversation
A background bash monitor was waking the agent even when the matched output had already been delivered inline (e.g. via a concurrent task_await or bash_output on the same task), producing a redundant synthetic wake the instant the read returned. emitMonitorMatch now drops a pending match when the agent read cursor (proc.outputBytesRead) has caught up to the monitor scan offset (monitor.lastReadOffset), so a wake only fires for output the agent has not yet seen. Genuinely-new matches and fire-and-forget monitors (cursor stays at 0) still wake as before.
|
@codex review |
![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: 619c39dbb4
ℹ️ 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".
getOutput advances outputBytesRead past an unterminated trailing line that it only buffers (incompleteLineBuffer) rather than returning. Comparing the raw read cursor would suppress the monitor's on-exit match for that buffered-but-unshown line. Subtract the still-buffered fragment so suppression only happens for content the agent has actually been shown.
|
@codex review Addressed the P2 (buffered unterminated match). The suppression now compares the monitor scan offset against the shown offset — |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a86a4cb295
ℹ️ 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".
Comparing the agent's shown offset against the monitor's raw scan cursor (lastReadOffset) failed to suppress a wake when a complete matched line was followed by an unterminated fragment: the agent saw the line (getOutput returns it, buffers only the fragment) but the scan cursor sat past the fragment, so the deferred flush still woke about already-seen output. Track matchedThroughOffset -- the byte offset through the end of the last complete line that produced a match, excluding any trailing fragment -- and suppress against that. Both sides now use end-of-complete-line semantics, so the comparison is exact.
|
@codex review Addressed the second P2 (complete matched line followed by a trailing fragment). The monitor now tracks |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc612a0f80
ℹ️ 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".
When a monitor poll read a matched line plus later complete non-matching output in the same chunk, the match was assigned the chunk's end offset. An agent that had read only the matched line (before the later line was written) then stayed below that inflated offset, so the deferred flush still woke it about already-shown output. Compute each complete line's absolute end offset by walking content's newlines from the chunk start, and assign each match its own line-end offset. matchedThroughOffset now anchors to the matched line, making suppression exact regardless of unmatched output later in the same poll.
|
@codex review Addressed the third P2 (per-line vs per-chunk offset). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f549819140
ℹ️ 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".
Reusing outputBytesRead as the 'agent has seen this' signal was fragile: getOutput advances it past content it does not actually show -- buffered unterminated fragments and, crucially, lines dropped by a filter/filter_exclude read. A read with filter='DONE' would wrongly mark a monitor's pending 'ERR' match as shown and suppress its wake. Replace the derived 'outputBytesRead minus current buffer' heuristic with a dedicated proc.shownThroughOffset: the file offset through the end of the last complete line delivered by an UNFILTERED read. It advances in exactly one place (getOutput's normal return, guarded by !filter), so filtered reads never count as showing output. emitMonitorMatch compares it against the matched line's end offset; both are absolute file offsets, making suppression order-independent (no reader/monitor race).
|
@codex review Addressed the filtered-read P2 — and stepped back to fix the root cause rather than patch another offset edge. The fragility came from overloading New design: a dedicated Added a regression test ( |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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
changed the title
## Summary This is the long-lived **auto-cleanup** PR. Each run, the auto-cleanup agent reviews new commits merged to `main`, rebases onto the latest `main`, and applies at most one extremely low-risk, behavior-preserving cleanup. The branch accumulates a small stack of independent cleanups until it is merged. **This run** extracts the duplicated model-string normalization chain (`trim` → `toLowerCase` → strip `provider:` prefix → strip `namespace/` segment) that `anthropicSupportsNativeXhigh` (`src/common/types/thinking.ts`) and `getExplicitThinkingPolicy` (`src/common/utils/thinking/policy.ts`) each inlined verbatim into a shared `stripModelProviderPrefixes` helper. This area was just touched by coder#3664 (Claude Sonnet 5), which relies on `anthropicSupportsNativeXhigh` for capability detection. Behavior-preserving. ## Included cleanups (diff vs `main`) 1. **`refactor: extract shared stripAnsiControlChars helper`** — `src/node/services/bashMonitorWakeStore.ts` and `src/node/services/backgroundProcessManager.ts` each defined a byte-for-byte identical `ANSI_ESCAPE_PATTERN` regex and inlined the same strip-ANSI + drop-control-chars expression (`sanitizeBashMonitorWakeLine` / `sanitizeMonitorLine`). Both now import the new `stripAnsiControlChars` from `src/node/utils/ansi.ts`. 2. **`refactor: dedupe InstructionsTab formatBytes into shared helper`** — `src/browser/components/InstructionsTab/InstructionsTab.tsx` defined a local `formatBytes(n)` that was byte-for-byte identical (only the parameter name differed) to the already-shared `formatBytes` in `src/common/utils/formatBytes.ts` (used by `FileReadToolCall`, `WebFetchToolCall`, `AttachFileToolCall`, `AgentSkillReadFileToolCall`, and the CLI tool formatters). The local copy is removed and the shared helper is imported. 3. **`refactor: drop dead hasTrailingNewline ternary branches in getOutput`** — `src/node/services/backgroundProcessManager.ts` had two `hasTrailingNewline ? allLines.slice(0, -1) : allLines.slice(0, -1)` ternaries inside `getOutput` whose two branches were identical, so the condition never affected the result. Both are replaced with the unconditional `allLines.slice(0, -1)`. In the first occurrence the now-unused `hasTrailingNewline` local is also removed; in the second it is kept because it is still consulted when computing `incompleteLineBuffer`. 4. **`refactor: extract shared stripModelProviderPrefixes helper`** — `anthropicSupportsNativeXhigh` (`src/common/types/thinking.ts`) and `getExplicitThinkingPolicy` (`src/common/utils/thinking/policy.ts`) each inlined the identical `trim → toLowerCase → strip 'provider:' → strip 'namespace/'` chain to reduce a model string to its bare model id for capability matching. The chain moves into a new exported `stripModelProviderPrefixes` helper in `thinking.ts`; both call sites now use it. In `getExplicitThinkingPolicy` the two intermediate locals (`normalized`, `withoutPrefix`) collapse into the single `withoutProviderNamespace` the helper returns. ## Background These cleanups remove duplicated or dead logic that drifts silently. - Runs 1–2 deduped byte-identical helpers (`stripAnsiControlChars`, `formatBytes`) that had been copied across files. - Run 3 followed up coder#3663, which simplified the `hasTrailingNewline ? X : X` pattern in `processMonitorContent` but left two identical instances in `getOutput` untouched. - Run 4 targets the model-string normalization that coder#3664 (Claude Sonnet 5) leaned on: `getExplicitThinkingPolicy` and `anthropicSupportsNativeXhigh` both need the bare model id and had copy-pasted the same normalization. Consolidating it keeps capability predicates consistent about how `provider:` prefixes and gateway `namespace/` segments are stripped. ## Implementation - `stripAnsiControlChars` (run 1): new `src/node/utils/ansi.ts` strips ANSI/VT escape sequences and drops non-printable control characters (keeping tab and any code point >= 0x20). The regex was copied verbatim from the prior inline definitions, so it is byte-identical. - `formatBytes` dedup (run 2): the local function in `InstructionsTab.tsx` is deleted and `formatBytes` is imported from `@/common/utils/formatBytes`; the shared helper is character-for-character equivalent, so the single call site renders identical output. - `hasTrailingNewline` dedup (run 3): the two dead ternaries in `getOutput` collapse to `allLines.slice(0, -1)`. The polling-loop occurrence dropped its `hasTrailingNewline` declaration (it had no other use in that scope); the final-processing occurrence keeps the declaration since `proc.incompleteLineBuffer` still reads `!hasTrailingNewline`. - `stripModelProviderPrefixes` (run 4): the new exported helper runs the same two `.replace()` calls in the same order as the prior inline chains, so it is behavior-identical. `getThinkingDisplayLabel` in the same file is intentionally left alone — it only strips the `provider:` prefix and still inspects `withoutPrefix.startsWith("openai/")`, so it needs the un-stripped namespace and does not fit this helper. ## Validation - `bun test src/common/utils/thinking/policy.test.ts src/common/utils/ai/providerOptions.test.ts src/common/utils/ai/models.test.ts src/common/utils/tools/tools.test.ts` (226 pass) exercises the normalization paths. - `make static-check` passes for these TS-only changes (ESLint, both TypeScript configs, and Prettier all green). The run's only failure is `fmt-shell-check` aborting because `shfmt` is not installed in this environment; the changes touch no shell/Docker files. ## Risks None. All four cleanups are behavior-preserving: the shared helpers reuse the verbatim prior logic, and collapsing a `cond ? X : X` ternary cannot change the result since both branches are identical. --- Auto-cleanup checkpoint: e1169d7 --- _Generated with `mux` • Model: `anthropic:claude-opus-4-8` • Thinking: `xhigh` • Cost: `$0.00`_ <!-- mux-attribution: model=anthropic:claude-opus-4-8 thinking=xhigh costs=0.00 --> --------- Co-authored-by: mux-bot[bot] <264182336+mux-bot[bot]@users.noreply.github.com>
A background bash monitor could wake the agent about output (e.g. "ALL DONE exit=0") that a concurrent task_await had already returned inline. PR #3663 added the right offset primitives but enforced the "don't re-report shown output" invariant only once, at emit time (emitMonitorMatch). The monitor exit-flush and the task_await read are independent async loops, so on process exit the flush can win the race against the read that advances shownThroughOffset, emit the wake, and persist it -- then the drain delivers it later with no re-check. Move the authoritative check to delivery. The match's matchedThroughOffset now travels through the payload into the persisted wake record; drainBashMonitorWakes re-checks it against the settled shown-frontier (getSettledShownThroughOffset, which awaits any in-flight unfiltered read) and supersedes any record already shown. The emit-time check is demoted to a cheap fast-path early-out. Legacy records without the offset and managers without the query fail open (deliver). --- _Generated with `mux` • Model: `anthropic:claude-opus-4-8` • Thinking: `xhigh` • Cost: `$5.81`_ <!-- mux-attribution: model=anthropic:claude-opus-4-8 thinking=xhigh costs=5.81 -->
…oder#3691) ## 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 coder#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`_ <!-- mux-attribution: model=anthropic:claude-opus-4-8 thinking=xhigh costs=26.26 -->
1 participant
Summary
A background bash
monitorcould wake the agent about output that had already been delivered inline, producing a redundant synthetic wake immediately after the read returned (e.g. atask_awaiton a bash task returns the matching FAIL line, then a wake about that same line fires the instant the await resolves).Background
monitormatches and the agent's own reads (task_await/bash_output) run on independent paths throughBackgroundProcessManager. The monitor scans output and emitsmonitor:match; agent reads advance a read cursor viagetOutput(). Nothing connected the two, so when a match flush was deferred (cooldown window) and the agent read the same bytes inline in the meantime, the deferred flush still fired and double-reported output the agent had already seen.Implementation
Suppression is decided at the single emit chokepoint,
emitMonitorMatch, by comparing two absolute file byte offsets:monitor.matchedThroughOffset— the end offset of the last complete line that produced a match. Tracked per matched line (computed by walking the chunk's newlines from its start offset), so a match is never inflated by later or unmatched output in the same poll.proc.shownThroughOffset— the end offset of the last complete line actually delivered to the agent by an unfilteredgetOutputread. It advances in exactly one place and only when nofilter/filter_excludeis in play, so a filtered read (which may drop matched lines) never counts as having shown them.If
shownThroughOffset >= matchedThroughOffset, the matched output was already delivered inline and the pending match is dropped instead of emitted. Because both sides are absolute file offsets, the comparison is order-independent — there is no race between the reader advancing its cursor and the monitor recording its match.This replaces an earlier, fragile approach that reused the raw read cursor (
outputBytesRead) as the "agent has seen this" signal. That cursor advances past content the agent is not actually shown — buffered unterminated fragments, and lines filtered out of a read — which is what made the naive comparison leak wakes in several edge cases.The change is upstream of
WorkspaceService: once a wake is emitted, all existing queue/drain/tool-end delivery behavior is unchanged. Genuinely-new matches and fire-and-forget monitors (whose shown offset stays at 0) wake exactly as before.Risks
Low. The new state is monitor-local and the suppression only narrows emission in the already-shown case. The main behaviors to preserve are covered by tests: drop after an unfiltered read past the match; still wake for a line only buffered unterminated (matched on exit); still wake when later/unmatched output shares the poll; and still wake when a filtered read advanced the cursor without showing the match.
Generated with
mux• Model:anthropic:claude-opus-4-8• Thinking:xhigh• Cost:$15.86