feat(llm): add GitHub Copilot provider via Copilot CLI ACP mode - #10529
Draft
eliandoran wants to merge 5 commits into
Draft
feat(llm): add GitHub Copilot provider via Copilot CLI ACP mode#10529eliandoran wants to merge 5 commits into
eliandoran wants to merge 5 commits into
Conversation
Adds a `copilot-agent` LLM provider mirroring the Claude Agent design: it drives the user's installed GitHub Copilot CLI in ACP mode (`copilot --acp`) as a subprocess, so users with a Copilot subscription can use the in-app chat without an API key. Authentication is owned by the CLI (`copilot login`, or credentials shared with another editor). - acp_client.ts: dependency-free ACP (ndjson JSON-RPC over stdio) client - copilot_agent.ts: chatChunks() provider — session mapping/resume, streaming, live model catalog, fail-closed tool permission policy - copilot_binary.ts: bring-your-own-binary resolver (TRILIUM_COPILOT_PATH or `copilot` on PATH), Windows .cmd-shim aware - copilot_mcp_endpoint.ts: private loopback HTTP MCP endpoint exposing note tools to the agent (ACP takes MCP servers by URL, not in-process) - transcript.ts: hash/replay helpers extracted from claude_agent.ts and shared between both agent providers - register the provider server-side (llm/index.ts) and client-side (AddProviderModal card, icon, translations) Note tools are pre-approved via `--allow-tool=trilium` and built-in file/shell tools denied via `--deny-tool`; the permission callback denies anything reaching it, so the worst case is a note tool failing to run, never a shell command on the server host. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
🖥️ App preview is ready! 🔗 Preview URL: https://pr-10529.trilium-app.pages.dev ✅ All checks passed This preview will be updated automatically with new commits. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new GitHub Copilot Agent provider (copilot-agent) that drives the GitHub Copilot CLI in Agent Client Protocol (ACP) mode, allowing users to use the in-app chat with their Copilot subscription. It includes a transport-only ACP client, a loopback MCP endpoint to expose note tools, and shared transcript helpers extracted from the Claude Agent provider. The review feedback highlights several robustness and security improvements, including enforcing a size limit on the request body in the private MCP endpoint to prevent Denial of Service (DoS) crashes, preventing a potential crash in the ACP client's line handler if a line parses to null, handling abort signals immediately in the generator loop to avoid hanging, registering a generic error handler on the MCP HTTP server, wrapping synchronous send calls in a try-catch block to prevent resource leaks, supporting both direct and wrapped content blocks in tool outputs, and adding defensive checks for falsy or non-array content in buildPromptBlocks to prevent TypeErrors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+116
to
+123
| async function readJsonBody(req: http.IncomingMessage): Promise<unknown> { | ||
| const chunks: Buffer[] = []; | ||
| for await (const chunk of req) { | ||
| chunks.push(chunk as Buffer); | ||
| } | ||
| const text = Buffer.concat(chunks).toString("utf8"); | ||
| return text ? JSON.parse(text) : undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
Reading the entire request body into memory without a size limit poses a Denial of Service (DoS) risk via Out-Of-Memory (OOM) crashes if a client sends an excessively large payload. Enforce a reasonable maximum size limit (e.g., 10MB) while buffering the request chunks.
Suggested change
| async function readJsonBody(req: http.IncomingMessage): Promise<unknown> { | |
| const chunks: Buffer[] = []; | |
| for await (const chunk of req) { | |
| chunks.push(chunk as Buffer); | |
| } | |
| const text = Buffer.concat(chunks).toString("utf8"); | |
| return text ? JSON.parse(text) : undefined; | |
| } | |
| async function readJsonBody(req: http.IncomingMessage): Promise<unknown> { | |
| const chunks: Buffer[] = []; | |
| let totalLength = 0; | |
| const MAX_SIZE = 10 * 1024 * 1024; // 10MB limit | |
| for await (const chunk of req) { | |
| totalLength += chunk.length; | |
| if (totalLength > MAX_SIZE) { | |
| throw new Error("Request body too large"); | |
| } | |
| chunks.push(chunk as Buffer); | |
| } | |
| const text = Buffer.concat(chunks).toString("utf8"); | |
| return text ? JSON.parse(text) : undefined; | |
| } |
Comment on lines
+152
to
+158
| let message: JsonRpcMessage; | ||
| try { | ||
| message = JSON.parse(line) as JsonRpcMessage; | ||
| } catch { | ||
| // Not part of the protocol stream (e.g. a stray banner) — ignore. | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
If the subprocess outputs a line containing null (which is valid JSON), JSON.parse(line) will return null. Accessing message.id on the next line will then throw a TypeError: Cannot read properties of null and crash the line handler. Ensure the parsed message is a non-null object before proceeding.
let message: JsonRpcMessage;
try {
const parsed = JSON.parse(line);
if (!parsed || typeof parsed !== "object") {
return;
}
message = parsed as JsonRpcMessage;
} catch {
// Not part of the protocol stream (e.g. a stray banner) — ignore.
return;
}
Comment on lines
+250
to
+280
| const onAbort = () => { | ||
| if (sessionId) { | ||
| client?.notify("session/cancel", { sessionId }); | ||
| } | ||
| }; | ||
| signal?.addEventListener("abort", onAbort, { once: true }); | ||
|
|
||
| try { | ||
| const promptPromise = client.request<{ stopReason?: string }>( | ||
| "session/prompt", | ||
| { sessionId, prompt: buildPromptBlocks(lastMessage.content, prefix) }, | ||
| PROMPT_TIMEOUT_MS | ||
| ); | ||
|
|
||
| // Drain updates as they arrive until the prompt resolves (and | ||
| // then whatever is still queued). | ||
| let result: { stopReason?: string } | undefined; | ||
| let promptError: unknown; | ||
| const done = promptPromise | ||
| .then(r => { result = r; }) | ||
| .catch(err => { promptError = err; }) | ||
| .finally(() => wakeup?.()); | ||
|
|
||
| let finished = false; | ||
| void done.then(() => { finished = true; wakeup?.(); }); | ||
| while (!finished || chunkQueue.length > 0) { | ||
| if (chunkQueue.length === 0) { | ||
| await new Promise<void>(resolve => { wakeup = resolve; }); | ||
| wakeup = undefined; | ||
| continue; | ||
| } |
Contributor
There was a problem hiding this comment.
If the agent subprocess hangs or takes too long to respond to session/cancel upon abort, the generator loop will remain suspended on wakeup for up to 15 minutes (PROMPT_TIMEOUT_MS). Wake up the generator immediately on abort and break the loop so that the finally block can run and dispose of the client/subprocess immediately.
const onAbort = () => {
if (sessionId) {
client?.notify("session/cancel", { sessionId });
}
wakeup?.();
};
signal?.addEventListener("abort", onAbort, { once: true });
try {
const promptPromise = client.request<{ stopReason?: string }>(
"session/prompt",
{ sessionId, prompt: buildPromptBlocks(lastMessage.content, prefix) },
PROMPT_TIMEOUT_MS
);
// Drain updates as they arrive until the prompt resolves (and
// then whatever is still queued).
let result: { stopReason?: string } | undefined;
let promptError: unknown;
const done = promptPromise
.then(r => { result = r; })
.catch(err => { promptError = err; })
.finally(() => wakeup?.());
let finished = false;
void done.then(() => { finished = true; wakeup?.(); });
while ((!finished && !signal?.aborted) || chunkQueue.length > 0) {
if (chunkQueue.length === 0) {
if (signal?.aborted) {
break;
}
await new Promise<void>(resolve => { wakeup = resolve; });
wakeup = undefined;
continue;
}
Comment on lines
+67
to
+70
| await new Promise<void>((resolve, reject) => { | ||
| server.once("error", reject); | ||
| server.listen(0, "127.0.0.1", () => resolve()); | ||
| }); |
Contributor
There was a problem hiding this comment.
If the HTTP server encounters an error after successfully starting to listen, the lack of an active error listener will cause the error to propagate as an unhandled exception, crashing the entire Node.js process. Register a generic error handler on the server once it starts listening.
await new Promise<void>((resolve, reject) => {
const onError = (err: Error) => reject(err);
server.once("error", onError);
server.listen(0, "127.0.0.1", () => {
server.removeListener("error", onError);
server.on("error", err => getLog().error(`Copilot MCP HTTP server error: ${err}`));
resolve();
});
});| reject(err); | ||
| } | ||
| }); | ||
| this.send({ jsonrpc: "2.0", id, method, params }); |
Contributor
There was a problem hiding this comment.
If this.send throws an error synchronously (e.g., if the stdin stream is destroyed/closed), the promise will reject but the timeout timer will remain active and the pending request ID will leak in this.pending. Wrap this.send in a try-catch block to clean up the timer and pending entry immediately.
try {
this.send({ jsonrpc: "2.0", id, method, params });
} catch (err) {
clearTimeout(timer);
this.pending.delete(id);
reject(err instanceof Error ? err : new Error(String(err)));
}
Comment on lines
+571
to
+577
| const texts = content | ||
| .map(item => { | ||
| if (item && typeof item === "object" && "content" in item) { | ||
| return extractText((item as { content?: AcpContentBlock }).content); | ||
| } | ||
| return ""; | ||
| }) |
Contributor
There was a problem hiding this comment.
In ACP/MCP, tool output content can be an array of content blocks directly (e.g., [{ type: "text", text: "..." }]) rather than wrapped in an object with a content property. Support both direct and wrapped content blocks to prevent empty tool results.
const texts = content
.map(item => {
if (item && typeof item === "object") {
if ("content" in item) {
return extractText((item as { content?: unknown }).content);
}
return extractText(item);
}
return "";
})
Comment on lines
+594
to
+598
| function buildPromptBlocks(content: string | LlmMessagePart[], prefix: string): AcpContentBlock[] { | ||
| if (typeof content === "string") { | ||
| const text = prefix ? `${prefix}\n\n${content}` : content; | ||
| return [{ type: "text", text }]; | ||
| } |
Contributor
There was a problem hiding this comment.
If content is falsy or not an array/string (e.g., if the message content is missing or malformed), buildPromptBlocks will throw a TypeError when trying to iterate over it. Add defensive checks to handle falsy or non-array content gracefully.
function buildPromptBlocks(content: string | LlmMessagePart[] | undefined | null, prefix: string): AcpContentBlock[] {
if (!content) {
return prefix ? [{ type: "text", text: prefix }] : [];
}
if (typeof content === "string") {
const text = prefix ? `${prefix}\n\n${content}` : content;
return [{ type: "text", text }];
}
if (!Array.isArray(content)) {
return prefix ? [{ type: "text", text: prefix }] : [];
}
Bundle ReportChanges will increase total bundle size by 6.81kB (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: client-esmAssets Changed:
Files in
view changes for bundle: standalone-esmAssets Changed:
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
Greptile SummaryThis PR adds a
Confidence Score: 5/5The change is safe to merge; both previously flagged concerns (stdin EPIPE crash, missing body-size cap) are addressed, and the new code handles subprocess lifecycle, permission denial, and secret-path isolation correctly. The implementation is thorough with 80+ new unit tests covering session resume/reseed, abort, permission denial, and stop-reason mapping. The two flagged observations are non-blocking edge cases that do not affect the happy path or the security boundary. No files require special attention before merging. The two minor behavioural edge cases are in Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+48
to
+56
| const AVAILABLE_MODELS: ModelInfo[] = [ | ||
| { id: "auto", name: "Auto", pricing: { input: 0, output: 0 }, isDefault: true, isSubscription: true }, | ||
| { id: "claude-sonnet-5", name: "Claude Sonnet 5", pricing: { input: 0, output: 0 }, costMultiplier: 1, isSubscription: true }, | ||
| { id: "claude-haiku-4.5", name: "Claude Haiku 4.5", pricing: { input: 0, output: 0 }, costMultiplier: 0.3, isSubscription: true }, | ||
| { id: "gpt-5.4", name: "GPT-5.4", pricing: { input: 0, output: 0 }, costMultiplier: 1, isSubscription: true }, | ||
| { id: "gpt-5.3-codex", name: "GPT-5.3-Codex", pricing: { input: 0, output: 0 }, costMultiplier: 1, isSubscription: true }, | ||
| { id: "gpt-5.4-mini", name: "GPT-5.4 mini", pricing: { input: 0, output: 0 }, costMultiplier: 0.3, isSubscription: true }, | ||
| { id: "gpt-5-mini", name: "GPT-5 mini", pricing: { input: 0, output: 0 }, costMultiplier: 0, isSubscription: true } | ||
| ]; |
Contributor
There was a problem hiding this comment.
AVAILABLE_MODELS is a static snapshot; when GitHub adds, renames, or removes models in a CLI update the list silently goes stale — users won't see new models, or will try to select models that the CLI no longer accepts. The session/new response from the CLI already carries the authoritative model catalog (the CLI uses it for its own picker), so the data is available without an extra round-trip. Consider persisting the model list returned by the first session/new call (similar to how resolveCopilotBinaryPath caches the probe result) instead of maintaining a hand-rolled snapshot.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Addresses PR review feedback on the Copilot Agent provider. Each fix below
is covered by a test that fails against the unpatched source.
Crashes:
- acp_client: stdin had no "error" listener, so an EPIPE from writing to
an already-exited subprocess (a late session/cancel, or an agent-request
reply resumed after dispose()) was re-thrown by EventEmitter and took
the server down.
- acp_client: a bare `null` line on stdout parses as valid JSON, then
threw a TypeError on the property reads that follow. Only accept objects.
Hang:
- copilot_agent: on abort the drain loop stayed suspended until the agent
honoured session/cancel, holding the subprocess alive for up to
PROMPT_TIMEOUT_MS (15 min). Wake the loop on abort and stop once the
queue is drained. Bailing early leaves the session's real history
unknown, so the session mapping is no longer recorded for an aborted
turn -- a later turn reseeds instead of resuming a diverged session.
Robustness:
- copilot_mcp_endpoint: cap the buffered request body. The public /mcp
route is bounded by Express's body parser; this raw listener bypassed it.
- copilot_mcp_endpoint: keep an error listener attached past startup, so
a post-listen error is logged rather than silently dropped.
- acp_client: a synchronous send() failure left the timeout armed and the
id stranded in `pending` until it elapsed.
- copilot_agent: flattenToolContent now accepts direct content blocks as
well as ACP's wrapped form, which previously fell back to raw JSON.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Writing tests for the loopback MCP endpoint surfaced a security control
that never fired:
allowedHosts: [req.headers.host ?? ""].filter(h => /^127\.0\.0\.1:\d+$/.test(h))
The allowlist was derived from the very header it validates. A rebound
Host filters down to an empty array, and the MCP SDK skips the check
entirely when the list is empty (webStandardStreamableHttp.js:113,
`if (this._allowedHosts && this._allowedHosts.length > 0)`), so
enableDnsRebindingProtection could never reject anything -- the 128-bit
secret path was the only real access control. The allowlist is now
pinned to the address we bind and hand to the agent, so a rebound Host
gets a 403. A request with no Host at all is refused earlier, with a 400
from the transport.
Coverage for the files this PR adds, all now at 100% lines/functions:
- copilot_mcp_endpoint.ts 1.88% -> 100% (54/54 lines, 15/15 fns)
- copilot_agent.ts 62.72% -> 100% (215/215, 38/38)
- acp_client.ts 80.72% -> 100% (79/79, 19/19)
- llm/index.ts 50% -> 100% (50/50, 13/13)
Notable additions: the session/load resume-and-reseed paths, generateTitle,
the update collector's mute/filter/tool-call branches, LRU session
eviction, the ACP request timeout and synchronous-send failure, and the
endpoint's failed-start, body-cap and error-response paths.
wrapSystemInstructions' empty-prompt guard is marked /* v8 ignore */: it
is unreachable because buildSystemPrompt always appends the Markdown
hints and there is exactly one caller.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
npm installs a bare extensionless `copilot` (a POSIX sh script for Git Bash) alongside `copilot.cmd`. findOnPath probed the empty extension first, so on Windows it resolved the sh script, which Node cannot spawn -- surfacing as the misleading "Found GitHub Copilot CLI at ... but it failed to run (spawn ENOENT)". Probe the Windows-native extensions and skip the bare name entirely, matching claude_binary.ts. Skipping (rather than keeping "" as a fallback) means a directory holding only the unusable sh script yields the actionable "not found -- install it" message instead. This was masked for anyone running the server from a VS Code terminal: VS Code puts its own bundled CLI on PATH, and that directory ships only copilot.bat/.ps1, so the bad candidate never existed there. Users who follow the error message's own `npm install -g @github/copilot` advice and start the server from a plain shell hit it every time. The existing Windows test passed because it stubbed only copilot.cmd as existing; the new one stubs the real npm layout (both files present). Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a
copilot-agentLLM provider that lets users with a GitHub Copilot subscription use Trilium's built-in chat without an API key — mirroring the existing Claude Agent design, but driving the user's installed GitHub Copilot CLI in ACP mode (copilot --acp) as a subprocess. Authentication is owned entirely by the CLI (copilot login, or credentials shared with another editor integration on the machine).What's here
acp_client.ts— dependency-free Agent Client Protocol client (newline-delimited JSON-RPC over stdio): framing, request/response correlation, agent→client requests, subprocess lifecycle.copilot_agent.ts— the provider. ImplementschatChunks(): maps chat notes to ACP sessions (resume when the transcript matches, reseed via history replay when it diverged), streamssession/updatenotifications intoLlmStreamChunks, selects models viasession/set_model, and exposes the CLI's live model catalog with premium-request multipliers mapped ontocostMultiplier.copilot_binary.ts— bring-your-own-binary resolver (TRILIUM_COPILOT_PATH→copiloton PATH), Windows.cmd-shim aware.copilot_mcp_endpoint.ts— private loopback HTTP MCP endpoint (random port + 128-bit secret path) exposing note tools to the agent. ACP takes MCP servers by URL, not in-process, and this is deliberately independent of the user-facingmcpEnabledtoggle.transcript.ts— hash/replay helpers extracted fromclaude_agent.tsand shared between both agent providers.llm/index.ts) and client-side (AddProviderModalcard, icon, translations).Security
Note tools are pre-approved via
--allow-tool=triliumand built-in file/shell/network tools are denied via--deny-tool. The permission callback is fail-closed: it denies anything that reaches it, so the worst case is a note tool failing to run — never a shell command on the server host. Verified live against CLI 1.0.71 (--deny-tool=shellblocks shell before any permission request reaches the callback).Validation
acp_client,copilot_agent,copilot_binary), typecheck clean, lint clean.Known constraints (draft — needs decision before merge)
🤖 Generated with Claude Code