diff --git a/internal/verifier/llm.go b/internal/verifier/llm.go index 8c8fabd..7054ed7 100644 --- a/internal/verifier/llm.go +++ b/internal/verifier/llm.go @@ -113,7 +113,9 @@ type ActionCandidate struct { // Kind is the resulting action kind. Kind ActionKind // Description is the rendered action shown to the model and echoed back as - // chosen_action, e.g. `Tap "Add credit"`. Dedup keys on it, so it is unique. + // chosen_action, e.g. `Tap "Add credit"`. Two entries may render the same + // when the screen holds two controls a user reads alike; Index is what tells + // them apart, and it is what the model picks by. Description string // Label is the target label the selected LabelSource named (empty for // gestures). @@ -162,7 +164,8 @@ const ( // SAME tree the seeded picker draws: weighted branches recurse (accumulating the // selection probability), authored actions()/whenRoute leaves are called once // for their concrete actions, and builtin verbs come straight from the picker's -// own enumeration. Identical descriptions dedup, summing weight. +// own enumeration. Candidates that would execute the same action dedup, summing +// weight; two controls that merely read alike stay two entries. // // labelSource selects the channel each target is named by. An unrecognized // value (including the zero value) names targets by visible text; the CLI @@ -491,20 +494,33 @@ func (v *Verifier) enumerateBuiltin(verb string) ([]builtinCandidate, error) { return entries, nil } -// finalizeCandidates renders each candidate's description, dedups identical -// descriptions (summing weight), numbers the survivors 1..N, and rounds the -// accumulated probability into a percentage Weight. +// candidateIdentity is what a candidate would DO. Two candidates sharing it are +// the same action reached through two paths of the action tree, so folding them +// into one numbered entry loses nothing; two that differ are different actions +// however alike they read, so folding them would put one of them out of reach. +// llmText is part of it because it decides where the typed text comes from: the +// model writes it for a builtin typing candidate, while an authored one replays +// the value already sitting in Action.Text. +type candidateIdentity struct { + action Action + llmText bool +} + +// finalizeCandidates renders each candidate's description, dedups by what the +// candidate executes (summing weight), numbers the survivors 1..N, and rounds +// the accumulated probability into a percentage Weight. func finalizeCandidates(raw []ActionCandidate) []ActionCandidate { - seen := make(map[string]int, len(raw)) + seen := make(map[candidateIdentity]int, len(raw)) result := make([]ActionCandidate, 0, len(raw)) for _, candidate := range raw { candidate.Description = describeCandidate(candidate) - if index, ok := seen[candidate.Description]; ok { + identity := candidateIdentity{action: candidate.Action, llmText: candidate.LLMText} + if index, ok := seen[identity]; ok { result[index].prob += candidate.prob result[index].Weighted = result[index].Weighted || candidate.Weighted continue } - seen[candidate.Description] = len(result) + seen[identity] = len(result) result = append(result, candidate) } for i := range result { @@ -517,8 +533,11 @@ func finalizeCandidates(raw []ActionCandidate) []ActionCandidate { } // describeCandidate renders the plain, echo-friendly description shown in the -// numbered list. It is the dedup key, so it must be stable and unique per -// distinct action. +// numbered list. It is display only: dedup keys on the action, so two entries +// may read alike without merging and Index is what separates them. Do not add an +// ordinal or a coordinate to pull those apart: this string IS the observation +// channel a labelling experiment varies, so a disambiguator here would name a +// target through a channel the label source deliberately withholds. func describeCandidate(candidate ActionCandidate) string { switch candidate.Kind { case ActionKindTap: @@ -538,10 +557,11 @@ func describeCandidate(candidate ActionCandidate) string { case ActionKindScroll: return "Scroll " + candidate.Direction case ActionKindSwipe: - // A swipe carries endpoints and no selector, so the coordinates are what - // keep two swipes distinct. The label is prepended when the origin - // element has one, because "swipe that row" is the interaction a model - // reaches for and a bare pair of points does not say which row. + // A swipe is a drag across the screen, so where it runs is the whole of + // what it does and a reader needs the endpoints to picture it. The label + // is prepended when the origin element has one, because "swipe that row" + // is the interaction a model reaches for and a bare pair of points does + // not say which row. where := fmt.Sprintf("from (%d,%d) to (%d,%d)", candidate.Action.FromX, candidate.Action.FromY, candidate.Action.ToX, candidate.Action.ToY) diff --git a/internal/verifier/llm_test.go b/internal/verifier/llm_test.go index 5fbe69b..ce7948e 100644 --- a/internal/verifier/llm_test.go +++ b/internal/verifier/llm_test.go @@ -41,6 +41,18 @@ const labelChannelTreeJSON = `{ ] }` +// sharedLabelTreeJSON is a list where two rows read exactly the same to a user +// ("Delete") while the app tells them apart by identifier. It is the shape a +// list of removable items has in any real app. +const sharedLabelTreeJSON = `{ + "attributes": {"bounds": "[0,0,1080,2400]"}, + "children": [ + {"attributes": {"resource-id": "delete_alpha", "text": "Delete", "bounds": "[0,100,1080,200]"}, "clickable": true, "enabled": true, "children": []}, + {"attributes": {"resource-id": "delete_beta", "text": "Delete", "bounds": "[0,200,1080,300]"}, "clickable": true, "enabled": true, "children": []}, + {"attributes": {"resource-id": "checkout", "text": "Checkout", "bounds": "[0,400,1080,500]"}, "clickable": true, "enabled": true, "children": []} + ] +}` + // enumVerifier loads a spec whose actions root is the given plain-object graph // and stages the given tree, so Candidates walks a controlled action tree. The // spec is bundled with the goja runtime entry because the model arm reads the @@ -125,12 +137,12 @@ func TestCandidatesIdentifierChannelFallsBackToClassThenBareControl(t *testing.T } } -// TestCandidatesIdentifierChannelMergesControlsItCannotTellApart pins the cost -// of the channel rather than a defect in it: dedup keys on the rendered line, so -// two identifier-less controls of one class arrive as ONE numbered entry and the -// model can only reach the first. The list the identifier arm picks from is -// therefore shorter than the text arm's on the same screen. -func TestCandidatesIdentifierChannelMergesControlsItCannotTellApart(t *testing.T) { +// TestCandidatesIdentifierChannelKeepsControlsItCannotNameApartReachable is the +// cost of the channel, bounded: two identifier-less controls of one class read +// the same in the numbered list, but they stay TWO entries, each carrying its +// own action, so the model can act on either by number. A channel that renames +// controls must never shrink the action space. +func TestCandidatesIdentifierChannelKeepsControlsItCannotNameApartReachable(t *testing.T) { text := enumVerifier(t, "{kind:'builtin', verb:'taps'}", labelChannelTreeJSON). Candidates(LabelSourceVisibleText) identifier := enumVerifier(t, "{kind:'builtin', verb:'taps'}", labelChannelTreeJSON). @@ -139,15 +151,44 @@ func TestCandidatesIdentifierChannelMergesControlsItCannotTellApart(t *testing.T if count(text, `Tap "Remember me"`) != 1 || count(text, `Tap "Stay signed in"`) != 1 { t.Fatalf("the text channel should address both checkboxes, got %v", descriptions(text)) } - if got := count(identifier, `Tap "android.widget.CheckBox"`); got != 1 { - t.Errorf("the two checkboxes should merge into one line, got %d: %v", got, descriptions(identifier)) + checkboxes := candidatesMatching(identifier, `Tap "android.widget.CheckBox"`) + if len(checkboxes) != 2 { + t.Fatalf("the two checkboxes should be two entries, got %d: %v", + len(checkboxes), descriptions(identifier)) } - if len(identifier) >= len(text) { - t.Errorf("identifier list (%d) should be shorter than the text list (%d): %v vs %v", + if checkboxes[0].Action == checkboxes[1].Action { + t.Errorf("both entries execute the same action: %+v", checkboxes[0].Action) + } + if len(identifier) != len(text) { + t.Errorf("identifier list (%d) and text list (%d) must offer the same actions: %v vs %v", len(identifier), len(text), descriptions(identifier), descriptions(text)) } } +// TestCandidatesReachBothControlsSharingOneVisibleLabel is the reachability +// floor: two rows a user reads as the same word are two different controls, so +// both get a number and the second number taps the second row. Dedup that keyed +// on the rendered line dropped the second one, putting it out of reach of any +// prompt or policy. +func TestCandidatesReachBothControlsSharingOneVisibleLabel(t *testing.T) { + candidates := enumVerifier(t, "{kind:'builtin', verb:'taps'}", sharedLabelTreeJSON). + Candidates(LabelSourceVisibleText) + + deletes := candidatesMatching(candidates, `Tap "Delete"`) + if len(deletes) != 2 { + t.Fatalf("want both Delete rows reachable, got %d: %v", len(deletes), descriptions(candidates)) + } + if got := deletes[0].Action.On; got != "id:delete_alpha" { + t.Errorf("first entry targets %q, want id:delete_alpha", got) + } + if got := deletes[1].Action.On; got != "id:delete_beta" { + t.Errorf("second entry targets %q, want id:delete_beta", got) + } + if deletes[0].Index == deletes[1].Index { + t.Errorf("both entries share number %d, so the model cannot address them apart", deletes[0].Index) + } +} + // TestCandidatesVisibleTextFallsBackToTheIdentifier is where the two channels // agree: a control carrying nothing readable is named by its identifier in both, // so a screen built entirely from such controls is one cell, not two. @@ -175,25 +216,56 @@ func TestCandidatesTypingLabelFollowsTheLabelSource(t *testing.T) { } } -// TestLabelSourceChangesOnlyTheDescription is the claim the factorial rests on: -// the channel renames the target and does nothing else, so a difference in -// defect yield cannot come from the two arms executing different actions. +// TestLabelSourceChangesOnlyTheDescription is the claim the labelling factorial +// rests on: the channel renames every target and does nothing else. Both arms +// enumerate the same candidates, in the same order, at the same weights, +// carrying the same executable actions; the description and the label are the +// only things that move. Anything else and the two cells would be picking from +// different action spaces, so a difference in defect yield could not be +// attributed to how the controls were named. func TestLabelSourceChangesOnlyTheDescription(t *testing.T) { - text := enumVerifier(t, "{kind:'builtin', verb:'taps'}", labelChannelTreeJSON). - Candidates(LabelSourceVisibleText) - identifier := enumVerifier(t, "{kind:'builtin', verb:'taps'}", labelChannelTreeJSON). - Candidates(LabelSourceResourceID) - - readable, ok := findCandidate(text, `Tap "Add credit"`) - if !ok { - t.Fatalf("missing the text-labelled tap: %v", descriptions(text)) + const everyLabelledVerb = `{kind:'weighted', branches:[ + [1,{kind:'builtin',verb:'taps'}], + [1,{kind:'builtin',verb:'typing'}], + [1,{kind:'builtin',verb:'swipes'}] + ]}` + fixtures := []struct { + name string + tree string + }{ + {"identifiers collide", labelChannelTreeJSON}, + {"visible text collides", sharedLabelTreeJSON}, } - named, ok := findCandidate(identifier, `Tap "add_credit_button"`) - if !ok { - t.Fatalf("missing the identifier-labelled tap: %v", descriptions(identifier)) + withoutNames := func(candidate ActionCandidate) ActionCandidate { + candidate.Description = "" + candidate.Label = "" + return candidate } - if readable.Action != named.Action { - t.Errorf("same control, different action:\n text=%+v\n id=%+v", readable.Action, named.Action) + for _, fixture := range fixtures { + t.Run(fixture.name, func(t *testing.T) { + text := enumVerifier(t, everyLabelledVerb, fixture.tree).Candidates(LabelSourceVisibleText) + identifier := enumVerifier(t, everyLabelledVerb, fixture.tree).Candidates(LabelSourceResourceID) + if len(text) == 0 { + t.Fatal("fixture yielded no candidates") + } + if len(text) != len(identifier) { + t.Fatalf("different action spaces: %d text candidates vs %d identifier ones:\n%v\n%v", + len(text), len(identifier), descriptions(text), descriptions(identifier)) + } + renamed := false + for i := range text { + if withoutNames(text[i]) != withoutNames(identifier[i]) { + t.Errorf("candidate %d differs beyond its name:\n text=%+v\n id=%+v", + i+1, text[i], identifier[i]) + } + if text[i].Description != identifier[i].Description { + renamed = true + } + } + if !renamed { + t.Error("no candidate was renamed, so this fixture does not exercise the channel") + } + }) } } @@ -429,6 +501,16 @@ func descriptions(candidates []ActionCandidate) []string { return out } +func candidatesMatching(candidates []ActionCandidate, description string) []ActionCandidate { + var matched []ActionCandidate + for _, candidate := range candidates { + if candidate.Description == description { + matched = append(matched, candidate) + } + } + return matched +} + func count(candidates []ActionCandidate, description string) int { n := 0 for _, candidate := range candidates {