Ignore Codex raw response completion events - #1314
Merged
ymichael merged 1 commit intoAug 11, 2026
Merged
Conversation
Collaborator
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and product behavior. I will post one final review after all checks finish. |
ymichael
deleted the
bb/categorize-codex-raw-response-completed-thr_xd3ysrtrv5
branch
August 11, 2026 03:48
SawyerHood
reviewed
Aug 11, 2026
| "process/outputDelta": "unknown", | ||
| // Internal per-response accounting; thread/tokenUsage/updated carries the | ||
| // user-facing token state. | ||
| "rawResponse/completed": "noise", |
Collaborator
There was a problem hiding this comment.
🚨 slopcop/review — P3: Stop this unused notification at the source.
BB enables experimentalRawEvents because it needs rawResponseItem/completed for command output recovery.
However, BB does not use rawResponse/completed.
This patch drops the notification only after Codex creates, serializes, sends, and BB parses it.
Codex supports an exact method filter through initialize.params.capabilities.optOutNotificationMethods.
Please add rawResponse/completed to that list and test the initialize request.
This change keeps required item events and avoids work for each upstream response.
SawyerHood
reviewed
Aug 11, 2026
SawyerHood
left a comment
SawyerHood
left a comment
Collaborator
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5
This change hides an extra machine receipt.
The user still sees the useful token total.
It is like discarding a duplicate shop receipt.
Findings
- P3: BB can stop
rawResponse/completedat Codex initialization. - The current patch drops each event after Codex sends it.
- Codex supports an exact filter through
optOutNotificationMethods. - I found no security or correctness issue.
- The protocol bump to 97 is correct because the daemon output changes.
Architecture
The notification method map duplicates the coverage map.
The code can derive membership from the coverage map and remove one source of stale keys.
This note does not identify a current bug.
Checks
- Both affected package type checks passed.
- All 165 focused agent-runtime tests passed.
- All 49 host-daemon-contract tests passed.
- GitHub CI passed at the exact head SHA.
- The full runtime suite passed 875 of 876 tests.
- One unrelated process-tail test failed twice outside this pull-request diff.
I did not start a development server because this provider notification has no direct browser action.
The adapter tests isolate the event path more accurately.
This is a comment-only review. I found no blocking issue.
SawyerHood
pushed a commit
that referenced
this pull request
Aug 11, 2026
…thread (#1321) # Problem Every thread on the host freezes at "waiting" and only a full app restart clears it. This has now happened **four times on bb-app 0.36.0**, each time triggered by a single event the server could never store. The daemon holds **one in-memory event queue for the whole host** and reposts it as a single batch. When the head of that queue is an event the server deterministically refuses, the batch can never succeed — so every other thread's `turn/started`, `item/*` and `turn/completed` events pile up behind it and never reach the database. The UI reads the database, so every thread looks stuck. ### From the logs `~/.bb/logs/server.3.log` — the first rejection, then the same one repeating verbatim: ```json {"level":40,"time":1786360836499,"eventType":"provider/unhandled","scopeKind":"turn", "threadId":"thr_fpx3vkax5h","turnId":"auto-compact-1", "errorMessage":"Cannot append provider/unhandled for turn auto-compact-1 before turn/started is stored", "errorName":"MissingStoredTurnStartedError","msg":"Rejected daemon event before turn/started"} {"level":40,"time":1786360836616,"eventType":"provider/unhandled","scopeKind":"turn", "threadId":"thr_fpx3vkax5h","turnId":"auto-compact-1", ... } ``` Every occurrence, grouped by the turn that poisoned the queue: | Thread | Turn | Rejections | Window | |---|---|---:|---| | `thr_dwmzmanhn5` | `auto-compact-2` | 1911 | 08-07 14:57:19 → 15:27:06 (29.8 min) | | `thr_fpx3vkax5h` | `auto-compact-1` | 505 | 08-10 13:20:36 → 13:25:57 (5.3 min) | | `thr_sdc5dy277m` | `auto-compact-3` | 171 | 08-10 13:48:47 → 13:52:43 (3.9 min) | | `thr_qifimqh4a6` | `auto-compact-1` | 260 | 08-10 15:08:46 → 15:14:00 (5.2 min) | Every window ends at a restart, never at a recovery. During the 13:20 window the server logged **no thread activity whatsoever** — only the rejections: ``` 1 [plugin:connect] rpc listAccountServers failed: not_paired 5 Skipping malformed prompt history row 1 [plugin:agent-limits] disposed <- the restart ``` The 15:08 occurrence is visible directly in the database. Rows inserted per minute across all threads, spanning that window: ``` 15:03 | 90 events | 4 threads 15:04 | 91 | 1 15:05 | 66 | 1 15:06 | 22 | 1 15:07 | 32 | 1 15:08 | 35 | 2 <- poison event lands at 15:08:46 15:09 | 0 | 0 15:10 | 0 | 0 15:11 | 1 | 1 15:12 | 0 | 0 15:13 | 4 | 2 15:14 | 29 | 4 <- restart at 15:14:00 15:15 | 48 | 3 ``` Five minutes in which the whole machine persisted essentially nothing, then instant recovery on restart. Those events are gone: the queue is in-memory, so the restart that clears the wedge also discards everything held behind it, leaving a hole in each affected thread's transcript. # Root cause 1. **A provider-minted turn id is trusted.** `createUnhandledProviderEvent` falls back to reading `turnId` out of the raw provider event when the caller does not supply one: ```ts const turnId = args.turnId ?? getTurnIdFromRawEvent(args.rawEvent); ``` Codex labels its automatic-compaction traffic `auto-compact-N`. The string `auto-compact` appears nowhere in bb's source — it is entirely provider-minted, and every `provider/unhandled` event on all four affected threads carries `providerId: "codex"`. bb never opened that turn, so it never emitted a `turn/started` for it. Critically, every caller supplies `turnId` from bb's own turn registry and omits it *only when bb has no active turn* — precisely the case where a scraped id is guaranteed wrong. 2. **The server hard-rejects the orphan.** `resolveDaemonTurnStartDisposition` finds no stored `turn/started`; the escape hatch `ORPHAN_DROPPABLE_TURN_EVENT_TYPES` held only the two usage-snapshot types, so it throws `MissingStoredTurnStartedError`. 3. **The whole batch dies with it.** `/session/events` appends every event in one `immediate` transaction, so the throw rolls all of them back and returns `409 invalid_request`. 4. **The daemon reposts it forever.** The drain loop takes the entire queue as one batch and splices only on success: ```ts try { response = await options.postEvents(batch) } catch (error) { logger.error(..., "Failed to post daemon events; will retry on the next flush"); return; // queue untouched } queue.splice(0, batch.length); // only reached on success ``` The daemon already knows this class of error is permanent — `defaultRetryableForStatus(409)` is `false`, and `ServerResponseError.retryable` carries that verdict — but nothing consults it. # Fix **1. `apps/host-daemon/src/event-sink.ts` — never repost a batch the server permanently refused.** On a non-retryable `invalid_request`, the sink bisects the batch, drops the events that are undeliverable by construction, and delivers the rest. Since the server appends in one transaction and rolls back entirely on refusal, nothing was committed and re-posting the halves cannot duplicate. Isolating k bad events costs O(k log n) posts. The `invalid_request` code check is what keeps this narrow: `/session/events` also fails non-retryably with `401 unauthorized` and `401 inactive_session`, and those say nothing about the events themselves. Those must stay queued for the session the daemon is about to reopen, not be discarded one at a time — there is a regression test for exactly this. **2. `packages/agent-runtime/src/shared/provider-unhandled-event.ts` — stop trusting provider turn ids.** Only a turn id the caller vouched for scopes the event; the raw-event fallback is gone. **3. `packages/db/src/data/events.ts` — `provider/unhandled` becomes orphan-droppable.** A backstop, in the spirit of the existing comment about fork usage snapshots. An unhandled passthrough event is diagnostic only: losing one is a non-event, failing the batch it rode in with is not. Turn-content events still require a stored `turn/started`, so genuine ordering bugs are still caught. **4. The queue-backup tripwire logs at `warn`, not `debug`.** It never once fired in any of the four incidents, so there was no signal short of noticing the UI had stopped moving. Fix 1 is the load-bearing one. Fixes 2 and 3 close this particular trigger; only fix 1 stops the *next* unknown orphan event from wedging the host. ## Note on ordering of fixes 1 and 3 Fix 3 alone repairs already-enrolled daemons: an old daemon talking to a new server stops receiving 409s, so the wedge cannot recur even before it updates. Fix 1 is what makes the daemon resilient to the next unknown case. # Protocol version Bumped `HOST_DAEMON_PROTOCOL_VERSION` 99 → 100, matching the convention used by #1224, #1208, #1232, #1314 and #1236 for daemon-behaviour changes. Nothing in the wire *schema* changed, and both directions are compatible (old daemon + new server is in fact the repair path above) — the bump is here to push fix 1 out to enrolled machines rather than leave them on a build that wedges. Happy to drop it if you would rather not force an update cycle for this. # Tests Written as reproductions first, and confirmed failing against the base commit before the fix: | Test | Package | Reproduces | |---|---|---| | `ignores a provider-supplied turn id the caller did not vouch for` | `@bb/agent-runtime` | root cause — `auto-compact-1` scraped from raw params | | `drops orphan provider/unhandled events instead of failing the batch` | `@bb/db` | the batch-wide rollback | | `drops a permanently rejected event instead of retrying it forever` | `@bb/host-daemon` | the infinite repost | | `delivers events queued behind a permanently rejected event` | `@bb/host-daemon` | **the wedge itself** — healthy traffic from other threads gets through | | `accepts a batch carrying a provider/unhandled event for a turn bb never started` | `@bb/server` | end-to-end at the `/internal/session/events` route that produced the 409 | Plus guards against over-correcting: - `keeps events queued when the session, not the batch, is rejected` — a 401 must not bisect the queue away. - `keeps retrying a batch that fails for a retryable reason` — 5xx behaviour unchanged. One existing expectation changed: `codex/adapter.test.ts > translateEvent unknown codex notifications fall back to provider/unhandled` now expects thread scope. That path handles notifications which *failed* schema parsing, so nothing there vouches for the turn id; Codex notifications bb does parse still carry turn scope. Comment in the test explains it. ## Verification Rebased onto `d07c1ce28` and re-verified there. `pnpm exec turbo run test` on `@bb/db`, `@bb/agent-runtime`, `@bb/host-daemon`, `@bb/host-daemon-contract`, `@bb/server`, `@bb/integration-tests`: ``` @bb/host-daemon-contract 49 passed (49) @bb/host-daemon 526 passed (526) @bb/db 378 passed (378) @bb/server 1405 passed (1405) @bb/integration-tests 55 passed (55) @bb/agent-runtime 894 passed | 1 failed (895) ``` `typecheck` and `lint` clean across all of them. The single `@bb/agent-runtime` failure — `runtime.process-lifecycle.test.ts > bounds provider stderr while data arrives without a newline` — **fails identically on unmodified `origin/main`** and is unrelated to this change. --- Fixes #1320 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
rawResponse/completednotifications as provider noise instead of rendering an unhandled-event rowTests
pnpm exec turbo run typecheck --filter=@bb/agent-runtime --filter=@bb/host-daemon-contractpnpm exec turbo run test --filter=@bb/agent-runtime --force -- src/codex/adapter.test.ts src/provider-visibility.test.ts(165 passed)pnpm exec turbo run test --filter=@bb/host-daemon-contract --force(49 passed)git diff --checkBroader suite note
The full
@bb/agent-runtimesuite passed 875 tests but twice failed the unrelatedruntime.process-lifecycle.test.tsstderr-tail timing assertion. The changed Codex adapter and provider-visibility suites pass cleanly.