Open
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:
📝 WalkthroughWalkthroughThis pull request adds comprehensive support for GitHub Copilot CLI as a new beta agent to the application. The implementation includes Copilot-specific CLI argument handling, JSON output parsing, session storage with remote SSH support, image prompt embedding, error pattern matching, and full UI/renderer integration including wizard support and command discovery. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Terminal
participant Spawner as ChildProcessSpawner
participant Copilot as Copilot CLI
participant StdoutHandler as StdoutHandler
participant Parser as CopilotOutputParser
participant Storage as CopilotSessionStorage
Client->>Spawner: spawn(config with prompt, imagePromptBuilder)
Spawner->>Spawner: Check if imagePromptBuilder provided
alt Image + Prompt Present
Spawner->>Spawner: Build image temp files
Spawner->>Spawner: Call imagePromptBuilder(imagePaths)
Spawner->>Spawner: Prepend image prompt to original prompt
Spawner->>Copilot: Invoke with -p "<image_prompt + text>"
else No Image Embedding
Spawner->>Copilot: Invoke with standard args + --output-format json
end
Copilot->>Copilot: Process with streaming JSON-line output
Copilot->>StdoutHandler: Stream concatenated JSON objects (newline-less)
StdoutHandler->>StdoutHandler: Buffer partial JSON until complete object
alt Buffer Overflow
StdoutHandler->>StdoutHandler: Mark jsonBufferCorrupted, discard, resync
end
StdoutHandler->>StdoutHandler: Split concatenated objects, extract remainder
StdoutHandler->>Parser: processLine(jsonLineString)
Parser->>Parser: Deserialize JSON to CopilotRawMessage
alt Message Type
Parser->>Parser: assistant.message (phase: final_answer) → result event
Parser->>Parser: assistant.message_delta → partial text event
Parser->>Parser: assistant.reasoning_delta → partial reasoning event
Parser->>Parser: tool.execution_start → tool_use (running)
Parser->>Parser: tool.execution_complete → tool_use (completed/failed)
Parser->>Parser: session.shutdown → usage event
end
Parser->>StdoutHandler: Emit ParsedEvent (text/tool_use/result/usage)
StdoutHandler->>StdoutHandler: Dedup tool calls by toolCallId, extract sessionId
StdoutHandler->>Client: Emit structured event to listeners
alt On Result Event
StdoutHandler->>Storage: Store session ID from result
Storage->>Storage: Map to ~/.copilot/session-state/<sessionId>/
Storage->>Storage: Read workspace.yaml (project metadata)
Storage->>Storage: Parse events.jsonl (tool requests, token counts)
Client->>Storage: listSessions(projectPath) for history
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
105-110:⚠️ Potential issue | 🟡 MinorSupported-agent statements are now inconsistent in this README.
After adding Copilot in Line 109, the earlier support lists at Line 15 and Line 86 still omit it. That creates contradictory guidance for new users.
📝 Suggested doc sync
-Run multiple agents in parallel with a Linear/Superhuman-level responsive interface. Currently supporting **Claude Code**, **OpenAI Codex**, **OpenCode**, and **Factory Droid** with plans for additional agentic coding tools (Gemini CLI, Qwen3 Coder) based on user demand. +Run multiple agents in parallel with a Linear/Superhuman-level responsive interface. Currently supporting **Claude Code**, **OpenAI Codex**, **OpenCode**, **Factory Droid**, and **GitHub Copilot CLI (Beta)**, with plans for additional agentic coding tools (Gemini CLI, Qwen3 Coder) based on user demand. -> **Note**: Maestro supports Claude Code, OpenAI Codex, OpenCode, and Factory Droid. Support for additional agents (Gemini CLI, Qwen3 Coder) may be added in future releases based on community demand. +> **Note**: Maestro supports Claude Code, OpenAI Codex, OpenCode, Factory Droid, and GitHub Copilot CLI (Beta). Support for additional agents (Gemini CLI, Qwen3 Coder) may be added in future releases based on community demand.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 105 - 110, The README has inconsistent "supported agent" lists—update all occurrences of the supported AI coding agents so they match and include "GitHub Copilot CLI" alongside "Claude Code", "OpenAI Codex", and "OpenCode"; search for the agent bullet lists (the "At least one supported AI coding agent installed and authenticated" section and any other prerequisite/overview lists) and add the "GitHub Copilot CLI" entry with the same link text/format as used in the newly added item to keep wording and links consistent across the README.src/main/parsers/error-patterns.ts (1)
858-958:⚠️ Potential issue | 🟠 MajorMirror Copilot into
SSH_ERROR_PATTERNStoo.Registering Copilot here leaves the SSH-specific fallback table above unchanged. On SSH-backed Copilot sessions,
bash: copilot: command not foundand/path/copilot: No such file or directorywill miss the friendly install diagnostic and collapse to the generic crash path.Suggested follow-up in
SSH_ERROR_PATTERNS.agent_crashed+ { + pattern: + /bash:.*copilot.*command not found|sh:.*copilot.*command not found|zsh:.*command not found:.*copilot/i, + message: 'GitHub Copilot command not found. Ensure Copilot CLI is installed.', + recoverable: false, + }, { // Agent binary missing (executable file not found at path) // More specific pattern: requires path-like structure before the binary name // Matches: "/usr/local/bin/claude: No such file or directory" // Does NOT match: "claude: error: File 'foo.txt': No such file or directory" (normal file errors) - pattern: /\/[^\s:]*\/(claude|opencode|codex):\s*No such file or directory/i, + pattern: /\/[^\s:]*\/(claude|opencode|codex|copilot):\s*No such file or directory/i, message: 'Agent binary not found at the specified path. Ensure the agent is installed.', recoverable: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/error-patterns.ts` around lines 858 - 958, The Copilot-specific error patterns (COPILOT_ERROR_PATTERNS) were registered only in patternRegistry and not mirrored into SSH_ERROR_PATTERNS, so SSH-backed Copilot sessions miss Copilot-specific install/crash diagnostics; update SSH_ERROR_PATTERNS.agent_crashed to include the same Copilot entries (or merge/copied relevant patterns from COPILOT_ERROR_PATTERNS.agent_crashed such as the "no prompt provided.*interactive terminal" and install-related patterns) and/or add a registration step that maps 'copilot' patterns into the SSH_ERROR_PATTERNS set, ensuring patternRegistry and SSH_ERROR_PATTERNS both include the Copilot patterns so messages like "bash: copilot: command not found" match the friendly install diagnostic.
🧹 Nitpick comments (6)
src/__tests__/main/utils/remote-fs.test.ts (1)
171-183: Good test coverage for the home-relative path expansion feature.The test correctly verifies the primary use case:
~/...paths are expanded to"$HOME/..."in SSH commands. This aligns well with the Copilot session storage path handling.Consider adding additional test cases for comprehensive coverage of
shellEscapeRemotePath:🧪 Optional: additional edge case tests
it('expands bare ~ to $HOME', async () => { const deps = createMockDeps({ stdout: 'file.txt\n', stderr: '', exitCode: 0, }); await readDirRemote('~', baseConfig, deps); const call = (deps.execSsh as any).mock.calls[0][1]; const remoteCommand = call[call.length - 1]; expect(remoteCommand).toContain('"$HOME"'); }); it('escapes special characters in home-relative paths', async () => { const deps = createMockDeps({ stdout: 'file.txt\n', stderr: '', exitCode: 0, }); await readDirRemote('~/path$with"special', baseConfig, deps); const call = (deps.execSsh as any).mock.calls[0][1]; const remoteCommand = call[call.length - 1]; // Should contain escaped special chars within double quotes expect(remoteCommand).toMatch(/"\$HOME\/path.*special"/); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/utils/remote-fs.test.ts` around lines 171 - 183, The test currently covers expansion of home-relative paths but we should add two more unit tests for shellEscapeRemotePath behavior: one to verify a bare "~" is expanded to "$HOME" when calling readDirRemote, and another to ensure special characters inside a home-relative path (e.g., ~/path$with"special) are safely escaped and still wrapped in double quotes in the SSH command; add tests in src/__tests__/main/utils/remote-fs.test.ts that call readDirRemote with '~' and with a path containing special chars, then inspect the mocked deps.execSsh call (as done in the existing test) to assert the remote command contains '"$HOME"' for the bare-tilde case and a quoted/escaped "$HOME/..." string (use a regex match) for the special-character case, using the same createMockDeps fixture and baseConfig so they follow existing test patterns.src/renderer/components/UsageDashboard/SessionStats.tsx (1)
88-101: Same consolidation opportunity asAgentEfficiencyChart.tsx.This
formatAgentNamefunction duplicates the sharedAGENT_DISPLAY_NAMESmapping. Consider importinggetAgentDisplayNamefromsrc/shared/agentMetadata.tsto keep agent display names in a single location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/SessionStats.tsx` around lines 88 - 101, Replace the local formatAgentName mapping with the shared lookup: import and call getAgentDisplayName (from the shared agent metadata module) inside formatAgentName (or replace formatAgentName with a thin wrapper around getAgentDisplayName), remove the duplicated names object, and keep the same fallback behavior (return the original toolType if getAgentDisplayName returns undefined). Update any references to formatAgentName to continue working unchanged.src/__tests__/main/process-manager/spawners/PtySpawner.test.ts (1)
57-89: Good test coverage for Windows shell wrapping.The test correctly validates that
.cmdfiles are wrapped incmd.exewith proper escaping. Consider adding a complementary test case verifying that non-.cmdexecutables (e.g., direct binary paths) are NOT wrapped incmd.exeto ensure theneedsWindowsShellguard works both ways.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/process-manager/spawners/PtySpawner.test.ts` around lines 57 - 89, Add a complementary unit test to PtySpawner.test.ts that verifies non-.cmd executables are NOT wrapped by cmd.exe: create a similar test using PtySpawner.spawn with a command like 'C:\\Program Files\\git\\bin\\git.exe' (or another .exe path), requiresPty: true, and assert mockPtySpawn was called with the original executable path and the original args (not 'C:\\Windows\\System32\\cmd.exe' and not the combined string), and that the cwd is preserved; place the test alongside the existing "wraps Windows .cmd agents" test and reference PtySpawner.spawn and mockPtySpawn in the assertions.src/renderer/components/UsageDashboard/AgentEfficiencyChart.tsx (1)
52-65: Consider using sharedgetAgentDisplayNamefromagentMetadata.ts.This local
formatAgentNamefunction duplicates theAGENT_DISPLAY_NAMESmapping fromsrc/shared/agentMetadata.ts. The same pattern exists inSessionStats.tsx. Consider importinggetAgentDisplayNamefrom the shared module to maintain a single source of truth for agent display names.♻️ Suggested refactor
+import { getAgentDisplayName } from '../../../shared/agentMetadata'; -/** - * Format agent type display name - */ -function formatAgentName(agent: string): string { - const names: Record<string, string> = { - 'claude-code': 'Claude Code', - opencode: 'OpenCode', - 'openai-codex': 'OpenAI Codex', - codex: 'Codex', - 'gemini-cli': 'Gemini CLI', - 'qwen3-coder': 'Qwen3 Coder', - 'factory-droid': 'Factory Droid', - copilot: 'GitHub Copilot', - terminal: 'Terminal', - }; - return names[agent] || agent; -} +// Use getAgentDisplayName directly from shared moduleThen replace
formatAgentName(agent.agent)calls withgetAgentDisplayName(agent.agent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/UsageDashboard/AgentEfficiencyChart.tsx` around lines 52 - 65, formatAgentName duplicates the AGENT_DISPLAY_NAMES mapping; replace this local mapper with the shared function by importing getAgentDisplayName from src/shared/agentMetadata.ts and use getAgentDisplayName(agent) wherever formatAgentName is called (and likewise update SessionStats.tsx uses). Remove the local formatAgentName function and its names map, update imports to include getAgentDisplayName, and ensure calls like formatAgentName(agent.agent) become getAgentDisplayName(agent.agent) so the app uses a single source of truth.src/main/agents/path-prober.ts (1)
432-444: Remove duplicated Unix probe path for Copilot.
homebrew('copilot')already includes/usr/local/bin/copilot, so the explicit entry is redundant.♻️ Proposed cleanup
copilot: [ // Homebrew installation (primary method on macOS) ...homebrew('copilot'), - // GitHub CLI installation - '/usr/local/bin/copilot', path.join(home, '.local', 'bin', 'copilot'), // npm global ...npmGlobal('copilot'),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/agents/path-prober.ts` around lines 432 - 444, The copilot probe array contains a redundant explicit path '/usr/local/bin/copilot' because homebrew('copilot') already expands to that; remove the literal '/usr/local/bin/copilot' entry from the copilot array and leave the spread of homebrew('copilot'), npmGlobal('copilot'), nodeVersionManagers('copilot'), and other entries (path.join(home, '.local', 'bin', 'copilot') and path.join(home, 'bin', 'copilot')) unchanged so probes remain comprehensive without duplication.src/main/storage/copilot-session-storage.ts (1)
249-259: Consider consistent parameter passing for clarity.The path helpers work correctly, but there's an inconsistency: the SSH branch explicitly passes
sshConfigtogetSessionDirwhile the local branch omits it. SincegetSessionDirhandles both cases via ternary, this is functionally correct but could be clearer.🔧 Optional: Consistent parameter passing
private getWorkspacePath(sessionId: string, sshConfig?: SshRemoteConfig): string { return sshConfig ? path.posix.join(this.getSessionDir(sessionId, sshConfig), 'workspace.yaml') - : path.join(this.getSessionDir(sessionId), 'workspace.yaml'); + : path.join(this.getSessionDir(sessionId, sshConfig), 'workspace.yaml'); } private getEventsPath(sessionId: string, sshConfig?: SshRemoteConfig): string { return sshConfig ? path.posix.join(this.getSessionDir(sessionId, sshConfig), 'events.jsonl') - : path.join(this.getSessionDir(sessionId), 'events.jsonl'); + : path.join(this.getSessionDir(sessionId, sshConfig), 'events.jsonl'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-session-storage.ts` around lines 249 - 259, Both helpers getWorkspacePath and getEventsPath should call getSessionDir with the same arguments in both branches for clarity; change the local-branch calls to pass the optional sshConfig as well (i.e., call getSessionDir(sessionId, sshConfig) in both the sshConfig ? and : branches) so getSessionDir usage is consistent across the functions (references: getWorkspacePath, getEventsPath, getSessionDir, SshRemoteConfig).
🤖 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/releases.md`:
- Line 20: Several section headings in docs/releases.md (for example the "Major
0.15.x Additions" heading) were promoted to top-level (#) and flattened the
document hierarchy; revert these headings to the proper subordinate level (use
##, ###, etc. to match surrounding headings) so intra-section headings align
with the document's existing structure. Locate each offending heading text
(e.g., "Major 0.15.x Additions" and the other headings listed in the review) and
replace the leading single '#' with the correct number of '#' characters to
match neighboring sections and restore the navigation/TOC structure.
- Line 101: Fix the user-facing typos/branding in the sentence starting "🗄️
Document Graphs. Launch from file preview or from the FIle tree panel…" by
normalizing capitalization and tightening wording — e.g., change to "Document
graphs: launch from the file preview or file tree panel and explore
relationships between Markdown documents that link to other documents and
external URLs." Replace the incorrect "FIle" casing and ensure consistent
lowercase/branding for "file preview" and "file tree panel", and apply the same
corrections to the other identical occurrences of this phrasing elsewhere in the
document.
In `@src/main/parsers/copilot-output-parser.ts`:
- Line 76: The parser currently keeps mutable state in the singleton field
"toolNames" on CopilotOutputParser, causing cross-session collisions; move this
state into the per-process session (e.g., ManagedProcess) and stop storing it on
the parser singleton. Remove the private toolNames field from
CopilotOutputParser and instead add a session-scoped Map on ManagedProcess (or
accept a session-scoped map/object), update the parser API (methods that
reference toolNames) to take a session or toolNames map parameter (or add a
setSessionToolNames(sessionId|map) method), and change usages in
ChildProcessSpawner.getOutputParser('copilot') and any caller sites to pass the
ManagedProcess/session instance or its tool map so each Copilot run uses its own
toolNames map keyed by toolCallId. Ensure no other mutable parser state remains
shared across processes.
- Around line 285-317: The current logic in the error parsing block (inside
copilot-output-parser.ts) fabricates an errorText when only an exitCode exists,
causing detectErrorFromExit() in StdoutHandler to be bypassed; change the
behavior in the function using extractErrorText, getErrorPatterns, and
matchErrorPattern so that if no actual textual error is present (i.e.,
extractErrorText(msg.error) and extractErrorText(msg.data?.error) both return
falsy) you return null instead of synthesizing `Copilot exited with code
${msg.exitCode}`, thereby preserving the !errorEmitted guard and allowing
detectErrorFromExit() to run its stderr/stdout-based classification
(auth_expired, session_expired, rate_limited, etc.).
In `@src/main/storage/copilot-session-storage.ts`:
- Around line 384-386: The code falls back to new Date().toISOString() for
timestamp which can falsely set creation to "now"; change the fallback so
timestamp = metadata.created_at || metadata.updated_at || workspace file mtime
(read via fs.statSync/stat and use mtime.toISOString()) or a neutral default
(e.g., null or epoch) instead of current time; update the related
variables/logic around timestamp and modifiedAt in copilot-session-storage.ts
(refer to projectRoot, timestamp, modifiedAt, metadata, projectPath) so
created_at prefers existing updated_at or file mtime before using any
non-deterministic current time.
In `@src/main/utils/agent-args.ts`:
- Around line 69-73: The overlap check for jsonOutputArgs in the block that
references agent.jsonOutputArgs, options.prompt and finalArgs.some(...) is too
permissive because it treats tokens independently and can falsely suppress
multi-token flags; change the check to detect whole-sequence occurrences instead
of any-token matches (e.g., implement or call a containsSequence(sequence,
array) helper that checks for contiguous subarray equality between
agent.jsonOutputArgs and finalArgs) and use that in place of the current
finalArgs.some(...) test so multi-token flags like ["--output-format","json"]
are only considered duplicates when the exact sequence exists in finalArgs.
---
Outside diff comments:
In `@README.md`:
- Around line 105-110: The README has inconsistent "supported agent"
lists—update all occurrences of the supported AI coding agents so they match and
include "GitHub Copilot CLI" alongside "Claude Code", "OpenAI Codex", and
"OpenCode"; search for the agent bullet lists (the "At least one supported AI
coding agent installed and authenticated" section and any other
prerequisite/overview lists) and add the "GitHub Copilot CLI" entry with the
same link text/format as used in the newly added item to keep wording and links
consistent across the README.
In `@src/main/parsers/error-patterns.ts`:
- Around line 858-958: The Copilot-specific error patterns
(COPILOT_ERROR_PATTERNS) were registered only in patternRegistry and not
mirrored into SSH_ERROR_PATTERNS, so SSH-backed Copilot sessions miss
Copilot-specific install/crash diagnostics; update
SSH_ERROR_PATTERNS.agent_crashed to include the same Copilot entries (or
merge/copied relevant patterns from COPILOT_ERROR_PATTERNS.agent_crashed such as
the "no prompt provided.*interactive terminal" and install-related patterns)
and/or add a registration step that maps 'copilot' patterns into the
SSH_ERROR_PATTERNS set, ensuring patternRegistry and SSH_ERROR_PATTERNS both
include the Copilot patterns so messages like "bash: copilot: command not found"
match the friendly install diagnostic.
---
Nitpick comments:
In `@src/__tests__/main/process-manager/spawners/PtySpawner.test.ts`:
- Around line 57-89: Add a complementary unit test to PtySpawner.test.ts that
verifies non-.cmd executables are NOT wrapped by cmd.exe: create a similar test
using PtySpawner.spawn with a command like 'C:\\Program
Files\\git\\bin\\git.exe' (or another .exe path), requiresPty: true, and assert
mockPtySpawn was called with the original executable path and the original args
(not 'C:\\Windows\\System32\\cmd.exe' and not the combined string), and that the
cwd is preserved; place the test alongside the existing "wraps Windows .cmd
agents" test and reference PtySpawner.spawn and mockPtySpawn in the assertions.
In `@src/__tests__/main/utils/remote-fs.test.ts`:
- Around line 171-183: The test currently covers expansion of home-relative
paths but we should add two more unit tests for shellEscapeRemotePath behavior:
one to verify a bare "~" is expanded to "$HOME" when calling readDirRemote, and
another to ensure special characters inside a home-relative path (e.g.,
~/path$with"special) are safely escaped and still wrapped in double quotes in
the SSH command; add tests in src/__tests__/main/utils/remote-fs.test.ts that
call readDirRemote with '~' and with a path containing special chars, then
inspect the mocked deps.execSsh call (as done in the existing test) to assert
the remote command contains '"$HOME"' for the bare-tilde case and a
quoted/escaped "$HOME/..." string (use a regex match) for the special-character
case, using the same createMockDeps fixture and baseConfig so they follow
existing test patterns.
In `@src/main/agents/path-prober.ts`:
- Around line 432-444: The copilot probe array contains a redundant explicit
path '/usr/local/bin/copilot' because homebrew('copilot') already expands to
that; remove the literal '/usr/local/bin/copilot' entry from the copilot array
and leave the spread of homebrew('copilot'), npmGlobal('copilot'),
nodeVersionManagers('copilot'), and other entries (path.join(home, '.local',
'bin', 'copilot') and path.join(home, 'bin', 'copilot')) unchanged so probes
remain comprehensive without duplication.
In `@src/main/storage/copilot-session-storage.ts`:
- Around line 249-259: Both helpers getWorkspacePath and getEventsPath should
call getSessionDir with the same arguments in both branches for clarity; change
the local-branch calls to pass the optional sshConfig as well (i.e., call
getSessionDir(sessionId, sshConfig) in both the sshConfig ? and : branches) so
getSessionDir usage is consistent across the functions (references:
getWorkspacePath, getEventsPath, getSessionDir, SshRemoteConfig).
In `@src/renderer/components/UsageDashboard/AgentEfficiencyChart.tsx`:
- Around line 52-65: formatAgentName duplicates the AGENT_DISPLAY_NAMES mapping;
replace this local mapper with the shared function by importing
getAgentDisplayName from src/shared/agentMetadata.ts and use
getAgentDisplayName(agent) wherever formatAgentName is called (and likewise
update SessionStats.tsx uses). Remove the local formatAgentName function and its
names map, update imports to include getAgentDisplayName, and ensure calls like
formatAgentName(agent.agent) become getAgentDisplayName(agent.agent) so the app
uses a single source of truth.
In `@src/renderer/components/UsageDashboard/SessionStats.tsx`:
- Around line 88-101: Replace the local formatAgentName mapping with the shared
lookup: import and call getAgentDisplayName (from the shared agent metadata
module) inside formatAgentName (or replace formatAgentName with a thin wrapper
around getAgentDisplayName), remove the duplicated names object, and keep the
same fallback behavior (return the original toolType if getAgentDisplayName
returns undefined). Update any references to formatAgentName to continue working
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2c06fcc-e9fc-417c-8839-1610ea0af7c9
📒 Files selected for processing (44)
CLAUDE-AGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mddocs/installation.mddocs/releases.mdsrc/__tests__/main/agents/definitions.test.tssrc/__tests__/main/agents/detector.test.tssrc/__tests__/main/agents/session-storage.test.tssrc/__tests__/main/parsers/copilot-output-parser.test.tssrc/__tests__/main/parsers/index.test.tssrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/__tests__/main/process-manager/spawners/PtySpawner.test.tssrc/__tests__/main/utils/agent-args.test.tssrc/__tests__/main/utils/remote-fs.test.tssrc/__tests__/renderer/hooks/useAgentListeners.test.tssrc/__tests__/shared/agentIds.test.tssrc/__tests__/shared/agentMetadata.test.tssrc/main/agents/capabilities.tssrc/main/agents/definitions.tssrc/main/agents/path-prober.tssrc/main/parsers/agent-output-parser.tssrc/main/parsers/copilot-output-parser.tssrc/main/parsers/error-patterns.tssrc/main/parsers/index.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/process-manager/spawners/PtySpawner.tssrc/main/process-manager/types.tssrc/main/storage/copilot-session-storage.tssrc/main/storage/index.tssrc/main/utils/agent-args.tssrc/main/utils/remote-fs.tssrc/renderer/components/NewInstanceModal.tsxsrc/renderer/components/UsageDashboard/AgentEfficiencyChart.tsxsrc/renderer/components/UsageDashboard/LongestAutoRunsTable.tsxsrc/renderer/components/UsageDashboard/SessionStats.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen.tsxsrc/renderer/constants/agentIcons.tssrc/renderer/hooks/agent/useAgentListeners.tssrc/shared/agentConstants.tssrc/shared/agentIds.tssrc/shared/agentMetadata.ts
Greptile SummaryThis PR wires GitHub Copilot CLI into Maestro as a new beta agent end-to-end, including agent detection, spawn definitions, a dedicated JSONL output parser, session storage backed by Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Renderer UI
participant IPC as IPC Handler
participant CS as ChildProcessSpawner
participant CP as copilot CLI
participant SH as StdoutHandler
participant PF as ParserFactory
participant COP as CopilotOutputParser
UI->>IPC: spawn({ toolType:'copilot', prompt })
IPC->>IPC: buildAgentArgs() → adds --allow-all-tools --silent --output-format json
IPC->>CS: spawn(config)
CS->>PF: createOutputParser('copilot')
PF-->>CS: new CopilotOutputParser()
CS->>CP: exec copilot -p "..." --output-format json
CP-->>SH: stdout chunk (concatenated JSON objects)
SH->>SH: extractConcatenatedJsonObjects(buffer)
loop each complete JSON object
SH->>COP: parseJsonObject(parsed)
alt assistant.message (phase=final_answer)
COP-->>SH: ParsedEvent{type:'result', text, toolUseBlocks}
SH->>SH: isResultMessage() → true
SH->>UI: emit('result', sessionId, text)
else tool.execution_start
COP-->>SH: ParsedEvent{type:'tool_use', status:'running'}
SH->>SH: emittedToolCallIds.add(id)
SH->>UI: emit('tool-execution', running)
else tool.execution_complete
COP-->>SH: ParsedEvent{type:'tool_use', status:'completed'}
SH->>SH: emittedToolCallIds.delete(id)
SH->>UI: emit('tool-execution', completed)
else session.shutdown
COP-->>SH: ParsedEvent{type:'usage', inputTokens, outputTokens}
SH->>UI: emit('usage', stats)
end
end
CP-->>SH: process exit(0)
SH->>UI: emit('exit', sessionId, 0)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/process-manager/handlers/StdoutHandler.ts (1)
505-515: PruneemittedToolCallIdsfor embedded tool blocks.Line 532 adds every
tool.idinto a per-processSet, but this path never removes it; only explicitcompleted/failedtool_useevents do. For agents that only surface embedded tool blocks, the cache grows for the whole session. Clearing it on turn completion/result emission, or using a smaller windowed dedupe cache, would avoid that buildup.Also applies to: 525-533
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 505 - 515, The emitted tool-call ID cache (getEmittedToolCallIds(managedProcess)) is only being added to for running/tool_use with a toolName but not cleared for embedded tool blocks, causing unbounded growth; update StdoutHandler to prune those IDs when a turn completes or a result is emitted (e.g., hook into the existing turn-completion/result-emission path to call getEmittedToolCallIds(managedProcess).clear() or selectively delete the embedded tool IDs), and ensure the existing delete branch (getEmittedToolCallIds(...).delete(...)) still handles explicit completed/failed events; alternatively implement a small windowed/size-limited dedupe set for emittedToolCallIds to bound memory.
🤖 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/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 505-515: The emitted tool-call ID cache
(getEmittedToolCallIds(managedProcess)) is only being added to for
running/tool_use with a toolName but not cleared for embedded tool blocks,
causing unbounded growth; update StdoutHandler to prune those IDs when a turn
completes or a result is emitted (e.g., hook into the existing
turn-completion/result-emission path to call
getEmittedToolCallIds(managedProcess).clear() or selectively delete the embedded
tool IDs), and ensure the existing delete branch
(getEmittedToolCallIds(...).delete(...)) still handles explicit completed/failed
events; alternatively implement a small windowed/size-limited dedupe set for
emittedToolCallIds to bound memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58639db4-84a4-41cd-9289-5c47bdec2557
📒 Files selected for processing (4)
src/__tests__/main/agents/session-storage.test.tssrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/storage/copilot-session-storage.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/storage/copilot-session-storage.ts (1)
450-451:⚠️ Potential issue | 🟡 MinorAvoid non-deterministic
new Date()fallback for session creation time.Line 450 can assign “now” to older sessions missing
created_at, which distorts ordering/history metadata.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-session-storage.ts` around lines 450 - 451, Replace the non-deterministic fallback new Date().toISOString() when computing timestamp/modifiedAt: set timestamp = metadata.created_at || metadata.updated_at || null (or undefined) and set modifiedAt = metadata.updated_at || metadata.created_at || null so ordering relies only on existing metadata fields (metadata.created_at and metadata.updated_at) instead of generating "now"; update any downstream consumers to handle null/undefined if both fields are missing.
🧹 Nitpick comments (3)
src/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.ts (1)
54-62: Align the parser mock with the fullAgentOutputParsercontract.Line 54–62 omits newer parser methods, which can mask integration breaks if
ChildProcessSpawnerstarts depending on them.🧪 Suggested mock extension
vi.mock('../../../../main/parsers', () => ({ createOutputParser: vi.fn(() => ({ agentId: 'claude-code', parseJsonLine: vi.fn(), + parseJsonObject: vi.fn(), extractUsage: vi.fn(), extractSessionId: vi.fn(), extractSlashCommands: vi.fn(), isResultMessage: vi.fn(), detectErrorFromLine: vi.fn(), + detectErrorFromParsed: vi.fn(), + detectErrorFromExit: vi.fn(), })), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.ts` around lines 54 - 62, The mock returned by createOutputParser in the test doesn't implement the full AgentOutputParser contract, which risks masking regressions in ChildProcessSpawner; update the mock in ChildProcessSpawner.test.ts so createOutputParser: vi.fn(() => ({ ... })) includes all current methods used by AgentOutputParser (add any newer methods such as parseStream/close/reset/getState or other recently added helpers) and ensure each method is a vi.fn() or appropriate stub so tests exercise the same API surface as the real AgentOutputParser used by ChildProcessSpawner.src/main/parsers/parser-factory.ts (1)
21-27: Keep parser registration and parser-constructor sources unified.Line 21–27 duplicates parser membership already maintained in
src/main/parsers/index.ts. Consider deriving both from one shared source to prevent future drift when adding/removing agents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/parser-factory.ts` around lines 21 - 27, PARSER_CONSTRUCTORS currently duplicates the parser membership maintained in src/main/parsers/index.ts; unify them by making a single source of truth (e.g., export a shared registry or factory list from the module that currently exports the parser implementations) and import that into parser-factory.ts instead of hardcoding PARSER_CONSTRUCTORS; specifically, replace the hardcoded mapping of string keys to constructors (the PARSER_CONSTRUCTORS constant that returns AgentOutputParser instances like ClaudeOutputParser, OpenCodeOutputParser, CodexOutputParser, FactoryDroidOutputParser, CopilotOutputParser) with a reference to the shared registry exported from the parsers index so additions/removals update both places automatically.src/main/utils/agent-args.ts (1)
60-66: Surface conflicting output-format overrides explicitly.Line 60–66 silently treats an existing flag key with a different value as “already handled.” Consider logging a warning here so users know JSON parsing may be disabled by conflicting custom args.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/agent-args.ts` around lines 60 - 66, The code silently skips adding JSON output args when a flag key (flagKey) already exists; update the block so when flagKey?.startsWith('-') && jsonOutputArgs.length > 1 you check if haystack already contains flagKey and whether the following value differs from jsonOutputArgs[1]; if a conflicting value is detected emit a clear warning (use the project's logger or console.warn) that includes flagKey and the existing conflicting value before returning haystack.includes(flagKey). Ensure you do not change the existing return behavior (keep returning haystack.includes(flagKey)) and reference the existing variables flagKey, jsonOutputArgs, and haystack.
🤖 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/releases.md`:
- Line 22: In the "Maestro Symphony" release blurb update the compound adjective
by changing the phrase "open source projects" to "open-source projects" so the
adjective modifying "projects" is hyphenated; locate the sentence containing
"Browse curated issues from projects with the `runmaestro.ai` label" (the line
starting with "🎶 **Maestro Symphony**") and replace the unhyphenated phrase
accordingly.
- Line 90: Replace the phrase "cross context memory" with the hyphenated
compound adjective "cross-context memory" in the sentence that reads "Agents are
now inherently aware of your activity history as seen in the history panel 📜
(this is built-in cross context memory!)" so it reads "...(this is built-in
cross-context memory!)".
In `@src/main/process-manager/types.ts`:
- Line 73: The overflow/resync paths in StdoutHandler currently reset jsonBuffer
and toggle jsonBufferCorrupted but do not clear the dedupe Set
emittedToolCallIds; update those code paths in StdoutHandler to also reset
emittedToolCallIds (e.g., clear the Set or replace with a new Set) whenever
jsonBuffer is cleared or jsonBufferCorrupted is toggled so stale IDs are removed
and valid tool events are not suppressed.
---
Duplicate comments:
In `@src/main/storage/copilot-session-storage.ts`:
- Around line 450-451: Replace the non-deterministic fallback new
Date().toISOString() when computing timestamp/modifiedAt: set timestamp =
metadata.created_at || metadata.updated_at || null (or undefined) and set
modifiedAt = metadata.updated_at || metadata.created_at || null so ordering
relies only on existing metadata fields (metadata.created_at and
metadata.updated_at) instead of generating "now"; update any downstream
consumers to handle null/undefined if both fields are missing.
---
Nitpick comments:
In `@src/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.ts`:
- Around line 54-62: The mock returned by createOutputParser in the test doesn't
implement the full AgentOutputParser contract, which risks masking regressions
in ChildProcessSpawner; update the mock in ChildProcessSpawner.test.ts so
createOutputParser: vi.fn(() => ({ ... })) includes all current methods used by
AgentOutputParser (add any newer methods such as
parseStream/close/reset/getState or other recently added helpers) and ensure
each method is a vi.fn() or appropriate stub so tests exercise the same API
surface as the real AgentOutputParser used by ChildProcessSpawner.
In `@src/main/parsers/parser-factory.ts`:
- Around line 21-27: PARSER_CONSTRUCTORS currently duplicates the parser
membership maintained in src/main/parsers/index.ts; unify them by making a
single source of truth (e.g., export a shared registry or factory list from the
module that currently exports the parser implementations) and import that into
parser-factory.ts instead of hardcoding PARSER_CONSTRUCTORS; specifically,
replace the hardcoded mapping of string keys to constructors (the
PARSER_CONSTRUCTORS constant that returns AgentOutputParser instances like
ClaudeOutputParser, OpenCodeOutputParser, CodexOutputParser,
FactoryDroidOutputParser, CopilotOutputParser) with a reference to the shared
registry exported from the parsers index so additions/removals update both
places automatically.
In `@src/main/utils/agent-args.ts`:
- Around line 60-66: The code silently skips adding JSON output args when a flag
key (flagKey) already exists; update the block so when flagKey?.startsWith('-')
&& jsonOutputArgs.length > 1 you check if haystack already contains flagKey and
whether the following value differs from jsonOutputArgs[1]; if a conflicting
value is detected emit a clear warning (use the project's logger or
console.warn) that includes flagKey and the existing conflicting value before
returning haystack.includes(flagKey). Ensure you do not change the existing
return behavior (keep returning haystack.includes(flagKey)) and reference the
existing variables flagKey, jsonOutputArgs, and haystack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb155574-0c18-4323-bbda-85481f877666
📒 Files selected for processing (12)
docs/releases.mdsrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/__tests__/main/utils/agent-args.test.tssrc/main/parsers/copilot-output-parser.tssrc/main/parsers/index.tssrc/main/parsers/parser-factory.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/process-manager/types.tssrc/main/storage/copilot-session-storage.tssrc/main/utils/agent-args.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/process-manager/handlers/StdoutHandler.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
docs/releases.md (2)
170-182:⚠️ Potential issue | 🟡 MinorRestore heading hierarchy under v0.12.x.
Line 170, Line 174, Line 178, and Line 182 use
##, which flattens section structure under the existing## v0.12.xheading. These should be subordinate headings.✏️ Suggested fix
-## Show Thinking +### Show Thinking -## GitHub Spec-Kit Integration +### GitHub Spec-Kit Integration -## Context Management Tools +### Context Management Tools -## Changes Specific to v0.12.3: +### Changes Specific to v0.12.3:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` around lines 170 - 182, The release notes use top-level "##" headings for "Show Thinking", "GitHub Spec-Kit Integration", "Context Management Tools", and "Changes Specific to v0.12.3" which flattens them under the existing "## v0.12.x" section; update those headings to subordinate headings (e.g., change the "##" tokens for the lines containing "Show Thinking", "GitHub Spec-Kit Integration", "Context Management Tools", and "Changes Specific to v0.12.3" to "###" so they correctly nest under the v0.12.x heading and restore proper hierarchy).
176-176:⚠️ Potential issue | 🟡 MinorFix typo in feature reference (“Wortrees” → “Worktrees”).
Line 176 contains a typo that makes the sentence look unpolished in user-facing docs.
✏️ Suggested fix
-... which thanks to Wortrees from v0.11.x allows us to run in parallel! +... which thanks to Worktrees from v0.11.x allows us to run in parallel!🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` at line 176, Replace the typo "Wortrees" with "Worktrees" in the release note sentence that mentions the GitHub Spec-Kit commands and overrides `/speckit-implement` (the sentence that currently reads "...which thanks to Wortrees from v0.11.x allows us to run in parallel!"); update the text to "Worktrees" so the feature name is spelled correctly and matches the Worktrees feature reference.
🤖 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/releases.md`:
- Line 266: Replace the non-hyphenated term "prerelease" with the hyphenated
"pre-release" in the release notes entry that currently reads "Enhanced update
checker to filter prerelease tags like -rc, -beta 🚀" and ensure every other
occurrence of "prerelease" in the document matches the "pre-release" spelling so
the document uses a single consistent variant.
- Line 180: In the release notes sentence containing "You will received
(configurable) warnings at 60% and 80% context consumption with a hint to
compact," fix the grammar by replacing "will received" with "will receive" so
the sentence reads "You will receive (configurable) warnings at 60% and 80%
context consumption with a hint to compact."
---
Duplicate comments:
In `@docs/releases.md`:
- Around line 170-182: The release notes use top-level "##" headings for "Show
Thinking", "GitHub Spec-Kit Integration", "Context Management Tools", and
"Changes Specific to v0.12.3" which flattens them under the existing "##
v0.12.x" section; update those headings to subordinate headings (e.g., change
the "##" tokens for the lines containing "Show Thinking", "GitHub Spec-Kit
Integration", "Context Management Tools", and "Changes Specific to v0.12.3" to
"###" so they correctly nest under the v0.12.x heading and restore proper
hierarchy).
- Line 176: Replace the typo "Wortrees" with "Worktrees" in the release note
sentence that mentions the GitHub Spec-Kit commands and overrides
`/speckit-implement` (the sentence that currently reads "...which thanks to
Wortrees from v0.11.x allows us to run in parallel!"); update the text to
"Worktrees" so the feature name is spelled correctly and matches the Worktrees
feature reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8528c65-8f97-4c6a-b074-4f9ee45da3b7
📒 Files selected for processing (3)
docs/releases.mdsrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/main/process-manager/handlers/StdoutHandler.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/releases.md (1)
170-178:⚠️ Potential issue | 🟡 MinorDemote nested v0.12 subsection headings to the correct level.
Line 170, Line 174, and Line 178 are
##headings inside an existing##section (v0.12.x), which flattens hierarchy and can break docs navigation. These should be###to stay nested correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` around lines 170 - 178, The three subsection headings "Show Thinking", "GitHub Spec-Kit Integration", and "Context Management Tools" are using "##" but should be nested under the surrounding v0.12.x section; update each of these headings to "###" so they remain correctly nested in the v0.12.x section and preserve docs navigation and TOC generation.
🤖 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/releases.md`:
- Line 176: Replace the typo "Wortrees" with "Worktrees" in the release note
text that currently reads "thanks to Wortrees from v0.11.x" so it becomes
"thanks to Worktrees from v0.11.x"; update the exact string in the release notes
entry (the sentence containing "/speckit-implement" and the "thanks to
Wortrees..." fragment) to correct the user-facing text.
---
Duplicate comments:
In `@docs/releases.md`:
- Around line 170-178: The three subsection headings "Show Thinking", "GitHub
Spec-Kit Integration", and "Context Management Tools" are using "##" but should
be nested under the surrounding v0.12.x section; update each of these headings
to "###" so they remain correctly nested in the v0.12.x section and preserve
docs navigation and TOC generation.
✅ Actions performedReviews resumed. |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/utils/agent-args.ts (1)
197-206:⚠️ Potential issue | 🟠 MajorRe-run JSON-output conflict checks after custom args are appended.
buildAgentArgs()injectsagent.jsonOutputArgsbeforesrc/main/ipc/handlers/process.tscallsapplyAgentConfigOverrides(). A user override like--output-format textlands here after the earlierhasJsonOutputFlag()pass, so the final argv can still contain conflicting output-format flags and break batch-mode JSON parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/agent-args.ts` around lines 197 - 206, After appending parsedCustomArgs to finalArgs in buildAgentArgs (using effectiveCustomArgs, parseCustomArgs, and finalArgs), re-run the existing JSON-output conflict detection (the same logic used by hasJsonOutputFlag / agent.jsonOutputArgs validation) to detect any new conflicting output-format flags introduced by session custom args; if a conflict is found, resolve it consistently (either remove the conflicting flag, prefer the later session override, or throw a clear error) and ensure applyAgentConfigOverrides sees the corrected argv so batch-mode JSON parsing won’t be broken.src/main/process-manager/handlers/StdoutHandler.ts (2)
366-384:⚠️ Potential issue | 🟠 MajorUse agent-specific SSH re-auth guidance here.
This branch still tells users to run
claude loginfor every remoteauth_expirederror. Copilot can now hit it too, so non-Claude sessions will get the wrong recovery command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 366 - 384, The auth_expired error handler currently always suggests `claude login`; update the logic in the block that checks agentError.type === 'auth_expired' (inside the stdout handling where parsed, toolType, outputParser.detectErrorFromParsed/detectErrorFromLine and managedProcess.sshRemoteHost are used) to choose the correct re-auth command based on the agent/tool type (use the existing toolType or a managedProcess property if available). Replace the hardcoded `claude login` message with a conditional that picks the appropriate guidance per tool (e.g., for Claude keep `claude login`, for Copilot provide the Copilot-specific re-auth steps), ensuring the message still includes the remote host when managedProcess.sshRemoteHost is set and sets agentError.message accordingly.
548-564:⚠️ Potential issue | 🟠 MajorDon't reopen completed Copilot tools from
toolUseBlocks.
CopilotOutputParser.parseAssistantMessage()attachestoolUseBlockstofinal_answerresult events. By the time this loop runs, the matchingtool.execution_completehas already cleared the ID fromemittedToolCallIds, so the final answer re-emits the tool as a freshrunningexecution and leaves it hanging in the UI.🔧 Minimal guard
- if (event.toolUseBlocks?.length) { + if (event.type === 'text' && event.toolUseBlocks?.length) { for (const tool of event.toolUseBlocks) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 548 - 564, The loop over event.toolUseBlocks can re-emit tools that were already completed; add a guard at the top of the loop (in StdoutHandler.ts where event.toolUseBlocks, getEmittedToolCallIds(managedProcess), emittedToolCallIds.has(tool.id), emittedToolCallIds.add(tool.id) and this.emitter.emit('tool-execution'...) are used) to skip any tool block that represents a completed execution — e.g. check for a completion indicator on the tool (tool.execution_complete === true or tool.state?.status === 'completed' or tool.completed === true) and continue without adding the id or emitting; only add the id and emit when the tool is not marked completed.
♻️ Duplicate comments (1)
docs/releases.md (1)
170-178:⚠️ Potential issue | 🟡 MinorFix heading level under v0.12.x section
At Line 170, Line 174, and Line 178, these subsection headings use
##even though they are nested under## v0.12.x. This flattens the document hierarchy and hurts TOC/navigation.✏️ Suggested fix
-## Show Thinking +### Show Thinking -## GitHub Spec-Kit Integration +### GitHub Spec-Kit Integration -## Context Management Tools +### Context Management Tools🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/releases.md` around lines 170 - 178, The three subsection headings "Show Thinking", "GitHub Spec-Kit Integration", and "Context Management Tools" are using top-level level-2 headings (##) under the existing "v0.12.x" section which flattens the TOC; change each of those headings to level-3 (###) so they are correctly nested under the "v0.12.x" section (update the lines containing those exact heading texts).
🤖 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/releases.md`:
- Around line 240-242: The release note bullet "Add Sentry crashing reporting
monitoring with opt-out 🐛" contains a wording typo; update that sentence to
clear, grammatical user-facing text such as "Add Sentry crash reporting
(opt-out) 🐛" or "Add Sentry crash reporting with opt-out 🐛" so the phrasing
around Sentry and opt-out is correct; locate and replace the exact string "Add
Sentry crashing reporting monitoring with opt-out 🐛" in the releases.md
content.
In `@src/main/storage/copilot-session-storage.ts`:
- Around line 151-169: The matching is currently case-sensitive so Windows paths
like "C:\Repo" vs "c:\repo" won't match; update normalizePath/matchesProject to
case-fold Windows-style paths only: detect Windows paths (e.g., presence of a
drive letter followed by ':' or backslashes) after normalizing separators and
trailing slashes, then call .toLowerCase() on normalizedProject,
metadata.git_root and metadata.cwd when a Windows-style path is detected, but
leave POSIX/SSH paths case-sensitive; ensure matchesProject still compares
git_root, cwd, and cwd.startsWith(`${normalizedProject}/`) using the possibly
lowercased values.
- Around line 499-503: The catch block that currently returns null for every
exception when loading Copilot session metadata (referencing sessionId and
logger.debug in copilot-session-storage.ts) must distinguish expected "not
found" or permission/read-recoverable errors from unexpected failures: catch
only file-not-found/readable errors (e.g., ENOENT/EPERM or use an existing
isNotFound/isFsError helper) and return null there, but for all other exceptions
call the Sentry/reporting utility (e.g., Sentry.captureException or the
project's reportError helper) with the error and contextual info, then rethrow
or return a structured error instead of swallowing it; update the catch in the
loadSessionMetadata/loadCopilotSessionMetadata function accordingly so
unexpected issues are surfaced.
---
Outside diff comments:
In `@src/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 366-384: The auth_expired error handler currently always suggests
`claude login`; update the logic in the block that checks agentError.type ===
'auth_expired' (inside the stdout handling where parsed, toolType,
outputParser.detectErrorFromParsed/detectErrorFromLine and
managedProcess.sshRemoteHost are used) to choose the correct re-auth command
based on the agent/tool type (use the existing toolType or a managedProcess
property if available). Replace the hardcoded `claude login` message with a
conditional that picks the appropriate guidance per tool (e.g., for Claude keep
`claude login`, for Copilot provide the Copilot-specific re-auth steps),
ensuring the message still includes the remote host when
managedProcess.sshRemoteHost is set and sets agentError.message accordingly.
- Around line 548-564: The loop over event.toolUseBlocks can re-emit tools that
were already completed; add a guard at the top of the loop (in StdoutHandler.ts
where event.toolUseBlocks, getEmittedToolCallIds(managedProcess),
emittedToolCallIds.has(tool.id), emittedToolCallIds.add(tool.id) and
this.emitter.emit('tool-execution'...) are used) to skip any tool block that
represents a completed execution — e.g. check for a completion indicator on the
tool (tool.execution_complete === true or tool.state?.status === 'completed' or
tool.completed === true) and continue without adding the id or emitting; only
add the id and emit when the tool is not marked completed.
In `@src/main/utils/agent-args.ts`:
- Around line 197-206: After appending parsedCustomArgs to finalArgs in
buildAgentArgs (using effectiveCustomArgs, parseCustomArgs, and finalArgs),
re-run the existing JSON-output conflict detection (the same logic used by
hasJsonOutputFlag / agent.jsonOutputArgs validation) to detect any new
conflicting output-format flags introduced by session custom args; if a conflict
is found, resolve it consistently (either remove the conflicting flag, prefer
the later session override, or throw a clear error) and ensure
applyAgentConfigOverrides sees the corrected argv so batch-mode JSON parsing
won’t be broken.
---
Duplicate comments:
In `@docs/releases.md`:
- Around line 170-178: The three subsection headings "Show Thinking", "GitHub
Spec-Kit Integration", and "Context Management Tools" are using top-level
level-2 headings (##) under the existing "v0.12.x" section which flattens the
TOC; change each of those headings to level-3 (###) so they are correctly nested
under the "v0.12.x" section (update the lines containing those exact heading
texts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fdefe50e-6d6c-4ac2-9afd-e4b49d997ae3
📒 Files selected for processing (5)
docs/releases.mdsrc/main/parsers/copilot-output-parser.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/storage/copilot-session-storage.tssrc/main/utils/agent-args.ts
Author
|
Fixed the "outside diff range" comment about empty The |
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)
src/main/process-manager/handlers/StdoutHandler.ts (1)
549-565:⚠️ Potential issue | 🟡 MinorTool ID deduplication for Claude Code never cleans up entries.
For OpenCode/Codex (lines 537-539), tool IDs are removed on
completed/failed. However, for Claude CodetoolUseBlocks(lines 551-557), IDs are added but never removed. Over long sessions with many tool invocations,emittedToolCallIdswill grow unbounded.Consider clearing tool IDs for Claude Code on result emission or implementing a bounded LRU cache if Claude Code doesn't emit explicit completion events.
🛠️ Suggested fix: Clear tool IDs on result emission
// Handle result if ( managedProcess.toolType !== 'codex' && outputParser.isResultMessage(event) && !managedProcess.resultEmitted ) { managedProcess.resultEmitted = true; + // Clear tool call tracking on turn completion to prevent unbounded growth + managedProcess.emittedToolCallIds?.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 549 - 565, The Claude Code toolUseBlocks path adds tool IDs to emittedToolCallIds (via getEmittedToolCallIds(managedProcess)) but never removes them, causing unbounded growth; update the StdoutHandler handling of event.toolUseBlocks (the loop that calls this.emitter.emit('tool-execution', ...)) to clear the ID after the corresponding result is emitted (or immediately after emit if Claude never sends completion), e.g., call emittedToolCallIds.delete(tool.id) at the appropriate place or replace the emittedToolCallIds store with a bounded/LRU cache implementation referenced by getEmittedToolCallIds to cap growth.
🧹 Nitpick comments (1)
src/main/process-manager/handlers/StdoutHandler.ts (1)
466-470: Consider using structured logger instead of console.log.The codebase uses
logger.debugfor structured logging (e.g., lines 442-450), but these debug statements useconsole.log. For consistency and production observability, consider converting tologger.debugor removing if these are temporary debugging artifacts.♻️ Suggested change
- // DEBUG: Log usage extracted from parser - console.log('[StdoutHandler] Usage from parser (line 255 path)', { - sessionId, - toolType: managedProcess.toolType, - parsedUsage: usage, - }); + logger.debug('[ProcessManager] Usage extracted from parser', 'ProcessManager', { + sessionId, + toolType: managedProcess.toolType, + parsedUsage: usage, + });Also applies to lines 488-491, 681-686, 694-698.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/process-manager/handlers/StdoutHandler.ts` around lines 466 - 470, Replace ad-hoc console.log calls in StdoutHandler (e.g., the debug at "Usage from parser (line 255 path)" that logs sessionId, managedProcess.toolType, parsedUsage) with the module's structured logger.debug calls used elsewhere; ensure you call logger.debug with the same payload keys (sessionId, toolType, parsedUsage) and a clear message, and update the other similar console.log occurrences referenced (around the other noted lines) to use logger.debug or remove them if they were temporary; maintain existing log level and formatting consistent with the nearby logger.debug statements.
🤖 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 `@src/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 549-565: The Claude Code toolUseBlocks path adds tool IDs to
emittedToolCallIds (via getEmittedToolCallIds(managedProcess)) but never removes
them, causing unbounded growth; update the StdoutHandler handling of
event.toolUseBlocks (the loop that calls this.emitter.emit('tool-execution',
...)) to clear the ID after the corresponding result is emitted (or immediately
after emit if Claude never sends completion), e.g., call
emittedToolCallIds.delete(tool.id) at the appropriate place or replace the
emittedToolCallIds store with a bounded/LRU cache implementation referenced by
getEmittedToolCallIds to cap growth.
---
Nitpick comments:
In `@src/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 466-470: Replace ad-hoc console.log calls in StdoutHandler (e.g.,
the debug at "Usage from parser (line 255 path)" that logs sessionId,
managedProcess.toolType, parsedUsage) with the module's structured logger.debug
calls used elsewhere; ensure you call logger.debug with the same payload keys
(sessionId, toolType, parsedUsage) and a clear message, and update the other
similar console.log occurrences referenced (around the other noted lines) to use
logger.debug or remove them if they were temporary; maintain existing log level
and formatting consistent with the nearby logger.debug statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d03ec43-b1f4-48b6-841c-e16d3a3f7555
📒 Files selected for processing (1)
src/main/process-manager/handlers/StdoutHandler.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main/storage/copilot-session-storage.ts`:
- Around line 178-187: The function buildToolUse currently returns an empty
array when all toolRequests are filtered out, which is truthy and leads to
creating meaningless assistant messages; change buildToolUse (and the analogous
implementation at the other occurrence around lines 229-239) to first compute
the mapped result, then return undefined if that result.length === 0, otherwise
return the result (i.e., avoid returning []). Update references to buildToolUse
so callers only see a defined value when there are actual tool entries.
- Around line 287-302: The try-catch in getLocalDirectorySize currently swallows
all errors and returns 0; change it to only return 0 for expected/benign errors
(e.g., ENOENT/NOT_FOUND) and for any other unexpected I/O failures call the
project's Sentry utility (e.g., captureException or reportError) with contextual
details (sessionDir, entryPath, function name) and then rethrow the error (or
propagate it) so failures are observable; apply the same pattern to the
analogous catch blocks at the other locations mentioned (around lines 430-435
and 539-545) so unexpected permission/read/path errors are reported rather than
silently degraded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6458a1b5-e270-4e64-a4c1-aa9ca212c335
📒 Files selected for processing (1)
src/main/storage/copilot-session-storage.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main/storage/copilot-session-storage.ts`:
- Around line 543-557: readEventsFile currently swallows all errors; change its
catch to mirror other functions: catch the error, if it represents a
missing/unreadable file (e.g., error.code === 'ENOENT' or equivalent remote "not
found" condition from readRemoteFile) return null, otherwise report the
exception to Sentry (captureException/reporting helper used elsewhere) and
rethrow the error so it bubbles to Sentry/upper layers; keep references to
getEventsPath and readRemoteFile when locating the logic to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f029b5a-7496-4a01-9015-5aab3f628c54
📒 Files selected for processing (2)
src/main/agents/path-prober.tssrc/main/storage/copilot-session-storage.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/agents/path-prober.ts
Author
|
Good to go! I have a follow up PR that will add some additional enhancements depending on if this is accepted. |
Contributor
|
Whats the enhancement over using OpenCode with Github Copilot? |
Author
The Github Copilot CLI has its own set of features and capabilities. You should give it a try if you're using copilot with OpenCode! https://github.com/github/copilot-cli With that maintainability argument, you could say why not run everything through OpenCode instead of Claude Code, Codex CLI, or Gemini CLI. 😄 Github Copilot CLI runs in the same manner as other implemented CLI agents, so it should be modular in nature and not adding any additional overhead. IMO the value of Maestro is bringing together multiple agents, each bringing their own capabilities beyond the model access, and GH CLI falls into that bucket. |
Contributor
|
Didnt mean to sound like i don't want to implement it, you are right to say that with OpenCode we could just implement everything over that. But i remember there was something with GitHub CLI that didn't fit into the way Maestro harnesses other CLI tools. But i can't remember what it exactly it was. Maybe you figured something i did not. |
Contributor
|
@greptileai re-review |
Author
No problem, I totally get it! There's been a TON of improvements to Copilot CLI over the last few months, totally could have had gaps initially that have now been filled. I missed the question on "What is the follow up introducing?". The follow up includes support for referencing images and better UX for insights on what GHCLI is doing while it's working. I want to also add support for the slash commands - which is where some of the additional functionality/features of the CLI are going to become relevant. This PR is really "get the basics in place" and follow up feature PRs will slowly turn on the applicable capabilities. This PR was already getting very large, so I thought having follow-up feature PRs will be easiest to digest. |
Contributor
|
I'll checkout Github CLI and pull the PR :) |
Collaborator
|
Hey @nolanmclark — thanks for the contribution! This is a thorough, well-structured integration that follows existing patterns correctly. The parser factory refactor, concatenated JSON handling with overflow protection, and tool call deduplication are particularly well done. A few gaps we'd like addressed before merge: Functional Gaps
Process
Thanks again for the solid work 🎵 |
nolanmclark
added a commit
to nolanmclark/Maestro
that referenced
this pull request
Mar 17, 2026
…sage stats, SSH docs - Add token_exhaustion error patterns for Copilot (7 patterns matching prompt-too-long, context-exceeded, token-limit, etc.) - Extract live usage stats from session.shutdown modelMetrics in parser (emits usage events for the context bar) - Extend CopilotRawMessage with modelMetrics type for session.shutdown - Set supportsUsageStats: true in Copilot capabilities - Document SSH interactive mode limitation in CLAUDE-AGENTS.md Read-only mode (--allow-tool/--deny-tool enforcement) and group chat support were already addressed in the feat/gh-cli-improvements merge.
Author
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/utils/ssh-command-builder.ts (1)
379-397:⚠️ Potential issue | 🟠 MajorRedact
stdinScriptpreviews for image uploads.This addition routes Copilot images through the existing heredoc path. Lines 394-395 place raw base64 into
stdinScript, and Line 469-480 still logs ascriptPreviewat info level, so remote Copilot image uploads can leak prompt text and image bytes into logs.Proposed fix
+ const hasSensitiveScriptContent = + Boolean(remoteOptions.stdinInput) || + Boolean(remoteOptions.prompt) || + allRemoteTempPaths.length > 0; + logger.info('SSH command built with stdin script', '[ssh-command-builder]', { host: config.host, username: config.username || '(using SSH config/system default)', port: config.port, @@ - scriptPreview: stdinScript.length > 500 ? stdinScript.substring(0, 500) + '...' : stdinScript, + scriptPreview: hasSensitiveScriptContent + ? '[redacted]' + : stdinScript.length > 500 + ? stdinScript.substring(0, 500) + '...' + : stdinScript, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/utils/ssh-command-builder.ts` around lines 379 - 397, The code currently pushes raw image base64 (parsed.base64) into scriptLines and also leaves it available in stdinScript and scriptPreview, which can leak image bytes and prompt text; update the image handling in the remoteOptions.images branch so image data is only written into the heredoc scriptLines (and not appended to stdinScript or included in scriptPreview), ensure all references to parsed.base64 are removed from any stdinScript construction and that scriptPreview generation (wherever scriptPreview or stdinScript is assembled) redacts or omits entries corresponding to remoteOptions.images, and keep tracking of paths via allRemoteTempPaths and the heredoc markers (MAESTRO_IMG_*_EOF) in place so image uploads still use the heredoc/base64-decode flow without logging sensitive content.
🧹 Nitpick comments (5)
src/__tests__/renderer/components/TerminalOutput.test.tsx (1)
1701-1728: Scope failed-icon assertions to the specific log row to reduce brittleness.Line 1726 and Line 1727 currently assert glyphs globally. Scoping to the
Command failedrow will make this test less fragile if other UI text/icons include!or×.Suggested test hardening
-import { render, screen, fireEvent, waitFor, act } from '@testing-library/react'; +import { render, screen, fireEvent, waitFor, act, within } from '@testing-library/react'; ... - expect(screen.getByText('!')).toBeInTheDocument(); - expect(screen.queryByText('×')).not.toBeInTheDocument(); + const failedRow = screen.getByText('Command failed').closest('[data-log-index]'); + expect(failedRow).not.toBeNull(); + expect(within(failedRow as HTMLElement).getByText('!')).toBeInTheDocument(); + expect(within(failedRow as HTMLElement).queryByText('×')).not.toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/TerminalOutput.test.tsx` around lines 1701 - 1728, The assertions for the failed-icon are global and brittle; instead scope them to the specific log row for the "Command failed" entry: locate the row containing the text (e.g., via screen.getByText('Command failed') and its parent/closest row element), import and use within(...) from `@testing-library/react`, then call within(row).getByText('!') and within(row).queryByText('×') so the checks apply only to the TerminalOutput row created by createLogEntry / createDefaultSession / createDefaultProps.src/__tests__/renderer/hooks/useAgentListeners.test.ts (1)
900-905: Extract duplicated RAF stubbing into a shared helper.These two tests duplicate the same global RAF setup. A small helper keeps setup consistent and reduces maintenance noise.
♻️ Minimal cleanup
@@ function createMockDeps(overrides: Partial<UseAgentListenersDeps> = {}): UseAgentListenersDeps { @@ } + +function stubImmediateRaf() { + vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { + callback(0); + return 1; + }); + vi.stubGlobal('cancelAnimationFrame', vi.fn()); +} @@ it('shows lightweight progress when thinking is hidden', async () => { - vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { - callback(0); - return 1; - }); - vi.stubGlobal('cancelAnimationFrame', vi.fn()); + stubImmediateRaf(); @@ it('keeps active tool progress visible while thinking stays hidden', async () => { - vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => { - callback(0); - return 1; - }); - vi.stubGlobal('cancelAnimationFrame', vi.fn()); + stubImmediateRaf();Also applies to: 932-937
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useAgentListeners.test.ts` around lines 900 - 905, Extract the duplicated requestAnimationFrame stubbing into a small helper (e.g., stubRequestAnimationFrame) and use it from both tests ('shows lightweight progress when thinking is hidden' and the other test with identical stubbing) instead of repeating vi.stubGlobal calls; the helper should call vi.stubGlobal('requestAnimationFrame', (cb) => { cb(0); return 1; }) and vi.stubGlobal('cancelAnimationFrame', vi.fn()), be importable or defined near the top of the test file (or in a shared test utils module), and ensure any teardown/restoration is handled (use vi.restoreAllMocks or explicit restores in afterEach) so the global stubs do not leak between tests.src/__tests__/main/process-manager/handlers/StdoutHandler.test.ts (1)
358-568: Please add one Copilotusagecase here.This block covers most Copilot event types, but not the new
session.shutdown/modelMetricsusage path from this PR. A single handler-level assertion on theusageevent would keep the parser→StdoutHandlerwiring from regressing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/process-manager/handlers/StdoutHandler.test.ts` around lines 358 - 568, Add a test that sends a Copilot session.shutdown payload (via handler.handleData with a JSON string of type 'session.shutdown' containing modelMetrics/usage) and assert the StdoutHandler emits a 'usage' event; use createTestContext with CopilotOutputParser, register a spy on emitter.on('usage', ...), call handler.handleData(sessionId, JSON.stringify({type: 'session.shutdown', data: { modelMetrics: {/* sample */}, usage: { totalTokens: 123, promptTokens: 45, completionTokens: 78 } }})), and expect the usage spy to have been called with the sessionId and the expected usage object to prevent regression in the parser→StdoutHandler wiring.src/__tests__/renderer/services/inlineWizardConversation.test.ts (1)
285-316: Make this test cover Copilot’s actual read-only path.The mock omits Copilot’s
batchModeArgsandjsonOutputArgs, so this stays green even if--allow-all-toolssurvives read-only mode or JSON output stops being requested.Proposed test hardening
const mockAgent = { id: 'copilot', available: true, command: 'copilot', args: [], + batchModeArgs: ['--allow-all-tools', '--silent'], + jsonOutputArgs: ['--output-format', 'json'], readOnlyArgs: [ '--allow-tool=read,url', '--deny-tool=write,shell,memory,github', '--no-ask-user', ], @@ expect(spawnCall.args).toEqual( expect.arrayContaining([ '--allow-tool=read,url', '--deny-tool=write,shell,memory,github', '--no-ask-user', ]) ); + expect(spawnCall.args).not.toContain('--allow-all-tools'); + expect(spawnCall.args).toEqual( + expect.arrayContaining(['--output-format', 'json']) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/services/inlineWizardConversation.test.ts` around lines 285 - 316, The test currently only stubs mockAgent.readOnlyArgs so it can miss Copilot's batchModeArgs and jsonOutputArgs; update the test's mockAgent to include the real Copilot properties batchModeArgs and jsonOutputArgs (e.g., mockAgent.batchModeArgs and mockAgent.jsonOutputArgs) and change the assertion on spawnCall.args to expect.arrayContaining([...mockAgent.readOnlyArgs, ...mockAgent.batchModeArgs, ...mockAgent.jsonOutputArgs]) after calling startInlineWizardConversation/sendWizardMessage so the test covers Copilot's true read-only + batch + JSON output argument set.src/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.ts (1)
199-212: Add one more resume test for split-arg form (--resume <id>).This test locks
--resume=<id>only; adding the split variant would protect both supported forms.Suggested test addition
+ it('treats --resume <id> as a resumed session when building env', () => { + const { spawner } = createTestContext(); + + spawner.spawn( + createBaseConfig({ + toolType: 'copilot', + command: 'copilot', + args: ['--output-format', 'json', '--resume', 'session-123'], + prompt: 'continue', + }) + ); + + expect(buildChildProcessEnv).toHaveBeenCalledWith(undefined, true, undefined); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.ts` around lines 199 - 212, Add a new test alongside the existing "treats --resume=<id> as a resumed session when building env" case to cover the split-arg form `--resume <id>`: in the same test file (ChildProcessSpawner.test.ts) use createTestContext() and call spawner.spawn with createBaseConfig where args are ['--output-format', 'json', '--resume', 'session-123'] (instead of the combined '--resume=session-123'), then assert buildChildProcessEnv was called with the same parameters (undefined, true, undefined); this ensures the spawn/parse logic in spawn/createBaseConfig handles both '--resume=<id>' and '--resume', '<id>' forms.
🤖 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/__tests__/main/agents/session-storage.test.ts`:
- Around line 375-407: The tests rely on os.homedir() fixtures but don't account
for process.env.COPILOT_CONFIG_DIR; modify the test suite setup to unset
COPILOT_CONFIG_DIR during the suite: in beforeEach save the original value
(e.g., const _origCopilotConfigDir = process.env.COPILOT_CONFIG_DIR), then
delete process.env.COPILOT_CONFIG_DIR (or set to undefined) before creating
fixtures (so CopilotSessionStorage will use homedir-based path), and in
afterEach restore the original value (process.env.COPILOT_CONFIG_DIR =
_origCopilotConfigDir) to avoid leaking environment changes; apply this around
the existing beforeEach/afterEach that remove /tmp/maestro-session-storage-home
and reference the session fixture helpers like writeCopilotSessionFixture and
CopilotSessionStorage.
In `@src/__tests__/main/process-manager/handlers/StdoutHandler.test.ts`:
- Around line 44-48: The test-wide mock that sets matchErrorPattern to always
return null prevents exercising the Copilot token_exhaustion error path; remove
or change the mock in StdoutHandler.test.ts so matchErrorPattern uses the real
implementation (e.g., use vi.importActual or avoid mocking matchErrorPattern)
while you may still stub getErrorPatterns or matchSshErrorPattern, or
alternatively add a focused test case that does not neutralize matchErrorPattern
and triggers the token_exhaustion pattern so the StdoutHandler's pattern-based
error handling is exercised.
In `@src/__tests__/renderer/hooks/useAgentListeners.test.ts`:
- Around line 421-471: The test in useAgentListeners.test that checks log
removal after data resumes is too permissive by asserting aiTabs[0].logs equals
[] — change the setup to include an extra non-target log entry (e.g., a log with
id 'log-kept' and source not 'error' or not tied to the recoveredError) in the
createMockTab logs, then update the assertions to verify that the
recovered-error/hidden-progress entries are removed while the seeded non-target
log remains (assert updated?.aiTabs[0]?.logs contains the kept log and does not
contain the recovered log); keep references to useAgentListeners, the
recoveredError object, and window.maestro.agentError.clearError in the test so
behavior is still validated.
In `@src/main/preload/agents.ts`:
- Around line 34-40: The preload capability interface in agents.ts is missing
the imageResumeMode property introduced in src/main/agents/capabilities.ts;
update the preload contract used by window.maestro.agents.getCapabilities() to
include imageResumeMode?: boolean alongside the existing fields (e.g.,
supportsThinkingDisplay, supportsContextMerge, usesJsonLineOutput,
usesCombinedContextWindow) so renderer callers can type-check resume-image
behavior against the IPC payload.
In `@src/main/process-manager/spawners/ChildProcessSpawner.ts`:
- Around line 217-219: The resume-detection currently uses finalArgs which may
include prompt-driven tokens and yield false positives; update the isResuming
computation in ChildProcessSpawner (the const isResuming declaration) to inspect
the original raw args array (args) instead of finalArgs, i.e., check
args.some(arg => arg === '--resume' || arg.startsWith('--resume=')) ||
args.includes('--session'), so resume is only detected from the actual CLI
inputs rather than any prompt-injected tokens.
In `@src/main/storage/copilot-session-storage.ts`:
- Around line 426-445: listSessionIds currently treats any readDirRemote failure
as an empty directory, which swallows SSH errors; update listSessionIds (and
analogous remote helpers like readFileRemote and directorySizeRemote used by
readEventsFile/loadSessionInfo) so that when readDirRemote returns
result.success === false you only return [] for genuine not-found/unreadable
conditions (map result.error.code for remote equivalents of
ENOENT/EACCES/EPERM), but for other remote errors call captureException(...)
with operation metadata and rethrow the error (or return a rejected Promise) so
Sentry sees transport/auth failures instead of silently returning empty results;
reference the readDirRemote call inside listSessionIds and similarly adjust
readRemoteFile/readFileRemote and directorySizeRemote call sites used by
readEventsFile and loadSessionInfo.
- Around line 367-399: Both read paths must re-check project ownership before
returning session data: call the existing matchesProject(...) guard inside
readSessionMessages and getSearchableMessages (using the same identifiers passed
in, e.g., _projectPath and sessionId and sshConfig) and if it returns false
return the empty/default result ({ messages: [], total: 0, hasMore: false } for
readSessionMessages and [] for getSearchableMessages); add a regression test in
session-storage.test.ts that asserts a session ID from another project does not
return messages to lock this behavior.
---
Outside diff comments:
In `@src/main/utils/ssh-command-builder.ts`:
- Around line 379-397: The code currently pushes raw image base64
(parsed.base64) into scriptLines and also leaves it available in stdinScript and
scriptPreview, which can leak image bytes and prompt text; update the image
handling in the remoteOptions.images branch so image data is only written into
the heredoc scriptLines (and not appended to stdinScript or included in
scriptPreview), ensure all references to parsed.base64 are removed from any
stdinScript construction and that scriptPreview generation (wherever
scriptPreview or stdinScript is assembled) redacts or omits entries
corresponding to remoteOptions.images, and keep tracking of paths via
allRemoteTempPaths and the heredoc markers (MAESTRO_IMG_*_EOF) in place so image
uploads still use the heredoc/base64-decode flow without logging sensitive
content.
---
Nitpick comments:
In `@src/__tests__/main/process-manager/handlers/StdoutHandler.test.ts`:
- Around line 358-568: Add a test that sends a Copilot session.shutdown payload
(via handler.handleData with a JSON string of type 'session.shutdown' containing
modelMetrics/usage) and assert the StdoutHandler emits a 'usage' event; use
createTestContext with CopilotOutputParser, register a spy on
emitter.on('usage', ...), call handler.handleData(sessionId,
JSON.stringify({type: 'session.shutdown', data: { modelMetrics: {/* sample */},
usage: { totalTokens: 123, promptTokens: 45, completionTokens: 78 } }})), and
expect the usage spy to have been called with the sessionId and the expected
usage object to prevent regression in the parser→StdoutHandler wiring.
In `@src/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.ts`:
- Around line 199-212: Add a new test alongside the existing "treats
--resume=<id> as a resumed session when building env" case to cover the
split-arg form `--resume <id>`: in the same test file
(ChildProcessSpawner.test.ts) use createTestContext() and call spawner.spawn
with createBaseConfig where args are ['--output-format', 'json', '--resume',
'session-123'] (instead of the combined '--resume=session-123'), then assert
buildChildProcessEnv was called with the same parameters (undefined, true,
undefined); this ensures the spawn/parse logic in spawn/createBaseConfig handles
both '--resume=<id>' and '--resume', '<id>' forms.
In `@src/__tests__/renderer/components/TerminalOutput.test.tsx`:
- Around line 1701-1728: The assertions for the failed-icon are global and
brittle; instead scope them to the specific log row for the "Command failed"
entry: locate the row containing the text (e.g., via screen.getByText('Command
failed') and its parent/closest row element), import and use within(...) from
`@testing-library/react`, then call within(row).getByText('!') and
within(row).queryByText('×') so the checks apply only to the TerminalOutput row
created by createLogEntry / createDefaultSession / createDefaultProps.
In `@src/__tests__/renderer/hooks/useAgentListeners.test.ts`:
- Around line 900-905: Extract the duplicated requestAnimationFrame stubbing
into a small helper (e.g., stubRequestAnimationFrame) and use it from both tests
('shows lightweight progress when thinking is hidden' and the other test with
identical stubbing) instead of repeating vi.stubGlobal calls; the helper should
call vi.stubGlobal('requestAnimationFrame', (cb) => { cb(0); return 1; }) and
vi.stubGlobal('cancelAnimationFrame', vi.fn()), be importable or defined near
the top of the test file (or in a shared test utils module), and ensure any
teardown/restoration is handled (use vi.restoreAllMocks or explicit restores in
afterEach) so the global stubs do not leak between tests.
In `@src/__tests__/renderer/services/inlineWizardConversation.test.ts`:
- Around line 285-316: The test currently only stubs mockAgent.readOnlyArgs so
it can miss Copilot's batchModeArgs and jsonOutputArgs; update the test's
mockAgent to include the real Copilot properties batchModeArgs and
jsonOutputArgs (e.g., mockAgent.batchModeArgs and mockAgent.jsonOutputArgs) and
change the assertion on spawnCall.args to
expect.arrayContaining([...mockAgent.readOnlyArgs, ...mockAgent.batchModeArgs,
...mockAgent.jsonOutputArgs]) after calling
startInlineWizardConversation/sendWizardMessage so the test covers Copilot's
true read-only + batch + JSON output argument set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d638915-ce08-4905-9fd3-0a835f6d2938
📒 Files selected for processing (28)
CLAUDE-AGENTS.mdCLAUDE.mdCONTRIBUTING.mdsrc/__tests__/main/agents/capabilities.test.tssrc/__tests__/main/agents/definitions.test.tssrc/__tests__/main/agents/session-storage.test.tssrc/__tests__/main/ipc/handlers/agents.test.tssrc/__tests__/main/parsers/copilot-output-parser.test.tssrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/__tests__/main/utils/ssh-command-builder.test.tssrc/__tests__/renderer/components/TerminalOutput.test.tsxsrc/__tests__/renderer/hooks/useAgentListeners.test.tssrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/__tests__/renderer/services/inlineWizardConversation.test.tssrc/cli/services/agent-spawner.tssrc/main/agents/capabilities.tssrc/main/agents/definitions.tssrc/main/ipc/handlers/agents.tssrc/main/ipc/handlers/process.tssrc/main/parsers/copilot-output-parser.tssrc/main/parsers/error-patterns.tssrc/main/preload/agents.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/process-manager/spawners/PtySpawner.tssrc/main/process-manager/types.tssrc/main/storage/copilot-session-storage.tssrc/main/utils/ssh-command-builder.ts
✅ Files skipped from review due to trivial changes (3)
- src/cli/services/agent-spawner.ts
- CONTRIBUTING.md
- src/tests/main/ipc/handlers/agents.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/process-manager/types.ts
- src/tests/main/parsers/copilot-output-parser.test.ts
- src/main/parsers/error-patterns.ts
- src/main/process-manager/spawners/PtySpawner.ts
- src/main/parsers/copilot-output-parser.ts
- CLAUDE.md
Author
|
@coderabbitai - responded to all comments and resolved issues. Review again. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/storage/copilot-session-storage.ts (2)
457-462: SSH remote errors are silently flattened to empty results.When
readDirRemote()fails for any reason (auth failure, network error, SSH timeout), the code returns an empty array, making SSH regressions appear as "no Copilot sessions" in the UI.Consider distinguishing recoverable "not found" errors from unexpected failures to improve debuggability:
🔧 Suggested improvement
if (sshConfig) { const result = await readDirRemote(sessionStateDir, sshConfig); if (!result.success || !result.data) { + // Log remote failures for debugging - don't report expected "not found" cases + if (result.error && !result.error.includes('No such file or directory')) { + logger.warn('Failed to list remote Copilot session directory', LOG_CONTEXT, { + error: result.error, + sessionStateDir, + }); + } return []; } return result.data.filter((entry) => entry.isDirectory).map((entry) => entry.name); }This follows the same hardening pattern applied to the local filesystem branches. As per coding guidelines: "Do not silently swallow errors with try-catch blocks that only log. Let exceptions bubble up to Sentry for error tracking in production."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-session-storage.ts` around lines 457 - 462, The current SSH branch in copilot-session-storage (when sshConfig is truthy) swallows all remote errors by returning [] when readDirRemote(sessionStateDir, sshConfig) yields result.success === false; update the logic in the block that calls readDirRemote so that it only returns an empty array for a clear "not found" / directory-missing condition and otherwise throws or propagates the error (or wraps it with context) so failures (auth, network, timeout) surface to Sentry; specifically modify the code handling the Promise result from readDirRemote to inspect error details on failure and rethrow unexpected errors instead of returning [] while still returning [] only for an explicit not-found condition.
594-609: Remote file/directory operations silently swallow all errors.
readRemoteFile()andgetRemoteDirectorySize()return safe defaults (null/0) for all failures, including SSH auth errors, network issues, and malformed responses. This makes it difficult to diagnose remote connectivity problems.Consider adding observability for unexpected remote failures:
🔧 Suggested improvement
private async readRemoteFile( filePath: string, sshConfig: SshRemoteConfig ): Promise<string | null> { const result = await readFileRemote(filePath, sshConfig); + if (!result.success && result.error && !result.error.includes('No such file or directory')) { + logger.debug('Failed to read remote file', LOG_CONTEXT, { filePath, error: result.error }); + } return result.success && result.data ? result.data : null; } private async getRemoteDirectorySize( sessionDir: string, sshConfig: SshRemoteConfig ): Promise<number> { const result = await directorySizeRemote(sessionDir, sshConfig); + if (!result.success && result.error && !result.error.includes('No such file or directory')) { + logger.debug('Failed to get remote directory size', LOG_CONTEXT, { sessionDir, error: result.error }); + } return result.success && result.data ? result.data : 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-session-storage.ts` around lines 594 - 609, Both readRemoteFile and getRemoteDirectorySize currently swallow all remote call failures; update them to surface unexpected failures by checking the returned result and, when result.success is false or result.data is missing, log the error details (including SSH target info like sshConfig.host and any result.error/exception) via the module logger (e.g., processLogger or the file's logger) and then rethrow or return a distinct failure (throw for connectivity/auth errors, keep safe defaults only for explicit "not found" cases). Specifically modify readRemoteFile and getRemoteDirectorySize to inspect directorySizeRemote/readFileRemote responses, emit structured/loggable error messages with identifying info, and only suppress errors after confirming the failure is expected (e.g., file-not-found).
🤖 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/main/storage/copilot-session-storage.ts`:
- Around line 457-462: The current SSH branch in copilot-session-storage (when
sshConfig is truthy) swallows all remote errors by returning [] when
readDirRemote(sessionStateDir, sshConfig) yields result.success === false;
update the logic in the block that calls readDirRemote so that it only returns
an empty array for a clear "not found" / directory-missing condition and
otherwise throws or propagates the error (or wraps it with context) so failures
(auth, network, timeout) surface to Sentry; specifically modify the code
handling the Promise result from readDirRemote to inspect error details on
failure and rethrow unexpected errors instead of returning [] while still
returning [] only for an explicit not-found condition.
- Around line 594-609: Both readRemoteFile and getRemoteDirectorySize currently
swallow all remote call failures; update them to surface unexpected failures by
checking the returned result and, when result.success is false or result.data is
missing, log the error details (including SSH target info like sshConfig.host
and any result.error/exception) via the module logger (e.g., processLogger or
the file's logger) and then rethrow or return a distinct failure (throw for
connectivity/auth errors, keep safe defaults only for explicit "not found"
cases). Specifically modify readRemoteFile and getRemoteDirectorySize to inspect
directorySizeRemote/readFileRemote responses, emit structured/loggable error
messages with identifying info, and only suppress errors after confirming the
failure is expected (e.g., file-not-found).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cdcfa97a-b4cd-4fe3-85e8-e0fd0f121e3c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/__tests__/main/agents/session-storage.test.tssrc/main/preload/agents.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/storage/copilot-session-storage.ts
Author
|
Thanks @pedramamini! All items addressed:
|
Author
|
@coderabbitai everything addressed, re-review now |
|
✅ Actions performedReview triggered.
|
nolanmclark
force-pushed
the
feat/github-copilot-cli-support
branch
from
c20e887 to
ba2412a
Compare
Add GitHub Copilot as a beta agent in Maestro with end-to-end integration: - Agent definitions, capabilities, metadata, icon, and default context window - Dedicated Copilot output parser for --output-format json - Copilot session storage backed by ~/.copilot/session-state - Copilot-specific error pattern handling - Process handling for streamed/concatenated JSON events and session ID extraction - Path probing for Copilot installs on macOS and Windows - Updated onboarding/agent selection UI to include GitHub Copilot - Updated docs and agent reference tables Supports interactive use, batch prompts, JSON output, and session resume. Session history works for both local and SSH-backed environments.
nolanmclark
force-pushed
the
feat/github-copilot-cli-support
branch
from
ba2412a to
9207232
Compare
Skip repeated Copilot message and reasoning summaries once deltas have already streamed, keep reasoning out of the final buffered response, and treat Copilot result events as completion markers so final output is emitted exactly once. Session: 1b292e6b-7c6c-4499-8f00-7f328149077d Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/__tests__/main/agents/detector.test.ts (1)
281-293: Derive the expected count from the shared agent list.These literals will need manual updates every time
AGENT_DEFINITIONSchanges, even when detection logic is still correct. Pulling the expected total from the exported agent source of truth will make the test less brittle.Also applies to: 928-929
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/agents/detector.test.ts` around lines 281 - 293, Replace the hard-coded expected agent count and brittle per-id expects by deriving the expected total and expected IDs from the shared source-of-truth export (AGENT_DEFINITIONS) used elsewhere in the codebase: import AGENT_DEFINITIONS into the test, use AGENT_DEFINITIONS.length for the expect(agents.length) assertion, and replace the multiple expect(agentIds).toContain(...) checks with a comparison that every id in AGENT_DEFINITIONS (e.g., AGENT_DEFINITIONS.map(d => d.id) or Object.keys(AGENT_DEFINITIONS) depending on its shape) is present in agentIds; apply the same change to the other occurrence referenced in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Around line 979-984: Replace the hard-coded example version strings ("0.1.0"
in the package.json snippet and "v0.1.0" used elsewhere) in CONTRIBUTING.md with
a generic placeholder (e.g., v0.MINOR.PATCH) and add a concise branch-specific
example reflecting the repo's odd/even release flow; locate the package.json
example and the textual tag examples (the "0.1.0" value and any "v0.1.0"
occurrences) and update them to use the placeholder plus a one-line example
showing how to form a tag for a release branch (for example: main -> vX.Y.Z or
release/X -> vX.Y.Z-RC) so examples remain evergreen and consistent with the
documented flow.
In `@src/main/ipc/handlers/agents.ts`:
- Around line 56-58: discoverCopilotSlashCommands() currently returns string[]
(spreading COPILOT_BUILTIN_COMMANDS) but the renderer expects objects with
cmd.name and cmd.prompt; update discoverCopilotSlashCommands() to return the
same payload shape as other agents (e.g., array of objects containing name and
prompt derived from each COPILOT_BUILTIN_COMMAND entry) so the renderer’s
accessors (cmd.name, cmd.prompt, and name.startsWith) work unchanged; ensure
COPILOT_BUILTIN_COMMANDS mapping logic is applied inside
discoverCopilotSlashCommands() (or alternatively update the preload types and
renderer mapping in tandem) and keep the exported symbol
discoverCopilotSlashCommands() signature consistent with other discovery
handlers.
In `@src/main/process-manager/spawners/ChildProcessSpawner.ts`:
- Around line 347-357: The JSON-detection logic (argsContain, argsHaveFlagValue,
and isStreamJsonMode) is checking finalArgs which may include the user prompt
and cause false positives; update these checks to inspect the
original/unmodified CLI arguments array (the raw args passed into the spawner)
instead of finalArgs so prompts like "Explain --json" won't trigger JSON mode.
Locate the isStreamJsonMode computation in ChildProcessSpawner (the argsContain
and argsHaveFlagValue helpers) and replace finalArgs with the original args
variable (the unmodified input args/CLI array used to spawn the process) in
those helper calls.
In `@src/main/storage/copilot-session-storage.ts`:
- Around line 435-451: In sessionMatchesProject, make the check fail-closed:
treat missing/unreadable metadata or a null result from readRemoteFile as a
non-match (return false) instead of true. Concretely, after calling
getWorkspacePath and reading the file (readRemoteFile or fs.readFile), if
workspaceContent is falsy or readRemoteFile returned null return false; wrap
parseWorkspaceMetadata in a try/catch and return false on parse errors; only
rethrow truly unexpected exceptions from the outer try so Sentry can capture
them. Ensure references to sessionMatchesProject, getWorkspacePath,
readRemoteFile, fs.readFile, parseWorkspaceMetadata, and matchesProject are
updated accordingly.
- Around line 152-160: The normalizePath function currently strips trailing
slashes which turns the POSIX root "/" into an empty string; modify
normalizePath to return "/" when the input (after replacing backslashes) is
exactly "/" so the lone root is preserved, and similarly ensure Windows roots
like "C:/" are preserved (case-fold after checking root) — update the logic in
normalizePath and apply the same guard to the related comparison helper used by
matchesProject so that a single root path is not treated as “no project filter.”
- Around line 621-623: The checks treat result.data truthily so empty string
('') and zero (0) are treated as failures; change the guards in the
readFileRemote call (the block using readFileRemote(filePath, sshConfig) and the
analogous block at the other occurrence around lines 638-640) to treat any
defined data as success: return the payload when result.success is true and
result.data !== undefined (and !== null if needed) instead of checking
truthiness, and only call this.isExpectedRemoteError when result.error is
actually present (error !== undefined && error !== null); apply the same
explicit-null/undefined logic to the other helper call in the same file.
In `@src/renderer/components/TerminalOutput.tsx`:
- Around line 575-619: The memoized LogItemComponent comparator currently only
compares text fields so changes to log.metadata.hiddenProgress and
log.metadata.toolState.status don't trigger re-renders; update the comparator
used with LogItemComponent (the memo equality function) to also compare
log.metadata.hiddenProgress (or the whole hiddenProgress object) and
log.metadata.toolState.status (or toolState.status), or alternatively include
log.metadata (or the minimal derived status fields) in the props passed to the
component so state/status flips cause re-rendering.
In `@src/renderer/components/Wizard/services/conversationManager.ts`:
- Around line 793-794: The onboarding wizard path is missing Copilot's read-only
CLI args: update buildArgsForAgent to detect agent === 'copilot' (or the copilot
branch) and, when onboardingWizard (or the onboarding flow flag used by the
parser) is true, set agent.readOnlyArgs to the same read-only flags used by the
parser/wizard path so Copilot runs in read-only mode; mirror this change in the
other similar branch noted around the second occurrence (the block referenced at
851-864) so both places pass the read-only args to Copilot instead of falling
through to the default branch.
In `@src/renderer/hooks/agent/useAgentListeners.ts`:
- Around line 122-196: The formatter formatToolProgressText currently only
treats status === 'failed' as a failure; update it to also treat status ===
'error' (e.g., change checks from status === 'failed' to status === 'failed' ||
status === 'error') so error states produce the same failure messages and icons;
ensure any other status comparisons that should treat 'error' like 'failed' are
updated here and mirror the change in TerminalOutput.tsx (the same status checks
that decide failure icons) so both the text and icon reflect error states.
In `@src/renderer/types/index.ts`:
- Line 623: The Session.agentCommands shape was changed to remove the prompt
field but consumers (useAgentListeners.onSlashCommands,
src/renderer/stores/agentStore.ts and useInputProcessing) still expect
session.agentCommands[*].prompt; restore backward compatibility by adding
prompt?: string to the agentCommands type declaration (the agentCommands?: {
command: string; description: string }[] definition) so the Session type becomes
agentCommands?: { command: string; description: string; prompt?: string }[];
this preserves the existing contract until those consumers are migrated.
---
Nitpick comments:
In `@src/__tests__/main/agents/detector.test.ts`:
- Around line 281-293: Replace the hard-coded expected agent count and brittle
per-id expects by deriving the expected total and expected IDs from the shared
source-of-truth export (AGENT_DEFINITIONS) used elsewhere in the codebase:
import AGENT_DEFINITIONS into the test, use AGENT_DEFINITIONS.length for the
expect(agents.length) assertion, and replace the multiple
expect(agentIds).toContain(...) checks with a comparison that every id in
AGENT_DEFINITIONS (e.g., AGENT_DEFINITIONS.map(d => d.id) or
Object.keys(AGENT_DEFINITIONS) depending on its shape) is present in agentIds;
apply the same change to the other occurrence referenced in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51d4bc61-6db5-4c39-ac3e-50c76bbf50ce
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
CLAUDE-AGENTS.mdCLAUDE.mdCONTRIBUTING.mdREADME.mddocs/installation.mddocs/releases.mdsrc/__tests__/main/agents/capabilities.test.tssrc/__tests__/main/agents/definitions.test.tssrc/__tests__/main/agents/detector.test.tssrc/__tests__/main/agents/session-storage.test.tssrc/__tests__/main/parsers/copilot-output-parser.test.tssrc/__tests__/main/parsers/index.test.tssrc/__tests__/main/process-manager/handlers/StdoutHandler.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/__tests__/main/utils/agent-args.test.tssrc/__tests__/main/utils/remote-fs.test.tssrc/__tests__/main/utils/ssh-command-builder.test.tssrc/__tests__/renderer/components/TerminalOutput.test.tsxsrc/__tests__/renderer/hooks/useAgentListeners.test.tssrc/__tests__/renderer/hooks/useWizardHandlers.test.tssrc/__tests__/renderer/services/inlineWizardConversation.test.tssrc/__tests__/shared/agentIds.test.tssrc/__tests__/shared/agentMetadata.test.tssrc/main/agents/capabilities.tssrc/main/agents/definitions.tssrc/main/agents/path-prober.tssrc/main/ipc/handlers/agents.tssrc/main/ipc/handlers/process.tssrc/main/parsers/agent-output-parser.tssrc/main/parsers/copilot-output-parser.tssrc/main/parsers/error-patterns.tssrc/main/parsers/index.tssrc/main/parsers/parser-factory.tssrc/main/preload/agents.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/process-manager/spawners/PtySpawner.tssrc/main/process-manager/types.tssrc/main/storage/copilot-session-storage.tssrc/main/storage/index.tssrc/main/utils/agent-args.tssrc/main/utils/remote-fs.tssrc/main/utils/ssh-command-builder.tssrc/renderer/components/NewInstanceModal.tsxsrc/renderer/components/TerminalOutput.tsxsrc/renderer/components/UsageDashboard/AgentEfficiencyChart.tsxsrc/renderer/components/UsageDashboard/LongestAutoRunsTable.tsxsrc/renderer/components/UsageDashboard/SessionStats.tsxsrc/renderer/components/Wizard/screens/AgentSelectionScreen.tsxsrc/renderer/components/Wizard/services/conversationManager.tssrc/renderer/constants/agentIcons.tssrc/renderer/constants/app.tssrc/renderer/hooks/agent/useAgentListeners.tssrc/renderer/hooks/wizard/useWizardHandlers.tssrc/renderer/services/inlineWizardConversation.tssrc/renderer/types/index.tssrc/renderer/utils/hiddenProgress.tssrc/shared/agentConstants.tssrc/shared/agentIds.tssrc/shared/agentMetadata.ts
✅ Files skipped from review due to trivial changes (13)
- README.md
- src/tests/shared/agentIds.test.ts
- docs/installation.md
- src/renderer/components/UsageDashboard/AgentEfficiencyChart.tsx
- src/renderer/constants/agentIcons.ts
- src/shared/agentConstants.ts
- src/renderer/components/UsageDashboard/LongestAutoRunsTable.tsx
- src/renderer/components/UsageDashboard/SessionStats.tsx
- src/shared/agentMetadata.ts
- src/main/agents/path-prober.ts
- src/shared/agentIds.ts
- src/tests/shared/agentMetadata.test.ts
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (26)
- src/main/parsers/index.ts
- src/tests/main/utils/remote-fs.test.ts
- src/main/parsers/agent-output-parser.ts
- src/main/storage/index.ts
- src/tests/main/utils/agent-args.test.ts
- src/main/parsers/parser-factory.ts
- src/tests/main/parsers/index.test.ts
- src/renderer/components/NewInstanceModal.tsx
- src/tests/main/agents/capabilities.test.ts
- src/tests/main/utils/ssh-command-builder.test.ts
- src/tests/main/process-manager/spawners/ChildProcessSpawner.test.ts
- src/main/process-manager/types.ts
- src/main/utils/remote-fs.ts
- src/tests/renderer/components/TerminalOutput.test.tsx
- src/tests/renderer/hooks/useWizardHandlers.test.ts
- src/main/utils/ssh-command-builder.ts
- src/renderer/components/Wizard/screens/AgentSelectionScreen.tsx
- src/main/preload/agents.ts
- src/main/utils/agent-args.ts
- src/main/ipc/handlers/process.ts
- src/main/process-manager/spawners/PtySpawner.ts
- src/main/agents/capabilities.ts
- src/main/parsers/copilot-output-parser.ts
- src/tests/main/agents/session-storage.test.ts
- src/tests/main/agents/definitions.test.ts
- src/main/process-manager/handlers/StdoutHandler.ts
- Add prompt field to Session.agentCommands type (critical: agentStore/useInputProcessing reference it)
- Fix discoverCopilotSlashCommands to return {name, description} objects matching renderer expectations
- Detect JSON mode from pre-prompt CLI args to avoid false positives from prompt content
- Preserve POSIX root '/' in normalizePath (was collapsing to empty string)
- Fail closed on unverifiable session ownership instead of fail-open
- Use != null checks for remote read results to handle valid empty strings and zero sizes
- Add hiddenProgress and toolState.status to LogItemComponent memo comparator
- Add Copilot case to wizard buildArgsForAgent with jsonOutputArgs and readOnlyArgs
- Handle 'error' tool state as failure in formatToolProgressText and TerminalOutput
- Replace hardcoded 0.1.0 version examples in CONTRIBUTING.md with X.Y.Z placeholders
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
3 participants
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.
Summary
This PR adds GitHub Copilot CLI support to Maestro as a new beta agent. It wires Copilot into Maestro’s agent system end-to-end, including agent detection, launch definitions, capability metadata, JSON output parsing, session resume support, and session history loading from ~/.copilot/session-state. It also updates onboarding and UI surfaces so Copilot can be selected and managed alongside the existing agents.
What changed:
Notes:
Summary by CodeRabbit
Release Notes
New Features
Documentation