fix(agents): drop streamTo silently for non-ACP spawn requests#58686
fix(agents): drop streamTo silently for non-ACP spawn requests#58686Mintalix wants to merge 5 commits into
Conversation
Some callers retain ACP-only fields when falling back from ACP to subagent runtime. Previously the tool returned an error; now streamTo is silently dropped for subagent requests and forwarded only for runtime=acp. Schema description updated to document the ACP-only contract.
Greptile SummaryThis PR removes an overly strict error guard that rejected Key changes:
The implementation is correct and consistent — Confidence Score: 5/5Safe to merge — the fix is minimal, well-scoped, and fully covered by new tests. No P0 or P1 issues found. The logic change is correct: No files require special attention. Reviews (1): Last reviewed commit: "fix(agents): drop streamTo silently for ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Fixes sessions_spawn to treat streamTo as an ACP-only hint by forwarding it only for runtime="acp" and silently ignoring it for subagent spawns, preventing otherwise-valid subagent spawn requests from being rejected.
Changes:
- Update
sessions_spawntool behavior to dropstreamTofor non-ACP runtimes instead of returning an error. - Update the
streamToschema description to document the ACP-only contract. - Replace the prior rejection test with regression tests asserting
streamTois ignored for explicit/implicit subagent runtime.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/agents/tools/sessions-spawn-tool.ts | Drops streamTo for subagent runtime; updates schema docs to clarify ACP-only semantics. |
| src/agents/tools/sessions-spawn-tool.test.ts | Adds/updates unit tests to lock in “ignore streamTo on subagent” behavior. |
bshelby88
left a comment
bshelby88
left a comment
There was a problem hiding this comment.
LGTM! The logic is correct:
streamTo = runtime === "acp" ? requestedStreamTo : undefinedcorrectly drops the param for non-ACP requests- Test coverage is good — both explicit
runtime="subagent"and omitted runtime (defaulting to subagent) are tested expect.not.objectContainingcorrectly verifies streamTo is NOT passed to the spawn function- Schema description update documents the ACP-only behavior
Silently dropping is the right call here — more graceful than erroring when callers fall back from ACP to subagent with leftover ACP-specific fields.
|
@vincentkoc Friendly ping on this one. Since #55483 was closed as dirty and #56906 has been quiet, I opened #58686 on a clean branch with the full fix and regression tests. When you have a chance, could you take a look and let me know if we should continue here and close #56906? Thanks. |
|
Thanks @Mintalix for the focused fix and regression coverage in this PR. ProjectClownfish is closing this as superseded by #72614. #72614 carries the same narrow If #72614 misses a behavior covered here, please reply and we can reopen or split the gap back out. |
Summary
Problem: sessions_spawn returned { status: "error" } when streamTo: "parent" was passed with runtime: "subagent" or with runtime omitted (defaults to subagent). Callers that retain the full ACP parameter set after a runtime fallback were rejected.
Why it matters: streamTo is an output-routing hint with no effect on subagent spawns. Erroring on a no-op field breaks valid spawn flows for callers that don't strip ACP-only params before calling.
What changed: Removed the hard-error guard on streamTo for non-ACP runtime. streamTo is now resolved before the runtime branch and forwarded only when runtime === "acp"; silently dropped otherwise. Schema description updated to document the ACP-only contract.
What did NOT change (scope boundary): resumeSessionId still errors for non-ACP. All ACP spawn paths, the streamTo !== "parent" registry-tracking guard, and subagent spawn behavior are unchanged.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
This PR fixes a bug or regression
Root Cause / Regression History (if applicable)
Regression Test Plan (if applicable)
Coverage level that should have caught this:
Target test or file:
src/agents/tools/sessions-spawn-tool.test.ts
Scenario the test should lock in:
Why this is the smallest reliable guardrail:
Both tests directly exercise the removed error branch and the new silent-drop path with zero infrastructure dependencies.
Existing test that already covers this (if any):
Replaced 'rejects streamTo when runtime is not "acp"' (which asserted the old error behavior).
If no new test is added, why not:
N/A — two tests added.
User-visible / Behavior Changes
Callers that previously received:
{ status: "error", error: "streamTo is only supported for runtime=acp; got runtime=subagent" }
Will now receive a normal:
{ status: "accepted", ... }
No change for callers that do not pass streamTo, or for ACP callers.
Diagram (if applicable)
N/A
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
{ status: "accepted", childSessionKey: "...", runId: "..." }
Actual (before fix)
{ status: "error", error: "streamTo is only supported for runtime=acp; got runtime=subagent" }
Evidence
Human Verification (required)
Verified scenarios:
Edge cases checked:
What you did not verify:
Review Conversations
Compatibility / Migration
Risks and Mitigations
Risk: Callers that intentionally passed streamTo on a subagent spawn expecting an error to signal misuse will no longer receive feedback.
Mitigation:
streamTo is documented as an ACP-only output-routing hint in the schema description; it has no observable effect on subagent spawns. Silent drop is the correct semantic. Callers can inspect the updated schema description to understand the contract.