chore: extract ACP stdio transport pre-patch#3310
Open
ssfdust wants to merge 2 commits into
Open
Conversation
|
|
3f70ae9 to
ffde445
Compare
Extracts independently-mergable fixes from the ongoing ACP work (tailcallhq#2858) that can land on main immediately. Three classes of fix: 1. stdin pre-read encapsulation (cli.rs, main.rs) Add Cli::uses_stdin() method that centralizes the decision of which subcommands own stdin. Subcommands opt in via uses_stdin() rather than having the startup pipeline guess from a hardcoded exclusion list. This makes it easy to add new stdin-consuming subcommands (e.g. an ACP stdio server) without touching the startup logic. 2. Tool output leaking to stdout (context.rs, tool_executor.rs) Shell tool execution streamed raw output to io::stdout(), corrupting the ACP JSON-RPC stream. Add silent: bool field on ToolCallContext (the correct home for per-invocation runtime flags). Default false; ACP sets .silent(true) to route output to io::sink() instead. 3. Stderr vs stdout discipline (main.rs, sandbox.rs) Panic hook and worktree status messages used println! which writes to stdout. Changed to eprintln! (stderr) to avoid contaminating structured transport channels. Signed-off-by: ssfdust <ssfdust@gmail.com> Co-authored-by: ForgeCode <noreply@forgecode.dev> Co-authored-by: DeepSeek <deepseek-ai@deepseek.com>
c24650f to
b355433
Compare
Add a `tool_silent` flag to `ChatRequest` that propagates through the entire tool execution chain down to the command executor. When `true`, shell tool output is suppressed on stdout to protect JSON-RPC transports. - chat_request: add `tool_silent: bool` field (default false) - orch: add `tool_silent` field, propagate to ToolCallContext - app: forward ChatRequest.tool_silent to Orchestrator - agent_executor: inherit silent flag from parent ToolCallContext - executor: clean up TODO(acp), add inline comment Signed-off-by: ssfdust <ssfdust@gmail.com> Co-Authored-By: ForgeCode <noreply@forgecode.dev> Co-Authored-By: DeepSeek <contact@deepseek.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ACP stdio transport pre-patch
Extracts independently-mergable fixes from the ongoing ACP work in #2858.
Background
When spawning
forge machine stdioas an ACP subprocess, stdin is a pipethat remains open for bidirectional JSON-RPC communication. This patch
resolves three classes of bug discovered during ACP integration testing.
Changes
stdin pre-read encapsulation (
cli.rs,main.rs)Added
Cli::uses_stdin()method that centralizes the decision ofwhich subcommands own stdin. Subcommands opt in via
uses_stdin()rather than having the startup pipeline guess from a hardcoded
exclusion list. This makes it easy to add new stdin-consuming
subcommands (e.g. an ACP stdio server) without touching the
startup logic.
tool output leaking to stdout (
context.rs,tool_executor.rs,chat_request.rs,orch.rs,app.rs,agent_executor.rs,executor.rs)Shell tool execution always streamed raw output to
io::stdout(),corrupting the ACP JSON-RPC stream. Fixed in two layers:
Lower layer (pre-patch): Added
silent: boolonToolCallContextand an
if silent { io::sink() }guard in the command executor.Default
false; call sites opt in via.silent(true)to routeoutput to
io::sink()instead of stdout.Upper layer (propagation): Added
tool_silent: boolonChatRequest(the per-invocation DTO) and threaded it through the entire call
stack —
ForgeApp::chat()forwards it toOrchestrator, whichsets it on
ToolCallContext. Agent-as-tool recursion inherits theparent's flag automatically. The propagation chain:
stderr vs stdout discipline (
main.rs,sandbox.rs)Panic hook and worktree status messages used
println!, whichwrites to stdout and contaminates structured transport. Changed
to
eprintln!(stderr).