Skip to content

fix(flow): let a flow launch an app that can never be instrumented - #677

Open
filip131311 wants to merge 1 commit into
mainfrom
filip/flow-launch-non-injectable
Open

fix(flow): let a flow launch an app that can never be instrumented#677
filip131311 wants to merge 1 commit into
mainfrom
filip/flow-launch-non-injectable

Conversation

@filip131311

Copy link
Copy Markdown
Collaborator

Fixes #623.

Reproduced

[0] launch  error  could not connect to native devtools for com.apple.Preferences.
                   Re-run to relaunch the app and retry… restart the argent server…
[1] echo    skip
[2] tap     skip          ok=false errored=1 skipped=2

Control — the identical flow without the launch step passes (ok=true). So the runner was fine; the gate alone made system apps undriveable, and it gave advice that can never work for them.

The signal already existed: isInjectableBundleId is used by native-devtools-status (whose docs call injectable: false terminal — do NOT restart/retry) and by describe. The flow gate never consulted it, so it burned the full 8 s NATIVE_READY_TIMEOUT_MS waiting for a connection that cannot occur.

After:

[0] launch  pass   ⚠ com.apple.Preferences is an Apple system app… coordinate steps work;
                     selector-based steps cannot resolve for this app.
[1] echo    pass
[2] tap     pass          ok=true, ~3s total

Removing the wait exposes a race the wait was hiding — so this closes it too

This is the part I'd want a reviewer to look at hardest.

Selector steps reach resolveNativeTargetApp, which auto-targets whatever app is connected. chooseFrontmostConnectedApp has a weak tier accepting applicationState === "inactive" — exactly what an app reports while it backgrounds. Today the first tree read lands ~9.5 s after launch (8 s gate + 1.5 s settle), comfortably past that window. Without the wait it lands at ~1.5 s, inside it.

So a selector step could resolve against a different app's tree. Most conditions then fail noisily, but hidden-shaped ones (assert: { hidden: X }, await: { hidden: X }, a when: { hidden: X } guard reporting a green skip) would pass. The fix records that the launched app cannot be instrumented and fails tree reads immediately with a terminal reason, which closes it.

Without that guard the later path was also worse than the gate it replaced: NATIVE_TARGET_NO_CONNECTED_APPS says "Launch or restart the app first", after a 3 s settle.

[1] await  fail  `com.apple.Preferences` is an Apple system app, so … 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.

Design choices

Guard on tree reads, not step kindssettleTree + waitForCondition. That covers tap/long-press/await/assert/scroll-to/snapshot in two places, and leaves selector-less pinch/rotate working (they degrade to a default aspect and never read a tree). Guarding by step kind would have missed scroll-to, which calls settleTree directly.

No pre-flight "does this flow use selectors" scan. launch can appear anywhere, including inside a nested fragment, and run: fragments are read lazily at execution time — so "does this flow contain selectors" is ill-posed (which steps? after this launch? under a when: that may not be entered?).

Pass, not fail. The launch genuinely succeeded — the issue's own control proves the rest of such a flow runs. Failing it would re-introduce the bug for coordinate flows.

No 5th status. The CLI maps status with no fallback, so an unknown value renders undefined. Used the existing warning field instead: ⚠ replaces the pass glyph, the text prints under the step, and it's counted in the summary. Both renderers already handle it as legacy wire-compat, so an old CLI renders it correctly — those comments are updated, since it's now actually produced.

Also fixed

The create-flow skill's recording walkthrough used com.apple.Preferences with selector steps (tap: { text: General }). That could never have been captured (selector capture reads the same tree) or replayed. Switched to a third-party app.

Not fixed, deliberately

A fragment with no launch step, run while a system app is frontmost, never sets the flag and still gets the raw message. That's the issue's own control case.

Checks

  • 3088 tests pass; 2 new, both failing against the pre-fix source. All 542 existing flow tests pass unmodified, including flow-composition.test.ts:213, which pins the retry-worded failure for a genuinely injectable app.
  • CLI (277) and MCP (78) renderer suites pass.
  • One unrelated flake on a first full run that passed on re-run.
  • Skills gate 10.0; extract-tools 46/46; prettier, eslint, both typechecks clean; lock untouched.

A flow's launch step waits for native devtools on iOS, and an Apple system app
is a platform binary with library validation, so that connection can never
happen. The step burned the full 8s timeout, failed, and took the rest of the
flow with it — even when nothing in the flow needed the view hierarchy. The
advice it gave ("re-run", "restart the argent server") could never work, which
is the retry loop #453 set out to remove.

