Skip to content

feat: per-call observe level; no state in observations - #62

Merged
cryptotavares merged 3 commits into
cryptotavares/mmcli-p1-05-observe-mutating-onlyfrom
cryptotavares/mmcli-p1-06-observe-level-no-state
Oct 29, 2026
Merged

cryptotavares merged 3 commits into
cryptotavares/mmcli-p1-05-observe-mutating-onlyfrom
cryptotavares/mmcli-p1-06-observe-level-no-state

Conversation

@cryptotavares

@cryptotavares cryptotavares commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary: Let each tool call choose observe: 'none' | 'diff' | 'full'. Remove app state from observations and stop the mobile driver from inventing wallet-state values.

Stack context: mm CLI v2, Phase 1, P1-06 + P1-07, combined at the shared observation boundary. Base: cryptotavares/mmcli-p1-05-observe-mutating-only; review #61 first. Followed by #64.

Problem

Tool categories fix the observation policy. Callers cannot skip a snapshot while chaining clicks or request one after a read. run_steps uses a separate includeObservations policy.

Observations also combine screen data with app state. Mobile returns guessed wallet values, including isUnlocked: true and currentScreen: "unknown". Browser observation collection reads extension state and can spend another 500 ms rechecking an unknown screen.

These concerns belong at different boundaries: the daemon controls observations, while get_state reports platform state.

Solution

Add one shared observation field to registered tool schemas. Consume it in the daemon, outside the tool-handler contract.

flowchart TD
    A[CLI --observe or HTTP observe] --> B[Validate the shared observe field]
    B --> C[Strip observe before handler dispatch]
    C --> D[Execute the tool or batch]
    D --> E[Resolve explicit level or default]
    E -->|none or no active session| F[Return the tool response]
    E -->|diff or full with active session| G[Collect test IDs and a11y only]
    G --> H[Compact options; full skips diff]
    H --> F

    classDef new fill:#fff3cd,stroke:#d39e00
    class A,B,C,E,G,H new
Loading

Yellow boxes are new.

withObserve() preserves existing schema refinements and defaults through intersections. Both HTTP and batch dispatch call omitObserve() before the handler. Consumer launch implementations therefore receive their existing input shape.

For run_steps, the top-level level wins. Otherwise, the last executed step's args.observe wins, then diff. Timeout-skipped steps do not count. Earlier per-step levels do not trigger separate observations.

Observation collection reads only test IDs and accessibility nodes. It no longer calls getExtensionState() or getAppState(). Browser collection waits for page settling for mutating, batch, and CDP calls. Read-only and discovery calls skip that wait.

What changes

