Preserve strict MCP auto-review outcomes (#40031)

## What changed

Propagate canonical denial, timeout, and abort responses from strict MCP
auto-review instead of replacing them with a generic decline. This preserves
the reviewer's action and metadata, including denial rationale, while still
failing closed when no canonical decision is available.

Update the fallback message to direct the agent to request explicit user
approval before proceeding.

## Testing

Add unit and app-server integration coverage for strict auto-review denials
and cancellations.

GitOrigin-RevId: d8126ab223ab546b95e2d984f6e4dd540a324a59
This commit is contained in:
Ankush Gupta
2026-08-21 23:08:37 +00:00
committed by copyberry
parent 50ea8fd411
commit 970b7f2ff4
4 changed files with 71 additions and 16 deletions

View File

@@ -101,8 +101,8 @@ const TOOL_NAME: &str = "calendar_confirm_action";
const TOOL_CALL_ID: &str = "call-calendar-confirm";
const NEXT_TURN_TOOL_CALL_ID: &str = "call-calendar-next-turn";
const ELICITATION_MESSAGE: &str = "Allow this request?";
const STRICT_DECLINE_MESSAGE: &str =
"Strict automated review failed. Do not proceed or ask the user for approval.";
const STRICT_DECLINE_MESSAGE: &str = "Automated review of this operation failed. Do not proceed without asking the user for explicit approval.";
const GUARDIAN_DENIAL_RATIONALE: &str = "The calendar action exceeds the user's authorization.";
const OPENAI_FORM_MESSAGE: &str = "Select a template";
const IMAGE_DATA_URL: &str =
"data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciLz4=";
@@ -529,12 +529,14 @@ async fn start_elicitation_services(
if let ElicitationScenario::Strict(strict) = scenario {
let completion = streams.pop().expect("parent model completion");
for approved in strict.review_outcomes() {
let assessment = if *approved {
json!({ "outcome": "allow" })
} else {
json!({ "outcome": "deny", "rationale": GUARDIAN_DENIAL_RATIONALE })
};
streams.push(responses::sse(vec![
responses::ev_response_created("resp-guardian"),
responses::ev_assistant_message(
"msg-guardian",
&json!({ "outcome": if *approved { "allow" } else { "deny" } }).to_string(),
),
responses::ev_assistant_message("msg-guardian", &assessment.to_string()),
responses::ev_completed("resp-guardian"),
]));
}
@@ -1094,17 +1096,31 @@ impl ServerHandler for ElicitationAppsMcpServer {
.map_err(|err| {
rmcp::ErrorData::internal_error(err.to_string(), None)
})?;
let expected = if strict.review_outcomes().get(index) == Some(&true) {
json!({
let expected = match strict.review_outcomes().get(index) {
Some(true) => json!({
"action": "accept",
"content": {},
"_meta": { "approvals_reviewer": "auto_review" },
})
} else {
json!({
}),
Some(false) => json!({
"action": "decline",
"_meta": {
"approvals_reviewer": "auto_review",
"message": format!(
"This action was rejected due to unacceptable risk.\n\
Reason: {GUARDIAN_DENIAL_RATIONALE}\n\
The agent must not attempt to achieve the same outcome via workaround, \
indirect execution, or policy circumvention. \
Proceed only with a materially safer alternative, \
or if the user explicitly approves the action after being informed of the risk. \
Otherwise, stop and request user input."
),
},
}),
None => json!({
"action": "decline",
"_meta": { "message": STRICT_DECLINE_MESSAGE },
})
}),
};
assert_eq!(
serde_json::to_value(result)

View File

@@ -41,8 +41,7 @@ use tokio::sync::oneshot;
static NEXT_ELICITATION_REQUEST_ID: AtomicU64 = AtomicU64::new(0);
const STRICT_AUTO_REVIEW_DECLINE_MESSAGE: &str =
"Strict automated review failed. Do not proceed or ask the user for approval.";
const STRICT_AUTO_REVIEW_DECLINE_MESSAGE: &str = "Automated review of this operation failed. Do not proceed without asking the user for explicit approval.";
#[derive(Debug, Clone)]
pub struct ElicitationReviewRequest {
@@ -278,8 +277,12 @@ impl ElicitationRequestManager {
.await
{
Ok(Some(response))
if response.action == ElicitationAction::Accept
if (response.action == ElicitationAction::Accept
&& response.content == Some(serde_json::json!({}))
|| matches!(
response.action,
ElicitationAction::Decline | ElicitationAction::Cancel
) && response.content.is_none())
&& response
.meta
.as_ref()

View File

@@ -227,6 +227,35 @@ async fn strict_auto_review_respects_explicit_elicitation_denials() {
}
}
#[tokio::test]
async fn strict_auto_review_preserves_guardian_denials_and_cancellations() {
for response in [
ElicitationResponse {
action: ElicitationAction::Decline,
content: None,
meta: Some(json!({
"approvals_reviewer": "auto_review",
"message": "The user has not authorized sending this data. Ask the user for approval.",
})),
},
ElicitationResponse {
action: ElicitationAction::Cancel,
content: None,
meta: Some(json!({ "approvals_reviewer": "auto_review" })),
},
] {
let reviewer = RecordingReviewer::new(Ok(Some(response.clone())));
let (_, events, sender) = elicitation_fixture(
AskForApproval::Never,
PermissionProfile::Disabled,
Some(reviewer.clone()),
);
assert_eq!(send_elicitation(&sender, Some(json!(true))).await, response);
assert_eq!(reviewer.calls.load(Relaxed), 1);
assert!(events.is_empty(), "strict review must not emit an event");
}
}
#[tokio::test]
async fn strict_auto_review_fails_closed_without_a_canonical_decision() {
for marker in ["null", "\"true\"", "1", "{}", "[true]"] {

View File

@@ -807,7 +807,14 @@ async fn review_guardian_mcp_elicitation(
)
.await;
return Ok(matches!(decision, ReviewDecision::Approved).then(|| {
return Ok(matches!(
decision,
ReviewDecision::Approved
| ReviewDecision::Denied { .. }
| ReviewDecision::TimedOut
| ReviewDecision::Abort
)
.then(|| {
mcp_elicitation_response_from_guardian_decision(decision, &turn_context.model_info)
}));
}