@sigitcodereview / sigit / commits / e9cbf50

Stop a tool command from wedging the ACP session

In ACP mode sigit's stdin is the JSON-RPC pipe from the editor. run_command piped stdout and stderr but left stdin inherited, so every shell command it spawned held that pipe. A command that reads stdin, which for the release flow means a git commit reaching a signing passphrase prompt, blocks on it and reads the client's next request out of the pipe. The request is then gone: the agent never sees it, the editor waits for a reply that cannot come, and the session stays dead well past the 120-second command timeout. That is getsigit/sigit-si#54. spawn_shell now sets stdin to null, as do the git helpers behind the co-author trailer and the hook runner, which spawned the same way. A command that wants input gets EOF and reports an error instead of hanging. Two more things in the same function that read as hangs: The wait loop only read the pipes after the command exited, so anything that outgrew the pipe buffer blocked writing while we blocked waiting, and the 120-second timeout was the only way out. A 400 KB run of echo reproduces it. Output is now drained on threads while the command runs, the shape hooks.rs already uses. The timeout killed the shell and nothing else. sh -c may fork rather than exec, so whatever it started kept running with the pipe write ends open, and the shell was never reaped. The child now leads its own process group on Unix and the timeout kills the group. tests/acp_tool_stdin.rs drives the real binary over ACP and fails on the old code by timing out exactly the way the editor did.

paydii committed Sep 5, 2026 at 01:22 UTC e9cbf50e7c040f023bda31f78850e68168f3880b

5 files changed +401 -16

.agents/AGENTS.md

+7 -1

@@ -126,7 +126,13 @@ feeds results back. Neither the loop nor ACP/TUI surfaces depend on a concrete b

126 spec list (`all_tools`) and the execute `match` (`execute_tool`). `run_command` also enforces

127 commit attribution: when a command creates a new commit that lacks the

128 `Co-Authored-By: siGit Code` trailer (`COMMIT_CO_AUTHOR_TRAILER`), it amends the trailer in —

129 - unless the commit already exists on a remote, which is never rewritten. Also owns the `task`

129 + unless the commit already exists on a remote, which is never rewritten. Every child process it

130 + spawns (`spawn_shell`, the `git` helpers, and `hooks.rs`) sets `stdin` to null and never

131 + inherits it: in ACP mode sigit's stdin is the JSON-RPC pipe from the editor, so a command that

132 + reads stdin both blocks forever and eats the client's next request, wedging the session past

133 + any timeout. On Unix the shell also leads its own process group so the timeout kills the whole

134 + tree, and stdout/stderr are drained on threads while the command runs — waiting first and

135 + reading after deadlocks as soon as the output outgrows the pipe buffer. Also owns the `task`

136 tool: a nested agent loop in a fresh conversation, offered only when `subagent_available()`

137 (a subagent factory is registered — see `register_subagent_factory_for` in `main.rs`; on-device

138 registers a `None`-returning factory since onde has a single shared history). A subagent's

CHANGELOG.md

+21

@@ -1,5 +1,26 @@

1 # Changelog

3 +## Unreleased

4 +

5 +### Fixed

6 +

7 +- **A shell command can no longer wedge the editor session.** Tool commands

8 + inherited siGit's own stdin, which in ACP mode is the JSON-RPC pipe from the

9 + editor. A command that read stdin — a signing passphrase prompt, a pager, a

10 + `git` credential ask — blocked on it and consumed the client's next request

11 + out of the pipe, so nothing typed afterwards ever reached the agent and the

12 + session stayed dead past the command timeout. Commands, hooks, and the

13 + co-author `git` helpers now run with stdin closed, so a command that wants

14 + input gets EOF and reports an error instead

15 +- **A command that prints a lot no longer stalls for two minutes.**

16 + `run_command` waited for the command to exit before reading its output, so

17 + anything past the pipe buffer (a build, a verbose test run) blocked writing

18 + while siGit blocked waiting, until the 120-second timeout killed it. Output

19 + is now drained while the command runs

20 +- **The command timeout now stops the whole command.** It killed only the

21 + shell, leaving anything that shell had started running and unreaped; it now

22 + kills the process group

23 +

24 ## 1.5.6

