feat: improve Guardian denial recovery#26128
Conversation
Co-authored-by: Codex noreply@openai.com
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: add5728ffd
ℹ️ 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 denied_action.denial_kind != GuardianDenialKind::Soft | ||
| || denied_action.action != event.action |
There was a problem hiding this comment.
Require full payloads for exact denial retries
For soft-denied MCP calls and patch applications, GuardianAssessmentAction only contains the server/tool or touched file list, not the MCP arguments or patch body that Guardian actually reviewed. This equality check therefore treats a later approval for the same tool/files as an exact match even when the payload differs, and the injected approval tells the model the user approved the “exact action”; in those connector/patch scenarios that can let a user approval for one denied payload be reused as authorization context for another. Include the reviewed payload in the stored/compared action, or disable explicit retry for action variants that are only summaries.
Useful? React with 👍 / 👎.
| let Some(denied_action) = self.actions.get(&event.id) else { | ||
| return false; |
There was a problem hiding this comment.
Persist retryable denials across thread replay
When a soft denial is shown from replay or after reopening a thread, the TUI still offers /approve, but this registry is only populated in the live SessionServices for denials produced during the current process. A resumed/recreated session has the historical GuardianAssessmentEvent but no matching guardian_denied_actions entry, so the approval is silently ignored by core while the client reports it was recorded. Either reconstruct/persist these one-use entries from thread history, or avoid offering approval for replayed denials that cannot be claimed.
Useful? React with 👍 / 👎.
| return with_cached_approval( | ||
| &session.services, | ||
| "shell", | ||
| keys, |
There was a problem hiding this comment.
Keep auto-review cache separate from user approvals
This now stores Guardian ApprovedForSession decisions in the same tool_approvals cache that the manual approval path checks. In a session where the user switches between Auto Review and normal user approval, a low-risk Guardian approval can later bypass the user prompt in manual mode (and a prior manual session approval can bypass Guardian in Auto Review mode) for the same command key. Key the cache by reviewer/source or keep Guardian session approvals separate so changing review mode actually changes who reviews the next approval.
Useful? React with 👍 / 👎.
Superseded
This combined PR has been split into independently reviewable changes:
/approveguidancePaired App PR: openai/openai#986632
The core behavior PRs target
mainindependently. #26232 depends only on #26231.