mirror of
https://github.com/openai/codex.git
synced 2026-10-29 19:08:43 +00:00
Include terminal launch commands in Guardian stdin reviews (#57758)
## Why Guardian's `write_stdin` approval requests lacked the command used to launch the receiving terminal, leaving reviews without that context. ## What changed - Carry `original_command` from the terminal's launch command into the approval request and Guardian action JSON. This describes the original launch, not the terminal's current state. - Exclude `original_command` from the 8,000-byte stdin approval size check so launch context does not consume the budget for input, permission details, and the approval reason. Launch context remains subject to Guardian's normal request budget. ## Testing Extend serialization coverage to check `original_command` and the strict stdin review integration test to verify that a launch command longer than 8,000 bytes reaches Guardian intact. GitOrigin-RevId: cc765a6bf80d080baf94c959eafa6227b2889e1f
This commit is contained in:
@@ -195,7 +195,6 @@ impl From<&Arc<TurnContext>> for GuardianReviewContext {
|
||||
#[cfg(test)]
|
||||
use codex_guardian_reviewer::guardian_output_schema;
|
||||
|
||||
pub(crate) use approval_request::format_guardian_action_pretty;
|
||||
#[cfg(test)]
|
||||
use approval_request::guardian_assessment_action;
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -653,6 +653,7 @@ async fn approval_permissions_use_the_owning_environment() -> anyhow::Result<()>
|
||||
approval_id: "stdin-secondary".to_string(),
|
||||
environment_id: "secondary".to_string(),
|
||||
process_id: 42,
|
||||
original_command: "bash".to_string(),
|
||||
input: "cat private/secret.txt\n".to_string(),
|
||||
cwd: cwd.clone().into(),
|
||||
tty: true,
|
||||
|
||||
@@ -82,6 +82,7 @@ pub(crate) enum ApprovalAction {
|
||||
approval_id: String,
|
||||
environment_id: String,
|
||||
process_id: i32,
|
||||
original_command: String,
|
||||
input: String,
|
||||
cwd: PathUri,
|
||||
tty: bool,
|
||||
@@ -182,6 +183,7 @@ impl ApprovalAction {
|
||||
approval_id,
|
||||
environment_id,
|
||||
process_id,
|
||||
original_command: _,
|
||||
input,
|
||||
cwd,
|
||||
tty,
|
||||
@@ -337,6 +339,7 @@ impl ApprovalAction {
|
||||
approval_id,
|
||||
environment_id,
|
||||
process_id,
|
||||
original_command,
|
||||
input,
|
||||
cwd,
|
||||
tty,
|
||||
@@ -348,6 +351,7 @@ impl ApprovalAction {
|
||||
approval_id,
|
||||
environment_id,
|
||||
process_id,
|
||||
original_command,
|
||||
input,
|
||||
cwd,
|
||||
tty,
|
||||
|
||||
@@ -934,7 +934,7 @@ impl UnifiedExecProcessManager {
|
||||
"terminal input and permission details are too large to review safely; use a smaller input or start a new terminal with fewer grants".to_string(),
|
||||
))
|
||||
};
|
||||
let reviewed = crate::guardian::format_guardian_action_pretty(
|
||||
let mut reviewed = codex_guardian_reviewer::guardian_approval_request_to_json(
|
||||
&approval
|
||||
.clone()
|
||||
.into_guardian_request(/*exec_command_cwd_convention*/ None)
|
||||
@@ -943,8 +943,14 @@ impl UnifiedExecProcessManager {
|
||||
})?,
|
||||
)
|
||||
.map_err(|_| unreviewable_input_error())?;
|
||||
// Bound the entire serialized action plus its reason, including JSON
|
||||
// escaping. Reject, never execute an unreviewed tail.
|
||||
// Launch context uses Guardian's normal request budget, not the stdin limit.
|
||||
if let Some(fields) = reviewed.as_object_mut() {
|
||||
fields.remove("original_command");
|
||||
}
|
||||
let reviewed =
|
||||
serde_json::to_string_pretty(&reviewed).map_err(|_| unreviewable_input_error())?;
|
||||
// Bound the input and permission details plus their reason, including
|
||||
// JSON escaping. Reject, never execute an unreviewed tail.
|
||||
let oversized =
|
||||
reviewed.len().saturating_add(approval_reason.len()) > MAX_STDIN_APPROVAL_BYTES;
|
||||
let size_check_result = if oversized {
|
||||
|
||||
@@ -276,6 +276,7 @@ impl ProcessEntry {
|
||||
approval_id: context.call_id.clone(),
|
||||
environment_id: self.environment_id.clone(),
|
||||
process_id: self.process_id,
|
||||
original_command: self.hook_command.clone(),
|
||||
input: input.to_string(),
|
||||
cwd: self.cwd.clone(),
|
||||
tty: self.tty,
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
//! Exercises retained terminal grants and strict stdin review through the agent API.
|
||||
|
||||
use anyhow::Context;
|
||||
use anyhow::Result;
|
||||
use codex_core::TurnInputRequest;
|
||||
use codex_core::config::Config;
|
||||
@@ -342,13 +343,14 @@ async fn strict_stdin_review_reaches_guardian_with_sandbox_prompts_disabled() ->
|
||||
extensions.approval_review_contributor(calls.clone());
|
||||
let harness = harness(test_codex().with_extensions(Arc::new(extensions.build()))).await?;
|
||||
let test = harness.test();
|
||||
let original_command = format!("/bin/bash --noprofile --norc # {}", "x".repeat(8_000));
|
||||
let opened = mount_sse_sequence(
|
||||
harness.server(),
|
||||
vec![
|
||||
tool_response(
|
||||
"open",
|
||||
"exec_command",
|
||||
json!({"cmd":"/bin/bash --noprofile --norc", "tty":true, "yield_time_ms":200}),
|
||||
json!({"cmd":original_command, "tty":true, "yield_time_ms":200}),
|
||||
),
|
||||
tool_response(
|
||||
"baseline",
|
||||
@@ -420,13 +422,21 @@ async fn strict_stdin_review_reaches_guardian_with_sandbox_prompts_disabled() ->
|
||||
*calls.0.lock().unwrap(),
|
||||
vec!["strict", "denied", "allowed"]
|
||||
);
|
||||
assert_eq!(
|
||||
requests
|
||||
.iter()
|
||||
.filter(|request| request.body_contains_text("Planned action JSON:"))
|
||||
.count(),
|
||||
2
|
||||
);
|
||||
let original_commands = requests
|
||||
.iter()
|
||||
.filter(|request| request.body_contains_text("Planned action JSON:"))
|
||||
.map(|request| {
|
||||
let text = request.message_input_texts("user").join("\n");
|
||||
let (_, action) = text
|
||||
.rsplit_once("Planned action JSON:\n")
|
||||
.context("planned stdin action")?;
|
||||
let (action, _) = action
|
||||
.split_once("\n>>> APPROVAL REQUEST END")
|
||||
.context("end of planned stdin action")?;
|
||||
Ok(serde_json::from_str::<Value>(action)?["original_command"].clone())
|
||||
})
|
||||
.collect::<Result<Vec<_>>>()?;
|
||||
assert_eq!(original_commands, vec![json!(original_command); 2]);
|
||||
let output = requests.last().unwrap().function_call_output("allowed");
|
||||
assert!(output.to_string().contains("STDIN_REVIEW_OK"), "{output}");
|
||||
Ok(())
|
||||
|
||||
@@ -37,6 +37,8 @@ pub enum GuardianApprovalRequest {
|
||||
approval_id: String,
|
||||
environment_id: String,
|
||||
process_id: i32,
|
||||
/// Command used to launch the receiving terminal, not its current state.
|
||||
original_command: String,
|
||||
input: String,
|
||||
cwd: PathUri,
|
||||
tty: bool,
|
||||
|
||||
@@ -38,6 +38,7 @@ struct ApplyPatchApprovalAction<'a> {
|
||||
struct WriteStdinApprovalAction<'a> {
|
||||
tool: &'static str,
|
||||
session_id: i32,
|
||||
original_command: &'a str,
|
||||
chars: &'a str,
|
||||
cwd: LegacyAppPathString,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
@@ -140,6 +141,7 @@ pub fn guardian_approval_request_to_json(
|
||||
),
|
||||
GuardianApprovalRequest::WriteStdin {
|
||||
process_id,
|
||||
original_command,
|
||||
input,
|
||||
cwd,
|
||||
tty,
|
||||
@@ -149,6 +151,7 @@ pub fn guardian_approval_request_to_json(
|
||||
} => serialize_guardian_action(WriteStdinApprovalAction {
|
||||
tool: "write_stdin",
|
||||
session_id: *process_id,
|
||||
original_command,
|
||||
chars: input,
|
||||
cwd: cwd.clone().into(),
|
||||
sandbox_permissions: *sandbox_permissions,
|
||||
|
||||
@@ -16,7 +16,7 @@ use pretty_assertions::assert_eq;
|
||||
#[test_case::test_case(SandboxPermissions::UseDefault)]
|
||||
#[test_case::test_case(SandboxPermissions::RequireEscalated)]
|
||||
#[test_case::test_case(SandboxPermissions::WithAdditionalPermissions)]
|
||||
fn guardian_write_stdin_preserves_input_and_foreign_cwd(
|
||||
fn guardian_write_stdin_preserves_original_command_input_and_foreign_cwd(
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
) -> serde_json::Result<()> {
|
||||
let cwd = PathUri::parse("file:///C:/workspace").expect("valid executor cwd");
|
||||
@@ -34,6 +34,7 @@ fn guardian_write_stdin_preserves_input_and_foreign_cwd(
|
||||
approval_id: "terminal-write".to_string(),
|
||||
environment_id: "windows-executor".to_string(),
|
||||
process_id: 1000,
|
||||
original_command: "python helper.py --project 'example site'".to_string(),
|
||||
input: input.to_string(),
|
||||
cwd: cwd.clone(),
|
||||
tty: true,
|
||||
@@ -45,6 +46,7 @@ fn guardian_write_stdin_preserves_input_and_foreign_cwd(
|
||||
"tool": "write_stdin",
|
||||
"environment_id": "windows-executor",
|
||||
"session_id": 1000,
|
||||
"original_command": "python helper.py --project 'example site'",
|
||||
"chars": input,
|
||||
"cwd": r"C:\workspace",
|
||||
"sandbox_permissions": sandbox_permissions,
|
||||
|
||||
Reference in New Issue
Block a user