From 1b9fa1c9edf63d6effe471cb6ac44c7830dc07f8 Mon Sep 17 00:00:00 2001 From: Carlo Zottmann Date: Sat, 28 Feb 2026 16:30:41 +0100 Subject: [PATCH 1/6] fix: classify stderr warnings correctly and prioritize xcresult output When xcresult parsing succeeds and tests actually ran (totalTestCount > 0), stderr lines are redundant noise (e.g. "multiple matching destinations") and are filtered out. The xcresult summary is placed first in the response. When xcresult reports 0 tests (build failed before tests could run), the xcresult is meaningless and stderr is preserved since it contains the actual compilation errors. Fixes #231 --- CHANGELOG.md | 1 + .../device/__tests__/test_device.test.ts | 84 +++++++++-- src/mcp/tools/device/test_device.ts | 32 +++- .../tools/macos/__tests__/test_macos.test.ts | 137 ++++++++++++++++++ src/mcp/tools/macos/test_macos.ts | 33 ++++- src/utils/test-common.ts | 31 +++- 6 files changed, 290 insertions(+), 28 deletions(-) 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/mcp/tools/device/__tests__/test_device.test.ts b/src/mcp/tools/device/__tests__/test_device.test.ts index 1179dda4..6851a35b 100644 --- a/src/mcp/tools/device/__tests__/test_device.test.ts +++ b/src/mcp/tools/device/__tests__/test_device.test.ts @@ -168,9 +168,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 +214,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 +264,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 +362,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 +401,7 @@ describe('test_device plugin', () => { ); expect(result.content).toHaveLength(2); - expect(result.content[0].text).toContain('✅'); + 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..a6e9d042 100644 --- a/src/mcp/tools/device/test_device.ts +++ b/src/mcp/tools/device/test_device.ts @@ -76,13 +76,18 @@ const publicSchemaObject = baseSchemaObject.omit({ * (JavaScript implementation - no actual interface, this is just documentation) */ +interface XcresultSummary { + formatted: string; + totalTestCount: number; +} + /** * 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( @@ -98,7 +103,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 +261,31 @@ 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 (e.g. build failed), the xcresult is empty/meaningless. + // Fall back to the original response which contains the actual build errors. + if (xcresult.totalTestCount === 0) { + log('info', 'xcresult reports 0 tests — falling back to raw build output'); + return testResult; + } + + // When xcresult has real test data, it's the authoritative source. + // Drop stderr lines — they're redundant noise (e.g. "multiple matching destinations"). + const filteredContent = (testResult.content || []).filter( + (item) => item.type !== 'text' || !item.text.includes('[stderr]'), + ); 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..67790683 100644 --- a/src/mcp/tools/macos/test_macos.ts +++ b/src/mcp/tools/macos/test_macos.ts @@ -86,10 +86,15 @@ export type TestMacosParams = z.infer; /** * Parse xcresult bundle using xcrun xcresulttool */ +interface XcresultSummary { + formatted: string; + totalTestCount: number; +} + async function parseXcresultBundle( resultBundlePath: string, executor: CommandExecutor = getDefaultCommandExecutor(), -): Promise { +): Promise { try { const result = await executor( ['xcrun', 'xcresulttool', 'get', 'test-results', 'summary', '--path', resultBundlePath], @@ -113,7 +118,12 @@ async function parseXcresultBundle( throw new Error('Invalid JSON output: expected object'); } - return formatTestSummary(summary as Record); + const summaryRecord = summary as Record; + return { + formatted: formatTestSummary(summaryRecord), + totalTestCount: + typeof summaryRecord.totalTestCount === 'number' ? summaryRecord.totalTestCount : 0, + }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); log('error', `Error parsing xcresult bundle: ${errorMessage}`); @@ -287,20 +297,31 @@ 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 (e.g. build failed), the xcresult is empty/meaningless. + // Fall back to the original response which contains the actual build errors. + if (xcresult.totalTestCount === 0) { + log('info', 'xcresult reports 0 tests — falling back to raw build output'); + return testResult; + } + + // When xcresult has real test data, it's the authoritative source. + // Drop stderr lines — they're redundant noise (e.g. "multiple matching destinations"). + const filteredContent = (testResult.content ?? []).filter( + (item) => item.type !== 'text' || !item.text.includes('[stderr]'), + ); 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/test-common.ts b/src/utils/test-common.ts index 8703f134..ab60a15d 100644 --- a/src/utils/test-common.ts +++ b/src/utils/test-common.ts @@ -56,10 +56,15 @@ interface TestSummary { }>; } +interface XcresultSummary { + formatted: string; + totalTestCount: number; +} + /** * Parse xcresult bundle using xcrun xcresulttool */ -export async function parseXcresultBundle(resultBundlePath: string): Promise { +export async function parseXcresultBundle(resultBundlePath: string): Promise { try { const execAsync = promisify(exec); const { stdout } = await execAsync( @@ -68,7 +73,10 @@ export async function parseXcresultBundle(resultBundlePath: string): Promise item.type !== 'text' || !item.text.includes('[stderr]'), + ); 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, }; From 314dc20d9b242622cea4d7381434b3e6dc1834dd Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Mon, 2 Mar 2026 14:22:07 +0000 Subject: [PATCH 2/6] fix(test-tools): Prioritize xcresult summaries and preserve non-stderr output Make xcresult handling consistent across shared, macOS, and device test tools. Parse totalTestCount with strict numeric checks so empty/invalid values cannot suppress build-output fallback behavior. Filter stderr at line level instead of dropping entire text blocks. This preserves useful status and diagnostics while still removing stderr noise when xcresult summaries are authoritative. Extract shared stderr content filtering into a common utility to remove duplication and keep behavior aligned across test paths. Update device tool tests to assert summary-first ordering. Fixes #254 --- .../device/__tests__/test_device.test.ts | 1 + src/mcp/tools/device/test_device.ts | 14 ++++----- src/mcp/tools/macos/test_macos.ts | 24 +++++++------- src/utils/test-common.ts | 16 +++++----- src/utils/test-result-content.ts | 31 +++++++++++++++++++ 5 files changed, 56 insertions(+), 30 deletions(-) create mode 100644 src/utils/test-result-content.ts diff --git a/src/mcp/tools/device/__tests__/test_device.test.ts b/src/mcp/tools/device/__tests__/test_device.test.ts index 6851a35b..fcb7d866 100644 --- a/src/mcp/tools/device/__tests__/test_device.test.ts +++ b/src/mcp/tools/device/__tests__/test_device.test.ts @@ -401,6 +401,7 @@ describe('test_device plugin', () => { ); expect(result.content).toHaveLength(2); + expect(result.content[0].text).toContain('Test Results Summary:'); 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 a6e9d042..8c8a86c8 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 } from '../../../utils/test-result-content.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -267,18 +268,15 @@ export async function testDeviceLogic( // Clean up temporary directory await cleanup(); - // If no tests ran (e.g. build failed), the xcresult is empty/meaningless. - // Fall back to the original response which contains the actual build 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 — falling back to raw build output'); + log('info', 'xcresult reports 0 tests — returning raw build output'); return testResult; } - // When xcresult has real test data, it's the authoritative source. - // Drop stderr lines — they're redundant noise (e.g. "multiple matching destinations"). - const filteredContent = (testResult.content || []).filter( - (item) => item.type !== 'text' || !item.text.includes('[stderr]'), - ); + // xcresult summary should be first. Drop stderr-only noise while preserving non-stderr lines. + const filteredContent = filterStderrContent(testResult.content); return { content: [ { diff --git a/src/mcp/tools/macos/test_macos.ts b/src/mcp/tools/macos/test_macos.ts index 67790683..e87d6f96 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 } from '../../../utils/test-result-content.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -67,6 +68,11 @@ const testMacosSchema = z.preprocess( export type TestMacosParams = z.infer; +interface XcresultSummary { + formatted: string; + totalTestCount: number; +} + /** * Type definition for test summary structure from xcresulttool * @typedef {Object} TestSummary @@ -86,11 +92,6 @@ export type TestMacosParams = z.infer; /** * Parse xcresult bundle using xcrun xcresulttool */ -interface XcresultSummary { - formatted: string; - totalTestCount: number; -} - async function parseXcresultBundle( resultBundlePath: string, executor: CommandExecutor = getDefaultCommandExecutor(), @@ -303,18 +304,15 @@ export async function testMacosLogic( // Clean up temporary directory await fileSystemExecutor.rm(tempDir, { recursive: true, force: true }); - // If no tests ran (e.g. build failed), the xcresult is empty/meaningless. - // Fall back to the original response which contains the actual build 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 — falling back to raw build output'); + log('info', 'xcresult reports 0 tests — returning raw build output'); return testResult; } - // When xcresult has real test data, it's the authoritative source. - // Drop stderr lines — they're redundant noise (e.g. "multiple matching destinations"). - const filteredContent = (testResult.content ?? []).filter( - (item) => item.type !== 'text' || !item.text.includes('[stderr]'), - ); + // xcresult summary should be first. Drop stderr-only noise while preserving non-stderr lines. + const filteredContent = filterStderrContent(testResult.content); return { content: [ { diff --git a/src/utils/test-common.ts b/src/utils/test-common.ts index ab60a15d..1111c209 100644 --- a/src/utils/test-common.ts +++ b/src/utils/test-common.ts @@ -25,6 +25,7 @@ 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 { filterStderrContent } from './test-result-content.ts'; /** * Type definition for test summary structure from xcresulttool @@ -75,7 +76,7 @@ export async function parseXcresultBundle(resultBundlePath: string): Promise item.type !== 'text' || !item.text.includes('[stderr]'), - ); + // xcresult summary should be first. Drop stderr-only noise while preserving non-stderr lines. + const filteredContent = filterStderrContent(testResult.content); const combinedResponse: ToolResponse = { content: [ { diff --git a/src/utils/test-result-content.ts b/src/utils/test-result-content.ts new file mode 100644 index 00000000..ea3deaff --- /dev/null +++ b/src/utils/test-result-content.ts @@ -0,0 +1,31 @@ +import type { ToolResponseContent } from '../types/common.ts'; + +export function filterStderrContent( + content: ToolResponseContent[] | undefined, +): ToolResponseContent[] { + if (!content) { + return []; + } + + const filtered: ToolResponseContent[] = []; + content.forEach((item) => { + if (item.type !== 'text') { + filtered.push(item); + return; + } + + const filteredText = item.text + .split('\n') + .filter((line) => !line.includes('[stderr]')) + .join('\n') + .trim(); + + if (filteredText.length === 0) { + return; + } + + filtered.push({ ...item, text: filteredText }); + }); + + return filtered; +} From 3c643a0284f7b65b074429b4d13ac1dcd8fd0ef4 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Mon, 2 Mar 2026 23:17:22 +0000 Subject: [PATCH 3/6] fix(test-tools): clean up orphaned separators and consolidate XcresultSummary Strip orphaned `---` separator lines left behind after filtering [stderr] lines from Claude Code consolidated output. Also move the XcresultSummary interface into the shared test-result-content module to eliminate duplication. --- src/mcp/tools/device/test_device.ts | 12 +---------- src/mcp/tools/macos/test_macos.ts | 7 +------ src/utils/test-common.ts | 7 +------ src/utils/test-result-content.ts | 32 ++++++++++++++++++++++++----- 4 files changed, 30 insertions(+), 28 deletions(-) diff --git a/src/mcp/tools/device/test_device.ts b/src/mcp/tools/device/test_device.ts index 8c8a86c8..f821049f 100644 --- a/src/mcp/tools/device/test_device.ts +++ b/src/mcp/tools/device/test_device.ts @@ -27,7 +27,7 @@ import { getSessionAwareToolSchemaShape, } from '../../../utils/typed-tool-factory.ts'; import { nullifyEmptyStrings } from '../../../utils/schema-helpers.ts'; -import { filterStderrContent } from '../../../utils/test-result-content.ts'; +import { filterStderrContent, type XcresultSummary } from '../../../utils/test-result-content.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -72,16 +72,6 @@ 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) - */ - -interface XcresultSummary { - formatted: string; - totalTestCount: number; -} - /** * Parse xcresult bundle using xcrun xcresulttool */ diff --git a/src/mcp/tools/macos/test_macos.ts b/src/mcp/tools/macos/test_macos.ts index e87d6f96..a2b60fcd 100644 --- a/src/mcp/tools/macos/test_macos.ts +++ b/src/mcp/tools/macos/test_macos.ts @@ -27,7 +27,7 @@ import { getSessionAwareToolSchemaShape, } from '../../../utils/typed-tool-factory.ts'; import { nullifyEmptyStrings } from '../../../utils/schema-helpers.ts'; -import { filterStderrContent } from '../../../utils/test-result-content.ts'; +import { filterStderrContent, type XcresultSummary } from '../../../utils/test-result-content.ts'; // Unified schema: XOR between projectPath and workspacePath const baseSchemaObject = z.object({ @@ -68,11 +68,6 @@ const testMacosSchema = z.preprocess( export type TestMacosParams = z.infer; -interface XcresultSummary { - formatted: string; - totalTestCount: number; -} - /** * Type definition for test summary structure from xcresulttool * @typedef {Object} TestSummary diff --git a/src/utils/test-common.ts b/src/utils/test-common.ts index 1111c209..b0cfba9e 100644 --- a/src/utils/test-common.ts +++ b/src/utils/test-common.ts @@ -25,7 +25,7 @@ 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 { filterStderrContent } from './test-result-content.ts'; +import { filterStderrContent, type XcresultSummary } from './test-result-content.ts'; /** * Type definition for test summary structure from xcresulttool @@ -57,11 +57,6 @@ interface TestSummary { }>; } -interface XcresultSummary { - formatted: string; - totalTestCount: number; -} - /** * Parse xcresult bundle using xcrun xcresulttool */ diff --git a/src/utils/test-result-content.ts b/src/utils/test-result-content.ts index ea3deaff..4e75d7fe 100644 --- a/src/utils/test-result-content.ts +++ b/src/utils/test-result-content.ts @@ -1,5 +1,10 @@ import type { ToolResponseContent } from '../types/common.ts'; +export interface XcresultSummary { + formatted: string; + totalTestCount: number; +} + export function filterStderrContent( content: ToolResponseContent[] | undefined, ): ToolResponseContent[] { @@ -14,11 +19,28 @@ export function filterStderrContent( return; } - const filteredText = item.text - .split('\n') - .filter((line) => !line.includes('[stderr]')) - .join('\n') - .trim(); + const lines = item.text.split('\n').filter((line) => !line.includes('[stderr]')); + + // Clean up orphaned separators left by consolidateContentForClaudeCode. + // That function joins content blocks with `\n---\n`, so removing [stderr] + // lines can leave bare `---` lines stacked together or dangling at edges. + const cleaned: string[] = []; + for (const line of lines) { + if ( + line.trim() === '---' && + (cleaned.length === 0 || cleaned[cleaned.length - 1].trim() === '---') + ) { + continue; + } + cleaned.push(line); + } + + // Remove trailing separator + while (cleaned.length > 0 && cleaned[cleaned.length - 1].trim() === '---') { + cleaned.pop(); + } + + const filteredText = cleaned.join('\n').trim(); if (filteredText.length === 0) { return; From 6e2b81c5e551634afed813e3b2402a0bae4a0a1a Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Tue, 3 Mar 2026 07:52:20 +0000 Subject: [PATCH 4/6] refactor(cli): extract shared coerceLogLevel to deduplicate warning-to-warn coercers Three identical inline coerce functions in cli.ts, yargs-app.ts, and daemon.ts all mapped 'warning' to 'warn' for backwards compatibility. Extract a single coerceLogLevel export from logger.ts and reference it from all three call sites. --- src/cli.ts | 7 ++----- src/cli/commands/daemon.ts | 6 ++---- src/cli/yargs-app.ts | 7 ++----- src/utils/logger.ts | 9 +++++++++ 4 files changed, 15 insertions(+), 14 deletions(-) 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/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/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 From b220e73428b0ed1a941a1c54a5604c5d61ca2bf5 Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Tue, 3 Mar 2026 09:15:56 +0000 Subject: [PATCH 5/6] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20refactor:=20centralize?= =?UTF-8?q?=20consolidateContentForClaudeCode=20at=20tool=20factory=20boun?= =?UTF-8?q?dary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move consolidation from individual call sites (build-utils, test-common) into the typed-tool-factory handlers so it applies once at the boundary. This eliminates scattered consolidation calls and simplifies logic functions. Supporting changes: - Cache isRunningUnderClaudeCode() result since it runs on every tool call - Accept optional EnvironmentDetector in consolidation for testability - Inject CommandExecutor/FileSystemExecutor into test-common for DI - Simplify filterStderrContent now that separator cleanup is unnecessary - Add unit tests for consolidation logic and factory wiring --- .../__tests__/consolidate-content.test.ts | 172 ++++++++++++++++++ .../typed-tool-factory-consolidation.test.ts | 113 ++++++++++++ src/utils/build-utils.ts | 14 +- src/utils/environment.ts | 10 + src/utils/test-common.ts | 56 +++--- src/utils/test-result-content.ts | 39 +--- src/utils/typed-tool-factory.ts | 7 +- src/utils/validation.ts | 10 +- 8 files changed, 353 insertions(+), 68 deletions(-) create mode 100644 src/utils/__tests__/consolidate-content.test.ts create mode 100644 src/utils/__tests__/typed-tool-factory-consolidation.test.ts 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/test-common.ts b/src/utils/test-common.ts index b0cfba9e..90411e6a 100644 --- a/src/utils/test-common.ts +++ b/src/utils/test-common.ts @@ -12,19 +12,16 @@ * - 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'; /** @@ -60,15 +57,23 @@ 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; + const summary = JSON.parse(result.output || '{}') as TestSummary; return { formatted: formatTestSummary(summary), totalTestCount: typeof summary.totalTestCount === 'number' ? summary.totalTestCount : 0, @@ -165,7 +170,8 @@ export async function handleTestLogic( platform: XcodePlatform; testRunnerEnv?: Record; }, - executor?: CommandExecutor, + executor: CommandExecutor = getDefaultCommandExecutor(), + fileSystemExecutor: FileSystemExecutor = getDefaultFileSystemExecutor(), ): Promise { log( 'info', @@ -174,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 @@ -201,7 +209,7 @@ export async function handleTestLogic( }, params.preferXcodebuild, 'test', - executor ?? getDefaultCommandExecutor(), + executor, execOpts, ); @@ -212,25 +220,24 @@ 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 xcresult = 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 }); // 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 consolidateContentForClaudeCode(testResult); + return testResult; } // xcresult summary should be first. Drop stderr-only noise while preserving non-stderr lines. @@ -246,26 +253,23 @@ export async function handleTestLogic( 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 index 4e75d7fe..7cbde0c1 100644 --- a/src/utils/test-result-content.ts +++ b/src/utils/test-result-content.ts @@ -12,42 +12,21 @@ export function filterStderrContent( return []; } - const filtered: ToolResponseContent[] = []; - content.forEach((item) => { + return content.flatMap((item): ToolResponseContent[] => { if (item.type !== 'text') { - filtered.push(item); - return; + return [item]; } - const lines = item.text.split('\n').filter((line) => !line.includes('[stderr]')); - - // Clean up orphaned separators left by consolidateContentForClaudeCode. - // That function joins content blocks with `\n---\n`, so removing [stderr] - // lines can leave bare `---` lines stacked together or dangling at edges. - const cleaned: string[] = []; - for (const line of lines) { - if ( - line.trim() === '---' && - (cleaned.length === 0 || cleaned[cleaned.length - 1].trim() === '---') - ) { - continue; - } - cleaned.push(line); - } - - // Remove trailing separator - while (cleaned.length > 0 && cleaned[cleaned.length - 1].trim() === '---') { - cleaned.pop(); - } - - const filteredText = cleaned.join('\n').trim(); + const filteredText = item.text + .split('\n') + .filter((line) => !line.includes('[stderr]')) + .join('\n') + .trim(); if (filteredText.length === 0) { - return; + return []; } - filtered.push({ ...item, text: filteredText }); + return [{ ...item, text: filteredText }]; }); - - return filtered; } 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; From 90cec9a8150caae5bb94782263617e1c16c75d4e Mon Sep 17 00:00:00 2001 From: Cameron Cooke Date: Tue, 3 Mar 2026 09:16:16 +0000 Subject: [PATCH 6/6] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20refactor:=20simplify?= =?UTF-8?q?=20xcresult=20parsing,=20init=20policy,=20and=20stale=20comment?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reduce verbose type guards in test_macos formatTestSummary using direct casts and nullish coalescing - Remove unused JSDoc typedef for TestSummary in test_macos - Consolidate enforceInstallPolicy guard conditions into single check - Simplify init report agentsGuidance construction - Remove stale comments in test_device --- src/cli/commands/init.ts | 29 ++--- .../device/__tests__/test_device.test.ts | 3 +- src/mcp/tools/device/test_device.ts | 1 - src/mcp/tools/macos/test_macos.ts | 109 ++++-------------- 4 files changed, 35 insertions(+), 107 deletions(-) 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/mcp/tools/device/__tests__/test_device.test.ts b/src/mcp/tools/device/__tests__/test_device.test.ts index fcb7d866..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'; diff --git a/src/mcp/tools/device/test_device.ts b/src/mcp/tools/device/test_device.ts index f821049f..7d9732fb 100644 --- a/src/mcp/tools/device/test_device.ts +++ b/src/mcp/tools/device/test_device.ts @@ -80,7 +80,6 @@ async function parseXcresultBundle( executor: CommandExecutor = getDefaultCommandExecutor(), ): Promise { try { - // Use injected executor for testing const result = await executor( ['xcrun', 'xcresulttool', 'get', 'test-results', 'summary', '--path', resultBundlePath], 'Parse xcresult bundle', diff --git a/src/mcp/tools/macos/test_macos.ts b/src/mcp/tools/macos/test_macos.ts index a2b60fcd..1dbdf7ba 100644 --- a/src/mcp/tools/macos/test_macos.ts +++ b/src/mcp/tools/macos/test_macos.ts @@ -68,22 +68,6 @@ 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 */ @@ -103,22 +87,10 @@ 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'); - } - - const summaryRecord = summary as Record; + const summary = JSON.parse(result.output || '{}') as Record; return { - formatted: formatTestSummary(summaryRecord), - totalTestCount: - typeof summaryRecord.totalTestCount === 'number' ? summaryRecord.totalTestCount : 0, + formatted: formatTestSummary(summary), + totalTestCount: typeof summary.totalTestCount === 'number' ? summary.totalTestCount : 0, }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); @@ -155,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(''); } } @@ -189,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(''); @@ -213,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'}`, + ); }); }