fix(agent): preserve output after trailing model failure - #3533
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change preserves completed streamed output after non-timeout provider failures follow a completed tool handoff. Timeout errors now produce failed terminal states. Shared timeout classification covers stream, idle, bootstrap, and chat stream timeout messages. ChangesStream completion handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/streaming/stream-outcome.ts (1)
130-143: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply the completed-output gate to
providerError.Lines 142-143 return a failed outcome whenever
providerErroris set withoutthrownError. This bypasses the completed-output gate. A completed tool handoff can therefore fail when the caller reports the trailing provider failure asproviderError.
src/agent/streaming/stream-outcome.ts#L130-L143: apply the output-and-finish-signal exception before boththrownErrorandproviderErrorfailure paths.src/agent/streaming/stream-outcome.test.ts#L135-L142: add a completedtool_handoffcase withproviderErrorand nothrownError; expecttool_handoff.As per coding guidelines, “For behavior changes, add or update a focused failing test before changing implementation.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent/streaming/stream-outcome.ts` around lines 130 - 143, Apply the completed-output gate in the stream-outcome classification flow before handling either thrownError or providerError, so completed output with a valid finish signal returns the successful tool_handoff outcome even when only providerError is present. First add a focused completed tool_handoff test with providerError and no thrownError in src/agent/streaming/stream-outcome.test.ts lines 135-142, then update the logic around the existing failure paths in src/agent/streaming/stream-outcome.ts lines 130-143.Source: Coding guidelines
🧹 Nitpick comments (1)
src/agent/hosted/hosted-chat-finalization.ts (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the internal import alias.
This import crosses from the
hostedmodule to thestreamingmodule. Replace the relative path with the configured#veryfront/*alias.Based on learnings, use
#veryfront/*aliases when an import crosses a module boundary. As per coding guidelines, use#veryfront/*for internal source imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent/hosted/hosted-chat-finalization.ts` at line 12, Update the import of hasCompletedStepSignal in hosted-chat-finalization.ts to use the configured `#veryfront/`* internal alias for the streaming module instead of the relative path, leaving the imported symbol unchanged.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/agent/streaming/stream-outcome.ts`:
- Around line 130-143: Apply the completed-output gate in the stream-outcome
classification flow before handling either thrownError or providerError, so
completed output with a valid finish signal returns the successful tool_handoff
outcome even when only providerError is present. First add a focused completed
tool_handoff test with providerError and no thrownError in
src/agent/streaming/stream-outcome.test.ts lines 135-142, then update the logic
around the existing failure paths in src/agent/streaming/stream-outcome.ts lines
130-143.
---
Nitpick comments:
In `@src/agent/hosted/hosted-chat-finalization.ts`:
- Line 12: Update the import of hasCompletedStepSignal in
hosted-chat-finalization.ts to use the configured `#veryfront/`* internal alias
for the streaming module instead of the relative path, leaving the imported
symbol unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 263c2631-ffec-48e0-b459-af04c819b860
📒 Files selected for processing (5)
src/agent/hosted/hosted-chat-finalization.test.tssrc/agent/hosted/hosted-chat-finalization.tssrc/agent/hosted/stream-finalization.test.tssrc/agent/streaming/stream-outcome.test.tssrc/agent/streaming/stream-outcome.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d83e58cce1
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/agent/hosted/stream-terminal-error.ts (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the internal import alias for this cross-module import.
This import crosses from
src/agent/hostedtosrc/agent/streaming. Use the repository’s#veryfront/*alias instead.As per coding guidelines, internal
src/**/*.tsimports use#veryfront/*. Based on learnings, relative imports are reserved for same-directory siblings, and this import crosses module boundaries.Proposed fix
-import { isStreamTimeoutError } from "../streaming/stream-outcome.ts"; +import { isStreamTimeoutError } from "`#veryfront/agent/streaming/stream-outcome.ts`";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent/hosted/stream-terminal-error.ts` at line 4, Update the import of isStreamTimeoutError in stream-terminal-error.ts to use the repository’s `#veryfront/`* internal alias for the streaming module instead of the relative cross-module path; leave same-directory relative imports unchanged.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/agent/hosted/stream-terminal-error.ts`:
- Line 4: Update the import of isStreamTimeoutError in stream-terminal-error.ts
to use the repository’s `#veryfront/`* internal alias for the streaming module
instead of the relative cross-module path; leave same-directory relative imports
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b2cc01c7-7d22-477f-b973-d5759d98c1bf
📒 Files selected for processing (5)
src/agent/hosted/hosted-chat-finalization.test.tssrc/agent/hosted/hosted-chat-finalization.tssrc/agent/hosted/stream-terminal-error.tssrc/agent/streaming/stream-outcome.test.tssrc/agent/streaming/stream-outcome.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/agent/hosted/hosted-chat-finalization.ts
- src/agent/streaming/stream-outcome.test.ts
Summary
This is the runtime portion of veryfront/veryfront-issue-inbox#338. No API persistence change is required because the runtime now reports the canonical run as completed. The Agent consumer must be updated to the released package before the issue is closed.
Verification
deno test --no-check --allow-all src/agent/hosted/hosted-chat-finalization.test.ts src/agent/hosted/stream-finalization.test.ts src/agent/streaming/stream-outcome.test.tsdeno task test:unit(3,778 passed, 28,062 steps; 1 ignored)deno task fmt:checkdeno task lintdeno task typecheckSummary by CodeRabbit
Bug Fixes
Tests