Make deny canonical for filesystem permission entries#23493
Conversation
Co-authored-by: Codex noreply@openai.com
| io::ErrorKind::InvalidInput, | ||
| format!( | ||
| "filesystem glob path `{path}` only supports `none` access; use an exact path or trailing `/**` for `{access}` subtree access" | ||
| "filesystem glob path `{path}` only supports `none` or `deny` access; use an exact path or trailing `/**` for `{access}` subtree access" |
There was a problem hiding this comment.
Done in 12f9df1. This message now uses only deny.
| assert!( | ||
| err.to_string() | ||
| .contains("filesystem glob path `src/**/*.rs` only supports `none` access"), | ||
| .contains("filesystem glob path `src/**/*.rs` only supports `none` or `deny` access"), |
There was a problem hiding this comment.
Done in 12f9df1. The expected error string now uses only deny.
Co-authored-by: Codex noreply@openai.com
viyatb-oai
changed the title
deny for filesystem permission entriesdeny canonical for filesystem permission entries
| "oneOf": [ | ||
| { | ||
| "enum": [ | ||
| "read", | ||
| "write" | ||
| ], | ||
| "type": "string" | ||
| }, | ||
| { | ||
| "description": "`none` is a legacy input alias retained temporarily for compatibility.", | ||
| "enum": [ | ||
| "deny" | ||
| ], | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
This codegen seems a bit unfortunate...
![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: 12f9df1292
ℹ️ 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".
| Read, | ||
| Write, | ||
| None | ||
| Deny |
There was a problem hiding this comment.
none alias in v2 filesystem access
When an app-server client built against the previous v2 schema replies to item/permissions/requestApproval (or sends an experimental additional-permissions payload) with an entry whose access is still "none", this v2 enum now rejects the JSON before it can be converted to the core type that still has #[serde(alias = "none")]. Since the prior generated TypeScript exposed "none" as the valid value, mixed-version clients will fail permission approvals involving deny-read entries unless this wrapper enum also accepts the legacy alias on Deny.
Useful? React with 👍 / 👎.
| "enum": [ | ||
| "deny" |
There was a problem hiding this comment.
none in the config schema
For users who still have existing config.toml permission profiles with "none", the Rust deserializer accepts them via the new alias, but the generated config.schema.json no longer does, so schema-based validation/editors will mark otherwise supported configs as invalid. If none is intentionally retained as a temporary compatibility input, this enum should include the legacy value (or another schema allowance) alongside canonical deny.
Useful? React with 👍 / 👎.
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Once we drop support for the legacy alias, hopefully the codegen will be cleaned up!
2 participants
Why
Filesystem permission profiles used
nonefor deny-read entries, which is less direct than the action the entry actually represents. This change makesdenythe canonical filesystem permission spelling while preserving compatibility for older configs that still sendnone.What changed
FileSystemAccessMode::NonetoDenydenyas the canonical valuenoneonly as a legacy input alias for temporary config compatibilityValidation
cargo test -p codex-protocolcargo test -p codex-app-server-protocolcargo test -p codex-core config_toml_deserializes_permission_profiles --libcargo test -p codex-core read_write_glob_patterns_still_reject_non_subpath_globs --libEarlier in the session, a broad
cargo test -p codex-corerun reached unrelated pre-existing failures in timing/snapshot/git-info tests under this environment; the targeted surfaces touched by this PR passed cleanly.