Bypass review for always-allow MCP tools in auto-review#20069
Conversation
68c966f to
98778dd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98778ddf41
ℹ️ 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".
98778dd to
ecfb989
Compare
| if matches!( | ||
| approval_policy, | ||
| AskForApproval::OnRequest | AskForApproval::Granular(_) | ||
| ) && context.approvals_reviewer == Some(ApprovalsReviewer::AutoReview) |
There was a problem hiding this comment.
hmm do we need this line? && context.approvals_reviewer == Some(ApprovalsReviewer::AutoReview)
should approve in the tool context just mean approve whether it's a human or auto-reviewer? cc @mzeng-openai
There was a problem hiding this comment.
oh, is it due to this behavior?
Preserved existing tests that verify ARC can still block always-allow MCP tools outside guardian auto-review mode.
There was a problem hiding this comment.
That's my read as well. I'd get feedback from @fouad-openai and see if we can get rid of this as well as a follow up, but not blocking this change.
There was a problem hiding this comment.
[codex] Yes, that guard is intentional for this PR scope. The requested behavior is to bypass ARC only for always-allow tools when approvals are routed to guardian auto-review; non-auto-review approval modes should preserve the existing ARC path so ARC can still block. I agree the broader policy could be revisited as a follow-up with @fouad-openai.
706e57c to
e568240
Compare
Why
When an MCP or app tool is configured with approval mode
approve(always allow), users expect that decision to be authoritative. In guardian auto-review mode, ARC could still returnask-user, which then routed the approval question into guardian with the ARC reason as context. That meant a tool explicitly configured as always allowed still went through both safety monitors before running.This change keeps the existing ARC behavior for non-auto-review sessions, but avoids the ARC-to-guardian sequence when
approvals_reviewer = auto_reviewand the tool approval mode isapprove.What changed
approval_mode == approveandapprovals_reviewer == auto_review.Verification
cargo test -p codex-core --lib mcp_tool_call