The signal was already there: isInjectableBundleId, used by
native-devtools-status and by describe, whose own docs call injectable:false a
terminal state. The gate never consulted it. It does now, and the launch passes
with a warning saying what does and does not work for such an app.

Removing the wait exposes something the wait was accidentally hiding, so the
same change has to close it. Selector steps reach resolveNativeTargetApp, which
auto-targets whatever app is connected and accepts one reporting "inactive" as
foreground-like. An app the user was driving reports exactly that while it
backgrounds. Today the first tree read lands ~9.5s after launch, past that
window; without the wait it lands at ~1.5s, inside it — so a selector could
resolve against a different app's tree, and a hidden assertion against it would
pass. The run now records that the launched app cannot be instrumented and
fails tree reads immediately with a terminal reason.

The guard sits on settleTree and waitForCondition rather than on step kinds, so
it covers tap, long-press, await, assert, scroll-to and snapshot at once, and
leaves selector-less pinch and rotate working — those degrade to a default
aspect and never read a tree.

Reporting a pass rather than a failure is deliberate: the launch did succeed,
and the control case in the issue proves the rest of such a flow runs fine. The
warning field was already rendered by both the CLI and MCP as legacy
wire-compat; it is now produced, so those comments no longer describe it as
something only an old server sends.

The recording walkthrough in the create-flow skill used com.apple.Preferences
with selector steps, which could never have been captured or replayed. It now
uses a third-party app.
@latekvo

latekvo commented Aug 3, 2026

Copy link
Copy Markdown
Member

This is likely a duplicate of #560, although the very same bug seems fixed using a different approach.

@latekvo latekvo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Diplomat]: Reviewed against bce41535, comparing behaviour with origin/main (3fff176a). Evidence is one identical test file run on both, driven through the real createRunFlowTool: the native-devtools service is faked (the pattern in native-target-app.test.ts), not fetchFlowTree, so the whole chain runs — settleTree → fetchFlowTree → queryFullHierarchyTree → resolveNativeTargetApp → queryViewHierarchy → adaptFullHierarchy.

main: 16/16 passed. This branch: 4 failed | 12 passed.

Two things I want to say plainly before the findings. The launch-step fix is right — a flow that launches a system app should not have its launch step errored, and the control in #623 proves the rest of such a flow runs. And the race this PR raises in its description is real: I reproduced it, and on a build with the gate skip kept but the two guards removed, launch of a non-connecting system app followed by assert hidden does flip to a false pass. That case is genuine and the guard does close it.

The findings below are about the guard's derivation, not about whether the problem is worth solving.

}

