diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f77d0f9152..5d08e91455 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -60,20 +60,41 @@ All submissions, including submissions by project members, require review. We use [GitHub pull requests](https://docs.github.com/articles/about-pull-requests) for this purpose. -If your pull request involves changes to `packages/cli` (the frontend), we -recommend running our automated frontend review tool. **Note: This tool is -currently experimental.** It helps detect common React anti-patterns, testing -issues, and other frontend-specific best practices that are easy to miss. +To assist with the review process, we provide an automated review tool that +helps detect common anti-patterns, testing issues, and other best practices that +are easy to miss. -To run the review tool, enter the following command from within Gemini CLI: +#### Using the automated review tool -```text -/review-frontend -``` +You can run the review tool in two ways: -Replace `` with your pull request number. Authors are encouraged to -run this on their own PRs for self-review, and reviewers should use it to -augment their manual review process. +1. **Using the helper script (Recommended):** We provide a script that + automatically handles checking out the PR into a separate worktree, + installing dependencies, building the project, and launching the review + tool. + + ```bash + ./scripts/review.sh [model] + ``` + + **Authors are strongly encouraged to run this script on their own PRs** + immediately after creation. This allows you to catch and fix simple issues + locally before a maintainer performs a full review. + + **Note on Models:** By default, the script uses the latest Pro model + (`gemini-3.1-pro-preview`). If you do not have enough Pro quota, you can run + it with the latest Flash model instead: + `./scripts/review.sh gemini-3-flash-preview`. + +2. **Manually from within Gemini CLI:** If you already have the PR checked out + and built, you can run the tool directly from the CLI prompt: + + ```text + /review-frontend + ``` + +Replace `` with your pull request number. Reviewers should use this +tool to augment, not replace, their manual review process. ### Self-assigning and unassigning issues diff --git a/docs/cli/cli-reference.md b/docs/cli/cli-reference.md index 6cafb7dd52..167801ca05 100644 --- a/docs/cli/cli-reference.md +++ b/docs/cli/cli-reference.md @@ -8,7 +8,8 @@ and parameters. | Command | Description | Example | | ---------------------------------- | ---------------------------------- | ------------------------------------------------------------ | | `gemini` | Start interactive REPL | `gemini` | -| `gemini "query"` | Query non-interactively, then exit | `gemini "explain this project"` | +| `gemini -p "query"` | Query non-interactively | `gemini -p "summarize README.md"` | +| `gemini "query"` | Query and continue interactively | `gemini "explain this project"` | | `cat file \| gemini` | Process piped content | `cat logs.txt \| gemini`
`Get-Content logs.txt \| gemini` | | `gemini -i "query"` | Execute and continue interactively | `gemini -i "What is the purpose of this project?"` | | `gemini -r "latest"` | Continue most recent session | `gemini -r "latest"` | @@ -20,9 +21,9 @@ and parameters. ### Positional arguments -| Argument | Type | Description | -| -------- | ----------------- | ------------------------------------------------------------------------------------------------------------------ | -| `query` | string (variadic) | Positional prompt. Defaults to one-shot mode. Use `-i/--prompt-interactive` to execute and continue interactively. | +| Argument | Type | Description | +| -------- | ----------------- | ---------------------------------------------------------------------------------------------------------- | +| `query` | string (variadic) | Positional prompt. Defaults to interactive mode in a TTY. Use `-p/--prompt` for non-interactive execution. | ## Interactive commands @@ -47,7 +48,7 @@ These commands are available within the interactive REPL. | `--version` | `-v` | - | - | Show CLI version number and exit | | `--help` | `-h` | - | - | Show help information | | `--model` | `-m` | string | `auto` | Model to use. See [Model Selection](#model-selection) for available values. | -| `--prompt` | `-p` | string | - | Prompt text. Appended to stdin input if provided. **Deprecated:** Use positional arguments instead. | +| `--prompt` | `-p` | string | - | Prompt text. Appended to stdin input if provided. Forces non-interactive mode. | | `--prompt-interactive` | `-i` | string | - | Execute prompt and continue in interactive mode | | `--sandbox` | `-s` | boolean | `false` | Run in a sandboxed environment for safer execution | | `--approval-mode` | - | string | `default` | Approval mode for tool execution. Choices: `default`, `auto_edit`, `yolo` | diff --git a/docs/cli/headless.md b/docs/cli/headless.md index 7de3287639..dd9a385313 100644 --- a/docs/cli/headless.md +++ b/docs/cli/headless.md @@ -6,7 +6,7 @@ structured text or JSON output without an interactive terminal UI. ## Technical reference Headless mode is triggered when the CLI is run in a non-TTY environment or when -providing a query as a positional argument without the interactive flag. +providing a query with the `-p` (or `--prompt`) flag. ### Output formats diff --git a/docs/cli/tutorials/automation.md b/docs/cli/tutorials/automation.md index fb1d8d48d2..4285cdcf3b 100644 --- a/docs/cli/tutorials/automation.md +++ b/docs/cli/tutorials/automation.md @@ -19,14 +19,15 @@ Headless mode runs Gemini CLI once and exits. It's perfect for: ## How to use headless mode -Run Gemini CLI in headless mode by providing a prompt as a positional argument. -This bypasses the interactive chat interface and prints the response to standard -output (stdout). +Run Gemini CLI in headless mode by providing a prompt with the `-p` (or +`--prompt`) flag. This bypasses the interactive chat interface and prints the +response to standard output (stdout). Positional arguments without the flag +default to interactive mode, unless the input or output is piped or redirected. Run a single command: ```bash -gemini "Write a poem about TypeScript" +gemini -p "Write a poem about TypeScript" ``` ## How to pipe input to Gemini CLI @@ -40,19 +41,19 @@ Pipe a file: **macOS/Linux** ```bash -cat error.log | gemini "Explain why this failed" +cat error.log | gemini -p "Explain why this failed" ``` **Windows (PowerShell)** ```powershell -Get-Content error.log | gemini "Explain why this failed" +Get-Content error.log | gemini -p "Explain why this failed" ``` Pipe a command: ```bash -git diff | gemini "Write a commit message for these changes" +git diff | gemini -p "Write a commit message for these changes" ``` ## Use Gemini CLI output in scripts @@ -78,7 +79,7 @@ one. echo "Generating docs for $file..." # Ask Gemini CLI to generate the documentation and print it to stdout - gemini "Generate a Markdown documentation summary for @$file. Print the + gemini -p "Generate a Markdown documentation summary for @$file. Print the result to standard output." > "${file%.py}.md" done ``` @@ -92,7 +93,7 @@ one. $newName = $_.Name -replace '\.py$', '.md' # Ask Gemini CLI to generate the documentation and print it to stdout - gemini "Generate a Markdown documentation summary for @$($_.Name). Print the result to standard output." | Out-File -FilePath $newName -Encoding utf8 + gemini -p "Generate a Markdown documentation summary for @$($_.Name). Print the result to standard output." | Out-File -FilePath $newName -Encoding utf8 } ``` @@ -214,7 +215,7 @@ wrapper that writes the message for you. # Ask Gemini to write the message echo "Generating commit message..." - msg=$(echo "$diff" | gemini "Write a concise Conventional Commit message for this diff. Output ONLY the message.") + msg=$(echo "$diff" | gemini -p "Write a concise Conventional Commit message for this diff. Output ONLY the message.") # Commit with the generated message git commit -m "$msg" @@ -251,7 +252,7 @@ wrapper that writes the message for you. # Ask Gemini to write the message Write-Host "Generating commit message..." - $msg = $diff | gemini "Write a concise Conventional Commit message for this diff. Output ONLY the message." + $msg = $diff | gemini -p "Write a concise Conventional Commit message for this diff. Output ONLY the message." # Commit with the generated message git commit -m "$msg" diff --git a/docs/reference/policy-engine.md b/docs/reference/policy-engine.md index 38a0b4d50c..c0a331d99d 100644 --- a/docs/reference/policy-engine.md +++ b/docs/reference/policy-engine.md @@ -219,6 +219,10 @@ Here is a breakdown of the fields available in a TOML policy rule: # A unique name for the tool, or an array of names. toolName = "run_shell_command" +# (Optional) The name of a subagent. If provided, the rule only applies to tool calls +# made by this specific subagent. +subagent = "generalist" + # (Optional) The name of an MCP server. Can be combined with toolName # to form a composite name like "mcpName__toolName". mcpName = "my-custom-server" diff --git a/docs/tools/file-system.md b/docs/tools/file-system.md index 09c792f84d..a6beb1d76d 100644 --- a/docs/tools/file-system.md +++ b/docs/tools/file-system.md @@ -67,7 +67,7 @@ Finds files matching specific glob patterns across the workspace. `Found 5 file(s) matching "*.ts" within src, sorted by modification time (newest first):\nsrc/file1.ts\nsrc/subdir/file2.ts...` - **Confirmation:** No. -## 5. `grep_search` (SearchText) +### `grep_search` (SearchText) `grep_search` searches for a regular expression pattern within the content of files in a specified directory. Can filter files by a glob pattern. Returns the @@ -103,7 +103,7 @@ lines containing matches, along with their file paths and line numbers. ``` - **Confirmation:** No. -## 6. `replace` (Edit) +### `replace` (Edit) `replace` replaces text within a file. By default, the tool expects to find and replace exactly ONE occurrence of `old_string`. If you want to replace multiple diff --git a/eslint.config.js b/eslint.config.js index d3a267f30a..a0a0429119 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -35,6 +35,11 @@ const commonRestrictedSyntaxRules = [ message: 'Do not throw string literals or non-Error objects. Throw new Error("...") instead.', }, + { + selector: 'CallExpression[callee.name="fetch"]', + message: + 'Use safeFetch() from "@/utils/fetch" instead of the global fetch() to ensure SSRF protection. If you are implementing a custom security layer, use an eslint-disable comment and explain why.', + }, ]; export default tseslint.config( diff --git a/package-lock.json b/package-lock.json index 85448711c7..8963837258 100644 --- a/package-lock.json +++ b/package-lock.json @@ -84,9 +84,9 @@ } }, "node_modules/@a2a-js/sdk": { - "version": "0.3.8", - "resolved": "https://registry.npmjs.org/@a2a-js/sdk/-/sdk-0.3.8.tgz", - "integrity": "sha512-vAg6JQbhOnHTzApsB7nGzCQ9r7PuY4GMr8gt88dIR8Wc8G8RSqVTyTmFeMurgzcYrtHYXS3ru2rnDoGj9UDeSw==", + "version": "0.3.10", + "resolved": "https://registry.npmjs.org/@a2a-js/sdk/-/sdk-0.3.10.tgz", + "integrity": "sha512-t6w5ctnwJkSOMRl6M9rn95C1FTHCPqixxMR0yWXtzhZXEnF6mF1NAK0CfKlG3cz+tcwTxkmn287QZC3t9XPgrA==", "license": "Apache-2.0", "dependencies": { "uuid": "^11.1.0" @@ -95,9 +95,17 @@ "node": ">=18" }, "peerDependencies": { + "@bufbuild/protobuf": "^2.10.2", + "@grpc/grpc-js": "^1.11.0", "express": "^4.21.2 || ^5.1.0" }, "peerDependenciesMeta": { + "@bufbuild/protobuf": { + "optional": true + }, + "@grpc/grpc-js": { + "optional": true + }, "express": { "optional": true } @@ -515,6 +523,12 @@ "node": ">=18" } }, + "node_modules/@bufbuild/protobuf": { + "version": "2.11.0", + "resolved": "https://registry.npmjs.org/@bufbuild/protobuf/-/protobuf-2.11.0.tgz", + "integrity": "sha512-sBXGT13cpmPR5BMgHE6UEEfEaShh5Ror6rfN3yEK5si7QVrtZg8LEPQb0VVhiLRUslD2yLnXtnRzG035J/mZXQ==", + "license": "(Apache-2.0 AND BSD-3-Clause)" + }, "node_modules/@bundled-es-modules/cookie": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/@bundled-es-modules/cookie/-/cookie-2.0.1.tgz", @@ -1582,18 +1596,36 @@ } }, "node_modules/@grpc/grpc-js": { - "version": "1.13.4", - "resolved": "https://registry.npmjs.org/@grpc/grpc-js/-/grpc-js-1.13.4.tgz", - "integrity": "sha512-GsFaMXCkMqkKIvwCQjCrwH+GHbPKBjhwo/8ZuUkWHqbI73Kky9I+pQltrlT0+MWpedCoosda53lgjYfyEPgxBg==", + "version": "1.14.3", + "resolved": "https://registry.npmjs.org/@grpc/grpc-js/-/grpc-js-1.14.3.tgz", + "integrity": "sha512-Iq8QQQ/7X3Sac15oB6p0FmUg/klxQvXLeileoqrTRGJYLV+/9tubbr9ipz0GKHjmXVsgFPo/+W+2cA8eNcR+XA==", "license": "Apache-2.0", "dependencies": { - "@grpc/proto-loader": "^0.7.13", + "@grpc/proto-loader": "^0.8.0", "@js-sdsl/ordered-map": "^4.4.2" }, "engines": { "node": ">=12.10.0" } }, + "node_modules/@grpc/grpc-js/node_modules/@grpc/proto-loader": { + "version": "0.8.0", + "resolved": "https://registry.npmjs.org/@grpc/proto-loader/-/proto-loader-0.8.0.tgz", + "integrity": "sha512-rc1hOQtjIWGxcxpb9aHAfLpIctjEnsDehj0DAiVfBlmT84uvR0uUtN2hEi/ecvWVjXUGf5qPF4qEgiLOx1YIMQ==", + "license": "Apache-2.0", + "dependencies": { + "lodash.camelcase": "^4.3.0", + "long": "^5.0.0", + "protobufjs": "^7.5.3", + "yargs": "^17.7.2" + }, + "bin": { + "proto-loader-gen-types": "build/bin/proto-loader-gen-types.js" + }, + "engines": { + "node": ">=6" + } + }, "node_modules/@grpc/proto-loader": { "version": "0.7.15", "resolved": "https://registry.npmjs.org/@grpc/proto-loader/-/proto-loader-0.7.15.tgz", @@ -17447,11 +17479,13 @@ "version": "0.34.0-nightly.20260304.28af4e127", "license": "Apache-2.0", "dependencies": { - "@a2a-js/sdk": "^0.3.8", + "@a2a-js/sdk": "^0.3.10", + "@bufbuild/protobuf": "^2.11.0", "@google-cloud/logging": "^11.2.1", "@google-cloud/opentelemetry-cloud-monitoring-exporter": "^0.21.0", "@google-cloud/opentelemetry-cloud-trace-exporter": "^3.0.0", "@google/genai": "1.41.0", + "@grpc/grpc-js": "^1.14.3", "@iarna/toml": "^2.2.5", "@joshua.litt/get-ripgrep": "^0.0.3", "@modelcontextprotocol/sdk": "^1.23.0", @@ -17489,6 +17523,7 @@ "html-to-text": "^9.0.5", "https-proxy-agent": "^7.0.6", "ignore": "^7.0.0", + "ipaddr.js": "^1.9.1", "js-yaml": "^4.1.1", "marked": "^15.0.12", "mime": "4.0.7", diff --git a/packages/cli/src/commands/mcp/list.test.ts b/packages/cli/src/commands/mcp/list.test.ts index aaaf667815..54534961dd 100644 --- a/packages/cli/src/commands/mcp/list.test.ts +++ b/packages/cli/src/commands/mcp/list.test.ts @@ -14,11 +14,16 @@ import { type Mock, } from 'vitest'; import { listMcpServers } from './list.js'; -import { loadSettings, mergeSettings } from '../../config/settings.js'; +import { + loadSettings, + mergeSettings, + type LoadedSettings, +} from '../../config/settings.js'; import { createTransport, debugLogger } from '@google/gemini-cli-core'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { ExtensionStorage } from '../../config/extensions/storage.js'; import { ExtensionManager } from '../../config/extension-manager.js'; +import { McpServerEnablementManager } from '../../config/mcp/index.js'; vi.mock('../../config/settings.js', async (importOriginal) => { const actual = @@ -45,6 +50,8 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { CONNECTED: 'CONNECTED', CONNECTING: 'CONNECTING', DISCONNECTED: 'DISCONNECTED', + BLOCKED: 'BLOCKED', + DISABLED: 'DISABLED', }, Storage: Object.assign( vi.fn().mockImplementation((_cwd: string) => ({ @@ -54,6 +61,7 @@ vi.mock('@google/gemini-cli-core', async (importOriginal) => { })), { getGlobalSettingsPath: () => '/tmp/gemini/settings.json', + getGlobalGeminiDir: () => '/tmp/gemini', }, ), GEMINI_DIR: '.gemini', @@ -96,6 +104,12 @@ describe('mcp list command', () => { beforeEach(() => { vi.resetAllMocks(); vi.spyOn(debugLogger, 'log').mockImplementation(() => {}); + McpServerEnablementManager.resetInstance(); + // Use a mock for isFileEnabled to avoid reading real files + vi.spyOn( + McpServerEnablementManager.prototype, + 'isFileEnabled', + ).mockResolvedValue(true); mockTransport = { close: vi.fn() }; mockClient = { @@ -265,7 +279,10 @@ describe('mcp list command', () => { mockClient.connect.mockResolvedValue(undefined); mockClient.ping.mockResolvedValue(undefined); - await listMcpServers(settingsWithAllowlist); + await listMcpServers({ + merged: settingsWithAllowlist, + isTrusted: true, + } as unknown as LoadedSettings); expect(debugLogger.log).toHaveBeenCalledWith( expect.stringContaining('allowed-server'), @@ -304,4 +321,56 @@ describe('mcp list command', () => { ), ); }); + + it('should display blocked status for servers in excluded list', async () => { + const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); + mockedLoadSettings.mockReturnValue({ + merged: { + ...defaultMergedSettings, + mcp: { + excluded: ['blocked-server'], + }, + mcpServers: { + 'blocked-server': { command: '/test/server' }, + }, + }, + isTrusted: true, + }); + + await listMcpServers(); + + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'blocked-server: /test/server (stdio) - Blocked', + ), + ); + expect(mockedCreateTransport).not.toHaveBeenCalled(); + }); + + it('should display disabled status for servers disabled via enablement manager', async () => { + const defaultMergedSettings = mergeSettings({}, {}, {}, {}, true); + mockedLoadSettings.mockReturnValue({ + merged: { + ...defaultMergedSettings, + mcpServers: { + 'disabled-server': { command: '/test/server' }, + }, + }, + isTrusted: true, + }); + + vi.spyOn( + McpServerEnablementManager.prototype, + 'isFileEnabled', + ).mockResolvedValue(false); + + await listMcpServers(); + + expect(debugLogger.log).toHaveBeenCalledWith( + expect.stringContaining( + 'disabled-server: /test/server (stdio) - Disabled', + ), + ); + expect(mockedCreateTransport).not.toHaveBeenCalled(); + }); }); diff --git a/packages/cli/src/commands/mcp/list.ts b/packages/cli/src/commands/mcp/list.ts index 421c822a55..a1df1a8027 100644 --- a/packages/cli/src/commands/mcp/list.ts +++ b/packages/cli/src/commands/mcp/list.ts @@ -6,8 +6,11 @@ // File for 'gemini mcp list' command import type { CommandModule } from 'yargs'; -import { type MergedSettings, loadSettings } from '../../config/settings.js'; -import type { MCPServerConfig } from '@google/gemini-cli-core'; +import { + type MergedSettings, + loadSettings, + type LoadedSettings, +} from '../../config/settings.js'; import { MCPServerStatus, createTransport, @@ -15,8 +18,13 @@ import { applyAdminAllowlist, getAdminBlockedMcpServersMessage, } from '@google/gemini-cli-core'; +import type { MCPServerConfig } from '@google/gemini-cli-core'; import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { ExtensionManager } from '../../config/extension-manager.js'; +import { + canLoadServer, + McpServerEnablementManager, +} from '../../config/mcp/index.js'; import { requestConsentNonInteractive } from '../../config/extensions/consent.js'; import { promptForSetting } from '../../config/extensions/extensionSettings.js'; import { exitCli } from '../utils.js'; @@ -61,13 +69,13 @@ export async function getMcpServersFromConfig( async function testMCPConnection( serverName: string, config: MCPServerConfig, + isTrusted: boolean, + activeSettings: MergedSettings, ): Promise { - const settings = loadSettings(); - // SECURITY: Only test connection if workspace is trusted or if it's a remote server. // stdio servers execute local commands and must never run in untrusted workspaces. const isStdio = !!config.command; - if (isStdio && !settings.isTrusted) { + if (isStdio && !isTrusted) { return MCPServerStatus.DISCONNECTED; } @@ -80,7 +88,7 @@ async function testMCPConnection( sanitizationConfig: { enableEnvironmentVariableRedaction: true, allowedEnvironmentVariables: [], - blockedEnvironmentVariables: settings.merged.advanced.excludedEnvVars, + blockedEnvironmentVariables: activeSettings.advanced.excludedEnvVars, }, emitMcpDiagnostic: ( severity: 'info' | 'warning' | 'error', @@ -105,7 +113,7 @@ async function testMCPConnection( debugLogger.log(message, error); } }, - isTrustedFolder: () => settings.isTrusted, + isTrustedFolder: () => isTrusted, }; let transport; @@ -135,14 +143,40 @@ async function testMCPConnection( async function getServerStatus( serverName: string, server: MCPServerConfig, + isTrusted: boolean, + activeSettings: MergedSettings, ): Promise { + const mcpEnablementManager = McpServerEnablementManager.getInstance(); + const loadResult = await canLoadServer(serverName, { + adminMcpEnabled: activeSettings.admin?.mcp?.enabled ?? true, + allowedList: activeSettings.mcp?.allowed, + excludedList: activeSettings.mcp?.excluded, + enablement: mcpEnablementManager.getEnablementCallbacks(), + }); + + if (!loadResult.allowed) { + if ( + loadResult.blockType === 'admin' || + loadResult.blockType === 'allowlist' || + loadResult.blockType === 'excludelist' + ) { + return MCPServerStatus.BLOCKED; + } + return MCPServerStatus.DISABLED; + } + // Test all server types by attempting actual connection - return testMCPConnection(serverName, server); + return testMCPConnection(serverName, server, isTrusted, activeSettings); } -export async function listMcpServers(settings?: MergedSettings): Promise { +export async function listMcpServers( + loadedSettingsArg?: LoadedSettings, +): Promise { + const loadedSettings = loadedSettingsArg ?? loadSettings(); + const activeSettings = loadedSettings.merged; + const { mcpServers, blockedServerNames } = - await getMcpServersFromConfig(settings); + await getMcpServersFromConfig(activeSettings); const serverNames = Object.keys(mcpServers); if (blockedServerNames.length > 0) { @@ -165,7 +199,12 @@ export async function listMcpServers(settings?: MergedSettings): Promise { for (const serverName of serverNames) { const server = mcpServers[serverName]; - const status = await getServerStatus(serverName, server); + const status = await getServerStatus( + serverName, + server, + loadedSettings.isTrusted, + activeSettings, + ); let statusIndicator = ''; let statusText = ''; @@ -178,6 +217,14 @@ export async function listMcpServers(settings?: MergedSettings): Promise { statusIndicator = chalk.yellow('…'); statusText = 'Connecting'; break; + case MCPServerStatus.BLOCKED: + statusIndicator = chalk.red('⛔'); + statusText = 'Blocked'; + break; + case MCPServerStatus.DISABLED: + statusIndicator = chalk.gray('○'); + statusText = 'Disabled'; + break; case MCPServerStatus.DISCONNECTED: default: statusIndicator = chalk.red('✗'); @@ -203,14 +250,14 @@ export async function listMcpServers(settings?: MergedSettings): Promise { } interface ListArgs { - settings?: MergedSettings; + loadedSettings?: LoadedSettings; } export const listCommand: CommandModule = { command: 'list', describe: 'List all configured MCP servers', handler: async (argv) => { - await listMcpServers(argv.settings); + await listMcpServers(argv.loadedSettings); await exitCli(); }, }; diff --git a/packages/cli/src/config/extension.test.ts b/packages/cli/src/config/extension.test.ts index f8e66bf8e2..38264b285a 100644 --- a/packages/cli/src/config/extension.test.ts +++ b/packages/cli/src/config/extension.test.ts @@ -31,6 +31,7 @@ import { loadSettings, createTestMergedSettings, SettingScope, + resetSettingsCacheForTesting, } from './settings.js'; import { isWorkspaceTrusted, @@ -161,6 +162,7 @@ describe('extension tests', () => { beforeEach(() => { vi.clearAllMocks(); + resetSettingsCacheForTesting(); keychainData = {}; mockKeychainStorage = { getSecret: vi diff --git a/packages/cli/src/config/settings.test.ts b/packages/cli/src/config/settings.test.ts index 5589ef11ba..7092f26a99 100644 --- a/packages/cli/src/config/settings.test.ts +++ b/packages/cli/src/config/settings.test.ts @@ -13,7 +13,7 @@ vi.mock('os', async (importOriginal) => { const actualOs = await importOriginal(); return { ...actualOs, - homedir: vi.fn(() => '/mock/home/user'), + homedir: vi.fn(() => path.resolve('/mock/home/user')), platform: vi.fn(() => 'linux'), }; }); @@ -76,6 +76,7 @@ import { LoadedSettings, sanitizeEnvVar, createTestMergedSettings, + resetSettingsCacheForTesting, } from './settings.js'; import { FatalConfigError, @@ -91,7 +92,7 @@ import { } from './settingsSchema.js'; import { createMockSettings } from '../test-utils/settings.js'; -const MOCK_WORKSPACE_DIR = '/mock/workspace'; +const MOCK_WORKSPACE_DIR = path.resolve(path.resolve('/mock/workspace')); // Use the (mocked) GEMINI_DIR for consistency const MOCK_WORKSPACE_SETTINGS_PATH = path.join( MOCK_WORKSPACE_DIR, @@ -102,6 +103,10 @@ const MOCK_WORKSPACE_SETTINGS_PATH = path.join( // A more flexible type for test data that allows arbitrary properties. type TestSettings = Settings & { [key: string]: unknown }; +// Helper to normalize paths for test assertions, making them OS-agnostic +const normalizePath = (p: string | fs.PathOrFileDescriptor) => + path.normalize(p.toString()); + vi.mock('fs', async (importOriginal) => { // Get all the functions from the real 'fs' module const actualFs = await importOriginal(); @@ -174,12 +179,15 @@ describe('Settings Loading and Merging', () => { beforeEach(() => { vi.resetAllMocks(); + resetSettingsCacheForTesting(); mockFsExistsSync = vi.mocked(fs.existsSync); mockFsMkdirSync = vi.mocked(fs.mkdirSync); mockStripJsonComments = vi.mocked(stripJsonComments); - vi.mocked(osActual.homedir).mockReturnValue('/mock/home/user'); + vi.mocked(osActual.homedir).mockReturnValue( + path.resolve('/mock/home/user'), + ); (mockStripJsonComments as unknown as Mock).mockImplementation( (jsonString: string) => jsonString, ); @@ -224,20 +232,25 @@ describe('Settings Loading and Merging', () => { }, ])( 'should load $scope settings if only $scope file exists', - ({ scope, path, content }) => { + ({ scope, path: p, content }) => { (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === path, + (pathLike: fs.PathLike) => + path.normalize(pathLike.toString()) === path.normalize(p), ); (fs.readFileSync as Mock).mockImplementation( - (p: fs.PathOrFileDescriptor) => { - if (p === path) return JSON.stringify(content); + (pathDesc: fs.PathOrFileDescriptor) => { + if (path.normalize(pathDesc.toString()) === path.normalize(p)) + return JSON.stringify(content); return '{}'; }, ); const settings = loadSettings(MOCK_WORKSPACE_DIR); - expect(fs.readFileSync).toHaveBeenCalledWith(path, 'utf-8'); + expect(fs.readFileSync).toHaveBeenCalledWith( + expect.stringContaining(path.basename(p)), + 'utf-8', + ); expect( settings[scope as 'system' | 'user' | 'workspace'].settings, ).toEqual(content); @@ -246,12 +259,14 @@ describe('Settings Loading and Merging', () => { ); it('should merge system, user and workspace settings, with system taking precedence over workspace, and workspace over user', () => { - (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => - p === getSystemSettingsPath() || - p === USER_SETTINGS_PATH || - p === MOCK_WORKSPACE_SETTINGS_PATH, - ); + (mockFsExistsSync as Mock).mockImplementation((p: fs.PathLike) => { + const normP = path.normalize(p.toString()); + return ( + normP === path.normalize(getSystemSettingsPath()) || + normP === path.normalize(USER_SETTINGS_PATH) || + normP === path.normalize(MOCK_WORKSPACE_SETTINGS_PATH) + ); + }); const systemSettingsContent = { ui: { theme: 'system-theme', @@ -290,11 +305,12 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) + const normP = path.normalize(p.toString()); + if (normP === path.normalize(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === USER_SETTINGS_PATH) + if (normP === path.normalize(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normP === path.normalize(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return ''; }, @@ -390,13 +406,13 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemDefaultsPath()) + if (normalizePath(p) === normalizePath(getSystemDefaultsPath())) return JSON.stringify(systemDefaultsContent); - if (p === getSystemSettingsPath()) + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return ''; }, @@ -449,11 +465,11 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -489,11 +505,11 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -523,11 +539,11 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -576,11 +592,12 @@ describe('Settings Loading and Merging', () => { 'should handle $description correctly', ({ path, content, expected }) => { (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === path, + (p: fs.PathLike) => normalizePath(p) === normalizePath(path), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === path) return JSON.stringify(content); + if (normalizePath(p) === normalizePath(path)) + return JSON.stringify(content); return '{}'; }, ); @@ -598,7 +615,8 @@ describe('Settings Loading and Merging', () => { it('should merge excludedProjectEnvVars with workspace taking precedence over user', () => { (mockFsExistsSync as Mock).mockImplementation( (p: fs.PathLike) => - p === USER_SETTINGS_PATH || p === MOCK_WORKSPACE_SETTINGS_PATH, + normalizePath(p) === normalizePath(USER_SETTINGS_PATH) || + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH), ); const userSettingsContent = { general: {}, @@ -611,9 +629,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return ''; }, @@ -643,15 +661,16 @@ describe('Settings Loading and Merging', () => { it('should default contextFileName to undefined if not in any settings file', () => { (mockFsExistsSync as Mock).mockImplementation( (p: fs.PathLike) => - p === USER_SETTINGS_PATH || p === MOCK_WORKSPACE_SETTINGS_PATH, + normalizePath(p) === normalizePath(USER_SETTINGS_PATH) || + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH), ); const userSettingsContent = { ui: { theme: 'dark' } }; const workspaceSettingsContent = { tools: { sandbox: true } }; (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return ''; }, @@ -678,11 +697,12 @@ describe('Settings Loading and Merging', () => { 'should load telemetry setting from $scope settings', ({ path, content, expected }) => { (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === path, + (p: fs.PathLike) => normalizePath(p) === normalizePath(path), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === path) return JSON.stringify(content); + if (normalizePath(p) === normalizePath(path)) + return JSON.stringify(content); return '{}'; }, ); @@ -697,9 +717,9 @@ describe('Settings Loading and Merging', () => { const workspaceSettingsContent = { telemetry: { enabled: false } }; (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -720,7 +740,8 @@ describe('Settings Loading and Merging', () => { it('should merge MCP servers correctly, with workspace taking precedence', () => { (mockFsExistsSync as Mock).mockImplementation( (p: fs.PathLike) => - p === USER_SETTINGS_PATH || p === MOCK_WORKSPACE_SETTINGS_PATH, + normalizePath(p) === normalizePath(USER_SETTINGS_PATH) || + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH), ); const userSettingsContent = { mcpServers: { @@ -751,9 +772,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return ''; }, @@ -822,11 +843,12 @@ describe('Settings Loading and Merging', () => { 'should handle MCP servers when only in $scope settings', ({ path, content, expected }) => { (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === path, + (p: fs.PathLike) => normalizePath(p) === normalizePath(path), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === path) return JSON.stringify(content); + if (normalizePath(p) === normalizePath(path)) + return JSON.stringify(content); return '{}'; }, ); @@ -881,11 +903,11 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -932,11 +954,11 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -983,8 +1005,11 @@ describe('Settings Loading and Merging', () => { (mockFsExistsSync as Mock).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) return JSON.stringify(userContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) + return JSON.stringify(userContent); + if ( + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH) + ) return JSON.stringify(workspaceContent); return '{}'; }, @@ -1008,9 +1033,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1038,13 +1063,13 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) return JSON.stringify(systemSettingsContent); - if (p === getSystemDefaultsPath()) + if (normalizePath(p) === normalizePath(getSystemDefaultsPath())) return JSON.stringify(systemDefaultsContent); - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1073,14 +1098,16 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) { + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) { // Simulate JSON.parse throwing for user settings vi.spyOn(JSON, 'parse').mockImplementationOnce(() => { throw userReadError; }); return invalidJsonContent; // Content that would cause JSON.parse to throw } - if (p === MOCK_WORKSPACE_SETTINGS_PATH) { + if ( + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH) + ) { // Simulate JSON.parse throwing for workspace settings vi.spyOn(JSON, 'parse').mockImplementationOnce(() => { throw workspaceReadError; @@ -1119,11 +1146,12 @@ describe('Settings Loading and Merging', () => { someUrl: 'https://test.com/${TEST_API_KEY}', }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1149,11 +1177,12 @@ describe('Settings Loading and Merging', () => { nested: { value: '$WORKSPACE_ENDPOINT' }, }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === MOCK_WORKSPACE_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1201,13 +1230,15 @@ describe('Settings Loading and Merging', () => { (mockFsExistsSync as Mock).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) { + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) { return JSON.stringify(systemSettingsContent); } - if (p === USER_SETTINGS_PATH) { + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) { return JSON.stringify(userSettingsContent); } - if (p === MOCK_WORKSPACE_SETTINGS_PATH) { + if ( + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH) + ) { return JSON.stringify(workspaceSettingsContent); } return '{}'; @@ -1266,9 +1297,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1280,14 +1311,15 @@ describe('Settings Loading and Merging', () => { it('should use user dnsResolutionOrder if workspace is not defined', () => { (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); const userSettingsContent = { advanced: { dnsResolutionOrder: 'verbatim' }, }; (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1300,11 +1332,12 @@ describe('Settings Loading and Merging', () => { it('should leave unresolved environment variables as is', () => { const userSettingsContent: TestSettings = { apiKey: '$UNDEFINED_VAR' }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1326,11 +1359,12 @@ describe('Settings Loading and Merging', () => { path: '/path/$VAR_A/${VAR_B}/end', }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1350,11 +1384,12 @@ describe('Settings Loading and Merging', () => { list: ['$ITEM_1', '${ITEM_2}', 'literal'], }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1389,11 +1424,12 @@ describe('Settings Loading and Merging', () => { }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1434,11 +1470,12 @@ describe('Settings Loading and Merging', () => { serverAddress: '${TEST_HOST}:${TEST_PORT}/api', }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1454,7 +1491,9 @@ describe('Settings Loading and Merging', () => { }); describe('when GEMINI_CLI_SYSTEM_SETTINGS_PATH is set', () => { - const MOCK_ENV_SYSTEM_SETTINGS_PATH = '/mock/env/system/settings.json'; + const MOCK_ENV_SYSTEM_SETTINGS_PATH = path.resolve( + '/mock/env/system/settings.json', + ); beforeEach(() => { process.env['GEMINI_CLI_SYSTEM_SETTINGS_PATH'] = @@ -1496,8 +1535,8 @@ describe('Settings Loading and Merging', () => { }); it('should correctly skip workspace-level loading if workspaceDir is a symlink to home', () => { - const mockHomeDir = '/mock/home/user'; - const mockSymlinkDir = '/mock/symlink/to/home'; + const mockHomeDir = path.resolve('/mock/home/user'); + const mockSymlinkDir = path.resolve('/mock/symlink/to/home'); const mockWorkspaceSettingsPath = path.join( mockSymlinkDir, GEMINI_DIR, @@ -1541,6 +1580,79 @@ describe('Settings Loading and Merging', () => { isWorkspaceHomeDirSpy.mockRestore(); } }); + + describe('caching', () => { + it('should cache loadSettings results', () => { + const mockedRead = vi.mocked(fs.readFileSync); + mockedRead.mockClear(); + mockedRead.mockReturnValue('{}'); + (mockFsExistsSync as Mock).mockReturnValue(true); + + const settings1 = loadSettings(MOCK_WORKSPACE_DIR); + const settings2 = loadSettings(MOCK_WORKSPACE_DIR); + + expect(mockedRead).toHaveBeenCalledTimes(5); // system, systemDefaults, user, workspace, and potentially an env file + expect(settings1).toBe(settings2); + }); + + it('should use separate cache for different workspace directories', () => { + const mockedRead = vi.mocked(fs.readFileSync); + mockedRead.mockClear(); + mockedRead.mockReturnValue('{}'); + (mockFsExistsSync as Mock).mockReturnValue(true); + + const workspace1 = path.resolve('/mock/workspace1'); + const workspace2 = path.resolve('/mock/workspace2'); + + const settings1 = loadSettings(workspace1); + const settings2 = loadSettings(workspace2); + + expect(mockedRead).toHaveBeenCalledTimes(10); // 5 for each workspace + expect(settings1).not.toBe(settings2); + }); + + it('should clear cache when saveSettings is called for user settings', () => { + const mockedRead = vi.mocked(fs.readFileSync); + mockedRead.mockClear(); + mockedRead.mockReturnValue('{}'); + (mockFsExistsSync as Mock).mockReturnValue(true); + + const settings1 = loadSettings(MOCK_WORKSPACE_DIR); + expect(mockedRead).toHaveBeenCalledTimes(5); + + saveSettings(settings1.user); + + const settings2 = loadSettings(MOCK_WORKSPACE_DIR); + expect(mockedRead).toHaveBeenCalledTimes(10); // Should have re-read from disk + expect(settings1).not.toBe(settings2); + }); + + it('should clear all caches when saveSettings is called for workspace settings', () => { + const mockedRead = vi.mocked(fs.readFileSync); + mockedRead.mockClear(); + mockedRead.mockReturnValue('{}'); + (mockFsExistsSync as Mock).mockReturnValue(true); + + const workspace1 = path.resolve('/mock/workspace1'); + const workspace2 = path.resolve('/mock/workspace2'); + + const settings1W1 = loadSettings(workspace1); + const settings1W2 = loadSettings(workspace2); + + expect(mockedRead).toHaveBeenCalledTimes(10); + + // Save settings for workspace 1 + saveSettings(settings1W1.workspace); + + const settings2W1 = loadSettings(workspace1); + const settings2W2 = loadSettings(workspace2); + + // Both workspace caches should have been cleared and re-read from disk (+10 reads) + expect(mockedRead).toHaveBeenCalledTimes(20); + expect(settings1W1).not.toBe(settings2W1); + expect(settings1W2).not.toBe(settings2W2); + }); + }); }); describe('excludedProjectEnvVars integration', () => { @@ -1562,12 +1674,13 @@ describe('Settings Loading and Merging', () => { }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === MOCK_WORKSPACE_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1578,16 +1691,18 @@ describe('Settings Loading and Merging', () => { loadSettings as unknown as { findEnvFile: () => string } ).findEnvFile; (loadSettings as unknown as { findEnvFile: () => string }).findEnvFile = - () => '/mock/project/.env'; + () => path.resolve('/mock/project/.env'); // Mock fs.readFileSync for .env file content const originalReadFileSync = fs.readFileSync; (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === '/mock/project/.env') { + if (p === path.resolve('/mock/project/.env')) { return 'DEBUG=true\nDEBUG_MODE=1\nGEMINI_API_KEY=test-key'; } - if (p === MOCK_WORKSPACE_SETTINGS_PATH) { + if ( + normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH) + ) { return JSON.stringify(workspaceSettingsContent); } return '{}'; @@ -1621,12 +1736,13 @@ describe('Settings Loading and Merging', () => { }; (mockFsExistsSync as Mock).mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1658,9 +1774,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1702,9 +1818,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1734,9 +1850,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1767,9 +1883,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1940,9 +2056,9 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); - if (p === MOCK_WORKSPACE_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(MOCK_WORKSPACE_SETTINGS_PATH)) return JSON.stringify(workspaceSettingsContent); return '{}'; }, @@ -1966,7 +2082,7 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -1994,7 +2110,7 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -2039,7 +2155,7 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -2226,7 +2342,8 @@ describe('Settings Loading and Merging', () => { it('should trigger migration automatically during loadSettings', () => { mockFsExistsSync.mockImplementation( - (p: fs.PathLike) => p === USER_SETTINGS_PATH, + (p: fs.PathLike) => + normalizePath(p) === normalizePath(USER_SETTINGS_PATH), ); const userSettingsContent = { general: { @@ -2235,7 +2352,7 @@ describe('Settings Loading and Merging', () => { }; (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -2270,10 +2387,10 @@ describe('Settings Loading and Merging', () => { vi.mocked(fs.existsSync).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) { + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) { return JSON.stringify(systemSettingsContent); } - if (p === getSystemDefaultsPath()) { + if (normalizePath(p) === normalizePath(getSystemDefaultsPath())) { return JSON.stringify(systemDefaultsContent); } return '{}'; @@ -2343,7 +2460,7 @@ describe('Settings Loading and Merging', () => { vi.mocked(fs.existsSync).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) { + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) { return JSON.stringify(systemSettingsContent); } return '{}'; @@ -2394,7 +2511,7 @@ describe('Settings Loading and Merging', () => { vi.mocked(fs.existsSync).mockReturnValue(true); (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === USER_SETTINGS_PATH) + if (normalizePath(p) === normalizePath(USER_SETTINGS_PATH)) return JSON.stringify(userSettingsContent); return '{}'; }, @@ -2430,13 +2547,16 @@ describe('Settings Loading and Merging', () => { it('should save settings using updateSettingsFilePreservingFormat', () => { const mockUpdateSettings = vi.mocked(updateSettingsFilePreservingFormat); const settingsFile = createMockSettings({ ui: { theme: 'dark' } }).user; - settingsFile.path = '/mock/settings.json'; + settingsFile.path = path.resolve('/mock/settings.json'); saveSettings(settingsFile); - expect(mockUpdateSettings).toHaveBeenCalledWith('/mock/settings.json', { - ui: { theme: 'dark' }, - }); + expect(mockUpdateSettings).toHaveBeenCalledWith( + path.resolve('/mock/settings.json'), + { + ui: { theme: 'dark' }, + }, + ); }); it('should create directory if it does not exist', () => { @@ -2445,14 +2565,19 @@ describe('Settings Loading and Merging', () => { mockFsExistsSync.mockReturnValue(false); const settingsFile = createMockSettings({}).user; - settingsFile.path = '/mock/new/dir/settings.json'; + settingsFile.path = path.resolve('/mock/new/dir/settings.json'); saveSettings(settingsFile); - expect(mockFsExistsSync).toHaveBeenCalledWith('/mock/new/dir'); - expect(mockFsMkdirSync).toHaveBeenCalledWith('/mock/new/dir', { - recursive: true, - }); + expect(mockFsExistsSync).toHaveBeenCalledWith( + path.resolve('/mock/new/dir'), + ); + expect(mockFsMkdirSync).toHaveBeenCalledWith( + path.resolve('/mock/new/dir'), + { + recursive: true, + }, + ); }); it('should emit error feedback if saving fails', () => { @@ -2463,7 +2588,7 @@ describe('Settings Loading and Merging', () => { }); const settingsFile = createMockSettings({}).user; - settingsFile.path = '/mock/settings.json'; + settingsFile.path = path.resolve('/mock/settings.json'); saveSettings(settingsFile); @@ -2491,7 +2616,7 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) { + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) { return JSON.stringify(systemSettingsContent); } return '{}'; @@ -2538,7 +2663,7 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) { + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) { return JSON.stringify(systemSettingsContent); } return '{}'; @@ -2579,7 +2704,7 @@ describe('Settings Loading and Merging', () => { (fs.readFileSync as Mock).mockImplementation( (p: fs.PathOrFileDescriptor) => { - if (p === getSystemSettingsPath()) { + if (normalizePath(p) === normalizePath(getSystemSettingsPath())) { return JSON.stringify(systemSettingsContent); } return '{}'; @@ -2694,7 +2819,7 @@ describe('Settings Loading and Merging', () => { beforeEach(() => { const emptySettingsFile: SettingsFile = { - path: '/mock/path', + path: path.resolve('/mock/path'), settings: {}, originalSettings: {}, }; @@ -3019,7 +3144,7 @@ describe('LoadedSettings Isolation and Serializability', () => { // Create a minimal LoadedSettings instance const emptyScope = { - path: '/mock/settings.json', + path: path.resolve('/mock/settings.json'), settings: {}, originalSettings: {}, } as unknown as SettingsFile; diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 422dda6115..a195931803 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -18,6 +18,7 @@ import { coreEvents, homedir, type AdminControlsSettings, + createCache, } from '@google/gemini-cli-core'; import stripJsonComments from 'strip-json-comments'; import { DefaultLight } from '../ui/themes/builtin/light/default-light.js'; @@ -615,6 +616,20 @@ export function loadEnvironment( } } +// Cache to store the results of loadSettings to avoid redundant disk I/O. +const settingsCache = createCache({ + storage: 'map', + defaultTtl: 10000, // 10 seconds +}); + +/** + * Resets the settings cache. Used exclusively for test isolation. + * @internal + */ +export function resetSettingsCacheForTesting() { + settingsCache.clear(); +} + /** * Loads settings from user and workspace directories. * Project settings override user settings. @@ -622,6 +637,16 @@ export function loadEnvironment( export function loadSettings( workspaceDir: string = process.cwd(), ): LoadedSettings { + const normalizedWorkspaceDir = path.resolve(workspaceDir); + return settingsCache.getOrCreate(normalizedWorkspaceDir, () => + _doLoadSettings(normalizedWorkspaceDir), + ); +} + +/** + * Internal implementation of the settings loading logic. + */ +function _doLoadSettings(workspaceDir: string): LoadedSettings { let systemSettings: Settings = {}; let systemDefaultSettings: Settings = {}; let userSettings: Settings = {}; @@ -1029,6 +1054,9 @@ export function migrateDeprecatedSettings( } export function saveSettings(settingsFile: SettingsFile): void { + // Clear the entire cache on any save. + settingsCache.clear(); + try { // Ensure the directory exists const dirPath = path.dirname(settingsFile.path); diff --git a/packages/cli/src/config/settings_validation_warning.test.ts b/packages/cli/src/config/settings_validation_warning.test.ts index 498f803dd9..435c797d81 100644 --- a/packages/cli/src/config/settings_validation_warning.test.ts +++ b/packages/cli/src/config/settings_validation_warning.test.ts @@ -81,6 +81,7 @@ import { loadSettings, USER_SETTINGS_PATH, type LoadedSettings, + resetSettingsCacheForTesting, } from './settings.js'; const MOCK_WORKSPACE_DIR = '/mock/workspace'; @@ -88,6 +89,7 @@ const MOCK_WORKSPACE_DIR = '/mock/workspace'; describe('Settings Validation Warning', () => { beforeEach(() => { vi.clearAllMocks(); + resetSettingsCacheForTesting(); (fs.readFileSync as Mock).mockReturnValue('{}'); (fs.existsSync as Mock).mockReturnValue(false); }); diff --git a/packages/cli/src/test-utils/AppRig.tsx b/packages/cli/src/test-utils/AppRig.tsx index 3ff65c4067..a9aea95376 100644 --- a/packages/cli/src/test-utils/AppRig.tsx +++ b/packages/cli/src/test-utils/AppRig.tsx @@ -36,7 +36,10 @@ import { MockShellExecutionService, } from './MockShellExecutionService.js'; import { createMockSettings } from './settings.js'; -import { type LoadedSettings } from '../config/settings.js'; +import { + type LoadedSettings, + resetSettingsCacheForTesting, +} from '../config/settings.js'; import { AuthState, StreamingState } from '../ui/types.js'; import { randomUUID } from 'node:crypto'; import type { @@ -171,6 +174,7 @@ export class AppRig { async initialize() { this.setupEnvironment(); + resetSettingsCacheForTesting(); this.settings = this.createRigSettings(); const approvalMode = diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 67f2d5dd84..dfa2d4af86 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -119,7 +119,7 @@ import { type InitializationResult } from '../core/initializer.js'; import { useFocus } from './hooks/useFocus.js'; import { useKeypress, type Key } from './hooks/useKeypress.js'; import { KeypressPriority } from './contexts/KeypressContext.js'; -import { keyMatchers, Command } from './keyMatchers.js'; +import { Command } from './keyMatchers.js'; import { useLoadingIndicator } from './hooks/useLoadingIndicator.js'; import { useShellInactivityStatus } from './hooks/useShellInactivityStatus.js'; import { useFolderTrust } from './hooks/useFolderTrust.js'; @@ -164,7 +164,7 @@ import { NewAgentsChoice } from './components/NewAgentsNotification.js'; import { isSlashCommand } from './utils/commandUtils.js'; import { useTerminalTheme } from './hooks/useTerminalTheme.js'; import { useTimedMessage } from './hooks/useTimedMessage.js'; -import { shouldDismissShortcutsHelpOnHotkey } from './utils/shortcutsHelp.js'; +import { useIsHelpDismissKey } from './utils/shortcutsHelp.js'; import { useSuspend } from './hooks/useSuspend.js'; import { useRunEventNotifications } from './hooks/useRunEventNotifications.js'; import { isNotificationsEnabled } from '../utils/terminalNotifications.js'; @@ -205,6 +205,7 @@ import { useVisibilityToggle, APPROVAL_MODE_REVEAL_DURATION_MS, } from './hooks/useVisibilityToggle.js'; +import { useKeyMatchers } from './hooks/useKeyMatchers.js'; /** * The fraction of the terminal width to allocate to the shell. @@ -219,6 +220,8 @@ const SHELL_WIDTH_FRACTION = 0.89; const SHELL_HEIGHT_PADDING = 10; export const AppContainer = (props: AppContainerProps) => { + const isHelpDismissKey = useIsHelpDismissKey(); + const keyMatchers = useKeyMatchers(); const { config, initializationResult, resumedSessionData } = props; const settings = useSettings(); const { reset } = useOverflowActions()!; @@ -1654,7 +1657,7 @@ Logging in with Google... Restarting Gemini CLI to continue. debugLogger.log('[DEBUG] Keystroke:', JSON.stringify(key)); } - if (shortcutsHelpVisible && shouldDismissShortcutsHelpOnHotkey(key)) { + if (shortcutsHelpVisible && isHelpDismissKey(key)) { setShortcutsHelpVisible(false); } @@ -1848,6 +1851,8 @@ Logging in with Google... Restarting Gemini CLI to continue. settings.merged.general.devtools, showErrorDetails, triggerExpandHint, + keyMatchers, + isHelpDismissKey, ], ); diff --git a/packages/cli/src/ui/auth/ApiAuthDialog.tsx b/packages/cli/src/ui/auth/ApiAuthDialog.tsx index 2caad6fd27..a62d34c866 100644 --- a/packages/cli/src/ui/auth/ApiAuthDialog.tsx +++ b/packages/cli/src/ui/auth/ApiAuthDialog.tsx @@ -13,7 +13,8 @@ import { useTextBuffer } from '../components/shared/text-buffer.js'; import { useUIState } from '../contexts/UIStateContext.js'; import { clearApiKey, debugLogger } from '@google/gemini-cli-core'; import { useKeypress } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; interface ApiAuthDialogProps { onSubmit: (apiKey: string) => void; @@ -28,6 +29,7 @@ export function ApiAuthDialog({ error, defaultValue = '', }: ApiAuthDialogProps): React.JSX.Element { + const keyMatchers = useKeyMatchers(); const { terminalWidth } = useUIState(); const viewportWidth = terminalWidth - 8; diff --git a/packages/cli/src/ui/commands/setupGithubCommand.ts b/packages/cli/src/ui/commands/setupGithubCommand.ts index a125b1eda4..c583db394a 100644 --- a/packages/cli/src/ui/commands/setupGithubCommand.ts +++ b/packages/cli/src/ui/commands/setupGithubCommand.ts @@ -120,6 +120,7 @@ async function downloadFiles({ downloads.push( (async () => { const endpoint = `${REPO_DOWNLOAD_URL}/refs/tags/${releaseTag}/${SOURCE_DIR}/${fileBasename}`; + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(endpoint, { method: 'GET', dispatcher: proxy ? new ProxyAgent(proxy) : undefined, diff --git a/packages/cli/src/ui/components/AdminSettingsChangedDialog.tsx b/packages/cli/src/ui/components/AdminSettingsChangedDialog.tsx index b697dc17c4..2507d31f2b 100644 --- a/packages/cli/src/ui/components/AdminSettingsChangedDialog.tsx +++ b/packages/cli/src/ui/components/AdminSettingsChangedDialog.tsx @@ -8,9 +8,11 @@ import { Box, Text } from 'ink'; import { theme } from '../semantic-colors.js'; import { useKeypress } from '../hooks/useKeypress.js'; import { useUIActions } from '../contexts/UIActionsContext.js'; -import { Command, keyMatchers } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; export const AdminSettingsChangedDialog = () => { + const keyMatchers = useKeyMatchers(); const { handleRestart } = useUIActions(); useKeypress( diff --git a/packages/cli/src/ui/components/AgentConfigDialog.test.tsx b/packages/cli/src/ui/components/AgentConfigDialog.test.tsx index 05cd4a47f5..52cda094e0 100644 --- a/packages/cli/src/ui/components/AgentConfigDialog.test.tsx +++ b/packages/cli/src/ui/components/AgentConfigDialog.test.tsx @@ -327,5 +327,31 @@ describe('AgentConfigDialog', () => { expect(frame).toContain('false'); unmount(); }); + it('should respond to availableTerminalHeight and truncate list', async () => { + const settings = createMockSettings(); + // Agent config has about 6 base items + 2 per tool + // Render with very small height (20) + const { lastFrame, unmount } = render( + + + , + ); + await waitFor(() => + expect(lastFrame()).toContain('Configure: Test Agent'), + ); + + const frame = lastFrame(); + // At height 20, it should be heavily truncated and show '▼' + expect(frame).toContain('▼'); + unmount(); + }); }); }); diff --git a/packages/cli/src/ui/components/AgentConfigDialog.tsx b/packages/cli/src/ui/components/AgentConfigDialog.tsx index 4079c6df77..819b32d7b0 100644 --- a/packages/cli/src/ui/components/AgentConfigDialog.tsx +++ b/packages/cli/src/ui/components/AgentConfigDialog.tsx @@ -110,6 +110,8 @@ interface AgentConfigDialogProps { settings: LoadedSettings; onClose: () => void; onSave?: () => void; + /** Available terminal height for dynamic windowing */ + availableTerminalHeight?: number; } /** @@ -192,6 +194,7 @@ export function AgentConfigDialog({ settings, onClose, onSave, + availableTerminalHeight, }: AgentConfigDialogProps): React.JSX.Element { // Scope selector state (User by default) const [selectedScope, setSelectedScope] = useState( @@ -395,12 +398,6 @@ export function AgentConfigDialog({ [pendingOverride, saveFieldValue], ); - // Footer content - const footerContent = - modifiedFields.size > 0 ? ( - Changes saved automatically. - ) : null; - return ( 0 + ? { + content: ( + + Changes saved automatically. + + ), + height: 1, + } + : undefined + } /> ); } diff --git a/packages/cli/src/ui/components/AppHeader.tsx b/packages/cli/src/ui/components/AppHeader.tsx index b9601e772a..0b15f917a6 100644 --- a/packages/cli/src/ui/components/AppHeader.tsx +++ b/packages/cli/src/ui/components/AppHeader.tsx @@ -17,16 +17,30 @@ import { theme } from '../semantic-colors.js'; import { ThemedGradient } from './ThemedGradient.js'; import { CliSpinner } from './CliSpinner.js'; +import { isAppleTerminal } from '@google/gemini-cli-core'; + interface AppHeaderProps { version: string; showDetails?: boolean; } -const ICON = `▝▜▄ +const DEFAULT_ICON = `▝▜▄ ▝▜▄ ▗▟▀ ▝▀ `; +/** + * The default Apple Terminal.app adds significant line-height padding between + * rows. This breaks Unicode block-drawing characters that rely on vertical + * adjacency (like half-blocks). This version is perfectly symmetric vertically, + * which makes the padding gaps look like an intentional "scanline" design + * rather than a broken image. + */ +const MAC_TERMINAL_ICON = `▝▜▄ + ▝▜▄ + ▗▟▀ +▗▟▀ `; + export const AppHeader = ({ version, showDetails = true }: AppHeaderProps) => { const settings = useSettings(); const config = useConfig(); @@ -39,6 +53,8 @@ export const AppHeader = ({ version, showDetails = true }: AppHeaderProps) => { settings.merged.ui.hideBanner || config.getScreenReader() ); + const ICON = isAppleTerminal() ? MAC_TERMINAL_ICON : DEFAULT_ICON; + if (!showDetails) { return ( diff --git a/packages/cli/src/ui/components/AppHeaderIcon.test.tsx b/packages/cli/src/ui/components/AppHeaderIcon.test.tsx new file mode 100644 index 0000000000..c16febea66 --- /dev/null +++ b/packages/cli/src/ui/components/AppHeaderIcon.test.tsx @@ -0,0 +1,49 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { renderWithProviders } from '../../test-utils/render.js'; +import { AppHeader } from './AppHeader.js'; + +// We mock the entire module to control the isAppleTerminal export +vi.mock('@google/gemini-cli-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + isAppleTerminal: vi.fn(), + }; +}); + +import { isAppleTerminal } from '@google/gemini-cli-core'; + +describe('AppHeader Icon Rendering', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it('renders the default icon in standard terminals', async () => { + vi.mocked(isAppleTerminal).mockReturnValue(false); + + const result = renderWithProviders(); + await result.waitUntilReady(); + + await expect(result).toMatchSvgSnapshot(); + }); + + it('renders the symmetric icon in Apple Terminal', async () => { + vi.mocked(isAppleTerminal).mockReturnValue(true); + + const result = renderWithProviders(); + await result.waitUntilReady(); + + await expect(result).toMatchSvgSnapshot(); + }); +}); diff --git a/packages/cli/src/ui/components/AskUserDialog.tsx b/packages/cli/src/ui/components/AskUserDialog.tsx index 284e4e1df8..e55617a724 100644 --- a/packages/cli/src/ui/components/AskUserDialog.tsx +++ b/packages/cli/src/ui/components/AskUserDialog.tsx @@ -20,7 +20,7 @@ import { BaseSelectionList } from './shared/BaseSelectionList.js'; import type { SelectionListItem } from '../hooks/useSelectionList.js'; import { TabHeader, type Tab } from './shared/TabHeader.js'; import { useKeypress, type Key } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { checkExhaustive } from '@google/gemini-cli-core'; import { TextInput } from './shared/TextInput.js'; import { formatCommand } from '../utils/keybindingUtils.js'; @@ -36,6 +36,7 @@ import { RenderInline } from '../utils/InlineMarkdownRenderer.js'; import { MaxSizedBox } from './shared/MaxSizedBox.js'; import { UIStateContext } from '../contexts/UIStateContext.js'; import { useAlternateBuffer } from '../hooks/useAlternateBuffer.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; /** Padding for dialog content to prevent text from touching edges. */ const DIALOG_PADDING = 4; @@ -208,6 +209,7 @@ const ReviewView: React.FC = ({ progressHeader, extraParts, }) => { + const keyMatchers = useKeyMatchers(); const unansweredCount = questions.length - Object.keys(answers).length; const hasUnanswered = unansweredCount > 0; @@ -288,6 +290,7 @@ const TextQuestionView: React.FC = ({ progressHeader, keyboardHints, }) => { + const keyMatchers = useKeyMatchers(); const isAlternateBuffer = useAlternateBuffer(); const prefix = '> '; const horizontalPadding = 1; // 1 for cursor @@ -325,7 +328,7 @@ const TextQuestionView: React.FC = ({ } return false; }, - [buffer, textValue], + [buffer, textValue, keyMatchers], ); useKeypress(handleExtraKeys, { isActive: true, priority: true }); @@ -487,6 +490,7 @@ const ChoiceQuestionView: React.FC = ({ progressHeader, keyboardHints, }) => { + const keyMatchers = useKeyMatchers(); const isAlternateBuffer = useAlternateBuffer(); const numOptions = (question.options?.length ?? 0) + (question.type !== 'yesno' ? 1 : 0); @@ -680,6 +684,7 @@ const ChoiceQuestionView: React.FC = ({ customBuffer, onEditingCustomOption, customOptionText, + keyMatchers, ], ); @@ -950,6 +955,7 @@ export const AskUserDialog: React.FC = ({ availableHeight: availableHeightProp, extraParts, }) => { + const keyMatchers = useKeyMatchers(); const uiState = useContext(UIStateContext); const availableHeight = availableHeightProp ?? @@ -999,7 +1005,7 @@ export const AskUserDialog: React.FC = ({ } return false; }, - [onCancel, submitted, isEditingCustomOption], + [onCancel, submitted, isEditingCustomOption, keyMatchers], ); useKeypress(handleCancel, { @@ -1032,7 +1038,7 @@ export const AskUserDialog: React.FC = ({ } return false; }, - [questions.length, submitted, goToNextTab, goToPrevTab], + [questions.length, submitted, goToNextTab, goToPrevTab, keyMatchers], ); useKeypress(handleNavigation, { diff --git a/packages/cli/src/ui/components/BackgroundShellDisplay.tsx b/packages/cli/src/ui/components/BackgroundShellDisplay.tsx index 16093ef0d7..946e062c19 100644 --- a/packages/cli/src/ui/components/BackgroundShellDisplay.tsx +++ b/packages/cli/src/ui/components/BackgroundShellDisplay.tsx @@ -16,7 +16,7 @@ import { } from '@google/gemini-cli-core'; import { cpLen, cpSlice, getCachedStringWidth } from '../utils/textUtils.js'; import { type BackgroundShell } from '../hooks/shellCommandProcessor.js'; -import { Command, keyMatchers } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { useKeypress } from '../hooks/useKeypress.js'; import { formatCommand } from '../utils/keybindingUtils.js'; import { @@ -30,6 +30,7 @@ import { RadioButtonSelect, type RadioSelectItem, } from './shared/RadioButtonSelect.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; interface BackgroundShellDisplayProps { shells: Map; @@ -60,6 +61,7 @@ export const BackgroundShellDisplay = ({ isFocused, isListOpenProp, }: BackgroundShellDisplayProps) => { + const keyMatchers = useKeyMatchers(); const { dismissBackgroundShell, setActiveBackgroundShellPid, diff --git a/packages/cli/src/ui/components/DialogManager.tsx b/packages/cli/src/ui/components/DialogManager.tsx index 5119c1b343..de62401e1e 100644 --- a/packages/cli/src/ui/components/DialogManager.tsx +++ b/packages/cli/src/ui/components/DialogManager.tsx @@ -252,6 +252,7 @@ export const DialogManager = ({ displayName={uiState.selectedAgentDisplayName} definition={uiState.selectedAgentDefinition} settings={settings} + availableTerminalHeight={terminalHeight - staticExtraHeight} onClose={uiActions.closeAgentConfigDialog} onSave={async () => { // Reload agent registry to pick up changes diff --git a/packages/cli/src/ui/components/ExitPlanModeDialog.test.tsx b/packages/cli/src/ui/components/ExitPlanModeDialog.test.tsx index 2bf1f723a6..35d0d2e719 100644 --- a/packages/cli/src/ui/components/ExitPlanModeDialog.test.tsx +++ b/packages/cli/src/ui/components/ExitPlanModeDialog.test.tsx @@ -10,7 +10,7 @@ import { renderWithProviders } from '../../test-utils/render.js'; import { waitFor } from '../../test-utils/async.js'; import { ExitPlanModeDialog } from './ExitPlanModeDialog.js'; import { useKeypress } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { ApprovalMode, validatePlanContent, @@ -18,6 +18,7 @@ import { type FileSystemService, } from '@google/gemini-cli-core'; import * as fs from 'node:fs'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; vi.mock('../utils/editorUtils.js', () => ({ openFileInEditor: vi.fn(), @@ -402,6 +403,7 @@ Implement a comprehensive authentication system with multiple providers. }: { children: React.ReactNode; }) => { + const keyMatchers = useKeyMatchers(); useKeypress( (key) => { if (keyMatchers[Command.QUIT](key)) { diff --git a/packages/cli/src/ui/components/ExitPlanModeDialog.tsx b/packages/cli/src/ui/components/ExitPlanModeDialog.tsx index 39e1b8a155..d5f1983c14 100644 --- a/packages/cli/src/ui/components/ExitPlanModeDialog.tsx +++ b/packages/cli/src/ui/components/ExitPlanModeDialog.tsx @@ -22,8 +22,9 @@ import { useConfig } from '../contexts/ConfigContext.js'; import { AskUserDialog } from './AskUserDialog.js'; import { openFileInEditor } from '../utils/editorUtils.js'; import { useKeypress } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { formatCommand } from '../utils/keybindingUtils.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; export interface ExitPlanModeDialogProps { planPath: string; @@ -147,6 +148,7 @@ export const ExitPlanModeDialog: React.FC = ({ width, availableHeight, }) => { + const keyMatchers = useKeyMatchers(); const config = useConfig(); const { stdin, setRawMode } = useStdin(); const planState = usePlanContent(planPath, config); diff --git a/packages/cli/src/ui/components/FooterConfigDialog.tsx b/packages/cli/src/ui/components/FooterConfigDialog.tsx index c31dc73e45..03560d4e21 100644 --- a/packages/cli/src/ui/components/FooterConfigDialog.tsx +++ b/packages/cli/src/ui/components/FooterConfigDialog.tsx @@ -11,13 +11,14 @@ import { theme } from '../semantic-colors.js'; import { useSettingsStore } from '../contexts/SettingsContext.js'; import { useUIState } from '../contexts/UIStateContext.js'; import { useKeypress, type Key } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { FooterRow, type FooterRowItem } from './Footer.js'; import { ALL_ITEMS, resolveFooterState } from '../../config/footerItems.js'; import { SettingScope } from '../../config/settings.js'; import { BaseSelectionList } from './shared/BaseSelectionList.js'; import type { SelectionListItem } from '../hooks/useSelectionList.js'; import { DialogFooter } from './shared/DialogFooter.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; interface FooterConfigDialogProps { onClose?: () => void; @@ -82,6 +83,7 @@ function footerConfigReducer( export const FooterConfigDialog: React.FC = ({ onClose, }) => { + const keyMatchers = useKeyMatchers(); const { settings, setSetting } = useSettingsStore(); const { constrainHeight, terminalHeight, staticExtraHeight } = useUIState(); const [state, dispatch] = useReducer(footerConfigReducer, undefined, () => diff --git a/packages/cli/src/ui/components/HooksDialog.tsx b/packages/cli/src/ui/components/HooksDialog.tsx index d820aba6e7..4fd7b9ff9d 100644 --- a/packages/cli/src/ui/components/HooksDialog.tsx +++ b/packages/cli/src/ui/components/HooksDialog.tsx @@ -9,7 +9,8 @@ import { useState, useMemo } from 'react'; import { Box, Text } from 'ink'; import { theme } from '../semantic-colors.js'; import { useKeypress } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; /** * Hook entry type matching HookRegistryEntry from core @@ -49,6 +50,7 @@ export const HooksDialog: React.FC = ({ onClose, maxVisibleHooks = DEFAULT_MAX_VISIBLE_HOOKS, }) => { + const keyMatchers = useKeyMatchers(); const [scrollOffset, setScrollOffset] = useState(0); // Flatten hooks with their event names for easier scrolling diff --git a/packages/cli/src/ui/components/InputPrompt.test.tsx b/packages/cli/src/ui/components/InputPrompt.test.tsx index b8148b0bef..85e6b8d6aa 100644 --- a/packages/cli/src/ui/components/InputPrompt.test.tsx +++ b/packages/cli/src/ui/components/InputPrompt.test.tsx @@ -44,7 +44,7 @@ import { terminalCapabilityManager } from '../utils/terminalCapabilityManager.js import type { UIState } from '../contexts/UIStateContext.js'; import { isLowColorDepth } from '../utils/terminalUtils.js'; import { cpLen } from '../utils/textUtils.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { defaultKeyMatchers, Command } from '../keyMatchers.js'; import type { Key } from '../hooks/useKeypress.js'; import { appEvents, @@ -197,7 +197,7 @@ describe('InputPrompt', () => { visualCursor: [0, 0], visualScrollRow: 0, handleInput: vi.fn((key: Key) => { - if (keyMatchers[Command.CLEAR_INPUT](key)) { + if (defaultKeyMatchers[Command.CLEAR_INPUT](key)) { if (mockBuffer.text.length > 0) { mockBuffer.setText(''); return true; diff --git a/packages/cli/src/ui/components/InputPrompt.tsx b/packages/cli/src/ui/components/InputPrompt.tsx index 373571f07d..1d82c87f70 100644 --- a/packages/cli/src/ui/components/InputPrompt.tsx +++ b/packages/cli/src/ui/components/InputPrompt.tsx @@ -36,7 +36,7 @@ import { } from '../hooks/useCommandCompletion.js'; import type { Key } from '../hooks/useKeypress.js'; import { useKeypress } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { formatCommand } from '../utils/keybindingUtils.js'; import type { CommandContext, SlashCommand } from '../commands/types.js'; import type { Config } from '@google/gemini-cli-core'; @@ -72,8 +72,9 @@ import { useMouseClick } from '../hooks/useMouseClick.js'; import { useMouse, type MouseEvent } from '../contexts/MouseContext.js'; import { useUIActions } from '../contexts/UIActionsContext.js'; import { useAlternateBuffer } from '../hooks/useAlternateBuffer.js'; -import { shouldDismissShortcutsHelpOnHotkey } from '../utils/shortcutsHelp.js'; +import { useIsHelpDismissKey } from '../utils/shortcutsHelp.js'; import { useRepeatedKeyPress } from '../hooks/useRepeatedKeyPress.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; /** * Returns if the terminal can be trusted to handle paste events atomically @@ -207,6 +208,8 @@ export const InputPrompt: React.FC = ({ suggestionsPosition = 'below', setBannerVisible, }) => { + const isHelpDismissKey = useIsHelpDismissKey(); + const keyMatchers = useKeyMatchers(); const { stdout } = useStdout(); const { merged: settings } = useSettings(); const kittyProtocol = useKittyKeyboardProtocol(); @@ -737,7 +740,7 @@ export const InputPrompt: React.FC = ({ return true; } - if (shortcutsHelpVisible && shouldDismissShortcutsHelpOnHotkey(key)) { + if (shortcutsHelpVisible && isHelpDismissKey(key)) { setShortcutsHelpVisible(false); } @@ -1265,6 +1268,8 @@ export const InputPrompt: React.FC = ({ shouldShowSuggestions, isShellSuggestionsVisible, forceShowShellSuggestions, + keyMatchers, + isHelpDismissKey, ], ); diff --git a/packages/cli/src/ui/components/PolicyUpdateDialog.tsx b/packages/cli/src/ui/components/PolicyUpdateDialog.tsx index e6ed75c4db..ad48571fff 100644 --- a/packages/cli/src/ui/components/PolicyUpdateDialog.tsx +++ b/packages/cli/src/ui/components/PolicyUpdateDialog.tsx @@ -16,7 +16,8 @@ import { theme } from '../semantic-colors.js'; import type { RadioSelectItem } from './shared/RadioButtonSelect.js'; import { RadioButtonSelect } from './shared/RadioButtonSelect.js'; import { useKeypress } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; export enum PolicyUpdateChoice { ACCEPT = 'accept', @@ -34,6 +35,7 @@ export const PolicyUpdateDialog: React.FC = ({ request, onClose, }) => { + const keyMatchers = useKeyMatchers(); const isProcessing = useRef(false); const handleSelect = useCallback( diff --git a/packages/cli/src/ui/components/RewindConfirmation.tsx b/packages/cli/src/ui/components/RewindConfirmation.tsx index bbfbf9dbee..fa58995731 100644 --- a/packages/cli/src/ui/components/RewindConfirmation.tsx +++ b/packages/cli/src/ui/components/RewindConfirmation.tsx @@ -13,7 +13,8 @@ import type { RadioSelectItem } from './shared/RadioButtonSelect.js'; import type { FileChangeStats } from '../utils/rewindFileOps.js'; import { useKeypress } from '../hooks/useKeypress.js'; import { formatTimeAgo } from '../utils/formatters.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; export enum RewindOutcome { RewindAndRevert = 'rewind_and_revert', @@ -58,6 +59,7 @@ export const RewindConfirmation: React.FC = ({ terminalWidth, timestamp, }) => { + const keyMatchers = useKeyMatchers(); const isScreenReaderEnabled = useIsScreenReaderEnabled(); useKeypress( (key) => { diff --git a/packages/cli/src/ui/components/RewindViewer.tsx b/packages/cli/src/ui/components/RewindViewer.tsx index 26f7282f61..0a9f858d3d 100644 --- a/packages/cli/src/ui/components/RewindViewer.tsx +++ b/packages/cli/src/ui/components/RewindViewer.tsx @@ -19,9 +19,10 @@ import { useKeypress } from '../hooks/useKeypress.js'; import { useRewind } from '../hooks/useRewind.js'; import { RewindConfirmation, RewindOutcome } from './RewindConfirmation.js'; import { stripReferenceContent } from '../utils/formatters.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { CliSpinner } from './CliSpinner.js'; import { ExpandableText } from './shared/ExpandableText.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; interface RewindViewerProps { conversation: ConversationRecord; @@ -48,6 +49,7 @@ export const RewindViewer: React.FC = ({ onExit, onRewind, }) => { + const keyMatchers = useKeyMatchers(); const [isRewinding, setIsRewinding] = useState(false); const { terminalWidth, terminalHeight } = useUIState(); const isScreenReaderEnabled = useIsScreenReaderEnabled(); diff --git a/packages/cli/src/ui/components/SettingsDialog.tsx b/packages/cli/src/ui/components/SettingsDialog.tsx index 23e8a55a7d..b8136254f3 100644 --- a/packages/cli/src/ui/components/SettingsDialog.tsx +++ b/packages/cli/src/ui/components/SettingsDialog.tsx @@ -346,94 +346,9 @@ export function SettingsDialog({ [showRestartPrompt, onRestartRequest], ); - // Calculate effective max items and scope visibility based on terminal height - const { effectiveMaxItemsToShow, showScopeSelection, showSearch } = - useMemo(() => { - // Only show scope selector if we have a workspace - const hasWorkspace = settings.workspace.path !== undefined; - - // Search box is hidden when restart prompt is shown to save space and avoid key conflicts - const shouldShowSearch = !showRestartPrompt; - - if (!availableTerminalHeight) { - return { - effectiveMaxItemsToShow: Math.min(MAX_ITEMS_TO_SHOW, items.length), - showScopeSelection: hasWorkspace, - showSearch: shouldShowSearch, - }; - } - - // Layout constants based on BaseSettingsDialog structure: - // 4 for border (2) and padding (2) - const DIALOG_PADDING = 4; - const SETTINGS_TITLE_HEIGHT = 1; - // 3 for box + 1 for marginTop + 1 for spacing after - const SEARCH_SECTION_HEIGHT = shouldShowSearch ? 5 : 0; - const SCROLL_ARROWS_HEIGHT = 2; - const ITEMS_SPACING_AFTER = 1; - // 1 for Label + 3 for Scope items + 1 for spacing after - const SCOPE_SECTION_HEIGHT = hasWorkspace ? 5 : 0; - const HELP_TEXT_HEIGHT = 1; - const RESTART_PROMPT_HEIGHT = showRestartPrompt ? 1 : 0; - const ITEM_HEIGHT = 3; // Label + description + spacing - - const currentAvailableHeight = availableTerminalHeight - DIALOG_PADDING; - - const baseFixedHeight = - SETTINGS_TITLE_HEIGHT + - SEARCH_SECTION_HEIGHT + - SCROLL_ARROWS_HEIGHT + - ITEMS_SPACING_AFTER + - HELP_TEXT_HEIGHT + - RESTART_PROMPT_HEIGHT; - - // Calculate max items with scope selector - const heightWithScope = baseFixedHeight + SCOPE_SECTION_HEIGHT; - const availableForItemsWithScope = - currentAvailableHeight - heightWithScope; - const maxItemsWithScope = Math.max( - 1, - Math.floor(availableForItemsWithScope / ITEM_HEIGHT), - ); - - // Calculate max items without scope selector - const availableForItemsWithoutScope = - currentAvailableHeight - baseFixedHeight; - const maxItemsWithoutScope = Math.max( - 1, - Math.floor(availableForItemsWithoutScope / ITEM_HEIGHT), - ); - - // In small terminals, hide scope selector if it would allow more items to show - let shouldShowScope = hasWorkspace; - let maxItems = maxItemsWithScope; - - if (hasWorkspace && availableTerminalHeight < 25) { - // Hide scope selector if it gains us more than 1 extra item - if (maxItemsWithoutScope > maxItemsWithScope + 1) { - shouldShowScope = false; - maxItems = maxItemsWithoutScope; - } - } - - return { - effectiveMaxItemsToShow: Math.min(maxItems, items.length), - showScopeSelection: shouldShowScope, - showSearch: shouldShowSearch, - }; - }, [ - availableTerminalHeight, - items.length, - settings.workspace.path, - showRestartPrompt, - ]); - - const footerContent = showRestartPrompt ? ( - - Changes that require a restart have been modified. Press r to exit and - apply changes now. - - ) : null; + // Decisions on what features to enable + const hasWorkspace = settings.workspace.path !== undefined; + const showSearch = !showRestartPrompt; return ( + Changes that require a restart have been modified. Press r to + exit and apply changes now. + + ), + height: 1, + } + : undefined + } /> ); } diff --git a/packages/cli/src/ui/components/ShellInputPrompt.tsx b/packages/cli/src/ui/components/ShellInputPrompt.tsx index 26e32d946f..dae0f65312 100644 --- a/packages/cli/src/ui/components/ShellInputPrompt.tsx +++ b/packages/cli/src/ui/components/ShellInputPrompt.tsx @@ -10,7 +10,8 @@ import { useKeypress } from '../hooks/useKeypress.js'; import { ShellExecutionService } from '@google/gemini-cli-core'; import { keyToAnsi, type Key } from '../hooks/keyToAnsi.js'; import { ACTIVE_SHELL_MAX_LINES } from '../constants.js'; -import { Command, keyMatchers } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; export interface ShellInputPromptProps { activeShellPtyId: number | null; @@ -23,6 +24,7 @@ export const ShellInputPrompt: React.FC = ({ focus = true, scrollPageSize = ACTIVE_SHELL_MAX_LINES, }) => { + const keyMatchers = useKeyMatchers(); const handleShellInputSubmit = useCallback( (input: string) => { if (activeShellPtyId) { @@ -73,7 +75,13 @@ export const ShellInputPrompt: React.FC = ({ return false; }, - [focus, handleShellInputSubmit, activeShellPtyId, scrollPageSize], + [ + focus, + handleShellInputSubmit, + activeShellPtyId, + scrollPageSize, + keyMatchers, + ], ); useKeypress(handleInput, { isActive: focus }); diff --git a/packages/cli/src/ui/components/UserIdentity.test.tsx b/packages/cli/src/ui/components/UserIdentity.test.tsx index 5391944d26..aa7f4d3da2 100644 --- a/packages/cli/src/ui/components/UserIdentity.test.tsx +++ b/packages/cli/src/ui/components/UserIdentity.test.tsx @@ -51,6 +51,24 @@ describe('', () => { unmount(); }); + it('should render the user email on the very first frame (regression test)', () => { + const mockConfig = makeFakeConfig(); + vi.spyOn(mockConfig, 'getContentGeneratorConfig').mockReturnValue({ + authType: AuthType.LOGIN_WITH_GOOGLE, + model: 'gemini-pro', + } as unknown as ContentGeneratorConfig); + vi.spyOn(mockConfig, 'getUserTierName').mockReturnValue(undefined); + + const { lastFrameRaw, unmount } = renderWithProviders( + , + ); + + // Assert immediately on the first available frame before any async ticks happen + const output = lastFrameRaw(); + expect(output).toContain('test@example.com'); + unmount(); + }); + it('should render login message if email is missing', async () => { // Modify the mock for this specific test vi.mocked(UserAccountManager).mockImplementationOnce( diff --git a/packages/cli/src/ui/components/UserIdentity.tsx b/packages/cli/src/ui/components/UserIdentity.tsx index 98c62ec68f..7b07a4f91c 100644 --- a/packages/cli/src/ui/components/UserIdentity.tsx +++ b/packages/cli/src/ui/components/UserIdentity.tsx @@ -5,7 +5,7 @@ */ import type React from 'react'; -import { useMemo, useEffect, useState } from 'react'; +import { useMemo } from 'react'; import { Box, Text } from 'ink'; import { theme } from '../semantic-colors.js'; import { @@ -20,13 +20,12 @@ interface UserIdentityProps { export const UserIdentity: React.FC = ({ config }) => { const authType = config.getContentGeneratorConfig()?.authType; - const [email, setEmail] = useState(); - - useEffect(() => { + const email = useMemo(() => { if (authType) { const userAccountManager = new UserAccountManager(); - setEmail(userAccountManager.getCachedGoogleAccount() ?? undefined); + return userAccountManager.getCachedGoogleAccount() ?? undefined; } + return undefined; }, [authType]); const tierName = useMemo( diff --git a/packages/cli/src/ui/components/ValidationDialog.tsx b/packages/cli/src/ui/components/ValidationDialog.tsx index 6e126ea4ef..f94de6b86d 100644 --- a/packages/cli/src/ui/components/ValidationDialog.tsx +++ b/packages/cli/src/ui/components/ValidationDialog.tsx @@ -16,7 +16,8 @@ import { type ValidationIntent, } from '@google/gemini-cli-core'; import { useKeypress } from '../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; interface ValidationDialogProps { validationLink?: string; @@ -32,6 +33,7 @@ export function ValidationDialog({ learnMoreUrl, onChoice, }: ValidationDialogProps): React.JSX.Element { + const keyMatchers = useKeyMatchers(); const [state, setState] = useState('choosing'); const [errorMessage, setErrorMessage] = useState(''); diff --git a/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon-AppHeader-Icon-Rendering-renders-the-default-icon-in-standard-terminals.snap.svg b/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon-AppHeader-Icon-Rendering-renders-the-default-icon-in-standard-terminals.snap.svg new file mode 100644 index 0000000000..4e9d0e67a5 --- /dev/null +++ b/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon-AppHeader-Icon-Rendering-renders-the-default-icon-in-standard-terminals.snap.svg @@ -0,0 +1,30 @@ + + + + + ▝ + ▜ + ▄ + Gemini CLI + v1.0.0 + ▝ + ▜ + ▄ + ▗ + ▟ + ▀ + ▝ + ▀ + Tips for getting started: + 1. Create + GEMINI.md + files to customize your interactions + 2. + /help + for more information + 3. Ask coding questions, edit code or run commands + 4. Be specific for the best results + + \ No newline at end of file diff --git a/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon-AppHeader-Icon-Rendering-renders-the-symmetric-icon-in-Apple-Terminal.snap.svg b/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon-AppHeader-Icon-Rendering-renders-the-symmetric-icon-in-Apple-Terminal.snap.svg new file mode 100644 index 0000000000..fa8373acc7 --- /dev/null +++ b/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon-AppHeader-Icon-Rendering-renders-the-symmetric-icon-in-Apple-Terminal.snap.svg @@ -0,0 +1,31 @@ + + + + + ▝ + ▜ + ▄ + Gemini CLI + v1.0.0 + ▝ + ▜ + ▄ + ▗ + ▟ + ▀ + ▗ + ▟ + ▀ + Tips for getting started: + 1. Create + GEMINI.md + files to customize your interactions + 2. + /help + for more information + 3. Ask coding questions, edit code or run commands + 4. Be specific for the best results + + \ No newline at end of file diff --git a/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon.test.tsx.snap new file mode 100644 index 0000000000..2bb5276ee8 --- /dev/null +++ b/packages/cli/src/ui/components/__snapshots__/AppHeaderIcon.test.tsx.snap @@ -0,0 +1,31 @@ +// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html + +exports[`AppHeader Icon Rendering > renders the default icon in standard terminals 1`] = ` +" + ▝▜▄ Gemini CLI v1.0.0 + ▝▜▄ + ▗▟▀ + ▝▀ + + +Tips for getting started: +1. Create GEMINI.md files to customize your interactions +2. /help for more information +3. Ask coding questions, edit code or run commands +4. Be specific for the best results" +`; + +exports[`AppHeader Icon Rendering > renders the symmetric icon in Apple Terminal 1`] = ` +" + ▝▜▄ Gemini CLI v1.0.0 + ▝▜▄ + ▗▟▀ + ▗▟▀ + + +Tips for getting started: +1. Create GEMINI.md files to customize your interactions +2. /help for more information +3. Ask coding questions, edit code or run commands +4. Be specific for the best results" +`; diff --git a/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx index 40e5a7e781..b650ee4d9d 100644 --- a/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ShellToolMessage.test.tsx @@ -191,49 +191,63 @@ describe('', () => { 10, 8, false, + true, ], [ 'uses ACTIVE_SHELL_MAX_LINES when availableTerminalHeight is large', 100, ACTIVE_SHELL_MAX_LINES - 3, false, + true, ], [ 'uses full availableTerminalHeight when focused in alternate buffer mode', 100, 98, // 100 - 2 true, + false, ], [ 'defaults to ACTIVE_SHELL_MAX_LINES in alternate buffer when availableTerminalHeight is undefined', undefined, ACTIVE_SHELL_MAX_LINES - 3, false, + false, ], - ])('%s', async (_, availableTerminalHeight, expectedMaxLines, focused) => { - const { lastFrame, waitUntilReady, unmount } = renderShell( - { - resultDisplay: LONG_OUTPUT, - renderOutputAsMarkdown: false, - availableTerminalHeight, - ptyId: 1, - status: CoreToolCallStatus.Executing, - }, - { - useAlternateBuffer: true, - uiState: { - activePtyId: focused ? 1 : 2, - embeddedShellFocused: focused, + ])( + '%s', + async ( + _, + availableTerminalHeight, + expectedMaxLines, + focused, + constrainHeight, + ) => { + const { lastFrame, waitUntilReady, unmount } = renderShell( + { + resultDisplay: LONG_OUTPUT, + renderOutputAsMarkdown: false, + availableTerminalHeight, + ptyId: 1, + status: CoreToolCallStatus.Executing, }, - }, - ); + { + useAlternateBuffer: true, + uiState: { + activePtyId: focused ? 1 : 2, + embeddedShellFocused: focused, + constrainHeight, + }, + }, + ); - await waitUntilReady(); - const frame = lastFrame(); - expect(frame.match(/Line \d+/g)?.length).toBe(expectedMaxLines); - expect(frame).toMatchSnapshot(); - unmount(); - }); + await waitUntilReady(); + const frame = lastFrame(); + expect(frame.match(/Line \d+/g)?.length).toBe(expectedMaxLines); + expect(frame).toMatchSnapshot(); + unmount(); + }, + ); it('fully expands in standard mode when availableTerminalHeight is undefined', async () => { const { lastFrame, unmount } = renderShell( diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index b97a29565b..1ace75633c 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -29,7 +29,7 @@ import { import { useKeypress } from '../../hooks/useKeypress.js'; import { theme } from '../../semantic-colors.js'; import { useSettings } from '../../contexts/SettingsContext.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; import { formatCommand } from '../../utils/keybindingUtils.js'; import { AskUserDialog } from '../AskUserDialog.js'; import { ExitPlanModeDialog } from '../ExitPlanModeDialog.js'; @@ -40,6 +40,7 @@ import { toUnicodeUrl, type DeceptiveUrlDetails, } from '../../utils/urlSecurityUtils.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; export interface ToolConfirmationMessageProps { callId: string; @@ -67,6 +68,7 @@ export const ToolConfirmationMessage: React.FC< availableTerminalHeight, terminalWidth, }) => { + const keyMatchers = useKeyMatchers(); const { confirm, isDiffingEnabled } = useToolActions(); const [mcpDetailsExpansionState, setMcpDetailsExpansionState] = useState<{ callId: string; diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index df4354b1c4..e3869b6e1b 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -9,7 +9,11 @@ import { ToolMessage, type ToolMessageProps } from './ToolMessage.js'; import { describe, it, expect, vi } from 'vitest'; import { StreamingState } from '../../types.js'; import { Text } from 'ink'; -import { type AnsiOutput, CoreToolCallStatus } from '@google/gemini-cli-core'; +import { + type AnsiOutput, + CoreToolCallStatus, + Kind, +} from '@google/gemini-cli-core'; import { renderWithProviders } from '../../../test-utils/render.js'; import { tryParseJSON } from '../../../utils/jsonoutput.js'; @@ -435,4 +439,99 @@ describe('', () => { expect(output).toMatchSnapshot(); unmount(); }); + + describe('Truncation', () => { + it('applies truncation for Kind.Agent when availableTerminalHeight is provided', async () => { + const multilineString = Array.from( + { length: 30 }, + (_, i) => `Line ${i + 1}`, + ).join('\n'); + + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + { + uiActions, + uiState: { + streamingState: StreamingState.Idle, + constrainHeight: true, + }, + width: 80, + useAlternateBuffer: false, + }, + ); + await waitUntilReady(); + const output = lastFrame(); + + // Since kind=Kind.Agent and availableTerminalHeight is provided, it should truncate to SUBAGENT_MAX_LINES (15) + // and show the FIRST lines (overflowDirection='bottom') + expect(output).toContain('Line 1'); + expect(output).toContain('Line 14'); + expect(output).not.toContain('Line 16'); + expect(output).not.toContain('Line 30'); + unmount(); + }); + + it('does NOT apply truncation for Kind.Agent when availableTerminalHeight is undefined', async () => { + const multilineString = Array.from( + { length: 30 }, + (_, i) => `Line ${i + 1}`, + ).join('\n'); + + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + { + uiActions, + uiState: { streamingState: StreamingState.Idle }, + width: 80, + useAlternateBuffer: false, + }, + ); + await waitUntilReady(); + const output = lastFrame(); + + expect(output).toContain('Line 1'); + expect(output).toContain('Line 30'); + unmount(); + }); + + it('does NOT apply truncation for Kind.Read', async () => { + const multilineString = Array.from( + { length: 30 }, + (_, i) => `Line ${i + 1}`, + ).join('\n'); + + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + { + uiActions, + uiState: { streamingState: StreamingState.Idle }, + width: 80, + useAlternateBuffer: false, + }, + ); + await waitUntilReady(); + const output = lastFrame(); + + expect(output).toContain('Line 1'); + expect(output).toContain('Line 30'); + unmount(); + }); + }); }); diff --git a/packages/cli/src/ui/components/messages/ToolMessage.tsx b/packages/cli/src/ui/components/messages/ToolMessage.tsx index 7c2277d4be..5747f7677f 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.tsx @@ -21,8 +21,9 @@ import { useFocusHint, FocusHint, } from './ToolShared.js'; -import { type Config, CoreToolCallStatus } from '@google/gemini-cli-core'; +import { type Config, CoreToolCallStatus, Kind } from '@google/gemini-cli-core'; import { ShellInputPrompt } from '../ShellInputPrompt.js'; +import { SUBAGENT_MAX_LINES } from '../../constants.js'; export type { TextEmphasis }; @@ -45,6 +46,7 @@ export const ToolMessage: React.FC = ({ description, resultDisplay, status, + kind, availableTerminalHeight, terminalWidth, emphasis = 'medium', @@ -133,6 +135,12 @@ export const ToolMessage: React.FC = ({ terminalWidth={terminalWidth} renderOutputAsMarkdown={renderOutputAsMarkdown} hasFocus={isThisShellFocused} + maxLines={ + kind === Kind.Agent && availableTerminalHeight !== undefined + ? SUBAGENT_MAX_LINES + : undefined + } + overflowDirection={kind === Kind.Agent ? 'bottom' : 'top'} /> {isThisShellFocused && config && ( diff --git a/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx b/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx index f7d158d68c..02f466e72f 100644 --- a/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolResultDisplay.test.tsx @@ -6,35 +6,15 @@ import { renderWithProviders } from '../../../test-utils/render.js'; import { ToolResultDisplay } from './ToolResultDisplay.js'; -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import type { AnsiOutput } from '@google/gemini-cli-core'; -// Mock UIStateContext partially -const mockUseUIState = vi.fn(); -vi.mock('../../contexts/UIStateContext.js', async (importOriginal) => { - const actual = - await importOriginal(); - return { - ...actual, - useUIState: () => mockUseUIState(), - }; -}); - -// Mock useAlternateBuffer -const mockUseAlternateBuffer = vi.fn(); -vi.mock('../../hooks/useAlternateBuffer.js', () => ({ - useAlternateBuffer: () => mockUseAlternateBuffer(), -})); - describe('ToolResultDisplay', () => { beforeEach(() => { vi.clearAllMocks(); - mockUseUIState.mockReturnValue({ renderMarkdown: true }); - mockUseAlternateBuffer.mockReturnValue(false); }); it('uses ScrollableList for ANSI output in alternate buffer mode', async () => { - mockUseAlternateBuffer.mockReturnValue(true); const content = 'ansi content'; const ansiResult: AnsiOutput = [ [ @@ -56,6 +36,7 @@ describe('ToolResultDisplay', () => { terminalWidth={80} maxLines={10} />, + { useAlternateBuffer: true }, ); await waitUntilReady(); const output = lastFrame(); @@ -65,13 +46,13 @@ describe('ToolResultDisplay', () => { }); it('uses Scrollable for non-ANSI output in alternate buffer mode', async () => { - mockUseAlternateBuffer.mockReturnValue(true); const { lastFrame, waitUntilReady, unmount } = renderWithProviders( , + { useAlternateBuffer: true }, ); await waitUntilReady(); const output = lastFrame(); @@ -82,13 +63,13 @@ describe('ToolResultDisplay', () => { }); it('passes hasFocus prop to scrollable components', async () => { - mockUseAlternateBuffer.mockReturnValue(true); const { lastFrame, waitUntilReady, unmount } = renderWithProviders( , + { useAlternateBuffer: true }, ); await waitUntilReady(); @@ -99,6 +80,7 @@ describe('ToolResultDisplay', () => { it('renders string result as markdown by default', async () => { const { lastFrame, waitUntilReady, unmount } = renderWithProviders( , + { useAlternateBuffer: false }, ); await waitUntilReady(); const output = lastFrame(); @@ -115,6 +97,10 @@ describe('ToolResultDisplay', () => { availableTerminalHeight={20} renderOutputAsMarkdown={false} />, + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, ); await waitUntilReady(); const output = lastFrame(); @@ -131,6 +117,10 @@ describe('ToolResultDisplay', () => { terminalWidth={80} availableTerminalHeight={20} />, + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, ); await waitUntilReady(); const output = lastFrame(); @@ -150,6 +140,7 @@ describe('ToolResultDisplay', () => { terminalWidth={80} availableTerminalHeight={20} />, + { useAlternateBuffer: false }, ); await waitUntilReady(); const output = lastFrame(); @@ -179,6 +170,7 @@ describe('ToolResultDisplay', () => { terminalWidth={80} availableTerminalHeight={20} />, + { useAlternateBuffer: false }, ); await waitUntilReady(); const output = lastFrame(); @@ -197,6 +189,7 @@ describe('ToolResultDisplay', () => { terminalWidth={80} availableTerminalHeight={20} />, + { useAlternateBuffer: false }, ); await waitUntilReady(); const output = lastFrame({ allowEmpty: true }); @@ -206,7 +199,6 @@ describe('ToolResultDisplay', () => { }); it('does not fall back to plain text if availableHeight is set and not in alternate buffer', async () => { - mockUseAlternateBuffer.mockReturnValue(false); // availableHeight calculation: 20 - 1 - 5 = 14 > 3 const { lastFrame, waitUntilReady, unmount } = renderWithProviders( { availableTerminalHeight={20} renderOutputAsMarkdown={true} />, + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, ); await waitUntilReady(); const output = lastFrame(); @@ -223,7 +219,6 @@ describe('ToolResultDisplay', () => { }); it('keeps markdown if in alternate buffer even with availableHeight', async () => { - mockUseAlternateBuffer.mockReturnValue(true); const { lastFrame, waitUntilReady, unmount } = renderWithProviders( { availableTerminalHeight={20} renderOutputAsMarkdown={true} />, + { useAlternateBuffer: true }, ); await waitUntilReady(); const output = lastFrame(); @@ -309,6 +305,10 @@ describe('ToolResultDisplay', () => { availableTerminalHeight={20} maxLines={3} />, + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, ); await waitUntilReady(); const output = lastFrame(); @@ -341,6 +341,10 @@ describe('ToolResultDisplay', () => { maxLines={25} availableTerminalHeight={undefined} />, + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, ); await waitUntilReady(); const output = lastFrame(); diff --git a/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx b/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx index 05b94442db..0bbe3446e0 100644 --- a/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx +++ b/packages/cli/src/ui/components/messages/ToolResultDisplay.tsx @@ -9,7 +9,7 @@ import { Box, Text } from 'ink'; import { DiffRenderer } from './DiffRenderer.js'; import { MarkdownDisplay } from '../../utils/MarkdownDisplay.js'; import { AnsiOutputText, AnsiLineText } from '../AnsiOutput.js'; -import { MaxSizedBox } from '../shared/MaxSizedBox.js'; +import { SlicingMaxSizedBox } from '../shared/SlicingMaxSizedBox.js'; import { theme } from '../../semantic-colors.js'; import { type AnsiOutput, @@ -26,10 +26,6 @@ import { ACTIVE_SHELL_MAX_LINES } from '../../constants.js'; import { calculateToolContentMaxLines } from '../../utils/toolLayoutUtils.js'; import { SubagentProgressDisplay } from './SubagentProgressDisplay.js'; -// Large threshold to ensure we don't cause performance issues for very large -// outputs that will get truncated further MaxSizedBox anyway. -const MAXIMUM_RESULT_DISPLAY_CHARACTERS = 20000; - export interface ToolResultDisplayProps { resultDisplay: string | object | undefined; availableTerminalHeight?: number; @@ -37,6 +33,7 @@ export interface ToolResultDisplayProps { renderOutputAsMarkdown?: boolean; maxLines?: number; hasFocus?: boolean; + overflowDirection?: 'top' | 'bottom'; } interface FileDiffResult { @@ -51,6 +48,7 @@ export const ToolResultDisplay: React.FC = ({ renderOutputAsMarkdown = true, maxLines, hasFocus = false, + overflowDirection = 'top', }) => { const { renderMarkdown } = useUIState(); const isAlternateBuffer = useAlternateBuffer(); @@ -78,180 +76,147 @@ export const ToolResultDisplay: React.FC = ({ [], ); - const { truncatedResultDisplay, hiddenLinesCount } = React.useMemo(() => { - let hiddenLines = 0; - // Only truncate string output if not in alternate buffer mode to ensure - // we can scroll through the full output. - if (typeof resultDisplay === 'string' && !isAlternateBuffer) { - let text = resultDisplay; - if (text.length > MAXIMUM_RESULT_DISPLAY_CHARACTERS) { - text = '...' + text.slice(-MAXIMUM_RESULT_DISPLAY_CHARACTERS); - } - if (maxLines) { - const hasTrailingNewline = text.endsWith('\n'); - const contentText = hasTrailingNewline ? text.slice(0, -1) : text; - const lines = contentText.split('\n'); - if (lines.length > maxLines) { - // We will have a label from MaxSizedBox. Reserve space for it. - const targetLines = Math.max(1, maxLines - 1); - hiddenLines = lines.length - targetLines; - text = - lines.slice(-targetLines).join('\n') + - (hasTrailingNewline ? '\n' : ''); - } - } - return { truncatedResultDisplay: text, hiddenLinesCount: hiddenLines }; - } - - if (Array.isArray(resultDisplay) && !isAlternateBuffer && maxLines) { - if (resultDisplay.length > maxLines) { - // We will have a label from MaxSizedBox. Reserve space for it. - const targetLines = Math.max(1, maxLines - 1); - return { - truncatedResultDisplay: resultDisplay.slice(-targetLines), - hiddenLinesCount: resultDisplay.length - targetLines, - }; - } - } - - return { truncatedResultDisplay: resultDisplay, hiddenLinesCount: 0 }; - }, [resultDisplay, isAlternateBuffer, maxLines]); - - if (!truncatedResultDisplay) return null; + if (!resultDisplay) return null; // 1. Early return for background tools (Todos) - if ( - typeof truncatedResultDisplay === 'object' && - 'todos' in truncatedResultDisplay - ) { + if (typeof resultDisplay === 'object' && 'todos' in resultDisplay) { // display nothing, as the TodoTray will handle rendering todos return null; } - // 2. High-performance path: Virtualized ANSI in interactive mode - if (isAlternateBuffer && Array.isArray(truncatedResultDisplay)) { - // If availableHeight is undefined, fallback to a safe default to prevents infinite loop - // where Container grows -> List renders more -> Container grows. - const limit = maxLines ?? availableHeight ?? ACTIVE_SHELL_MAX_LINES; - const listHeight = Math.min( - // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion - (truncatedResultDisplay as AnsiOutput).length, - limit, - ); + const renderContent = (contentData: string | object | undefined) => { + // Check if string content is valid JSON and pretty-print it + const prettyJSON = + typeof contentData === 'string' ? tryParseJSON(contentData) : null; + const formattedJSON = prettyJSON + ? JSON.stringify(prettyJSON, null, 2) + : null; - return ( - - 1} - keyExtractor={keyExtractor} - initialScrollIndex={SCROLL_TO_ITEM_END} - hasFocus={hasFocus} + let content: React.ReactNode; + + if (formattedJSON) { + // Render pretty-printed JSON + content = ( + + {formattedJSON} + + ); + } else if (isSubagentProgress(contentData)) { + content = ; + } else if (typeof contentData === 'string' && renderOutputAsMarkdown) { + content = ( + + ); + } else if (typeof contentData === 'string' && !renderOutputAsMarkdown) { + content = ( + + {contentData} + + ); + } else if (typeof contentData === 'object' && 'fileDiff' in contentData) { + content = ( + + ); + } else { + const shouldDisableTruncation = + isAlternateBuffer || + (availableTerminalHeight === undefined && maxLines === undefined); + + content = ( + + ); + } + + // Final render based on session mode + if (isAlternateBuffer) { + return ( + + {content} + + ); + } + + return content; + }; + + // ASB Mode Handling (Interactive/Fullscreen) + if (isAlternateBuffer) { + // Virtualized path for large ANSI arrays + if (Array.isArray(resultDisplay)) { + const limit = maxLines ?? availableHeight ?? ACTIVE_SHELL_MAX_LINES; + const listHeight = Math.min( + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + (resultDisplay as AnsiOutput).length, + limit, + ); + + return ( + + 1} + keyExtractor={keyExtractor} + initialScrollIndex={SCROLL_TO_ITEM_END} + hasFocus={hasFocus} + /> + + ); + } + + // Standard path for strings/diffs in ASB + return ( + + {renderContent(resultDisplay)} ); } - // 3. Compute content node for non-virtualized paths - // Check if string content is valid JSON and pretty-print it - const prettyJSON = - typeof truncatedResultDisplay === 'string' - ? tryParseJSON(truncatedResultDisplay) - : null; - const formattedJSON = prettyJSON ? JSON.stringify(prettyJSON, null, 2) : null; - - let content: React.ReactNode; - - if (formattedJSON) { - // Render pretty-printed JSON - content = ( - - {formattedJSON} - - ); - } else if (isSubagentProgress(truncatedResultDisplay)) { - content = ; - } else if ( - typeof truncatedResultDisplay === 'string' && - renderOutputAsMarkdown - ) { - content = ( - - ); - } else if ( - typeof truncatedResultDisplay === 'string' && - !renderOutputAsMarkdown - ) { - content = ( - - {truncatedResultDisplay} - - ); - } else if ( - typeof truncatedResultDisplay === 'object' && - 'fileDiff' in truncatedResultDisplay - ) { - content = ( - - ); - } else { - const shouldDisableTruncation = - isAlternateBuffer || - (availableTerminalHeight === undefined && maxLines === undefined); - - content = ( - - ); - } - - // 4. Final render based on session mode - if (isAlternateBuffer) { - return ( - - {content} - - ); - } - + // Standard Mode Handling (History/Scrollback) + // We use SlicingMaxSizedBox which includes MaxSizedBox for precision truncation + hidden labels return ( - - {content} - + {(truncatedResultDisplay) => renderContent(truncatedResultDisplay)} + ); }; diff --git a/packages/cli/src/ui/components/messages/ToolResultDisplayOverflow.test.tsx b/packages/cli/src/ui/components/messages/ToolResultDisplayOverflow.test.tsx new file mode 100644 index 0000000000..b809e89748 --- /dev/null +++ b/packages/cli/src/ui/components/messages/ToolResultDisplayOverflow.test.tsx @@ -0,0 +1,101 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { renderWithProviders } from '../../../test-utils/render.js'; +import { ToolResultDisplay } from './ToolResultDisplay.js'; +import { describe, it, expect } from 'vitest'; +import { type AnsiOutput } from '@google/gemini-cli-core'; + +describe('ToolResultDisplay Overflow', () => { + it('shows the head of the content when overflowDirection is bottom (string)', async () => { + const content = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5'; + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, + ); + await waitUntilReady(); + const output = lastFrame(); + + expect(output).toContain('Line 1'); + expect(output).toContain('Line 2'); + expect(output).not.toContain('Line 3'); // Line 3 is replaced by the "hidden" label + expect(output).not.toContain('Line 4'); + expect(output).not.toContain('Line 5'); + expect(output).toContain('hidden'); + unmount(); + }); + + it('shows the tail of the content when overflowDirection is top (string default)', async () => { + const content = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5'; + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, + ); + await waitUntilReady(); + const output = lastFrame(); + + expect(output).not.toContain('Line 1'); + expect(output).not.toContain('Line 2'); + expect(output).not.toContain('Line 3'); + expect(output).toContain('Line 4'); + expect(output).toContain('Line 5'); + expect(output).toContain('hidden'); + unmount(); + }); + + it('shows the head of the content when overflowDirection is bottom (ANSI)', async () => { + const ansiResult: AnsiOutput = Array.from({ length: 5 }, (_, i) => [ + { + text: `Line ${i + 1}`, + fg: '', + bg: '', + bold: false, + italic: false, + underline: false, + dim: false, + inverse: false, + }, + ]); + const { lastFrame, waitUntilReady, unmount } = renderWithProviders( + , + { + useAlternateBuffer: false, + uiState: { constrainHeight: true }, + }, + ); + await waitUntilReady(); + const output = lastFrame(); + + expect(output).toContain('Line 1'); + expect(output).toContain('Line 2'); + expect(output).not.toContain('Line 3'); + expect(output).not.toContain('Line 4'); + expect(output).not.toContain('Line 5'); + expect(output).toContain('hidden'); + unmount(); + }); +}); diff --git a/packages/cli/src/ui/components/messages/__snapshots__/ToolResultDisplayOverflow.test.tsx.snap b/packages/cli/src/ui/components/messages/__snapshots__/ToolResultDisplayOverflow.test.tsx.snap deleted file mode 100644 index aab4b690a1..0000000000 --- a/packages/cli/src/ui/components/messages/__snapshots__/ToolResultDisplayOverflow.test.tsx.snap +++ /dev/null @@ -1,16 +0,0 @@ -// Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html - -exports[`ToolResultDisplay Overflow > should display "press ctrl-o" hint when content overflows in ToolGroupMessage 1`] = ` -"╭──────────────────────────────────────────────────────────────────────────╮ -│ ✓ test-tool a test tool │ -│ │ -│ line 45 │ -│ line 46 │ -│ line 47 │ -│ line 48 │ -│ line 49 │ -│ line 50 █ │ -╰──────────────────────────────────────────────────────────────────────────╯ - Press Ctrl+O to show more lines -" -`; diff --git a/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx b/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx index 4047ec9ef8..5cc731e3f7 100644 --- a/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx +++ b/packages/cli/src/ui/components/shared/BaseSettingsDialog.test.tsx @@ -174,7 +174,10 @@ describe('BaseSettingsDialog', () => { it('should render footer content when provided', async () => { const { lastFrame, unmount } = await renderDialog({ - footerContent: Custom Footer, + footer: { + content: Custom Footer, + height: 1, + }, }); expect(lastFrame()).toContain('Custom Footer'); @@ -801,4 +804,57 @@ describe('BaseSettingsDialog', () => { unmount(); }); }); + + describe('responsiveness', () => { + it('should show the scope selector when availableHeight is sufficient (25)', async () => { + const { lastFrame, unmount } = await renderDialog({ + availableHeight: 25, + showScopeSelector: true, + }); + + const frame = lastFrame(); + expect(frame).toContain('Apply To'); + unmount(); + }); + + it('should hide the scope selector when availableHeight is small (24) to show more items', async () => { + const { lastFrame, unmount } = await renderDialog({ + availableHeight: 24, + showScopeSelector: true, + }); + + const frame = lastFrame(); + expect(frame).not.toContain('Apply To'); + unmount(); + }); + + it('should reduce the number of visible items based on height', async () => { + // At height 25, it should show 2 items (math: (25-4 - (10+5))/3 = 2) + const { lastFrame, unmount } = await renderDialog({ + availableHeight: 25, + items: createMockItems(10), + }); + + const frame = lastFrame(); + // Items 0 and 1 should be there + expect(frame).toContain('Boolean Setting'); + expect(frame).toContain('String Setting'); + // Item 2 should NOT be there + expect(frame).not.toContain('Number Setting'); + unmount(); + }); + + it('should show scroll indicators when list is truncated by height', async () => { + const { lastFrame, unmount } = await renderDialog({ + availableHeight: 25, + items: createMockItems(10), + }); + + const frame = lastFrame(); + // Shows both scroll indicators when the list is truncated by height + expect(frame).toContain('▼'); + expect(frame).toContain('▲'); + unmount(); + }); + }); }); diff --git a/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx b/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx index 05cef4fcf2..45dda8b38c 100644 --- a/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx +++ b/packages/cli/src/ui/components/shared/BaseSettingsDialog.tsx @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import React, { useState, useEffect, useCallback, useRef } from 'react'; +import React, { useMemo, useState, useCallback } from 'react'; import { Box, Text } from 'ink'; import chalk from 'chalk'; import { theme } from '../../semantic-colors.js'; @@ -17,15 +17,13 @@ import { getScopeItems } from '../../../utils/dialogScopeUtils.js'; import { RadioButtonSelect } from './RadioButtonSelect.js'; import { TextInput } from './TextInput.js'; import type { TextBuffer } from './text-buffer.js'; -import { - cpSlice, - cpLen, - stripUnsafeCharacters, - cpIndexToOffset, -} from '../../utils/textUtils.js'; +import { cpSlice, cpLen, cpIndexToOffset } from '../../utils/textUtils.js'; import { useKeypress, type Key } from '../../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; +import { useSettingsNavigation } from '../../hooks/useSettingsNavigation.js'; +import { useInlineEditBuffer } from '../../hooks/useInlineEditBuffer.js'; import { formatCommand } from '../../utils/keybindingUtils.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; /** * Represents a single item in the settings dialog. @@ -60,7 +58,6 @@ export interface BaseSettingsDialogProps { title: string; /** Optional border color for the dialog */ borderColor?: string; - // Search (optional feature) /** Whether to show the search input. Default: true */ searchEnabled?: boolean; @@ -106,9 +103,14 @@ export interface BaseSettingsDialogProps { currentItem: SettingsDialogItem | undefined, ) => boolean; - // Optional extra content below help text (for restart prompt, etc.) - /** Optional footer content (e.g., restart prompt) */ - footerContent?: React.ReactNode; + /** Available terminal height for dynamic windowing */ + availableHeight?: number; + + /** Optional footer configuration */ + footer?: { + content: React.ReactNode; + height: number; + }; } /** @@ -132,68 +134,114 @@ export function BaseSettingsDialog({ onItemClear, onClose, onKeyPress, - footerContent, + availableHeight, + footer, }: BaseSettingsDialogProps): React.JSX.Element { + const keyMatchers = useKeyMatchers(); + // Calculate effective max items and scope visibility based on terminal height + const { effectiveMaxItemsToShow, finalShowScopeSelector } = useMemo(() => { + const initialShowScope = showScopeSelector; + const initialMaxItems = maxItemsToShow; + + if (!availableHeight) { + return { + effectiveMaxItemsToShow: initialMaxItems, + finalShowScopeSelector: initialShowScope, + }; + } + + // Layout constants based on BaseSettingsDialog structure: + const DIALOG_PADDING = 4; + const SETTINGS_TITLE_HEIGHT = 1; + // Account for the unconditional spacer below search/title section + const SEARCH_SECTION_HEIGHT = searchEnabled ? 5 : 1; + const SCROLL_ARROWS_HEIGHT = 2; + const ITEMS_SPACING_AFTER = 1; + const SCOPE_SECTION_HEIGHT = 5; + const HELP_TEXT_HEIGHT = 1; + const FOOTER_CONTENT_HEIGHT = footer?.height ?? 0; + const ITEM_HEIGHT = 3; + + const currentAvailableHeight = availableHeight - DIALOG_PADDING; + + const baseFixedHeight = + SETTINGS_TITLE_HEIGHT + + SEARCH_SECTION_HEIGHT + + SCROLL_ARROWS_HEIGHT + + ITEMS_SPACING_AFTER + + HELP_TEXT_HEIGHT + + FOOTER_CONTENT_HEIGHT; + + // Calculate max items with scope selector + const heightWithScope = baseFixedHeight + SCOPE_SECTION_HEIGHT; + const availableForItemsWithScope = currentAvailableHeight - heightWithScope; + const maxItemsWithScope = Math.max( + 1, + Math.floor(availableForItemsWithScope / ITEM_HEIGHT), + ); + + // Calculate max items without scope selector + const availableForItemsWithoutScope = + currentAvailableHeight - baseFixedHeight; + const maxItemsWithoutScope = Math.max( + 1, + Math.floor(availableForItemsWithoutScope / ITEM_HEIGHT), + ); + + // In small terminals, hide scope selector if it would allow more items to show + let shouldShowScope = initialShowScope; + let maxItems = initialShowScope ? maxItemsWithScope : maxItemsWithoutScope; + + if (initialShowScope && availableHeight < 25) { + // Hide scope selector if it gains us more than 1 extra item + if (maxItemsWithoutScope > maxItemsWithScope + 1) { + shouldShowScope = false; + maxItems = maxItemsWithoutScope; + } + } + + return { + effectiveMaxItemsToShow: Math.min(maxItems, items.length), + finalShowScopeSelector: shouldShowScope, + }; + }, [ + availableHeight, + maxItemsToShow, + items.length, + searchEnabled, + showScopeSelector, + footer, + ]); + // Internal state - const [activeIndex, setActiveIndex] = useState(0); - const [scrollOffset, setScrollOffset] = useState(0); + const { activeIndex, windowStart, moveUp, moveDown } = useSettingsNavigation({ + items, + maxItemsToShow: effectiveMaxItemsToShow, + }); + + const { editState, editDispatch, startEditing, commitEdit, cursorVisible } = + useInlineEditBuffer({ + onCommit: (key, value) => { + const itemToCommit = items.find((i) => i.key === key); + if (itemToCommit) { + onEditCommit(key, value, itemToCommit); + } + }, + }); + + const { + editingKey, + buffer: editBuffer, + cursorPos: editCursorPos, + } = editState; + const [focusSection, setFocusSection] = useState<'settings' | 'scope'>( 'settings', ); - const [editingKey, setEditingKey] = useState(null); - const [editBuffer, setEditBuffer] = useState(''); - const [editCursorPos, setEditCursorPos] = useState(0); - const [cursorVisible, setCursorVisible] = useState(true); - - const prevItemsRef = useRef(items); - - // Preserve focus when items change (e.g., search filter) - useEffect(() => { - const prevItems = prevItemsRef.current; - if (prevItems !== items) { - const prevActiveItem = prevItems[activeIndex]; - if (prevActiveItem) { - const newIndex = items.findIndex((i) => i.key === prevActiveItem.key); - if (newIndex !== -1) { - // Item still exists in the filtered list, keep focus on it - setActiveIndex(newIndex); - // Adjust scroll offset to ensure the item is visible - let newScroll = scrollOffset; - if (newIndex < scrollOffset) newScroll = newIndex; - else if (newIndex >= scrollOffset + maxItemsToShow) - newScroll = newIndex - maxItemsToShow + 1; - - const maxScroll = Math.max(0, items.length - maxItemsToShow); - setScrollOffset(Math.min(newScroll, maxScroll)); - } else { - // Item was filtered out, reset to the top - setActiveIndex(0); - setScrollOffset(0); - } - } else { - setActiveIndex(0); - setScrollOffset(0); - } - prevItemsRef.current = items; - } - }, [items, activeIndex, scrollOffset, maxItemsToShow]); - - // Cursor blink effect - useEffect(() => { - if (!editingKey) return; - setCursorVisible(true); - const interval = setInterval(() => { - setCursorVisible((v) => !v); - }, 500); - return () => clearInterval(interval); - }, [editingKey]); - - // Ensure focus stays on settings when scope selection is hidden - useEffect(() => { - if (!showScopeSelector && focusSection === 'scope') { - setFocusSection('settings'); - } - }, [showScopeSelector, focusSection]); + const effectiveFocusSection = + !finalShowScopeSelector && focusSection === 'scope' + ? 'settings' + : focusSection; // Scope selector items const scopeItems = getScopeItems().map((item) => ({ @@ -202,43 +250,20 @@ export function BaseSettingsDialog({ })); // Calculate visible items based on scroll offset - const visibleItems = items.slice(scrollOffset, scrollOffset + maxItemsToShow); + const visibleItems = items.slice( + windowStart, + windowStart + effectiveMaxItemsToShow, + ); // Show scroll indicators if there are more items than can be displayed - const showScrollUp = items.length > maxItemsToShow; - const showScrollDown = items.length > maxItemsToShow; + const showScrollUp = items.length > effectiveMaxItemsToShow; + const showScrollDown = items.length > effectiveMaxItemsToShow; // Get current item const currentItem = items[activeIndex]; - // Start editing a field - const startEditing = useCallback((key: string, initialValue: string) => { - setEditingKey(key); - setEditBuffer(initialValue); - setEditCursorPos(cpLen(initialValue)); - setCursorVisible(true); - }, []); - - // Commit edit and exit edit mode - const commitEdit = useCallback(() => { - if (editingKey && currentItem) { - onEditCommit(editingKey, editBuffer, currentItem); - } - setEditingKey(null); - setEditBuffer(''); - setEditCursorPos(0); - }, [editingKey, editBuffer, currentItem, onEditCommit]); - - // Handle scope highlight (for RadioButtonSelect) - const handleScopeHighlight = useCallback( - (scope: LoadableSettingScope) => { - onScopeChange?.(scope); - }, - [onScopeChange], - ); - - // Handle scope select (for RadioButtonSelect) - const handleScopeSelect = useCallback( + // Handle scope changes (for RadioButtonSelect) + const handleScopeChange = useCallback( (scope: LoadableSettingScope) => { onScopeChange?.(scope); }, @@ -248,8 +273,8 @@ export function BaseSettingsDialog({ // Keyboard handling useKeypress( (key: Key) => { - // Let parent handle custom keys first - if (onKeyPress?.(key, currentItem)) { + // Let parent handle custom keys first (only if not editing) + if (!editingKey && onKeyPress?.(key, currentItem)) { return; } @@ -260,44 +285,31 @@ export function BaseSettingsDialog({ // Navigation within edit buffer if (keyMatchers[Command.MOVE_LEFT](key)) { - setEditCursorPos((p) => Math.max(0, p - 1)); + editDispatch({ type: 'MOVE_LEFT' }); return; } if (keyMatchers[Command.MOVE_RIGHT](key)) { - setEditCursorPos((p) => Math.min(cpLen(editBuffer), p + 1)); + editDispatch({ type: 'MOVE_RIGHT' }); return; } if (keyMatchers[Command.HOME](key)) { - setEditCursorPos(0); + editDispatch({ type: 'HOME' }); return; } if (keyMatchers[Command.END](key)) { - setEditCursorPos(cpLen(editBuffer)); + editDispatch({ type: 'END' }); return; } // Backspace if (keyMatchers[Command.DELETE_CHAR_LEFT](key)) { - if (editCursorPos > 0) { - setEditBuffer((b) => { - const before = cpSlice(b, 0, editCursorPos - 1); - const after = cpSlice(b, editCursorPos); - return before + after; - }); - setEditCursorPos((p) => p - 1); - } + editDispatch({ type: 'DELETE_LEFT' }); return; } // Delete if (keyMatchers[Command.DELETE_CHAR_RIGHT](key)) { - if (editCursorPos < cpLen(editBuffer)) { - setEditBuffer((b) => { - const before = cpSlice(b, 0, editCursorPos); - const after = cpSlice(b, editCursorPos + 1); - return before + after; - }); - } + editDispatch({ type: 'DELETE_RIGHT' }); return; } @@ -316,70 +328,35 @@ export function BaseSettingsDialog({ // Up/Down in edit mode - commit and navigate if (keyMatchers[Command.DIALOG_NAVIGATION_UP](key)) { commitEdit(); - const newIndex = activeIndex > 0 ? activeIndex - 1 : items.length - 1; - setActiveIndex(newIndex); - if (newIndex === items.length - 1) { - setScrollOffset(Math.max(0, items.length - maxItemsToShow)); - } else if (newIndex < scrollOffset) { - setScrollOffset(newIndex); - } + moveUp(); return; } if (keyMatchers[Command.DIALOG_NAVIGATION_DOWN](key)) { commitEdit(); - const newIndex = activeIndex < items.length - 1 ? activeIndex + 1 : 0; - setActiveIndex(newIndex); - if (newIndex === 0) { - setScrollOffset(0); - } else if (newIndex >= scrollOffset + maxItemsToShow) { - setScrollOffset(newIndex - maxItemsToShow + 1); - } + moveDown(); return; } // Character input - let ch = key.sequence; - let isValidChar = false; - if (type === 'number') { - isValidChar = /[0-9\-+.]/.test(ch); - } else { - isValidChar = ch.length === 1 && ch.charCodeAt(0) >= 32; - // Sanitize string input to prevent unsafe characters - ch = stripUnsafeCharacters(ch); - } - - if (isValidChar && ch.length > 0) { - setEditBuffer((b) => { - const before = cpSlice(b, 0, editCursorPos); - const after = cpSlice(b, editCursorPos); - return before + ch + after; + if (key.sequence) { + editDispatch({ + type: 'INSERT_CHAR', + char: key.sequence, + isNumberType: type === 'number', }); - setEditCursorPos((p) => p + 1); } return; } // Not in edit mode - handle navigation and actions - if (focusSection === 'settings') { + if (effectiveFocusSection === 'settings') { // Up/Down navigation with wrap-around if (keyMatchers[Command.DIALOG_NAVIGATION_UP](key)) { - const newIndex = activeIndex > 0 ? activeIndex - 1 : items.length - 1; - setActiveIndex(newIndex); - if (newIndex === items.length - 1) { - setScrollOffset(Math.max(0, items.length - maxItemsToShow)); - } else if (newIndex < scrollOffset) { - setScrollOffset(newIndex); - } + moveUp(); return true; } if (keyMatchers[Command.DIALOG_NAVIGATION_DOWN](key)) { - const newIndex = activeIndex < items.length - 1 ? activeIndex + 1 : 0; - setActiveIndex(newIndex); - if (newIndex === 0) { - setScrollOffset(0); - } else if (newIndex >= scrollOffset + maxItemsToShow) { - setScrollOffset(newIndex - maxItemsToShow + 1); - } + moveDown(); return true; } @@ -412,7 +389,7 @@ export function BaseSettingsDialog({ } // Tab - switch focus section - if (key.name === 'tab' && showScopeSelector) { + if (key.name === 'tab' && finalShowScopeSelector) { setFocusSection((s) => (s === 'settings' ? 'scope' : 'settings')); return; } @@ -427,7 +404,7 @@ export function BaseSettingsDialog({ }, { isActive: true, - priority: focusSection === 'settings' && !editingKey, + priority: effectiveFocusSection === 'settings', }, ); @@ -444,10 +421,10 @@ export function BaseSettingsDialog({ {/* Title */} - {focusSection === 'settings' ? '> ' : ' '} + {effectiveFocusSection === 'settings' ? '> ' : ' '} {title}{' '} @@ -459,7 +436,7 @@ export function BaseSettingsDialog({ borderColor={ editingKey ? theme.border.default - : focusSection === 'settings' + : effectiveFocusSection === 'settings' ? theme.ui.focus : theme.border.default } @@ -468,7 +445,7 @@ export function BaseSettingsDialog({ marginTop={1} > @@ -490,9 +467,10 @@ export function BaseSettingsDialog({ )} {visibleItems.map((item, idx) => { - const globalIndex = idx + scrollOffset; + const globalIndex = idx + windowStart; const isActive = - focusSection === 'settings' && activeIndex === globalIndex; + effectiveFocusSection === 'settings' && + activeIndex === globalIndex; // Compute display value with edit mode cursor let displayValue: string; @@ -602,21 +580,21 @@ export function BaseSettingsDialog({ {/* Scope Selection */} - {showScopeSelector && ( + {finalShowScopeSelector && ( - - {focusSection === 'scope' ? '> ' : ' '}Apply To + + {effectiveFocusSection === 'scope' ? '> ' : ' '}Apply To item.value === selectedScope, )} - onSelect={handleScopeSelect} - onHighlight={handleScopeHighlight} - isFocused={focusSection === 'scope'} - showNumbers={focusSection === 'scope'} - priority={focusSection === 'scope'} + onSelect={handleScopeChange} + onHighlight={handleScopeChange} + isFocused={effectiveFocusSection === 'scope'} + showNumbers={effectiveFocusSection === 'scope'} + priority={effectiveFocusSection === 'scope'} /> )} @@ -627,12 +605,13 @@ export function BaseSettingsDialog({ (Use Enter to select, {formatCommand(Command.CLEAR_SCREEN)} to reset - {showScopeSelector ? ', Tab to change focus' : ''}, Esc to close) + {finalShowScopeSelector ? ', Tab to change focus' : ''}, Esc to + close) {/* Footer content (e.g., restart prompt) */} - {footerContent && {footerContent}} + {footer && {footer.content}} ); diff --git a/packages/cli/src/ui/components/shared/MaxSizedBox.tsx b/packages/cli/src/ui/components/shared/MaxSizedBox.tsx index ee91d34f57..e88dcd4b76 100644 --- a/packages/cli/src/ui/components/shared/MaxSizedBox.tsx +++ b/packages/cli/src/ui/components/shared/MaxSizedBox.tsx @@ -20,7 +20,7 @@ import { formatCommand } from '../../utils/keybindingUtils.js'; */ export const MINIMUM_MAX_HEIGHT = 2; -interface MaxSizedBoxProps { +export interface MaxSizedBoxProps { children?: React.ReactNode; maxWidth?: number; maxHeight?: number; diff --git a/packages/cli/src/ui/components/shared/Scrollable.tsx b/packages/cli/src/ui/components/shared/Scrollable.tsx index a7227c7087..a1f9be0b7c 100644 --- a/packages/cli/src/ui/components/shared/Scrollable.tsx +++ b/packages/cli/src/ui/components/shared/Scrollable.tsx @@ -19,8 +19,9 @@ import { useKeypress, type Key } from '../../hooks/useKeypress.js'; import { useScrollable } from '../../contexts/ScrollProvider.js'; import { useAnimatedScrollbar } from '../../hooks/useAnimatedScrollbar.js'; import { useBatchedScroll } from '../../hooks/useBatchedScroll.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; import { useOverflowActions } from '../../contexts/OverflowContext.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; interface ScrollableProps { children?: React.ReactNode; @@ -45,6 +46,7 @@ export const Scrollable: React.FC = ({ flexGrow, reportOverflow = false, }) => { + const keyMatchers = useKeyMatchers(); const [scrollTop, setScrollTop] = useState(0); const viewportRef = useRef(null); const contentRef = useRef(null); diff --git a/packages/cli/src/ui/components/shared/ScrollableList.tsx b/packages/cli/src/ui/components/shared/ScrollableList.tsx index b7085329a3..33a3f72310 100644 --- a/packages/cli/src/ui/components/shared/ScrollableList.tsx +++ b/packages/cli/src/ui/components/shared/ScrollableList.tsx @@ -22,7 +22,8 @@ import { useScrollable } from '../../contexts/ScrollProvider.js'; import { Box, type DOMElement } from 'ink'; import { useAnimatedScrollbar } from '../../hooks/useAnimatedScrollbar.js'; import { useKeypress, type Key } from '../../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; const ANIMATION_FRAME_DURATION_MS = 33; @@ -46,6 +47,7 @@ function ScrollableList( props: ScrollableListProps, ref: React.Ref>, ) { + const keyMatchers = useKeyMatchers(); const { hasFocus, width } = props; const virtualizedListRef = useRef>(null); const containerRef = useRef(null); diff --git a/packages/cli/src/ui/components/shared/SearchableList.tsx b/packages/cli/src/ui/components/shared/SearchableList.tsx index 1611bc2842..046040af90 100644 --- a/packages/cli/src/ui/components/shared/SearchableList.tsx +++ b/packages/cli/src/ui/components/shared/SearchableList.tsx @@ -11,7 +11,8 @@ import { useSelectionList } from '../../hooks/useSelectionList.js'; import { TextInput } from './TextInput.js'; import type { TextBuffer } from './text-buffer.js'; import { useKeypress } from '../../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; /** * Generic interface for items in a searchable list. @@ -85,6 +86,7 @@ export function SearchableList({ onSearch, resetSelectionOnItemsChange = false, }: SearchableListProps): React.JSX.Element { + const keyMatchers = useKeyMatchers(); const { filteredItems, searchBuffer, maxLabelWidth } = useSearch({ items, onSearch, diff --git a/packages/cli/src/ui/components/shared/SlicingMaxSizedBox.test.tsx b/packages/cli/src/ui/components/shared/SlicingMaxSizedBox.test.tsx new file mode 100644 index 0000000000..184c968836 --- /dev/null +++ b/packages/cli/src/ui/components/shared/SlicingMaxSizedBox.test.tsx @@ -0,0 +1,123 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { render } from '../../../test-utils/render.js'; +import { OverflowProvider } from '../../contexts/OverflowContext.js'; +import { SlicingMaxSizedBox } from './SlicingMaxSizedBox.js'; +import { Box, Text } from 'ink'; +import { describe, it, expect } from 'vitest'; + +describe('', () => { + it('renders string data without slicing when it fits', async () => { + const { lastFrame, waitUntilReady, unmount } = render( + + + {(truncatedData) => {truncatedData}} + + , + ); + await waitUntilReady(); + expect(lastFrame()).toContain('Hello World'); + unmount(); + }); + + it('slices string data by characters when very long', async () => { + const veryLongString = 'A'.repeat(25000); + const { lastFrame, waitUntilReady, unmount } = render( + + + {(truncatedData) => {truncatedData.length}} + + , + ); + await waitUntilReady(); + // 20000 characters + 3 for '...' + expect(lastFrame()).toContain('20003'); + unmount(); + }); + + it('slices string data by lines when maxLines is provided', async () => { + const multilineString = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5'; + const { lastFrame, waitUntilReady, unmount } = render( + + + {(truncatedData) => {truncatedData}} + + , + ); + await waitUntilReady(); + // maxLines=3, so it should keep 3-1 = 2 lines + expect(lastFrame()).toContain('Line 1'); + expect(lastFrame()).toContain('Line 2'); + expect(lastFrame()).not.toContain('Line 3'); + expect(lastFrame()).toContain( + '... last 3 lines hidden (Ctrl+O to show) ...', + ); + unmount(); + }); + + it('slices array data when maxLines is provided', async () => { + const dataArray = ['Item 1', 'Item 2', 'Item 3', 'Item 4', 'Item 5']; + const { lastFrame, waitUntilReady, unmount } = render( + + + {(truncatedData) => ( + + {truncatedData.map((item, i) => ( + {item} + ))} + + )} + + , + ); + await waitUntilReady(); + // maxLines=3, so it should keep 3-1 = 2 items + expect(lastFrame()).toContain('Item 1'); + expect(lastFrame()).toContain('Item 2'); + expect(lastFrame()).not.toContain('Item 3'); + expect(lastFrame()).toContain( + '... last 3 lines hidden (Ctrl+O to show) ...', + ); + unmount(); + }); + + it('does not slice when isAlternateBuffer is true', async () => { + const multilineString = 'Line 1\nLine 2\nLine 3\nLine 4\nLine 5'; + const { lastFrame, waitUntilReady, unmount } = render( + + + {(truncatedData) => {truncatedData}} + + , + ); + await waitUntilReady(); + expect(lastFrame()).toContain('Line 5'); + expect(lastFrame()).not.toContain('hidden'); + unmount(); + }); +}); diff --git a/packages/cli/src/ui/components/shared/SlicingMaxSizedBox.tsx b/packages/cli/src/ui/components/shared/SlicingMaxSizedBox.tsx new file mode 100644 index 0000000000..b756c40ee2 --- /dev/null +++ b/packages/cli/src/ui/components/shared/SlicingMaxSizedBox.tsx @@ -0,0 +1,103 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useMemo } from 'react'; +import { MaxSizedBox, type MaxSizedBoxProps } from './MaxSizedBox.js'; + +// Large threshold to ensure we don't cause performance issues for very large +// outputs that will get truncated further MaxSizedBox anyway. +const MAXIMUM_RESULT_DISPLAY_CHARACTERS = 20000; + +export interface SlicingMaxSizedBoxProps + extends Omit { + data: T; + maxLines?: number; + isAlternateBuffer?: boolean; + children: (truncatedData: T) => React.ReactNode; +} + +/** + * An extension of MaxSizedBox that performs explicit slicing of the input data + * (string or array) before rendering. This is useful for performance and to + * ensure consistent truncation behavior for large outputs. + */ +export function SlicingMaxSizedBox({ + data, + maxLines, + isAlternateBuffer, + children, + ...boxProps +}: SlicingMaxSizedBoxProps) { + const { truncatedData, hiddenLinesCount } = useMemo(() => { + let hiddenLines = 0; + const overflowDirection = boxProps.overflowDirection ?? 'top'; + + // Only truncate string output if not in alternate buffer mode to ensure + // we can scroll through the full output. + if (typeof data === 'string' && !isAlternateBuffer) { + let text: string = data as string; + if (text.length > MAXIMUM_RESULT_DISPLAY_CHARACTERS) { + if (overflowDirection === 'bottom') { + text = text.slice(0, MAXIMUM_RESULT_DISPLAY_CHARACTERS) + '...'; + } else { + text = '...' + text.slice(-MAXIMUM_RESULT_DISPLAY_CHARACTERS); + } + } + if (maxLines) { + const hasTrailingNewline = text.endsWith('\n'); + const contentText = hasTrailingNewline ? text.slice(0, -1) : text; + const lines = contentText.split('\n'); + if (lines.length > maxLines) { + // We will have a label from MaxSizedBox. Reserve space for it. + const targetLines = Math.max(1, maxLines - 1); + hiddenLines = lines.length - targetLines; + if (overflowDirection === 'bottom') { + text = + lines.slice(0, targetLines).join('\n') + + (hasTrailingNewline ? '\n' : ''); + } else { + text = + lines.slice(-targetLines).join('\n') + + (hasTrailingNewline ? '\n' : ''); + } + } + } + return { + truncatedData: text, + hiddenLinesCount: hiddenLines, + }; + } + + if (Array.isArray(data) && !isAlternateBuffer && maxLines) { + if (data.length > maxLines) { + // We will have a label from MaxSizedBox. Reserve space for it. + const targetLines = Math.max(1, maxLines - 1); + const hiddenCount = data.length - targetLines; + return { + truncatedData: + overflowDirection === 'bottom' + ? data.slice(0, targetLines) + : data.slice(-targetLines), + hiddenLinesCount: hiddenCount, + }; + } + } + + return { truncatedData: data, hiddenLinesCount: 0 }; + }, [data, isAlternateBuffer, maxLines, boxProps.overflowDirection]); + + return ( + + {/* eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion */} + {children(truncatedData as unknown as T)} + + ); +} diff --git a/packages/cli/src/ui/components/shared/TextInput.tsx b/packages/cli/src/ui/components/shared/TextInput.tsx index 8a4745eea7..cc3fcaeb8d 100644 --- a/packages/cli/src/ui/components/shared/TextInput.tsx +++ b/packages/cli/src/ui/components/shared/TextInput.tsx @@ -14,7 +14,8 @@ import { theme } from '../../semantic-colors.js'; import type { TextBuffer } from './text-buffer.js'; import { expandPastePlaceholders } from './text-buffer.js'; import { cpSlice, cpIndexToOffset } from '../../utils/textUtils.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; export interface TextInputProps { buffer: TextBuffer; @@ -31,6 +32,7 @@ export function TextInput({ onCancel, focus = true, }: TextInputProps): React.JSX.Element { + const keyMatchers = useKeyMatchers(); const { text, handleInput, @@ -55,7 +57,7 @@ export function TextInput({ const handled = handleInput(key); return handled; }, - [handleInput, onCancel, onSubmit, text, buffer.pastedContent], + [handleInput, onCancel, onSubmit, text, buffer.pastedContent, keyMatchers], ); useKeypress(handleKeyPress, { isActive: focus, priority: true }); diff --git a/packages/cli/src/ui/components/shared/text-buffer.ts b/packages/cli/src/ui/components/shared/text-buffer.ts index 34d757a61b..808fc8a554 100644 --- a/packages/cli/src/ui/components/shared/text-buffer.ts +++ b/packages/cli/src/ui/components/shared/text-buffer.ts @@ -25,11 +25,12 @@ import { } from '../../utils/textUtils.js'; import { parsePastedPaths } from '../../utils/clipboardUtils.js'; import type { Key } from '../../contexts/KeypressContext.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; import type { VimAction } from './vim-buffer-actions.js'; import { handleVimAction } from './vim-buffer-actions.js'; import { LRU_BUFFER_PERF_CACHE_LIMIT } from '../../constants.js'; import { openFileInEditor } from '../../utils/editorUtils.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; export const LARGE_PASTE_LINE_THRESHOLD = 5; export const LARGE_PASTE_CHAR_THRESHOLD = 500; @@ -2708,6 +2709,7 @@ export function useTextBuffer({ singleLine = false, getPreferredEditor, }: UseTextBufferProps): TextBuffer { + const keyMatchers = useKeyMatchers(); const initialState = useMemo((): TextBufferState => { const lines = initialText.split('\n'); const [initialCursorRow, initialCursorCol] = calculateInitialCursorPosition( @@ -3270,6 +3272,7 @@ export function useTextBuffer({ text, visualCursor, visualLines, + keyMatchers, ], ); diff --git a/packages/cli/src/ui/components/triage/TriageDuplicates.tsx b/packages/cli/src/ui/components/triage/TriageDuplicates.tsx index 878cacfed0..4de6568189 100644 --- a/packages/cli/src/ui/components/triage/TriageDuplicates.tsx +++ b/packages/cli/src/ui/components/triage/TriageDuplicates.tsx @@ -10,7 +10,8 @@ import Spinner from 'ink-spinner'; import type { Config } from '@google/gemini-cli-core'; import { debugLogger, spawnAsync, LlmRole } from '@google/gemini-cli-core'; import { useKeypress } from '../../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; interface Issue { number: number; @@ -106,6 +107,7 @@ export const TriageDuplicates = ({ onExit: () => void; initialLimit?: number; }) => { + const keyMatchers = useKeyMatchers(); const [state, setState] = useState({ status: 'loading', issues: [], diff --git a/packages/cli/src/ui/components/triage/TriageIssues.tsx b/packages/cli/src/ui/components/triage/TriageIssues.tsx index 595384a124..e6779d6c02 100644 --- a/packages/cli/src/ui/components/triage/TriageIssues.tsx +++ b/packages/cli/src/ui/components/triage/TriageIssues.tsx @@ -10,9 +10,10 @@ import Spinner from 'ink-spinner'; import type { Config } from '@google/gemini-cli-core'; import { debugLogger, spawnAsync, LlmRole } from '@google/gemini-cli-core'; import { useKeypress } from '../../hooks/useKeypress.js'; -import { keyMatchers, Command } from '../../keyMatchers.js'; +import { Command } from '../../keyMatchers.js'; import { TextInput } from '../shared/TextInput.js'; import { useTextBuffer } from '../shared/text-buffer.js'; +import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; interface Issue { number: number; @@ -69,6 +70,7 @@ export const TriageIssues = ({ initialLimit?: number; until?: string; }) => { + const keyMatchers = useKeyMatchers(); const [state, setState] = useState({ status: 'loading', issues: [], diff --git a/packages/cli/src/ui/components/views/McpStatus.test.tsx b/packages/cli/src/ui/components/views/McpStatus.test.tsx index 1c600069c1..e4808f31c4 100644 --- a/packages/cli/src/ui/components/views/McpStatus.test.tsx +++ b/packages/cli/src/ui/components/views/McpStatus.test.tsx @@ -16,7 +16,6 @@ describe('McpStatus', () => { servers: { 'server-1': { url: 'http://localhost:8080', - name: 'server-1', description: 'A test server', }, }, @@ -200,6 +199,38 @@ describe('McpStatus', () => { unmount(); }); + it('renders correctly with both blocked and unblocked servers', async () => { + const { lastFrame, unmount, waitUntilReady } = render( + , + ); + await waitUntilReady(); + expect(lastFrame()).toMatchSnapshot(); + unmount(); + }); + + it('renders only blocked servers when no configured servers exist', async () => { + const { lastFrame, unmount, waitUntilReady } = render( + , + ); + await waitUntilReady(); + expect(lastFrame()).toMatchSnapshot(); + unmount(); + }); + it('renders correctly with a connecting server', async () => { const { lastFrame, unmount, waitUntilReady } = render( , diff --git a/packages/cli/src/ui/components/views/McpStatus.tsx b/packages/cli/src/ui/components/views/McpStatus.tsx index 473c3de547..c007d14635 100644 --- a/packages/cli/src/ui/components/views/McpStatus.tsx +++ b/packages/cli/src/ui/components/views/McpStatus.tsx @@ -48,7 +48,12 @@ export const McpStatus: React.FC = ({ showDescriptions, showSchema, }) => { - const serverNames = Object.keys(servers); + const serverNames = Object.keys(servers).filter( + (serverName) => + !blockedServers.some( + (blockedServer) => blockedServer.name === serverName, + ), + ); if (serverNames.length === 0 && blockedServers.length === 0) { return ( @@ -82,7 +87,6 @@ export const McpStatus: React.FC = ({ Configured MCP servers: - {serverNames.map((serverName) => { const server = servers[serverName]; const serverTools = tools.filter( diff --git a/packages/cli/src/ui/components/views/__snapshots__/McpStatus.test.tsx.snap b/packages/cli/src/ui/components/views/__snapshots__/McpStatus.test.tsx.snap index eb1d1de83c..71a34c5026 100644 --- a/packages/cli/src/ui/components/views/__snapshots__/McpStatus.test.tsx.snap +++ b/packages/cli/src/ui/components/views/__snapshots__/McpStatus.test.tsx.snap @@ -17,12 +17,6 @@ A test server exports[`McpStatus > renders correctly with a blocked server 1`] = ` "Configured MCP servers: -🟢 server-1 - Ready (1 tool) -A test server - Tools: - - tool-1 - A test tool - 🔴 server-1 (from test-extension) - Blocked " `; @@ -83,6 +77,19 @@ A test server " `; +exports[`McpStatus > renders correctly with both blocked and unblocked servers 1`] = ` +"Configured MCP servers: + +🟢 server-1 - Ready (1 tool) +A test server + Tools: + - tool-1 + A test tool + +🔴 server-2 (from test-extension) - Blocked +" +`; + exports[`McpStatus > renders correctly with expired OAuth status 1`] = ` "Configured MCP servers: @@ -172,3 +179,10 @@ A test server A test tool " `; + +exports[`McpStatus > renders only blocked servers when no configured servers exist 1`] = ` +"Configured MCP servers: + +🔴 server-1 (from test-extension) - Blocked +" +`; diff --git a/packages/cli/src/ui/constants.ts b/packages/cli/src/ui/constants.ts index 448dc37523..db52be1105 100644 --- a/packages/cli/src/ui/constants.ts +++ b/packages/cli/src/ui/constants.ts @@ -50,6 +50,9 @@ export const ACTIVE_SHELL_MAX_LINES = 15; // Max lines to preserve in history for completed shell commands export const COMPLETED_SHELL_MAX_LINES = 15; +// Max lines to show for subagent results before collapsing +export const SUBAGENT_MAX_LINES = 15; + /** Minimum terminal width required to show the full context used label */ export const MIN_TERMINAL_WIDTH_FOR_FULL_LABEL = 100; diff --git a/packages/cli/src/ui/hooks/toolMapping.ts b/packages/cli/src/ui/hooks/toolMapping.ts index 1bc6d09903..e06ebf5bb5 100644 --- a/packages/cli/src/ui/hooks/toolMapping.ts +++ b/packages/cli/src/ui/hooks/toolMapping.ts @@ -103,6 +103,7 @@ export function mapToDisplay( ...baseDisplayProperties, status: call.status, isClientInitiated: !!call.request.isClientInitiated, + kind: call.tool?.kind, resultDisplay, confirmationDetails, outputFile, diff --git a/packages/cli/src/ui/hooks/useApprovalModeIndicator.ts b/packages/cli/src/ui/hooks/useApprovalModeIndicator.ts index 1b5076027f..84e465106f 100644 --- a/packages/cli/src/ui/hooks/useApprovalModeIndicator.ts +++ b/packages/cli/src/ui/hooks/useApprovalModeIndicator.ts @@ -11,7 +11,8 @@ import { getAdminErrorMessage, } from '@google/gemini-cli-core'; import { useKeypress } from './useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from './useKeyMatchers.js'; import type { HistoryItemWithoutId } from '../types.js'; import { MessageType } from '../types.js'; @@ -30,6 +31,7 @@ export function useApprovalModeIndicator({ isActive = true, allowPlanMode = false, }: UseApprovalModeIndicatorArgs): ApprovalMode { + const keyMatchers = useKeyMatchers(); const currentConfigValue = config.getApprovalMode(); const [showApprovalMode, setApprovalMode] = useState(currentConfigValue); diff --git a/packages/cli/src/ui/hooks/useExtensionUpdates.test.tsx b/packages/cli/src/ui/hooks/useExtensionUpdates.test.tsx index a558686bd8..95212b023c 100644 --- a/packages/cli/src/ui/hooks/useExtensionUpdates.test.tsx +++ b/packages/cli/src/ui/hooks/useExtensionUpdates.test.tsx @@ -24,7 +24,10 @@ import { } from '../../config/extensions/update.js'; import { ExtensionUpdateState } from '../state/extensions.js'; import { ExtensionManager } from '../../config/extension-manager.js'; -import { loadSettings } from '../../config/settings.js'; +import { + loadSettings, + resetSettingsCacheForTesting, +} from '../../config/settings.js'; vi.mock('os', async (importOriginal) => { const mockedOs = await importOriginal(); @@ -59,6 +62,7 @@ describe('useExtensionUpdates', () => { let extensionManager: ExtensionManager; beforeEach(() => { + resetSettingsCacheForTesting(); vi.mocked(loadAgentsFromDirectory).mockResolvedValue({ agents: [], errors: [], diff --git a/packages/cli/src/ui/hooks/useInlineEditBuffer.test.ts b/packages/cli/src/ui/hooks/useInlineEditBuffer.test.ts new file mode 100644 index 0000000000..b22ee62c81 --- /dev/null +++ b/packages/cli/src/ui/hooks/useInlineEditBuffer.test.ts @@ -0,0 +1,158 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { renderHook } from '../../test-utils/render.js'; +import { act } from 'react'; +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { useInlineEditBuffer } from './useInlineEditBuffer.js'; + +describe('useEditBuffer', () => { + let mockOnCommit: Mock; + + beforeEach(() => { + vi.clearAllMocks(); + mockOnCommit = vi.fn(); + }); + + it('should initialize with empty state', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + expect(result.current.editState.editingKey).toBeNull(); + expect(result.current.editState.buffer).toBe(''); + expect(result.current.editState.cursorPos).toBe(0); + }); + + it('should start editing correctly', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + act(() => result.current.startEditing('my-key', 'initial')); + + expect(result.current.editState.editingKey).toBe('my-key'); + expect(result.current.editState.buffer).toBe('initial'); + expect(result.current.editState.cursorPos).toBe(7); // End of string + }); + + it('should commit edit and reset state', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + + act(() => result.current.startEditing('my-key', 'text')); + act(() => result.current.commitEdit()); + + expect(mockOnCommit).toHaveBeenCalledWith('my-key', 'text'); + expect(result.current.editState.editingKey).toBeNull(); + expect(result.current.editState.buffer).toBe(''); + }); + + it('should move cursor left and right', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + act(() => result.current.startEditing('key', 'ab')); // cursor at 2 + + act(() => result.current.editDispatch({ type: 'MOVE_LEFT' })); + expect(result.current.editState.cursorPos).toBe(1); + + act(() => result.current.editDispatch({ type: 'MOVE_LEFT' })); + expect(result.current.editState.cursorPos).toBe(0); + + // Shouldn't go below 0 + act(() => result.current.editDispatch({ type: 'MOVE_LEFT' })); + expect(result.current.editState.cursorPos).toBe(0); + + act(() => result.current.editDispatch({ type: 'MOVE_RIGHT' })); + expect(result.current.editState.cursorPos).toBe(1); + }); + + it('should handle home and end', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + act(() => result.current.startEditing('key', 'testing')); // cursor at 7 + + act(() => result.current.editDispatch({ type: 'HOME' })); + expect(result.current.editState.cursorPos).toBe(0); + + act(() => result.current.editDispatch({ type: 'END' })); + expect(result.current.editState.cursorPos).toBe(7); + }); + + it('should delete characters to the left (backspace)', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + act(() => result.current.startEditing('key', 'abc')); // cursor at 3 + + act(() => result.current.editDispatch({ type: 'DELETE_LEFT' })); + expect(result.current.editState.buffer).toBe('ab'); + expect(result.current.editState.cursorPos).toBe(2); + + // Move to start, shouldn't delete + act(() => result.current.editDispatch({ type: 'HOME' })); + act(() => result.current.editDispatch({ type: 'DELETE_LEFT' })); + expect(result.current.editState.buffer).toBe('ab'); + }); + + it('should delete characters to the right (delete tab)', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + act(() => result.current.startEditing('key', 'abc')); + act(() => result.current.editDispatch({ type: 'HOME' })); // cursor at 0 + + act(() => result.current.editDispatch({ type: 'DELETE_RIGHT' })); + expect(result.current.editState.buffer).toBe('bc'); + expect(result.current.editState.cursorPos).toBe(0); + }); + + it('should insert valid characters into string', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + act(() => result.current.startEditing('key', 'ab')); + act(() => result.current.editDispatch({ type: 'MOVE_LEFT' })); // cursor at 1 + + act(() => + result.current.editDispatch({ + type: 'INSERT_CHAR', + char: 'x', + isNumberType: false, + }), + ); + expect(result.current.editState.buffer).toBe('axb'); + expect(result.current.editState.cursorPos).toBe(2); + }); + + it('should validate number character insertions', () => { + const { result } = renderHook(() => + useInlineEditBuffer({ onCommit: mockOnCommit }), + ); + act(() => result.current.startEditing('key', '12')); + + // Valid number char + act(() => + result.current.editDispatch({ + type: 'INSERT_CHAR', + char: '.', + isNumberType: true, + }), + ); + expect(result.current.editState.buffer).toBe('12.'); + + // Invalid number char + act(() => + result.current.editDispatch({ + type: 'INSERT_CHAR', + char: 'a', + isNumberType: true, + }), + ); + expect(result.current.editState.buffer).toBe('12.'); // Unchanged + }); +}); diff --git a/packages/cli/src/ui/hooks/useInlineEditBuffer.ts b/packages/cli/src/ui/hooks/useInlineEditBuffer.ts new file mode 100644 index 0000000000..c3dbb05016 --- /dev/null +++ b/packages/cli/src/ui/hooks/useInlineEditBuffer.ts @@ -0,0 +1,152 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useReducer, useCallback, useEffect, useState } from 'react'; +import { cpSlice, cpLen, stripUnsafeCharacters } from '../utils/textUtils.js'; + +export interface EditBufferState { + editingKey: string | null; + buffer: string; + cursorPos: number; +} + +export type EditBufferAction = + | { type: 'START_EDIT'; key: string; initialValue: string } + | { type: 'COMMIT_EDIT' } + | { type: 'MOVE_LEFT' } + | { type: 'MOVE_RIGHT' } + | { type: 'HOME' } + | { type: 'END' } + | { type: 'DELETE_LEFT' } + | { type: 'DELETE_RIGHT' } + | { type: 'INSERT_CHAR'; char: string; isNumberType: boolean }; + +const initialState: EditBufferState = { + editingKey: null, + buffer: '', + cursorPos: 0, +}; + +function editBufferReducer( + state: EditBufferState, + action: EditBufferAction, +): EditBufferState { + switch (action.type) { + case 'START_EDIT': + return { + editingKey: action.key, + buffer: action.initialValue, + cursorPos: cpLen(action.initialValue), + }; + + case 'COMMIT_EDIT': + return initialState; + + case 'MOVE_LEFT': + return { + ...state, + cursorPos: Math.max(0, state.cursorPos - 1), + }; + + case 'MOVE_RIGHT': + return { + ...state, + cursorPos: Math.min(cpLen(state.buffer), state.cursorPos + 1), + }; + + case 'HOME': + return { ...state, cursorPos: 0 }; + + case 'END': + return { ...state, cursorPos: cpLen(state.buffer) }; + + case 'DELETE_LEFT': { + if (state.cursorPos === 0) return state; + const before = cpSlice(state.buffer, 0, state.cursorPos - 1); + const after = cpSlice(state.buffer, state.cursorPos); + return { + ...state, + buffer: before + after, + cursorPos: state.cursorPos - 1, + }; + } + + case 'DELETE_RIGHT': { + if (state.cursorPos === cpLen(state.buffer)) return state; + const before = cpSlice(state.buffer, 0, state.cursorPos); + const after = cpSlice(state.buffer, state.cursorPos + 1); + return { + ...state, + buffer: before + after, + }; + } + + case 'INSERT_CHAR': { + let ch = action.char; + let isValidChar = false; + + if (action.isNumberType) { + isValidChar = /[0-9\-+.]/.test(ch); + } else { + isValidChar = ch.length === 1 && ch.charCodeAt(0) >= 32; + ch = stripUnsafeCharacters(ch); + } + + if (!isValidChar || ch.length === 0) return state; + + const before = cpSlice(state.buffer, 0, state.cursorPos); + const after = cpSlice(state.buffer, state.cursorPos); + return { + ...state, + buffer: before + ch + after, + cursorPos: state.cursorPos + 1, + }; + } + + default: + return state; + } +} + +export interface UseEditBufferProps { + onCommit: (key: string, value: string) => void; +} + +export function useInlineEditBuffer({ onCommit }: UseEditBufferProps) { + const [state, dispatch] = useReducer(editBufferReducer, initialState); + const [cursorVisible, setCursorVisible] = useState(true); + + useEffect(() => { + if (!state.editingKey) { + setCursorVisible(true); + return; + } + setCursorVisible(true); + const interval = setInterval(() => { + setCursorVisible((v) => !v); + }, 500); + return () => clearInterval(interval); + }, [state.editingKey, state.buffer, state.cursorPos]); + + const startEditing = useCallback((key: string, initialValue: string) => { + dispatch({ type: 'START_EDIT', key, initialValue }); + }, []); + + const commitEdit = useCallback(() => { + if (state.editingKey) { + onCommit(state.editingKey, state.buffer); + } + dispatch({ type: 'COMMIT_EDIT' }); + }, [state.editingKey, state.buffer, onCommit]); + + return { + editState: state, + editDispatch: dispatch, + startEditing, + commitEdit, + cursorVisible, + }; +} diff --git a/packages/cli/src/ui/hooks/useKeyMatchers.ts b/packages/cli/src/ui/hooks/useKeyMatchers.ts new file mode 100644 index 0000000000..a42a066ee0 --- /dev/null +++ b/packages/cli/src/ui/hooks/useKeyMatchers.ts @@ -0,0 +1,17 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useMemo } from 'react'; +import type { KeyMatchers } from '../keyMatchers.js'; +import { defaultKeyMatchers } from '../keyMatchers.js'; + +/** + * Hook to retrieve the currently active key matchers. + * This prepares the codebase for dynamic or custom key bindings in the future. + */ +export function useKeyMatchers(): KeyMatchers { + return useMemo(() => defaultKeyMatchers, []); +} diff --git a/packages/cli/src/ui/hooks/useSelectionList.ts b/packages/cli/src/ui/hooks/useSelectionList.ts index f74c1b1dc2..9f73c54da4 100644 --- a/packages/cli/src/ui/hooks/useSelectionList.ts +++ b/packages/cli/src/ui/hooks/useSelectionList.ts @@ -6,8 +6,9 @@ import { useReducer, useRef, useEffect, useCallback } from 'react'; import { useKeypress, type Key } from './useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import { debugLogger } from '@google/gemini-cli-core'; +import { useKeyMatchers } from './useKeyMatchers.js'; export interface SelectionListItem { key: string; @@ -290,6 +291,7 @@ export function useSelectionList({ focusKey, priority, }: UseSelectionListOptions): UseSelectionListResult { + const keyMatchers = useKeyMatchers(); const baseItems = toBaseItems(items); const [state, dispatch] = useReducer(selectionListReducer, { @@ -460,7 +462,7 @@ export function useSelectionList({ } return false; }, - [dispatch, itemsLength, showNumbers], + [dispatch, itemsLength, showNumbers, keyMatchers], ); useKeypress(handleKeypress, { diff --git a/packages/cli/src/ui/hooks/useSettingsNavigation.test.ts b/packages/cli/src/ui/hooks/useSettingsNavigation.test.ts new file mode 100644 index 0000000000..5a64119f40 --- /dev/null +++ b/packages/cli/src/ui/hooks/useSettingsNavigation.test.ts @@ -0,0 +1,121 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { renderHook } from '../../test-utils/render.js'; +import { act } from 'react'; +import { describe, it, expect } from 'vitest'; +import { useSettingsNavigation } from './useSettingsNavigation.js'; + +describe('useSettingsNavigation', () => { + const mockItems = [ + { key: 'a' }, + { key: 'b' }, + { key: 'c' }, + { key: 'd' }, + { key: 'e' }, + ]; + + it('should initialize with the first item active', () => { + const { result } = renderHook(() => + useSettingsNavigation({ items: mockItems, maxItemsToShow: 3 }), + ); + expect(result.current.activeIndex).toBe(0); + expect(result.current.activeItemKey).toBe('a'); + expect(result.current.windowStart).toBe(0); + }); + + it('should move down correctly', () => { + const { result } = renderHook(() => + useSettingsNavigation({ items: mockItems, maxItemsToShow: 3 }), + ); + act(() => result.current.moveDown()); + expect(result.current.activeIndex).toBe(1); + expect(result.current.activeItemKey).toBe('b'); + }); + + it('should move up correctly', () => { + const { result } = renderHook(() => + useSettingsNavigation({ items: mockItems, maxItemsToShow: 3 }), + ); + act(() => result.current.moveDown()); // to index 1 + act(() => result.current.moveUp()); // back to 0 + expect(result.current.activeIndex).toBe(0); + }); + + it('should wrap around from top to bottom', () => { + const { result } = renderHook(() => + useSettingsNavigation({ items: mockItems, maxItemsToShow: 3 }), + ); + act(() => result.current.moveUp()); + expect(result.current.activeIndex).toBe(4); + expect(result.current.activeItemKey).toBe('e'); + }); + + it('should wrap around from bottom to top', () => { + const { result } = renderHook(() => + useSettingsNavigation({ items: mockItems, maxItemsToShow: 3 }), + ); + // Move to last item + // Move to last item (index 4) + act(() => result.current.moveDown()); // 1 + act(() => result.current.moveDown()); // 2 + act(() => result.current.moveDown()); // 3 + act(() => result.current.moveDown()); // 4 + expect(result.current.activeIndex).toBe(4); + + // Move down once more + act(() => result.current.moveDown()); + expect(result.current.activeIndex).toBe(0); + }); + + it('should adjust scrollOffset when moving down past visible area', () => { + const { result } = renderHook(() => + useSettingsNavigation({ items: mockItems, maxItemsToShow: 3 }), + ); + + act(() => result.current.moveDown()); // index 1 + act(() => result.current.moveDown()); // index 2, still offset 0 + expect(result.current.windowStart).toBe(0); + + act(() => result.current.moveDown()); // index 3, offset should be 1 + expect(result.current.windowStart).toBe(1); + }); + + it('should adjust scrollOffset when moving up past visible area', () => { + const { result } = renderHook(() => + useSettingsNavigation({ items: mockItems, maxItemsToShow: 3 }), + ); + + act(() => result.current.moveDown()); // 1 + act(() => result.current.moveDown()); // 2 + act(() => result.current.moveDown()); // 3 + expect(result.current.windowStart).toBe(1); + + act(() => result.current.moveUp()); // index 2 + act(() => result.current.moveUp()); // index 1, offset should become 1 + act(() => result.current.moveUp()); // index 0, offset should become 0 + expect(result.current.windowStart).toBe(0); + }); + + it('should handle item preservation when list filters (Part 1 logic)', () => { + let items = mockItems; + const { result, rerender } = renderHook( + ({ list }) => useSettingsNavigation({ items: list, maxItemsToShow: 3 }), + { initialProps: { list: items } }, + ); + + act(() => result.current.moveDown()); + act(() => result.current.moveDown()); // Item 'c' + expect(result.current.activeItemKey).toBe('c'); + + // Filter items but keep 'c' + items = [mockItems[0], mockItems[2], mockItems[4]]; // 'a', 'c', 'e' + rerender({ list: items }); + + expect(result.current.activeItemKey).toBe('c'); + expect(result.current.activeIndex).toBe(1); // 'c' is now at index 1 + }); +}); diff --git a/packages/cli/src/ui/hooks/useSettingsNavigation.ts b/packages/cli/src/ui/hooks/useSettingsNavigation.ts new file mode 100644 index 0000000000..1f47b2eb74 --- /dev/null +++ b/packages/cli/src/ui/hooks/useSettingsNavigation.ts @@ -0,0 +1,124 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useMemo, useReducer, useCallback } from 'react'; + +export interface UseSettingsNavigationProps { + items: Array<{ key: string }>; + maxItemsToShow: number; +} + +type NavState = { + activeItemKey: string | null; + windowStart: number; +}; + +type NavAction = { type: 'MOVE_UP' } | { type: 'MOVE_DOWN' }; + +function calculateSlidingWindow( + start: number, + activeIndex: number, + itemCount: number, + windowSize: number, +): number { + // User moves up above the window start + if (activeIndex < start) { + start = activeIndex; + // User moves down below the window end + } else if (activeIndex >= start + windowSize) { + start = activeIndex - windowSize + 1; + } + // User is inside the window but performed search or terminal resized + const maxScroll = Math.max(0, itemCount - windowSize); + const bounded = Math.min(start, maxScroll); + return Math.max(0, bounded); +} + +function createNavReducer( + items: Array<{ key: string }>, + maxItemsToShow: number, +) { + return function navReducer(state: NavState, action: NavAction): NavState { + if (items.length === 0) return state; + + const currentIndex = items.findIndex((i) => i.key === state.activeItemKey); + const activeIndex = currentIndex !== -1 ? currentIndex : 0; + + switch (action.type) { + case 'MOVE_UP': { + const newIndex = activeIndex > 0 ? activeIndex - 1 : items.length - 1; + return { + activeItemKey: items[newIndex].key, + windowStart: calculateSlidingWindow( + state.windowStart, + newIndex, + items.length, + maxItemsToShow, + ), + }; + } + case 'MOVE_DOWN': { + const newIndex = activeIndex < items.length - 1 ? activeIndex + 1 : 0; + return { + activeItemKey: items[newIndex].key, + windowStart: calculateSlidingWindow( + state.windowStart, + newIndex, + items.length, + maxItemsToShow, + ), + }; + } + default: { + return state; + } + } + }; +} + +export function useSettingsNavigation({ + items, + maxItemsToShow, +}: UseSettingsNavigationProps) { + const reducer = useMemo( + () => createNavReducer(items, maxItemsToShow), + [items, maxItemsToShow], + ); + + const [state, dispatch] = useReducer(reducer, { + activeItemKey: items[0]?.key ?? null, + windowStart: 0, + }); + + // Retain the proper highlighting when items change (e.g. search) + const activeIndex = useMemo(() => { + if (items.length === 0) return 0; + const idx = items.findIndex((i) => i.key === state.activeItemKey); + return idx !== -1 ? idx : 0; + }, [items, state.activeItemKey]); + + const windowStart = useMemo( + () => + calculateSlidingWindow( + state.windowStart, + activeIndex, + items.length, + maxItemsToShow, + ), + [state.windowStart, activeIndex, items.length, maxItemsToShow], + ); + + const moveUp = useCallback(() => dispatch({ type: 'MOVE_UP' }), []); + const moveDown = useCallback(() => dispatch({ type: 'MOVE_DOWN' }), []); + + return { + activeItemKey: state.activeItemKey, + activeIndex, + windowStart, + moveUp, + moveDown, + }; +} diff --git a/packages/cli/src/ui/hooks/useTabbedNavigation.test.ts b/packages/cli/src/ui/hooks/useTabbedNavigation.test.ts index 5eb1107a4d..e41a89d66d 100644 --- a/packages/cli/src/ui/hooks/useTabbedNavigation.test.ts +++ b/packages/cli/src/ui/hooks/useTabbedNavigation.test.ts @@ -9,12 +9,18 @@ import { act } from 'react'; import { renderHook } from '../../test-utils/render.js'; import { useTabbedNavigation } from './useTabbedNavigation.js'; import { useKeypress } from './useKeypress.js'; +import { useKeyMatchers } from './useKeyMatchers.js'; +import type { KeyMatchers } from '../keyMatchers.js'; import type { Key, KeypressHandler } from '../contexts/KeypressContext.js'; vi.mock('./useKeypress.js', () => ({ useKeypress: vi.fn(), })); +vi.mock('./useKeyMatchers.js', () => ({ + useKeyMatchers: vi.fn(), +})); + const createKey = (partial: Partial): Key => ({ name: partial.name || '', sequence: partial.sequence || '', @@ -26,13 +32,14 @@ const createKey = (partial: Partial): Key => ({ ...partial, }); +const mockKeyMatchers = { + 'cursor.left': vi.fn((key) => key.name === 'left'), + 'cursor.right': vi.fn((key) => key.name === 'right'), + 'dialog.next': vi.fn((key) => key.name === 'tab' && !key.shift), + 'dialog.previous': vi.fn((key) => key.name === 'tab' && key.shift), +} as unknown as KeyMatchers; + vi.mock('../keyMatchers.js', () => ({ - keyMatchers: { - 'cursor.left': vi.fn((key) => key.name === 'left'), - 'cursor.right': vi.fn((key) => key.name === 'right'), - 'dialog.next': vi.fn((key) => key.name === 'tab' && !key.shift), - 'dialog.previous': vi.fn((key) => key.name === 'tab' && key.shift), - }, Command: { MOVE_LEFT: 'cursor.left', MOVE_RIGHT: 'cursor.right', @@ -45,6 +52,7 @@ describe('useTabbedNavigation', () => { let capturedHandler: KeypressHandler; beforeEach(() => { + vi.mocked(useKeyMatchers).mockReturnValue(mockKeyMatchers); vi.mocked(useKeypress).mockImplementation((handler) => { capturedHandler = handler; }); diff --git a/packages/cli/src/ui/hooks/useTabbedNavigation.ts b/packages/cli/src/ui/hooks/useTabbedNavigation.ts index b4ed73264c..d7e406ce6b 100644 --- a/packages/cli/src/ui/hooks/useTabbedNavigation.ts +++ b/packages/cli/src/ui/hooks/useTabbedNavigation.ts @@ -6,7 +6,8 @@ import { useReducer, useCallback, useEffect, useRef } from 'react'; import { useKeypress, type Key } from './useKeypress.js'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from './useKeyMatchers.js'; /** * Options for the useTabbedNavigation hook. @@ -147,6 +148,7 @@ export function useTabbedNavigation({ isActive = true, onTabChange, }: UseTabbedNavigationOptions): UseTabbedNavigationResult { + const keyMatchers = useKeyMatchers(); const [state, dispatch] = useReducer(tabbedNavigationReducer, { currentIndex: Math.max(0, Math.min(initialIndex, tabCount - 1)), tabCount, @@ -231,6 +233,7 @@ export function useTabbedNavigation({ goToNextTab, goToPrevTab, isNavigationBlocked, + keyMatchers, ], ); diff --git a/packages/cli/src/ui/hooks/vim.ts b/packages/cli/src/ui/hooks/vim.ts index 9de771564c..1fcc0c61ca 100644 --- a/packages/cli/src/ui/hooks/vim.ts +++ b/packages/cli/src/ui/hooks/vim.ts @@ -9,7 +9,8 @@ import type { Key } from './useKeypress.js'; import type { TextBuffer } from '../components/shared/text-buffer.js'; import { useVimMode } from '../contexts/VimModeContext.js'; import { debugLogger } from '@google/gemini-cli-core'; -import { keyMatchers, Command } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; +import { useKeyMatchers } from './useKeyMatchers.js'; export type VimMode = 'NORMAL' | 'INSERT'; @@ -152,6 +153,7 @@ const vimReducer = (state: VimState, action: VimAction): VimState => { * @returns Object with vim state and input handler */ export function useVim(buffer: TextBuffer, onSubmit?: (value: string) => void) { + const keyMatchers = useKeyMatchers(); const { vimEnabled, vimMode, setVimMode } = useVimMode(); const [state, dispatch] = useReducer(vimReducer, initialVimState); @@ -439,7 +441,7 @@ export function useVim(buffer: TextBuffer, onSubmit?: (value: string) => void) { return buffer.handleInput(normalizedKey); }, - [buffer, dispatch, updateMode, onSubmit, checkDoubleEscape], + [buffer, dispatch, updateMode, onSubmit, checkDoubleEscape, keyMatchers], ); /** @@ -1202,6 +1204,7 @@ export function useVim(buffer: TextBuffer, onSubmit?: (value: string) => void) { executeCommand, updateMode, checkDoubleEscape, + keyMatchers, ], ); diff --git a/packages/cli/src/ui/keyMatchers.test.ts b/packages/cli/src/ui/keyMatchers.test.ts index 888393be83..e90f6334be 100644 --- a/packages/cli/src/ui/keyMatchers.test.ts +++ b/packages/cli/src/ui/keyMatchers.test.ts @@ -5,7 +5,11 @@ */ import { describe, it, expect } from 'vitest'; -import { keyMatchers, Command, createKeyMatchers } from './keyMatchers.js'; +import { + defaultKeyMatchers, + Command, + createKeyMatchers, +} from './keyMatchers.js'; import type { KeyBindingConfig } from '../config/keyBindings.js'; import { defaultKeyBindings } from '../config/keyBindings.js'; import type { Key } from './hooks/useKeypress.js'; @@ -422,14 +426,14 @@ describe('keyMatchers', () => { it(`should match ${command} correctly`, () => { positive.forEach((key) => { expect( - keyMatchers[command](key), + defaultKeyMatchers[command](key), `Expected ${command} to match ${JSON.stringify(key)}`, ).toBe(true); }); negative.forEach((key) => { expect( - keyMatchers[command](key), + defaultKeyMatchers[command](key), `Expected ${command} to NOT match ${JSON.stringify(key)}`, ).toBe(false); }); diff --git a/packages/cli/src/ui/keyMatchers.ts b/packages/cli/src/ui/keyMatchers.ts index f833e5ee09..259f1edd9e 100644 --- a/packages/cli/src/ui/keyMatchers.ts +++ b/packages/cli/src/ui/keyMatchers.ts @@ -68,7 +68,8 @@ export function createKeyMatchers( /** * Default key binding matchers using the default configuration */ -export const keyMatchers: KeyMatchers = createKeyMatchers(defaultKeyBindings); +export const defaultKeyMatchers: KeyMatchers = + createKeyMatchers(defaultKeyBindings); // Re-export Command for convenience export { Command }; diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index c9910179a5..3898461fb0 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -15,6 +15,7 @@ import { type SkillDefinition, type AgentDefinition, type ApprovalMode, + type Kind, CoreToolCallStatus, checkExhaustive, } from '@google/gemini-cli-core'; @@ -105,6 +106,7 @@ export interface IndividualToolCallDisplay { status: CoreToolCallStatus; // True when the tool was initiated directly by the user (slash/@/shell flows). isClientInitiated?: boolean; + kind?: Kind; confirmationDetails: SerializableConfirmationDetails | undefined; renderOutputAsMarkdown?: boolean; ptyId?: number; diff --git a/packages/cli/src/ui/utils/MarkdownDisplay.tsx b/packages/cli/src/ui/utils/MarkdownDisplay.tsx index 0200fbcb00..b3e88d9a01 100644 --- a/packages/cli/src/ui/utils/MarkdownDisplay.tsx +++ b/packages/cli/src/ui/utils/MarkdownDisplay.tsx @@ -414,6 +414,7 @@ const RenderListItemInternal: React.FC = ({ }) => { const prefix = type === 'ol' ? `${marker}. ` : `${marker} `; const prefixWidth = prefix.length; + // Account for leading whitespace (indentation level) plus the standard prefix padding const indentation = leadingWhitespace.length; const listResponseColor = theme.text.response ?? theme.text.primary; @@ -422,7 +423,7 @@ const RenderListItemInternal: React.FC = ({ paddingLeft={indentation + LIST_ITEM_PREFIX_PADDING} flexDirection="row" > - + {prefix} diff --git a/packages/cli/src/ui/utils/historyExportUtils.ts b/packages/cli/src/ui/utils/historyExportUtils.ts index 85a53dd330..325c880b2b 100644 --- a/packages/cli/src/ui/utils/historyExportUtils.ts +++ b/packages/cli/src/ui/utils/historyExportUtils.ts @@ -11,7 +11,9 @@ import type { Content } from '@google/genai'; /** * Serializes chat history to a Markdown string. */ -export function serializeHistoryToMarkdown(history: Content[]): string { +export function serializeHistoryToMarkdown( + history: readonly Content[], +): string { return history .map((item) => { const text = @@ -49,7 +51,7 @@ export function serializeHistoryToMarkdown(history: Content[]): string { * Options for exporting chat history. */ export interface ExportHistoryOptions { - history: Content[]; + history: readonly Content[]; filePath: string; } diff --git a/packages/cli/src/ui/utils/shortcutsHelp.ts b/packages/cli/src/ui/utils/shortcutsHelp.ts index 65ab8f2a13..a5f6d22e19 100644 --- a/packages/cli/src/ui/utils/shortcutsHelp.ts +++ b/packages/cli/src/ui/utils/shortcutsHelp.ts @@ -4,9 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { Command, keyMatchers } from '../keyMatchers.js'; +import { Command } from '../keyMatchers.js'; import type { Key } from '../hooks/useKeypress.js'; +import { useKeyMatchers } from '../hooks/useKeyMatchers.js'; -export function shouldDismissShortcutsHelpOnHotkey(key: Key): boolean { - return Object.values(Command).some((command) => keyMatchers[command](key)); +export function useIsHelpDismissKey(): (key: Key) => boolean { + const keyMatchers = useKeyMatchers(); + return (key: Key) => + Object.values(Command).some((command) => keyMatchers[command](key)); } diff --git a/packages/cli/src/ui/utils/toolLayoutUtils.ts b/packages/cli/src/ui/utils/toolLayoutUtils.ts index 6ba1b85c5e..c91919cffa 100644 --- a/packages/cli/src/ui/utils/toolLayoutUtils.ts +++ b/packages/cli/src/ui/utils/toolLayoutUtils.ts @@ -53,7 +53,7 @@ export function calculateToolContentMaxLines(options: { ) : undefined; - if (maxLinesLimit) { + if (maxLinesLimit !== undefined) { contentHeight = contentHeight !== undefined ? Math.min(contentHeight, maxLinesLimit) diff --git a/packages/cli/src/utils/gitUtils.ts b/packages/cli/src/utils/gitUtils.ts index e27673f0fe..83d89ad164 100644 --- a/packages/cli/src/utils/gitUtils.ts +++ b/packages/cli/src/utils/gitUtils.ts @@ -61,6 +61,7 @@ export const getLatestGitHubRelease = async ( const endpoint = `https://api.github.com/repos/google-github-actions/run-gemini-cli/releases/latest`; + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(endpoint, { method: 'GET', headers: { diff --git a/packages/core/package.json b/packages/core/package.json index 827c09bc61..8861046d01 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -21,11 +21,13 @@ "dist" ], "dependencies": { - "@a2a-js/sdk": "^0.3.8", + "@a2a-js/sdk": "^0.3.10", + "@bufbuild/protobuf": "^2.11.0", "@google-cloud/logging": "^11.2.1", "@google-cloud/opentelemetry-cloud-monitoring-exporter": "^0.21.0", "@google-cloud/opentelemetry-cloud-trace-exporter": "^3.0.0", "@google/genai": "1.41.0", + "@grpc/grpc-js": "^1.14.3", "@iarna/toml": "^2.2.5", "@joshua.litt/get-ripgrep": "^0.0.3", "@modelcontextprotocol/sdk": "^1.23.0", @@ -63,6 +65,7 @@ "html-to-text": "^9.0.5", "https-proxy-agent": "^7.0.6", "ignore": "^7.0.0", + "ipaddr.js": "^1.9.1", "js-yaml": "^4.1.1", "marked": "^15.0.12", "mime": "4.0.7", diff --git a/packages/core/src/agents/a2a-client-manager.ts b/packages/core/src/agents/a2a-client-manager.ts index e7070f3dfa..3d203d462d 100644 --- a/packages/core/src/agents/a2a-client-manager.ts +++ b/packages/core/src/agents/a2a-client-manager.ts @@ -25,6 +25,7 @@ import { import { v4 as uuidv4 } from 'uuid'; import { Agent as UndiciAgent } from 'undici'; import { debugLogger } from '../utils/debugLogger.js'; +import { safeLookup } from '../utils/fetch.js'; // Remote agents can take 10+ minutes (e.g. Deep Research). // Use a dedicated dispatcher so the global 5-min timeout isn't affected. @@ -32,10 +33,13 @@ const A2A_TIMEOUT = 1800000; // 30 minutes const a2aDispatcher = new UndiciAgent({ headersTimeout: A2A_TIMEOUT, bodyTimeout: A2A_TIMEOUT, + connect: { + lookup: safeLookup, // SSRF protection at connection level + }, }); const a2aFetch: typeof fetch = (input, init) => - // @ts-expect-error The `dispatcher` property is a Node.js extension to fetch not present in standard types. - fetch(input, { ...init, dispatcher: a2aDispatcher }); + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection + fetch(input, { ...init, dispatcher: a2aDispatcher } as RequestInit); export type SendMessageResult = | Message diff --git a/packages/core/src/agents/agent-scheduler.test.ts b/packages/core/src/agents/agent-scheduler.test.ts index dd6749d3a0..451fb276a2 100644 --- a/packages/core/src/agents/agent-scheduler.test.ts +++ b/packages/core/src/agents/agent-scheduler.test.ts @@ -27,6 +27,7 @@ describe('agent-scheduler', () => { mockMessageBus = {} as Mocked; mockToolRegistry = { getTool: vi.fn(), + getMessageBus: vi.fn().mockReturnValue(mockMessageBus), } as unknown as Mocked; mockConfig = { getMessageBus: vi.fn().mockReturnValue(mockMessageBus), diff --git a/packages/core/src/agents/agent-scheduler.ts b/packages/core/src/agents/agent-scheduler.ts index ecb4ed960a..983f814b0f 100644 --- a/packages/core/src/agents/agent-scheduler.ts +++ b/packages/core/src/agents/agent-scheduler.ts @@ -57,10 +57,11 @@ export async function scheduleAgentTools( // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment const agentConfig: Config = Object.create(config); agentConfig.getToolRegistry = () => toolRegistry; + agentConfig.getMessageBus = () => toolRegistry.getMessageBus(); const scheduler = new Scheduler({ config: agentConfig, - messageBus: config.getMessageBus(), + messageBus: toolRegistry.getMessageBus(), getPreferredEditor: getPreferredEditor ?? (() => undefined), schedulerId, parentCallId, diff --git a/packages/core/src/agents/browser/browserAgentInvocation.ts b/packages/core/src/agents/browser/browserAgentInvocation.ts index 3bdb4fa2d5..a2ae2b9c9b 100644 --- a/packages/core/src/agents/browser/browserAgentInvocation.ts +++ b/packages/core/src/agents/browser/browserAgentInvocation.ts @@ -17,6 +17,7 @@ import { randomUUID } from 'node:crypto'; import type { Config } from '../../config/config.js'; import { LocalAgentExecutor } from '../local-executor.js'; +import { safeJsonToMarkdown } from '../../utils/markdownUtils.js'; import { BaseToolInvocation, type ToolResult, @@ -414,6 +415,8 @@ export class BrowserAgentInvocation extends BaseToolInvocation< const output = await executor.run(this.params, signal); + const displayResult = safeJsonToMarkdown(output.result); + const resultContent = `Browser agent finished. Termination Reason: ${output.terminate_reason} Result: @@ -425,7 +428,7 @@ Browser Agent Finished Termination Reason: ${output.terminate_reason} Result: -${output.result} +${displayResult} `; if (updateOutput) { diff --git a/packages/core/src/agents/local-executor.ts b/packages/core/src/agents/local-executor.ts index fd450c5efa..dd5b78a9a6 100644 --- a/packages/core/src/agents/local-executor.ts +++ b/packages/core/src/agents/local-executor.ts @@ -19,6 +19,7 @@ import { ToolRegistry } from '../tools/tool-registry.js'; import { DiscoveredMCPTool } from '../tools/mcp-tool.js'; import { CompressionStatus } from '../core/turn.js'; import { type ToolCallRequestInfo } from '../scheduler/types.js'; +import { type Message } from '../confirmation-bus/types.js'; import { ChatCompressionService } from '../services/chatCompressionService.js'; import { getDirectoryContextString } from '../utils/environmentContext.js'; import { promptIdContext } from '../utils/promptIdContext.js'; @@ -113,10 +114,27 @@ export class LocalAgentExecutor { runtimeContext: Config, onActivity?: ActivityCallback, ): Promise> { + const parentMessageBus = runtimeContext.getMessageBus(); + + // Create an override object to inject the subagent name into tool confirmation requests + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + const subagentMessageBus = Object.create( + parentMessageBus, + ) as typeof parentMessageBus; + subagentMessageBus.publish = async (message: Message) => { + if (message.type === 'tool-confirmation-request') { + return parentMessageBus.publish({ + ...message, + subagent: definition.name, + }); + } + return parentMessageBus.publish(message); + }; + // Create an isolated tool registry for this agent instance. const agentToolRegistry = new ToolRegistry( runtimeContext, - runtimeContext.getMessageBus(), + subagentMessageBus, ); const parentToolRegistry = runtimeContext.getToolRegistry(); const allAgentNames = new Set( diff --git a/packages/core/src/agents/local-invocation.ts b/packages/core/src/agents/local-invocation.ts index 4bd2bc171a..02bfb4efe0 100644 --- a/packages/core/src/agents/local-invocation.ts +++ b/packages/core/src/agents/local-invocation.ts @@ -6,6 +6,7 @@ import type { Config } from '../config/config.js'; import { LocalAgentExecutor } from './local-executor.js'; +import { safeJsonToMarkdown } from '../utils/markdownUtils.js'; import { BaseToolInvocation, type ToolResult, @@ -245,6 +246,8 @@ export class LocalSubagentInvocation extends BaseToolInvocation< throw cancelError; } + const displayResult = safeJsonToMarkdown(output.result); + const resultContent = `Subagent '${this.definition.name}' finished. Termination Reason: ${output.terminate_reason} Result: @@ -256,7 +259,7 @@ Subagent ${this.definition.name} Finished Termination Reason:\n ${output.terminate_reason} Result: -${output.result} +${displayResult} `; return { diff --git a/packages/core/src/agents/remote-invocation.ts b/packages/core/src/agents/remote-invocation.ts index a8c75ec51c..40dd142638 100644 --- a/packages/core/src/agents/remote-invocation.ts +++ b/packages/core/src/agents/remote-invocation.ts @@ -25,6 +25,7 @@ import { extractIdsFromResponse, A2AResultReassembler } from './a2aUtils.js'; import { GoogleAuth } from 'google-auth-library'; import type { AuthenticationHandler } from '@a2a-js/sdk/client'; import { debugLogger } from '../utils/debugLogger.js'; +import { safeJsonToMarkdown } from '../utils/markdownUtils.js'; import type { AnsiOutput } from '../utils/terminalSerializer.js'; import { A2AAuthProviderFactory } from './auth-provider/factory.js'; @@ -222,7 +223,7 @@ export class RemoteAgentInvocation extends BaseToolInvocation< return { llmContent: [{ text: finalOutput }], - returnDisplay: finalOutput, + returnDisplay: safeJsonToMarkdown(finalOutput), }; } catch (error: unknown) { const partialOutput = reassembler.toString(); diff --git a/packages/core/src/code_assist/oauth2.ts b/packages/core/src/code_assist/oauth2.ts index e238a4a860..654ba0e10a 100644 --- a/packages/core/src/code_assist/oauth2.ts +++ b/packages/core/src/code_assist/oauth2.ts @@ -700,6 +700,7 @@ async function fetchAndCacheUserInfo(client: OAuth2Client): Promise { return; } + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch( 'https://www.googleapis.com/oauth2/v2/userinfo', { diff --git a/packages/core/src/commands/types.ts b/packages/core/src/commands/types.ts index 31491a27be..d9cc7a24e9 100644 --- a/packages/core/src/commands/types.ts +++ b/packages/core/src/commands/types.ts @@ -31,7 +31,7 @@ export interface MessageActionReturn { export interface LoadHistoryActionReturn { type: 'load_history'; history: HistoryType; - clientHistory: Content[]; // The history for the generative client + clientHistory: readonly Content[]; // The history for the generative client } /** diff --git a/packages/core/src/confirmation-bus/message-bus.test.ts b/packages/core/src/confirmation-bus/message-bus.test.ts index 8342d53b1b..34e36167a9 100644 --- a/packages/core/src/confirmation-bus/message-bus.test.ts +++ b/packages/core/src/confirmation-bus/message-bus.test.ts @@ -160,6 +160,7 @@ describe('MessageBus', () => { { name: 'test-tool', args: {} }, 'test-server', annotations, + undefined, ); }); diff --git a/packages/core/src/confirmation-bus/message-bus.ts b/packages/core/src/confirmation-bus/message-bus.ts index 3dd61995ab..33aa10355b 100644 --- a/packages/core/src/confirmation-bus/message-bus.ts +++ b/packages/core/src/confirmation-bus/message-bus.ts @@ -56,6 +56,7 @@ export class MessageBus extends EventEmitter { message.toolCall, message.serverName, message.toolAnnotations, + message.subagent, ); switch (decision) { diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index aefafe0fa0..277c821da3 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -38,6 +38,10 @@ export interface ToolConfirmationRequest { * Optional tool annotations (e.g., readOnlyHint, destructiveHint) from MCP. */ toolAnnotations?: Record; + /** + * Optional subagent name, if this tool call was initiated by a subagent. + */ + subagent?: string; /** * Optional rich details for the confirmation UI (diffs, counts, etc.) */ diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index db6c5fb574..bbef15d491 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -255,7 +255,7 @@ export class GeminiClient { return this.chat !== undefined; } - getHistory(): Content[] { + getHistory(): readonly Content[] { return this.getChat().getHistory(); } @@ -263,7 +263,7 @@ export class GeminiClient { this.getChat().stripThoughtsFromHistory(); } - setHistory(history: Content[]) { + setHistory(history: readonly Content[]) { this.getChat().setHistory(history); this.updateTelemetryTokenCount(); this.forceFullIdeContext = true; @@ -1171,7 +1171,7 @@ export class GeminiClient { /** * Masks bulky tool outputs to save context window space. */ - private async tryMaskToolOutputs(history: Content[]): Promise { + private async tryMaskToolOutputs(history: readonly Content[]): Promise { if (!this.config.getToolOutputMaskingEnabled()) { return; } diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index ae5f46db37..1c0f1a5685 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -470,7 +470,7 @@ export class GeminiChat { private async makeApiCallAndProcessStream( modelConfigKey: ModelConfigKey, - requestContents: Content[], + requestContents: readonly Content[], prompt_id: string, abortSignal: AbortSignal, role: LlmRole, @@ -489,7 +489,7 @@ export class GeminiChat { let currentGenerateContentConfig: GenerateContentConfig = newAvailabilityConfig; let lastConfig: GenerateContentConfig = currentGenerateContentConfig; - let lastContentsToUse: Content[] = requestContents; + let lastContentsToUse: Content[] = [...requestContents]; const getAvailabilityContext = createAvailabilityContextProvider( this.config, @@ -528,9 +528,9 @@ export class GeminiChat { abortSignal, }; - let contentsToUse = supportsModernFeatures(modelToUse) - ? contentsForPreviewModel - : requestContents; + let contentsToUse: Content[] = supportsModernFeatures(modelToUse) + ? [...contentsForPreviewModel] + : [...requestContents]; const hookSystem = this.config.getHookSystem(); if (hookSystem) { @@ -687,16 +687,10 @@ export class GeminiChat { * @return History contents alternating between user and model for the entire * chat session. */ - getHistory(curated: boolean = false): Content[] { + getHistory(curated: boolean = false): readonly Content[] { const history = curated ? extractCuratedHistory(this.history) : this.history; - // Return a shallow copy of the array to prevent callers from mutating - // the internal history array (push/pop/splice). Content objects are - // shared references — callers MUST NOT mutate them in place. - // This replaces a prior structuredClone() which deep-copied the entire - // conversation on every call, causing O(n) memory pressure per turn - // that compounded into OOM crashes in long-running sessions. return [...history]; } @@ -714,8 +708,8 @@ export class GeminiChat { this.history.push(content); } - setHistory(history: Content[]): void { - this.history = history; + setHistory(history: readonly Content[]): void { + this.history = [...history]; this.lastPromptTokenCount = estimateTokenCountSync( this.history.flatMap((c) => c.parts || []), ); @@ -742,7 +736,9 @@ export class GeminiChat { // To ensure our requests validate, the first function call in every model // turn within the active loop must have a `thoughtSignature` property. // If we do not do this, we will get back 400 errors from the API. - ensureActiveLoopHasThoughtSignatures(requestContents: Content[]): Content[] { + ensureActiveLoopHasThoughtSignatures( + requestContents: readonly Content[], + ): readonly Content[] { // First, find the start of the active loop by finding the last user turn // with a text message, i.e. that is not a function response. let activeLoopStartIndex = -1; diff --git a/packages/core/src/core/logger.ts b/packages/core/src/core/logger.ts index 362601f895..c75d4d7ffa 100644 --- a/packages/core/src/core/logger.ts +++ b/packages/core/src/core/logger.ts @@ -27,7 +27,7 @@ export interface LogEntry { } export interface Checkpoint { - history: Content[]; + history: readonly Content[]; authType?: AuthType; } diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index 6aaafa6054..01934d9019 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -111,6 +111,7 @@ export class MCPOAuthProvider { scope: config.scopes?.join(' ') || '', }; + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(registrationUrl, { method: 'POST', headers: { @@ -300,6 +301,7 @@ export class MCPOAuthProvider { ? { Accept: 'text/event-stream' } : { Accept: 'application/json' }; + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(mcpServerUrl, { method: 'HEAD', headers, diff --git a/packages/core/src/mcp/oauth-utils.ts b/packages/core/src/mcp/oauth-utils.ts index 320c3b9685..207b694181 100644 --- a/packages/core/src/mcp/oauth-utils.ts +++ b/packages/core/src/mcp/oauth-utils.ts @@ -97,6 +97,7 @@ export class OAuthUtils { resourceMetadataUrl: string, ): Promise { try { + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(resourceMetadataUrl); if (!response.ok) { return null; @@ -121,6 +122,7 @@ export class OAuthUtils { authServerMetadataUrl: string, ): Promise { try { + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(authServerMetadataUrl); if (!response.ok) { return null; diff --git a/packages/core/src/policy/policy-engine.ts b/packages/core/src/policy/policy-engine.ts index 0d6a043da0..a2f64bf356 100644 --- a/packages/core/src/policy/policy-engine.ts +++ b/packages/core/src/policy/policy-engine.ts @@ -74,6 +74,7 @@ function ruleMatches( serverName: string | undefined, currentApprovalMode: ApprovalMode, toolAnnotations?: Record, + subagent?: string, ): boolean { // Check if rule applies to current approval mode if (rule.modes && rule.modes.length > 0) { @@ -82,6 +83,13 @@ function ruleMatches( } } + // Check subagent if specified (only for PolicyRule, SafetyCheckerRule doesn't have it) + if ('subagent' in rule && rule.subagent) { + if (rule.subagent !== subagent) { + return false; + } + } + // Strictly enforce mcpName identity if the rule dictates it if (rule.mcpName) { if (rule.mcpName === '*') { @@ -203,6 +211,7 @@ export class PolicyEngine { allowRedirection?: boolean, rule?: PolicyRule, toolAnnotations?: Record, + subagent?: string, ): Promise { if (!command) { return { @@ -294,6 +303,7 @@ export class PolicyEngine { { name: toolName, args: { command: subCmd, dir_path } }, serverName, toolAnnotations, + subagent, ); // subResult.decision is already filtered through applyNonInteractiveMode by this.check() @@ -352,6 +362,7 @@ export class PolicyEngine { toolCall: FunctionCall, serverName: string | undefined, toolAnnotations?: Record, + subagent?: string, ): Promise { // Case 1: Metadata injection is the primary and safest way to identify an MCP server. // If we have explicit `_serverName` metadata (usually injected by tool-registry for active tools), use it. @@ -419,6 +430,7 @@ export class PolicyEngine { serverName, this.approvalMode, toolAnnotations, + subagent, ), ); @@ -437,6 +449,7 @@ export class PolicyEngine { rule.allowRedirection, rule, toolAnnotations, + subagent, ); decision = shellResult.decision; if (shellResult.rule) { @@ -463,9 +476,10 @@ export class PolicyEngine { this.defaultDecision, serverName, shellDirPath, - undefined, + false, undefined, toolAnnotations, + subagent, ); decision = shellResult.decision; matchedRule = shellResult.rule; @@ -485,6 +499,7 @@ export class PolicyEngine { serverName, this.approvalMode, toolAnnotations, + subagent, ) ) { debugLogger.debug( diff --git a/packages/core/src/policy/toml-loader.ts b/packages/core/src/policy/toml-loader.ts index c91930a21d..83dda26e9e 100644 --- a/packages/core/src/policy/toml-loader.ts +++ b/packages/core/src/policy/toml-loader.ts @@ -38,6 +38,7 @@ const MAX_TYPO_DISTANCE = 3; */ const PolicyRuleSchema = z.object({ toolName: z.union([z.string(), z.array(z.string())]).optional(), + subagent: z.string().optional(), mcpName: z.string().optional(), argsPattern: z.string().optional(), commandPrefix: z.union([z.string(), z.array(z.string())]).optional(), @@ -464,6 +465,7 @@ export async function loadPoliciesFromToml( const policyRule: PolicyRule = { toolName: effectiveToolName, + subagent: rule.subagent, mcpName: rule.mcpName, decision: rule.decision, priority: transformPriority(rule.priority, tier), diff --git a/packages/core/src/policy/types.ts b/packages/core/src/policy/types.ts index f59821b093..53a0433a15 100644 --- a/packages/core/src/policy/types.ts +++ b/packages/core/src/policy/types.ts @@ -110,6 +110,12 @@ export interface PolicyRule { */ toolName?: string; + /** + * The name of the subagent this rule applies to. + * If undefined, the rule applies regardless of whether it's the main agent or a subagent. + */ + subagent?: string; + /** * Identifies the MCP server this rule applies to. * Enables precise rule matching against `serverName` metadata instead diff --git a/packages/core/src/routing/routingStrategy.ts b/packages/core/src/routing/routingStrategy.ts index a2f9448989..a2aaf8c14f 100644 --- a/packages/core/src/routing/routingStrategy.ts +++ b/packages/core/src/routing/routingStrategy.ts @@ -31,7 +31,7 @@ export interface RoutingDecision { */ export interface RoutingContext { /** The full history of the conversation. */ - history: Content[]; + history: readonly Content[]; /** The immediate request parts to be processed. */ request: PartListUnion; /** An abort signal to cancel an LLM call during routing. */ diff --git a/packages/core/src/safety/context-builder.ts b/packages/core/src/safety/context-builder.ts index c7b33f5e2f..f73cae6e42 100644 --- a/packages/core/src/safety/context-builder.ts +++ b/packages/core/src/safety/context-builder.ts @@ -74,7 +74,9 @@ export class ContextBuilder { } // Helper to convert Google GenAI Content[] to Safety Protocol ConversationTurn[] - private convertHistoryToTurns(history: Content[]): ConversationTurn[] { + private convertHistoryToTurns( + history: readonly Content[], + ): ConversationTurn[] { const turns: ConversationTurn[] = []; let currentUserRequest: { text: string } | undefined; diff --git a/packages/core/src/services/chatCompressionService.ts b/packages/core/src/services/chatCompressionService.ts index 8dceb18f4b..a1f9c12f2c 100644 --- a/packages/core/src/services/chatCompressionService.ts +++ b/packages/core/src/services/chatCompressionService.ts @@ -130,7 +130,7 @@ export function modelStringToModelConfigAlias(model: string): string { * contain massive tool outputs (like large grep results or logs). */ async function truncateHistoryToBudget( - history: Content[], + history: readonly Content[], config: Config, ): Promise { let functionResponseTokenCounter = 0; diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index cd8d1e53c1..6dd24fd42a 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -664,7 +664,7 @@ export class ChatRecordingService { * Updates the conversation history based on the provided API Content array. * This is used to persist changes made to the history (like masking) back to disk. */ - updateMessagesFromHistory(history: Content[]): void { + updateMessagesFromHistory(history: readonly Content[]): void { if (!this.conversationFile) return; try { diff --git a/packages/core/src/services/keychainService.ts b/packages/core/src/services/keychainService.ts index ed28218c11..a43890f89b 100644 --- a/packages/core/src/services/keychainService.ts +++ b/packages/core/src/services/keychainService.ts @@ -13,6 +13,7 @@ import { KeychainSchema, KEYCHAIN_TEST_PREFIX, } from './keychainTypes.js'; +import { isRecord } from '../utils/markdownUtils.js'; /** * Service for interacting with OS-level secure storage (e.g. keytar). @@ -111,7 +112,7 @@ export class KeychainService { private async loadKeychainModule(): Promise { const moduleName = 'keytar'; const module: unknown = await import(moduleName); - const potential = (this.isRecord(module) && module['default']) || module; + const potential = (isRecord(module) && module['default']) || module; const result = KeychainSchema.safeParse(potential); if (result.success) { @@ -126,10 +127,6 @@ export class KeychainService { return null; } - private isRecord(obj: unknown): obj is Record { - return typeof obj === 'object' && obj !== null; - } - // Performs a set-get-delete cycle to verify keychain functionality. private async isKeychainFunctional(keychain: Keychain): Promise { const testAccount = `${KEYCHAIN_TEST_PREFIX}${crypto.randomBytes(8).toString('hex')}`; diff --git a/packages/core/src/services/toolOutputMaskingService.ts b/packages/core/src/services/toolOutputMaskingService.ts index 8a7ae0090d..9d5a3fb2c2 100644 --- a/packages/core/src/services/toolOutputMaskingService.ts +++ b/packages/core/src/services/toolOutputMaskingService.ts @@ -43,7 +43,7 @@ const EXEMPT_TOOLS = new Set([ ]); export interface MaskingResult { - newHistory: Content[]; + newHistory: readonly Content[]; maskedCount: number; tokensSaved: number; } @@ -67,7 +67,10 @@ export interface MaskingResult { * are preserved until they collectively reach the threshold. */ export class ToolOutputMaskingService { - async mask(history: Content[], config: Config): Promise { + async mask( + history: readonly Content[], + config: Config, + ): Promise { const maskingConfig = await config.getToolOutputMaskingConfig(); if (!maskingConfig.enabled || history.length === 0) { return { newHistory: history, maskedCount: 0, tokensSaved: 0 }; diff --git a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts index 51c5ab382f..310622aea4 100644 --- a/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts +++ b/packages/core/src/telemetry/clearcut-logger/clearcut-logger.ts @@ -474,6 +474,7 @@ export class ClearcutLogger { let result: LogResponse = {}; try { + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(CLEARCUT_URL, { method: 'POST', body: safeJsonStringify(request), diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index af55facaa3..7932e35f38 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -104,6 +104,10 @@ export enum MCPServerStatus { CONNECTING = 'connecting', /** Server is connected and ready to use */ CONNECTED = 'connected', + /** Server is blocked via configuration and cannot be used */ + BLOCKED = 'blocked', + /** Server is disabled and cannot be used */ + DISABLED = 'disabled', } /** @@ -1899,6 +1903,7 @@ export async function connectToMcpServer( acceptHeader = 'application/json'; } + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(urlToFetch, { method: 'HEAD', headers: { diff --git a/packages/core/src/utils/checkpointUtils.ts b/packages/core/src/utils/checkpointUtils.ts index 2252fdf70b..4e1989efbd 100644 --- a/packages/core/src/utils/checkpointUtils.ts +++ b/packages/core/src/utils/checkpointUtils.ts @@ -14,7 +14,7 @@ import type { ToolCallRequestInfo } from '../scheduler/types.js'; export interface ToolCallData { history?: HistoryType; - clientHistory?: Content[]; + clientHistory?: readonly Content[]; commitHash?: string; toolCall: { name: string; diff --git a/packages/core/src/utils/fetch.test.ts b/packages/core/src/utils/fetch.test.ts new file mode 100644 index 0000000000..3eddefaf3d --- /dev/null +++ b/packages/core/src/utils/fetch.test.ts @@ -0,0 +1,291 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest'; +import { + isPrivateIp, + isPrivateIpAsync, + isAddressPrivate, + safeLookup, + safeFetch, + fetchWithTimeout, + PrivateIpError, +} from './fetch.js'; +import * as dnsPromises from 'node:dns/promises'; +import * as dns from 'node:dns'; + +vi.mock('node:dns/promises', () => ({ + lookup: vi.fn(), +})); + +// We need to mock node:dns for safeLookup since it uses the callback API +vi.mock('node:dns', () => ({ + lookup: vi.fn(), +})); + +// Mock global fetch +const originalFetch = global.fetch; +global.fetch = vi.fn(); + +describe('fetch utils', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterAll(() => { + global.fetch = originalFetch; + }); + + describe('isAddressPrivate', () => { + it('should identify private IPv4 addresses', () => { + expect(isAddressPrivate('10.0.0.1')).toBe(true); + expect(isAddressPrivate('127.0.0.1')).toBe(true); + expect(isAddressPrivate('172.16.0.1')).toBe(true); + expect(isAddressPrivate('192.168.1.1')).toBe(true); + }); + + it('should identify non-routable and reserved IPv4 addresses (RFC 6890)', () => { + expect(isAddressPrivate('0.0.0.0')).toBe(true); + expect(isAddressPrivate('100.64.0.1')).toBe(true); + expect(isAddressPrivate('192.0.0.1')).toBe(true); + expect(isAddressPrivate('192.0.2.1')).toBe(true); + expect(isAddressPrivate('192.88.99.1')).toBe(true); + // Benchmark range (198.18.0.0/15) + expect(isAddressPrivate('198.18.0.0')).toBe(true); + expect(isAddressPrivate('198.18.0.1')).toBe(true); + expect(isAddressPrivate('198.19.255.255')).toBe(true); + expect(isAddressPrivate('198.51.100.1')).toBe(true); + expect(isAddressPrivate('203.0.113.1')).toBe(true); + expect(isAddressPrivate('224.0.0.1')).toBe(true); + expect(isAddressPrivate('240.0.0.1')).toBe(true); + }); + + it('should identify private IPv6 addresses', () => { + expect(isAddressPrivate('::1')).toBe(true); + expect(isAddressPrivate('fc00::')).toBe(true); + expect(isAddressPrivate('fd00::')).toBe(true); + expect(isAddressPrivate('fe80::')).toBe(true); + expect(isAddressPrivate('febf::')).toBe(true); + }); + + it('should identify special local addresses', () => { + expect(isAddressPrivate('0.0.0.0')).toBe(true); + expect(isAddressPrivate('::')).toBe(true); + expect(isAddressPrivate('localhost')).toBe(true); + }); + + it('should identify link-local addresses', () => { + expect(isAddressPrivate('169.254.169.254')).toBe(true); + }); + + it('should identify IPv4-mapped IPv6 private addresses', () => { + expect(isAddressPrivate('::ffff:127.0.0.1')).toBe(true); + expect(isAddressPrivate('::ffff:10.0.0.1')).toBe(true); + expect(isAddressPrivate('::ffff:169.254.169.254')).toBe(true); + expect(isAddressPrivate('::ffff:192.168.1.1')).toBe(true); + expect(isAddressPrivate('::ffff:172.16.0.1')).toBe(true); + expect(isAddressPrivate('::ffff:0.0.0.0')).toBe(true); + expect(isAddressPrivate('::ffff:100.64.0.1')).toBe(true); + expect(isAddressPrivate('::ffff:a9fe:101')).toBe(true); // 169.254.1.1 + }); + + it('should identify public addresses as non-private', () => { + expect(isAddressPrivate('8.8.8.8')).toBe(false); + expect(isAddressPrivate('93.184.216.34')).toBe(false); + expect(isAddressPrivate('2001:4860:4860::8888')).toBe(false); + expect(isAddressPrivate('::ffff:8.8.8.8')).toBe(false); + }); + }); + + describe('isPrivateIp', () => { + it('should identify private IPs in URLs', () => { + expect(isPrivateIp('http://10.0.0.1/')).toBe(true); + expect(isPrivateIp('https://127.0.0.1:8080/')).toBe(true); + expect(isPrivateIp('http://localhost/')).toBe(true); + expect(isPrivateIp('http://[::1]/')).toBe(true); + }); + + it('should identify public IPs in URLs as non-private', () => { + expect(isPrivateIp('http://8.8.8.8/')).toBe(false); + expect(isPrivateIp('https://google.com/')).toBe(false); + }); + }); + + describe('isPrivateIpAsync', () => { + it('should identify private IPs directly', async () => { + expect(await isPrivateIpAsync('http://10.0.0.1/')).toBe(true); + }); + + it('should identify domains resolving to private IPs', async () => { + vi.mocked(dnsPromises.lookup).mockImplementation( + async () => + // eslint-disable-next-line @typescript-eslint/no-explicit-any + [{ address: '10.0.0.1', family: 4 }] as any, + ); + expect(await isPrivateIpAsync('http://malicious.com/')).toBe(true); + }); + + it('should identify domains resolving to public IPs as non-private', async () => { + vi.mocked(dnsPromises.lookup).mockImplementation( + async () => + // eslint-disable-next-line @typescript-eslint/no-explicit-any + [{ address: '8.8.8.8', family: 4 }] as any, + ); + expect(await isPrivateIpAsync('http://google.com/')).toBe(false); + }); + + it('should throw error if DNS resolution fails (fail closed)', async () => { + vi.mocked(dnsPromises.lookup).mockRejectedValue(new Error('DNS Error')); + await expect(isPrivateIpAsync('http://unreachable.com/')).rejects.toThrow( + 'Failed to verify if URL resolves to private IP', + ); + }); + + it('should return false for invalid URLs instead of throwing verification error', async () => { + expect(await isPrivateIpAsync('not-a-url')).toBe(false); + }); + }); + + describe('safeLookup', () => { + it('should filter out private IPs', async () => { + const addresses = [ + { address: '8.8.8.8', family: 4 }, + { address: '10.0.0.1', family: 4 }, + ]; + + vi.mocked(dns.lookup).mockImplementation((( + _h: string, + _o: dns.LookupOptions, + cb: ( + err: Error | null, + addr: Array<{ address: string; family: number }>, + ) => void, + ) => { + cb(null, addresses); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + }) as any); + + const result = await new Promise< + Array<{ address: string; family: number }> + >((resolve, reject) => { + safeLookup('example.com', { all: true }, (err, filtered) => { + if (err) reject(err); + else resolve(filtered); + }); + }); + + expect(result).toHaveLength(1); + expect(result[0].address).toBe('8.8.8.8'); + }); + + it('should allow explicit localhost', async () => { + const addresses = [{ address: '127.0.0.1', family: 4 }]; + + vi.mocked(dns.lookup).mockImplementation((( + _h: string, + _o: dns.LookupOptions, + cb: ( + err: Error | null, + addr: Array<{ address: string; family: number }>, + ) => void, + ) => { + cb(null, addresses); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + }) as any); + + const result = await new Promise< + Array<{ address: string; family: number }> + >((resolve, reject) => { + safeLookup('localhost', { all: true }, (err, filtered) => { + if (err) reject(err); + else resolve(filtered); + }); + }); + + expect(result).toHaveLength(1); + expect(result[0].address).toBe('127.0.0.1'); + }); + + it('should error if all resolved IPs are private', async () => { + const addresses = [{ address: '10.0.0.1', family: 4 }]; + + vi.mocked(dns.lookup).mockImplementation((( + _h: string, + _o: dns.LookupOptions, + cb: ( + err: Error | null, + addr: Array<{ address: string; family: number }>, + ) => void, + ) => { + cb(null, addresses); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + }) as any); + + await expect( + new Promise((resolve, reject) => { + safeLookup('malicious.com', { all: true }, (err, filtered) => { + if (err) reject(err); + else resolve(filtered); + }); + }), + ).rejects.toThrow(PrivateIpError); + }); + }); + + describe('safeFetch', () => { + it('should forward to fetch with dispatcher', async () => { + vi.mocked(global.fetch).mockResolvedValue(new Response('ok')); + + const response = await safeFetch('https://example.com'); + expect(response.status).toBe(200); + expect(global.fetch).toHaveBeenCalledWith( + 'https://example.com', + expect.objectContaining({ + dispatcher: expect.any(Object), + }), + ); + }); + + it('should handle Refusing to connect errors', async () => { + vi.mocked(global.fetch).mockRejectedValue(new PrivateIpError()); + + await expect(safeFetch('http://10.0.0.1')).rejects.toThrow( + 'Access to private network is blocked', + ); + }); + }); + + describe('fetchWithTimeout', () => { + it('should handle timeouts', async () => { + vi.mocked(global.fetch).mockImplementation( + (_input, init) => + new Promise((_resolve, reject) => { + if (init?.signal) { + init.signal.addEventListener('abort', () => { + const error = new Error('The operation was aborted'); + error.name = 'AbortError'; + // @ts-expect-error - for mocking purposes + error.code = 'ABORT_ERR'; + reject(error); + }); + } + }), + ); + + await expect(fetchWithTimeout('http://example.com', 50)).rejects.toThrow( + 'Request timed out after 50ms', + ); + }); + + it('should handle private IP errors via handleFetchError', async () => { + vi.mocked(global.fetch).mockRejectedValue(new PrivateIpError()); + + await expect(fetchWithTimeout('http://10.0.0.1', 1000)).rejects.toThrow( + 'Access to private network is blocked: http://10.0.0.1', + ); + }); + }); +}); diff --git a/packages/core/src/utils/fetch.ts b/packages/core/src/utils/fetch.ts index b3df053614..a324172d94 100644 --- a/packages/core/src/utils/fetch.ts +++ b/packages/core/src/utils/fetch.ts @@ -6,7 +6,10 @@ import { getErrorMessage, isNodeError } from './errors.js'; import { URL } from 'node:url'; +import * as dns from 'node:dns'; +import { lookup } from 'node:dns/promises'; import { Agent, ProxyAgent, setGlobalDispatcher } from 'undici'; +import ipaddr from 'ipaddr.js'; const DEFAULT_HEADERS_TIMEOUT = 300000; // 5 minutes const DEFAULT_BODY_TIMEOUT = 300000; // 5 minutes @@ -19,15 +22,20 @@ setGlobalDispatcher( }), ); -const PRIVATE_IP_RANGES = [ - /^10\./, - /^127\./, - /^172\.(1[6-9]|2[0-9]|3[0-1])\./, - /^192\.168\./, - /^::1$/, - /^fc00:/, - /^fe80:/, -]; +// Local extension of RequestInit to support Node.js/undici dispatcher +interface NodeFetchInit extends RequestInit { + dispatcher?: Agent | ProxyAgent; +} + +/** + * Error thrown when a connection to a private IP address is blocked for security reasons. + */ +export class PrivateIpError extends Error { + constructor(message = 'Refusing to connect to private IP address') { + super(message); + this.name = 'PrivateIpError'; + } +} export class FetchError extends Error { constructor( @@ -40,15 +48,234 @@ export class FetchError extends Error { } } +/** + * Sanitizes a hostname by stripping IPv6 brackets if present. + */ +export function sanitizeHostname(hostname: string): string { + return hostname.startsWith('[') && hostname.endsWith(']') + ? hostname.slice(1, -1) + : hostname; +} + +/** + * Checks if a hostname is a local loopback address allowed for development/testing. + */ +export function isLoopbackHost(hostname: string): boolean { + const sanitized = sanitizeHostname(hostname); + return ( + sanitized === 'localhost' || + sanitized === '127.0.0.1' || + sanitized === '::1' + ); +} + +/** + * A custom DNS lookup implementation for undici agents that prevents + * connection to private IP ranges (SSRF protection). + */ +export function safeLookup( + hostname: string, + options: dns.LookupOptions | number | null | undefined, + callback: ( + err: Error | null, + addresses: Array<{ address: string; family: number }>, + ) => void, +): void { + // Use the callback-based dns.lookup to match undici's expected signature. + // We explicitly handle the 'all' option to ensure we get an array of addresses. + const lookupOptions = + typeof options === 'number' ? { family: options } : { ...options }; + const finalOptions = { ...lookupOptions, all: true }; + + dns.lookup(hostname, finalOptions, (err, addresses) => { + if (err) { + callback(err, []); + return; + } + + const addressArray = Array.isArray(addresses) ? addresses : []; + const filtered = addressArray.filter( + (addr) => !isAddressPrivate(addr.address) || isLoopbackHost(hostname), + ); + + if (filtered.length === 0 && addressArray.length > 0) { + callback(new PrivateIpError(), []); + return; + } + + callback(null, filtered); + }); +} + +// Dedicated dispatcher with connection-level SSRF protection (safeLookup) +const safeDispatcher = new Agent({ + headersTimeout: DEFAULT_HEADERS_TIMEOUT, + bodyTimeout: DEFAULT_BODY_TIMEOUT, + connect: { + lookup: safeLookup, + }, +}); + export function isPrivateIp(url: string): boolean { try { const hostname = new URL(url).hostname; - return PRIVATE_IP_RANGES.some((range) => range.test(hostname)); - } catch (_e) { + return isAddressPrivate(hostname); + } catch { return false; } } +/** + * Checks if a URL resolves to a private IP address. + * Performs DNS resolution to prevent DNS rebinding/SSRF bypasses. + */ +export async function isPrivateIpAsync(url: string): Promise { + try { + const parsed = new URL(url); + const hostname = parsed.hostname; + + // Fast check for literal IPs or localhost + if (isAddressPrivate(hostname)) { + return true; + } + + // Resolve DNS to check the actual target IP + const addresses = await lookup(hostname, { all: true }); + return addresses.some((addr) => isAddressPrivate(addr.address)); + } catch (e) { + if ( + e instanceof Error && + e.name === 'TypeError' && + e.message.includes('Invalid URL') + ) { + return false; + } + throw new Error(`Failed to verify if URL resolves to private IP: ${url}`, { + cause: e, + }); + } +} + +/** + * IANA Benchmark Testing Range (198.18.0.0/15). + * Classified as 'unicast' by ipaddr.js but is reserved and should not be + * accessible as public internet. + */ +const IANA_BENCHMARK_RANGE = ipaddr.parseCIDR('198.18.0.0/15'); + +/** + * Checks if an address falls within the IANA benchmark testing range. + */ +function isBenchmarkAddress(addr: ipaddr.IPv4 | ipaddr.IPv6): boolean { + const [rangeAddr, rangeMask] = IANA_BENCHMARK_RANGE; + return ( + addr instanceof ipaddr.IPv4 && + rangeAddr instanceof ipaddr.IPv4 && + addr.match(rangeAddr, rangeMask) + ); +} + +/** + * Internal helper to check if an IP address string is in a private or reserved range. + */ +export function isAddressPrivate(address: string): boolean { + const sanitized = sanitizeHostname(address); + + if (sanitized === 'localhost') { + return true; + } + + try { + if (!ipaddr.isValid(sanitized)) { + return false; + } + + const addr = ipaddr.parse(sanitized); + + // Special handling for IPv4-mapped IPv6 (::ffff:x.x.x.x) + // We unmap it and check the underlying IPv4 address. + if (addr instanceof ipaddr.IPv6 && addr.isIPv4MappedAddress()) { + return isAddressPrivate(addr.toIPv4Address().toString()); + } + + // Explicitly block IANA benchmark testing range. + if (isBenchmarkAddress(addr)) { + return true; + } + + return addr.range() !== 'unicast'; + } catch { + // If parsing fails despite isValid(), we treat it as potentially unsafe. + return true; + } +} + +/** + * Internal helper to map varied fetch errors to a standardized FetchError. + * Centralizes security-related error mapping (e.g. PrivateIpError). + */ +function handleFetchError(error: unknown, url: string): never { + if (error instanceof PrivateIpError) { + throw new FetchError( + `Access to private network is blocked: ${url}`, + 'ERR_PRIVATE_NETWORK', + { cause: error }, + ); + } + + if (error instanceof FetchError) { + throw error; + } + + throw new FetchError( + getErrorMessage(error), + isNodeError(error) ? error.code : undefined, + { cause: error }, + ); +} + +/** + * Enhanced fetch with SSRF protection. + * Prevents access to private/internal networks at the connection level. + */ +export async function safeFetch( + input: RequestInfo | URL, + init?: RequestInit, +): Promise { + const nodeInit: NodeFetchInit = { + ...init, + dispatcher: safeDispatcher, + }; + + try { + // eslint-disable-next-line no-restricted-syntax + return await fetch(input, nodeInit); + } catch (error) { + const url = + input instanceof Request + ? input.url + : typeof input === 'string' + ? input + : input.toString(); + handleFetchError(error, url); + } +} + +/** + * Creates an undici ProxyAgent that incorporates safe DNS lookup. + */ +export function createSafeProxyAgent(proxyUrl: string): ProxyAgent { + return new ProxyAgent({ + uri: proxyUrl, + connect: { + lookup: safeLookup, + }, + }); +} + +/** + * Performs a fetch with a specified timeout and connection-level SSRF protection. + */ export async function fetchWithTimeout( url: string, timeout: number, @@ -67,17 +294,21 @@ export async function fetchWithTimeout( } } + const nodeInit: NodeFetchInit = { + ...options, + signal: controller.signal, + dispatcher: safeDispatcher, + }; + try { - const response = await fetch(url, { - ...options, - signal: controller.signal, - }); + // eslint-disable-next-line no-restricted-syntax + const response = await fetch(url, nodeInit); return response; } catch (error) { if (isNodeError(error) && error.code === 'ABORT_ERR') { throw new FetchError(`Request timed out after ${timeout}ms`, 'ETIMEDOUT'); } - throw new FetchError(getErrorMessage(error), undefined, { cause: error }); + handleFetchError(error, url.toString()); } finally { clearTimeout(timeoutId); } diff --git a/packages/core/src/utils/markdownUtils.test.ts b/packages/core/src/utils/markdownUtils.test.ts new file mode 100644 index 0000000000..246198c1d2 --- /dev/null +++ b/packages/core/src/utils/markdownUtils.test.ts @@ -0,0 +1,128 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { jsonToMarkdown, safeJsonToMarkdown } from './markdownUtils.js'; + +describe('markdownUtils', () => { + describe('jsonToMarkdown', () => { + it('should handle primitives', () => { + expect(jsonToMarkdown('hello')).toBe('hello'); + expect(jsonToMarkdown(123)).toBe('123'); + expect(jsonToMarkdown(true)).toBe('true'); + expect(jsonToMarkdown(null)).toBe('null'); + expect(jsonToMarkdown(undefined)).toBe('undefined'); + }); + + it('should handle simple arrays', () => { + const data = ['a', 'b', 'c']; + expect(jsonToMarkdown(data)).toBe('- a\n- b\n- c'); + }); + + it('should handle simple objects and convert camelCase to Space Case', () => { + const data = { userName: 'Alice', userAge: 30 }; + expect(jsonToMarkdown(data)).toBe( + '- **User Name**: Alice\n- **User Age**: 30', + ); + }); + + it('should handle empty structures', () => { + expect(jsonToMarkdown([])).toBe('[]'); + expect(jsonToMarkdown({})).toBe('{}'); + }); + + it('should handle nested structures with proper indentation', () => { + const data = { + userInfo: { + fullName: 'Bob Smith', + userRoles: ['admin', 'user'], + }, + isActive: true, + }; + const result = jsonToMarkdown(data); + expect(result).toBe( + '- **User Info**:\n' + + ' - **Full Name**: Bob Smith\n' + + ' - **User Roles**:\n' + + ' - admin\n' + + ' - user\n' + + '- **Is Active**: true', + ); + }); + + it('should render tables for arrays of similar objects with Space Case keys', () => { + const data = [ + { userId: 1, userName: 'Item 1' }, + { userId: 2, userName: 'Item 2' }, + ]; + const result = jsonToMarkdown(data); + expect(result).toBe( + '| User Id | User Name |\n| --- | --- |\n| 1 | Item 1 |\n| 2 | Item 2 |', + ); + }); + + it('should handle pipe characters, backslashes, and newlines in table data', () => { + const data = [ + { colInfo: 'val|ue', otherInfo: 'line\nbreak', pathInfo: 'C:\\test' }, + ]; + const result = jsonToMarkdown(data); + expect(result).toBe( + '| Col Info | Other Info | Path Info |\n| --- | --- | --- |\n| val\\|ue | line break | C:\\\\test |', + ); + }); + + it('should fallback to lists for arrays with mixed objects', () => { + const data = [ + { userId: 1, userName: 'Item 1' }, + { userId: 2, somethingElse: 'Item 2' }, + ]; + const result = jsonToMarkdown(data); + expect(result).toContain('- **User Id**: 1'); + expect(result).toContain('- **Something Else**: Item 2'); + }); + + it('should properly indent nested tables', () => { + const data = { + items: [ + { id: 1, name: 'A' }, + { id: 2, name: 'B' }, + ], + }; + const result = jsonToMarkdown(data); + const lines = result.split('\n'); + expect(lines[0]).toBe('- **Items**:'); + expect(lines[1]).toBe(' | Id | Name |'); + expect(lines[2]).toBe(' | --- | --- |'); + expect(lines[3]).toBe(' | 1 | A |'); + expect(lines[4]).toBe(' | 2 | B |'); + }); + + it('should indent subsequent lines of multiline strings', () => { + const data = { + description: 'Line 1\nLine 2\nLine 3', + }; + const result = jsonToMarkdown(data); + expect(result).toBe('- **Description**: Line 1\n Line 2\n Line 3'); + }); + }); + + describe('safeJsonToMarkdown', () => { + it('should convert valid JSON', () => { + const json = JSON.stringify({ keyName: 'value' }); + expect(safeJsonToMarkdown(json)).toBe('- **Key Name**: value'); + }); + + it('should return original string for invalid JSON', () => { + const notJson = 'Not a JSON string'; + expect(safeJsonToMarkdown(notJson)).toBe(notJson); + }); + + it('should handle plain strings that look like numbers or booleans but are valid JSON', () => { + expect(safeJsonToMarkdown('123')).toBe('123'); + expect(safeJsonToMarkdown('true')).toBe('true'); + }); + }); +}); diff --git a/packages/core/src/utils/markdownUtils.ts b/packages/core/src/utils/markdownUtils.ts new file mode 100644 index 0000000000..ea0fee8eb8 --- /dev/null +++ b/packages/core/src/utils/markdownUtils.ts @@ -0,0 +1,147 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Converts a camelCase string to a Space Case string. + * e.g., "camelCaseString" -> "Camel Case String" + */ +function camelToSpace(text: string): string { + const result = text.replace(/([A-Z])/g, ' $1'); + return result.charAt(0).toUpperCase() + result.slice(1).trim(); +} + +/** + * Converts a JSON-compatible value into a readable Markdown representation. + * + * @param data The data to convert. + * @param indent The current indentation level (for internal recursion). + * @returns A Markdown string representing the data. + */ +export function jsonToMarkdown(data: unknown, indent = 0): string { + const spacing = ' '.repeat(indent); + + if (data === null) { + return 'null'; + } + + if (data === undefined) { + return 'undefined'; + } + + if (Array.isArray(data)) { + if (data.length === 0) { + return '[]'; + } + + if (isArrayOfSimilarObjects(data)) { + return renderTable(data, indent); + } + + return data + .map((item) => { + if ( + typeof item === 'object' && + item !== null && + Object.keys(item).length > 0 + ) { + const rendered = jsonToMarkdown(item, indent + 1); + return `${spacing}-\n${rendered}`; + } + const rendered = jsonToMarkdown(item, indent + 1).trimStart(); + return `${spacing}- ${rendered}`; + }) + .join('\n'); + } + + if (typeof data === 'object') { + const entries = Object.entries(data); + if (entries.length === 0) { + return '{}'; + } + + return entries + .map(([key, value]) => { + const displayKey = camelToSpace(key); + if ( + typeof value === 'object' && + value !== null && + Object.keys(value).length > 0 + ) { + const renderedValue = jsonToMarkdown(value, indent + 1); + return `${spacing}- **${displayKey}**:\n${renderedValue}`; + } + const renderedValue = jsonToMarkdown(value, indent + 1).trimStart(); + return `${spacing}- **${displayKey}**: ${renderedValue}`; + }) + .join('\n'); + } + + if (typeof data === 'string') { + return data + .split('\n') + .map((line, i) => (i === 0 ? line : spacing + line)) + .join('\n'); + } + + return String(data); +} + +/** + * Safely attempts to parse a string as JSON and convert it to Markdown. + * If parsing fails, returns the original string. + * + * @param text The text to potentially convert. + * @returns The Markdown representation or the original text. + */ +export function safeJsonToMarkdown(text: string): string { + try { + const parsed: unknown = JSON.parse(text); + return jsonToMarkdown(parsed); + } catch { + return text; + } +} + +export function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value); +} + +function isArrayOfSimilarObjects( + data: unknown[], +): data is Array> { + if (data.length === 0) { + return false; + } + if (!data.every(isRecord)) return false; + const firstKeys = Object.keys(data[0]).sort().join(','); + return data.every((item) => Object.keys(item).sort().join(',') === firstKeys); +} + +function renderTable(data: Array>, indent = 0): string { + const spacing = ' '.repeat(indent); + const keys = Object.keys(data[0]); + const displayKeys = keys.map(camelToSpace); + const header = `${spacing}| ${displayKeys.join(' | ')} |`; + const separator = `${spacing}| ${keys.map(() => '---').join(' | ')} |`; + const rows = data.map( + (item) => + `${spacing}| ${keys + .map((key) => { + const val = item[key]; + if (typeof val === 'object' && val !== null) { + return JSON.stringify(val) + .replace(/\\/g, '\\\\') + .replace(/\|/g, '\\|'); + } + return String(val) + .replace(/\\/g, '\\\\') + .replace(/\|/g, '\\|') + .replace(/\n/g, ' '); + }) + .join(' | ')} |`, + ); + return [header, separator, ...rows].join('\n'); +} diff --git a/packages/core/src/utils/oauth-flow.ts b/packages/core/src/utils/oauth-flow.ts index e13fd37837..45318efdb5 100644 --- a/packages/core/src/utils/oauth-flow.ts +++ b/packages/core/src/utils/oauth-flow.ts @@ -454,6 +454,7 @@ export async function exchangeCodeForToken( params.append('resource', resource); } + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(config.tokenUrl, { method: 'POST', headers: { @@ -507,6 +508,7 @@ export async function refreshAccessToken( params.append('resource', resource); } + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(tokenUrl, { method: 'POST', headers: { diff --git a/packages/core/src/utils/paths.test.ts b/packages/core/src/utils/paths.test.ts index 227afaf44a..4563c0485b 100644 --- a/packages/core/src/utils/paths.test.ts +++ b/packages/core/src/utils/paths.test.ts @@ -484,6 +484,10 @@ describe('shortenPath', () => { }); describe('resolveToRealPath', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + it.each([ { description: @@ -542,6 +546,28 @@ describe('resolveToRealPath', () => { expect(resolveToRealPath(childPath)).toBe(expectedPath); }); + + it('should prevent infinite recursion on malicious symlink structures', () => { + const maliciousPath = path.resolve('malicious', 'symlink'); + + vi.spyOn(fs, 'realpathSync').mockImplementation(() => { + const err = new Error('ENOENT') as NodeJS.ErrnoException; + err.code = 'ENOENT'; + throw err; + }); + + vi.spyOn(fs, 'lstatSync').mockImplementation( + () => ({ isSymbolicLink: () => true }) as fs.Stats, + ); + + vi.spyOn(fs, 'readlinkSync').mockImplementation(() => + ['..', 'malicious', 'symlink'].join(path.sep), + ); + + expect(() => resolveToRealPath(maliciousPath)).toThrow( + /Infinite recursion detected/, + ); + }); }); describe('normalizePath', () => { diff --git a/packages/core/src/utils/paths.ts b/packages/core/src/utils/paths.ts index aa167e3558..338d4017e5 100644 --- a/packages/core/src/utils/paths.ts +++ b/packages/core/src/utils/paths.ts @@ -375,7 +375,12 @@ export function resolveToRealPath(pathStr: string): string { return robustRealpath(path.resolve(resolvedPath)); } -function robustRealpath(p: string): string { +function robustRealpath(p: string, visited = new Set()): string { + const key = process.platform === 'win32' ? p.toLowerCase() : p; + if (visited.has(key)) { + throw new Error(`Infinite recursion detected in robustRealpath: ${p}`); + } + visited.add(key); try { return fs.realpathSync(p); } catch (e: unknown) { @@ -385,14 +390,25 @@ function robustRealpath(p: string): string { if (stat.isSymbolicLink()) { const target = fs.readlinkSync(p); const resolvedTarget = path.resolve(path.dirname(p), target); - return robustRealpath(resolvedTarget); + return robustRealpath(resolvedTarget, visited); + } + } catch (lstatError: unknown) { + // Not a symlink, or lstat failed. Re-throw if it's not an expected + // ENOENT (e.g., a permissions error), otherwise resolve parent. + if ( + !( + lstatError && + typeof lstatError === 'object' && + 'code' in lstatError && + lstatError.code === 'ENOENT' + ) + ) { + throw lstatError; } - } catch { - // Not a symlink, or lstat failed. Just resolve parent. } const parent = path.dirname(p); if (parent === p) return p; - return path.join(robustRealpath(parent), path.basename(p)); + return path.join(robustRealpath(parent, visited), path.basename(p)); } throw e; } diff --git a/packages/sdk/src/session.ts b/packages/sdk/src/session.ts index 8332ef29d0..59ed857937 100644 --- a/packages/sdk/src/session.ts +++ b/packages/sdk/src/session.ts @@ -226,7 +226,7 @@ export class GeminiCliSession { break; } - const transcript: Content[] = client.getHistory(); + const transcript: readonly Content[] = client.getHistory(); const context: SessionContext = { sessionId, transcript, diff --git a/packages/sdk/src/types.ts b/packages/sdk/src/types.ts index 9b6bf7093a..f6ba1cd8a5 100644 --- a/packages/sdk/src/types.ts +++ b/packages/sdk/src/types.ts @@ -51,7 +51,7 @@ export interface AgentShell { export interface SessionContext { sessionId: string; - transcript: Content[]; + transcript: readonly Content[]; cwd: string; timestamp: string; fs: AgentFilesystem; diff --git a/packages/vscode-ide-companion/src/extension.ts b/packages/vscode-ide-companion/src/extension.ts index 456ec6e872..e8cef91c2b 100644 --- a/packages/vscode-ide-companion/src/extension.ts +++ b/packages/vscode-ide-companion/src/extension.ts @@ -42,6 +42,7 @@ async function checkForUpdates( const currentVersion = context.extension.packageJSON.version; // Fetch extension details from the VSCode Marketplace. + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch( 'https://marketplace.visualstudio.com/_apis/public/gallery/extensionquery', { diff --git a/packages/vscode-ide-companion/src/ide-server.test.ts b/packages/vscode-ide-companion/src/ide-server.test.ts index eb28638a78..b3d39bf832 100644 --- a/packages/vscode-ide-companion/src/ide-server.test.ts +++ b/packages/vscode-ide-companion/src/ide-server.test.ts @@ -356,6 +356,7 @@ describe('IDEServer', () => { }); it('should reject request without auth token', async () => { + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(`http://localhost:${port}/mcp`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, @@ -370,6 +371,7 @@ describe('IDEServer', () => { }); it('should allow request with valid auth token', async () => { + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(`http://localhost:${port}/mcp`, { method: 'POST', headers: { @@ -387,6 +389,7 @@ describe('IDEServer', () => { }); it('should reject request with invalid auth token', async () => { + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(`http://localhost:${port}/mcp`, { method: 'POST', headers: { @@ -413,6 +416,7 @@ describe('IDEServer', () => { ]; for (const header of malformedHeaders) { + // eslint-disable-next-line no-restricted-syntax -- TODO: Migrate to safeFetch for SSRF protection const response = await fetch(`http://localhost:${port}/mcp`, { method: 'POST', headers: { diff --git a/scripts/review.sh b/scripts/review.sh new file mode 100755 index 0000000000..653fd92baf --- /dev/null +++ b/scripts/review.sh @@ -0,0 +1,132 @@ +#!/bin/bash +# scripts/review.sh +# +# Usage: ./scripts/review.sh [model] + +set -e + +if [ -z "$1" ]; then + echo "Usage: $0 [model]" + exit 1 +fi +pr="$1" +model="${2:-gemini-3.1-pro-preview}" +REPO="google-gemini/gemini-cli" +REVIEW_DIR="$HOME/git/review/gemini-cli" + +if [ ! -d "$REVIEW_DIR" ]; then + echo "ERROR: Directory $REVIEW_DIR does not exist." + echo "" + echo "Please create a new gemini-cli clone at that directory to use for reviews." + echo "Instructions:" + echo " mkdir -p ~/git/review" + echo " cd ~/git/review" + echo " git clone https://github.com/google-gemini/gemini-cli.git" + exit 1 +fi + +# 1. Check if the PR exists before doing anything else +echo "review: Validating PR $pr on $REPO..." +if ! gh pr view "$pr" -R "$REPO" > /dev/null 2>&1; then + echo "ERROR: Could not find PR #$pr in $REPO." + echo "Are you sure $pr is a Pull Request number and not an Issue number?" + exit 1 +fi + +echo "review: Opening PR $pr in browser..." +if [[ "$(uname)" == "Darwin" ]]; then + open "https://github.com/$REPO/pull/$pr" || true +else + xdg-open "https://github.com/$REPO/pull/$pr" || true +fi + +echo "review: Changing directory to $REVIEW_DIR" +cd "$REVIEW_DIR" || exit 1 + +# 2. Fetch latest main to ensure we have a clean starting point +echo "review: Fetching latest from origin..." +git fetch origin main + +# 3. Handle worktree creation +WORKTREE_PATH="pr_$pr" +if [ -d "$WORKTREE_PATH" ]; then + echo "review: Worktree directory $WORKTREE_PATH already exists." + # Check if it's actually a registered worktree + if git worktree list | grep -q "$WORKTREE_PATH"; then + echo "review: Reusing existing worktree..." + else + echo "review: Directory exists but is not a worktree. Cleaning up..." + rm -rf "$WORKTREE_PATH" + fi +fi + +if [ ! -d "$WORKTREE_PATH" ]; then + echo "review: Adding new worktree at $WORKTREE_PATH..." + # Create a detached worktree from origin/main + git worktree add --detach "$WORKTREE_PATH" origin/main +fi + +echo "review: Changing directory to $WORKTREE_PATH" +cd "$WORKTREE_PATH" || exit 1 + +# 4. Checkout the PR +echo "review: Checking out PR $pr..." +gh pr checkout "$pr" -f -R "$REPO" + +# 5. Clean and Build +echo "review: Clearing possibly stale node_modules..." +rm -rf node_modules +rm -rf packages/core/dist/ +rm -rf packages/cli/node_modules/ +rm -rf packages/core/node_modules/ + +echo "review: Installing npm dependencies..." +npm install + +echo "--- build ---" +temp_dir_base="${TMPDIR:-/tmp}" +build_log_file=$(mktemp "${temp_dir_base}/npm_build_log.XXXXXX") || { + echo "Attempting to create temporary file in current directory as a fallback." >&2 + build_log_file=$(mktemp "./npm_build_log_fallback.XXXXXX") + if [ $? -ne 0 ] || [ -z "$build_log_file" ]; then + echo "ERROR: Critical - Failed to create any temporary build log file. Aborting." >&2 + exit 1 + fi +} + +build_status=0 +build_command_to_run="FORCE_COLOR=1 CLICOLOR_FORCE=1 npm run build" + +echo "Running build. Output (with colors) will be shown below and saved to: $build_log_file" +echo "Build command: $build_command_to_run" + +if [[ "$(uname)" == "Darwin" ]]; then + script -q "$build_log_file" /bin/sh -c "$build_command_to_run" + build_status=$? +else + if script -q -e -c "$build_command_to_run" "$build_log_file"; then + build_status=0 + else + build_status=$? + fi +fi + +if [ $build_status -ne 0 ]; then + echo "ERROR: npm build failed with exit status $build_status." >&2 + echo "Review output above. Full log (with color codes) was in $build_log_file." >&2 + exit 1 +else + if grep -q -i -E "\berror\b|\bfailed\b|ERR!|FATAL|critical" "$build_log_file"; then + echo "ERROR: npm build completed with exit status 0, but suspicious error patterns were found in the build output." >&2 + echo "Review output above. Full log (with color codes) was in $build_log_file." >&2 + exit 1 + fi + echo "npm build completed successfully (exit status 0, no critical error patterns found in log)." + rm -f "$build_log_file" +fi + +echo "-- running ---" +if ! npm start -- -m "$model" -i="/review-frontend $pr"; then + echo "ERROR: npm start failed. Please check its output for details." >&2 + exit 1 +fi