Skip to content

app-server: use permission ids and runtime workspace roots#22611

Merged
bolinfest merged 1 commit into
mainfrom
pr22611
May 15, 2026
Merged

app-server: use permission ids and runtime workspace roots#22611
bolinfest merged 1 commit into
mainfrom
pr22611

Conversation

@bolinfest

@bolinfest bolinfest commented May 14, 2026

Copy link
Copy Markdown
Collaborator

Why

This PR builds on #22610 and is the app-server side of the migration from mutable per-turn SandboxPolicy replacement toward selecting immutable permission profiles by id plus mutable runtime workspace roots.

Once permission profiles can carry their own immutable workspace_roots, app-server no longer needs to mutate the selected PermissionProfile just to represent thread-specific filesystem context. The mutable part now lives on the thread as explicit runtimeWorkspaceRoots, while :workspace_roots remains symbolic until the sandbox is realized for a turn.

What Changed

  • Replaced the v2 permission-selection wrapper surface with plain profile ids for thread/start, thread/resume, thread/fork, and turn/start.
  • Removed the API surface for profile modifications (PermissionProfileSelectionParams, PermissionProfileModificationParams, ActivePermissionProfileModification).
  • Added experimental runtimeWorkspaceRoots fields to the thread lifecycle and turn-start APIs.
  • Threaded runtime workspace roots through core session/thread snapshots, turn overrides, app-server request handling, and command execution permission resolution.
  • Kept session permission state symbolic so later runtime root updates and cwd-only implicit-root retargeting rebind :workspace_roots correctly.
  • Updated the embedded clients just enough to send and restore the new thread state.
  • Refreshed the generated schema/TypeScript artifacts and the app-server README to match the new contract.

Verification

Targeted coverage for this layer lives in:

  • codex-rs/app-server-protocol/src/protocol/v2/tests.rs
  • codex-rs/app-server/tests/suite/v2/thread_start.rs
  • codex-rs/app-server/tests/suite/v2/thread_resume.rs
  • codex-rs/app-server/tests/suite/v2/turn_start.rs
  • codex-rs/core/src/session/tests.rs

The key regression checks exercise that:

  • runtimeWorkspaceRoots resolve against the effective cwd on thread start.
  • Profile-declared workspace roots are excluded from the runtime workspace roots returned by app-server.
  • A turn-level runtime workspace-root update persists onto the thread and is returned by thread/resume.
  • A named permission profile selected on one turn remains symbolic so a later runtime-root-only turn update changes the actual sandbox writes.
  • A cwd-only turn update retargets the implicit runtime cwd root while preserving additional runtime roots.
  • The protocol fixtures and generated client artifacts stay in sync with the string-based permission selection contract.

Stack created with Sapling. Best reviewed with ReviewStack.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4554fcc19e

ℹ️ 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".

Comment thread codex-rs/app-server/src/request_processors/turn_processor.rs
Comment thread codex-rs/core/src/session/session.rs
bolinfest added a commit that referenced this pull request May 14, 2026
#22624)

## Why

