Conversation
af42be1 to
46f2ed2
Compare
bd7080b to
e2ae150
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2ae150346
ℹ️ 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".
| ThreadParamsMode::Remote => { | ||
| PermissionProfile::from_legacy_sandbox_policy_for_cwd(&sandbox.to_core(), cwd) | ||
| } |
There was a problem hiding this comment.
Preserve full permission profile for remote bootstrap
For ThreadParamsMode::Remote, this maps session permissions from the legacy sandbox field only, which is lossy for profiles that include restricted read rules. The protocol itself documents that restricted-read variants are no longer representable in sandbox and require permissionProfile (see app-server-protocol/src/protocol/v2.rs around the readOnly.access / workspaceWrite.readOnlyAccess rejection paths), so threads started/resumed/forked against a remote server can now be widened in the TUI state to generic read-only/workspace-write permissions instead of the actual constrained profile.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[codex] Addressed in 8cae849. I restored the experimental response-side permissionProfile on thread/start, thread/resume, and thread/fork as the exact runtime permissions payload, kept activePermissionProfile as provenance/display metadata, and updated bootstrap mapping to prefer response permissionProfile with legacy/local fallbacks only for older or missing app-server responses. Added TUI and exec coverage for preserving the response profile instead of reconstructing it from sandbox or local config.
138eef5 to
80d8db2
Compare
There was a problem hiding this comment.
Walked through the story of the PR in the suggested order. Code looks good.
I only have a minor concern from the TUI user's perspective, which may be either me being naïve or something we already plan on addressing at a later time:
When I first run /status I see permissions as Workspace (on-request), which is somewhat foreign to my current understanding (as a user) of how permissions work.
Then I use /permissions to change my permissions to, let's say, "Full Access". When I do /status again, I see Permissions: Full Access which is reassuring.
Next I change the permissions to Auto-review and on /status I now see Workspace (on-request) again, which leads me to believe something is wrong, because that was the Default permissions when I started the session.
Not sure if this makes sense, since I don't have the end-goal in mind but I thought it was worth sharing.
I am approving the PR since the code looks good and the guided walkthrough also made me make sense of the changes.
9f1ad76 to
55470a4
Compare
|
@fcoury-oai thanks for calling this out. I agreed the underlying state was correct but the /status label was ambiguous: Auto-review intentionally uses the Workspace permission profile with on-request approvals routed through the auto-reviewer. I updated the status formatter so that state now renders as |
Why
PermissionProfileis the canonical runtime permissions payload, but it intentionally describes the compiled permissions rather than where those permissions came from. That makes it awkward for clients to show a concise, stable label like:workspaceor a user-defined profile name without reverse-engineering the profile shape.This PR adds explicit sidecar metadata for the selected profile:
The runtime still honors the exact
PermissionProfile;ActivePermissionProfileexists for display/provenance. This lets clients render labels likeWorkspace,Read Only, orProfile safe-worktreewithout trying to infer those names from the compiled permissions. Theextendsfield is reserved for future profile inheritance, andmodificationscurrently records bounded user-requested changes such as additional writable roots.The app-server request model also now reflects the direction we want for permissions: clients should select a named profile and use supported mutations, rather than replacing the full runtime profile wholesale.
This keeps UI state explainable and avoids having clients invent permission shapes that cannot be tied back to a known profile.
What changed
Protocol and Runtime Model
ActivePermissionProfile { id, extends, modifications }to the protocol model.PermissionProfileSelectionParamsfor selecting a named built-in or user-defined profile plus supported bounded modifications.PermissionProfileas the exact runtime permissions payload.ActivePermissionProfileis metadata only and is cleared when it can no longer faithfully describe the effective permissions.Config and Session State
default_permissionsor the implicit built-in default, including:workspace,:read-only,:danger-no-sandbox, and user-defined[permissions.<id>]profiles.:workspaceprofiles that depend on legacy[sandbox_workspace_write]customizations. Explicitly selecting:workspacethrough the new API intentionally ignores those legacy roots/network/tmp settings, so omitting metadata preserves behavior by making clients fall back to the legacy sandbox projection.:read-onlyinstead of being treated as a legacy workspace-only knob. The implicit:workspacepath still preserves legacy workspace metadata carveout behavior.App-Server API
thread/start,thread/resume,thread/fork, andturn/startrequest params now use experimentalpermissions: PermissionProfileSelectionParamsinstead of experimental rawpermissionProfileoverrides.permissionProfileas the exact active runtime permissions and now expose experimentalactivePermissionProfileas the display/provenance metadata.sandboxfield remains the compatibility projection.turn/startpermission selections are rejected before the turn starts if requirements force the selected profile to fall back, preserving the previous request-time validation behavior.TUI and Exec Clients
sandbox_mode/-s, exec continues to send the legacysandboxprojection so remote app-server sessions preserve the requested mode.turn/startuses active-profile selection for embedded app-server sessions when available, but falls back to the legacy sandbox projection when no active profile id exists so local permission-preset changes are not dropped.Read Only,Workspace, orNo Sandbox/Full Access, while user-defined profiles render asProfile <id>and Auto-review shows asWorkspace (auto-review)./statusno longer labels active built-in profiles asCustom, usesworkspacein user-facing permissions text instead of the legacyworkspace-writelabel, distinguishes Auto-review from manual on-request review, and shows bounded modifications such as+ 1 writable rootwhen present.Schema and Docs
permissionsselections andactivePermissionProfileresponses.Review guide
codex-rs/protocol/src/models.rsandcodex-rs/app-server-protocol/src/protocol/v2.rsfor the shape ofActivePermissionProfileandPermissionProfileSelectionParams.codex-rs/core/src/config/mod.rsandcodex-rs/core/src/session/session.rsfor the key invariant: runtime enforcement still comes fromPermissionProfile, while active-profile metadata is only kept when the selected profile remains faithful after constraints and can be re-selected without losing legacy config semantics.codex-rs/app-server/src/codex_message_processor.rs,codex-rs/tui/src/app_server_session.rs,codex-rs/exec/src/lib.rs, andcodex-rs/tui/src/status/card.rsfor the API plumbing and UI display behavior.Verification
codex-coretests cover active metadata for built-ins, user-defined profiles, additional writable roots, legacy workspace settings, requirement-forced fallback, and operation-kind routing.cargo test -p codex-app-server-protocol, targetedcodex-app-servertests for invalid turn permission selection, and generated schema fixture updates.codex-tuiandcodex-exectests cover embedded named-profile selection, remote legacy fallback, no-active-profile fallback, session response hydration, and status labels including writable-root modifications./statuslabel changes, includingWorkspace (auto-review).just fixruns for the changed Rust crates plusjust argument-comment-lint.