Conversation
aa0a3a7 to
4af9fac
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 766bbc471a
ℹ️ 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".
| #[serde(tag = "type", rename_all = "snake_case")] | ||
| #[ts(tag = "type")] | ||
| pub enum PermissionProfile { |
There was a problem hiding this comment.
Keep
PermissionProfile deserialization backward compatible
Switching PermissionProfile to a tagged enum requires a type discriminator, so previously persisted rollout lines that serialized permission_profile as { network, file_system } will fail to deserialize after upgrade. RolloutRecorder::load_rollout_items drops any line that fails serde_json::from_value::<RolloutLine>, so older TurnContext entries get silently skipped, which can corrupt resume/fork reconstruction by losing turn context metadata from existing threads. Please add a legacy-compatible deserialization path (e.g., untagged fallback) so old rollout files continue to load.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in the current head. PermissionProfile now uses a custom untagged deserialize path that accepts both the tagged enum shape and the previous rollout { network, file_system } shape, with permission_profile_deserializes_legacy_rollout_shape covering the old format including glob_scan_max_depth.
1e4cc1c to
fa7ac07
Compare
Why
PermissionProfileis becoming the canonical permissions abstraction, but the old shape only carried optional filesystem and network fields. It could describe allowed access, but not who is responsible for enforcing it. That madeDangerFullAccessandExternalSandboxlossy when profiles were exported, cached, or round-tripped through app-server APIs.The important model change is that active permissions are now a disjoint union over the enforcement mode. Conceptually:
This distinction matters because
Disabledmeans Codex should apply no outer sandbox at all, whileExternalmeans filesystem isolation is owned by an outside caller. Those are not equivalent to a broad managed sandbox. For example, macOS cannot nest Seatbelt inside Seatbelt, so an inner sandbox may require the outer Codex layer to use no sandbox rather than a permissive one.How Existing Modeling Maps
Legacy
SandboxPolicyremains a boundary projection, but it now maps into the higher-fidelity profile model:ReadOnlyandWorkspaceWritemap toPermissionProfile::Managedwith restricted filesystem entries plus the corresponding network policy.DangerFullAccessmaps toPermissionProfile::Disabled, preserving the “no outer sandbox” intent instead of treating it as a lax managed sandbox.ExternalSandbox { network_access }maps toPermissionProfile::External { network }, preserving external filesystem enforcement while still carrying the active network policy.SandboxPolicycannot faithfully express, such as managed unrestricted filesystem plus restricted network, stayManagedinstead of being collapsed intoExternalSandbox.AdditionalPermissionProfile; fullPermissionProfileis reserved for complete active runtime permissions.What Changed
PermissionProfileinto a tagged union:managed,disabled, andexternal.AdditionalPermissionProfilefor command/session/turn overlays.restrictedentries orunrestricted;glob_scan_max_depthis non-zero when present.{ network, file_system }profile shape during deserialization.DangerFullAccessround-trips asdisabled,ExternalSandboxround-trips asexternal, and managed unrestricted filesystem + restricted network stays managed instead of being mistaken for external enforcement.:root = writeplus deny entries.command/execREADME example for the taggedpermissionProfileshape.Compatibility
Legacy
SandboxPolicyremains available at config/API boundaries as the compatibility projection. Existing rollout lines with the oldPermissionProfileshape continue to load. The app-serverpermissionProfilefield is experimental, so its v2 wire shape is intentionally updated to match the higher-fidelity model.Verification
just write-app-server-schemacargo check --testscargo test -p codex-protocol permission_profilecargo test -p codex-protocol preserving_deny_entries_keeps_unrestricted_policy_enforceablecargo test -p codex-app-server-protocol permission_profile_file_system_permissionscargo test -p codex-app-server-protocol serialize_client_responsecargo test -p codex-core session_configured_reports_permission_profile_for_external_sandboxjust fixjust fix -p codex-protocoljust fix -p codex-app-server-protocoljust fix -p codex-corejust fix -p codex-app-server