mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Record reviewed actions with security risk scores (#40901)
## What changed - Add optional `call_id` and `action` fields to `SecurityRiskScore` so a persisted score identifies the tool call and bounded action that produced it. - Populate the fields after Guardian V2 classifies an action while leaving fail-closed and legacy scores without provenance. ## Testing - Verify scores preserve action details for direct tool calls and nested code-mode calls. - Verify rollout serialization retains the new fields. GitOrigin-RevId: fce69af37f368cecf1d9bce42bfc38a969e969b7
This commit is contained in:
1 parent
62fb56ee56
commit
d4998d611a
9 files changed
+130
-3
No files matched your search
BIN
Binary file not shown.
@@ -206,6 +206,8 @@ fn ignores_legacy_abort_without_turn_id_and_context_only_records() {
|
|||||||
}));
|
}));
|
||||||
let security_risk = project(RolloutItem::SecurityRiskScore(SecurityRiskScore {
|
let security_risk = project(RolloutItem::SecurityRiskScore(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_string(), 0.92)]),
|
scores: BTreeMap::from([("action_risk".to_string(), 0.92)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
|||||||
@@ -157,6 +157,8 @@ async fn record_initial_history_ignores_security_risk_scores() {
|
|||||||
let user_item = user_message("visible user input");
|
let user_item = user_message("visible user input");
|
||||||
let security_risk = SecurityRiskScore {
|
let security_risk = SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("credential_access".to_string(), 0.92)]),
|
scores: BTreeMap::from([("credential_access".to_string(), 0.92)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
@@ -453,6 +453,8 @@ impl GuardianV2Extension {
|
|||||||
fn record_fail_closed_score(thread_store: &ExtensionData, sampled_at: SystemTime) {
|
fn record_fail_closed_score(thread_store: &ExtensionData, sampled_at: SystemTime) {
|
||||||
let score = SecurityRiskScore {
|
let score = SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 1.0)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 1.0)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: Some(sampled_at.into()),
|
sampled_at: Some(sampled_at.into()),
|
||||||
};
|
};
|
||||||
thread_store.insert_if(score.clone(), |previous| {
|
thread_store.insert_if(score.clone(), |previous| {
|
||||||
@@ -772,6 +774,10 @@ impl GuardianV2Extension {
|
|||||||
};
|
};
|
||||||
let score = SecurityRiskScore {
|
let score = SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), action_risk)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), action_risk)]),
|
||||||
|
call_id: Some(call_id.clone()),
|
||||||
|
action: Some(
|
||||||
|
serde_json::from_str(&planned_action).map_err(|error| error.to_string())?,
|
||||||
|
),
|
||||||
sampled_at: Some(sampled_at.into()),
|
sampled_at: Some(sampled_at.into()),
|
||||||
};
|
};
|
||||||
let accepted =
|
let accepted =
|
||||||
|
|||||||
@@ -259,6 +259,8 @@ fn fail_closed_score_preserves_classification_order() {
|
|||||||
let newest_sampled_at = newer_sampled_at + Duration::from_secs(1);
|
let newest_sampled_at = newer_sampled_at + Duration::from_secs(1);
|
||||||
let newer_score = SecurityRiskScore {
|
let newer_score = SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: Some(newer_sampled_at.into()),
|
sampled_at: Some(newer_sampled_at.into()),
|
||||||
};
|
};
|
||||||
thread_store.insert(newer_score.clone());
|
thread_store.insert(newer_score.clone());
|
||||||
@@ -272,6 +274,8 @@ fn fail_closed_score_preserves_classification_order() {
|
|||||||
GuardianV2Extension::record_fail_closed_score(&thread_store, newest_sampled_at);
|
GuardianV2Extension::record_fail_closed_score(&thread_store, newest_sampled_at);
|
||||||
let fail_closed_score = SecurityRiskScore {
|
let fail_closed_score = SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 1.0)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 1.0)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: Some(newest_sampled_at.into()),
|
sampled_at: Some(newest_sampled_at.into()),
|
||||||
};
|
};
|
||||||
assert!(!thread_store.insert_if(newer_score.clone(), |previous| {
|
assert!(!thread_store.insert_if(newer_score.clone(), |previous| {
|
||||||
@@ -412,6 +416,8 @@ async fn computer_use_only_scores_cannot_approve_other_actions() -> Result<()> {
|
|||||||
thread_store.insert(config);
|
thread_store.insert(config);
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
let progress = thread_store
|
let progress = thread_store
|
||||||
@@ -673,6 +679,25 @@ async fn sample_configured_conversation_history(
|
|||||||
guardian_policy: Option<&str>,
|
guardian_policy: Option<&str>,
|
||||||
guardian_config: &str,
|
guardian_config: &str,
|
||||||
model_defaults: Option<GuardianV2ModelConfig>,
|
model_defaults: Option<GuardianV2ModelConfig>,
|
||||||
|
) -> Result<(serde_json::Value, TestCodex, ExtensionRegistry<Config>)> {
|
||||||
|
sample_configured_conversation_history_with_source(
|
||||||
|
conversation_history,
|
||||||
|
arguments,
|
||||||
|
guardian_policy,
|
||||||
|
guardian_config,
|
||||||
|
model_defaults,
|
||||||
|
ToolCallSource::Direct,
|
||||||
|
)
|
||||||
|
.await
|
||||||
|
}
|
||||||
|
|
||||||
|
async fn sample_configured_conversation_history_with_source(
|
||||||
|
conversation_history: Vec<ResponseItem>,
|
||||||
|
arguments: &str,
|
||||||
|
guardian_policy: Option<&str>,
|
||||||
|
guardian_config: &str,
|
||||||
|
model_defaults: Option<GuardianV2ModelConfig>,
|
||||||
|
source: ToolCallSource,
|
||||||
) -> Result<(serde_json::Value, TestCodex, ExtensionRegistry<Config>)> {
|
) -> Result<(serde_json::Value, TestCodex, ExtensionRegistry<Config>)> {
|
||||||
let thread_server = responses::start_mock_server().await;
|
let thread_server = responses::start_mock_server().await;
|
||||||
let guardian_policy = guardian_policy.map(str::to_owned);
|
let guardian_policy = guardian_policy.map(str::to_owned);
|
||||||
@@ -789,7 +814,7 @@ async fn sample_configured_conversation_history(
|
|||||||
tool_name: &tool_name,
|
tool_name: &tool_name,
|
||||||
payload: &tool_payload,
|
payload: &tool_payload,
|
||||||
conversation_history: Arc::new(conversation_history),
|
conversation_history: Arc::new(conversation_history),
|
||||||
source: ToolCallSource::Direct,
|
source,
|
||||||
})
|
})
|
||||||
.await;
|
.await;
|
||||||
|
|
||||||
@@ -837,6 +862,8 @@ impl GuardianFailureFixture {
|
|||||||
let thread_store = self.test.codex.thread_extension_data();
|
let thread_store = self.test.codex.thread_extension_data();
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
let turn_store = ExtensionData::new("turn-1");
|
let turn_store = ExtensionData::new("turn-1");
|
||||||
@@ -1196,6 +1223,8 @@ max_recent_non_user_entries = 8
|
|||||||
assert_eq!(thread_store.get::<StrictReviewReason>(), None);
|
assert_eq!(thread_store.get::<StrictReviewReason>(), None);
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -1217,6 +1246,8 @@ max_recent_non_user_entries = 8
|
|||||||
);
|
);
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.55)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.55)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -1561,14 +1592,23 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu
|
|||||||
(2, false, 384, true)
|
(2, false, 384, true)
|
||||||
);
|
);
|
||||||
assert!(thread_store.get::<NodeReplReviewEvidence>().is_some());
|
assert!(thread_store.get::<NodeReplReviewEvidence>().is_some());
|
||||||
tokio::time::timeout(Duration::from_secs(5), async {
|
let score = tokio::time::timeout(Duration::from_secs(5), async {
|
||||||
while thread_store.get::<SecurityRiskScore>().is_none() {
|
loop {
|
||||||
|
if let Some(score) = thread_store.get::<SecurityRiskScore>() {
|
||||||
|
return score;
|
||||||
|
}
|
||||||
tokio::task::yield_now().await;
|
tokio::task::yield_now().await;
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
.await?;
|
.await?;
|
||||||
|
assert_eq!(
|
||||||
|
score.action,
|
||||||
|
Some(serde_json::from_str::<serde_json::Value>(action)?)
|
||||||
|
);
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.65)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -1723,6 +1763,8 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
|||||||
score.as_ref(),
|
score.as_ref(),
|
||||||
&SecurityRiskScore {
|
&SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_string(), 1.0)]),
|
scores: BTreeMap::from([("action_risk".to_string(), 1.0)]),
|
||||||
|
call_id: Some("call-1".to_owned()),
|
||||||
|
action: Some(json!({"path": "README.md", "tool": "read_file"})),
|
||||||
sampled_at: score.sampled_at,
|
sampled_at: score.sampled_at,
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
@@ -1761,6 +1803,8 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
|||||||
);
|
);
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_string(), 0.5)]),
|
scores: BTreeMap::from([("action_risk".to_string(), 0.5)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -1777,6 +1821,8 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
|||||||
|
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_string(), 0.49)]),
|
scores: BTreeMap::from([("action_risk".to_string(), 0.49)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -1794,6 +1840,8 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
|||||||
let disabled_thread_store = ExtensionData::new("disabled-thread");
|
let disabled_thread_store = ExtensionData::new("disabled-thread");
|
||||||
disabled_thread_store.insert(SecurityRiskScore {
|
disabled_thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_string(), 0.25)]),
|
scores: BTreeMap::from([("action_risk".to_string(), 0.25)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -1811,6 +1859,57 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
|
async fn contributor_persists_nested_code_mode_action_with_score() -> Result<()> {
|
||||||
|
skip_if_no_network!(Ok(()));
|
||||||
|
|
||||||
|
let (_request, test, _registry) = sample_configured_conversation_history_with_source(
|
||||||
|
Vec::new(),
|
||||||
|
r#"{"path":"README.md"}"#,
|
||||||
|
Some(TEST_GUARDIAN_POLICY),
|
||||||
|
"",
|
||||||
|
/*model_defaults*/ None,
|
||||||
|
ToolCallSource::CodeMode {
|
||||||
|
cell_id: "cell-1".to_owned(),
|
||||||
|
runtime_tool_call_id: "nested-1".to_owned(),
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.await?;
|
||||||
|
test.codex.ensure_rollout_materialized().await;
|
||||||
|
|
||||||
|
let score = tokio::time::timeout(Duration::from_secs(5), async {
|
||||||
|
loop {
|
||||||
|
if let Some(score) = test
|
||||||
|
.codex
|
||||||
|
.load_history(/*include_archived*/ false)
|
||||||
|
.await?
|
||||||
|
.items
|
||||||
|
.into_iter()
|
||||||
|
.find_map(|item| match item {
|
||||||
|
RolloutItem::SecurityRiskScore(score) => Some(score),
|
||||||
|
_ => None,
|
||||||
|
})
|
||||||
|
{
|
||||||
|
return Ok::<_, anyhow::Error>(score);
|
||||||
|
}
|
||||||
|
tokio::task::yield_now().await;
|
||||||
|
}
|
||||||
|
})
|
||||||
|
.await??;
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
score,
|
||||||
|
SecurityRiskScore {
|
||||||
|
scores: BTreeMap::from([("action_risk".to_owned(), 1.0)]),
|
||||||
|
call_id: Some("call-1".to_owned()),
|
||||||
|
action: Some(json!({"path": "README.md", "tool": "read_file"})),
|
||||||
|
sampled_at: score.sampled_at,
|
||||||
|
}
|
||||||
|
);
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn contributor_skips_models_requiring_managed_guardian_review() -> Result<()> {
|
async fn contributor_skips_models_requiring_managed_guardian_review() -> Result<()> {
|
||||||
skip_if_no_network!(Ok(()));
|
skip_if_no_network!(Ok(()));
|
||||||
@@ -1881,6 +1980,8 @@ async fn contributor_skips_models_requiring_managed_guardian_review() -> Result<
|
|||||||
thread_store.insert(model_info);
|
thread_store.insert(model_info);
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -1961,6 +2062,8 @@ async fn contributor_counts_failed_thread_lookups_toward_score_lag() -> Result<(
|
|||||||
.await?;
|
.await?;
|
||||||
thread_store.insert(SecurityRiskScore {
|
thread_store.insert(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 0.25)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
@@ -2451,6 +2554,8 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result<
|
|||||||
fail_closed_score.as_ref(),
|
fail_closed_score.as_ref(),
|
||||||
&SecurityRiskScore {
|
&SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_owned(), 1.0)]),
|
scores: BTreeMap::from([("action_risk".to_owned(), 1.0)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: fail_closed_score.sampled_at,
|
sampled_at: fail_closed_score.sampled_at,
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
|
|||||||
@@ -66,6 +66,8 @@ fn requires_a_strict_nonempty_model_prefix() {
|
|||||||
}
|
}
|
||||||
let security_risk = RolloutItem::SecurityRiskScore(SecurityRiskScore {
|
let security_risk = RolloutItem::SecurityRiskScore(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_string(), 0.92)]),
|
scores: BTreeMap::from([("action_risk".to_string(), 0.92)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
});
|
});
|
||||||
metadata_changed.push(security_risk.clone());
|
metadata_changed.push(security_risk.clone());
|
||||||
|
|||||||
@@ -728,6 +728,8 @@ mod tests {
|
|||||||
RolloutItem::ResponseItem(skill_message.into()),
|
RolloutItem::ResponseItem(skill_message.into()),
|
||||||
RolloutItem::SecurityRiskScore(SecurityRiskScore {
|
RolloutItem::SecurityRiskScore(SecurityRiskScore {
|
||||||
scores: BTreeMap::from([("action_risk".to_string(), 0.92)]),
|
scores: BTreeMap::from([("action_risk".to_string(), 0.92)]),
|
||||||
|
call_id: None,
|
||||||
|
action: None,
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
}),
|
}),
|
||||||
RolloutItem::ResponseItem(subagent_message.clone().into()),
|
RolloutItem::ResponseItem(subagent_message.clone().into()),
|
||||||
|
|||||||
@@ -12,6 +12,12 @@ use std::collections::BTreeMap;
|
|||||||
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, JsonSchema)]
|
#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, JsonSchema)]
|
||||||
pub struct SecurityRiskScore {
|
pub struct SecurityRiskScore {
|
||||||
pub scores: BTreeMap<String, f64>,
|
pub scores: BTreeMap<String, f64>,
|
||||||
|
/// The tool call whose action produced this score, when available.
|
||||||
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
|
pub call_id: Option<String>,
|
||||||
|
/// The bounded tool action that was reviewed to produce this score.
|
||||||
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
|
pub action: Option<serde_json::Value>,
|
||||||
/// When sampling started, if this snapshot was written by a timestamp-aware client.
|
/// When sampling started, if this snapshot was written by a timestamp-aware client.
|
||||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||||
#[schemars(with = "Option<String>")]
|
#[schemars(with = "Option<String>")]
|
||||||
|
|||||||
@@ -445,6 +445,8 @@ async fn load_rollout_items_preserves_security_risk_scores() -> std::io::Result<
|
|||||||
("action_risk".to_string(), 0.76),
|
("action_risk".to_string(), 0.76),
|
||||||
("data_exfiltration".to_string(), 0.31),
|
("data_exfiltration".to_string(), 0.31),
|
||||||
]),
|
]),
|
||||||
|
call_id: Some("call-1".to_owned()),
|
||||||
|
action: Some(serde_json::json!({"path": "README.md", "tool": "read_file"})),
|
||||||
sampled_at: None,
|
sampled_at: None,
|
||||||
};
|
};
|
||||||
let security_risk_item = RolloutItem::SecurityRiskScore(security_risk.clone());
|
let security_risk_item = RolloutItem::SecurityRiskScore(security_risk.clone());
|
||||||
|
|||||||
Reference in new issue
Block a user