From d60f7479e6c1350ba9efa9146feee651df1a1fb0 Mon Sep 17 00:00:00 2001 From: Filip131311 Date: Sat, 1 Aug 2026 06:25:26 +0200 Subject: [PATCH] fix(lens): group variants by the element label, not the matcher MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit propose_variant keyed an element's identity on "match", defaulted to {by:'text', value: element}. Because the default equals the label, calls that omitted "match" appeared to accumulate correctly and hid the problem — but supplying it, which the docs recommend, produced a second picker card, so the human was asked to choose between halves of one set. The key was symmetric, so it failed the other way too, and that direction is worse: two different labels sharing a matcher value merged. Staging a variant for 'Footer link' filed it under 'Header logo' and returned that other label in the response, with nothing to indicate a different element had absorbed it. A picker card would have shown one component's variants under another's name. Identity is now the label, compared ignoring case and surrounding whitespace so it agrees with the id slug, which already folds both. "match" becomes a locator. One the agent supplies replaces the label-derived default, since that default is a synthesized placeholder and a real matcher is strictly better. Two different explicit matchers for one label is ambiguous — the agent may have meant two elements — so the first is kept, and the second is reported in the result and named in the hint together with the remedy, because dropping it silently would be the same class of bug as the one being fixed. The preview window locks a card's anchor against the matcher it first resolved and never invalidates it, so an upgraded locator would have left the card homed on the old node. It now re-homes when the matcher changes. Variant ids were drawn from a store-wide counter while variantCount was per-proposal, so an element's second variant could be called v5. That mismatch is how the reporter first noticed something was wrong. Ids are per-element now; the counter was already reset every round, so store-wide uniqueness was never a property anything could rely on. Both tools stay behind the argent-lens flag. --- packages/skills/skills/argent-lens/SKILL.md | 2 +- .../src/tools/variants/propose-variant.ts | 26 +++-- .../src/utils/variant-proposals.ts | 55 +++++++++-- .../test/variant-proposals.test.ts | 96 +++++++++++++++++++ packages/ui/index.html | 13 +++ 5 files changed, 177 insertions(+), 15 deletions(-) diff --git a/packages/skills/skills/argent-lens/SKILL.md b/packages/skills/skills/argent-lens/SKILL.md index ec32a5eb8..36fd5114a 100644 --- a/packages/skills/skills/argent-lens/SKILL.md +++ b/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). diff --git a/packages/tool-server/src/tools/variants/propose-variant.ts b/packages/tool-server/src/tools/variants/propose-variant.ts index 38d243246..c09a258ab 100644 --- a/packages/tool-server/src/tools/variants/propose-variant.ts +++ b/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, }; }, }; diff --git a/packages/tool-server/src/utils/variant-proposals.ts b/packages/tool-server/src/utils/variant-proposals.ts index f84e96555..48f2d2289 100644 --- a/packages/tool-server/src/utils/variant-proposals.ts +++ b/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(); 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,6 +540,8 @@ export class VariantProposalStore { element: string; variantCount: number; totalElements: number; + matchApplied: VariantMatch; + matchIgnored?: VariantMatch; } { this.autoRollIfCompleted(); @@ -533,25 +549,46 @@ export class VariantProposalStore { // 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 } : {}), }; } diff --git a/packages/tool-server/test/variant-proposals.test.ts b/packages/tool-server/test/variant-proposals.test.ts index 27a9f48d2..9d136bd81 100644 --- a/packages/tool-server/test/variant-proposals.test.ts +++ b/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"]); + }); +}); diff --git a/packages/ui/index.html b/packages/ui/index.html index 87777e559..2002666d0 100644 --- a/packages/ui/index.html +++ b/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);