COMPUTER-USE part 2: CU-2, CU-8, CU-9-verify (stacks on PR #134)#135
COMPUTER-USE part 2: CU-2, CU-8, CU-9-verify (stacks on PR #134)#135thecombatwombat wants to merge 14 commits into
Conversation
The previous shell-payload guard joined every arg after `shell` with spaces
and ran a single regex against the result, blocking URLs and other tokens
containing embedded `&`, `|`, `$`, etc. — even though execa never hands the
joined string to a shell. Agents could not fire an intent with a query-string
URL via `adb-shell`.
Tighten the guard, don't loosen it: each arg is now scanned individually
against a documented SHELL_COMPOSITION_PATTERNS list (backticks, `$()`,
`${VAR}`, `$IDENT`, subshells, leading chain ops, `;`, `&&`/`||`, and `|`
with surrounding whitespace). Embedded `&`/`|` inside a single token now
flow through.
Add `AdbAdapter.startIntent` — a typed entrypoint that validates action,
data, package, component, and extras before building `am start -W` argv —
and expose it as `adb-app start-intent`. Action regex, length caps, null-byte
rejection, and key/value sanitization run BEFORE any argv is constructed,
so the boundary stays safe even if the shell guard ever changes.
- src/services/process-runner.ts: per-arg validateArgForShellComposition;
factored SHELL_COMPOSITION_PATTERNS table with rationale per entry.
- src/adapters/adb.ts: startIntent(deviceId, intent) + validators + parser.
- src/tools/adb-app.ts: new `start-intent` op + handler + schema fields.
- docs/rtfm/adb.md: documented the new op.
- DECISIONS.md: recorded the tightening rationale.
- Tests: 12 new per-arg guard cases (URL-safe, real chains still blocked),
12 adapter-level startIntent cases, 6 tool-handler cases. All 96 pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ui-query find cached match results in context.lastFindResults so ui-action could refer to them by elementIndex. Between the two calls the screen can change — a list scrolls, a dialog dismisses, a snackbar appears — and the cached (centerX, centerY) now points at something else entirely. Capture a content fingerprint (text|resourceId|className|bounds) per match at find time. On the elementIndex path, re-dump the tree, locate the node at the cached center, recompute its fingerprint, and reject with the new STALE_ELEMENT_INDEX error code if it differs (or if no node is there). OCR and grid matches get an empty fingerprint and skip the check — they have no stable identity across calls. Backward compat: empty lastFindFingerprints also skips the check, so existing callers that pre-populate the cache keep working unchanged. - src/types/errors.ts: STALE_ELEMENT_INDEX code. - src/tools/util-fingerprint.ts: fingerprint helpers. - src/server.ts: lastFindFingerprints on ServerContext. - src/tools/ui-find.ts: populate fingerprints at both store sites. - src/tools/ui-action.ts: assertElementStillFresh + handleTap refactor into per-source resolveTap helpers (also brings handleTap under the 80-line function cap). - docs/rtfm/ui.md: documented the new lifetime contract. - DECISIONS.md: option-(a) rationale over persistent IDs / decline. - Tests: 13 new (fingerprint helpers + 7 stale-element scenarios). Existing cross-tool-state test updated to mock ui.dump. All 42 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ui-action `input` had no way to confirm the typed text actually landed —
silent no-ops when the field wasn't focused, autocomplete transforming the
value, or input-method quirks. Agents had to follow every input with a
separate ui-query find to inspect the field.
Ship the verify half of THE-113:
- `verify: true` on the input op (requires a selector) captures the target
field's text before and after via fresh `find` calls, then returns
{ verified, containsRequested, changed, inputBefore, inputAfter }.
- `verified = containsRequested || changed` so the looser `changed` signal
catches autocomplete / case-mutation cases where the final value isn't
literally the requested string.
- Internal finds snapshot/restore `lastFindResults` and
`lastFindFingerprints` so verify's reads don't clobber the agent's cache.
- Default false (opt-in cost: 2 extra accessibility dumps).
Defer the clipboard-paste half. The CU-2 per-arg shell guard unblocked the
path, but device-capability detection (Android version, focused-field
clipboard permissions, IME quirks) is its own scope. DECISIONS captures the
sketch: `AdbAdapter.pasteText` setting clipboard via `cmd clipboard
set-primary-clip` then sending KEYCODE_PASTE — agent text containing `&`
flows through the new per-arg guard as a single arg.
Extract handleScroll to ui-action-scroll.ts so ui-action.ts stays under the
500-line file cap. Verify path added ~75 lines to handleInput; without the
extraction the file would have crossed 570.
- src/tools/ui-action.ts: schema `verify` field, readSelectorText helper,
handleInput with verify branch, handleScroll thin wrapper.
- src/tools/ui-action-scroll.ts: extracted scroll handler with injected
deps to avoid a circular import.
- docs/rtfm/ui.md: documented verify usage.
- DECISIONS.md: ship-verify / defer-clipboard rationale + clipboard design.
- Tests: 6 new (verify=false default, success, no-change, autocomplete case,
no-selector rejection, lastFindResults isolation).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…THE-106, THE-113) Three small follow-ups exposed by the full test suite after the CU-2/CU-8/CU-9 ship: 1. computeAccessibilityFingerprint crashed on AccessibilityNode fixtures that omit `bounds` (integration test mocks for the icon-recognition fallback chain). Defaulted to a 0,0,0,0 bounds when the field is missing so the fingerprint stays a deterministic string. Real `parseUiDump` output always sets bounds; only test mocks were affected. 2. ui-nearest-to.test.ts > "tap after nearestTo find" passed pre-CU-8 because ui-action.tap (with elementIndex) didn't re-dump the tree. CU-8's stale-element check does, and the test's mock dump tree didn't include the matched button — so the fingerprint check at the cached center found no node and threw STALE_ELEMENT_INDEX. Updated the fixture to put the matched btn into the dump tree's children. The find's containment scan already needed a consistent tree; this just completes it. 3. CU-2's start-intent fields (+112 tokens) and CU-9's verify field (+44 tokens) pushed the tool-schema budget past the 2200 ceiling. Regenerated docs/contracts/tool-schema-tokens.json (the visibility snapshot) and bumped the ceiling to 2700 with a history entry documenting which work added which tokens. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…re/computer-use-part-2 # Conflicts: # docs/contracts/tool-schema-tokens.json # src/tools/ui-action.ts # tests/tools/token-budget.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22f9373b3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Chain operator at the START of an arg (or the arg IS the operator). | ||
| { pattern: /^(&&|\|\||[;&|])/, description: "chain operator at start" }, |
There was a problem hiding this comment.
When callers use the existing adb-shell path, AdbAdapter.shell() still passes the whole user command as a single argument after shell, so a command like echo ok & reboot reaches the Android remote shell as one command string. This new per-arg allowlist only rejects & at the beginning of that string, so embedded single-ampersand chaining/backgrounding now bypasses COMMAND_BLOCKED and can execute the second command; the guard needs to reject whitespace-delimited & (at least for single-string shell payloads) while still allowing URL query & in typed argv paths.
Useful? React with 👍 / 👎.
| if (intent.package !== undefined) { | ||
| args.push("-n", intent.component ?? `${intent.package}/.MainActivity`); |
There was a problem hiding this comment.
For start-intent calls that provide packageName but no explicit component, this turns a package restriction into an explicit component launch of pkg/.MainActivity. The am intent syntax treats a bare trailing package as setPackage(...), while -n requires a real component, so deep links targeted at apps whose exported activity is not exactly .MainActivity will fail instead of resolving within that package. Use the package form when only packageName is supplied, and reserve -n for caller-provided components.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR delivers three features on top of
Confidence Score: 4/5CU-8 and CU-9 are solid and ready to ship; the shell guard in CU-2 has two gaps that block valid inputs through start-intent without affecting device security. The stale-element and input-verify features are correctly implemented. The CU-2 shell guard contains two logic errors: the &&/|| SHELL_COMPOSITION_PATTERNS fire on raw arg strings including single-quoted spans, and rejectUnquotedAmpersand does not handle backslash escaping outside quoted spans, so values with both a single-quote and ampersand are incorrectly blocked. src/services/process-runner.ts — the two interacting bugs in SHELL_COMPOSITION_PATTERNS and rejectUnquotedAmpersand Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant AdbAppTool
participant AdbAdapter
participant ProcessRunner
participant DeviceShell as Device /bin/sh
Agent->>AdbAppTool: "start-intent {action, data, extras}"
AdbAppTool->>AdbAdapter: startIntent(deviceId, intent)
AdbAdapter->>AdbAdapter: validateStartIntentInput()
AdbAdapter->>AdbAdapter: quoteForDeviceShell(data/extras values)
AdbAdapter->>ProcessRunner: run(adb, [..., shell, am, start, -d, URL])
ProcessRunner->>ProcessRunner: validateShellPayload() per arg
ProcessRunner->>DeviceShell: adb shell am start -W -a ACTION -d URL
DeviceShell-->>ProcessRunner: Status: ok / Error
ProcessRunner-->>AdbAdapter: RunResult
AdbAdapter->>AdbAdapter: parseStartIntentOutput()
AdbAdapter-->>AdbAppTool: StartIntentResult
AdbAppTool-->>Agent: "{intentStarted, status, ok, raw}"
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/services/process-runner.ts:295
**`&&`/`||` pattern fires inside single-quoted spans — false-blocks valid data URIs**
`/&&|\|\|/` is tested against the raw arg string without any quote awareness. When `AdbAdapter.startIntent` wraps a data URI containing `&&` via `quoteForDeviceShell`, the resulting arg is `'https://example.com/?a=1&&b=2'`. The pattern still matches `&&` inside the single-quoted span and throws `COMMAND_BLOCKED`, even though the device shell would treat the entire thing as literal data.
Concrete failure: `start-intent` with `data = "https://x/?state=abc&&redirect=1"` → `quoteForDeviceShell` produces `'https://x/?state=abc&&redirect=1'` → this pattern fires → rejected. Yet `&&` in this token is pure data; on the device `/bin/sh` sees a single-quoted argument with no control operators. The same applies to `||` in extras values.
Single `&` was given a quote-aware treatment via `rejectUnquotedAmpersand`; `&&` and `||` need the same. Both checks must agree on what counts as "inside a quote span."
### Issue 2 of 2
src/services/process-runner.ts:244-249
**`rejectUnquotedAmpersand` mishandles the `'\''` escape sequence produced by `quoteForDeviceShell`**
`quoteForDeviceShell` uses the POSIX close-escape-reopen pattern `'\''` to embed a literal `'` inside a single-quoted value. For `"it's & more"` it produces `'it'\''s & more'`. The scanner doesn't treat `\` as an escape outside quoted spans, so it processes `'\''` as: close-quote (`inSingle=false`) → skip `\` → open-quote → immediately close-quote — leaving `s & more` as unquoted. The bare `&` then triggers COMMAND_BLOCKED.
Adding a single backslash-skip outside quotes fixes the state machine to match POSIX: when `!inSingle && !inDouble` and `ch === '\\'`, advance `i` to skip the escaped character before evaluating quote toggles.
```suggestion
for (let i = 0; i < arg.length; i++) {
const ch = arg[i];
// Outside any quoted span, `\` escapes the next character (POSIX).
// This is required to track quote state correctly through the `'\''`
// pattern that `quoteForDeviceShell` emits for embedded single quotes.
if (!inSingle && !inDouble && ch === "\\") {
i++;
continue;
}
if (!inDouble && ch === "'") {
inSingle = !inSingle;
continue;
}
```
Reviews (5): Last reviewed commit: "fix: respect quote boundaries when guard..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // CU-2 (THE-106): argv-aware metacharacter check. Each arg is scanned | ||
| // individually for shell composition patterns. This is a TIGHTENING of the | ||
| // previous joined-string check, which over-blocked any URL or quoted | ||
| // string containing a literal `&`, `|`, etc. — even though execa never | ||
| // hands the joined string to a shell. The contract is: a single arg may | ||
| // contain literal `&`/`|` as data (URL query strings, content), but no | ||
| // arg may START with a chain operator or contain unbalanced shell | ||
| // structure that would let a payload split into multiple commands if it | ||
| // were ever passed through `sh -c`. | ||
| for (const arg of payloadArgs) { | ||
| this.validateArgForShellComposition(arg); | ||
| } |
There was a problem hiding this comment.
adb shell routes payload to the device shell — embedded & is still a metacharacter on-device
The per-arg guard correctly observes that execa doesn't invoke a host shell, so a URL like https://example.com/?foo=bar&baz=qux is safe at the host OS level. However, adb shell <args> works differently: ADB joins all post-shell args with spaces and sends the resulting string to /bin/sh on the Android device. The device shell then tokenises on & regardless of where it appears in a token. In practice https://example.com/?foo=bar&baz=qux causes the device to run am start -d https://example.com/?foo=bar in the background and evaluate baz=qux as a variable assignment — the -W wait result is lost. More critically, a caller-controlled URL containing & <command> is not blocked by any current pattern (&& and || are caught, but a bare & with surrounding whitespace is not), allowing arbitrary device-side command execution.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/process-runner.ts
Line: 155-166
Comment:
**`adb shell` routes payload to the device shell — embedded `&` is still a metacharacter on-device**
The per-arg guard correctly observes that `execa` doesn't invoke a host shell, so a URL like `https://example.com/?foo=bar&baz=qux` is safe at the host OS level. However, `adb shell <args>` works differently: ADB joins all post-`shell` args with spaces and sends the resulting string to `/bin/sh` on the Android device. The device shell then tokenises on `&` regardless of where it appears in a token. In practice `https://example.com/?foo=bar&baz=qux` causes the device to run `am start -d https://example.com/?foo=bar` in the background and evaluate `baz=qux` as a variable assignment — the `-W` wait result is lost. More critically, a caller-controlled URL containing `& <command>` is not blocked by any current pattern (`&&` and `||` are caught, but a bare `&` with surrounding whitespace is not), allowing arbitrary device-side command execution.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const result = await this.adb(args); | ||
| const parsed = parseStartIntentOutput(result.stdout); | ||
| if (!parsed.ok && result.exitCode !== 0) { |
There was a problem hiding this comment.
&& error gate silently swallows exit-0 am start failures
if (!parsed.ok && result.exitCode !== 0) requires BOTH conditions before throwing. Android's am start sometimes exits 0 while printing Error: Activity not started, ... — in that case parsed.ok is false but the condition short-circuits and returns { ok: false } silently. Using || would be more defensive and consistent with how other adapter methods gate on exit code.
| if (!parsed.ok && result.exitCode !== 0) { | |
| if (!parsed.ok || result.exitCode !== 0) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/adapters/adb.ts
Line: 220
Comment:
**`&&` error gate silently swallows exit-0 `am start` failures**
`if (!parsed.ok && result.exitCode !== 0)` requires BOTH conditions before throwing. Android's `am start` sometimes exits 0 while printing `Error: Activity not started, ...` — in that case `parsed.ok` is false but the condition short-circuits and returns `{ ok: false }` silently. Using `||` would be more defensive and consistent with how other adapter methods gate on exit code.
```suggestion
if (!parsed.ok || result.exitCode !== 0) {
```
How can I resolve this? If you propose a fix, please make it concise.…w-ups) Two Greptile findings on PR #135 / CU-2, fixed together because they share the same adapter method and review thread: 1. `adb shell <argv>` joins post-`shell` args with spaces and ships the string to /bin/sh on the device. The device shell tokenises on `&`, `;`, etc. regardless of host argv boundaries — a URL like `https://x/?a=1&b=2` backgrounds at the `&`, dropping the `-W` wait result; worst case, a caller-controlled URL containing `& <cmd>` runs the second command on-device. Fix: single-quote `data`, the resolved component / `<package>/.MainActivity`, and each `--es` value via a new `quoteForDeviceShell()` helper (POSIX-portable escape for embedded single quotes). The host-side per-arg guard stays untouched — it remains defense-in-depth against composition characters in any shell arg, and the new `;`-in-`data` test pins that contract. 2. The post-`am start` error gate used `!parsed.ok && exitCode !== 0`, which silently returned `{ ok: false }` when `am start` exits 0 while writing `Error: Activity not started, ...` to stdout. Flipped to `||` so the gate fires on either signal. Co-Authored-By: Claude <noreply@anthropic.com>
…re/computer-use-part-2
|
@greptileai please re-review — addressed the two findings on this PR:
Also merged the parent branch ( |
|
@codex review — addressed the two findings on this PR since your initial pass on commit
PR #134 fixes were also merged into this branch (merge commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91aeaa92e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Chain operator at the START of an arg (or the arg IS the operator). | ||
| { pattern: /^(&&|\|\||[;&|])/, description: "chain operator at start" }, |
There was a problem hiding this comment.
For raw adb-shell commands, payloads like echo ok& reboot still pass this guard because a single & is only rejected at the start of an arg. adb shell executes a shell command (Android documents the form as adb ... shell shell_command, and this file's start-intent comments note the args are re-joined before /bin/sh), so a trailing & backgrounds the first command and lets the following blocked command run, bypassing the safety checks that should reject chained commands.
Useful? React with 👍 / 👎.
| if (intent.package !== undefined) { | ||
| const component = intent.component ?? `${intent.package}/.MainActivity`; | ||
| args.push("-n", quoteForDeviceShell(component)); |
There was a problem hiding this comment.
When callers supply packageName to “limit the intent to a specific package” without an explicit component, this turns it into -n <package>/.MainActivity. That makes common cases such as VIEW URLs or apps whose launcher/activity is not .MainActivity fail with an unresolved component instead of letting Android resolve the intent within the target package/component spec (the am intent syntax distinguishes -n <COMPONENT> from a package-only target).
Useful? React with 👍 / 👎.
| const SEP = ""; | ||
|
|
||
| export function computeAccessibilityFingerprint(node: AccessibilityNode): string { | ||
| // Some test/mock fixtures omit bounds. Treat missing bounds as 0,0,0,0 — | ||
| // the fingerprint still works as an identity comparison and any real dump | ||
| // from `parseUiDump` populates the field. | ||
| const b = node.bounds ?? { left: 0, top: 0, right: 0, bottom: 0 }; | ||
| return [ | ||
| node.text ?? "", | ||
| node.resourceId ?? "", | ||
| node.className ?? "", | ||
| `${b.left},${b.top},${b.right},${b.bottom}`, | ||
| ].join(SEP); |
There was a problem hiding this comment.
SEP = "" means the fingerprint is the raw concatenation text + resourceId + className + bounds with no delimiter. Two nodes with different field splits but the same total concatenation produce identical fingerprints, so the stale-element check silently passes when it should reject. For example, a node with text="" and resourceId="android.widget.Button" hashes identically to one with text="", resourceId="", and className="android.widget.Button". The inline comment describes the format as text|resourceId|className|bounds (pipe-separated), but the code uses "". The fix is to set SEP to any non-empty character — the pipe | or \x00 are natural choices.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/util-fingerprint.ts
Line: 31-43
Comment:
**Empty fingerprint separator causes undetectable staleness**
`SEP = ""` means the fingerprint is the raw concatenation `text + resourceId + className + bounds` with no delimiter. Two nodes with different field splits but the same total concatenation produce identical fingerprints, so the stale-element check silently passes when it should reject. For example, a node with `text=""` and `resourceId="android.widget.Button"` hashes identically to one with `text=""`, `resourceId=""`, and `className="android.widget.Button"`. The inline comment describes the format as `text|resourceId|className|bounds` (pipe-separated), but the code uses `""`. The fix is to set `SEP` to any non-empty character — the pipe `|` or `\x00` are natural choices.
How can I resolve this? If you propose a fix, please make it concise.…U-2 follow-up) Codex flagged that `echo ok& reboot` slipped through the per-arg guard — a single `&` was only rejected at arg start, so a payload that backgrounded the first command and chained a blocked second one bypassed the dangerous-command list. Same exploit class as a `;` or `&&` chain, just spelled differently. Add a `(?<!&)&(\s|$)` pattern: a `&` not preceded by another `&` (i.e. not part of `&&`, which has its own pattern) and followed by whitespace or end-of-arg is unambiguously a chain operator, never data. URLs with `?a=1&b=2` keep flowing through because both sides of the `&` are non-whitespace. Co-Authored-By: Claude <noreply@anthropic.com>
…8 follow-up) The separator was previously a literal SOH (\x01) byte, which renders as empty (\"\") in most editors. Greptile read it as empty and flagged a fingerprint-collision bug that doesn't actually exist in the running code — the SOH IS a real delimiter, just invisible. Using \"|\" matches the comment block (\"Fingerprint = text|resourceId|className|bounds\") and removes the footgun. `|` doesn't appear in any of the constituent fields (resource ids use `pkg:id/name`, class names use dots, bounds are digits + commas) so there's no collision risk against legitimate content. Added a regression test that fails fast if SEP ever gets emptied back out. Co-Authored-By: Claude <noreply@anthropic.com>
…low-up) Codex flagged that synthesising `<package>/.MainActivity` when only a package was supplied broke common intents — VIEW URLs and apps whose launcher isn't `.MainActivity` failed with an unresolved component. The right primitive is `-p <package>`, which limits the intent to the target package and lets Android resolve the activity. Explicit `component` still flows through `-n`. The branch order changes too: component takes precedence over package, since an explicit component should be respected even if a package was also supplied. Co-Authored-By: Claude <noreply@anthropic.com>
…re/computer-use-part-2 # Conflicts: # docs/contracts/replicant-mcp.contract.json # docs/contracts/tool-schema-tokens.json
|
@greptileai @codex review — addressed all three findings on this PR:
Also merged PR #134's follow-up commits ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc735c3240
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // alphanumerics (`?a=1&b=2`) with no surrounding whitespace — those flow | ||
| // through. The lookbehind keeps the pattern from double-firing on `&&`, | ||
| // which is already caught above. | ||
| { pattern: /(?<!&)&(\s|$)/, description: "single & chain operator" }, |
There was a problem hiding this comment.
For raw adb-shell payloads, a user can still run a chained command by omitting whitespace after the background operator, e.g. echo ok&reboot: /bin/sh treats & as a command terminator even when it is glued to the next word, but this pattern only rejects & before whitespace or end-of-arg. Because adb-shell passes the entire user command as the post-shell payload, this bypasses the safety guard and allows the blocked second command to execute.
Useful? React with 👍 / 👎.
| const containsRequested = inputAfter !== null && inputAfter.includes(input.text); | ||
| const changed = inputAfter !== inputBefore; | ||
| result.verified = containsRequested || changed; |
There was a problem hiding this comment.
verified reports true when inputAfter is null and inputBefore is any non-null string (including ""), because null !== "" evaluates to true. A disappearing element post-input — e.g. a search field that's replaced by a results view, or a focus shift — causes changed = true and thus verified = true, even though we have no evidence the text was actually entered. The caller can't distinguish "input worked" from "element vanished" by checking verified alone.
| const containsRequested = inputAfter !== null && inputAfter.includes(input.text); | |
| const changed = inputAfter !== inputBefore; | |
| result.verified = containsRequested || changed; | |
| const containsRequested = inputAfter !== null && inputAfter.includes(input.text); | |
| const changed = inputAfter !== null && inputAfter !== inputBefore; | |
| result.verified = containsRequested || changed; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tools/ui-action.ts
Line: 457-459
Comment:
`verified` reports `true` when `inputAfter` is `null` and `inputBefore` is any non-null string (including `""`), because `null !== ""` evaluates to `true`. A disappearing element post-input — e.g. a search field that's replaced by a results view, or a focus shift — causes `changed = true` and thus `verified = true`, even though we have no evidence the text was actually entered. The caller can't distinguish "input worked" from "element vanished" by checking `verified` alone.
```suggestion
const containsRequested = inputAfter !== null && inputAfter.includes(input.text);
const changed = inputAfter !== null && inputAfter !== inputBefore;
result.verified = containsRequested || changed;
```
How can I resolve this? If you propose a fix, please make it concise.…llow-up) Greptile P1 on the ui-action input-verify path: when the input op set `verify: true` and the target element disappeared after typing — e.g. a search field replaced by a results view, or a focus shift detaching the EditText — `readSelectorText` returned `null`. The `changed` clause compared `inputAfter !== inputBefore`, so `null !== ""` evaluated true and we reported `verified: true` despite zero evidence the text was actually applied. Callers couldn't distinguish "input worked" from "element vanished". Guard the `changed` clause with the same `inputAfter !== null` check we already apply to `containsRequested`. Vanished element now correctly reports `verified: false`, `containsRequested: false`, `changed: false`, `inputAfter: null` — preserving the diagnostic signal in the response. Added a focused regression test in ui-action-input-verify.test.ts that mocks the after-read to return an empty element resolution, asserts the new contract, and would have caught the original bug (confirmed RED on the pre-fix code, GREEN after the patch). Co-Authored-By: Claude <noreply@anthropic.com>
…U-2 follow-up #2) Codex P1: the previous `(?<!&)&(\s|$)` pattern only rejected `&` followed by whitespace or end-of-arg, so `echo ok&PWNED` still slipped past the adb-shell guard. `/bin/sh` treats an unquoted `&` as a control operator regardless of what follows, so the device shell would background `echo` and run the second command — bypassing the dangerous-command list. The obvious tightening (`(?<!&)&(?!&)`) would also reject the legitimate F3 flow: `AdbAdapter.startIntent` wraps user-controlled URLs in single quotes via `quoteForDeviceShell`, so `'https://x/?a=1&b=2'` contains `&` between alphanumerics — data, not a control operator, because it's inside a quoted span. A regex can't honour quote boundaries. Replace the regex with a quote-aware scanner `rejectUnquotedAmpersand` that walks the arg, toggles `inSingle` / `inDouble` on unescaped quotes, and rejects any `&` outside any quoted span (skipping `&&` since its dedicated SHELL_COMPOSITION_PATTERNS entry covers it). TDD discipline: three new RED-first tests cover `echo ok&PWNED`, `cmd&5`, and `cmd&x` — all of which the old regex allowed through. Two acceptance tests cover `'https://x/?a=1&b=2'` (single-quoted) and `echo "ok & noop"` (double-quoted). Updated the legacy three URL tests to wrap in single quotes, reflecting how every production caller now emits them via `AdbAdapter.startIntent`. Full suite: 792 passed, 8 skipped (above the 786 baseline). Coverage, contracts, complexity, and lint all clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
@greptileai @codex review — addressed both P1s from your last pass, plus pulled PR #134's verified-flag fix into this branch:
Device-validated end-to-end on a Pixel 10 Pro XL: glued- |
| // `;` anywhere is composition (no legitimate adb data use). | ||
| { pattern: /;/, description: "semicolon chain" }, | ||
| // `&&` / `||` anywhere is composition. | ||
| { pattern: /&&|\|\|/, description: "&& or || chain" }, |
There was a problem hiding this comment.
&&/|| pattern fires inside single-quoted spans — false-blocks valid data URIs
/&&|\|\|/ is tested against the raw arg string without any quote awareness. When AdbAdapter.startIntent wraps a data URI containing && via quoteForDeviceShell, the resulting arg is 'https://example.com/?a=1&&b=2'. The pattern still matches && inside the single-quoted span and throws COMMAND_BLOCKED, even though the device shell would treat the entire thing as literal data.
Concrete failure: start-intent with data = "https://x/?state=abc&&redirect=1" → quoteForDeviceShell produces 'https://x/?state=abc&&redirect=1' → this pattern fires → rejected. Yet && in this token is pure data; on the device /bin/sh sees a single-quoted argument with no control operators. The same applies to || in extras values.
Single & was given a quote-aware treatment via rejectUnquotedAmpersand; && and || need the same. Both checks must agree on what counts as "inside a quote span."
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/process-runner.ts
Line: 295
Comment:
**`&&`/`||` pattern fires inside single-quoted spans — false-blocks valid data URIs**
`/&&|\|\|/` is tested against the raw arg string without any quote awareness. When `AdbAdapter.startIntent` wraps a data URI containing `&&` via `quoteForDeviceShell`, the resulting arg is `'https://example.com/?a=1&&b=2'`. The pattern still matches `&&` inside the single-quoted span and throws `COMMAND_BLOCKED`, even though the device shell would treat the entire thing as literal data.
Concrete failure: `start-intent` with `data = "https://x/?state=abc&&redirect=1"` → `quoteForDeviceShell` produces `'https://x/?state=abc&&redirect=1'` → this pattern fires → rejected. Yet `&&` in this token is pure data; on the device `/bin/sh` sees a single-quoted argument with no control operators. The same applies to `||` in extras values.
Single `&` was given a quote-aware treatment via `rejectUnquotedAmpersand`; `&&` and `||` need the same. Both checks must agree on what counts as "inside a quote span."
How can I resolve this? If you propose a fix, please make it concise.| for (let i = 0; i < arg.length; i++) { | ||
| const ch = arg[i]; | ||
| if (!inDouble && ch === "'") { | ||
| inSingle = !inSingle; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
rejectUnquotedAmpersand mishandles the '\'' escape sequence produced by quoteForDeviceShell
quoteForDeviceShell uses the POSIX close-escape-reopen pattern '\'' to embed a literal ' inside a single-quoted value. For "it's & more" it produces 'it'\''s & more'. The scanner doesn't treat \ as an escape outside quoted spans, so it processes '\'' as: close-quote (inSingle=false) → skip \ → open-quote → immediately close-quote — leaving s & more as unquoted. The bare & then triggers COMMAND_BLOCKED.
Adding a single backslash-skip outside quotes fixes the state machine to match POSIX: when !inSingle && !inDouble and ch === '\\', advance i to skip the escaped character before evaluating quote toggles.
| for (let i = 0; i < arg.length; i++) { | |
| const ch = arg[i]; | |
| if (!inDouble && ch === "'") { | |
| inSingle = !inSingle; | |
| continue; | |
| } | |
| for (let i = 0; i < arg.length; i++) { | |
| const ch = arg[i]; | |
| // Outside any quoted span, `\` escapes the next character (POSIX). | |
| // This is required to track quote state correctly through the `'\''` | |
| // pattern that `quoteForDeviceShell` emits for embedded single quotes. | |
| if (!inSingle && !inDouble && ch === "\\") { | |
| i++; | |
| continue; | |
| } | |
| if (!inDouble && ch === "'") { | |
| inSingle = !inSingle; | |
| continue; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/services/process-runner.ts
Line: 244-249
Comment:
**`rejectUnquotedAmpersand` mishandles the `'\''` escape sequence produced by `quoteForDeviceShell`**
`quoteForDeviceShell` uses the POSIX close-escape-reopen pattern `'\''` to embed a literal `'` inside a single-quoted value. For `"it's & more"` it produces `'it'\''s & more'`. The scanner doesn't treat `\` as an escape outside quoted spans, so it processes `'\''` as: close-quote (`inSingle=false`) → skip `\` → open-quote → immediately close-quote — leaving `s & more` as unquoted. The bare `&` then triggers COMMAND_BLOCKED.
Adding a single backslash-skip outside quotes fixes the state machine to match POSIX: when `!inSingle && !inDouble` and `ch === '\\'`, advance `i` to skip the escaped character before evaluating quote toggles.
```suggestion
for (let i = 0; i < arg.length; i++) {
const ch = arg[i];
// Outside any quoted span, `\` escapes the next character (POSIX).
// This is required to track quote state correctly through the `'\''`
// pattern that `quoteForDeviceShell` emits for embedded single quotes.
if (!inSingle && !inDouble && ch === "\\") {
i++;
continue;
}
if (!inDouble && ch === "'") {
inSingle = !inSingle;
continue;
}
```
How can I resolve this? If you propose a fix, please make it concise.…ape (CU-2 follow-up #3) Greptile P1 #1: the regex array still ran `&&` / `||` / `;` / pipe-with- whitespace against the raw arg without quote awareness. After follow-up #2 quote-aware'd the single `&` check, those leftover regex checks now disagreed with the scanner on what counted as "inside a quote span". Concrete consequence: a single-quoted URL emitted by `quoteForDeviceShell` that contained `&&` (e.g. `'https://x/?state=abc&&redirect=1'`) tripped the `&&` regex even though the device shell sees it as literal data. Greptile P1 #2: the scanner walked the POSIX close-escape-reopen pattern `'\''` (emitted by `quoteForDeviceShell` for embedded apostrophes) as close-quote → skip-`\` → open-quote → close-quote, leaving the chars after the escape OUTSIDE any quoted span. A subsequent `&` in the original data then tripped COMMAND_BLOCKED. Refactor (not patch-on-patch): unify all composition checks into ONE character walk in `scanArgForComposition`. Quote+escape state is now the single source of truth — `&&` / `||` / `;` / `&` / `|` / `$` / `` ` `` all share the same notion of "inside a quoted span". Outside any quote span, a `\` is treated as a POSIX escape and advances `i` past the next char (this is what makes `'\''` parse correctly: the close-quote ends the span, then the bare `\'` is an escaped apostrophe outside any span, then a fresh open-quote starts the next span). Two whole-arg regex pre-checks remain (`^(.*)$` subshell wrapper and `^(&&|\|\||[;&|])` chain-at-start) — they're cheap and don't need quote awareness because they only inspect the boundaries. TDD discipline: four new RED-first tests cover the regressions — `&&` / `||` / `;` inside a single-quoted token (allow), and the `'\''` escape pattern with embedded `&` (allow). Three negative tests pin that unquoted `cmd&PWNED`, `cmd&&other`, and `cmd||other` still reject. The pre-existing `tests/adapters/adb.test.ts` defense-in-depth test for `;` in data was inverted to match the new contract: the quote-aware guard now agrees with the device shell that the char is literal inside the quoted token, so the host-side rejection must come from somewhere other than the metacharacter guard. Full suite: 800 passed, 8 skipped (above the 792 baseline). Build, lint, contracts, complexity, and coverage thresholds all clean. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
Second half of the COMPUTER-USE initiative under THE-104. Base of this PR is `feature/computer-use` (PR #134), not master. When #134 merges, GitHub will automatically retarget this PR at master.
Three tickets, four commits on top of #134 (plus a merge of #134's complexity fixes). Implemented by a delegated orchestrator subagent that spawned per-ticket workers and re-assembled.
Included tickets
CU-2 (feat): typed start-intent + argv-aware guard — `THE-106`
`validateShellPayload` previously joined all args after `shell` with spaces and regex-tested the joined string. That was over-broad: a URL containing `&` inside a single arg tripped the guard even though the device shell never sees `&` as a metacharacter when args are passed as separate argv. The guard is now per-arg, scanning each individual arg in `payloadArgs` for chain-attack patterns (`^&`, `^;`, `^|`, backtick-prefixed, `${...`, `$(...`) rather than the joined string. URLs with query params flow through; real chain attempts still get blocked.
This is a tightening, not a loosening: the guard now distinguishes "this arg encodes a chain attempt" from "this arg contains `&` somewhere in the middle of a longer token." Pre-existing CU-1 instruction "do not loosen the guard" was for caller-side fixes; CU-2's whole point is to fix the guard's false-positives on argv form.
On top of that, new `AdbAdapter.startIntent(deviceId, intent)` with typed input validation:
Exposed as a new `adb-app start-intent` operation. Reproducer that previously returned `COMMAND_BLOCKED` (URL with `?foo=bar&baz=qux`) now flows through.
CU-8 (feat): stale-element protection — `THE-112`
`elementIndex` was a query-response-scoped index that became silently stale if the screen changed between `find` and `tap`. Implementing option (a) from the ticket: at find time, store a content fingerprint (text | resourceId | className | bounds) per returned element in `context.lastFindFingerprints`. On `ui-action` with `elementIndex`, re-dump the tree, locate the node at the cached center, recompute the fingerprint, reject with a new `STALE_ELEMENT_INDEX` ErrorCode if it doesn't match. OCR/grid elements skip the check (no AX fields to fingerprint). Default behavior preserved when fingerprints absent (backward compat).
`handleTap` was refactored into per-source resolver helpers (`resolveScreenshotIdTap`, `resolveElementIndexTap`, `resolveTap`) so the new stale check sits in its own helper instead of stretching `handleTap` further.
DECISIONS.md records why option (a) over (b) "persistent session-scoped ID" or (c) "decline."
CU-9 (feat): input verify — `THE-113`
`ui-action input` accepts a new `verify: boolean` (default `false`). When true:
Internal finds snapshot/restore `lastFindResults` so the verify pass doesn't clobber what the agent is iterating over.
Clipboard-paste half is deferred to a follow-up. CU-2's argv-aware guard unblocks the path (so the previous "guard rejects `&` in paste payload" issue is gone), but the remaining scope — Android-version capability detection for `cmd content` vs older `service call clipboard` mechanics — is its own ticket. Design captured in DECISIONS.md.
Test totals
Scope discipline
Out of scope
Test plan
Notes for reviewers
Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com