fix: prepare ExecPolicy in exec-server for execpolicy2 cutover #6888
Conversation
bolinfest
force-pushed
the
pr6888
branch
3 times, most recently
from
9ed0ab7 to
000ae54
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
| match result.action { | ||
| ElicitationAction::Accept => EscalateAction::Escalate, | ||
| ElicitationAction::Decline => EscalateAction::Deny { |
There was a problem hiding this comment.
When an exec policy returns Decision::Prompt with run_with_escalated_permissions set to false, an accepted elicitation is still mapped to EscalateAction::Escalate here, so prompted commands always run with elevated permissions even when the policy only intended an in-sandbox run. This over-privileges approved commands and changes the policy’s decision; the prompt branch should respect the run_with_escalated_permissions flag when translating an Accept.
Useful? React with 👍 / 👎.
| Run, | ||
| /// The command should be escalated to the server for execution. | ||
| Escalate, | ||
| /// The command should be denied. |
There was a problem hiding this comment.
| /// The command should be denied. | |
| /// The command should not be executed. |
| pub fn new(bash_path: PathBuf, policy: Arc<dyn EscalationPolicy>) -> Self { | ||
| Self { bash_path, policy } | ||
| } |
There was a problem hiding this comment.
nit: i feel like this should just take dyn EscalationPolicy and we can do the Arc::new internally? the fact that it's wrapped in Arc is kind of an implementation detail.
There was a problem hiding this comment.
But then we have to wrap it in Box, which I don't think is any better?
There was a problem hiding this comment.
This is a bit more ceremony, but it works:
pub fn new<P>(bash_path: PathBuf, policy: P) -> Self
where
P: EscalationPolicy + Send + Sync + 'static,
{
Self {
bash_path,
policy: Arc::new(policy),
}
}| pub(crate) struct ExecPolicyOutcome { | ||
| pub(crate) decision: Decision, | ||
| pub(crate) run_with_escalated_permissions: bool, | ||
| } |
There was a problem hiding this comment.
This should be an enum; decision: Deny, run_with_escalated_permissions: true e.g. makes no sense
This PR introduces an extra layer of abstraction to prepare us for the migration to execpolicy2:
EscalationPolicy, whosedetermine_action()method is responsible for producing theEscalateActionExecPolicytypedef is changed to return an intermediateExecPolicyOutcomeinstead ofEscalateActionEscalationPolicy,McpEscalationPolicy, composesExecPolicyExecPolicyOutcomeincludescodex_execpolicy2::Decision, which has aPromptvariantMcpEscalationPolicygetsDecision::Promptback fromExecPolicy, it prompts the user via an MCP elicitation and maps the result into anElicitationActionDeclineorCancel, we introduce a new variant,EscalateAction::Deny, which the client handles by returning exit code1without running anythingNote the way the elicitation is created is still not quite right, but I will fix that once we have things running end-to-end for real in a follow-up PR.