Conversation
This was referenced Apr 25, 2026
viyatb-oai
approved these changes
Apr 25, 2026
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91a07d0d32
ℹ️ 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".
| @@ -23,6 +37,29 @@ pub(crate) fn sandbox_tag( | |||
| .unwrap_or("none") | |||
Contributor
There was a problem hiding this comment.
This path reports a platform sandbox tag for every PermissionProfile::Managed, but managed profiles can still run with no sandbox when they are effectively unrestricted (for example full-disk write + network enabled, where sandbox selection does not require platform isolation). In that case, after-tool hook payloads and turn metadata will claim a sandbox backend was used even though execution was unsandboxed, which can mislead downstream policy hooks and analytics that trust the sandbox field.
Useful? React with 👍 / 👎.
bolinfest
added a commit
that referenced
this pull request
Apr 25, 2026
## Why Windows can represent the same canonical local path with either a normal drive path or a verbatim device path prefix. The failure pattern that motivated this PR was an assertion diff like `C:\...` versus `\\?\C:\...`: different spellings, same file. That became visible while validating the permissions stack above this PR. The stack increasingly routes paths through `AbsolutePathBuf`, which normalizes supported Windows device prefixes, while several existing tests still built expected values directly with `std::fs::canonicalize()` or compared `AbsolutePathBuf::as_path()` to a raw `PathBuf`. On Windows, that can make tests fail because the two sides choose different textual forms for an otherwise equivalent canonical path. This PR is intentionally split out as the bottom PR below #19606. The runtime permissions migration should not carry unrelated Windows test stabilization, and reviewers should be able to verify this as a test-only change before looking at the larger permissions changes. ## Failure Modes Covered - `conversation_summary` expected rollout paths were built from raw canonicalized `PathBuf`s, while app-server responses could carry `AbsolutePathBuf`-normalized paths. - `thread_resume` compared returned thread paths directly to previously stored or fixture paths, so a verbatim-prefix spelling could fail an otherwise correct resume. - `marketplace_add` compared plugin install roots through `as_path()` against raw canonicalized paths, reproducing the same `C:\...` versus `\\?\C:\...` mismatch in both app-server and core-plugin coverage. ## What Changed - In `app-server/tests/suite/conversation_summary.rs`, normalize both expected rollout paths and received `ConversationSummary.path` values through `AbsolutePathBuf` before comparing the full summary object. - In `app-server/tests/suite/v2/thread_resume.rs`, normalize both sides of thread path comparisons before asserting equality. This keeps the tests focused on whether resume returned the same existing path, not whether Windows used the same string spelling. - In `app-server/tests/suite/v2/marketplace_add.rs` and `core-plugins/src/marketplace_add.rs`, compare install roots as `AbsolutePathBuf` values instead of comparing an absolute-path wrapper to a raw canonicalized `PathBuf`. ## Behavior This PR does not change production app-server or marketplace behavior. It only changes tests to assert semantic path identity across Windows path spelling variants. It also leaves API response values untouched; the normalization happens inside assertions only. ## Verification Targeted local checks run while extracting this fix: - `cargo test -p codex-app-server get_conversation_summary_by_thread_id_reads_rollout` - `cargo test -p codex-app-server get_conversation_summary_by_relative_rollout_path_resolves_from_codex_home` - `cargo test -p codex-app-server thread_resume_prefers_path_over_thread_id` Windows-specific confidence comes from the Bazel Windows CI job for this PR, since the failure is platform-specific. ## Docs No docs update is needed because this is test-only infrastructure stabilization. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19604). * #19395 * #19394 * #19393 * #19392 * #19606 * __->__ #19604
bolinfest
force-pushed
the
pr19606
branch
5 times, most recently
from
5f1fed5 to
0066622
Compare
This was referenced Apr 26, 2026
bolinfest
force-pushed
the
pr19606
branch
6 times, most recently
from
8eff69b to
848e51c
Compare
bolinfest
force-pushed
the
pr19606
branch
18 times, most recently
from
897e6b7 to
b5f87a0
Compare
bolinfest
force-pushed
the
pr19606
branch
2 times, most recently
from
7fdbbbc to
c966040
Compare
bolinfest
added a commit
that referenced
this pull request
Apr 26, 2026
## Why Windows Bazel runs in the permissions stack exposed that app-server integration tests were launching normal plugin startup warmups in every subprocess. Those warmups can call `https://chatgpt.com/backend-api/plugins/featured` when a test is not specifically exercising plugin startup, which adds slow background work, noisy stderr, and dependence on external network state. The relevant startup/featured-plugin behavior was introduced across #15042 and #15264. A few app-server tests also had long optional waits or unbounded cleanup paths, making failures expensive to diagnose and contributing to slow Windows shards. One external-agent config test from #18246 used a GitHub-style marketplace source, which was enough to exercise the pending remote-import path but also meant the background completion task could attempt a real clone. ## What Changed - Adds explicit `AppServerRuntimeOptions` / `PluginStartupTasks` plumbing and a hidden debug-only `--disable-plugin-startup-tasks-for-tests` app-server flag, so integration tests can suppress startup plugin warmups without adding a production env-var gate. - Has the app-server test harness pass that hidden flag by default, while opting plugin-startup coverage back in for tests that intentionally exercise startup sync and featured-plugin warmup behavior. - Lowers normal app-server subprocess logging from `info`/`debug` to `warn` to avoid multi-megabyte stderr output in Bazel logs. - Prevents the external-agent config test from attempting a real marketplace clone by using an invalid non-local source while still exercising the pending-import completion path. - Bounds optional filesystem/realtime waits and fake WebSocket test-server shutdown so failures produce targeted timeouts instead of hanging a shard. - Fixes the Unix script-resolution test in `rmcp-client` to exercise PATH resolution directly and include the actual spawn error in failures. ## Verification - `cargo check -p codex-app-server` - `cargo clippy -p codex-app-server --tests -- -D warnings` - `cargo test -p codex-rmcp-client program_resolver::tests::test_unix_executes_script_without_extension` - `cargo test -p codex-app-server --test all external_agent_config_import_sends_completion_notification_after_pending_plugins_finish -- --nocapture` - `cargo test -p codex-app-server --test all plugin_list_uses_warmed_featured_plugin_ids_cache_on_first_request -- --nocapture` - Windows Local Bazel passed with this test-hardening bundle before it was extracted from #19606. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19683). * #19395 * #19394 * #19393 * #19392 * #19606 * __->__ #19683
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
This supersedes #19391. During stack repair, GitHub marked #19391 as merged into a temporary stack branch rather than into
main, so the runtime-config change needed a fresh PR.PermissionProfileis now the canonical permissions shape after #19231 because it can distinguishManaged,Disabled, andExternalenforcement while also carrying filesystem rules that legacySandboxPolicycannot represent cleanly. Core config and session state still needed to accept profile-backed permissions without forcing every profile through the strict legacy bridge, which rejected valid runtime profiles such as direct write roots.The unrelated CI/test hardening that previously rode along with this PR has been split into #19683 so this PR stays focused on the permissions model migration.
What Changed
Permissions.permission_profileandSessionConfiguration.permission_profileas constrained runtime state, while keepingsandbox_policyas a legacy compatibility projection.PermissionProfile, split filesystem/network policies, and legacySandboxPolicyprojections synchronized.SandboxPolicyexactly.glob_scan_max_depthwhen command/session profiles are narrowed.PermissionProfile::read_only()andPermissionProfile::workspace_write()presets that match legacy defaults.Verification
cargo test -p codex-core direct_write_rootscargo test -p codex-core runtime_roots_to_legacy_projectioncargo test -p codex-app-server requested_permissions_trust_project_uses_permission_profile_intentStack created with Sapling. Best reviewed with ReviewStack.