fix(core): close supervisor streams with output guardrails - #1417
noooooooookro wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 540c815 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe guardrail pipeline now finalizes when it receives a finish part and continues consuming the base stream. A regression test verifies supervisor stream completion and the emitted finish event. A patch changeset documents the fix. ChangesSupervisor output guardrail completion
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The supervisor output-guardrail completion fix has regression coverage for the merged supervisor stream path, with no concrete unresolved risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/agent/streaming/guardrail-stream.ts">
<violation number="1" location="packages/core/src/agent/streaming/guardrail-stream.ts:142">
P2: When `finish` arrives, `finalizeGuardrails()` resolves `finalizePromise` before the merged supervisor stream is drained and before the sanitized finish part is enqueued and `controller.close()` runs. If the drain, the post-loop enqueue, or `controller.close()` then throws, the `catch` calls `finalizeReject(error)` on an already-resolved promise, so the rejection is silently dropped; consumers awaiting `finalizePromise` (e.g. `createSanitizedTextPromise` in `agent.ts:2598`) see success even though the guarded stream errored. Previously `finalizeResolve()` was the last statement of the success path, so any error rejected the promise.</violation>
</file>
<file name="packages/core/src/agent/streaming/guardrail-stream.spec.ts">
<violation number="1" location="packages/core/src/agent/streaming/guardrail-stream.spec.ts:387">
P3: This regression test never exercises the fix's drain-after-finish path: the mocked stream ends at `finish` with no parts after it, so the new `if (finishPart) continue;` branch never runs. Tracing `createMergedFullStream` (agent.ts:2507), the pre-fix `break` also completes cleanly on this input because the writer is closed once the final part is read, so the test passes before the fix and would not catch a re-introduction of #1415. The merged stream needs to emit parts after `finish` (e.g. sub-agent output after the parent finish) for the hang to be reproduced.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| : {}; | ||
| await runner.finalize(metadata); | ||
| finalized = true; | ||
| finalizeResolve(); |
There was a problem hiding this comment.
P2: When finish arrives, finalizeGuardrails() resolves finalizePromise before the merged supervisor stream is drained and before the sanitized finish part is enqueued and controller.close() runs. If the drain, the post-loop enqueue, or controller.close() then throws, the catch calls finalizeReject(error) on an already-resolved promise, so the rejection is silently dropped; consumers awaiting finalizePromise (e.g. createSanitizedTextPromise in agent.ts:2598) see success even though the guarded stream errored. Previously finalizeResolve() was the last statement of the success path, so any error rejected the promise.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/agent/streaming/guardrail-stream.ts, line 142:
<comment>When `finish` arrives, `finalizeGuardrails()` resolves `finalizePromise` before the merged supervisor stream is drained and before the sanitized finish part is enqueued and `controller.close()` runs. If the drain, the post-loop enqueue, or `controller.close()` then throws, the `catch` calls `finalizeReject(error)` on an already-resolved promise, so the rejection is silently dropped; consumers awaiting `finalizePromise` (e.g. `createSanitizedTextPromise` in `agent.ts:2598`) see success even though the guarded stream errored. Previously `finalizeResolve()` was the last statement of the success path, so any error rejected the promise.</comment>
<file context>
@@ -127,14 +127,33 @@ export function createGuardrailPipeline(
+ : {};
+ await runner.finalize(metadata);
+ finalized = true;
+ finalizeResolve();
+ };
</file context>
| { type: "text-start", id: "text-1" }, | ||
| { type: "text-delta", id: "text-1", delta: "hello" }, | ||
| { type: "text-end", id: "text-1" }, | ||
| { type: "finish", finishReason, usage }, |
There was a problem hiding this comment.
P3: This regression test never exercises the fix's drain-after-finish path: the mocked stream ends at finish with no parts after it, so the new if (finishPart) continue; branch never runs. Tracing createMergedFullStream (agent.ts:2507), the pre-fix break also completes cleanly on this input because the writer is closed once the final part is read, so the test passes before the fix and would not catch a re-introduction of #1415. The merged stream needs to emit parts after finish (e.g. sub-agent output after the parent finish) for the hang to be reproduced.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/src/agent/streaming/guardrail-stream.spec.ts, line 387:
<comment>This regression test never exercises the fix's drain-after-finish path: the mocked stream ends at `finish` with no parts after it, so the new `if (finishPart) continue;` branch never runs. Tracing `createMergedFullStream` (agent.ts:2507), the pre-fix `break` also completes cleanly on this input because the writer is closed once the final part is read, so the test passes before the fix and would not catch a re-introduction of #1415. The merged stream needs to emit parts after `finish` (e.g. sub-agent output after the parent finish) for the hang to be reproduced.</comment>
<file context>
@@ -371,6 +371,52 @@ describe("Output guardrail streaming integration", () => {
+ { type: "text-start", id: "text-1" },
+ { type: "text-delta", id: "text-1", delta: "hello" },
+ { type: "text-end", id: "text-1" },
+ { type: "finish", finishReason, usage },
+ ],
+ }),
</file context>
1 participant
What
finishpart.Why
The guardrail pipeline previously stopped consuming the merged stream as soon as it received
finish. For supervisor agents, this caused the merged stream cleanup and guardrail finalization to wait on each other, leaving the stream open indefinitely.Verification
guardrail-stream.spec.ts: 8 tests passed@voltagent/coretypecheck passed@voltagent/corebuild passedFixes #1415
Summary by cubic
Fixes supervisor streams hanging indefinitely when output guardrails are enabled. Previously, the guardrail pipeline stopped consuming the merged supervisor stream as soon as it received
finish, so stream cleanup and guardrail finalization waited on each other. Now the pipeline keeps draining the merged stream and finalizes guardrails before closing the guarded stream. Adds a regression test covering a supervisor with output guardrails. Fixes #1415.Written for commit 540c815. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation