Skip to content

fix(schema): Preserve video fps constraints

8206d7b
Select commit
Loading
Failed to load commit list.
Sign in for the full log view
Merged

feat(ui-automation): Add rs/1 runtime automation parity #416

fix(schema): Preserve video fps constraints
8206d7b
Select commit
Loading
Failed to load commit list.
GitHub Actions / warden: code-review completed May 19, 2026 in 41m 18s

6 issues

code-review: Found 6 issues (4 medium, 2 low)

Medium

Post-action snapshot failure misreports a successful batch as failed - `src/mcp/tools/ui-automation/batch.ts:186-209`

If captureRuntimeSnapshotAfterAction throws (e.g., describe-ui transiently fails), the catch block returns createUiActionFailureResult even though the batch steps already executed successfully. When preserveSnapshot is false, clearRuntimeSnapshot has already run at that point, so the caller loses both the old snapshot and receives a misleading failure result for an action that actually completed.

Also found at:

  • src/mcp/tools/ui-automation/batch.ts:211-217
  • src/mcp/tools/ui-automation/shared/post-action-snapshot.ts:8-25
  • src/mcp/tools/ui-automation/swipe.ts:143-148
  • src/mcp/tools/ui-automation/tap.ts:123-130
  • src/mcp/tools/ui-automation/type_text.ts:157-169
`batchElements` is always empty, making batch next-step guidance dead code - `src/mcp/tools/ui-automation/shared/runtime-next-steps.ts:349`

In createRuntimeSnapshotNextSteps, batchElements is declared as const batchElements: typeof tapElements = [] (line 349) and never populated. As a result, batchElements.length >= 2 is always false, so the 'Batch same-screen taps' next-step branch (lines 391–402) is unreachable. The JSDoc explicitly lists batch as a supported guidance type and the PR introduces batch flow handling, so this looks like an incomplete implementation rather than intentional. Secondary effects: !batchElements.length in shouldPrioritizeScroll is always true (currently a no-op), and the batchElements.length >= 2 clause in hasUsefulRuntimeGuidance never contributes. Net impact is a silently missing feature, not a runtime error — hence medium severity. Consider either populating batchElements by selecting additional same-screen tap candidates from tapElements, or removing the dead branches until the batch heuristic is ready.

Also found at:

  • src/mcp/tools/ui-automation/snapshot_ui.ts:143-149
O(n²) construction of `descendantsByRoot` map may be slow on large UI trees - `src/mcp/tools/ui-automation/shared/runtime-next-steps.ts:237`

For every record in records, records.filter(...) scans all n records, producing an O(n²) build step; for deep AX snapshots with hundreds of elements this can become noticeably slow on each UI action.

Also found at:

  • src/mcp/tools/ui-automation/shared/runtime-snapshot.ts:419-428
  • src/mcp/tools/ui-automation/shared/runtime-next-steps.ts:155-162
`/tab/` regex incorrectly matches "table", misclassifying list elements as tabs - `src/mcp/tools/ui-automation/shared/runtime-snapshot.ts:123`

The pattern /tab/.test(roleText) matches any string containing the substring "tab", so accessibility nodes with roles like "AXTable" (normalized to "axtable") will return 'tab' instead of 'list', causing them to gain a 'tap' action and lose the 'swipeWithin' action that deriveActions grants to 'list' and 'scroll-view' roles.

Low

Three error paths in `resolveElementSelector` have no test coverage - `src/mcp/tools/ui-automation/shared/wait-predicate.ts:131-174`

The resolveElementSelector function returns SNAPSHOT_MISSING, SNAPSHOT_EXPIRED, and ELEMENT_REF_NOT_FOUND errors, but none of these paths are exercised in wait_for_ui.test.ts — only TARGET_NOT_FOUND is tested. Consider adding unit tests that call wait_for_ui with an elementRef when no snapshot exists, when the snapshot is expired, and when the elementRef is absent from the snapshot.

