Skip to content

Stop orphaned commands, fix relative paths on macOS, and close IPC/hook gaps - #125

Open
parth-lyzr wants to merge 3 commits into
release/v3from
fix/harness-stress
Open

parth-lyzr wants to merge 3 commits into
release/v3from
fix/harness-stress

Conversation

@parth-lyzr

Copy link
Copy Markdown
Collaborator

Live runs of v3 showed five harness bugs. Each one reproduced with two different models, so they come from the harness, not the model. This PR fixes all five. Each fix has a regression test.

Fixes

Problem Fix
On macOS, relative read_file / apply_patch fail after any shell command (cannot read calc.py). The cwd from the end marker kept a trailing \r (.../pytest\r/calc.py). crates/terminal/src/buffer.rs: trim \r from the marker line before parsing it.
IPC abort, IPC kill, closing stdin, SIGTERM, SIGINT, SDK abort() and --timeout-s all left the running command alive with ppid 1. kill() sent SIGHUP to bash only, and with job control on the command is in its own process group. New crates/terminal/src/stop.rs: SIGTERM, then SIGKILL after up to 1 s, to every process group in the shell's session. Groups come from /proc on Linux and ps -t <pty> on macOS. --ipc now handles SIGTERM/SIGINT (exit 143/130) instead of dying without cleanup.
IPC turn_end.usage / session_end.costs left out sub-agent spend: it summed only the parent's streamed calls. crates/cli/src/ipc.rs: build usage from core's RunReport.usage, which already includes children. requests adds subagent_turns.
Two IPC processes with the same resume sessionId both ran, and the last save silently replaced the other conversation. ipc.rs: exclusive flock on .gitagent/sessions/<id>/lock, taken before ready. The second opener gets system/error and exits 2.
A compliance or fail_open: false hook that could not load (for example, a missing script) was skipped with a warning, so the command it should block ran. crates/loader/src/agent_hooks.rs: load returns an error for these hooks, so the run refuses to start. A compliance hook can no longer set fail_open: true. Ordinary hooks still warn and get skipped.

The new tests also exposed an older race. When a finished background session is hung up, bash's SIGHUP could reach a nohup job before nohup had set SIGHUP to ignore, killing the server. hang_up() now waits up to 200 ms first, but only if the session still has other process groups (stop.rs let_jobs_start).

Effect on --bench

  • Not touched: the IPC, session-lock and hook changes. --ipc conflicts with --bench, and the bench profile has no hooks. bench_prompt_and_tools_match_the_published_run passes.
  • Touched: the \r trim and the terminal stop changes.
    • The trim only removes failed relative-path calls.
    • --linger is unchanged: it never calls kill(), so lingering servers survive.
    • The Ctrl-C fallback now also stops a command that ignores Ctrl-C, instead of orphaning it.
    • A finished background session with live jobs takes up to 200 ms longer to hang up.
    • Single-run mode gets no new signal handler.

Testing

  • cargo test --workspace: 366 passed, 0 failed.
  • cargo clippy on the touched crates: no warnings.
  • Live runs on openai:gpt-6-luna against this branch, all passing:
    • pytest, CLI-flag and npm coding tasks: pass, with zero cannot read errors and no \r in paths;
    • all 7 stop routes: no surviving child in ps afterwards;
    • IPC usage with a sub-agent: 10,132 input, 377 output and 6,502 cache-read tokens, matching the parent + child sum exactly;
    • same sessionId in two processes: the second is refused with exit 2, and the first conversation stays intact;
    • missing compliance hook: startup refused with exit 2, and the blocked command never ran;
    • nohup server: reachable with curl, then cleaned up at the end;
    • --linger server: still reachable after run_end.
  • The Linux /proc process-group lookup compiles, and its parse test passes on macOS. It has not been run on Linux.

Parth Sharma added 3 commits October 28, 2026 18:17
- IPC: turn_end usage and session_end costs come from core's run report,
  which includes sub-agent model calls (their events are disabled, so the
  streamed model_text totals left them out). Requests add the report's
  subagent_turns; a turn without a report (aborted, errored) falls back to
  the streamed calls. Teardown usage is counted the same way.
- IPC: a resumable session (resume: true) takes an exclusive advisory lock
  on .gitagent/sessions/<id>/lock for the process's life. A second process
  opening the same sessionId gets system/error + session_end and exits 2
  instead of loading and later overwriting the other's conversation.
- Loader: a hook marked compliance: true or fail_open: false that cannot
  be loaded (missing or escaping script, bad timeout, unknown event) now
  fails loading, so the run refuses to start. A compliance hook may not set
  fail_open: true. Ordinary hooks keep warn-and-skip.
…rker cwd

- Terminal: on macOS the end marker line can end in \r\n, so the parsed cwd
  kept a trailing \r and relative read_file/apply_patch paths failed. The
  marker line is now trimmed of \r before parsing.
- Terminal: Session::kill (end of run, --timeout-s, IPC abort/kill/closed
  stdin, Terminal drop) only hung up the shell. Job control puts the running
  command and each background job in its own process group, so they
  survived with ppid 1. kill now sends SIGTERM (and SIGCONT) to every
  process group in the shell's session, waits up to 1 s, then SIGKILLs what
  is left, the shell included. Groups are listed from /proc on Linux (by
  session id) and with `ps -t <pty>` on macOS, plus the foreground group
  and the shell's own group.
- Terminal: a finished background session and the "did not stop on Ctrl-C"
  fallback only hang up (SIGTERM/SIGKILL to the foreground command, SIGHUP
  to the shell), so `nohup` servers started there keep running as before.
  --linger is unchanged: it never calls kill.
- IPC: SIGTERM/SIGINT now drop the session (and its shells) before exiting
  with 128+signal, instead of dying with the commands still running.
The shell passes its SIGHUP on to every job, and nohup ignores SIGHUP only
once it runs. A background session whose last command was 'nohup server &'
was hung up the moment its end marker arrived, so the SIGHUP reached the job
while it was still being forked and killed it. hang_up now waits 200 ms
first when the shell has jobs (stop::let_jobs_start), so nohup and disowned
jobs survive; plain jobs still die with the shell. kill/drop are unchanged.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant