fix(run): keep the final turn in the session when a streamed output guardrail errors - #4271
fix(run): keep the final turn in the session when a streamed output guardrail errors#4271LeSingh1 wants to merge 1 commit into
Conversation
seratch
left a comment
There was a problem hiding this comment.
Please add a regression test that exercises the public cancellation path: block an async output guardrail, call RunResultStreaming.cancel(), exhaust the stream, then start another run with the same session and assert that the tool side effect occurred only once. The current test raises CancelledError directly and does not prove ownership or persistence behavior under an actual cancellation request.
13f5176 to
88c35a3
Compare
|
Added in 88c35a3 — It uses the public path you described: the output guardrail parks on an For the side-effect half, the tool increments a counter and a second run over the same session (same agent, guardrail removed) asserts Teeth-checked by restoring I left the original parametrized test in place since it still covers the guardrail-raises case, which |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88c35a3583
ℹ️ 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".
seratch
left a comment
There was a problem hiding this comment.
Thanks for adding the public-path coverage. The ordinary guardrail-error half is valid, but the current head also changes the released semantics of cancel(): default immediate mode cancels outstanding work, while after_turn is the mode that finishes the turn and saves session state. Catching CancelledError and awaiting save_items() means stream_events() can remain blocked on an arbitrary session backend after immediate cancellation.
Please narrow the second handler to ordinary Exception, excluding asyncio.CancelledError. Keep the streamed/non-streamed regression for non-tripwire guardrail errors, and remove or replace tests that require default cancellation or a guardrail-raised CancelledError to persist the full turn. Add focused coverage showing that immediate cancellation during a parked output guardrail remains prompt and does not start a final-turn session write, with finally cleanup for spawned tasks. After that, this should be ready for re-review.
…uardrail errors The non-streamed path distinguishes two failure modes. A tripwire means the output was judged undeliverable, so only the already-committed tool items are persisted. A guardrail error or a cancellation leaves the verdict unknown, so the completed final turn is persisted whole and stays replayable. `_finalize_streamed_final_output` collapsed both into one `except Exception` and applied the tripwire-only retention to every failure, and because `asyncio.CancelledError` is a `BaseException` it caught nothing at all there. For a `stop_on_first_tool` turn a raising guardrail left the session as `user, function_call, function_call_output` instead of `user, message, function_call, function_call_output`, and a cancelled guardrail left only `user`, dropping the record of a tool that had already run. Split the handler so streamed runs follow the same rule.
88c35a3 to
37e545a
Compare
|
Done in 37e545a — you were right, and the test now proves it. The second handler is narrowed to New It has teeth in the direction you cared about — restoring One correction to my first attempt at this test: I initially blocked all session writes and it hung on 379 passed across the runner, cancel, guardrail and soft-cancel suites. mypy clean on both touched files; the 42 errors in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37e545a4b7
ℹ️ 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".
| if retained_items: | ||
| await save_items(retained_items, response_id, store_setting) | ||
| raise | ||
| except Exception: |
There was a problem hiding this comment.
Preserve non-immediate guardrail cancellation parity
When an output guardrail is cancelled for any reason other than the default result.cancel() path, such as a guardrail awaiting a subtask that is cancelled while _cancel_mode is still "none" or "after_turn", this except Exception branch is skipped. The run-loop task then finishes as cancelled and stream_events() treats cancelled tasks as non-errors, so the caller loses the guardrail failure and the completed final turn is not persisted, while the non-streamed path catches (Exception, asyncio.CancelledError) and saves it. Fresh evidence: the current patch explicitly leaves CancelledError outside the new save branch; gate the CancelledError save only when _cancel_mode == "immediate".
AGENTS.md reference: AGENTS.md:L124-L124
Useful? React with 👍 / 👎.
The non-streamed path distinguishes two failure modes. A tripwire means the output was judged undeliverable, so only the already-committed tool items are persisted. A guardrail error or a cancellation leaves the verdict unknown, so the completed final turn is persisted whole and stays replayable — see the comment on
except (Exception, asyncio.CancelledError)inrun.py._finalize_streamed_final_outputcollapsed both into oneexcept Exceptionand applied the tripwire-only retention to every failure. Becauseasyncio.CancelledErroris aBaseException, it caught nothing at all there.For a
stop_on_first_toolturn, a raising guardrail leaves the streamed session asuser, function_call, function_call_outputinstead ofuser, message, function_call, function_call_output; a cancelled guardrail leaves onlyuser, dropping the record of a tool that had already run, so the next run re-issues the side effect.Split the handler so streamed runs follow the same rule. Follow-up to #4148, which added the streamed retention for the tripwire case only. Test parametrized over streamed/non-streamed and error/cancellation; both streamed variants fail before the fix.