diff --git a/.github/workflows/agent-session-drift-check.yml b/.github/workflows/agent-session-drift-check.yml new file mode 100644 index 0000000000..3601f5ab09 --- /dev/null +++ b/.github/workflows/agent-session-drift-check.yml @@ -0,0 +1,132 @@ +# yaml-language-server: $schema=https://json.schemastore.org/github-workflow.json + +name: 'Agent Session Drift Check' + +on: + pull_request: + branches: + - 'main' + - 'release/**' + paths: + - 'packages/cli/src/nonInteractiveCli.ts' + - 'packages/cli/src/nonInteractiveCliAgentSession.ts' + +concurrency: + group: '${{ github.workflow }}-${{ github.head_ref || github.ref }}' + cancel-in-progress: true + +jobs: + check-drift: + name: 'Check Agent Session Drift' + runs-on: 'ubuntu-latest' + if: "github.repository == 'google-gemini/gemini-cli'" + permissions: + contents: 'read' + pull-requests: 'write' + steps: + - name: 'Detect drift and comment' + uses: 'actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea' # ratchet:actions/github-script@v8 + with: + script: |- + // === Pair configuration — append here to cover more pairs === + const PAIRS = [ + { + legacy: 'packages/cli/src/nonInteractiveCli.ts', + session: 'packages/cli/src/nonInteractiveCliAgentSession.ts', + label: 'non-interactive CLI', + }, + // Future pairs can be added here. Remember to also add both + // paths to the `paths:` filter at the top of this workflow. + // Example: + // { + // legacy: 'packages/core/src/agents/local-invocation.ts', + // session: 'packages/core/src/agents/local-session-invocation.ts', + // label: 'local subagent invocation', + // }, + ]; + // ============================================================ + + const prNumber = context.payload.pull_request.number; + const { owner, repo } = context.repo; + + // Use the API to list changed files — no checkout/git diff needed. + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, + repo, + pull_number: prNumber, + per_page: 100, + }); + const changed = new Set(files.map((f) => f.filename)); + + const warnings = []; + for (const { legacy, session, label } of PAIRS) { + const legacyChanged = changed.has(legacy); + const sessionChanged = changed.has(session); + if (legacyChanged && !sessionChanged) { + warnings.push( + `**${label}**: \`${legacy}\` was modified but \`${session}\` was not.`, + ); + } else if (!legacyChanged && sessionChanged) { + warnings.push( + `**${label}**: \`${session}\` was modified but \`${legacy}\` was not.`, + ); + } + } + + const MARKER = ''; + + // Look up our existing drift comment (for upsert/cleanup). + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: prNumber, + per_page: 100, + }); + const existing = comments.find( + (c) => c.user?.type === 'Bot' && c.body?.includes(MARKER), + ); + + if (warnings.length === 0) { + core.info('No drift detected.'); + // If drift was previously flagged and is now resolved, remove the comment. + if (existing) { + await github.rest.issues.deleteComment({ + owner, + repo, + comment_id: existing.id, + }); + core.info(`Deleted stale drift comment ${existing.id}.`); + } + return; + } + + const body = [ + MARKER, + '### ⚠️ Invocation Drift Warning', + '', + 'The following file pairs should generally be kept in sync during the AgentSession migration:', + '', + ...warnings.map((w) => `- ${w}`), + '', + 'If this is intentional (e.g., a bug fix specific to one implementation), you can ignore this comment.', + '', + '_This check will be removed once the legacy implementations are deleted._', + ].join('\n'); + + if (existing) { + core.info(`Updating existing drift comment ${existing.id}.`); + await github.rest.issues.updateComment({ + owner, + repo, + comment_id: existing.id, + body, + }); + } else { + core.info('Creating new drift comment.'); + await github.rest.issues.createComment({ + owner, + repo, + issue_number: prNumber, + body, + }); + } diff --git a/packages/cli/src/ui/auth/ApiAuthDialog.test.tsx b/packages/cli/src/ui/auth/ApiAuthDialog.test.tsx index d46e0295a1..12d01fac4c 100644 --- a/packages/cli/src/ui/auth/ApiAuthDialog.test.tsx +++ b/packages/cli/src/ui/auth/ApiAuthDialog.test.tsx @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { render } from '../../test-utils/render.js'; +import { renderWithProviders } from '../../test-utils/render.js'; import { waitFor } from '../../test-utils/async.js'; import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; import { ApiAuthDialog } from './ApiAuthDialog.js'; @@ -40,11 +40,16 @@ vi.mock('../components/shared/text-buffer.js', async (importOriginal) => { }; }); -vi.mock('../contexts/UIStateContext.js', () => ({ - useUIState: vi.fn(() => ({ - terminalWidth: 80, - })), -})); +vi.mock('../contexts/UIStateContext.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + useUIState: vi.fn(() => ({ + terminalWidth: 80, + })), + }; +}); const mockedUseKeypress = useKeypress as Mock; const mockedUseTextBuffer = useTextBuffer as Mock; @@ -73,7 +78,7 @@ describe('ApiAuthDialog', () => { }); it('renders correctly', async () => { - const { lastFrame, unmount } = await render( + const { lastFrame, unmount } = await renderWithProviders( , ); expect(lastFrame()).toMatchSnapshot(); @@ -81,7 +86,7 @@ describe('ApiAuthDialog', () => { }); it('renders with a defaultValue', async () => { - const { unmount } = await render( + const { unmount } = await renderWithProviders( { 'calls $expectedCall.name when $keyName is pressed', async ({ keyName, sequence, expectedCall, args }) => { mockBuffer.text = 'submitted-key'; // Set for the onSubmit case - const { unmount } = await render( + const { unmount } = await renderWithProviders( , ); // calls[0] is the ApiAuthDialog's useKeypress (Ctrl+C handler) @@ -133,7 +138,7 @@ describe('ApiAuthDialog', () => { ); it('displays an error message', async () => { - const { lastFrame, unmount } = await render( + const { lastFrame, unmount } = await renderWithProviders( { }); it('calls clearApiKey and clears buffer when Ctrl+C is pressed', async () => { - const { unmount } = await render( + const { unmount } = await renderWithProviders( , ); // Call 0 is ApiAuthDialog (isActive: true) diff --git a/packages/cli/src/ui/components/AskUserDialog.tsx b/packages/cli/src/ui/components/AskUserDialog.tsx index 295d54eb73..7e1dbf9c00 100644 --- a/packages/cli/src/ui/components/AskUserDialog.tsx +++ b/packages/cli/src/ui/components/AskUserDialog.tsx @@ -13,7 +13,8 @@ import { useReducer, useContext, } from 'react'; -import { Box, Text } from 'ink'; +import { Box, Text, type DOMElement } from 'ink'; +import { useMouseClick } from '../hooks/useMouseClick.js'; import { theme } from '../semantic-colors.js'; import { checkExhaustive, type Question } from '@google/gemini-cli-core'; import { BaseSelectionList } from './shared/BaseSelectionList.js'; @@ -85,6 +86,24 @@ function autoBoldIfPlain(text: string): string { return text; } +const ClickableCheckbox: React.FC<{ + isChecked: boolean; + onClick: () => void; +}> = ({ isChecked, onClick }) => { + const ref = useRef(null); + useMouseClick(ref, () => { + onClick(); + }); + + return ( + + + [{isChecked ? 'x' : ' '}] + + + ); +}; + interface AskUserDialogState { answers: { [key: string]: string }; isEditingCustomOption: boolean; @@ -919,13 +938,14 @@ const ChoiceQuestionView: React.FC = ({ return ( {showCheck && ( - - [{isChecked ? 'x' : ' '}] - + { + if (!context.isSelected) { + handleSelect(optionItem); + } + }} + /> )} = ({ {showCheck && ( - - [{isChecked ? 'x' : ' '}] - + { + if (!context.isSelected) { + handleSelect(optionItem); + } + }} + /> )} {' '} diff --git a/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx b/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx index b873de80d9..49bcb92728 100644 --- a/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx +++ b/packages/cli/src/ui/components/shared/BaseSelectionList.test.tsx @@ -4,7 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import { act } from 'react'; import { renderWithProviders } from '../../../test-utils/render.js'; import { BaseSelectionList, @@ -14,8 +15,10 @@ import { import { useSelectionList } from '../../hooks/useSelectionList.js'; import { Text } from 'ink'; import type { theme } from '../../semantic-colors.js'; +import { useMouseClick } from '../../hooks/useMouseClick.js'; vi.mock('../../hooks/useSelectionList.js'); +vi.mock('../../hooks/useMouseClick.js'); const mockTheme = { text: { primary: 'COLOR_PRIMARY', secondary: 'COLOR_SECONDARY' }, @@ -35,6 +38,7 @@ describe('BaseSelectionList', () => { const mockOnSelect = vi.fn(); const mockOnHighlight = vi.fn(); const mockRenderItem = vi.fn(); + const mockSetActiveIndex = vi.fn(); const items = [ { value: 'A', label: 'Item A', key: 'A' }, @@ -54,7 +58,7 @@ describe('BaseSelectionList', () => { ) => { vi.mocked(useSelectionList).mockReturnValue({ activeIndex, - setActiveIndex: vi.fn(), + setActiveIndex: mockSetActiveIndex, }); mockRenderItem.mockImplementation( @@ -484,6 +488,79 @@ describe('BaseSelectionList', () => { }); }); + describe('Mouse Interaction', () => { + it('should register mouse click handler for each item', async () => { + const { unmount } = await renderComponent(); + + // items are A, B (disabled), C + expect(useMouseClick).toHaveBeenCalledTimes(3); + unmount(); + }); + + it('should update activeIndex on first click and call onSelect on second click', async () => { + const { unmount, waitUntilReady } = await renderComponent(); + await waitUntilReady(); + + // items[0] is 'A' (enabled) + // items[1] is 'B' (disabled) + // items[2] is 'C' (enabled) + + // Get the mouse click handler for the third item (index 2) + const mouseClickHandler = (useMouseClick as Mock).mock.calls[2][1]; + + // First click on item C + act(() => { + mouseClickHandler(); + }); + + expect(mockSetActiveIndex).toHaveBeenCalledWith(2); + expect(mockOnSelect).not.toHaveBeenCalled(); + + // Now simulate being on item C (isSelected = true) + // Rerender or update mocks for the next check + await renderComponent({}, 2); + + // Get the updated mouse click handler for item C + // useMouseClick is called 3 more times on rerender + const updatedMouseClickHandler = (useMouseClick as Mock).mock.calls[5][1]; + + // Second click on item C + act(() => { + updatedMouseClickHandler(); + }); + + expect(mockOnSelect).toHaveBeenCalledWith('C'); + unmount(); + }); + + it('should not call onSelect when a disabled item is clicked', async () => { + const { unmount, waitUntilReady } = await renderComponent(); + await waitUntilReady(); + + // items[1] is 'B' (disabled) + const mouseClickHandler = (useMouseClick as Mock).mock.calls[1][1]; + + act(() => { + mouseClickHandler(); + }); + + expect(mockSetActiveIndex).not.toHaveBeenCalled(); + expect(mockOnSelect).not.toHaveBeenCalled(); + unmount(); + }); + + it('should pass isActive: isFocused to useMouseClick', async () => { + const { unmount } = await renderComponent({ isFocused: false }); + + expect(useMouseClick).toHaveBeenCalledWith( + expect.any(Object), + expect.any(Function), + { isActive: false }, + ); + unmount(); + }); + }); + describe('Scroll Arrows (showScrollArrows)', () => { const longList = Array.from({ length: 10 }, (_, i) => ({ value: `Item ${i + 1}`, diff --git a/packages/cli/src/ui/components/shared/BaseSelectionList.tsx b/packages/cli/src/ui/components/shared/BaseSelectionList.tsx index 455069f03f..353fca196f 100644 --- a/packages/cli/src/ui/components/shared/BaseSelectionList.tsx +++ b/packages/cli/src/ui/components/shared/BaseSelectionList.tsx @@ -5,13 +5,14 @@ */ import type React from 'react'; -import { useState } from 'react'; -import { Text, Box } from 'ink'; +import { useState, useRef } from 'react'; +import { Text, Box, type DOMElement } from 'ink'; import { theme } from '../../semantic-colors.js'; import { useSelectionList, type SelectionListItem, } from '../../hooks/useSelectionList.js'; +import { useMouseClick } from '../../hooks/useMouseClick.js'; export interface RenderItemContext { isSelected: boolean; @@ -38,6 +39,119 @@ export interface BaseSelectionListProps< renderItem: (item: TItem, context: RenderItemContext) => React.ReactNode; } +interface SelectionListItemRowProps< + T, + TItem extends SelectionListItem = SelectionListItem, +> { + item: TItem; + itemIndex: number; + isSelected: boolean; + isFocused: boolean; + showNumbers: boolean; + selectedIndicator: string; + numberColumnWidth: number; + onSelect: (value: T) => void; + setActiveIndex: (index: number) => void; + renderItem: (item: TItem, context: RenderItemContext) => React.ReactNode; +} + +function SelectionListItemRow< + T, + TItem extends SelectionListItem = SelectionListItem, +>({ + item, + itemIndex, + isSelected, + isFocused, + showNumbers, + selectedIndicator, + numberColumnWidth, + onSelect, + setActiveIndex, + renderItem, +}: SelectionListItemRowProps) { + const containerRef = useRef(null); + + useMouseClick( + containerRef, + () => { + if (!item.disabled) { + if (isSelected) { + // Second click on the same item triggers submission + onSelect(item.value); + } else { + // First click highlights the item + setActiveIndex(itemIndex); + } + } + }, + { isActive: isFocused }, + ); + + let titleColor = theme.text.primary; + let numberColor = theme.text.primary; + + if (isSelected) { + titleColor = theme.ui.focus; + numberColor = theme.ui.focus; + } else if (item.disabled) { + titleColor = theme.text.secondary; + numberColor = theme.text.secondary; + } + + if (!isFocused && !item.disabled) { + numberColor = theme.text.secondary; + } + + if (!showNumbers) { + numberColor = theme.text.secondary; + } + + const itemNumberText = `${String(itemIndex + 1).padStart( + numberColumnWidth, + )}.`; + + return ( + + {/* Radio button indicator */} + + + {isSelected ? selectedIndicator : ' '} + + + + {/* Item number */} + {showNumbers && !item.hideNumber && ( + + {itemNumberText} + + )} + + {/* Custom content via render prop */} + + {renderItem(item, { + isSelected, + titleColor, + numberColor, + })} + + + ); +} + /** * Base component for selection lists that provides common UI structure * and keyboard navigation logic via the useSelectionList hook. @@ -70,7 +184,7 @@ export function BaseSelectionList< selectedIndicator = '●', renderItem, }: BaseSelectionListProps): React.JSX.Element { - const { activeIndex } = useSelectionList({ + const { activeIndex, setActiveIndex } = useSelectionList({ items, initialIndex, onSelect, @@ -107,10 +221,12 @@ export function BaseSelectionList< ); const numberColumnWidth = String(items.length).length; + const showArrows = showScrollArrows && items.length > maxItemsToShow; + return ( {/* Use conditional coloring instead of conditional rendering */} - {showScrollArrows && items.length > maxItemsToShow && ( + {showArrows && ( 0 @@ -126,71 +242,24 @@ export function BaseSelectionList< const itemIndex = effectiveScrollOffset + index; const isSelected = activeIndex === itemIndex; - // Determine colors based on selection and disabled state - let titleColor = theme.text.primary; - let numberColor = theme.text.primary; - - if (isSelected) { - titleColor = theme.ui.focus; - numberColor = theme.ui.focus; - } else if (item.disabled) { - titleColor = theme.text.secondary; - numberColor = theme.text.secondary; - } - - if (!isFocused && !item.disabled) { - numberColor = theme.text.secondary; - } - - if (!showNumbers) { - numberColor = theme.text.secondary; - } - - const itemNumberText = `${String(itemIndex + 1).padStart( - numberColumnWidth, - )}.`; - return ( - - {/* Radio button indicator */} - - - {isSelected ? selectedIndicator : ' '} - - - - {/* Item number */} - {showNumbers && !item.hideNumber && ( - - {itemNumberText} - - )} - - {/* Custom content via render prop */} - - {renderItem(item, { - isSelected, - titleColor, - numberColor, - })} - - + item={item} + itemIndex={itemIndex} + isSelected={isSelected} + isFocused={isFocused} + showNumbers={showNumbers} + selectedIndicator={selectedIndicator} + numberColumnWidth={numberColumnWidth} + onSelect={onSelect} + setActiveIndex={setActiveIndex} + renderItem={renderItem} + /> ); })} - {showScrollArrows && items.length > maxItemsToShow && ( + {showArrows && ( ({ useKeypress: vi.fn(), })); +vi.mock('../../hooks/useMouseClick.js', () => ({ + useMouseClick: vi.fn(), +})); + vi.mock('./text-buffer.js', async (importOriginal) => { const actual = await importOriginal(); const mockTextBuffer = { @@ -69,6 +74,7 @@ vi.mock('./text-buffer.js', async (importOriginal) => { const mockedUseKeypress = useKeypress as Mock; const mockedUseTextBuffer = useTextBuffer as Mock; +const mockedUseMouseClick = useMouseClick as Mock; describe('TextInput', () => { const onCancel = vi.fn(); @@ -84,6 +90,7 @@ describe('TextInput', () => { cursor: [0, 0], visualCursor: [0, 0], viewportVisualLines: [''], + visualScrollRow: 0, pastedContent: {} as Record, handleInput: vi.fn((key) => { if (key.sequence) { @@ -408,4 +415,36 @@ describe('TextInput', () => { expect(lastFrame()).toContain('line2'); unmount(); }); + + it('registers mouse click handler for free-form text input', async () => { + const { unmount } = await render( + , + ); + + expect(mockedUseMouseClick).toHaveBeenCalledWith( + expect.any(Object), + expect.any(Function), + expect.objectContaining({ isActive: true, name: 'left-press' }), + ); + unmount(); + }); + + it('registers mouse click handler for placeholder view', async () => { + mockBuffer.text = ''; + const { unmount } = await render( + , + ); + + expect(mockedUseMouseClick).toHaveBeenCalledWith( + expect.any(Object), + expect.any(Function), + expect.objectContaining({ isActive: true, name: 'left-press' }), + ); + unmount(); + }); }); diff --git a/packages/cli/src/ui/components/shared/TextInput.tsx b/packages/cli/src/ui/components/shared/TextInput.tsx index 479757d8f3..4cb8a6c94c 100644 --- a/packages/cli/src/ui/components/shared/TextInput.tsx +++ b/packages/cli/src/ui/components/shared/TextInput.tsx @@ -5,8 +5,8 @@ */ import type React from 'react'; -import { useCallback } from 'react'; -import { Text, Box } from 'ink'; +import { useCallback, useRef } from 'react'; +import { Text, Box, type DOMElement } from 'ink'; import { useKeypress, type Key } from '../../hooks/useKeypress.js'; import chalk from 'chalk'; import { theme } from '../../semantic-colors.js'; @@ -14,6 +14,7 @@ import { expandPastePlaceholders, type TextBuffer } from './text-buffer.js'; import { cpSlice, cpIndexToOffset } from '../../utils/textUtils.js'; import { Command } from '../../key/keyMatchers.js'; import { useKeyMatchers } from '../../hooks/useKeyMatchers.js'; +import { useMouseClick } from '../../hooks/useMouseClick.js'; export interface TextInputProps { buffer: TextBuffer; @@ -31,6 +32,8 @@ export function TextInput({ focus = true, }: TextInputProps): React.JSX.Element { const keyMatchers = useKeyMatchers(); + const containerRef = useRef(null); + const { text, handleInput, @@ -40,6 +43,17 @@ export function TextInput({ } = buffer; const [cursorVisualRowAbsolute, cursorVisualColAbsolute] = visualCursor; + useMouseClick( + containerRef, + (_event, relativeX, relativeY) => { + if (focus) { + const visRowAbsolute = visualScrollRow + relativeY; + buffer.moveToVisualPosition(visRowAbsolute, relativeX); + } + }, + { isActive: focus, name: 'left-press' }, + ); + const handleKeyPress = useCallback( (key: Key) => { if (key.name === 'escape' && onCancel) { @@ -64,7 +78,7 @@ export function TextInput({ if (showPlaceholder) { return ( - + {focus ? ( {chalk.inverse(placeholder[0] || ' ')} @@ -78,7 +92,7 @@ export function TextInput({ } return ( - + {viewportVisualLines.map((lineText, idx) => { const currentVisualRow = visualScrollRow + idx; const isCursorLine = diff --git a/packages/core/src/core/__snapshots__/prompts.test.ts.snap b/packages/core/src/core/__snapshots__/prompts.test.ts.snap index 0e9cc591d1..9132791974 100644 --- a/packages/core/src/core/__snapshots__/prompts.test.ts.snap +++ b/packages/core/src/core/__snapshots__/prompts.test.ts.snap @@ -119,6 +119,7 @@ The following tools are available in Plan Mode: - **Directives:** If the request is a **Directive** (e.g., "Fix bug Y"), follow the workflow below. 5. **Plan Storage:** Save plans as Markdown (.md) using descriptive filenames. 6. **Direct Modification:** If asked to modify code, explain you are in Plan Mode and use \`exit_plan_mode\` to request approval. +7. **Presenting Plan:** When seeking informal agreement on a plan, or any time the user asks to see the plan, you MUST output the full content of the plan in the chat response. This overrides the "Minimal Output" guideline. ## Planning Workflow Plan Mode uses an adaptive planning workflow where the research depth, plan structure, and consultation level are proportional to the task's complexity. @@ -139,6 +140,7 @@ Write the implementation plan to \`/tmp/plans/\`. The plan's structure adapts to - **Simple Tasks:** Include a bulleted list of specific **Changes** and **Verification** steps. - **Standard Tasks:** Include an **Objective**, **Key Files & Context**, **Implementation Steps**, and **Verification & Testing**. - **Complex Tasks:** Include **Background & Motivation**, **Scope & Impact**, **Proposed Solution**, **Alternatives Considered**, a phased **Implementation Plan**, **Verification**, and **Migration & Rollback** strategies. +- **Alignment Check:** After drafting the plan, you MUST present it to the user in the chat (adhering to Rule 7 for presenting plans) to ensure alignment on the specific details. Ask for feedback or confirmation, and proceed to Step 4 (Review & Approval) once the user agrees with the detailed plan. ### 4. Review & Approval ONLY use the \`exit_plan_mode\` tool to present the plan for formal approval AFTER you have reached an informal agreement with the user in the chat regarding the proposed strategy. When called, this tool will present the plan and formally request approval. @@ -297,6 +299,7 @@ The following tools are available in Plan Mode: - **Directives:** If the request is a **Directive** (e.g., "Fix bug Y"), follow the workflow below. 5. **Plan Storage:** Save plans as Markdown (.md) using descriptive filenames. 6. **Direct Modification:** If asked to modify code, explain you are in Plan Mode and use \`exit_plan_mode\` to request approval. +7. **Presenting Plan:** When seeking informal agreement on a plan, or any time the user asks to see the plan, you MUST output the full content of the plan in the chat response. This overrides the "Minimal Output" guideline. ## Planning Workflow Plan Mode uses an adaptive planning workflow where the research depth, plan structure, and consultation level are proportional to the task's complexity. @@ -317,6 +320,7 @@ Write the implementation plan to \`/tmp/plans/\`. The plan's structure adapts to - **Simple Tasks:** Include a bulleted list of specific **Changes** and **Verification** steps. - **Standard Tasks:** Include an **Objective**, **Key Files & Context**, **Implementation Steps**, and **Verification & Testing**. - **Complex Tasks:** Include **Background & Motivation**, **Scope & Impact**, **Proposed Solution**, **Alternatives Considered**, a phased **Implementation Plan**, **Verification**, and **Migration & Rollback** strategies. +- **Alignment Check:** After drafting the plan, you MUST present it to the user in the chat (adhering to Rule 7 for presenting plans) to ensure alignment on the specific details. Ask for feedback or confirmation, and proceed to Step 4 (Review & Approval) once the user agrees with the detailed plan. ### 4. Review & Approval ONLY use the \`exit_plan_mode\` tool to present the plan for formal approval AFTER you have reached an informal agreement with the user in the chat regarding the proposed strategy. When called, this tool will present the plan and formally request approval. @@ -596,6 +600,7 @@ The following tools are available in Plan Mode: - **Directives:** If the request is a **Directive** (e.g., "Fix bug Y"), follow the workflow below. 5. **Plan Storage:** Save plans as Markdown (.md) using descriptive filenames. 6. **Direct Modification:** If asked to modify code, explain you are in Plan Mode and use \`exit_plan_mode\` to request approval. +7. **Presenting Plan:** When seeking informal agreement on a plan, or any time the user asks to see the plan, you MUST output the full content of the plan in the chat response. This overrides the "Minimal Output" guideline. ## Planning Workflow Plan Mode uses an adaptive planning workflow where the research depth, plan structure, and consultation level are proportional to the task's complexity. @@ -616,6 +621,7 @@ Write the implementation plan to \`/tmp/project-temp/plans/\`. The plan's struct - **Simple Tasks:** Include a bulleted list of specific **Changes** and **Verification** steps. - **Standard Tasks:** Include an **Objective**, **Key Files & Context**, **Implementation Steps**, and **Verification & Testing**. - **Complex Tasks:** Include **Background & Motivation**, **Scope & Impact**, **Proposed Solution**, **Alternatives Considered**, a phased **Implementation Plan**, **Verification**, and **Migration & Rollback** strategies. +- **Alignment Check:** After drafting the plan, you MUST present it to the user in the chat (adhering to Rule 7 for presenting plans) to ensure alignment on the specific details. Ask for feedback or confirmation, and proceed to Step 4 (Review & Approval) once the user agrees with the detailed plan. ### 4. Review & Approval ONLY use the \`exit_plan_mode\` tool to present the plan for formal approval AFTER you have reached an informal agreement with the user in the chat regarding the proposed strategy. When called, this tool will present the plan and formally request approval. diff --git a/packages/core/src/prompts/snippets.ts b/packages/core/src/prompts/snippets.ts index a2853c8964..c420f22ae3 100644 --- a/packages/core/src/prompts/snippets.ts +++ b/packages/core/src/prompts/snippets.ts @@ -586,6 +586,7 @@ ${options.planModeToolsList} - **Directives:** If the request is a **Directive** (e.g., "Fix bug Y"), follow the workflow below. 5. **Plan Storage:** Save plans as Markdown (.md) using descriptive filenames. 6. **Direct Modification:** If asked to modify code, explain you are in Plan Mode and use ${formatToolName(EXIT_PLAN_MODE_TOOL_NAME)} to request approval. +7. **Presenting Plan:** When seeking informal agreement on a plan, or any time the user asks to see the plan, you MUST output the full content of the plan in the chat response. This overrides the "Minimal Output" guideline. ## Planning Workflow Plan Mode uses an adaptive planning workflow where the research depth, plan structure, and consultation level are proportional to the task's complexity. @@ -605,7 +606,7 @@ The depth of your consultation should be proportional to the task's complexity. Write the implementation plan to \`${options.plansDir}/\`. The plan's structure adapts to the task: - **Simple Tasks:** Include a bulleted list of specific **Changes** and **Verification** steps. - **Standard Tasks:** Include an **Objective**, **Key Files & Context**, **Implementation Steps**, and **Verification & Testing**. -- **Complex Tasks:** Include **Background & Motivation**, **Scope & Impact**, **Proposed Solution**, **Alternatives Considered**, a phased **Implementation Plan**, **Verification**, and **Migration & Rollback** strategies. +- **Complex Tasks:** Include **Background & Motivation**, **Scope & Impact**, **Proposed Solution**, **Alternatives Considered**, a phased **Implementation Plan**, **Verification**, and **Migration & Rollback** strategies.${options.interactive ? '\n- **Alignment Check:** After drafting the plan, you MUST present it to the user in the chat (adhering to Rule 7 for presenting plans) to ensure alignment on the specific details. Ask for feedback or confirmation, and proceed to Step 4 (Review & Approval) once the user agrees with the detailed plan.' : ''} ### 4. Review & Approval ONLY use the ${formatToolName(EXIT_PLAN_MODE_TOOL_NAME)} tool to present the plan for formal approval AFTER you have reached an informal agreement with the user in the chat regarding the proposed strategy. When called, this tool will present the plan and ${options.interactive ? 'formally request approval.' : 'begin implementation.'} diff --git a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts index 0dc3c76f74..a431085c33 100644 --- a/packages/core/src/sandbox/linux/LinuxSandboxManager.ts +++ b/packages/core/src/sandbox/linux/LinuxSandboxManager.ts @@ -37,6 +37,7 @@ import { createSandboxDenialCache, type SandboxDenialCache, } from '../utils/sandboxDenialUtils.js'; +import { isErrnoException } from '../utils/fsUtils.js'; import { handleReadWriteCommands } from '../utils/sandboxReadWriteUtils.js'; import { buildBwrapArgs } from './bwrapArgsBuilder.js'; @@ -116,9 +117,12 @@ function touch(filePath: string, isDirectory: boolean) { assertValidPathString(filePath); try { // If it exists (even as a broken symlink), do nothing - if (fs.lstatSync(filePath)) return; - } catch { - // Ignore ENOENT + fs.lstatSync(filePath); + return; + } catch (e: unknown) { + if (isErrnoException(e) && e.code !== 'ENOENT') { + throw e; + } } if (isDirectory) { @@ -136,9 +140,22 @@ function touch(filePath: string, isDirectory: boolean) { export class LinuxSandboxManager implements SandboxManager { private static maskFilePath: string | undefined; private readonly denialCache: SandboxDenialCache = createSandboxDenialCache(); + private governanceFilesInitialized = false; constructor(private readonly options: GlobalSandboxOptions) {} + private ensureGovernanceFilesExist(workspace: string): void { + if (this.governanceFilesInitialized) return; + + // These must exist on the host before running the sandbox to ensure they are protected. + for (const file of GOVERNANCE_FILES) { + const filePath = join(workspace, file.path); + touch(filePath, file.isDirectory); + } + + this.governanceFilesInitialized = true; + } + isKnownSafeCommand(args: string[]): boolean { return isKnownSafeCommand(args); } @@ -258,17 +275,14 @@ export class LinuxSandboxManager implements SandboxManager { mergedAdditional, ); - for (const file of GOVERNANCE_FILES) { - const filePath = join(this.options.workspace, file.path); - touch(filePath, file.isDirectory); - } + this.ensureGovernanceFilesExist(resolvedPaths.workspace.resolved); const bwrapArgs = await buildBwrapArgs({ resolvedPaths, workspaceWrite, networkAccess: mergedAdditional.network ?? false, maskFilePath: this.getMaskFilePath(), - isWriteCommand: req.command === '__write', + isReadOnlyCommand: req.command === '__read', }); const bpfPath = getSeccompBpfPath(); diff --git a/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts b/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts index b9584062bc..6cff168d21 100644 --- a/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts +++ b/packages/core/src/sandbox/linux/bwrapArgsBuilder.test.ts @@ -92,7 +92,7 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => { workspaceWrite: false, networkAccess: false, maskFilePath: '/tmp/mask', - isWriteCommand: false, + isReadOnlyCommand: false, }; it('should correctly format the base arguments', async () => { @@ -188,7 +188,7 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => { expect(args[args.indexOf('/opt/tools') - 1]).toBe('--bind-try'); }); - it('should bind the parent directory of a non-existent path', async () => { + it('should bind the parent directory of a non-existent path with --bind-try when isReadOnlyCommand is false', async () => { vi.mocked(fs.existsSync).mockImplementation((p) => { if (p === '/home/user/workspace/new-file.txt') return false; return true; @@ -196,10 +196,10 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => { const args = await buildBwrapArgs({ ...defaultOptions, + isReadOnlyCommand: false, resolvedPaths: createResolvedPaths({ policyAllowed: ['/home/user/workspace/new-file.txt'], }), - isWriteCommand: true, }); const parentDir = '/home/user/workspace'; @@ -208,6 +208,26 @@ describe.skipIf(os.platform() === 'win32')('buildBwrapArgs', () => { expect(args[bindIndex - 2]).toBe('--bind-try'); }); + it('should bind the parent directory of a non-existent path with --ro-bind-try when isReadOnlyCommand is true', async () => { + vi.mocked(fs.existsSync).mockImplementation((p) => { + if (p === '/home/user/workspace/new-file.txt') return false; + return true; + }); + + const args = await buildBwrapArgs({ + ...defaultOptions, + isReadOnlyCommand: true, + resolvedPaths: createResolvedPaths({ + policyAllowed: ['/home/user/workspace/new-file.txt'], + }), + }); + + const parentDir = '/home/user/workspace'; + const bindIndex = args.lastIndexOf(parentDir); + expect(bindIndex).not.toBe(-1); + expect(args[bindIndex - 2]).toBe('--ro-bind-try'); + }); + it('should parameterize forbidden paths and explicitly deny them', async () => { vi.mocked(fs.statSync).mockImplementation((p) => { if (p.toString().includes('cache')) { diff --git a/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts b/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts index d7fec044e3..591bba8a0e 100644 --- a/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts +++ b/packages/core/src/sandbox/linux/bwrapArgsBuilder.ts @@ -23,7 +23,7 @@ export interface BwrapArgsOptions { workspaceWrite: boolean; networkAccess: boolean; maskFilePath: string; - isWriteCommand: boolean; + isReadOnlyCommand: boolean; } /** @@ -37,7 +37,7 @@ export async function buildBwrapArgs( workspaceWrite, networkAccess, maskFilePath, - isWriteCommand, + isReadOnlyCommand, } = options; const { workspace } = resolvedPaths; @@ -79,10 +79,13 @@ export async function buildBwrapArgs( bwrapArgs.push('--bind-try', allowedPath, allowedPath); } else { // If the path doesn't exist, we still want to allow access to its parent - // to enable creating it. Since allowedPath is already resolved by resolveSandboxPaths, - // its parent is also correctly resolved. + // to enable creating it. const parent = dirname(allowedPath); - bwrapArgs.push(isWriteCommand ? '--bind-try' : bindFlag, parent, parent); + bwrapArgs.push( + isReadOnlyCommand ? '--ro-bind-try' : '--bind-try', + parent, + parent, + ); } } diff --git a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts index 3e1862998e..963743a78d 100644 --- a/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts +++ b/packages/core/src/sandbox/macos/MacOsSandboxManager.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { MacOsSandboxManager } from './MacOsSandboxManager.js'; -import type { ExecutionPolicy } from '../../services/sandboxManager.js'; +import { type ExecutionPolicy } from '../../services/sandboxManager.js'; import * as seatbeltArgsBuilder from './seatbeltArgsBuilder.js'; import fs from 'node:fs'; import os from 'node:os'; @@ -191,28 +191,6 @@ describe('MacOsSandboxManager', () => { }); }); - describe('governance files', () => { - it('should ensure governance files exist', async () => { - await manager.prepareCommand({ - command: 'echo', - args: [], - cwd: mockWorkspace, - env: {}, - policy: mockPolicy, - }); - - // The seatbelt builder internally handles governance files, so we simply verify - // it is invoked correctly with the right workspace. - expect(seatbeltArgsBuilder.buildSeatbeltProfile).toHaveBeenCalledWith( - expect.objectContaining({ - resolvedPaths: expect.objectContaining({ - workspace: { resolved: mockWorkspace, original: mockWorkspace }, - }), - }), - ); - }); - }); - describe('allowedPaths', () => { it('should parameterize allowed paths and normalize them', async () => { await manager.prepareCommand({ diff --git a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts index 0f60f5906f..42fa196749 100644 --- a/packages/core/src/sandbox/windows/WindowsSandboxManager.ts +++ b/packages/core/src/sandbox/windows/WindowsSandboxManager.ts @@ -5,7 +5,7 @@ */ import fs from 'node:fs'; -import path from 'node:path'; +import path, { join } from 'node:path'; import os from 'node:os'; import { fileURLToPath } from 'node:url'; import { @@ -33,6 +33,7 @@ import { } from './commandSafety.js'; import { verifySandboxOverrides } from '../utils/commandUtils.js'; import { parseWindowsSandboxDenials } from './windowsSandboxDenialUtils.js'; +import { isErrnoException } from '../utils/fsUtils.js'; import { isSubpath, resolveToRealPath, @@ -53,10 +54,13 @@ const __dirname = path.dirname(__filename); */ export class WindowsSandboxManager implements SandboxManager { static readonly HELPER_EXE = 'GeminiSandbox.exe'; + private readonly helperPath: string; - private initialized = false; private readonly denialCache: SandboxDenialCache = createSandboxDenialCache(); + private static helperCompiled = false; + private governanceFilesInitialized = false; + constructor(private readonly options: GlobalSandboxOptions) { this.helperPath = path.resolve(__dirname, WindowsSandboxManager.HELPER_EXE); } @@ -86,33 +90,20 @@ export class WindowsSandboxManager implements SandboxManager { return this.options; } - /** - * Ensures a file or directory exists. - */ - private touch(filePath: string, isDirectory: boolean): void { - assertValidPathString(filePath); - try { - // If it exists (even as a broken symlink), do nothing - if (fs.lstatSync(filePath)) return; - } catch { - // Ignore ENOENT + private ensureGovernanceFilesExist(workspace: string): void { + if (this.governanceFilesInitialized) return; + + // These must exist on the host before running the sandbox to ensure they are protected. + for (const file of GOVERNANCE_FILES) { + const filePath = join(workspace, file.path); + touch(filePath, file.isDirectory); } - if (isDirectory) { - fs.mkdirSync(filePath, { recursive: true }); - } else { - const dir = path.dirname(filePath); - if (!fs.existsSync(dir)) { - fs.mkdirSync(dir, { recursive: true }); - } - fs.closeSync(fs.openSync(filePath, 'a')); - } + this.governanceFilesInitialized = true; } - private async ensureInitialized(): Promise { - if (this.initialized) return; - if (os.platform() !== 'win32') { - this.initialized = true; + private async ensureHelperCompiled(): Promise { + if (WindowsSandboxManager.helperCompiled || os.platform() !== 'win32') { return; } @@ -207,14 +198,14 @@ export class WindowsSandboxManager implements SandboxManager { ); } - this.initialized = true; + WindowsSandboxManager.helperCompiled = true; } /** * Prepares a command for sandboxed execution on Windows. */ async prepareCommand(req: SandboxRequest): Promise { - await this.ensureInitialized(); + await this.ensureHelperCompiled(); const sanitizationConfig = getSecureSanitizationConfig( req.policy?.sanitizationConfig, @@ -276,6 +267,8 @@ export class WindowsSandboxManager implements SandboxManager { mergedAdditional, ); + this.ensureGovernanceFilesExist(resolvedPaths.workspace.resolved); + // 1. Collect all forbidden paths. // We start with explicitly forbidden paths from the options and request. const forbiddenManifest = new Set( @@ -402,14 +395,6 @@ export class WindowsSandboxManager implements SandboxManager { // No-op for read access on Windows. } - // 4. Protected governance files - // These must exist on the host before running the sandbox to prevent - // the sandboxed process from creating them with Low integrity. - for (const file of GOVERNANCE_FILES) { - const filePath = path.join(resolvedPaths.workspace.resolved, file.path); - this.touch(filePath, file.isDirectory); - } - // 5. Generate Manifests const tempDir = await fs.promises.mkdtemp( path.join(os.tmpdir(), 'gemini-cli-sandbox-'), @@ -471,3 +456,29 @@ export class WindowsSandboxManager implements SandboxManager { ); } } + +/** + * Ensures a file or directory exists. + */ +function touch(filePath: string, isDirectory: boolean): void { + assertValidPathString(filePath); + try { + // If it exists (even as a broken symlink), do nothing + fs.lstatSync(filePath); + return; + } catch (e: unknown) { + if (isErrnoException(e) && e.code !== 'ENOENT') { + throw e; + } + } + + if (isDirectory) { + fs.mkdirSync(filePath, { recursive: true }); + } else { + const dir = path.dirname(filePath); + if (!fs.existsSync(dir)) { + fs.mkdirSync(dir, { recursive: true }); + } + fs.closeSync(fs.openSync(filePath, 'a')); + } +} diff --git a/packages/core/src/services/keychainService.test.ts b/packages/core/src/services/keychainService.test.ts index 7649271a02..8e88c1bf8b 100644 --- a/packages/core/src/services/keychainService.test.ts +++ b/packages/core/src/services/keychainService.test.ts @@ -53,7 +53,7 @@ vi.mock('../utils/events.js', () => ({ })); vi.mock('../utils/debugLogger.js', () => ({ - debugLogger: { log: vi.fn() }, + debugLogger: { debug: vi.fn() }, })); vi.mock('node:os', async (importOriginal) => { @@ -153,14 +153,14 @@ describe('KeychainService', () => { // Because it falls back to FileKeychain, it is always available. expect(available).toBe(true); - expect(debugLogger.log).toHaveBeenCalledWith( + expect(debugLogger.debug).toHaveBeenCalledWith( expect.stringContaining('encountered an error'), 'locked', ); expect(coreEvents.emitTelemetryKeychainAvailability).toHaveBeenCalledWith( expect.objectContaining({ available: false }), ); - expect(debugLogger.log).toHaveBeenCalledWith( + expect(debugLogger.debug).toHaveBeenCalledWith( expect.stringContaining('Using FileKeychain fallback'), ); expect(FileKeychain).toHaveBeenCalled(); @@ -173,7 +173,7 @@ describe('KeychainService', () => { const available = await service.isAvailable(); expect(available).toBe(true); - expect(debugLogger.log).toHaveBeenCalledWith( + expect(debugLogger.debug).toHaveBeenCalledWith( expect.stringContaining('failed structural validation'), expect.objectContaining({ getPassword: expect.any(Array) }), ); @@ -191,7 +191,7 @@ describe('KeychainService', () => { const available = await service.isAvailable(); expect(available).toBe(true); - expect(debugLogger.log).toHaveBeenCalledWith( + expect(debugLogger.debug).toHaveBeenCalledWith( expect.stringContaining('functional verification failed'), ); expect(FileKeychain).toHaveBeenCalled(); @@ -243,7 +243,7 @@ describe('KeychainService', () => { ); expect(mockKeytar.setPassword).not.toHaveBeenCalled(); expect(FileKeychain).toHaveBeenCalled(); - expect(debugLogger.log).toHaveBeenCalledWith( + expect(debugLogger.debug).toHaveBeenCalledWith( expect.stringContaining('MacOS default keychain not found'), ); }); diff --git a/packages/core/src/services/keychainService.ts b/packages/core/src/services/keychainService.ts index 89ec0dd662..0d12e68ffb 100644 --- a/packages/core/src/services/keychainService.ts +++ b/packages/core/src/services/keychainService.ts @@ -114,7 +114,7 @@ export class KeychainService { } // If native failed or was skipped, return the secure file fallback. - debugLogger.log('Using FileKeychain fallback for secure storage.'); + debugLogger.debug('Using FileKeychain fallback for secure storage.'); return new FileKeychain(); } @@ -130,7 +130,7 @@ export class KeychainService { // Probing macOS prevents process-blocking popups when no keychain exists. if (os.platform() === 'darwin' && !this.isMacOSKeychainAvailable()) { - debugLogger.log( + debugLogger.debug( 'MacOS default keychain not found; skipping functional verification.', ); return null; @@ -140,12 +140,15 @@ export class KeychainService { return keychainModule; } - debugLogger.log('Keychain functional verification failed'); + debugLogger.debug('Keychain functional verification failed'); return null; } catch (error) { // Avoid logging full error objects to prevent PII exposure. const message = error instanceof Error ? error.message : String(error); - debugLogger.log('Keychain initialization encountered an error:', message); + debugLogger.debug( + 'Keychain initialization encountered an error:', + message, + ); return null; } } @@ -162,7 +165,7 @@ export class KeychainService { return potential as Keychain; } - debugLogger.log( + debugLogger.debug( 'Keychain module failed structural validation:', result.error.flatten().fieldErrors, ); diff --git a/packages/core/src/services/sandboxManager.integration.test.ts b/packages/core/src/services/sandboxManager.integration.test.ts index 65adeaacbb..3481c53ca7 100644 --- a/packages/core/src/services/sandboxManager.integration.test.ts +++ b/packages/core/src/services/sandboxManager.integration.test.ts @@ -3,13 +3,22 @@ * Copyright 2026 Google LLC * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { + describe, + it, + expect, + beforeAll, + beforeEach, + afterEach, + afterAll, +} from 'vitest'; import { createSandboxManager } from './sandboxManagerFactory.js'; import { ShellExecutionService } from './shellExecutionService.js'; import { getSecureSanitizationConfig } from './environmentSanitization.js'; import { type SandboxManager, type SandboxedCommand, + GOVERNANCE_FILES, } from './sandboxManager.js'; import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; @@ -139,10 +148,10 @@ Status: ${result.status} (expected ${expected})${ } describe('SandboxManager Integration', () => { - const tempDirectories: string[] = []; + let tempDirectories: string[] = []; /** - * Creates a temporary directory. + * Creates a temporary directory and tracks it for automatic cleanup after each test. * - macOS: Created in process.cwd() to avoid the seatbelt profile's global os.tmpdir() whitelist. * - Win/Linux: Created in os.tmpdir() because enforcing sandbox restrictions inside a large directory can be very slow. */ @@ -159,12 +168,15 @@ describe('SandboxManager Integration', () => { let workspace: string; let manager: SandboxManager; - beforeAll(() => { + beforeEach(() => { + tempDirectories = []; + // Create a fresh, isolated workspace for every test to prevent state + // leakage from causing intermittent or order-dependent test failures. workspace = createTempDir('workspace-'); manager = createSandboxManager({ enabled: true }, { workspace }); }); - afterAll(() => { + afterEach(() => { for (const dir of tempDirectories) { try { fs.rmSync(dir, { recursive: true, force: true }); @@ -172,147 +184,330 @@ describe('SandboxManager Integration', () => { // Best-effort cleanup } } + tempDirectories = []; }); - describe('Basic Execution', () => { - it('executes commands within the workspace', async () => { - const { command, args } = Platform.echo('sandbox test'); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, + describe('Execution & Environment', () => { + describe('Basic Execution', () => { + it('allows workspace execution', async () => { + const { command, args } = Platform.echo('sandbox test'); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(result.stdout.trim()).toBe('sandbox test'); }); - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'success'); - expect(result.stdout.trim()).toBe('sandbox test'); + // The Windows sandbox wrapper (GeminiSandbox.exe) uses standard pipes + // for I/O interception, which breaks ConPTY pseudo-terminal inheritance. + it.skipIf(Platform.isWindows)( + 'supports interactive terminals', + async () => { + const handle = await ShellExecutionService.execute( + Platform.isPty(), + workspace, + () => {}, + new AbortController().signal, + true, + { + sanitizationConfig: getSecureSanitizationConfig(), + sandboxManager: manager, + }, + ); + + const result = await handle.result; + expect(result.exitCode).toBe(0); + expect(result.output).toContain('True'); + }, + ); }); - // The Windows sandbox wrapper (GeminiSandbox.exe) uses standard pipes - // for I/O interception, which breaks ConPTY pseudo-terminal inheritance. - it.skipIf(Platform.isWindows)( - 'supports interactive pseudo-terminals (node-pty)', - async () => { - const handle = await ShellExecutionService.execute( - Platform.isPty(), - workspace, - () => {}, - new AbortController().signal, - true, + describe('Virtual Commands', () => { + it('handles virtual read commands', async () => { + const testFile = path.join(workspace, 'read-virtual.txt'); + fs.writeFileSync(testFile, 'virtual read success'); + + const sandboxed = await manager.prepareCommand({ + command: '__read', + args: [testFile], + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(result.stdout.trim()).toBe('virtual read success'); + }); + + it('handles virtual write commands', async () => { + const testFile = path.join(workspace, 'write-virtual.txt'); + + const sandboxed = await manager.prepareCommand({ + command: '__write', + args: [testFile], + cwd: workspace, + env: process.env, + }); + + // Executing __write directly via runCommand hangs because 'cat' waits for stdin. + // Instead, we verify the command was translated correctly. + if (Platform.isWindows) { + // On Windows, the native helper handles '__write' + expect(sandboxed.args.includes('__write')).toBe(true); + } else { + // On macOS/Linux, it is translated to a shell command with 'tee -- "$@" > /dev/null' + expect(sandboxed.args.join(' ')).toContain('tee --'); + } + }); + }); + + describe('Environment Sanitization', () => { + it('scrubs sensitive environment variables', async () => { + const checkEnvCmd = { + command: process.execPath, + args: [ + '-e', + 'console.log(process.env.TEST_SECRET_TOKEN || "MISSING")', + ], + }; + + const sandboxed = await manager.prepareCommand({ + ...checkEnvCmd, + cwd: workspace, + env: { ...process.env, TEST_SECRET_TOKEN: 'super-secret-value' }, + policy: { + sanitizationConfig: { + enableEnvironmentVariableRedaction: true, + blockedEnvironmentVariables: ['TEST_SECRET_TOKEN'], + }, + }, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + // By default, environment sanitization drops non-allowlisted vars or vars that look like secrets. + // Assuming TEST_SECRET_TOKEN is scrubbed: + expect(result.stdout.trim()).toBe('MISSING'); + }); + }); + }); + + describe('Sandbox Policies & Modes', () => { + describe('Plan Mode Transitions', () => { + it('allows writing plans in plan mode', async () => { + // In Plan Mode, modeConfig sets readonly: true, allowOverrides: true + const planManager = createSandboxManager( + { enabled: true }, + { workspace, modeConfig: { readonly: true, allowOverrides: true } }, + ); + + const plansDir = path.join(workspace, '.gemini/tmp/session-123/plans'); + fs.mkdirSync(plansDir, { recursive: true }); + const planFile = path.join(plansDir, 'feature-plan.md'); + + // The WriteFile tool requests explicit write access for the plan file path + const { command, args } = Platform.touch(planFile); + + const sandboxed = await planManager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + policy: { allowedPaths: [plansDir] }, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(fs.existsSync(planFile)).toBe(true); + }); + + it('allows workspace writes after exiting plan mode', async () => { + // Upon exiting Plan Mode, the sandbox transitions to autoEdit/accepting_edits + // which sets readonly: false, allowOverrides: true + const editManager = createSandboxManager( + { enabled: true }, + { workspace, modeConfig: { readonly: false, allowOverrides: true } }, + ); + + const taskFile = path.join(workspace, 'src/tasks/task.ts'); + const taskDir = path.dirname(taskFile); + fs.mkdirSync(taskDir, { recursive: true }); + + // Simulate a generic edit anywhere in the workspace + const { command, args } = Platform.touch(taskFile); + + const sandboxed = await editManager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + policy: { allowedPaths: [taskDir] }, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(fs.existsSync(taskFile)).toBe(true); + }); + }); + + describe('Workspace Write Policies', () => { + it('enforces read-only mode', async () => { + const testFile = path.join(workspace, 'readonly-test.txt'); + const { command, args } = Platform.touch(testFile); + + const readonlyManager = createSandboxManager( + { enabled: true }, { - sanitizationConfig: getSecureSanitizationConfig(), - sandboxManager: manager, + workspace, + modeConfig: { readonly: true, allowOverrides: true }, }, ); - const result = await handle.result; - expect(result.exitCode).toBe(0); - expect(result.output).toContain('True'); - }, - ); + const sandboxed = await readonlyManager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }); + + it('allows writes for approved tools', async () => { + const testFile = path.join(workspace, 'approved-test.txt'); + const command = Platform.isWindows ? 'cmd.exe' : 'sh'; + const args = Platform.isWindows + ? ['/c', `echo test > ${testFile}`] + : ['-c', `echo test > "${testFile}"`]; + + // The shell wrapper is stripped by getCommandRoots, so the root command evaluated is 'echo' + const approvedTool = 'echo'; + + const approvedManager = createSandboxManager( + { enabled: true }, + { + workspace, + modeConfig: { + readonly: true, + allowOverrides: true, + approvedTools: [approvedTool], + }, + }, + ); + + const sandboxed = await approvedManager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(fs.existsSync(testFile)).toBe(true); + }); + + it('allows writes in YOLO mode', async () => { + const testFile = path.join(workspace, 'yolo-test.txt'); + const { command, args } = Platform.touch(testFile); + + const yoloManager = createSandboxManager( + { enabled: true }, + { + workspace, + modeConfig: { readonly: true, yolo: true, allowOverrides: true }, + }, + ); + + const sandboxed = await yoloManager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(fs.existsSync(testFile)).toBe(true); + }); + }); }); - describe('File System Access', () => { - it('blocks access outside the workspace', async () => { - const blockedPath = Platform.getExternalBlockedPath(); - const { command, args } = Platform.touch(blockedPath); + describe('File System Security', () => { + describe('File System Access', () => { + it('prevents out-of-bounds access', async () => { + const blockedPath = Platform.getExternalBlockedPath(); + const { command, args } = Platform.touch(blockedPath); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); }); - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'failure'); - }); + it('supports dynamic permission expansion', async () => { + const tempDir = createTempDir('expansion-'); + const testFile = path.join(tempDir, 'test.txt'); + const { command, args } = Platform.touch(testFile); - it('allows dynamic expansion of permissions after a failure', async () => { - const tempDir = createTempDir('expansion-'); - const testFile = path.join(tempDir, 'test.txt'); - const { command, args } = Platform.touch(testFile); + // First attempt: fails due to sandbox restrictions + const sandboxed1 = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + const result1 = await runCommand(sandboxed1); + assertResult(result1, sandboxed1, 'failure'); + expect(fs.existsSync(testFile)).toBe(false); - // First attempt: fails due to sandbox restrictions - const sandboxed1 = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - }); - const result1 = await runCommand(sandboxed1); - assertResult(result1, sandboxed1, 'failure'); - expect(fs.existsSync(testFile)).toBe(false); - - // Second attempt: succeeds with additional permissions - const sandboxed2 = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - policy: { allowedPaths: [tempDir] }, - }); - const result2 = await runCommand(sandboxed2); - assertResult(result2, sandboxed2, 'success'); - expect(fs.existsSync(testFile)).toBe(true); - }); - - it('grants access to explicitly allowed paths', async () => { - const allowedDir = createTempDir('allowed-'); - const testFile = path.join(allowedDir, 'test.txt'); - - const { command, args } = Platform.touch(testFile); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - policy: { allowedPaths: [allowedDir] }, + // Second attempt: succeeds with additional permissions + const sandboxed2 = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + policy: { allowedPaths: [tempDir] }, + }); + const result2 = await runCommand(sandboxed2); + assertResult(result2, sandboxed2, 'success'); + expect(fs.existsSync(testFile)).toBe(true); }); - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'success'); - expect(fs.existsSync(testFile)).toBe(true); - }); + it('allows access to authorized paths', async () => { + const allowedDir = createTempDir('allowed-'); + const testFile = path.join(allowedDir, 'test.txt'); - it('blocks write access to forbidden paths within the workspace', async () => { - const tempWorkspace = createTempDir('workspace-'); - const forbiddenDir = path.join(tempWorkspace, 'forbidden'); - const testFile = path.join(forbiddenDir, 'test.txt'); - fs.mkdirSync(forbiddenDir); + const { command, args } = Platform.touch(testFile); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + policy: { allowedPaths: [allowedDir] }, + }); - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [forbiddenDir], - }, - ); - const { command, args } = Platform.touch(testFile); - - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(fs.existsSync(testFile)).toBe(true); }); - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'failure'); - }); - - // Windows icacls does not reliably block read-up access for Low Integrity - // processes, so we skip read-specific assertions on Windows. The internal - // tool architecture prevents read bypasses via the C# wrapper and __read. - it.skipIf(Platform.isWindows)( - 'blocks read access to forbidden paths within the workspace', - async () => { + it('protects forbidden paths from writes', async () => { const tempWorkspace = createTempDir('workspace-'); const forbiddenDir = path.join(tempWorkspace, 'forbidden'); const testFile = path.join(forbiddenDir, 'test.txt'); fs.mkdirSync(forbiddenDir); - fs.writeFileSync(testFile, 'secret data'); const osManager = createSandboxManager( { enabled: true }, @@ -321,8 +516,7 @@ describe('SandboxManager Integration', () => { forbiddenPaths: async () => [forbiddenDir], }, ); - - const { command, args } = Platform.cat(testFile); + const { command, args } = Platform.touch(testFile); const sandboxed = await osManager.prepareCommand({ command, @@ -333,333 +527,403 @@ describe('SandboxManager Integration', () => { const result = await runCommand(sandboxed); assertResult(result, sandboxed, 'failure'); - }, - ); + }); - it('blocks access to files inside forbidden directories recursively', async () => { - const tempWorkspace = createTempDir('workspace-'); - const forbiddenDir = path.join(tempWorkspace, 'forbidden'); - const nestedDir = path.join(forbiddenDir, 'nested'); - const nestedFile = path.join(nestedDir, 'test.txt'); + // Windows icacls does not reliably block read-up access for Low Integrity + // processes, so we skip read-specific assertions on Windows. The internal + // tool architecture prevents read bypasses via the C# wrapper and __read. + it.skipIf(Platform.isWindows)( + 'protects forbidden paths from reads', + async () => { + const tempWorkspace = createTempDir('workspace-'); + const forbiddenDir = path.join(tempWorkspace, 'forbidden'); + const testFile = path.join(forbiddenDir, 'test.txt'); + fs.mkdirSync(forbiddenDir); + fs.writeFileSync(testFile, 'secret data'); - // Create the base forbidden directory first so the manager can restrict access to it. - fs.mkdirSync(forbiddenDir); + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [forbiddenDir], + }, + ); - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [forbiddenDir], + const { command, args } = Platform.cat(testFile); + + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); }, ); - // Execute a dummy command so the manager initializes its restrictions. - const dummyCommand = await osManager.prepareCommand({ - ...Platform.echo('init'), - cwd: tempWorkspace, - env: process.env, - }); - await runCommand(dummyCommand); + it('protects forbidden directories recursively', async () => { + const tempWorkspace = createTempDir('workspace-'); + const forbiddenDir = path.join(tempWorkspace, 'forbidden'); + const nestedDir = path.join(forbiddenDir, 'nested'); + const nestedFile = path.join(nestedDir, 'test.txt'); - // Now create the nested items. They will inherit the sandbox restrictions from their parent. - fs.mkdirSync(nestedDir, { recursive: true }); - fs.writeFileSync(nestedFile, 'secret'); + // Create the base forbidden directory first so the manager can restrict access to it. + fs.mkdirSync(forbiddenDir); - const { command, args } = Platform.touch(nestedFile); + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [forbiddenDir], + }, + ); - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, + // Execute a dummy command so the manager initializes its restrictions. + const dummyCommand = await osManager.prepareCommand({ + ...Platform.echo('init'), + cwd: tempWorkspace, + env: process.env, + }); + await runCommand(dummyCommand); + + // Now create the nested items. They will inherit the sandbox restrictions from their parent. + fs.mkdirSync(nestedDir, { recursive: true }); + fs.writeFileSync(nestedFile, 'secret'); + + const { command, args } = Platform.touch(nestedFile); + + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); }); - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'failure'); + it('prioritizes denials over allowances', async () => { + const tempWorkspace = createTempDir('workspace-'); + const conflictDir = path.join(tempWorkspace, 'conflict'); + const testFile = path.join(conflictDir, 'test.txt'); + fs.mkdirSync(conflictDir); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [conflictDir], + }, + ); + const { command, args } = Platform.touch(testFile); + + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, + policy: { + allowedPaths: [conflictDir], + }, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }); + + it('handles missing paths gracefully', async () => { + const tempWorkspace = createTempDir('workspace-'); + const nonExistentPath = path.join(tempWorkspace, 'does-not-exist'); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [nonExistentPath], + }, + ); + const { command, args } = Platform.echo('survived'); + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: tempWorkspace, + env: process.env, + policy: { + allowedPaths: [nonExistentPath], + }, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(result.stdout.trim()).toBe('survived'); + }); + + it('prevents creation of forbidden files', async () => { + const tempWorkspace = createTempDir('workspace-'); + const nonExistentFile = path.join(tempWorkspace, 'never-created.txt'); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [nonExistentFile], + }, + ); + + // We use touch to attempt creation of the file + const { command: cmdTouch, args: argsTouch } = + Platform.touch(nonExistentFile); + + const sandboxedCmd = await osManager.prepareCommand({ + command: cmdTouch, + args: argsTouch, + cwd: tempWorkspace, + env: process.env, + }); + + // Execute the command, we expect it to fail (permission denied or read-only file system) + const result = await runCommand(sandboxedCmd); + + assertResult(result, sandboxedCmd, 'failure'); + expect(fs.existsSync(nonExistentFile)).toBe(false); + }); + + it('restricts symlinks to forbidden targets', async () => { + const tempWorkspace = createTempDir('workspace-'); + const targetFile = path.join(tempWorkspace, 'target.txt'); + const symlinkFile = path.join(tempWorkspace, 'link.txt'); + + fs.writeFileSync(targetFile, 'secret data'); + fs.symlinkSync(targetFile, symlinkFile); + + const osManager = createSandboxManager( + { enabled: true }, + { + workspace: tempWorkspace, + forbiddenPaths: async () => [symlinkFile], + }, + ); + + // Attempt to write to the target file directly + const { command: cmdTarget, args: argsTarget } = + Platform.touch(targetFile); + const commandTarget = await osManager.prepareCommand({ + command: cmdTarget, + args: argsTarget, + cwd: tempWorkspace, + env: process.env, + }); + + const resultTarget = await runCommand(commandTarget); + assertResult(resultTarget, commandTarget, 'failure'); + + // Attempt to write via the symlink + const { command: cmdLink, args: argsLink } = + Platform.touch(symlinkFile); + const commandLink = await osManager.prepareCommand({ + command: cmdLink, + args: argsLink, + cwd: tempWorkspace, + env: process.env, + }); + + const resultLink = await runCommand(commandLink); + assertResult(resultLink, commandLink, 'failure'); + }); }); - it('prioritizes forbiddenPaths over allowedPaths', async () => { - const tempWorkspace = createTempDir('workspace-'); - const conflictDir = path.join(tempWorkspace, 'conflict'); - const testFile = path.join(conflictDir, 'test.txt'); - fs.mkdirSync(conflictDir); + describe('Governance Files', () => { + it('prevents modification of governance files', async () => { + // Ensure workspace is initialized and governance files are created + const { command: echoCmd, args: echoArgs } = Platform.echo('test'); + await manager.prepareCommand({ + command: echoCmd, + args: echoArgs, + cwd: workspace, + env: process.env, + // Even if the entire workspace is explicitly allowed, governance files must be protected + policy: { allowedPaths: [workspace] }, + }); - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [conflictDir], - }, - ); - const { command, args } = Platform.touch(testFile); + for (const file of GOVERNANCE_FILES) { + const filePath = path.join(workspace, file.path); + // Try to append to/overwrite the file or create a file inside the directory + const { command, args } = file.isDirectory + ? Platform.touch(path.join(filePath, 'evil.txt')) + : Platform.touch(filePath); - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, - policy: { - allowedPaths: [conflictDir], - }, + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + } }); - - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'failure'); }); - it('gracefully ignores non-existent paths in allowedPaths and forbiddenPaths', async () => { - const tempWorkspace = createTempDir('workspace-'); - const nonExistentPath = path.join(tempWorkspace, 'does-not-exist'); + describe('Git Worktree Support', () => { + it('supports git worktrees', async () => { + const mainRepo = createTempDir('main-repo-'); + const worktreeDir = createTempDir('worktree-'); - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [nonExistentPath], - }, - ); - const { command, args } = Platform.echo('survived'); - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: tempWorkspace, - env: process.env, - policy: { - allowedPaths: [nonExistentPath], - }, + const mainGitDir = path.join(mainRepo, '.git'); + fs.mkdirSync(mainGitDir, { recursive: true }); + fs.writeFileSync( + path.join(mainGitDir, 'config'), + '[core]\n\trepositoryformatversion = 0\n', + ); + + const worktreeGitDir = path.join( + mainGitDir, + 'worktrees', + 'test-worktree', + ); + fs.mkdirSync(worktreeGitDir, { recursive: true }); + + // Create the .git file in the worktree directory pointing to the worktree git dir + fs.writeFileSync( + path.join(worktreeDir, '.git'), + `gitdir: ${worktreeGitDir}\n`, + ); + + // Create the backlink from worktree git dir to the worktree's .git file + const backlinkPath = path.join(worktreeGitDir, 'gitdir'); + fs.writeFileSync(backlinkPath, path.join(worktreeDir, '.git')); + + // Create a file in the worktree git dir that we want to access + const secretFile = path.join(worktreeGitDir, 'secret.txt'); + fs.writeFileSync(secretFile, 'git-secret'); + + const osManager = createSandboxManager( + { enabled: true }, + { workspace: worktreeDir }, + ); + + const { command, args } = Platform.cat(secretFile); + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: worktreeDir, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'success'); + expect(result.stdout.trim()).toBe('git-secret'); }); - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'success'); - expect(result.stdout.trim()).toBe('survived'); - }); + it('protects git worktree metadata', async () => { + const mainRepo = createTempDir('main-repo-'); + const worktreeDir = createTempDir('worktree-'); - it('prevents creation of non-existent forbidden paths', async () => { - const tempWorkspace = createTempDir('workspace-'); - const nonExistentFile = path.join(tempWorkspace, 'never-created.txt'); + const mainGitDir = path.join(mainRepo, '.git'); + fs.mkdirSync(mainGitDir, { recursive: true }); - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [nonExistentFile], - }, - ); + const worktreeGitDir = path.join( + mainGitDir, + 'worktrees', + 'test-worktree', + ); + fs.mkdirSync(worktreeGitDir, { recursive: true }); - // We use touch to attempt creation of the file - const { command: cmdTouch, args: argsTouch } = - Platform.touch(nonExistentFile); + fs.writeFileSync( + path.join(worktreeDir, '.git'), + `gitdir: ${worktreeGitDir}\n`, + ); + fs.writeFileSync( + path.join(worktreeGitDir, 'gitdir'), + path.join(worktreeDir, '.git'), + ); - const sandboxedCmd = await osManager.prepareCommand({ - command: cmdTouch, - args: argsTouch, - cwd: tempWorkspace, - env: process.env, + const targetFile = path.join(worktreeGitDir, 'secret.txt'); + + const osManager = createSandboxManager( + { enabled: true }, + // Use YOLO mode to ensure the workspace is fully writable, but git worktrees should still be read-only + { workspace: worktreeDir, modeConfig: { yolo: true } }, + ); + + const { command, args } = Platform.touch(targetFile); + const sandboxed = await osManager.prepareCommand({ + command, + args, + cwd: worktreeDir, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + expect(fs.existsSync(targetFile)).toBe(false); }); - - // Execute the command, we expect it to fail (permission denied or read-only file system) - const result = await runCommand(sandboxedCmd); - - assertResult(result, sandboxedCmd, 'failure'); - expect(fs.existsSync(nonExistentFile)).toBe(false); - }); - - it('blocks access to both a symlink and its target when the symlink is forbidden', async () => { - const tempWorkspace = createTempDir('workspace-'); - const targetFile = path.join(tempWorkspace, 'target.txt'); - const symlinkFile = path.join(tempWorkspace, 'link.txt'); - - fs.writeFileSync(targetFile, 'secret data'); - fs.symlinkSync(targetFile, symlinkFile); - - const osManager = createSandboxManager( - { enabled: true }, - { - workspace: tempWorkspace, - forbiddenPaths: async () => [symlinkFile], - }, - ); - - // Attempt to write to the target file directly - const { command: cmdTarget, args: argsTarget } = - Platform.touch(targetFile); - const commandTarget = await osManager.prepareCommand({ - command: cmdTarget, - args: argsTarget, - cwd: tempWorkspace, - env: process.env, - }); - - const resultTarget = await runCommand(commandTarget); - assertResult(resultTarget, commandTarget, 'failure'); - - // Attempt to write via the symlink - const { command: cmdLink, args: argsLink } = Platform.touch(symlinkFile); - const commandLink = await osManager.prepareCommand({ - command: cmdLink, - args: argsLink, - cwd: tempWorkspace, - env: process.env, - }); - - const resultLink = await runCommand(commandLink); - assertResult(resultLink, commandLink, 'failure'); }); }); - describe('Git Worktree Support', () => { - it('allows access to git common directory in a worktree', async () => { - const mainRepo = createTempDir('main-repo-'); - const worktreeDir = createTempDir('worktree-'); + describe('Network Security', () => { + describe('Network Access', () => { + let server: http.Server; + let url: string; - const mainGitDir = path.join(mainRepo, '.git'); - fs.mkdirSync(mainGitDir, { recursive: true }); - fs.writeFileSync( - path.join(mainGitDir, 'config'), - '[core]\n\trepositoryformatversion = 0\n', - ); - - const worktreeGitDir = path.join( - mainGitDir, - 'worktrees', - 'test-worktree', - ); - fs.mkdirSync(worktreeGitDir, { recursive: true }); - - // Create the .git file in the worktree directory pointing to the worktree git dir - fs.writeFileSync( - path.join(worktreeDir, '.git'), - `gitdir: ${worktreeGitDir}\n`, - ); - - // Create the backlink from worktree git dir to the worktree's .git file - const backlinkPath = path.join(worktreeGitDir, 'gitdir'); - fs.writeFileSync(backlinkPath, path.join(worktreeDir, '.git')); - - // Create a file in the worktree git dir that we want to access - const secretFile = path.join(worktreeGitDir, 'secret.txt'); - fs.writeFileSync(secretFile, 'git-secret'); - - const osManager = createSandboxManager( - { enabled: true }, - { workspace: worktreeDir }, - ); - - const { command, args } = Platform.cat(secretFile); - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: worktreeDir, - env: process.env, - }); - - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'success'); - expect(result.stdout.trim()).toBe('git-secret'); - }); - - it('blocks write access to git common directory in a worktree', async () => { - const mainRepo = createTempDir('main-repo-'); - const worktreeDir = createTempDir('worktree-'); - - const mainGitDir = path.join(mainRepo, '.git'); - fs.mkdirSync(mainGitDir, { recursive: true }); - - const worktreeGitDir = path.join( - mainGitDir, - 'worktrees', - 'test-worktree', - ); - fs.mkdirSync(worktreeGitDir, { recursive: true }); - - fs.writeFileSync( - path.join(worktreeDir, '.git'), - `gitdir: ${worktreeGitDir}\n`, - ); - fs.writeFileSync( - path.join(worktreeGitDir, 'gitdir'), - path.join(worktreeDir, '.git'), - ); - - const targetFile = path.join(worktreeGitDir, 'secret.txt'); - - const osManager = createSandboxManager( - { enabled: true }, - // Use YOLO mode to ensure the workspace is fully writable, but git worktrees should still be read-only - { workspace: worktreeDir, modeConfig: { yolo: true } }, - ); - - const { command, args } = Platform.touch(targetFile); - const sandboxed = await osManager.prepareCommand({ - command, - args, - cwd: worktreeDir, - env: process.env, - }); - - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'failure'); - expect(fs.existsSync(targetFile)).toBe(false); - }); - }); - - describe('Network Access', () => { - let server: http.Server; - let url: string; - - beforeAll(async () => { - server = http.createServer((_, res) => { - res.setHeader('Connection', 'close'); - res.writeHead(200); - res.end('ok'); - }); - await new Promise((resolve, reject) => { - server.on('error', reject); - server.listen(0, '127.0.0.1', () => { - const addr = server.address() as import('net').AddressInfo; - url = `http://127.0.0.1:${addr.port}`; - resolve(); + beforeAll(async () => { + server = http.createServer((_, res) => { + res.setHeader('Connection', 'close'); + res.writeHead(200); + res.end('ok'); + }); + await new Promise((resolve, reject) => { + server.on('error', reject); + server.listen(0, '127.0.0.1', () => { + const addr = server.address() as import('net').AddressInfo; + url = `http://127.0.0.1:${addr.port}`; + resolve(); + }); }); }); - }); - afterAll(async () => { - if (server) await new Promise((res) => server.close(() => res())); - }); + afterAll(async () => { + if (server) await new Promise((res) => server.close(() => res())); + }); - // Windows Job Object rate limits exempt loopback (127.0.0.1) traffic, - // so this test cannot verify loopback blocking on Windows. - it.skipIf(Platform.isWindows)( - 'blocks network access by default', - async () => { + // Windows Job Object rate limits exempt loopback (127.0.0.1) traffic, + // so this test cannot verify loopback blocking on Windows. + it.skipIf(Platform.isWindows)( + 'prevents unauthorized network access', + async () => { + const { command, args } = Platform.curl(url); + const sandboxed = await manager.prepareCommand({ + command, + args, + cwd: workspace, + env: process.env, + }); + + const result = await runCommand(sandboxed); + assertResult(result, sandboxed, 'failure'); + }, + ); + + it('allows authorized network access', async () => { const { command, args } = Platform.curl(url); const sandboxed = await manager.prepareCommand({ command, args, cwd: workspace, env: process.env, + policy: { networkAccess: true }, }); const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'failure'); - }, - ); - - it('grants network access when explicitly allowed', async () => { - const { command, args } = Platform.curl(url); - const sandboxed = await manager.prepareCommand({ - command, - args, - cwd: workspace, - env: process.env, - policy: { networkAccess: true }, + assertResult(result, sandboxed, 'success'); + if (!Platform.isWindows) { + expect(result.stdout.trim()).toBe('ok'); + } }); - - const result = await runCommand(sandboxed); - assertResult(result, sandboxed, 'success'); - if (!Platform.isWindows) { - expect(result.stdout.trim()).toBe('ok'); - } }); }); }); diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index 4a14b671a0..50b17aa735 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -20,6 +20,8 @@ import { MCPOAuthTokenStorage } from '../mcp/oauth-token-storage.js'; import { OAuthUtils } from '../mcp/oauth-utils.js'; import type { PromptRegistry } from '../prompts/prompt-registry.js'; import { + ErrorCode, + McpError, PromptListChangedNotificationSchema, ResourceListChangedNotificationSchema, ToolListChangedNotificationSchema, @@ -35,6 +37,7 @@ import { isEnabled, McpClient, populateMcpServerCommand, + discoverPrompts, type McpContext, } from './mcp-client.js'; import type { ToolRegistry } from './tool-registry.js'; @@ -320,6 +323,25 @@ describe('mcp-client', () => { ); }); + it('should return empty array for discoverPrompts on MethodNotFound error without diagnostic', async () => { + const mockedClient = { + getServerCapabilities: vi.fn().mockReturnValue({ prompts: {} }), + listPrompts: vi + .fn() + .mockRejectedValue( + new McpError(ErrorCode.MethodNotFound, 'Method not supported'), + ), + }; + const result = await discoverPrompts( + 'test-server', + mockedClient as unknown as ClientLib.Client, + MOCK_CONTEXT, + ); + expect(result).toEqual([]); + // MethodNotFound errors should be silently ignored regardless of message text + expect(MOCK_CONTEXT.emitMcpDiagnostic).not.toHaveBeenCalled(); + }); + it('should not discover tools if server does not support them', async () => { const mockedClient = { connect: vi.fn(), diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index a7852050fc..0441063f81 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -27,6 +27,8 @@ import { ReadResourceResultSchema, ResourceListChangedNotificationSchema, ToolListChangedNotificationSchema, + ErrorCode, + McpError, PromptListChangedNotificationSchema, ProgressNotificationSchema, type GetPromptResult, @@ -1250,6 +1252,10 @@ export async function connectAndDiscover( } } +function isMcpMethodNotFoundError(error: unknown): boolean { + return error instanceof McpError && error.code === ErrorCode.MethodNotFound; +} + /** * Discovers and sanitizes tools from a connected MCP client. * It retrieves function declarations from the client, filters out disabled tools, @@ -1329,10 +1335,7 @@ export async function discoverTools( } return discoveredTools; } catch (error) { - if ( - error instanceof Error && - !error.message?.includes('Method not found') - ) { + if (!isMcpMethodNotFoundError(error)) { cliConfig.emitMcpDiagnostic( 'error', `Error discovering tools from ${mcpServerName}: ${getErrorMessage( @@ -1456,8 +1459,7 @@ export async function discoverPrompts( ), })); } catch (error) { - // It's okay if the method is not found, which is a common case. - if (error instanceof Error && error.message?.includes('Method not found')) { + if (isMcpMethodNotFoundError(error)) { return []; } cliConfig.emitMcpDiagnostic( @@ -1505,7 +1507,7 @@ async function listResources( cursor = response.nextCursor ?? undefined; } while (cursor); } catch (error) { - if (error instanceof Error && error.message?.includes('Method not found')) { + if (isMcpMethodNotFoundError(error)) { return []; } cliConfig.emitMcpDiagnostic( diff --git a/packages/core/src/utils/compatibility.test.ts b/packages/core/src/utils/compatibility.test.ts index 28fa26453c..f3ac7e35e5 100644 --- a/packages/core/src/utils/compatibility.test.ts +++ b/packages/core/src/utils/compatibility.test.ts @@ -195,6 +195,7 @@ describe('compatibility', () => { desc: '256 colors are not supported', }, ])('should return $expected when $desc', ({ depth, term, expected }) => { + vi.stubEnv('COLORTERM', ''); process.stdout.getColorDepth = vi.fn().mockReturnValue(depth); if (term !== undefined) { vi.stubEnv('TERM', term); @@ -203,6 +204,13 @@ describe('compatibility', () => { } expect(supports256Colors()).toBe(expected); }); + + it('should return true when COLORTERM is kmscon', () => { + process.stdout.getColorDepth = vi.fn().mockReturnValue(4); + vi.stubEnv('TERM', 'linux'); + vi.stubEnv('COLORTERM', 'kmscon'); + expect(supports256Colors()).toBe(true); + }); }); describe('supportsTrueColor', () => { @@ -230,6 +238,12 @@ describe('compatibility', () => { expected: true, desc: 'getColorDepth returns >= 24', }, + { + colorterm: 'kmscon', + depth: 4, + expected: true, + desc: 'COLORTERM is kmscon', + }, { colorterm: '', depth: 8, @@ -409,6 +423,18 @@ describe('compatibility', () => { ); }); + it('should return no color warnings for kmscon terminal', () => { + vi.mocked(os.platform).mockReturnValue('linux'); + vi.stubEnv('TERMINAL_EMULATOR', ''); + vi.stubEnv('TERM', 'linux'); + vi.stubEnv('COLORTERM', 'kmscon'); + process.stdout.getColorDepth = vi.fn().mockReturnValue(4); + + const warnings = getCompatibilityWarnings(); + expect(warnings.find((w) => w.id === '256-color')).toBeUndefined(); + expect(warnings.find((w) => w.id === 'true-color')).toBeUndefined(); + }); + it('should return no warnings in a standard environment with true color', () => { vi.mocked(os.platform).mockReturnValue('darwin'); vi.stubEnv('TERMINAL_EMULATOR', ''); diff --git a/packages/core/src/utils/compatibility.ts b/packages/core/src/utils/compatibility.ts index 8a997b42cf..47ef078831 100644 --- a/packages/core/src/utils/compatibility.ts +++ b/packages/core/src/utils/compatibility.ts @@ -85,6 +85,11 @@ export function supports256Colors(): boolean { return true; } + // Terminals supporting true color (like kmscon) also support 256 colors + if (supportsTrueColor()) { + return true; + } + return false; } @@ -95,7 +100,8 @@ export function supportsTrueColor(): boolean { // Check COLORTERM environment variable if ( process.env['COLORTERM'] === 'truecolor' || - process.env['COLORTERM'] === '24bit' + process.env['COLORTERM'] === '24bit' || + process.env['COLORTERM'] === 'kmscon' ) { return true; } diff --git a/perf-tests/baselines.json b/perf-tests/baselines.json index 1dd52a5213..d6972342d4 100644 --- a/perf-tests/baselines.json +++ b/perf-tests/baselines.json @@ -12,6 +12,11 @@ "cpuTotalUs": 12157, "timestamp": "2026-04-08T22:28:19.098Z" }, + "asian-language-conv": { + "wallClockMs": 2315.1, + "cpuTotalUs": 6283, + "timestamp": "2026-04-14T15:22:56.133Z" + }, "skill-loading-time": { "wallClockMs": 930.0920409999962, "cpuTotalUs": 1323, diff --git a/perf-tests/perf-usage.test.ts b/perf-tests/perf-usage.test.ts index 1a361eda5d..4bbc5ab0ea 100644 --- a/perf-tests/perf-usage.test.ts +++ b/perf-tests/perf-usage.test.ts @@ -98,6 +98,36 @@ describe('CPU Performance Tests', () => { } }); + it('asian-language-conv: verify perf is acceptable ', async () => { + const result = await harness.runScenario( + 'asian-language-conv', + async () => { + const rig = new TestRig(); + try { + rig.setup('perf-asian-language', { + fakeResponsesPath: join(__dirname, 'perf.asian-language.responses'), + }); + + return await harness.measure('asian-language', async () => { + await rig.run({ + args: ['嗨'], + timeout: 120000, + env: { GEMINI_API_KEY: 'fake-perf-test-key' }, + }); + }); + } finally { + await rig.cleanup(); + } + }, + ); + + if (UPDATE_BASELINES) { + harness.updateScenarioBaseline(result); + } else { + harness.assertWithinBaseline(result); + } + }); + it('skill-loading-time: startup with many skills within baseline', async () => { const SKILL_COUNT = 20; diff --git a/perf-tests/perf.asian-language.responses b/perf-tests/perf.asian-language.responses new file mode 100644 index 0000000000..8f3c71775b --- /dev/null +++ b/perf-tests/perf.asian-language.responses @@ -0,0 +1,2 @@ +{"method":"generateContent","response":{"candidates":[{"content":{"parts":[{"text":"0"}],"role":"model"},"finishReason":"STOP","index":0}]}} +{"method":"generateContentStream","response":[{"candidates":[{"content":{"parts":[{"text":"你好！我是 Gemini CLI，你的 AI 编程助手"}],"role":"model"},"finishReason":"STOP","index":0}],"usageMetadata":{"promptTokenCount":20648,"candidatesTokenCount":12,"totalTokenCount":20769,"promptTokensDetails":[{"modality":"TEXT","tokenCount":5}]}}]}