refactor(codex): run host-installed codex CLI instead of bundled SDK - #136
refactor(codex): run host-installed codex CLI instead of bundled SDK#136Waishnav wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe change replaces the Codex SDK with a host-installed Codex CLI runtime. It adds command resolution, version validation, diagnostic errors, version-aware provider availability, UI and API metadata, and documentation for Codex configuration. ChangesCodex CLI delegation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DevSpaceAdapter
participant CodexCommandResolver
participant CodexCliLocalAgentRuntime
participant CodexCLI
DevSpaceAdapter->>CodexCommandResolver: resolve CODEX_COMMAND or PATH
CodexCommandResolver->>CodexCLI: probe --version
CodexCLI-->>CodexCommandResolver: executable and version
DevSpaceAdapter->>CodexCliLocalAgentRuntime: run with command, environment, and version
CodexCliLocalAgentRuntime->>CodexCLI: spawn CLI with prompt and arguments
CodexCLI-->>CodexCliLocalAgentRuntime: JSONL events and stderr
CodexCliLocalAgentRuntime-->>DevSpaceAdapter: response, session ID, or diagnostic error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
Greptile SummaryThis PR replaces the bundled Codex SDK integration with execution of the user's host-installed Codex CLI and exposes detected version information throughout provider availability surfaces.
Confidence Score: 3/5The PR should not merge until failed Codex probes are rejected and subprocess stdin errors are converted into recorded run failures. The new direct-CLI path can falsely advertise unusable commands as available, and an early Codex exit can emit an unhandled stdin error that terminates the worker instead of persisting the failure. Files Needing Attention: src/local-agent-codex.ts, src/local-agent-runtime.ts, src/local-agent-availability.ts
|
| Filename | Overview |
|---|---|
| src/local-agent-codex.ts | Adds Codex command discovery and version probing, but failed probes other than ENOENT are incorrectly accepted as available. |
| src/local-agent-runtime.ts | Replaces the SDK runtime with direct CLI subprocess handling; missing stdin error handling can terminate failed workers. |
| src/local-agent-adapters.ts | Connects the Codex adapter to command resolution and the new CLI runtime. |
| src/local-agent-availability.ts | Reports Codex CLI versions and minimum-version status, but inherits failed-command misclassification from the resolver. |
| src/server.ts | Extends workspace provider output and formatting with detected and minimum versions. |
| src/ui/workspace-app.tsx | Displays available provider version metadata in workspace card titles. |
| package.json | Removes the bundled Codex SDK dependency as intended. |
Sequence Diagram
sequenceDiagram
participant User
participant DevSpace
participant Resolver as Codex Resolver
participant CLI as Host Codex CLI
User->>DevSpace: Run Codex subagent
DevSpace->>Resolver: Resolve command and probe --version
Resolver-->>DevSpace: Executable and version
DevSpace->>CLI: codex exec --experimental-json
DevSpace->>CLI: Prompt over stdin
CLI-->>DevSpace: JSON-line events
DevSpace-->>User: Persist response and session ID
Reviews (1): Last reviewed commit: "refactor(codex): run host-installed code..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const spawnCode = | ||
| probe.error && "code" in probe.error ? probe.error.code : undefined; | ||
| if (spawnCode === "ENOENT") continue; | ||
| return { | ||
| executable: candidate, | ||
| version: parseCodexVersion(probe.stdout), | ||
| }; |
There was a problem hiding this comment.
When CODEX_COMMAND or a PATH candidate is non-executable, times out, or exits unsuccessfully, this resolver accepts it unless the error is ENOENT; availability then reports Codex as usable, but every delegated run fails or hangs.
| const spawnCode = | |
| probe.error && "code" in probe.error ? probe.error.code : undefined; | |
| if (spawnCode === "ENOENT") continue; | |
| return { | |
| executable: candidate, | |
| version: parseCodexVersion(probe.stdout), | |
| }; | |
| if (probe.error || probe.status !== 0) continue; | |
| return { | |
| executable: candidate, | |
| version: parseCodexVersion(probe.stdout), | |
| }; |
| child.stdin.write(prompt); | ||
| child.stdin.end(); |
There was a problem hiding this comment.
When Codex exits or closes its input pipe while DevSpace writes the prompt, child.stdin emits an unhandled EPIPE because only the child process has an error listener, terminating the detached worker before it records the failed session.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
README.md (1)
151-157: 🩺 Stability & Availability | 🔵 TrivialDocument the server restart requirement.
createServercaptures provider availability once at startup insrc/server.ts, Lines 1699-1701. If the user changesCODEX_COMMAND,PATH, or the installed Codex version whiledevspace serveis running,open_workspaceand the serve banner can show stale availability data. State that users must restart the server after provider changes. Mirror the note indocs/configuration.md, Line 130.As per coding guidelines, verify the actual user-consumption path, including restart requirements.
🤖 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 `@README.md` around lines 151 - 157, Update the Codex provider documentation in README.md and its corresponding section in docs/configuration.md to state that users must restart the running devspace server after changing CODEX_COMMAND, PATH, or the installed Codex version so availability data refreshes.Source: Coding guidelines
src/server.test.ts (1)
107-110: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the card payload consumed by the widget.
src/ui/workspace-app.tsx, functionrenderWorkspacePayload, reads provider metadata from_meta.card.agentProviders. This test only checksstructuredContent.agentProviders. A regression in card metadata could break the widget while this test still passes. Assert the card metadata or add a widget-level test forversionandminimumVersion.As per coding guidelines, verify the actual user-consumption path, including widgets and rendered artifacts.
🤖 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 `@src/server.test.ts` around lines 107 - 110, Update the test around structuredContent in the server response to validate the provider version and minimumVersion from _meta.card.agentProviders, matching the metadata path consumed by renderWorkspacePayload in the workspace UI. Keep the existing structuredContent assertions if useful, but ensure the card payload independently verifies the codex values.Source: Coding guidelines
🤖 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 `@src/local-agent-availability.test.ts`:
- Around line 11-14: Update the test around checkLocalAgentProviderAvailability
to use a controlled CODEX_COMMAND fixture that emits a supported Codex version
instead of probing the host installation. Preserve the assertions for
availability, version format, and minimumVersion; if host validation is intended
separately, move it to an integration check that skips when Codex is
unavailable.
In `@src/local-agent-codex.ts`:
- Around line 34-46: Reject unvalidated Codex commands across all affected
sites: in src/local-agent-codex.ts:34-46, have resolveCodexCommand return a
result only when the --version probe succeeds and parseCodexVersion yields a
version, while preserving probe-failure diagnostics separately; in
src/local-agent-availability.ts:82-103, report missing or invalid detected
versions as unavailable; and in src/local-agent-adapters.ts:55-66, enforce
MINIMUM_CODEX_VERSION before constructing CodexCliLocalAgentRuntime, including
direct adapter usage.
In `@src/local-agent-runtime.ts`:
- Around line 27-250: Move Codex-specific symbols and execution logic out of the
core local-agent runtime into a dedicated adapter module such as
local-agent-codex.ts, including CodexCliInvocation, CodexCliTurn,
CodexCliRunner, CodexCliRuntimeOptions, codexCliArguments, parseCodexCliLines,
CodexCliLocalAgentRuntime, createCodexCliLocalAgentRuntime,
createCodexCliSpawnRunner, codexCliError, sandboxModeFor, and failureMessage.
Keep only generic local-agent contracts and policy in local-agent-runtime.ts,
updating imports and exports so callers retain the existing adapter API.
In `@src/server.test.ts`:
- Around line 98-119: Move the client/server connection setup, including the
Promise.all call, inside the existing try block so cleanup always runs when
connection establishment fails. Update the finally cleanup around client.close,
server.close, store.close, and rm to continue executing subsequent cleanup steps
if an earlier close operation rejects.
In `@src/ui/workspace-app.tsx`:
- Around line 551-564: Update the provider metadata flow in the returned
WorkspaceChip data and renderWorkspaceChips so version and minimumVersion are
included in the accessible name or an equivalent keyboard-accessible details
control, rather than only WorkspaceChip.title. Verify the rendered widget
exposes this information to keyboard and screen-reader users, including logo
providers whose current ariaLabel contains only the provider name.
---
Nitpick comments:
In `@README.md`:
- Around line 151-157: Update the Codex provider documentation in README.md and
its corresponding section in docs/configuration.md to state that users must
restart the running devspace server after changing CODEX_COMMAND, PATH, or the
installed Codex version so availability data refreshes.
In `@src/server.test.ts`:
- Around line 107-110: Update the test around structuredContent in the server
response to validate the provider version and minimumVersion from
_meta.card.agentProviders, matching the metadata path consumed by
renderWorkspacePayload in the workspace UI. Keep the existing structuredContent
assertions if useful, but ensure the card payload independently verifies the
codex values.
🪄 Autofix
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 Plus
Run ID: b6164ba5-6e60-41b7-8d10-32d4b35fb5e0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
README.mddocs/configuration.mdpackage.jsonskills/subagent-delegation/SKILL.mdsrc/cli.tssrc/local-agent-adapters.test.tssrc/local-agent-adapters.tssrc/local-agent-availability.test.tssrc/local-agent-availability.tssrc/local-agent-codex.tssrc/local-agent-runtime.test.tssrc/local-agent-runtime.tssrc/server.test.tssrc/server.tssrc/ui/card-types.tssrc/ui/workspace-app.tsx
💤 Files with no reviewable changes (1)
- package.json
| const availableCodex = checkLocalAgentProviderAvailability("codex"); | ||
| assert.equal(availableCodex.available, true); | ||
| assert.match(availableCodex.version ?? "", /^\d+\.\d+/); | ||
| assert.equal(availableCodex.minimumVersion, "0.142.5"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Do not require a host Codex installation in this unit test.
This assertion probes the real host environment. It fails on clean developer or CI hosts without a compatible Codex CLI. Use a controlled CODEX_COMMAND fixture that emits a supported version. If this is an integration check, separate it and skip it when the host requirement is absent.
🧰 Tools
🪛 GitHub Actions: CI / 0_Smoke (macos-latest).txt
[error] 12-12: npm test failed: AssertionError expected true but received false (false !== true).
🪛 GitHub Actions: CI / 1_Smoke (ubuntu-latest).txt
[error] 12-12: npm test failed: AssertionError [ERR_ASSERTION] expected true but received false.
🪛 GitHub Actions: CI / 2_Smoke (windows-latest).txt
[error] 12-12: Test assertion failed: expected true but received false (AssertionError [ERR_ASSERTION]). The npm test command failed with exit code 1.
🪛 GitHub Actions: CI / Smoke (macos-latest)
[error] 12-12: npm test failed: assertion expected true but received false (AssertionError [ERR_ASSERTION]). Command failed with exit code 1.
🪛 GitHub Actions: CI / Smoke (ubuntu-latest)
[error] 12-12: Test assertion failed: expected true but received false (AssertionError [ERR_ASSERTION]). The 'npm test' command failed with exit code 1.
🪛 GitHub Actions: CI / Smoke (windows-latest)
[error] 12-12: npm test failed: AssertionError because the actual value was false but the expected value was true. Command failed with exit code 1.
🤖 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 `@src/local-agent-availability.test.ts` around lines 11 - 14, Update the test
around checkLocalAgentProviderAvailability to use a controlled CODEX_COMMAND
fixture that emits a supported Codex version instead of probing the host
installation. Preserve the assertions for availability, version format, and
minimumVersion; if host validation is intended separately, move it to an
integration check that skips when Codex is unavailable.
| const probe = spawnSync(candidate, ["--version"], { | ||
| encoding: "utf8", | ||
| env: probeEnv, | ||
| windowsHide: true, | ||
| timeout: 5_000, | ||
| }); | ||
| const spawnCode = | ||
| probe.error && "code" in probe.error ? probe.error.code : undefined; | ||
| if (spawnCode === "ENOENT") continue; | ||
| return { | ||
| executable: candidate, | ||
| version: parseCodexVersion(probe.stdout), | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unvalidated Codex commands before availability or execution.
resolveCodexCommand returns a command after any probe failure except ENOENT. A timeout, permission failure, nonzero --version exit, or unparseable output can therefore produce version: undefined. The availability path then reports the provider as available. The execution path can also run a version below MINIMUM_CODEX_VERSION.
src/local-agent-codex.ts#L34-L46: return a resolved command only after a successful version probe with a parsed version. Preserve probe-failure diagnostics separately.src/local-agent-availability.ts#L82-L103: report a missing or invalid detected version as unavailable.src/local-agent-adapters.ts#L55-L66: enforce the version floor before constructingCodexCliLocalAgentRuntime, including direct adapter use.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 3 files
src/local-agent-codex.ts#L34-L46(this comment)src/local-agent-availability.ts#L82-L103src/local-agent-adapters.ts#L55-L66
🤖 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 `@src/local-agent-codex.ts` around lines 34 - 46, Reject unvalidated Codex
commands across all affected sites: in src/local-agent-codex.ts:34-46, have
resolveCodexCommand return a result only when the --version probe succeeds and
parseCodexVersion yields a version, while preserving probe-failure diagnostics
separately; in src/local-agent-availability.ts:82-103, report missing or invalid
detected versions as unavailable; and in src/local-agent-adapters.ts:55-66,
enforce MINIMUM_CODEX_VERSION before constructing CodexCliLocalAgentRuntime,
including direct adapter usage.
| export interface CodexCliInvocation { | ||
| readonly command: string; | ||
| readonly args: string[]; | ||
| readonly env: NodeJS.ProcessEnv; | ||
| readonly prompt: string; | ||
| } | ||
|
|
||
| interface CodexClientLike { | ||
| startThread(options?: ThreadOptions): CodexThreadLike; | ||
| resumeThread(id: string, options?: ThreadOptions): CodexThreadLike; | ||
| export interface CodexCliTurn { | ||
| readonly threadId: string | null; | ||
| readonly finalResponse: string; | ||
| readonly items: unknown[]; | ||
| } | ||
|
|
||
| type CodexFactory = (options?: CodexOptions) => CodexClientLike; | ||
| export type CodexCliRunner = ( | ||
| invocation: CodexCliInvocation, | ||
| ) => Promise<CodexCliTurn>; | ||
|
|
||
| function sandboxModeFor(writeMode: LocalAgentWriteMode | undefined): SandboxMode { | ||
| switch (writeMode) { | ||
| case "allowed": | ||
| return "workspace-write"; | ||
| case "full_access": | ||
| return "danger-full-access"; | ||
| case "read_only": | ||
| case undefined: | ||
| return "read-only"; | ||
| export interface CodexCliRuntimeOptions { | ||
| readonly command: string; | ||
| readonly env: NodeJS.ProcessEnv; | ||
| readonly version?: string; | ||
| readonly runner?: CodexCliRunner; | ||
| } | ||
|
|
||
| interface ParsedCodexCliLines { | ||
| threadId: string | null; | ||
| finalResponse: string; | ||
| items: unknown[]; | ||
| failure: unknown; | ||
| } | ||
|
|
||
| const CODEX_APPROVAL_POLICY = "never"; | ||
|
|
||
| export function codexCliArguments(input: LocalAgentRunInput): string[] { | ||
| const args = ["exec", "--experimental-json"]; | ||
| if (input.model) { | ||
| args.push("--model", input.model); | ||
| } | ||
| if (input.thinking) { | ||
| args.push("--config", `model_reasoning_effort="${input.thinking}"`); | ||
| } | ||
| args.push("--config", `approval_policy="${CODEX_APPROVAL_POLICY}"`); | ||
| args.push("--sandbox", sandboxModeFor(input.writeMode)); | ||
| args.push("--cd", input.workspace); | ||
| if (input.providerSessionId) { | ||
| args.push("resume", input.providerSessionId); | ||
| } | ||
| return args; | ||
| } | ||
|
|
||
| function threadOptionsFor(input: LocalAgentRunInput): ThreadOptions { | ||
| return { | ||
| workingDirectory: input.workspace, | ||
| sandboxMode: sandboxModeFor(input.writeMode), | ||
| approvalPolicy: "never", | ||
| model: input.model, | ||
| modelReasoningEffort: input.thinking as ModelReasoningEffort | undefined, | ||
| }; | ||
| // Mirror the SDK's event handling, minus the parts DevSpace does not use: | ||
| // `thread.started` supplies the thread id, `item.completed` yields the final | ||
| // agent message and the item log, and a `turn.failed` event aborts the run. | ||
| export function parseCodexCliLines(lines: string[]): ParsedCodexCliLines { | ||
| let threadId: string | null = null; | ||
| let finalResponse = ""; | ||
| const items: unknown[] = []; | ||
| let failure: unknown; | ||
| for (const rawLine of lines) { | ||
| const line = rawLine.trim(); | ||
| if (!line) continue; | ||
| let event: Record<string, unknown>; | ||
| try { | ||
| event = JSON.parse(line) as Record<string, unknown>; | ||
| } catch { | ||
| throw new Error(`Failed to parse codex CLI output line: ${line.slice(0, 200)}`); | ||
| } | ||
| switch (event.type) { | ||
| case "thread.started": { | ||
| if (typeof event.thread_id === "string") threadId = event.thread_id; | ||
| break; | ||
| } | ||
| case "item.completed": { | ||
| const record = event.item as Record<string, unknown> | undefined; | ||
| if (record) items.push(record); | ||
| if (record?.type === "agent_message" && typeof record.text === "string") { | ||
| finalResponse = record.text; | ||
| } | ||
| break; | ||
| } | ||
| case "turn.failed": { | ||
| failure = failureMessage(event.error); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return { threadId, finalResponse, items, failure }; | ||
| } | ||
|
|
||
| export class CodexSdkLocalAgentRuntime implements LocalAgentRuntime { | ||
| export class CodexCliLocalAgentRuntime implements LocalAgentRuntime { | ||
| readonly provider = "codex" as const; | ||
| private readonly codex: CodexClientLike; | ||
| private readonly runner: CodexCliRunner; | ||
|
|
||
| constructor(codex: CodexClientLike) { | ||
| this.codex = codex; | ||
| constructor(private readonly options: CodexCliRuntimeOptions) { | ||
| this.runner = options.runner ?? createCodexCliSpawnRunner({ version: options.version }); | ||
| } | ||
|
|
||
| async run(input: LocalAgentRunInput): Promise<LocalAgentRunResult> { | ||
| const options = threadOptionsFor(input); | ||
| const thread = input.providerSessionId | ||
| ? this.codex.resumeThread(input.providerSessionId, options) | ||
| : this.codex.startThread(options); | ||
| const turn = await thread.run(input.prompt); | ||
|
|
||
| const turn = await this.runner({ | ||
| command: this.options.command, | ||
| args: codexCliArguments(input), | ||
| env: this.options.env, | ||
| prompt: input.prompt, | ||
| }); | ||
| return { | ||
| provider: this.provider, | ||
| providerSessionId: thread.id, | ||
| providerSessionId: turn.threadId, | ||
| finalResponse: turn.finalResponse, | ||
| items: turn.items, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| export async function createCodexSdkLocalAgentRuntime( | ||
| options?: CodexOptions, | ||
| codexFactory?: CodexFactory, | ||
| ): Promise<CodexSdkLocalAgentRuntime> { | ||
| const factory = codexFactory ?? (await defaultCodexFactory()); | ||
| return new CodexSdkLocalAgentRuntime(factory(options)); | ||
| export function createCodexCliLocalAgentRuntime( | ||
| options: CodexCliRuntimeOptions, | ||
| ): CodexCliLocalAgentRuntime { | ||
| return new CodexCliLocalAgentRuntime(options); | ||
| } | ||
|
|
||
| async function defaultCodexFactory(): Promise<CodexFactory> { | ||
| const module = await import("@openai/codex-sdk"); | ||
| return (options) => new module.Codex(options) as Codex; | ||
| export function createCodexCliSpawnRunner(options: { version?: string } = {}): CodexCliRunner { | ||
| const version = options.version; | ||
| return async (invocation) => { | ||
| const { command, args, env, prompt } = invocation; | ||
| const child = spawn(command, args, { | ||
| env, | ||
| windowsHide: true, | ||
| }); | ||
| let spawnError: Error | undefined; | ||
| let stderr = ""; | ||
| const exitPromise = new Promise<{ code: number | null; signal: NodeJS.Signals | null }>( | ||
| (resolve) => { | ||
| child.once("exit", (code, signal) => resolve({ code, signal })); | ||
| }, | ||
| ); | ||
| child.once("error", (error) => { | ||
| spawnError = error; | ||
| }); | ||
| if (!child.stdin) { | ||
| child.kill(); | ||
| throw codexCliError("codex CLI did not expose stdin", version); | ||
| } | ||
| child.stdin.write(prompt); | ||
| child.stdin.end(); | ||
| if (child.stderr) { | ||
| child.stderr.on("data", (chunk: Buffer) => { | ||
| stderr += chunk.toString("utf8"); | ||
| }); | ||
| } | ||
| const output = child.stdout; | ||
| if (!output) { | ||
| child.kill(); | ||
| throw codexCliError("codex CLI did not expose stdout", version); | ||
| } | ||
| const lines: string[] = []; | ||
| const reader = createInterface({ | ||
| input: output, | ||
| crlfDelay: Infinity, | ||
| }); | ||
| try { | ||
| for await (const line of reader) { | ||
| lines.push(line); | ||
| } | ||
| } finally { | ||
| reader.close(); | ||
| } | ||
| if (spawnError) { | ||
| throw codexCliError( | ||
| `Failed to start codex CLI: ${spawnError.message}`, | ||
| version, | ||
| stderr, | ||
| ); | ||
| } | ||
|
|
||
| const parsed = parseCodexCliLines(lines); | ||
| if (parsed.failure) { | ||
| throw codexCliError(`codex turn failed: ${String(parsed.failure)}`, version, stderr); | ||
| } | ||
| const { code, signal } = await exitPromise; | ||
| if (code !== 0 || signal) { | ||
| throw codexCliError( | ||
| `codex CLI exited with ${signal ? `signal ${signal}` : `code ${code ?? 1}`}`, | ||
| version, | ||
| stderr, | ||
| ); | ||
| } | ||
| return { | ||
| threadId: parsed.threadId, | ||
| finalResponse: parsed.finalResponse, | ||
| items: parsed.items, | ||
| }; | ||
| }; | ||
| } | ||
|
|
||
| // Surface the CLI version and raw stderr in the exception so the session error | ||
| // row lets the host reason about model gates without forensics. | ||
| export function codexCliError(message: string, version?: string, stderr?: string): Error { | ||
| const details = [ | ||
| message, | ||
| version ? `codex version: ${version}` : undefined, | ||
| stderr && stderr.trim() ? `stderr:\n${stderr.trim()}` : undefined, | ||
| ].filter(Boolean).join("\n"); | ||
| return new Error(details); | ||
| } | ||
|
|
||
| function sandboxModeFor(writeMode: LocalAgentWriteMode | undefined): string { | ||
| switch (writeMode) { | ||
| case "allowed": | ||
| return "workspace-write"; | ||
| case "full_access": | ||
| return "danger-full-access"; | ||
| case "read_only": | ||
| case undefined: | ||
| return "read-only"; | ||
| } | ||
| } | ||
|
|
||
| function failureMessage(error: unknown): unknown { | ||
| if (error && typeof error === "object") { | ||
| const message = (error as Record<string, unknown>).message; | ||
| if (typeof message === "string" && message.trim()) return message; | ||
| } | ||
| if (typeof error === "string" && error.trim()) return error; | ||
| return error; | ||
| } No newline at end of file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move Codex CLI translation out of the core runtime.
CodexCliInvocation, CLI arguments, JSONL parsing, sandbox mapping, and subprocess handling are Codex adapter details. Keep generic local-agent contracts in the core module. Move Codex-specific execution to src/local-agent-codex.ts or a dedicated Codex adapter module.
As per coding guidelines, “Keep DevSpace policy in the core domain and provider-specific translation in adapters; do not let Pi, MCP host, or model-provider terminology become the core domain model.”
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 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 `@src/local-agent-runtime.ts` around lines 27 - 250, Move Codex-specific
symbols and execution logic out of the core local-agent runtime into a dedicated
adapter module such as local-agent-codex.ts, including CodexCliInvocation,
CodexCliTurn, CodexCliRunner, CodexCliRuntimeOptions, codexCliArguments,
parseCodexCliLines, CodexCliLocalAgentRuntime, createCodexCliLocalAgentRuntime,
createCodexCliSpawnRunner, codexCliError, sandboxModeFor, and failureMessage.
Keep only generic local-agent contracts and policy in local-agent-runtime.ts,
updating imports and exports so callers retain the existing adapter API.
Source: Coding guidelines
| const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); | ||
| const client = new Client({ name: "devspace-test-client", version: "1.0.0" }); | ||
| await Promise.all([ | ||
| client.connect(clientTransport), | ||
| server.connect(serverTransport), | ||
| ]); | ||
| try { | ||
| const opened = await callOpen(client, project, "chat-1"); | ||
|
|
||
| const structured = structuredContent(opened); | ||
| const providers = structured.agentProviders as Array<Record<string, unknown>>; | ||
| assert.equal(providers.find((provider) => provider.name === "codex")?.version, "0.147.0"); | ||
| assert.equal(providers.find((provider) => provider.name === "codex")?.minimumVersion, "0.142.5"); | ||
|
|
||
| const text = responseText(opened); | ||
| assert.match(text, /Available subagent providers: codex \(0\.147\.0, min 0\.142\.5\)/); | ||
| assert.match(text, /Unavailable subagent providers: pi \(pi executable not found\)/); | ||
| } finally { | ||
| await client.close(); | ||
| await server.close(); | ||
| store.close(); | ||
| await rm(root, { recursive: true, force: true }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep connection setup inside the cleanup scope.
Promise.all(...) runs before try. If either connect call rejects, the test skips client.close(), server.close(), store.close(), and rm(...). Move connection setup into try. Make cleanup continue after a failed close operation.
Proposed cleanup structure
- await Promise.all([
- client.connect(clientTransport),
- server.connect(serverTransport),
- ]);
try {
+ await Promise.all([
+ client.connect(clientTransport),
+ server.connect(serverTransport),
+ ]);
const opened = await callOpen(client, project, "chat-1");
...
} finally {
- await client.close();
- await server.close();
- store.close();
- await rm(root, { recursive: true, force: true });
+ try {
+ await client.close();
+ } finally {
+ try {
+ await server.close();
+ } finally {
+ try {
+ store.close();
+ } finally {
+ await rm(root, { recursive: true, force: true });
+ }
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); | |
| const client = new Client({ name: "devspace-test-client", version: "1.0.0" }); | |
| await Promise.all([ | |
| client.connect(clientTransport), | |
| server.connect(serverTransport), | |
| ]); | |
| try { | |
| const opened = await callOpen(client, project, "chat-1"); | |
| const structured = structuredContent(opened); | |
| const providers = structured.agentProviders as Array<Record<string, unknown>>; | |
| assert.equal(providers.find((provider) => provider.name === "codex")?.version, "0.147.0"); | |
| assert.equal(providers.find((provider) => provider.name === "codex")?.minimumVersion, "0.142.5"); | |
| const text = responseText(opened); | |
| assert.match(text, /Available subagent providers: codex \(0\.147\.0, min 0\.142\.5\)/); | |
| assert.match(text, /Unavailable subagent providers: pi \(pi executable not found\)/); | |
| } finally { | |
| await client.close(); | |
| await server.close(); | |
| store.close(); | |
| await rm(root, { recursive: true, force: true }); | |
| const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); | |
| const client = new Client({ name: "devspace-test-client", version: "1.0.0" }); | |
| try { | |
| await Promise.all([ | |
| client.connect(clientTransport), | |
| server.connect(serverTransport), | |
| ]); | |
| const opened = await callOpen(client, project, "chat-1"); | |
| const structured = structuredContent(opened); | |
| const providers = structured.agentProviders as Array<Record<string, unknown>>; | |
| assert.equal(providers.find((provider) => provider.name === "codex")?.version, "0.147.0"); | |
| assert.equal(providers.find((provider) => provider.name === "codex")?.minimumVersion, "0.142.5"); | |
| const text = responseText(opened); | |
| assert.match(text, /Available subagent providers: codex \(0\.147\.0, min 0\.142\.5\)/); | |
| assert.match(text, /Unavailable subagent providers: pi \(pi executable not found\)/); | |
| } finally { | |
| try { | |
| await client.close(); | |
| } finally { | |
| try { | |
| await server.close(); | |
| } finally { | |
| try { | |
| store.close(); | |
| } finally { | |
| await rm(root, { recursive: true, force: true }); | |
| } | |
| } | |
| } | |
| } |
🤖 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 `@src/server.test.ts` around lines 98 - 119, Move the client/server connection
setup, including the Promise.all call, inside the existing try block so cleanup
always runs when connection establishment fails. Update the finally cleanup
around client.close, server.close, store.close, and rm to continue executing
subsequent cleanup steps if an earlier close operation rejects.
| const version = provider.version | ||
| ? `${provider.version}${provider.minimumVersion ? ` (min ${provider.minimumVersion})` : ""}` | ||
| : undefined; | ||
| return { | ||
| label: name, | ||
| logo, | ||
| bareLogo: Boolean(logo), | ||
| ariaLabel: name, | ||
| tone: unavailable ? "muted" as const : undefined, | ||
| title: unavailable ? provider.reason ?? "Provider unavailable" : name, | ||
| title: unavailable | ||
| ? provider.reason ?? "Provider unavailable" | ||
| : version | ||
| ? `${name} ${version}` | ||
| : name, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Expose provider diagnostics without pointer hover.
version and minimumVersion are only assigned to WorkspaceChip.title. renderWorkspaceChips creates non-focusable span elements. For logo providers, aria-label remains only the provider name. Therefore the detected version and minimum version are not reliably available to keyboard and screen-reader users. Add the metadata to the accessible name and/or render a visible, focusable details control.
As per coding guidelines, verify the actual user-consumption path, including widgets and rendered artifacts.
🤖 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 `@src/ui/workspace-app.tsx` around lines 551 - 564, Update the provider
metadata flow in the returned WorkspaceChip data and renderWorkspaceChips so
version and minimumVersion are included in the accessible name or an equivalent
keyboard-accessible details control, rather than only WorkspaceChip.title.
Verify the rendered widget exposes this information to keyboard and
screen-reader users, including logo providers whose current ariaLabel contains
only the provider name.
Source: Coding guidelines
1 participant
DevSpace bundled the
@openai/codex-sdk, which drifts away from thecodexa user actually has installed: the bundled version, its auth, and its model behavior can all disagree with the user's own CLI. That makes provider failures hard to diagnose and follow-up sessions hard to reason about.Codex subagents now execute the user's host-installed
codexbinary (PATH orCODEX_COMMAND) instead of the bundled SDK. The detected CLI version and a minimum supported version are reported wherever agent availability is shown (open_workspace, the serve banner, anddevspace agentsoutput), and failed runs are stamped with the Codex version and raw stderr so a model gate keyed on CLI version can be identified without digging through logs. Sessions store in~/.codex/sessionsand resume follow-ups the same way the user's owncodexwould.Summary by CodeRabbit
New Features
CODEX_COMMANDconfiguration for selecting a Codex executable.Documentation