Skip to content

Add Codex launch arguments setting#2892

Open
jamesx0416 wants to merge 1 commit into
pingdotgg:mainfrom
jamesx0416:t3code/codex-launch-args
Open

Add Codex launch arguments setting#2892
jamesx0416 wants to merge 1 commit into
pingdotgg:mainfrom
jamesx0416:t3code/codex-launch-args

Conversation

@jamesx0416
Copy link
Copy Markdown
Contributor

@jamesx0416 jamesx0416 commented Jun 1, 2026

What Changed

  • Added a Codex provider launchArgs setting to the shared settings schema and provider settings form.
  • Pass configured launch args to codex app-server for provider probes and live session startup.
  • Added focused coverage for schema trimming, settings form visibility, argv construction, and runtime/probe propagation.

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 arguments text field to Codex provider settings.

Screenshots will be added separately.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Validation:

  • bun fmt
  • bun lint
  • bun typecheck

Open in Devin Review

Note

Medium Risk
Changes how the Codex child process is spawned for every session and health probe; misconfigured flags could break connectivity, but there is no new auth or data-path logic.

Overview
Adds a Codex launchArgs setting (schema, settings patch, and provider form field) so users can pass extra flags to codex app-server, mirroring Claude’s launch-arguments UX.

Configured values are trimmed, split on whitespace into argv tokens via codexAppServerArgs (always prefixed with app-server), and forwarded on provider status probes, live session spawn (CodexSessionRuntime), and adapter → runtime wiring. Tests cover argv building, probe/adapter propagation, and settings trimming/visibility.

Reviewed by Cursor Bugbot for commit 0d0a277. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Add launchArgs setting to Codex provider for passing custom CLI arguments

  • Adds a launchArgs string field to CodexSettings and CodexSettingsPatch in settings.ts, trimmed on decode with an empty string default.
  • Propagates launchArgs through the provider stack: checkCodexProviderStatusprobeCodexAppServerProvidermakeCodexSessionRuntime, which appends the whitespace-split args to the app-server CLI invocation.
  • Introduces a codexAppServerArgs helper in CodexProvider.ts and CodexSessionRuntime.ts to build the final arg list consistently.
  • Updates test fixtures and snapshots across the server and web apps to include the new empty launchArgs field.

Macroscope summarized 0d0a277.

Summary by CodeRabbit

  • New Features
    • Added configurable launch arguments for the Codex provider. Users can now pass custom arguments to the Codex app-server process through a new "Launch arguments" setting in the provider configuration.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds support for configurable launch arguments to the Codex provider. The launchArgs field is added to CodexSettings, parsed via a new helper function, threaded through provider probing and session runtime initialization, and passed to the spawned app-server child process.

Changes

Codex Launch Arguments

Layer / File(s) Summary
CodexSettings schema with launchArgs field
packages/contracts/src/settings.ts, packages/contracts/src/settings.test.ts
CodexSettings struct gains optional launchArgs string field (default empty string) with UI annotations. ServerSettingsPatch extends to allow launchArgs updates. Schema validation tests verify whitespace trimming behavior.
Launch arguments parsing helper
apps/server/src/provider/Layers/CodexProvider.ts, apps/server/src/provider/Layers/codexLaunchArgs.test.ts
New exported codexAppServerArgs(launchArgs) function converts whitespace-delimited argument string into argv array prefixed with "app-server". Unit tests verify empty input returns only ["app-server"] and non-empty input splits and appends correctly.
Provider probing with launch arguments
apps/server/src/provider/Layers/CodexProvider.ts, apps/server/src/provider/Layers/ProviderRegistry.test.ts
probeCodexAppServerProvider input type gains optional launchArgs field. Spawn call for CodexClient uses computed arguments from helper instead of fixed array. checkCodexProviderStatus passes configured launchArgs to probe invocation. Integration test verifies probe receives expected arguments.
Session runtime with launch arguments
apps/server/src/provider/Layers/CodexSessionRuntime.ts, apps/server/src/provider/Layers/CodexAdapter.ts, apps/server/src/provider/Layers/CodexAdapter.test.ts
CodexSessionRuntimeOptions extends with optional launchArgs field. Runtime imports helper and updates child-process spawn to compute and pass arguments. Adapter passes launchArgs from config to runtime options. Integration test verifies configured arguments flow through to created runtime instance.
Test fixture and assertion updates
apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts, apps/server/src/serverSettings.test.ts, apps/web/src/components/KeybindingsToast.browser.tsx, apps/web/src/components/settings/ProviderSettingsForm.test.ts
Test helpers and fixtures updated to include launchArgs: "" in Codex provider configuration objects, and expected assertion values updated to include the new field.