Swipe MCP success test removed without replacement - `src/snapshot-tests/suites/ui-automation-suite.ts:124`

In src/snapshot-tests/suites/ui-automation-suite.ts, the prior swipe it('success', ...) MCP snapshot test was replaced with it('error - target not actionable', ...). The MCP suite no longer exercises a successful swipe; only two error cases remain (swipe--error-not-actionable, swipe--error-no-simulator). A swipe--success fixture still exists under __fixtures__/cli/json/ui-automation/, but no test references swipe--success anywhere in src/snapshot-tests (verified by grep). Given that swipe is a primary UI-automation action being expanded in this PR (post-action snapshot capture, next-step generation), consider adding a success-path test using an element ref that supports swipeWithin (e.g., a scrollable element in the test app), or document why the happy path is not snapshot-tested. This is a test-coverage gap, not a runtime defect.


⏱ 39m 10s · 3.3M in / 165.1k out · $6.91

Annotations

Check warning on line 209 in src/mcp/tools/ui-automation/batch.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

Post-action snapshot failure misreports a successful batch as failed

If `captureRuntimeSnapshotAfterAction` throws (e.g., `describe-ui` transiently fails), the catch block returns `createUiActionFailureResult` even though the batch steps already executed successfully. When `preserveSnapshot` is false, `clearRuntimeSnapshot` has already run at that point, so the caller loses both the old snapshot and receives a misleading failure result for an action that actually completed.

Check warning on line 217 in src/mcp/tools/ui-automation/batch.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[9Q7-KEB] Post-action snapshot failure misreports a successful batch as failed (additional location)

If `captureRuntimeSnapshotAfterAction` throws (e.g., `describe-ui` transiently fails), the catch block returns `createUiActionFailureResult` even though the batch steps already executed successfully. When `preserveSnapshot` is false, `clearRuntimeSnapshot` has already run at that point, so the caller loses both the old snapshot and receives a misleading failure result for an action that actually completed.

Check warning on line 25 in src/mcp/tools/ui-automation/shared/post-action-snapshot.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[9Q7-KEB] Post-action snapshot failure misreports a successful batch as failed (additional location)

If `captureRuntimeSnapshotAfterAction` throws (e.g., `describe-ui` transiently fails), the catch block returns `createUiActionFailureResult` even though the batch steps already executed successfully. When `preserveSnapshot` is false, `clearRuntimeSnapshot` has already run at that point, so the caller loses both the old snapshot and receives a misleading failure result for an action that actually completed.

Check warning on line 148 in src/mcp/tools/ui-automation/swipe.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[9Q7-KEB] Post-action snapshot failure misreports a successful batch as failed (additional location)

If `captureRuntimeSnapshotAfterAction` throws (e.g., `describe-ui` transiently fails), the catch block returns `createUiActionFailureResult` even though the batch steps already executed successfully. When `preserveSnapshot` is false, `clearRuntimeSnapshot` has already run at that point, so the caller loses both the old snapshot and receives a misleading failure result for an action that actually completed.

Check warning on line 130 in src/mcp/tools/ui-automation/tap.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[9Q7-KEB] Post-action snapshot failure misreports a successful batch as failed (additional location)

If `captureRuntimeSnapshotAfterAction` throws (e.g., `describe-ui` transiently fails), the catch block returns `createUiActionFailureResult` even though the batch steps already executed successfully. When `preserveSnapshot` is false, `clearRuntimeSnapshot` has already run at that point, so the caller loses both the old snapshot and receives a misleading failure result for an action that actually completed.

Check warning on line 169 in src/mcp/tools/ui-automation/type_text.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[9Q7-KEB] Post-action snapshot failure misreports a successful batch as failed (additional location)

