Preserve terminal stream disconnect classification - #1563
Conversation
…eam-disconnect-thr_957f3xid9y
|
🚨 SLOP COP 🚨 · I am SlopCop. I am reviewing this pull request for security, code quality, performance, architecture, and practical behavior. I will post the complete review after the parallel checks finish. |
| error: { | ||
| message: | ||
| "stream disconnected before completion: error sending request for url (https://chatgpt.com/backend-api/codex/responses)", | ||
| codexErrorInfo: "other", |
There was a problem hiding this comment.
🚨 slopcop/review — Please add one negative case for an unrelated other error.
The PR states that unrelated generic errors stay unchanged. This test proves only the positive match.
Add a second case with message: "request failed". Assert category: "unknown" and providerCode: "other". This case will protect the narrow guard.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: Codex sometimes puts the wrong name on a lost connection. This change adds the correct label, like a label on a toy box.
Review result
I found no security, correctness, performance, or architecture defect. The Codex adapter is the correct layer, and no duplicate classifier exists.
One minor test gap remains. The new test covers the matching other error, but it does not protect unrelated other errors. I left an inline suggestion for a negative case.
The anchored regular expression has a bounded cost. The category change does not affect retries, process restarts, credentials, or rate-limit recovery.
I ran these checks:
pnpm exec turbo run test --filter=@bb/agent-runtime --force: 46 files and 943 tests passed.pnpm exec turbo run typecheck --filter=@bb/agent-runtime: passed.- A direct adapter replay returned
stream-disconnectedwithresponseStreamDisconnected. - All required GitHub CI checks pass.
I did not run a browser test because this change has no web route. The direct adapter replay tested the complete changed path.
This is a comment-only review. I did not approve the PR or request changes.
What changed
stream-disconnectedclassification when Codex exhausts reconnect attempts and reports the terminal failure ascodexErrorInfo: "other".stream disconnected before completionmessage prefix; retry semantics and unrelated generic errors are unchanged.Local reproduction
The deterministic replay below uses the exact terminal Codex event from the incident. It does not require intentionally breaking your network.
1. Reproduce on
mainFrom a fresh clone:
Observed on
main(e1ec66e8cwhen this PR was opened):This is the bug: the terminal event loses the structured disconnect category, so BB renders the final timeline row as Provider error.
2. Verify this PR
In the same clone:
Rerun the same
node ... <<'NODE'block above. Expected output:BB can now render the terminal row consistently as Provider stream disconnected.
Optional real-network reproduction
Before this fix, reconnect attempts carry
responseStreamDisconnected, but the finalwillRetry: falseevent can carryproviderCode: "other"andcategory: "unknown"even though its detail still starts withstream disconnected before completion:. Network behavior and retry timing make this path less deterministic than the event replay above.Root cause
Codex preserves
responseStreamDisconnectedduring reconnect attempts, then can downgrade the same terminal failure tocodexErrorInfo: "other"after its retry budget is exhausted. BB trusted that degraded terminal value and discarded the more specific classification encoded in the unchanged message.Verification
main: reproducedcategory: "unknown".category: "stream-disconnected".pnpm exec turbo run test --filter=@bb/agent-runtime: 46 files, 943 tests passed.pnpm exec turbo run typecheck --filter=@bb/agent-runtime: passed.BB-Thread-ID: thr_957f3xid9y