Add Codex launch arguments setting#2892
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a trimmed ChangesCodex Launch Arguments
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size:S
ApprovabilityVerdict: Needs human review This PR introduces a new user-configurable feature that affects how processes are spawned at runtime. New capabilities that change process execution behavior warrant human review regardless of implementation simplicity. You can customize Macroscope's approvability policy. Learn more. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/contracts/src/settings.ts (1)
209-213: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd
"launchArgs"to theorderarray for consistency.The
orderarray should include"launchArgs"to ensure the field appears in the intended position in the settings form.ClaudeSettings(line 255) includeslaunchArgsin its order array, and the test expectation inProviderSettingsForm.test.ts(line 21) assumeslaunchArgsappears aftershadowHomePath.♻️ Proposed fix
}, { - order: ["binaryPath", "homePath", "shadowHomePath"], + order: ["binaryPath", "homePath", "shadowHomePath", "launchArgs"], }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/contracts/src/settings.ts` around lines 209 - 213, The settings schema object currently sets order: ["binaryPath", "homePath", "shadowHomePath"] but omits "launchArgs", causing form ordering and tests to fail; update that object's order array to include "launchArgs" after "shadowHomePath" so it matches ClaudeSettings and the expectation in ProviderSettingsForm.test.ts, ensuring the "order" array change is applied to the same settings object in packages/contracts/src/settings.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/src/provider/Layers/CodexSessionRuntime.ts`:
- Around line 721-724: The CodexAppServerSpawnError message currently logs a
hardcoded "${options.binaryPath} app-server" which can mismatch the actual
command; update the error construction where CodexAppServerSpawnError is thrown
(after the spawner.spawn call) to include the computed appServerArgs (from
codexAppServerArgs(options.launchArgs)) along with options.binaryPath so the
logged command reflects ChildProcess.make(options.binaryPath, appServerArgs,
...) and any flags from options.launchArgs; ensure you reference appServerArgs
and options.binaryPath in the error message so spawn failures show the real
command invoked.
---
Outside diff comments:
In `@packages/contracts/src/settings.ts`:
- Around line 209-213: The settings schema object currently sets order:
["binaryPath", "homePath", "shadowHomePath"] but omits "launchArgs", causing
form ordering and tests to fail; update that object's order array to include
"launchArgs" after "shadowHomePath" so it matches ClaudeSettings and the
expectation in ProviderSettingsForm.test.ts, ensuring the "order" array change
is applied to the same settings object in packages/contracts/src/settings.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b83f8476-9f14-45be-9c09-5bab8a90c3d4
📒 Files selected for processing (12)
apps/server/src/provider/Layers/CodexAdapter.test.tsapps/server/src/provider/Layers/CodexAdapter.tsapps/server/src/provider/Layers/CodexProvider.tsapps/server/src/provider/Layers/CodexSessionRuntime.tsapps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.tsapps/server/src/provider/Layers/ProviderRegistry.test.tsapps/server/src/provider/Layers/codexLaunchArgs.test.tsapps/server/src/serverSettings.test.tsapps/web/src/components/KeybindingsToast.browser.tsxapps/web/src/components/settings/ProviderSettingsForm.test.tspackages/contracts/src/settings.test.tspackages/contracts/src/settings.ts
github-actions
Bot
added
size:M
github-actions
Bot
added
size:S
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
What Changed
launchArgssetting to the shared settings schema and provider settings form.codex app-serverfor provider probes and live session startup.Why
Codex instances need the same kind of configurable launch arguments Claude already exposes, so users can pass app-server CLI flags like
-c model_provider="codex-lb"without changing the binary path. The field is split into argv tokens directly and never shell-executed.UI Changes
Adds a small
Launch argumentstext field to Codex provider settings.Screenshots will be added separately.
Checklist
Validation:
bun fmtbun lintbun typecheckNote
Medium Risk
Changes how the Codex child process is spawned from user-supplied strings; mistakes in launch args could break sessions or probes, though args are tokenized without a shell.
Overview
Adds a Codex
launchArgsprovider setting (schema, settings patch, and Launch arguments field in provider settings), mirroring Claude’s pattern.Configured values are passed through
codex app-serverstartup: a sharedcodexAppServerArgshelper prependsapp-serverand splits the string on whitespace into argv (no shell).CodexSessionRuntime,CodexAdapter, and provider status probes all use it; spawn error messages reflect the full command.Tests cover argv building, schema trim/default, form field visibility, adapter/runtime/probe propagation, and fixture updates for the new default field.
Reviewed by Cursor Bugbot for commit eba666f. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add
launchArgssetting to configure Codex app-server CLI argumentslaunchArgsstring field toCodexSettingsandCodexSettingsPatchin settings.ts; the value is trimmed and defaults to empty.launchArgsthrough the provider status check, session runtime, and process spawn in CodexSessionRuntime.ts and CodexProvider.ts.codexAppServerArgshelper splits the string into argv, returning['app-server']for empty/whitespace input and['app-server', ...args]otherwise.Macroscope summarized eba666f.
Summary by CodeRabbit