feat: Add GitHub Copilot CLI as first-class agent with multi-model support#805
Closed
scriptease wants to merge 27 commits intoRunMaestro:rcfrom
Closed
feat: Add GitHub Copilot CLI as first-class agent with multi-model support#805scriptease wants to merge 27 commits intoRunMaestro:rcfrom
scriptease wants to merge 27 commits intoRunMaestro:rcfrom
Conversation
Add copilot-cli agent support with full JSONL output parsing, session storage browsing, error detection, and UI integration. Agent definition: - Binary: copilot, batch mode via -p flag - JSON output: --output-format json (JSONL) - Session resume: --resume SESSION-ID - YOLO mode: --allow-all - Model selection: --model flag Output parser (verified against actual CLI output): - 11 event types: session lifecycle, streaming deltas, tool execution, assistant messages, and result with session ID - Accumulates outputTokens from assistant.message events Session storage: - Reads ~/.copilot/session-state/<uuid>/workspace.yaml for metadata - Parses events.jsonl for message history - Supports pagination, search, and project filtering Also includes: - Error patterns (auth, rate limit, network, token exhaustion) - UI: added to SUPPORTED_AGENTS and wizard agent tiles - copilot-instructions.md for Copilot sessions in this repo Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
41 tests covering all 11 JSONL event types verified against actual copilot CLI output. Includes: - Session lifecycle: mcp_server_status_changed, mcp_servers_loaded, tools_updated - Conversation: user.message, assistant.turn_start/end, message_delta, message - Tool execution: tool.execution_start, tool.execution_complete - Completion: result with sessionId and usage - Error detection: auth, rate limit, network, exit codes - End-to-end: full session simulations (simple + tool use) - Edge cases: empty deltas, long output truncation, token accumulation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ssues On Windows, long prompts passed as PowerShell CLI args get garbled due to escaping issues with special characters. Changed copilot-cli to use '-p -' (read from stdin) and sendPromptViaStdinRaw=true. Also added sendPromptViaStdinRaw as an agent definition field so other agents can opt into stdin-based prompt delivery. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When an output parser is registered (JSONL agents like copilot-cli, codex, opencode, factory-droid), non-JSON lines from stdout are now suppressed instead of being displayed to the user as raw text. This prevents PowerShell profile banners and MCP server startup messages from cluttering the agent output. Only agents WITHOUT an output parser (terminal, legacy mode) continue to emit raw non-JSON lines, which is correct for terminal sessions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The promptArgs function was being skipped when sendPromptViaStdinRaw=true because the spawner's promptViaStdin guard prevents adding prompt args. But '-p -' isn't prompt text — it's a flag telling copilot to read stdin. Moved to batchModeArgs so it's always present in batch mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
copilot -p - means 'prompt is the literal dash character', not 'read from stdin'. When stdin is piped (sendPromptViaStdinRaw), copilot reads it automatically without -p. Removed -p - from batchModeArgs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot CLI outputs MCP server startup messages, PowerShell profile banners, and initialization noise to stderr. These were being displayed via the onStderr renderer handler. Now suppressed for all JSONL agents with output parsers (except Codex which has special stderr handling). Error detection still runs first, so real errors are captured. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two fixes for the raw JSONL display issue: 1. detectErrorFromParsed now handles session.error events (Copilot CLI format: data.message, data.errorType). Previously, session.error wasn't caught because it has no top-level 'error' field, causing the error to fall through to detectErrorFromExit which dumps the full stdoutBuffer in raw. 2. detectErrorFromExit no longer includes stderr/stdout in raw — for JSONL agents the stdoutBuffer contains all parsed JSON events which is noise, not useful error context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The root cause of raw JSONL display: copilot-cli's --output-format json args didn't match the isStreamJsonMode heuristic check, so the process was treated as batch-JSON (single JSON blob) instead of streaming JSONL. On exit, handleBatchModeExit tried to JSON.parse the entire buffer, failed, and dumped the raw content to the display. Fixed by adding outputParser presence as a signal for stream-json mode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
P1: Reset accumulatedOutputTokens on result event (singleton parser was carrying tokens across sessions). Report tokens unconditionally even when result event has no usage field. P2: Add UUID validation to session storage public methods to prevent path traversal. Update Phase 2 parity matrix to reflect shipped features. Derive GRID_ROWS from AGENT_TILES.length. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fetch available models from models.dev API (github-copilot provider) for the copilot-cli agent. Falls back to user configured model from ~/.copilot/config.json if the API is unavailable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t-cli agent Agent-Logs-Url: https://github.com/dwizzzle/Maestro/sessions/51ef253a-81f5-4558-bde0-0ae0f74e7e04 Co-authored-by: dwizzzle <37511114+dwizzzle@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…SSH, fix stale comment 1. StderrHandler: narrow stderr suppression to copilot-cli only instead of all agents with output parsers. The previous condition (outputParser && toolType !== 'codex') silently dropped stderr for OpenCode and Factory Droid, which is a behavioral change to shipped agents. 2. CopilotCliSessionStorage: add logger.warn() when sshConfig is provided, since SSH remote session storage is not yet implemented. Previously silently fell back to local sessions. Added SSH remote storage as explicit Phase 2 deferred item in the plan doc. 3. capabilities.ts: update stale comment that said session storage was deferred to Phase 2 — it shipped in Phase 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merge additional error patterns from @adamcongdon parallel Copilot CLI implementation: - auth_expired: invalid token, please login patterns - rate_limited: HTTP 429 status code pattern - network_error: ECONNRESET/ENOTFOUND, request timeout patterns - token_exhaustion: context exceeded, too many tokens alternates - agent_crashed: fatal/unexpected/internal/unhandled error pattern - session_not_found: new category for session not found / invalid session Also add suggested launcher flags (--no-auto-update, --no-ask-user) to Phase 2 plan doc, credited to @adamcongdon. Co-Authored-By: Adam Congdon <automating.adam@gmail.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot CLI was grayed out with 'Coming Soon' in the Create New Agent modal because it was missing from the SUPPORTED_AGENTS array. The wizard AgentSelectionScreen already had it marked as supported. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…pawn The tab naming handler built its own spawn config but omitted the agent definition's sendPromptViaStdinRaw and noPromptSeparator flags. For copilot-cli (which requires stdin prompt delivery), this caused the prompt to be appended as a positional CLI argument with '--' separator, triggering 'error: too many arguments. Expected 0 arguments but got 1.' The error was then silently suppressed by the copilot-cli stderr filter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 PR adds comprehensive support for GitHub Copilot CLI as a new agent type in Maestro, including agent configuration, JSONL output parsing, session storage, error handling, and UI integration across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Renderer
participant Main
participant ChildProcess as Child Process
participant Parser as Output Parser
participant Storage as Session Storage
User->>Renderer: Select Copilot CLI agent & send prompt
Renderer->>Main: spawn IPC (sendPromptViaStdinRaw: true)
Main->>Main: Load copilot-cli agent config<br/>(definitions, capabilities)
Main->>ChildProcess: Spawn 'copilot --allow-all --output-format json'
Main->>ChildProcess: Write prompt to stdin
ChildProcess->>ChildProcess: Process prompt, emit JSONL events
ChildProcess-->>Main: stdout (JSONL lines)
Main->>Parser: parseJsonLine(line)
Parser->>Parser: transformMessage(type) to normalized event
Parser-->>Main: ParsedEvent (text/tool_use/usage/system)
Main->>Main: Accumulate outputTokens per session
Main-->>Renderer: Emit normalized events
Main->>Storage: readSessionMessages(projectPath, sessionId)
Storage->>Storage: Parse events.jsonl from<br/>~/.copilot/session-state/
Storage-->>Main: SessionMessage[] with pagination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
src/__tests__/main/agents/detector.test.ts (1)
325-326: Avoid hardcoded agent counts in tests.Line 326 and Line 973 hardcode
9, which will break on every future agent addition/removal. Prefer deriving from the canonical ID list (AGENT_IDS.length) and keep the explicittoContain('copilot-cli')assertions.Also applies to: 972-973
🤖 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 325 - 326, Replace the hardcoded agent count assertions in the detector tests with a dynamic check based on the canonical AGENT_IDS list: instead of expect(agents.length).toBe(9) use expect(agents.length).toBe(AGENT_IDS.length) (or equivalent), while retaining explicit inclusion checks like expect(agents).toContain('copilot-cli'); locate the assertions around the tests referencing agents (e.g., the block using the agents variable and the toContain('copilot-cli') check) and update both occurrences so the test no longer hardcodes numeric counts but derives the expected length from AGENT_IDS.src/__tests__/main/parsers/index.test.ts (1)
52-57: Consolidate parser-count expectation to a single source in this test file.These assertions repeat the
5literal in several places. Consider using one shared expected parser list/count constant so future parser additions only require one edit.Also applies to: 62-67, 76-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/parsers/index.test.ts` around lines 52 - 57, Replace hard-coded literal counts in this test file with a single shared constant (e.g., EXPECTED_PARSERS or EXPECTED_PARSER_COUNT) defined near the top of the file and reference it in the tests that call initializeOutputParsers() and getAllOutputParsers(); update the assertions in the tests that currently use 5 (the one checking parsers.length and the other occurrences around lines 62–67 and 76) to use that shared constant so adding/removing parsers only requires updating one value and keep tests in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Line 68: Update the documentation reference that currently points to the
renderer shim `src/renderer/constants/themes.ts` to instead point to the
canonical shared module `src/shared/themes.ts` (and continue to reference
`src/shared/theme-types.ts` for the `ThemeId` type); locate the doc line listing
"Themes" and replace the path so contributors are directed to
`src/shared/themes.ts` as the source-of-truth and `src/shared/theme-types.ts`
for `ThemeId`, avoiding edits to the re-export shim
`src/renderer/constants/themes.ts`.
In `@docs/Copilot-CLI-Phase2-Plan.md`:
- Around line 21-23: Summary: The docs instruct running Copilot with -p but the
PR sends prompts via raw stdin; update the doc to use the same stdin transport
so captured JSONL matches what the integration emits. Change the guidance in
docs/Copilot-CLI-Phase2-Plan.md to instruct piping the prompt into Copilot (use
the stdin transport) when running `copilot --output-format json --allow-all`,
capture that output, and re-run the event shape investigation; also mention
re-validating the parser at src/main/parsers/copilot-cli-output-parser.ts
against the piped stdin JSONL output so the heuristic matching is refined to the
actual schema emitted by the stdin path.
In `@src/main/agents/detector.ts`:
- Around line 365-402: The fetch to 'https://models.dev/api.json' in the
function/section that returns models for agentId should be bounded with an
AbortController and short timeout: create an AbortController, pass its signal
into fetch, set a timer (e.g., 2–3s) to call controller.abort(), and clear the
timer on successful response; handle the abort error path the same as other
fetch failures so the code falls back to reading ~/.copilot/config.json and
continues to return models; update the try/catch around fetch (and any nested
await response.json()) to respect the abort and avoid waiting indefinitely.
In `@src/main/ipc/handlers/process.ts`:
- Line 645: The line that sets sendPromptViaStdinRaw in the IPC payload is
overwriting the per-request value from ...config with the agent default; change
the assignment so it prefers the request-level flag first and falls back to
agent.sendPromptViaStdinRaw only if config.sendPromptViaStdinRaw is undefined
(i.e., use config.sendPromptViaStdinRaw ?? agent?.sendPromptViaStdinRaw). Update
the object property where sendPromptViaStdinRaw is set (near the
sendPromptViaStdinRaw: agent?.sendPromptViaStdinRaw entry) to use that
nullish-coalescing pattern so IPC callers can force raw-stdin per request while
still honoring the agent default when absent.
In `@src/main/parsers/copilot-cli-output-parser.ts`:
- Around line 283-289: The default branch in transformMessage() treats errors by
checking msg.error only, causing session.error payloads (which hold the message
at data.message) to be rendered as "Unknown error"; update transformMessage() to
explicitly detect msg.type === 'session.error' (or
msg.type?.includes('session.error')) and pass msg.data (or msg.data.message)
into the same extractErrorText() logic (or reuse detectErrorFromParsed()'s
extraction) so the parsed event's text matches detectErrorFromParsed(); ensure
the returned object for error events sets type: 'error', text:
extractErrorText(...) and raw: msg just like other error paths.
In `@src/main/parsers/error-patterns.ts`:
- Around line 953-955: The existing error-pattern entry with pattern
/copilot.*command not found/i should be broadened to also match reversed
phrasing like "command not found: copilot"; update the regex for that entry (the
object with keys pattern/message/recoverable) to a case-insensitive pattern that
matches both "copilot ... command not found" and "command not found ... copilot"
(for example by alternating the two forms or using a non-capturing group),
leaving the message and recoverable fields unchanged.
In `@src/main/process-manager/handlers/StderrHandler.ts`:
- Around line 110-119: The current early return in StderrHandler.ts for toolType
=== 'copilot-cli' silently swallows all stderr; change this so we only suppress
when the stderr is empty or matches known benign patterns (use the existing
outputParser or a small benignRegex list to detect those cases) and otherwise do
not return—log the stderr as warning/error with sessionId and cleanedStderr and
allow normal error handling to continue (so upstream detection or exceptions can
surface to Sentry). Locate the conditional that references toolType,
outputParser, logger and cleanedStderr and replace the unconditional return with
a guarded suppression check (isBenign via outputParser or regex), otherwise log
and proceed.
In `@src/main/process-manager/handlers/StdoutHandler.ts`:
- Around line 241-245: Change the suppression log in StdoutHandler.ts so it uses
a lower-severity level and stops including raw content previews: replace the
logger.info call inside the non-JSON suppression branch (the call that currently
passes sessionId, toolType: managedProcess.toolType, and linePreview:
line.substring(0, 80)) with a debug-level log (e.g., logger.debug) and remove
the linePreview field; if you need visibility, log a generic marker or a
rate-limited counter instead but do not emit any portion of the suppressed line.
In `@src/main/process-manager/spawners/ChildProcessSpawner.ts`:
- Around line 340-351: The isStreamJsonMode flag in ChildProcessSpawner should
not be set solely because getOutputParser returned a parser; remove the
unconditional `!!outputParser` check and instead detect streaming JSON only when
the parser explicitly indicates it supports streaming (e.g., check a property
like outputParser?.supportsStreaming or outputParser?.isStreamJsonMode) or when
command args/config require JSON; update the isStreamJsonMode expression to use
that explicit property (or add such a property to the parser interface if
missing) so non-JSON parser-enabled agents are not forced into stream-json
handling.
In `@src/main/storage/copilot-cli-session-storage.ts`:
- Around line 246-326: readSessionMessages and getSessionPath currently accept
any valid sessionId because sessions are stored globally; update both to
re-parse the session's workspace metadata (workspace.yaml or equivalent stored
alongside events.jsonl in the directory returned by
getCopilotSessionDir()/sessionId) and verify the stored cwd/project path matches
the provided _projectPath before returning data. In readSessionMessages
(function readSessionMessages) after validating isValidSessionId(sessionId) and
computing sessionDir/eventsPath, load and parse the workspace metadata (e.g.,
workspace.yaml) and if its cwd does not equal the incoming _projectPath then
return { messages: [], total: 0, hasMore: false }; similarly in getSessionPath
(function getSessionPath) perform the same workspace metadata check and return
null if the session's cwd differs. Keep the existing SSH warning behavior
unchanged.
---
Nitpick comments:
In `@src/__tests__/main/agents/detector.test.ts`:
- Around line 325-326: Replace the hardcoded agent count assertions in the
detector tests with a dynamic check based on the canonical AGENT_IDS list:
instead of expect(agents.length).toBe(9) use
expect(agents.length).toBe(AGENT_IDS.length) (or equivalent), while retaining
explicit inclusion checks like expect(agents).toContain('copilot-cli'); locate
the assertions around the tests referencing agents (e.g., the block using the
agents variable and the toContain('copilot-cli') check) and update both
occurrences so the test no longer hardcodes numeric counts but derives the
expected length from AGENT_IDS.
In `@src/__tests__/main/parsers/index.test.ts`:
- Around line 52-57: Replace hard-coded literal counts in this test file with a
single shared constant (e.g., EXPECTED_PARSERS or EXPECTED_PARSER_COUNT) defined
near the top of the file and reference it in the tests that call
initializeOutputParsers() and getAllOutputParsers(); update the assertions in
the tests that currently use 5 (the one checking parsers.length and the other
occurrences around lines 62–67 and 76) to use that shared constant so
adding/removing parsers only requires updating one value and keep tests in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9f4aa4b-9d17-49b9-bf38-e9af329b7fd7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
.github/copilot-instructions.mddocs/Copilot-CLI-Phase2-Plan.mdsrc/__tests__/main/agents/detector.test.tssrc/__tests__/main/parsers/copilot-cli-output-parser.test.tssrc/__tests__/main/parsers/index.test.tssrc/__tests__/main/process-manager/spawners/ChildProcessSpawner.test.tssrc/main/agents/capabilities.tssrc/main/agents/definitions.tssrc/main/agents/detector.tssrc/main/ipc/handlers/process.tssrc/main/ipc/handlers/tabNaming.tssrc/main/parsers/copilot-cli-output-parser.tssrc/main/parsers/error-patterns.tssrc/main/parsers/index.tssrc/main/process-manager/handlers/StderrHandler.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/storage/copilot-cli-session-storage.tssrc/main/storage/index.tssrc/renderer/components/NewInstanceModal/types.tssrc/renderer/components/Wizard/screens/AgentSelectionScreen.tsxsrc/shared/agentConstants.tssrc/shared/agentIds.tssrc/shared/agentMetadata.ts
Greptile SummaryThis PR adds GitHub Copilot CLI as a fully integrated first-class agent across all 9 registry touchpoints (agent ID, definition, capabilities, metadata, context window, output parser, error patterns, session storage, wizard/modal tiles), and introduces a multi-model picker backed by the models.dev API. The integration is thorough and well-tested; prior P1 findings (singleton token-counter leak, cwd-less session bleed) were addressed in preceding commits. One P1 remains: four Confidence Score: 4/5Near-merge-ready; one debug console.log cleanup required before shipping to production All prior P1 findings (singleton token leak, cwd-less session bleed in listSessions) have been resolved. The integration is complete across all 9 touchpoints and the test suite is comprehensive (665-line parser tests). The single remaining P1 is the four debug console.log statements in StdoutHandler.ts that fire in production for every agent on every usage event — a mechanical cleanup required before merge. src/main/process-manager/handlers/StdoutHandler.ts — four debug console.log statements must be removed Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Renderer UI
participant IPC as IPC (agents:getModels)
participant Det as AgentDetector
participant API as models.dev API
participant FS as ~/.copilot/config.json
participant Spawn as ChildProcessSpawner
participant Parser as CopilotCliOutputParser
participant Store as CopilotCliSessionStorage
UI->>IPC: getModels('copilot-cli')
IPC->>Det: discoverModels('copilot-cli')
Det->>API: GET /api.json (3s timeout)
API-->>Det: github-copilot provider models
Det->>FS: readFileSync config.json (prepend current model)
Det-->>UI: sorted model list
UI->>Spawn: spawn(copilot --output-format json --allow-all)
Note over Spawn: isStreamJsonMode=true (--output-format json)
Spawn->>Spawn: sendPromptViaStdinRaw → write prompt to stdin
Spawn-->>Parser: JSONL lines via stdout
Parser-->>Parser: assistant.message_delta → text (isPartial)
Parser-->>Parser: assistant.message (text only) → result
Parser-->>Parser: result event → usage (+ inject copilotAccumulatedTokens)
UI->>Store: listSessions(projectPath)
Store->>FS: readdir ~/.copilot/session-state/
Store->>FS: read workspace.yaml per UUID dir
Note over Store: skip if meta.cwd absent or ≠ projectPath
Store-->>UI: AgentSessionInfo[]
Reviews (3): Last reviewed commit: "fix: detect --output-format json as stre..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Inline fixes: - copilot-instructions.md: point Themes to src/shared/themes.ts (canonical) - Phase2 plan: use stdin transport (echo | copilot) to match integration - detector.ts: add AbortController with 3s timeout on models.dev fetch - process.ts: prefer per-request sendPromptViaStdinRaw over agent default - copilot-cli-output-parser.ts: handle session.error data.message payload - error-patterns.ts: broaden 'command not found' regex to match both orderings - StderrHandler.ts: replace blanket suppression with benign-pattern matching - StdoutHandler.ts: downgrade suppression log to debug, remove content preview - ChildProcessSpawner.ts: remove !!outputParser from isStreamJsonMode check - copilot-cli-session-storage.ts: verify projectPath in readSessionMessages and getSessionPath against workspace.yaml cwd Nitpicks: - detector.test.ts: use AGENT_IDS.length instead of hardcoded 9 - parsers/index.test.ts: extract EXPECTED_PARSER_COUNT constant Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ession filter P1: CopilotCliOutputParser.accumulatedOutputTokens is instance state on a singleton. If a session is killed or errors before emitting a result event, the residual count inflates usage stats for the next session. Fix: add resetState() as an optional method on AgentOutputParser interface, implement it in CopilotCliOutputParser, and call it from ExitHandler on every process exit. P2: listSessions skipped the project-path filter when workspace.yaml had no cwd field, causing workspace-less sessions to appear in every project's session browser. Fix: skip sessions without a cwd instead of including them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
@greptile please review |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/storage/copilot-cli-session-storage.ts (1)
264-278:⚠️ Potential issue | 🟠 MajorFail closed when the session’s workspace metadata can’t be verified.
Lines 277-278 and 358-360 reopen access whenever
workspace.yamlis missing or unreadable. Because Copilot stores sessions globally, that turns a known UUID from another repo back into a readable/openable session. Return an empty result /nullon metadata read or parse failure whenprojectPathis provided.Also applies to: 345-360
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-cli-session-storage.ts` around lines 264 - 278, The catch that currently swallows errors when reading/parsing workspace.yaml should fail closed when a projectPath is provided: inside the block that verifies "session belongs to the requested project" (the code that builds workspacePath, reads via fs.readFile and calls parseWorkspaceYaml), replace the empty catch handler with logic that returns an empty result (or null) when projectPath is truthy so unreadable/missing workspace.yaml prevents access; apply the same change to the duplicate verification block later (the other catch around parseWorkspaceYaml at lines ~345-360) so both checks return empty/null on read/parse failure instead of allowing access.
🧹 Nitpick comments (1)
src/__tests__/main/agents/detector.test.ts (1)
973-974: Keep this assertion derived fromAGENT_IDS.This test hardcodes
9right after the suite was updated to useAGENT_IDS.length, so the next agent addition will break it again. Reuse the shared constant here too.🤖 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 973 - 974, The test assertion currently hardcodes the expected agent count as 9; change it to use the shared AGENT_IDS constant so it stays correct when agents are added (replace expect(result.length).toBe(9) with expect(result.length).toBe(AGENT_IDS.length)), ensuring AGENT_IDS is imported or referenced in the test scope and leaving the rest of the test (e.g., the result variable and surrounding assertions) unchanged.
🤖 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/Copilot-CLI-Phase2-Plan.md`:
- Around line 49-55: Update the checklist to match the actual BaseSessionStorage
contract: remove mentions of implementing searchSessions() and getGlobalStats(),
and instead require implementing getSessionPath(projectPath, sessionId),
deleteMessagePair(projectPath, sessionId, messageId) and
getSearchableMessages(projectPath, sessionId) in addition to
listSessions(projectPath, options) and readSessionMessages(projectPath,
sessionId, options); keep the step to register the concrete storage in
initializeSessionStorages() so the new implementation (class extending
BaseSessionStorage) is discoverable.
In `@src/main/storage/copilot-cli-session-storage.ts`:
- Around line 152-169: The code currently lowercases resolved paths (e.g.,
normalizedProjectPath and normalizedCwd) which causes incorrect matches on
case-sensitive platforms; change comparisons to use path.resolve(...) verbatim
and perform case-folding only when running on Windows (process.platform ===
'win32') so Windows uses .toLowerCase() for both sides while other platforms
compare the exact resolved strings; apply the same Windows-only case-insensitive
logic to the other path comparisons noted (the checks around the symbols used at
the other locations).
---
Duplicate comments:
In `@src/main/storage/copilot-cli-session-storage.ts`:
- Around line 264-278: The catch that currently swallows errors when
reading/parsing workspace.yaml should fail closed when a projectPath is
provided: inside the block that verifies "session belongs to the requested
project" (the code that builds workspacePath, reads via fs.readFile and calls
parseWorkspaceYaml), replace the empty catch handler with logic that returns an
empty result (or null) when projectPath is truthy so unreadable/missing
workspace.yaml prevents access; apply the same change to the duplicate
verification block later (the other catch around parseWorkspaceYaml at lines
~345-360) so both checks return empty/null on read/parse failure instead of
allowing access.
---
Nitpick comments:
In `@src/__tests__/main/agents/detector.test.ts`:
- Around line 973-974: The test assertion currently hardcodes the expected agent
count as 9; change it to use the shared AGENT_IDS constant so it stays correct
when agents are added (replace expect(result.length).toBe(9) with
expect(result.length).toBe(AGENT_IDS.length)), ensuring AGENT_IDS is imported or
referenced in the test scope and leaving the rest of the test (e.g., the result
variable and surrounding assertions) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85d97464-e97b-4e2e-ba82-88aeea1b2a99
📒 Files selected for processing (12)
.github/copilot-instructions.mddocs/Copilot-CLI-Phase2-Plan.mdsrc/__tests__/main/agents/detector.test.tssrc/__tests__/main/parsers/index.test.tssrc/main/agents/detector.tssrc/main/ipc/handlers/process.tssrc/main/parsers/copilot-cli-output-parser.tssrc/main/parsers/error-patterns.tssrc/main/process-manager/handlers/StderrHandler.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/spawners/ChildProcessSpawner.tssrc/main/storage/copilot-cli-session-storage.ts
✅ Files skipped from review due to trivial changes (3)
- src/tests/main/parsers/index.test.ts
- .github/copilot-instructions.md
- src/main/parsers/copilot-cli-output-parser.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/agents/detector.ts
- src/main/process-manager/handlers/StderrHandler.ts
…ail-closed storage - Phase2 plan: update BaseSessionStorage method checklist to match actual contract (getSessionPath, deleteMessagePair, getSearchableMessages instead of searchSessions/getGlobalStats) - copilot-cli-session-storage: case-fold paths only on Windows (process.platform === 'win32') to fix incorrect matches on case-sensitive filesystems; change catch handlers to fail closed (return empty/null) when workspace.yaml is unreadable instead of allowing access - detector.test.ts: replace second hardcoded agent count with AGENT_IDS.length Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main/storage/copilot-cli-session-storage.ts (2)
153-154:⚠️ Potential issue | 🟠 MajorMake path comparison case-insensitive on Windows only.
Line 153/171/274/356 lowercases both paths on every platform. That can match the wrong project on
case-sensitive filesystems.Proposed fix
+function normalizePathForCompare(p: string): string { + const resolved = path.resolve(p); + return process.platform === 'win32' ? resolved.toLowerCase() : resolved; +} + -const normalizedProjectPath = path.resolve(projectPath).toLowerCase(); +const normalizedProjectPath = normalizePathForCompare(projectPath); -const normalizedCwd = path.resolve(meta.cwd).toLowerCase(); +const normalizedCwd = normalizePathForCompare(meta.cwd); -if (meta.cwd && path.resolve(meta.cwd).toLowerCase() !== path.resolve(projectPath).toLowerCase()) { +if (meta.cwd && normalizePathForCompare(meta.cwd) !== normalizePathForCompare(projectPath)) { return { messages: [], total: 0, hasMore: false }; } -if (meta.cwd && path.resolve(meta.cwd).toLowerCase() !== path.resolve(projectPath).toLowerCase()) { +if (meta.cwd && normalizePathForCompare(meta.cwd) !== normalizePathForCompare(projectPath)) { return null; }Also applies to: 171-173, 273-275, 355-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-cli-session-storage.ts` around lines 153 - 154, Current code unconditionally lowercases resolved paths (e.g., normalizedProjectPath and normalizedExistingProjectPath) causing incorrect matches on case-sensitive filesystems; change it to only normalize case on Windows by checking process.platform === 'win32' (resolve paths with path.resolve(...) first, then call toLowerCase() only when platform is 'win32') and apply the same conditional behavior to all places where you currently lowercase both paths before comparing (the variables used in project session lookup and removal).
278-280:⚠️ Potential issue | 🟠 MajorDon’t fail open when project ownership cannot be verified.
Line 278 and Line 360 currently allow session access if
workspace.yamlcan’t be read. That still
permits cross-project reads when UUID is known.Proposed fix
- } catch { - // If workspace.yaml can't be read, allow access (session may be incomplete) - } + } catch { + return { messages: [], total: 0, hasMore: false }; + } ... - } catch { - // If workspace.yaml can't be read, allow access - } + } catch { + return null; + }Also applies to: 360-362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-cli-session-storage.ts` around lines 278 - 280, The current catch blocks that swallow errors when reading workspace.yaml (the try/catch around the ownership verification logic in copilot-cli-session-storage.ts) "fail open" and allow session access if the file can’t be read; change those catch blocks to deny access instead: when reading/parsing workspace.yaml fails, log the error and return a negative auth result (e.g., return false or throw an authorization error) from the ownership-check function rather than allowing access. Apply the same fix to both occurrences (the catch at the shown block and the similar catch around lines ~360-362) so failed reads consistently reject the session.
🤖 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/parsers/copilot-cli-output-parser.ts`:
- Around line 103-112: The parser's mutable field accumulatedOutputTokens and
its resetState() make token counts global and unsafe for concurrent Copilot
sessions; move token accounting out of the singleton parser and instead track
output tokens per-session (e.g., add a token counter on the session/process
abstraction such as ManagedProcess or accept a sessionId/context in parse calls
and key counts by that id), remove or deprecate accumulatedOutputTokens from the
parser, update all references (including places noted around lines ~187 and
~283) to increment/decrement the per-session counter rather than the parser
field, and ensure resetState() clears only parser-local non-session state or
delegates per-session resets to the process/session manager.
In `@src/main/process-manager/handlers/ExitHandler.ts`:
- Around line 201-205: The current ExitHandler calls outputParser.resetState()
unconditionally which can clear shared parser state still used by sibling
sessions; instead, change the logic in ExitHandler so you only call
outputParser.resetState() when no other active sessions are referencing that
parser (e.g., ask a SessionManager/ParserRegistry whether this is the last
session using that parser, or maintain a reference count on the parser and
decrement it here and reset only when count reaches zero). Specifically, update
the exit flow around outputParser.resetState to query
SessionManager.isLastSessionForParser(outputParser) or use
outputParser.decrementRefCount() and call resetState() only when it returns 0,
leaving shared parser instances intact while siblings remain active.
In `@src/main/storage/copilot-cli-session-storage.ts`:
- Around line 176-177: The code uses meta.id directly for sessionId which can be
malformed and later break UUID checks in readSessionMessages/getSessionPath;
update the block around sessionId (where meta.id || entry.name is assigned) to
validate meta.id (e.g., using an existing UUID validator or a small isValidUuid
helper), prefer a valid meta.id, fall back to entry.name only if it's valid, and
if neither is a valid identifier skip adding the session metadata (and
optionally log/debug a warning); ensure readSessionMessages and getSessionPath
will only see validated IDs.
---
Duplicate comments:
In `@src/main/storage/copilot-cli-session-storage.ts`:
- Around line 153-154: Current code unconditionally lowercases resolved paths
(e.g., normalizedProjectPath and normalizedExistingProjectPath) causing
incorrect matches on case-sensitive filesystems; change it to only normalize
case on Windows by checking process.platform === 'win32' (resolve paths with
path.resolve(...) first, then call toLowerCase() only when platform is 'win32')
and apply the same conditional behavior to all places where you currently
lowercase both paths before comparing (the variables used in project session
lookup and removal).
- Around line 278-280: The current catch blocks that swallow errors when reading
workspace.yaml (the try/catch around the ownership verification logic in
copilot-cli-session-storage.ts) "fail open" and allow session access if the file
can’t be read; change those catch blocks to deny access instead: when
reading/parsing workspace.yaml fails, log the error and return a negative auth
result (e.g., return false or throw an authorization error) from the
ownership-check function rather than allowing access. Apply the same fix to both
occurrences (the catch at the shown block and the similar catch around lines
~360-362) so failed reads consistently reject the session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23c00f6f-ad1c-40ff-a350-78dc2a03d5ca
📒 Files selected for processing (4)
src/main/parsers/agent-output-parser.tssrc/main/parsers/copilot-cli-output-parser.tssrc/main/process-manager/handlers/ExitHandler.tssrc/main/storage/copilot-cli-session-storage.ts
…date Phase 2 doc Address CI review findings: 1. Move accumulatedOutputTokens from singleton parser to per-session ManagedProcess.copilotAccumulatedTokens. Parser now reports per-event tokens via usage field on each assistant.message. StdoutHandler accumulates on ManagedProcess and injects the total into the result event. Removes resetState() from parser interface and ExitHandler. Fixes concurrent session token leak. 2. Validate meta.id with isValidSessionId() in listSessions before using it as sessionId. Skips sessions with malformed IDs. 3. Update Phase 2 doc to list actual BaseSessionStorage abstract methods (getSessionPath, deleteMessagePair, getSearchableMessages) instead of non-existent searchSessions/getGlobalStats. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/main/storage/copilot-cli-session-storage.ts (1)
164-181:⚠️ Potential issue | 🟡 MinorValidate the chosen
sessionIdbefore exposing it.Line 181 prefers
meta.idblindly. That letslistSessions()return IDs thatreadSessionMessages()andgetSessionPath()later reject, and stray non-UUID directories can still leak into the list. Prefer a validmeta.id, fall back toentry.nameonly if it is valid, otherwise skip the entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/storage/copilot-cli-session-storage.ts` around lines 164 - 181, listSessions() currently trusts meta.id or entry.name blindly which can return IDs that readSessionMessages()/getSessionPath() will later reject; validate the chosen sessionId (e.g., ensure it's a UUID or the same format your other helpers accept) before exposing it. In the code around parseWorkspaceYaml(), normalizePath(), and the block that sets sessionId = meta.id || entry.name, change logic to prefer meta.id only if it passes validation, otherwise use entry.name only if entry.name itself is valid, and if neither is valid skip the directory; ensure the same validation routine is used by readSessionMessages() and getSessionPath() so listed IDs are always resolvable.
🤖 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/Copilot-CLI-Phase2-Plan.md`:
- Around line 36-56: Update the plan to stop re-proposing already-implemented
session storage and instead list only the remaining gaps: remove the "New:
CopilotCliSessionStorage" and register/registering items and instead note that
CopilotCliSessionStorage and BaseSessionStorage implementations exist; update
the matrix to mark session storage as complete; add concise TODOs for SSH remote
support and follow-up hardening (e.g., SSH-aware
getSessionPath/listSessions/readSessionMessages and deleteMessagePair behavior)
and mention that initializeSessionStorages() should be verified for registration
consistency and supportsSessionStorage flags in capabilities.ts.
In `@src/main/storage/copilot-cli-session-storage.ts`:
- Around line 279-288: The code currently only denies access when meta.cwd
exists and differs; change both places that read parseWorkspaceYaml (the block
using workspacePath, parseWorkspaceYaml, normalizePath and the similar block
around lines 364-373) to require meta.cwd to be present and equal to
projectPath: after parsing yaml, if (!meta.cwd || normalizePath(meta.cwd) !==
normalizePath(projectPath)) return the empty result; this ensures missing or
malformed cwd also fails closed before returning messages or paths.
- Around line 326-328: The pagination logic uses || which treats 0 and negative
values incorrectly; update the code that sets limit and offset (the variables
named limit and offset used to compute paginatedMessages via messages.slice) to
use nullish coalescing (??) and then clamp values: set limit = Math.max(0,
(options?.limit ?? totalMessages)) and offset = Math.max(0, (options?.offset ??
0)) (or equivalent) before slicing so a provided 0 is honored and negatives are
prevented.
- Around line 190-197: firstMessage is incorrectly seeded from summary which
hides the real first user prompt; change the logic so firstMessage is initially
empty/undefined (do not assign summary), iterate events and set firstMessage to
the first user.message content found (String(event.data.content).slice(0,200));
after the loop, if no user message was found then set firstMessage = summary as
a fallback; apply the identical change to the other similar block around the
second occurrence (the block that currently mirrors lines 220-226) so both
places use the same fallback behavior.
- Around line 105-124: The loadSessionEvents function currently swallows all
read errors and returns an empty array; update it to only treat a missing file
as recoverable by catching the error from fs.readFile, check if error.code ===
'ENOENT' and return [] in that case, but re-throw any other errors so they
surface to callers/Sentry; also capture/parses errors when JSON.parse fails
(keep skipping unparseable lines) but ensure the outer try/catch uses the caught
error variable to perform the ENOENT check and rethrow otherwise.
---
Duplicate comments:
In `@src/main/storage/copilot-cli-session-storage.ts`:
- Around line 164-181: listSessions() currently trusts meta.id or entry.name
blindly which can return IDs that readSessionMessages()/getSessionPath() will
later reject; validate the chosen sessionId (e.g., ensure it's a UUID or the
same format your other helpers accept) before exposing it. In the code around
parseWorkspaceYaml(), normalizePath(), and the block that sets sessionId =
meta.id || entry.name, change logic to prefer meta.id only if it passes
validation, otherwise use entry.name only if entry.name itself is valid, and if
neither is valid skip the directory; ensure the same validation routine is used
by readSessionMessages() and getSessionPath() so listed IDs are always
resolvable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27434b22-0c3f-4b68-bbb5-952c51a87c27
📒 Files selected for processing (3)
docs/Copilot-CLI-Phase2-Plan.mdsrc/__tests__/main/agents/detector.test.tssrc/main/storage/copilot-cli-session-storage.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/__tests__/main/parsers/copilot-cli-output-parser.test.ts (1)
415-438: Add regression coverage forsession.error/data.messagepayloads.The suite currently exercises
errorobjects and strings, but not the parser’s other supported error shapes. A small case forsession.errorand one for{ type: 'error', data: { message: ... } }would lock down the special-case extraction path that the parser now depends on.Also applies to: 546-562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/parsers/copilot-cli-output-parser.test.ts` around lines 415 - 438, Add two test cases to the 'error events' suite that cover the parser's other supported error shapes: one that passes a JSON line where a session object contains an error (e.g., { type: 'error', session: { error: { message: '...' } } }) and one where the payload is { type: 'error', data: { message: '...' } }; for each call parser.parseJsonLine(...) and assert event?.type === 'error' and event?.text equals the message string to lock down the session.error and data.message extraction paths in parseJsonLine.src/main/parsers/copilot-cli-output-parser.ts (1)
238-247: Fall back todetailedContentbefore emitting blank tool output.The local result shape includes both
contentanddetailedContent, but onlycontentis used here. If Copilot returns a completed tool payload with onlydetailedContent, the UI will show an empty tool result even though output was available.Suggested fix
- let output = result?.content || ''; + let output = result?.content ?? result?.detailedContent ?? '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/parsers/copilot-cli-output-parser.ts` around lines 238 - 247, The parser currently only uses result.content and can emit an empty tool output when only result.detailedContent is present; update the logic that initializes output (from msg.data?.result) to prefer result.content but fall back to result.detailedContent before defaulting to '' (i.e., use result.content || result.detailedContent || ''), then apply the existing MAX_TOOL_OUTPUT_LENGTH truncation on that chosen string; references: msg.data?.result, result, output, MAX_TOOL_OUTPUT_LENGTH.
🤖 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/parsers/copilot-cli-output-parser.ts`:
- Around line 375-379: The branch that detects structured errors only uses
obj.error, so payloads like {type: 'error', data: {message: '...'}} are missed;
update the error-handling branch in copilot-cli-output-parser (the block setting
parsedJson and errorText) to also consider obj.data?.message as a fallback for
extractErrorText (e.g., call extractErrorText on obj.error first and if falsy
call it on obj.data?.message or use a null-coalescing expression), then keep the
same parsedJson assignment and normalize 'Unknown error' to null so
detectErrorFromParsed() and transformMessage() see the message correctly.
---
Nitpick comments:
In `@src/__tests__/main/parsers/copilot-cli-output-parser.test.ts`:
- Around line 415-438: Add two test cases to the 'error events' suite that cover
the parser's other supported error shapes: one that passes a JSON line where a
session object contains an error (e.g., { type: 'error', session: { error: {
message: '...' } } }) and one where the payload is { type: 'error', data: {
message: '...' } }; for each call parser.parseJsonLine(...) and assert
event?.type === 'error' and event?.text equals the message string to lock down
the session.error and data.message extraction paths in parseJsonLine.
In `@src/main/parsers/copilot-cli-output-parser.ts`:
- Around line 238-247: The parser currently only uses result.content and can
emit an empty tool output when only result.detailedContent is present; update
the logic that initializes output (from msg.data?.result) to prefer
result.content but fall back to result.detailedContent before defaulting to ''
(i.e., use result.content || result.detailedContent || ''), then apply the
existing MAX_TOOL_OUTPUT_LENGTH truncation on that chosen string; references:
msg.data?.result, result, output, MAX_TOOL_OUTPUT_LENGTH.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87db6243-96be-453c-a710-41878210c858
📒 Files selected for processing (5)
src/__tests__/main/parsers/copilot-cli-output-parser.test.tssrc/main/parsers/copilot-cli-output-parser.tssrc/main/process-manager/handlers/StdoutHandler.tssrc/main/process-manager/types.tssrc/main/storage/copilot-cli-session-storage.ts
✅ Files skipped from review due to trivial changes (1)
- src/main/process-manager/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/process-manager/handlers/StdoutHandler.ts
Author
|
@greptile what do you think |
|
Solid PR overall — comprehensive, well-structured, and the iterative review cycle clearly improved it. Here's my read: Strengths:
What the review cycle caught and fixed:
Remaining concerns I'd flag:
Verdict: Mergeable if items 1 and 3 are addressed or explicitly deferred with a comment. The core integration is sound and the review cycle did its job. Tip: You can customize Greptile's behavior for this repo with |
…in Copilot CLI parser Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… session history - Revert assistant.message with content+toolRequests back to type: text (not result) — these are mid-turn messages before tool execution, emitting result prematurely consumes the StdoutHandler result slot - Replace custom slice(offset, offset+limit) pagination with BaseSessionStorage.applyMessagePagination() so session history loads from the end (most recent first), matching all other storage implementations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… suppression - Add sendPromptViaStdinRaw to GroomingProcessManager interface and spawn call so copilot-cli grooming sends prompts via stdin instead of CLI args (same class of bug previously fixed for tab naming) - Narrow MCP stderr suppression from /mcp.*server/i (matches any MCP message) to /mcp server .*(start|connect|ready|loaded|initializ)/i so only startup noise is suppressed and real MCP failures (timeouts, crashes) flow through Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
|
Fixed regression The On exit, Fixed by adding |
Author
|
@greptile one more |
… take effect getStdinFlags returned false (not undefined) on macOS/Linux and for SSH sessions. Since the IPC handler uses ?? to fall back to the agent definition default, false blocked the fallback — copilot-cli sendPromptViaStdinRaw:true was overridden to false, causing too many arguments when the prompt was appended as a CLI arg instead of sent via stdin. Now returns undefined for non-applicable platforms so ?? correctly falls through to agent.sendPromptViaStdinRaw. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…arsing isStreamJsonMode checked for --format but not --output-format. Since String.includes is used, --output-format does not match --format as a substring. Without stream-json mode, copilot-cli JSONL was accumulated in a batch buffer and dumped as raw JSON on process exit instead of being parsed line-by-line through the output parser. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
scriptease
force-pushed
the
feat/copilot-cli-agent-rc
branch
from
a57dbee to
8f2b52d
Compare
This was referenced Apr 23, 2026
Collaborator
|
Hey @scriptease — thanks for this PR. Rather than merging any one of the three concurrent Copilot-CLI PRs in isolation, we rolled a combined integration into a Agent id/display is |
Collaborator
|
Superseded by Copilot-RC combined merge. Shipping in v0.16.12-RC. |
pedramamini
added a commit
that referenced
this pull request
Apr 23, 2026
Combines work from three PRs: - #566 (nolanmclark) — broad Copilot integration: images, SSH-remote session storage, hidden-progress UI, slash commands, Windows path probes, read-only CLI enforcement, group chat moderation, usage stats - #701 (dwizzzle) — original copilot-cli agent implementation and JSONL parser foundation (absorbed via #805's lineage) - #805 (scriptease) — models.dev multi-model picker (cherry-picked as per-process helpers), parser refinements Drops Aider from new-agent flows; removes Qwen3 wizard tile. Shipping in v0.16.12-RC. Co-authored-by: Nolan Clark <nolanmclark@gmail.com> Co-authored-by: David Weston <david@dwizzzle.com> Co-authored-by: Florian Agsteiner <florian.agsteiner@caperwhite.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
copilot) as a fully integrated agent in Maestro, with JSONL output parsing, session storage, error detection, and batch mode supportMulti-Model Support
Copilot CLI uniquely supports switching between models from different providers through a single agent. The model picker fetches available models from the
github-copilotprovider on models.dev and also reads the user's configured model from~/.copilot/config.json.Currently available models (via models.dev):
claude-opus-4.6gpt-5.4gemini-3.1-pro-previewgrok-code-fast-1claude-opus-4.5gpt-5.4-minigemini-3-pro-previewclaude-sonnet-4.6gpt-5.2gemini-3-flash-previewclaude-sonnet-4.5gpt-5.2-codexgemini-2.5-proclaude-sonnet-4gpt-5.1claude-opus-41gpt-5.1-codexclaude-haiku-4.5gpt-5.1-codex-maxgpt-5.1-codex-minigpt-5gpt-5-minigpt-4.1gpt-4oKnown Issue: Gemini models not yet available
Gemini models appear in the models.dev catalog and the model picker but are not yet accepted by the Copilot CLI binary. Selecting one produces:
This is tracked upstream at github/copilot-cli#1703. The error is handled gracefully — users see a message directing them to check GitHub Copilot feature settings.
Contributors
What's included
Core integration:
copilotbinary,--output-format jsonJSONL output,--allow-allbatch mode, stdin prompt deliveryassistant.message_delta,assistant.message,result,session.errorevents~/.copilot/session-state/<uuid>/(workspace.yaml + events.jsonl)Model picker:
github-copilotprovider)~/.copilot/config.jsonmodelBug fixes from review:
copilot-clionly (was affecting OpenCode and Factory Droid)sendPromptViaStdinRawflagcopilot-clitoSUPPORTED_AGENTSin NewInstanceModalTest plan
Summary by CodeRabbit
Release Notes
New Features
Documentation