From 2cbe03c3fbe7df39bb9ae3fd5a98194395e948ee Mon Sep 17 00:00:00 2001 From: PJ Date: Wed, 12 Aug 2026 23:43:19 +0530 Subject: [PATCH] fix(analyze): divide by actions that ran Defects per thousand actions counted every step, including steps that chose nothing and steps whose action was never dispatched. The inflation is policy-dependent, so it does not cancel between arms: on the fixture campaign the model arm's yield was reported at 60.3 per thousand against a true 120.7, because half its steps did nothing. A runs.jsonl without the count is refused by name and line rather than read as zero actions, which would report every per-action rate wrongly. The report also carries steps beside actions now, so the gap is visible rather than folded into a denominator. Claude-Session: https://claude.ai/code/session_01A5KmftdEJ49A9z5mF5ESrX --- cmd/internal-tools/analyze/analysis.go | 7 +- cmd/internal-tools/analyze/analysis_test.go | 51 +++++++++++++- cmd/internal-tools/analyze/end_to_end_test.go | 68 +++++++++++-------- cmd/internal-tools/analyze/load.go | 15 ++++ cmd/internal-tools/analyze/load_test.go | 53 +++++++++++++-- cmd/internal-tools/analyze/report.go | 4 +- 6 files changed, 162 insertions(+), 36 deletions(-) diff --git a/cmd/internal-tools/analyze/analysis.go b/cmd/internal-tools/analyze/analysis.go index 35b6cc3..8789bd7 100644 --- a/cmd/internal-tools/analyze/analysis.go +++ b/cmd/internal-tools/analyze/analysis.go @@ -23,6 +23,7 @@ type armSummary struct { MedianStepsToFirstViolation *float64 `json:"median_steps_to_first_violation"` SurvivalCurve []survivalPoint `json:"survival_curve,omitempty"` ViolationRate *float64 `json:"violation_rate"` + TotalSteps int `json:"total_steps"` TotalActions int `json:"total_actions"` TotalRunHours float64 `json:"total_run_hours"` Detections int `json:"detections"` @@ -144,7 +145,11 @@ func summarize(current arm) armSummary { continue } summary.Usable++ - summary.TotalActions += item.Steps + summary.TotalSteps += item.Steps + // Steps and actions differ by the steps that chose no action and the + // steps whose action was never dispatched. Only dispatched actions + // exercised the app, so only they belong in a per-action rate. + summary.TotalActions += item.Actions summary.TotalRunHours += float64(item.DurationMillis) / float64(time.Hour/time.Millisecond) if item.ClampedToBudget { summary.EventsHeldAtBudget++ diff --git a/cmd/internal-tools/analyze/analysis_test.go b/cmd/internal-tools/analyze/analysis_test.go index 5311db6..565cace 100644 --- a/cmd/internal-tools/analyze/analysis_test.go +++ b/cmd/internal-tools/analyze/analysis_test.go @@ -10,6 +10,7 @@ func violatingRun(seed int64, steps, origin int, properties ...string) classifie return classifiedRun{ Seed: seed, Steps: steps, + Actions: steps, DurationMillis: 60_000, OriginStep: origin, Violated: true, @@ -18,7 +19,7 @@ func violatingRun(seed int64, steps, origin int, properties ...string) classifie } func cleanRun(seed int64, steps int) classifiedRun { - return classifiedRun{Seed: seed, Steps: steps, DurationMillis: 60_000} + return classifiedRun{Seed: seed, Steps: steps, Actions: steps, DurationMillis: 60_000} } func TestSummarize_ArmWhereNoRunViolated(t *testing.T) { @@ -76,6 +77,54 @@ func TestSummarize_ArmWhereEveryRunViolated(t *testing.T) { } } +// A step that chose no action, and a step whose action was never dispatched, +// left the app untouched. Counting them would inflate the denominator of every +// per-action rate, and the inflation differs by arm so it does not cancel. +func TestSummarize_CountsDispatchedActionsNotSteps(t *testing.T) { + summary := summarize(arm{ + Name: "declines", + Budget: 40, + Runs: []classifiedRun{ + {Seed: 1, Steps: 40, Actions: 10, DurationMillis: 3_600_000, + Violated: true, OriginStep: 12, ViolatedProperties: []string{"cartTotal"}}, + {Seed: 2, Steps: 40, Actions: 6, DurationMillis: 3_600_000}, + }, + }) + if summary.TotalSteps != 80 { + t.Errorf("total steps %d, want 80", summary.TotalSteps) + } + if summary.TotalActions != 16 { + t.Errorf("total actions %d, want 16 dispatched of 80 steps", summary.TotalActions) + } + if summary.DefectsPerThousandActions == nil { + t.Fatal("no defects per thousand actions") + } + expected := 1000.0 / 16.0 + if math.Abs(*summary.DefectsPerThousandActions-expected) > 1e-9 { + t.Errorf("defects per thousand actions %v, want %v", *summary.DefectsPerThousandActions, expected) + } +} + +func TestSummarize_ArmThatDispatchedNothingHasNoPerActionRate(t *testing.T) { + summary := summarize(arm{ + Name: "inert", + Budget: 20, + Runs: []classifiedRun{ + {Seed: 1, Steps: 20, Actions: 0, DurationMillis: 3_600_000, + Violated: true, OriginStep: 3, ViolatedProperties: []string{"cartTotal"}}, + }, + }) + if summary.TotalActions != 0 || summary.TotalSteps != 20 { + t.Errorf("steps %d actions %d, want 20 and 0", summary.TotalSteps, summary.TotalActions) + } + if summary.DefectsPerThousandActions != nil { + t.Errorf("defects per thousand actions %v, want none with nothing dispatched", *summary.DefectsPerThousandActions) + } + if summary.DefectsPerHour == nil || *summary.DefectsPerHour != 1 { + t.Errorf("defects per hour %v, want 1: the run still consumed an hour", summary.DefectsPerHour) + } +} + func TestSummarize_ArmWithNoUsableRunsAfterExclusions(t *testing.T) { summary := summarize(arm{ Name: "broken", diff --git a/cmd/internal-tools/analyze/end_to_end_test.go b/cmd/internal-tools/analyze/end_to_end_test.go index 1ce33c0..7ce181e 100644 --- a/cmd/internal-tools/analyze/end_to_end_test.go +++ b/cmd/internal-tools/analyze/end_to_end_test.go @@ -37,38 +37,40 @@ func seededArmRecords() []map[string]any { // Ten runs: two violate early, one violates late, six run the budget clean, // one times out and is missing data rather than a censored observation. return []map[string]any{ - {"seed": 1, "exit_code": 0, "steps": 60, "duration_millis": 300000, "first_violation_origin_step": nil}, - {"seed": 2, "exit_code": 0, "steps": 18, "duration_millis": 120000, + {"seed": 1, "exit_code": 0, "steps": 60, "actions": 58, "duration_millis": 300000, "first_violation_origin_step": nil}, + {"seed": 2, "exit_code": 0, "steps": 18, "actions": 16, "duration_millis": 120000, "first_violation_origin_step": 14, "violated_properties": []string{"cartTotalMatches"}}, - {"seed": 3, "exit_code": 0, "steps": 60, "duration_millis": 300000, "first_violation_origin_step": nil}, - {"seed": 4, "exit_code": 0, "steps": 60, "duration_millis": 300000, "first_violation_origin_step": nil}, - {"seed": 5, "exit_code": 0, "steps": 44, "duration_millis": 240000, + {"seed": 3, "exit_code": 0, "steps": 60, "actions": 58, "duration_millis": 300000, "first_violation_origin_step": nil}, + {"seed": 4, "exit_code": 0, "steps": 60, "actions": 58, "duration_millis": 300000, "first_violation_origin_step": nil}, + {"seed": 5, "exit_code": 0, "steps": 44, "actions": 42, "duration_millis": 240000, "first_violation_origin_step": 41, "violated_properties": []string{"cartTotalMatches", "backLeavesApp"}}, - {"seed": 6, "exit_code": 0, "steps": 60, "duration_millis": 300000, "first_violation_origin_step": nil}, - {"seed": 7, "exit_code": -1, "timed_out": true, "duration_millis": 900000}, - {"seed": 8, "exit_code": 0, "steps": 60, "duration_millis": 300000, "first_violation_origin_step": nil}, - {"seed": 9, "exit_code": 0, "steps": 60, "duration_millis": 300000, "first_violation_origin_step": nil}, - {"seed": 10, "exit_code": 0, "steps": 21, "duration_millis": 130000, + {"seed": 6, "exit_code": 0, "steps": 60, "actions": 58, "duration_millis": 300000, "first_violation_origin_step": nil}, + {"seed": 7, "exit_code": -1, "timed_out": true, "actions": 0, "duration_millis": 900000}, + {"seed": 8, "exit_code": 0, "steps": 60, "actions": 58, "duration_millis": 300000, "first_violation_origin_step": nil}, + {"seed": 9, "exit_code": 0, "steps": 60, "actions": 58, "duration_millis": 300000, "first_violation_origin_step": nil}, + {"seed": 10, "exit_code": 0, "steps": 21, "actions": 19, "duration_millis": 130000, "first_violation_origin_step": 19, "violated_properties": []string{"cartTotalMatches"}}, } } func llmArmRecords() []map[string]any { - // Eight runs: six violate, one clean, one failed to launch. + // Eight runs: six violate, one clean, one failed to launch. This arm + // dispatches an action on about half its steps, which is the asymmetry the + // per-action denominator has to survive. return []map[string]any{ - {"seed": 1, "exit_code": 0, "steps": 7, "duration_millis": 400000, + {"seed": 1, "exit_code": 0, "steps": 7, "actions": 3, "duration_millis": 400000, "first_violation_origin_step": 5, "violated_properties": []string{"cartTotalMatches"}}, - {"seed": 2, "exit_code": 0, "steps": 9, "duration_millis": 420000, + {"seed": 2, "exit_code": 0, "steps": 9, "actions": 5, "duration_millis": 420000, "first_violation_origin_step": 8, "violated_properties": []string{"backLeavesApp"}}, - {"seed": 3, "exit_code": 0, "steps": 60, "duration_millis": 1800000, "first_violation_origin_step": nil}, - {"seed": 4, "exit_code": 0, "steps": 5, "duration_millis": 380000, + {"seed": 3, "exit_code": 0, "steps": 60, "actions": 30, "duration_millis": 1800000, "first_violation_origin_step": nil}, + {"seed": 4, "exit_code": 0, "steps": 5, "actions": 2, "duration_millis": 380000, "first_violation_origin_step": 3, "violated_properties": []string{"cartTotalMatches"}}, - {"seed": 5, "exit_code": 0, "steps": 13, "duration_millis": 500000, + {"seed": 5, "exit_code": 0, "steps": 13, "actions": 7, "duration_millis": 500000, "first_violation_origin_step": 11, "violated_properties": []string{"cartTotalMatches", "priceNeverNegative"}}, - {"seed": 6, "exit_code": -1, "launch_error": "fork/exec sanderling: no such file or directory"}, - {"seed": 7, "exit_code": 0, "steps": 6, "duration_millis": 390000, + {"seed": 6, "exit_code": -1, "actions": 0, "launch_error": "fork/exec sanderling: no such file or directory"}, + {"seed": 7, "exit_code": 0, "steps": 6, "actions": 3, "duration_millis": 390000, "first_violation_origin_step": 6, "violated_properties": []string{"cartTotalMatches"}}, - {"seed": 8, "exit_code": 0, "steps": 16, "duration_millis": 520000, + {"seed": 8, "exit_code": 0, "steps": 16, "actions": 8, "duration_millis": 520000, "first_violation_origin_step": 15, "violated_properties": []string{"backLeavesApp"}}, } } @@ -134,8 +136,8 @@ func TestRun_EndToEndOverFixtureCampaignDirectories(t *testing.T) { if seeded.DistinctDefects != 2 || seeded.SingletonDefects != 1 { t.Errorf("seeded defects %d singletons %d, want 2 and 1", seeded.DistinctDefects, seeded.SingletonDefects) } - if seeded.TotalActions != 443 { - t.Errorf("seeded actions %d, want 443", seeded.TotalActions) + if seeded.TotalSteps != 443 || seeded.TotalActions != 425 { + t.Errorf("seeded steps %d actions %d, want 443 and 425", seeded.TotalSteps, seeded.TotalActions) } llm := byName["llm"] @@ -148,6 +150,18 @@ func TestRun_EndToEndOverFixtureCampaignDirectories(t *testing.T) { if llm.MedianStepsToFirstViolation == nil || *llm.MedianStepsToFirstViolation != 8 { t.Errorf("llm median %v, want 8", llm.MedianStepsToFirstViolation) } + // The arm dispatches an action on half its steps, so counting steps would + // halve its yield per thousand actions and flatter it against seeded. + if llm.TotalSteps != 116 || llm.TotalActions != 58 { + t.Errorf("llm steps %d actions %d, want 116 and 58", llm.TotalSteps, llm.TotalActions) + } + if llm.Detections != 7 { + t.Fatalf("llm detections %d, want 7", llm.Detections) + } + expected := 7000.0 / 58.0 + if math.Abs(*llm.DefectsPerThousandActions-expected) > 1e-9 { + t.Errorf("llm defects per thousand actions %v, want %v", *llm.DefectsPerThousandActions, expected) + } if result.LogRank == nil { t.Fatal("no log-rank result") @@ -181,13 +195,13 @@ func TestRun_ReportsBothArmsWhenOneHasNothingUsable(t *testing.T) { good := filepath.Join(root, "good") broken := filepath.Join(root, "broken") buildFixtureCampaign(t, good, "good", 30, []map[string]any{ - {"seed": 1, "exit_code": 0, "steps": 30}, - {"seed": 2, "exit_code": 0, "steps": 9, "duration_millis": 1000, + {"seed": 1, "exit_code": 0, "steps": 30, "actions": 28}, + {"seed": 2, "exit_code": 0, "steps": 9, "actions": 9, "duration_millis": 1000, "first_violation_origin_step": 9, "violated_properties": []string{"cartTotalMatches"}}, }) buildFixtureCampaign(t, broken, "broken", 30, []map[string]any{ - {"seed": 1, "exit_code": 3}, - {"seed": 2, "timed_out": true, "exit_code": -1}, + {"seed": 1, "exit_code": 3, "actions": 0}, + {"seed": 2, "timed_out": true, "exit_code": -1, "actions": 0}, }) var stdout bytes.Buffer @@ -210,7 +224,7 @@ func TestRun_JsonToStdout(t *testing.T) { root := t.TempDir() directory := filepath.Join(root, "only") buildFixtureCampaign(t, directory, "only", 20, []map[string]any{ - {"seed": 1, "exit_code": 0, "steps": 20}, + {"seed": 1, "exit_code": 0, "steps": 20, "actions": 17}, }) var stdout bytes.Buffer if err := run([]string{"--json", "-", "--campaign", directory}, &stdout, io.Discard); err != nil { @@ -232,7 +246,7 @@ func TestRun_JsonToStdout(t *testing.T) { func TestRun_RejectsTheSameDirectoryTwice(t *testing.T) { root := t.TempDir() directory := filepath.Join(root, "one") - buildFixtureCampaign(t, directory, "one", 20, []map[string]any{{"seed": 1, "exit_code": 0, "steps": 20}}) + buildFixtureCampaign(t, directory, "one", 20, []map[string]any{{"seed": 1, "exit_code": 0, "steps": 20, "actions": 20}}) err := run([]string{directory, directory}, io.Discard, io.Discard) if err == nil || !strings.Contains(err.Error(), "twice") { t.Fatalf("error %v, want a refusal to double count", err) diff --git a/cmd/internal-tools/analyze/load.go b/cmd/internal-tools/analyze/load.go index b622f67..3b9aa3c 100644 --- a/cmd/internal-tools/analyze/load.go +++ b/cmd/internal-tools/analyze/load.go @@ -39,6 +39,11 @@ type runRecord struct { Steps int `json:"steps"` FirstViolationOriginStep *int `json:"first_violation_origin_step"` ViolatedProperties []string `json:"violated_properties"` + // Actions is the count of steps that dispatched an action, and it is a + // pointer so that a runs.jsonl written before the campaign tool counted + // them is refused rather than read as an arm that acted zero times. The + // campaign tool always emits the field, so its absence dates the file. + Actions *int `json:"actions"` } // Exclusion reasons. A run that failed or timed out is missing data, not a @@ -55,6 +60,7 @@ const ( type classifiedRun struct { Seed int64 Steps int + Actions int DurationMillis int64 OriginStep int Violated bool @@ -110,6 +116,12 @@ func loadCampaign(directory string) (manifest, []runRecord, error) { if err := json.Unmarshal([]byte(raw), &record); err != nil { return manifest{}, nil, fmt.Errorf("%s line %d in %s: %w", recordsFileName, lineNumber, directory, err) } + if record.Actions == nil { + return manifest{}, nil, fmt.Errorf("%s line %d in %s has no actions count: it was written before "+ + "dispatched actions were counted, and reading the missing count as zero would report every "+ + "per-action rate wrongly; re-run the campaign to produce it", + recordsFileName, lineNumber, directory) + } records = append(records, record) } if err := scanner.Err(); err != nil { @@ -127,6 +139,9 @@ func classify(record runRecord, budget int) classifiedRun { DurationMillis: record.DurationMillis, ViolatedProperties: slices.Clone(record.ViolatedProperties), } + if record.Actions != nil { + item.Actions = *record.Actions + } switch { case record.LaunchError != "": item.ExcludedBecause = reasonLaunchError diff --git a/cmd/internal-tools/analyze/load_test.go b/cmd/internal-tools/analyze/load_test.go index ce8f1fa..f4099dc 100644 --- a/cmd/internal-tools/analyze/load_test.go +++ b/cmd/internal-tools/analyze/load_test.go @@ -94,18 +94,26 @@ func writeCampaign(t *testing.T, directory string, declared map[string]any, reco } } +func TestClassify_CarriesTheDispatchedActionCount(t *testing.T) { + actions := 7 + item := classify(runRecord{Seed: 4, Steps: 30, Actions: &actions}, 50) + if item.Steps != 30 || item.Actions != 7 { + t.Errorf("run %+v, want 30 steps and 7 actions", item) + } +} + func TestGroupArms_PoolsDirectoriesSharingAnArmAndReportsMissingSeeds(t *testing.T) { root := t.TempDir() writeCampaign(t, filepath.Join(root, "north"), map[string]any{ "arm": "seeded", "max_steps": 40, "seeds": []int{1, 2, 3}, }, []map[string]any{ - {"seed": 1, "exit_code": 0, "steps": 40}, - {"seed": 2, "exit_code": 0, "steps": 9, "first_violation_origin_step": 9}, + {"seed": 1, "exit_code": 0, "steps": 40, "actions": 33}, + {"seed": 2, "exit_code": 0, "steps": 9, "actions": 8, "first_violation_origin_step": 9}, }) writeCampaign(t, filepath.Join(root, "south"), map[string]any{ "arm": "seeded", "max_steps": 40, "seeds": []int{4}, }, []map[string]any{ - {"seed": 4, "exit_code": 0, "steps": 40}, + {"seed": 4, "exit_code": 0, "steps": 40, "actions": 40}, }) arms, err := groupArms([]string{filepath.Join(root, "north"), filepath.Join(root, "south")}) @@ -129,9 +137,9 @@ func TestGroupArms_PoolsDirectoriesSharingAnArmAndReportsMissingSeeds(t *testing func TestGroupArms_RejectsDisagreeingStepBudgets(t *testing.T) { root := t.TempDir() writeCampaign(t, filepath.Join(root, "a"), map[string]any{"arm": "seeded", "max_steps": 40, "seeds": []int{1}}, - []map[string]any{{"seed": 1, "exit_code": 0, "steps": 40}}) + []map[string]any{{"seed": 1, "exit_code": 0, "steps": 40, "actions": 40}}) writeCampaign(t, filepath.Join(root, "b"), map[string]any{"arm": "seeded", "max_steps": 80, "seeds": []int{2}}, - []map[string]any{{"seed": 2, "exit_code": 0, "steps": 80}}) + []map[string]any{{"seed": 2, "exit_code": 0, "steps": 80, "actions": 80}}) _, err := groupArms([]string{filepath.Join(root, "a"), filepath.Join(root, "b")}) if err == nil || !strings.Contains(err.Error(), "different budgets") { @@ -153,7 +161,8 @@ func TestGroupArms_ReportsBadRecordLines(t *testing.T) { root := t.TempDir() directory := filepath.Join(root, "a") writeCampaign(t, directory, map[string]any{"arm": "seeded", "max_steps": 40, "seeds": []int{1}}, nil) - if err := os.WriteFile(filepath.Join(directory, recordsFileName), []byte("{\"seed\":1}\nnot json\n"), 0o644); err != nil { + if err := os.WriteFile(filepath.Join(directory, recordsFileName), + []byte("{\"seed\":1,\"actions\":0}\nnot json\n"), 0o644); err != nil { t.Fatal(err) } _, err := groupArms([]string{directory}) @@ -161,3 +170,35 @@ func TestGroupArms_ReportsBadRecordLines(t *testing.T) { t.Fatalf("error %v, want the offending line number", err) } } + +// A runs.jsonl written before the campaign counted dispatched actions has no +// such field. Reading the absence as zero would divide by zero, so the whole +// campaign is refused instead. +func TestGroupArms_RefusesRecordsWithoutADispatchedActionCount(t *testing.T) { + root := t.TempDir() + directory := filepath.Join(root, "old-format") + writeCampaign(t, directory, map[string]any{"arm": "seeded", "max_steps": 40, "seeds": []int{1, 2}}, + []map[string]any{ + {"seed": 1, "exit_code": 0, "steps": 40, "actions": 40}, + {"seed": 2, "exit_code": 0, "steps": 40}, + }) + _, err := groupArms([]string{directory}) + if err == nil { + t.Fatal("a runs.jsonl without an action count was accepted") + } + for _, fragment := range []string{"line 2", "actions", directory} { + if !strings.Contains(err.Error(), fragment) { + t.Errorf("error %q is missing %q", err, fragment) + } + } +} + +func TestGroupArms_RefusesAnExcludedRecordWithoutAnActionCount(t *testing.T) { + root := t.TempDir() + directory := filepath.Join(root, "old-format") + writeCampaign(t, directory, map[string]any{"arm": "seeded", "max_steps": 40, "seeds": []int{1}}, + []map[string]any{{"seed": 1, "exit_code": 3}}) + if _, err := groupArms([]string{directory}); err == nil { + t.Fatal("an old-format record was accepted because the run was excluded anyway") + } +} diff --git a/cmd/internal-tools/analyze/report.go b/cmd/internal-tools/analyze/report.go index 47054b8..ced1ee6 100644 --- a/cmd/internal-tools/analyze/report.go +++ b/cmd/internal-tools/analyze/report.go @@ -32,11 +32,13 @@ func writeReport(result analysis, out io.Writer) { fmt.Fprintln(out) fmt.Fprintln(out, "a detection is one distinct property violated in one run; run hours sum the per-run wall clock") - writeTable(out, []string{"arm", "actions", "run hours", "detections", "defects/1k actions", "defects/hour", "distinct defects", "found in one run"}, + fmt.Fprintln(out, "actions count the steps that dispatched one; the rest chose nothing or had the choice thrown away") + writeTable(out, []string{"arm", "steps", "actions", "run hours", "detections", "defects/1k actions", "defects/hour", "distinct defects", "found in one run"}, func(add func(...string)) { for _, summary := range result.Arms { add( summary.Arm, + strconv.Itoa(summary.TotalSteps), strconv.Itoa(summary.TotalActions), fmt.Sprintf("%.2f", summary.TotalRunHours), strconv.Itoa(summary.Detections),