Skip to content

Commit f9c8555

Browse files
thymikeethymikee
committed
fix(interaction): close the adversarial-review findings on the empty-fill clear
- android adb-shell: the delete burst is sized from the value being REMOVED (pre-mutation read; the attempt's cap when unreadable), not from the empty incoming text, which sent the 12/24-delete minimums and could never empty a field longer than 36 characters. - android: the unconfirmed soft-success no longer applies to an empty expectation — nothing app-formats the empty value, so residue after a clear is a failed clear, and the soft-success also skipped the second, bigger delete burst. - android masked fields: an empty expectation accepts an observed masked node with no dump text (a masked field WITH content dumps its bullet run), so clearing a password field no longer fails after the clear worked — matching iOS, where a secure-field clear succeeds unverified. - find: 'find <q> fill ""' now reaches the fill leaf as the clear request on both the CLI reader and the daemon positional parse; a MISSING value keeps its refusal at each producer, so the typed value: string contract is unchanged. - maestro export: a recorded clear exports as tapOn + eraseText instead of a vacuous inputText: "" (with the 50-character-default warning). - the missing-text refusals teach the clear form: (use "" to clear the field). Full unit suite green (1061 files); each behavioral fix carries a test observed red against the prior code.
1 parent 120e689 commit f9c8555

12 files changed

Lines changed: 166 additions & 14 deletions

File tree

‎packages/maestro/src/internal/export-flow.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,20 @@ function convertFillAction(
266266
}
267267
const tapTarget = readTapTarget(target, undefined, resolveSelector);
268268
if (!tapTarget) return { kind: 'unsupported', message: 'fill target is not Maestro-compatible' };
269+
if (text.length === 0) {
270+
// The clear request (#2063): `inputText: ""` types nothing in Maestro, so the recorded
271+
// clear would silently become a no-op. `eraseText` is Maestro's clear verb; without a
272+
// count it erases up to its 50-character default, which is a bound this export cannot
273+
// recover the real length for.
274+
return {
275+
kind: 'commands',
276+
commands: [{ tapOn: tapTarget }, 'eraseText'],
277+
warnings: [
278+
...readLabelSelectorWarnings(target),
279+
'fill "" exports as tapOn + eraseText; Maestro erases at most its 50-character default',
280+
],
281+
};
282+
}
269283
return {
270284
kind: 'commands',
271285
commands: [{ tapOn: tapTarget }, { inputText: text }],

‎packages/selectors/src/internal/find.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,15 @@ export function parseFindArgs(args: string[]): ParsedFindArgs {
234234
return { locator, query, action: 'wait', timeoutMs: timeoutMs ?? undefined };
235235
}
236236
if (action === 'fill' || action === 'type') {
237-
return { locator, query, action, value: actionTokens.slice(1).join(' ') };
237+
// `undefined` when NO value token followed, `''` when an empty one did: `find <q> fill ""`
238+
// is the clear request (#2063) and must stay distinguishable from a forgotten argument.
239+
const valueTokens = actionTokens.slice(1);
240+
return {
241+
locator,
242+
query,
243+
action,
244+
value: valueTokens.length === 0 ? undefined : valueTokens.join(' '),
245+
};
238246
}
239247
throw new AppError('INVALID_ARGS', `Unsupported find action: ${actionTokens[0]}`, {
240248
hint: UNSUPPORTED_FIND_ACTION_HINT,

‎src/cli/commands/__tests__/replay-maestro-export.test.ts‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,26 @@ screenshot "./artifacts/checkout"
4545
]);
4646
});
4747

