mirror of
https://github.com/openai/codex.git
synced 2026-11-05 16:29:31 +00:00
Clear stale Guardian reviews when turns end (#34371)
## Why Pending Guardian review state could outlive an interrupted or completed turn, leaving stale review details visible during a following goal turn. ## What changed - Clear pending Guardian reviews during both turn completion and finalization. - Restore an active MCP startup header after cleanup, falling back to `Working` when no MCP server is starting. ## Testing Added coverage for interrupted and completed turns, including goal continuation and overlapping MCP startup status. GitOrigin-RevId: a15ed81a5d6005e9294089f4424a8dedbc6e9cb5
This commit is contained in:
1 parent
b6de5b524c
commit
5c18cc0acc
5 files changed
+163
-104
No files matched your search
@@ -21,6 +21,32 @@ fn auto_review_denial_event() -> GuardianAssessmentEvent {
|
||||
}
|
||||
}
|
||||
|
||||
fn guardian_command_event(
|
||||
id: &str,
|
||||
turn_id: &str,
|
||||
command: &str,
|
||||
status: GuardianAssessmentStatus,
|
||||
) -> GuardianAssessmentEvent {
|
||||
let terminal = status != GuardianAssessmentStatus::InProgress;
|
||||
GuardianAssessmentEvent {
|
||||
id: id.to_string(),
|
||||
target_item_id: Some(format!("{id}-target")),
|
||||
turn_id: turn_id.to_string(),
|
||||
started_at_ms: 0,
|
||||
completed_at_ms: terminal.then_some(1),
|
||||
status,
|
||||
risk_level: terminal.then_some(GuardianRiskLevel::High),
|
||||
user_authorization: terminal.then_some(GuardianUserAuthorization::Low),
|
||||
rationale: terminal.then(|| "Would delete important data.".to_string()),
|
||||
decision_source: terminal.then_some(GuardianAssessmentDecisionSource::Agent),
|
||||
action: GuardianAssessmentAction::Command {
|
||||
source: GuardianCommandSource::Shell,
|
||||
command: command.to_string(),
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn auto_review_denials_popup_lists_stored_auto_review_denials() {
|
||||
let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
@@ -525,23 +551,12 @@ async fn guardian_parallel_reviews_render_aggregate_status_snapshot() {
|
||||
("guardian-1", "rm -rf '/tmp/guardian target 1'"),
|
||||
("guardian-2", "rm -rf '/tmp/guardian target 2'"),
|
||||
] {
|
||||
chat.on_guardian_assessment(GuardianAssessmentEvent {
|
||||
id: id.to_string(),
|
||||
target_item_id: Some(format!("{id}-target")),
|
||||
turn_id: "turn-1".to_string(),
|
||||
started_at_ms: 0,
|
||||
completed_at_ms: None,
|
||||
status: GuardianAssessmentStatus::InProgress,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
rationale: None,
|
||||
decision_source: None,
|
||||
action: GuardianAssessmentAction::Command {
|
||||
source: GuardianCommandSource::Shell,
|
||||
command: command.to_string(),
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
},
|
||||
});
|
||||
chat.on_guardian_assessment(guardian_command_event(
|
||||
id,
|
||||
"turn-1",
|
||||
command,
|
||||
GuardianAssessmentStatus::InProgress,
|
||||
));
|
||||
}
|
||||
|
||||
let rendered = render_bottom_popup(&chat, /*width*/ 72);
|
||||
@@ -556,57 +571,24 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial()
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
chat.on_task_started();
|
||||
|
||||
chat.on_guardian_assessment(GuardianAssessmentEvent {
|
||||
id: "guardian-1".to_string(),
|
||||
target_item_id: Some("guardian-1-target".to_string()),
|
||||
turn_id: "turn-1".to_string(),
|
||||
started_at_ms: 0,
|
||||
completed_at_ms: None,
|
||||
status: GuardianAssessmentStatus::InProgress,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
rationale: None,
|
||||
decision_source: None,
|
||||
action: GuardianAssessmentAction::Command {
|
||||
source: GuardianCommandSource::Shell,
|
||||
command: "rm -rf '/tmp/guardian target 1'".to_string(),
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
},
|
||||
});
|
||||
chat.on_guardian_assessment(GuardianAssessmentEvent {
|
||||
id: "guardian-2".to_string(),
|
||||
target_item_id: Some("guardian-2-target".to_string()),
|
||||
turn_id: "turn-1".to_string(),
|
||||
started_at_ms: 0,
|
||||
completed_at_ms: None,
|
||||
status: GuardianAssessmentStatus::InProgress,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
rationale: None,
|
||||
decision_source: None,
|
||||
action: GuardianAssessmentAction::Command {
|
||||
source: GuardianCommandSource::Shell,
|
||||
command: "rm -rf '/tmp/guardian target 2'".to_string(),
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
},
|
||||
});
|
||||
chat.on_guardian_assessment(GuardianAssessmentEvent {
|
||||
id: "guardian-1".to_string(),
|
||||
target_item_id: Some("guardian-1-target".to_string()),
|
||||
turn_id: "turn-1".to_string(),
|
||||
started_at_ms: 0,
|
||||
completed_at_ms: Some(1),
|
||||
status: GuardianAssessmentStatus::Denied,
|
||||
risk_level: Some(GuardianRiskLevel::High),
|
||||
user_authorization: Some(GuardianUserAuthorization::Low),
|
||||
rationale: Some("Would delete important data.".to_string()),
|
||||
decision_source: Some(GuardianAssessmentDecisionSource::Agent),
|
||||
action: GuardianAssessmentAction::Command {
|
||||
source: GuardianCommandSource::Shell,
|
||||
command: "rm -rf '/tmp/guardian target 1'".to_string(),
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
},
|
||||
});
|
||||
chat.on_guardian_assessment(guardian_command_event(
|
||||
"guardian-1",
|
||||
"turn-1",
|
||||
"rm -rf '/tmp/guardian target 1'",
|
||||
GuardianAssessmentStatus::InProgress,
|
||||
));
|
||||
chat.on_guardian_assessment(guardian_command_event(
|
||||
"guardian-2",
|
||||
"turn-1",
|
||||
"rm -rf '/tmp/guardian target 2'",
|
||||
GuardianAssessmentStatus::InProgress,
|
||||
));
|
||||
chat.on_guardian_assessment(guardian_command_event(
|
||||
"guardian-1",
|
||||
"turn-1",
|
||||
"rm -rf '/tmp/guardian target 1'",
|
||||
GuardianAssessmentStatus::Denied,
|
||||
));
|
||||
|
||||
assert_eq!(
|
||||
chat.status_state.current_status.header,
|
||||
@@ -617,3 +599,52 @@ async fn guardian_parallel_reviews_keep_remaining_review_visible_after_denial()
|
||||
Some("rm -rf '/tmp/guardian target 2'".to_string())
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_cleanup_drops_stale_reviews_and_restores_mcp_status() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
chat.set_mcp_startup_expected_servers(["alpha".to_string()]);
|
||||
handle_turn_started(&mut chat, "turn-1");
|
||||
chat.on_mcp_server_status_updated(McpServerStatusUpdatedNotification {
|
||||
thread_id: None,
|
||||
name: "alpha".to_string(),
|
||||
status: McpServerStartupState::Starting,
|
||||
error: None,
|
||||
failure_reason: None,
|
||||
});
|
||||
chat.on_guardian_assessment(guardian_command_event(
|
||||
"stale-review",
|
||||
"turn-1",
|
||||
"rm -rf '/tmp/stale-review'",
|
||||
GuardianAssessmentStatus::InProgress,
|
||||
));
|
||||
handle_turn_interrupted(&mut chat, "turn-1");
|
||||
|
||||
assert!(chat.status_state.pending_guardian_review_status.is_empty());
|
||||
assert_eq!(
|
||||
chat.status_state.current_status.header,
|
||||
"Booting MCP server: alpha"
|
||||
);
|
||||
|
||||
handle_turn_started(&mut chat, "turn-2");
|
||||
chat.on_guardian_assessment(guardian_command_event(
|
||||
"current-review",
|
||||
"turn-2",
|
||||
"rm -rf '/tmp/current-review'",
|
||||
GuardianAssessmentStatus::InProgress,
|
||||
));
|
||||
|
||||
let rendered = render_bottom_popup(&chat, /*width*/ 72);
|
||||
assert_chatwidget_snapshot!(
|
||||
"guardian_goal_continuation_drops_stale_reviews",
|
||||
normalize_snapshot_paths(rendered)
|
||||
);
|
||||
|
||||
handle_turn_completed(&mut chat, "turn-2", /*duration_ms*/ None);
|
||||
assert!(chat.status_state.pending_guardian_review_status.is_empty());
|
||||
assert_eq!(
|
||||
chat.status_state.current_status.header,
|
||||
"Booting MCP server: alpha"
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user