Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/selectors/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
parseFindSelectorExpression,
FIND_LOCATORS,
FIND_VALUE_REQUIRED_MESSAGE,
UNSUPPORTED_FIND_ACTION_HINT,
} from './internal/find.ts';
import {
buildSelectorCandidates,
Expand Down Expand Up @@ -108,6 +109,7 @@ export {
SELECTOR_EXPRESSION_REQUIRED_MESSAGE,
SELECTOR_KEY_NAMES,
STALE_REF_HINT,
UNSUPPORTED_FIND_ACTION_HINT,
};

/** A single native runner selector suitable for direct iOS lookup. */
Expand Down
16 changes: 15 additions & 1 deletion packages/selectors/src/internal/find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,18 @@ export type ParsedFindArgs = {
/** Shared by `checkFindArgs` and `findCommand`, which validates already-parsed options. */
export const FIND_VALUE_REQUIRED_MESSAGE = 'find requires a value';

/**
* Shared by both `Unsupported find action` throw sites (this file's raw-token
* parser and `src/commands/interaction/selectors.ts`'s typed CLI reader) so
* the recovery guidance cannot drift between them. `find` has no press/
* longpress/swipe action of its own — the fix is to resolve the ref through
* `find`, then dispatch the gesture as its own top-level command.
*/
export const UNSUPPORTED_FIND_ACTION_HINT =
'find actions: click (default), focus, fill, type, exists, wait, get text, get attrs — ' +
'there is no press/longpress/swipe find action. Run find "<text>" to list matches, then ' +
'act on the resolved @ref directly, e.g. press @eNN.';

export type FindArgumentCheck =
| { ok: true; parsed: ParsedFindArgs }
| { ok: false; code: 'INVALID_ARGS'; message: string };
Expand Down Expand Up @@ -206,7 +218,9 @@ export function parseFindArgs(args: string[]): ParsedFindArgs {
const value = actionTokens.slice(1).join(' ');
return { locator, query, action: 'type', value };
}
throw new AppError('INVALID_ARGS', `Unsupported find action: ${actionTokens[0]}`);
throw new AppError('INVALID_ARGS', `Unsupported find action: ${actionTokens[0]}`, {
hint: UNSUPPORTED_FIND_ACTION_HINT,
});
}

export function parseFindSelectorExpression(locator: FindLocator, query: string): string | null {
Expand Down
6 changes: 3 additions & 3 deletions scripts/__tests__/help-conformance-sample-producers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,9 @@ export const SAMPLE_PRODUCERS: SampleProducer[] = [
sample: AMBIGUOUS_MATCH_SAMPLE,
render: () => {
const matches = [
{ ref: 'e2', label: 'Follow' },
{ ref: 'e5', label: 'Follow' },
{ ref: 'e9', label: 'Follow' },
{ ref: 'e2', type: 'Button', label: 'Follow' },
{ ref: 'e5', type: 'Button', label: 'Follow' },
{ ref: 'e9', type: 'Button', label: 'Follow' },
] as Parameters<typeof buildAmbiguousMatchError>[0];
const response = buildAmbiguousMatchError(matches, 'text', 'Follow');
assertErrorResponse(response, 'an ambiguous find');
Expand Down
9 changes: 5 additions & 4 deletions scripts/help-conformance-cases.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ Use the output already shown to determine whether the feed-search UI is present,
recovery: { code: 'AMBIGUOUS_MATCH', sample: AMBIGUOUS_MATCH_SAMPLE },
task: quiz(
AMBIGUOUS_MATCH_SAMPLE,
'The intent is to follow the @callstack.com account row. The candidate refs were not shown. What command should run next?',
'The intent is to follow the @callstack.com account row. The candidate refs shown (@e2, @e5, @e9) all carry the identical "Follow" label, so this output alone cannot tell them apart. What command should run next?',
),
expectations: ['validPlanCommands', 'fullPrefix'],
matchers: [
Expand All @@ -439,12 +439,13 @@ Use the output already shown to determine whether the feed-search UI is present,
},
],
forbidden: [
// The candidates live in error details the human output never printed,
// so a ref-targeting command here would be a guess.
// #1597: candidates now print (ref, role, label), but all 3 here share
// the exact same "Follow" label — picking any single @eN from this
// output alone would still be an unverified guess, not a resolved match.
{ id: 'noGuessedRef', pattern: /(?:^|\n)agent-device\s+(?:press|click)\s+@e\d/i },
{
id: 'noVerbatimRetry',
pattern: /(?:^|\n)agent-device\s+find\s+text\s+"?follow"?\s+press\b/i,
pattern: /(?:^|\n)agent-device\s+find\s+text\s+"?follow"?\s*(?:\n|$)/i,
},
{ id: 'noRawCoordinateTarget', pattern: RAW_COORDINATE_TARGET },
],
Expand Down
18 changes: 13 additions & 5 deletions scripts/help-conformance-sample-outputs.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,21 @@ Hint: Ref @e12 was minted from snapshot s5 but the session's ref frame is now s7

// AMBIGUOUS_MATCH from buildAmbiguousMatchError (src/daemon/handlers/find.ts)
// — the parity test drives that exact producer. The by-design rejection
// instead of silent disambiguation: candidate refs live in details, which the
// human rendering does not print, so the agent must re-observe or narrow, not
// guess a ref it never saw.
// instead of silent disambiguation: #1597 made the candidate refs (ref, role,
// label/identifier — the same compact rendering as snapshot -i) print
// unconditionally via formatAmbiguousMatchCandidateLines
// (src/utils/output.ts), capped at AMBIGUOUS_MATCH_CANDIDATE_LIMIT (5) with a
// "+N more" marker. Here all 3 candidates share the identical "Follow" label,
// so the printed refs still cannot be told apart from this output alone —
// the agent must re-observe or narrow, not guess which @ref is the right row.
export const AMBIGUOUS_MATCH_SAMPLE = {
command: 'agent-device find text "Follow" press',
command: 'agent-device find text "Follow"',
output: `Error (AMBIGUOUS_MATCH): find matched 3 elements for text "Follow". Use a more specific locator or selector.
Hint: Multiple candidates matched. Narrow the query or pass an exact identifier.`,
Hint: Multiple candidates matched. Narrow the query or pass an exact identifier.
Candidates:
@e2 [button] "Follow"
@e5 [button] "Follow"
@e9 [button] "Follow"`,
};

// APP_NOT_INSTALLED from buildAppNotInstalledError
Expand Down
1 change: 1 addition & 0 deletions scripts/layering/facade-symbols.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export const FACADE_SYMBOLS: readonly (readonly [string, readonly string[]])[] =
'SelectorProjection',
'SelectorResolution',
'SimpleSelectorTarget',
'UNSUPPORTED_FIND_ACTION_HINT',
'buildSelectorCandidates',
'buildSelectorChainForNode',
'checkElementTargetArgs',
Expand Down
25 changes: 25 additions & 0 deletions src/commands/interaction/selectors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { expect, test } from 'vitest';
import { UNSUPPORTED_FIND_ACTION_HINT } from '@agent-device/selectors';
import { AppError } from '@agent-device/kernel/errors';
import type { CliFlags } from '@agent-device/contracts/command';
import { selectorCliReaders } from './selectors.ts';

const BASE_FLAGS: CliFlags = { json: false, help: false, version: false };

// #1597: this CLI reader has its own "Unsupported find action" throw site,
// separate from packages/selectors/src/internal/find.ts's raw-positional
// parser (the daemon-side path). Both must carry the same recovery hint —
// this pins the reader actually reachable from `agent-device find <text>
// press` on the terminal.
test('find CLI reader attaches the supported-actions hint on an unsupported action', () => {
try {
selectorCliReaders.find(['text', 'Follow', 'press'], BASE_FLAGS);
expect.unreachable('find CLI reader should have thrown for an unsupported action');
} catch (error) {
expect(error).toBeInstanceOf(AppError);
const appError = error as AppError;
expect(appError.code).toBe('INVALID_ARGS');
expect(appError.message).toBe('Unsupported find action: press');
expect(appError.details?.hint).toBe(UNSUPPORTED_FIND_ACTION_HINT);
}
});
10 changes: 8 additions & 2 deletions src/commands/interaction/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import { PUBLIC_COMMANDS } from '../../command-catalog.ts';
import type { FindOptions, IsOptions } from '@agent-device/contracts/client';
import type { CliFlags } from '@agent-device/contracts/command';
import { AppError } from '@agent-device/kernel/errors';
import { checkIsPredicate, normalizeIsPositionals } from '@agent-device/selectors';
import {
checkIsPredicate,
normalizeIsPositionals,
UNSUPPORTED_FIND_ACTION_HINT,
} from '@agent-device/selectors';
import {
direct,
optionalCliNumber,
Expand Down Expand Up @@ -102,7 +106,9 @@ function readFindOptionsFromPositionals(positionals: string[], flags: CliFlags):
if (action === 'click' || action === 'focus' || action === 'exists') {
return { ...base, locator, query: readRequiredQuery(query), action };
}
throw new AppError('INVALID_ARGS', `Unsupported find action: ${action}`);
throw new AppError('INVALID_ARGS', `Unsupported find action: ${action}`, {
hint: UNSUPPORTED_FIND_ACTION_HINT,
});
}

function readIsOptionsFromPositionals(positionals: string[], flags: CliFlags): IsOptions {
Expand Down
28 changes: 28 additions & 0 deletions src/daemon/handlers/__tests__/find-args.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
isReadOnlyFindAction,
parseFindArgs,
parseFindSelectorExpression,
UNSUPPORTED_FIND_ACTION_HINT,
} from '@agent-device/selectors';

test('parseFindArgs defaults to click with any locator', () => {
Expand Down Expand Up @@ -42,6 +43,33 @@ test('parseFindArgs throws on unsupported action', () => {
);
});

// #1597: a bare "Unsupported find action" left the agent no path forward
// besides guessing — the hint must name every action find actually supports
// and show the two-command recovery shape (resolve the ref via find, then
// dispatch the gesture, e.g. press, as its own command).
test('parseFindArgs attaches the supported-actions hint with the two-step recovery shape on an unsupported action', () => {
try {
parseFindArgs(['text', 'Follow', 'press']);
expect.unreachable('parseFindArgs should have thrown for an unsupported action');
} catch (error) {
expect(error).toMatchObject({
code: 'INVALID_ARGS',
message: 'Unsupported find action: press',
details: { hint: UNSUPPORTED_FIND_ACTION_HINT },
});
}
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('click (default)');
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('focus');
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('fill');
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('type');
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('exists');
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('wait');
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('get text');
expect(UNSUPPORTED_FIND_ACTION_HINT).toContain('get attrs');
expect(UNSUPPORTED_FIND_ACTION_HINT).toMatch(/find "<text>" to list matches/);
expect(UNSUPPORTED_FIND_ACTION_HINT).toMatch(/press @eNN/);
});

test('parseFindArgs with bare locator yields empty query', () => {
const parsed = parseFindArgs(['text']);
expect(parsed.locator).toBe('text');
Expand Down
89 changes: 89 additions & 0 deletions src/daemon/handlers/__tests__/find.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,95 @@ test('handleFindCommands click prefers semantic controls over matching container
expect(invokeCalls[0]!.positionals?.[0]).toBe('@e5');
});

// #1597: an ambiguous find must let the agent act on the right @ref straight
// from the error, so the response carries snapshot-line-rendered candidates
// (ref, role, label) instead of a bare "matched N elements" message. Capped
// at AMBIGUOUS_MATCH_CANDIDATE_LIMIT (5); the true total keeps riding
// `matches` so a "+N more" marker can be computed at render time.
test('handleFindCommands ambiguous match lists snapshot-line candidates capped at 5', async () => {
const followButton = (ref: string, index: number, x: number) => ({
index,
ref,
type: 'Button',
label: 'Follow',
hittable: true,
rect: { x, y: 100, width: 80, height: 40 },
parentIndex: 0,
});

const { response } = await runFindClickScenario({
positionals: ['Follow', 'click'],
nodes: [
{ index: 0, ref: 'e1', type: 'Application', rect: { x: 0, y: 0, width: 800, height: 1200 } },
followButton('e2', 1, 0),
followButton('e3', 2, 90),
followButton('e4', 3, 180),
followButton('e5', 4, 270),
followButton('e6', 5, 360),
followButton('e7', 6, 450),
],
});

expect(response.ok).toBe(false);
if (response.ok) return;
expect(response.error.code).toBe('AMBIGUOUS_MATCH');
// The old bare message ("find matched 6 elements ... Use a more specific
// locator or selector.") gave the agent nothing to act on directly — this
// proves the fix red against that shape: `candidates` must exist, be
// snapshot-line rendered, and be capped below the true match count.
expect(response.error.details?.matches).toBe(6);
const candidates = response.error.details?.candidates;
expect(Array.isArray(candidates)).toBe(true);
expect(candidates).toHaveLength(5);
expect(candidates).toEqual([
'@e2 [button] "Follow"',
'@e3 [button] "Follow"',
'@e4 [button] "Follow"',
'@e5 [button] "Follow"',
'@e6 [button] "Follow"',
]);
});

test('handleFindCommands ambiguous match with few candidates lists them all uncapped', async () => {
const { response } = await runFindClickScenario({
positionals: ['Follow', 'click'],
nodes: [
{ index: 0, ref: 'e1', type: 'Application', rect: { x: 0, y: 0, width: 800, height: 1200 } },
{
index: 1,
ref: 'e2',
type: 'Button',
label: 'Follow',
hittable: true,
rect: { x: 0, y: 100, width: 80, height: 40 },
parentIndex: 0,
},
{
index: 2,
ref: 'e3',
type: 'Button',
// No label — exact-matches "Follow" via its identifier instead, so
// this candidate exercises the label/identifier fallback.
identifier: 'FOLLOW',
hittable: true,
rect: { x: 90, y: 100, width: 80, height: 40 },
parentIndex: 0,
},
],
});

expect(response.ok).toBe(false);
if (response.ok) return;
expect(response.error.code).toBe('AMBIGUOUS_MATCH');
expect(response.error.details?.matches).toBe(2);
// No label on e3, so the candidate line falls back to its identifier —
// "label/identifier" per #1597, same as any other snapshot line.
expect(response.error.details?.candidates).toEqual([
'@e2 [button] "Follow"',
'@e3 [button] "FOLLOW"',
]);
});

test('handleFindCommands focus uses the promoted actionable node center', async () => {
const { response } = await runFindClickScenario({
positionals: ['Account', 'focus'],
Expand Down
21 changes: 15 additions & 6 deletions src/daemon/handlers/find.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ import { expireRefFrame } from '../ref-frame.ts';
import type { DaemonInvokeFn, DaemonRequest, DaemonResponse, SessionState } from '../types.ts';
import { SessionStore } from '../session-store.ts';
import { contextFromFlags } from '../context.ts';
import { extractNodeText } from '@agent-device/contracts/snapshot';
import {
resolveActionableTouchNode,
resolveActionableTouchResolution,
} from '../../core/interaction-targeting.ts';
import { isSnapshotNodeInteractionBlocked } from '../../snapshot/snapshot-occlusion.ts';
import { formatSnapshotLine } from '../../snapshot/snapshot-lines.ts';
import { readCommandMessage, successText } from '../../utils/success-text.ts';
import { errorResponse, noActiveSessionError } from './response.ts';
import { withSystemSurfaceDisclosure } from './system-surface-disclosure.ts';
Expand Down Expand Up @@ -540,6 +540,17 @@ function publicFindFlags(flags: DaemonRequest['flags']): Record<string, unknown>
return { ...(stripInternalInteractionFlags(flags) ?? {}) };
}

// #1597: an agent reading an ambiguous-match error must be able to act on the
// right @ref immediately, without a follow-up snapshot round trip. Candidate
// lines reuse the exact snapshot-line renderer (`formatSnapshotLine`) so a
// candidate reads identically to its row in `snapshot -i` output: ref, role,
// label/identifier. Capped at AMBIGUOUS_MATCH_CANDIDATE_LIMIT to bound the
// error payload — `matches` (the true total) is what a "+N more" marker is
// computed from at render time (src/utils/output.ts, src/mcp/tool-error.ts).
// Module-local: no consumer outside this file needs the raw cap, only the
// already-capped `candidates` array on the response.
const AMBIGUOUS_MATCH_CANDIDATE_LIMIT = 5;

// Exported as the single AMBIGUOUS_MATCH producer so the help-benchmark
// sample parity test renders the exact error this handler returns; a message
// change here fails that gate instead of drifting past it.
Expand All @@ -548,11 +559,9 @@ export function buildAmbiguousMatchError(
locator: FindLocator,
query: string,
): DaemonResponse {
const candidates = matches.slice(0, 8).map((candidate) => {
const label =
extractNodeText(candidate) || candidate.label || candidate.identifier || candidate.type || '';
return `@${candidate.ref}${label ? `(${label})` : ''}`;
});
const candidates = matches
.slice(0, AMBIGUOUS_MATCH_CANDIDATE_LIMIT)
.map((candidate) => formatSnapshotLine(candidate, 0, false));
return errorResponse(
'AMBIGUOUS_MATCH',
`find matched ${matches.length} elements for ${locator} "${query}". Use a more specific locator or selector.`,
Expand Down
Loading
Loading