Enable OpenCode and Droid in Maestro CLI#520
Closed
chr1syy wants to merge 14 commits intoRunMaestro:mainfrom
Closed
Enable OpenCode and Droid in Maestro CLI#520chr1syy wants to merge 14 commits intoRunMaestro:mainfrom
chr1syy wants to merge 14 commits intoRunMaestro:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughAdds OpenCode and Factory Droid agent support across the CLI: detection, command resolution, streaming spawn/parsing, per-session spawn overrides (customPath/customArgs/customEnvVars/customModel/sshRemoteConfig), storage accessors for agent configs/SSH remotes, tests, and related type/signature updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI Command
participant Batch as Batch Processor
participant Spawner as Agent Spawner
participant AgentCLI as External Agent CLI
participant Parser as Output Parser
User->>CLI: run/send (agent id, session, optional overrides)
CLI->>Batch: enqueue/execute task (includes spawnOverrides)
Batch->>Spawner: spawnAgent(toolType, cwd, prompt, sessionId, spawnOverrides)
Spawner->>Spawner: detectXxx(customPath?, sshRemoteConfig?) -> resolve command/env
Spawner->>AgentCLI: invoke agent CLI (possibly via SSH) with args/env
AgentCLI-->>Spawner: streaming output
Spawner->>Parser: parse streaming output -> AgentResult
Spawner-->>Batch: return AgentResult
Batch-->>CLI: present/summarize result
CLI-->>User: output (JSON or human)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR extends the Maestro CLI to support two new agent types — OpenCode and Factory Droid — alongside the existing Critical finding:
The new OpenCode and Factory Droid detection and spawn logic is well-structured, but the dispatcher leaves the two pre-existing agent types without override support. Confidence Score: 2/5
Sequence DiagramsequenceDiagram
participant CLI as CLI (send / run-playbook)
participant BP as batch-processor
participant SA as spawnAgent
participant Detect as detect*(customPath?)
participant Spawn as spawnXxxAgent
participant Resolve as resolveAgentInvocation
participant Storage as getAgentConfigValues
CLI->>Detect: detectClaude / detectCodex / detectOpenCode / detectDroid
Detect-->>CLI: { available, path }
CLI->>SA: spawnAgent(toolType, cwd, prompt, sessionId, overrides)
BP->>SA: spawnAgent(toolType, cwd, prompt, undefined, spawnOverrides)
alt toolType = claude-code or codex
SA->>Spawn: spawnClaudeAgent / spawnCodexAgent(cwd, prompt, sessionId)
Note over SA,Spawn: ⚠️ overrides NOT forwarded — custom model/args/env silently dropped
else toolType = opencode or factory-droid
SA->>Spawn: spawnOpenCodeAgent / spawnFactoryDroidAgent(cwd, prompt, sessionId, overrides)
Spawn->>Resolve: resolveAgentInvocation(toolType, cwd, prompt, sessionId, overrides)
Resolve->>Storage: getAgentConfigValues(toolType)
Storage-->>Resolve: agentConfigValues
Resolve-->>Spawn: { args, env, contextWindow }
end
Spawn-->>SA: AgentResult
SA-->>CLI: AgentResult
Last reviewed commit: 76ad2d3 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
886
to
+900
| @@ -490,6 +891,14 @@ export async function spawnAgent( | |||
| return spawnClaudeAgent(cwd, prompt, agentSessionId); | |||
| } | |||
|
|
|||
| if (toolType === 'opencode') { | |||
| return spawnOpenCodeAgent(cwd, prompt, agentSessionId, overrides); | |||
| } | |||
|
|
|||
| if (toolType === 'factory-droid') { | |||
| return spawnFactoryDroidAgent(cwd, prompt, agentSessionId, overrides); | |||
| } | |||
There was a problem hiding this comment.
overrides parameter not forwarded to claude-code and codex agents
The overrides parameter (carrying customPath, customArgs, customEnvVars, and customModel) is passed into spawnAgent from both send.ts and batch-processor.ts, but it is never forwarded to spawnCodexAgent (line 887) or spawnClaudeAgent (line 891).
Currently, both of those functions have hardcoded argument lists and don't accept an overrides parameter:
spawnClaudeAgent(cwd: string, prompt: string, agentSessionId?: string)— no overridesspawnCodexAgent(cwd: string, prompt: string, agentSessionId?: string)— no overrides
In contrast, the new agent functions do correctly accept and use overrides:
spawnOpenCodeAgent(..., overrides?: AgentSpawnOverrides)spawnFactoryDroidAgent(..., overrides?: AgentSpawnOverrides)
As a result, any custom model, custom CLI args, or custom environment variables configured for a Claude Code or Codex session are silently discarded in CLI mode, even though the PR adds those fields to SessionInfo and plumbs them through to spawnAgent.
To fix this consistently, spawnClaudeAgent and spawnCodexAgent should either:
- Accept and apply an
overridesparameter (matching the new agents), or - Be refactored to use
resolveAgentInvocationthe same way the new agents do
Example fix:
if (toolType === 'codex') {
return spawnCodexAgent(cwd, prompt, agentSessionId, overrides);
}
if (toolType === 'claude-code') {
return spawnClaudeAgent(cwd, prompt, agentSessionId, overrides);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cli.md (1)
244-244:⚠️ Potential issue | 🟡 MinorRequirements section is incomplete - missing Factory Droid.
The requirements mention "Claude Code, Codex, or OpenCode" but omit Factory Droid, which is now a supported agent type according to line 80.
📝 Suggested fix
-- At least one AI agent CLI must be installed and in PATH (Claude Code, Codex, or OpenCode) +- At least one AI agent CLI must be installed and in PATH (Claude Code, Codex, OpenCode, or Factory Droid)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/cli.md` at line 244, The requirements line that currently reads "At least one AI agent CLI must be installed and in PATH (Claude Code, Codex, or OpenCode)" needs to include the newly supported agent "Factory Droid"; update that sentence in docs/cli.md to list "Factory Droid" alongside Claude Code, Codex, and OpenCode so the Requirements section matches the supported agents referenced elsewhere (see the agent list around line 80).
🧹 Nitpick comments (1)
src/cli/services/agent-spawner.ts (1)
886-900: Add clarifying comment explaining intentional override parameter handling.The
overridesparameter is intentionally omitted fromspawnCodexAgentandspawnClaudeAgentcalls. The implementations already document this design: the CLI agent spawner lacks access to the settings store with global shell env vars and relies solely on the environment Maestro is running in (global shell vars are primarily used by the desktop app). However, a comment at the call site (lines 886–892) explaining why overrides are selectively passed would improve code clarity for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 886 - 900, Add a brief clarifying comment above the conditional block in agent-spawner.ts (around the spawnCodexAgent, spawnClaudeAgent, spawnOpenCodeAgent, spawnFactoryDroidAgent calls) explaining that the omissions of the overrides parameter for spawnCodexAgent and spawnClaudeAgent are intentional because those agents rely only on the environment Maestro is running in and do not have access to the CLI settings store/global shell env vars, whereas spawnOpenCodeAgent and spawnFactoryDroidAgent intentionally accept overrides from the CLI; keep the comment concise and reference the difference in env/source of settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/cli.md`:
- Line 244: The requirements line that currently reads "At least one AI agent
CLI must be installed and in PATH (Claude Code, Codex, or OpenCode)" needs to
include the newly supported agent "Factory Droid"; update that sentence in
docs/cli.md to list "Factory Droid" alongside Claude Code, Codex, and OpenCode
so the Requirements section matches the supported agents referenced elsewhere
(see the agent list around line 80).
---
Nitpick comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 886-900: Add a brief clarifying comment above the conditional
block in agent-spawner.ts (around the spawnCodexAgent, spawnClaudeAgent,
spawnOpenCodeAgent, spawnFactoryDroidAgent calls) explaining that the omissions
of the overrides parameter for spawnCodexAgent and spawnClaudeAgent are
intentional because those agents rely only on the environment Maestro is running
in and do not have access to the CLI settings store/global shell env vars,
whereas spawnOpenCodeAgent and spawnFactoryDroidAgent intentionally accept
overrides from the CLI; keep the comment concise and reference the difference in
env/source of settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0bc08f30-fd9e-42a3-beb6-b3ee498be836
📒 Files selected for processing (7)
docs/cli.mdsrc/cli/commands/run-playbook.tssrc/cli/commands/send.tssrc/cli/services/agent-spawner.tssrc/cli/services/batch-processor.tssrc/cli/services/storage.tssrc/shared/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/services/agent-spawner.ts (1)
808-916: Consider extracting common spawn logic to reduce duplication.
spawnFactoryDroidAgentandspawnOpenCodeAgentare nearly identical (~100 lines each), differing only in:
- Tool type (
'factory-droid'vs'opencode')- Command getter (
getDroidCommandvsgetOpenCodeCommand)- Parser instantiation (
FactoryDroidOutputParservsOpenCodeOutputParser)- Error message text
This is a candidate for extraction into a generic helper, though the existing Claude/Codex spawn functions follow a similar separated pattern. Deferring is acceptable if consistency with the existing approach is preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 808 - 916, The two functions spawnFactoryDroidAgent and spawnOpenCodeAgent duplicate nearly identical spawn/parse/resolve logic; refactor by extracting a generic spawnAgent helper that accepts parameters for toolType (passed to resolveAgentInvocation), command getter (getDroidCommand/getOpenCodeCommand), parser constructor (FactoryDroidOutputParser/OpenCodeOutputParser), and an error message template; have spawnFactoryDroidAgent and spawnOpenCodeAgent call this helper (keeping resolveAgentInvocation, child spawn, stdout/stderr handling, merging usage via mergeUsageStats, and the close/error handlers centralized) so only the differing symbols (tool type string, command getter, parser class, and error text) remain in the small wrapper functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 258-260: Cached path returns a hardcoded source 'settings' even
when it was discovered via PATH; update the detection functions (detectOpenCode,
detectClaude, detectCodex, detectDroid) to preserve or correct the source when
returning a cached value by either (A) changing the cache to store an object
{path, source} and reading that here, or (B) if you prefer a minimal change,
return source: 'path' whenever the cached value (e.g., cachedOpenCodePath) is
used; update the cache write sites in the corresponding detection functions to
save the correct source if you choose option A.
---
Nitpick comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 808-916: The two functions spawnFactoryDroidAgent and
spawnOpenCodeAgent duplicate nearly identical spawn/parse/resolve logic;
refactor by extracting a generic spawnAgent helper that accepts parameters for
toolType (passed to resolveAgentInvocation), command getter
(getDroidCommand/getOpenCodeCommand), parser constructor
(FactoryDroidOutputParser/OpenCodeOutputParser), and an error message template;
have spawnFactoryDroidAgent and spawnOpenCodeAgent call this helper (keeping
resolveAgentInvocation, child spawn, stdout/stderr handling, merging usage via
mergeUsageStats, and the close/error handlers centralized) so only the differing
symbols (tool type string, command getter, parser class, and error text) remain
in the small wrapper functions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 743a3e1f-a5b5-492e-bf62-7a0fe9c18d34
📒 Files selected for processing (2)
docs/cli.mdsrc/cli/services/agent-spawner.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/cli.md
- Track cached CLI path source and unify OpenCode/Droid spawn flow. - Only pass send overrides when present; adjust CLI tests and agent-args typing. Session: 019cbff3-a9dd-71f1-b326-7c9fb965b50e
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/__tests__/cli/services/agent-spawner.test.ts (1)
72-76: Mock wiring looks good, but test coverage for new agents is missing.The
mockGetAgentConfigValuesis correctly wired and defaults to an empty object. However, the test suite doesn't include tests for:
detectOpenCode/detectDroiddetection functionsspawnAgentwith'opencode'or'factory-droid'tool typesConsider adding test cases for the new agent paths to ensure parity with the existing Claude/Codex coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/cli/services/agent-spawner.test.ts` around lines 72 - 76, Add tests exercising the new agent detection and spawn paths: extend the test file to stub/mock getAgentConfigValues via mockGetAgentConfigValues to return values that trigger detectOpenCode and detectDroid, then assert detectOpenCode and detectDroid behavior directly; add spawnAgent test cases that call spawnAgent with tool types 'opencode' and 'factory-droid' and verify the spawned agent type/behavior (similar assertions to existing Claude/Codex tests). Locate usages of mockGetAgentConfigValues, the detectOpenCode and detectDroid functions, and the spawnAgent function in the test suite and mirror the existing Claude/Codex test patterns to cover these new branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/__tests__/cli/services/agent-spawner.test.ts`:
- Around line 72-76: Add tests exercising the new agent detection and spawn
paths: extend the test file to stub/mock getAgentConfigValues via
mockGetAgentConfigValues to return values that trigger detectOpenCode and
detectDroid, then assert detectOpenCode and detectDroid behavior directly; add
spawnAgent test cases that call spawnAgent with tool types 'opencode' and
'factory-droid' and verify the spawned agent type/behavior (similar assertions
to existing Claude/Codex tests). Locate usages of mockGetAgentConfigValues, the
detectOpenCode and detectDroid functions, and the spawnAgent function in the
test suite and mirror the existing Claude/Codex test patterns to cover these new
branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1a1d6ec-329b-43bb-838f-c893610e86c6
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/__tests__/cli/services/agent-spawner.test.tssrc/cli/commands/send.tssrc/cli/services/agent-spawner.tssrc/main/utils/agent-args.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/commands/send.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/services/agent-spawner.ts (1)
117-120:⚠️ Potential issue | 🟠 MajorNormalize Windows
whereoutput before taking the first match.When
wherereturns multiple matches,stdout.trim().split('\n')[0]leaves a trailing\ron Windows. That cached path is then invalid forisExecutable()/spawn(). Split on\r?\nor trim each line before selecting the first entry.Suggested fix
- if (code === 0 && stdout.trim()) { - resolve(stdout.trim().split('\n')[0]); // First match + if (code === 0 && stdout.trim()) { + const firstMatch = stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .find(Boolean); + resolve(firstMatch); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 117 - 120, The code in the proc.on('close', (code) => { ... }) handler resolves the executable path using stdout.trim().split('\n')[0], which can leave a trailing '\r' on Windows; update the resolution to normalize line endings by splitting on /\r?\n/ (or splitting on '\n' and trimming each line) and then take the first non-empty trimmed entry before calling resolve so the cached path is valid for isExecutable() and spawn(); adjust the logic in the function that spawns the child process (the proc.on('close' callback) to use the normalized/trimmed firstMatch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 156-164: The code is caching or masking one-off customPathOverride
into shared cachedClaudePath and falling back to PATH detection; instead, in the
branches that handle customPathOverride (the block using
isExecutable(customPathOverride), writing cachedClaudePath, and returning
available true), stop mutating cachedClaudePath and return the result for that
call only (return { available: true, path: customPathOverride, source:
'settings' } directly). If the override is not executable, do not fall back to
PATH detection—return { available: false, path: customPathOverride, source:
'settings' } (or otherwise indicate unavailable) rather than logging and
continuing; apply the same change to the similar blocks around the other ranges
(the blocks referenced at 209-217, 258-266, 307-315) so overrides remain
per-call and do not leak into cachedClaudePath.
- Around line 578-605: The spawnStreamingAgent implementation always calls
spawn(...) locally; update it to support SSH by (1) getting sshRemoteConfig from
resolveAgentInvocation (or from overrides) alongside args/env/contextWindow, (2)
if sshRemoteConfig is present, call wrapSpawnWithSsh(...) from
ssh-spawn-wrapper.ts instead of directly calling spawn, passing the command
(from commandGetter(overrides?.customPath)), args, options (SpawnOptions), and
the SSH store adapter, and (3) ensure the same returned child/process interface
is used downstream so streaming logic remains unchanged (update references to
spawnStreamingAgent/commandGetter/resolveAgentInvocation/sshRemoteConfig/wrapSpawnWithSsh
accordingly).
---
Outside diff comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 117-120: The code in the proc.on('close', (code) => { ... })
handler resolves the executable path using stdout.trim().split('\n')[0], which
can leave a trailing '\r' on Windows; update the resolution to normalize line
endings by splitting on /\r?\n/ (or splitting on '\n' and trimming each line)
and then take the first non-empty trimmed entry before calling resolve so the
cached path is valid for isExecutable() and spawn(); adjust the logic in the
function that spawns the child process (the proc.on('close' callback) to use the
normalized/trimmed firstMatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55eaf705-0358-4464-9dc3-a46a2cf8f0f3
📒 Files selected for processing (1)
src/cli/services/agent-spawner.ts
Contributor
Author
|
@coderabbitai re-review please |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/cli/services/agent-spawner.ts (2)
256-271: Cache-match check is a good optimization, but override still leaks.The check at lines 257-263 avoids redundant
isExecutablecalls when the override matches the cached path. However, valid overrides are still written to the global cache (line 265), and invalid overrides fall through to PATH detection rather than returning unavailable. This is the same issue pattern asdetectClaude/detectCodex, though mitigated by the match-check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 256 - 271, The override handling leaks invalid overrides into PATH detection and writes to the global cache regardless; update the logic in the override branch around cachedOpenCodePath and customPathOverride so you only set cachedOpenCodePath (and return available=true) when await isExecutable(customPathOverride) succeeds, and if isExecutable returns false immediately return { available: false, path: customPathOverride, source: 'settings' } instead of falling through — mirror the safe behavior used in detectClaude/detectCodex and ensure cachedOpenCodePath is never assigned for non-executable overrides.
569-585: Consider stricter typing for parser interface.The
StreamJsonParsertype usesanyforparseJsonLinereturn and method parameters. While the concrete parsers (OpenCodeOutputParser,FactoryDroidOutputParser) likely have proper internal typing, a shared event type or discriminated union would improve type safety at the boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 569 - 585, The StreamJsonParser interface currently uses any for parseJsonLine and its event parameter; define a shared Event type (or discriminated union) and replace all any occurrences in StreamJsonParser with that concrete type so callers get proper typing. Update the signatures of parseJsonLine, extractSessionId, isResultMessage, and extractUsage to use the new Event type (or union) and adjust OpenCodeOutputParser and FactoryDroidOutputParser implementations to return/accept that Event type (or narrow variants) so the parser boundary is strongly typed and compatible with downstream consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli.md`:
- Around line 244-245: Update the requirements text in the CLI docs where the
bullet currently reads that an AI agent CLI "must be installed and in PATH" to
reflect that the CLI supports customPath overrides; change the wording to allow
binaries that are either in PATH or specified via the customPath configuration
(reference the phrase "installed and in PATH" in the current bullets) and keep
the second bullet about Maestro config files intact. Ensure the new sentence
mentions the supported agent names (Claude Code, Codex, OpenCode, Factory Droid)
and explicitly references the customPath option so readers know non-PATH
installs are supported.
In `@src/shared/types.ts`:
- Around line 31-34: SessionInfo currently omits SSH configuration causing
CLI-only agent paths (send.ts, batch-processor.ts, run-playbook.ts) to pass
AgentSpawnOverrides (customPath/customArgs/customEnvVars/customModel) to
spawnAgent() without any SSH wrapping, so add ssh support to the CLI surface:
extend SessionInfo (or AgentSpawnOverrides) to include AgentSshRemoteConfig
(sshRemoteConfig?: AgentSshRemoteConfig), update the detect* functions to check
sshRemoteConfig in addition to customPath, and change the CLI spawn callers that
pass AgentSpawnOverrides to call the same wrapSpawnWithSsh() logic (or wrap
their spawnAgent() invocation when sshRemoteConfig is present) so
SessionOverrides/AgentSshRemoteConfig are honored during preflight and spawn
just like group-chat-agent.ts does.
---
Nitpick comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 256-271: The override handling leaks invalid overrides into PATH
detection and writes to the global cache regardless; update the logic in the
override branch around cachedOpenCodePath and customPathOverride so you only set
cachedOpenCodePath (and return available=true) when await
isExecutable(customPathOverride) succeeds, and if isExecutable returns false
immediately return { available: false, path: customPathOverride, source:
'settings' } instead of falling through — mirror the safe behavior used in
detectClaude/detectCodex and ensure cachedOpenCodePath is never assigned for
non-executable overrides.
- Around line 569-585: The StreamJsonParser interface currently uses any for
parseJsonLine and its event parameter; define a shared Event type (or
discriminated union) and replace all any occurrences in StreamJsonParser with
that concrete type so callers get proper typing. Update the signatures of
parseJsonLine, extractSessionId, isResultMessage, and extractUsage to use the
new Event type (or union) and adjust OpenCodeOutputParser and
FactoryDroidOutputParser implementations to return/accept that Event type (or
narrow variants) so the parser boundary is strongly typed and compatible with
downstream consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c0e94b5-b2e6-4e92-9a02-b5f919d474f7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/cli.mdsrc/__tests__/cli/services/agent-spawner.test.tssrc/cli/commands/run-playbook.tssrc/cli/commands/send.tssrc/cli/services/agent-spawner.tssrc/cli/services/batch-processor.tssrc/cli/services/storage.tssrc/main/utils/agent-args.tssrc/shared/types.ts
Contributor
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/cli/services/agent-spawner.ts (1)
172-189:⚠️ Potential issue | 🟠 MajorKeep
customPathOverrideper-call.These branches still cache a one-off override and still mask an invalid override by falling back to PATH detection. That leaks one session's override into later detections, and it can make preflight return
available: trueeven though the later spawn will still use the invalid override and fail withENOENT.Suggested direction
if (customPathOverride) { if (await isExecutable(customPathOverride)) { - cachedOpenCodePath = { path: customPathOverride, source: 'settings' }; return { available: true, path: customPathOverride, source: 'settings' }; } - console.error( - `Warning: Custom OpenCode path "${customPathOverride}" is not executable, falling back to PATH detection` - ); + console.error(`Warning: Custom OpenCode path "${customPathOverride}" is not executable`); + return { available: false, path: customPathOverride, source: 'settings' }; }Apply the same pattern to Claude, Codex, and Droid: do not write override values into the shared cache, and do not fall through to PATH detection for an invalid override.
Also applies to: 229-245, 282-305, 342-365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 172 - 189, The code is caching customPathOverride into cachedClaudePath and falling back to PATH detection when the override is invalid, which leaks a one-off override across calls and masks invalid overrides; change the logic in the Claude detection block (and analogously in the Codex and Droid blocks) so that when customPathOverride is provided you validate it with isExecutable and return its result per-call without writing it into cachedClaudePath (or cachedCodexPath/cachedDroidPath), and if the override is invalid return available: false (do not fall through to PATH detection or mutate the shared cache); keep caching only results derived from PATH detection.
🧹 Nitpick comments (1)
src/cli/commands/send.ts (1)
146-165: Consider simplifying the overrides handling.The current pattern works correctly, but could be simplified since
spawnAgenthandles undefined override fields gracefully:♻️ Optional simplification
- const hasOverrides = - agent.customPath !== undefined || - agent.customArgs !== undefined || - agent.customEnvVars !== undefined || - agent.customModel !== undefined || - agent.sshRemoteConfig !== undefined; - const overrides = hasOverrides - ? { - customPath: agent.customPath, - customArgs: agent.customArgs, - customEnvVars: agent.customEnvVars, - customModel: agent.customModel, - sshRemoteConfig: agent.sshRemoteConfig, - } - : undefined; - - // Spawn agent — spawnAgent handles --resume vs --session-id internally - const result = overrides - ? await spawnAgent(agent.toolType, agent.cwd, message, options.session, overrides) - : await spawnAgent(agent.toolType, agent.cwd, message, options.session); + // Spawn agent — spawnAgent handles --resume vs --session-id internally + const result = await spawnAgent(agent.toolType, agent.cwd, message, options.session, { + customPath: agent.customPath, + customArgs: agent.customArgs, + customEnvVars: agent.customEnvVars, + customModel: agent.customModel, + sshRemoteConfig: agent.sshRemoteConfig, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/send.ts` around lines 146 - 165, Replace the hasOverrides/ternary pattern with a single overrides object and always pass it to spawnAgent: construct overrides = { customPath: agent.customPath, customArgs: agent.customArgs, customEnvVars: agent.customEnvVars, customModel: agent.customModel, sshRemoteConfig: agent.sshRemoteConfig } (no conditional) and call spawnAgent(agent.toolType, agent.cwd, message, options.session, overrides); this relies on spawnAgent to handle undefined fields and removes the unnecessary hasOverrides/ternary logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 476-497: When overrides.sshRemoteConfig is present, avoid passing
a locally-cached absolute binary path from getClaudeCommand (or other
get*Command calls) into wrapSpawnWithSsh; instead, if overrides.customPath is
not provided, set the pre-wrap spawn command to the agent's remote-resolvable
binary name (use agentDef.binaryName or the agent's canonical binary name) so
wrapSpawnWithSsh receives a path that will be resolved on the remote host.
Concretely: in spawnClaudeAgent / spawnStreamingAgent (and here where
spawnCommand is derived via getClaudeCommand) check overrides?.sshRemoteConfig
and overrides?.customPath before using get*Command; if sshRemoteConfig is true
and no customPath, replace spawnCommand with agentDef.binaryName (leave
spawnArgs, cwd, env handling and buildExpandedEnv usage intact) so the SSH
wrapper gets a remote-resolvable command.
---
Duplicate comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 172-189: The code is caching customPathOverride into
cachedClaudePath and falling back to PATH detection when the override is
invalid, which leaks a one-off override across calls and masks invalid
overrides; change the logic in the Claude detection block (and analogously in
the Codex and Droid blocks) so that when customPathOverride is provided you
validate it with isExecutable and return its result per-call without writing it
into cachedClaudePath (or cachedCodexPath/cachedDroidPath), and if the override
is invalid return available: false (do not fall through to PATH detection or
mutate the shared cache); keep caching only results derived from PATH detection.
---
Nitpick comments:
In `@src/cli/commands/send.ts`:
- Around line 146-165: Replace the hasOverrides/ternary pattern with a single
overrides object and always pass it to spawnAgent: construct overrides = {
customPath: agent.customPath, customArgs: agent.customArgs, customEnvVars:
agent.customEnvVars, customModel: agent.customModel, sshRemoteConfig:
agent.sshRemoteConfig } (no conditional) and call spawnAgent(agent.toolType,
agent.cwd, message, options.session, overrides); this relies on spawnAgent to
handle undefined fields and removes the unnecessary hasOverrides/ternary logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e404069-5b4d-4f70-8dfa-e20a020ffb4a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/cli.mdsrc/__tests__/cli/services/agent-spawner.test.tssrc/cli/commands/run-playbook.tssrc/cli/commands/send.tssrc/cli/services/agent-spawner.tssrc/cli/services/batch-processor.tssrc/cli/services/storage.tssrc/main/utils/agent-args.tssrc/shared/types.ts
Contributor
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/cli/services/agent-spawner.ts (2)
172-180:⚠️ Potential issue | 🟠 MajorKeep one-off
customPathOverridevalues out of the shared cache.These branches still cache a per-session override and fall through to cached/PATH detection when the override is invalid. That lets availability checks return
trueeven though the later spawn will still use the bad override, and it also leaks one session’s override into later detections.Suggested fix pattern
if (customPathOverride) { if (await isExecutable(customPathOverride)) { - cachedClaudePath = { path: customPathOverride, source: 'settings' }; return { available: true, path: customPathOverride, source: 'settings' }; } - console.error( - `Warning: Custom Claude path "${customPathOverride}" is not executable, falling back to PATH detection` - ); + return { available: false, path: customPathOverride, source: 'settings' }; }Apply the same pattern to
detectCodex,detectOpenCode, anddetectDroid.Also applies to: 229-237, 282-297, 342-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 172 - 180, The customPathOverride branch is storing per-session one-off overrides into the shared cache (e.g., setting cachedClaudePath = { path: customPathOverride, source: 'settings' }) which leaks into later detections and can wrongly report availability; change the logic so that when customPathOverride is provided and isExecutable(customPathOverride) succeeds you return the result ({ available: true, path: customPathOverride, source: 'settings' }) without mutating the shared cachedClaudePath, and when it fails do not write to the cache — just warn and fall back to normal detection; apply the same non-caching pattern to the equivalent branches in detectCodex, detectOpenCode, and detectDroid and only set the shared cached* variables when the value is discovered via PATH detection or other persistent discovery logic.
865-880:⚠️ Potential issue | 🟠 MajorUse the remote-resolvable Codex binary name for SSH runs.
This branch still seeds
wrapSpawnWithSsh()fromgetCodexCommand(). If Codex was previously detected locally, the cached absolute path can be forwarded to the remote host, which then tries to execute a path from the caller machine.Minimal fix
- let spawnCommand = getCodexCommand(overrides?.customPath); + let spawnCommand = + overrides?.sshRemoteConfig && !overrides?.customPath && agentDef?.binaryName + ? agentDef.binaryName + : getCodexCommand(overrides?.customPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 865 - 880, The SSH branch is passing the local absolute path from getCodexCommand() to wrapSpawnWithSsh(), causing remote hosts to try executing a local path; when overrides.sshRemoteConfig is set, switch the command to the remote-resolvable binary name (use agentDef?.binaryName or fallback to getCodexCommand()) before calling wrapSpawnWithSsh so the remote executes the binary name, not a local path; update the logic around spawnCommand/spawnArgs (the block containing overrides.sshRemoteConfig, wrapSpawnWithSsh, getCodexCommand, and agentDef?.binaryName) to set spawnCommand appropriately and then call wrapSpawnWithSsh with that value.
🧹 Nitpick comments (2)
src/cli/commands/send.ts (2)
146-160: Consider filtering out undefined fields from overrides.When
hasOverridesis true, the object includes all fields even if only one is defined. This passes unnecessaryundefinedproperties tospawnAgent.♻️ Optional: Build overrides with only defined fields
- const hasOverrides = - agent.customPath !== undefined || - agent.customArgs !== undefined || - agent.customEnvVars !== undefined || - agent.customModel !== undefined || - agent.sshRemoteConfig !== undefined; - const overrides = hasOverrides - ? { - customPath: agent.customPath, - customArgs: agent.customArgs, - customEnvVars: agent.customEnvVars, - customModel: agent.customModel, - sshRemoteConfig: agent.sshRemoteConfig, - } - : undefined; + const overrides = Object.fromEntries( + Object.entries({ + customPath: agent.customPath, + customArgs: agent.customArgs, + customEnvVars: agent.customEnvVars, + customModel: agent.customModel, + sshRemoteConfig: agent.sshRemoteConfig, + }).filter(([, v]) => v !== undefined) + ); + const hasOverrides = Object.keys(overrides).length > 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/send.ts` around lines 146 - 160, The overrides object currently includes all keys even when their values are undefined (built under hasOverrides), which passes unnecessary undefined props to spawnAgent; change the construction of overrides (referencing hasOverrides, overrides, and agent.customPath / customArgs / customEnvVars / customModel / sshRemoteConfig) so it only contains keys whose values are !== undefined—e.g. create an empty object and conditionally assign each property if defined or build then filter entries (Object.entries/...filter) and return undefined when no keys remain—so spawnAgent receives only present overrides.
163-165: Conditional call can be simplified.Since
spawnAgentacceptsundefinedfor the optionaloverridesparameter, the ternary is redundant.♻️ Optional: Simplify to single call
- const result = overrides - ? await spawnAgent(agent.toolType, agent.cwd, message, options.session, overrides) - : await spawnAgent(agent.toolType, agent.cwd, message, options.session); + const result = await spawnAgent(agent.toolType, agent.cwd, message, options.session, overrides);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/send.ts` around lines 163 - 165, The ternary that conditionally calls spawnAgent is redundant because spawnAgent accepts an optional overrides parameter; replace the conditional expression in the assignment to result with a single call to spawnAgent(agent.toolType, agent.cwd, message, options.session, overrides) so overrides can be passed directly (it may be undefined) and remove the duplicate call paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/cli.md`:
- Line 244: Update the sentence that currently requires an agent CLI in PATH or
via customPath to also mention SSH-only setups by referencing the
sshRemoteConfig option; specifically, change the line that says "At least one AI
agent CLI must be installed in PATH or set via customPath (Claude Code, Codex,
OpenCode, or Factory Droid)" to include "or configured via sshRemoteConfig" (and
optionally a brief note that the CLI can run through an SSH remote host), so
users know SSH-only agent configurations are supported.
In `@src/__tests__/cli/services/agent-spawner.test.ts`:
- Around line 87-93: The mock factory for '../../../cli/services/storage' is
missing an export for readSshRemotes which causes module evaluation to fail;
update the vi.mock call that defines mockGetAgentCustomPath and
mockGetAgentConfigValues to also export readSshRemotes (e.g., add a
mockReadSshRemotes function or vi.fn that returns a sensible default like an
empty array/object) so imports of readSshRemotes in agent-spawner.ts resolve
during tests; keep the existing mockGetAgentCustomPath and
mockGetAgentConfigValues names and behavior and add the new mock export inside
the same mock factory.
---
Duplicate comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 172-180: The customPathOverride branch is storing per-session
one-off overrides into the shared cache (e.g., setting cachedClaudePath = {
path: customPathOverride, source: 'settings' }) which leaks into later
detections and can wrongly report availability; change the logic so that when
customPathOverride is provided and isExecutable(customPathOverride) succeeds you
return the result ({ available: true, path: customPathOverride, source:
'settings' }) without mutating the shared cachedClaudePath, and when it fails do
not write to the cache — just warn and fall back to normal detection; apply the
same non-caching pattern to the equivalent branches in detectCodex,
detectOpenCode, and detectDroid and only set the shared cached* variables when
the value is discovered via PATH detection or other persistent discovery logic.
- Around line 865-880: The SSH branch is passing the local absolute path from
getCodexCommand() to wrapSpawnWithSsh(), causing remote hosts to try executing a
local path; when overrides.sshRemoteConfig is set, switch the command to the
remote-resolvable binary name (use agentDef?.binaryName or fallback to
getCodexCommand()) before calling wrapSpawnWithSsh so the remote executes the
binary name, not a local path; update the logic around spawnCommand/spawnArgs
(the block containing overrides.sshRemoteConfig, wrapSpawnWithSsh,
getCodexCommand, and agentDef?.binaryName) to set spawnCommand appropriately and
then call wrapSpawnWithSsh with that value.
---
Nitpick comments:
In `@src/cli/commands/send.ts`:
- Around line 146-160: The overrides object currently includes all keys even
when their values are undefined (built under hasOverrides), which passes
unnecessary undefined props to spawnAgent; change the construction of overrides
(referencing hasOverrides, overrides, and agent.customPath / customArgs /
customEnvVars / customModel / sshRemoteConfig) so it only contains keys whose
values are !== undefined—e.g. create an empty object and conditionally assign
each property if defined or build then filter entries (Object.entries/...filter)
and return undefined when no keys remain—so spawnAgent receives only present
overrides.
- Around line 163-165: The ternary that conditionally calls spawnAgent is
redundant because spawnAgent accepts an optional overrides parameter; replace
the conditional expression in the assignment to result with a single call to
spawnAgent(agent.toolType, agent.cwd, message, options.session, overrides) so
overrides can be passed directly (it may be undefined) and remove the duplicate
call paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cc96888-8054-48ce-815a-a324aef7f12d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/cli.mdsrc/__tests__/cli/services/agent-spawner.test.tssrc/cli/commands/run-playbook.tssrc/cli/commands/send.tssrc/cli/services/agent-spawner.tssrc/cli/services/batch-processor.tssrc/cli/services/storage.tssrc/main/utils/agent-args.tssrc/shared/types.ts
Contributor
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/commands/send.ts (1)
109-170:⚠️ Potential issue | 🟠 MajorGate SSH overrides on
enabled.
AgentSshRemoteConfigis a config object, not a truthy switch. This path forwards any definedsshRemoteConfiginto both detection andspawnAgent(), butspawnAgent()currently flips to SSH mode on presence alone. A stored{ enabled: false, remoteId: null }will therefore break a localsendcall by trying to wrap it for SSH anyway.Normalize the SSH config once and reuse it
+ const sshRemoteConfig = agent.sshRemoteConfig?.enabled ? agent.sshRemoteConfig : undefined; + // Verify agent CLI is available if (agent.toolType === 'claude-code') { - const claude = await detectClaude(agent.customPath, agent.sshRemoteConfig); + const claude = await detectClaude(agent.customPath, sshRemoteConfig); if (!claude.available) { emitErrorJson( 'Claude Code CLI not found. Install with: npm install -g `@anthropic-ai/claude-code`', @@ } else if (agent.toolType === 'codex') { - const codex = await detectCodex(agent.customPath, agent.sshRemoteConfig); + const codex = await detectCodex(agent.customPath, sshRemoteConfig); @@ } else if (agent.toolType === 'opencode') { - const opencode = await detectOpenCode(agent.customPath, agent.sshRemoteConfig); + const opencode = await detectOpenCode(agent.customPath, sshRemoteConfig); @@ } else if (agent.toolType === 'factory-droid') { - const droid = await detectDroid(agent.customPath, agent.sshRemoteConfig); + const droid = await detectDroid(agent.customPath, sshRemoteConfig); @@ - if (agent.sshRemoteConfig !== undefined) { - next.sshRemoteConfig = agent.sshRemoteConfig; + if (sshRemoteConfig) { + next.sshRemoteConfig = sshRemoteConfig; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/send.ts` around lines 109 - 170, The SSH config object (agent.sshRemoteConfig) is treated as a truthy switch but may have enabled: false; normalize it once (e.g., const ssh = agent.sshRemoteConfig && agent.sshRemoteConfig.enabled ? agent.sshRemoteConfig : undefined) and use that normalized ssh value for all calls: pass ssh into detectClaude/detectCodex/detectOpenCode/detectDroid and set next.sshRemoteConfig = ssh when building overrides, and finally pass the same ssh-aware overrides into spawnAgent; this ensures SSH mode is only used when AgentSshRemoteConfig.enabled is true.
🧹 Nitpick comments (1)
src/__tests__/cli/services/agent-spawner.test.ts (1)
702-794: Add SSH coverage for the new detector paths.These cases only exercise local PATH lookup and cache reuse. The production change also threads
sshRemoteConfigthroughdetectOpenCode()anddetectDroid(), so a remote-only regression would still pass this suite.Based on learnings: When implementing features that spawn agent processes, support SSH remote execution by checking for sshRemoteConfig, using wrapSpawnWithSsh() from ssh-spawn-wrapper.ts, and passing the SSH store adapter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/cli/services/agent-spawner.test.ts` around lines 702 - 794, Tests for detectOpenCode and detectDroid only cover local PATH detection and cache behavior and miss exercising SSH execution paths; update the tests to also supply an sshRemoteConfig to the detector calls and assert that the code uses wrapSpawnWithSsh by ensuring the spawn path is wrapped and the SSH store adapter is passed into the spawn wrapper. Specifically, in the detectOpenCode and detectDroid test blocks, add cases that call freshDetectOpenCode(overridePath, sshRemoteConfig) and freshDetectDroid(overridePath, sshRemoteConfig) (or otherwise import/construct sshRemoteConfig), stub/wrap wrapSpawnWithSsh from ssh-spawn-wrapper.ts to return a spawn function, and verify the wrapped spawn was invoked so remote-only behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 847-869: The SSH branch currently overwrites spawnCommand with
agentDef?.binaryName ignoring overrides?.customPath; change it to preserve a
provided custom path the same way spawnClaudeAgent and spawnStreamingAgent do by
only falling back to agentDef?.binaryName when !overrides?.customPath (e.g., set
spawnCommand = !overrides?.customPath ? agentDef?.binaryName ??
getCodexCommand() : getCodexCommand(overrides?.customPath)), then pass that
spawnCommand into wrapSpawnWithSsh and continue to set spawnArgs, spawnCwd and
spawnEnv from the sshWrapped result.
In `@src/cli/services/batch-processor.ts`:
- Around line 442-455: The code forwards session.sshRemoteConfig into spawnAgent
via spawnOverrides which enables SSH-backed Auto Run while the processor still
does local document I/O (see the remainingTasks / while (remainingTasks > 0)
loop), causing stuck replay on remote sessions; fix by preventing SSH-backed
runs here: if session.sshRemoteConfig is set, do not include sshRemoteConfig in
spawnOverrides (or skip/throw before calling spawnAgent), or alternatively move
the playbook document read/write/recount logic (the code that computes
remainingTasks and updates playbook docs) into the same remote execution context
before passing sshRemoteConfig to spawnAgent so local files aren’t used for
remote sessions. Ensure the change references spawnOverrides, spawnAgent,
remainingTasks and the while loop so SSH runs are blocked until the document
flow is remote-aware.
---
Outside diff comments:
In `@src/cli/commands/send.ts`:
- Around line 109-170: The SSH config object (agent.sshRemoteConfig) is treated
as a truthy switch but may have enabled: false; normalize it once (e.g., const
ssh = agent.sshRemoteConfig && agent.sshRemoteConfig.enabled ?
agent.sshRemoteConfig : undefined) and use that normalized ssh value for all
calls: pass ssh into detectClaude/detectCodex/detectOpenCode/detectDroid and set
next.sshRemoteConfig = ssh when building overrides, and finally pass the same
ssh-aware overrides into spawnAgent; this ensures SSH mode is only used when
AgentSshRemoteConfig.enabled is true.
---
Nitpick comments:
In `@src/__tests__/cli/services/agent-spawner.test.ts`:
- Around line 702-794: Tests for detectOpenCode and detectDroid only cover local
PATH detection and cache behavior and miss exercising SSH execution paths;
update the tests to also supply an sshRemoteConfig to the detector calls and
assert that the code uses wrapSpawnWithSsh by ensuring the spawn path is wrapped
and the SSH store adapter is passed into the spawn wrapper. Specifically, in the
detectOpenCode and detectDroid test blocks, add cases that call
freshDetectOpenCode(overridePath, sshRemoteConfig) and
freshDetectDroid(overridePath, sshRemoteConfig) (or otherwise import/construct
sshRemoteConfig), stub/wrap wrapSpawnWithSsh from ssh-spawn-wrapper.ts to return
a spawn function, and verify the wrapped spawn was invoked so remote-only
behavior is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 619f4509-dd5b-4779-a5a3-8b55b142e994
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
docs/cli.mdsrc/__tests__/cli/services/agent-spawner.test.tssrc/cli/commands/run-playbook.tssrc/cli/commands/send.tssrc/cli/services/agent-spawner.tssrc/cli/services/batch-processor.tssrc/cli/services/storage.tssrc/main/utils/agent-args.tssrc/shared/types.ts
Contributor
Author
|
@coderabbitai re-review |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/cli/services/agent-spawner.ts (1)
161-179:⚠️ Potential issue | 🟠 MajorDon’t fall back after a bad per-session
customPath.These detector branches are the preflight for the same override later passed into
spawnAgent(). IfcustomPathOverrideis invalid but the binary exists on PATH, detection reportsavailable: true, and the later spawn still uses the bad override and fails withENOENT. Return unavailable for that call instead of continuing to PATH lookup.Also applies to: 217-235, 269-287, 321-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/agent-spawner.ts` around lines 161 - 179, The detector functions (e.g., detectClaude) currently ignore a provided customPathOverride when it is non-executable and continue to report available=true if a PATH binary exists, which causes spawnAgent to later fail with the invalid override; change the logic so that if customPathOverride is provided but is not executable the detector returns available: false (and path/source not set) immediately instead of falling back to PATH detection. Locate similar branches in the other detectors noted (the blocks around lines 217-235, 269-287, 321-339) and apply the same behavior so any invalid per-session customPathOverride causes the detector to return unavailable rather than continuing to probe PATH or report available:true. Ensure spawnAgent still receives the same override behavior (i.e., detectors must not claim availability when a provided override is invalid).src/cli/services/batch-processor.ts (1)
448-469:⚠️ Potential issue | 🟠 MajorEnabled SSH Auto Run is being silently dropped here.
if (!session.sshRemoteConfig?.enabled)omits the override for the only case that should use it, so SSH-configured playbooks just run locally with no warning. Since this loop still reads and writes documents on the local filesystem, it’s safer to fail fast when SSH is enabled than to ignore the session setting.Safer guard
const spawnOverrides: { customPath?: string; customArgs?: string; customEnvVars?: Record<string, string>; customModel?: string; sshRemoteConfig?: AgentSshRemoteConfig; } = { customPath: session.customPath, customArgs: session.customArgs, customEnvVars: session.customEnvVars, customModel: session.customModel, }; - if (!session.sshRemoteConfig?.enabled) { - spawnOverrides.sshRemoteConfig = session.sshRemoteConfig; - } + if (session.sshRemoteConfig?.enabled) { + throw new Error( + 'CLI Auto Run does not support sshRemoteConfig yet because playbook documents are managed on the local filesystem.' + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/services/batch-processor.ts` around lines 448 - 469, The current guard incorrectly negates the SSH check and omits passing session.sshRemoteConfig into spawnOverrides for SSH-enabled sessions; update the condition around spawnOverrides.sshRemoteConfig so that when session.sshRemoteConfig?.enabled is true you assign spawnOverrides.sshRemoteConfig = session.sshRemoteConfig (instead of only assigning when not enabled), and if the loop is not designed to run remotely, add a fail-fast check before calling spawnAgent (e.g., throw or log an error and exit) when session.sshRemoteConfig?.enabled to avoid silently running playbooks locally; reference spawnOverrides, session.sshRemoteConfig, and spawnAgent to locate and fix the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 466-482: The SSH wrapping currently triggers for any
sshRemoteConfig object; change the guard to check enabled by using
overrides?.sshRemoteConfig?.enabled before calling wrapSpawnWithSsh so
disabled-but-present configs do not force remote execution. Update the three
similar blocks (the one using wrapSpawnWithSsh that assigns
spawnCommand/spawnArgs/spawnCwd/spawnEnv and any other occurrences around the
referenced ranges) to gate on sshRemoteConfig.enabled, and keep using
wrapSpawnWithSsh(...) and buildExpandedEnv(...) unchanged inside the new
conditional; also mirror this check in the related detection code paths (e.g.,
detectClaude, detectCodex, detectOpenCode, detectDroid) so all spawn paths
consistently require sshRemoteConfig.enabled.
- Around line 847-849: The spawnCommand selection currently favors
agentDef.binaryName and never consults getCodexCommand(), which ignores the
cached path from detectCodex(); change the logic so you first call
getCodexCommand(overrides?.customPath) and use its result if truthy, otherwise
fall back to agentDef?.binaryName (or a default); update the expression that
defines spawnCommand to prefer getCodexCommand(...) before using
agentDef.binaryName so cached/custom paths are respected.
---
Duplicate comments:
In `@src/cli/services/agent-spawner.ts`:
- Around line 161-179: The detector functions (e.g., detectClaude) currently
ignore a provided customPathOverride when it is non-executable and continue to
report available=true if a PATH binary exists, which causes spawnAgent to later
fail with the invalid override; change the logic so that if customPathOverride
is provided but is not executable the detector returns available: false (and
path/source not set) immediately instead of falling back to PATH detection.
Locate similar branches in the other detectors noted (the blocks around lines
217-235, 269-287, 321-339) and apply the same behavior so any invalid
per-session customPathOverride causes the detector to return unavailable rather
than continuing to probe PATH or report available:true. Ensure spawnAgent still
receives the same override behavior (i.e., detectors must not claim availability
when a provided override is invalid).
In `@src/cli/services/batch-processor.ts`:
- Around line 448-469: The current guard incorrectly negates the SSH check and
omits passing session.sshRemoteConfig into spawnOverrides for SSH-enabled
sessions; update the condition around spawnOverrides.sshRemoteConfig so that
when session.sshRemoteConfig?.enabled is true you assign
spawnOverrides.sshRemoteConfig = session.sshRemoteConfig (instead of only
assigning when not enabled), and if the loop is not designed to run remotely,
add a fail-fast check before calling spawnAgent (e.g., throw or log an error and
exit) when session.sshRemoteConfig?.enabled to avoid silently running playbooks
locally; reference spawnOverrides, session.sshRemoteConfig, and spawnAgent to
locate and fix the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b92e2dc3-dc7c-48d0-ba27-15b7643ea742
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
docs/cli.mdsrc/__tests__/cli/commands/send.test.tssrc/__tests__/cli/services/agent-spawner.test.tssrc/cli/commands/run-playbook.tssrc/cli/commands/send.tssrc/cli/services/agent-spawner.tssrc/cli/services/batch-processor.tssrc/cli/services/storage.tssrc/main/utils/agent-args.tssrc/shared/types.ts
Comment on lines
+466
to
+482
| if (overrides?.sshRemoteConfig) { | ||
| const sshWrapped = await wrapSpawnWithSsh( | ||
| { | ||
| command: spawnCommand, | ||
| args: spawnArgs, | ||
| cwd, | ||
| customEnvVars: effectiveCustomEnvVars, | ||
| agentBinaryName: agentDef?.binaryName, | ||
| }, | ||
| overrides.sshRemoteConfig, | ||
| getCliSshRemoteStore() | ||
| ); | ||
| spawnCommand = sshWrapped.command; | ||
| spawnArgs = sshWrapped.args; | ||
| spawnCwd = sshWrapped.cwd; | ||
| spawnEnv = buildExpandedEnv(sshWrapped.customEnvVars); | ||
| } |
There was a problem hiding this comment.
Only SSH-wrap when the override is actually enabled.
detectClaude(), detectCodex(), detectOpenCode(), and detectDroid() all treat SSH as active only when sshRemoteConfig.enabled is true, but these spawn paths wrap on any sshRemoteConfig object. A persisted-but-disabled config will therefore pass local preflight and still be launched remotely. Gate these branches on overrides?.sshRemoteConfig?.enabled.
Based on learnings, When implementing features that spawn agent processes, support SSH remote execution by checking for sshRemoteConfig, using wrapSpawnWithSsh() from ssh-spawn-wrapper.ts, and passing the SSH store adapter.
Also applies to: 652-668, 854-870
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/services/agent-spawner.ts` around lines 466 - 482, The SSH wrapping
currently triggers for any sshRemoteConfig object; change the guard to check
enabled by using overrides?.sshRemoteConfig?.enabled before calling
wrapSpawnWithSsh so disabled-but-present configs do not force remote execution.
Update the three similar blocks (the one using wrapSpawnWithSsh that assigns
spawnCommand/spawnArgs/spawnCwd/spawnEnv and any other occurrences around the
referenced ranges) to gate on sshRemoteConfig.enabled, and keep using
wrapSpawnWithSsh(...) and buildExpandedEnv(...) unchanged inside the new
conditional; also mirror this check in the related detection code paths (e.g.,
detectClaude, detectCodex, detectOpenCode, detectDroid) so all spawn paths
consistently require sshRemoteConfig.enabled.
Comment on lines
+847
to
+849
| let spawnCommand = !overrides?.customPath | ||
| ? agentDef?.binaryName || getCodexCommand() | ||
| : getCodexCommand(overrides?.customPath); |
There was a problem hiding this comment.
Local Codex runs ignore the detected/custom binary path.
When overrides?.customPath is absent, this always selects agentDef.binaryName (codex) and never consults getCodexCommand(). That bypasses the cached path populated by detectCodex(), so installations that rely on the stored custom path pass preflight and then fail at spawn.
Suggested fix
- let spawnCommand = !overrides?.customPath
- ? agentDef?.binaryName || getCodexCommand()
- : getCodexCommand(overrides?.customPath);
+ let spawnCommand =
+ overrides?.sshRemoteConfig && !overrides?.customPath && agentDef?.binaryName
+ ? agentDef.binaryName
+ : getCodexCommand(overrides?.customPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/services/agent-spawner.ts` around lines 847 - 849, The spawnCommand
selection currently favors agentDef.binaryName and never consults
getCodexCommand(), which ignores the cached path from detectCodex(); change the
logic so you first call getCodexCommand(overrides?.customPath) and use its
result if truthy, otherwise fall back to agentDef?.binaryName (or a default);
update the expression that defines spawnCommand to prefer getCodexCommand(...)
before using agentDef.binaryName so cached/custom paths are respected.
Contributor
Author
|
Escalated through reviews, ne PR for minimal changes: #524 |
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
1 participant
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.
This pull request extends CLI agent support by adding compatibility for two new agent types,
opencodeandfactory-droid, alongside existing ones. It updates error handling, agent detection logic, and configuration management to accommodate these new agents. The changes also ensure that custom agent configuration values are passed through all relevant code paths.Agent support and error handling:
opencodeandfactory-droidagent types in both therun-playbookandsendCLI commands, including detection logic and error handling for missing CLIs. [1] [2] [3] [4]docs/cli.mdto includeOPENCODE_NOT_FOUNDandDROID_NOT_FOUND.Agent configuration management:
customPath,customArgs,customEnvVars,customModel) are passed to agent detection and spawning functions throughout the CLI codebase. [1] [2] [3] [4] [5]getAgentConfigValuesinsrc/cli/services/storage.tsto retrieve stored config values for agents.SessionInfointerface to include new fields for custom agent configuration.Summary by CodeRabbit