Skip to content

feat: collect observations only for mutating and batch tools - #61

Merged
cryptotavares merged 2 commits into
cryptotavares/mmcli-p1-04-remove-knowledge-storefrom
cryptotavares/mmcli-p1-05-observe-mutating-only
Oct 29, 2026
Merged

cryptotavares merged 2 commits into
cryptotavares/mmcli-p1-04-remove-knowledge-storefrom
cryptotavares/mmcli-p1-05-observe-mutating-only

Conversation

@cryptotavares

@cryptotavares cryptotavares commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

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 (cryptotavares/mmcli-p1-04-remove-knowledge-store); followed by #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.

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
Loading

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.
Testing
  • 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.

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)
Notes from the proof run
  • 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).
Transcripts (ext, final commit)
$ 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
Unit tests
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.

@cryptotavares
cryptotavares added this pull request to stack #63 October 23, 2026 20:59
@cryptotavares
cryptotavares marked this pull request as ready for review October 26, 2026 23:46
@cryptotavares
cryptotavares requested a review from a team as a code owner October 26, 2026 23:46
Every tool call collected an observation (state, test IDs, a11y tree; on
mobile also two device snapshots and an app-state query), even read-only and
discovery tools whose response never included it. That work only fed the
knowledge store, which is gone.

- shouldCollectObservations: true only for mutating tools and run_steps
  (unless includeObservations is 'none'). shouldIncludeObservationsInResponse
  now builds on it, so the two rules cannot drift.
- New 'cdp' tool category: cdp calls are mostly reads (Runtime.evaluate),
  so they no longer collect or return an observation.
- accessibility_snapshot now resets the diff baseline, like describe_screen
  (both return the full tree).

BREAKING CHANGE: readonly, discovery and cdp tools no longer collect
observations. They no longer return `observations` (cdp did before), no
longer refresh the session refMap through an observation, and no longer
move the diff baseline for the next mutating call. A cdp call that changes
state (e.g. Runtime.evaluate with a click) must be followed by
describe_screen to get fresh refs. The public ToolCategory type gains
'cdp', so exhaustive switches on it need a new case.
@cryptotavares
cryptotavares force-pushed the cryptotavares/mmcli-p1-05-observe-mutating-only branch from 9ca4a99 to 396d378 Compare October 28, 2026 11:28
@cryptotavares cryptotavares changed the title feat!: collect observations only for mutating and batch tools feat: collect observations only for mutating and batch tools 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.

When a 'failures' batch succeeds (return snapshot only when something went wrong), we still collect an observation. We don't return it, but it becomes the new diff baseline (lastObservation). So the next mutating call returns a diff against a screen the agent never saw.

Was this on purpose, for example to keep the ref map fresh after the batch? If not, could we skip collection when the batch succeeds and reset lastObservation? The next mutation would then return a full observation, and we'd save the observation cost on successful batches.

To further elaborate. Diagram 1: shows what happens today
The agent last saw the Send screen (A). The batch moves the app to Confirm (B). The daemon captures B silently and makes it the baseline. The next click diffs against B, so the agent gets unchanged: 42 for nodes it never received and has to call describe-screen again.

Image

Diagram 2: possible alternatives

  • Skip and reset: no observation after a successful batch, so the next mutation returns the full screen. This is the fastest option.
  • Collect but keep the old baseline: the diff is relative to what the agent last saw. It's consistent, but still pays for the observation.
Image

@seaona

seaona commented Oct 29, 2026

Copy link
Copy Markdown
Member

okay, I've just got the answer in the next PR, so comment above solved 🙏
#62

@cryptotavares
cryptotavares merged commit 98b8dba into main Oct 29, 2026
27 checks passed
@cryptotavares
cryptotavares deleted the cryptotavares/mmcli-p1-05-observe-mutating-only branch October 29, 2026 13:52
cryptotavares added a commit that referenced this pull request Oct 29, 2026
> **Summary:** Remove the knowledge store and its per-tool step
recording. Keep session metadata as an explicit consumer write, separate
from tool execution and observation collection.

