fix(lens): group variants by the element label, not the matcher

packages/skills/skills/argent-lens/SKILL.md

@@ -18,7 +18,7 @@ You implement several candidate designs, capture each one running on the device,
 | `propose_variant`      | No        | Stage ONE variant for ONE element. Call once per variant. Keep working. |
 | `await_user_selection` | Yes       | Call ONCE after every variant is staged. Parks until the human is done. |
 
-`propose_variant` params: `element` (human name), optional `match` (`{ by: "text"|"label"|"identifier"|"role", value }`), optional `udid` (the device id you captured the variants on), and `variant` (`{ name, summary, code?, filePath?, previewImage?, frame? }`). Repeated calls with the same `element` accumulate variants on that element; different `element` values create separate cards.
+`propose_variant` params: `element` (human name), optional `match` (`{ by: "text"|"label"|"identifier"|"role", value }`), optional `udid` (the device id you captured the variants on), and `variant` (`{ name, summary, code?, filePath?, previewImage?, frame? }`). Repeated calls with the same `element` accumulate variants on that element; different `element` values create separate cards. The `element` label is the identity — matching ignores case and surrounding whitespace, and `match` never affects the grouping, so give genuinely different elements different labels. `match` is a locator only: the first one you supply for an element sticks, and a later conflicting one is reported back in the response rather than silently applied.
 
 **Always pass `udid`** (the same simulator/emulator id you screenshotted and described with). The preview window then streams _that_ device directly — the human never has to pick a simulator. Set it on the first `propose_variant` of a round; later calls may omit it (the last value wins).
 

packages/tool-server/src/tools/variants/propose-variant.ts

