Repository navigation
feat: collect observations only for mutating and batch tools - #61
Conversation
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.
9ca4a99 to
396d378
Compare
seaona
left a comment
There was a problem hiding this comment.
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.
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.
|
okay, I've just got the answer in the next PR, so comment above solved 🙏 |
> **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>
> **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>
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
cdpcalls use themutatingcategory. A small result, such asRuntime.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 newYellow boxes are new.
shouldIncludeObservationsInResponsecallsshouldCollectObservationsfirst. A shared batch-policy helper supplies the default, so collection and response inclusion use the same rule.The registry gives
cdpits own public category. CDP method execution stays in the platform driver. A state-changing CDP call needs a subsequentmm describe-screento refresh refs. Per-callobserveopt-in remains a P1-06 follow-up.accessibility_snapshotnow clears the diff baseline, likedescribe_screen,launch, andcleanup. The next collected mutation observation is full and compacted. Other read calls leave the baseline unchanged.What changes
readonlyordiscoverycallobservationMsis zerocdpcallmutating; collects and returns an observation when availablecdp; skips automatic collection and returns noobservationsrun_stepspolicyallby default;noneskips collection;failuresfilters the responsefailuresreturns observations only for a tool error or failed summaryaccessibility_snapshotfollowed by a mutationDecisions for reviewers
cdpto publicToolCategoryfailuresstill collects an observation and advances the baseline, but omits it from the response.Review guide
Read the files in this order:
src/tools/registry.tscdpcategory, tool assignment, and unchanged safe default for unknown tool names.src/server/create-server.tssrc/server/create-server.test.tssrc/tools/registry.test.tscdpclassification and registry/category coverage.README.md,SKILL.mdTesting
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=1passes: 5 files, 181 tests.yarn lintpasses. Production type checking passes withyarn tsc --project tsconfig.build.json --noEmit.yarn tsc --project tsconfig.test.json --noEmitfails 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.ProofandUnit testssections below retain the original historical evidence. These fresh checks do not repeat live-device validation.Breaking changes
readonly,discoveryandcdptools no longer collect observations. As a result:observations(cdpused to include them);cdpcall that changes state (for exampleRuntime.evaluatewith a.click()) must now be followed bydescribe-screento get fresh refs.ToolCategorytype gains'cdp'. Exhaustive switches on it need a new case.Consumer impact
test/e2e/playwright/llm-workflow/observations.tests/llm-workflow/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.cdp Runtime.evaluate 1+1bytes, ext / iOS / Androidget-statetotalMs, ext / iOS / AndroidcdptotalMs, ext / iOS / Androidlist-testidstotalMs, ext / iOS / AndroidobservationMsforget-state,get-text,hermes-targets,list-testids,describe-screen,screenshot,accessibility-snapshot,cdpobservationskey), all platformsclick --testid unlock-submitobservations, ext / iOS / AndroidNotes from the proof run
toolMswent up in the after run (list-testidsp50 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 existinglist-testidstimeout). Totals still fell because observation time is now 0.wait-for,dismiss-keyboard) still hit the 8 s observation timeout and return noobservations. This already happened before the change (P1-04 build: iOSclick-unlock8003 ms).Transcripts (ext, final commit)
Unit tests
New or changed: per-category tests for
shouldCollectObservationsandshouldIncludeObservationsInResponse; endpoint tests forobservationMs(0 for readonly, discovery and cdp; > 0 for mutating); read tools don't move the diff baseline;accessibility_snapshotresets it; the cdp category in the registry.