fix(core): skip exec approval for permissionless skill scripts (#13791)
## Summary - Treat skill scripts with no permission profile, or an explicitly empty one, as permissionless and run them with the turn's existing sandbox instead of forcing an exec approval prompt. - Keep the approval flow unchanged for skills that do declare additional permissions. - Update the skill approval tests to assert that permissionless skill scripts do not prompt on either the initial run or a rerun. ## Why Permissionless skills should inherit the current turn sandbox directly. Prompting for exec approval in that case adds friction without granting any additional capability.
This commit is contained in:
parent
0243734300
commit
8b81284975
2 changed files with 33 additions and 41 deletions
|
|
@ -570,9 +570,22 @@ impl EscalationPolicy for CoreShellActionProvider {
|
|||
// 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.
|
||||
// For now, scripts that look like they belong to skills bypass
|
||||
// general exec policy evaluation. Permissionless skills inherit the
|
||||
// turn sandbox directly; skills with declared permissions still
|
||||
// prompt here before applying their permission profile.
|
||||
let prompt_permissions = skill.permission_profile.clone();
|
||||
if prompt_permissions
|
||||
.as_ref()
|
||||
.is_none_or(PermissionProfile::is_empty)
|
||||
{
|
||||
tracing::debug!(
|
||||
"Matched {program:?} to permissionless skill {skill:?}, inheriting turn sandbox"
|
||||
);
|
||||
return Ok(EscalationDecision::escalate(
|
||||
EscalationExecution::TurnDefault,
|
||||
));
|
||||
}
|
||||
tracing::debug!("Matched {program:?} to skill {skill:?}, prompting for approval");
|
||||
let needs_escalation = true;
|
||||
let decision_source = DecisionSource::SkillScript {
|
||||
|
|
@ -585,7 +598,7 @@ impl EscalationPolicy for CoreShellActionProvider {
|
|||
program,
|
||||
argv,
|
||||
workdir,
|
||||
skill.permission_profile.clone(),
|
||||
prompt_permissions,
|
||||
Self::skill_escalation_execution(&skill),
|
||||
decision_source,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -265,8 +265,7 @@ permissions:
|
|||
Ok(())
|
||||
}
|
||||
|
||||
/// Look for `additional_permissions == None`, then verify that both the first
|
||||
/// run and the cached session-approval rerun stay inside the turn sandbox.
|
||||
/// Permissionless skills should inherit the turn sandbox without prompting.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn shell_zsh_fork_skill_without_permissions_inherits_turn_sandbox() -> Result<()> {
|
||||
|
|
@ -307,7 +306,7 @@ async fn shell_zsh_fork_skill_without_permissions_inherits_turn_sandbox() -> Res
|
|||
)
|
||||
.await?;
|
||||
|
||||
let (script_path_str, command) = skill_script_command(&test, "sandboxed.sh")?;
|
||||
let (_, command) = skill_script_command(&test, "sandboxed.sh")?;
|
||||
|
||||
let first_call_id = "zsh-fork-skill-permissions-1";
|
||||
let first_arguments = shell_command_arguments(&command)?;
|
||||
|
|
@ -327,22 +326,11 @@ async fn shell_zsh_fork_skill_without_permissions_inherits_turn_sandbox() -> Res
|
|||
)
|
||||
.await?;
|
||||
|
||||
let maybe_approval = wait_for_exec_approval_request(&test).await;
|
||||
let approval = match maybe_approval {
|
||||
Some(approval) => approval,
|
||||
None => panic!("expected exec approval request before completion"),
|
||||
};
|
||||
assert_eq!(approval.call_id, first_call_id);
|
||||
assert_eq!(approval.command, vec![script_path_str.clone()]);
|
||||
assert_eq!(approval.additional_permissions, None);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::ApprovedForSession,
|
||||
})
|
||||
.await?;
|
||||
let first_approval = wait_for_exec_approval_request(&test).await;
|
||||
assert!(
|
||||
first_approval.is_none(),
|
||||
"expected permissionless skill script to skip exec approval"
|
||||
);
|
||||
|
||||
wait_for_turn_complete(&test).await;
|
||||
|
||||
|
|
@ -383,7 +371,7 @@ async fn shell_zsh_fork_skill_without_permissions_inherits_turn_sandbox() -> Res
|
|||
let cached_approval = wait_for_exec_approval_request(&test).await;
|
||||
assert!(
|
||||
cached_approval.is_none(),
|
||||
"expected second run to reuse the cached session approval"
|
||||
"expected permissionless skill rerun to continue skipping exec approval"
|
||||
);
|
||||
|
||||
let second_output = second_mocks
|
||||
|
|
@ -406,7 +394,7 @@ async fn shell_zsh_fork_skill_without_permissions_inherits_turn_sandbox() -> Res
|
|||
}
|
||||
|
||||
/// Empty skill permissions should behave like no skill override and inherit the
|
||||
/// turn sandbox instead of forcing an explicit read-only skill sandbox.
|
||||
/// turn sandbox without prompting.
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn shell_zsh_fork_skill_with_empty_permissions_inherits_turn_sandbox() -> Result<()> {
|
||||
|
|
@ -447,7 +435,7 @@ async fn shell_zsh_fork_skill_with_empty_permissions_inherits_turn_sandbox() ->
|
|||
)
|
||||
.await?;
|
||||
|
||||
let (script_path_str, command) = skill_script_command(&test, "sandboxed.sh")?;
|
||||
let (_, command) = skill_script_command(&test, "sandboxed.sh")?;
|
||||
|
||||
let first_call_id = "zsh-fork-skill-empty-permissions-1";
|
||||
let first_arguments = shell_command_arguments(&command)?;
|
||||
|
|
@ -467,20 +455,11 @@ async fn shell_zsh_fork_skill_with_empty_permissions_inherits_turn_sandbox() ->
|
|||
)
|
||||
.await?;
|
||||
|
||||
let approval = wait_for_exec_approval_request(&test)
|
||||
.await
|
||||
.expect("expected exec approval request before completion");
|
||||
assert_eq!(approval.call_id, first_call_id);
|
||||
assert_eq!(approval.command, vec![script_path_str.clone()]);
|
||||
assert_eq!(approval.additional_permissions, None);
|
||||
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::ApprovedForSession,
|
||||
})
|
||||
.await?;
|
||||
let first_approval = wait_for_exec_approval_request(&test).await;
|
||||
assert!(
|
||||
first_approval.is_none(),
|
||||
"expected empty skill permissions to skip exec approval"
|
||||
);
|
||||
|
||||
wait_for_turn_complete(&test).await;
|
||||
|
||||
|
|
@ -520,7 +499,7 @@ async fn shell_zsh_fork_skill_with_empty_permissions_inherits_turn_sandbox() ->
|
|||
let cached_approval = wait_for_exec_approval_request(&test).await;
|
||||
assert!(
|
||||
cached_approval.is_none(),
|
||||
"expected second run to reuse the cached session approval"
|
||||
"expected empty-permissions skill rerun to continue skipping exec approval"
|
||||
);
|
||||
|
||||
let second_output = second_mocks
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue