Skip to content

tui/exec: show effective workspace roots in summaries#22612

Open
bolinfest wants to merge 1 commit into
pr22611from
pr22612
Open

tui/exec: show effective workspace roots in summaries#22612
bolinfest wants to merge 1 commit into
pr22611from
pr22612

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented May 14, 2026

Why

This PR builds on #22611.

After runtimeWorkspaceRoots moved onto thread state, the user-facing summaries were still inconsistent about which roots they showed. In particular, /status and the exec startup summary could under-report extra workspace roots from --add-dir or from profile-defined workspace_roots, which made the new model look incorrect even when the permissions themselves were right.

What Changed

  • switched the TUI status surfaces to summarize against Config::effective_workspace_roots()
  • updated the exec human-output summary to render from the effective permission profile instead of the raw constrained profile
  • added focused regressions for both the TUI and exec code paths so extra workspace roots stay visible in user-facing summaries

Verification

Targeted coverage for this follow-up lives in:

  • codex-rs/tui/src/status/tests.rs
  • codex-rs/exec/src/event_processor_with_human_output_tests.rs

The added regressions verify that:

  • status output includes profile-defined workspace roots in the effective permissions summary
  • exec startup output includes runtime workspace roots instead of collapsing back to cwd only

Stack created with Sapling. Best reviewed with ReviewStack.

Copy link
Copy Markdown
Contributor

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

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: 5691ca3924

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