26 ### What changed

src/hooks.rs

@@ -198,6 +198,9 @@ fn run_hook(cmd: &str, context: &HookContext, cwd: &Path, timeout: Duration) {

198 .arg(&substituted)

199 .current_dir(cwd)

200 .envs(context.env_vars())

201 + // Never the agent's stdin: in ACP mode that is the JSON-RPC pipe from

202 + // the editor, and a hook reading it swallows the client's next request.

203 + .stdin(Stdio::null())

204 .stdout(Stdio::piped())

205 .stderr(Stdio::piped());

206 // Put the hook in its own process group so a timeout kill can take out the

src/tools.rs

+135 -15

@@ -1941,22 +1941,57 @@ fn exec_delete_file(arguments: &str) -> String {

1941

1942 const COMMAND_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(120);

1943 const COMMAND_OUTPUT_LIMIT: usize = 50_000;

1944 +/// How long to keep waiting for the output drains once the command itself has

1945 +/// exited. Only a process that outlived the shell and still holds a pipe can

1946 +/// use this up, so it is short.

1947 +const OUTPUT_GRACE: std::time::Duration = std::time::Duration::from_secs(2);

1948

1949 /// Spawn `command_str` through the platform shell with piped stdout/stderr.

1950 /// Shared by the foreground and background paths of `run_command`.

1951 +///

1952 +/// stdin is `/dev/null`, never inherited. In ACP mode sigit's stdin is the

1953 +/// JSON-RPC pipe from the editor, and a child holding that fd can read the

1954 +/// client's next request out of it: the agent never sees the request, the

1955 +/// editor waits forever for a reply, and the session is wedged past any

1956 +/// timeout this function's callers apply. Null also means a command that wants

1957 +/// input gets EOF and fails instead of blocking on input nobody can type.

1958 +///

1959 +/// On Unix the child also leads its own process group, so a timeout can kill

1960 +/// the whole tree. `sh -c` may fork rather than exec (dash does), and a

1961 +/// surviving grandchild keeps the pipe write ends open.

1962 fn spawn_shell(command_str: &str, cwd_path: &Path) -> std::io::Result<std::process::Child> {

1963 #[cfg(unix)]

1964 let (shell, flag) = ("sh", "-c");

1965 #[cfg(windows)]

1966 let (shell, flag) = ("cmd", "/C");

1967

1953 - Command::new(shell)

1968 + let mut command = Command::new(shell);

1969 + command

1970 .arg(flag)

1971 .arg(command_str)

1972 .current_dir(cwd_path)

1973 + .stdin(std::process::Stdio::null())

1974 .stdout(std::process::Stdio::piped())

1958 - .stderr(std::process::Stdio::piped())

1959 - .spawn()

1975 + .stderr(std::process::Stdio::piped());

1976 + #[cfg(unix)]

1977 + std::os::unix::process::CommandExt::process_group(&mut command, 0);

1978 + command.spawn()

1979 +}

1980 +

1981 +/// SIGKILL a shell spawned by [`spawn_shell`] and everything it started, then

1982 +/// reap it so the timeout path doesn't leave a zombie behind.

1983 +fn kill_shell_tree(child: &mut std::process::Child) {

1984 + #[cfg(unix)]

1985 + {

1986 + // Negative pid is the process group; the child is its leader. Fall

1987 + // back to the child alone if the group is already gone.

1988 + if unsafe { libc::kill(-(child.id() as i32), libc::SIGKILL) } != 0 {

1989 + let _ = child.kill();

1990 + }

1991 + }

1992 + #[cfg(windows)]

1993 + let _ = child.kill();

1994 + let _ = child.wait();

1995 }

1996

1997 /// Trailer identifying siGit Code as the co-author of commits it creates.

@@ -1970,10 +2005,15 @@ fn spawn_shell(command_str: &str, cwd_path: &Path) -> std::io::Result<std::proce

2005 pub const COMMIT_CO_AUTHOR_TRAILER: &str = "Co-Authored-By: siGit Code <noreply@sigit.si>";

2006

2007 /// Run `git <args>` in `cwd`, returning trimmed stdout on success.

2008 +///

2009 +/// stdin is null for the same reason as [`spawn_shell`]: git can ask for input

2010 +/// (a credential, a signing passphrase), and inheriting the ACP pipe would let

2011 +/// it read the editor's next request instead.

2012 fn git_stdout(cwd: &Path, args: &[&str]) -> Option<String> {

2013 let output = Command::new("git")

2014 .args(args)

2015 .current_dir(cwd)

2016 + .stdin(std::process::Stdio::null())

2017 .output()

2018 .ok()?;

2019 output

@@ -2008,6 +2048,7 @@ fn ensure_commit_co_author(cwd: &Path) -> Option<String> {

2048 let amend = Command::new("git")

2049 .args(["commit", "--amend", "--no-edit", "--trailer"])

2050 .arg(COMMIT_CO_AUTHOR_TRAILER)

2051 + .stdin(std::process::Stdio::null())

2052 .current_dir(cwd)

2053 .output()

2054 .ok()?;

@@ -2086,13 +2127,33 @@ fn exec_run_command(arguments: &str) -> String {

2127 Err(err) => return format!("Error: failed to spawn command: {err}"),

2128 };

2129

2130 + // Drain both pipes on their own threads. Waiting first and reading after

2131 + // deadlocks the moment a command writes more than the pipe buffer holds

2132 + // (64 KiB on Linux, less on macOS): the child blocks on the full pipe, we

2133 + // block on the child, and the only way out is the timeout. A build or a

2134 + // verbose test run crosses that on its own.

2135 + let (chunks_tx, chunks_rx) = std::sync::mpsc::channel::<(u8, Vec<u8>)>();

2136 + let drain = |tag: u8, pipe: Option<Box<dyn std::io::Read + Send>>| {

2137 + let tx = chunks_tx.clone();

2138 + std::thread::spawn(move || {

2139 + let mut buf = Vec::new();

2140 + if let Some(mut pipe) = pipe {

2141 + let _ = pipe.read_to_end(&mut buf);

2142 + }

2143 + let _ = tx.send((tag, buf));

2144 + });

2145 + };

2146 + drain(0, child.stdout.take().map(|pipe| Box::new(pipe) as _));

2147 + drain(1, child.stderr.take().map(|pipe| Box::new(pipe) as _));

2148 + drop(chunks_tx);

2149 +

2150 let start = std::time::Instant::now();

2090 - loop {

2151 + let status = loop {

2152 match child.try_wait() {

2092 - Ok(Some(_status)) => break,

2153 + Ok(Some(status)) => break status,

2154 Ok(None) => {

2155 if start.elapsed() >= COMMAND_TIMEOUT {

2095 - let _ = child.kill();

2156 + kill_shell_tree(&mut child);

2157 return format!(

2158 "Error: command timed out after {} seconds and was killed.",

2159 COMMAND_TIMEOUT.as_secs()

@@ -2102,17 +2163,27 @@ fn exec_run_command(arguments: &str) -> String {

2163 }

2164 Err(err) => return format!("Error: failed to wait on command: {err}"),

2165 }

2105 - }

2106 -

2107 - let output = match child.wait_with_output() {

2108 - Ok(o) => o,

2109 - Err(err) => return format!("Error: failed to read command output: {err}"),

2166 };

2167

2112 - let exit_code = output.status.code().unwrap_or(-1);

2168 + // Collect what the drains read, bounded. The shell has exited, but a

2169 + // grandchild that outlived it can still hold a pipe open, and joining

2170 + // those threads would wait on it forever.

2171 + let mut stdout_bytes = Vec::new();

2172 + let mut stderr_bytes = Vec::new();

2173 + let deadline = std::time::Instant::now() + OUTPUT_GRACE;

2174 + for _ in 0..2 {

2175 + let remaining = deadline.saturating_duration_since(std::time::Instant::now());

2176 + match chunks_rx.recv_timeout(remaining) {

2177 + Ok((0, bytes)) => stdout_bytes = bytes,

2178 + Ok((_, bytes)) => stderr_bytes = bytes,

2179 + Err(_) => break,

2180 + }

2181 + }

2182 +

2183 + let exit_code = status.code().unwrap_or(-1);

2184 let mut combined = String::new();

2114 - combined.push_str(&String::from_utf8_lossy(&output.stdout));

2115 - combined.push_str(&String::from_utf8_lossy(&output.stderr));

2185 + combined.push_str(&String::from_utf8_lossy(&stdout_bytes));

2186 + combined.push_str(&String::from_utf8_lossy(&stderr_bytes));

2187

2188 // A new commit appeared under this command: make sure it carries the

2189 // siGit co-author trailer (see `ensure_commit_co_author`).

@@ -2136,7 +2207,7 @@ fn exec_run_command(arguments: &str) -> String {

2207 combined

2208 };

2209

2139 - if output.status.success() {

2210 + if status.success() {

2211 if truncated.is_empty() {

2212 format!("Command succeeded (exit code {exit_code}) with no output.")

2213 } else {

@@ -3085,6 +3156,55 @@ mod tests {

3156 );

3157 }

3158

3159 + /// The wait loop used to read the pipes only after the child exited, so a

3160 + /// command that outgrew the pipe buffer blocked on the write, we blocked on

3161 + /// the child, and the whole thing sat there until the 120-second timeout.

3162 + #[test]

3163 + fn run_command_survives_more_output_than_a_pipe_buffer_holds() {

3164 + let dir = std::env::temp_dir();

3165 + // ~400 KB, well past any platform's pipe buffer. It also crosses

3166 + // COMMAND_OUTPUT_LIMIT, so the result is truncated; the point of the

3167 + // test is that the command finishes at all.

3168 + let line = "x".repeat(199);

3169 + let command = format!("for i in $(seq 1 2000); do echo {line}; done");

3170 + let args = json!({"command": command, "cwd": dir.to_str().unwrap()}).to_string();

3171 +

3172 + let started = std::time::Instant::now();

3173 + let result = exec_run_command(&args);

3174 +

3175 + assert!(

3176 + started.elapsed() < std::time::Duration::from_secs(30),

3177 + "run_command took {:?}; it deadlocked on the pipe",

3178 + started.elapsed()

3179 + );

3180 + assert!(

3181 + result.contains("Exit code 0"),

3182 + "unexpected result: {result}"

3183 + );

3184 + assert!(

3185 + result.contains("output truncated"),

3186 + "expected the 50000-byte cap to bite: {result}"

3187 + );

3188 + }

3189 +

3190 + /// A command that wants input gets EOF instead of blocking forever on a

3191 + /// pipe nobody is going to write to (in ACP mode, the editor's own).

3192 + #[test]

3193 + fn run_command_gives_a_command_that_reads_stdin_an_empty_one() {

3194 + let dir = std::env::temp_dir();

3195 + let args = json!({"command": "cat", "cwd": dir.to_str().unwrap()}).to_string();

3196 +

3197 + let started = std::time::Instant::now();

3198 + let result = exec_run_command(&args);

3199 +

3200 + assert!(

3201 + started.elapsed() < std::time::Duration::from_secs(30),

3202 + "run_command blocked on stdin for {:?}",

3203 + started.elapsed()

3204 + );

3205 + assert!(result.contains("no output"), "unexpected result: {result}");

3206 + }

3207 +

3208 #[test]

3209 fn run_command_appends_co_author_trailer_to_new_commits() {

3210 let dir = init_test_repo("coauthor_append");

tests/acp_tool_stdin.rs new

+235

@@ -0,0 +1,235 @@

1 +//! Regression: a tool's child process must never share the agent's stdin.

2 +//!

3 +//! In ACP mode stdin is the JSON-RPC pipe from the editor. `run_command` spawns

4 +//! through the platform shell, and a child that inherits that pipe can read the

5 +//! client's next request out of it. The request is then gone: the agent never

6 +//! sees it, the editor waits for a reply that cannot come, and the session is

7 +//! wedged for good — the symptom filed as "acp: model hangs".

8 +

9 +use std::collections::VecDeque;

10 +use std::io::{BufRead, BufReader, Read, Write};

11 +use std::net::TcpListener;

12 +use std::process::{Child, ChildStdin, Command, Stdio};

13 +use std::sync::Mutex;

14 +use std::sync::mpsc::{Receiver, channel};

15 +use std::time::{Duration, Instant};

16 +

17 +use serde_json::{Value, json};

18 +

19 +const TIMEOUT: Duration = Duration::from_secs(30);

20 +

21 +fn sse_body(chunks: &[Value]) -> String {

22 + let mut body = String::new();

23 + for chunk in chunks {

24 + body.push_str(&format!("data: {chunk}\n\n"));

25 + }

26 + body.push_str("data: [DONE]\n\n");

27 + body

28 +}

29 +

30 +fn sse_tool_call(id: &str, name: &str, arguments: &str) -> String {

31 + sse_body(&[json!({

32 + "choices": [{"delta": {"tool_calls": [{

33 + "index": 0,

34 + "id": id,

35 + "function": {"name": name, "arguments": arguments},

36 + }]}}]

37 + })])

38 +}

39 +

40 +fn sse_text(text: &str) -> String {

41 + sse_body(&[json!({"choices": [{"delta": {"content": text}}]})])

42 +}

43 +

44 +struct FakeEndpoint {

45 + port: u16,

46 +}

47 +

48 +fn start_fake_endpoint(responses: Vec<String>) -> FakeEndpoint {

49 + let listener = TcpListener::bind("127.0.0.1:0").expect("bind fake endpoint");

50 + let port = listener.local_addr().unwrap().port();

51 + let queue = Mutex::new(VecDeque::from(responses));

52 +

53 + std::thread::spawn(move || {

54 + for stream in listener.incoming() {

55 + let Ok(mut stream) = stream else { continue };

56 + let mut reader = BufReader::new(match stream.try_clone() {

57 + Ok(clone) => clone,

58 + Err(_) => continue,

59 + });

60 + let mut content_length = 0usize;

61 + loop {

62 + let mut line = String::new();

63 + if reader.read_line(&mut line).unwrap_or(0) == 0 {

64 + break;

65 + }

66 + let line = line.trim();

67 + if line.is_empty() {

68 + break;

69 + }

70 + if let Some(length) = line.to_ascii_lowercase().strip_prefix("content-length:") {

71 + content_length = length.trim().parse().unwrap_or(0);

72 + }

73 + }

74 + let mut body = vec![0u8; content_length];

75 + if reader.read_exact(&mut body).is_err() {

76 + continue;

77 + }

78 + let payload = queue

79 + .lock()

80 + .unwrap()

81 + .pop_front()

82 + .unwrap_or_else(|| "data: [DONE]\n\n".to_string());

83 + let response = format!(

84 + "HTTP/1.1 200 OK\r\ncontent-type: text/event-stream\r\n\

85 + content-length: {}\r\nconnection: close\r\n\r\n{}",

86 + payload.len(),

87 + payload

88 + );

89 + let _ = stream.write_all(response.as_bytes());

90 + let _ = stream.flush();

91 + }

92 + });

93 +

94 + FakeEndpoint { port }

95 +}

96 +

97 +struct AgentUnderTest {

98 + child: Child,

99 + stdin: ChildStdin,

100 + incoming: Receiver<Value>,

101 + next_id: u64,

102 +}

103 +

104 +fn spawn_agent(port: u16, config_dir: &std::path::Path) -> AgentUnderTest {

105 + let mut child = Command::new(env!("CARGO_BIN_EXE_sigit"))

106 + .env("OPENAI_BASE_URL", format!("http://127.0.0.1:{port}"))

107 + .env("OPENAI_API_KEY", "test-key")

108 + .env("SIGIT_MODEL", "scripted-model")

109 + .env("SIGIT_CONFIG_DIR", config_dir)

110 + .env("SIGIT_MCP", "off")

111 + .env("SIGIT_PERMISSIONS", "allow")

112 + .env_remove("SIGIT_LOCAL_INFERENCE")

113 + .stdin(Stdio::piped())

114 + .stdout(Stdio::piped())

115 + .stderr(Stdio::null())

116 + .spawn()

117 + .expect("spawn sigit in ACP mode");

118 +

119 + let stdout = child.stdout.take().unwrap();

120 + let (message_tx, incoming) = channel();

121 + std::thread::spawn(move || {

122 + for line in BufReader::new(stdout).lines() {

123 + let Ok(line) = line else { break };

124 + if let Ok(message) = serde_json::from_str::<Value>(&line)

125 + && message_tx.send(message).is_err()

126 + {

127 + break;

128 + }

129 + }

130 + });

131 +

132 + let stdin = child.stdin.take().unwrap();

133 + AgentUnderTest {

134 + child,

135 + stdin,

136 + incoming,

137 + next_id: 0,

138 + }

139 +}

140 +

141 +impl AgentUnderTest {

142 + fn request(&mut self, method: &str, params: Value) -> u64 {

143 + self.next_id += 1;

144 + let id = self.next_id;

145 + let mut line =

146 + json!({"jsonrpc": "2.0", "id": id, "method": method, "params": params}).to_string();

147 + line.push('\n');

148 + self.stdin.write_all(line.as_bytes()).expect("write stdin");

149 + self.stdin.flush().expect("flush stdin");

150 + id

151 + }

152 +

153 + fn wait_for_response(&mut self, id: u64) -> Value {

154 + let deadline = Instant::now() + TIMEOUT;

155 + loop {

156 + let remaining = deadline.saturating_duration_since(Instant::now());

157 + let Ok(message) = self.incoming.recv_timeout(remaining) else {

158 + panic!("timed out waiting for the response to request {id}");

159 + };

160 + if message["id"] == id && message.get("method").is_none() {

161 + assert!(

162 + message.get("error").is_none(),

163 + "request {id} failed: {message}"

164 + );

165 + return message;

166 + }

167 + }

168 + }

169 +}

170 +

171 +impl Drop for AgentUnderTest {

172 + fn drop(&mut self) {

173 + let _ = self.child.kill();

174 + let _ = self.child.wait();

175 + }

176 +}

177 +

178 +#[test]

179 +fn a_command_that_reads_stdin_cannot_eat_the_clients_next_request() {

180 + let scratch = std::env::temp_dir().join(format!("sigit_acp_stdin_{}", std::process::id()));

181 + let config_dir = scratch.join("config");

182 + let work = scratch.join("work");

183 + std::fs::create_dir_all(&config_dir).unwrap();

184 + std::fs::create_dir_all(&work).unwrap();

185 + let stolen = work.join("stolen.txt");

186 +

187 + // A command that drains stdin to a file. With stdin inherited it reads the

188 + // JSON-RPC pipe; with stdin closed it gets EOF and exits immediately.

189 + let command = format!("cat > {}", stolen.display());

190 + let endpoint = start_fake_endpoint(vec![

191 + sse_tool_call(

192 + "call_1",

193 + "run_command",

194 + &json!({"command": command, "cwd": work.to_str().unwrap()}).to_string(),

195 + ),

196 + sse_text("done"),

197 + sse_text("still here"),

198 + ]);

199 +

200 + let mut agent = spawn_agent(endpoint.port, &config_dir);

201 +

202 + let id = agent.request(

203 + "initialize",

204 + json!({"protocolVersion": 1, "clientCapabilities": {}}),

205 + );

206 + agent.wait_for_response(id);

207 +

208 + let id = agent.request("session/new", json!({"cwd": work, "mcpServers": []}));

209 + let session_id = agent.wait_for_response(id)["result"]["sessionId"]

210 + .as_str()

211 + .expect("session id")

212 + .to_string();

213 +

214 + let first = agent.request(

215 + "session/prompt",

216 + json!({"sessionId": session_id, "prompt": [{"type": "text", "text": "commit it"}]}),

217 + );

218 + agent.wait_for_response(first);

219 +

220 + // The client's next request must reach the agent.

221 + let second = agent.request(

222 + "session/prompt",

223 + json!({"sessionId": session_id, "prompt": [{"type": "text", "text": "and again"}]}),

224 + );

225 + agent.wait_for_response(second);

226 +

227 + let swallowed = std::fs::read_to_string(&stolen).unwrap_or_default();

228 + assert!(

229 + swallowed.is_empty(),

230 + "the tool's child read {} bytes off the agent's stdin: {swallowed}",

231 + swallowed.len()

232 + );

233 +

234 + std::fs::remove_dir_all(&scratch).ok();

235 +}