feat(ai-copilot): replace the REST transport with the Copilot CLI - #17919
Conversation
953b36f to
71e2428
Compare
|
Thank you for the pullrequest! |
|
Thanks a lot for testing this, and for confirming that the models show up for you as well! 👍
|
|
About the |
I'm not sure I understand what you mean by "agent level integration". But re prompt caching: I would assume, we break the internal prompt caching by merging and recreating a new session. I'm not very familiar with the Copilot quota accounting, but I also guess this will eat up the quota more quickly than it would need to. So not necessarily as part of this PR but as a follow-up, it might be good to look into mapping a Theia session to a Copilot CLI session. We did something similar with the Claude Code integration. The only caveat is that when users switch language models (from something else to Copilot), there is no corresponding Copilot CLI session yet. As a fallback, we could do what we do right now (merging and creating a new session) at the cost is that they'd loose prompt caching after each switch, but I think this is acceptable. |
71e2428 to
3da7373
Compare
|
@planger I don't think Copilot has any cross-model caching, so the fallback should be fine. |
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Replaces the Copilot REST transport in @theia/ai-copilot with the official Copilot CLI, spoken to over @github/copilot-sdk. The CLI is a prerequisite on the backend host (CopilotCliLocator), its bundled SDK copy is loaded at runtime (CopilotSdkLoader) against a hand-mirrored type file plus a CI drift check, the sign-in is driven via copilot login --device-code against a private temporary Copilot home whose token is taken over into Theia's keystore, and Theia's tools are bridged into the CLI session as custom tools while the agent prompt becomes the session system message. Old OAuth app, CopilotOAuthConfig, CopilotLanguageModel and the openai dependency are gone.
The approach holds up: the credential isolation (private home, useLoggedInUser: false, token variables stripped from the child env), the empty runtime mode with availableTools: ['custom:*'], the per-request session that is deleted afterwards, and the @preDestroy teardown per frontend connection all look deliberate and are backed by a solid test suite (137 tests pass, lint and compile clean here).
Where a maintainer should look:
- Tool results are handed to the CLI raw instead of through
formatToolCallContentForModel, so MCP HTML results and denied-confirmation errors reach the model as JSON blobs. - Sign-in lifecycle in
CopilotCliAuthProvider: theloginResultdeferred can reject with no consumer attached (unhandled rejection),cancelLogindrops it without settling it (awaitForSignInRPC then hangs), and there is no@preDestroy, so a login in flight survives a closed connection. - Docs:
Migration.mdlists a preference (ai-features.copilot.useSdk) that never existed on master, the changelog still has a#TBDlink, and the README does not cover the case @richter-mvtec hit (a CLI without a bundled SDK). - Behaviour change:
ai-features.copilot.enterpriseUrlnow silently has no effect while signed in.
Also worth a maintainer decision: the fragile parts by nature are the ones that parse CLI output (device code, account label, failure line) and scrape the persisted token by regex from the private home. They are well tested and well commented, but they will break on CLI changes with no compile-time signal, unlike the SDK mirror. I did not verify anything in the UI, since this needs a live Copilot subscription and an installed CLI.
3da7373 to
147ad01
Compare
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Follow-up review after the latest push. Went through the diff since my last review (commit 3da7373 to 147ad01) and it addresses every point I raised:
- Tool results now go through
formatToolCallContentForModelfor the model while the structured result still reaches the stream/UI. CopilotCliAuthProvider: the loginDeferrednow has an observer attached immediately (no more unhandled rejection),cancelLoginresolves it withfalsebefore dropping the reference (no more hungwaitForSignIn), and a new@preDestroyhook cancels an in-flight login when the frontend connection closes. All three are backed by the newcancelLoginunit test and the provider is bound in the same connection-scoped container asCopilotSdkClientProvider, so the lifecycle hook actually fires.- The previously non-localized strings across
CopilotCliAuthProvider,CopilotSdkClientProviderand the language models manager are now wrapped innls.localize. enterpriseUrlchanges while signed in now trigger a notification prompting a fresh sign-in instead of silently doing nothing.- Docs/CI cleanup: Node 22 to 24 in the mirror workflow, the changelog
#TBDlink, the staleuseSdkpreference inMigration.md, the missingcopilot-sdk-mappersexport, and the leftover blanketanydisable are all gone. The README now documents the CLI-without-bundled-SDK case raised by @richter-mvtec.
I checked each fix against the surrounding code (connection-scoped binding for the @preDestroy hook, the ToolHandler/formatToolCallContentForModel types, the DialogError shape used in the auth dialog) and didn't find anything newly broken. No new inline comments from me this round.
These previous comments can be resolved as they are now handled:
- raw tool results reaching the model instead of going through
formatToolCallContentForModel loginResultdeferred could reject with no consumer attachedcancelLoginleft a pendingwaitForLoginhanging foreverCopilotCliAuthProviderhad no@preDestroy, leaking an in-flight login on connection closeai-features.copilot.enterpriseUrlsilently had no effect while signed in- README missing the CLI-without-bundled-SDK case
doc/Migration.mdreferencing a preference that never existed on master
I can't resolve them myself as I would need write permission on this repository.
- replace the direct REST transport with the official Copilot CLI: access to the models is granted per OAuth application, and the built-in application is not entitled for the current lineup, so the REST path only ever exposed a small legacy subset - sign in from within the application by driving `copilot login --device-code` and reporting its progress in the existing dialog - keep the credentials in the application: the login runs against a private Copilot home, its token is taken over into the Theia credential store, and signing out removes that entry without touching credentials of the machine - accept nothing else: the token is passed to the runtime explicitly and tokens are removed from its environment, so neither those nor a sign-in of the GitHub CLI can make the application appear signed in - bridge tool calling: the tools of a request are declared to the CLI, their handlers delegate back to Theia, and every invocation is reported as a tool call so that it is recorded and rendered as usual - send the prompt of a Theia agent as the system message of the session, replacing the agent instructions of the CLI; the sections that tool calling and safety rely on are kept - run the runtime without the ambient behaviour of the CLI: only the tools of a request are available, while host instructions, skills, memory, the session store, host git operations and plugins stay off, and a permission request that managed policy reserves for a decision is answered as unconfirmable instead of being left pending - treat the CLI as a prerequisite on the backend host rather than as something the application ships: it is located through the new `ai-features.copilot.executablePath` preference, `COPILOT_CLI_PATH`, the installation of the application, the `PATH` and the global `npm` directory, following the launcher that a global installation leaves behind, and its native executable is spawned, since the backend has no Node.js at hand and a packaged application cannot execute a binary from inside its archive - load the SDK from the CLI that serves the requests instead of depending on `@github/copilot-sdk`, whose dependency on the CLI would carry a large proprietary binary into the tree and the lockfile of every application; the part of its API that is used is mirrored in `copilot-sdk-types.ts`, attributed in `NOTICE.md` and checked against a released SDK by `scripts/copilot-sdk-mirror` and the scheduled workflow that runs it, and an installed SDK is still used when the CLI does not carry one - keep the machine clean: the session of a request is deleted once it is answered, the runtime runs against a Copilot home below the configuration directory of Theia, and the CLI of a connection is stopped when it closes - report models as unavailable when they cannot be listed, rather than as ready, and offer a dated model release only when its family is not offered as well - mark the integration as experimental: its preferences carry the `experimental` tag, so the settings UI reports them as subject to change - remove the OAuth app sign-in, `CopilotOAuthConfig`, `CopilotLanguageModel` and the `openai` dependencies
147ad01 to
b8d214e
Compare
planger
left a comment
There was a problem hiding this comment.
Excellent, tested login with and without confirming in Copilot. Handles both cases gracefully. Tested streaming and tool calling, both work fine.
Thank you very much for bringing the Copilot integration finally to a useful state!
What it does
@theia/ai-copilotnow serves all requests through the official GitHub Copilot CLI instead of theCopilot REST API. GitHub grants access to the Copilot models per OAuth application, and Theia's is not
entitled for the current lineup, so the REST path only ever exposed a small legacy subset regardless of
the user's subscription. The CLI is entitled, so routing through it makes the current models available.
copilot login --device-code) and keeps using theexisting dialog.
taken over into Theia's credential store, and signing out removes that entry. A token in the
environment or a GitHub CLI sign-in is not used, so neither can make Theia appear signed in.
instructions of the CLI.
available, it uses a Copilot home below Theia's configuration directory, the session of a request is
deleted once answered, and the CLI of a frontend connection is stopped when it closes.
experimentaltag.CopilotOAuthConfig,CopilotLanguageModeland theopenaidependencies. See the breaking changes and
doc/Migration.md.The CLI is a prerequisite on the backend host rather than something the application ships. Users install
it with
npm install -g @github/copilot, and it is located through the newai-features.copilot.executablePathpreference,COPILOT_CLI_PATH, the installation of theapplication, the
PATHand the globalnpmdirectory.Neither the CLI nor
@github/copilot-sdkis a dependency of the extension, since the SDK package pullsin the CLI as a large platform-specific binary that would land in the tree and lockfile of every
application. The SDK is loaded at runtime from the CLI that serves the requests, and the part of its API
that is used is mirrored in
copilot-sdk-types.ts, with a scheduled workflow checking the mirroragainst the released SDK.
Many thanks to @jdubois, whose #17629 introduced the SDK transport as an opt-in path and showed that this
is the way to reach the current models. This builds directly on that work: his
copilot-sdk-mappers.tsis kept and extended (session system message, de-duplication of dated model releases), while the client
provider and language model are reworked around the new authentication, tool handling and CLI lookup.
Kept as a draft until I have tested it on Windows: the CLI lookup has a Windows-specific path (a global
npm install puts a
copilot.cmdshim on thePATH, and thenpm root -gfallback goes through ashell) that is covered by unit tests but not by a real run. Feedback on the approach is welcome
meanwhile.
How to test
Requires an active GitHub Copilot subscription and the locally installed copilot CLI:
npm install -g @github/copilot.1. Sign in, and check which application you authorize
readable and copyable.
was "Theia Copilot OAuth Access" by eclipsesource, so seeing that name would mean the old flow is
still in use.
account.
2. Where the credentials and the sessions end up
theia-copilotentry, and no newcopilot-clientries.theia-copilot-login-*directory should be left behind in the temp directory.~/.copilotshould hold no token, andcopiloton the command line should not list the prompts sentfrom Theia; those live in
<Theia config dir>/copilotand are deleted per request.3. Models and chat
Copilot: using the Copilot CLI at ...andCopilot: discovered N models [...]with the current lineup, not just GPT-4o, and without both a family and its dated releases.
copilot/...model in the AI Configuration and send a chat message. The answer streams.4. Tool calling
this workspace have?" with the Universal agent.
5. Sign out and isolation
This is the point of the credential design, so it is worth a careful look.
theia-copilotkeyring entry is gone.gh auth loginon the machine must not make Theia appear signed in.Same for
GH_TOKENorGITHUB_TOKENset for the backend process.6. Finding the CLI, and the upgrade path
ai-features.copilot.executablePathempty, the CLI is found on thePATHor in the globalnpmdirectory. Starting the application from a desktop launcher rather than a shell is the interesting
case, since the npm prefix is often not on the
PATHthere.that the previous sign-in was removed and asks for a new one, and the old
theia-copilot-authkeyringentry is gone.
Optional, if available: a Copilot Business seat needs no host configuration, and for GitHub Enterprise
with data residency set
ai-features.copilot.enterpriseUrlbefore signing in.Follow-ups
Breaking changes
Attribution
Builds on the work of @jdubois in #17629, which is the basis of this change.
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers