Conversation
![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: 9f863363a1
ℹ️ 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".
| cwd, | ||
| ); | ||
| intersect_permission_profiles(effective_permissions.clone(), granted_permissions, cwd) | ||
| == materialized_effective_permissions |
There was a problem hiding this comment.
This comparison still fails for the real sticky-grant shape this PR is trying to preserve. After a prior approval stores :cwd write plus **/*.env deny as /repo write plus /repo/**/*.env, a later command that repeats the original relative request gets merged with the stored grant before this helper runs. intersect_permission_profiles(effective, effective, cwd) then materializes both :cwd and /repo into duplicate /repo write entries, while intersect_permission_profiles(effective, granted, cwd) only has one. The equality check returns false, so an already-approved permission profile is no longer treated as preapproved and can be rejected when only request-permissions preapproval is enabled. De-duplicate after materializing entries, or compare normalized canonical profiles.
There was a problem hiding this comment.
let mut accepted_entries = Vec::new();
for entry in granted_file_system.entries.iter().filter(|entry| {
granted_file_system_entry_within_request(
&requested_file_system,
&requested_policy,
requested_read_deny_matcher.as_ref(),
entry,
cwd,
)
}) {
let entry = materialize_cwd_dependent_entry(entry, cwd);
if !accepted_entries.contains(&entry) {
accepted_entries.push(entry);
}
}
let mut entries = accepted_entries.clone();
viyatb-oai
left a comment
viyatb-oai
left a comment
There was a problem hiding this comment.
looks good - one small fix needed!
3 participants
Why
#18275 anchors session-scoped
:cwdand:project_rootsgrants to the request cwd before recording them for reuse. Relative deny glob entries need the same treatment. Without anchoring, a stored session permission can keep a pattern such as**/*.envrelative, then reinterpret that deny against a later turn cwd. That makes the persisted profile depend on the cwd at reuse time instead of the cwd that was reviewed and approved.What changed
intersect_permission_profilesnow materializes retainedFileSystemPath::GlobPatternentries against the request cwd, matching the existing materialization for cwd-sensitive special paths.Materialized accepted grants are now deduplicated before deny retention runs. This keeps the sticky-grant preapproval shape stable when a repeated request is merged with the stored grant and both
:cwd = writeand the materialized absolute cwd write are present.The preapproval check compares against the same materialized form, so a later request for the same cwd-relative deny glob still matches the stored anchored grant instead of re-prompting or rejecting.
Tests cover both the storage path and the preapproval path: a session-scoped
:cwd = writegrant with**/*.env = noneis stored with both the cwd write and deny glob anchored to the original request cwd, cannot be reused from a later cwd, and remains preapproved when re-requested from the original cwd after merging with the stored grant.Verification
cargo test -p codex-sandboxing policy_transformscargo test -p codex-core --lib relative_deny_glob_grants_remain_preapproved_after_materializationcargo clippy -p codex-sandboxing --tests -- -D clippy::redundant_clonecargo clippy -p codex-core --lib -- -D clippy::redundant_cloneStack created with Sapling. Best reviewed with ReviewStack.