Fix sessions_spawn for subagent runtime with ACP-only fields#56342
Fix sessions_spawn for subagent runtime with ACP-only fields#56342shenkq97 wants to merge 1 commit into
Conversation
Greptile SummaryThis PR changes
Confidence Score: 5/5Safe to merge — no P0/P1 issues; the fix is correctly scoped and well-tested. All remaining findings are P2: a minor schema description mismatch and a slightly redundant variable pattern that doesn't affect correctness. The core behavioral change is sound, ACP behavior is untouched, and the new tests provide adequate coverage for the fixed subagent path. No files require special attention beyond the minor schema description update in sessions-spawn-tool.ts.
|
| Filename | Overview |
|---|---|
| src/agents/tools/sessions-spawn-tool.ts | Removes the hard-rejection guard for ACP-only fields (streamTo, resumeSessionId) on the subagent path and replaces it with a silent-strip via effectiveStreamTo / effectiveResumeSessionId. The actual stripping is implicit — neither field is forwarded to spawnSubagentDirect — so the effective* variables only matter inside the ACP branch where they always equal the originals. Minor: the resumeSessionId schema description still says 'Requires runtime=acp' rather than 'ignored for other runtimes'. |
| src/agents/tools/sessions-spawn-tool.test.ts | Two previously rejection-asserting tests are updated to assert successful subagent spawns when ACP-only fields are present. The new assertions correctly verify that spawnSubagentDirectMock is called and spawnAcpDirectMock is not. ACP-path tests are untouched. |
Comments Outside Diff (1)
-
src/agents/tools/sessions-spawn-tool.ts, line 28-33 (link)Schema description overstates the constraint after this change
The
resumeSessionIddescription still saysRequires runtime="acp", which implies a hard rejection. After this PR the actual behavior is "silently ignored when runtime ≠ acp." A schema-following model that reads the description literally will still try to pairresumeSessionIdwithruntime="acp", which is the correct intent — but if it doesn't, the field is now quietly dropped rather than rejected. The description no longer accurately describes the enforcement semantics.Prompt To Fix With AI
This is a comment left during a code review. Path: src/agents/tools/sessions-spawn-tool.ts Line: 28-33 Comment: **Schema description overstates the constraint after this change** The `resumeSessionId` description still says `Requires runtime="acp"`, which implies a hard rejection. After this PR the actual behavior is "silently ignored when runtime ≠ acp." A schema-following model that reads the description literally will still try to pair `resumeSessionId` with `runtime="acp"`, which is the correct intent — but if it doesn't, the field is now quietly dropped rather than rejected. The description no longer accurately describes the enforcement semantics. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/agents/tools/sessions-spawn-tool.ts
Line: 28-33
Comment:
**Schema description overstates the constraint after this change**
The `resumeSessionId` description still says `Requires runtime="acp"`, which implies a hard rejection. After this PR the actual behavior is "silently ignored when runtime ≠ acp." A schema-following model that reads the description literally will still try to pair `resumeSessionId` with `runtime="acp"`, which is the correct intent — but if it doesn't, the field is now quietly dropped rather than rejected. The description no longer accurately describes the enforcement semantics.
```suggestion
resumeSessionId: Type.Optional(
Type.String({
description:
'Resume an existing agent session by its ID (e.g. a Codex session UUID from ~/.codex/sessions/). Only meaningful with runtime="acp"; ignored for other runtimes. The agent replays conversation history via session/load instead of starting fresh.',
}),
),
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/tools/sessions-spawn-tool.ts
Line: 133-134
Comment:
**`effective*` variables are always equal to the originals when consumed**
Both `effectiveResumeSessionId` and `effectiveStreamTo` are only ever read inside the `if (runtime === "acp")` block (lines 149 and 154). Because that guard guarantees `runtime === "acp"`, the ternary expression always resolves to the original value there — the `undefined` branch is dead code at the use site. The actual stripping happens implicitly because the subagent path never references those fields at all.
This isn't a bug, but it may cause future maintainers to believe the variables "do the stripping" when in fact they're unnecessary. A short inline comment or simply using the original variables directly inside the ACP block (relying on the existing guard) would be cleaner:
```ts
// ACP-only fields are not forwarded to the subagent path
// (spawnSubagentDirect never receives them).
```
or just continue using `resumeSessionId` / `streamTo` directly inside the `if (runtime === "acp")` block, which is already protected by the runtime guard.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Fix sessions_spawn for subagent runtime ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const effectiveResumeSessionId = runtime === "acp" ? resumeSessionId : undefined; | ||
| const effectiveStreamTo = runtime === "acp" ? streamTo : undefined; |
There was a problem hiding this comment.
effective* variables are always equal to the originals when consumed
Both effectiveResumeSessionId and effectiveStreamTo are only ever read inside the if (runtime === "acp") block (lines 149 and 154). Because that guard guarantees runtime === "acp", the ternary expression always resolves to the original value there — the undefined branch is dead code at the use site. The actual stripping happens implicitly because the subagent path never references those fields at all.
This isn't a bug, but it may cause future maintainers to believe the variables "do the stripping" when in fact they're unnecessary. A short inline comment or simply using the original variables directly inside the ACP block (relying on the existing guard) would be cleaner:
// ACP-only fields are not forwarded to the subagent path
// (spawnSubagentDirect never receives them).or just continue using resumeSessionId / streamTo directly inside the if (runtime === "acp") block, which is already protected by the runtime guard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/tools/sessions-spawn-tool.ts
Line: 133-134
Comment:
**`effective*` variables are always equal to the originals when consumed**
Both `effectiveResumeSessionId` and `effectiveStreamTo` are only ever read inside the `if (runtime === "acp")` block (lines 149 and 154). Because that guard guarantees `runtime === "acp"`, the ternary expression always resolves to the original value there — the `undefined` branch is dead code at the use site. The actual stripping happens implicitly because the subagent path never references those fields at all.
This isn't a bug, but it may cause future maintainers to believe the variables "do the stripping" when in fact they're unnecessary. A short inline comment or simply using the original variables directly inside the ACP block (relying on the existing guard) would be cleaner:
```ts
// ACP-only fields are not forwarded to the subagent path
// (spawnSubagentDirect never receives them).
```
or just continue using `resumeSessionId` / `streamTo` directly inside the `if (runtime === "acp")` block, which is already protected by the runtime guard.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
martingarramon
left a comment
martingarramon
left a comment
There was a problem hiding this comment.
Clean fix — the forgiving-strip approach is the right call for schema-following models that emit all visible fields.
One thing I noticed: effectiveResumeSessionId and effectiveStreamTo are only read inside the if (runtime === "acp") branch, where they always equal the originals. So the variables serve as intent documentation rather than functional guards — you'd get the same behavior by just removing the two error-return blocks. Not a blocker, just an observation.
Could a debug-level log when stripping ACP-only fields be useful? Something like log.debug("Ignoring ACP-only field %s for runtime=%s", field, runtime) — would help operators debug cases where models emit unexpected parameters, without adding noise for users.
|
Closing this as duplicate or superseded after Codex automated review. Close PR #56342 as superseded by open PR #68397. The bug is still present on current main, so this is not implemented-on-main: Best possible solution: Close #56342 as the older superseded PR, keep review and landing focused on #68397 or an equivalent maintainer patch that silently drops What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex Review notes: model gpt-5.5, reasoning high; reviewed against 7e376e5aba32. |
2 participants
Summary
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
Regression Test Plan (if applicable)
User-visible / Behavior Changes
Subagent spawns become more robust when models include ACP-only fields that are irrelevant to runtime="subagent".
Diagram (if applicable)
Before:
LLM tool call -> sessions_spawn(runtime=subagent, streamTo=parent) -> guard rejects -> no spawn
After:
LLM tool call -> sessions_spawn(runtime=subagent, streamTo=parent) -> ACP-only fields ignored -> spawnSubagentDirect(...) -> success
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Human Verification (required)
Review Conversations
Compatibility / Migration
Risks and Mitigations