fix(run_state): keep local shell tool outputs when restoring a RunState - #4249
fix(run_state): keep local shell tool outputs when restoring a RunState#4249hsusul wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 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".
| # 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", |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Can you resolve the review comments?
Summary
Component:
RunStatedeserialization (_deserialize_tool_call_output_raw_item) /LocalShellToolresume.Problem. A run that used a
LocalShellToolcannot 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_idis the key the runner pairs calls with outputs on —_completed_call_ids_by_type()inrun_internal/items.pyreads onlycall_id, anddrop_orphan_function_calls()prunes any call whose id is not in that set.On resume, that raw item was validated against the Responses
LocalShellCallOutputshape, which declaresid(required) and has nocall_idat all. Two consequences:idmissing)._deserialize_itemscatches theValidationError, logsFailed to deserialize item, andcontinues — the output vanishes. The now-orphanedlocal_shell_callis pruned on the next turn, so both items are gone.call_id. Even withidpresent, the TypedDict adapter strips unknown keys, socall_idis 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]— nolocal_shell_call, nolocal_shell_call_output.Change
Deserialize
local_shell_call_outputthe 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 acceptedToolCallOutputTypesmember, so no new shape is introduced.This preserves whatever the producer wrote —
call_idfor SDK-produced items, and bothidandcall_idfor API-shaped ones — which is strictly more preserving than the previous behaviour for every input._LOCAL_SHELL_OUTPUT_ADAPTERbecomes unused and is removed.LocalShellCallOutputstays in_TOOL_CALL_OUTPUT_UNION_ADAPTERand in the return annotation, so no public or private signature changes.Why minimal / why not a producer fix. Making
LocalShellActionalso emitiddoes not fix resume: validation still stripscall_id, so the pairing indrop_orphan_function_callsstill fails and the call is still pruned. Changing the runner to pair local shell items onidinstead 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_VERSIONis 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_idon alocal_shell_call_outputon the wire is out of scope; this PR does not change what the SDK sends to the model, only what survives aRunStateround trip.Test plan
Three tests added to
tests/test_local_shell_tool.py, all driven through the publicRunner.run/RunState.from_jsonpath withFakeModel(no API key, no network):test_local_shell_output_survives_run_state_roundtrip— a real run with aLocalShellTool, serialized withto_state().to_json(), string round-tripped, and restored; asserts thelocal_shell_call_outputitem survives with itscall_idand output.test_resumed_local_shell_run_replays_call_and_output— resumes that snapshot throughRunner.runand asserts the next model input contains both thelocal_shell_calland itslocal_shell_call_outputwith matchingcall_id.test_api_shaped_local_shell_output_still_restores— rewrites the snapshot's shell output into the Responses API shape (addsid) and asserts bothidandcall_idsurvive, covering the field-stripping half of the bug.All three fail on
upstream/main@b47a0e4band pass after the change. The two pre-existing tests in that file pass before and after, showing the producer path is untouched: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 -qrun 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.responsesparam models).Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR