Skip to content

Commit 724e603

Browse files
committed
fix: guard the AMBIGUOUS_MATCH candidate renderer against device-domain shapes
Review on #1602 (P2): formatAmbiguousMatchCandidateLines ran for every normalized error and stringified details.candidates unconditionally, but device-domain AMBIGUOUS_MATCH/APP_NOT_INSTALLED errors (findBootedAppleSimulatorWithApp, src/core/dispatch-resolve.ts) reuse that key for { id, name } device objects with no `matches` field — CLI and MCP would have printed "Candidates: [object Object]" for those. The renderer now requires numeric details.matches AND every candidate to be a string before rendering anything, restricting it to buildAmbiguousMatchError's element-match shape; unrecognized shapes render nothing, same as before this feature existed. Added regression tests against the exact device-error shape on both text surfaces. Also unexports AMBIGUOUS_MATCH_CANDIDATE_LIMIT (fallow flagged it as an unused production export) — it has no consumer outside find.ts.
1 parent e3a20a9 commit 724e603

4 files changed

Lines changed: 82 additions & 3 deletions

File tree

src/daemon/handlers/find.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -547,7 +547,9 @@ function publicFindFlags(flags: DaemonRequest['flags']): Record<string, unknown>
547547
// label/identifier. Capped at AMBIGUOUS_MATCH_CANDIDATE_LIMIT to bound the
548548
// error payload — `matches` (the true total) is what a "+N more" marker is
549549
// computed from at render time (src/utils/output.ts, src/mcp/tool-error.ts).
550-
export const AMBIGUOUS_MATCH_CANDIDATE_LIMIT = 5;
550+
// Module-local: no consumer outside this file needs the raw cap, only the
551+
// already-capped `candidates` array on the response.
552+
const AMBIGUOUS_MATCH_CANDIDATE_LIMIT = 5;
551553

552554
// Exported as the single AMBIGUOUS_MATCH producer so the help-benchmark
553555
// sample parity test renders the exact error this handler returns; a message

src/mcp/__tests__/tool-error.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,3 +37,26 @@ test('formatToolErrorText omits the candidates block for non-ambiguous errors',
3737

3838
assert.equal(text.includes('Candidates:'), false);
3939
});
40+
41+
// P2 review on #1597: device-domain AMBIGUOUS_MATCH (findBootedAppleSimulatorWithApp,
42+
// src/core/dispatch-resolve.ts) reuses `details.candidates` for `{ id, name }`
43+
// device objects with no `matches` field — must never render as
44+
// "Candidates:\n [object Object]" on the MCP text path either.
45+
test('formatToolErrorText renders device-domain candidate objects as nothing, never [object Object]', () => {
46+
const err = new AppError(
47+
'AMBIGUOUS_MATCH',
48+
'Multiple booted iOS simulators have com.example.app installed',
49+
{
50+
appTarget: 'com.example.app',
51+
candidates: [
52+
{ id: 'SIM-001', name: 'iPhone 17 Pro' },
53+
{ id: 'SIM-002', name: 'iPhone 17' },
54+
],
55+
},
56+
);
57+
58+
const text = formatToolErrorText(normalizeToolError(err));
59+
60+
assert.equal(text.includes('[object Object]'), false);
61+
assert.equal(text.includes('Candidates:'), false);
62+
});

src/utils/__tests__/output.test.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1786,6 +1786,48 @@ test('formatAmbiguousMatchCandidateLines returns nothing when details carry no c
17861786
assert.deepEqual(formatAmbiguousMatchCandidateLines({ candidates: [] }), []);
17871787
});
17881788

1789+
// P2 review on #1597: `details.candidates` is not unique to the find
1790+
// handler's element-match shape. The device-domain resolver
1791+
// (findBootedAppleSimulatorWithApp, src/core/dispatch-resolve.ts) reuses the
1792+
// same key for `{ id, name }` device objects on both AMBIGUOUS_MATCH and
1793+
// APP_NOT_INSTALLED, and never sets `details.matches` — this must render
1794+
// nothing (its behavior before this renderer existed), never
1795+
// "Candidates:\n [object Object]".
1796+
test('printHumanError renders device-domain candidate objects as nothing, never [object Object]', () => {
1797+
const deviceCandidates = [
1798+
{ id: 'SIM-001', name: 'iPhone 17 Pro' },
1799+
{ id: 'SIM-002', name: 'iPhone 17' },
1800+
];
1801+
1802+
const ambiguousDeviceErr = new AppError(
1803+
'AMBIGUOUS_MATCH',
1804+
'Multiple booted iOS simulators have com.example.app installed',
1805+
{ appTarget: 'com.example.app', candidates: deviceCandidates },
1806+
);
1807+
const ambiguousOutput = captureStderr(() => printHumanError(ambiguousDeviceErr));
1808+
assert.equal(ambiguousOutput.includes('[object Object]'), false);
1809+
assert.equal(ambiguousOutput.includes('Candidates:'), false);
1810+
1811+
const notInstalledErr = new AppError(
1812+
'APP_NOT_INSTALLED',
1813+
'No booted iOS simulator has com.example.app installed',
1814+
{ appTarget: 'com.example.app', candidates: deviceCandidates },
1815+
);
1816+
const notInstalledOutput = captureStderr(() => printHumanError(notInstalledErr));
1817+
assert.equal(notInstalledOutput.includes('[object Object]'), false);
1818+
assert.equal(notInstalledOutput.includes('Candidates:'), false);
1819+
1820+
// The formatter itself, not just the CLI render, must reject this shape —
1821+
// both the missing `matches` and the object-shaped entries disqualify it.
1822+
assert.deepEqual(
1823+
formatAmbiguousMatchCandidateLines({
1824+
appTarget: 'com.example.app',
1825+
candidates: deviceCandidates,
1826+
}),
1827+
[],
1828+
);
1829+
});
1830+
17891831
test('printHumanError shows an unavailable screen reason and omitted suggestions hint', () => {
17901832
const err = new AppError('REPLAY_DIVERGENCE', 'Replay failed at step 1', {
17911833
divergence: {

src/utils/output.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,16 +70,28 @@ export function printHumanError(
7070
// AMBIGUOUS_MATCH_CANDIDATE_LIMIT by buildAmbiguousMatchError,
7171
// src/daemon/handlers/find.ts) and `details.matches` (the true total) to
7272
// compute the "+N more" marker for whatever the cap omitted.
73+
//
74+
// `details.candidates` is NOT unique to that one producer: the device-domain
75+
// AMBIGUOUS_MATCH/APP_NOT_INSTALLED resolvers (findBootedAppleSimulatorWithApp,
76+
// src/core/dispatch-resolve.ts) reuse the same key for `{ id, name }` device
77+
// objects, and never set `details.matches` at all. Both guards below —
78+
// numeric `matches` and every candidate being a pre-rendered string — must
79+
// hold together, or this renders "[object Object]" for that shape instead of
80+
// silently rendering nothing (its prior, pre-#1597 behavior).
7381
export function formatAmbiguousMatchCandidateLines(
7482
details: Record<string, unknown> | undefined,
7583
): string[] {
7684
const candidates = details?.candidates;
7785
if (!Array.isArray(candidates) || candidates.length === 0) return [];
78-
const totalMatches = typeof details?.matches === 'number' ? details.matches : candidates.length;
86+
if (typeof details?.matches !== 'number') return [];
87+
if (!candidates.every((candidate): candidate is string => typeof candidate === 'string')) {
88+
return [];
89+
}
90+
const totalMatches = details.matches;
7991
const remaining = totalMatches - candidates.length;
8092
return [
8193
'Candidates:',
82-
...candidates.map((candidate) => ` ${String(candidate)}`),
94+
...candidates.map((candidate) => ` ${candidate}`),
8395
...(remaining > 0 ? [` +${remaining} more`] : []),
8496
];
8597
}

0 commit comments

Comments
 (0)