48+
test('exports the empty-fill clear as eraseText, never a vacuous inputText', () => {
49+
// `fill <target> ""` is the clear-field primitive (#2063); Maestro's `inputText: ""` types
50+
// nothing, so the recorded clear must become its clear verb.
51+
const result = exportReplayScriptToMaestro(`context platform=ios target=mobile
52+
open com.example.app
53+
fill id="email" ""
54+
`);
55+
56+
const docs = parseYamlDocs(result.yaml);
57+
expect(docs[1]).toEqual(['launchApp', { tapOn: { id: 'email' } }, 'eraseText']);
58+
expect(result.warnings).toEqual([
59+
{
60+
line: 3,
61+
action: 'fill id="email"',
62+
message:
63+
'fill "" exports as tapOn + eraseText; Maestro erases at most its 50-character default',
64+
},
65+
]);
66+
});
67+
4868
test('exports coordinate gestures and sleep waits with warnings', () => {
4969
const result = exportReplayScriptToMaestro(`open com.example.app
5070
click 120 240

‎src/commands/interaction/interactions.test.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { expect, test } from 'vitest';
22
import { interactionCliReaders, interactionDaemonWriters } from './interactions.ts';
3+
import { selectorCliReaders } from './selectors.ts';
34
import { AppError } from '@agent-device/kernel/errors';
45
import type { CliFlags } from '@agent-device/contracts/command';
56

@@ -71,3 +72,15 @@ test('fill projects an empty text as its own positional', () => {
7172

7273
expect(request.positionals).toEqual(['@e57', '']);
7374
});
75+
76+
// `find <q> fill ""` is the same clear request through the find grammar (#2063): the empty
77+
// value reaches the typed options, while a missing value is refused at the reader so the typed
78+
// `value: string` contract stays intact.
79+
test('find fill reads an empty value as the clear request and refuses a missing one', () => {
80+
const cleared = selectorCliReaders.find(['text', 'Save', 'fill', ''], BASE_FLAGS);
81+
expect(cleared).toMatchObject({ action: 'fill', value: '' });
82+
83+
expect(() => selectorCliReaders.find(['text', 'Save', 'fill'], BASE_FLAGS)).toThrow(
84+
/find fill requires text \(use "" to clear the field\)/,
85+
);
86+
});

‎src/commands/interaction/metadata.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,9 @@ const findFields = {
188188
locator: enumField(FIND_LOCATORS),
189189
query: requiredField(stringField()),
190190
action: enumField(FIND_ACTION_VALUES),
191-
value: stringField(),
191+
// `find <q> fill ""` is the clear request (#2063); the daemon handler still refuses a MISSING
192+
// value, and an empty `type` value.
193+
value: stringField(undefined, { allowEmpty: true }),
192194
timeoutMs: integerField(),
193195
first: booleanField(),
194196
last: booleanField(),

‎src/commands/interaction/selectors.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,12 +97,23 @@ function readFindOptionsFromPositionals(positionals: string[], flags: CliFlags):
9797
};
9898
}
9999
if (action === 'fill' || action === 'type') {
100+
// An empty value positional is the clear request (#2063); only a MISSING one is refused,
101+
// here, so `value` stays present on the typed options.
102+
const valuePositionals = positionals.slice(actionOffset + 1);
103+
if (valuePositionals.length === 0) {
104+
throw new AppError(
105+
'INVALID_ARGS',
106+
action === 'fill'
107+
? 'find fill requires text (use "" to clear the field)'
108+
: 'find type requires text',
109+
);
110+
}
100111
return {
101112
...base,
102113
locator,
103114
query: readRequiredQuery(query),
104115
action,
105-
value: positionals.slice(actionOffset + 1).join(' '),
116+
value: valuePositionals.join(' '),
106117
};
107118
}
108119
if (action === 'click' || action === 'focus' || action === 'exists' || action === 'list') {

‎src/daemon/handlers/__tests__/find-args.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,18 @@ test('parseFindSelectorExpression only treats bare selector-shaped queries as se
100100
expect(parseFindSelectorExpression('any', 'a=b')).toBeNull();
101101
});
102102

103+
// `find <q> fill ""` is the clear request (#2063): an EMPTY value token must survive the parse,
104+
// while NO value token reads as undefined so the handler's missing-text refusal still fires —
105+
// the parse used to collapse both to ''.
106+
test('parseFindArgs distinguishes an empty fill value from a missing one', () => {
107+
const cleared = parseFindArgs(['text', 'Save', 'fill', '']);
108+
expect(cleared).toMatchObject({ action: 'fill', value: '' });
109+
110+
const missing = parseFindArgs(['text', 'Save', 'fill']);
111+
expect(missing.action).toBe('fill');
112+
expect(missing.value).toBeUndefined();
113+
});
114+
103115
// #1271 stage 2: `--record` is statically scoped to snapshot/get/is via each
104116
// command schema's `allowedFlags`, but `find`'s observe-vs-mutate split is a
105117
// POSITIONAL, so it is validated dynamically against this shared predicate.

‎src/daemon/handlers/find.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -267,8 +267,9 @@ async function handleFindFill(
267267
value: string | undefined,
268268
): Promise<DaemonResponse> {
269269
const { req, sessionName, sessionStore, session, invoke, command, publicFlags } = ctx;
270-
if (!value) {
271-
return errorResponse('INVALID_ARGS', 'find fill requires text');
270+
// `''` is the clear request (#2063); only a MISSING value is an error.
271+
if (value === undefined) {
272+
return errorResponse('INVALID_ARGS', 'find fill requires text (use "" to clear the field)');
272273
}
273274
const response = await invoke({
274275
token: req.token,

‎src/daemon/handlers/interaction-touch-targets.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,13 @@ export function parseFillTarget(positionals: string[]): ParsedFillTarget {
119119
const parsed = readFillTargetFromPositionals(positionals);
120120
const text = parsed.text;
121121
if (!hasFillText(text))
122-
return { ok: false, response: errorResponse('INVALID_ARGS', 'fill requires text after ref') };
122+
return {
123+
ok: false,
124+
response: errorResponse(
125+
'INVALID_ARGS',
126+
'fill requires text after ref (use "" to clear the field)',
127+
),
128+
};
123129
return {
124130
ok: true,
125131
target: {
@@ -138,7 +144,10 @@ export function parseFillTarget(positionals: string[]): ParsedFillTarget {
138144
if (!hasFillText(text))
139145
return {
140146
ok: false,
141-
response: errorResponse('INVALID_ARGS', 'fill requires text after coordinates'),
147+
response: errorResponse(
148+
'INVALID_ARGS',
149+
'fill requires text after coordinates (use "" to clear the field)',
150+
),
142151
};
143152
return { ok: true, target: { kind: 'point', x: coordinates.x, y: coordinates.y }, text };
144153
}
@@ -159,7 +168,10 @@ export function parseFillTarget(positionals: string[]): ParsedFillTarget {
159168
if (!hasFillText(parsed.text) || (parsed.text.length > 0 && !parsed.text.trim())) {
160169
return {
161170
ok: false,
162-
response: errorResponse('INVALID_ARGS', 'fill requires text after selector'),
171+
response: errorResponse(
172+
'INVALID_ARGS',
173+
'fill requires text after selector (use "" to clear the field)',
174+
),
163175
};
164176
}
165177
return {

‎src/platforms/android/__tests__/text-input-fill.test.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,49 @@ test('verifyAndroidFilledTextInHierarchy does not ignore broader case mismatches
506506
assert.equal(verification.ok, false);
507507
});
508508

509+
// The delete burst used to be sized from the INCOMING text, so the empty clear (#2063) sent the
510+
// 12/24-delete minimums and could never empty a field longer than 36 characters — leaving residue
511+
// after reporting failure. The clear must be sized from the value being removed.
512+
test('fillAndroid sizes the empty clear from the field content, not the empty text', async () => {
513+
let value = 'a'.repeat(70);
514+
let deletes = 0;
515+
await withFillAdb(
516+
async (args) => {
517+
if (args[0] === 'shell' && args[1] === 'input' && args[2] === 'keyevent') {
518+
const count = args.filter((arg) => arg === 'KEYCODE_DEL').length;
519+
deletes += count;
520+
value = value.slice(0, Math.max(0, value.length - count));
521+
}
522+
return adbResult('');
523+
},
524+
() => androidInputXml({ text: value }),
525+
async () => {
526+
await fillAndroid(ANDROID_EMULATOR, 10, 10, '');
527+
},
528+
);
529+
assert.equal(value, '');
530+
assert.ok(deletes >= 70, `expected the clear to cover the 70-char value, sent ${deletes}`);
531+
});
532+
533+
// A masked value only ever compares by length, and the empty expectation (#2063) accepts an
534+
// observed masked node with no dump text: a masked field WITH content dumps its bullet run.
535+
// Matches iOS, where clearing a secure field succeeds unverified instead of failing after the
536+
// clear worked.
537+
test('verifyAndroidFilledTextInHierarchy accepts a cleared masked field for an empty expectation', () => {
538+
const cleared = verifyAndroidFilledTextInHierarchy(passwordHierarchy(''), 10, 10, '');
539+
assert.equal(cleared.ok, true);
540+
assert.equal(cleared.masked, true);
541+
542+
const residue = verifyAndroidFilledTextInHierarchy(
543+
passwordHierarchy(maskBullets('Test@123')),
544+
10,
545+
10,
546+
'',
547+
);
548+
assert.equal(residue.ok, false);
549+
assert.equal(residue.reason, 'masked_unverified');
550+
});
551+
509552
test('fillAndroid accepts matching-length masked password verification', async () => {
510553
let typed = '';
511554
await withFillAdb(

0 commit comments

Comments
 (0)