fix(approvals): skip needs_approval_checker when status resolved in _collect_runs_by_approval#3259
fix(approvals): skip needs_approval_checker when status resolved in _collect_runs_by_approval#3259adityasingh2400 wants to merge 2 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ 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". |
adityasingh2400
force-pushed
the
fix/tp2-audit
branch
from
c9feff2 to
da0d210
Compare
|
This PR is stale because it has been open for 10 days with no activity. |
…collect_runs_by_approval Mirrors openai#3229 for non-function tools. When approval_status is already True or False, _collect_runs_by_approval still invokes the user-supplied needs_approval_checker before deciding. The checker may have side effects (telemetry, network) and a non-UserError exception is swallowed; a UserError would short-circuit even an already-approved call. Move the approval_status is True check above the checker invocation so explicit approve/reject decisions short-circuit cleanly.
adityasingh2400
force-pushed
the
fix/tp2-audit
branch
from
da0d210 to
1b1770d
Compare
|
Rebased onto current main. The change still applies cleanly and the suite is green: ruff format, ruff lint, mypy, and the 52 tests in tests/test_hitl_error_scenarios.py all pass locally. The fix skips the needs_approval_checker call in _collect_runs_by_approval once the approval status is already resolved, which avoids the redundant checker invocation. Marking it fresh after the stale ping, happy to take it out of draft whenever it is useful to review. |
2 participants
Summary
Mirrors #3229 for non-function tools (shell, apply_patch, custom).
_collect_runs_by_approvalinvokes the user-suppliedneeds_approval_checkereven whenapproval_statusis alreadyTrueorFalse. The checker may have side effects (telemetry, network) and a non-UserErrorexception is silently swallowed; aUserErrorwould short-circuit an already-approved call.The parallel function
_select_function_tool_runs_for_resumewas fixed in #3229 with the same reasoning.Fix
Move the
approval_status is Trueearly-return above the checker invocation so explicit approve/reject decisions short-circuit cleanly. Rejected (is False) was already short-circuited.Test plan
test_collect_runs_by_approval_skips_checker_when_status_resolvedconfirms the checker is not invoked for approved/rejected shell calls.tests/test_hitl_error_scenarios.py(51 tests) and related HITL/approval suites pass.