Condition or boundary Before After
Default tool call Category controls collection; batch has a separate policy diff for mutating tools and batches; none for read-only, discovery, and CDP tools
Explicit observe: 'none' No shared override Skips collection and returns no observations; observationMs is zero
Explicit observe: 'diff' or 'full' with an active session No shared override diff uses the previous snapshot; full returns the whole option-collapsed tree without diff metadata
CLI --observe <level> Unsupported Accepted anywhere; injected into POST /tool/*, /launch, and /cleanup object bodies; an existing body field wins
run_steps.includeObservations Accepts all, none, or failures Fails validation with includeObservations was removed; use observe
Observation payload App state, test IDs, and a11y Test IDs and a11y only; use get_state for app state
Mobile getAppState() Adds guessed wallet fields Returns { isLoaded, extensionId, source: 'stub' }; unread fields are omitted

For ordinary calls, a full observation becomes the next diff baseline; none leaves it unchanged. Existing reset tools still clear it: describe_screen, accessibility_snapshot, launch, and cleanup.

An invalid CLI level exits with code 1 and prints:

Error: --observe requires one of: none, diff, full

Decisions for reviewers

Decision Reason
Combine P1-06 and P1-07 Observation selection and state removal change the same collection path. Keeping one boundary avoids separate platform policies.
Consume observe outside handlers The daemon owns collection. Shared validation accepts the field without passing it into consumer implementations.
Collect once after a batch The final screen supplies the batch observation. Top-level configuration takes precedence over the last executed step.
Keep full compacted Callers skip diffing but retain option collapsing. The existing discovery tools remain available for unfiltered trees.
Mark mobile state as partial Optional ExtensionState fields distinguish unread data from real false, empty, or null values.
Keep the registered-function dispatch guard The HTTP executor checks registry membership and callable type before dispatching a request-derived tool name.

Review guide

Read the files in this order:

# File What to check
1 src/validation/schemas.ts, src/tools/types/tool-inputs.ts Shared levels, schema intersections, removed batch policy, and omitObserve.
2 src/server/create-server.ts, src/tools/batch.ts, src/tools/registry.ts Override precedence, timeout-skipped steps, handler isolation, category defaults, and registered-function dispatch.
3 src/tools/types/observation.ts, src/server/observation-compaction.ts State-free snapshots, option compaction, full-versus-diff behavior, and baseline handling.
4 src/capabilities/types.ts, src/platform/mobile-platform-driver.ts Optional state fields and the mobile partial-state result.
5 src/cli/mm.ts Global parsing, exact error text, request-body injection, and body-field precedence.
6 Changed tests, README.md, SKILL.md HTTP and batch behavior, CLI injection, mobile state, and caller migration guidance.
Testing
  • Fresh check at remote head 9f8803964119722b13c10e989be20aa5e035c0bc: CI=true yarn vitest run src/cli/mm.test.ts src/platform/mobile-platform-driver.test.ts src/server/create-server.test.ts src/server/observation-compaction.test.ts src/tools/batch.test.ts src/tools/registry.test.ts src/tools/types/observation.test.ts src/validation/schemas.test.ts --coverage.enabled=false --maxWorkers=2 passes: 8 files, 683 tests. Vitest reports no type errors.
  • yarn lint and the production type check, yarn tsc --noEmit --emitDeclarationOnly false -p tsconfig.build.json, pass.
  • Standalone yarn tsc --noEmit -p tsconfig.json fails with nine test-fixture diagnostics already present at this remote head. These affect Hermes, mobile backend mocks, a server test table, mock-network result narrowing, and a screenshot fixture.
  • The tests cover category defaults, explicit overrides, last-executed-step selection, stripped handler input, full/diff baselines, and state-free observations.
  • The Proof and Unit tests sections below retain the original historical evidence. These fresh checks do not repeat live platform validation.

Breaking changes

  • run_steps.includeObservations is removed. Sending it is a validation error (includeObservations was removed; use observe). The 'failures' mode is gone.
  • Observations no longer contain state:
    • StepRecordObservation has no state field;
    • createDefaultObservation(testIds, nodes) drops its first parameter.
  • ExtensionState: currentUrl, isUnlocked, currentScreen, accountAddress, networkName, chainId and balance are optional. New field: source?: 'stub'.
  • MobilePlatformDriver.getAppState() returns { isLoaded, extensionId, source: 'stub' } only.
  • shouldCollectObservations and shouldIncludeObservationsInResponse are replaced by resolveObserveLevel(category, input, toolResult).
  • toolSchemas entries are now intersections that include observe. The individual *InputSchema exports are unchanged.

Consumer impact

Consumer Change needed Done on branch
Extension test/e2e/playwright/llm-workflow/ None. Its own ExtensionState with required fields still type-checks against the looser core type, and nothing reads observations.state or includeObservations. Jest: 144 passed. n/a
Mobile tests/llm-workflow/ The state snapshot capability stops inventing isUnlocked/currentScreen/null fields: with no session it reports {isLoaded:false, extensionId:'', source:'stub'}, and it omits currentScreen when no known testId matches. Jest: 232 passed. cryptotavares/mm-cli-v2-mobile 35fc72cc609

Follow-up, not in this PR: on mobile, core get_state calls the driver directly, so the Mobile capability's screen detection only runs on browser. Phase 2 mm state (P2-09) covers this.

Proof

Tested with a yalc build of this branch in the Extension (e2e build, headless) and Mobile (dev client on iOS sim and Android emulator), on wallet home after unlock. Before = #61 tip; its proof is reused, except the Extension click bench, which was run on both builds back to back.

Same command at each level (after; bytes / totalMs):

Platform none diff full
ext (click a home tab) 197 / 17 6760 / 581 9441 / 586
iOS (wait-for wallet-screen) 183 / 880 7495 / 2923 58865 / 2905
Android (click tab-bar-item-Wallet) 193 / 1794 7672 / 3671 1059191 / 3666
Check Result
click --observe none no observations, observationMs 0, on all 3 platforms
get-state --observe full observation with testIds + a11y only: no state, no diff
run-steps, top-level observe: none / last step observe: full no observations / full tree without diff (all 3 platforms)
run-steps with includeObservations exit 1: includeObservations was removed; use observe
observations.state in any of the 55 after outputs none
Mobile get-state, before → after isUnlocked: true, currentScreen: "unknown", currentUrl: "", 4 × null → {isLoaded, extensionId, source: "stub"}
ext click observationMs p50 (bench, 5 runs, back to back) 564 → 555 ms; bytes 7335 → 6760 (the state block)
ext smoke click-unlock (unlock → home) observationMs 71 → 15 ms (the re-check loop no longer runs while the screen is unknown)

The bench gain is small because on home the screen is already known, so the 500 ms re-check rarely ran. Most of the remaining ~550 ms is the animation wait plus the a11y snapshot. P1-10 targets the animation wait.

Transcripts (ext)
$ yarn mm click --testid unlock-submit --observe none
{
  "clicked": true,
  "target": "testId:unlock-submit",
  "meta": { "timing": { "queueMs": 0, "toolMs": 32, "observationMs": 0, "totalMs": 33 } }
}

$ yarn mm run-steps '{"steps":[{"tool":"get_state"}],"includeObservations":"none"}'
Error: includeObservations: includeObservations was removed; use observe
exit=1

$ yarn mm --observe bogus get-state
Error: --observe requires one of: none, diff, full
exit=1
Unit tests
Test Files  46 passed (46)
     Tests  1450 passed (1450)
Type Errors no errors
All files | 96.94 | 93.52 | 94.91 | 97.12
yarn lint → exit 0

New or changed:

  • resolveObserveLevel: category defaults, an explicit level wins, an invalid level is ignored, run_steps last executed step and failure fallback.
  • Endpoint tests: click with none; get_state with full/diff; cdp with full waits for animations; observe is not passed to the handler; /cleanup body.
  • No getExtensionState during observation (spy).
  • Mobile getAppState shape.
  • withObserve/omitObserve, includeObservations rejected, and the CLI --observe parsing and injection.

Comment thread src/server/create-server.ts Fixed
@cryptotavares

Copy link
Copy Markdown
Collaborator Author

Fixed CodeQL js/unvalidated-dynamic-method-call in 1bbfc70 (check the registry entry is a function before calling it).

@cryptotavares
cryptotavares force-pushed the cryptotavares/mmcli-p1-06-observe-level-no-state branch 2 times, most recently from 2ee766a to 9f88039 Compare October 24, 2026 01:35
@cryptotavares
cryptotavares marked this pull request as ready for review October 27, 2026 13:40
@cryptotavares
cryptotavares requested a review from a team as a code owner October 27, 2026 13:40
Callers can now choose the observation they get back, and observations no
longer carry app state (which was wrong on mobile and slow on browser).

- Every tool input accepts `observe: 'none' | 'diff' | 'full'` (shared
  observeLevelSchema, added to toolSchemas with withObserve()). The daemon
  reads it and strips it before the tool runs. Defaults: diff for mutating
  tools and run_steps, none for readonly/discovery/cdp. `full` returns the
  whole (option-collapsed) tree without diffing and becomes the new diff
  baseline.
- CLI: global `--observe <none|diff|full>` flag, added to POST /tool/*,
  /launch and /cleanup bodies.
- run_steps: `includeObservations` is removed. The batch observation uses the
  top-level `observe`, else the last executed step's `observe`, else diff.
- Observations no longer include `state`. The getExtensionState/getAppState
  calls and the 500 ms unknown-screen recheck loop are gone from the
  observation path. Use get_state for state.
- The mobile driver's getAppState reports only what it knows:
  { isLoaded, extensionId, source: 'stub' }. `source: 'stub'` marks partial
  state; fields not read from the app are omitted.

BREAKING CHANGE: `run_steps.includeObservations` is removed and now fails
validation with a hint to use `observe` (the 'failures' mode is gone).
MobilePlatformDriver.getAppState() now returns only
{ isLoaded, extensionId, source: 'stub' } (no isUnlocked, currentScreen,
currentUrl or null wallet fields). Observations no
longer contain `state`; StepRecordObservation has no `state` field and
createDefaultObservation(testIds, nodes) drops its state parameter.
ExtensionState fields other than isLoaded and extensionId are optional and a
`source?: 'stub'` field is added. shouldCollectObservations and
shouldIncludeObservationsInResponse are replaced by resolveObserveLevel.
toolSchemas entries are now intersections that include `observe`.
CodeQL (js/unvalidated-dynamic-method-call) flagged the tool call because
the tool name comes from the request path. Look the tool up only when the
registry has the name, and check the entry is a function.
@cryptotavares
cryptotavares force-pushed the cryptotavares/mmcli-p1-06-observe-level-no-state branch from fae671b to 9991596 Compare October 28, 2026 11:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@cryptotavares cryptotavares changed the title feat!: per-call observe level; no state in observations feat: per-call observe level; no state in observations Oct 28, 2026

@seaona seaona 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.

2 small comments, let me know what you think 🙏

// Tools that may change the screen (mutating, batch, and a
// cdp call that asked for an observation) wait for the page
// to settle first.
if (category !== 'readonly' && category !== 'discovery') {

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.

Minor: with --observe none the mutation returns without settling, and reads skip the settle even with --observe full
When both are chained in one shell call (mm click … --observe none && mm get-state --observe full), the snapshot can land mid-animation. Is it worth settling whenever an observation is collected, or is it fine to just note it in SKILL.md?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Documented in #71 (SKILL.md and README): only mutating, batch and cdp calls wait for animations; reads and describe-screen read the page at once, so click --observe none && get-state --observe full can catch a screen mid-animation. The recommendation is to let the action itself observe (click --observe full). Since #65 made the settle cheap (about one frame on an idle page), waiting on every observation is an option for later.

Comment thread SKILL.md
| **CDP** | cdp | No — raw result only |
| **Batch** | run_steps | Controlled by `includeObservations` param |
```bash
mm click e3 --observe none # chaining clicks: skip the observation

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.

With --observe none the ref map isn't refreshed, so the next ref-based action (e.g. mm click e5) uses selectors from the previous screen. If the screen changed, that selector can match a different element.
Some refs fail loudly with MM_TARGET_NOT_FOUND (after #66, the ones backed by a data-testid or id). But role-and-name and nth-child path refs, and mobile label refs, can silently click the wrong element.

Maybe worth adding a small note: "after --observe none, call describe-screen before using refs again" ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, added to SKILL.md, the README and mm --help in #71: after --observe none refs are not refreshed, testId/id refs fail with MM_TARGET_NOT_FOUND, but role/name, DOM-path and mobile label refs can match a different element, so target by testId or run describe-screen before using refs again.

@cryptotavares
cryptotavares merged commit f4ae920 into main Oct 29, 2026
27 of 28 checks passed
@cryptotavares
cryptotavares deleted the cryptotavares/mmcli-p1-06-observe-level-no-state branch October 29, 2026 13:52
cryptotavares added a commit that referenced this pull request Oct 29, 2026
> **Summary:** Skip automatic observation collection for read-only,
discovery, and raw CDP tools. Keep observations for mutating tools and
eligible batches, and reset the diff baseline after accessibility
snapshots.

**Stack context:** mm CLI v2, Phase 1, **P1-05**. Base:
[#60](#60)
(`cryptotavares/mmcli-p1-04-remove-knowledge-store`); followed by
[#62](#62). Review #60
first.

## Problem

The daemon collects observations after read-only and discovery calls,
although their responses omit those observations. That work also
refreshes refs and advances the diff baseline. The knowledge store that
consumed it is gone after #60.

Raw `cdp` calls use the `mutating` category. A small result, such as
`Runtime.evaluate 1+1`, therefore triggers and returns an additional
observation. On mobile, observation collection queries app state and
requests test IDs and an accessibility tree.

## Solution

Keep the policy at the shared HTTP executor. Tool handlers and platform
drivers retain their existing responsibilities.

```mermaid
flowchart TD
    A[Queued tool finishes] --> B{Category and batch policy}
    B -->|readonly, discovery, cdp, or batch none| C[Skip observation collection]
    B -->|mutating or eligible batch| D{Active session?}
    D -->|yes| E[Collect observation through platform driver]
    D -->|no| C
    E --> F[Apply response inclusion policy]
    F -->|included| G[Compact against last observation]
    F -->|omitted| H[Return result and timing]
    C --> H
    G --> H

    classDef new fill:#fff3cd,stroke:#d39e00
    class B,C,F new
```

Yellow boxes are new.

`shouldIncludeObservationsInResponse` calls `shouldCollectObservations`
first. A shared batch-policy helper supplies the default, so collection
and response inclusion use the same rule.

The registry gives `cdp` its own public category. CDP method execution
stays in the platform driver. A state-changing CDP call needs a
subsequent `mm describe-screen` to refresh refs. Per-call `observe`
opt-in remains a **P1-06** follow-up.

`accessibility_snapshot` now clears the diff baseline, like
`describe_screen`, `launch`, and `cleanup`. The next collected mutation
observation is full and compacted. Other read calls leave the baseline
unchanged.

## What changes

| Condition or boundary | Before | After |
|---|---|---|
| `readonly` or `discovery` call | Collects an observation but omits it
from the response | Skips automatic collection; `observationMs` is zero
|
| Raw `cdp` call | Uses `mutating`; collects and returns an observation
when available | Uses `cdp`; skips automatic collection and returns no
`observations` |
| Mutating call with an active session | Collects and returns an
observation when available | Same collection, compaction, and non-fatal
failure handling |
| `run_steps` policy | `all` by default; `none` skips collection;
`failures` filters the response | Same batch-level policy; `failures`
returns observations only for a tool error or failed summary |
| Ref refresh | Automatic observations refresh the session refMap |
Skipped observations do not refresh refs; tree-producing discovery
handlers still update refs themselves |
| `accessibility_snapshot` followed by a mutation | Retains an older
diff baseline | Clears the baseline; the next mutation returns a full
compact observation |

## Decisions for reviewers

| Decision | Reason |
|---|---|
| Stop collection at the executor boundary | Removes redundant browser
and mobile work without changing each read-tool handler. |
| Add `cdp` to public `ToolCategory` | Makes the default explicit. CDP
methods can change state, so callers must request fresh discovery
afterward. |
| Keep collection separate from response inclusion | A successful batch
with `failures` still collects an observation and advances the baseline,
but omits it from the response. |
| Clear the baseline after accessibility snapshots | The caller receives
a tree directly; the next mutation must not diff against an older
observation. |

## Review guide

Read the files in this order:

| # | File | What to check |
|---|---|---|
| 1 | `src/tools/registry.ts` | Public `cdp` category, tool assignment,
and unchanged safe default for unknown tool names. |
| 2 | `src/server/create-server.ts` | Collection predicate, shared batch
policy, response inclusion, and accessibility baseline reset. |
| 3 | `src/server/create-server.test.ts` | Per-category timing, skipped
collection, batch failures, and baseline behavior. |
| 4 | `src/tools/registry.test.ts` | `cdp` classification and
registry/category coverage. |
| 5 | `README.md`, `SKILL.md` | Observation rules, CDP ref-refresh
instructions, and baseline reset documentation. |

<details>
<summary>Testing</summary>

- Fresh check at remote head `74b9819d26e4e1ff63b5a9be675a4b3cefd94e68`:
`CI=true yarn vitest run src/server/create-server.test.ts
src/tools/registry.test.ts src/tools/batch.test.ts src/tools/cdp.test.ts
src/tools/discovery-tools.test.ts --coverage.enabled=false
--maxWorkers=2 --minWorkers=1` passes: 5 files, 181 tests.
- `yarn lint` passes. Production type checking passes with `yarn tsc
--project tsconfig.build.json --noEmit`.
- `yarn tsc --project tsconfig.test.json --noEmit` fails at the
unchanged remote head with nine diagnostics. They affect the new
timing-table callback and existing mobile-driver, network-mock, and
screenshot tests. Vitest reports no type errors, but its result does not
establish a clean explicit test-project type check.
- The `Proof` and `Unit tests` sections below retain the original
historical evidence. These fresh checks do not repeat live-device
validation.

</details>

## Breaking changes

- `readonly`, `discovery` and `cdp` tools no longer collect
observations. As a result:
- their responses no longer include `observations` (`cdp` used to
include them);
- they no longer refresh the session refMap through an observation
(discovery tools still set it themselves);
  - they no longer move the diff baseline for the next mutating call.
- A `cdp` call that changes state (for example `Runtime.evaluate` with a
`.click()`) must now be followed by `describe-screen` to get fresh refs.
- The public `ToolCategory` type gains `'cdp'`. Exhaustive switches on
it need a new case.

## Consumer impact

| Consumer | Change needed | Done on branch |
|---|---|---|
| Extension `test/e2e/playwright/llm-workflow/` | None. No code or test
reads `observations`. | n/a |
| Mobile `tests/llm-workflow/` | None. Same check. | n/a |

## Proof

Tested with a yalc build of this branch in the Extension (e2e build,
headless) and in Mobile (dev client on iOS sim and Android emulator), on
wallet home after unlock. Before = #60 tip (`0.12.0+f2caa574`), after =
`0.12.0+ce3fd9f0`. Ext was rechecked on the final commit
(`0.12.0+8a7091bf`). Bench: `scripts/bench.mjs`, 5 runs (Android 3),
p50.

| Check | Before | After |
|---|---|---|
| `cdp Runtime.evaluate 1+1` bytes, ext / iOS / Android | 7408 / 59228 /
1054409 (smoke) | **267 / 240 / 244** |
| `get-state` totalMs, ext / iOS / Android | 695 / 5736 / 6345 | **9 /
652 / 129** |
| `cdp` totalMs, ext / iOS / Android | 963 / 3334 / 6882 | **2 / 6 /
497** |
| `list-testids` totalMs, ext / iOS / Android | 1340 / 8901 / 13665 |
632 / 3835 / 11639 |
| `observationMs` for `get-state`, `get-text`, `hermes-targets`,
`list-testids`, `describe-screen`, `screenshot`,
`accessibility-snapshot`, `cdp` | 53–8001 | **0** (no `observations`
key), all platforms |
| `click --testid unlock-submit` observations, ext / iOS / Android | yes
| yes (71 / 4638 / 2355 ms) |

<details>
<summary>Notes from the proof run</summary>

- Host load was 7.7–18.4 during the runs, so mobile toolMs varies a lot
from run to run.
- Android discovery `toolMs` went up in the after run (`list-testids`
p50 5975 → 11639 ms). This change does not touch discovery tools. The
likely cause is variation in the ~1 MB Android a11y snapshot (the same
area as the existing `list-testids` timeout). Totals still fell because
observation time is now 0.
- On Android, some mutating calls (`wait-for`, `dismiss-keyboard`) still
hit the 8 s observation timeout and return no `observations`. This
already happened before the change (P1-04 build: iOS `click-unlock` 8003
ms).

</details>

<details>
<summary>Transcripts (ext, final commit)</summary>

```
$ yarn mm cdp Runtime.evaluate '{"expression":"1+1","returnByValue":true}'
{
  "method": "Runtime.evaluate",
  "result": {
    "result": {
      "type": "number",
      "value": 2,
      "description": "2"
    }
  },
  "meta": { "timing": { "queueMs": 0, "toolMs": 2, "observationMs": 0, "totalMs": 3 } }
}

click-unlock     obs=yes observationMs=16
describe-screen  obs=no  observationMs=0
get-state        obs=no  observationMs=0
list-testids     obs=no  observationMs=0
cdp-eval         obs=no  observationMs=0  bytes=266
screenshot       obs=no  observationMs=0
```

</details>

<details>
<summary>Unit tests</summary>

```
Test Files  46 passed (46)
     Tests  1406 passed (1406)
Type Errors no errors
All files | 96.92 | 93.37 | 94.85 | 97.1
yarn lint → exit 0
```

New or changed: per-category tests for `shouldCollectObservations` and
`shouldIncludeObservationsInResponse`; endpoint tests for
`observationMs` (0 for readonly, discovery and cdp; > 0 for mutating);
read tools don't move the diff baseline; `accessibility_snapshot` resets
it; the cdp category in the registry.

</details>
cryptotavares added a commit that referenced this pull request Oct 29, 2026
…refs (#64)

> **Summary:** Cancel timed-out requests cooperatively and fence ref-map
commits against aborted or older requests. Capture mobile trees and test
IDs together, and settle mobile observations after mutations.
>
> **Scope change (2026-10-27):** the ref-stability layer (renumbering
refs so unchanged elements keep their numbers) was removed in `6944868`.
Refs are positional again, as on `main`. See [Why no ref
stability](#why-no-ref-stability).

**Stack context:** mm CLI v2, Phase 1, **P1-08 + P1-09**, combined in
one PR. Base: [#62](#62)
(`cryptotavares/mmcli-p1-06-observe-level-no-state`); followed by
[#65](#65).

## Problem

A queue timeout rejects the caller, but the task can keep running and
overwrite another request's refs. An observation timeout also leaves
collection running.

Mobile observations read test IDs and the tree from separate snapshots.
A click can return the old screen or `Unlock, busy`.

## Solution

Keep cancellation and commit fencing at separate boundaries:

1. **Cooperative cancellation:** `RequestQueue` passes an `AbortSignal`
and aborts it before timeout rejection. Tools check supported
checkpoints. Observations use a linked controller and their own
deadline.
2. **Commit fencing:** the server owns a small `RefStore` (94 lines).
Each started task gets a generation. Every ref write
(`sessionManager.setRefMap` / `clearRefMap` from tools, and the
observation) checks the generation and request signal, even when a tool
ignores cancellation.

```mermaid
flowchart TD
    A[Queued task starts] --> B[Create signal and ref generation]
    B --> C[Run tool through guarded context]
    C --> D[Optional observation checks linked abort signal]
    C -->|ref write| F{Request current and not aborted?}
    D -->|ref write| F
    F -->|yes| G[Commit map]
    F -->|no| X[Drop write. Keep current refs.]
    T[Queue timeout aborts request] -.-> C
    T -.-> D
    T -.-> F

    classDef new fill:#fff3cd,stroke:#d39e00
    classDef stop fill:#f8d7da,stroke:#c82333
    class B,C,D,F,G,T new
    class X stop
```

Yellow boxes are new. Red boxes stop the ref write.

Tools write refs through `context.sessionManager`, which the daemon
proxies: `setRefMap` and `clearRefMap` go through the request handle.
Other members retain the real receiver, including `#private` fields.
`ISessionManager` stays unchanged.

Optional `captureScreen()` derives both mobile outputs from one
snapshot; drivers without it retain separate reads. After eligible
mutations, retry busy screens and allow one unchanged-screen retry,
compared against the last tree the agent received (`lastScreen`: the
delivered observation, else the `describe_screen` / full
`accessibility_snapshot` tree). Launch, cleanup, and non-batch tools
that set refs skip settling.

## What changes

| Condition or boundary | Before | After |
|---|---|---|
| Queued task times out | Remaining work can continue and replace refs |
Abort checkpoints stop cooperative work; the commit fence drops later
ref writes |
| Observation reaches its own timeout | Collection continues after the
response | Abort collection; the final checkpoint prevents its late ref
write |
| Observation not delivered (`observe none`) | Still became the diff
baseline | Not recorded; the next diff is against what the agent last
saw |
| `list_testids` | Reads the tree and replaces refs despite returning no
refs | Collect only test IDs; leave refs unchanged |
| Mobile discovery and observations | Separate snapshots can disagree or
show an unsettled screen | Share a snapshot when supported; bound
post-mutation settle retries |

Queue timeout errors use HTTP 500 with `TOOL_EXECUTION_FAILED` and `Tool
execution timed out after ${effectiveTimeout}ms`. `MM_REQUEST_ABORTED`
is the internal checkpoint response. Cancellation does not undo an
action already dispatched to a driver. Refresh with `describe-screen`
before retrying.

## Decisions for reviewers

| Decision | Reason |
|---|---|
| Keep abort checkpoints and the commit fence | A driver operation can
finish after cancellation. Ref ownership must remain safe independently.
Synchronous abort-handler settlement can win the queue's timeout race. |
| No ref stability; refs stay positional | See [Why no ref
stability](#why-no-ref-stability). |
| The observation's refMap always wins, also after a tool set refs | The
observation is the last tree in the response, so every ref in it
resolves. |
| `lastScreen` updates only after a delivered response | A timed-out
request's tree is never the settle baseline. |

## Review guide

Read the files in this order:

| # | File | What to check |
|---|---|---|
| 1 | `src/server/request-queue.ts`, `src/types/http.ts` |
Abort-before-reject ordering, bounded settle wait, timeout clamp, and
optional context additions. |
| 2 | `src/server/ref-store.ts` | Generation and abort checks;
`toolSetRefs`. |
| 3 | `src/server/create-server.ts` | Proxy receiver binding
(`setRefMap`, `clearRefMap`), observation cancellation checkpoints,
`screenAfterResponse` / `lastScreen`, batch behavior, separate diff
baseline. |
| 4 | `src/tools/utils.ts`, `src/tools/interaction.ts`,
`src/tools/batch.ts`, `src/tools/discovery-tools.ts` | Checkpoints and
`list_testids` tree-read removal. |
| 5 | `src/platform/types.ts`, `src/platform/mobile-platform-driver.ts`,
`src/server/screen-settle.ts` | Optional capture boundary, shared
snapshot, busy detection, and bounded retries. |
| 6 | Tests beside these modules; `README.md`, `SKILL.md` | Timeout
isolation, matching exceptions, snapshot consistency, and caller
recovery guidance. |

<details>
<summary>Testing</summary>

At `6944868`: `yarn test` 52 files / 1528 tests pass; coverage 97.03 /
93.95 / 95.21 / 97.19; `yarn lint` passes. Every branch above it in the
stack (#65, #66, #67, #44, #68) was restacked and passes `yarn test` +
`yarn lint` at its own tip.

</details>

## Breaking changes

None.
- `ToolContext.signal`, `IPlatformDriver.captureScreen` and
`ScreenCapture` are optional additions.
- `RequestQueue.enqueue` now passes the signal to its callback. Existing
callbacks that take no argument keep working.

## Consumer impact

| Consumer | Change needed | Done on branch |
|---|---|---|
| Extension `test/e2e/playwright/llm-workflow/` | None. It does not use
`RequestQueue`, and its `setRefMap`/`getRefMap` implementation is
unchanged. | n/a |
| Mobile `tests/llm-workflow/` | None (same). | n/a |

## Why no ref stability

The first version of this PR also renumbered refs so an element kept its
number across observations (`stabilizeRefs`, a session high-water mark,
baseline checks, merging tool and observation refs: about 500 lines of
code and 900 of tests). It was removed because the problem it solves is
rare:

- Refs are deterministic per tree: the same screen gives the same refs,
and an unchanged screen diffs as empty (device proof below).
- Since P1-05/P1-06, read commands collect no observation, so they no
longer renumber anything.
- The only case it fixed is an element inserted above the one the agent
holds. The response then carries the new refs (the diff lists the
shifted nodes, or the full tree when every node shifted), and SKILL.md
tells agents to use the refs from the latest response.
- No baseline defect (B1–B15) was a wrong-element click caused by
renumbering.

If real runs show agents clicking stale refs, it can come back as its
own ticket.

## Proof

**After the shrink (`6944868`, run on the restacked stack tip):**

| Check | Extension | iOS | Android |
|---|---|---|---|
| Timed-out click, then `describe-screen` (queueMs 0), then click by ref
| pass | pass | pass |
| `describe-screen` twice: identical refs | 17/17 | 67/67 | 242/242 |
| Unchanged screen, 3 taps: diff | 4,243 B, +0 −0 | 4,860 B, +0 −0 |
5,306 B, +0 −0 |
| Navigate away and back, click by the latest ref | pass | pass | pass |

Android navigation observations can still show the previous screen (the
new screen mounts about 2 s later). That is the existing Android settle
limit noted in #44, not changed here.

**Earlier revision (with ref stability), kept for history:**

Tested with yalc builds in the Extension (e2e build, headless) and
Mobile (dev client on iOS sim and Android emulator). The first full run
used an earlier revision (e804b95). The final commit got a targeted
mobile recheck plus a smoke run on all three platforms. Details are in
the proof file in the mm-cli-v2 workspace.

| Check | Before (#62) | After |
|---|---|---|
| `click --testid does-not-exist --timeout 500`, then `describe-screen`,
then click by ref | – | works on ext, iOS and Android (queueMs 0,
intended element clicked) |
| Android unlock-click observation | login screen ("Unlock, busy") in
3/3 | no login or busy in 4/4 (3 wallet screen, 1 tab bar only) |
| Unchanged-screen diff, iOS (`wait-for wallet-screen`) | 7495 B | 7518
B, diff +0 −0 |
| Unchanged-screen diff, Android (`click tab-bar-item-Wallet`, p50 of 3)
| 7672 B, observationMs 1924 | 40–110 KB, observationMs **965–1001**
(one snapshot instead of two) |
| Ext bench p50 (click / get-state / list-testids totalMs) | 580 / 5 /
565 | 582 / 6 / **60** (`list_testids` no longer reads the tree) |

The Android unchanged-screen diff gap in that revision is closed by #67
and #44 (see the table above).
cryptotavares added a commit that referenced this pull request Oct 29, 2026
…TimeoutMs cap (#71)

> **Summary:** Fix the review comments on #62, #64 and #65 in one PR on
top of the stack. The approved PRs stay unchanged.

**Stack context:** mm CLI v2, Phase 1, **P1-43**. Base:
[#69](#69), the top of
the stack.

## Problem

| Comment | Problem |
|---|---|
| [#65: off-screen
animations](#65 (comment))
| The browser settle ignored animations whose target was outside the
viewport or had zero size. A drawer sliding in, a bottom sheet or an
accordion opening from height 0 start that way, so the observation
caught them mid-move. |
| [#64: late
click](#64 (comment))
| `click` / `type` checked the request signal only before the target
wait. When the request timed out during the wait, the agent got "timed
out", but the click still ran once the target appeared. |
| [#64:
batchTimeoutMs](#64 (comment))
| `batchTimeoutMs` accepted up to 300 s, but the queue and the CLI stop
`run_steps` after 30 s. |
| [#62: refs after
`none`](#62 (comment))
| `--observe none` does not refresh refs; some refs can then click a
different element. Not documented. |
| [#62: reads and
animations](#62 (comment))
| Reads do not wait for animations, even with `--observe full`. Not
documented. |

## Solution

```mermaid
flowchart TD
    A[click / type] --> B[Wait for target]
    B --> C{Request timed out?}
    C -->|yes| D[MM_REQUEST_ABORTED, no action]
    C -->|no| E[Click / tap / type]
    E --> F[Mutating observation]
    F --> G[Wait for every running finite animation, max 1 s]
    G --> H[Read tree]

    classDef new fill:#fff3cd,stroke:#d39e00
    class C,D,G new
```

Yellow boxes are new or changed.

## What changes

| Commit | Change |
|---|---|
| `fix: wait for animations that start off screen or at zero size` |
`animationsSettled` waits for every running, finite animation of an
element. Infinite animations are still ignored; the 1 s cap is
unchanged. |
| `fix: do not click or type after the request timed out` |
`IPlatformDriver.click` / `type` take an optional `signal`. Both drivers
call `signal.throwIfAborted()` after the target wait, right before the
action (mobile `type` also after the focus tap). The tools pass
`context.signal` and return `MM_REQUEST_ABORTED`. |
| `fix!: cap run_steps batchTimeoutMs at 30 s` | Schema range
1000–30000, like the `cdp` `timeoutMs`. |
| `docs: …` | README, SKILL.md and CLI help: refs after `--observe
none`, reads and animations, abort limits, `batchTimeoutMs`. |

## Decisions for reviewers

| Decision | Reason |
|---|---|
| One new PR instead of changing #62/#64/#65 | Pushing to an approved PR
dismisses its approval, and the restack would change every PR above it.
|
| An action that already started is not cancelled |
`@metamask/device-mcp` takes no signal (`waitForElement`, `tapElement`,
`typeText`), and Playwright's actionability checks are part of
`locator.click`. The driver checks the signal between steps. Cancelling
inside device-mcp needs a device-mcp change (follow-up). |
| Cap `batchTimeoutMs` at 30 s instead of raising the request budget for
batches | Matches the CLI and queue default; long flows can be split
into several batches. |
| Reads stay without the animation wait (docs only) | It is a less exact
snapshot, not a wrong action. Can be revisited. |
| The 5 s testId collection cap stays | Agreed with the reviewer: watch
real usage first. |

## Breaking changes

`run_steps` rejects `batchTimeoutMs` above 30000 (validation error),
also when a server raised `requestTimeoutMs`.

## Review guide

| # | File | What to check |
|---|---|---|
| 1 | `src/tools/utils/in-page.ts` | `animationsSettled` without the
viewport/size filter. |
| 2 | `src/platform/playwright-driver.ts`,
`src/platform/mobile-platform-driver.ts`, `src/platform/types.ts` |
`throwIfAborted()` placement. |
| 3 | `src/tools/interaction.ts` | Signal passed; abort mapped to
`MM_REQUEST_ABORTED`. |
| 4 | `src/validation/schemas.ts` | `batchTimeoutMs` range and
description. |
| 5 | `README.md`, `SKILL.md`, `src/cli/mm.ts` | Docs. |

<details>
<summary>Testing</summary>

`yarn test`: 51 files, 1809 tests pass. `yarn lint` passes. Each commit
passes the tests on its own.

**Extension** (e2e fixture wallet, headless), before = #69, after = this
branch:

| Check | Before | After |
|---|---|---|
| Drawer sliding in from off screen (400 ms): observationMs | 515–516
(did not wait) | 914–929 (waited) |
| Accordion from height 0 (400 ms): observationMs | 515–516 | 916–917 |
| Home click observationMs | 513–514 | 512–515 |
| Send / account list click observationMs | 12–183 / 14–164 | 14–212 /
15–164 |
| `run_steps` click whose target appears 1 s after the 30 s queue
timeout | **clicked** | **not clicked** |

**Android** (device-mcp 0.6.0, real funds, benign taps): all smoke and
ref checks pass, same as #69.

**iOS** (device-mcp 0.6.0, real funds, benign taps): all smoke and ref
checks pass, same as #69. (A first attempt hit the stale idb companion
after a simulator reboot, `dtuhidd connection ... invalidated`; fixed by
restarting the companion. See MetaMask/device-mcp#43 and #70.)

</details>
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.

5 participants