Repository navigation
STA-251: add external coding agent integrations - #1541
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds ACP external-agent support across agent-core, browser runtime services, provider preferences, model selection, structured user questions, tool rendering, and local-agent setup. It adds Codex, Claude Code, and OpenCode integrations with bundled ACP and MCP executables. ChangesACP external agents and provider integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
| Filename | Overview |
|---|---|
| apps/browser/src/backend/agents/acp/runtime.ts | Implements serialized ACP sessions, turn lifecycle management, streamed output mapping, approvals, structured input, cancellation, and session persistence. |
| apps/browser/src/backend/agents/acp/form-lifecycle.ts | Coordinates one active Stagewise structured-input form and publishes completed or cancelled results. |
| apps/browser/src/backend/agents/acp/stagewise-mcp-bridge.ts | Provides an authenticated loopback callback bridge for structured external-agent input requests. |
| apps/browser/src/backend/agents/acp/process-client.ts | Manages ACP subprocess startup, protocol initialization, callback dispatch, cancellation, and shutdown. |
| packages/agent-core/src/host/external-agent-runtime.ts | Defines the host-agnostic runtime boundary used to delegate agent turns to external coding agents. |
Reviews (13): Last reviewed commit: "fix(stagewise): discard stale ACP sessio..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/agent-core/src/agents/base-agent.ts (1)
3241-3285: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnguarded awaits on the external runtime skip required local cleanup. Each lifecycle method awaits a call into an out-of-process external agent runtime with no error containment. The external process can die or reject at any time. A rejection then propagates out of the lifecycle method and skips the local state cleanup that the caller performs after it.
packages/agent-core/src/agents/base-agent.ts#L3241-L3285: wrapawait externalStopin a try/catch that logs and reports the failure. A rejection here makesinternalStopreject after local cleanup already finished, sostop()skipssetIsWorkingFalse()at line 968 and the UI keeps showing the agent as working.flushQueue()skips itsrunStep()at line 954, andrecoverInterruptedRun()skips setting_pendingSyntheticContinuation.packages/agent-core/src/agents/base-agent.ts#L3873-L3873: wrapawait this.externalRuntime?.teardown()in a try/catch.AgentManager.onTeardownloops overactiveAgentscallingagent.onTeardown()with no try/catch, so one rejecting teardown aborts the loop and leaks every remaining agent's resources.The same root cause affects
respondToApprovalat line 861, which is flagged separately because it also wedges approval state.🤖 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/agent-core/src/agents/base-agent.ts` around lines 3241 - 3285, Contain failures from external runtime lifecycle calls so local cleanup and subsequent agents continue even when the out-of-process runtime rejects: in internalStop around externalStop (packages/agent-core/src/agents/base-agent.ts:3241-3285), wrap the await in try/catch, log the failure, and report it without rethrowing; in onTeardown at externalRuntime.teardown (packages/agent-core/src/agents/base-agent.ts:3873), apply the same contained logging/reporting behavior so AgentManager.onTeardown can continue its loop. The separately flagged respondToApproval path requires the same treatment but is not a direct change site for this comment.
🧹 Nitpick comments (3)
apps/browser/src/backend/agents/providers/codex/app-server-client.ts (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer the application
LoggeroverConsole.The constructor types
loggerasPick<Console, 'debug' | 'warn'>, and both call sites (models.tsandchatgpt-auth.ts) pass the globalconsole.AcpProcessClientinapps/browser/src/backend/agents/acp/process-client.tsusesPick<Logger, 'debug' | 'warn'>instead. Use the sameLoggertype here so ACP process output reaches the structured log sinks.🤖 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/browser/src/backend/agents/providers/codex/app-server-client.ts` around lines 31 - 34, Update the AcpProcessClient constructor logger parameter to use Pick<Logger, 'debug' | 'warn'> instead of Pick<Console, 'debug' | 'warn'>, and ensure both callers in models.ts and chatgpt-auth.ts pass the application Logger so output reaches structured logging.apps/browser/src/backend/agents/providers/opencode/acp-adapter.ts (1)
24-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the approval mapping and the MCP timeout constant.
environment(Line 25) andprocessKeyForApproval(Line 47) repeat the same expressionapprovalMode === 'alwaysAllow' ? 'allow' : 'ask'. The two values must stay equal, because the process cache key must match the configured permission. Extract one helper. Also name the12 * 60 * 60 * 1_000timeout.♻️ Proposed refactor
+const MCP_TIMEOUT_MS = 12 * 60 * 60 * 1_000; + +function bashPermission(approvalMode: string): string { + return approvalMode === 'alwaysAllow' ? 'allow' : 'ask'; +} + export const openCodeAcpAdapter: AcpAdapter = {async environment(env, approvalMode) { - const permission = approvalMode === 'alwaysAllow' ? 'allow' : 'ask'; + const permission = bashPermission(approvalMode);- mcp_timeout: Math.max( - numberValue(experimental.mcp_timeout) ?? 0, - 12 * 60 * 60 * 1_000, - ), + mcp_timeout: Math.max( + numberValue(experimental.mcp_timeout) ?? 0, + MCP_TIMEOUT_MS, + ),processKeyForApproval(approvalMode) { - return approvalMode === 'alwaysAllow' ? 'allow' : 'ask'; + return bashPermission(approvalMode); },🤖 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/browser/src/backend/agents/providers/opencode/acp-adapter.ts` around lines 24 - 48, Extract the repeated approval mapping from environment and processKeyForApproval into a shared helper, and use it in both methods so the configured permission and cache key remain identical. Replace the inline MCP timeout calculation in environment with a named constant representing the 12-hour timeout.apps/browser/src/backend/agents/providers/codex/stagewise-harness.ts (1)
56-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKey
blocksby type and id.
textBlocklooks upblocksbyidonly. If atextpart and areasoningpart ever share an id, the second lookup returns the first block and appends reasoning text into the text block. Include the type in the key to remove that coupling.♻️ Proposed refactor
- const existing = blocks.get(id); + const key = `${type}:${id}`; + const existing = blocks.get(key); if (existing) return existing; const block = { type, text: '', providerMetadata } as TextBlock; - blocks.set(id, block); + blocks.set(key, block);🤖 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/browser/src/backend/agents/providers/codex/stagewise-harness.ts` around lines 56 - 67, Update the blocks lookup and storage in textBlock so entries are keyed by both type and id, ensuring text and reasoning parts with the same id create and retrieve separate blocks while preserving existing reuse behavior for matching type-id pairs.
🤖 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/browser/src/backend/agents/acp/adapter.ts`:
- Around line 132-142: Update the Node version probe in prepareAcpProcess,
specifically the execFileAsync call, to include an appropriate timeout option so
a non-terminating node --version process cannot leave startup pending. Preserve
the existing version parsing and minimumNodeMajor validation behavior.
- Around line 111-116: Ensure Windows shell launches quote executable paths and
arguments before passing them to cmd.exe: in
apps/browser/src/backend/agents/acp/adapter.ts:111-116, document or redesign
needsShell to enforce this requirement; in
apps/browser/src/backend/agents/acp/process-client.ts:65-70, quote
launch.command and launch.args when launch.shell is true; and in
apps/browser/src/backend/agents/providers/codex/app-server-client.ts:57-61,
quote executable whenever needsShell(executable) returns true, including paths
and values containing spaces or shell metacharacters.
In `@apps/browser/src/backend/agents/acp/form-lifecycle.ts`:
- Around line 53-65: Update the userInput promise handling in the form lifecycle
flow so the promise returned by requestUserInput always has a rejection handler
attached before Promise.race executes. Preserve the existing cancellation result
and cleanup behavior while ensuring a later rejection after cancelled wins
cannot become unhandled.
In `@apps/browser/src/backend/agents/acp/process-client.ts`:
- Around line 109-122: Update the spawn-and-initialize flow in start to bound
the initialize request with the existing timeout mechanism, ensuring an
unresponsive agent causes initialization to fail. Wrap the sequence so every
start failure invokes this.close() before rethrowing the original error.
In `@apps/browser/src/backend/agents/acp/runtime.test.ts`:
- Around line 620-642: Update the “hides Claude internal plan files” test to
derive the home directory with os.homedir() instead of process.env.HOME, and use
that value when constructing both Claude plan file paths. Preserve the existing
assertion and test behavior across platforms.
In `@apps/browser/src/backend/agents/acp/stagewise-mcp-bridge.ts`:
- Around line 69-75: Update AcpMcpBridge.close to call
server.closeAllConnections() after awaiting server.close(), ensuring idle
keep-alive sockets are closed during shutdown on Node 22.12+.
In `@apps/browser/src/backend/agents/providers/codex/acp-adapter.ts`:
- Around line 26-28: Update sessionModeForApproval to use Codex’s broader
agent-full-access mode for alwaysAllow, while preserving read-only for alwaysAsk
and the existing smart behavior. In runtime.ts, assign alwaysAllow a distinct
new Codex session key so it does not reuse the smart process key and can
establish the increased-access session.
In `@apps/browser/src/backend/agents/providers/codex/app-server-client.ts`:
- Around line 152-159: Update CodexAppServerClient.close to preserve an error
listener on the child process instead of removing all listeners, preventing
kill-related error events from becoming uncaught. After sending SIGTERM, add a
bounded fallback that sends SIGKILL if the process remains running, including
the shell-wrapper case, while keeping shutdown idempotent and avoiding unbounded
timers.
- Around line 123-150: Make start idempotent by caching its in-flight/completed
promise and returning that promise on subsequent calls instead of spawning
another process; update the start flow and its process assignment accordingly.
Reset the cached start promise in both close and handleExit so the client can
start cleanly after shutdown or unexpected exit, while preserving discoverModels
and refreshChatGptAccount behavior.
In `@apps/browser/src/backend/agents/providers/codex/models.ts`:
- Around line 33-43: Update the Codex model mapping around the returned object
to use the per-model context_window/contextWindow value when provided, and fall
back to a named context-window constant only when absent. Remove the hardcoded
258_000 value while preserving the existing model capability and reasoning
fields.
In `@apps/browser/src/backend/agents/providers/opencode/provider.ts`:
- Around line 100-104: Update discoverOpenCodeModels around the execFileAsync
call for “models --verbose” to enforce a finite timeout; retain the
child-process handle, race execution against the timeout, kill the child when
the timeout wins, and route the timeout through the existing discovery error
path.
- Around line 38-89: Rename the local const output binding in
parseOpenCodeModels to avoid colliding with the function’s output parameter, and
update the output modality references to use the new binding.
In `@apps/browser/src/backend/services/preferences.ts`:
- Around line 2054-2059: Update preferences.getLocalAgentAvailability to safely
look up PROVIDER_TYPE_DISPLAY_INFO for unknown typeId values, following the
guarded lookup pattern already used near the existing reference, and return the
established unavailable/error result instead of dereferencing undefined. Reuse
the resulting displayInfo.displayName in the error message, and call
this.assertNotDisposed() at the start of the method to match other public
methods.
In
`@apps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.ts`:
- Around line 162-164: Re-validate the flat-parsed input in the
ask-user-questions tool by passing it through
askUserQuestionsToolInputSchema.parse and remove the unsafe
AskUserQuestionsToolInput cast, while retaining
askUserQuestionsToolInputSchemaFlat as the provider-facing inputSchema. In
apps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.ts
lines 162-164, update the parse flow accordingly; in
apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts
lines 94-97, add a comment beside the two schema exports explaining the flat
variant’s strict-mode purpose and that callers must re-validate with
askUserQuestionsToolInputSchema before treating values as
AskUserQuestionsToolInput.
In
`@apps/browser/src/ui/screens/main/agent-chat/chat/_components/message-part-ui/tools/external-agent.tsx`:
- Around line 29-51: Update the external tool status logic around isFinished,
label, and resultText to derive an explicit error state from part.state ===
'output-error' before treating output-* states as finished. Failed calls must
display a failed label instead of Changed, plan status, or Finished/Running, and
errorText must take precedence so it remains visible even when plan or location
data exists.
In
`@apps/browser/src/ui/screens/main/agent-chat/chat/_components/model-select.tsx`:
- Around line 306-310: Update the runtime context-window fallback logic around
runtimeContextWindow so zero is treated as unknown, not as a valid size. Use a
truthiness check before overriding the catalog context-window value, ensuring
the footer retains the catalog denominator and does not render “0k context.”
In `@packages/agent-core/src/agents/base-agent.ts`:
- Around line 861-863: Update sendToolApprovalResponse around the
externalRuntime.respondToApproval call to contain any rejection from the
external runtime, ensuring resolveApproval always executes afterward. Preserve
the handledExternally result for successful responses, but allow local approval
state to resolve even when the external call fails, including the corresponding
path around the later occurrence.
- Around line 2354-2359: In runExternalStep, re-check this._stepGeneration
against the captured stepGeneration after the awaited saveState and before
reading queuedMessages or invoking onIdle/emitNotificationEvent. Return
immediately when the generation changed, so a stopped turn neither emits done
nor schedules runStep; preserve the existing success behavior when the
generation still matches.
---
Outside diff comments:
In `@packages/agent-core/src/agents/base-agent.ts`:
- Around line 3241-3285: Contain failures from external runtime lifecycle calls
so local cleanup and subsequent agents continue even when the out-of-process
runtime rejects: in internalStop around externalStop
(packages/agent-core/src/agents/base-agent.ts:3241-3285), wrap the await in
try/catch, log the failure, and report it without rethrowing; in onTeardown at
externalRuntime.teardown (packages/agent-core/src/agents/base-agent.ts:3873),
apply the same contained logging/reporting behavior so AgentManager.onTeardown
can continue its loop. The separately flagged respondToApproval path requires
the same treatment but is not a direct change site for this comment.
---
Nitpick comments:
In `@apps/browser/src/backend/agents/providers/codex/app-server-client.ts`:
- Around line 31-34: Update the AcpProcessClient constructor logger parameter to
use Pick<Logger, 'debug' | 'warn'> instead of Pick<Console, 'debug' | 'warn'>,
and ensure both callers in models.ts and chatgpt-auth.ts pass the application
Logger so output reaches structured logging.
In `@apps/browser/src/backend/agents/providers/codex/stagewise-harness.ts`:
- Around line 56-67: Update the blocks lookup and storage in textBlock so
entries are keyed by both type and id, ensuring text and reasoning parts with
the same id create and retrieve separate blocks while preserving existing reuse
behavior for matching type-id pairs.
In `@apps/browser/src/backend/agents/providers/opencode/acp-adapter.ts`:
- Around line 24-48: Extract the repeated approval mapping from environment and
processKeyForApproval into a shared helper, and use it in both methods so the
configured permission and cache key remain identical. Replace the inline MCP
timeout calculation in environment with a named constant representing the
12-hour timeout.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3743ac56-7872-416c-ac8e-fd1cafb6cd4a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (69)
apps/browser/.gitignoreapps/browser/package.jsonapps/browser/scripts/bundle-acp-agents.tsapps/browser/src/backend/agents/acp/adapter-registry.tsapps/browser/src/backend/agents/acp/adapter.test.tsapps/browser/src/backend/agents/acp/adapter.tsapps/browser/src/backend/agents/acp/elicitation-mapper.tsapps/browser/src/backend/agents/acp/form-lifecycle.tsapps/browser/src/backend/agents/acp/process-client.tsapps/browser/src/backend/agents/acp/prompt-builder.test.tsapps/browser/src/backend/agents/acp/prompt-builder.tsapps/browser/src/backend/agents/acp/runtime.test.tsapps/browser/src/backend/agents/acp/runtime.tsapps/browser/src/backend/agents/acp/stagewise-mcp-bridge.tsapps/browser/src/backend/agents/acp/stagewise-mcp-server.tsapps/browser/src/backend/agents/acp/tool-mapper.tsapps/browser/src/backend/agents/providers/claude-code/acp-adapter.tsapps/browser/src/backend/agents/providers/claude-code/provider.tsapps/browser/src/backend/agents/providers/codex/acp-adapter.tsapps/browser/src/backend/agents/providers/codex/app-server-client.tsapps/browser/src/backend/agents/providers/codex/chatgpt-auth.tsapps/browser/src/backend/agents/providers/codex/codex-harness.tsapps/browser/src/backend/agents/providers/codex/models.tsapps/browser/src/backend/agents/providers/codex/stagewise-harness.test.tsapps/browser/src/backend/agents/providers/codex/stagewise-harness.tsapps/browser/src/backend/agents/providers/external-agent.tsapps/browser/src/backend/agents/providers/opencode/acp-adapter.tsapps/browser/src/backend/agents/providers/opencode/provider.test.tsapps/browser/src/backend/agents/providers/opencode/provider.tsapps/browser/src/backend/agents/providers/opencode/tool-normalizer.tsapps/browser/src/backend/agents/providers/registry.tsapps/browser/src/backend/main.tsapps/browser/src/backend/services/agent-core-bridge/host-models.tsapps/browser/src/backend/services/agent-core-bridge/host.test.tsapps/browser/src/backend/services/preferences.test.tsapps/browser/src/backend/services/preferences.tsapps/browser/src/backend/services/toolbox/index.tsapps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.tsapps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.tsapps/browser/src/shared/karton-contracts/ui/agent/tools/types.tsapps/browser/src/shared/karton-contracts/ui/index.tsapps/browser/src/shared/karton-contracts/ui/shared-types.tsapps/browser/src/shared/model-thinking-capabilities.test.tsapps/browser/src/shared/model-thinking-capabilities.tsapps/browser/src/shared/provider-instance-helpers.test.tsapps/browser/src/shared/provider-instance-helpers.tsapps/browser/src/ui/components/provider-instance-logo.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_components/chat-history.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_components/footer-status-card/index.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_components/message-part-ui/tools/ask-user-questions.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_components/message-part-ui/tools/external-agent.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_components/message-part-ui/tools/unknown.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_components/model-presets-shared.tsapps/browser/src/ui/screens/main/agent-chat/chat/_components/model-select.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_components/panel-footer.tsxapps/browser/src/ui/screens/main/agent-chat/chat/_lib/chat-history-scroll-event.tsapps/browser/src/ui/screens/settings/sections/model-presets-section.tsxapps/browser/src/ui/screens/settings/sections/models-providers-section.tsxapps/browser/src/ui/utils/model-thinking.tspackages/agent-core/src/agents/base-agent.tspackages/agent-core/src/host/external-agent-runtime.tspackages/agent-core/src/host/host.tspackages/agent-core/src/host/index.tspackages/agent-core/src/services/agent-manager/agent-manager.tspackages/agent-core/src/services/agent-manager/state-mutations/simple.tspackages/agent-core/src/services/agent-manager/universal-toolbox.tspackages/agent-core/src/types/agent.tspackages/agent-shell/src/engine/session-manager.test.tspackages/agent-shell/src/engine/session-manager.ts
Comment on lines
+111
to
+116
| export function needsShell( | ||
| command: string, | ||
| platform: NodeJS.Platform = process.platform, | ||
| ): boolean { | ||
| return platform === 'win32' && /\.(?:cmd|bat)$/i.test(command); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unquoted command passed to cmd.exe when needsShell enables the shell. needsShell returns true for Windows .cmd and .bat files. Every caller then sets shell: true on spawn. With shell: true, Node joins the command and arguments into one string for cmd.exe and applies no quoting. A resolved path that contains a space, for example C:\Program Files\nodejs\codex.cmd, breaks the launch. Paths supplied through CODEX_PATH, CLAUDE_CODE_EXECUTABLE, or OPENCODE_EXECUTABLE can also carry shell metacharacters into cmd.exe.
apps/browser/src/backend/agents/acp/adapter.ts#L111-L116: document the quoting requirement ofneedsShell, or return a prepared, quoted command instead of a boolean.apps/browser/src/backend/agents/acp/process-client.ts#L65-L70: quotelaunch.commandandlaunch.argswhenlaunch.shellistrue.apps/browser/src/backend/agents/providers/codex/app-server-client.ts#L57-L61: quoteexecutablewhenneedsShell(executable)returnstrue.
🧰 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 { execFile } 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
apps/browser/src/backend/agents/acp/adapter.ts#L111-L116(this comment)apps/browser/src/backend/agents/acp/process-client.ts#L65-L70apps/browser/src/backend/agents/providers/codex/app-server-client.ts#L57-L61
🤖 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/browser/src/backend/agents/acp/adapter.ts` around lines 111 - 116,
Ensure Windows shell launches quote executable paths and arguments before
passing them to cmd.exe: in
apps/browser/src/backend/agents/acp/adapter.ts:111-116, document or redesign
needsShell to enforce this requirement; in
apps/browser/src/backend/agents/acp/process-client.ts:65-70, quote
launch.command and launch.args when launch.shell is true; and in
apps/browser/src/backend/agents/providers/codex/app-server-client.ts:57-61,
quote executable whenever needsShell(executable) returns true, including paths
and values containing spaces or shell metacharacters.
Source: Linters/SAST tools
Comment on lines
+162
to
+164
| const params = askUserQuestionsToolInputSchemaFlat.parse( | ||
| input, | ||
| ) as unknown as AskUserQuestionsToolInput; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The flat parse is cast to a stronger type without re-validation. askUserQuestionsToolInputSchemaFlat makes options, defaultValue, and the other per-type fields optional for every type value. AskUserQuestionsToolInput derives from askUserQuestionsToolInputSchema, which requires options with min(1) for radio-group and checkbox-group. Casting the flat parse result to the structured type asserts guarantees the flat schema never checked.
apps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.ts#L162-L164: after the flat parse, pass the result throughaskUserQuestionsToolInputSchema.parse(...)and remove theas unknown as AskUserQuestionsToolInputcast. Keep the flat schema as the tool'sinputSchemafor provider strict-mode compatibility.apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts#L94-L97: add a comment next to the two exports that records why the flat variant exists and that callers must re-validate withaskUserQuestionsToolInputSchemabefore treating the value asAskUserQuestionsToolInput.
External ACP agents now reach this path through ExternalAgentRuntimeContext.requestUserInput, which accepts unknown, so no upstream schema constrains the payload.
📍 Affects 2 files
apps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.ts#L162-L164(this comment)apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts#L94-L97
🤖 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/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.ts`
around lines 162 - 164, Re-validate the flat-parsed input in the
ask-user-questions tool by passing it through
askUserQuestionsToolInputSchema.parse and remove the unsafe
AskUserQuestionsToolInput cast, while retaining
askUserQuestionsToolInputSchemaFlat as the provider-facing inputSchema. In
apps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.ts
lines 162-164, update the parse flow accordingly; in
apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts
lines 94-97, add a comment beside the two schema exports explaining the flat
variant’s strict-mode purpose and that callers must re-validate with
askUserQuestionsToolInputSchema before treating values as
AskUserQuestionsToolInput.
Comment on lines
+306
to
+310
| const runtimeContextWindow = useKartonState((s) => | ||
| openAgent | ||
| ? s.agents.instances[openAgent]?.state.contextWindowSize | ||
| : undefined, | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat a zero runtime context-window size as unknown.
Line 364 only rejects undefined. If the runtime reports 0, Line 373 renders 0k context and Line 372 writes contextWindowRaw: 0, which replaces the catalog value that the footer token bar uses as a denominator. Use a truthiness check so an unknown size keeps the catalog value.
🛡️ Proposed fix
const usesRuntimeContext =
- runtimeContextWindow !== undefined &&
+ !!runtimeContextWindow &&
entry.modelId === selectedModel &&
entry.instanceId ===
(selectedProviderInstanceId ?? DEFAULT_INSTANCE_ID);Also applies to: 363-375
🤖 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/browser/src/ui/screens/main/agent-chat/chat/_components/model-select.tsx`
around lines 306 - 310, Update the runtime context-window fallback logic around
runtimeContextWindow so zero is treated as unknown, not as a valid size. Use a
truthiness check before overriding the catalog context-window value, ensuring
the footer retains the catalog denominator and does not render “0k context.”
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts">
<violation number="1" location="apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts:63">
P2: The flat schema accepts defaults invalid for the selected control, so checkbox defaults such as `'true'` are silently rendered as unchecked. Make defaults conditional on `type`, matching `questionFieldSchema`.</violation>
<violation number="2" location="apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts:69">
P1: A flattened radio or checkbox-group can omit `options`, then opening its form throws on `field.options.map`. Preserve the per-type required `options` constraint when accepting flat input, or normalize/reject these fields before storing them.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| min: z.number().optional(), | ||
| max: z.number().optional(), | ||
| required: z.boolean().optional(), | ||
| options: z.array(questionFieldOptionSchema).min(1).optional(), |
There was a problem hiding this comment.
P1: A flattened radio or checkbox-group can omit options, then opening its form throws on field.options.map. Preserve the per-type required options constraint when accepting flat input, or normalize/reject these fields before storing them.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts, line 69:
<comment>A flattened radio or checkbox-group can omit `options`, then opening its form throws on `field.options.map`. Preserve the per-type required `options` constraint when accepting flat input, or normalize/reject these fields before storing them.</comment>
<file context>
@@ -0,0 +1,129 @@
+ min: z.number().optional(),
+ max: z.number().optional(),
+ required: z.boolean().optional(),
+ options: z.array(questionFieldOptionSchema).min(1).optional(),
+ allowOther: z.boolean().optional(),
+ defaultValues: z.array(z.string()).optional(),
</file context>
| type: z.enum(['input', 'radio-group', 'checkbox', 'checkbox-group']), | ||
| inputType: z.enum(['text', 'email', 'number', 'password']).optional(), | ||
| placeholder: z.string().optional(), | ||
| defaultValue: z.union([z.string(), z.number(), z.boolean()]).optional(), |
There was a problem hiding this comment.
P2: The flat schema accepts defaults invalid for the selected control, so checkbox defaults such as 'true' are silently rendered as unchecked. Make defaults conditional on type, matching questionFieldSchema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts, line 63:
<comment>The flat schema accepts defaults invalid for the selected control, so checkbox defaults such as `'true'` are silently rendered as unchecked. Make defaults conditional on `type`, matching `questionFieldSchema`.</comment>
<file context>
@@ -0,0 +1,129 @@
+ type: z.enum(['input', 'radio-group', 'checkbox', 'checkbox-group']),
+ inputType: z.enum(['text', 'email', 'number', 'password']).optional(),
+ placeholder: z.string().optional(),
+ defaultValue: z.union([z.string(), z.number(), z.boolean()]).optional(),
+ minLength: z.number().optional(),
+ maxLength: z.number().optional(),
</file context>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/browser/src/backend/agents/providers/codex/app-server-client.ts (1)
127-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake process startup idempotent.
discoverModelsandrefreshChatGptAccounteach callstart.
Each call spawns a newcodex app-serverprocess and overwrites
this.process.
The previous process remains running. Its exit is then ignored by
handleExit.Cache the in-flight or completed startup promise. Reset that promise in
closeandhandleExit.Proposed fix
+ private startPromise: Promise<void> | null = null; + + private ensureStarted(): Promise<void> { + this.startPromise ??= this.start().catch((error) => { + this.startPromise = null; + throw error; + }); + return this.startPromise; + } + public async discoverModels(): Promise<CodexModel[]> { - await this.start(); + await this.ensureStarted();🤖 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/browser/src/backend/agents/providers/codex/app-server-client.ts` around lines 127 - 145, Make app-server startup idempotent by caching the in-flight or completed startup promise used by the start method, so repeated calls from discoverModels and refreshChatGptAccount reuse the same process instead of spawning another. Reset the cached startup promise in both close and handleExit, while preserving existing process lifecycle and exit handling.apps/browser/src/backend/agents/acp/runtime.test.ts (1)
632-643: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBuild the plan path with
nodePath.jointo keep the test platform independent.Line 632 concatenates
homedir()with forward slashes. On Windowshomedir()returns a backslash path, soplanPathbecomes a mixed-separator string such asC:\Users\x/.claude/plans/test.md. If the Claude adapter derives its hidden path prefix withnodePath.join, the prefix uses backslashes and the prefix match fails. UsenodePath.joinfor both thelocationsentry and the diffpath.♻️ Proposed fix
- const planPath = `${homedir()}/.claude/plans/test.md`; + const planPath = nodePath.join(homedir(), '.claude', 'plans', 'test.md');🤖 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/browser/src/backend/agents/acp/runtime.test.ts` around lines 632 - 643, Update the planPath construction in the runtime test to use nodePath.join(homedir(), ...) instead of string concatenation, and reuse that normalized path for both the locations entry and the diff path in runtime.upsertTool.
🧹 Nitpick comments (3)
packages/agent-core/src/host/models.ts (1)
208-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the default when the method is omitted.
Callers treat only an explicit
falseas a rejection, so an unimplemented method or atruereturn both allow the entry. State that contract in the doc comment so hosts do not assume a missing implementation blocks utility calls.♻️ Proposed doc update
- /** Returns whether an entry can serve internal utility requests. */ + /** + * Returns whether an entry can serve internal utility requests. + * + * Callers reject an entry only when this returns `false`. Hosts that + * omit this method allow every entry. + */ supportsUtilityCalls?(entry: UtilityModelEntry): boolean;🤖 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/agent-core/src/host/models.ts` around lines 208 - 210, Update the doc comment for supportsUtilityCalls to state that omitted or undefined implementations, and an explicit true result, allow utility calls; only an explicit false rejects them.apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.test.ts (1)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rejection reason, and add a positive-path case.
Line 18 uses a bare
.toThrow(). The test passes for any thrown error, including one unrelated to the flattened-field mismatch. Assert the failing path or message. The suite also has no case that a valid nested input parses, so an over-strict schema change would not fail either test.♻️ Proposed change
- expect(() => askUserQuestionsToolInputSchema.parse(flattened)).toThrow(); + const result = askUserQuestionsToolInputSchema.safeParse(flattened); + expect(result.success).toBe(false); + expect(result.error?.issues[0]?.path).toContain('fields'); + }); + + it('accepts a valid nested input', () => { + expect(() => + askUserQuestionsToolInputSchema.parse( + input([ + { + type: 'radio-group', + questionId: 'mode', + label: 'Mode', + options: [{ value: 'a', label: 'A' }], + }, + ]), + ), + ).not.toThrow(); });Adjust the option shape to match the nested schema.
🤖 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/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.test.ts` around lines 13 - 19, Update the tests around askUserQuestionsToolInputSchema to assert the specific validation path or message for flattened input being rejected, rather than using a bare toThrow. Add a positive case proving valid nested option input parses successfully, and adjust the test fixture’s option shape to match the nested schema.packages/agent-core/src/agents/shared/history-compression/index.ts (1)
272-272: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe new
supportsUtilityCallsguard can skip every candidate, and both call sites then report a misleading error. When the guard rejects all entries, no attempt runs,lastErrorstaysundefined, and each function throws a generic "all models failed" error. An external-agent-only preset reaches this path.
packages/agent-core/src/agents/shared/history-compression/index.ts#L272-L272: track whether any attempt ran in the loop and in the fallback branch at lines 304-310, then throw a distinct "no utility-capable model configured" error at line 332.packages/agent-core/src/agents/shared/title-generation/index.ts#L166-L166: apply the same attempt tracking and throw the distinct error at line 267.🤖 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/agent-core/src/agents/shared/history-compression/index.ts` at line 272, Track whether any model attempt runs in the history-compression loop and its fallback branch in packages/agent-core/src/agents/shared/history-compression/index.ts (lines 272-272 and 304-310), then have the error path at line 332 report a distinct “no utility-capable model configured” error when none ran; apply the same attempt tracking and distinct error handling in packages/agent-core/src/agents/shared/title-generation/index.ts (line 166, with the error at line 267).
🤖 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.
Duplicate comments:
In `@apps/browser/src/backend/agents/acp/runtime.test.ts`:
- Around line 632-643: Update the planPath construction in the runtime test to
use nodePath.join(homedir(), ...) instead of string concatenation, and reuse
that normalized path for both the locations entry and the diff path in
runtime.upsertTool.
In `@apps/browser/src/backend/agents/providers/codex/app-server-client.ts`:
- Around line 127-145: Make app-server startup idempotent by caching the
in-flight or completed startup promise used by the start method, so repeated
calls from discoverModels and refreshChatGptAccount reuse the same process
instead of spawning another. Reset the cached startup promise in both close and
handleExit, while preserving existing process lifecycle and exit handling.
---
Nitpick comments:
In
`@apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.test.ts`:
- Around line 13-19: Update the tests around askUserQuestionsToolInputSchema to
assert the specific validation path or message for flattened input being
rejected, rather than using a bare toThrow. Add a positive case proving valid
nested option input parses successfully, and adjust the test fixture’s option
shape to match the nested schema.
In `@packages/agent-core/src/agents/shared/history-compression/index.ts`:
- Line 272: Track whether any model attempt runs in the history-compression loop
and its fallback branch in
packages/agent-core/src/agents/shared/history-compression/index.ts (lines
272-272 and 304-310), then have the error path at line 332 report a distinct “no
utility-capable model configured” error when none ran; apply the same attempt
tracking and distinct error handling in
packages/agent-core/src/agents/shared/title-generation/index.ts (line 166, with
the error at line 267).
In `@packages/agent-core/src/host/models.ts`:
- Around line 208-210: Update the doc comment for supportsUtilityCalls to state
that omitted or undefined implementations, and an explicit true result, allow
utility calls; only an explicit false rejects them.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dda0571c-a885-40aa-a760-96f2068dc929
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
apps/browser/package.jsonapps/browser/src/backend/agents/acp/adapter.test.tsapps/browser/src/backend/agents/acp/adapter.tsapps/browser/src/backend/agents/acp/elicitation-mapper.tsapps/browser/src/backend/agents/acp/form-lifecycle.tsapps/browser/src/backend/agents/acp/process-client.tsapps/browser/src/backend/agents/acp/prompt-builder.test.tsapps/browser/src/backend/agents/acp/prompt-builder.tsapps/browser/src/backend/agents/acp/runtime.test.tsapps/browser/src/backend/agents/acp/runtime.tsapps/browser/src/backend/agents/acp/tool-mapper.tsapps/browser/src/backend/agents/providers/codex/app-server-client.tsapps/browser/src/backend/agents/providers/opencode/acp-adapter.tsapps/browser/src/backend/agents/providers/opencode/provider.tsapps/browser/src/backend/agents/providers/opencode/tool-normalizer.tsapps/browser/src/backend/services/agent-core-bridge/host-models.tsapps/browser/src/backend/services/agent-core-bridge/host.test.tsapps/browser/src/backend/services/preferences.test.tsapps/browser/src/backend/services/preferences.tsapps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.tsapps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.test.tsapps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.tsapps/browser/src/ui/screens/main/agent-chat/chat/_components/message-part-ui/tools/external-agent.tsxpackages/agent-core/src/agents/base-agent.tspackages/agent-core/src/agents/shared/history-compression/index.tspackages/agent-core/src/agents/shared/title-generation.test.tspackages/agent-core/src/agents/shared/title-generation/index.tspackages/agent-core/src/host/models.tspackages/agent-core/src/services/agent-manager/state-mutations/simple.tspackages/agent-core/src/store/state-annotation.md
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/agent-core/src/services/agent-manager/state-mutations/simple.ts
- apps/browser/package.json
- apps/browser/src/backend/services/agent-core-bridge/host.test.ts
- apps/browser/src/backend/agents/providers/opencode/tool-normalizer.ts
- apps/browser/src/backend/agents/providers/opencode/acp-adapter.ts
- apps/browser/src/backend/agents/acp/adapter.test.ts
- apps/browser/src/shared/karton-contracts/ui/agent/tools/ask-user-questions.ts
- apps/browser/src/ui/screens/main/agent-chat/chat/_components/message-part-ui/tools/external-agent.tsx
- apps/browser/src/backend/services/toolbox/tools/user-interaction/ask-user-questions.ts
- apps/browser/src/backend/agents/acp/form-lifecycle.ts
- apps/browser/src/backend/agents/acp/elicitation-mapper.ts
- apps/browser/src/backend/agents/acp/tool-mapper.ts
- apps/browser/src/backend/services/preferences.test.ts
- apps/browser/src/backend/services/preferences.ts
- packages/agent-core/src/agents/base-agent.ts
- apps/browser/src/backend/agents/acp/prompt-builder.ts
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 31 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
glenntws
force-pushed
the
codex-subscription-integration
branch
from
August 15, 2026 08:23
69716d5 to
f8bb1d2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f8bb1d2. Configure here.
| codexHome || nodePath.join(homedir(), '.codex'), | ||
| 'auth.json', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Codex auth ignores shell CODEX_HOME
Medium Severity
getCodexChatGptAuth resolves auth.json from Electron process.env.CODEX_HOME or os.homedir(), while CodexAppServerClient launches the CLI with the login-shell environment. Users who set CODEX_HOME in their shell profile can get working model discovery/usage via app-server but failed ChatGPT auth for the Stagewise Codex harness, or refresh tokens in one home and read them from another.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f8bb1d2. Configure here.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
agent-coreNote
High Risk
Large new subprocess/ACP surface area touching tool execution, file-edit approvals, OAuth/token reads, and session persistence; mistakes could affect security or data outside mounted workspaces.
Overview
Adds external coding agents (Codex, Claude Code, OpenCode) as first-class provider instances by wiring a new
AcpAgentRuntimeintoagent-corethroughexternalAgentRuntimeFactory, while the native Stagewise agent keeps its existing path.The runtime spawns bundled or user-installed CLIs over Agent Client Protocol, maps streaming sessions, tool calls, diffs, shell output, plans, images, and permissions into native Stagewise message parts, and routes smart / always-ask / always-allow approval (including workspace auto-approve for in-mount edits). A local MCP server + HTTP callback bridge exposes
stagewise_request_user_inputso agents can drive the same structured question UI as the built-in agent.Build & CI:
bundle:acpesbuild step shipscodex-acp,claude-agent-acp, andstagewise-mcp-serverartifacts; monorepo smoke tests now run the packaged app from an isolated temp directory withNODE_PATHunset.Providers: Registry gains ACP harness types plus
codex-stagewise(ChatGPT OAuth via Codex CLI auth, OpenAI Responses middleware for utility calls). Model discovery and subscription usage limits are added for Codex, Claude Code, and OpenCode. Utility-model resolution skips external-agent instances so title/compression stay on API-capable models; preferences block duplicate local-agent connections.Reviewed by Cursor Bugbot for commit f8bb1d2. Bugbot is set up for automated code reviews on this repo. Configure here.