From 8c1656bf5630353e0475ceabc46ca2cba9243d8f Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Wed, 15 Oct 2025 14:29:16 -0700 Subject: [PATCH] Don't always fall back on a git clone when installing extensions (#11229) --- packages/cli/src/config/extension.test.ts | 187 +++++++++++++++++- packages/cli/src/config/extension.ts | 22 ++- .../cli/src/config/extensions/github.test.ts | 16 +- packages/cli/src/config/extensions/github.ts | 135 ++++++++++--- 4 files changed, 314 insertions(+), 46 deletions(-) diff --git a/packages/cli/src/config/extension.test.ts b/packages/cli/src/config/extension.test.ts index b5af3a5792..4aa1ad7081 100644 --- a/packages/cli/src/config/extension.test.ts +++ b/packages/cli/src/config/extension.test.ts @@ -29,11 +29,11 @@ import { ExtensionDisableEvent, ExtensionEnableEvent, } from '@google/gemini-cli-core'; -import { execSync } from 'node:child_process'; import { SettingScope } from './settings.js'; import { isWorkspaceTrusted } from './trustedFolders.js'; import { createExtension } from '../test-utils/createExtension.js'; import { ExtensionEnablementManager } from './extensions/extensionEnablement.js'; +import { join } from 'node:path'; const mockGit = { clone: vi.fn(), @@ -47,6 +47,17 @@ const mockGit = { path: vi.fn(), }; +const mockDownloadFromGithubRelease = vi.hoisted(() => vi.fn()); + +vi.mock('./extensions/github.js', async (importOriginal) => { + const original = + await importOriginal(); + return { + ...original, + downloadFromGitHubRelease: mockDownloadFromGithubRelease, + }; +}); + vi.mock('simple-git', () => ({ simpleGit: vi.fn((path: string) => { mockGit.path.mockReturnValue(path); @@ -123,8 +134,6 @@ describe('extension tests', () => { source: undefined, }); vi.spyOn(process, 'cwd').mockReturnValue(tempWorkspaceDir); - vi.mocked(execSync).mockClear(); - Object.values(mockGit).forEach((fn) => fn.mockReset()); }); afterEach(() => { @@ -813,6 +822,11 @@ describe('extension tests', () => { ); }); mockGit.getRemotes.mockResolvedValue([{ name: 'origin' }]); + mockDownloadFromGithubRelease.mockResolvedValue({ + success: false, + failureReason: 'no release data', + type: 'github-release', + }); await installOrUpdateExtension( { source: gitUrl, type: 'git' }, @@ -1087,6 +1101,173 @@ This extension will run the following MCP servers: ), ).rejects.toThrow('Invalid extension name: "bad_name"'); }); + + describe('installing from github', () => { + const gitUrl = 'https://github.com/google/gemini-test-extension.git'; + const extensionName = 'gemini-test-extension'; + + beforeEach(() => { + // Mock the git clone behavior for github installs that fallback to it. + mockGit.clone.mockImplementation(async (_, destination) => { + fs.mkdirSync(path.join(mockGit.path(), destination), { + recursive: true, + }); + fs.writeFileSync( + path.join(mockGit.path(), destination, EXTENSIONS_CONFIG_FILENAME), + JSON.stringify({ name: extensionName, version: '1.0.0' }), + ); + }); + mockGit.getRemotes.mockResolvedValue([{ name: 'origin' }]); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('should install from a github release successfully', async () => { + const targetExtDir = path.join(userExtensionsDir, extensionName); + mockDownloadFromGithubRelease.mockResolvedValue({ + success: true, + tagName: 'v1.0.0', + type: 'github-release', + }); + + const tempDir = path.join(tempHomeDir, 'temp-ext'); + fs.mkdirSync(tempDir, { recursive: true }); + createExtension({ + extensionsDir: tempDir, + name: extensionName, + version: '1.0.0', + }); + vi.spyOn(ExtensionStorage, 'createTmpDir').mockResolvedValue( + join(tempDir, extensionName), + ); + + await installOrUpdateExtension( + { source: gitUrl, type: 'github-release' }, + async () => true, + ); + + expect(fs.existsSync(targetExtDir)).toBe(true); + const metadataPath = path.join(targetExtDir, INSTALL_METADATA_FILENAME); + expect(fs.existsSync(metadataPath)).toBe(true); + const metadata = JSON.parse(fs.readFileSync(metadataPath, 'utf-8')); + expect(metadata).toEqual({ + source: gitUrl, + type: 'github-release', + releaseTag: 'v1.0.0', + }); + }); + + it('should fallback to git clone if github release download fails and user consents', async () => { + mockDownloadFromGithubRelease.mockResolvedValue({ + success: false, + failureReason: 'failed to download asset', + errorMessage: 'download failed', + type: 'github-release', + }); + const requestConsent = vi.fn().mockResolvedValue(true); + + await installOrUpdateExtension( + { source: gitUrl, type: 'github-release' }, // Use github-release to force consent + requestConsent, + ); + + // It gets called once to ask for a git clone, and once to consent to + // the actual extension features. + expect(requestConsent).toHaveBeenCalledTimes(2); + expect(requestConsent).toHaveBeenCalledWith( + expect.stringContaining( + 'Would you like to attempt to install via "git clone" instead?', + ), + ); + expect(mockGit.clone).toHaveBeenCalled(); + const metadataPath = path.join( + userExtensionsDir, + extensionName, + INSTALL_METADATA_FILENAME, + ); + const metadata = JSON.parse(fs.readFileSync(metadataPath, 'utf-8')); + expect(metadata.type).toBe('git'); + }); + + it('should throw an error if github release download fails and user denies consent', async () => { + mockDownloadFromGithubRelease.mockResolvedValue({ + success: false, + errorMessage: 'download failed', + type: 'github-release', + }); + const requestConsent = vi.fn().mockResolvedValue(false); + + await expect( + installOrUpdateExtension( + { source: gitUrl, type: 'github-release' }, + requestConsent, + ), + ).rejects.toThrow( + `Failed to install extension ${gitUrl}: download failed`, + ); + + expect(requestConsent).toHaveBeenCalledExactlyOnceWith( + expect.stringContaining( + 'Would you like to attempt to install via "git clone" instead?', + ), + ); + expect(mockGit.clone).not.toHaveBeenCalled(); + }); + + it('should fallback to git clone without consent if no release data is found on first install', async () => { + mockDownloadFromGithubRelease.mockResolvedValue({ + success: false, + failureReason: 'no release data', + type: 'github-release', + }); + const requestConsent = vi.fn().mockResolvedValue(true); + + await installOrUpdateExtension( + { source: gitUrl, type: 'git' }, + requestConsent, + ); + + // We should not see the request to use git clone, this is a repo that + // has no github releases so it is the only install method. + expect(requestConsent).toHaveBeenCalledExactlyOnceWith( + expect.stringContaining( + 'Installing extension "gemini-test-extension"', + ), + ); + expect(mockGit.clone).toHaveBeenCalled(); + const metadataPath = path.join( + userExtensionsDir, + extensionName, + INSTALL_METADATA_FILENAME, + ); + const metadata = JSON.parse(fs.readFileSync(metadataPath, 'utf-8')); + expect(metadata.type).toBe('git'); + }); + + it('should ask for consent if no release data is found for an existing github-release extension', async () => { + mockDownloadFromGithubRelease.mockResolvedValue({ + success: false, + failureReason: 'no release data', + errorMessage: 'No release data found', + type: 'github-release', + }); + const requestConsent = vi.fn().mockResolvedValue(true); + + await installOrUpdateExtension( + { source: gitUrl, type: 'github-release' }, // Note the type + requestConsent, + ); + + expect(requestConsent).toHaveBeenCalledWith( + expect.stringContaining( + 'Would you like to attempt to install via "git clone" instead?', + ), + ); + expect(mockGit.clone).toHaveBeenCalled(); + }); + }); }); describe('uninstallExtension', () => { diff --git a/packages/cli/src/config/extension.ts b/packages/cli/src/config/extension.ts index bfaed90743..2d097120c4 100644 --- a/packages/cli/src/config/extension.ts +++ b/packages/cli/src/config/extension.ts @@ -464,16 +464,26 @@ export async function installOrUpdateExtension( installMetadata.type === 'github-release' ) { tempDir = await ExtensionStorage.createTmpDir(); - try { - const result = await downloadFromGitHubRelease( - installMetadata, - tempDir, - ); + const result = await downloadFromGitHubRelease(installMetadata, tempDir); + if (result.success) { installMetadata.type = result.type; installMetadata.releaseTag = result.tagName; - } catch (_error) { + } else if ( + // This repo has no github releases, and wasn't explicitly installed + // from a github release, unconditionally just clone it. + (result.failureReason === 'no release data' && + installMetadata.type === 'git') || + // Otherwise ask the user if they would like to try a git clone. + (await requestConsent( + `Error downloading github release for ${installMetadata.source} with the following error: ${result.errorMessage}.\n\nWould you like to attempt to install via "git clone" instead?`, + )) + ) { await cloneFromGit(installMetadata, tempDir); installMetadata.type = 'git'; + } else { + throw new Error( + `Failed to install extension ${installMetadata.source}: ${result.errorMessage}`, + ); } localSourcePath = tempDir; } else if ( diff --git a/packages/cli/src/config/extensions/github.test.ts b/packages/cli/src/config/extensions/github.test.ts index bd5ac27ba6..0bbd7fd856 100644 --- a/packages/cli/src/config/extensions/github.test.ts +++ b/packages/cli/src/config/extensions/github.test.ts @@ -348,21 +348,21 @@ describe('git extension helpers', () => { describe('parseGitHubRepoForReleases', () => { it('should parse owner and repo from a full GitHub URL', () => { const source = 'https://github.com/owner/repo.git'; - const { owner, repo } = parseGitHubRepoForReleases(source); + const { owner, repo } = parseGitHubRepoForReleases(source)!; expect(owner).toBe('owner'); expect(repo).toBe('repo'); }); it('should parse owner and repo from a full GitHub URL without .git', () => { const source = 'https://github.com/owner/repo'; - const { owner, repo } = parseGitHubRepoForReleases(source); + const { owner, repo } = parseGitHubRepoForReleases(source)!; expect(owner).toBe('owner'); expect(repo).toBe('repo'); }); it('should parse owner and repo from a full GitHub URL with a trailing slash', () => { const source = 'https://github.com/owner/repo/'; - const { owner, repo } = parseGitHubRepoForReleases(source); + const { owner, repo } = parseGitHubRepoForReleases(source)!; expect(owner).toBe('owner'); expect(repo).toBe('repo'); }); @@ -374,23 +374,21 @@ describe('git extension helpers', () => { ); }); - it('should fail on a non-GitHub URL', () => { + it('should return null on a non-GitHub URL', () => { const source = 'https://example.com/owner/repo.git'; - expect(() => parseGitHubRepoForReleases(source)).toThrow( - 'Invalid GitHub repository source: https://example.com/owner/repo.git. Expected "owner/repo" or a github repo uri.', - ); + expect(parseGitHubRepoForReleases(source)).toBe(null); }); it('should parse owner and repo from a shorthand string', () => { const source = 'owner/repo'; - const { owner, repo } = parseGitHubRepoForReleases(source); + const { owner, repo } = parseGitHubRepoForReleases(source)!; expect(owner).toBe('owner'); expect(repo).toBe('repo'); }); it('should handle .git suffix in repo name', () => { const source = 'owner/repo.git'; - const { owner, repo } = parseGitHubRepoForReleases(source); + const { owner, repo } = parseGitHubRepoForReleases(source)!; expect(owner).toBe('owner'); expect(repo).toBe('repo'); }); diff --git a/packages/cli/src/config/extensions/github.ts b/packages/cli/src/config/extensions/github.ts index 68e9a3b149..ef95cb26fe 100644 --- a/packages/cli/src/config/extensions/github.ts +++ b/packages/cli/src/config/extensions/github.ts @@ -79,16 +79,22 @@ export async function cloneFromGit( export function parseGitHubRepoForReleases(source: string): { owner: string; repo: string; -} { +} | null { // Default to a github repo path, so `source` can be just an org/repo const parsedUrl = URL.parse(source, 'https://github.com'); + if (!parsedUrl) { + throw new Error(`Invalid repo URL: ${source}`); + } // The pathname should be "/owner/repo". const parts = parsedUrl?.pathname .substring(1) .split('/') // Remove the empty segments, fixes trailing slashes .filter((part) => part !== ''); - if (parts?.length !== 2 || parsedUrl?.host !== 'github.com') { + if (parsedUrl?.host !== 'github.com') { + return null; + } + if (parts?.length !== 2) { throw new Error( `Invalid GitHub repository source: ${source}. Expected "owner/repo" or a github repo uri.`, ); @@ -110,7 +116,7 @@ export async function fetchReleaseFromGithub( repo: string, ref?: string, allowPreRelease?: boolean, -): Promise { +): Promise { if (ref) { return await fetchJson( `https://api.github.com/repos/${owner}/${repo}/releases/tags/${ref}`, @@ -120,9 +126,14 @@ export async function fetchReleaseFromGithub( if (!allowPreRelease) { // Grab the release that is tagged as the "latest", github does not allow // this to be a pre-release so we can blindly grab it. - return await fetchJson( - `https://api.github.com/repos/${owner}/${repo}/releases/latest`, - ); + try { + return await fetchJson( + `https://api.github.com/repos/${owner}/${repo}/releases/latest`, + ); + } catch (_) { + // This can fail if there is no release marked latest. In that case + // we want to just try the pre-release logic below. + } } // If pre-releases are allowed, we just grab the most recent release. @@ -130,7 +141,7 @@ export async function fetchReleaseFromGithub( `https://api.github.com/repos/${owner}/${repo}/releases?per_page=1`, ); if (releases.length === 0) { - throw new Error('No releases found'); + return null; } return releases[0]; } @@ -206,7 +217,14 @@ export async function checkForExtensionUpdate( console.error(`No "source" provided for extension.`); return ExtensionUpdateState.ERROR; } - const { owner, repo } = parseGitHubRepoForReleases(source); + const repoInfo = parseGitHubRepoForReleases(source); + if (!repoInfo) { + console.error( + `Source is not a valid GitHub repository for release checks: ${source}`, + ); + return ExtensionUpdateState.ERROR; + } + const { owner, repo } = repoInfo; const releaseData = await fetchReleaseFromGithub( owner, @@ -214,6 +232,9 @@ export async function checkForExtensionUpdate( installMetadata.ref, installMetadata.allowPreRelease, ); + if (!releaseData) { + return ExtensionUpdateState.ERROR; + } if (releaseData.tag_name !== releaseTag) { return ExtensionUpdateState.UPDATE_AVAILABLE; } @@ -226,28 +247,57 @@ export async function checkForExtensionUpdate( return ExtensionUpdateState.ERROR; } } + export interface GitHubDownloadResult { - tagName: string; + tagName?: string; type: 'git' | 'github-release'; + success: boolean; + failureReason?: + | 'failed to fetch release data' + | 'no release data' + | 'no release asset found' + | 'failed to download asset' + | 'failed to extract asset' + | 'unknown'; + errorMessage?: string; } + export async function downloadFromGitHubRelease( installMetadata: ExtensionInstallMetadata, destination: string, ): Promise { const { source, ref, allowPreRelease: preRelease } = installMetadata; - const { owner, repo } = parseGitHubRepoForReleases(source); + let releaseData: GithubReleaseData | null = null; try { - const releaseData = await fetchReleaseFromGithub( - owner, - repo, - ref, - preRelease, - ); - if (!releaseData) { - throw new Error( - `No release data found for ${owner}/${repo} at tag ${ref}`, - ); + const parts = parseGitHubRepoForReleases(source); + if (!parts) { + return { + failureReason: 'no release data', + success: false, + type: 'github-release', + errorMessage: `Not a github repo: ${source}`, + }; + } + const { owner, repo } = parts; + + try { + releaseData = await fetchReleaseFromGithub(owner, repo, ref, preRelease); + if (!releaseData) { + return { + failureReason: 'no release data', + success: false, + type: 'github-release', + errorMessage: `No release data found for ${owner}/${repo} at tag ${ref}`, + }; + } + } catch (error) { + return { + failureReason: 'failed to fetch release data', + success: false, + type: 'github-release', + errorMessage: `Failed to fetch release data for ${owner}/${repo} at tag ${ref}: ${getErrorMessage(error)}`, + }; } const asset = findReleaseAsset(releaseData.assets); @@ -266,9 +316,13 @@ export async function downloadFromGitHubRelease( } } if (!archiveUrl) { - throw new Error( - `No assets found for release with tag ${releaseData.tag_name}`, - ); + return { + failureReason: 'no release asset found', + success: false, + type: 'github-release', + tagName: releaseData.tag_name, + errorMessage: `No assets found for release with tag ${releaseData.tag_name}`, + }; } let downloadedAssetPath = path.join( destination, @@ -280,9 +334,29 @@ export async function downloadFromGitHubRelease( downloadedAssetPath += '.zip'; } - await downloadFile(archiveUrl, downloadedAssetPath); + try { + await downloadFile(archiveUrl, downloadedAssetPath); + } catch (error) { + return { + failureReason: 'failed to download asset', + success: false, + type: 'github-release', + tagName: releaseData.tag_name, + errorMessage: `Failed to download asset from ${archiveUrl}: ${getErrorMessage(error)}`, + }; + } - await extractFile(downloadedAssetPath, destination); + try { + await extractFile(downloadedAssetPath, destination); + } catch (error) { + return { + failureReason: 'failed to extract asset', + success: false, + type: 'github-release', + tagName: releaseData.tag_name, + errorMessage: `Failed to extract asset from ${downloadedAssetPath}: ${getErrorMessage(error)}`, + }; + } // For regular github releases, the repository is put inside of a top level // directory. In this case we should see exactly two file in the destination @@ -316,11 +390,16 @@ export async function downloadFromGitHubRelease( return { tagName: releaseData.tag_name, type: 'github-release', + success: true, }; } catch (error) { - throw new Error( - `Failed to download release from ${installMetadata.source}: ${getErrorMessage(error)}`, - ); + return { + failureReason: 'unknown', + success: false, + type: 'github-release', + tagName: releaseData?.tag_name, + errorMessage: `Failed to download release from ${installMetadata.source}: ${getErrorMessage(error)}`, + }; } }