Skip to content

fix(acp): keep tool_call content, rawInput, and rawOutput in captured trajectories - #1100

Open
Benjamin-eecs wants to merge 2 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-acp-capture-raw-tool-io
Open

fix(acp): keep tool_call content, rawInput, and rawOutput in captured trajectories#1100
Benjamin-eecs wants to merge 2 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-acp-capture-raw-tool-io

Conversation

@Benjamin-eecs

@Benjamin-eecs Benjamin-eecs commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Description

  • acp/session.py: ToolCallRecord gains raw_input / raw_output. The tool_call branch now keeps the opening notification's content and raw I/O; the tool_call_update branch merges rawInput / rawOutput when present (later wins). Status handling is unchanged.
  • trajectories/_capture.py: _events_to_trajectory writes raw_input / raw_output only when set, so agents that never send them produce byte-identical trajectories.
  • viewer/payload.py: a tool call without content blocks renders from its raw I/O: execute shows the command, then the formatted output with a nonzero exit code stated; other kinds show the JSON. Content blocks still win when both exist.

Motivation and Context

Fixes #1099. codex-acp 1.6.0 reports a command as rawInput on the opening tool_call, its output as rawOutput on the completing update, and file-change diffs as content on the opening tool_call. The capture read none of those, so every codex trajectory in the FrontierPhysics batch has empty tool calls and the viewer shows headers only.

How Has This Been Tested?

  • tests/test_tool_call_raw_io.py (5 tests, docstrings name [BUG] ACP capture drops tool_call content, rawInput, and rawOutput; codex-acp trajectories have no commands, outputs, or patches #1099): raw fields survive on the record, opening-call content survives, trajectory keys appear only when set, raw rendering of commands/outputs/exit codes, payload fallback with content taking precedence.
  • pytest tests/: all passed (29 skipped, playwright absent). ruff check, ruff format --check, ty check src/ clean.
  • Shape verified against codex-acp 1.6.0 source (createCommandExecutionUpdate, createCommandExecutionCompleteUpdate, createFileChangeUpdate) and a 540-rollout batch where 0 of 64 sampled tool calls carried content.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Devin Review

Copilot AI lite review requested due to automatic review settings September 4, 2026 17:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

Comment on lines +433 to +436
initial_content = update.get("content")
if isinstance(initial_content, list) and initial_content:
record.content.extend(initial_content)
record.absorb_raw_io(update)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Completed file edits remain pending

When an opening tool_call reports a completed file edit, its status remains pending despite capturing its content. Timeout diagnostics then mark finished work incomplete.

Prompt for agents
The tool_call branch in src/benchflow/acp/session.py now captures opening content for codex-acp file-change events, but ToolCallRecord still starts as PENDING and this branch ignores update["status"]. Codex file-change notifications can be complete in this opening event without a later tool_call_update. Parse and apply a present opening status using the same unknown-status handling as the update branch, while preserving PENDING when no status is supplied. Add coverage that a completed opening-only file-change call is terminal and absent from pending_tool_call_ids().
Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed in 301c72f: the opening tool_call's status is applied with the same unknown-status handling as updates, PENDING stays the default when absent, and a test covers a completed opening-only edit leaving pending_tool_call_ids().

@Galius5136

Copy link
Copy Markdown

I tried pretty hard to break this and I’m comfortable approving it.

I verified the capture path end to end with the shipped ACPClient against a codex-shaped ACP stdio agent. Only the agent process is stubbed; session handling, tool-call capture, redaction, trajectory writing, and viewer payload generation are the real BenchFlow code.

On the PR head:

  • commands and outputs are preserved;
  • non-zero exit codes are rendered;
  • edit patches are retained;
  • already-completed opening tool calls no longer remain pending;
  • the stock ACP agent produces a byte-identical trajectory before/after this change;
  • secrets placed in rawInput / rawOutput are still redacted;
  • all 10 logical source branches I mutated were independently caught by the tests;
  • targeted tests and lint/type gates are clean;
  • the three full-suite failures reproduce identically with the base source and are unrelated.

I found three non-blocking follow-ups:

  1. The new raw I/O reaches the trajectory and viewer, but the export paths still ignore it. In particular, acp_events_to_messages / trainer output retains only the tool-call title, and the ATIF exporter emits arguments: {} even when raw_input contains the arguments. I’d treat exporter support as a separate follow-up rather than expanding this PR.

  2. deepagents_acp_shim.py and harvey_lab_acp_shim.py currently emit tool arguments under input, not ACP rawInput, so they still lose their arguments through this path. Fixing the shims to emit the ACP field seems cleaner than teaching the capture layer a non-standard alias.

  3. Small robustness nit: _raw_io_texts treats the string "0" as a non-zero exit code and appends [exit code 0]. Codex sends a numeric exit code, so this is not on the path this PR targets.

None of these block the fix as scoped. Approved.

@kywch

kywch commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

I encountered the same issue while running ClawsBench with codex-acp.

Across 44 trials, all 770 Codex tool-call events were present, including IDs, titles, kinds, and statuses. However, all 770 had empty content and lacked structured command input/output.

ClawsBench is not the loss boundary. BenchFlow’s native trajectory/acp_trajectory.jsonl was already missing those payloads. ClawsBench only parses and re-serializes those events into trajectory.raw.json.

PR #1100 appears to solve the completed-call raw-capture problem described in #1099. Once merged, ClawsBench’s trajectory.raw.json should inherit raw_input, raw_output, opening content, and status without requiring a ClawsBench-specific capture change.

I agree adjacent consumers should be handled separately from this PR, but the sooner they are fixed, the better:

  • ATIF still emits empty arguments and ignores raw output.
  • ADP still emits empty kwargs and ignores raw input/output.
  • verifiers.jsonl conversion ignores raw input/output.
  • Viewer handling may suppress raw-I/O fallback when terminal-reference content is present.
  • Interrupted calls can still lose streamed output because _meta.terminal_output_delta is not retained.

For ClawsBench, this means trajectory.raw.json should improve after #1100, while trajectory.json remains lossy because it comes from BenchFlow’s ATIF artifact.

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.

[BUG] ACP capture drops tool_call content, rawInput, and rawOutput; codex-acp trajectories have no commands, outputs, or patches

4 participants