Commit Graph

8 Commits

  • feat: include available decisions in command approval requests (#12758)
    Command-approval clients currently infer which choices to show from
    side-channel fields like `networkApprovalContext`,
    `proposedExecpolicyAmendment`, and `additionalPermissions`. That makes
    the request shape harder to evolve, and it forces each client to
    replicate the server's heuristics instead of receiving the exact
    decision list for the prompt.
    
    This PR introduces a mapping between `CommandExecutionApprovalDecision`
    and `codex_protocol::protocol::ReviewDecision`:
    
    ```rust
    impl From<CoreReviewDecision> for CommandExecutionApprovalDecision {
        fn from(value: CoreReviewDecision) -> Self {
            match value {
                CoreReviewDecision::Approved => Self::Accept,
                CoreReviewDecision::ApprovedExecpolicyAmendment {
                    proposed_execpolicy_amendment,
                } => Self::AcceptWithExecpolicyAmendment {
                    execpolicy_amendment: proposed_execpolicy_amendment.into(),
                },
                CoreReviewDecision::ApprovedForSession => Self::AcceptForSession,
                CoreReviewDecision::NetworkPolicyAmendment {
                    network_policy_amendment,
                } => Self::ApplyNetworkPolicyAmendment {
                    network_policy_amendment: network_policy_amendment.into(),
                },
                CoreReviewDecision::Abort => Self::Cancel,
                CoreReviewDecision::Denied => Self::Decline,
            }
        }
    }
    ```
    
    And updates `CommandExecutionRequestApprovalParams` to have a new field:
    
    ```rust
    available_decisions: Option<Vec<CommandExecutionApprovalDecision>>
    ```
    
    when, if specified, should make it easier for clients to display an
    appropriate list of options in the UI.
    
    This makes it possible for `CoreShellActionProvider::prompt()` in
    `unix_escalation.rs` to specify the `Vec<ReviewDecision>` directly,
    adding support for `ApprovedForSession` when approving a skill script,
    which was previously missing in the TUI.
    
    Note this results in a significant change to `exec_options()` in
    `approval_overlay.rs`, as the displayed options are now derived from
    `available_decisions: &[ReviewDecision]`.
    
    ## What Changed
    
    - Add `available_decisions` to
    [`ExecApprovalRequestEvent`](https://github.com/openai/codex/blob/de00e932dd9801de0a4faac0519162099753f331/codex-rs/protocol/src/approvals.rs#L111-L175),
    including helpers to derive the legacy default choices when older
    senders omit the field.
    - Map `codex_protocol::protocol::ReviewDecision` to app-server
    `CommandExecutionApprovalDecision` and expose the ordered list as
    experimental `availableDecisions` in
    [`CommandExecutionRequestApprovalParams`](https://github.com/openai/codex/blob/de00e932dd9801de0a4faac0519162099753f331/codex-rs/app-server-protocol/src/protocol/v2.rs#L3798-L3807).
    - Thread optional `available_decisions` through the core approval path
    so Unix shell escalation can explicitly request `ApprovedForSession` for
    session-scoped approvals instead of relying on client heuristics.
    [`unix_escalation.rs`](https://github.com/openai/codex/blob/de00e932dd9801de0a4faac0519162099753f331/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs#L194-L214)
    - Update the TUI approval overlay to build its buttons from the ordered
    decision list, while preserving the legacy fallback when
    `available_decisions` is missing.
    - Update the app-server README, test client output, and generated schema
    artifacts to document and surface the new field.
    
    ## Testing
    
    - Add `approval_overlay.rs` coverage for explicit decision lists,
    including the generic `ApprovedForSession` path and network approval
    options.
    - Update `chatwidget/tests.rs` and app-server protocol tests to populate
    the new optional field and keep older event shapes working.
    
    ## Developers Docs
    
    - If we document `item/commandExecution/requestApproval` on
    [developers.openai.com/codex](https://developers.openai.com/codex), add
    experimental `availableDecisions` as the preferred source of approval
    choices and note that older servers may omit it.
  • Revert "Add skill approval event/response (#12633)" (#12811)
    This reverts commit https://github.com/openai/codex/pull/12633. We no
    longer need this PR, because we favor sending normal exec command
    approval server request with `additional_permissions` of skill
    permissions instead
  • Revert "Ensure shell command skills trigger approval (#12697)" (#12721)
    This reverts commit daf0f03ac8.
    
    # External (non-OpenAI) Pull Request Requirements
    
    Before opening this Pull Request, please read the dedicated
    "Contributing" markdown file or your PR may be closed:
    https://github.com/openai/codex/blob/main/docs/contributing.md
    
    If your PR conforms to our contribution guidelines, replace this text
    with a detailed and high quality description of your changes.
    
    Include a link to a bug report or enhancement request.
  • fix: enforce sandbox envelope for zsh fork execution (#12800)
    ## Why
    Zsh fork execution was still able to bypass the `WorkspaceWrite` model
    in edge cases because the fork path reconstructed command execution
    without preserving sandbox wrappers, and command extraction only
    accepted shell invocations in a narrow positional shape. This can allow
    commands to run with broader filesystem access than expected, which
    breaks the sandbox safety model.
    
    ## What changed
    - Preserved the sandboxed `ExecRequest` produced by
    `attempt.env_for(...)` when entering the zsh fork path in
    [`unix_escalation.rs`](https://github.com/openai/codex/blob/main/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs).
    - Updated `CoreShellCommandExecutor` to execute the sandboxed command
    and working directory captured from `attempt.env_for(...)`, instead of
    re-running a freshly reconstructed shell command.
    - Made zsh-fork script extraction robust to wrapped invocations by
    scanning command arguments for `-c`/`-lc` rather than only matching the
    first positional form.
    - Added unit tests in `unix_escalation.rs` to lock in wrapper-tolerant
    parsing behavior and keep unsupported shell forms rejected.
    - Tightened the regression in
    [`skill_approval.rs`](https://github.com/openai/codex/blob/main/codex-rs/core/tests/suite/skill_approval.rs):
    - `shell_zsh_fork_still_enforces_workspace_write_sandbox` now uses an
    explicit `WorkspaceWrite` policy with `exclude_tmpdir_env_var: true` and
    `exclude_slash_tmp: true`.
    - The test attempts to write to `/tmp/...`, which is only reliably
    outside writable roots with those explicit exclusions set.
    
    ## Verification
    - Added and passed the new unit tests around `extract_shell_script`
    parsing behavior with wrapped command shapes.
      - `extract_shell_script_supports_wrapped_command_prefixes`
      - `extract_shell_script_rejects_unsupported_shell_invocation`
    - Verified the regression with the focused integration test:
    `shell_zsh_fork_still_enforces_workspace_write_sandbox`.
    
    ## Manual Testing
    
    Prior to this change, if I ran Codex via:
    
    ```
    just codex --config zsh_path=/Users/mbolin/code/codex2/codex-rs/app-server/tests/suite/zsh --enable shell_zsh_fork
    ```
    
    and asked:
    
    ```
    what is the output of /bin/ps
    ```
    
    it would run it, even though the default sandbox should prevent the
    agent from running `/bin/ps` because it is setuid on MacOS.
    
    But with this change, I now see the expected failure because it is
    blocked by the sandbox:
    
    ```
    /bin/ps exited with status 1 and produced no output in this environment.
    ```
  • Surface skill permission profiles in zsh-fork exec approvals (#12753)
    ## Summary
    
    - Preserve each skill’s raw permissions block as a permission_profile on
    SkillMetadata during skill loading.
    - Keep compiling that same metadata into the existing runtime
    Permissions object, so current enforcement
        behavior stays intact.
    - When zsh-fork intercepts execution of a script that belongs to a
    skill, include the skill’s
        permission_profile in the exec approval request.
    - This lets approval UIs show the extra filesystem access the skill
    declared when prompting for approval.
  • feat: zsh-fork forces scripts/**/* for skills to trigger a prompt (#12730)
    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
  • Ensure shell command skills trigger approval (#12697)
    Summary
    - detect skill-invoking shell commands based on the original command
    string, request approvals when needed, and cache positive decisions per
    session
    - keep implicit skill invocation emitted after approval and keep skill
    approval decline messaging centralized to the shell handler
    - expand and adjust skill approval tests to cover shell-based skill
    scripts while matching the new detection expectations
    
    Testing
    - Not run (not requested)
  • Add skill approval event/response (#12633)
    Set the stage for skill-level permission approval in addition to
    command-level.
    
    Behind a feature flag.