export async function settleTree(env: ActionEnv): Promise<DescribeNode | undefined> {
if (env.nonInjectableApp) {

Copy link
Copy Markdown
Member

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.Preferences in listConnectedBundleIds(), isConnected() returning true, applicationState: "active" and a getFullHierarchy payload containing Settings, the step assert: { visible: "Settings" } after launch: com.apple.Preferences:

main @3fff176a  ->  launch:pass, assert:pass   ok=true
this branch     ->  launch:pass, assert:fail   ok=false

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 containing Settings. Same for tap: { text: Search }.

That state is not hypothetical: it is what was measured on an iOS 18.5 simulator during the review of #560lsof showing both libArgentInjectionBootstrap.dylib and libNativeDevtoolsIos.dylib mapped into the running process, the process holding a live unix peer of the tool-server's own /tmp/argent-nd-<udid>.sock, native-devtools-status returning "connected": true and "injectable": false in the same response, and a selector fragment passing 4/4. #453 recorded connected: false on 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:

no `launch` step   ->  3/3 pass on BOTH branches   (the guard never arms)
`launch` prefixed  ->  assert:fail, tap:skip, assert:skip   on this branch only

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.nonInjectableApp guards, keeping the flow-run.ts:298 gate 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: waitForNativeDevtools tests api.isConnected(bundleId) at the top of its loop, before the first sleep, and isConnected is a synchronous Map.has. A connected app therefore already returns in ~0 ms on main. Measured, launching a connected system app takes 1507 ms on this branch and 1508 ms on main — 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 there main errored the launch, so no selector step ran.

// wait is the one condition that would otherwise resolve TRUE off an
// unreadable screen.
if (env.nonInjectableApp) {
return { ok: false, reason: nonInjectableTreeReason(env.nonInjectableApp) };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Diplomat]: This early return omits indeterminate, so a when: guard reads an unreadable tree as a plainly-false condition and green-skips the block.

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.

probeWhenCondition's own docstring states the contract: "indeterminate distinguishes an unreadable tree (the caller errors — unknown is not false) from a plainly unmet condition (the caller skips)." execWhenStep branches on exactly that flag, and its docstring adds that "silently skipping would let a broken tree source turn every guarded dismissal into a green no-op." Every other unreadable-tree exit in this same function sets it (the lastTrustedReadAt === undefined arm, and both !lastReadTrusted arms).

Driving launch: com.apple.Preferences then when: { hidden: "Onboarding" } wrapping an echo, against a registry whose resolveService throws:

this branch  ->  launch:pass, when:skip, echo:skip    ok=TRUE
                 when reason: 'condition not met (hidden text="Onboarding") — block skipped (1 step)'
main         ->  launch:error, when:skip              ok=false

The run reports green and the report asserts the element was not hidden, which nothing observed. The sibling await: { hidden: ... } on the identical state correctly reports fail, so the same condition yields two different verdicts depending only on which directive asked. That is the shape of #519, reached through the when: door.

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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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: flow-ios-tree.ts:287 calls resolveNativeTargetApp(nativeApi, undefined), which auto-targets from the connected list and never consults state.nonInjectableApp. Two consequences fall out of the mismatch.

It is bypassable. tool: { name: launch-app, args: { bundleId: com.apple.Preferences } } followed by the same assert never sets the flag:

tool:pass, assert:pass    on BOTH branches

Same app, same launch, same tree — the guard is silent because the launch did not go through the launch: directive. So the terminal claim holds or not depending on which spelling of "launch this app" the flow used.

It fires for an app that was never the read target. Launching com.apple.Preferences while a connected, active, injectable com.example.myapp is what auto-target would resolve: this branch blocks that read with "com.apple.Preferences ... can never be injected", naming an app that is not the one the read would have used.

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 ` +

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Diplomat]: This warning tells the flow author that snapshot works for such an app, and the guard added in this PR contradicts it.

snapshot: { cropOn } routes through waitForFrame into settleTree, which is the flow-actions.ts:342 throw. On launch: com.apple.Preferences + snapshot: { name: row, cropOn: { text: Settings } }:

"kind": "snapshot", "status": "error",
"reason": "`com.apple.Preferences` is an Apple system app, ... This is terminal ..."

On main the same step gets past the tree read. A plain snapshot with no cropOn does work, because flow-visual.ts swallows that throw — so the sentence is true for one form of the directive and false for the other. The PR description's design-choices paragraph lists snapshot as covered by the guard, which is the opposite of what this line tells the author.

filip131311 added a commit that referenced this pull request Aug 4, 2026
…#697)

Fixes #606.

## The bug

A flow step that runs another orchestrator reported `pass` whatever the
nested run did. `case "tool"` treats any non-throwing result as a pass,
and both `flow-execute` and `run-sequence` report failure *in their
result* rather than by throwing.

Reproduced live — the **same flow**, run two ways:

```
run DIRECTLY                            ->  ok=False  passed=0  failed=1
run NESTED via raw `tool: flow-execute`  ->  OUTER ok=True  passed=1  failed=0
                                             step status = pass
                                             sub-report right there: ok=False failed=1
```

The failing verdict was sitting inside the very object being reported as
a pass. A second shape lost the verdict entirely: a sub-flow whose
`executionPrerequisite` was never acknowledged returns a notice, runs
**zero** steps, and also reported a green pass.

## The fix

Two shapes, mapped to statuses the runner already has:

| nested result | status | why |
|---|---|---|
| `ok: false` | **fail** | the composed flow ran and its assertions
failed — what an inline `run:` composition already produces |
| `notice`, zero steps | **error** | never runnable as written; the
class the runner already uses for an unreadable fragment or a cyclic
reference |
| `aborted: true` | **skip** | matches the runner's own rule that a
cancelled step is a skip, never a failure |

Both fail and error hard-stop — in this runner *every* fail and error
does (`state.stopped`), and there is no continue-on-failure concept: a
per-step `optional:` is rejected at parse time because `when:` already
expresses it.

### `run-sequence` had the same hole

Found while reviewing the plan. `run-sequence` has **no verdict field at
all** — every failure path (disallowed tool, unsupported operation,
unmet `await-ui-element`, a tool that threw) pushes an `error` entry,
`break`s, and returns normally. So a flow step whose sequence stopped at
step 1 of 8 also reported a pass. Fixed here rather than left as a known
identical bug on the same line.

### Why not a blanket `ok === false` rule

There is no `ok` contract in this codebase to generalise. The only other
soft-verdict tool spells it `success` (`await-ui-element`),
`run-sequence` spells it neither way, and `case "tool"` dispatches tools
whose results are typed `unknown` or `Record<string, unknown>` — several
carrying app-derived payloads. A blanket rule would silently bind all of
those, and every tool added later, to "a key called `ok` decides my
flow's verdict". `isUnmetUiWaitResult` set the precedent for naming the
tool instead. **There is a test pinning this**, so a future blanket
refactor trips.

Verified exhaustively: exactly one registered tool returns a top-level
`ok` in its result — `flow-execute`. In particular
`settings-permissions` does *not*; its `{ok: false}` is a private
per-`pm`-invocation type that either throws (already an error) or
returns `applied`/`skipped` (a legitimate pass).

## Verified live

```
raw-b-fail   direct  -> ok=False failed=1
             nested  -> OUTER ok=False failed=1, step fail
                        reason: flow "b-fail" failed: 0 passed, 1 failed, 0 errored (await: …)
                        sub-report attached
raw-b-prereq nested  -> OUTER ok=False errored=1, step error
                        reason: flow "b-prereq" did not run — its execution prerequisite was not
                        acknowledged: Settings must be open. Add prerequisiteAcknowledged: true to
                        the step's args, or compose with run: instead.
```

The direct and nested verdicts now agree, which is the exact discrepancy
in the issue.

11 new tests in a new file; the 5 behavioural ones each confirmed to
fail before and pass after. Full suite 3097 passing; lint, prettier,
`typecheck:tests` and the tool-description gate clean.

## Reporting shape

One failing step carrying a summarising `reason` **plus the whole
sub-report in `result`** — the pass path already attached `result`, so
this is the same shape with a non-pass status. The nested steps are
deliberately *not* spliced into the outer `steps[]`: `run:` can expand
inline only because it shares one `ExecState` (one index sequence, one
depth base, one device, one baseline dir), whereas a raw `tool:
flow-execute` is a separate runner invocation. Splicing would mean
renumbering indices and re-homing artifacts — a wire-format change for a
bug fix. Nothing is lost: MCP renders `result` for any step that has
one, and the CLI (which renders only `reason`) gets the sub-flow's own
first failure inside the reason string.

No new `StepReport` fields, no wire-format change; older clients render
the new line unchanged.

## Merge ordering and conflicts

- **Merge #696 (#607) first.** Before it, a nested `flow-execute` could
run against a stale baked-in device and legitimately report `ok: false`;
with this landed that becomes a parent failure and would read like a
regression *caused by* this PR. No code-level conflict — #696 touches
`flow-device.ts` and three other test files.
- **#578 rewrites this exact block** (`case "tool"`, adding an
`evidence` code to each return) and adds
`StepReport.failure`/`durationMs`. Whichever lands second should give
the new branches an evidence code — likely `nested-flow-failed` /
`nested-flow-prerequisite-unacknowledged` / `nested-flow-aborted` — so
its CI diagnostics classify the composition case. **#677** also edits
`execLeafStep` and adds `StepReport.warning`.

## Behaviour change worth knowing

Previously-green flows containing a nested composition that was silently
failing will now go red. That is the fix, but it surfaces pre-existing
breakage on upgrade. The likeliest one is a recorded raw step missing
`prerequisiteAcknowledged`, which becomes a hard error instead of a
silent no-op — the reason names both remedies.

## Follow-up, deliberately not bundled

Raw `tool: flow-execute` nesting has **no cycle or depth guard**:
`MAX_RUN_DEPTH` and the run-stack cycle check cover only `run:`, so a
flow whose raw step names itself recurses through fresh runner
invocations. Independent of this issue; filing separately.

---

> **Stacked on #649** (`filip/flow-deviceless-and-counts`). Both edit
the import block of `packages/tool-server/src/tools/flows/flow-run.ts` —
#649 widens the `./flow-device` import for its device-optional run, this
one adds `./flow-nested-outcome`. Rebased on #649, so review only the
top commit. GitHub retargets it to `main` when #649 merges.

Co-authored-by: Filip131311 <f.kaminski2000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

flow launch step requires native devtools, so a coordinate-only flow can never launch a non-injectable app (com.apple.*)

2 participants