@@ -10,8 +10,10 @@ const zodSchema = z.object({
     .max(200)
     .describe(
       'Human name of the on-screen element this variant targets, e.g. "Foo button" or ' +
-        '"profile header". Repeated calls with the same element accumulate multiple variants ' +
-        "on it. Used as the default screen matcher when `match` is omitted."
+        '"profile header". This label IS the element\'s identity: repeated calls with the same ' +
+        "label (ignoring case and surrounding whitespace) accumulate variants on one picker card, " +
+        "whatever `match` they pass. Give genuinely different elements different labels. Also used " +
+        "as the default screen matcher when `match` is omitted."
     ),
   udid: z
     .string()
@@ -38,8 +40,11 @@ const zodSchema = z.object({
     .optional()
     .describe(
       "Optional precise matcher so the floating variant bubble anchors to the right element on " +
-        "the streamed screen. Defaults to { by: 'text', value: element }. Get exact " +
-        "labels/identifiers from the `describe` tool first for reliable anchoring."
+        "the streamed screen. Defaults to { by: 'text', value: element }. This is a locator only — " +
+        "it does not affect which element variants group under. A matcher you supply replaces the " +
+        "label-derived default, but if you then supply a different one for the same label the " +
+        "first is kept and the response reports what was ignored. Get exact labels/identifiers " +
+        "from the `describe` tool first for reliable anchoring."
     ),
   variant: z
     .object({
@@ -149,14 +154,23 @@ it does not wait for the user.`,
       const finish = variantProposalStore.isCliSession()
         ? "end your turn — the user's feedback will arrive as a message"
         : "call await_user_selection once";
+      // A dropped matcher has to be said out loud: the agent asked to locate
+      // the element one way and the card will use another, and the likeliest
+      // cause is that two different elements were given the same label.
+      const ignored = res.matchIgnored
+        ? ` Kept the matcher this element already had (${res.matchApplied.by}=${res.matchApplied.value}) ` +
+          `and ignored ${res.matchIgnored.by}=${res.matchIgnored.value} — variants group by the ` +
+          `\`element\` label, so give a different label if these are different elements.`
+        : "";
+
       return {
         ...res,
         hint:
-          res.variantCount === 1
+          (res.variantCount === 1
             ? `Staged the first variant for "${res.element}". Propose more variants (for this or ` +
               `other elements), then ${finish} when done.`
             : `"${res.element}" now has ${res.variantCount} variants. Keep proposing, then ` +
-              `${finish} when every element is covered.`,
+              `${finish} when every element is covered.`) + ignored,
       };
     },
   };

packages/tool-server/src/utils/variant-proposals.ts

@@ -69,6 +69,12 @@ export interface ElementProposal {
   /** Human-facing name the agent used, e.g. "Foo button". */
   element: string;
   match: VariantMatch;
+  /**
+   * True when the agent supplied `match` itself. A synthesized default is a
+   * placeholder, so a later explicit matcher may replace it; one the agent
+   * chose is never overwritten.
+   */
+  matchExplicit: boolean;
   variants: Variant[];
   createdAt: number;
 }
@@ -308,6 +314,16 @@ const MAX_PENDING_OUTCOMES = 32;
 const MAX_MATCH_VALUE_LENGTH = 200;
 const MAX_ANNOTATIONS = 200;
 
+/**
+ * Identity key for an element. `element` is the human-facing label AND the
+ * identity — repeated calls with the same label accumulate on one card — so it
+ * is compared case- and whitespace-insensitively, matching how `slug` folds the
+ * same string into an id.
+ */
+function elementKey(element: string): string {
+  return element.trim().toLowerCase().replace(/\s+/g, " ");
+}
+
 function slug(s: string): string {
   return s
     .toLowerCase()
@@ -362,7 +378,6 @@ export class VariantProposalStore {
   private ownedDevices = new Set<string>();
   private submitted: SubmittedSelection[] = [];
   private submittedAnnotations: ElementAnnotation[] = [];
-  private variantSeq = 0;
   /** Parked await_user_selection calls. */
   private waitersList: Waiter[] = [];
   /** Frozen result of the current round once the user submits. */
@@ -431,7 +446,6 @@ export class VariantProposalStore {
     this.globalComment = "";
     this.submitted = [];
     this.submittedAnnotations = [];
-    this.variantSeq = 0;
     this.lastOutcome = null;
     this.events.emit("changed");
   }
@@ -526,32 +540,55 @@ export class VariantProposalStore {
     element: string;
     variantCount: number;
     totalElements: number;
+    matchApplied: VariantMatch;
+    matchIgnored?: VariantMatch;
   } {
     this.autoRollIfCompleted();
 
     // Remember which device these variants are for, so the window streams it
     // directly. Last non-empty value wins; usually set once on the first call.
     if (input.udid && input.udid.trim()) this.device = input.udid.trim();
 
-    const match: VariantMatch = input.match ?? { by: "text", value: input.element };
-    const key = `${match.by}:${match.value.trim().toLowerCase()}`;
+    // Identity is the element label, not the matcher. Keying on the matcher
+    // split one element into two cards whenever `match` was supplied, and — the
+    // other way round — filed variants under an unrelated element whenever two
+    // labels happened to share a matcher value.
+    const key = elementKey(input.element);
+    const explicitMatch = input.match;
+    const match: VariantMatch = explicitMatch ?? { by: "text", value: input.element };
+
+    let proposal = this.proposals.find((p) => elementKey(p.element) === key);
+    let matchIgnored: VariantMatch | undefined;
 
-    let proposal = this.proposals.find(
-      (p) => `${p.match.by}:${p.match.value.trim().toLowerCase()}` === key
-    );
     if (!proposal) {
       proposal = {
         id: `el-${slug(input.element) || "element"}-${this.proposals.length + 1}`,
         element: input.element,
         match,
+        matchExplicit: Boolean(explicitMatch),
         variants: [],
         createdAt: Date.now(),
       };
       this.proposals.push(proposal);
+    } else if (explicitMatch) {
+      if (!proposal.matchExplicit) {
+        // The stored locator was synthesized from the label; a real one the
+        // agent supplied is strictly better, so take it.
+        proposal.match = explicitMatch;
+        proposal.matchExplicit = true;
+      } else if (
+        proposal.match.by !== explicitMatch.by ||
+        proposal.match.value !== explicitMatch.value
+      ) {
+        // Two different explicit locators for one label is ambiguous — the
+        // agent may have meant two different elements. Keep the first (the UI
+        // has already anchored on it) and tell the caller what was dropped.
+        matchIgnored = explicitMatch;
+      }
     }
 
     const variant: Variant = {
-      id: `v${++this.variantSeq}`,
+      id: `v${proposal.variants.length + 1}`,
       name: input.variant.name,
       summary: input.variant.summary,
       code: input.variant.code,
@@ -570,6 +607,8 @@ export class VariantProposalStore {
       element: proposal.element,
       variantCount: proposal.variants.length,
       totalElements: this.proposals.length,
+      matchApplied: proposal.match,
+      ...(matchIgnored ? { matchIgnored } : {}),
     };
   }
 

packages/tool-server/test/variant-proposals.test.ts

@@ -977,3 +977,99 @@ describe("VariantProposalStore — roundAbandoned telemetry event", () => {
     expect(stats).toHaveLength(1);
   });
 });
+
+describe("VariantProposalStore — element is the identity (issue #624)", () => {
+  it("keeps variants on one element when a later call supplies a matcher", () => {
+    // The reported bug: identity was keyed on the matcher, so following the
+    // docs' advice to pass `match` forked a second picker card and the human
+    // was asked to choose between halves of one set.
+    const s = new VariantProposalStore();
+    const r1 = s.proposeVariant({ element: "QA probe button", variant: variant("A") });
+    const r2 = s.proposeVariant({
+      element: "QA probe button",
+      match: { by: "text", value: "SUBMIT" },
+      variant: variant("B"),
+    });
+
+    expect(r2.elementId).toBe(r1.elementId);
+    expect(r2.variantCount).toBe(2);
+    expect(r2.totalElements).toBe(1);
+  });
+
+  it("does not file one element's variants under another that shares a matcher", () => {
+    // The unreported direction, and the more damaging one: the key was
+    // symmetric, so two different labels sharing a matcher value merged — the
+    // response even came back naming the other element.
+    const s = new VariantProposalStore();
+    const shared = { by: "text" as const, value: "SHARED" };
+    const r1 = s.proposeVariant({ element: "Header logo", match: shared, variant: variant("A") });
+    const r2 = s.proposeVariant({ element: "Footer link", match: shared, variant: variant("B") });
+
+    expect(r2.elementId).not.toBe(r1.elementId);
+    expect(r1.element).toBe("Header logo");
+    expect(r2.element).toBe("Footer link");
+    expect(r2.totalElements).toBe(2);
+  });
+
+  it("treats the label case- and whitespace-insensitively, as the id slug does", () => {
+    const s = new VariantProposalStore();
+    const r1 = s.proposeVariant({ element: "Foo button", variant: variant("A") });
+    const r2 = s.proposeVariant({ element: "  foo   BUTTON ", variant: variant("B") });
+
+    expect(r2.elementId).toBe(r1.elementId);
+    expect(r2.variantCount).toBe(2);
+  });
+
+  it("upgrades a label-derived matcher to a real one the agent supplies", () => {
+    // The default matcher is synthesized from the label, so it is a placeholder
+    // — replacing it with something the agent actually chose is an improvement.
+    const s = new VariantProposalStore();
+    s.proposeVariant({ element: "Search field", variant: variant("A") });
+    const r2 = s.proposeVariant({
+      element: "Search field",
+      match: { by: "identifier", value: "search-input" },
+      variant: variant("B"),
+    });
+
+    expect(r2.matchApplied).toEqual({ by: "identifier", value: "search-input" });
+    expect(r2.matchIgnored).toBeUndefined();
+    expect(s.snapshot().proposals[0]!.match).toEqual({
+      by: "identifier",
+      value: "search-input",
+    });
+  });
+
+  it("keeps the first explicit matcher and reports the one it ignored", () => {
+    // Two different explicit matchers for one label is ambiguous — the agent
+    // may have meant two elements. The card is already anchored on the first,
+    // so keep it, but never drop the second silently.
+    const s = new VariantProposalStore();
+    s.proposeVariant({
+      element: "Buy",
+      match: { by: "identifier", value: "buy-top" },
+      variant: variant("A"),
+    });
+    const r2 = s.proposeVariant({
+      element: "Buy",
+      match: { by: "identifier", value: "buy-bottom" },
+      variant: variant("B"),
+    });
+
+    expect(r2.matchApplied).toEqual({ by: "identifier", value: "buy-top" });
+    expect(r2.matchIgnored).toEqual({ by: "identifier", value: "buy-bottom" });
+  });
+
+  it("numbers variants within their own element", () => {
+    // The ids used to come from a store-wide counter, so an element's second
+    // variant could be called v5 — which read as a per-element sequence and
+    // was how the reporter first noticed something was wrong.
+    const s = new VariantProposalStore();
+    s.proposeVariant({ element: "Foo", variant: variant("A") });
+    s.proposeVariant({ element: "Foo", variant: variant("B") });
+    s.proposeVariant({ element: "Bar", variant: variant("C") });
+
+    const snap = s.snapshot();
+    expect(snap.proposals[0]!.variants.map((v) => v.id)).toEqual(["v1", "v2"]);
+    expect(snap.proposals[1]!.variants.map((v) => v.id)).toEqual(["v1"]);
+  });
+});

packages/ui/index.html

@@ -2950,6 +2950,7 @@
         cards: new Map(), // id -> { el, sig, target, anim, animVel, home, pos, vel, pinned, gone, spawned, dragging }
         thumbFrames: {}, // id -> frozen frame for the static preview crop
         anchors: {}, // id -> last-matched element frame {x,y,width,height} (disambiguates shared labels by size)
+        matchSigs: {}, // id -> matcher the anchor was locked against; a change re-homes the card
         conn: new Map(), // id -> { path, dot } reused SVG nodes
         pinEls: new Map(), // annId -> reused pin element
         annId: 0,
@@ -3272,6 +3273,7 @@
                 vp.annotations = [];
                 vp.thumbFrames = {};
                 vp.anchors = {};
+                vp.matchSigs = {};
                 vp.revealOffscreen = false;
                 vp.offscreenPromptCollapsed = false;
                 vp.inspectorUsed = false; // new round → reset per-round usage flags
@@ -5051,6 +5053,17 @@
           // element is followed across big scroll jumps, and when it leaves the
           // screen the bubble goes off-screen instead of re-homing onto a
           // same-text impostor. See vpMatchNode / vpDescMatches.
+          // The locator can be upgraded mid-round (a later call supplies a real
+          // matcher where the first only had the label-derived default). The
+          // anchor was locked against the OLD matcher, so drop it and re-home.
+          const matchSig = p.match ? p.match.by + ":" + p.match.value : "";
+          if (vp.matchSigs && vp.matchSigs[p.id] !== matchSig) {
+            if (vp.matchSigs[p.id] !== undefined) {
+              delete vp.anchors[p.id];
+              delete vp.thumbFrames[p.id];
+            }
+            vp.matchSigs[p.id] = matchSig;
+          }
           let anchor = vp.anchors[p.id];
           if (!anchor) {
             const av = p.variants && p.variants.find((v) => v.frame);