Preserve review targets and scope misalignment continuation metadata (#51217)

## What changed

- Carry opaque `review_target` values through misalignment errors and expose optional `reviewTarget` in the app-server protocol and generated clients. Preserve the value verbatim and redact it from debug output.
- Keep target-based `misalignment_override` metadata on turn requests, preserve it across steering updates, and exclude it from compaction and external MCP metadata. Reject overrides from global Responses metadata while retaining legacy timestamp behavior for per-turn overrides.

## Testing

Extend coverage for error propagation, debug and rollout redaction, steering preservation, and metadata filtering. Verify that automatic compaction omits the review target while turn requests before and after compaction retain it.

GitOrigin-RevId: 8c1abb076a206ff5927a4acc31bd25e3efc2bff9
This commit is contained in:
yunhan-wei
2026-10-06 01:26:11 +00:00
committed by copyberry
parent a811c72969
commit 4171362e44
34 changed files with 279 additions and 23 deletions
@@ -3049,6 +3049,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -15455,6 +15455,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -11467,6 +11467,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -165,6 +165,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -832,6 +832,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -936,6 +936,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -858,6 +858,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -858,6 +858,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -858,6 +858,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -952,6 +952,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -858,6 +858,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -936,6 +936,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -858,6 +858,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -832,6 +832,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -858,6 +858,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -832,6 +832,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -832,6 +832,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -832,6 +832,13 @@
"null"
]
},
"reviewTarget": {
"description": "Opaque server-issued block target. Presence alone does not enable target-based continuation.",
"type": [
"string",
"null"
]
},
"steer": {
"anyOf": [
{
@@ -15,4 +15,8 @@ detailedExplanation: string | null,
/**
* Instruction to submit as the next turn's user input if continuation is confirmed.
*/
steer: MisalignmentSteer | null, };
steer: MisalignmentSteer | null,
/**
* Opaque server-issued block target. Presence alone does not enable target-based continuation.
*/
reviewTarget?: string | null, };
+3 -1
View File
@@ -2410,7 +2410,9 @@ mod tests {
== Path::new("v2/GetAccountTokenUsageResponse.ts")
&& field_prefix.trim() == "threadUsage?")
|| (path == Path::new("v2/McpServerOauthLoginCompletedNotification.ts")
&& field_prefix.trim() == "loginId?");
&& field_prefix.trim() == "loginId?")
|| (path == Path::new("v2/MisalignmentErrorDetails.ts")
&& field_prefix.trim() == "reviewTarget?");
if field_prefix.chars().rev().find(|c| !c.is_whitespace()) == Some('?')
&& !allow_optional_nullable
&& !legacy_optional_nullable_field
@@ -442,6 +442,9 @@ pub struct MisalignmentErrorDetails {
pub detailed_explanation: Option<String>,
/// Instruction to submit as the next turn's user input if continuation is confirmed.
pub steer: Option<MisalignmentSteer>,
/// Opaque server-issued block target. Presence alone does not enable target-based continuation.
#[ts(optional = nullable)]
pub review_target: Option<String>,
}
impl fmt::Debug for MisalignmentErrorDetails {
@@ -454,6 +457,7 @@ impl fmt::Debug for MisalignmentErrorDetails {
&self.detailed_explanation.is_some(),
)
.field("has_steer", &self.steer.is_some())
.field("has_review_target", &self.review_target.is_some())
.finish()
}
}
@@ -464,6 +468,7 @@ impl From<CoreMisalignmentErrorDetails> for MisalignmentErrorDetails {
error_type: value.error_type,
detailed_explanation: value.detailed_explanation,
steer: value.steer.map(Into::into),
review_target: value.review_target,
}
}
}
@@ -490,6 +490,10 @@ async fn turn_steer_updates_client_metadata_on_follow_up_responses_request_v2()
let start_metadata = HashMap::from([
("fiber_run_id".to_string(), "fiber-start-123".to_string()),
("source".to_string(), "initial-source".to_string()),
(
"misalignment_override".to_string(),
r#"{"review_target":"RB._~:-opaque"}"#.to_string(),
),
]);
let turn_req = mcp
.send_turn_start_request(TurnStartParams {
@@ -519,6 +523,10 @@ async fn turn_steer_updates_client_metadata_on_follow_up_responses_request_v2()
("fiber_run_id".to_string(), "fiber-steer-456".to_string()),
("origin".to_string(), "gaas".to_string()),
("source".to_string(), "steer-source".to_string()),
(
"misalignment_override".to_string(),
r#"{"review_target":"RC"}"#.to_string(),
),
]);
let steer_req = mcp
.send_turn_steer_request(TurnSteerParams {
@@ -570,6 +578,10 @@ async fn turn_steer_updates_client_metadata_on_follow_up_responses_request_v2()
assert_eq!(second_metadata["turn_id"].as_str(), Some(turn_id.as_str()));
assert_eq!(second_metadata["turn_trigger"].as_str(), Some("user"));
assert_eq!(second_metadata["source"].as_str(), Some("steer-source"));
assert_eq!(
second_metadata["misalignment_override"],
first_metadata["misalignment_override"],
);
let event = wait_for_analytics_event(&server, DEFAULT_READ_TIMEOUT, "codex_turn_event").await?;
assert_eq!(
@@ -24,6 +24,7 @@ use wiremock::ResponseTemplate;
const MESSAGE: &str = "This request violated the misalignment policy.";
const EXPLANATION: &str = "The agent attempted to transfer your files externally.";
const STEER: &str = "Continue without transferring the user's files externally.";
const REVIEW_TARGET: &str = "RB._~:-opaque";
#[tokio::test]
async fn streamed_policy_violation_completes_turn_with_typed_terminal_error() -> Result<()> {
@@ -104,6 +105,7 @@ async fn streamed_policy_violation_exposes_resumable_misalignment_details() -> R
"misalignment": {
"error_type": "unauthorized_data_transfer",
"detailed_explanation": EXPLANATION,
"review_target": REVIEW_TARGET,
"steer": { "message": STEER }
}
}
@@ -129,6 +131,7 @@ async fn http_403_policy_violation_exposes_resumable_misalignment_details() -> R
"misalignment": {
"error_type": "unauthorized_data_transfer",
"detailed_explanation": EXPLANATION,
"review_target": REVIEW_TARGET,
"steer": { "message": STEER }
}
}
@@ -165,6 +168,7 @@ async fn classification_only_policy_violation_preserves_the_terminal_error() ->
response,
Some(MisalignmentErrorDetails {
error_type: Some("unsafe_activity".to_string()),
review_target: None,
detailed_explanation: None,
steer: None,
}),
@@ -255,6 +259,7 @@ async fn assert_policy_violation_completes_turn_with_typed_terminal_error(
.await?;
assert!(!rollout.contains(EXPLANATION));
assert!(!rollout.contains(STEER));
assert!(!rollout.contains(REVIEW_TARGET));
Ok(())
}
@@ -262,6 +267,7 @@ async fn assert_policy_violation_completes_turn_with_typed_terminal_error(
fn resumable_misalignment() -> MisalignmentErrorDetails {
MisalignmentErrorDetails {
error_type: Some("unauthorized_data_transfer".to_string()),
review_target: Some(REVIEW_TARGET.to_string()),
detailed_explanation: Some(EXPLANATION.to_string()),
steer: Some(MisalignmentSteer {
message: STEER.to_string(),
@@ -387,6 +387,7 @@ fn map_api_error_preserves_misalignment_details_from_403_body() {
"code": "misalignment_policy_violation",
"misalignment": {
"error_type": "unauthorized_data_transfer",
"review_target": " RB/opaque== ",
"detailed_explanation": "The agent attempted an external transfer.",
"steer": { "message": "Do not transfer the user's files." }
}
@@ -413,6 +414,7 @@ fn map_api_error_preserves_misalignment_details_from_403_body() {
misalignment,
&Some(MisalignmentErrorDetails {
error_type: Some("unauthorized_data_transfer".to_string()),
review_target: Some(" RB/opaque== ".to_string()),
detailed_explanation: Some("The agent attempted an external transfer.".to_string()),
steer: Some(codex_protocol::protocol::MisalignmentSteer {
message: "Do not transfer the user's files.".to_string(),
@@ -432,6 +434,7 @@ fn map_api_error_preserves_misalignment_details_from_wrapped_websocket_error() {
"code": "misalignment_policy_violation",
"misalignment": {
"error_type": "future_safety_category",
"review_target": " RB/opaque== ",
"detailed_explanation": "The agent attempted an external transfer.",
"steer": { "message": "Do not transfer the user's files." }
}
@@ -461,6 +464,7 @@ fn map_api_error_preserves_misalignment_details_from_wrapped_websocket_error() {
misalignment,
&Some(MisalignmentErrorDetails {
error_type: Some("future_safety_category".to_string()),
review_target: Some(" RB/opaque== ".to_string()),
detailed_explanation: Some("The agent attempted an external transfer.".to_string()),
steer: Some(codex_protocol::protocol::MisalignmentSteer {
message: "Do not transfer the user's files.".to_string(),
+1
View File
@@ -1251,6 +1251,7 @@ mod tests {
misalignment,
&Some(MisalignmentErrorDetails {
error_type: Some("future_safety_category".to_string()),
review_target: None,
detailed_explanation: Some(
"The agent attempted an external transfer.".to_string()
),
+39 -6
View File
@@ -50,6 +50,10 @@ const REASONING_EFFORT_KEY: &str = "reasoning_effort";
const USER_INPUT_REQUESTED_DURING_TURN_KEY: &str = "user_input_requested_during_turn";
const WORKSPACE_KIND_KEY: &str = "workspace_kind";
fn is_targeted_continuation(value: &str) -> bool {
serde_json::from_str::<Value>(value).is_ok_and(|value| value.get("review_target").is_some())
}
/// Captured execution settings shared by Responses and MCP metadata.
pub(crate) struct ExecutionMetadata<'a> {
pub(crate) model: &'a str,
@@ -273,6 +277,14 @@ impl TurnMetadataState {
execution: ExecutionMetadata<'_>,
) -> Option<serde_json::Value> {
let mut responses_metadata = self.mcp_metadata_template();
if responses_metadata
.extra
.get("misalignment_override")
.is_some_and(|value| is_targeted_continuation(value))
{
// A review target is for Responses admission, not external tools or hooks.
responses_metadata.extra.remove("misalignment_override");
}
execution.apply_to(&mut responses_metadata);
// Never serialize harness-owned tool inventory for external MCP servers.
responses_metadata.tool_namespaces_info = None;
@@ -306,11 +318,22 @@ impl TurnMetadataState {
window_id: String,
request_kind: CodexResponsesRequestKind,
) -> CodexResponsesMetadata {
let mut metadata = self.responses_metadata_template();
if matches!(request_kind, CodexResponsesRequestKind::Compaction(_))
&& metadata
.extra
.get("misalignment_override")
.is_some_and(|value| is_targeted_continuation(value))
{
// Target-based intent belongs to the pending turn, not compaction. Preserve the
// legacy timestamp path until servers can admit pre-turn compaction separately.
metadata.extra.remove("misalignment_override");
}
CodexResponsesMetadata {
installation_id,
window_id,
request_kind: Some(request_kind),
..self.responses_metadata_template()
..metadata
}
}
@@ -364,22 +387,32 @@ impl TurnMetadataState {
&self,
responsesapi_client_metadata: HashMap<String, String>,
) {
*self
let mut metadata = filter_extra_metadata(responsesapi_client_metadata);
let mut current = self
.responsesapi_client_metadata
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner) =
filter_extra_metadata(responsesapi_client_metadata);
.unwrap_or_else(std::sync::PoisonError::into_inner);
if let Some(continuation) = current
.get("misalignment_override")
.filter(|value| is_targeted_continuation(value))
{
// Steering may update other metadata, but cannot replace the reviewed block.
metadata.insert("misalignment_override".to_string(), continuation.clone());
}
*current = metadata;
}
pub(crate) fn set_responses_api_metadata(
&self,
responses_api_metadata: BTreeMap<String, String>,
) {
let mut metadata = filter_extra_metadata(responses_api_metadata);
// Only explicit per-turn metadata can acknowledge a reviewed block.
metadata.remove("misalignment_override");
*self
.responses_api_metadata
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner) =
filter_extra_metadata(responses_api_metadata);
.unwrap_or_else(std::sync::PoisonError::into_inner) = metadata;
}
pub(crate) fn workspace_kind(&self) -> Option<String> {
+37 -10
View File
@@ -1031,8 +1031,12 @@ fn turn_metadata_state_bounds_trigger_only_for_analytics() {
}
}
#[test]
fn turn_metadata_state_overlays_compaction_only_on_compaction_requests() {
#[test_case::test_case(r#"{"review_target":"RB._~:-opaque"}"#, None; "target")]
#[test_case::test_case(r#"{"timestamp":1750000000}"#, Some(r#"{"timestamp":1750000000}"#); "legacy_timestamp")]
fn turn_metadata_state_overlays_compaction_only_on_compaction_requests(
continuation: &str,
compaction_override: Option<&str>,
) {
let temp_dir = TempDir::new().expect("temp dir");
let cwd = temp_dir.path().abs();
let permission_profile = PermissionProfile::read_only();
@@ -1051,14 +1055,32 @@ fn turn_metadata_state_overlays_compaction_only_on_compaction_requests() {
/*auto_review_enabled*/ false,
&model_info_from_slug("gpt-5.4"),
);
state.set_responses_api_metadata(BTreeMap::from([(
"codex_security_surface".to_string(),
"sdk".to_string(),
)]));
state.set_responsesapi_client_metadata(HashMap::from([(
"compaction".to_string(),
"client-supplied".to_string(),
)]));
state.set_responses_api_metadata(BTreeMap::from([
("codex_security_surface".to_string(), "sdk".to_string()),
(
"misalignment_override".to_string(),
r#"{"review_target":"RC"}"#.to_string(),
),
]));
let ordinary_json: Value =
serde_json::from_str(&test_turn_responses_metadata_json(&state, "thread-a:1"))
.expect("json");
assert!(ordinary_json.get("misalignment_override").is_none());
state.set_responsesapi_client_metadata(HashMap::from([
("compaction".to_string(), "client-supplied".to_string()),
(
"misalignment_override".to_string(),
continuation.to_string(),
),
]));
let mcp_metadata = state
.current_meta_value_for_mcp_request(test_mcp_turn_metadata_context())
.expect("MCP metadata");
assert_eq!(
mcp_metadata["misalignment_override"].as_str(),
compaction_override
);
let compact_header = test_compaction_responses_metadata_json(
&state,
@@ -1075,6 +1097,10 @@ fn turn_metadata_state_overlays_compaction_only_on_compaction_requests() {
assert_eq!(compact_json["turn_id"].as_str(), Some("turn-a"));
assert_eq!(compact_json[WINDOW_ID_KEY].as_str(), Some("thread-a:2"));
assert_eq!(compact_json["codex_security_surface"].as_str(), Some("sdk"));
assert_eq!(
compact_json["misalignment_override"].as_str(),
compaction_override
);
assert_eq!(
compact_json["compaction"],
serde_json::json!({
@@ -1088,6 +1114,7 @@ fn turn_metadata_state_overlays_compaction_only_on_compaction_requests() {
let regular_header = test_turn_responses_metadata_json(&state, "thread-a:3");
let regular_json: Value = serde_json::from_str(&regular_header).expect("json");
assert_eq!(regular_json["misalignment_override"], continuation);
assert_eq!(regular_json["request_kind"].as_str(), Some("turn"));
assert_eq!(regular_json[WINDOW_ID_KEY].as_str(), Some("thread-a:3"));
assert_eq!(regular_json["codex_security_surface"].as_str(), Some("sdk"));
+26 -5
View File
@@ -4765,18 +4765,39 @@ async fn snapshot_request_shape_mid_turn_continuation_compaction() {
config.model_context_window = Some(context_window);
config.model_auto_compact_token_limit = Some(limit);
});
let codex = builder.build(&server).await.unwrap().codex;
let codex = builder.build_with_auto_env(&server).await.unwrap().codex;
let continuation = r#"{"review_target":"RB._~:-opaque"}"#;
codex
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
text: FUNCTION_CALL_LIMIT_MSG.into(),
text_elements: Vec::new(),
}]))
.start_or_steer_turn(
TurnInputRequest::user_input(vec![UserInput::Text {
text: FUNCTION_CALL_LIMIT_MSG.into(),
text_elements: Vec::new(),
}])
.with_responses_metadata(Some(std::collections::HashMap::from([(
"misalignment_override".to_string(),
continuation.to_string(),
)]))),
)
.await
.unwrap();
wait_for_event(&codex, |msg| matches!(msg, EventMsg::TurnComplete(_))).await;
for (request, expected) in [
(first_turn_mock.single_request(), Some(continuation)),
(auto_compact_mock.single_request(), None),
(post_auto_compact_mock.single_request(), Some(continuation)),
] {
let metadata: Value = serde_json::from_str(
request.body_json()["client_metadata"]["x-codex-turn-metadata"]
.as_str()
.expect("turn metadata"),
)
.expect("valid turn metadata");
assert_eq!(metadata["misalignment_override"].as_str(), expected);
}
// Assert first request captured expected user message that triggers function call.
let first_request = first_turn_mock.single_request().input();
assert!(
+6
View File
@@ -2040,6 +2040,9 @@ pub struct MisalignmentErrorDetails {
/// Model-visible instruction to submit if the user elects to continue.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub steer: Option<MisalignmentSteer>,
/// Opaque server-issued block target, echoed verbatim only on explicit continuation.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub review_target: Option<String>,
}
impl fmt::Debug for MisalignmentErrorDetails {
@@ -2052,6 +2055,7 @@ impl fmt::Debug for MisalignmentErrorDetails {
&self.detailed_explanation.is_some(),
)
.field("has_steer", &self.steer.is_some())
.field("has_review_target", &self.review_target.is_some())
.finish()
}
}
@@ -5869,6 +5873,7 @@ mod tests {
misalignment: Some(MisalignmentErrorDetails {
error_type: Some("unauthorized_data_transfer".to_string()),
detailed_explanation: Some("Sensitive customer explanation".to_string()),
review_target: Some("sensitive-review-target".to_string()),
steer: Some(MisalignmentSteer {
message: "Sensitive customer steering".to_string(),
}),
@@ -5890,6 +5895,7 @@ mod tests {
let debug = format!("{event:?}");
assert!(!debug.contains("Sensitive customer explanation"));
assert!(!debug.contains("Sensitive customer steering"));
assert!(!debug.contains("sensitive-review-target"));
}
#[test]
@@ -13,6 +13,7 @@ fn policy_error() -> AppServerTurnError {
codex_error_info: Some(AppServerCodexErrorInfo::MisalignmentPolicyViolation),
additional_details: None,
misalignment: Some(MisalignmentErrorDetails {
review_target: Some("RB".to_string()),
error_type: None,
detailed_explanation: Some(
"The proposed action exceeded the request.\n\n".repeat(1_500),
@@ -237,6 +237,7 @@ async fn misalignment_turn_end_discards_history_search_and_question_drafts() {
fn review_details() -> codex_app_server_protocol::MisalignmentErrorDetails {
codex_app_server_protocol::MisalignmentErrorDetails {
error_type: Some("new_category".to_string()),
review_target: None,
detailed_explanation: Some(
"The agent proposed a change outside your instructions.".to_string(),
),
@@ -8763,6 +8763,13 @@ class MisalignmentErrorDetails(BaseModel):
description="Open-ended classification; clients must accept categories added by Responses.",
),
] = None
review_target: Annotated[
str | None,
Field(
alias="reviewTarget",
description="Opaque server-issued block target. Presence alone does not enable target-based continuation.",
),
] = None
steer: Annotated[
MisalignmentSteer | None,
Field(