Conversation
dylan-hurd-oai
force-pushed
the
dh--on-request-rule
branch
from
1386305 to
6b549df
Compare
dylan-hurd-oai
changed the title
dylan-hurd-oai
force-pushed
the
dh--on-request-rule
branch
from
6b549df to
05bd7cf
Compare
|
@codex review |
![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.
Reviewed commit: 7c819dfd7d
ℹ️ 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".
| { | ||
| return None; | ||
| } | ||
|
|
||
| Some(ExecPolicyAmendment::new(prefix_rule.clone())) |
There was a problem hiding this comment.
When RequestRule is enabled, prefix_rule from the tool call is turned into a persisted ExecPolicyAmendment without verifying that it is a prefix of any parsed command segment. In the case where a prompt-injected or buggy model supplies a broader or unrelated prefix (e.g. ['rm'] while running a benign command), the approval UI can present and persist a rule that grants escalation for commands the user did not intend to authorize. Consider rejecting or normalizing prefix_rule unless it is a prefix of the command being approved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex update to validate prefix_rule against the command
There was a problem hiding this comment.
Summary
- Added command-prefix validation when deriving requested execpolicy amendments, ensuring only prefixes that match parsed commands are accepted. codex-rs/core/src/exec_policy.rsL135-L185 codex-rs/core/src/exec_policy.rsL407-L437
- Added a test covering rejection of non-prefix request rules to confirm fallback behavior. codex-rs/core/src/exec_policy.rsL990-L1019
Testing
- ❌
cargo test -p codex-core(fails: default_client::tests::test_create_client_sets_default_headers; exec::tests::kill_child_process_group_kills_grandchildren_on_timeout; models_manager::manager::tests::{refresh_available_models_drops_removed_remote_models, refresh_available_models_refetches_when_cache_stale, refresh_available_models_sorts_by_priority, refresh_available_models_uses_cache_when_fresh}; shell_snapshot::tests::{linux_bash_snapshot_includes_sections, linux_sh_snapshot_includes_sections, timed_out_snapshot_shell_is_terminated, try_new_creates_and_deletes_snapshot_file}; unified_exec::tests::{multi_unified_exec_sessions, reusing_completed_process_returns_unknown_process, unified_exec_persists_across_requests})
| let include_apply_patch_tool = features.enabled(Feature::ApplyPatchFreeform); | ||
| let include_collab_tools = features.enabled(Feature::Collab); | ||
| let include_collaboration_modes_tools = features.enabled(Feature::CollaborationModes); | ||
| let request_rule_enabled = features.enabled(Feature::RequestRule); |
There was a problem hiding this comment.
Ideally, it would be nice to know at this point whether an enterprise has disallowed this via requirements.toml or some other mechanism. Perhaps we should support feature enablement in general in requirements.toml? /cc @gt-oai
There was a problem hiding this comment.
I wonder if requirements.toml should instead just disallow changing exec policy? i.e. regardless of whether model suggests one or we derive it, requirements.toml should just be able to disable the "don't ask again" / persist behavior.
codex-rs/protocol/src/prompts/permissions/approval_policy/on_request_rule.md
Outdated
Show resolved
Hide resolved
| ## prefix_rule guidance | ||
| When choosing a `prefix_rule`, request one that will allow you to fulfill similar requests from the user in the future without re-requesting escalation. It should be categorical and reasonably scoped to similar capabilities. You MUST NOT pass the entire command into `prefix_rule`. | ||
|
|
||
| <good_example reason="frequently run command"> |
There was a problem hiding this comment.
should this be in triple backticks?
| Some(lines.join("\n")) | ||
| } | ||
|
|
||
| fn render_command_prefix(prefix: &[String]) -> String { |
There was a problem hiding this comment.
In practice, I wonder whether this could be large enough that we would consider it a problem?
There was a problem hiding this comment.
Yeah - @bolinfest given other discussions I'm going to avoid re-injecting the whole thing in every message, and we can set up different logic that adds it as a diff
dylan-hurd-oai
force-pushed
the
dh--on-request-rule
branch
from
49a80a9 to
433c5fa
Compare
2 participants
Summary
Instead of trying to derive the prefix_rule for a command mechanically, let's let the model decide for us.
Testing