Report non-zero process exits in tool logs

[stevetalkai]

Summary

• let logged tool operations derive structured metadata from successful return values
• record process session state and exit codes
• mark completed non-zero and signal exits as failed instead of successful

Why

exec_command and write_stdin can 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.ts
• pnpm run typecheck
• 3 regression tests passed

Summary by CodeRabbit

• New Features

  • Tool activity logs now include process session IDs, running status, and exit codes.
  • Command execution and input-writing operations report success or failure details more accurately.
  • Failed process completions include a generated termination error message.

• Tests

  • Added coverage for running processes, successful completion, and failed completion scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: a2b9bdc4-f8fb-4cf5-ada4-825722397a47

📥 Commits

Reviewing files that changed from the base of the PR and between 69a00ee and de1c534.

📒 Files selected for processing (4)

• src/tool-surfaces/codex.test.ts
• src/tool-surfaces/codex.ts
• src/tool-surfaces/shared.ts
• src/tool-surfaces/types.ts

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.

---

📝 Walkthrough

Walkthrough

The change adds process-state fields to tool logs, derives them from ProcessSnapshot, and connects the derivation to exec_command and write_stdin. Tests cover running processes, successful completion, and failed completion.

Changes

Process log tracking

Layer / File(s)	Summary
Logging contract and result metadata src/tool-surfaces/types.ts, src/tool-surfaces/shared.ts	ToolLogFields now supports session identifiers, running state, and exit codes. runLoggedToolOperation accepts result metadata and applies an optional success override.
Process metadata derivation and tool wiring src/tool-surfaces/codex.ts, src/tool-surfaces/codex.test.ts	processLogFields derives process fields and failure messages from ProcessSnapshot. Both Codex tools pass the helper to runLoggedToolOperation. Tests cover running, successful, and failed processes.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to de1c5

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

Loading

Suggested reviewers: waishnav

Poem

A rabbit logs a running hare
With session numbers neat and fair
Zero exits earn a cheer
Failed runs leave errors clear
The tools now tell the tale with care

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: reporting non-zero process exits in tool logs.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

The 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.

• Adds result-derived metadata support to the shared logged-operation wrapper.
• Records process session, running, and exit-code fields.
• Adds regression coverage for running, zero-exit, and nonzero-exit snapshots.

Confidence Score: 4/5

The 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

Important Files Changed

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]

Loading

_{Reviews (1): Last reviewed commit: "fix: report non-zero process exits in to..." | Re-trigger Greptile}