If `captureRuntimeSnapshotAfterAction` throws (e.g., `describe-ui` transiently fails), the catch block returns `createUiActionFailureResult` even though the batch steps already executed successfully. When `preserveSnapshot` is false, `clearRuntimeSnapshot` has already run at that point, so the caller loses both the old snapshot and receives a misleading failure result for an action that actually completed.

Check warning on line 349 in src/mcp/tools/ui-automation/shared/runtime-next-steps.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

`batchElements` is always empty, making batch next-step guidance dead code

In `createRuntimeSnapshotNextSteps`, `batchElements` is declared as `const batchElements: typeof tapElements = []` (line 349) and never populated. As a result, `batchElements.length >= 2` is always false, so the 'Batch same-screen taps' next-step branch (lines 391–402) is unreachable. The JSDoc explicitly lists batch as a supported guidance type and the PR introduces batch flow handling, so this looks like an incomplete implementation rather than intentional. Secondary effects: `!batchElements.length` in `shouldPrioritizeScroll` is always true (currently a no-op), and the `batchElements.length >= 2` clause in `hasUsefulRuntimeGuidance` never contributes. Net impact is a silently missing feature, not a runtime error — hence medium severity. Consider either populating `batchElements` by selecting additional same-screen tap candidates from `tapElements`, or removing the dead branches until the batch heuristic is ready.

Check warning on line 149 in src/mcp/tools/ui-automation/snapshot_ui.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[MWL-XV8] `batchElements` is always empty, making batch next-step guidance dead code (additional location)

In `createRuntimeSnapshotNextSteps`, `batchElements` is declared as `const batchElements: typeof tapElements = []` (line 349) and never populated. As a result, `batchElements.length >= 2` is always false, so the 'Batch same-screen taps' next-step branch (lines 391–402) is unreachable. The JSDoc explicitly lists batch as a supported guidance type and the PR introduces batch flow handling, so this looks like an incomplete implementation rather than intentional. Secondary effects: `!batchElements.length` in `shouldPrioritizeScroll` is always true (currently a no-op), and the `batchElements.length >= 2` clause in `hasUsefulRuntimeGuidance` never contributes. Net impact is a silently missing feature, not a runtime error — hence medium severity. Consider either populating `batchElements` by selecting additional same-screen tap candidates from `tapElements`, or removing the dead branches until the batch heuristic is ready.

Check warning on line 237 in src/mcp/tools/ui-automation/shared/runtime-next-steps.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

O(n²) construction of `descendantsByRoot` map may be slow on large UI trees

For every record in `records`, `records.filter(...)` scans all n records, producing an O(n²) build step; for deep AX snapshots with hundreds of elements this can become noticeably slow on each UI action.

Check warning on line 428 in src/mcp/tools/ui-automation/shared/runtime-snapshot.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[3YG-XCZ] O(n²) construction of `descendantsByRoot` map may be slow on large UI trees (additional location)

For every record in `records`, `records.filter(...)` scans all n records, producing an O(n²) build step; for deep AX snapshots with hundreds of elements this can become noticeably slow on each UI action.

Check warning on line 162 in src/mcp/tools/ui-automation/shared/runtime-next-steps.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

[3YG-XCZ] O(n²) construction of `descendantsByRoot` map may be slow on large UI trees (additional location)

For every record in `records`, `records.filter(...)` scans all n records, producing an O(n²) build step; for deep AX snapshots with hundreds of elements this can become noticeably slow on each UI action.

Check warning on line 123 in src/mcp/tools/ui-automation/shared/runtime-snapshot.ts

See this annotation in the file changed.

@github-actions github-actions / warden: code-review

`/tab/` regex incorrectly matches "table", misclassifying list elements as tabs

The pattern `/tab/.test(roleText)` matches any string containing the substring "tab", so accessibility nodes with roles like "AXTable" (normalized to "axtable") will return `'tab'` instead of `'list'`, causing them to gain a `'tap'` action and lose the `'swipeWithin'` action that `deriveActions` grants to `'list'` and `'scroll-view'` roles.