agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

feat: zsh-fork forces scripts/**/* for skills to trigger a prompt (#12730)

Browse Source 
Direct skill-script matches force `Decision::Prompt`, so skill-backed
scripts require explicit approval before they run. (Note "allow for
session" is not supported in this PR, but will be done in a follow-up.)

In the process of implementing this, I fixed an important bug:
`ShellZshFork` is supposed to keep ordinary allowed execs on the
client-side `Run` path so later `execve()` calls are still intercepted
and reviewed. After the shell-escalation port, `Decision::Allow` still
mapped to `Escalate`, which moved `zsh` to server-side execution too
early. That broke the intended flow for skill-backed scripts and made
the approval prompt depend on the wrong execution path.

## What changed
- In `codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs`,
`Decision::Allow` now returns `Run` unless escalation is actually
required.
- Removed the zsh-specific `argv[0]` fallback. With the `Allow -> Run`
fix in place, zsh's later `execve()` of the script is intercepted
normally, so the skill match happens on the script path itself.
- Kept the skill-path handling in `determine_action()` focused on the
direct `program` match path.

## Verification
- Updated `shell_zsh_fork_prompts_for_skill_script_execution` in
`codex-rs/core/tests/suite/skill_approval.rs` (gated behind `cfg(unix)`)
to:
- run under `SandboxPolicy::new_workspace_write_policy()` instead of
`DangerFullAccess`
  - assert the approval command contains only the script path
- assert the approved run returns both stdout and stderr markers in the
shell output
- Ran `cargo test -p codex-core
shell_zsh_fork_prompts_for_skill_script_execution -- --nocapture`

## Manual Testing

Run the dev build:

```
just codex --config zsh_path=/Users/mbolin/code/codex2/codex-rs/app-server/tests/suite/zsh --enable shell_zsh_fork
```

I have created `/Users/mbolin/.agents/skills/mbolin-test-skill` with:

```
├── scripts
│   └── hello-mbolin.sh
└── SKILL.md
```

The skill:

```
---
name: mbolin-test-skill
description: Used to exercise various features of skills.
---

When this skill is invoked, run the `hello-mbolin.sh` script and report the output.
```

The script:

```
set -e

# Note this script will fail if run with network disabled.
curl --location openai.com
```

Use `$mbolin-test-skill` to invoke the skill manually and verify that I
get prompted to run `hello-mbolin.sh`.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/12730).
* #12750
* __->__ #12730
This commit is contained in:
Michael Bolin
2026-02-24 23:51:26 -08:00
committed by GitHub
Unverified
parent c086b36b58
commit 59398125f6
2 changed files with 356 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
+146 -43
View File
@@ -8,6 +8,7 @@ use crate::exec::is_likely_sandbox_denied;
use crate::features::Feature;
use crate::sandboxing::SandboxPermissions;
use crate::shell::ShellType;
use crate::skills::SkillMetadata;
use crate::tools::runtimes::build_command_spec;
use crate::tools::sandboxing::SandboxAttempt;
use crate::tools::sandboxing::ToolCtx;
@@ -174,11 +175,12 @@ impl CoreShellActionProvider {
async fn prompt(
&self,
command: &[String],
program: &Path,
argv: &[String],
workdir: &Path,
stopwatch: &Stopwatch,
) -> anyhow::Result<ReviewDecision> {
let command = command.to_vec();
let command = join_program_and_argv(program, argv);
let workdir = workdir.to_path_buf();
let session = self.session.clone();
let turn = self.turn.clone();
@@ -202,49 +204,41 @@ impl CoreShellActionProvider {
})
.await)
}
}
#[async_trait::async_trait]
impl EscalationPolicy for CoreShellActionProvider {
async fn determine_action(
/// Because we should be intercepting execve(2) calls, `program` should be
/// an absolute path. The idea is that we check to see whether it matches
/// any skills.
async fn find_skill(&self, program: &Path) -> Option<SkillMetadata> {
let force_reload = false;
let skills_outcome = self
.session
.services
.skills_manager
.skills_for_cwd(&self.turn.cwd, force_reload)
.await;
for skill in skills_outcome.skills {
// We intentionally ignore "enabled" status here for now.
let Some(skill_root) = skill.path_to_skills_md.parent() else {
continue;
};
if program.starts_with(skill_root.join("scripts")) {
return Some(skill);
}
}
None
}
async fn process_decision(
&self,
file: &Path,
decision: Decision,
needs_escalation: bool,
program: &Path,
argv: &[String],
workdir: &Path,
) -> anyhow::Result<EscalateAction> {
let command = std::iter::once(file.to_string_lossy().to_string())
.chain(argv.iter().cloned())
.collect::<Vec<_>>();
let (commands, used_complex_parsing) =
if let Some(commands) = parse_shell_lc_plain_commands(&command) {
(commands, false)
} else if let Some(single_command) = parse_shell_lc_single_command_prefix(&command) {
(vec![single_command], true)
} else {
(vec![command.clone()], false)
};
let fallback = |cmd: &[String]| {
crate::exec_policy::render_decision_for_unmatched_command(
self.approval_policy,
&self.sandbox_policy,
cmd,
self.sandbox_permissions,
used_complex_parsing,
)
};
let evaluation = {
let policy = self.policy.read().await;
policy.check_multiple(commands.iter(), &fallback)
};
// When true, means the Evaluation was due to *.rules, not the
// fallback function.
let decision_driven_by_policy =
Self::decision_driven_by_policy(&evaluation.matched_rules, evaluation.decision);
let needs_escalation =
self.sandbox_permissions.requires_escalated_permissions() || decision_driven_by_policy;
Ok(match evaluation.decision {
let action = match decision {
Decision::Forbidden => EscalateAction::Deny {
reason: Some("Execution forbidden by policy".to_string()),
},
@@ -258,7 +252,7 @@ impl EscalationPolicy for CoreShellActionProvider {
reason: Some("Execution forbidden by policy".to_string()),
}
} else {
match self.prompt(&command, workdir, &self.stopwatch).await? {
match self.prompt(program, argv, workdir, &self.stopwatch).await? {
ReviewDecision::Approved
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
| ReviewDecision::ApprovedForSession => {
@@ -291,8 +285,88 @@ impl EscalationPolicy for CoreShellActionProvider {
}
}
}
Decision::Allow => EscalateAction::Escalate,
})
Decision::Allow => {
if needs_escalation {
EscalateAction::Escalate
} else {
EscalateAction::Run
}
}
};
tracing::debug!(
"Policy decision for command {program:?} is {decision:?}, leading to escalation action {action:?}",
);
Ok(action)
}
}
#[async_trait::async_trait]
impl EscalationPolicy for CoreShellActionProvider {
async fn determine_action(
&self,
program: &Path,
argv: &[String],
workdir: &Path,
) -> anyhow::Result<EscalateAction> {
tracing::debug!(
"Determining escalation action for command {program:?} with args {argv:?} in {workdir:?}"
);
// In the usual case, the execve wrapper reports the command being
// executed in `program`, so a direct skill lookup is sufficient.
if let Some(skill) = self.find_skill(program).await {
// For now, we always prompt for scripts that look like they belong
// to skills, which means we ignore exec policy rules for those
// scripts.
tracing::debug!("Matched {program:?} to skill {skill:?}, prompting for approval");
// TODO(mbolin): We should read the permissions associated with the
// skill and use those specific permissions in the
// EscalateAction::Run case, rather than always escalating when a
// skill matches.
let needs_escalation = true;
return self
.process_decision(Decision::Prompt, needs_escalation, program, argv, workdir)
.await;
}
let command = join_program_and_argv(program, argv);
let (commands, used_complex_parsing) =
if let Some(commands) = parse_shell_lc_plain_commands(&command) {
(commands, false)
} else if let Some(single_command) = parse_shell_lc_single_command_prefix(&command) {
(vec![single_command], true)
} else {
(vec![command.clone()], false)
};
let fallback = |cmd: &[String]| {
crate::exec_policy::render_decision_for_unmatched_command(
self.approval_policy,
&self.sandbox_policy,
cmd,
self.sandbox_permissions,
used_complex_parsing,
)
};
let evaluation = {
let policy = self.policy.read().await;
policy.check_multiple(commands.iter(), &fallback)
};
// When true, means the Evaluation was due to *.rules, not the
// fallback function.
let decision_driven_by_policy =
Self::decision_driven_by_policy(&evaluation.matched_rules, evaluation.decision);
let needs_escalation =
self.sandbox_permissions.requires_escalated_permissions() || decision_driven_by_policy;
self.process_decision(
evaluation.decision,
needs_escalation,
program,
argv,
workdir,
)
.await
}
}
@@ -403,14 +477,28 @@ fn map_exec_result(
Ok(output)
}
/// Convert an intercepted exec `(program, argv)` into a command vector suitable
/// for display and policy parsing.
///
/// The intercepted `argv` includes `argv[0]`, but once we have normalized the
/// executable path in `program`, we should replace the original `argv[0]`
/// rather than duplicating it as an apparent user argument.
fn join_program_and_argv(program: &Path, argv: &[String]) -> Vec<String> {
std::iter::once(program.to_string_lossy().to_string())
.chain(argv.iter().skip(1).cloned())
.collect::<Vec<_>>()
}
#[cfg(test)]
mod tests {
use super::ParsedShellCommand;
use super::extract_shell_script;
use super::join_program_and_argv;
use super::map_exec_result;
use crate::exec::SandboxType;
use codex_shell_escalation::ExecResult;
use pretty_assertions::assert_eq;
use std::path::Path;
use std::time::Duration;
#[test]
@@ -431,6 +519,21 @@ mod tests {
);
}
#[test]
fn join_program_and_argv_replaces_original_argv_zero() {
assert_eq!(
join_program_and_argv(
Path::new("/tmp/tool"),
&["./tool".into(), "--flag".into(), "value".into()],
),
vec!["/tmp/tool", "--flag", "value"]
);
assert_eq!(
join_program_and_argv(Path::new("/tmp/tool"), &["./tool".into()]),
vec!["/tmp/tool"]
);
}
#[test]
fn map_exec_result_preserves_stdout_and_stderr() {
let out = map_exec_result(