This is a small precursor to the larger permissions-migration work. Both
the comparison stack in
[#22401](#22401) /
[#22402](#22402) and the alternate
stack in [#22610](#22610) /
[#22611](#22611) /
[#22612](#22612) are easier to
review if the terminology is already settled underneath them.

Because `:project_roots` and `:danger-no-sandbox` have not shipped as
stable user-facing surface area, carrying them forward as aliases would
just add more migration logic to the later stacks. This PR removes that
ambiguity now so the follow-on work can rely on one spelling for each
built-in concept.

## What Changed

- renamed the config-facing special filesystem key from `:project_roots`
to `:workspace_roots`
- dropped unpublished `:project_roots` parsing support in
`core/src/config/permissions.rs`, so new config only recognizes
`:workspace_roots`
- renamed the built-in full-access permission profile id from
`:danger-no-sandbox` to `:danger-full-access`
- dropped unpublished `:danger-no-sandbox` support entirely, including
the old active-profile canonicalization path, and added explicit
rejection coverage for the legacy id
- introduced shared built-in permission-profile id constants in
`codex-rs/protocol/src/models.rs`
- updated `core`, `app-server`, and `tui` call sites that special-case
built-in profiles to use the shared constants and canonical ids
- updated tests and the Linux sandbox README to use `:workspace_roots` /
`:danger-full-access`

## Verification

I focused verification on the three places this rename can regress:
config parsing, active-profile identity surfaced back out of `core`, and
user/server call sites that special-case built-in profiles.

Targeted checks:

-
`config::tests::default_permissions_can_select_builtin_profile_without_permissions_table`
-
`config::tests::default_permissions_read_only_applies_additional_writable_roots_as_modifications`
-
`config::tests::default_permissions_can_select_builtin_full_access_profile`
- `config::tests::legacy_danger_no_sandbox_is_rejected`
- `workspace_root` filtered `codex-core` tests
-
`request_processors::thread_processor::thread_processor_tests::thread_processor_behavior_tests::requested_permissions_trust_project_uses_permission_profile_intent`
-
`suite::v2::turn_start::turn_start_rejects_invalid_permission_selection_before_starting_turn`
- `status::tests::status_snapshot_shows_auto_review_permissions`
-
`status::tests::status_permissions_full_disk_managed_with_network_is_danger_full_access`
-
`app_server_session::tests::embedded_turn_permissions_use_active_profile_selection`
@bolinfest bolinfest requested a review from viyatb-oai May 14, 2026 16:07
@bolinfest bolinfest force-pushed the pr22610 branch 2 times, most recently from 99a5efa to 559afb6 Compare May 14, 2026 16:34
@bolinfest bolinfest force-pushed the pr22611 branch 2 times, most recently from 5b6acbb to cd12cc2 Compare May 14, 2026 16:45
@bolinfest bolinfest force-pushed the pr22611 branch 2 times, most recently from 572a5c3 to 8429c89 Compare May 14, 2026 17:24
@bolinfest bolinfest force-pushed the pr22610 branch 2 times, most recently from 9ccddb3 to 2fbcb85 Compare May 14, 2026 18:06
@bolinfest bolinfest force-pushed the pr22611 branch 2 times, most recently from f42586d to fefc579 Compare May 14, 2026 19:45
@bolinfest bolinfest changed the base branch from pr22610 to pr22683 May 14, 2026 19:45
bolinfest added a commit that referenced this pull request May 15, 2026
## Why

This PR is the invariant-cleanup layer that follows the workspace-roots
base merged in [#22610](#22610).

#22610 adds `[permissions.<id>.workspace_roots]` and keeps runtime
workspace roots separate from the raw permission profile, but its
in-memory representation is intentionally transitional: `Permissions`
still carries the selected profile identity next to a constrained
`PermissionProfile`. That makes APIs such as
`set_constrained_permission_profile_with_active_profile()` fragile
because the id and value only mean the right thing when every caller
keeps them in sync.

This PR introduces a single resolved profile state so profile identity,
`extends`, the profile value, and profile-declared workspace roots
travel together. The next PR,
[#22611](#22611), builds on this by
changing the app-server turn API to select permission profiles by id
plus runtime workspace roots.

## Stack Context

- #22610, now merged: adds profile-declared `workspace_roots`, runtime
workspace roots, and `:workspace_roots` materialization.
- This PR: replaces the parallel active-profile/profile-value fields
with `PermissionProfileState`.
- #22611: switches app-server turn updates toward profile ids plus
runtime workspace roots.
- #22612: updates TUI/exec summaries to show the effective workspace
roots.

Keeping this separate from #22611 is deliberate: reviewers can validate
the internal state invariant before reviewing the app-server protocol
migration.

## What Changed

- Added `ResolvedPermissionProfile::{Legacy, BuiltIn, Named}` and
`PermissionProfileState`.
- Typed built-in profile ids with `BuiltInPermissionProfileId`.
- Moved selected profile identity and profile-declared workspace roots
into the resolved state.
- Replaced `Permissions` parallel profile fields with one
`permission_profile_state`.
- Removed `set_constrained_permission_profile_with_active_profile()`
from session sync paths.
- Kept trusted session replay/`SessionConfigured` compatibility through
explicit session snapshot helpers.
- Updated session configuration, MCP initialization, app-server, exec,
TUI, and guardian call sites to consume `&PermissionProfile` directly.

## Review Guide

Start with `codex-rs/core/src/config/resolved_permission_profile.rs`; it
is the new invariant boundary. Then review
`codex-rs/core/src/config/mod.rs` to see how config loading records
active profile identity and profile workspace roots. The remaining
call-site changes are mostly mechanical fallout from
`Permissions::permission_profile()` returning `&PermissionProfile`
instead of `&Constrained<PermissionProfile>`.

## Verification

The existing config/session coverage now constructs and asserts through
`PermissionProfileState`. The workspace-root config test also asserts
that profile-declared roots are preserved in the resolved state, which
is the behavior #22611 relies on when runtime roots become mutable
through the app-server API.

---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22683).
* #22612
* #22611
* __->__ #22683
Base automatically changed from pr22683 to main May 15, 2026 01:47
Comment thread codex-rs/core/src/session/mod.rs Outdated

@viyatb-oai viyatb-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock. I left one P2 regression suggestion inline: keep session permission state symbolic so turn-level runtimeWorkspaceRoots-only updates actually rebind :workspace_roots instead of silently continuing to use the old concrete roots.

@viyatb-oai viyatb-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a P1 compile blocker on a second pass, so I am withdrawing the unblock approval for now: codex-rs/tui/src/chatwidget/tests/history_replay.rs still has one ThreadSessionState initializer in session_configured_preserves_profile_workspace_roots without the new required runtime_workspace_roots field. The neighboring initializers were updated, but this one was missed, so the TUI test target will not compile until it is added (the natural value there is session_workspace_roots.clone()).

The earlier P2 suggestion about keeping session permission state symbolic still stands.

@viyatb-oai viyatb-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head bc34cfa2c8. The two issues I raised are fixed: session permission state now stays symbolic for runtimeWorkspaceRoots rebinding, and the missing runtime_workspace_roots test field was added. Approving.

@bolinfest bolinfest force-pushed the pr22611 branch 7 times, most recently from 2391b44 to e4b24d9 Compare May 15, 2026 04:14
@bolinfest bolinfest merged commit 8a5306f into main May 15, 2026
47 of 48 checks passed
@bolinfest bolinfest deleted the pr22611 branch May 15, 2026 06:00
@github-actions github-actions Bot locked and limited conversation to collaborators May 15, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants