From 5b3dbca343b6487ae6d584e9193814bc2286f645 Mon Sep 17 00:00:00 2001 From: David Mora Date: Sun, 7 Jun 2026 13:12:09 -0600 Subject: [PATCH] fix(agy): pure SpawnArgs, no temp-file leak, forward signals to agy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #53 (Gemini Code Assist, Codex): - SpawnArgs is now a pure, I/O-free argument builder. Previously it created a temp log file, which leaked on every Engine.Validate() call (zero Session), leaked again because Start re-calls SpawnArgs and overwrites the path, and leaked on single-turn runs (run_turn/probe) where ResumeArgs never runs to delete it — and it violated the Spawner pure-builder contract. - The shell wrapper now allocates its OWN temp log via mktemp and removes it on exit, extracts the new conversation ID from the log and emits it as an {"type":"agy_session",...} stdout sentinel. ParseLine captures that ID (write-once atomic), so ResumeArgs no longer reads/removes a file side channel (matches the codex/opencode stdout-capture pattern). No temp file can leak. - The wrapper runs agy in the background and forwards SIGTERM/SIGINT to it. The CLI engine signals cmd.Process (= sh, not agy) on Stop/replacement; a plain `sh -c` does not forward signals to its child, so timeouts/session_stop could orphan the agy agent. Forwarding ensures agy is terminated. Verified live: turn-1 ID capture + turn-2 resume (2/2), no leftover temp files, and SIGTERM to the wrapper kills agy (no orphan). make qa + make mcp-qa green. Co-Authored-By: Claude Opus 4.8 (1M context) --- engine/cli/agy/agy.go | 113 ++++++++++++------------ engine/cli/agy/agy_test.go | 167 ++++++++++++++++++++++------------- engine/cli/agy/parse.go | 28 ++++-- engine/cli/agy/parse_test.go | 6 ++ 4 files changed, 191 insertions(+), 123 deletions(-) diff --git a/engine/cli/agy/agy.go b/engine/cli/agy/agy.go index 905b2cc..a70dc3e 100644 --- a/engine/cli/agy/agy.go +++ b/engine/cli/agy/agy.go @@ -2,8 +2,6 @@ package agy import ( "errors" - "os" - "regexp" "strings" "sync/atomic" @@ -21,21 +19,46 @@ const ( const defaultBinary = "agy" -// validConversationID matches the UUID format used by agy conversation IDs. -var validConversationID = regexp.MustCompile( - `Created conversation ([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})`, -) - -// shellWrapper is the sh -c script that runs agy (via $0) and emits a JSON -// MessageResult sentinel on clean exit. Using "$0" instead of embedding the -// binary path in the script prevents shell injection via WithBinary. -const shellWrapper = `"$0" "$@"; _E=$?; [ $_E -eq 0 ] && printf '{"type":"result","stop_reason":"end_turn"}\n'; exit $_E` - -// Backend is an Antigravity CLI backend for agentrun. -// It implements cli.Spawner, cli.Parser, and cli.Resumer for a spawn-per-turn model. +// shellWrapper is the sh -c script that runs agy and adapts it to the agentrun +// CLI engine. agy is plain-text and records the conversation ID only in its log +// file, so the wrapper: +// +// - allocates its OWN temp log via mktemp (so SpawnArgs stays a pure, I/O-free +// argument builder and nothing leaks on Engine.Validate or single-turn runs), +// - runs agy in the background and forwards SIGTERM/SIGINT to it, so the engine +// signalling sh on Stop/replacement actually terminates agy instead of +// orphaning it, +// - on clean exit, extracts "Created conversation " from the log and emits +// it as an {"type":"agy_session",...} sentinel for the parser to capture (so +// ResumeArgs needs no file side channel), followed by the MessageResult +// sentinel, +// - always removes its temp log and preserves agy's exit code. +// +// The binary is passed as argv[0] ($0), never interpolated into the script, so +// WithBinary cannot inject shell metacharacters. +const shellWrapper = `__log=$(mktemp 2>/dev/null) || __log= +if [ -n "$__log" ]; then set -- --log-file "$__log" "$@"; fi +"$0" "$@" & +__pid=$! +trap 'kill -TERM "$__pid" 2>/dev/null' TERM INT +wait "$__pid" +__e=$? +trap - TERM INT +if [ "$__e" -eq 0 ]; then + if [ -n "$__log" ]; then + __cid=$(sed -n 's/.*Created conversation \([0-9a-f-][0-9a-f-]*\).*/\1/p' "$__log" 2>/dev/null | head -n 1) + if [ -n "$__cid" ]; then printf '{"type":"agy_session","id":"%s"}\n' "$__cid"; fi + fi + printf '{"type":"result","stop_reason":"end_turn"}\n' +fi +if [ -n "$__log" ]; then rm -f "$__log"; fi +exit "$__e"` + +// Backend is an Antigravity CLI backend for agentrun. It implements cli.Spawner, +// cli.Parser, and cli.Resumer for a spawn-per-turn model. The conversation ID is +// captured (write-once) from the wrapper's agy_session sentinel by ParseLine. type Backend struct { binary string - logFile string // path to the temporary log file for capturing the session ID resumeID atomic.Pointer[string] } @@ -69,9 +92,8 @@ func New(opts ...Option) *Backend { return b } -// buildWrapperArgs wraps agyArgs + prompt in a sh -c invocation that emits -// the MessageResult sentinel on success. The binary is passed as argv[0] ($0) -// to avoid shell injection via binary path metacharacters. +// buildWrapperArgs wraps agyArgs + prompt in the sh -c invocation. The binary is +// passed as argv[0] ($0) to avoid shell injection via binary path metacharacters. func (b *Backend) buildWrapperArgs(agyArgs []string, prompt string) (string, []string) { args := make([]string, 0, len(agyArgs)+5) // argv[0] = b.binary (becomes $0 in the script); remaining args become $@. @@ -81,20 +103,11 @@ func (b *Backend) buildWrapperArgs(agyArgs []string, prompt string) (string, []s return "sh", args } -// SpawnArgs builds exec.Cmd arguments for a new agy session. +// SpawnArgs builds exec.Cmd arguments for a new agy session. It is a pure +// argument builder with no side effects: the temporary log file is allocated by +// the shell wrapper at run time, not here (Engine.Validate also calls SpawnArgs). func (b *Backend) SpawnArgs(session agentrun.Session) (string, []string) { - // Create a temp log file so agy records the new conversation ID. - // If creation fails we omit --log-file; ResumeArgs falls back to OptionResumeID. - if f, err := os.CreateTemp("", "agy-*.log"); err == nil { - f.Close() - b.logFile = f.Name() - } - - var agyArgs []string - if b.logFile != "" { - agyArgs = append(agyArgs, "--log-file", b.logFile) - } - agyArgs = appendSessionArgs(agyArgs, session) + agyArgs := appendSessionArgs(nil, session) prompt := session.Prompt if jsonutil.ContainsNull(prompt) { @@ -104,7 +117,8 @@ func (b *Backend) SpawnArgs(session agentrun.Session) (string, []string) { return b.buildWrapperArgs(agyArgs, prompt) } -// ResumeArgs builds exec.Cmd arguments to resume an existing agy session. +// ResumeArgs builds exec.Cmd arguments to resume an existing agy session, using +// the conversation ID captured by ParseLine (or OptionResumeID as a fallback). func (b *Backend) ResumeArgs(session agentrun.Session, initialPrompt string) (string, []string, error) { if jsonutil.ContainsNull(initialPrompt) { return "", nil, errors.New("agy: initial prompt contains null bytes") @@ -113,31 +127,9 @@ func (b *Backend) ResumeArgs(session agentrun.Session, initialPrompt string) (st return "", nil, err } - // Determine the conversation UUID to resume. - var uuid string - if captured := b.resumeID.Load(); captured != nil { - uuid = *captured - } - - if uuid == "" && b.logFile != "" { - // First resume: read the log file written by SpawnArgs to get the UUID. - if data, err := os.ReadFile(b.logFile); err == nil { - if m := validConversationID.FindSubmatch(data); len(m) == 2 { - uuid = string(m[1]) - b.resumeID.Store(&uuid) - } - } - _ = os.Remove(b.logFile) - b.logFile = "" - } - - // Fallback to explicitly-provided resume ID. + uuid := b.resolveResumeID(session) if uuid == "" { - uuid = session.Options[agentrun.OptionResumeID] - } - - if uuid == "" { - return "", nil, errors.New("agy: no conversation ID available (not captured from log and OptionResumeID not set)") + return "", nil, errors.New("agy: no conversation ID available (not captured from output and OptionResumeID not set)") } agyArgs := []string{"--conversation", uuid} @@ -147,6 +139,15 @@ func (b *Backend) ResumeArgs(session agentrun.Session, initialPrompt string) (st return binary, args, nil } +// resolveResumeID returns the auto-captured conversation ID (write-once from the +// agy_session sentinel) or the explicit OptionResumeID fallback. +func (b *Backend) resolveResumeID(session agentrun.Session) string { + if p := b.resumeID.Load(); p != nil { + return *p + } + return session.Options[agentrun.OptionResumeID] +} + func appendSessionArgs(args []string, session agentrun.Session) []string { if session.Model != "" && !jsonutil.ContainsNull(session.Model) && !strings.HasPrefix(session.Model, "-") { args = append(args, "--model", session.Model) diff --git a/engine/cli/agy/agy_test.go b/engine/cli/agy/agy_test.go index df372d7..e5a2784 100644 --- a/engine/cli/agy/agy_test.go +++ b/engine/cli/agy/agy_test.go @@ -1,12 +1,13 @@ package agy import ( + "errors" "os" - "path/filepath" "strings" "testing" "github.com/dmora/agentrun" + "github.com/dmora/agentrun/engine/cli" ) func TestBackend_SpawnArgs(t *testing.T) { @@ -26,98 +27,140 @@ func TestBackend_SpawnArgs(t *testing.T) { // args[2] is the binary (argv[0]/$0 inside the script), not a literal "sh". if len(args) < 3 || args[0] != "-c" || args[2] != "agy" { - t.Errorf("SpawnArgs wrapper shell signature mismatch: %q", args) + t.Fatalf("SpawnArgs wrapper shell signature mismatch: %q", args) } wrapperScript := args[1] - if !strings.Contains(wrapperScript, `"$0" "$@"`) { - t.Errorf("Wrapper script missing injection-safe invocation: %s", wrapperScript) - } - if !strings.Contains(wrapperScript, `{"type":"result","stop_reason":"end_turn"}`) { - t.Errorf("Wrapper script missing MessageResult sentinel: %s", wrapperScript) + for _, want := range []string{ + `"$0" "$@"`, // injection-safe invocation + `mktemp`, // wrapper self-manages its log (no Go-side temp file) + `trap`, // signal forwarding to the agy child + resultSentinel, // MessageResult sentinel + `"type":"agy_session"`, // conversation-ID sentinel + } { + if !strings.Contains(wrapperScript, want) { + t.Errorf("wrapper script missing %q:\n%s", want, wrapperScript) + } } - // The remaining args are passed to agy + // SpawnArgs must NOT inject --log-file (the wrapper allocates it at run time) + // and must be a pure builder. agyArgs := args[3:] - - // Check log file injection - if len(agyArgs) < 2 || agyArgs[0] != "--log-file" { - t.Errorf("Missing --log-file injection: %q", agyArgs) - } - if b.logFile == "" { - t.Error("Backend.logFile not set by SpawnArgs") - } - - // Check model - foundModel := false - for i, arg := range agyArgs { - if arg == "--model" && i+1 < len(agyArgs) && agyArgs[i+1] == "gemini-test" { - foundModel = true + for _, a := range agyArgs { + if a == "--log-file" { + t.Errorf("SpawnArgs should not inject --log-file; wrapper manages it: %q", agyArgs) } } - if !foundModel { - t.Errorf("Missing --model flag: %q", agyArgs) - } - // Check prompt and skip perms + assertContainsPair(t, agyArgs, "--model", "gemini-test") if agyArgs[len(agyArgs)-2] != "--print" || agyArgs[len(agyArgs)-1] != "hello world" { - t.Errorf("Prompt not properly appended: %q", agyArgs) + t.Errorf("prompt not appended as final --print arg: %q", agyArgs) + } + if !containsArg(agyArgs, "--dangerously-skip-permissions") { + t.Errorf("missing skip-permissions flag: %q", agyArgs) } +} - foundSkip := false - for _, arg := range agyArgs { - if arg == "--dangerously-skip-permissions" { - foundSkip = true - } +// TestBackend_SpawnArgs_PureNoSideEffects guards the Spawner contract: SpawnArgs +// is called by Engine.Validate with a zero Session and again at Start, so it must +// build args without creating files or mutating capture state. +func TestBackend_SpawnArgs_PureNoSideEffects(t *testing.T) { + before := countTempLogs(t) + b := New() + b.SpawnArgs(agentrun.Session{}) // Validate-style call + b.SpawnArgs(agentrun.Session{Prompt: "x"}) // Start-style call + if b.resumeID.Load() != nil { + t.Error("SpawnArgs must not set the captured resume ID") } - if !foundSkip { - t.Errorf("Missing skip permissions flag: %q", agyArgs) + if after := countTempLogs(t); after != before { + t.Errorf("SpawnArgs created temp log files: before=%d after=%d", before, after) } } -func TestBackend_ResumeArgs(t *testing.T) { +func TestBackend_ResumeArgs_CapturedID(t *testing.T) { b := New() - - // Mock a log file - tmpDir := t.TempDir() - logPath := filepath.Join(tmpDir, "test.log") - logContent := "server.go:753] Created conversation d8e79181-5db2-4ea9-88e2-eea15ddab587\n" - if err := os.WriteFile(logPath, []byte(logContent), 0600); err != nil { - t.Fatal(err) + // Simulate turn 1: the wrapper's agy_session sentinel is parsed, capturing + // the conversation ID. + const id = "d8e79181-5db2-4ea9-88e2-eea15ddab587" + if _, err := b.ParseLine(`{"type":"agy_session","id":"` + id + `"}`); !errors.Is(err, cli.ErrSkipLine) { + t.Fatalf("agy_session sentinel: err = %v, want ErrSkipLine", err) } - - b.logFile = logPath - - session := agentrun.Session{ - Options: map[string]string{}, + if got := b.resumeID.Load(); got == nil || *got != id { + t.Fatalf("conversation ID not captured: %v", got) } - bin, args, err := b.ResumeArgs(session, "turn 2") + bin, args, err := b.ResumeArgs(agentrun.Session{}, "turn 2") if err != nil { - t.Fatalf("ResumeArgs failed: %v", err) + t.Fatalf("ResumeArgs: %v", err) } - if bin != "sh" { t.Errorf("ResumeArgs binary = %q, want sh", bin) } - agyArgs := args[3:] - if len(agyArgs) < 2 || agyArgs[0] != "--conversation" || agyArgs[1] != "d8e79181-5db2-4ea9-88e2-eea15ddab587" { - t.Errorf("ResumeArgs did not properly parse/inject conversation ID: %q", agyArgs) + assertContainsPair(t, agyArgs, "--conversation", id) + if agyArgs[len(agyArgs)-1] != "turn 2" { + t.Errorf("resume prompt not appended: %q", agyArgs) + } +} + +func TestBackend_ResumeArgs_OptionFallback(t *testing.T) { + b := New() + const id = "a1b2c3d4-e5f6-7a8b-9c0d-e1f2a3b4c5d6" + session := agentrun.Session{Options: map[string]string{agentrun.OptionResumeID: id}} + _, args, err := b.ResumeArgs(session, "msg") + if err != nil { + t.Fatalf("ResumeArgs: %v", err) + } + assertContainsPair(t, args[3:], "--conversation", id) +} + +func TestBackend_ResumeArgs_NoID(t *testing.T) { + b := New() + if _, _, err := b.ResumeArgs(agentrun.Session{}, "msg"); err == nil { + t.Error("ResumeArgs with no captured ID and no OptionResumeID should error") } +} - // Verify log file was deleted - if _, err := os.Stat(logPath); !os.IsNotExist(err) { - t.Errorf("Log file was not deleted after ResumeArgs") +func TestBackend_ResumeArgs_NullBytePrompt(t *testing.T) { + b := New() + if _, _, err := b.ResumeArgs(agentrun.Session{}, "bad\x00prompt"); err == nil { + t.Error("ResumeArgs with null-byte prompt should error") } +} - // Second resume should use atomic pointer - _, args2, err := b.ResumeArgs(session, "turn 3") +// assertContainsPair checks that args contains flag immediately followed by value. +func assertContainsPair(t *testing.T, args []string, flag, value string) { + t.Helper() + for i, a := range args { + if a == flag && i+1 < len(args) && args[i+1] == value { + return + } + } + t.Errorf("args missing %q %q: %q", flag, value, args) +} + +func containsArg(args []string, s string) bool { + for _, a := range args { + if a == s { + return true + } + } + return false +} + +// countTempLogs counts agy-*.log files in the OS temp dir (used to assert +// SpawnArgs creates none). +func countTempLogs(t *testing.T) int { + t.Helper() + matches, err := os.ReadDir(os.TempDir()) if err != nil { - t.Fatalf("Second ResumeArgs failed: %v", err) + t.Fatalf("read temp dir: %v", err) } - agyArgs2 := args2[3:] - if len(agyArgs2) < 2 || agyArgs2[0] != "--conversation" || agyArgs2[1] != "d8e79181-5db2-4ea9-88e2-eea15ddab587" { - t.Errorf("Second ResumeArgs did not reuse conversation ID: %q", agyArgs2) + n := 0 + for _, e := range matches { + if strings.HasPrefix(e.Name(), "agy-") && strings.HasSuffix(e.Name(), ".log") { + n++ + } } + return n } diff --git a/engine/cli/agy/parse.go b/engine/cli/agy/parse.go index 10efe65..cea3c38 100644 --- a/engine/cli/agy/parse.go +++ b/engine/cli/agy/parse.go @@ -1,6 +1,7 @@ package agy import ( + "regexp" "strings" "time" @@ -8,17 +9,26 @@ import ( "github.com/dmora/agentrun/engine/cli" ) -// ParseLine implements cli.Parser for the agy backend. -// It maps plain text lines to agentrun.MessageText, and detects the -// synthesized MessageResult sentinel emitted by the shell wrapper. +// resultSentinel is the line the shell wrapper prints on a clean turn, mapped to +// MessageResult. Must stay in sync with the printf in shellWrapper. +const resultSentinel = `{"type":"result","stop_reason":"end_turn"}` + +// agySessionSentinel matches the conversation-ID line the shell wrapper emits +// after a new conversation is created, e.g. +// +// {"type":"agy_session","id":"d8e79181-5db2-4ea9-88e2-eea15ddab587"} +var agySessionSentinel = regexp.MustCompile(`^\{"type":"agy_session","id":"([0-9a-fA-F-]{36})"\}$`) + +// ParseLine implements cli.Parser for the agy backend. It maps plain-text lines +// to agentrun.MessageText, recognizes the synthesized MessageResult sentinel, and +// captures the conversation ID (write-once) from the agy_session sentinel. func (b *Backend) ParseLine(line string) (agentrun.Message, error) { trimmed := strings.TrimSpace(line) if trimmed == "" { return agentrun.Message{}, cli.ErrSkipLine } - // Detect the exact sentinel emitted by the shell wrapper. - if trimmed == `{"type":"result","stop_reason":"end_turn"}` { + if trimmed == resultSentinel { return agentrun.Message{ Type: agentrun.MessageResult, StopReason: agentrun.StopEndTurn, @@ -26,6 +36,14 @@ func (b *Backend) ParseLine(line string) (agentrun.Message, error) { }, nil } + // Capture the conversation ID emitted by the wrapper; it is engine plumbing, + // not agent output, so it is not surfaced to the consumer. + if m := agySessionSentinel.FindStringSubmatch(trimmed); m != nil { + id := m[1] + b.resumeID.CompareAndSwap(nil, &id) // write-once + return agentrun.Message{}, cli.ErrSkipLine + } + return agentrun.Message{ Type: agentrun.MessageText, Content: line, diff --git a/engine/cli/agy/parse_test.go b/engine/cli/agy/parse_test.go index 2dfc479..40f9e43 100644 --- a/engine/cli/agy/parse_test.go +++ b/engine/cli/agy/parse_test.go @@ -46,6 +46,11 @@ func TestParseLine(t *testing.T) { Content: `{"type":"not_result"}`, }, }, + { + name: "agy_session sentinel is captured, not surfaced", + line: `{"type":"agy_session","id":"d8e79181-5db2-4ea9-88e2-eea15ddab587"}`, + wantErr: cli.ErrSkipLine, + }, } for _, tc := range tests { @@ -78,6 +83,7 @@ func FuzzParseLine(f *testing.F) { f.Add(`{"type":"result","stop_reason":"end_turn"}`) f.Add("Some random text output") f.Add("\x00\x00\x00") + f.Add(`{"type":"agy_session","id":"d8e79181-5db2-4ea9-88e2-eea15ddab587"}`) b := New() f.Fuzz(func(_ *testing.T, line string) {