From 27f6a675be6f97d6ae41e2729f2146c9b5c8ad9f Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 1 Apr 2026 20:57:19 -0400 Subject: [PATCH] fix(cursor): address review feedback and translator edge cases --- src/commands/cursor-command-display.ts | 4 +- src/commands/cursor-command.ts | 2 +- src/commands/help-command.ts | 7 +- src/cursor/cursor-translator.ts | 102 ++++++++--- src/utils/config-manager.ts | 9 + .../unit/commands/help-command-parity.test.ts | 9 + tests/unit/cursor/cursor-daemon.test.ts | 75 +++++++- tests/unit/cursor/cursor-protobuf.test.ts | 168 +++++++++++++++++- 8 files changed, 338 insertions(+), 38 deletions(-) diff --git a/src/commands/cursor-command-display.ts b/src/commands/cursor-command-display.ts index 5e1daf57..5bc1f4d8 100644 --- a/src/commands/cursor-command-display.ts +++ b/src/commands/cursor-command-display.ts @@ -1,5 +1,6 @@ import type { CursorAuthStatus, CursorDaemonStatus, CursorModel } from '../cursor/types'; import type { CursorConfig } from '../config/unified-config-types'; +import { getCcsDirDisplay } from '../utils/config-manager'; import { color } from '../utils/ui'; function printLines(lines: string[]): void { @@ -47,6 +48,7 @@ export function renderCursorStatus( daemonStatus: CursorDaemonStatus ): void { const localBaseUrl = `http://127.0.0.1:${cursorConfig.port}`; + const dirDisplay = getCcsDirDisplay(); const isReady = cursorConfig.enabled && authStatus.authenticated && !authStatus.expired && daemonStatus.running; @@ -90,7 +92,7 @@ export function renderCursorStatus( console.log(` Models route: ${localBaseUrl}/v1/models`); console.log(''); console.log('Client setup:'); - console.log(' Raw settings: ~/.ccs/cursor.settings.json'); + console.log(` Raw settings: ${dirDisplay}/cursor.settings.json`); console.log(' Subcommands: ccs cursor help'); if (isReady) { diff --git a/src/commands/cursor-command.ts b/src/commands/cursor-command.ts index 05a9245b..b103b735 100644 --- a/src/commands/cursor-command.ts +++ b/src/commands/cursor-command.ts @@ -1,7 +1,7 @@ /** * Cursor CLI Command * - * Handles `ccs cursor ` commands. + * Handles `ccs cursor [subcommand]` commands. */ import { diff --git a/src/commands/help-command.ts b/src/commands/help-command.ts index a30e6829..494f6779 100644 --- a/src/commands/help-command.ts +++ b/src/commands/help-command.ts @@ -2,7 +2,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { initUI, box, color, dim, sectionHeader, subheader } from '../utils/ui'; import { isUnifiedMode } from '../config/unified-config-loader'; -import { getCcsDir, getCcsDirSource } from '../utils/config-manager'; +import { getCcsDirDisplay } from '../utils/config-manager'; import { CLIPROXY_DEFAULT_PORT } from '../cliproxy/config/port-manager'; import { getOfficialChannelsSupportMessage } from '../channels/official-channels-runtime'; @@ -129,8 +129,7 @@ Run ${color('ccs config', 'command')} for web dashboard`.trim(); writeLine(''); // Resolve display path for dynamic sections - const [dirSource] = getCcsDirSource(); - const dirDisplay = dirSource === 'default' ? '~/.ccs' : getCcsDir(); + const dirDisplay = getCcsDirDisplay(); // Usage section writeLine(subheader('Usage:')); @@ -288,7 +287,7 @@ Run ${color('ccs config', 'command')} for web dashboard`.trim(); 'Auto-detects token from Cursor installation', ], [ - ['ccs cursor ', 'Use Cursor IDE integration'], + ['ccs cursor', 'Cursor status + local daemon connection details'], ['ccs cursor auth', 'Import Cursor token'], ['ccs cursor auth --manual --token --machine-id ', 'Manual token import'], ['ccs cursor status', 'Show connection status'], diff --git a/src/cursor/cursor-translator.ts b/src/cursor/cursor-translator.ts index fa6617c7..72112568 100644 --- a/src/cursor/cursor-translator.ts +++ b/src/cursor/cursor-translator.ts @@ -55,6 +55,7 @@ interface OpenAIRequestBody { const MAX_TOOL_RESULT_CHARS = 12_000; const TOOL_RESULT_SERIALIZATION_FALLBACK = '[unserializable content]'; const TOOL_USE_ARGUMENTS_FALLBACK = '{}'; +const TOOL_CALL_ID_PATTERN = /^[a-zA-Z0-9_-]+$/; function isTextPart(part: OpenAIContentPart): part is OpenAITextPart { return part.type === 'text'; @@ -68,19 +69,19 @@ function isToolResultPart(part: OpenAIContentPart): part is OpenAIToolResultPart return part.type === 'tool_result'; } -function extractTextContent(content: OpenAIMessage['content']): string { +function extractTextContent(content: OpenAIMessage['content'], separator = ''): string { if (typeof content === 'string') { return content; } - let text = ''; + const parts: string[] = []; for (const part of content) { if (isTextPart(part) && part.text) { - text += part.text; + parts.push(part.text); } } - return text; + return parts.join(separator); } function stringifyUnknown(value: unknown, fallback = ''): string { @@ -101,16 +102,16 @@ function truncateToolResultText(text: string): string { return text; } - let suffix = '\n[truncated]'; - let keepLength = Math.max(MAX_TOOL_RESULT_CHARS - suffix.length, 0); - let omittedChars = text.length - keepLength; - - suffix = `\n[truncated ${omittedChars} chars]`; - keepLength = Math.max(MAX_TOOL_RESULT_CHARS - suffix.length, 0); - omittedChars = text.length - keepLength; - suffix = `\n[truncated ${omittedChars} chars]`; - - return `${text.slice(0, keepLength)}${suffix}`; + let omittedChars = text.length - MAX_TOOL_RESULT_CHARS; + while (true) { + const suffix = `\n[truncated ${omittedChars} chars]`; + const keepLength = Math.max(MAX_TOOL_RESULT_CHARS - suffix.length, 0); + const nextOmittedChars = text.length - keepLength; + if (nextOmittedChars === omittedChars) { + return `${text.slice(0, keepLength)}${suffix}`; + } + omittedChars = nextOmittedChars; + } } function escapeXml(text: string): string { @@ -133,6 +134,19 @@ function normalizeToolCallId(toolCallId: string | undefined): string { return typeof toolCallId === 'string' ? toolCallId.split('\n')[0] : ''; } +function sanitizeToolCallId(toolCallId: string | undefined): string { + const normalizedId = normalizeToolCallId(toolCallId).trim(); + if (!normalizedId) { + return ''; + } + + if (TOOL_CALL_ID_PATTERN.test(normalizedId)) { + return normalizedId; + } + + return normalizedId.replace(/[^a-zA-Z0-9_-]/g, ''); +} + function extractToolResultText(content: unknown): string { if (content === undefined) { return ''; @@ -146,7 +160,7 @@ function extractToolResultText(content: unknown): string { return content .filter(isTextPart) .map((part) => part.text || '') - .join(''); + .join('\n'); } return stringifyUnknown(content, TOOL_RESULT_SERIALIZATION_FALLBACK); @@ -161,9 +175,34 @@ function resolveToolUseId( messageIndex: number, partIndex: number ): string { - return typeof part.id === 'string' && part.id.length > 0 - ? part.id - : createFallbackToolUseId(messageIndex, partIndex); + const sanitizedId = sanitizeToolCallId(part.id); + return sanitizedId || createFallbackToolUseId(messageIndex, partIndex); +} + +function requireToolResultId(toolCallId: string | undefined, location: string): string { + const sanitizedId = sanitizeToolCallId(toolCallId); + if (sanitizedId) { + return sanitizedId; + } + + throw new Error(`${location} must include a valid tool result id`); +} + +function normalizeAssistantToolCalls( + toolCalls: NonNullable, + messageIndex: number +): NonNullable { + return toolCalls.map((toolCall, toolCallIndex) => ({ + ...toolCall, + id: sanitizeToolCallId(toolCall.id) || createFallbackToolUseId(messageIndex, toolCallIndex), + function: { + name: toolCall.function?.name || 'tool', + arguments: + typeof toolCall.function?.arguments === 'string' + ? toolCall.function.arguments + : stringifyUnknown(toolCall.function?.arguments ?? {}, TOOL_USE_ARGUMENTS_FALLBACK), + }, + })); } function rememberToolCallMeta( @@ -259,7 +298,8 @@ function mergeAssistantToolCalls( function renderUserContent( content: OpenAIMessage['content'], - toolCallMetaMap: Map + toolCallMetaMap: Map, + messageIndex: number ): string { if (typeof content === 'string') { return content; @@ -267,7 +307,8 @@ function renderUserContent( const parts: string[] = []; let textBuffer = ''; - for (const part of content) { + for (let partIndex = 0; partIndex < content.length; partIndex++) { + const part = content[partIndex]; if (isTextPart(part) && part.text) { textBuffer += part.text; continue; @@ -282,7 +323,10 @@ function renderUserContent( textBuffer = ''; } - const toolCallId = part.tool_use_id || ''; + const toolCallId = requireToolResultId( + part.tool_use_id, + `messages[${messageIndex}].content[${partIndex}]` + ); const normalizedId = normalizeToolCallId(toolCallId); const toolName = toolCallMetaMap.get(toolCallId)?.name || toolCallMetaMap.get(normalizedId)?.name || 'tool'; @@ -317,7 +361,10 @@ function convertMessages(messages: OpenAIMessage[]): CursorMessage[] { } if (msg.role === 'tool') { - const toolCallId = msg.tool_call_id || ''; + const toolCallId = requireToolResultId( + msg.tool_call_id, + `messages[${messageIndex}].tool_call_id` + ); const normalizedToolCallId = normalizeToolCallId(toolCallId); const rememberedToolName = toolCallMetaMap.get(toolCallId)?.name || toolCallMetaMap.get(normalizedToolCallId)?.name; @@ -325,19 +372,20 @@ function convertMessages(messages: OpenAIMessage[]): CursorMessage[] { result.push({ role: 'user', - content: buildToolResultBlock(toolName, toolCallId, extractTextContent(msg.content)), + content: buildToolResultBlock(toolName, toolCallId, extractTextContent(msg.content, '\n')), }); continue; } if (msg.role === 'user' || msg.role === 'assistant') { if (msg.role === 'assistant') { + const normalizedToolCalls = normalizeAssistantToolCalls(msg.tool_calls || [], messageIndex); const assistantToolCalls = mergeAssistantToolCalls( - msg.tool_calls || [], + normalizedToolCalls, extractToolCallsFromContent(msg.content, messageIndex) ); - if (msg.tool_calls?.length) { - rememberToolCallMeta(toolCallMetaMap, msg.tool_calls); + if (normalizedToolCalls.length > 0) { + rememberToolCallMeta(toolCallMetaMap, normalizedToolCalls); } rememberToolUseParts(toolCallMetaMap, msg.content, messageIndex); @@ -355,7 +403,7 @@ function convertMessages(messages: OpenAIMessage[]): CursorMessage[] { }); } } else { - const content = renderUserContent(msg.content, toolCallMetaMap); + const content = renderUserContent(msg.content, toolCallMetaMap, messageIndex); if (content) { result.push({ role: 'user', diff --git a/src/utils/config-manager.ts b/src/utils/config-manager.ts index e20e3660..f32b83a5 100644 --- a/src/utils/config-manager.ts +++ b/src/utils/config-manager.ts @@ -102,6 +102,15 @@ export function getCcsDirSource(): [string, string] { return [r.source, r.dir]; } +/** + * Get the CCS directory as a user-facing display path. + * Keeps the default path concise while preserving explicit overrides. + */ +export function getCcsDirDisplay(): string { + const [source, dir] = getCcsDirSource(); + return source === 'default' ? '~/.ccs' : dir; +} + /** * Cloud sync folder patterns for security warning. */ diff --git a/tests/unit/commands/help-command-parity.test.ts b/tests/unit/commands/help-command-parity.test.ts index 08397323..913fe14f 100644 --- a/tests/unit/commands/help-command-parity.test.ts +++ b/tests/unit/commands/help-command-parity.test.ts @@ -88,6 +88,15 @@ describe('help command parity', () => { ).toBe(true); }); + test('root help documents bare cursor as the status entrypoint', async () => { + const lines: string[] = []; + await handleHelpCommand((line) => lines.push(line)); + + const rendered = stripAnsi(lines.join('\n')); + expect(rendered.includes('ccs cursor ')).toBe(false); + expect(rendered.includes('Cursor status + local daemon connection details')).toBe(true); + }); + test('root help explains Claude [1m] as an explicit CCS suffix with upstream limits', async () => { const lines: string[] = []; await handleHelpCommand((line) => lines.push(line)); diff --git a/tests/unit/cursor/cursor-daemon.test.ts b/tests/unit/cursor/cursor-daemon.test.ts index 8e17e2d0..1cf151ed 100644 --- a/tests/unit/cursor/cursor-daemon.test.ts +++ b/tests/unit/cursor/cursor-daemon.test.ts @@ -2,7 +2,7 @@ * Unit tests for Cursor daemon module */ -import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import { describe, it, expect, beforeEach, afterEach, spyOn } from 'bun:test'; import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; @@ -273,6 +273,33 @@ describe('stopDaemon', () => { } } }); + + it('refuses to stop when daemon ownership cannot be verified', async () => { + const killSpy = spyOn(process, 'kill').mockImplementation( + ((pid: number, signal?: NodeJS.Signals | number) => { + if (pid === process.pid && signal === 0) { + const err = new Error('EPERM') as NodeJS.ErrnoException; + err.code = 'EPERM'; + throw err; + } + + return true; + }) as typeof process.kill + ); + + writePidToFile(process.pid); + + try { + const result = await stopDaemon(); + + expect(result.success).toBe(false); + expect(result.error).toContain('unable to verify daemon ownership'); + expect(fs.existsSync(path.join(getTestCursorDir(), 'daemon.pid'))).toBe(true); + } finally { + killSpy.mockRestore(); + removePidFile(); + } + }); }); describe('handleCursorCommand', () => { @@ -366,9 +393,9 @@ describe('renderCursorStatus', () => { expect( logs.some((line) => line.includes('Anthropic base: http://127.0.0.1:20129')) ).toBe(true); - expect(logs.some((line) => line.includes('Raw settings: ~/.ccs/cursor.settings.json'))).toBe( - true - ); + expect( + logs.some((line) => line.includes(`Raw settings: ${getCcsDir()}/cursor.settings.json`)) + ).toBe(true); } finally { console.log = originalLog; } @@ -403,6 +430,46 @@ describe('renderCursorStatus', () => { console.log = originalLog; } }); + + it('falls back to ~/.ccs in status output when no CCS override is active', () => { + const originalLog = console.log; + const originalCcsHomeValue = process.env.CCS_HOME; + const logs: string[] = []; + + delete process.env.CCS_HOME; + console.log = (...args: unknown[]) => { + logs.push(args.map((arg) => String(arg)).join(' ')); + }; + + try { + renderCursorStatus( + { ...DEFAULT_CURSOR_CONFIG, enabled: true, port: 20129 }, + { + authenticated: true, + expired: false, + tokenAge: 0, + credentials: { + accessToken: 'a'.repeat(60), + machineId: '1234567890abcdef1234567890abcdef', + authMethod: 'manual', + importedAt: new Date().toISOString(), + }, + }, + { running: true, port: 20129, pid: 1234 } + ); + + expect(logs.some((line) => line.includes('Raw settings: ~/.ccs/cursor.settings.json'))).toBe( + true + ); + } finally { + if (originalCcsHomeValue !== undefined) { + process.env.CCS_HOME = originalCcsHomeValue; + } else { + delete process.env.CCS_HOME; + } + console.log = originalLog; + } + }); }); describe('renderCursorHelp', () => { diff --git a/tests/unit/cursor/cursor-protobuf.test.ts b/tests/unit/cursor/cursor-protobuf.test.ts index e2af6027..d3cdf709 100644 --- a/tests/unit/cursor/cursor-protobuf.test.ts +++ b/tests/unit/cursor/cursor-protobuf.test.ts @@ -223,7 +223,7 @@ describe('Message Translation', () => { expect(result.messages[0].tool_calls![0].function.name).toBe('get_weather'); }); - it('should accumulate tool results', () => { + it('should flatten tool results into user tool_result blocks', () => { const result = buildCursorRequest( 'gpt-4', { @@ -260,6 +260,56 @@ describe('Message Translation', () => { expect(result.messages[2]).toEqual({ role: 'user', content: 'What is the weather?' }); }); + it('should preserve consecutive tool messages as separate user turns', () => { + const result = buildCursorRequest( + 'gpt-4', + { + messages: [ + { + role: 'assistant', + content: '', + tool_calls: [ + { + id: 'call_one', + type: 'function', + function: { name: 'search_docs', arguments: '{"q":"cursor"}' }, + }, + { + id: 'call_two', + type: 'function', + function: { name: 'read_file', arguments: '{"path":"README.md"}' }, + }, + ], + }, + { + role: 'tool', + content: 'first result', + tool_call_id: 'call_one', + }, + { + role: 'tool', + content: 'second result', + tool_call_id: 'call_two', + }, + { role: 'user', content: 'Summarize both results.' }, + ], + }, + false, + {} + ); + + expect(result.messages).toHaveLength(4); + expect(result.messages[1]).toEqual({ + role: 'user', + content: expect.stringContaining('call_one'), + }); + expect(result.messages[2]).toEqual({ + role: 'user', + content: expect.stringContaining('call_two'), + }); + expect(result.messages[3]).toEqual({ role: 'user', content: 'Summarize both results.' }); + }); + it('should recover tool names for tool results without a name field', () => { const result = buildCursorRequest( 'gpt-4', @@ -332,6 +382,45 @@ describe('Message Translation', () => { expect(result.messages[1].content).toContain('{"answer":"Cursor integration ready"}'); }); + it('should preserve line breaks for multipart tool_result text content', () => { + const result = buildCursorRequest( + 'gpt-4', + { + messages: [ + { + role: 'assistant', + content: '', + tool_calls: [ + { + id: 'call_lines', + type: 'function', + function: { name: 'read_file', arguments: '{"path":"notes.txt"}' }, + }, + ], + }, + { + role: 'user', + content: [ + { + type: 'tool_result', + tool_use_id: 'call_lines', + content: [ + { type: 'text', text: 'first line' }, + { type: 'text', text: 'second line' }, + ], + }, + ], + }, + ], + }, + false, + {} + ); + + expect(result.messages).toHaveLength(2); + expect(result.messages[1].content).toContain('first line\nsecond line'); + }); + it('should convert assistant tool_use content blocks into tool_calls', () => { const result = buildCursorRequest( 'gpt-4', @@ -393,6 +482,38 @@ describe('Message Translation', () => { expect(result.messages[0].tool_calls![0].id).toBe('toolu_cursor_fallback_0_0'); }); + it('should normalize invalid assistant tool call ids before emitting tool results', () => { + const result = buildCursorRequest( + 'gpt-4', + { + messages: [ + { + role: 'assistant', + content: '', + tool_calls: [ + { + id: 'call_bad\nline two', + type: 'function', + function: { name: 'search_docs', arguments: '{"q":"cursor"}' }, + }, + ], + }, + { + role: 'tool', + content: 'done', + tool_call_id: 'call_bad\nline two', + }, + ], + }, + false, + {} + ); + + expect(result.messages).toHaveLength(2); + expect(result.messages[0].tool_calls![0].id).toBe('call_bad'); + expect(result.messages[1].content).toContain('call_bad'); + }); + it('should dedupe tool calls when assistant messages contain both tool_calls and tool_use blocks', () => { const result = buildCursorRequest( 'gpt-4', @@ -463,6 +584,10 @@ describe('Message Translation', () => { expect(result.messages).toHaveLength(2); expect(result.messages[1].content).toContain('[truncated '); expect(result.messages[1].content.length).toBeLessThan(12_250); + + const resultMatch = result.messages[1].content.match(/([\s\S]*)<\/result>/); + expect(resultMatch).not.toBeNull(); + expect(resultMatch?.[1].length).toBeLessThanOrEqual(12_000); }); it('should mark unserializable structured tool results explicitly', () => { @@ -504,6 +629,47 @@ describe('Message Translation', () => { expect(result.messages[1].content).toContain('[unserializable content]'); }); + it('should reject tool_result blocks without a valid tool_use_id', () => { + expect(() => + buildCursorRequest( + 'gpt-4', + { + messages: [ + { + role: 'user', + content: [ + { + type: 'tool_result', + content: 'done', + }, + ], + }, + ], + }, + false, + {} + ) + ).toThrow('messages[0].content[0] must include a valid tool result id'); + }); + + it('should reject tool role messages without a valid tool_call_id', () => { + expect(() => + buildCursorRequest( + 'gpt-4', + { + messages: [ + { + role: 'tool', + content: 'done', + }, + ], + }, + false, + {} + ) + ).toThrow('messages[0].tool_call_id must include a valid tool result id'); + }); + it('should handle array content format', () => { const result = buildCursorRequest( 'gpt-4',