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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds GitHub Copilot CLI support: agent metadata, capabilities, path probing, per-process parser factory, Copilot JSON parser and error patterns, Copilot session storage, PTY/child spawner and stdout buffering/resync changes, remote-fs escaping, renderer UI entries, tests, and docs updates. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Spawner as ChildProcessSpawner
participant PTY as PtySpawner
participant Copilot as Copilot CLI
participant Stdout as StdoutHandler
participant Parser as CopilotOutputParser
participant Storage as CopilotSessionStorage
participant UI as Renderer
User->>Spawner: invoke `copilot` with flags (e.g. --output-format json, -p)
Spawner->>Spawner: detect stream-json/resume and create per-process parser
Spawner->>PTY: spawn process (requiresPty true) / wrap in shell on Windows
PTY->>Copilot: execute Copilot CLI
Copilot-->>Stdout: stream concatenated JSON / mixed text
Stdout->>Stdout: buffer, split JSON, resync on corruption
Stdout->>Parser: deliver discrete JSON objects
Parser->>Stdout: emit text_delta, tool_use, result, error events
Parser->>Storage: persist session events (on result/sessionId)
UI->>Storage: list/search sessions
UI->>Spawner: request resume -> Spawner invokes Copilot with --resume=<id>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
📝 Coding Plan
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 |
✅ 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 end-to-end: agent definition, PTY-based spawning, a dedicated output parser for Copilot's concatenated JSON event stream, per-process parser instantiation (preventing cross-session Key findings from this pass:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User / Maestro UI
participant SP as ChildProcessSpawner / PtySpawner
participant CP as copilot CLI (PTY)
participant SH as StdoutHandler
participant PA as CopilotOutputParser
participant SS as CopilotSessionStorage
U->>SP: spawn(config { requiresPty, batchModeArgs, jsonOutputArgs })
SP->>SP: createOutputParser('copilot') → fresh instance
SP->>CP: PTY launch: copilot -p "…" --output-format json --allow-all-tools --silent
loop Streamed JSON events (concatenated, no newlines)
CP-->>SH: stdout chunk (1..N JSON objects)
SH->>SH: extractConcatenatedJsonObjects(buffer)
loop each complete JSON object
SH->>PA: parseJsonObject(parsed)
PA-->>SH: ParsedEvent (text / tool_use / result / system)
SH->>SH: emitSessionIdIfNeeded (from result.sessionId)
SH->>U: emit tool-execution / data / session-id events
end
SH->>SH: buffer remainder for next chunk
end
CP-->>SH: assistant.message { phase:"final_answer" }
PA-->>SH: ParsedEvent { type:"result", text, raw.data.phase:"final_answer" }
SH->>SH: isResultMessage → true → resultEmitted=true
SH->>U: emit data(finalText)
CP-->>SH: { type:"result", sessionId, exitCode:0 }
SH->>SH: extractCopilotSessionId → emit session-id once
U->>SS: listSessions(projectPath)
SS->>SS: readdir ~/.copilot/session-state/
SS->>SS: parseWorkspaceMetadata(workspace.yaml)
SS->>SS: matchesProject(metadata, projectPath)
SS->>SS: parseEvents(events.jsonl)
SS-->>U: AgentSessionInfo[]
|
![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
|
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
|
Good to go! I have a follow up PR that will add some additional enhancements depending on if this is accepted. |
|
Whats the enhancement over using OpenCode with Github Copilot? |
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. |
|
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. |
|
@greptileai re-review |
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. |
|
I'll checkout Github CLI and pull the PR :) |
2 participants
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
New Features
Documentation
Tests