mirror of
https://github.com/openai/codex.git
synced 2026-09-14 11:57:03 +00:00
fix
This commit is contained in:
@@ -292,6 +292,17 @@ async fn prepare_artifact_build(
|
||||
cwd: turn.cwd.clone(),
|
||||
staged_script: source_path.clone(),
|
||||
};
|
||||
let initial_approval_requirement = session
|
||||
.services
|
||||
.exec_policy
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
command: &command,
|
||||
approval_policy: turn.approval_policy.value(),
|
||||
sandbox_policy: turn.sandbox_policy.get(),
|
||||
sandbox_permissions: SandboxPermissions::UseDefault,
|
||||
prefix_rule: None,
|
||||
})
|
||||
.await;
|
||||
let escalation_approval_requirement = session
|
||||
.services
|
||||
.exec_policy
|
||||
@@ -303,23 +314,10 @@ async fn prepare_artifact_build(
|
||||
prefix_rule: None,
|
||||
})
|
||||
.await;
|
||||
let escalation_approval_requirement = match escalation_approval_requirement {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Skip { bypass_sandbox, .. } => {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
}
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval {
|
||||
reason,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
}
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Forbidden { reason } => {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Forbidden { reason }
|
||||
}
|
||||
};
|
||||
let initial_approval_requirement =
|
||||
clear_proposed_execpolicy_amendment(initial_approval_requirement);
|
||||
let escalation_approval_requirement =
|
||||
clear_proposed_execpolicy_amendment(escalation_approval_requirement);
|
||||
|
||||
let env = build_artifact_env(
|
||||
&installed_runtime,
|
||||
@@ -335,12 +333,35 @@ async fn prepare_artifact_build(
|
||||
timeout_ms: Some(timeout_ms),
|
||||
env,
|
||||
approval_key,
|
||||
initial_approval_requirement,
|
||||
escalation_approval_requirement,
|
||||
},
|
||||
_source_dir: source_dir,
|
||||
})
|
||||
}
|
||||
|
||||
fn clear_proposed_execpolicy_amendment(
|
||||
requirement: crate::tools::sandboxing::ExecApprovalRequirement,
|
||||
) -> crate::tools::sandboxing::ExecApprovalRequirement {
|
||||
match requirement {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Skip { bypass_sandbox, .. } => {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
}
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval { reason, .. } => {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval {
|
||||
reason,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
}
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Forbidden { reason } => {
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Forbidden { reason }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn ensure_artifact_build_launcher(codex_home: &Path) -> Result<PathBuf, FunctionCallError> {
|
||||
let launcher_path = artifact_build_launcher_path(codex_home);
|
||||
match fs::read_to_string(&launcher_path).await {
|
||||
@@ -525,7 +546,10 @@ fn format_artifact_stderr(output: &ExecToolCallOutput) -> String {
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::codex::make_session_and_context;
|
||||
use crate::config::Constrained;
|
||||
use crate::exec::StreamOutput;
|
||||
use crate::protocol::AskForApproval;
|
||||
use crate::protocol::SandboxPolicy;
|
||||
use codex_artifacts::RuntimeEntrypoints;
|
||||
use codex_artifacts::RuntimePathEntry;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -787,7 +811,9 @@ mod tests {
|
||||
|
||||
#[tokio::test]
|
||||
async fn prepare_artifact_build_uses_script_specific_approval_key_without_execpolicy_rule() {
|
||||
let (session, turn) = make_session_and_context().await;
|
||||
let (session, mut turn) = make_session_and_context().await;
|
||||
turn.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
turn.sandbox_policy = Constrained::allow_any(SandboxPolicy::new_read_only_policy());
|
||||
let runtime = codex_artifacts::InstalledArtifactRuntime::new(
|
||||
PathBuf::from("/runtime"),
|
||||
PINNED_ARTIFACT_RUNTIME_VERSION.to_string(),
|
||||
@@ -844,5 +870,19 @@ mod tests {
|
||||
.proposed_execpolicy_amendment()
|
||||
.is_none()
|
||||
);
|
||||
assert_eq!(
|
||||
prepared.request.initial_approval_requirement,
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
prepared.request.escalation_approval_requirement,
|
||||
crate::tools::sandboxing::ExecApprovalRequirement::NeedsApproval {
|
||||
reason: None,
|
||||
proposed_execpolicy_amendment: None,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -36,6 +36,7 @@ pub(crate) struct ArtifactExecRequest {
|
||||
pub(crate) timeout_ms: Option<u64>,
|
||||
pub(crate) env: HashMap<String, String>,
|
||||
pub(crate) approval_key: ArtifactApprovalKey,
|
||||
pub(crate) initial_approval_requirement: ExecApprovalRequirement,
|
||||
pub(crate) escalation_approval_requirement: ExecApprovalRequirement,
|
||||
}
|
||||
|
||||
@@ -77,10 +78,14 @@ impl Approvable<ArtifactExecRequest> for ArtifactRuntime {
|
||||
let command = req.command.clone();
|
||||
let cwd = req.cwd.clone();
|
||||
let approval_keys = self.approval_keys(req);
|
||||
let escalation_approval_requirement = req.escalation_approval_requirement.clone();
|
||||
let approval_requirement = if retry_reason.is_some() {
|
||||
req.escalation_approval_requirement.clone()
|
||||
} else {
|
||||
req.initial_approval_requirement.clone()
|
||||
};
|
||||
Box::pin(async move {
|
||||
if matches!(
|
||||
escalation_approval_requirement,
|
||||
approval_requirement,
|
||||
ExecApprovalRequirement::Forbidden { .. }
|
||||
) {
|
||||
return ReviewDecision::Denied;
|
||||
@@ -100,7 +105,7 @@ impl Approvable<ArtifactExecRequest> for ArtifactRuntime {
|
||||
cwd,
|
||||
retry_reason,
|
||||
None,
|
||||
escalation_approval_requirement
|
||||
approval_requirement
|
||||
.proposed_execpolicy_amendment()
|
||||
.cloned(),
|
||||
None,
|
||||
@@ -127,13 +132,13 @@ impl Approvable<ArtifactExecRequest> for ArtifactRuntime {
|
||||
&self,
|
||||
req: &ArtifactExecRequest,
|
||||
) -> Option<ExecApprovalRequirement> {
|
||||
Some(req.escalation_approval_requirement.clone())
|
||||
Some(req.initial_approval_requirement.clone())
|
||||
}
|
||||
|
||||
fn sandbox_mode_for_first_attempt(&self, req: &ArtifactExecRequest) -> SandboxOverride {
|
||||
sandbox_override_for_first_attempt(
|
||||
SandboxPermissions::UseDefault,
|
||||
&req.escalation_approval_requirement,
|
||||
&req.initial_approval_requirement,
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -194,6 +199,10 @@ mod tests {
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
staged_script: PathBuf::from("/tmp/source.mjs"),
|
||||
},
|
||||
initial_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
escalation_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
@@ -255,6 +264,10 @@ mod tests {
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
staged_script: PathBuf::from("/tmp/source-one.mjs"),
|
||||
},
|
||||
initial_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
escalation_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
@@ -277,6 +290,10 @@ mod tests {
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
staged_script: PathBuf::from("/tmp/source-two.mjs"),
|
||||
},
|
||||
initial_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
escalation_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
@@ -290,7 +307,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exec_approval_requirement_uses_request_requirement() {
|
||||
fn exec_approval_requirement_uses_initial_requirement() {
|
||||
let runtime = ArtifactRuntime;
|
||||
let req = ArtifactExecRequest {
|
||||
command: vec![
|
||||
@@ -309,21 +326,24 @@ mod tests {
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
staged_script: PathBuf::from("/tmp/source.mjs"),
|
||||
},
|
||||
initial_approval_requirement: ExecApprovalRequirement::Forbidden {
|
||||
reason: "blocked before first attempt".to_string(),
|
||||
},
|
||||
escalation_approval_requirement: ExecApprovalRequirement::Forbidden {
|
||||
reason: "blocked by policy".to_string(),
|
||||
reason: "blocked on retry".to_string(),
|
||||
},
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
runtime.exec_approval_requirement(&req),
|
||||
Some(ExecApprovalRequirement::Forbidden {
|
||||
reason: "blocked by policy".to_string(),
|
||||
reason: "blocked before first attempt".to_string(),
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sandbox_mode_for_first_attempt_honors_bypass_sandbox_requirement() {
|
||||
fn sandbox_mode_for_first_attempt_uses_initial_requirement() {
|
||||
let runtime = ArtifactRuntime;
|
||||
let req = ArtifactExecRequest {
|
||||
command: vec![
|
||||
@@ -342,6 +362,10 @@ mod tests {
|
||||
cwd: PathBuf::from("/tmp"),
|
||||
staged_script: PathBuf::from("/tmp/source.mjs"),
|
||||
},
|
||||
initial_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: false,
|
||||
proposed_execpolicy_amendment: None,
|
||||
},
|
||||
escalation_approval_requirement: ExecApprovalRequirement::Skip {
|
||||
bypass_sandbox: true,
|
||||
proposed_execpolicy_amendment: None,
|
||||
@@ -350,7 +374,7 @@ mod tests {
|
||||
|
||||
assert_eq!(
|
||||
runtime.sandbox_mode_for_first_attempt(&req),
|
||||
SandboxOverride::BypassSandboxFirstAttempt
|
||||
SandboxOverride::NoOverride
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user