Add /auto-review-denials retry approval flow#19058
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 075815efa6
ℹ️ 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".
|
|
or we can do /denials, i think it's more intuitive! thoughts? @etraut-openai @dylan-hurd-oai |
|
I guess I'd lean toward |
fe7d9a3 to
cc47130
Compare
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/tui/src/chatwidget.rs
Line 2362 in 11c469c
on_session_configured_with_display_and_fork_parent_title replaces self.thread_id but never resets recent_auto_review_denials. Since that cache is stored once on ChatWidget, a new thread can inherit denials from the previous thread, and /denials may submit a stale GuardianAssessmentEvent against the wrong thread context.
ℹ️ 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".
|
@codex review |
| content_entry(GuardianTranscriptEntryKind::User, content) | ||
| } | ||
| } | ||
| ResponseItem::Message { role, content, .. } if role == "developer" => { |
There was a problem hiding this comment.
so that auto-review is able to send the override message
There was a problem hiding this comment.
💡 Codex Review
codex/codex-rs/core/src/guardian/prompt.rs
Lines 303 to 304 in 101e973
Manual-approval developer entries are collected, but transcript rendering treats them as generic non-user entries under GUARDIAN_RECENT_ENTRY_LIMIT and message token budget. With enough newer entries, this approval context is dropped, so follow-up guardian reviews can miss the very retry-approval context this change intends to preserve.
ℹ️ 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".
| } | ||
|
|
||
| pub(crate) fn approve_recent_auto_review_denial(&mut self, thread_id: ThreadId, id: String) { | ||
| let Some(event) = self.recent_auto_review_denials.take(&id) else { |
There was a problem hiding this comment.
Keep denied action available until submit succeeds
approve_recent_auto_review_denial removes the selected denial from recent_auto_review_denials before the async SubmitThreadOp is actually sent/acknowledged. If thread_approve_guardian_denied_action fails, the user cannot reselect that denial and must wait for a new denial event. This makes transient app-server failures permanently consume the retry option.
Useful? React with 👍 / 👎.
| #[strum(to_string = "autoreview")] | ||
| AutoReviewDenials, |
There was a problem hiding this comment.
Match slash command token to documented feature name
The change is described as adding /auto-review-denials, but this variant is wired with #[strum(to_string = "autoreview")], making autoreview the serialized/parsed command token. Users invoking /auto-review-denials will not match this command, creating an intent/documentation mismatch for the new flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@won-openai which one are we using here? Let's keep this simple
| Treat this as approval to perform that exact action in the same context in which it was originally requested. | ||
| Do not assume this also authorizes similar operations with different payloads. | ||
|
|
||
| Stored Guardian assessment event JSON: | ||
| {event_json}"#, | ||
| Approved action: | ||
| {approved_action_json}"#, |
There was a problem hiding this comment.
Restore untrusted-data warning for approved action JSON
This developer message now embeds event.action JSON without the previous "untrusted data" guard. Because action fields can contain model/user-controlled text (e.g., command/tool args), that text is now placed in a privileged developer-role prompt and may be followed as instructions, weakening prompt-injection defenses in approval retries.
Useful? React with 👍 / 👎.
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
Approved with a few small things to address
|
|
||
| self.bottom_pane.show_selection_view(SelectionViewParams { | ||
| title: Some("Auto-review Denials".to_string()), | ||
| subtitle: Some("Select one denied action to approve for one retry.".to_string()), |
There was a problem hiding this comment.
Select a denied action to approve. - let's keep this simple for now
| #[strum(to_string = "autoreview")] | ||
| AutoReviewDenials, |
There was a problem hiding this comment.
@won-openai which one are we using here? Let's keep this simple
3fb01fb to
78a4d34
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78a4d346d3
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| let event_json = match serde_json::to_string_pretty(&event) { | ||
| Ok(event_json) => event_json, | ||
| let approved_action = serde_json::json!({ | ||
| "action": &event.action, | ||
| "outcome": "allowed", | ||
| }); |
There was a problem hiding this comment.
Verify denied review ID before injecting approval context
approve_guardian_denied_action only checks event.status == Denied and then turns the client-supplied payload into a developer approval message. A forged/stale GuardianAssessmentEvent can therefore authorize an action that was never actually denied in this thread. Validate event.id (and thread/turn binding) against server-side denied-review state before honoring it.
Useful? React with 👍 / 👎.
| Approved action: | ||
| {approved_action_json}"#, |
There was a problem hiding this comment.
Treat approved action payload as untrusted text
The developer message now inlines {approved_action_json} but no longer says to ignore instructions inside that JSON. Since action fields can contain model/user-controlled strings (commands, args), this can inject instruction-like text into privileged developer context and bias subsequent auto-review decisions.
Useful? React with 👍 / 👎.
Why
Auto-review can deny an action that the user later decides they want to retry. Today there is no TUI surface for selecting a recent denial and sending explicit approval context back into the session, so users have to restate intent manually and the retry can be reviewed without the original denied action context.
This adds a narrow TUI-driven path for approving a recent denied action while still keeping the retry inside the normal auto-review flow.
What Changed
/auto-review-denialsto open a picker of recent denied auto-review actions.Verification
cargo test -p codex-core collect_guardian_transcript_entries_keeps_manual_approval_developer_messagecargo test -p codex-tui auto_review_denialscargo test -p codex-tui approving_recent_denial_emits_structured_core_op_onceNotes
This intentionally keeps retries going through auto-review. The approval signal is context for the exact previously denied action, not a blanket bypass for similar future actions.