Skip to content

fix: use typed model refs for malformed element ids#1944

Open
monadoid wants to merge 5 commits intomainfrom
stg-1555
Open

fix: use typed model refs for malformed element ids#1944
monadoid wants to merge 5 commits intomainfrom
stg-1555

Conversation

@monadoid
Copy link
Copy Markdown
Contributor

@monadoid monadoid commented Apr 1, 2026

Why

We were asking the LLM to return a single elementId: string field, where the model was expected to return an encoded value like "0-76" or "16-21" that packed both the frame ordinal and backend node id into one string. That made it possible for the LLM to return structurally invalid data for that field, like "9786" or empty strings.

Several tickets were seeing this error:
No object generated: response did not match schema
STG-1486, STG-1555, STG-1634, STG-1690

What Changed

This PR fixes this by tightening up the required shape of the data returned by the LLM:
target: { frameOrdinal: number, backendNodeId: number }

This PR does not change anything user facing. We intentionally stop at the handler boundary. I also kept using the old encoded snapshot maps in the snapshot storage for now, even though it's internal code, to keep the PR focused, but we could update this in a future PR to make the codebase more typesafe.

  • Replaced model-generated elementId strings with typed target refs in the internal act/observe schemas.
  • Switched the internal model action shape to a method-specific union.
  • Added a small handler-boundary adapter that encodes typed refs back into the existing snapshot key format for XPath resolution.
  • Kept snapshot capture/storage and public snapshot output unchanged to avoid widening scope or introducing a public-facing break.
  • Added regression coverage for structured act, observe, and dragAndDrop element refs.

Summary by cubic

Switches model-facing element IDs to typed element refs for act/observe and enables strict structured outputs. Fixes malformed ID issues that caused null actions and hangs (STG-1486, STG-1555, STG-1634, STG-1690).

  • Bug Fixes

    • Replaced loose elementId strings with structured refs: frameOrdinal and backendNodeId.
    • Resolved typed refs to existing XPath snapshot keys at the handler boundary, preventing schema mismatches, “not-supported” fallbacks, and correctly handling dragAndDrop destinations.
    • Enforced strict JSON schema for OpenAI responses (OpenAIClient and aisdk) to reject malformed outputs.
  • Refactors

    • Introduced a discriminated union for model actions and a new modelActionResolver; inference.ts now uses modelActionSchema/modelActResponseSchema.
    • Updated act/observe handlers to consume typed actions; kept snapshot capture/storage and public outputs unchanged.
    • Added targeted regression tests for structured act, observe, dragAndDrop, and timeout flows; restored a snapshot payload comment.

Written for commit e526467. Summary will update on new commits. Review in cubic

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 1, 2026

⚠️ No Changeset found

Latest commit: e526467

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@monadoid monadoid marked this pull request as ready for review April 3, 2026 18:27
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 13 files

Confidence score: 4/5

  • This PR looks safe to merge overall, with a focused moderate-risk issue in a test file rather than production runtime code.
  • In packages/core/tests/unit/element-id-regression.test.ts, the vi.mock specifier missing .js may not match the ESM import, which can cause the mock to silently not apply and weaken regression protection.
  • Given severity 5/10 (with high confidence), this is a real correctness concern for test behavior, but it appears limited in scope and not strongly merge-blocking.
  • Pay close attention to packages/core/tests/unit/element-id-regression.test.ts - align mock/import specifiers (including .js) so ESM mocking applies reliably.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/core/tests/unit/element-id-regression.test.ts">

<violation number="1" location="packages/core/tests/unit/element-id-regression.test.ts:10">
P2: The `vi.mock` path is missing the `.js` extension, inconsistent with the actual import on line 7 and the other `vi.mock` on line 15. In ESM mode, mismatched specifiers can cause the mock to silently not apply.

(Based on your team's feedback about requiring explicit .js extensions on all relative ESM import specifiers.) [FEEDBACK_USED]</violation>
</file>
Architecture diagram
sequenceDiagram
    participant Handler as Act/Observe Handler
    participant Inf as Inference Layer
    participant LLM as LLM Client (OpenAI/AISDK)
    participant External as External LLM API
    participant Resolver as NEW: Model Action Resolver
    participant Snap as Snapshot Storage

    Note over Handler,Snap: Runtime Flow for Element Interaction

    Handler->>Snap: captureHybridSnapshot()
    Snap-->>Handler: combinedTree, xpathMap (encoded keys)

    Handler->>Inf: act() / observe()
    
    Note over Inf,LLM: Uses modelActResponseSchema / modelActionSchema

    Inf->>LLM: createChatCompletion(schema)
    
    LLM->>External: POST /completions (Strict JSON Mode)
    Note right of External: CHANGED: Returns structured target<br/>{frameOrdinal, backendNodeId}
    External-->>LLM: JSON Response
    
    LLM-->>Inf: Typed ModelAction
    Inf-->>Handler: Typed ModelAction

    Handler->>Resolver: NEW: resolveModelAction(action, xpathMap)
    
    rect rgb(240, 240, 240)
        Note over Resolver: Internal Mapping Logic
        Resolver->>Resolver: NEW: Encode target ref to "frame-node" string
        Resolver->>Resolver: Lookup XPath in map via encoded string
        
        alt method is dragAndDrop
            Resolver->>Resolver: NEW: Resolve destination ref to XPath
        end
    end

    Resolver-->>Handler: Resolved Action (with XPath selector)

    alt Action valid
        Handler->>Handler: performUnderstudyMethod(selector)
    else Element/XPath not found
        Handler-->>Handler: Handle missing element (Self-heal/Retry)
    end
Loading

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

import { waitForDomNetworkQuiet } from "../../lib/v3/handlers/handlerUtils/actHandlerUtils.js";
import { captureHybridSnapshot } from "../../lib/v3/understudy/a11y/snapshot/index.js";

vi.mock("../../lib/v3/handlers/handlerUtils/actHandlerUtils", () => ({
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Apr 3, 2026

Choose a reason for hiding this comment

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

P2: The vi.mock path is missing the .js extension, inconsistent with the actual import on line 7 and the other vi.mock on line 15. In ESM mode, mismatched specifiers can cause the mock to silently not apply.

(Based on your team's feedback about requiring explicit .js extensions on all relative ESM import specifiers.)

View Feedback

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/tests/unit/element-id-regression.test.ts, line 10:

<comment>The `vi.mock` path is missing the `.js` extension, inconsistent with the actual import on line 7 and the other `vi.mock` on line 15. In ESM mode, mismatched specifiers can cause the mock to silently not apply.

(Based on your team's feedback about requiring explicit .js extensions on all relative ESM import specifiers.) </comment>

<file context>
@@ -0,0 +1,316 @@
+import { waitForDomNetworkQuiet } from "../../lib/v3/handlers/handlerUtils/actHandlerUtils.js";
+import { captureHybridSnapshot } from "../../lib/v3/understudy/a11y/snapshot/index.js";
+
+vi.mock("../../lib/v3/handlers/handlerUtils/actHandlerUtils", () => ({
+  waitForDomNetworkQuiet: vi.fn(),
+  performUnderstudyMethod: vi.fn(),
</file context>
Fix with Cubic

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.

1 participant