fix: defer session save until after output guardrails - #3998
Conversation
Non-streamed runs were persisting final-turn assistant items before output guardrails ran, so a tripwire left rejected output in the session while streamed runs correctly deferred persistence.
![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: aa7c6b0a05
ℹ️ 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".
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Before merge, please preserve resumed-session persistence. When a RunState has _current_turn_persisted_item_count > 0, this head skips the eager final save and then save_turn_items_if_needed() returns without appending the accepted final items.
Please route the post-guardrail save through the existing resumed-turn save path using items_to_save_turn, and update the persisted count. Add a regression test that saves a completed streamed turn, resumes it non-stream with a passing output guardrail, and verifies that the accepted final assistant item is persisted exactly once while the earlier items remain unchanged.
When _current_turn_persisted_item_count is already positive, route the post-guardrail FinalOutput save through save_resumed_turn_items so accepted assistant items are appended instead of being skipped.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 564e67d5be
ℹ️ 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".
| if items_to_save_turn and not isinstance( | ||
| turn_result.next_step, NextStepFinalOutput | ||
| ): |
There was a problem hiding this comment.
When a RunState resumes a NextStepInterruption and the approved tool produces final output (for example, a needs_approval tool with tool_use_behavior="stop_on_first_tool"), execution takes the earlier resolve_interrupted_turn path, which persists turn_session_items before running output guardrails. This deferral therefore covers only the ordinary run-loop path, so a tripwire still leaves the rejected tool output in the session for resumed non-streamed runs, unlike the streamed path; defer the resume-path save until its output guardrails pass as well.
AGENTS.md reference: AGENTS.md:L118-L118
Useful? React with 👍 / 👎.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
3 participants
Summary
Non-streamed
Runner.run/Runner.run_syncpersisted final-turn session items before output guardrails ran. When anOutputGuardrailTripwireTriggeredexception was raised, the rejected assistant message remained in the session. Streamed runs already deferred persistence until after successful output guardrails, so the two paths disagreed.This pull request fixes that by deferring non-stream final-output session persistence until after output guardrails succeed, matching the streamed path and the session-persistence contract (preserve accepted user input; exclude invalidated assistant output). Related precedent: input-guardrail session rollback in #1840 / #1843.
Affected component:
AgentRunnernon-stream session persistence (src/agents/run.py)Root cause: For
NextStepFinalOutput, turn items were saved eagerly in the per-turn persistence block, then output guardrails ran afterward. A tripwire aborted the run after the rejected assistant item was already stored.Implementation: Skip the early per-turn save when
next_stepisNextStepFinalOutput. Persist those items via the existing post-guardrailsave_turn_items_if_neededpath (using the sameitems_to_save_turnlist). Non-final turns (tools, handoffs, interruptions) still save eagerly.Why minimal: One condition change plus reusing the existing success-path save. No public API changes. Resume/HITL interruption persistence paths are intentionally unchanged.
Non-goals: Realtime guardrails, tool-output guardrails, changing interruption/resume persistence semantics, docs-only updates.
Test plan
SimpleListSessionregression tests for:test_session_persists_only_new_step_itemsmonkeypatch to cover the deferred save path throughagent_runner_helpersuv run pytestfocused regression tests (pass; repeated 3× for stream/non-stream)bash .agents/skills/code-change-verification/scripts/run.sh(make format,make lint,make typecheck,make tests) — all passedIssue number
Related incomplete coverage from #1840 (input guardrails only). No new issue opened; submitting a focused fix PR directly.
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR