feat(acp): add sessions_spawn streamTo parent relay for ACP spawns#34055
feat(acp): add sessions_spawn streamTo parent relay for ACP spawns#34055vincentkoc wants to merge 46 commits intomainfrom
Conversation
Greptile SummaryThis PR adds Changes:
Issues found:
Confidence Score: 3/5
Last reviewed commit: 9726ec5 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9726ec5238
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 763045210a
ℹ️ 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".
| fs.mkdirSync(path.dirname(logPath), { | ||
| recursive: true, | ||
| }); | ||
| const line = `${JSON.stringify(entry)}\n`; | ||
| fs.appendFileSync(logPath, line, { | ||
| encoding: "utf-8", |
There was a problem hiding this comment.
The relay logs every assistant delta via writeLogLine, which currently does fs.mkdirSync and fs.appendFileSync for each event. In streamTo: "parent" runs with long outputs, this introduces synchronous disk I/O on the event loop for nearly every token and can slow both gateway responsiveness and relay updates. Because logPath is populated for normal ACP sessions, this path is exercised in production; use async/buffered writes (and one-time directory creation) to prevent per-delta blocking.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 3243053. Relay logging no longer uses sync fs calls in the assistant-delta path. It now buffers JSONL lines in-memory and flushes with async mkdir/appendFile via a serialized write chain (with one-time dir creation), so token streaming does not block the event loop with per-delta sync I/O.
| emit( | ||
| `Started ${relayLabel} session ${params.childSessionKey}. Streaming progress updates to parent session.`, | ||
| `${contextPrefix}:start`, | ||
| ); |
There was a problem hiding this comment.
The relay emits Started ... Streaming progress updates immediately on registration, but registration happens before spawnAcpDirect calls callGateway("agent"). If dispatch then fails (for example gateway timeout/unavailable), spawnAcpDirect returns an error and disposes the relay, yet this start event is still queued and later injected into the parent prompt as trusted runtime metadata even though no child run started. Emitting the start notice only after dispatch success (or adding a compensating failure event) avoids this false state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 3243053. We now register the relay with emitStartNotice: false before dispatch, and call notifyStarted() only after callGateway("agent") succeeds. On dispatch failure, we dispose the relay without emitting the start notice. See src/agents/acp-spawn.ts and src/agents/acp-spawn-parent-stream.ts.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61ec2233f9
ℹ️ 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".
| toTrimmedString((data as { delta?: unknown } | undefined)?.delta) ?? | ||
| toTrimmedString((data as { text?: unknown } | undefined)?.text); |
There was a problem hiding this comment.
In startAcpSpawnParentStreamRelay, assistant chunks are normalized with toTrimmedString before buffering, which strips leading/trailing whitespace from every delta. ACP streaming deltas commonly include leading spaces/newlines (for example tokenized chunks like " world"), so this causes words and paragraph boundaries to collapse in parent progress updates (streamTo: "parent"), making the relay summaries inaccurate exactly when direct child delivery is disabled. Keep raw delta text for buffering and only trim for emptiness checks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 96771db. Assistant delta buffering now preserves raw chunk whitespace and only trims for emptiness checks, so token boundaries like leading spaces/newlines are not dropped before relay buffering. Added regression coverage in acp-spawn-parent-stream.test.ts (preserves delta whitespace boundaries in progress relays).
|
This is merged in #34310, needed to address codex review issues on my own branch |
3 participants
Summary
sessions_spawnruns are non-blocking but gave poor parent-session visibility during long or blocked initial turns.sessions_spawn.streamTo: "parent"support forruntime: "acp".streamTo: "parent"is used, direct child delivery for that initial run is disabled.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
sessions_spawnnow acceptsstreamTo: "parent"for ACP runtime.runtime: "acp"+streamTo: "parent", requester session receives progress/no-output/completion/failure system-event updates for the initial child run.deliver: falsein that mode to avoid direct child-delivery duplication.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
sessions_spawnwith{ runtime: "acp", streamTo: "parent", task: "..." }from an active requester session.Expected
Actual
Evidence
Human Verification (required)
What you personally verified (not just CI), and how:
deliver=false)streamTo: "parent"rejected when requester session context is missingCompatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
streamTo: "parent"(default behavior unchanged)src/agents/acp-spawn-parent-stream.tssrc/agents/acp-spawn.tssrc/agents/tools/sessions-spawn-tool.tsstreamTo: "parent"Risks and Mitigations
streamTo: "parent"could silently fail.