diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index bee1ce9c9..a806423f1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -104,8 +104,11 @@ jobs: - name: Build run: bun run build + # Same layout PR Checks validates: eight unit shards plus the dedicated + # conversation-arc and context-efficiency passes. A single-process run + # here hit cross-file mock leaks that no PR run could surface. - name: Run unit tests - run: bun run test + run: bun run test:full - name: Smoke test run: bun run smoke diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1edcc9b43..cfa6b0029 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -267,6 +267,14 @@ only. Structured diff rendering in `src/native-ts/color-diff/index.ts` loads `highlight.js` separately. Run the highlighting and Markdown regression tests when changing this dependency or its ANSI renderer. +Keep `wrap-ansi` pinned to `9.0.2`. Version 10 expands tabs into spaces, and +`Cursor` (`src/utils/Cursor.ts`) and Ink's style mapping expect the wrapped +text to keep the original characters; with v10, a tab in a text input throws +and exits the session. Only the Node runtime loads the npm package: under Bun, +`src/ink/wrapAnsi.ts` uses `Bun.wrapAnsi`, so tests never exercise it. +`src/ink/wrapAnsi.test.ts` asserts the npm package keeps tabs. Adapt the tab +handling before upgrading past 9.x. + Keep all direct dependencies and devDependencies pinned to exact versions in the root and web manifests. Regenerate the relevant Bun lockfile when changing a pin. diff --git a/Dockerfile b/Dockerfile index e12ba30b6..8aea1b8f1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -5,6 +5,8 @@ WORKDIR /app # Copy dependency manifests first for better layer caching COPY package.json bun.lock .bun-version ./ +# The node-domexception override in package.json resolves to this local shim +COPY vendor/ vendor/ # Install the Bun version tracked by the repo RUN set -eu; \ diff --git a/bun.lock b/bun.lock index eaa3ff9d5..d7ba3cfaf 100644 --- a/bun.lock +++ b/bun.lock @@ -84,7 +84,7 @@ "usehooks-ts": "3.1.1", "vscode-languageserver-protocol": "3.18.5", "web-tree-sitter": "0.25.10", - "wrap-ansi": "10.0.2", + "wrap-ansi": "9.0.2", "ws": "8.22.0", "xss": "1.0.16", "yaml": "2.9.1", @@ -1144,7 +1144,7 @@ "statuses": ["statuses@2.0.2", "", {}, "sha512-DvEy55V3DB7uknRo+4iOGT5fP1slR8wQohVdknigZPMpMstaKJQWhwiYBACJE3Ul2pTnATihhBYnRhZQHGBiRw=="], - "string-width": ["string-width@8.3.0", "", { "dependencies": { "get-east-asian-width": "^1.5.0", "strip-ansi": "^7.1.2" } }, "sha512-ZbmZM0JCihQN91dWnxoipT2KOEyHqEyfRXUyjuRhW8b/xnqPDoq4gWEVApTVa9db2wN8mmoikgFBbjh71+cGeQ=="], + "string-width": ["string-width@7.2.0", "", { "dependencies": { "emoji-regex": "^10.3.0", "get-east-asian-width": "^1.0.0", "strip-ansi": "^7.1.0" } }, "sha512-tsaTIkKW9b4N+AEj+SVA+WhJzV7/zMhcSu78mLKWSk7cXMOSHsBKFWUs0fWwq8QyK3MgJBQRX6Gbi4kYbdvGkQ=="], "strip-ansi": ["strip-ansi@7.2.0", "", { "dependencies": { "ansi-regex": "^6.2.2" } }, "sha512-yDPMNjp4WyfYBkHnjIRLfca1i6KMyGCtsVgoKe/z1+6vukgaENdgGBZt+ZmKPc4gavvEZ5OgHfHdrazhgNyG7w=="], @@ -1218,7 +1218,7 @@ "word-wrap": ["word-wrap@1.2.5", "", {}, "sha512-BN22B5eaMMI9UMtjrGd5g5eCYPpCPDUy0FJXbYsaT5zYxjFOckS53SQDE3pWkVoWpHXVb3BrYcEN4Twa55B5cA=="], - "wrap-ansi": ["wrap-ansi@10.0.2", "", { "dependencies": { "ansi-styles": "^6.2.3", "string-width": "^8.2.0" } }, "sha512-6OLRNZqRntVGBm26ghZ5eZhCuWWyOkRCSn8XsOIuq5stpCE/tjLbdozd93naILZig0lmD8ANfRwnhn0DLeXGlg=="], + "wrap-ansi": ["wrap-ansi@9.0.2", "", { "dependencies": { "ansi-styles": "^6.2.1", "string-width": "^7.0.0", "strip-ansi": "^7.1.0" } }, "sha512-42AtmgqjV+X1VpdOfyTGOYRi0/zsoLqtXQckTmqTeybT+BDIbM/Guxo7x3pE2vtpr1ok6xRqM9OpBe+Jyoqyww=="], "wrappy": ["wrappy@1.0.2", "", {}, "sha512-l4Sp/DRseor9wL6EvV2+TuQn63dMkPjZ/sp9XkghTEbV9KlPS1xUsZ3u7/IQO4wxtcFB4bgpQPRcR3QCvezPcQ=="], @@ -1486,6 +1486,8 @@ "qrcode/yargs": ["yargs@15.4.1", "", { "dependencies": { "cliui": "^6.0.0", "decamelize": "^1.2.0", "find-up": "^4.1.0", "get-caller-file": "^2.0.1", "require-directory": "^2.1.1", "require-main-filename": "^2.0.0", "set-blocking": "^2.0.0", "string-width": "^4.2.0", "which-module": "^2.0.0", "y18n": "^4.0.0", "yargs-parser": "^18.1.2" } }, "sha512-aePbxDmcYW++PaqBsJ+HYUFwCdv4LVvdnhBy78E57PIor8/OVvhMrADFFEDh8DHDFRv/O9i3lPhsENjO7QX0+A=="], + "string-width/emoji-regex": ["emoji-regex@10.6.0", "", {}, "sha512-toUI84YS5YmxW219erniWD0CIVOo46xGKColeNQRgOzDorgBi1v4D71/OFzgD9GO2UGKIv1C3Sp8DAn0+j5w7A=="], + "type-is/content-type": ["content-type@2.0.0", "", {}, "sha512-j/O/d7GcZCyNl7/hwZAb606rzqkyvaDctLmckbxLzHvFBzTJHuGEdodATcP3yIRoDrLHkIATJuvzbFlp/ki2cQ=="], "xss/commander": ["commander@2.20.3", "", {}, "sha512-GpVkmM8vF2vQUkj2LvZmD35JxeJOLCwJ9cUkugyk2nuhbv3+mJvpLYYt+0+USMxE+oj+ey/lJEnhZw75x/OMcQ=="], diff --git a/package.json b/package.json index 85947217b..a92fae878 100644 --- a/package.json +++ b/package.json @@ -165,7 +165,7 @@ "usehooks-ts": "3.1.1", "vscode-languageserver-protocol": "3.18.5", "web-tree-sitter": "0.25.10", - "wrap-ansi": "10.0.2", + "wrap-ansi": "9.0.2", "ws": "8.22.0", "xss": "1.0.16", "yaml": "2.9.1", diff --git a/src/entrypoints/mcp.ts b/src/entrypoints/mcp.ts index 237df4833..1a03346df 100644 --- a/src/entrypoints/mcp.ts +++ b/src/entrypoints/mcp.ts @@ -6,7 +6,6 @@ process.env.CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS ??= 'true' import { Server } from '@modelcontextprotocol/sdk/server/index.js' -import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' import { ZodError } from 'zod/v4' import { CallToolRequestSchema, @@ -19,6 +18,7 @@ import { getDefaultAppState } from 'src/state/AppStateStore.js' import review from '../commands/review.js' import type { Command } from '../commands.js' import { getMcpToolsCommandsAndResources } from '../services/mcp/client.js' +import { createMcpStdioServerTransport } from '../services/mcp/stdioTransport.js' import type { MCPServerConnection } from '../services/mcp/types.js' import { findToolByName, @@ -257,7 +257,7 @@ export async function startMCPServer( ) async function runServer() { - const transport = new StdioServerTransport() + const transport = createMcpStdioServerTransport() await server.connect(transport) } diff --git a/src/ink/wrapAnsi.test.ts b/src/ink/wrapAnsi.test.ts new file mode 100644 index 000000000..4c865fd8e --- /dev/null +++ b/src/ink/wrapAnsi.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, test } from 'bun:test' +import wrapAnsiNpm from 'wrap-ansi' + +// The shipped CLI runs on Node, where ./wrapAnsi.ts falls back to the npm +// package; this suite runs on Bun, where it uses Bun.wrapAnsi. Exercise the +// npm package directly so a release that changes its output fails here rather +// than only in the shipped CLI. wrap-ansi 10 expands tabs into spaces, which +// made Cursor throw "Failed to find wrapped line in text" for any tab typed +// into a text input and exited the session. +const OPTIONS = { hard: true, trim: false } as const + +const SAMPLES: ReadonlyArray = [ + ['foo\tbar', 80], + ['\t\tindented', 80], + [`${'x'.repeat(78)}\tyy\tzz`, 80], + ['a\tb c\td e\tf g\th', 8], + ['\u001b[31mred\tword\u001b[39m tail', 6], + ['日本\t語の文章です', 6], + ['plain words wrap here', 7], +] + +describe('npm wrap-ansi (the Node runtime path)', () => { + test('keeps tab characters instead of expanding them', () => { + expect(wrapAnsiNpm('foo\tbar', 80, OPTIONS)).toBe('foo\tbar') + expect(wrapAnsiNpm('\t\tindented', 80, OPTIONS)).toBe('\t\tindented') + }) + + test('matches Bun.wrapAnsi, which the Bun-run tests exercise', () => { + for (const [input, columns] of SAMPLES) { + expect(wrapAnsiNpm(input, columns, OPTIONS)).toBe( + Bun.wrapAnsi(input, columns, OPTIONS), + ) + } + }) +}) diff --git a/src/schemas/hooks.test.ts b/src/schemas/hooks.test.ts index 5a706b3a0..cbd26c7ea 100644 --- a/src/schemas/hooks.test.ts +++ b/src/schemas/hooks.test.ts @@ -10,7 +10,17 @@ import { } from './hooks.js' import { PluginHooksSchema } from '../utils/plugins/schemas.js' import { SettingsSchema } from '../utils/settings/types.js' -import { parseSettingsFile } from '../utils/settings/settings.js' +import { + clearMdmSettingsCache, + parseCommandOutputAsSettings, + setMdmSettingsCache, +} from '../utils/settings/mdm/settings.js' +import { + getSettingsForSource, + loadManagedFileSettings, + parseSettingsFile, +} from '../utils/settings/settings.js' +import { resetSettingsCache } from '../utils/settings/settingsCache.js' import { validateSettingsFileContent } from '../utils/settings/validation.js' const validCommandHook = { @@ -120,7 +130,7 @@ describe('settings diagnostics for unsupported command hook args', () => { hooks: { UserPromptSubmit: [{ hooks: [hook] }] }, }) - test('settings loading preserves the explanation and rejects the file', () => { + test('settings loading skips the hook and keeps the explanation', () => { const fixtureDir = mkdtempSync(join(tmpdir(), 'hook-args-diagnostic-')) const settingsPath = join(fixtureDir, 'settings.json') try { @@ -128,9 +138,10 @@ describe('settings diagnostics for unsupported command hook args', () => { type: 'command', command: 'sh', args: ['-c', 'cat'], })) const result = parseSettingsFile(settingsPath) - expect(result.settings).toBeNull() + expect(result.settings).toEqual({}) expect(result.errors).toHaveLength(1) expect(result.errors[0]?.path).toBe('hooks.UserPromptSubmit.0.hooks.0.args') + expect(result.errors[0]?.message).toContain('Command hook was skipped') expect(result.errors[0]?.message).toContain('do not support an "args" field') expect(result.errors[0]?.message).toContain('complete invocation in "command"') } finally { @@ -138,6 +149,112 @@ describe('settings diagnostics for unsupported command hook args', () => { } }) + test('one hook with args does not discard the rest of the settings file', () => { + const fixtureDir = mkdtempSync(join(tmpdir(), 'hook-args-isolation-')) + const settingsPath = join(fixtureDir, 'settings.json') + try { + writeFileSync(settingsPath, JSON.stringify({ + permissions: { deny: ['Bash(rm -rf:*)'] }, + env: { OPENAI_MODEL: 'gpt-4o' }, + hooks: { + PreToolUse: [{ + matcher: 'Bash', + hooks: [ + { type: 'command', command: '~/bin/notify.sh', args: [] }, + validCommandHook, + ], + }], + UserPromptSubmit: [{ hooks: [validCommandHook] }], + }, + })) + const result = parseSettingsFile(settingsPath) + expect(result.settings?.permissions?.deny).toEqual(['Bash(rm -rf:*)']) + expect(result.settings?.env).toEqual({ OPENAI_MODEL: 'gpt-4o' }) + expect(result.settings?.hooks?.PreToolUse?.[0]?.hooks).toEqual([validCommandHook]) + expect(result.settings?.hooks?.UserPromptSubmit?.[0]?.hooks).toEqual([validCommandHook]) + expect(result.errors.map(error => error.path)).toEqual([ + 'hooks.PreToolUse.0.hooks.0.args', + ]) + } finally { + rmSync(fixtureDir, { recursive: true, force: true }) + } + }) + + test('managed settings from a command skip the hook and keep the rest', () => { + const { settings, errors } = parseCommandOutputAsSettings( + JSON.stringify({ + permissions: { deny: ['WebFetch'] }, + hooks: { + UserPromptSubmit: [{ + hooks: [{ type: 'command', command: 'sh', args: ['-c', 'cat'] }], + }], + }, + }), + 'managed-settings', + ) + expect(settings).toEqual({ permissions: { deny: ['WebFetch'] } }) + expect(errors.map(error => error.path)).toEqual([ + 'hooks.UserPromptSubmit.0.hooks.0.args', + ]) + }) + + test('skipping a hook removes only the structures it empties', () => { + const kept = { type: 'command' as const, command: 'echo kept' } + const skipped = { type: 'command', command: 'sh', args: ['-c', 'cat'] } + const { settings } = parseCommandOutputAsSettings( + JSON.stringify({ + hooks: { + PreToolUse: [ + { matcher: 'Bash', hooks: [skipped] }, + { matcher: 'Edit', hooks: [skipped, kept] }, + { matcher: 'Read', hooks: [] }, + ], + Stop: [{ hooks: [skipped] }], + }, + }), + 'managed-settings', + ) + expect(settings.hooks).toEqual({ + PreToolUse: [ + { matcher: 'Edit', hooks: [kept] }, + { matcher: 'Read', hooks: [] }, + ], + }) + }) + + test('a policy holding only a skipped hook does not outrank a lower-priority policy', () => { + const mdm = parseCommandOutputAsSettings( + JSON.stringify({ + hooks: { + PreToolUse: [{ + matcher: 'Bash', + hooks: [{ type: 'command', command: 'sh', args: ['-c', 'cat'] }], + }], + }, + }), + 'mdm', + ) + expect(mdm.settings).toEqual({}) + + const hkcu = parseCommandOutputAsSettings( + JSON.stringify({ permissions: { deny: ['WebFetch'] } }), + 'hkcu', + ) + try { + setMdmSettingsCache(mdm, hkcu) + resetSettingsCache() + // A managed settings file outranks HKCU, so expect it when this machine + // has one; either way the MDM source must not win. + const managedFile = loadManagedFileSettings().settings + expect(getSettingsForSource('policySettings')).toEqual( + managedFile ?? hkcu.settings, + ) + } finally { + clearMdmSettingsCache() + resetSettingsCache() + } + }) + test('settings edit validation explains every supplied JSON args value', () => { for (const args of [null, [], ['--flag'], '', 'value', 0, false, {}]) { const result = validateSettingsFileContent(settingsContent({ diff --git a/src/schemas/hooks.ts b/src/schemas/hooks.ts index 7920057ae..a30ffbd46 100644 --- a/src/schemas/hooks.ts +++ b/src/schemas/hooks.ts @@ -26,6 +26,13 @@ const IfConditionSchema = lazySchema(() => ), ) +/** Explanation shown wherever a command hook's unsupported `args` is reported. */ +export const COMMAND_HOOK_ARGS_UNSUPPORTED_MESSAGE = + 'Command hooks do not support an "args" field. Put the complete ' + + 'invocation in "command" (e.g. "command": "sh -c \'printf HANDLER_RAN; cat\'"); ' + + 'a supplied "args" would otherwise be silently discarded and a ' + + 'different command would run.' + // Internal factory for individual hook schemas (shared between exported // discriminated union members and the HookCommandSchema factory) function buildHookSchemas() { @@ -42,14 +49,11 @@ function buildHookSchemas() { // undefined, so `.optional()` only admits a programmatically-constructed // explicit `undefined`, which means "not supplied" — every JSON-parsed // value (null, array, string, …) is rejected. + // Settings files drop just the offending hook before validation + // (filterUnsupportedCommandHookArgs), so one bad hook can't discard the + // rest of the file, including its permission deny rules. args: z - .never({ - error: - 'Command hooks do not support an "args" field. Put the complete ' + - 'invocation in "command" (e.g. "command": "sh -c \'printf HANDLER_RAN; cat\'"); ' + - 'a supplied "args" would otherwise be silently discarded and a ' + - 'different command would run.', - }) + .never({ error: COMMAND_HOOK_ARGS_UNSUPPORTED_MESSAGE }) .optional() .describe( 'Not supported: put the complete invocation in "command" instead', diff --git a/src/services/lsp/LSPServerManager.test.ts b/src/services/lsp/LSPServerManager.test.ts index c58539ac7..920312b52 100644 --- a/src/services/lsp/LSPServerManager.test.ts +++ b/src/services/lsp/LSPServerManager.test.ts @@ -648,6 +648,28 @@ describe('LSP lifecycle notification failures', () => { await open.catch(() => {}) } }) + + test('the default timeout keeps a server that is busy for a few seconds', async () => { + jest.useFakeTimers() + const { manager, controls } = await createManager() + const server = controls.get('typescript')! + const gate = server.blockNextNotification('textDocument/didOpen') + const open = manager.openFile('/repo/src/busy-open.ts', 'contents') + await gate.started + + try { + jest.advanceTimersByTime(5_000) + gate.release() + await expect(open).resolves.toBeUndefined() + expect(server.stopCalls).not.toHaveBeenCalled() + expect(server.server.state).toBe('running') + } finally { + gate.release() + jest.runAllTimers() + jest.useRealTimers() + await open.catch(() => {}) + } + }) }) describe('LSP generation resynchronization', () => { diff --git a/src/services/lsp/LSPServerManager.ts b/src/services/lsp/LSPServerManager.ts index 36d74b92c..40c12cc9e 100644 --- a/src/services/lsp/LSPServerManager.ts +++ b/src/services/lsp/LSPServerManager.ts @@ -116,7 +116,12 @@ export type LSPServerManagerDependencies = { lifecycleNotificationTimeoutMs: number } -const DEFAULT_LSP_LIFECYCLE_NOTIFICATION_TIMEOUT_MS = 2_000 +// A timed-out didOpen/didChange/didClose stops the server so its document +// versions resynchronize on restart. A healthy server can stop reading stdin +// for several seconds while indexing, and a full-document write then waits on +// the pipe, so only treat it as wedged after the same budget as startup +// (LSPServerInstance's DEFAULT_LSP_STARTUP_TIMEOUT_MS). +const DEFAULT_LSP_LIFECYCLE_NOTIFICATION_TIMEOUT_MS = 30_000 const DEFAULT_DEPENDENCIES: LSPServerManagerDependencies = { loadServerConfigs: getAllLspServers, diff --git a/src/services/mcp/client.ts b/src/services/mcp/client.ts index eb96d7d9e..1c134ca67 100644 --- a/src/services/mcp/client.ts +++ b/src/services/mcp/client.ts @@ -113,6 +113,7 @@ import { import { buildMcpToolName } from './mcpStringUtils.js' import { normalizeNameForMCP } from './normalization.js' import { listAllMcpResources, paginateMcpList } from './pagination.js' +import { createMcpStdioClientTransport } from './stdioTransport.js' import { getLoggingSafeMcpBaseUrl } from './utils.js' /* eslint-disable @typescript-eslint/no-require-imports */ @@ -1199,7 +1200,7 @@ export const connectToServer = memoize( serverRef.args ?? [], process.env.CLAUDE_CODE_SHELL_PREFIX, ) - transport = new StdioClientTransport({ + transport = createMcpStdioClientTransport({ command: finalCommand, args: finalArgs, env: { diff --git a/src/services/mcp/stdioTransport.test.ts b/src/services/mcp/stdioTransport.test.ts new file mode 100644 index 000000000..8d2e04f56 --- /dev/null +++ b/src/services/mcp/stdioTransport.test.ts @@ -0,0 +1,223 @@ +import { describe, expect, test } from 'bun:test' +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js' +import { ReadBuffer } from '@modelcontextprotocol/sdk/shared/stdio.js' +import type { JSONRPCMessage } from '@modelcontextprotocol/sdk/types.js' +import { + MCP_STDIO_MAX_BUFFER_BYTES, + McpStdioReadBuffer, + createMcpStdioClientTransport, + createMcpStdioServerTransport, +} from './stdioTransport.js' + +const PAYLOAD_BYTES = 12 * 1024 * 1024 +const PIPE_CHUNK_BYTES = 64 * 1024 + +// A stdio "server" that writes one 12 MiB JSON-RPC notification (above the +// SDK's 10 MiB default) and exits once it is flushed. +const SERVER_SCRIPT = ` +const message = JSON.stringify({ + jsonrpc: '2.0', + method: 'notifications/message', + params: { level: 'info', data: 'x'.repeat(${PAYLOAD_BYTES}) }, +}) + '\\n' +process.stdout.write(message, () => process.exit(0)) +` + +type FirstEvent = { message?: JSONRPCMessage; error?: Error } + +async function firstEvent(transport: StdioClientTransport): Promise { + try { + return await new Promise((resolve, reject) => { + transport.onmessage = message => resolve({ message }) + transport.onerror = error => resolve({ error }) + transport.onclose = () => + reject(new Error('stdio server closed before sending a message')) + transport.start().catch(reject) + }) + } finally { + await transport.close() + } +} + +const serverParams = { + command: process.execPath, + args: ['-e', SERVER_SCRIPT], + stderr: 'pipe' as const, +} + +function notification(data: string): Buffer { + return Buffer.from( + `${JSON.stringify({ + jsonrpc: '2.0', + method: 'notifications/message', + params: { level: 'info', data }, + })}\n`, + ) +} + +function chunksOf(bytes: Buffer, size: number): Buffer[] { + const chunks: Buffer[] = [] + for (let offset = 0; offset < bytes.length; offset += size) { + chunks.push(bytes.subarray(offset, offset + size)) + } + return chunks +} + +type ReadOutcome = { message: JSONRPCMessage } | { error: string } + +/** Feeds chunks the way the SDK transports do: append, then drain. */ +function readAll( + buffer: Pick, + chunks: Buffer[], +): ReadOutcome[] { + const outcomes: ReadOutcome[] = [] + for (const chunk of chunks) { + try { + buffer.append(chunk) + } catch (error) { + outcomes.push({ error: (error as Error).message }) + continue + } + while (true) { + try { + const message = buffer.readMessage() + if (message === null) break + outcomes.push({ message }) + } catch (error) { + outcomes.push({ error: (error as Error).message }) + } + } + } + return outcomes +} + +describe('MCP stdio client transport', () => { + test('the limit is above the 10 MiB SDK default', () => { + expect(MCP_STDIO_MAX_BUFFER_BYTES).toBeGreaterThan(PAYLOAD_BYTES) + }) + + test('both transports read through the linear-time buffer', () => { + const client = createMcpStdioClientTransport(serverParams) + const server = createMcpStdioServerTransport() + for (const transport of [client, server]) { + expect( + (transport as unknown as { _readBuffer: unknown })._readBuffer, + ).toBeInstanceOf(McpStdioReadBuffer) + } + }) + + test('the SDK default drops a 12 MiB message', async () => { + const { message, error } = await firstEvent(new StdioClientTransport(serverParams)) + expect(message).toBeUndefined() + expect(error?.message).toContain('ReadBuffer exceeded maximum size') + }) + + test('delivers a 12 MiB message from a stdio server', async () => { + const { message, error } = await firstEvent( + createMcpStdioClientTransport(serverParams), + ) + expect(error).toBeUndefined() + expect(message).toMatchObject({ method: 'notifications/message' }) + const data = (message as { params?: { data?: string } } | undefined) + ?.params?.data + expect(data?.length).toBe(PAYLOAD_BYTES) + }) +}) + +describe('McpStdioReadBuffer', () => { + test('matches the SDK buffer for split, joined, CRLF, UTF-8 and invalid lines', () => { + const stream = Buffer.concat([ + notification('first'), + notification('日本語 ✓ split across chunks'), + Buffer.from('{"jsonrpc":"2.0","id":1,"result":{}}\r\n'), + Buffer.from('not json\n'), + notification('after the invalid line'), + Buffer.from('{"jsonrpc":"2.0","method":"partial"'), + ]) + for (const size of [1, 2, 3, 5, 7, 64, stream.length]) { + const chunks = chunksOf(stream, size) + const ours = readAll(new McpStdioReadBuffer(), chunks) + expect(ours).toEqual(readAll(new ReadBuffer(), chunks)) + expect(ours.filter(outcome => 'message' in outcome)).toHaveLength(4) + } + }) + + test('accepts a message at the limit and rejects one byte more, like the SDK', () => { + const message = notification('x'.repeat(1000)) + const chunks = chunksOf(message, 100) + + const atLimit = readAll(new McpStdioReadBuffer(message.length), chunks) + expect(atLimit).toEqual( + readAll(new ReadBuffer({ maxBufferSize: message.length }), chunks), + ) + expect(atLimit).toEqual([{ message: expect.anything() }]) + + const overLimit = new McpStdioReadBuffer(message.length - 1) + const outcomes = readAll(overLimit, chunks) + expect(outcomes).toEqual( + readAll(new ReadBuffer({ maxBufferSize: message.length - 1 }), chunks), + ) + expect(outcomes).toEqual([ + { error: `ReadBuffer exceeded maximum size of ${message.length - 1} bytes` }, + ]) + expect(overLimit.bufferedBytes).toBe(0) + }) + + test('reads a large chunked message with linear copying and scanning', () => { + const message = notification('x'.repeat(32 * 1024 * 1024)) + const chunks = chunksOf(message, PIPE_CHUNK_BYTES) + + const originalConcat = Buffer.concat + const originalIndexOf = Buffer.prototype.indexOf + let copiedBytes = 0 + let scannedBytes = 0 + Buffer.concat = function (list, totalLength) { + const result = originalConcat.call(Buffer, list, totalLength) + copiedBytes += result.length + return result + } as typeof Buffer.concat + Buffer.prototype.indexOf = function ( + this: Buffer, + value: Parameters[0], + byteOffset?: number, + encoding?: BufferEncoding, + ) { + const index = originalIndexOf.call(this, value, byteOffset, encoding) + const start = byteOffset ?? 0 + scannedBytes += (index === -1 ? this.length : index + 1) - start + return index + } as Buffer['indexOf'] + + const buffer = new McpStdioReadBuffer() + let outcomes: ReadOutcome[] + try { + outcomes = readAll(buffer, chunks) + } finally { + Buffer.concat = originalConcat + Buffer.prototype.indexOf = originalIndexOf + } + + expect(outcomes).toHaveLength(1) + expect(outcomes[0]).toHaveProperty('message.params.data.length', 32 * 1024 * 1024) + // Each byte is scanned once and the line is copied once. The SDK buffer + // copies and scans the pending bytes again for every chunk, about 256 + // times the message size here. + expect(scannedBytes).toBe(message.length) + expect(copiedBytes).toBe(message.length - 1) + expect(buffer.bufferedBytes).toBe(0) + }) + + test('keeps only the unread remainder after a message', () => { + const buffer = new McpStdioReadBuffer() + const first = notification('first') + buffer.append(Buffer.concat([first, Buffer.from('{"jsonrpc"')])) + expect(buffer.readMessage()).toMatchObject({ params: { data: 'first' } }) + expect(buffer.readMessage()).toBeNull() + expect(buffer.bufferedBytes).toBe('{"jsonrpc"'.length) + + buffer.clear() + buffer.append(first) + expect(buffer.readMessage()).toMatchObject({ params: { data: 'first' } }) + expect(buffer.bufferedBytes).toBe(0) + }) +}) diff --git a/src/services/mcp/stdioTransport.ts b/src/services/mcp/stdioTransport.ts new file mode 100644 index 000000000..c10296794 --- /dev/null +++ b/src/services/mcp/stdioTransport.ts @@ -0,0 +1,140 @@ +import { + StdioClientTransport, + type StdioServerParameters, +} from '@modelcontextprotocol/sdk/client/stdio.js' +import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' +import { deserializeMessage } from '@modelcontextprotocol/sdk/shared/stdio.js' +import type { JSONRPCMessage } from '@modelcontextprotocol/sdk/types.js' + +/** + * Largest single stdio MCP message we buffer. MCP SDK 1.32 added a 10 MiB + * default and closes the connection when one message exceeds it, which kills + * servers that return a base64 screenshot, file or large query result in one + * tool call (SDK 1.29 had no limit). Oversized results are still truncated + * before they reach the model. + */ +export const MCP_STDIO_MAX_BUFFER_BYTES = 128 * 1024 * 1024 + +const NEWLINE = 0x0a + +/** + * Splits a stdio byte stream into JSON-RPC messages in linear time. + * + * The SDK's `ReadBuffer` (1.32.1) concatenates the whole pending buffer on + * every chunk and searches it from the start, so an n-byte message costs + * O(n²): a 100 MiB message in 64 KiB pipe chunks blocks the event loop for + * about 10 seconds. It also keeps the remainder of a parsed message as a + * `subarray`, which holds the whole allocation alive even when it is empty. + * + * This buffer keeps the chunks in a list, looks at each byte once when + * searching for a newline, copies a message once when it is complete, and + * never keeps an empty remainder. Limit, error and parsing behaviour match the + * SDK's buffer. + */ +export class McpStdioReadBuffer { + private chunks: Buffer[] = [] + private size = 0 + // Where the newline search resumes. Bytes before it hold no newline. + private scanChunk = 0 + private scanOffset = 0 + + constructor(private readonly maxBufferSize = MCP_STDIO_MAX_BUFFER_BYTES) {} + + get bufferedBytes(): number { + return this.size + } + + append(chunk: Buffer): void { + if (this.size + chunk.length > this.maxBufferSize) { + this.clear() + throw new Error( + `ReadBuffer exceeded maximum size of ${this.maxBufferSize} bytes`, + ) + } + if (chunk.length === 0) return + this.chunks.push(chunk) + this.size += chunk.length + } + + readMessage(): JSONRPCMessage | null { + while (this.scanChunk < this.chunks.length) { + const chunk = this.chunks[this.scanChunk]! + const newline = chunk.indexOf(NEWLINE, this.scanOffset) + if (newline === -1) { + this.scanChunk++ + this.scanOffset = 0 + continue + } + return deserializeMessage(this.takeLine(this.scanChunk, newline)) + } + return null + } + + clear(): void { + this.chunks = [] + this.size = 0 + this.scanChunk = 0 + this.scanOffset = 0 + } + + /** Removes the bytes up to and including the newline and decodes them. */ + private takeLine(lastChunk: number, newline: number): string { + const last = this.chunks[lastChunk]! + const parts = this.chunks.slice(0, lastChunk) + parts.push(last.subarray(0, newline)) + const lineBytes = parts.length === 1 ? parts[0]! : Buffer.concat(parts) + + const rest = last.subarray(newline + 1) + const remaining = this.chunks.slice(lastChunk + 1) + if (rest.length > 0) remaining.unshift(rest) + this.chunks = remaining + this.size -= lineBytes.length + 1 + this.scanChunk = 0 + this.scanOffset = 0 + + return lineBytes.toString('utf8').replace(/\r$/, '') + } +} + +type ReadBufferHost = { _readBuffer?: unknown } + +function isReadBuffer(value: unknown): boolean { + if (!value || typeof value !== 'object') return false + const buffer = value as Record + return ( + typeof buffer.append === 'function' && + typeof buffer.readMessage === 'function' && + typeof buffer.clear === 'function' + ) +} + +/** + * Swaps the SDK transport's private `_readBuffer` for McpStdioReadBuffer. + * SDK 1.32.1 only calls `append`, `readMessage` and `clear` on it. If a later + * SDK (possible for SDK embedders, who bring their own) no longer has the + * field, the SDK's own buffer stays, still with the raised size limit. + */ +function useLinearReadBuffer(transport: object, maxBufferSize: number): void { + const host = transport as ReadBufferHost + if (!isReadBuffer(host._readBuffer)) return + host._readBuffer = new McpStdioReadBuffer(maxBufferSize) +} + +/** Creates the stdio client transport for a configured MCP server. */ +export function createMcpStdioClientTransport( + params: StdioServerParameters, +): StdioClientTransport { + const maxBufferSize = params.maxBufferSize ?? MCP_STDIO_MAX_BUFFER_BYTES + const transport = new StdioClientTransport({ ...params, maxBufferSize }) + useLinearReadBuffer(transport, maxBufferSize) + return transport +} + +/** Creates the stdio server transport for `openclaude mcp serve`. */ +export function createMcpStdioServerTransport(): StdioServerTransport { + const transport = new StdioServerTransport(undefined, undefined, { + maxBufferSize: MCP_STDIO_MAX_BUFFER_BYTES, + }) + useLinearReadBuffer(transport, MCP_STDIO_MAX_BUFFER_BYTES) + return transport +} diff --git a/src/utils/codexCredentials.test.ts b/src/utils/codexCredentials.test.ts index e38e99021..396d5cb89 100644 --- a/src/utils/codexCredentials.test.ts +++ b/src/utils/codexCredentials.test.ts @@ -2,12 +2,28 @@ * These tests avoid static imports so Bun can mock secureStorage before * codexCredentials is first loaded. */ -import { afterEach, beforeEach, describe, expect, mock, test } from 'bun:test' +import { afterAll, afterEach, beforeEach, describe, expect, mock, test } from 'bun:test' import { acquireSharedMutationLock, releaseSharedMutationLock, } from '../test/sharedMutationLock.js' +// mock.restore() does not undo mock.module(), so put the real storage modules +// back once this file is done. Otherwise a later file in the same process +// (e.g. src/entrypoints/sdk/mcpNeedsAuth.test.ts) reads the stubs below. +const nonce = `${Date.now()}-${Math.random()}` +const realPlainTextStorage = { + ...await import(`./secureStorage/plainTextStorage.js?real=${nonce}`), +} +const realSecureStorage = { + ...await import(`./secureStorage/index.js?real=${nonce}`), +} + +afterAll(() => { + mock.module('./secureStorage/plainTextStorage.js', () => realPlainTextStorage) + mock.module('./secureStorage/index.js', () => realSecureStorage) +}) + type CodexCredentialsModule = typeof import('./codexCredentials.js') function importFreshCodexCredentials( diff --git a/src/utils/fastMode.test.ts b/src/utils/fastMode.test.ts index c8f0763fe..bb91bca52 100644 --- a/src/utils/fastMode.test.ts +++ b/src/utils/fastMode.test.ts @@ -208,7 +208,12 @@ async function importActualProviders(): Promise { } async function prepareFastModeTestState(): Promise { - const { setIsInteractive } = await import('../bootstrap/state.js') + const { resetModelStringsForTestingOnly, setIsInteractive } = await import( + '../bootstrap/state.js' + ) + // Model strings are cached process-wide. An earlier suite may have cached + // them for another provider (OpenAI's map turns "opus" into gpt-4o). + resetModelStringsForTestingOnly() setIsInteractive(true) const { DEFAULT_GLOBAL_CONFIG, _setGlobalConfigCacheForTesting } = await import('./config.js') diff --git a/src/utils/settings/mdm/settings.ts b/src/utils/settings/mdm/settings.ts index 356ba8481..2508d56cf 100644 --- a/src/utils/settings/mdm/settings.ts +++ b/src/utils/settings/mdm/settings.ts @@ -24,6 +24,7 @@ import { logForDiagnosticsNoPII } from '../../diagLogs.js' import { readFileSync } from '../../fileRead.js' import { getFsImplementation } from '../../fsOperations.js' import { safeParseJSON } from '../../json.js' +import { clone } from '../../slowOperations.js' import { profileCheckpoint } from '../../startupProfiler.js' import { getManagedFilePath, @@ -32,6 +33,7 @@ import { import { type SettingsJson, SettingsSchema } from '../types.js' import { filterInvalidPermissionRules, + filterUnsupportedCommandHookArgs, formatZodError, type ValidationError, } from '../validation.js' @@ -206,12 +208,17 @@ export function parseCommandOutputAsSettings( stdout: string, sourcePath: string, ): { settings: SettingsJson; errors: ValidationError[] } { - const data = safeParseJSON(stdout, false) + // Copy the memoized parse result: the filters below edit it in place, and + // the 30-minute refresh parses the same output again. + const data = clone(safeParseJSON(stdout, false)) if (!data || typeof data !== 'object') { return { settings: {}, errors: [] } } - const ruleWarnings = filterInvalidPermissionRules(data, sourcePath) + const ruleWarnings = [ + ...filterInvalidPermissionRules(data, sourcePath), + ...filterUnsupportedCommandHookArgs(data, sourcePath), + ] const parseResult = SettingsSchema().safeParse(data) if (!parseResult.success) { const errors = formatZodError(parseResult.error, sourcePath) diff --git a/src/utils/settings/settings.transaction.test.ts b/src/utils/settings/settings.transaction.test.ts index bbcb7ab29..ac61b42fe 100644 --- a/src/utils/settings/settings.transaction.test.ts +++ b/src/utils/settings/settings.transaction.test.ts @@ -344,6 +344,38 @@ test('reads the merge base from disk after ownership, not from warm caches', asy }) }) +test('an unrelated update keeps a command hook that runtime loading skips', async () => { + await withIsolatedUserSettings((_root, settingsPath) => { + const hooks = { + PreToolUse: [ + { + matcher: 'Bash', + hooks: [ + { type: 'command', command: 'sh', args: ['-c', 'printf HOOK'] }, + { type: 'command', command: 'echo kept' }, + ], + }, + ], + } + writeFileSync( + settingsPath, + `${JSON.stringify({ env: { BEFORE: 'yes' }, hooks }, null, 2)}\n`, + ) + expect( + getSettingsForSource('userSettings')?.hooks?.PreToolUse?.[0]?.hooks, + ).toEqual([{ type: 'command', command: 'echo kept' }]) + + expect( + updateSettingsForSource('userSettings', { env: { AFTER: 'yes' } }), + ).toEqual({ error: null }) + + expect(JSON.parse(readFileSync(settingsPath, 'utf8'))).toEqual({ + env: { BEFORE: 'yes', AFTER: 'yes' }, + hooks, + }) + }) +}) + test('creates a missing settings file without weakening the synchronous result', async () => { await withIsolatedUserSettings((_root, settingsPath) => { expect(existsSync(settingsPath)).toBe(false) diff --git a/src/utils/settings/settings.ts b/src/utils/settings/settings.ts index 6fbe8cc60..9364b31a2 100644 --- a/src/utils/settings/settings.ts +++ b/src/utils/settings/settings.ts @@ -50,6 +50,7 @@ import { type SettingsJson, SettingsSchema } from './types.js' import { filterInvalidModelPricing, filterInvalidPermissionRules, + filterUnsupportedCommandHookArgs, formatZodError, type SettingsWithErrors, type ValidationError, @@ -201,7 +202,10 @@ export function parseSettingsFile(path: string): { } } -function parseSettingsFileUncached(path: string): { +function parseSettingsFileUncached( + path: string, + { keepUnsupportedHooks = false }: { keepUnsupportedHooks?: boolean } = {}, +): { settings: SettingsJson | null errors: ValidationError[] } { @@ -213,11 +217,19 @@ function parseSettingsFileUncached(path: string): { return { settings: {}, errors: [] } } - const data = safeParseJSON(content, false) + // safeParseJSON memoizes by content and returns the shared object. The + // filters below edit it in place, so work on a copy. Otherwise a later + // parse of the same content would see an already filtered object, + // losing both the warnings and the write-back copy of a skipped hook. + const data = clone(safeParseJSON(content, false)) - // Filter invalid permission rules before schema validation so one bad - // rule doesn't cause the entire settings file to be rejected. + // Filter invalid permission rules and unsupported hook fields before + // schema validation so one bad entry doesn't cause the entire settings + // file to be rejected. const ruleWarnings = filterInvalidPermissionRules(data, path) + const hookWarnings = keepUnsupportedHooks + ? [] + : filterUnsupportedCommandHookArgs(data, path) const modelPricingWarnings = filterInvalidModelPricing(data, path) const result = SettingsSchema().safeParse(data) @@ -226,13 +238,18 @@ function parseSettingsFileUncached(path: string): { const errors = formatZodError(result.error, path) return { settings: null, - errors: [...ruleWarnings, ...modelPricingWarnings, ...errors], + errors: [ + ...ruleWarnings, + ...hookWarnings, + ...modelPricingWarnings, + ...errors, + ], } } return { settings: result.data, - errors: [...ruleWarnings, ...modelPricingWarnings], + errors: [...ruleWarnings, ...hookWarnings, ...modelPricingWarnings], } } catch (error) { handleFileSystemError(error, path) @@ -446,7 +463,12 @@ export function updateSettingsForSource( targetPath => { // The transaction merge base must bypass both process-local settings // caches so a peer's completed update cannot be overwritten. - let existingSettings = parseSettingsFileUncached(targetPath).settings + // Skipping a command hook with `args` is a runtime decision. Keeping + // it here sends the file through the raw fallback below, so an + // unrelated update writes that hook back unchanged. + let existingSettings = parseSettingsFileUncached(targetPath, { + keepUnsupportedHooks: true, + }).settings // If validation failed, distinguish syntax errors from schema-invalid // objects whose unknown fields still need to survive the merge. diff --git a/src/utils/settings/validation.ts b/src/utils/settings/validation.ts index 3e542eca1..f5a7766d4 100644 --- a/src/utils/settings/validation.ts +++ b/src/utils/settings/validation.ts @@ -1,5 +1,6 @@ import type { ConfigScope } from 'src/services/mcp/types.js' import { type ZodError, type ZodIssue, z } from 'zod/v4' +import { COMMAND_HOOK_ARGS_UNSUPPORTED_MESSAGE } from '../../schemas/hooks.js' import { jsonParse } from '../slowOperations.js' import { plural } from '../stringUtils.js' import { validatePermissionRule } from './permissionValidation.js' @@ -269,6 +270,68 @@ export function filterInvalidPermissionRules( return warnings } +/** + * Removes command hooks that supply the unsupported `args` field from raw + * parsed JSON before schema validation. The hook is skipped (it never runs + * with a different command than intended), and the rest of the file, including + * its permission rules, still loads. Returns a warning for each removed hook. + * + * Matchers, events and the `hooks` object that end up empty because of this + * filter are removed too. Otherwise a managed source that held nothing but a + * skipped hook would still count as non-empty and win first-source-wins + * policy selection over a lower-priority source with real rules. + */ +export function filterUnsupportedCommandHookArgs( + data: unknown, + filePath: string, +): ValidationError[] { + if (!data || typeof data !== 'object') return [] + const root = data as Record + const hooks = root.hooks + if (!hooks || typeof hooks !== 'object' || Array.isArray(hooks)) return [] + const hooksByEvent = hooks as Record + + const warnings: ValidationError[] = [] + for (const [event, matchers] of Object.entries(hooksByEvent)) { + if (!Array.isArray(matchers)) continue + const keptMatchers = matchers.filter((matcher: unknown, matcherIndex) => { + if (!matcher || typeof matcher !== 'object') return true + const config = matcher as Record + if (!Array.isArray(config.hooks)) return true + const hookCount = config.hooks.length + const keptHooks = config.hooks.filter((hook: unknown, hookIndex) => { + if ( + !hook || + typeof hook !== 'object' || + (hook as Record).type !== 'command' || + (hook as Record).args === undefined + ) { + return true + } + warnings.push({ + file: filePath, + path: `hooks.${event}.${matcherIndex}.hooks.${hookIndex}.args`, + message: `Command hook was skipped. ${COMMAND_HOOK_ARGS_UNSUPPORTED_MESSAGE}`, + invalidValue: (hook as Record).args, + }) + return false + }) + config.hooks = keptHooks + return hookCount === 0 || keptHooks.length > 0 + }) + if (keptMatchers.length === matchers.length) continue + if (keptMatchers.length > 0) { + hooksByEvent[event] = keptMatchers + } else { + delete hooksByEvent[event] + } + } + if (warnings.length > 0 && Object.keys(hooksByEvent).length === 0) { + delete root.hooks + } + return warnings +} + export function filterInvalidModelPricing( data: unknown, filePath: string,