fix(agy): pure SpawnArgs, no temp-file leak, forward signals to agy#54
Conversation
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) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b3dbca343
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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)") |
There was a problem hiding this comment.
When a caller sends a replacement message while the first agy turn is still running, process.replaceSubprocess calls ResumeArgs before terminating the current subprocess (engine/cli/process.go:611-614). This code now resolves only the ID already captured from the wrapper's stdout sentinel, but that sentinel is emitted only after a clean exit, so interrupted turns return agy: no conversation ID available even if agy has already written the conversation ID to its log. The previous log-side read allowed these mid-turn replacements to resume; keep an early side channel or emit/capture the ID before ResumeArgs is needed.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the agy CLI backend to make SpawnArgs a pure argument builder with no side effects. Temporary log file allocation, signal forwarding, and conversation ID extraction are moved into the shell wrapper script, which emits an agy_session sentinel captured by ParseLine. Feedback was provided to address a potential race condition in the shell wrapper's signal handling, to use a more precise POSIX BRE pattern in sed for robust UUID extraction, and to prioritize root options over backend-specific options in SpawnArgs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if [ -n "$__log" ]; then set -- --log-file "$__log" "$@"; fi | ||
| "$0" "$@" & | ||
| __pid=$! | ||
| trap 'kill -TERM "$__pid" 2>/dev/null' TERM INT |
There was a problem hiding this comment.
The trap command kill -TERM "$__pid" is asynchronous. When the shell wrapper receives SIGTERM or SIGINT, it sends the signal to agy and immediately proceeds to delete the log file and exit, without waiting for agy to actually terminate. This can leave agy running as an orphaned process and causes a race condition where the log file is deleted while agy is still shutting down. Waiting for the process to exit inside the trap ensures a clean and graceful shutdown.
| trap 'kill -TERM "$__pid" 2>/dev/null' TERM INT | |
| trap 'kill -TERM "$__pid" 2>/dev/null; wait "$__pid" 2>/dev/null' TERM INT |
| 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) |
There was a problem hiding this comment.
In SpawnArgs, if root options (such as the session's conversation ID) are set, they must take precedence over backend-specific options like resumeID. If these root options are invalid, they should be silently skipped rather than falling back to backend-specific settings to avoid unexpected behavior and prevent sequential session pollution.
| 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) | |
| func (b *Backend) SpawnArgs(session agentrun.Session) (string, []string) { | |
| var resumeID string | |
| if session.ConversationID() != "" { | |
| if isValidUUID(session.ConversationID()) { | |
| resumeID = session.ConversationID() | |
| } | |
| } else if val := b.resumeID.Load(); val != nil { | |
| resumeID = val.(string) | |
| } |
References
- When designing an API with independent control surfaces (e.g., root options and backend-specific options), if root options are set, they should take precedence over backend-specific options. If root options are invalid, they should be silently skipped in lenient contexts (like
SpawnArgsincli.Spawnercontract) to avoid unexpected behavior for the consumer, rather than falling back to backend-specific settings which could be more surprising and harder to debug.
| 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) |
There was a problem hiding this comment.
The sed pattern [0-9a-f-][0-9a-f-]* is very loose and can match any sequence of hex characters and hyphens of any length. If the log output contains trailing characters or other hex/hyphen sequences, it could extract an invalid or malformed UUID. Since the Go parser regex expects an exact 36-character UUID, any mismatch will cause the conversation ID capture to fail. Using a precise POSIX BRE pattern for UUIDs ensures robust extraction.
| __cid=$(sed -n 's/.*Created conversation \([0-9a-f-][0-9a-f-]*\).*/\1/p' "$__log" 2>/dev/null | head -n 1) | |
| __cid=$(sed -n 's/.*Created conversation \\([0-9a-f]\\{8\\}-[0-9a-f]\\{4\\}-[0-9a-f]\\{4\\}-[0-9a-f]\\{4\\}-[0-9a-f]\\{12\\}\\).*/\\1/p' "$__log" 2>/dev/null | head -n 1) |
1 participant
Addresses post-merge review feedback on #53 from Gemini Code Assist and Codex.
Issues fixed
SpawnArgscreated a temp log file → leaked on everyEngine.Validate()(zero session), leaked again becauseStartre-callsSpawnArgsand overwrites the path, leaked on single-turn runs (noResumeArgs), and violated the "SpawnArgsis a pure builder" contract.agy-*.logfiles in the temp dir.sh -cwrapper meanscmd.Processissh, notagy. The engine signalscmd.Processon Stop/replacement, and a plainsh -cdoesn't forward signals to its child → agy left running orphaned.Approach
Rather than papering over with a finalizer, the backend now matches the codex/opencode capture pattern and hardens the wrapper:
SpawnArgsis pure — no file I/O. The shell wrapper allocates its own temp log viamktempandrms it on exit, so nothing leaks (Validate, single-turn, or test).Created conversation <id>from the log and emits a{"type":"agy_session","id":"..."}sentinel;ParseLinecaptures it write-once into an atomic pointer.ResumeArgsno longer reads/removes a file side channel.trapsSIGTERM/SIGINT, forwarding to the agy child so Stop/replacement actually terminates agy.Verification
make qa+make mcp-qagreen.🤖 Generated with Claude Code