From 628eb1949abc6591bc81ac5cba19461c085c2cb2 Mon Sep 17 00:00:00 2001 From: PJ Date: Tue, 18 Aug 2026 19:45:55 +0530 Subject: [PATCH] fix(analyze): refuse to compare attributed and unattributed denominators One arm's actions may include the login the spec's setup drove and the other's cannot, so a per-action rate over the two divides by different things and the tests rank the bookkeeping. --- cmd/internal-tools/analyze/analysis.go | 41 +++++++++++--- cmd/internal-tools/analyze/analysis_test.go | 61 +++++++++++++++++++++ 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/cmd/internal-tools/analyze/analysis.go b/cmd/internal-tools/analyze/analysis.go index ff33124..a244c1d 100644 --- a/cmd/internal-tools/analyze/analysis.go +++ b/cmd/internal-tools/analyze/analysis.go @@ -29,6 +29,7 @@ type armSummary struct { ViolationRate *float64 `json:"violation_rate"` TotalSteps int `json:"total_steps"` TotalActions int `json:"total_actions"` + UnattributedActions int `json:"unattributed_actions"` TotalRunHours float64 `json:"total_run_hours"` Detections int `json:"detections"` DefectsPerThousandActions *float64 `json:"defects_per_thousand_actions"` @@ -127,6 +128,9 @@ func baseAnalysis(arms []arm, now time.Time) (analysis, []arm, error) { if err := sameBudget(testable); err != nil { return analysis{}, nil, err } + if err := sameAttribution(testable); err != nil { + return analysis{}, nil, err + } if len(testable) >= 2 { names := make([]string, len(testable)) groups := make([][]observation, len(testable)) @@ -157,6 +161,28 @@ func sameBudget(arms []arm) error { return nil } +// sameAttribution refuses arms whose actions were counted against different +// denominators. An arm recorded before an action named its producer counts +// whatever the spec's setup dispatched among its actions, and an arm recorded +// after leaves the login out, so the same rate over the two divides by +// different things and the tests rank a bookkeeping difference. Two arms of the +// same unknown provenance are diluted alike and compare; one of each does not. +func sameAttribution(arms []arm) error { + for index := 1; index < len(arms); index++ { + unknown, attributed := arms[0], arms[index] + if (unknown.unattributedActions() == 0) == (attributed.unattributedActions() == 0) { + continue + } + if unknown.unattributedActions() == 0 { + unknown, attributed = attributed, unknown + } + return fmt.Errorf("arm %q counts %d action(s) of unknown provenance and arm %q counts none: "+ + "a denominator that may include the spec's setup cannot be compared against one that excludes it", + unknown.Name, unknown.unattributedActions(), attributed.Name) + } + return nil +} + func countCorrected(pairs []pairwiseResult) int { corrected := 0 for _, pair := range pairs { @@ -205,13 +231,14 @@ func comparePairs(arms []arm) []pairwiseResult { func summarize(current arm) armSummary { summary := armSummary{ - Arm: current.Name, - Generator: current.Generator, - Platform: current.Platform, - StepBudget: current.Budget, - Directories: current.Directories, - Recorded: len(current.Runs), - MissingSeeds: current.MissingSeeds, + Arm: current.Name, + Generator: current.Generator, + Platform: current.Platform, + StepBudget: current.Budget, + Directories: current.Directories, + Recorded: len(current.Runs), + MissingSeeds: current.MissingSeeds, + UnattributedActions: current.unattributedActions(), } runsPerDefect := map[string]int{} for _, item := range current.Runs { diff --git a/cmd/internal-tools/analyze/analysis_test.go b/cmd/internal-tools/analyze/analysis_test.go index 0b83203..f092054 100644 --- a/cmd/internal-tools/analyze/analysis_test.go +++ b/cmd/internal-tools/analyze/analysis_test.go @@ -352,6 +352,67 @@ func TestRun_RefusesToCompareArmsCensoredAtDifferentBudgets(t *testing.T) { } } +func writeSourcedCampaign(t *testing.T, directory, name string, steps, unattributed int) { + t.Helper() + const budget, runs, actions = 40, 6, 20 + seeds := make([]int, 0, runs) + records := make([]map[string]any, 0, runs) + for seed := 1; seed <= runs; seed++ { + seeds = append(seeds, seed) + records = append(records, map[string]any{ + "seed": seed, "exit_code": 0, "steps": steps, "actions": actions, + "monotonic_millis": 60_000, "unattributed_actions": unattributed, + }) + } + writeCampaign(t, directory, map[string]any{"arm": name, "max_steps": budget, "seeds": seeds}, records) +} + +// An arm whose actions name no producer counts whatever the spec's setup +// dispatched in the denominator of every per-action rate, and an arm whose +// actions name one leaves the login out of it. The two denominators measure +// different things, so a test that ranks one arm against the other reads a +// difference in what was counted as a difference in what the arms found. +func TestRun_RefusesToCompareArmsWhoseActionsWereCountedDifferently(t *testing.T) { + cases := []struct { + name string + arguments []string + }{ + {name: "independent samples"}, + {name: "paired", arguments: []string{"--paired"}}, + } + for _, test := range cases { + root := t.TempDir() + attributed := filepath.Join(root, "attributed") + legacy := filepath.Join(root, "legacy") + writeSourcedCampaign(t, attributed, "attributed", 40, 0) + writeSourcedCampaign(t, legacy, "legacy", 30, 20) + + err := run(append(test.arguments, attributed, legacy), io.Discard, io.Discard) + if err == nil { + t.Fatalf("%s: an arm of unknown provenance was tested against an attributed one without complaint", test.name) + } + for _, fragment := range []string{"attributed", "legacy", "120", "unknown provenance"} { + if !strings.Contains(err.Error(), fragment) { + t.Errorf("%s: error %q is missing %q", test.name, err, fragment) + } + } + } +} + +// Two arms recorded before actions named their producer are on the same +// denominator as each other, diluted the same way, so they compare. +func TestRun_ComparesTwoArmsThatBothNameNoProducer(t *testing.T) { + root := t.TempDir() + first := filepath.Join(root, "first") + second := filepath.Join(root, "second") + writeSourcedCampaign(t, first, "first", 40, 20) + writeSourcedCampaign(t, second, "second", 30, 20) + + if err := run([]string{first, second}, io.Discard, io.Discard); err != nil { + t.Fatalf("two arms of the same unknown provenance were refused: %v", err) + } +} + func manyRuns(count, originStep int) []classifiedRun { runs := make([]classifiedRun, 0, count) for index := 0; index < count; index++ {