-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Adds support for mcp server installation #4428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Draft until issues with the installer itself are addressed by the team that maintains it. |
383578b
to
b4f75a9
Compare
a72d602
to
ae46eb1
Compare
a134d87
to
97a6526
Compare
97a6526
to
28e01ee
Compare
1c0ba9b
to
1b59716
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we are conflating the CLI install with MCP "setup" and they are different/distinct tasks that should be handled as such.
The auto-install is only of the CLI, not MCP, and we should attempt to "auto-install" the CLI until successful (or some number of successive failures)
src/env/node/gk/cli/integration.ts
Outdated
switch (env.appName) { | ||
case 'Visual Studio Code': | ||
break; | ||
case 'Visual Studio Code - Insiders': | ||
appName = 'vscode-insiders'; | ||
break; | ||
case 'Cursor': | ||
appName = 'cursor'; | ||
break; | ||
case 'Windsurf': | ||
appName = 'windsurf'; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We reference in "VS Code - Exploration" (MS-provided adhoc build) and "VSCodium" (currently at 1.103.2) as well in other areas. Should we add these as well?
case 'Visual Studio Code - Exploration':
app = 'code-exploration';
break;
case 'VSCodium':
app = 'codium';
break;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those aren't supported by the "mcp install" command for the time being and should hit the default case and fail for now.
If we want them supported we should ask the CLI team to add them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked in a new getHostAppName
helper on main
that we should use here. And IMO we should send whatever it is to the CLI and let them say its not supported, so that way the CLI can update and we dont need to for additional support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eamodio I am now using that helper. I still need to transform the app name from that helper because, for example, we use code
and the command is expecting vscode
, etc.
I also always continue to the command if the app name exists, and if they show an error in the output I send failure telemetry and kick out early. Check the latest commit.
@eamodio This process is not installing a full-fledged GK CLI. It is installing something called "gk core", which is a lightweight version of the CLI designed to basically just support the MCP and a few other things. Should I still go ahead with calling all of this logic "install CLI"? |
docs/telemetry-events.md
Outdated
```typescript | ||
{ | ||
'attempts': number, | ||
'autoInstall': boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we get a version number or anything after the install? Because that would be great to have.
docs/telemetry-events.md
Outdated
### mcp/setup/completed | ||
|
||
> Sent when GitKraken MCP setup is completed | ||
|
||
```typescript | ||
{ | ||
'requiresUserCompletion': boolean | ||
} | ||
``` | ||
|
||
### mcp/setup/failed | ||
|
||
> Sent when GitKraken MCP setup fails | ||
|
||
```typescript | ||
{ | ||
'error.message': string, | ||
'reason': string | ||
} | ||
``` | ||
|
||
### mcp/setup/started | ||
|
||
> Sent when GitKraken MCP setup is started | ||
|
||
```typescript | ||
void | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure we have "source" plumbed in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the cli version would be great too
"gitlens.ai.mcp.install": { | ||
"label": "Install GitKraken MCP Server", | ||
"commandPalette": "gitlens:enabled && !gitlens:untrusted && gitlens:gk:organization:ai:enabled" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually we will want versions of this command to other usages (e.g. home, etc) so we can understand the usage and make sure source
is correct
c1a0b30
to
746e78b
Compare
src/env/node/gk/cli/integration.ts
Outdated
await window.withProgress( | ||
{ | ||
location: ProgressLocation.Notification, | ||
title: 'Setting up MCP integration...', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title: 'Setting up MCP integration...', | |
title: 'Setting up the GitKraken MCP...', |
src/env/node/gk/cli/integration.ts
Outdated
case CLIInstallErrorReason.WebEnvironmentUnsupported: | ||
void window.showErrorMessage( | ||
'MCP installation is not supported in the web environment.', | ||
); | ||
failureReason = 'web environment unsupported'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine as a fallback, but we shouldn't allow the command on the Web and kick out at the start.
@@ -311,6 +311,10 @@ | |||
"icon": "$(sparkle)", | |||
"commandPalette": "gitlens:enabled && !gitlens:untrusted && gitlens:gk:organization:ai:enabled" | |||
}, | |||
"gitlens.ai.mcp.install": { | |||
"label": "Install GitKraken MCP Server", | |||
"commandPalette": "gitlens:enabled && !gitlens:untrusted && gitlens:gk:organization:ai:enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add isWeb
(I think that is the context key) here to not show this on the web
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not isWeb
, or at least that's not working as intended because using && !isWeb
in the "when" clause prevents the command from appearing even when I'm not on web.
src/env/node/gk/cli/integration.ts
Outdated
let cliInstall = this.container.storage.get('gk:cli:install'); | ||
let cliPath = this.container.storage.get('gk:cli:path'); | ||
let cliProxyFileExists = true; | ||
if (cliPath != null) { | ||
try { | ||
await workspace.fs.stat( | ||
Uri.joinPath(Uri.file(cliPath), getPlatform() === 'windows' ? 'gk.exe' : 'gk'), | ||
); | ||
} catch { | ||
cliProxyFileExists = false; | ||
} | ||
} | ||
if (cliInstall?.status !== 'completed' || cliPath == null || !cliProxyFileExists) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest delegating all of this to this.installCLI()
as this will need to check these same things. And have that method return the path/install data this method needs.
src/env/node/gk/cli/integration.ts
Outdated
cliInstall = this.container.storage.get('gk:cli:install'); | ||
cliPath = this.container.storage.get('gk:cli:path'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHould be returned by this.installCLI
src/env/node/gk/cli/integration.ts
Outdated
}, | ||
); | ||
} catch (ex) { | ||
let failureReason = 'unknown error'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let failureReason = 'unknown error'; | |
'GitKraken MCP installation is not supported on the web.', |
CLIInstallErrorReason.ProxyFetch, | ||
undefined, | ||
`${response.status} ${response.statusText}`, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to throw if this already exists right?
throw new CLIInstallError( | ||
CLIInstallErrorReason.ProxyDownload, | ||
undefined, | ||
'Downloaded proxy archive data is empty', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the file already exists? We should overwrite it or nuke it first.
CLIInstallErrorReason.CoreDirectory, | ||
ex instanceof Error ? ex : undefined, | ||
ex instanceof Error ? ex.message : undefined, | ||
); | ||
} | ||
|
||
// Write the installer to the extension storage | ||
try { | ||
await workspace.fs.writeFile(cliProxyZipFilePath, new Uint8Array(cliProxyZipFileDownloadData)); | ||
} catch (ex) { | ||
throw new CLIInstallError( | ||
CLIInstallErrorReason.ProxyDownload, | ||
ex instanceof Error ? ex : undefined, | ||
'Failed to write proxy archive to global storage', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels fragile -- maybe we should use an unzip lib?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we can deal with that later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will need to test this on linux
src/env/node/gk/cli/integration.ts
Outdated
} | ||
} | ||
|
||
return cliVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we care about the status if the path exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't understand this comment based on the line it's attached to
@eamodio I tried addressing as many of your comments as I could, but most of them don't make sense because the code they are attached to got shifted. Would you mind resubmitting any that I did not address, with the latest on the branch? |
Closes #4357