diff --git a/CHANGELOG.md b/CHANGELOG.md index de4f011e..593a038f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### Fixed - Fixed `swift_package_build`, `swift_package_test`, and `swift_package_clean` swallowing compiler diagnostics on failure by treating empty stderr as falsy, so stdout diagnostics are included in the error response ([#243](https://github.com/getsentry/XcodeBuildMCP/issues/243)). +- Fixed stderr warnings (e.g. "multiple matching destinations") hiding actual test failures by prioritizing xcresult output when available ([#231](https://github.com/getsentry/XcodeBuildMCP/issues/231)) ## [2.1.0] diff --git a/src/cli.ts b/src/cli.ts index 552b5360..bf62844f 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -6,7 +6,7 @@ import { getSocketPath, getWorkspaceKey, resolveWorkspaceRoot } from './daemon/s import { startMcpServer } from './server/start-mcp-server.ts'; import { listCliWorkflowIdsFromManifest } from './runtime/tool-catalog.ts'; import { flushAndCloseSentry, initSentry, recordBootstrapDurationMetric } from './utils/sentry.ts'; -import { setLogLevel, type LogLevel } from './utils/logger.ts'; +import { coerceLogLevel, setLogLevel, type LogLevel } from './utils/logger.ts'; import { hydrateSentryDisabledEnvFromProjectConfig } from './utils/sentry-config.ts'; function findTopLevelCommand(argv: string[]): string | undefined { @@ -49,10 +49,7 @@ async function buildLightweightYargsApp(): Promise { - if (typeof value !== 'string') return value; - return value.trim().toLowerCase() === 'warning' ? 'warn' : value; - }, + coerce: coerceLogLevel, default: 'none', }) .option('style', { diff --git a/src/cli/commands/daemon.ts b/src/cli/commands/daemon.ts index 91f386fc..8dee250c 100644 --- a/src/cli/commands/daemon.ts +++ b/src/cli/commands/daemon.ts @@ -10,6 +10,7 @@ import { listDaemonRegistryEntries, readDaemonRegistryEntry, } from '../../daemon/daemon-registry.ts'; +import { coerceLogLevel } from '../../utils/logger.ts'; export interface DaemonCommandsOptions { defaultSocketPath: string; @@ -53,10 +54,7 @@ export function registerDaemonCommands(app: Argv, opts: DaemonCommandsOptions): 'info', 'debug', ] as const, - coerce: (value: unknown) => { - if (typeof value !== 'string') return value; - return value.trim().toLowerCase() === 'warning' ? 'warn' : value; - }, + coerce: coerceLogLevel, }) .option('tail', { type: 'number', diff --git a/src/cli/commands/init.ts b/src/cli/commands/init.ts index 129553e4..0779817f 100644 --- a/src/cli/commands/init.ts +++ b/src/cli/commands/init.ts @@ -626,20 +626,17 @@ export function registerInitCommand(app: Argv, ctx?: { workspaceRoot: string }): } } + const agentsGuidance = + agentsGuidanceStatus && agentsGuidancePath + ? { status: agentsGuidanceStatus, path: agentsGuidancePath, error: agentsGuidanceError } + : undefined; + const report: InitReport = { action: 'install', skillType: selection.skillType, installed: results, skipped: policy.skippedClients, - ...(agentsGuidanceStatus && agentsGuidancePath - ? { - agentsGuidance: { - status: agentsGuidanceStatus, - path: agentsGuidancePath, - ...(agentsGuidanceError ? { error: agentsGuidanceError } : {}), - }, - } - : {}), + ...(agentsGuidance ? { agentsGuidance } : {}), message: `Installed ${skillDisplayName(selection.skillType)} skill`, }; @@ -675,15 +672,13 @@ function enforceInstallPolicy( destFlag: string | undefined, selectionMode: InitSelection['selectionMode'], ): InstallPolicyResult { - if (skillType !== 'mcp') { - return { allowedTargets: targets, skippedClients: [] }; - } - - if (destFlag) { - return { allowedTargets: targets, skippedClients: [] }; - } + const skipPolicy = + skillType !== 'mcp' || + destFlag != null || + clientFlag === 'claude' || + selectionMode === 'interactive'; - if (clientFlag === 'claude' || selectionMode === 'interactive') { + if (skipPolicy) { return { allowedTargets: targets, skippedClients: [] }; } diff --git a/src/cli/yargs-app.ts b/src/cli/yargs-app.ts index 440967b6..ec2bd129 100644 --- a/src/cli/yargs-app.ts +++ b/src/cli/yargs-app.ts @@ -9,7 +9,7 @@ import { registerSetupCommand } from './commands/setup.ts'; import { registerToolsCommand } from './commands/tools.ts'; import { registerToolCommands } from './register-tool-commands.ts'; import { version } from '../version.ts'; -import { setLogLevel, type LogLevel } from '../utils/logger.ts'; +import { coerceLogLevel, setLogLevel, type LogLevel } from '../utils/logger.ts'; export interface YargsAppOptions { catalog: ToolCatalog; @@ -45,10 +45,7 @@ export function buildYargsApp(opts: YargsAppOptions): ReturnType { type: 'string', describe: 'Set log verbosity level', choices: ['none', 'error', 'warn', 'info', 'debug'] as const, - coerce: (value: unknown) => { - if (typeof value !== 'string') return value; - return value.trim().toLowerCase() === 'warning' ? 'warn' : value; - }, + coerce: coerceLogLevel, default: 'none', }) .option('style', { diff --git a/src/mcp/tools/device/__tests__/test_device.test.ts b/src/mcp/tools/device/__tests__/test_device.test.ts index 1179dda4..9c46ac7b 100644 --- a/src/mcp/tools/device/__tests__/test_device.test.ts +++ b/src/mcp/tools/device/__tests__/test_device.test.ts @@ -1,8 +1,7 @@ /** * Tests for test_device plugin * Following CLAUDE.md testing standards with literal validation - * Using pure dependency injection for deterministic testing - * NO VITEST MOCKING ALLOWED - Only createMockExecutor and manual stubs + * Using dependency injection for deterministic testing */ import { describe, it, expect, beforeEach } from 'vitest'; @@ -168,9 +167,9 @@ describe('test_device plugin', () => { ); expect(result.content).toHaveLength(2); - expect(result.content[0].text).toContain('✅'); - expect(result.content[1].text).toContain('Test Results Summary:'); - expect(result.content[1].text).toContain('MyScheme Tests'); + expect(result.content[0].text).toContain('Test Results Summary:'); + expect(result.content[0].text).toContain('MyScheme Tests'); + expect(result.content[1].text).toContain('✅'); }); it('should handle test failure scenarios', async () => { @@ -214,8 +213,8 @@ describe('test_device plugin', () => { ); expect(result.content).toHaveLength(2); - expect(result.content[1].text).toContain('Test Failures:'); - expect(result.content[1].text).toContain('testExample'); + expect(result.content[0].text).toContain('Test Failures:'); + expect(result.content[0].text).toContain('testExample'); }); it('should handle xcresult parsing failures gracefully', async () => { @@ -264,6 +263,70 @@ describe('test_device plugin', () => { expect(result.content[0].text).toContain('✅'); }); + it('should preserve stderr when xcresult reports zero tests (build failure)', async () => { + // When the build fails, xcresult exists but has totalTestCount: 0. + // stderr contains the actual compilation errors and must be preserved. + let callCount = 0; + const mockExecutor = async ( + _args: string[], + _description?: string, + _useShell?: boolean, + _opts?: { cwd?: string }, + _detached?: boolean, + ) => { + callCount++; + + // First call: xcodebuild test fails with compilation error + if (callCount === 1) { + return createMockCommandResponse({ + success: false, + output: '', + error: 'error: missing argument for parameter in call', + }); + } + + // Second call: xcresulttool succeeds but reports 0 tests + return createMockCommandResponse({ + success: true, + output: JSON.stringify({ + title: 'Test Results', + result: 'unknown', + totalTestCount: 0, + passedTests: 0, + failedTests: 0, + skippedTests: 0, + expectedFailures: 0, + }), + }); + }; + + const result = await testDeviceLogic( + { + projectPath: '/path/to/project.xcodeproj', + scheme: 'MyScheme', + deviceId: 'test-device-123', + configuration: 'Debug', + preferXcodebuild: false, + platform: 'iOS', + }, + mockExecutor, + createMockFileSystemExecutor({ + mkdtemp: async () => '/tmp/xcodebuild-test-buildfail', + tmpdir: () => '/tmp', + stat: async () => ({ isDirectory: () => false, mtimeMs: 0 }), + rm: async () => {}, + }), + ); + + // stderr with compilation error must be preserved + const allText = result.content.map((c) => c.text).join('\n'); + expect(allText).toContain('[stderr]'); + expect(allText).toContain('missing argument'); + + // xcresult summary should NOT be present + expect(allText).not.toContain('Test Results Summary:'); + }); + it('should support different platforms', async () => { // Mock xcresulttool output const mockExecutor = createMockExecutor({ @@ -298,7 +361,7 @@ describe('test_device plugin', () => { ); expect(result.content).toHaveLength(2); - expect(result.content[1].text).toContain('WatchApp Tests'); + expect(result.content[0].text).toContain('WatchApp Tests'); }); it('should handle optional parameters', async () => { @@ -337,7 +400,8 @@ describe('test_device plugin', () => { ); expect(result.content).toHaveLength(2); - expect(result.content[0].text).toContain('✅'); + expect(result.content[0].text).toContain('Test Results Summary:'); + expect(result.content[1].text).toContain('✅'); }); it('should handle workspace testing successfully', async () => { @@ -374,9 +438,9 @@ describe('test_device plugin', () => { ); expect(result.content).toHaveLength(2); - expect(result.content[0].text).toContain('✅'); - expect(result.content[1].text).toContain('Test Results Summary:'); - expect(result.content[1].text).toContain('WorkspaceScheme Tests'); + expect(result.content[0].text).toContain('Test Results Summary:'); + expect(result.content[0].text).toContain('WorkspaceScheme Tests'); + expect(result.content[1].text).toContain('✅'); }); }); }); diff --git a/src/mcp/tools/device/test_device.ts b/src/mcp/tools/device/test_device.ts index 257c7724..7d9732fb 100644 --- a/src/mcp/tools/device/test_device.ts +++ b/src/mcp/tools/device/test_device.ts @@ -27,6 +27,7 @@ import { getSessionAwareToolSchemaShape, } from '../../../utils/typed-tool-factory.ts'; import { nullifyEmptyStrings } from '../../../utils/schema-helpers.ts'; +import { filterStderrContent, type XcresultSummary } from '../../../utils/test-result-content.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -71,20 +72,14 @@ const publicSchemaObject = baseSchemaObject.omit({ platform: true, } as const); -/** - * Type definition for test summary structure from xcresulttool - * (JavaScript implementation - no actual interface, this is just documentation) - */ - /** * Parse xcresult bundle using xcrun xcresulttool */ async function parseXcresultBundle( resultBundlePath: string, executor: CommandExecutor = getDefaultCommandExecutor(), -): Promise { +): Promise { try { - // Use injected executor for testing const result = await executor( ['xcrun', 'xcresulttool', 'get', 'test-results', 'summary', '--path', resultBundlePath], 'Parse xcresult bundle', @@ -98,7 +93,11 @@ async function parseXcresultBundle( // Parse JSON response and format as human-readable const summaryData = JSON.parse(result.output) as Record; - return formatTestSummary(summaryData); + return { + formatted: formatTestSummary(summaryData), + totalTestCount: + typeof summaryData.totalTestCount === 'number' ? summaryData.totalTestCount : 0, + }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); log('error', `Error parsing xcresult bundle: ${errorMessage}`); @@ -252,20 +251,28 @@ export async function testDeviceLogic( throw new Error(`xcresult bundle not found at ${resultBundlePath}`); } - const testSummary = await parseXcresultBundle(resultBundlePath, executor); + const xcresult = await parseXcresultBundle(resultBundlePath, executor); log('info', 'Successfully parsed xcresult bundle'); // Clean up temporary directory await cleanup(); - // Return combined result - preserve isError from testResult (test failures should be marked as errors) + // If no tests ran (for example build/setup failed), xcresult summary is not useful. + // Return raw output so the original diagnostics stay visible. + if (xcresult.totalTestCount === 0) { + log('info', 'xcresult reports 0 tests — returning raw build output'); + return testResult; + } + + // xcresult summary should be first. Drop stderr-only noise while preserving non-stderr lines. + const filteredContent = filterStderrContent(testResult.content); return { content: [ - ...(testResult.content || []), { type: 'text', - text: '\nTest Results Summary:\n' + testSummary, + text: '\nTest Results Summary:\n' + xcresult.formatted, }, + ...filteredContent, ], isError: testResult.isError, }; diff --git a/src/mcp/tools/macos/__tests__/test_macos.test.ts b/src/mcp/tools/macos/__tests__/test_macos.test.ts index 71f6d562..75463ba5 100644 --- a/src/mcp/tools/macos/__tests__/test_macos.test.ts +++ b/src/mcp/tools/macos/__tests__/test_macos.test.ts @@ -507,6 +507,143 @@ describe('test_macos plugin (unified)', () => { ); }); + it('should filter out stderr lines when xcresult data is available', async () => { + // Regression test for #231: stderr warnings (e.g. "multiple matching destinations") + // should be dropped when xcresult parsing succeeds, since xcresult is authoritative. + let callCount = 0; + const mockExecutor = async ( + command: string[], + logPrefix?: string, + useShell?: boolean, + opts?: { env?: Record }, + detached?: boolean, + ) => { + callCount++; + void logPrefix; + void useShell; + void opts; + void detached; + + // First call: xcodebuild test fails with stderr warning + if (callCount === 1) { + return createMockCommandResponse({ + success: false, + output: '', + error: + 'WARNING: multiple matching destinations, using first match\n' + 'error: Test failed', + }); + } + + // Second call: xcresulttool succeeds + if (command.includes('xcresulttool')) { + return createMockCommandResponse({ + success: true, + output: JSON.stringify({ + title: 'Test Results', + result: 'FAILED', + totalTestCount: 5, + passedTests: 3, + failedTests: 2, + skippedTests: 0, + expectedFailures: 0, + }), + }); + } + + return createMockCommandResponse({ success: true, output: '' }); + }; + + const mockFileSystemExecutor = createTestFileSystemExecutor({ + mkdtemp: async () => '/tmp/xcodebuild-test-stderr', + }); + + const result = await testMacosLogic( + { + workspacePath: '/path/to/MyProject.xcworkspace', + scheme: 'MyScheme', + }, + mockExecutor, + mockFileSystemExecutor, + ); + + // stderr lines should be filtered out + const allText = result.content.map((c) => c.text).join('\n'); + expect(allText).not.toContain('[stderr]'); + + // xcresult summary should be present and first + expect(result.content[0].text).toContain('Test Results Summary:'); + + // Build status line should still be present + expect(allText).toContain('Test Run test failed for scheme MyScheme'); + }); + + it('should preserve stderr when xcresult reports zero tests (build failure)', async () => { + // When the build fails, xcresult exists but has totalTestCount: 0. + // In that case stderr contains the actual compilation errors and must be preserved. + let callCount = 0; + const mockExecutor = async ( + command: string[], + logPrefix?: string, + useShell?: boolean, + opts?: { env?: Record }, + detached?: boolean, + ) => { + callCount++; + void logPrefix; + void useShell; + void opts; + void detached; + + // First call: xcodebuild test fails with compilation error on stderr + if (callCount === 1) { + return createMockCommandResponse({ + success: false, + output: '', + error: 'error: missing argument for parameter in call', + }); + } + + // Second call: xcresulttool succeeds but reports 0 tests + if (command.includes('xcresulttool')) { + return createMockCommandResponse({ + success: true, + output: JSON.stringify({ + title: 'Test Results', + result: 'unknown', + totalTestCount: 0, + passedTests: 0, + failedTests: 0, + skippedTests: 0, + expectedFailures: 0, + }), + }); + } + + return createMockCommandResponse({ success: true, output: '' }); + }; + + const mockFileSystemExecutor = createTestFileSystemExecutor({ + mkdtemp: async () => '/tmp/xcodebuild-test-buildfail', + }); + + const result = await testMacosLogic( + { + workspacePath: '/path/to/MyProject.xcworkspace', + scheme: 'MyScheme', + }, + mockExecutor, + mockFileSystemExecutor, + ); + + // stderr with compilation error must be preserved (not filtered) + const allText = result.content.map((c) => c.text).join('\n'); + expect(allText).toContain('[stderr]'); + expect(allText).toContain('missing argument'); + + // xcresult summary should NOT be present (it's meaningless with 0 tests) + expect(allText).not.toContain('Test Results Summary:'); + }); + it('should return exact exception handling response', async () => { // Mock executor (won't be called due to mkdtemp failure) const mockExecutor = createMockExecutor({ diff --git a/src/mcp/tools/macos/test_macos.ts b/src/mcp/tools/macos/test_macos.ts index 09120fd9..1dbdf7ba 100644 --- a/src/mcp/tools/macos/test_macos.ts +++ b/src/mcp/tools/macos/test_macos.ts @@ -27,6 +27,7 @@ import { getSessionAwareToolSchemaShape, } from '../../../utils/typed-tool-factory.ts'; import { nullifyEmptyStrings } from '../../../utils/schema-helpers.ts'; +import { filterStderrContent, type XcresultSummary } from '../../../utils/test-result-content.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -67,29 +68,13 @@ const testMacosSchema = z.preprocess( export type TestMacosParams = z.infer; -/** - * Type definition for test summary structure from xcresulttool - * @typedef {Object} TestSummary - * @property {string} [title] - * @property {string} [result] - * @property {number} [totalTestCount] - * @property {number} [passedTests] - * @property {number} [failedTests] - * @property {number} [skippedTests] - * @property {number} [expectedFailures] - * @property {string} [environmentDescription] - * @property {Array} [devicesAndConfigurations] - * @property {Array} [testFailures] - * @property {Array} [topInsights] - */ - /** * Parse xcresult bundle using xcrun xcresulttool */ async function parseXcresultBundle( resultBundlePath: string, executor: CommandExecutor = getDefaultCommandExecutor(), -): Promise { +): Promise { try { const result = await executor( ['xcrun', 'xcresulttool', 'get', 'test-results', 'summary', '--path', resultBundlePath], @@ -102,18 +87,11 @@ async function parseXcresultBundle( } // Parse JSON response and format as human-readable - let summary: unknown; - try { - summary = JSON.parse(result.output || '{}'); - } catch (parseError) { - throw new Error(`Failed to parse JSON output: ${parseError}`); - } - - if (typeof summary !== 'object' || summary === null) { - throw new Error('Invalid JSON output: expected object'); - } - - return formatTestSummary(summary as Record); + const summary = JSON.parse(result.output || '{}') as Record; + return { + formatted: formatTestSummary(summary), + totalTestCount: typeof summary.totalTestCount === 'number' ? summary.totalTestCount : 0, + }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); log('error', `Error parsing xcresult bundle: ${errorMessage}`); @@ -149,31 +127,13 @@ function formatTestSummary(summary: Record): string { Array.isArray(summary.devicesAndConfigurations) && summary.devicesAndConfigurations.length > 0 ) { - const firstDeviceConfig: unknown = summary.devicesAndConfigurations[0]; - if ( - typeof firstDeviceConfig === 'object' && - firstDeviceConfig !== null && - 'device' in firstDeviceConfig - ) { - const device: unknown = (firstDeviceConfig as Record).device; - if (typeof device === 'object' && device !== null) { - const deviceRecord = device as Record; - const deviceName = - 'deviceName' in deviceRecord && typeof deviceRecord.deviceName === 'string' - ? deviceRecord.deviceName - : 'Unknown'; - const platform = - 'platform' in deviceRecord && typeof deviceRecord.platform === 'string' - ? deviceRecord.platform - : 'Unknown'; - const osVersion = - 'osVersion' in deviceRecord && typeof deviceRecord.osVersion === 'string' - ? deviceRecord.osVersion - : 'Unknown'; - - lines.push(`Device: ${deviceName} (${platform} ${osVersion})`); - lines.push(''); - } + const deviceConfig = summary.devicesAndConfigurations[0] as Record; + const device = deviceConfig.device as Record | undefined; + if (device) { + lines.push( + `Device: ${device.deviceName ?? 'Unknown'} (${device.platform ?? 'Unknown'} ${device.osVersion ?? 'Unknown'})`, + ); + lines.push(''); } } @@ -183,23 +143,13 @@ function formatTestSummary(summary: Record): string { summary.testFailures.length > 0 ) { lines.push('Test Failures:'); - summary.testFailures.forEach((failure: unknown, index: number) => { - if (typeof failure === 'object' && failure !== null) { - const failureRecord = failure as Record; - const testName = - 'testName' in failureRecord && typeof failureRecord.testName === 'string' - ? failureRecord.testName - : 'Unknown Test'; - const targetName = - 'targetName' in failureRecord && typeof failureRecord.targetName === 'string' - ? failureRecord.targetName - : 'Unknown Target'; - - lines.push(` ${index + 1}. ${testName} (${targetName})`); - - if ('failureText' in failureRecord && typeof failureRecord.failureText === 'string') { - lines.push(` ${failureRecord.failureText}`); - } + summary.testFailures.forEach((failureItem, index: number) => { + const failure = failureItem as Record; + lines.push( + ` ${index + 1}. ${failure.testName ?? 'Unknown Test'} (${failure.targetName ?? 'Unknown Target'})`, + ); + if (failure.failureText) { + lines.push(` ${failure.failureText}`); } }); lines.push(''); @@ -207,20 +157,11 @@ function formatTestSummary(summary: Record): string { if (summary.topInsights && Array.isArray(summary.topInsights) && summary.topInsights.length > 0) { lines.push('Insights:'); - summary.topInsights.forEach((insight: unknown, index: number) => { - if (typeof insight === 'object' && insight !== null) { - const insightRecord = insight as Record; - const impact = - 'impact' in insightRecord && typeof insightRecord.impact === 'string' - ? insightRecord.impact - : 'Unknown'; - const text = - 'text' in insightRecord && typeof insightRecord.text === 'string' - ? insightRecord.text - : 'No description'; - - lines.push(` ${index + 1}. [${impact}] ${text}`); - } + summary.topInsights.forEach((insightItem, index: number) => { + const insight = insightItem as Record; + lines.push( + ` ${index + 1}. [${insight.impact ?? 'Unknown'}] ${insight.text ?? 'No description'}`, + ); }); } @@ -287,20 +228,28 @@ export async function testMacosLogic( throw new Error(`xcresult bundle not found at ${resultBundlePath}`); } - const testSummary = await parseXcresultBundle(resultBundlePath, executor); + const xcresult = await parseXcresultBundle(resultBundlePath, executor); log('info', 'Successfully parsed xcresult bundle'); // Clean up temporary directory await fileSystemExecutor.rm(tempDir, { recursive: true, force: true }); - // Return combined result - preserve isError from testResult (test failures should be marked as errors) + // If no tests ran (for example build/setup failed), xcresult summary is not useful. + // Return raw output so the original diagnostics stay visible. + if (xcresult.totalTestCount === 0) { + log('info', 'xcresult reports 0 tests — returning raw build output'); + return testResult; + } + + // xcresult summary should be first. Drop stderr-only noise while preserving non-stderr lines. + const filteredContent = filterStderrContent(testResult.content); return { content: [ - ...(testResult.content ?? []), { type: 'text', - text: '\nTest Results Summary:\n' + testSummary, + text: '\nTest Results Summary:\n' + xcresult.formatted, }, + ...filteredContent, ], isError: testResult.isError, }; diff --git a/src/utils/__tests__/consolidate-content.test.ts b/src/utils/__tests__/consolidate-content.test.ts new file mode 100644 index 00000000..80815d8d --- /dev/null +++ b/src/utils/__tests__/consolidate-content.test.ts @@ -0,0 +1,172 @@ +/** + * Tests for consolidateContentForClaudeCode + * + * Exercises the consolidation path by injecting a mock EnvironmentDetector + * that reports Claude Code as active, bypassing the production guard that + * disables consolidation during tests. + */ +import { describe, it, expect } from 'vitest'; +import { consolidateContentForClaudeCode } from '../validation.ts'; +import { createMockEnvironmentDetector } from '../../test-utils/mock-executors.ts'; +import type { ToolResponse } from '../../types/common.ts'; + +const claudeCodeDetector = createMockEnvironmentDetector({ isRunningUnderClaudeCode: true }); +const nonClaudeCodeDetector = createMockEnvironmentDetector({ isRunningUnderClaudeCode: false }); + +describe('consolidateContentForClaudeCode', () => { + describe('when Claude Code is detected', () => { + it('should consolidate multiple text blocks into one', () => { + const response: ToolResponse = { + content: [ + { type: 'text', text: 'Block 1' }, + { type: 'text', text: 'Block 2' }, + { type: 'text', text: 'Block 3' }, + ], + }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + expect(result.content).toHaveLength(1); + expect(result.content[0].type).toBe('text'); + expect((result.content[0] as { type: 'text'; text: string }).text).toBe( + 'Block 1\n---\nBlock 2\n---\nBlock 3', + ); + }); + + it('should return single-block responses unchanged', () => { + const response: ToolResponse = { + content: [{ type: 'text', text: 'Only block' }], + }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + expect(result).toBe(response); + }); + + it('should return empty content unchanged', () => { + const response: ToolResponse = { content: [] }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + expect(result).toBe(response); + }); + + it('should preserve isError flag', () => { + const response: ToolResponse = { + content: [ + { type: 'text', text: 'Error A' }, + { type: 'text', text: 'Error B' }, + ], + isError: true, + }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + expect(result.isError).toBe(true); + expect(result.content).toHaveLength(1); + }); + + it('should skip non-text content blocks and return original when no text found', () => { + const response: ToolResponse = { + content: [ + { type: 'image', data: 'base64data', mimeType: 'image/png' }, + { type: 'image', data: 'base64data2', mimeType: 'image/jpeg' }, + ], + }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + expect(result).toBe(response); + }); + + it('should consolidate only text blocks when mixed with image blocks', () => { + const response: ToolResponse = { + content: [ + { type: 'text', text: 'Text A' }, + { type: 'image', data: 'base64data', mimeType: 'image/png' }, + { type: 'text', text: 'Text B' }, + ], + }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + expect(result.content).toHaveLength(1); + expect(result.content[0].type).toBe('text'); + expect((result.content[0] as { type: 'text'; text: string }).text).toBe( + 'Text A\n---\nText B', + ); + }); + + it('should add separators only between text blocks, not before first', () => { + const response: ToolResponse = { + content: [ + { type: 'text', text: 'First' }, + { type: 'text', text: 'Second' }, + ], + }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + const text = (result.content[0] as { type: 'text'; text: string }).text; + expect(text).not.toMatch(/^---/); + expect(text).toBe('First\n---\nSecond'); + }); + + it('should preserve extra properties on the response', () => { + const response: ToolResponse = { + content: [ + { type: 'text', text: 'A' }, + { type: 'text', text: 'B' }, + ], + _meta: { foo: 'bar' }, + }; + + const result = consolidateContentForClaudeCode(response, claudeCodeDetector); + + expect(result._meta).toEqual({ foo: 'bar' }); + expect(result.content).toHaveLength(1); + }); + }); + + describe('when Claude Code is NOT detected', () => { + it('should return multi-block responses unchanged', () => { + const response: ToolResponse = { + content: [ + { type: 'text', text: 'Block 1' }, + { type: 'text', text: 'Block 2' }, + ], + }; + + const result = consolidateContentForClaudeCode(response, nonClaudeCodeDetector); + + expect(result).toBe(response); + expect(result.content).toHaveLength(2); + }); + + it('should return single-block responses unchanged', () => { + const response: ToolResponse = { + content: [{ type: 'text', text: 'Only block' }], + }; + + const result = consolidateContentForClaudeCode(response, nonClaudeCodeDetector); + + expect(result).toBe(response); + }); + }); + + describe('without explicit detector (default behavior)', () => { + it('should use default detector and not consolidate in test env', () => { + const response: ToolResponse = { + content: [ + { type: 'text', text: 'Block 1' }, + { type: 'text', text: 'Block 2' }, + ], + }; + + const result = consolidateContentForClaudeCode(response); + + expect(result).toBe(response); + expect(result.content).toHaveLength(2); + }); + }); +}); diff --git a/src/utils/__tests__/typed-tool-factory-consolidation.test.ts b/src/utils/__tests__/typed-tool-factory-consolidation.test.ts new file mode 100644 index 00000000..4d8cb9f0 --- /dev/null +++ b/src/utils/__tests__/typed-tool-factory-consolidation.test.ts @@ -0,0 +1,113 @@ +/** + * Wiring tests: tool factory handlers must consolidate multi-block output under Claude Code. + * + * These tests mock the environment detector so that isRunningUnderClaudeCode() returns true, + * then verify the factory-produced handlers consolidate multi-block responses into a single + * text block. This is the centralised location for consolidation — individual logic functions + * no longer call consolidateContentForClaudeCode themselves. + */ +import { describe, it, expect, vi } from 'vitest'; +import * as z from 'zod'; +import { createMockExecutor } from '../../test-utils/mock-executors.ts'; +import type { ToolResponse } from '../../types/common.ts'; +import type { CommandExecutor } from '../command.ts'; + +vi.mock('../environment.ts', async (importOriginal) => { + const actual = (await importOriginal()) as Record; + return { + ...actual, + getDefaultEnvironmentDetector: () => ({ + isRunningUnderClaudeCode: () => true, + }), + }; +}); + +const { createTypedTool, createSessionAwareTool } = await import('../typed-tool-factory.ts'); + +const testSchema = z.object({ + name: z.string(), +}); + +type TestParams = z.infer; + +function multiBlockResponse(): ToolResponse { + return { + content: [ + { type: 'text', text: 'Block 1' }, + { type: 'text', text: 'Block 2' }, + { type: 'text', text: 'Block 3' }, + ], + }; +} + +function singleBlockResponse(): ToolResponse { + return { + content: [{ type: 'text', text: 'Only block' }], + }; +} + +describe('createTypedTool — Claude Code consolidation wiring', () => { + it('should consolidate multi-block response into a single text block', async () => { + const handler = createTypedTool( + testSchema, + async (_params: TestParams, _executor: CommandExecutor) => multiBlockResponse(), + () => createMockExecutor({ success: true, output: '' }), + ); + + const result = await handler({ name: 'test' }); + + expect(result.content).toHaveLength(1); + expect(result.content[0].type).toBe('text'); + const text = (result.content[0] as { type: 'text'; text: string }).text; + expect(text).toContain('Block 1'); + expect(text).toContain('Block 2'); + expect(text).toContain('Block 3'); + }); + + it('should leave single-block response unchanged', async () => { + const handler = createTypedTool( + testSchema, + async (_params: TestParams, _executor: CommandExecutor) => singleBlockResponse(), + () => createMockExecutor({ success: true, output: '' }), + ); + + const result = await handler({ name: 'test' }); + + expect(result.content).toHaveLength(1); + expect((result.content[0] as { type: 'text'; text: string }).text).toBe('Only block'); + }); +}); + +describe('createSessionAwareTool — Claude Code consolidation wiring', () => { + it('should consolidate multi-block response into a single text block', async () => { + const handler = createSessionAwareTool({ + internalSchema: testSchema, + logicFunction: async (_params: TestParams, _executor: CommandExecutor) => + multiBlockResponse(), + getExecutor: () => createMockExecutor({ success: true, output: '' }), + }); + + const result = await handler({ name: 'test' }); + + expect(result.content).toHaveLength(1); + expect(result.content[0].type).toBe('text'); + const text = (result.content[0] as { type: 'text'; text: string }).text; + expect(text).toContain('Block 1'); + expect(text).toContain('Block 2'); + expect(text).toContain('Block 3'); + }); + + it('should leave single-block response unchanged', async () => { + const handler = createSessionAwareTool({ + internalSchema: testSchema, + logicFunction: async (_params: TestParams, _executor: CommandExecutor) => + singleBlockResponse(), + getExecutor: () => createMockExecutor({ success: true, output: '' }), + }); + + const result = await handler({ name: 'test' }); + + expect(result.content).toHaveLength(1); + expect((result.content[0] as { type: 'text'; text: string }).text).toBe('Only block'); + }); +}); diff --git a/src/utils/build-utils.ts b/src/utils/build-utils.ts index 255f6207..41cb486a 100644 --- a/src/utils/build-utils.ts +++ b/src/utils/build-utils.ts @@ -21,7 +21,7 @@ import { log } from './logger.ts'; import { XcodePlatform, constructDestinationString } from './xcode.ts'; import type { CommandExecutor, CommandExecOptions } from './command.ts'; import type { ToolResponse, SharedBuildParams, PlatformBuildOptions } from '../types/common.ts'; -import { createTextResponse, consolidateContentForClaudeCode } from './validation.ts'; +import { createTextResponse } from './validation.ts'; import { isXcodemakeEnabled, isXcodemakeAvailable, @@ -306,7 +306,7 @@ export async function executeXcodeBuildCommand( }); } - return consolidateContentForClaudeCode(errorResponse); + return errorResponse; } log('info', `✅ ${platformOptions.logPrefix} ${buildAction} succeeded.`); @@ -369,7 +369,7 @@ Future builds will use the generated Makefile for improved performance. }); } - return consolidateContentForClaudeCode(successResponse); + return successResponse; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); @@ -382,11 +382,9 @@ Future builds will use the generated Makefile for improved performance. sentry: !isSpawnError, }); - return consolidateContentForClaudeCode( - createTextResponse( - `Error during ${platformOptions.logPrefix} ${buildAction}: ${errorMessage}`, - true, - ), + return createTextResponse( + `Error during ${platformOptions.logPrefix} ${buildAction}: ${errorMessage}`, + true, ); } } diff --git a/src/utils/environment.ts b/src/utils/environment.ts index 82aef678..8af6226e 100644 --- a/src/utils/environment.ts +++ b/src/utils/environment.ts @@ -25,14 +25,22 @@ export interface EnvironmentDetector { * Production implementation of environment detection */ export class ProductionEnvironmentDetector implements EnvironmentDetector { + private cachedResult: boolean | undefined; + isRunningUnderClaudeCode(): boolean { + if (this.cachedResult !== undefined) { + return this.cachedResult; + } + // Disable Claude Code detection during tests for environment-agnostic testing if (process.env.NODE_ENV === 'test' || process.env.VITEST === 'true') { + this.cachedResult = false; return false; } // Method 1: Check for Claude Code environment variables if (process.env.CLAUDECODE === '1' || process.env.CLAUDE_CODE_ENTRYPOINT === 'cli') { + this.cachedResult = true; return true; } @@ -45,6 +53,7 @@ export class ProductionEnvironmentDetector implements EnvironmentDetector { timeout: 1000, }).trim(); if (parentCommand.includes('claude')) { + this.cachedResult = true; return true; } } @@ -53,6 +62,7 @@ export class ProductionEnvironmentDetector implements EnvironmentDetector { log('debug', `Failed to detect parent process: ${error}`); } + this.cachedResult = false; return false; } } diff --git a/src/utils/logger.ts b/src/utils/logger.ts index 628a7b56..2a5ec9e2 100644 --- a/src/utils/logger.ts +++ b/src/utils/logger.ts @@ -144,6 +144,15 @@ export function normalizeLogLevel(raw: string): LogLevel | null { return null; } +/** + * Yargs coerce function for log-level options. + * Maps the deprecated 'warning' value to 'warn' for backwards compatibility. + */ +export function coerceLogLevel(value: unknown): unknown { + if (typeof value !== 'string') return value; + return value.trim().toLowerCase() === 'warning' ? 'warn' : value; +} + /** * Set the minimum log level for client-requested filtering * @param level The minimum log level to output diff --git a/src/utils/test-common.ts b/src/utils/test-common.ts index 8703f134..90411e6a 100644 --- a/src/utils/test-common.ts +++ b/src/utils/test-common.ts @@ -12,19 +12,17 @@ * - Temporary directory management for xcresult files */ -import { promisify } from 'util'; -import { exec } from 'child_process'; -import { mkdtemp, rm } from 'fs/promises'; -import { tmpdir } from 'os'; import { join } from 'path'; import { log } from './logger.ts'; import type { XcodePlatform } from './xcode.ts'; import { executeXcodeBuildCommand } from './build/index.ts'; -import { createTextResponse, consolidateContentForClaudeCode } from './validation.ts'; +import { createTextResponse } from './validation.ts'; import { normalizeTestRunnerEnv } from './environment.ts'; import type { ToolResponse } from '../types/common.ts'; import type { CommandExecutor, CommandExecOptions } from './command.ts'; -import { getDefaultCommandExecutor } from './command.ts'; +import { getDefaultCommandExecutor, getDefaultFileSystemExecutor } from './command.ts'; +import type { FileSystemExecutor } from './FileSystemExecutor.ts'; +import { filterStderrContent, type XcresultSummary } from './test-result-content.ts'; /** * Type definition for test summary structure from xcresulttool @@ -59,16 +57,27 @@ interface TestSummary { /** * Parse xcresult bundle using xcrun xcresulttool */ -export async function parseXcresultBundle(resultBundlePath: string): Promise { +export async function parseXcresultBundle( + resultBundlePath: string, + executor: CommandExecutor = getDefaultCommandExecutor(), +): Promise { try { - const execAsync = promisify(exec); - const { stdout } = await execAsync( - `xcrun xcresulttool get test-results summary --path "${resultBundlePath}"`, + const result = await executor( + ['xcrun', 'xcresulttool', 'get', 'test-results', 'summary', '--path', resultBundlePath], + 'Parse xcresult bundle', + true, ); + if (!result.success) { + throw new Error(result.error ?? 'Failed to parse xcresult bundle'); + } + // Parse JSON response and format as human-readable - const summary = JSON.parse(stdout) as TestSummary; - return formatTestSummary(summary); + const summary = JSON.parse(result.output || '{}') as TestSummary; + return { + formatted: formatTestSummary(summary), + totalTestCount: typeof summary.totalTestCount === 'number' ? summary.totalTestCount : 0, + }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); log('error', `Error parsing xcresult bundle: ${errorMessage}`); @@ -161,7 +170,8 @@ export async function handleTestLogic( platform: XcodePlatform; testRunnerEnv?: Record; }, - executor?: CommandExecutor, + executor: CommandExecutor = getDefaultCommandExecutor(), + fileSystemExecutor: FileSystemExecutor = getDefaultFileSystemExecutor(), ): Promise { log( 'info', @@ -170,7 +180,9 @@ export async function handleTestLogic( try { // Create temporary directory for xcresult bundle - const tempDir = await mkdtemp(join(tmpdir(), 'xcodebuild-test-')); + const tempDir = await fileSystemExecutor.mkdtemp( + join(fileSystemExecutor.tmpdir(), 'xcodebuild-test-'), + ); const resultBundlePath = join(tempDir, 'TestResults.xcresult'); // Add resultBundlePath to extraArgs @@ -197,7 +209,7 @@ export async function handleTestLogic( }, params.preferXcodebuild, 'test', - executor ?? getDefaultCommandExecutor(), + executor, execOpts, ); @@ -208,52 +220,56 @@ export async function handleTestLogic( // Check if the file exists try { - const { stat } = await import('fs/promises'); - await stat(resultBundlePath); + await fileSystemExecutor.stat(resultBundlePath); log('info', `xcresult bundle exists at: ${resultBundlePath}`); } catch { log('warn', `xcresult bundle does not exist at: ${resultBundlePath}`); throw new Error(`xcresult bundle not found at ${resultBundlePath}`); } - const testSummary = await parseXcresultBundle(resultBundlePath); + const xcresult = await parseXcresultBundle(resultBundlePath, executor); log('info', 'Successfully parsed xcresult bundle'); // Clean up temporary directory - await rm(tempDir, { recursive: true, force: true }); + await fileSystemExecutor.rm(tempDir, { recursive: true, force: true }); - // Return combined result - preserve isError from testResult (test failures should be marked as errors) + // If no tests ran (for example build/setup failed), xcresult summary is not useful. + // Return raw output so the original diagnostics stay visible. + if (xcresult.totalTestCount === 0) { + log('info', 'xcresult reports 0 tests — returning raw build output'); + return testResult; + } + + // xcresult summary should be first. Drop stderr-only noise while preserving non-stderr lines. + const filteredContent = filterStderrContent(testResult.content); const combinedResponse: ToolResponse = { content: [ - ...(testResult.content || []), { type: 'text', - text: '\nTest Results Summary:\n' + testSummary, + text: '\nTest Results Summary:\n' + xcresult.formatted, }, + ...filteredContent, ], isError: testResult.isError, }; - // Apply Claude Code workaround if enabled - return consolidateContentForClaudeCode(combinedResponse); + return combinedResponse; } catch (parseError) { // If parsing fails, return original test result log('warn', `Failed to parse xcresult bundle: ${parseError}`); // Clean up temporary directory even if parsing fails try { - await rm(tempDir, { recursive: true, force: true }); + await fileSystemExecutor.rm(tempDir, { recursive: true, force: true }); } catch (cleanupError) { log('warn', `Failed to clean up temporary directory: ${cleanupError}`); } - return consolidateContentForClaudeCode(testResult); + return testResult; } } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); log('error', `Error during test run: ${errorMessage}`); - return consolidateContentForClaudeCode( - createTextResponse(`Error during test run: ${errorMessage}`, true), - ); + return createTextResponse(`Error during test run: ${errorMessage}`, true); } } diff --git a/src/utils/test-result-content.ts b/src/utils/test-result-content.ts new file mode 100644 index 00000000..7cbde0c1 --- /dev/null +++ b/src/utils/test-result-content.ts @@ -0,0 +1,32 @@ +import type { ToolResponseContent } from '../types/common.ts'; + +export interface XcresultSummary { + formatted: string; + totalTestCount: number; +} + +export function filterStderrContent( + content: ToolResponseContent[] | undefined, +): ToolResponseContent[] { + if (!content) { + return []; + } + + return content.flatMap((item): ToolResponseContent[] => { + if (item.type !== 'text') { + return [item]; + } + + const filteredText = item.text + .split('\n') + .filter((line) => !line.includes('[stderr]')) + .join('\n') + .trim(); + + if (filteredText.length === 0) { + return []; + } + + return [{ ...item, text: filteredText }]; + }); +} diff --git a/src/utils/typed-tool-factory.ts b/src/utils/typed-tool-factory.ts index 8e86addd..4f1b4f04 100644 --- a/src/utils/typed-tool-factory.ts +++ b/src/utils/typed-tool-factory.ts @@ -13,6 +13,7 @@ import * as z from 'zod'; import type { ToolResponse } from '../types/common.ts'; import type { CommandExecutor } from './execution/index.ts'; import { createErrorResponse } from './responses/index.ts'; +import { consolidateContentForClaudeCode } from './validation.ts'; import { sessionStore, type SessionDefaults } from './session-store.ts'; import { isSessionDefaultsOptOutEnabled } from './environment.ts'; @@ -25,7 +26,8 @@ function createValidatedHandler( try { const validatedParams = schema.parse(args); - return await logicFunction(validatedParams, getContext()); + const response = await logicFunction(validatedParams, getContext()); + return consolidateContentForClaudeCode(response); } catch (error) { if (error instanceof z.ZodError) { const details = `Invalid parameters:\n${formatZodIssues(error)}`; @@ -242,7 +244,8 @@ function createSessionAwareHandler(opts: { } const validated = internalSchema.parse(merged); - return await logicFunction(validated, getContext()); + const response = await logicFunction(validated, getContext()); + return consolidateContentForClaudeCode(response); } catch (error) { if (error instanceof z.ZodError) { const details = `Invalid parameters:\n${formatZodIssues(error)}`; diff --git a/src/utils/validation.ts b/src/utils/validation.ts index 55922674..9d40b9fd 100644 --- a/src/utils/validation.ts +++ b/src/utils/validation.ts @@ -25,6 +25,7 @@ import { log } from './logger.ts'; import type { ToolResponse, ValidationResult } from '../types/common.ts'; import type { FileSystemExecutor } from './FileSystemExecutor.ts'; import { getDefaultEnvironmentDetector } from './environment.ts'; +import type { EnvironmentDetector } from './environment.ts'; /** * Creates a text response with the given message @@ -218,9 +219,14 @@ export function validateEnumParam( * @param response The original ToolResponse with multiple content blocks * @returns A new ToolResponse with consolidated content */ -export function consolidateContentForClaudeCode(response: ToolResponse): ToolResponse { +export function consolidateContentForClaudeCode( + response: ToolResponse, + detector?: EnvironmentDetector, +): ToolResponse { // Automatically detect if running under Claude Code - const shouldConsolidate = getDefaultEnvironmentDetector().isRunningUnderClaudeCode(); + const shouldConsolidate = ( + detector ?? getDefaultEnvironmentDetector() + ).isRunningUnderClaudeCode(); if (!shouldConsolidate || !response.content || response.content.length <= 1) { return response;