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);