config: &Config,
session_configured_event: &SessionConfiguredEvent,
) -> Vec<(&'static str, String)> {
let permission_profile = config.permissions.permission_profile();
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.

P2 Badge Avoid leaking internal roots in exec summary

For normal workspace-write configs, core/src/config/mod.rs adds the Codex memories directory to the stored permission profile as an internal writable root, and this now summarizes that fully materialized profile by enumerating all writable roots. In that scenario the exec startup banner will list $CODEX_HOME/memories alongside the user's workspace roots, which is not an effective workspace root and differs from the TUI path that summarizes effective_workspace_roots() instead; please summarize the effective workspace roots directly or filter internal roots here.

Useful? React with 👍 / 👎.

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:08
@bolinfest bolinfest requested a review from a team as a code owner May 14, 2026 16:36
@bolinfest bolinfest force-pushed the pr22612 branch 2 times, most recently from 5a5197b to 5cba401 Compare May 14, 2026 18:06
@bolinfest bolinfest force-pushed the pr22611 branch 2 times, most recently from 247bbda to 842662a Compare May 14, 2026 18:28
bolinfest added a commit that referenced this pull request May 15, 2026
## Why

This is the configuration/model half of the alternative permissions
migration we discussed as a comparison point for
[#22401](#22401) and
[#22402](#22402).

The old `workspace-write` model mixes three concerns that we want to
keep separate:
- reusable profile rules that should stay immutable once selected
- user/runtime workspace roots from `cwd`, `--add-dir`, and legacy
workspace-write config
- internal Codex writable roots such as memories, which should not be
shown as user workspace roots

This PR gives permission profiles first-class `workspace_roots` so users
can opt multiple repositories into the same `:workspace_roots` rules
without using broad absolute-path write grants. It also starts
separating the raw selected profile from the effective runtime profile
by making `Permissions` expose explicit accessors instead of public
mutable fields.

A representative `config.toml` looks like this:

```toml
default_permissions = "dev"

[permissions.dev.workspace_roots]
"~/code/openai" = true
"~/code/developers-website" = true

[permissions.dev.filesystem.":workspace_roots"]
"." = "write"
".codex" = "read"
".git" = "read"
".vscode" = "read"
```

If Codex starts in `~/code/codex` with that profile selected, the
effective workspace-root set becomes:
- `~/code/codex` from the runtime `cwd`
- `~/code/openai` from the profile
- `~/code/developers-website` from the profile

The `:workspace_roots` rules are materialized across each root, so
`.git`, `.codex`, and `.vscode` stay scoped the same way everywhere.
Runtime additions such as `--add-dir` can still layer on later stack
entries without mutating the selected profile.

## Stack Shape

This PR intentionally stops before the profile-identity cleanup in
[#22683](#22683) so the base review
stays focused on config loading, workspace-root materialization, and
compatibility with legacy `workspace-write`.

The representation in this PR is therefore transitional: `Permissions`
carries enough state to distinguish the raw constrained profile from the
effective runtime profile, and there are still call sites that must keep
the active profile identity and constrained profile value in sync. The
follow-up PR replaces that with a single resolved profile state
(`ResolvedPermissionProfile` / `PermissionProfileState`) that keeps the
profile id, immutable `PermissionProfile`, and profile-declared
workspace roots together. That follow-up removes APIs such as
`set_constrained_permission_profile_with_active_profile()` where
separate arguments could drift out of sync.

Downstream PRs then build on this base to switch app-server turn updates
to profile ids plus runtime workspace roots and to finish the
user-visible summary behavior. Reviewers should judge this PR as the
workspace-roots foundation, not as the final in-memory shape of selected
permission profiles.

## Review Guide

Suggested review order:

1. Start with `codex-rs/core/src/config/mod.rs`.
This is the main shape change in the base slice. `Permissions` now
stores a private raw `Constrained<PermissionProfile>` plus runtime
`workspace_roots`. Callers use `permission_profile()` when they need the
raw constrained value and `effective_permission_profile()` when they
need a materialized runtime profile. As noted above,
[#22683](#22683) replaces this
transitional shape with a resolved profile state that keeps identity and
profile data together.

2. Review `codex-rs/config/src/permissions_toml.rs` and
`codex-rs/core/src/config/permissions.rs`.
These add `[permissions.<id>.workspace_roots]`, resolve enabled entries
relative to the policy cwd, and keep `:workspace_roots` deny-read glob
patterns symbolic until the actual roots are known.

3. Review `codex-rs/protocol/src/permissions.rs` and
`codex-rs/protocol/src/models.rs`.
These add the policy/profile materialization helpers that expand exact
`:workspace_roots` entries and scoped deny-read globs over every
workspace root. This is also where `ActivePermissionProfileModification`
is removed from the core model.

4. Review the legacy bridge in
`Config::load_from_base_config_with_overrides` and
`Config::set_legacy_sandbox_policy`.
This is where legacy `workspace-write` roots become runtime workspace
roots, while Codex internal writable roots stay internal and do not
appear as user-facing workspace roots.

5. Then skim downstream call sites.
The interesting pattern is raw-vs-effective access: state/proxy/bwrap
paths keep the raw constrained profile, while execution, summaries, and
user-visible status use the effective profile and workspace-root list.

## What Changed

- added `[permissions.<id>.workspace_roots]` to the config model and
schema
- added runtime `workspace_roots` state to `Config`/`Permissions` and
`ConfigOverrides`
- made `Permissions` profile fields private and replaced direct mutation
with accessors/setters
- added `PermissionProfile` and `FileSystemSandboxPolicy` helpers for
materializing `:workspace_roots` exact paths and deny-read globs across
all roots
- moved legacy additional writable roots into runtime workspace-root
state instead of active profile modifications
- removed `ActivePermissionProfileModification` and its app-server
protocol/schema export
- updated sandbox/status summary paths so internal writable roots are
not reported as user workspace roots

## Verification Strategy

The targeted tests cover the behavior at the layers where regressions
are most likely:
- `codex-rs/core/src/config/config_tests.rs` verifies config loading,
legacy workspace-root seeding, effective profile materialization, and
memory-root handling.
- `codex-rs/core/src/config/permissions_tests.rs` verifies profile
`workspace_roots` parsing and `:workspace_roots` scoped/glob
compilation.
- `codex-rs/protocol/src/permissions.rs` unit tests verify exact and
glob materialization over multiple workspace roots.
- `codex-rs/tui/src/status/tests.rs` and
`codex-rs/utils/sandbox-summary/src/sandbox_summary.rs` verify the
user-facing summaries show effective workspace roots and hide internal
writes.

I also ran `cargo check --tests` locally after the latest stack refresh
to catch cross-crate API breakage from the private-field/accessor
changes.







---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/openai/codex/pull/22610).
* #22612
* #22611
* #22683
* __->__ #22610
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
Copy link
Copy Markdown
Collaborator

@viyatb-oai viyatb-oai left a comment

Choose a reason for hiding this comment

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

Approved. I did not find an additional regression introduced by this diff beyond the stacked #22611 state propagation issue already noted there.

@bolinfest bolinfest force-pushed the pr22612 branch 2 times, most recently from c10ca6e to e1395d8 Compare May 15, 2026 02:04
@bolinfest bolinfest force-pushed the pr22611 branch 2 times, most recently from 1a97dd8 to cbe9413 Compare May 15, 2026 02:18
@bolinfest bolinfest force-pushed the pr22612 branch 2 times, most recently from 1925bcd to 1b90490 Compare May 15, 2026 02:34
@bolinfest bolinfest force-pushed the pr22611 branch 2 times, most recently from d525c4b to 1d60544 Compare May 15, 2026 02:43
@bolinfest bolinfest force-pushed the pr22611 branch 2 times, most recently from bc34cfa to 49e6f61 Compare May 15, 2026 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants