Conversation
Expose a machine stdio entrypoint in Forge and route it through a real ACP stdio transport so Acepe can launch Forge as an installable provider instead of depending on an unpublished branch. Co-Authored-By: ForgeCode <noreply@forgecode.dev>
- Replace unbounded notification channel with bounded (1024) to apply backpressure when the client stalls - Add per-session model override to prevent concurrent sessions from interfering with each other - Replace From<Error> impl with explicit into_acp_error() per project guidelines - Extract classify_mcp_tool() and convert to free functions, removing the unnecessary ToolOutputConverter struct - Validate MCP server names (length, charset) to prevent injection - Add MAX_BLOB_SIZE (50 MB) guard on base64-decoded resources - Add I/O timeout (5 min) and graceful shutdown drain (5 s) to prevent indefinite hangs - Track cancellation via AtomicBool across loop iterations - Log warnings instead of silently ignoring reload/config errors - Add tests for tool kind mapping, file extraction, and edge cases Co-Authored-By: ForgeCode <noreply@forgecode.dev>
The store ran eagerly on function call, not when the future was awaited. Move it into the async block so the test actually verifies that the caller awaits the returned future. Co-Authored-By: ForgeCode <noreply@forgecode.dev>
Co-Authored-By: ForgeCode <noreply@forgecode.dev>
# Conflicts: # Cargo.lock
…l start, log in machine mode - Policy confirmations, MCP trust and follow-up choices raised during tool execution are forwarded as session/request_permission instead of opening a terminal picker on the protocol pipe (previously hung forever). - Acknowledge ChatResponse::ToolCallStart's notifier; the orchestrator waits on it before executing any tool. - Dispatch 'forge machine stdio' in main before the interactive UI and initialise file logging there; remove the acp_runner shim. - Drop the blanket 300s I/O timeout on the connection; the pipe closing ends it.
…h custom commands - set_config_option maps 'mode' and 'model' onto the existing handlers (newer clients no longer call session/set_mode). - Advertise custom commands via available_commands_update at session start and at each prompt; a '/name args' prompt runs the command with the terminal's semantics.
- Nine built-ins forge can run for a client (compact, commit, commit-preview,
info, tools, usage, workspace-{info,status,sync}), each calling the same
service the terminal calls. Terminal-only and client-owned commands are
deliberately not advertised.
- Publish the command list on the initialize response's _meta as well as the
session update, so a client can populate its menu before the first message.
- Await delivery of a built-in's output so it lands before the turn response.
A built-in can still raise a user question (MCP trust, a policy confirmation). Without the marker the bridge had no session to attribute it to, answered nothing, and /tools hung in a directory with an untrusted .mcp.json.
Awaiting anything after a built-in runs — the connection, a delivery signal, or a timer — leaves the turn without a response until the client sends more input. Queue the message like every other session update.
The terminal lists 45 commands behind ':'; a client had no way to switch agents at all. Advertise one command per agent (forge, muse, sage) that switches the session agent through the same handler as session/set_mode, plus /config and /workspace-init.
- alibaba_token_plan: qwen3.8-max-preview becomes the qwen3.8-max-0902 snapshot, and qwen3.8-flash is added. - Publish the active agent's model list on the initialize response _meta, so a client can show models before a session exists. - Declare the models as a 'model' session config option on session/new and session/load: clients apply a model with session/set_config_option, so SessionModelState alone left the picker unable to choose.
A client only sets a session model when it differs from forge's current one, so an unset session model meant '/info' claimed 'agent default' for a session that was using the configured model.
The hard-coded list had drifted from what the token-plan endpoint serves (qwen3.8-max, not qwen3.8-max-preview or a -0902 snapshot; deepseek-v4-flash is deepseek-v4-flash-0731 there). The endpoint publishes /models, and forge already reads OpenAI-compatible model lists from a URL, so use that.
The shell tool echoes command output through the console writer, which under 'forge machine stdio' was the JSON-RPC pipe: the first shell command in a turn corrupted the stream and the client dropped the session. Move the pipe to a private descriptor and point fd 1 at stderr before the ACP server starts, so nothing in the process can write into the protocol.
|
|
github-actions
Bot
added
type: feature
JDLondon7
marked this pull request as ready for review
September 2, 2026 16:51
|
Action required: PR inactive for 5 days. |
github-actions
Bot
added
the
state: inactive
Contributor
|
Independent review for #2968 at head The stdin exclusion, tool-start acknowledgement and permission forwarding address real historical problems. I do not think this head meets finite beta acceptance yet. Main findings below are static review, not independently reproduced ACP/editor integration failures:
Small optional fix offered below: the private protocol fd created by Validation actually performed: metadata-only offline Cargo check passed; contributor diff whitespace check passed; workspace Full crate tests, debug CLI Scope proposal / maintainer decision: default-off beta; explicit supported OS/client matrix (the issue mentions Windows and Zed); JSON-only startup; honored session cwd and non-persistent MCP config; correct advertised capabilities; tested permission/cancellation/error/session-isolation paths; passing crate/lint/actual CI plus one agreed editor smoke. Is the prior request to build ACP above rather than inside the API still required? Please also split out or justify the unrelated Alibaba model-list change. Durable local evidence: Optional close-on-exec patch and unit regression (not full-workspace verified)diff --git a/crates/forge_main/src/main.rs b/crates/forge_main/src/main.rs
index 150c1a765..9c3405759 100644
--- a/crates/forge_main/src/main.rs
+++ b/crates/forge_main/src/main.rs
@@ -134,15 +134,14 @@ async fn run() -> Result<()> {
// it, so move the pipe to a private descriptor and point fd 1 at
// stderr before anything else runs.
let protocol_out = {
- use std::os::fd::FromRawFd;
- // SAFETY: fd 1 and 2 are open for the life of the process; dup
- // returns a fresh descriptor this File then owns exclusively.
+ use std::os::fd::AsFd;
+
+ let pipe = duplicate_protocol_output(std::io::stdout().as_fd())?;
+ // SAFETY: fd 1 and 2 are open for the life of the process.
unsafe {
- let pipe = libc::dup(1);
- anyhow::ensure!(pipe >= 0, "failed to duplicate stdout");
anyhow::ensure!(libc::dup2(2, 1) >= 0, "failed to redirect stdout to stderr");
- std::fs::File::from_raw_fd(pipe)
}
+ pipe
};
let (api, user_choices) = ForgeAPI::init_acp(cwd, config);
let _guard = tracker::init_tracing(api.environment().log_path())?;
@@ -157,6 +156,12 @@ async fn run() -> Result<()> {
Ok(())
}
+/// Duplicates the protocol output with close-on-exec so tools cannot inherit it.
+#[cfg(unix)]
+fn duplicate_protocol_output(fd: std::os::fd::BorrowedFd<'_>) -> std::io::Result<std::fs::File> {
+ fd.try_clone_to_owned().map(std::fs::File::from)
+}
+
#[cfg(test)]
mod tests {
use forge_main::TopLevelCommand;
@@ -164,6 +169,21 @@ mod tests {
use super::*;
+ #[cfg(unix)]
+ #[test]
+ fn test_protocol_output_is_close_on_exec() {
+ use std::os::fd::{AsFd, AsRawFd};
+
+ let fixture = std::fs::File::open("/dev/null").unwrap();
+ let protocol_out = duplicate_protocol_output(fixture.as_fd()).unwrap();
+
+ // SAFETY: protocol_out owns this descriptor for the duration of the call.
+ let actual = unsafe { libc::fcntl(protocol_out.as_raw_fd(), libc::F_GETFD) };
+
+ let expected = libc::FD_CLOEXEC;
+ assert_eq!(actual, expected);
+ }
+
#[test]
fn test_stdin_detection_logic() {
// This test verifies that the logic for detecting stdin is correctCo-Authored-By: ForgeCode noreply@forgecode.dev |
github-actions
Bot
removed
the
state: inactive
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
5 participants
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.
Summary
Revives the ACP stdio transport from #2858 (closed for inactivity; the maintainer asked for it behind a beta flag — happy to add that) and fixes what surfaced when driving it end-to-end from a real ACP client (T3 Code) rather than a test harness. Every item below was a silent hang or a broken picker in the client; each has a reproduction in the commit message.
forge machine stdiono longer has stdin consumed as a piped prompt before the server starts (the JSON-RPC stream was eaten; server exited silently).ChatResponse::ToolCallStart's notifier: the orchestrator now waits on it before executing any tool, so every tool call hung over ACP.session/request_permissioninstead of opening a terminal picker on the pipe. The connection is single-threaded, so this crosses a channel into the connection task.git statusin a turn used to kill the session.session/set_config_optionimplemented formodeandmodel; current clients use it instead ofsession/set_mode, and the crate default answered "Method not found".compact,commit,commit-preview,config,info,tools,usage,workspace-*, and one per agent for switching) are advertised viaavailable_commands_updateand, because ACP can only advertise per session, also on the initialize response_metaso a client can populate its menu before the first message.SessionModelStateand as amodelsession config option (clients apply a choice through the latter), also on initialize_meta.alibaba_token_planreads its model list from the endpoint's/modelsinstead of a hand-maintained list that had drifted.Deliberately not exposed over ACP: terminal affordances (
:exit,:copy,:edit), and things a client owns in an ACP session (:new,:model,:conversation*).Notes for review
LocalSetinsidespawn_blockingsetup. Worth a look from whoever knows that runtime choice.:skillis not advertised: listing skills lives on the infra (SkillRepository), not on theServicesthe adapter holds.Test plan
@filementions, Supervised approve/decline, stop mid-turn, slash commands, agent switching, model picker.🤖 Generated with Claude Code