**Stack context:** mm CLI v2, Phase 1, **P1-04**, decision **D6**. Base:
[#59](#59); next:
[#61](#61). Review this
delta against #59.

## Problem

The shared HTTP executor awaits a knowledge-store write after each tool
execution with a session ID. It also records execution failures. The
store persists tool inputs, outcomes, observations, and artifact
references under `test-artifacts/llm-knowledge/<sessionId>/steps/`.

Four `knowledge_*` tools read that history. Decision **D6** removes this
subsystem from the core. Session metadata and HTTP observations still
need independent homes after the store disappears.

## Solution

Delete the store, its recording hooks, retrieval tools, schemas, CLI
commands, and public exports. Remove `knowledgeStore` from
`ServerConfig` and `ToolContext`, so tool execution no longer depends on
a history service.

Keep two boundaries explicit:

- The daemon collects observations and builds responses through the
existing execution path.
- Consumers call the exported `writeSessionMetadata(artifactsDir,
metadata)` when they persist launch metadata.

```mermaid
flowchart TD
    A[Tool request] --> B[Validate and run tool through queue]
    B --> C[Collect observations under existing policy]
    C --> D[Build timed HTTP response]
    E[Consumer launch metadata] --> F[Validate session ID in writeSessionMetadata]
    F -->|valid| G[Write session.json beneath artifactsDir]
    G --> I[Return absolute file path]
    F -->|invalid| H[Reject; no metadata write]

    classDef new fill:#fff3cd,stroke:#d39e00
    classDef stop fill:#f8d7da,stroke:#c82333
    class F,G,I new
    class H stop
```

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

The metadata writer creates missing directories, overwrites
`session.json`, and writes formatted JSON with a trailing newline.
Relative artifact directories resolve against the process working
directory. Invalid session IDs reject with `Invalid session id: "<id>"`
before directory creation.

Move `StepRecordObservation` and `createDefaultObservation` to
`src/tools/types/observation.ts`. Move `SessionMetadata` to
`src/tools/types/session.ts`, without `goal` or `flowTags`. Keep
free-form `tags`.

## What changes

| Condition or boundary | Before | After |
|---|---|---|
| Tool execution with a session ID | Awaits a step-record write after
execution | No knowledge-store recording on success or execution failure
|
| Tool registry and schemas | Include four `knowledge_*` tools | Remove
those tools; registry has 43 entries |
| `mm knowledge-*` | Routes to history tools | Unknown command; exit 1 |
| Launch `--goal` or `--flow-tags` | Adds knowledge metadata | Warns
about the unknown flag; omits its field |
| Persist session metadata | Store writes under `llm-knowledge/<id>/` |
Consumer calls the writer for `<artifactsDir>/<id>/session.json` |
| Observation collection and response policy | Collects beyond tools
that return observations | Same policy; later stack work limits
collection |

The CLI uses its existing unknown-command message:

```text
Error: unknown command 'knowledge-last'. Run 'mm --help' for usage.
```

Removed launch flags use the existing warning path, such as `Warning:
unknown launch flag '--goal'`. The warning alone does not reject launch.
The protected sections below list all breaking exports and consumer
migration details.

## Decisions for reviewers

| Decision | Reason |
|---|---|
| Remove the subsystem under **D6 / P1-04** | A compatibility wrapper
would retain the history dependency and its public surface. Consumers
migrate explicitly. |
| Let consumers persist session metadata | The writer accepts their
artifact directory without constructing a store or running inside each
tool request. |
| Keep observation behavior in this PR | Read-only and discovery
observations still feed the next response's diff. Changing collection
belongs to later stack work. |
| Keep `StepRecordObservation` and `createDefaultObservation` names |
Their implementations remain useful outside persistence. Moving them
avoids an additional public rename. |

## Review guide

Read the files in this order:

| # | File | What to check |
|---|---|---|
| 1 | `src/server/create-server.ts`, `src/types/http.ts` | Recording
disappears from success and failure paths; observations and timing
remain. |
| 2 | `src/server/session-metadata.ts`, `src/tools/types/session.ts` |
Consumer-owned persistence, path validation, and retained metadata
fields. |
| 3 | `src/tools/types/observation.ts`, `src/index.ts`,
`src/tools/types/index.ts` | Moved observation contracts and public
exports. |
| 4 | `src/tools/registry.ts`, `src/validation/schemas.ts`,
`src/cli/mm.ts` | Removed tools, launch fields, commands, and help text.
|
| 5 | `src/server/session-metadata.test.ts`,
`src/tools/types/observation.test.ts`, CLI/server/registry tests |
Metadata writes and rejection cases, moved defaults, command removal,
and retained observation policy. |
| 6 | `README.md`, `SKILL.md` | Updated architecture, metadata path,
command inventory, and launch options. |

<details>
<summary>Testing</summary>

- Fresh check at remote head `2f10dd5ac47198f9259f24de061ee2b0bd0209a8`:
`CI=true yarn vitest run --maxWorkers=2 --minWorkers=1
--coverage.enabled=false` passes: 46 files, 1,397 tests.
- `yarn lint:eslint`, `yarn prettier README.md SKILL.md --check`, and
`yarn tsc --noEmit --project tsconfig.build.json` pass.
- Standalone `yarn tsc --noEmit --project tsconfig.test.json` fails on
existing mobile-driver, network-mock, and screenshot test typing issues.
Vitest reports no type errors, so its result does not replace this
standalone check.
- Metadata tests cover the destination path, missing directories,
overwrite, and unsafe session IDs. CLI tests cover removed commands and
launch flags.
- The `Proof` and `Unit tests` sections below retain the original
historical evidence. These fresh checks do not repeat live consumer
validation.

</details>

## Breaking changes

- **Removed public exports:**
- Store: `KnowledgeStore`, `KnowledgeStoreConfig`,
`createKnowledgeStore`, `setKnowledgeStore`, `getKnowledgeStore`,
`hasKnowledgeStore`, `knowledgeStore`
  - Tokenization: `tokenize`, `tokenizeIdentifier`, `expandWithSynonyms`
- Tools and schemas: the `knowledge*Tool` functions,
`knowledge*InputSchema`, `knowledgeScopeSchema`,
`knowledgeFiltersSchema`, `Knowledge*Z`
- Constants and types: `FLOW_TAGS`, `FlowTag`, `STEP_LABELS`,
`StepLabel`, the `StepRecord*` types except `StepRecordObservation`, the
`PriorKnowledge*`, `Knowledge*`, `RecipeStep` and `SessionSummary` types
- Other: `ErrorCodes.MM_KNOWLEDGE_ERROR`, `extractTargetFromInput`,
`extractScreenshotInfo`, `extractToolOutcome`
- `ToolName` no longer includes `knowledge_*`.
- `ServerConfig.knowledgeStore` and `ToolContext.knowledgeStore` are
removed.
- Launch input no longer accepts `goal`/`flowTags`, and
`SessionMetadata` no longer has them. `mm launch --goal` now gives the
unknown-flag warning.
- `mm knowledge-*` commands are removed (unknown command, exit 1).
- **Path change:** session metadata moves from
`test-artifacts/llm-knowledge/<id>/session.json` to
`<artifactsDir>/<id>/session.json` (`test-artifacts/<id>/session.json`
with the consumers' `artifactsDir: 'test-artifacts'`). Consumers call
`writeSessionMetadata()` themselves.

## Consumer impact

| Consumer | Change needed | Done on branch |
|---|---|---|
| Extension `test/e2e/playwright/llm-workflow/` | Drop
`KnowledgeStore`/`setKnowledgeStore` from `daemon.ts`;
`metamask-provider.ts` calls `writeSessionMetadata(artifactsDir, …)`; no
`goal`/`flowTags`; tests; README, AGENTS.md |
`cryptotavares/mm-cli-v2-extension` 45c821d168a |
| Mobile `tests/llm-workflow/` | Same (write stays non-blocking) |
`cryptotavares/mm-cli-v2-mobile` 4e2661f209f |

## Proof

Tested with a yalc build of this branch (`0.12.0+f2caa574`) in the
Extension (e2e test build 13.56.0, headless) and Mobile (dev client
8.19.0, iOS sim + Android emulator), with the consumer commits above.

| Check | Before (#59) | After |
|---|---|---|
| Files written under `test-artifacts/llm-knowledge/` during a session |
one step file per tool call | **none** (ext 116 → 116 files, mobile 260
→ 260) |
| `session.json` per launch | `llm-knowledge/<id>/session.json` |
`test-artifacts/<id>/session.json` (ext ×2, iOS, Android) |
| `mm knowledge-last` | step list | `Error: unknown command
'knowledge-last'`, exit 1 |
| `mm launch --goal x` | accepted | `Warning: unknown launch flag
'--goal'`, launch ok |
| Smoke (ext / iOS / Android) | pass | pass / pass / pass (Android
`list-testids` 30 s timeout = known B11) |
| `rg -i knowledge src` | many | only the tests asserting the commands
are gone |
| Coverage (stmts / branch / funcs / lines) | 96.38 / 91.7 / 94.63 /
96.62 | 96.92 / 93.38 / 94.84 / 97.1 |

<details>
<summary>Transcripts</summary>

```
$ yarn mm knowledge-last
Error: unknown command 'knowledge-last'
exit=1

$ yarn mm launch --context e2e --state default --goal x
Warning: unknown launch flag '--goal'
... (launch ok, exit 0)

$ find test-artifacts -newer <marker> -type f      # Extension
test-artifacts/mm-mvl3vmad-zd909k/session.json
test-artifacts/mm-mvl3whgy-3sm2ye/session.json
test-artifacts/screenshots/p1-04-ext-home-1792768182733.png

$ cat test-artifacts/mm-mvl3vmad-zd909k/session.json
{ "schemaVersion": 1, "sessionId": "mm-mvl3vmad-zd909k", "createdAt": "...", "tags": [], "launch": { "stateMode": "default", ... } }
```

</details>

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

```
core:       Test Files 46 passed (46) · Tests 1397 passed (1397) · Type Errors no errors
            All files | 96.92 | 93.38 | 94.84 | 97.1
Extension:  yarn jest test/e2e/playwright/llm-workflow → 12 suites, 144 tests passed
Mobile:     yarn test:unit:llm-workflow → 18 suites, 231 tests passed
```

</details>
cryptotavares added a commit that referenced this pull request Oct 29, 2026
> **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](#61) first.
Followed by [#64](#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.

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

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:

```text
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. |

<details>
<summary>Testing</summary>

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

</details>

## 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.

<details>
<summary>Transcripts (ext)</summary>

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

</details>

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

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

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

3 participants