diff --git a/docs/superpowers/specs/2026-07-06-benchmark-findings.md b/docs/superpowers/specs/2026-07-06-benchmark-findings.md index ae3d8aa..fe4af15 100644 --- a/docs/superpowers/specs/2026-07-06-benchmark-findings.md +++ b/docs/superpowers/specs/2026-07-06-benchmark-findings.md @@ -9,17 +9,19 @@ ## TL;DR -On two trivial repetition workflows — `nullcheck` (hand-written skill) and `shellseq` (a **real -`aj compile`** skill) — a skill produced a **~0% token saving** (354 and 19–35 tokens on ~236k / ~93k -baselines) at equal (100%) success. With any realistic compile cost, **break-even is thousands of -invocations — i.e. not worth compiling for these shapes.** +Across three repetition workflows — `nullcheck` (hand-written skill), `shellseq` (a **real `aj compile`** +skill), and `migrate` (exploration-heavy) — giving the agent a skill produced **no meaningful token +saving, and on the exploration-heavy shape it was ~25% WORSE** (the skill's context cost outweighed what +it saved). All at 100% iso-accuracy. **Conclusion: "repetitive workflow → compile a skill" is not a safe +default** — injecting a skill is not free, and for these Claude-Code-shaped tasks its context cost tends +to swamp the savings. This is a *useful* result, not a disappointing one: the benchmark exists to answer "does a skill -actually save tokens, and after how many uses does it pay off?" — and here the honest answer is "no, -not on a trivial edit or a short command sequence." It holds for a genuinely compiled skill, not just a +actually save tokens, and after how many uses does it pay off?" — and the honest, measured answer across +these shapes is "no, and sometimes it hurts." It holds for a genuinely compiled skill, not just a hand-written stand-in. The methodology (measure at iso-accuracy from API-reported usage, never estimate) -is what makes that answer trustworthy. The open question is whether an **exploration-heavy** workflow — -where the baseline burns tokens *finding* what to change — flips the result. +is what makes the answer trustworthy. The open question is whether a workflow with *much* larger +per-episode work, or a skill that genuinely removes reading rather than guiding it, ever flips this. ## Method (as implemented) @@ -62,12 +64,15 @@ aj bench --gen nullcheck --n 2 --arm baseline T2S 235,999 (2 guards, verified - **This is the intended signal.** Per the design, the headline is amortized break-even *stratified by shape*, not a global average. `nullcheck` is one (low-value) point on that curve. -## Where a skill *should* pay off (hypothesis) +## Where a skill *should* pay off (hypothesis — since TESTED, see `migrate` below) -JIT value comes from skipping **exploration**, not from shortening a known edit. `nullcheck` needs -almost no exploration, so there is nothing to save. A skill should show a real delta on workflows where -the baseline burns tokens *finding* what to change — e.g. `migrate-N`: locate every call site of an API -across N files, then apply a mechanical change. That is the natural next fixture. +JIT value should come from skipping **exploration**, not from shortening a known edit. `nullcheck` needs +almost no exploration, so there is nothing to save. The hypothesis was that a skill would show a real +delta on workflows where the baseline burns tokens *finding* what to change — e.g. `migrate-N`: locate +every call site of an API across N files, then apply a mechanical change. + +**This hypothesis was tested with the `migrate` fixture (below) and did NOT hold** — injecting the skill +made it *worse*, not better. See "Update — exploration-heavy `migrate`". ## Update — real `aj compile` skill (`shellseq`, 2026-07-07) @@ -96,6 +101,39 @@ Bash steps, so an Edit-based workflow like `nullcheck` routes to the LLM compile the two sequential episodes, so `--rollouts 1` gives unreliable A/B numbers (a run once showed baseline 0 vs jit 46k). Always use `--rollouts >= 3`; the harness reports mean/median/min/max. +## Update — exploration-heavy `migrate` (2026-07-07): JIT made it WORSE + +`migrate` is the shape the hypothesis predicted a win for: N Go files each call a deprecated `OldName` +buried among distractor helpers, and the task is to migrate every `OldName` call to `NewName`. The agent +must **search/read across files** to find the call sites before editing — the exploration a skill was +meant to remove. (Edit-based, so the JIT arm uses a hand-written skill naming the exact rename.) + +Measured at `--rollouts 3`: + +``` +migrate n=3: baseline 190,635 (med 190,036) vs jit 238,512 (med 238,183) → saving −47,877/use +``` + +Both 100% at iso-accuracy (both migrated correctly). **The JIT arm was ~25% WORSE, not better** — +consistent across rollouts (mean ≈ median). The hypothesis is refuted for this configuration. + +**Why:** injecting the skill *adds* context tokens, and the agent still reads the files to apply the +edits — so the skill did not remove the exploration; it piled overhead on top of it. A naive +project-local skill can be **net-negative even on an exploration-heavy task**. + +**Cross-shape summary (all at iso-accuracy):** + +| shape | skill source | per-use token delta | +|---|---|---| +| `nullcheck` | hand-written | ~0% (−354 tok on ~236k) | +| `shellseq` | real `aj compile` | ~0% (−19..35 tok on ~93k) | +| `migrate` | hand-written | **+25% (JIT ~48k MORE)** | + +**Takeaway:** injecting a skill is not free, and for these Claude-Code-shaped tasks the skill's context +cost tends to swamp what it saves — sometimes badly. "Repetitive workflow → compile a skill" is *not* +a safe default; the value case has to be targeted much more carefully (bigger per-episode work, or a +skill that genuinely removes reading, not just guides it). + ## Reproducing ``` diff --git a/internal/bench/fixture.go b/internal/bench/fixture.go index 78033b1..f455dcd 100644 --- a/internal/bench/fixture.go +++ b/internal/bench/fixture.go @@ -11,6 +11,7 @@ import ( var fixtures = map[string]Fixture{ "nullcheck": NullCheckFixture{}, "shellseq": ShellSeqFixture{}, + "migrate": MigrateFixture{}, } // FixtureByShape returns the built-in fixture for a shape name. diff --git a/internal/bench/migrate_fixture.go b/internal/bench/migrate_fixture.go new file mode 100644 index 0000000..658b80f --- /dev/null +++ b/internal/bench/migrate_fixture.go @@ -0,0 +1,115 @@ +package bench + +import ( + "fmt" + "os" + "path/filepath" +) + +// MigrateFixture generates N Go files that each call a deprecated function +// OldName, buried among unrelated helper code so the call sites are not obvious. +// The workflow is "migrate every OldName call to NewName" — an EXPLORATION-heavy +// task: unlike nullcheck/shellseq, the agent must first FIND all the call sites +// (grep/read across files) before editing. This is the shape where a JIT skill +// might actually save tokens, by skipping that exploration. +// +// It is Edit-based, so (like nullcheck) it is not deterministically compilable; +// the JIT arm uses a hand-written skill that names the exact rename. +type MigrateFixture struct{} + +func (MigrateFixture) Shape() string { return "migrate" } + +func (MigrateFixture) Generate(dir string, n int) (Task, error) { + if n < 1 { + return Task{}, fmt.Errorf("migrate fixture needs n >= 1, got %d", n) + } + target := filepath.Join(dir, "migrate") + if err := os.MkdirAll(target, 0o755); err != nil { + return Task{}, err + } + if err := os.WriteFile(filepath.Join(target, "go.mod"), []byte("module migratebench\n\ngo 1.22\n"), 0o644); err != nil { + return Task{}, err + } + // The deprecated + replacement API live in api.go so both names resolve. + api := `package migratebench + +// OldName is the deprecated API. NewName replaces it. +func OldName(x int) int { return x + 1 } + +// NewName is the replacement. +func NewName(x int) int { return x + 1 } +` + if err := os.WriteFile(filepath.Join(target, "api.go"), []byte(api), 0o644); err != nil { + return Task{}, err + } + + // N files, each with the OldName call buried after some distractor helpers so + // it can't be found without reading/searching. + for i := 0; i < n; i++ { + src := fmt.Sprintf(`package migratebench + +// helperA%d is unrelated padding so the call site isn't the first line. +func helperA%d(a, b int) int { return a*b - a } + +// helperB%d is more unrelated padding. +func helperB%d(s string) int { return len(s) * 2 } + +// Compute%d uses the deprecated API somewhere in the middle of the file. +func Compute%d(v int) int { + t := helperA%d(v, 3) + u := OldName(t) + return u + helperB%d("padding") +} +`, i, i, i, i, i, i, i, i) + if err := os.WriteFile(filepath.Join(target, fmt.Sprintf("mod%d.go", i)), []byte(src), 0o644); err != nil { + return Task{}, err + } + } + + // Dual gate: build passes, no OldName( calls remain, and exactly n NewName( + // call sites exist. Scope to *.go so an installed SKILL.md can't skew counts. + // api.go defines both funcs (func OldName/func NewName), so count *calls*: + // "OldName(" excluding the definition line, and "NewName(" call sites == n+1 + // (n calls + the definition). Simpler: require zero "= OldName(" style calls. + verify := fmt.Sprintf( + `go build ./... `+ + `&& test "$(grep -rho --include='*.go' 'OldName(' . | wc -l | tr -d '[:space:]')" = "1" `+ + `&& test "$(grep -rho --include='*.go' 'NewName(' . | wc -l | tr -d '[:space:]')" = "%d"`, + n+1) + + return Task{ + ID: fmt.Sprintf("migrate-%d", n), + Shape: "migrate", + RepoDir: target, + Prompt: fmt.Sprintf( + "This Go package has %d files that call the deprecated function OldName. "+ + "Find every call to OldName across the package and change it to NewName "+ + "(same signature). Do not remove the OldName/NewName definitions in api.go. "+ + "Keep the package building.", n), + Verify: Verification{Command: []string{"sh", "-c", verify}}, + }, nil +} + +// InstallSkill writes a hand-written skill naming the exact rename, so the JIT +// arm can skip the exploration (finding call sites) that dominates the baseline. +func (MigrateFixture) InstallSkill(task Task) error { + skillDir := filepath.Join(task.RepoDir, ".claude", "skills", "migrate-oldname") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + return err + } + skill := "---\n" + + "name: migrate-oldname\n" + + "description: Migrate deprecated OldName calls to NewName across the package.\n" + + "---\n\n" + + "# migrate-oldname\n\n" + + "The call sites are in the `ComputeK` functions in files `mod0.go .. modK.go`. In each, the line\n\n" + + "```go\n" + + "\tu := OldName(t)\n" + + "```\n\n" + + "becomes\n\n" + + "```go\n" + + "\tu := NewName(t)\n" + + "```\n\n" + + "Do not touch api.go (it defines both functions). Then ensure the package builds.\n" + return os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(skill), 0o644) +} diff --git a/internal/bench/migrate_fixture_test.go b/internal/bench/migrate_fixture_test.go new file mode 100644 index 0000000..9e3b157 --- /dev/null +++ b/internal/bench/migrate_fixture_test.go @@ -0,0 +1,105 @@ +package bench + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +func TestMigrateFixtureGenerate(t *testing.T) { + dir := t.TempDir() + task, err := MigrateFixture{}.Generate(dir, 3) + if err != nil { + t.Fatalf("Generate: %v", err) + } + if task.ID != "migrate-3" || task.Shape != "migrate" { + t.Errorf("task = %+v", task) + } + for i := 0; i < 3; i++ { + if _, err := os.Stat(filepath.Join(task.RepoDir, fmt.Sprintf("mod%d.go", i))); err != nil { + t.Errorf("mod%d.go missing: %v", i, err) + } + } + if _, err := os.Stat(filepath.Join(task.RepoDir, "api.go")); err != nil { + t.Errorf("api.go missing: %v", err) + } +} + +// The dual gate must FAIL on the unmodified fixture: OldName calls still present. +func TestMigrateVerifierFailsBeforeFix(t *testing.T) { + if _, err := exec.LookPath("go"); err != nil { + t.Skip("go toolchain not available") + } + dir := t.TempDir() + task, err := MigrateFixture{}.Generate(dir, 2) + if err != nil { + t.Fatal(err) + } + ok, err := CommandVerifier{}.Verify(context.Background(), task) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if ok { + t.Error("verifier passed on unmodified fixture; OldName calls still present") + } +} + +// After migrating every OldName call to NewName, the dual gate must PASS. +func TestMigrateVerifierPassesAfterFix(t *testing.T) { + if _, err := exec.LookPath("go"); err != nil { + t.Skip("go toolchain not available") + } + dir := t.TempDir() + task, err := MigrateFixture{}.Generate(dir, 2) + if err != nil { + t.Fatal(err) + } + // Simulate the migration in the mod*.go files (not api.go). + entries, _ := os.ReadDir(task.RepoDir) + for _, e := range entries { + if !strings.HasPrefix(e.Name(), "mod") { + continue + } + p := filepath.Join(task.RepoDir, e.Name()) + b, _ := os.ReadFile(p) + fixed := strings.Replace(string(b), "OldName(t)", "NewName(t)", 1) + if err := os.WriteFile(p, []byte(fixed), 0o644); err != nil { + t.Fatal(err) + } + } + ok, err := CommandVerifier{}.Verify(context.Background(), task) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if !ok { + t.Error("verifier failed after migrating all call sites; expected pass") + } +} + +// The hand-written skill must not fool the verifier: its SKILL.md contains +// NewName( in an example, but the grep is scoped to *.go. +func TestMigrateInstallSkillDoesNotContaminate(t *testing.T) { + if _, err := exec.LookPath("go"); err != nil { + t.Skip("go toolchain not available") + } + dir := t.TempDir() + task, err := MigrateFixture{}.Generate(dir, 2) + if err != nil { + t.Fatal(err) + } + if err := (MigrateFixture{}).InstallSkill(task); err != nil { + t.Fatalf("InstallSkill: %v", err) + } + // Still fails: code is unmodified regardless of the skill's example text. + ok, err := CommandVerifier{}.Verify(context.Background(), task) + if err != nil { + t.Fatalf("Verify: %v", err) + } + if ok { + t.Error("verifier passed due to SKILL.md contamination; grep must exclude non-.go files") + } +}