🎯 2 (Simple) | ⏱️ ~12 minutes

🐰 Launch args flow like spring rain,
Through probes and runtimes, ne'er in vain,
Settings parse, adapters wire,
App-server rises higher, higher! 🚀

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding a Codex launch arguments setting. It accurately reflects the core functionality introduced across the PR.
Description check ✅ Passed The description covers all required sections: What Changed (feature details), Why (user need), and UI Changes. It explains the problem, approach, and includes test coverage notes. The checklist is mostly complete with key items checked.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. labels Jun 1, 2026
@macroscopeapp
Copy link
Copy Markdown
Contributor

macroscopeapp Bot commented Jun 1, 2026

Approvability

Verdict: Needs human review

This PR introduces a new user-facing configuration capability that modifies how the Codex subprocess is spawned by allowing arbitrary CLI arguments. Changes that affect subprocess execution warrant human review to verify security and behavioral implications.

You can customize Macroscope's approvability policy. Learn more.

@jamesx0416
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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 win

Add "launchArgs" to the order array for consistency.

The order array should include "launchArgs" to ensure the field appears in the intended position in the settings form. ClaudeSettings (line 255) includes launchArgs in its order array, and the test expectation in ProviderSettingsForm.test.ts (line 21) assumes launchArgs appears after shadowHomePath.

♻️ 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

📥 Commits

Reviewing files that changed from the base of the PR and between b3e8c03 and 0d0a277.

📒 Files selected for processing (12)
  • apps/server/src/provider/Layers/CodexAdapter.test.ts
  • apps/server/src/provider/Layers/CodexAdapter.ts
  • apps/server/src/provider/Layers/CodexProvider.ts
  • apps/server/src/provider/Layers/CodexSessionRuntime.ts
  • apps/server/src/provider/Layers/ProviderInstanceRegistryLive.test.ts
  • apps/server/src/provider/Layers/ProviderRegistry.test.ts
  • apps/server/src/provider/Layers/codexLaunchArgs.test.ts
  • apps/server/src/serverSettings.test.ts
  • apps/web/src/components/KeybindingsToast.browser.tsx
  • apps/web/src/components/settings/ProviderSettingsForm.test.ts
  • packages/contracts/src/settings.test.ts
  • packages/contracts/src/settings.ts

Comment on lines +721 to +724
const appServerArgs = codexAppServerArgs(options.launchArgs);
const child = yield* spawner
.spawn(
ChildProcess.make(options.binaryPath, ["app-server"], {
ChildProcess.make(options.binaryPath, appServerArgs, {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Error message doesn't reflect configured launch arguments.

The CodexAppServerSpawnError message on line 736 shows a fixed "${options.binaryPath} app-server" command, but the actual spawn uses appServerArgs which may contain additional flags from options.launchArgs. When debugging spawn failures with custom launch args, the error message won't match the actual command.

📝 Suggested fix to include computed args in error message
-            new CodexErrors.CodexAppServerSpawnError({
-              command: `${options.binaryPath} app-server`,
-              cause,
-            }),
+            new CodexErrors.CodexAppServerSpawnError({
+              command: `${options.binaryPath} ${appServerArgs.join(" ")}`,
+              cause,
+            }),
🤖 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 `@apps/server/src/provider/Layers/CodexSessionRuntime.ts` around lines 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant