fix(approvals): skip needs_approval_checker when status already resolved#3229
Merged
seratch merged 1 commit intoopenai:mainfrom May 8, 2026
Merged
Conversation
In `_select_function_tool_runs_for_resume`, the user-supplied `needs_approval_checker` was awaited unconditionally for every run, even when the approval status was already True or False. The result was discarded for resolved decisions, so the call was wasted work — but more importantly, if the checker raised, the rejection branch (`record_rejection`) was never reached and the rejection was lost. Reorder so the checker only runs when the approval state is None, matching the pattern used in `_collect_runs_by_approval`.
Draft
3 tasks
Member
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In
_select_function_tool_runs_for_resume, the user-suppliedneeds_approval_checkerwas awaited unconditionally for every run on the resume path, including runs whose approval status was already resolved (True/False).The checker's result is unused on those branches, so the eager
awaitwas wasted work — but more importantly, if a user-supplied checker raises (which is plausible for checkers that consult external state), the exception propagates out before the loop can reachrecord_rejection, silently dropping the rejection record for an explicitly-rejected call.The corresponding helper
_collect_runs_by_approvalalready orders these checks correctly (status checks first, thenneeds_approval_checker). This change brings_select_function_tool_runs_for_resumein line with that pattern: the checker is only awaited whenapproval_status is None.Changes
src/agents/run_internal/tool_planning.py: deferawait needs_approval_checker(run)until after theapproval_status is True/Falsebranches. Drop the unreachable trailingselected.append(run)sinceapproval_statusisbool | Noneand all three cases are covered.tests/test_hitl_error_scenarios.py: addtest_resume_skips_needs_approval_checker_when_status_resolved— a checker that raisesAssertionErroris not invoked for either pre-approved or pre-rejected calls, and the pre-rejected call still hitsrecord_rejection.Test plan
pytest tests/test_hitl_error_scenarios.py tests/test_run_internal_approvals.py(58 passed)pytest tests/ -k "approval or hitl or resume"(190 passed, 0 failed)pytest tests/test_run.py tests/test_run_step_processing.py tests/test_run_step_execution.py tests/test_run_impl_resume_paths.py tests/test_run_context_approvals.py tests/test_run_state.py tests/test_run_internal_approvals.py tests/test_run_internal_items.py(343 passed)