Skip to content

fix(run_state): keep local shell tool outputs when restoring a RunState - #4249

Open
hsusul wants to merge 2 commits into
openai:mainfrom
hsusul:fix/local-shell-call-output-resume
Open

fix(run_state): keep local shell tool outputs when restoring a RunState#4249
hsusul wants to merge 2 commits into
openai:mainfrom
hsusul:fix/local-shell-call-output-resume

Conversation

@hsusul

@hsusul hsusul commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Component: RunState deserialization (_deserialize_tool_call_output_raw_item) / LocalShellTool resume.

Problem. A run that used a LocalShellTool cannot be resumed: the local shell tool output is silently dropped from the restored state, and because the paired call is then an orphan, the shell call disappears too. The model never sees that the command ran.

LocalShellAction.execute (run_internal/tool_actions.py) writes the output item as:

{"type": "local_shell_call_output", "call_id": call.tool_call.call_id, "output": result}

call_id is the key the runner pairs calls with outputs on — _completed_call_ids_by_type() in run_internal/items.py reads only call_id, and drop_orphan_function_calls() prunes any call whose id is not in that set.

On resume, that raw item was validated against the Responses LocalShellCallOutput shape, which declares id (required) and has no call_id at all. Two consequences:

  1. SDK-produced items fail validation (id missing). _deserialize_items catches the ValidationError, logs Failed to deserialize item, and continues — the output vanishes. The now-orphaned local_shell_call is pruned on the next turn, so both items are gone.
  2. API-shaped items lose call_id. Even with id present, the TypedDict adapter strips unknown keys, so call_id is dropped and the paired call is orphaned anyway.

Before this change, resuming the run in the added test produces a model input of ["please run shell", message, message] — no local_shell_call, no local_shell_call_output.

Change

Deserialize local_shell_call_output the same way its sibling hosted-tool outputs (shell_call_output, apply_patch_call_output, custom_tool_call_output) are already handled: return the normalized mapping unchanged. dict[str, Any] is already part of the function's return type and an accepted ToolCallOutputTypes member, so no new shape is introduced.

This preserves whatever the producer wrote — call_id for SDK-produced items, and both id and call_id for API-shaped ones — which is strictly more preserving than the previous behaviour for every input.

_LOCAL_SHELL_OUTPUT_ADAPTER becomes unused and is removed. LocalShellCallOutput stays in _TOOL_CALL_OUTPUT_UNION_ADAPTER and in the return annotation, so no public or private signature changes.

Why minimal / why not a producer fix. Making LocalShellAction also emit id does not fix resume: validation still strips call_id, so the pairing in drop_orphan_function_calls still fails and the call is still pruned. Changing the runner to pair local shell items on id instead would touch the shared call/output pairing used by every tool family. The reader is the only place that is inconsistent with its three siblings, so that is where the fix goes.

Compatibility. The serialized format is unchanged, so CURRENT_SCHEMA_VERSION is not bumped — this only affects backward-read of persisted state, and it makes strictly more snapshots restorable than before. No public API, exception type, or hook contract changes. Streamed and non-streamed resume share this deserializer, so both paths are fixed by the same change.

Non-goals. Whether the Responses API itself accepts call_id on a local_shell_call_output on the wire is out of scope; this PR does not change what the SDK sends to the model, only what survives a RunState round trip.

Test plan

Three tests added to tests/test_local_shell_tool.py, all driven through the public Runner.run / RunState.from_json path with FakeModel (no API key, no network):

  • test_local_shell_output_survives_run_state_roundtrip — a real run with a LocalShellTool, serialized with to_state().to_json(), string round-tripped, and restored; asserts the local_shell_call_output item survives with its call_id and output.
  • test_resumed_local_shell_run_replays_call_and_output — resumes that snapshot through Runner.run and asserts the next model input contains both the local_shell_call and its local_shell_call_output with matching call_id.
  • test_api_shaped_local_shell_output_still_restores — rewrites the snapshot's shell output into the Responses API shape (adds id) and asserts both id and call_id survive, covering the field-stripping half of the bug.

All three fail on upstream/main @ b47a0e4b and pass after the change. The two pre-existing tests in that file pass before and after, showing the producer path is untouched:

# before (src/agents/run_state.py reverted)
$ uv run pytest tests/test_local_shell_tool.py -q
..FFF
E       assert 0 == 1                                    # output item dropped
E       assert 'local_shell_call' in [None, 'message', 'message']   # call pruned as orphan
E       KeyError: 'call_id'                              # call_id stripped by validation
3 failed, 2 passed

# after
$ uv run pytest tests/test_local_shell_tool.py -q
5 passed

Verification run from the repository root:

  • .agents/skills/code-change-verification/scripts/run.sh — all commands passed (format, lint, typecheck, tests).
  • make format / make lint — all checks passed.
  • make typecheck — mypy: no issues in 849 source files; pyright: 0 errors, 0 warnings.
  • make tests — 6668 passed, 29 skipped (parallel) and 38 passed, 5 skipped (serial).
  • uv run pytest tests/test_local_shell_tool.py tests/test_run_state.py tests/test_hitl_error_scenarios.py -q run 5× — 315 passed each time, no unclosed-resource or pending-task warnings.
  • git diff --check — clean.

Not run: make coverage, make build-docs, and the integration-test profiles (they need credentials or live services); no docs or examples are affected by this change.

Issue number

N/A (found by auditing SDK-constructed item payloads against the generated openai.types.responses param models).

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh
  • I've confirmed all verification steps pass
  • If using Codex, I've run /review before submitting this PR

@seratch seratch added this to the 0.20.x milestone Aug 6, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4f2d4b2e9a

ℹ️ 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".

Comment thread src/agents/run_state.py
Comment on lines +2381 to +2384
# LocalShellAction writes ``call_id`` (the key the runner pairs calls and outputs on) and
# no ``id``, so validating against the Responses ``LocalShellCallOutput`` shape both
# rejects SDK-produced items and strips ``call_id`` from API-shaped ones.
"local_shell_call_output",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Convert restored local-shell outputs to the API shape

When this state is resumed through OpenAIResponsesModel, this branch retains the SDK-produced {type, call_id, output} mapping and ToolCallOutputItem.to_input_item() forwards it unchanged. The Responses LocalShellCallOutput request contract requires id and does not define call_id, so the newly restored item is rejected by the real provider instead of completing the resume; the FakeModel test cannot expose that failure. Preserve call_id for internal orphan matching, but translate it to id and remove the internal field at the provider boundary.

AGENTS.md reference: AGENTS.md:L136-L136

Useful? React with 👍 / 👎.

@seratch seratch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you resolve the review comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants