fix(core): keep completed tool guardrail results when a resumed append fails - #4651
fix(core): keep completed tool guardrail results when a resumed append fails#4651ayaangazali wants to merge 1 commit into
Conversation
…d fails On a resumed turn that leaves another approval pending, the run publishes the turn's tool guardrail results only inside the NextStepInterruption branch, which sits after the resumed Session append. When that append raises, the approved tool has already executed and both guardrails have already completed, but their results never reach RunState, so they are absent from the checkpoint, from its serialization, and from every later result. Publish them alongside the other resumed state updates, before the append, in both the streaming and non-streaming paths.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ed6c3ee4f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| run_state._tool_input_guardrail_results = [ | ||
| *tool_input_guardrail_results, | ||
| *turn_result.tool_input_guardrail_results, | ||
| ] |
There was a problem hiding this comment.
When pre_approval_tool_input_guardrails=True, the interrupted state already contains the guardrail's pre-approval result, and the required resume-time recheck produces another result. A normal NextStepRunAgain resume intentionally excludes that recheck from run-wide accounting, but this unconditional concatenation stores both results if the resumed Session append fails; retrying from the live or serialized state then reports two input-guardrail results instead of the established count of one. Preserve the existing run-again accounting when checkpointing this failure path.
AGENTS.md reference: AGENTS.md:L147-L149
Useful? React with 👍 / 👎.
| run_state._tool_input_guardrail_results = [ | ||
| *accepted_tool_input_guardrail_results, | ||
| *turn_result.tool_input_guardrail_results, | ||
| ] | ||
| run_state._tool_output_guardrail_results = [ | ||
| *accepted_tool_output_guardrail_results, | ||
| *turn_result.tool_output_guardrail_results, |
There was a problem hiding this comment.
When streaming resumes the sole pending approval, the step becomes NextStepRunAgain, so the conditional below skips _accumulate_tool_guardrail_results and these assignments update only the source RunState. If the resumed Session append then raises, the supported recovery path of calling RunResultStreaming.to_state() copies the stale streamed-result lists rather than these updated lists; the detached checkpoint therefore loses both completed guardrail results, and a retry cannot report them. Publish the results to the streamed checkpoint before the append while retaining the accepted-list deduplication rules.
AGENTS.md reference: AGENTS.md:L147-L149
Useful? React with 👍 / 👎.
Summary
On a resumed turn that resolves one approval while another stays pending, the run publishes that turn's tool guardrail results only inside the
NextStepInterruptionbranch, and that branch sits after the resumedSessionappend:When that append raises, the approved tool has already executed and both its input and output guardrails have already returned, but nothing carries their results into
RunState. The run-level lists die with the raised call, andRunStateonly ever receives guardrail results from a built result (result.py), which never happens on this path. Nothing republishes them later either, because the recovery resume does not re-execute the tool. So the results are gone permanently: absent from the checkpoint, from its serialization, and from the finalRunResult.Measured with an allow-only input guardrail and an allow-only output guardrail on the approved tool, across sync and streamed and across a live state and a JSON round trip.
ranis how many times each guardrail actually executed:The change publishes them next to the other resumed state updates, before the append, in both
run.pyandrun_loop.py. Both paths already seed their run-level accumulators fromRunStateonce at run start, before the turn loop, so writing the turn's results into state mid-turn cannot double count. Confirmed on the clean path, which still reports(1, 1)and not(2, 2)in both sync and streamed.This is the second defect reported in #4646. It is independent of #4650: different root cause, different files, and the two diffs do not touch the same functions. #4650 makes the durable
Sessionrecover the dropped append; this one makes the completed guardrail results survive the same failure. Either can land without the other.Test plan
tests/test_run_impl_resume_paths.py::test_failed_resumed_append_keeps_completed_tool_guardrail_results, parametrized over streamed and non streamed and over a live state and a JSON round trip. It pauses on two approval-gated calls in one response, approves only the first, fails the resumed append, then asserts the checkpoint and its serialization each retain one input and one output guardrail result, and that the final result still reports them after the second approval is rejected.Verified it fails without the source change by reverting
src/and rerunning: all four cases fail..agents/skills/code-change-verification/scripts/run.shpasses end to end: format, lint, typecheck and the full test suite.Issue number
Partially addresses #4646
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PRSecond half of the same issue, split out because the root cause is a different one and I did not want to bury two fixes in one diff. I'm a freshman in college and still learning this part of the runner, so the thing I'd most like checked is my claim that publishing before the append cannot double count. I reasoned it from the accumulators being seeded once at run start and then verified the clean path stays at one result each, but you know the ordering here far better than I do.