fix(agent,shared): an empty model reply finished the turn as a success, and Stop was reported as a crash - #196
Conversation
…s, and Stop was reported as a crash Two crash classes from GlitchTip project `ai-designer` (19 events of `AI_NoOutputGeneratedError` across issues 67/75/91/93, 2 of `AbortError` in issue 76). Both are made at the same line of `ai@7.0.19` — the ternary in `streamText`'s `eventProcessor` flush that hands `rejectResultPromises` either `abortSignal.reason` or a fresh `NoOutputGeneratedError`. Confirmed by column: building this repo at `ab2527e` puts that `new` at `background.js` 115:36989, the exact frame all 14 events of issue 75 carry. The AbortError is the user pressing Stop. All five `AbortController.abort()` calls live in `background.ts` and all five are a deliberate end-of-turn (supersede, session-start, session-stop, conversation-new, nav abort); `AbortSignal.timeout` appears nowhere in `src/`, and the other abortable waits reject with a plain `Error`. Each already ends the turn with a reason the user sees, so reporting it as a crash is noise. Dropped at the Sentry seam only — `background.ts`'s `unhandledrejection` listener still logs it. The no-output error had a SILENT twin, and the twin is the user-visible bug. The SDK raises it only when no step completed at all; a gateway that answers 200 and closes still yields a finish chunk, so a step is recorded and the turn returned `stop: 'done'` with an empty reply and no error. An empty event-stream body, a JSON error object served as an event stream, and a chunk with `choices: []` all landed there — all three are how an OpenAI-compatible gateway says "that model id resolved to no provider" without a status the SDK's retry would act on. Now named. Also stamps `release` (`designer@<manifest version>`) — the field `scrubEvent` has always allowlisted and nothing ever set, which is why issue 93 could not be tied to a build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
On-demand reviews are free for the next 25 days. After that, they cost $0.25 per reviewed file. Or wait 33 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 77 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
Warning
|
This comment has been minimized.
This comment has been minimized.
…e panel phoned home at rest
`browserSessionIntegration` is in the browser SDK's DEFAULT set and calls
`captureSession()` from its `setupOnce`. It had been inert here only by
accident: `Client.sendSession` discards a session when the client has no
`release`. Stamping `release` in this branch is exactly what armed it, so every
side panel mount now POSTed a session envelope to GlitchTip before anything had
gone wrong.
Caught by `test/e2e/smoke.spec.ts` ("side panel makes zero blocked/remote font
or script requests"), which went red on CI and reproduces locally against the
built extension.
It comes out rather than being tolerated. It is unsolicited network traffic from
a panel at rest, in an extension whose whole crash-report seam exists to keep
things inside the browser; and `beforeSend` cannot police it, because that hook
sees error events, not session envelopes — both the scrub and the `isUserAbort`
drop sit downstream of a lane they never see.
The filter is subtractive (keep the SDK's defaults, minus the one lane we
refuse) and the integration list type is derived from
`Sentry.getDefaultIntegrations` rather than re-declared, since `@sentry/browser`
does not re-export `Integration`.
Guard: a unit test asserts the wired reducer removes `BrowserSession` from the
REAL default set and keeps every other entry — non-vacuous, because it first
asserts the default set actually contains it. Observed red before the fix
("Sentry.init was not called with an integrations reducer"). The stale
`not.toHaveProperty('integrations')` assertion the fix invalidates is replaced
by that test; the replay/tracing assertions beside it are untouched.
|
@coderabbitai review |
|
What
Two production crash classes from GlitchTip project
ai-designer(5 unresolved issues, 21 events, 2026-07-31 to 2026-08-18):AI_NoOutputGeneratedError— 19 events across issues 67 (2x), 75 (14x), 91 (3x), 93 (1x). A turn that produces nothing now says so, loudly and by name, instead of finishing as a silent success.AbortError— 2 events, issue 76. The user pressing Stop is no longer reported to GlitchTip as a crash.Plus: every report ships a
releasenow, so the next one can be tied to a build.Why
Where the error is made. Both classes are the same one line of
ai@7.0.19: the ternary instreamText'seventProcessorflush, which handsrejectResultPromiseseitherabortSignal.reason(anAbortError) or a freshNoOutputGeneratedError. Confirmed by column, not by guess: building this repo atab2527e(the tip while issue 75 was firing) putsnew NoOutputGeneratedError({message: "No output generated. Check the stream for errors."})atbackground.jsline 115 column 36989 — the exact frame all 14 events of issue 75 carry. Issue 76'sAbortErrorframes land in the message handler beside theturnAbort?.abort()calls.AbortErroris not a crash. Nothing here aborts on a deadline. All fiveAbortController.abort()calls live inbackground.tsand all five are a deliberate end-of-turn: supersede,session-start,session-stop(the Stop button),conversation-new, and the cross-document nav abort inonCommitted.AbortSignal.timeoutappears nowhere insrc/, and the only other abortable waits (waitForTabComplete,browseDelay) reject with a plainError. Each of the five already ends the turn with a reason the user sees —runTurnreturnsstop: 'aborted', and the nav abort posts its own message. Reporting it as an unhandled crash is noise, and noise is what makes the real reports get skimmed. It is dropped at the Sentry seam only:background.ts'sunhandledrejectionlistener still writes it to the conversation's debug log, and the worker console still prints it.AI_NoOutputGeneratedErrorhad a silent twin, and the twin is the user-visible bug. The SDK raises that error only when the stream carried no completed step at all — a 401/404/5xx on the first model call, whichrunTurnalready catches and shows. But an OpenAI-compatible gateway that answers 200 and then closes still yields a finish chunk, so a step IS recorded,streamTextreports a clean empty finish, andrunTurnreturnedstop: 'done'with an empty reply, no error, and nothing on screen. Driving the real@ai-sdk/openai-compatibleparser against a stubfetch, three realistic gateway replies all land there:text/event-streambodystop: done, empty, silentstop: error+ named messagestop: done, empty, silentstop: error+ named messagechoices: []stop: done, empty, silentstop: error+ named messageThat is "the model returned no output" as the user meets it, and all three are how a gateway says that model id resolved to no provider without an HTTP status the SDK's retry would act on. The config is the first thing to check, so the message says so.
Changes
src/agent/loop.ts— track whether the turn asked for any tool; a natural finish (stop === 'done') with no prose and no tool call flips tostop: 'error'and emitsEMPTY_TURN_ERROR. Gated ondoneso it can only describe a turn the model ended itself:aborted,budgetanderroreach already carry their own reason and never reach it.src/shared/sentry.ts—isUserAbort(exported, enumerated in its own doc comment);beforeSenddrops those events and scrubs the rest.scrubEventis untouched: privacy scrub and worth-reporting stay two seams.releaseis now set todesigner@<manifest version>— the fieldscrubEventhas always allowlisted and nothing ever populated.integrationsnow subtracts the SDK'sBrowserSessiondefault — see The release stamp had a side effect below.test/integration/empty-turn.test.ts(new) — the three gateway replies plus a bare mock finish, each pinned toEMPTY_TURN_ERRORby identity, not substring; two anti-vacuity partitions (a tools-only turn and a gateway that streams real content) must staydone.test/unit/sentry.test.ts— the abort drop asserted through the wiredbeforeSend,AI_NoOutputGeneratedErrorasserted to still ship (scrubbed), andisUserAbortfalse for a mixed multi-exception event and for an exception-less one.test/unit/sentry.test.ts— plus the session-integration guard: the wired reducer is run over the realgetDefaultIntegrations()set and must dropBrowserSessionand keep every other entry. It asserts the default set containsBrowserSessionfirst, so it cannot pass by filtering a name the SDK no longer ships.The release stamp had a side effect
Stamping
releasearmed a lane nobody asked for.browserSessionIntegrationis in the browser SDK's default integration set and callscaptureSession()from itssetupOnce; it had been inert here purely by accident, becauseClient.sendSessiondiscards a session when the client has norelease. So the moment this branch set one, every side panel mount POSTed a session envelope to GlitchTip before anything had gone wrong.Found by CI, not by reading:
test/e2e/smoke.spec.ts("side panel makes zero blocked/remote font or script requests") went red on aglitchtip.infra.developerz.ai/api/2/envelope/request from a freshly opened panel, and reproduces locally against the built extension.It is removed rather than tolerated, for two reasons. It is unsolicited network traffic from a panel at rest, in an extension whose entire crash-report seam exists to keep things inside the browser. And
beforeSendcannot police it: that hook sees error events, not session envelopes, so both the scrub and theisUserAbortdrop sit downstream of a lane they never see.The filter is subtractive — keep whatever the installed SDK ships by default, minus the one lane refused — and the list's element type is derived from
Sentry.getDefaultIntegrationsrather than re-declared, because@sentry/browserdoes not re-exportIntegration.Verification
loop.tsguard fails 4 of the 6 new integration tests and leaves both anti-vacuity partitions green; reverting thesentry.tswiring fails exactly the 2 newinitSentrytests.stop === 'done'gate is what keeps the two states apart, and that is pinned by an existing test, not by reasoning: drop the gate andagent-loop.test.ts's "reports an aborted turn without emitting an error" goes red withexpected 'error' to be 'aborted'— a Stop would be re-reported as an empty turn. Run red, then restored.integrations, the new unit test fails with "Sentry.init was not called with an integrations reducer".bun run gateclean — lint (415 files), typecheck (tsgo), 2709 unit + integration tests in 197 files, build, and 56 Playwright e2e tests against the loaded extension, including the smoke test that caught the envelope.Follow-up
Not fixed, and stated plainly: why these errors reach
unhandledrejectionat all is still open.runTurnalready parks a catch onresult.steps/finishReason/totalUsage, andai@7.0.19additionally callsmarkPromiseAsHandledon all five promisesrejectResultPromisestouches — verified present in the minifiedab2527ebundle that produced the reports. Eleven scenarios reproduced against bothMockLanguageModelV4and the real provider (empty stream, no finish chunk, provider error part, transport error mid-stream, HTTP 404, retries exhausted, abort mid-stream, step-2 empty, tool throw, and the three gateway replies above) produce zero unhandled rejections on currentmain. So the residual leak path is unidentified. Thereleasestamp is what closes that loop: the next report will name its build, which is precisely what issue 93 (a single event on 2026-08-18, from an install that could not be dated) could not.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.