-
Notifications
You must be signed in to change notification settings - Fork 63
fix(flow): let a flow launch an app that can never be instrumented #677
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,12 +52,22 @@ export interface ActionEnv { | |
| ctx?: ToolContext; | ||
| device: DeviceInfo; | ||
| signal?: AbortSignal; | ||
| /** | ||
| * Bundle id of an app the run launched that can never carry the | ||
| * view-hierarchy instrumentation. Set means every tree read is doomed, so | ||
| * they fail immediately with a terminal reason instead of polling a source | ||
| * that will never appear — and, more importantly, instead of auto-targeting | ||
| * whichever other app happens to still be connected. | ||
| */ | ||
| nonInjectableApp?: string; | ||
| } | ||
|
|
||
| /** Outcome of a selector directive: ok, or a machine-readable reason it failed. */ | ||
| export interface DirectiveOutcome { | ||
| ok: boolean; | ||
| reason?: string; | ||
| /** Caveat carried by a step that still succeeded. */ | ||
| warning?: string; | ||
| /** The run was cancelled mid-step — reported as a skip, not a step failure. */ | ||
| aborted?: boolean; | ||
| /** | ||
|
|
@@ -314,7 +324,24 @@ function flowSelectorToFrame(tree: DescribeNode, sel: FlowSelector): DescribeFra | |
| * convert the outage into a misleading "element not found" downstream. The | ||
| * throw lands in the step's structured report via `execLeafStep`'s catch. | ||
| */ | ||
| /** | ||
| * Why no selector can resolve against an app that cannot be instrumented. | ||
| * Deliberately not the native-devtools recovery text, which tells the caller to | ||
| * use `describe`/`screenshot` — the answer for a flow is coordinate steps. | ||
| */ | ||
| export function nonInjectableTreeReason(bundleId: string): string { | ||
| return ( | ||
| `\`${bundleId}\` is an Apple system app, so argent's view-hierarchy instrumentation can never ` + | ||
| `be injected into it and selector-based steps cannot resolve. This is terminal — relaunching or ` + | ||
| `restarting the argent server will not change it. Target this screen by coordinate ` + | ||
| `(\`tap: { x, y }\`) instead.` | ||
| ); | ||
| } | ||
|
|
||
| export async function settleTree(env: ActionEnv): Promise<DescribeNode | undefined> { | ||
| if (env.nonInjectableApp) { | ||
| throw new Error(nonInjectableTreeReason(env.nonInjectableApp)); | ||
| } | ||
| const deadline = Date.now() + SETTLE_TIMEOUT_MS; | ||
| let prevFp: string | undefined; | ||
| let prevTree: DescribeNode | undefined; | ||
|
|
@@ -992,6 +1019,12 @@ async function waitForCondition( | |
| }, | ||
| timeoutMs: number | ||
| ): Promise<DirectiveOutcome> { | ||
| // Not routed through settleTree, so it needs its own guard — and a `hidden` | ||
| // wait is the one condition that would otherwise resolve TRUE off an | ||
| // unreadable screen. | ||
| if (env.nonInjectableApp) { | ||
| return { ok: false, reason: nonInjectableTreeReason(env.nonInjectableApp) }; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat]: This early return omits This one does not depend on the injectability question at all — it fires when the premise is entirely correct, on an app that genuinely never connects.
Driving The run reports green and the report asserts the element was not hidden, which nothing observed. The sibling |
||
| } | ||
| const deadline = Date.now() + timeoutMs; | ||
|
|
||
| let lastMatches: ReturnType<typeof findAll> = []; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,11 @@ import { | |
| type ActionEnv, | ||
| type DirectiveOutcome, | ||
| } from "./flow-actions"; | ||
| import { nativeDevtoolsRef, type NativeDevtoolsApi } from "../../blueprints/native-devtools"; | ||
| import { | ||
| nativeDevtoolsRef, | ||
| isInjectableBundleId, | ||
| type NativeDevtoolsApi, | ||
| } from "../../blueprints/native-devtools"; | ||
| import { androidDevtoolsRef, type AndroidDevtoolsApi } from "../../blueprints/android-devtools"; | ||
| import { | ||
| chromiumCdpRef, | ||
|
|
@@ -111,6 +115,12 @@ export interface StepReport { | |
| * percentage, baseline written/updated). | ||
| */ | ||
| reason?: string; | ||
| /** | ||
| * A caveat about a step that still passed — currently, launching an app that | ||
| * can never carry the view-hierarchy instrumentation. Renderers show it as a | ||
| * `⚠` in place of the pass glyph and count it in the summary. | ||
| */ | ||
| warning?: string; | ||
| /** Underlying tool id for `tool` steps. */ | ||
| tool?: string; | ||
| /** Tool result for `tool` steps. */ | ||
|
|
@@ -282,6 +292,10 @@ async function treeSourceGate( | |
| signal?: AbortSignal | ||
| ): Promise<string | null> { | ||
| if (device.platform === "ios" && !signal?.aborted) { | ||
| // An app that can never be injected will never connect, so waiting the full | ||
| // timeout only delays advice that cannot work. The flow can still run every | ||
| // step that does not read the view hierarchy. | ||
| if (!isInjectableBundleId(bundleId)) return null; | ||
| const connected = await waitForNativeDevtools(registry, device, bundleId, signal); | ||
| if (!connected && !signal?.aborted) { | ||
| return ( | ||
|
|
@@ -396,11 +410,25 @@ async function runLaunch(state: ExecState, app: Launch): Promise<DirectiveOutcom | |
| return { ok: false, reason: `restart-app failed: ${errMsg(err)}` }; | ||
| } | ||
| if (!(await sleepOrAbort(POST_LAUNCH_SETTLE_MS, signal))) return ABORTED_OUTCOME; | ||
| // Recorded on every launch, so a later injectable launch clears it. | ||
| state.nonInjectableApp = | ||
| device.platform === "ios" && !isInjectableBundleId(bundleId) ? bundleId : undefined; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat]: The guard is keyed on the launched bundle id, but the read it guards resolves a different quantity: It is bypassable. Same app, same launch, same tree — the guard is silent because the launch did not go through the It fires for an app that was never the read target. Launching |
||
|
|
||
| const gate = await treeSourceGate(registry, device, bundleId, signal); | ||
| // The gate returns null (ready) on abort — check the signal before trusting | ||
| // it, or a cancelled gate would read as a launch that verified readiness. | ||
| if (signal?.aborted) return ABORTED_OUTCOME; | ||
| if (gate) return { ok: false, reason: gate }; | ||
| if (state.nonInjectableApp) { | ||
| return { | ||
| ok: true, | ||
| warning: | ||
| `${bundleId} is an Apple system app: it is a platform binary with library validation, so ` + | ||
| `argent's view-hierarchy instrumentation can never be injected into it. The app launched — ` + | ||
| `coordinate steps (\`tap: { x, y }\`), \`wait\` and \`snapshot\` work; selector-based steps ` + | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Diplomat]: This warning tells the flow author that
On |
||
| `cannot resolve for this app.`, | ||
| }; | ||
| } | ||
| return { ok: true }; | ||
| } | ||
|
|
||
|
|
@@ -1052,7 +1080,12 @@ async function execLeafStep( | |
| // A run cancelled mid-launch is a skip (matching the pre-step guard and | ||
| // the directives), never a step failure — the app did nothing wrong. | ||
| if (r.aborted) return { ...base, status: "skip", reason: r.reason }; | ||
| return { ...base, status: r.ok ? "pass" : "error", reason: r.reason }; | ||
| return { | ||
| ...base, | ||
| status: r.ok ? "pass" : "error", | ||
| reason: r.reason, | ||
| ...(r.warning ? { warning: r.warning } : {}), | ||
| }; | ||
| } | ||
|
|
||
| case "tap": | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Diplomat]: This derives a terminal verdict about the view hierarchy from
bundleId.startsWith("com.apple.")rather than from whether the hierarchy read succeeds, and where those two disagree it turns a passing flow into a failing one.With
com.apple.PreferencesinlistConnectedBundleIds(),isConnected()returning true,applicationState: "active"and agetFullHierarchypayload containingSettings, the stepassert: { visible: "Settings" }afterlaunch: com.apple.Preferences:with the reason
"... can never be injected into it and selector-based steps cannot resolve. This is terminal — relaunching or restarting the argent server will not change it."— emitted while the read that message describes as impossible had already returned a tree containingSettings. Same fortap: { text: Search }.That state is not hypothetical: it is what was measured on an iOS 18.5 simulator during the review of #560 —
lsofshowing bothlibArgentInjectionBootstrap.dylibandlibNativeDevtoolsIos.dylibmapped into the running process, the process holding a live unix peer of the tool-server's own/tmp/argent-nd-<udid>.sock,native-devtools-statusreturning"connected": trueand"injectable": falsein the same response, and a selector fragment passing 4/4. #453 recordedconnected: falseon iOS 26.5 and #623 was filed from a 26.5 matrix, so the evidence splits by runtime; the message states one runtime's reading as a universal.The clearest form of it is a pair that differs by one line of YAML. Same app, same connection, same three steps, same tree:
A second consequence: for a system app that genuinely never connects, this branch emits byte-identical output to the connected case above, so the two are no longer distinguishable from the report. On the guards-removed build the unreadable case still fails, but says
"could not read the UI tree: No native-devtools-connected apps are available for auto-targeting."On isolating it — deleting only the two
env.nonInjectableAppguards, keeping theflow-run.ts:298gate skip and the launch warning, turns the whole file green at 16/16. The gate skip is not implicated.On the 8 s the description cites as the cost being removed:
waitForNativeDevtoolstestsapi.isConnected(bundleId)at the top of its loop, before the first sleep, andisConnectedis a synchronousMap.has. A connected app therefore already returns in ~0 ms onmain. Measured, launching a connected system app takes 1507 ms on this branch and 1508 ms onmain— the wait being removed never fires for the case that regresses. The full 8 s elapses only when the app never connects (9522 ms), and theremainerrored the launch, so no selector step ran.