Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3099353c93
ℹ️ 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".
| thread_manager | ||
| .plugins_manager() | ||
| .maybe_start_plugin_startup_tasks_for_config(&config, auth_manager.clone()); | ||
| if std::env::var_os(TEST_DISABLE_PLUGIN_STARTUP_TASKS_ENV_VAR).is_none() { |
There was a problem hiding this comment.
Avoid shipping a test env gate in startup path
Gating maybe_start_plugin_startup_tasks_for_config behind CODEX_APP_SERVER_TEST_DISABLE_PLUGIN_STARTUP_TASKS changes production behavior based on a test-only environment variable. If that variable is present in a real app-server environment (for example from reused CI/test wrappers), startup plugin tasks are silently skipped, which prevents startup sync/warmup behavior and can leave plugin state stale until later user actions. This toggle should be constrained to test-only code paths rather than the main runtime constructor.
Useful? React with 👍 / 👎.
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/featuredwhen 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
AppServerRuntimeOptions/PluginStartupTasksplumbing and a hidden debug-only--disable-plugin-startup-tasks-for-testsapp-server flag, so integration tests can suppress startup plugin warmups without adding a production env-var gate.info/debugtowarnto avoid multi-megabyte stderr output in Bazel logs.rmcp-clientto exercise PATH resolution directly and include the actual spawn error in failures.Verification
cargo check -p codex-app-servercargo clippy -p codex-app-server --tests -- -D warningscargo test -p codex-rmcp-client program_resolver::tests::test_unix_executes_script_without_extensioncargo test -p codex-app-server --test all external_agent_config_import_sends_completion_notification_after_pending_plugins_finish -- --nocapturecargo test -p codex-app-server --test all plugin_list_uses_warmed_featured_plugin_ids_cache_on_first_request -- --nocaptureStack created with Sapling. Best reviewed with ReviewStack.