Skip to content

Commit 3611437

Browse files
committed
♻️ refactor: simplify xcresult parsing, init policy, and stale comments
- 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
1 parent 8fb702c commit 3611437

4 files changed

Lines changed: 35 additions & 107 deletions

File tree

src/cli/commands/init.ts

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -626,20 +626,17 @@ export function registerInitCommand(app: Argv, ctx?: { workspaceRoot: string }):
626626
}
627627
}
628628

629+
const agentsGuidance =
630+
agentsGuidanceStatus && agentsGuidancePath
631+
? { status: agentsGuidanceStatus, path: agentsGuidancePath, error: agentsGuidanceError }
632+
: undefined;
633+
629634
const report: InitReport = {
630635
action: 'install',
631636
skillType: selection.skillType,
632637
installed: results,
633638
skipped: policy.skippedClients,
634-
...(agentsGuidanceStatus && agentsGuidancePath
635-
? {
636-
agentsGuidance: {
637-
status: agentsGuidanceStatus,
638-
path: agentsGuidancePath,
639-
...(agentsGuidanceError ? { error: agentsGuidanceError } : {}),
640-
},
641-
}
642-
: {}),
639+
...(agentsGuidance ? { agentsGuidance } : {}),
643640
message: `Installed ${skillDisplayName(selection.skillType)} skill`,
644641
};
645642

