fix(approvals): consult stored status before needs_approval on execute#3937
fix(approvals): consult stored status before needs_approval on execute#3937hsusul wants to merge 2 commits into
Conversation
Resume and sticky decisions were re-evaluating dynamic needs_approval checkers before reading resolved approval status, so a checker that raises or changes result could abort an already-approved tool call.
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for the contribution. The underlying issue is valid, and checking stored approval status before invoking a dynamic needs_approval policy is the correct contract.
Before merging, please add failing-on-base regression coverage for each independently changed execution path: function, shell, custom, apply_patch, and Realtime. At minimum, cover the safety-relevant sticky-rejection case where always_reject=True has been recorded and the callable later returns False; the tool must remain rejected and its executor must not run. A parameterized test for the Runner paths plus a focused Realtime test is sufficient.
The implementation direction otherwise looks sound, and no documentation or public API changes are required.
Add parameterized Runner regressions for function/shell/custom/apply_patch and a Realtime case where always_reject wins over a later needs_approval False result without re-entering the checker or running the executor.
|
Thanks for the review! I’ve added the requested regression coverage for all independently changed execution paths. The Runner coverage is parameterized across function, shell, custom, and apply_patch, and I added a focused Realtime test. Each case verifies that a previously recorded always_reject=True decision remains authoritative when the later dynamic needs_approval callable returns False, and that neither the checker nor the underlying executor is invoked. I also verified that the five new cases fail against the pre-fix execute-path behavior and pass with this change. Formatting, linting, type checking, and the full test suite are passing. |
![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: 9326e53b93
ℹ️ 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".
| needs_approval = await self._function_needs_approval(function_tool, tool_call) | ||
| if self._closing or self._closed: | ||
| return None | ||
| if not needs_approval: | ||
| return True |
There was a problem hiding this comment.
With the default async_tool_calls=True, two calls can evaluate approval concurrently: call B can read None here and suspend in a slow checker while call A becomes pending and is rejected with always=True. If B's checker then returns False, this path immediately returns True and executes B despite the newly stored sticky rejection. Re-read get_approval_status after the await, before either bypassing approval or creating another pending request, so concurrent decisions remain authoritative.
AGENTS.md reference: AGENTS.md:L106-L106
Useful? React with 👍 / 👎.
2 participants
Summary
This pull request fixes HITL tool execution so resolved approval status is checked before dynamic
needs_approvalcheckers run.On resume (and for sticky approve/reject decisions), tool execution previously awaited
needs_approvalagain before reading stored status. A checker that raises, hits the network, or returns a different result could abort or bypass an already-approved/rejected call. Planning helpers already short-circuited correctly (#3229 / #3259); the execute path for function tools, shell/custom/apply_patch actions, and realtime tool approval did not.Changes:
get_approval_statusfirst in_maybe_execute_tool_approval, shell/custom/apply_patchAction.execute, and realtime_maybe_request_tool_approvalneeds_approvalwhen status is still unresolvedTest plan
test_execute_path_skips_needs_approval_checker_when_status_resolvedmainand passed after the fix.agents/skills/code-change-verification/scripts/run.sh(format, lint, typecheck, tests)Issue number
N/A — previously unreported; completes the execute-path half of the contract fixed for planning in #3229 / #3259.
Checks
.agents/skills/code-change-verification/scripts/run.sh