Report non-zero process exits in tool logs - #287
Conversation
Allow logged tool operations to derive result metadata from successful return values. Process tools now record running state and exit codes, and mark completed non-zero or signal exits as failed instead of reporting every resolved operation as successful.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adds process-state fields to tool logs, derives them from ChangesProcess log tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change improves process-exit reporting in tool logs without altering command execution behavior or authority, and no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant exec_command
participant write_stdin
participant runLoggedToolOperation
participant processLogFields
participant logToolCall
exec_command->>runLoggedToolOperation: Execute operation
write_stdin->>runLoggedToolOperation: Execute operation
runLoggedToolOperation->>processLogFields: Pass ProcessSnapshot
processLogFields-->>runLoggedToolOperation: Return process log fields
runLoggedToolOperation->>logToolCall: Merge fields and log result
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR enriches Codex process tool logs with session and exit metadata and derives log success from completed process results so nonzero exits are reported as failures.
Confidence Score: 4/5The signal-aware success classification should be fixed before merging because a terminated PTY process can still be reported as successful. processLogFields treats exitCode 0 as success without checking signal, while the PTY process path can produce a completed snapshot containing both values. Files Needing Attention: src/tool-surfaces/codex.ts, src/tool-surfaces/codex.test.ts
|
| Filename | Overview |
|---|---|
| src/tool-surfaces/codex.ts | Adds process-derived log metadata, but ignores the signal field when classifying PTY process outcomes. |
| src/tool-surfaces/shared.ts | Adds an optional result-metadata callback and merges its fields into successful-operation log events. |
| src/tool-surfaces/types.ts | Extends tool log fields with optional process session and exit-state metadata. |
| src/tool-surfaces/codex.test.ts | Covers running, zero-exit, and nonzero-exit snapshots but omits the PTY exitCode-zero-with-signal state. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[exec_command or write_stdin] --> B[ProcessSnapshot]
B --> C{Still running?}
C -->|Yes| D[Log success]
C -->|No| E{Exit code is zero?}
E -->|No| F[Log failure and termination error]
E -->|Yes| G[Log success]
G -. Signal may also be present .-> H[PTY signal exit misclassified]
Reviews (1): Last reviewed commit: "fix: report non-zero process exits in to..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| export function processLogFields(result: ProcessSnapshot): Partial<ToolLogFields> { | ||
| const success = result.running || result.exitCode === 0; |
There was a problem hiding this comment.
When a terminated PTY reports both exitCode: 0 and a signal, this expression classifies the process as successful, causing the tool call to be logged at the info level without an error even though it was terminated by a signal.
| const success = result.running || result.exitCode === 0; | |
| const success = result.running || (!result.signal && result.exitCode === 0); |
Knowledge Base Used: Agent tool surfaces
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
2 participants
Summary
Why
exec_commandandwrite_stdincan return normally even when the child process exits unsuccessfully. The current logging helper treats every resolved operation as a successful tool call, which makes diagnostics report false positives.Testing
pnpm exec tsx --test src/tool-surfaces/codex.test.tspnpm run typecheckSummary by CodeRabbit
New Features
Tests