@@ -675,15 +672,13 @@ function enforceInstallPolicy(
675672
destFlag: string | undefined,
676673
selectionMode: InitSelection['selectionMode'],
677674
): InstallPolicyResult {
678-
if (skillType !== 'mcp') {
679-
return { allowedTargets: targets, skippedClients: [] };
680-
}
681-
682-
if (destFlag) {
683-
return { allowedTargets: targets, skippedClients: [] };
684-
}
675+
const skipPolicy =
676+
skillType !== 'mcp' ||
677+
destFlag != null ||
678+
clientFlag === 'claude' ||
679+
selectionMode === 'interactive';
685680

686-
if (clientFlag === 'claude' || selectionMode === 'interactive') {
681+
if (skipPolicy) {
687682
return { allowedTargets: targets, skippedClients: [] };
688683
}
689684

src/mcp/tools/device/__tests__/test_device.test.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,7 @@
11
/**
22
* Tests for test_device plugin
33
* Following CLAUDE.md testing standards with literal validation
4-
* Using pure dependency injection for deterministic testing
5-
* NO VITEST MOCKING ALLOWED - Only createMockExecutor and manual stubs
4+
* Using dependency injection for deterministic testing
65
*/
76

87
import { describe, it, expect, beforeEach } from 'vitest';

src/mcp/tools/device/test_device.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@ async function parseXcresultBundle(
8080
executor: CommandExecutor = getDefaultCommandExecutor(),
8181
): Promise<XcresultSummary> {
8282
try {
83-
// Use injected executor for testing
8483
const result = await executor(
8584
['xcrun', 'xcresulttool', 'get', 'test-results', 'summary', '--path', resultBundlePath],
8685
'Parse xcresult bundle',

src/mcp/tools/macos/test_macos.ts

Lines changed: 22 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -68,22 +68,6 @@ const testMacosSchema = z.preprocess(
6868

6969
export type TestMacosParams = z.infer<typeof testMacosSchema>;
7070

71-
/**
72-
* Type definition for test summary structure from xcresulttool
73-
* @typedef {Object} TestSummary
74-
* @property {string} [title]
75-
* @property {string} [result]
76-
* @property {number} [totalTestCount]
77-
* @property {number} [passedTests]
78-
* @property {number} [failedTests]
79-
* @property {number} [skippedTests]
80-
* @property {number} [expectedFailures]
81-
* @property {string} [environmentDescription]
82-
* @property {Array<Object>} [devicesAndConfigurations]
83-
* @property {Array<Object>} [testFailures]
84-
* @property {Array<Object>} [topInsights]
85-
*/
86-
8771
/**
8872
* Parse xcresult bundle using xcrun xcresulttool
8973
*/
@@ -103,22 +87,10 @@ async function parseXcresultBundle(
10387
}
10488

10589
// Parse JSON response and format as human-readable
106-
let summary: unknown;
107-
try {
108-
summary = JSON.parse(result.output || '{}');
109-
} catch (parseError) {
110-
throw new Error(`Failed to parse JSON output: ${parseError}`);
111-
}
112-
113-
if (typeof summary !== 'object' || summary === null) {
114-
throw new Error('Invalid JSON output: expected object');
115-
}
116-
117-
const summaryRecord = summary as Record<string, unknown>;
90+
const summary = JSON.parse(result.output || '{}') as Record<string, unknown>;
11891
return {
119-
formatted: formatTestSummary(summaryRecord),
120-
totalTestCount:
121-
typeof summaryRecord.totalTestCount === 'number' ? summaryRecord.totalTestCount : 0,
92+
formatted: formatTestSummary(summary),
93+
totalTestCount: typeof summary.totalTestCount === 'number' ? summary.totalTestCount : 0,
12294
};
12395
} catch (error) {
12496
const errorMessage = error instanceof Error ? error.message : String(error);
@@ -155,31 +127,13 @@ function formatTestSummary(summary: Record<string, unknown>): string {
155127
Array.isArray(summary.devicesAndConfigurations) &&
156128
summary.devicesAndConfigurations.length > 0
157129
) {
158-
const firstDeviceConfig: unknown = summary.devicesAndConfigurations[0];
159-
if (
160-
typeof firstDeviceConfig === 'object' &&
161-
firstDeviceConfig !== null &&
162-
'device' in firstDeviceConfig
163-
) {
164-
const device: unknown = (firstDeviceConfig as Record<string, unknown>).device;
165-
if (typeof device === 'object' && device !== null) {
166-
const deviceRecord = device as Record<string, unknown>;
167-
const deviceName =
168-
'deviceName' in deviceRecord && typeof deviceRecord.deviceName === 'string'
169-
? deviceRecord.deviceName
170-
: 'Unknown';
171-
const platform =
172-
'platform' in deviceRecord && typeof deviceRecord.platform === 'string'
173-
? deviceRecord.platform
174-
: 'Unknown';
175-
const osVersion =
176-
'osVersion' in deviceRecord && typeof deviceRecord.osVersion === 'string'
177-
? deviceRecord.osVersion
178-
: 'Unknown';
179-
180-
lines.push(`Device: ${deviceName} (${platform} ${osVersion})`);
181-
lines.push('');
182-
}
130+
const deviceConfig = summary.devicesAndConfigurations[0] as Record<string, unknown>;
131+
const device = deviceConfig.device as Record<string, unknown> | undefined;
132+
if (device) {
133+
lines.push(
134+
`Device: ${device.deviceName ?? 'Unknown'} (${device.platform ?? 'Unknown'} ${device.osVersion ?? 'Unknown'})`,
135+
);
136+
lines.push('');
183137
}
184138
}
185139

@@ -189,44 +143,25 @@ function formatTestSummary(summary: Record<string, unknown>): string {
189143
summary.testFailures.length > 0
190144
) {
191145
lines.push('Test Failures:');
192-
summary.testFailures.forEach((failure: unknown, index: number) => {
193-
if (typeof failure === 'object' && failure !== null) {
194-
const failureRecord = failure as Record<string, unknown>;
195-
const testName =
196-
'testName' in failureRecord && typeof failureRecord.testName === 'string'
197-
? failureRecord.testName
198-
: 'Unknown Test';
199-
const targetName =
200-
'targetName' in failureRecord && typeof failureRecord.targetName === 'string'
201-
? failureRecord.targetName
202-
: 'Unknown Target';
203-
204-
lines.push(` ${index + 1}. ${testName} (${targetName})`);
205-
206-
if ('failureText' in failureRecord && typeof failureRecord.failureText === 'string') {
207-
lines.push(` ${failureRecord.failureText}`);
208-
}
146+
summary.testFailures.forEach((failureItem, index: number) => {
147+
const failure = failureItem as Record<string, unknown>;
148+
lines.push(
149+
` ${index + 1}. ${failure.testName ?? 'Unknown Test'} (${failure.targetName ?? 'Unknown Target'})`,
150+
);
151+
if (failure.failureText) {
152+
lines.push(` ${failure.failureText}`);
209153
}
210154
});
211155
lines.push('');
212156
}
213157

214158
if (summary.topInsights && Array.isArray(summary.topInsights) && summary.topInsights.length > 0) {
215159
lines.push('Insights:');
216-
summary.topInsights.forEach((insight: unknown, index: number) => {
217-
if (typeof insight === 'object' && insight !== null) {
218-
const insightRecord = insight as Record<string, unknown>;
219-
const impact =
220-
'impact' in insightRecord && typeof insightRecord.impact === 'string'
221-
? insightRecord.impact
222-
: 'Unknown';
223-
const text =
224-
'text' in insightRecord && typeof insightRecord.text === 'string'
225-
? insightRecord.text
226-
: 'No description';
227-
228-
lines.push(` ${index + 1}. [${impact}] ${text}`);
229-
}
160+
summary.topInsights.forEach((insightItem, index: number) => {
161+
const insight = insightItem as Record<string, unknown>;
162+
lines.push(
163+
` ${index + 1}. [${insight.impact ?? 'Unknown'}] ${insight.text ?? 'No description'}`,
164+
);
230165
});
231166
}
232167

0 commit comments

Comments
 (0)