feat(cli): add --visible flag to goal-run for desktop-owned Auto Runs - #1287
feat(cli): add --visible flag to goal-run for desktop-owned Auto Runs#1287pedramamini wants to merge 1 commit into
Conversation
`maestro-cli goal-run --visible <agent> "<goal>"` hands a Goal-Driven Auto
Run to the running desktop app so it executes as a live, desktop-owned Auto
Run in the same UI surface as the Go/Spec buttons, instead of running
headless in the CLI process. This lets a root/orchestrator agent launch a
monitored, UI-visible free-text Goal worker.
The visible path threads a new `launch_goal_run` message through the existing
CLI -> desktop WebSocket bridge (the same four-layer path used by
`dispatch --new-tab` and `auto-run --launch`) and terminates in the renderer
at the exact `startBatchRun({ goalConfig })` entry point the UI Go button
already uses. No new run engine: the desktop owns spawning, busy arbitration,
stop, and session-list visibility.
Behavior:
- Backward compatible: headless `goal-run` stays the default; `--visible` is
purely additive.
- Fails closed: `--visible` errors with MAESTRO_NOT_RUNNING when the desktop
app is unreachable instead of silently falling back to headless.
- Deterministic busy handling: a busy agent returns a clear AGENT_BUSY error.
- With `--json`, returns stable identifiers (ok, mode, visible, agent_id,
session_id, tab_id, status) plus a `maestro://session/<id>/tab/<id>` URI.
Layers touched: CLI command + registration, web-server message handler,
callback registry/types, main->renderer IPC bridge, preload, and the renderer
remote event listener. Adds CLI unit tests for the visible path.
📝 WalkthroughWalkthroughAdds ChangesVisible Goal Run
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant WebServer
participant Renderer
participant AutoRun
CLI->>WebServer: launch_goal_run(sessionId, goal config)
WebServer->>Renderer: remote:launchGoalRun(responseChannel)
Renderer->>Renderer: Validate session, agent state, goal, and AI tab
Renderer->>CLI: success, tabId
Renderer->>AutoRun: startBatchRun(goalConfig)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
Greptile SummaryThis PR adds desktop-visible Goal-Driven Auto Runs to the CLI. The main changes are:
Confidence Score: 4/5Visible launches can report success without starting and can overlap on the same agent.
src/renderer/hooks/remote/useAppRemoteEventListeners.ts Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as maestro-cli
participant WS as WebSocket server
participant Main as Electron main
participant Renderer as Renderer
participant Runner as Goal runner
CLI->>WS: launch_goal_run
WS->>Main: launchGoalRun(sessionId, config)
Main->>Renderer: remote:launchGoalRun
Renderer->>Renderer: Check session.state
Renderer-->>Main: success: true
Main-->>WS: launch_goal_run_result
WS-->>CLI: status: launched
Renderer->>Runner: startBatchRun(goalConfig)
Runner--xRenderer: Initialization may reject
Reviews (1): Last reviewed commit: "feat(cli): add --visible flag to goal-ru..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Ack before starting: startBatchRun is long-running and would exceed the | ||
| // IPC/CLI timeout if awaited (same pattern as maestro:configureAutoRun). | ||
| window.maestro.process.sendRemoteLaunchGoalRunResponse(responseChannel, { | ||
| success: true, | ||
| ...(tabId ? { tabId } : {}), | ||
| }); | ||
|
|
||
| // Goal mode is document-less; folderPath is only stored in batch state. | ||
| startBatchRun(sessionId, batchConfig, session.autoRunFolderPath || '').catch((err) => { |
There was a problem hiding this comment.
The response reports success: true before startBatchRun begins. If goal initialization then rejects, the catch only logs the error, so the CLI exits successfully with status: "launched" even though no run started.
| window.maestro.process.sendRemoteLaunchGoalRunResponse(responseChannel, { | ||
| success: false, | ||
| error: `Agent "${session.name || session.id}" is busy`, | ||
| }); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Two rapid requests can both observe an idle session.state because this check does not reserve the agent and goal startup does not immediately set that state to busy. Both requests can therefore report success and start overlapping goal loops for the same agent.
There was a problem hiding this comment.
Pull request overview
Adds a --visible mode to maestro-cli goal-run that routes the request over the existing CLI to desktop WebSocket bridge so the desktop app runs the Goal-Driven Auto Run in the same Auto Run UI surface as the Go/Spec buttons, and returns identifiers (including a maestro:// deep link) back to the CLI.
Changes:
- Add
--visibleflag and a CLI implementation that sends alaunch_goal_runcommand to the running desktop app and emits JSON/human output. - Thread a new
launch_goal_runmessage and callback through the web server, main to renderer IPC bridge, and preload API. - Add renderer-side remote listeners that translate the IPC event into
startBatchRun({ goalConfig }), plus unit tests for the visible CLI path.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli/index.ts | Adds the --visible CLI flag to goal-run. |
| src/cli/commands/goal-run.ts | Implements runVisibleGoalRun() and fail-closed behavior for desktop-owned visible runs. |
| src/main/web-server/handlers/messageHandlers.ts | Adds launch_goal_run WebSocket message handler and result emission. |
| src/main/web-server/managers/CallbackRegistry.ts | Adds launchGoalRun callback registration and invocation plumbing. |
| src/main/web-server/types.ts | Introduces LaunchGoalRunCallback and result typing. |
| src/main/web-server/WebServer.ts | Exposes setLaunchGoalRunCallback() and wires launchGoalRun into handler callbacks. |
| src/main/web-server/web-server-factory.ts | Bridges launchGoalRun from web server callbacks to main to renderer IPC (remote:launchGoalRun). |
| src/main/preload/process.ts | Adds onRemoteLaunchGoalRun subscription and sendRemoteLaunchGoalRunResponse. |
| src/renderer/global.d.ts | Extends the renderer process API typing for the new remote goal-run bridge. |
| src/renderer/hooks/remote/useRemoteIntegration.ts | Subscribes to onRemoteLaunchGoalRun and dispatches a maestro:launchGoalRun CustomEvent. |
| src/renderer/hooks/remote/useAppRemoteEventListeners.ts | Handles maestro:launchGoalRun by building goalConfig and calling startBatchRun. |
| src/tests/cli/commands/goalRunVisible.test.ts | Adds test coverage for the CLI visible goal-run path and output contract. |
| src/tests/renderer/hooks/useRemoteIntegration.test.ts | Updates renderer hook mocks for the new preload bridge methods. |
| src/tests/main/web-server/web-server-factory.test.ts | Updates web server factory mocks to include the new callback wiring. |
Comments suppressed due to low confidence (1)
src/main/web-server/handlers/messageHandlers.ts:1712
- maxIterations validation claims to require a positive integer, but the current logic accepts non-integers (for example 2.5) and silently floors them. Reject non-integer values to match the stated contract and avoid surprising behavior.
const n = Number(message.maxIterations);
if (!Number.isFinite(n) || n < 1) {
sendErrorResult('maxIterations must be a positive integer or null');
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private handleLaunchGoalRun(client: WebClient, message: WebClientMessage): void { | ||
| const sessionId = typeof message.sessionId === 'string' ? message.sessionId : ''; | ||
| const goal = typeof message.goal === 'string' ? message.goal.trim() : ''; | ||
| // Goals can contain user-authored content with secrets or PII — log length only. |
| /** | ||
| * Substring signature of MaestroClient / socket-level failures that mean the | ||
| * desktop app is down or unreachable (as opposed to a command it rejected). | ||
| * Mirrors the mapping in `dispatch.ts` so `--visible` can fail closed with a | ||
| * dedicated code instead of a generic error. | ||
| */ | ||
| function isMaestroUnreachable(message: string): boolean { | ||
| const lower = message.toLowerCase(); |
| // Focus the agent so the run is immediately visible, matching the UI | ||
| // Go button (which runs on the focused agent). | ||
| setActiveSessionId(sessionId); | ||
|
|
||
| // Ack before starting: startBatchRun is long-running and would exceed the | ||
| // IPC/CLI timeout if awaited (same pattern as maestro:configureAutoRun). | ||
| window.maestro.process.sendRemoteLaunchGoalRunResponse(responseChannel, { | ||
| success: true, | ||
| ...(tabId ? { tabId } : {}), | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/__tests__/cli/commands/goalRunVisible.test.ts (1)
1-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the JSON success/error paths; missing symmetric human-readable error-path tests.
All error-path tests (
MAESTRO_NOT_RUNNING,AGENT_BUSY,VISIBLE_LAUNCH_REJECTED) only assert JSON output. There's no test assertingconsole.error/formatErroroutput for these same failures whenuseJsonisfalse, even though a human-readable success case is covered. Consider adding one such case to close the gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/cli/commands/goalRunVisible.test.ts` around lines 1 - 130, The error-path coverage for runVisibleGoalRun only verifies JSON output, leaving the non-JSON behavior untested. Add a human-readable failure test using useJson=false, preferably for a representative rejection such as AGENT_BUSY, and assert console.error receives the formatted error while console.log is not used for the failure output; preserve the existing JSON-path tests.src/cli/commands/goal-run.ts (1)
29-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated desktop-unreachable detection logic.
The comment itself admits this function is a substring signature of MaestroClient / socket-level failures that mean the desktop app is down or unreachable, and states it "Mirrors the mapping in
dispatch.ts." Maintaining two copies of this string-matching heuristic risks silent drift (e.g. a new failure mode added to one file but not the other), which would break the fail-closed guarantee--visiblerelies on. Extract this into a shared helper (e.g.src/cli/services/maestro-errors.ts) and import it from bothgoal-run.tsanddispatch.ts.As per path instructions: "Before creating a new utility, helper, hook, component, type, or constant, check the relevant guide in
docs/agent-guides/and reuse or extend the canonical implementation instead of duplicating it."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/commands/goal-run.ts` around lines 29 - 48, Consolidate the duplicated Maestro desktop-unreachable detection into one shared helper, first checking the applicable guide in docs/agent-guides/ for the canonical location or implementation. Move or extend the logic represented by isMaestroUnreachable and update both goal-run.ts and dispatch.ts to import and reuse it, preserving the existing failure classifications and --visible fail-closed behavior.Source: Path instructions
src/renderer/hooks/remote/useAppRemoteEventListeners.ts (1)
584-663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo visible test coverage for this handler's own logic.
This handler is the actual execution owner for visible goal runs (busy/connecting short-circuit, empty-goal validation, tab-id resolution for the deep link, ack-before-start ordering), but the only tests provided for the visible-run flow are CLI-level (
goalRunVisible.test.ts, which mocks the transport and never exercises this code) anduseRemoteIntegration.test.ts(which only verifies the forwarding effect, not this handler). Consider adding unit tests here for: session-not-found, busy/connecting rejection, empty-goal rejection, tabId fallback to the first AI tab, and the success ack shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/hooks/remote/useAppRemoteEventListeners.ts` around lines 584 - 663, Add unit coverage for the maestro:launchGoalRun handler in useAppRemoteEventListeners, exercising session-not-found, busy/connecting rejection, and empty-goal validation paths. Also verify tabId resolution falls back to the first AI tab and that the success response acknowledges before starting with the expected response shape. Mock session state, event details, and process methods so tests target this handler rather than CLI transport or forwarding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/commands/goal-run.ts`:
- Around line 200-215: Move the getAgentDefinition(agent.toolType) and
AGENT_UNSUPPORTED validation to after the options.visible early-return in the
goal-run command, or guard it so visible runs bypass it. Ensure
runVisibleGoalRun executes for any toolType supported by the desktop app, while
retaining the existing validation for headless execution.
In `@src/main/preload/process.ts`:
- Around line 900-913: Update the callback failure handling around
Promise.resolve and the surrounding try/catch to send the fallback response,
then rethrow the original error so Sentry can capture unexpected renderer launch
failures. Preserve the existing error-to-message conversion and response channel
behavior in both asynchronous and synchronous failure paths.
In `@src/main/web-server/handlers/messageHandlers.ts`:
- Around line 1707-1714: Update the maxIterations validation in the message
handler to reject non-integer numeric values, not just non-finite or values
below 1. Preserve acceptance of positive integers and the existing
null/undefined behavior, and avoid silently coercing fractional inputs via
Math.floor.
- Around line 1735-1736: Update the rejection handler in the goal-run launch
flow to normalize the caught value into an Error before accessing its message,
including safe handling for null and other non-Error rejections. Use the
normalized error when reporting through sendErrorResult, and ensure unexpected
exceptions are still surfaced rather than silently swallowed.
---
Nitpick comments:
In `@src/__tests__/cli/commands/goalRunVisible.test.ts`:
- Around line 1-130: The error-path coverage for runVisibleGoalRun only verifies
JSON output, leaving the non-JSON behavior untested. Add a human-readable
failure test using useJson=false, preferably for a representative rejection such
as AGENT_BUSY, and assert console.error receives the formatted error while
console.log is not used for the failure output; preserve the existing JSON-path
tests.
In `@src/cli/commands/goal-run.ts`:
- Around line 29-48: Consolidate the duplicated Maestro desktop-unreachable
detection into one shared helper, first checking the applicable guide in
docs/agent-guides/ for the canonical location or implementation. Move or extend
the logic represented by isMaestroUnreachable and update both goal-run.ts and
dispatch.ts to import and reuse it, preserving the existing failure
classifications and --visible fail-closed behavior.
In `@src/renderer/hooks/remote/useAppRemoteEventListeners.ts`:
- Around line 584-663: Add unit coverage for the maestro:launchGoalRun handler
in useAppRemoteEventListeners, exercising session-not-found, busy/connecting
rejection, and empty-goal validation paths. Also verify tabId resolution falls
back to the first AI tab and that the success response acknowledges before
starting with the expected response shape. Mock session state, event details,
and process methods so tests target this handler rather than CLI transport or
forwarding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81d660a0-ab92-4947-8462-e50257e16fdb
📒 Files selected for processing (14)
src/__tests__/cli/commands/goalRunVisible.test.tssrc/__tests__/main/web-server/web-server-factory.test.tssrc/__tests__/renderer/hooks/useRemoteIntegration.test.tssrc/cli/commands/goal-run.tssrc/cli/index.tssrc/main/preload/process.tssrc/main/web-server/WebServer.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/managers/CallbackRegistry.tssrc/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/renderer/global.d.tssrc/renderer/hooks/remote/useAppRemoteEventListeners.tssrc/renderer/hooks/remote/useRemoteIntegration.ts
| // --visible hands the run to the desktop app, which owns spawning (using the | ||
| // agent's configured binary, SSH, etc.) and busy arbitration. Skip the | ||
| // headless-only preflight below (detectAgent / checkAgentBusy / runGoal). | ||
| if (options.visible) { | ||
| await runVisibleGoalRun( | ||
| agent.id, | ||
| { | ||
| goal: trimmedGoal, | ||
| exitCriteria: options.exitCriteria?.trim() ?? '', | ||
| maxIterations: parseMaxIterations(options.maxIterations, useJson), | ||
| }, | ||
| useJson | ||
| ); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--visible is still gated by the CLI's own agent-support registry, contradicting the "skip headless-only preflight" intent.
The comment says this branch exists to hand the run to the desktop app, which owns spawning (using the agent's configured binary, SSH, etc.) and busy arbitration, skipping the headless-only preflight below (detectAgent / checkAgentBusy / runGoal). However, the getAgentDefinition(agent.toolType) / AGENT_UNSUPPORTED check (lines 188-198) runs unconditionally before this branch, not "below" it. So a visible launch for any toolType the CLI's local registry doesn't recognize — even one the desktop app fully supports for spawning — will be rejected with AGENT_UNSUPPORTED before runVisibleGoalRun is ever called. This undercuts the PR's core premise that the desktop app owns agent support for visible runs.
Move the def/AGENT_UNSUPPORTED check after the options.visible early return (or skip it entirely when options.visible is set).
🐛 Proposed fix
- // Agent CLI must be supported and installed.
- const def = getAgentDefinition(agent.toolType);
- if (!def) {
- const message = `Agent type "${agent.toolType}" is not supported in CLI batch mode yet.`;
- if (useJson) {
- emitError(message, 'AGENT_UNSUPPORTED');
- } else {
- console.error(formatError(message));
- }
- process.exit(1);
- }
-
// --visible hands the run to the desktop app, which owns spawning (using the
// agent's configured binary, SSH, etc.) and busy arbitration. Skip the
// headless-only preflight below (detectAgent / checkAgentBusy / runGoal).
if (options.visible) {
await runVisibleGoalRun(
agent.id,
{
goal: trimmedGoal,
exitCriteria: options.exitCriteria?.trim() ?? '',
maxIterations: parseMaxIterations(options.maxIterations, useJson),
},
useJson
);
return;
}
+
+ // Agent CLI must be supported and installed (headless path only — visible
+ // runs are owned by the desktop app, which has its own agent support).
+ const def = getAgentDefinition(agent.toolType);
+ if (!def) {
+ const message = `Agent type "${agent.toolType}" is not supported in CLI batch mode yet.`;
+ if (useJson) {
+ emitError(message, 'AGENT_UNSUPPORTED');
+ } else {
+ console.error(formatError(message));
+ }
+ process.exit(1);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // --visible hands the run to the desktop app, which owns spawning (using the | |
| // agent's configured binary, SSH, etc.) and busy arbitration. Skip the | |
| // headless-only preflight below (detectAgent / checkAgentBusy / runGoal). | |
| if (options.visible) { | |
| await runVisibleGoalRun( | |
| agent.id, | |
| { | |
| goal: trimmedGoal, | |
| exitCriteria: options.exitCriteria?.trim() ?? '', | |
| maxIterations: parseMaxIterations(options.maxIterations, useJson), | |
| }, | |
| useJson | |
| ); | |
| return; | |
| } | |
| // --visible hands the run to the desktop app, which owns spawning (using the | |
| // agent's configured binary, SSH, etc.) and busy arbitration. Skip the | |
| // headless-only preflight below (detectAgent / checkAgentBusy / runGoal). | |
| if (options.visible) { | |
| await runVisibleGoalRun( | |
| agent.id, | |
| { | |
| goal: trimmedGoal, | |
| exitCriteria: options.exitCriteria?.trim() ?? '', | |
| maxIterations: parseMaxIterations(options.maxIterations, useJson), | |
| }, | |
| useJson | |
| ); | |
| return; | |
| } | |
| // Agent CLI must be supported and installed (headless path only - visible | |
| // runs are owned by the desktop app, which has its own agent support). | |
| const def = getAgentDefinition(agent.toolType); | |
| if (!def) { | |
| const message = `Agent type "${agent.toolType}" is not supported in CLI batch mode yet.`; | |
| if (useJson) { | |
| emitError(message, 'AGENT_UNSUPPORTED'); | |
| } else { | |
| console.error(formatError(message)); | |
| } | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/commands/goal-run.ts` around lines 200 - 215, Move the
getAgentDefinition(agent.toolType) and AGENT_UNSUPPORTED validation to after the
options.visible early-return in the goal-run command, or guard it so visible
runs bypass it. Ensure runVisibleGoalRun executes for any toolType supported by
the desktop app, while retaining the existing validation for headless execution.
| try { | ||
| // callback may return a promise even though typed as void | ||
| Promise.resolve(callback(sessionId, config, responseChannel)).catch((error) => { | ||
| ipcRenderer.send(responseChannel, { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| }); | ||
| } catch (error) { | ||
| ipcRenderer.send(responseChannel, { | ||
| success: false, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Rethrow callback failures after acknowledging them.
These paths send the fallback response but consume unexpected exceptions, preventing Sentry from seeing renderer launch failures. Send the response, then rethrow.
Proposed fix
Promise.resolve(callback(sessionId, config, responseChannel)).catch((error) => {
ipcRenderer.send(responseChannel, {
success: false,
error: error instanceof Error ? error.message : String(error),
});
+ throw error;
});
} catch (error) {
ipcRenderer.send(responseChannel, {
success: false,
error: error instanceof Error ? error.message : String(error),
});
+ throw error;
}As per coding guidelines, "Do not silently swallow unexpected exceptions. Handle known recoverable errors explicitly, rethrow unexpected errors so Sentry can capture them."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // callback may return a promise even though typed as void | |
| Promise.resolve(callback(sessionId, config, responseChannel)).catch((error) => { | |
| ipcRenderer.send(responseChannel, { | |
| success: false, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| }); | |
| } catch (error) { | |
| ipcRenderer.send(responseChannel, { | |
| success: false, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| try { | |
| // callback may return a promise even though typed as void | |
| Promise.resolve(callback(sessionId, config, responseChannel)).catch((error) => { | |
| ipcRenderer.send(responseChannel, { | |
| success: false, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| throw error; | |
| }); | |
| } catch (error) { | |
| ipcRenderer.send(responseChannel, { | |
| success: false, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| throw error; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/preload/process.ts` around lines 900 - 913, Update the callback
failure handling around Promise.resolve and the surrounding try/catch to send
the fallback response, then rethrow the original error so Sentry can capture
unexpected renderer launch failures. Preserve the existing error-to-message
conversion and response channel behavior in both asynchronous and synchronous
failure paths.
Source: Coding guidelines
| if (message.maxIterations !== undefined && message.maxIterations !== null) { | ||
| const n = Number(message.maxIterations); | ||
| if (!Number.isFinite(n) || n < 1) { | ||
| sendErrorResult('maxIterations must be a positive integer or null'); | ||
| return; | ||
| } | ||
| maxIterations = Math.floor(n); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject fractional iteration limits.
1.5 passes validation and is silently changed to 1, despite the public positive-integer contract.
Proposed fix
- if (!Number.isFinite(n) || n < 1) {
+ if (!Number.isFinite(n) || !Number.isInteger(n) || n < 1) {
sendErrorResult('maxIterations must be a positive integer or null');
return;
}
- maxIterations = Math.floor(n);
+ maxIterations = n;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (message.maxIterations !== undefined && message.maxIterations !== null) { | |
| const n = Number(message.maxIterations); | |
| if (!Number.isFinite(n) || n < 1) { | |
| sendErrorResult('maxIterations must be a positive integer or null'); | |
| return; | |
| } | |
| maxIterations = Math.floor(n); | |
| } | |
| if (message.maxIterations !== undefined && message.maxIterations !== null) { | |
| const n = Number(message.maxIterations); | |
| if (!Number.isFinite(n) || !Number.isInteger(n) || n < 1) { | |
| sendErrorResult('maxIterations must be a positive integer or null'); | |
| return; | |
| } | |
| maxIterations = n; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers.ts` around lines 1707 - 1714,
Update the maxIterations validation in the message handler to reject non-integer
numeric values, not just non-finite or values below 1. Preserve acceptance of
positive integers and the existing null/undefined behavior, and avoid silently
coercing fractional inputs via Math.floor.
| .catch((error) => { | ||
| sendErrorResult(`Failed to launch goal run: ${error.message}`); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Report and normalize bridge failures.
A non-Error rejection such as null makes error.message throw while handling the failure, so the CLI receives no typed result. Normalize and capture the error before replying.
Proposed fix
- .catch((error) => {
- sendErrorResult(`Failed to launch goal run: ${error.message}`);
+ .catch((error: unknown) => {
+ const err = error instanceof Error ? error : new Error(String(error));
+ captureException(err, {
+ extra: {
+ area: 'web-server',
+ handler: 'launch_goal_run',
+ sessionId,
+ requestId: message.requestId,
+ },
+ });
+ sendErrorResult(`Failed to launch goal run: ${err.message}`);
});As per coding guidelines, "Do not silently swallow unexpected exceptions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .catch((error) => { | |
| sendErrorResult(`Failed to launch goal run: ${error.message}`); | |
| .catch((error: unknown) => { | |
| const err = error instanceof Error ? error : new Error(String(error)); | |
| captureException(err, { | |
| extra: { | |
| area: 'web-server', | |
| handler: 'launch_goal_run', | |
| sessionId, | |
| requestId: message.requestId, | |
| }, | |
| }); | |
| sendErrorResult(`Failed to launch goal run: ${err.message}`); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/web-server/handlers/messageHandlers.ts` around lines 1735 - 1736,
Update the rejection handler in the goal-run launch flow to normalize the caught
value into an Error before accessing its message, including safe handling for
null and other non-Error rejections. Use the normalized error when reporting
through sendErrorResult, and ensure unexpected exceptions are still surfaced
rather than silently swallowed.
Source: Coding guidelines
Closes #1286
What
Adds
--visibletomaestro-cli goal-run:With
--visible, the Goal-Driven Auto Run is handed to the running desktop app so it executes as a live, desktop-owned Auto Run in the same UI surface as the Go/Spec buttons, instead of running headless in the CLI process. This lets a root/orchestrator agent launch a monitored, UI-visible free-text Goal worker without handing control back to the user.How
The visible path threads a new
launch_goal_runmessage through the existing CLI → desktop WebSocket bridge (the same four-layer pathdispatch --new-tabandauto-run --launchalready use) and terminates in the renderer at the exactstartBatchRun({ goalConfig })entry point the UI Go button uses. There is no new run engine: the desktop owns spawning, busy arbitration,stop-auto-run, andsession listvisibility, so those requirements come for free.Layers touched (all additive):
--visibleflag +runVisibleGoalRun()ingoal-run.ts, registration inindex.tslaunch_goal_runmessage handler, callback registry/types, WebServer wiringremote:launchGoalRunIPC bridge inweb-server-factory.ts, preload subscriptionmaestro:launchGoalRunremote event listener that builds agoalConfigand callsstartBatchRunBehavior / contract
goal-runstays the default;--visibleis purely additive (no deletions).--visibleerrors withMAESTRO_NOT_RUNNINGwhen the desktop app is unreachable rather than silently falling back to headless.AGENT_BUSYerror.--jsonreturns stable identifiers plus a deep link:{ "ok": true, "mode": "visible", "visible": true, "agent_id": "...", "session_id": "...", "tab_id": "...", "status": "launched", "uri": "maestro://session/<id>/tab/<id>" }Scope notes / open questions for the author
The issue's "recommended" API also listed
--new-tab/--tab/--focus/--no-focus/--waitand arun_id. This PR intentionally implements the core UI-parity MVP the issue calls the key requirement, and leaves those out for now because:--new-tab/--tabdon't map cleanly onto the existing engine. The run attaches to the agent and surfaces on its active AI tab, which is what we return astab_id.run_id/ providersession_idaren't known synchronously at launch (the provider session is assigned after the first agent turn), so they're omitted rather than faked.--wait.Happy to extend any of these if you'd prefer the fuller surface.
Tests
src/__tests__/cli/commands/goalRunVisible.test.tscovers the visible path: message shape, JSON identifiers + deep link, fail-closed (MAESTRO_NOT_RUNNING),AGENT_BUSY, generic rejection, and human-readable output.npm run lint(all tsconfigs) and the CLI + web-server + goalDriven + remote-integration suites pass locally.Summary by CodeRabbit
New Features
goal-run --visibleto launch Goal-Driven Auto Runs in the desktop interface.Tests