Skip to content

feat: scope execve session approvals by approved skill metadata#12814

Merged
bolinfest merged 1 commit intomainfrom
pr12814
Feb 25, 2026
Merged

feat: scope execve session approvals by approved skill metadata#12814
bolinfest merged 1 commit intomainfrom
pr12814

Conversation

@bolinfest
Copy link
Collaborator

@bolinfest bolinfest commented Feb 25, 2026

Previous to this change, determine_action() would

  1. check if program is associated with a skill
  2. if so, check if program is in execve_session_approvals to see whether the user needs to be prompted

This PR flips the order of these checks to try to set us up so that "session approvals" are always consulted first (which should soon extend to include session approvals derived from prefix_rule()s, as well).

Though to make the new ordering work, we need to record any relevant metadata to associate with the approval, which in the case of a skill-based approval is the SkillMetadata so that we can derive the PermissionProfile to include with the escalation. (Though as noted by the TODO, this PermissionProfile is not honored yet.)

The new ExecveSessionApproval struct is used to retain the necessary metadata.

What Changed

  • Replace the execve_session_approvals HashSet with a map that stores an ExecveSessionApproval alongside each approved program.
  • When a user chooses ApprovedForSession for a skill script, capture the matched SkillMetadata in the session approval entry.
  • Consult that cache before re-running find_skill(), and reuse the originally approved skill metadata and permission profile when allowing later execve callbacks in the same session.

@chatgpt-codex-connector
Copy link
Contributor

💡 Codex Review

#[experimental("item/commandExecution/requestApproval.availableDecisions")]
#[serde(default, skip_serializing_if = "Option::is_none")]
#[ts(optional = nullable)]
pub available_decisions: Option<Vec<CommandExecutionApprovalDecision>>,

P1 Badge Strip availableDecisions for non-experimental clients

available_decisions is marked experimental here, but CommandExecutionRequestApprovalParams::strip_experimental_fields() still only clears additional_permissions. I checked app-server/src/transport.rs (filter_outgoing_message_for_connection), and this stripper is the compatibility gate used when experimentalApi is disabled, so stable clients can still receive availableDecisions unexpectedly. Clients that validate request payloads against the non-experimental schema (or reject unknown fields) can fail item/commandExecution/requestApproval handling and block command approval.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@bolinfest bolinfest force-pushed the pr12814 branch 2 times, most recently from a8d528a to 4eaec7b Compare February 25, 2026 21:02
@bolinfest bolinfest force-pushed the pr12814 branch 2 times, most recently from ddf331f to 43daba8 Compare February 25, 2026 21:33
@bolinfest bolinfest force-pushed the pr12758 branch 2 times, most recently from bb269a5 to 7707752 Compare February 25, 2026 22:29
@bolinfest bolinfest force-pushed the pr12814 branch 2 times, most recently from 7322467 to 9f008e0 Compare February 25, 2026 22:40
@bolinfest bolinfest changed the base branch from pr12758 to main February 25, 2026 22:40
@bolinfest bolinfest enabled auto-merge (squash) February 25, 2026 23:14
@bolinfest bolinfest disabled auto-merge February 25, 2026 23:30
@bolinfest bolinfest merged commit a6a5976 into main Feb 25, 2026
56 of 64 checks passed
@bolinfest bolinfest deleted the pr12814 branch February 25, 2026 23:30
@github-actions github-actions bot locked and limited conversation to collaborators Feb 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants