proposing execpolicy amendment when prompting due to sandbox denial #7653
Conversation
zhao-oai
force-pushed
the
dev/zhao/sandbox-deny-amendment
branch
3 times, most recently
from
35d468b to
bd130e4
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
zhao-oai
force-pushed
the
dev/zhao/sandbox-deny-amendment
branch
from
bd130e4 to
5a520f0
Compare
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Overall looks good, but please try to tighten up the docstrings/naming.
| /// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"]. | ||
| fn proposed_execpolicy_amendment(evaluation: &Evaluation) -> Option<ExecPolicyAmendment> { | ||
| if evaluation.decision != Decision::Prompt { | ||
| // - If any execpolicy rule prompts, return None, because an amendment would not skip that policy requirement. |
There was a problem hiding this comment.
nit: prefer /// for docstrings
| // on `prog1`, we return `Some(vec!["prog1", "--option1", "arg1"])`. | ||
| // - execpolicy: contains a `prompt for prefix ["prog2"]` rule. For the same command as above, | ||
| // we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"]. | ||
| fn prompt_execpolicy_amendment(matched_rules: &[RuleMatch]) -> Option<ExecPolicyAmendment> { |
There was a problem hiding this comment.
Between the function name and the docstring, I'm confused what this function is for / how it works.
For example, the name starts with prompt_, but this does not do any "prompting" of its own.
I think it is about deriving a new value, so changing the name to start with create_ or derive_ would be clearer.
Unfortunately, all the names I can think of are quite long, like try_derive_execpolicy_amendment_for_prompt_rules().
| /// we return `None` because an execpolicy prompt still applies even if we amend execpolicy to allow ["prog1", "--option1", "arg1"]. | ||
| fn proposed_execpolicy_amendment(evaluation: &Evaluation) -> Option<ExecPolicyAmendment> { | ||
| if evaluation.decision != Decision::Prompt { | ||
| // - If any execpolicy rule prompts, return None, because an amendment would not skip that policy requirement. |
There was a problem hiding this comment.
I thought the first line returns None if any execpolicy rule does not prompt?
| }) | ||
| } | ||
|
|
||
| // - Note: we only use this amendment when the command fails to run in sandbox and codex prompts the user to run outside the sandbox |
Currently, we only show the “don’t ask again for commands that start with…” option when a command is immediately flagged as needing approval. However, there is another case where we ask for approval: When a command is initially auto-approved to run within sandbox, but it fails to run inside sandbox, we would like to attempt to retry running outside of sandbox. This will require a prompt to the user.
This PR addresses this latter case