From 229b40aa21a5e4d6957bc20a6d3d22b98da2715f Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Mon, 4 May 2026 12:48:59 -0700 Subject: [PATCH] core: fix apply_patch request permissions test (#21060) ## Why The Bazel test coverage change exposed `approved_folder_write_request_permissions_unblocks_later_apply_patch`, and `rust-ci-full.yml` showed the same test failing on `main` on macOS. There were two separate classes of problems here. ### Clean CI failure The test emits an `apply_patch` tool call, but its config did not enable the `apply_patch` tool, so the mocked response completed without an `apply-patch-call` output. After enabling the tool, the same path also needs the aggregate `codex-core` test binary to dispatch `--codex-run-as-fs-helper`; sandboxed `apply_patch` uses that helper under macOS Seatbelt. The test now also canonicalizes the temporary patch target before building the patch payload so the path matches normalized grants on macOS, where `/var` paths often normalize to `/private/var`. ### Local/enterprise config isolation The core test harness now builds its default test config with managed config disabled, so host-managed enterprise config cannot alter these tests. The request-permissions turns in this test also explicitly use the user reviewer path, keeping the assertions focused on `request_permissions` behavior rather than reviewer defaults from the host. ## What Changed - Enable `apply_patch` in `approved_folder_write_request_permissions_unblocks_later_apply_patch`. - Teach the core integration test binary to dispatch `CODEX_FS_HELPER_ARG1`, matching the existing apply-patch and linux-sandbox dispatch paths. - Canonicalize the tempdir-backed patch target before creating the patch. - Ignore managed config in default core test configs and explicitly pin this test to `ApprovalsReviewer::User`. ## Verification Run outside the Codex app sandbox because these macOS tests intentionally spawn Seatbelt: - `cargo test -p codex-core approved_folder_write_request_permissions_unblocks_later_apply_patch` - `cargo test -p codex-core approved_folder_write_request_permissions_unblocks_later_exec_without_sandbox_args` --- codex-rs/core/tests/common/lib.rs | 2 ++ codex-rs/core/tests/suite/mod.rs | 4 ++++ .../core/tests/suite/request_permissions_tool.rs | 13 ++++++++----- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/tests/common/lib.rs b/codex-rs/core/tests/common/lib.rs index c89e6a518..d2ef9ddc6 100644 --- a/codex-rs/core/tests/common/lib.rs +++ b/codex-rs/core/tests/common/lib.rs @@ -10,6 +10,7 @@ use tempfile::TempDir; use codex_config::CloudRequirementsLoader; use codex_config::ConfigRequirementsToml; +use codex_config::LoaderOverrides; use codex_config::NetworkRequirementsToml; use codex_core::CodexThread; use codex_core::config::Config; @@ -181,6 +182,7 @@ pub async fn load_default_config_for_test_with_cloud_requirements( cloud_requirements: CloudRequirementsLoader, ) -> Config { ConfigBuilder::default() + .loader_overrides(LoaderOverrides::without_managed_config_for_tests()) .codex_home(codex_home.path().to_path_buf()) .harness_overrides(default_test_overrides()) .cloud_requirements(cloud_requirements) diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index 2f9478f3d..ad3280ebf 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -1,5 +1,6 @@ // Aggregates all former standalone integration tests as modules. use codex_apply_patch::CODEX_CORE_APPLY_PATCH_ARG1; +use codex_exec_server::CODEX_FS_HELPER_ARG1; use codex_sandboxing::landlock::CODEX_LINUX_SANDBOX_ARG0; use codex_test_binary_support::TestBinaryDispatchGuard; use codex_test_binary_support::TestBinaryDispatchMode; @@ -16,6 +17,9 @@ pub static CODEX_ALIASES_TEMP_DIR: Option = { if argv1 == Some(CODEX_CORE_APPLY_PATCH_ARG1) { return TestBinaryDispatchMode::DispatchArg0Only; } + if argv1 == Some(CODEX_FS_HELPER_ARG1) { + return TestBinaryDispatchMode::DispatchArg0Only; + } if exe_name == CODEX_LINUX_SANDBOX_ARG0 { return TestBinaryDispatchMode::DispatchArg0Only; } diff --git a/codex-rs/core/tests/suite/request_permissions_tool.rs b/codex-rs/core/tests/suite/request_permissions_tool.rs index 7a24e91bd..94465c18d 100644 --- a/codex-rs/core/tests/suite/request_permissions_tool.rs +++ b/codex-rs/core/tests/suite/request_permissions_tool.rs @@ -264,7 +264,7 @@ async fn approved_folder_write_request_permissions_unblocks_later_exec_without_s "write outside the workspace", approval_policy, permission_profile, - /*approvals_reviewer*/ None, + Some(ApprovalsReviewer::User), ) .await?; @@ -340,6 +340,7 @@ async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Resu let permission_profile_for_config = permission_profile.clone(); let mut builder = test_codex().with_config(move |config| { + config.include_apply_patch_tool = true; config.permissions.approval_policy = Constrained::allow_any(approval_policy); config .permissions @@ -367,7 +368,10 @@ async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Resu } else { "patched-via-request-permissions" }; - let requested_file = requested_dir.path().join(requested_file_name); + let requested_file = requested_dir + .path() + .canonicalize()? + .join(requested_file_name); let requested_permissions = requested_directory_write_permissions(requested_dir.path()); let normalized_requested_permissions = normalized_directory_write_permissions(requested_dir.path())?; @@ -422,7 +426,7 @@ async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Resu "patch outside the workspace", approval_policy, permission_profile, - strict_auto_review.then_some(ApprovalsReviewer::User), + Some(ApprovalsReviewer::User), ) .await?; @@ -463,8 +467,7 @@ async fn apply_patch_after_request_permissions(strict_auto_review: bool) -> Resu EventMsg::TurnComplete(_) => {} EventMsg::ApplyPatchApprovalRequest(approval) => { panic!( - "unexpected apply_patch approval request after granted permissions: {:?}", - approval.call_id + "unexpected apply_patch approval request after granted permissions: {approval:?}", ) } other => panic!("unexpected event: {other:?}"),