From 8dbe7df5b92bc7469c943b4087705cc2eb24cc2f Mon Sep 17 00:00:00 2001 From: guy oron Date: Sun, 2 Aug 2026 16:24:30 +0300 Subject: [PATCH 1/9] test(eval): add 002-dead-config-field functional test case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a second code eval case that tests cross-file dead-code removal. The fixture is a Go project where Config.VerboseLogging is declared, defaulted, parsed, and tested — but never read by any consumer. The agent must trace references across config.go, fields.go, and config_test.go to remove it cleanly. This tests multi-file symbol-tracing reasoning, a step up from 001-fix-add's single-line arithmetic fix. The pattern is inspired by fullsend-ai/fullsend#5808, which produced identical fixes across different model configurations in A/B benchmarking (N=13 pairs). Signed-off-by: guy oron Co-Authored-By: Claude Opus 4.6 --- .../002-dead-config-field/annotations.yaml | 22 +++++ .../cases/002-dead-config-field/input.yaml | 24 ++++++ eval/code/cases/002-dead-config-field/repo | 1 + eval/code/repos/taskrunner/README.md | 3 + eval/code/repos/taskrunner/config/config.go | 47 ++++++++++ .../repos/taskrunner/config/config_test.go | 79 +++++++++++++++++ eval/code/repos/taskrunner/config/fields.go | 34 ++++++++ .../taskrunner/config/internal/yaml/yaml.go | 86 +++++++++++++++++++ eval/code/repos/taskrunner/go.mod | 3 + eval/code/repos/taskrunner/runner/runner.go | 74 ++++++++++++++++ 10 files changed, 373 insertions(+) create mode 100644 eval/code/cases/002-dead-config-field/annotations.yaml create mode 100644 eval/code/cases/002-dead-config-field/input.yaml create mode 120000 eval/code/cases/002-dead-config-field/repo create mode 100644 eval/code/repos/taskrunner/README.md create mode 100644 eval/code/repos/taskrunner/config/config.go create mode 100644 eval/code/repos/taskrunner/config/config_test.go create mode 100644 eval/code/repos/taskrunner/config/fields.go create mode 100644 eval/code/repos/taskrunner/config/internal/yaml/yaml.go create mode 100644 eval/code/repos/taskrunner/go.mod create mode 100644 eval/code/repos/taskrunner/runner/runner.go diff --git a/eval/code/cases/002-dead-config-field/annotations.yaml b/eval/code/cases/002-dead-config-field/annotations.yaml new file mode 100644 index 00000000..fe9fde4b --- /dev/null +++ b/eval/code/cases/002-dead-config-field/annotations.yaml @@ -0,0 +1,22 @@ +state: open + +expected_files: + - config/config.go + - config/fields.go + - config/config_test.go + +labels: + forbidden: [] + +max_turns: 60 +max_cost_usd: 4.00 + +code_expectations: | + The repo has a dead config field: Config.VerboseLogging is declared, defaulted, + parsed, and tested, but never read by any consumer (runner/runner.go doesn't + use it). A successful run creates a PR that removes VerboseLogging from: the + struct definition (config.go), the Defaults() return value (config.go), the + SetField() switch case (fields.go), and all test assertions (config_test.go). + Tests must still pass after removal. This case tests cross-file dead-code + removal — the agent must trace symbol references across multiple files to + determine what to change, not just fix a single line. diff --git a/eval/code/cases/002-dead-config-field/input.yaml b/eval/code/cases/002-dead-config-field/input.yaml new file mode 100644 index 00000000..133fdec0 --- /dev/null +++ b/eval/code/cases/002-dead-config-field/input.yaml @@ -0,0 +1,24 @@ +forge: github +fixture: + type: issue + title: "config: VerboseLogging field is parsed but never consumed" + body: | + ## Bug Report + + **What happened:** + `Config.VerboseLogging` (`config/config.go`, YAML key `verbose_logging`) is declared + in the struct, defaulted in `Defaults()`, parsed via `SetField()` in `config/fields.go`, + and round-tripped in tests (`config_test.go`). No code anywhere reads the value — + `runner.Run()` never checks it, there is no conditional log output, nothing branches + on it. + + **What should happen:** + The field should be removed as dead code. Remove the struct field, the default value, + the `SetField` case, and the test assertions that exercise it. + + **Steps to verify:** + 1. `grep -rn VerboseLogging` across the repo — only config/ files reference it + 2. `runner/runner.go` comments mention it but never reads it + 3. After removal, `go test ./...` should still pass + + Please remove `VerboseLogging` as dead config and keep all tests passing. diff --git a/eval/code/cases/002-dead-config-field/repo b/eval/code/cases/002-dead-config-field/repo new file mode 120000 index 00000000..2b3908fe --- /dev/null +++ b/eval/code/cases/002-dead-config-field/repo @@ -0,0 +1 @@ +../../repos/taskrunner \ No newline at end of file diff --git a/eval/code/repos/taskrunner/README.md b/eval/code/repos/taskrunner/README.md new file mode 100644 index 00000000..a9a80f5c --- /dev/null +++ b/eval/code/repos/taskrunner/README.md @@ -0,0 +1,3 @@ +# taskrunner + +A minimal task runner that reads a YAML config and executes registered tasks. diff --git a/eval/code/repos/taskrunner/config/config.go b/eval/code/repos/taskrunner/config/config.go new file mode 100644 index 00000000..4861bd2c --- /dev/null +++ b/eval/code/repos/taskrunner/config/config.go @@ -0,0 +1,47 @@ +package config + +import ( + "fmt" + "os" + + "github.com/eval-org/taskrunner/config/internal/yaml" +) + +// Config holds the task runner configuration. +type Config struct { + // MaxRetries controls how many times a failed task is retried. + MaxRetries int `yaml:"max_retries"` + + // Timeout is the per-task timeout in seconds. + Timeout int `yaml:"timeout"` + + // VerboseLogging enables detailed debug output. + VerboseLogging bool `yaml:"verbose_logging"` + + // Workers is the number of concurrent task workers. + Workers int `yaml:"workers"` +} + +// Defaults returns a Config with sensible default values. +func Defaults() Config { + return Config{ + MaxRetries: 3, + Timeout: 60, + VerboseLogging: false, + Workers: 4, + } +} + +// Load reads a YAML config file and returns a Config. +// Missing fields are filled with defaults. +func Load(path string) (Config, error) { + data, err := os.ReadFile(path) + if err != nil { + return Config{}, fmt.Errorf("reading config %s: %w", path, err) + } + cfg := Defaults() + if err := yaml.Unmarshal(data, &cfg); err != nil { + return Config{}, fmt.Errorf("parsing config %s: %w", path, err) + } + return cfg, nil +} diff --git a/eval/code/repos/taskrunner/config/config_test.go b/eval/code/repos/taskrunner/config/config_test.go new file mode 100644 index 00000000..e17e0cee --- /dev/null +++ b/eval/code/repos/taskrunner/config/config_test.go @@ -0,0 +1,79 @@ +package config + +import ( + "os" + "path/filepath" + "testing" +) + +func TestDefaults(t *testing.T) { + cfg := Defaults() + if cfg.MaxRetries != 3 { + t.Errorf("MaxRetries = %d, want 3", cfg.MaxRetries) + } + if cfg.Timeout != 60 { + t.Errorf("Timeout = %d, want 60", cfg.Timeout) + } + if cfg.VerboseLogging != false { + t.Errorf("VerboseLogging = %v, want false", cfg.VerboseLogging) + } + if cfg.Workers != 4 { + t.Errorf("Workers = %d, want 4", cfg.Workers) + } +} + +func TestLoad(t *testing.T) { + content := `max_retries: 5 +timeout: 120 +verbose_logging: true +workers: 8 +` + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + + if cfg.MaxRetries != 5 { + t.Errorf("MaxRetries = %d, want 5", cfg.MaxRetries) + } + if cfg.Timeout != 120 { + t.Errorf("Timeout = %d, want 120", cfg.Timeout) + } + if cfg.VerboseLogging != true { + t.Errorf("VerboseLogging = %v, want true", cfg.VerboseLogging) + } + if cfg.Workers != 8 { + t.Errorf("Workers = %d, want 8", cfg.Workers) + } +} + +func TestLoadPartial(t *testing.T) { + content := `timeout: 30 +` + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := Load(path) + if err != nil { + t.Fatal(err) + } + + if cfg.MaxRetries != 3 { + t.Errorf("MaxRetries = %d, want 3 (default)", cfg.MaxRetries) + } + if cfg.Timeout != 30 { + t.Errorf("Timeout = %d, want 30", cfg.Timeout) + } + if cfg.VerboseLogging != false { + t.Errorf("VerboseLogging = %v, want false (default)", cfg.VerboseLogging) + } +} diff --git a/eval/code/repos/taskrunner/config/fields.go b/eval/code/repos/taskrunner/config/fields.go new file mode 100644 index 00000000..55dfbe9d --- /dev/null +++ b/eval/code/repos/taskrunner/config/fields.go @@ -0,0 +1,34 @@ +package config + +import "github.com/eval-org/taskrunner/config/internal/yaml" + +// SetField implements the configFields interface for the minimal YAML parser. +func (c *Config) SetField(key, value string) error { + switch key { + case "max_retries": + v, err := yaml.ParseInt(value) + if err != nil { + return err + } + c.MaxRetries = v + case "timeout": + v, err := yaml.ParseInt(value) + if err != nil { + return err + } + c.Timeout = v + case "verbose_logging": + v, err := yaml.ParseBool(value) + if err != nil { + return err + } + c.VerboseLogging = v + case "workers": + v, err := yaml.ParseInt(value) + if err != nil { + return err + } + c.Workers = v + } + return nil +} diff --git a/eval/code/repos/taskrunner/config/internal/yaml/yaml.go b/eval/code/repos/taskrunner/config/internal/yaml/yaml.go new file mode 100644 index 00000000..90f3a971 --- /dev/null +++ b/eval/code/repos/taskrunner/config/internal/yaml/yaml.go @@ -0,0 +1,86 @@ +package yaml + +import ( + "fmt" + "regexp" + "strconv" + "strings" +) + +// Unmarshal is a minimal YAML parser for flat key-value configs. +// It supports string, int, and bool values only. +func Unmarshal(data []byte, v interface{}) error { + lines := strings.Split(string(data), "\n") + kvs := make(map[string]string) + re := regexp.MustCompile(`^(\w+):\s*(.+)$`) + for _, line := range lines { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + m := re.FindStringSubmatch(line) + if m == nil { + continue + } + kvs[m[1]] = m[2] + } + return applyToStruct(kvs, v) +} + +func applyToStruct(kvs map[string]string, v interface{}) error { + type configFields interface { + SetField(key, value string) error + } + if s, ok := v.(configFields); ok { + for k, val := range kvs { + if err := s.SetField(k, val); err != nil { + return err + } + } + return nil + } + return applyReflect(kvs, v) +} + +func applyReflect(kvs map[string]string, v interface{}) error { + type yamlField struct { + Name string + Set func(string) error + } + + cfg, ok := v.(interface { + YAMLFields() []struct { + Key string + Set func(string) error + } + }) + if !ok { + return fmt.Errorf("target does not implement YAMLFields or configFields") + } + + for _, f := range cfg.YAMLFields() { + if val, exists := kvs[f.Key]; exists { + if err := f.Set(val); err != nil { + return fmt.Errorf("setting %s: %w", f.Key, err) + } + } + } + return nil +} + +// ParseBool parses a YAML boolean string. +func ParseBool(s string) (bool, error) { + switch strings.ToLower(strings.TrimSpace(s)) { + case "true", "yes", "on": + return true, nil + case "false", "no", "off": + return false, nil + default: + return false, fmt.Errorf("invalid bool: %q", s) + } +} + +// ParseInt parses a YAML integer string. +func ParseInt(s string) (int, error) { + return strconv.Atoi(strings.TrimSpace(s)) +} diff --git a/eval/code/repos/taskrunner/go.mod b/eval/code/repos/taskrunner/go.mod new file mode 100644 index 00000000..29ad2e62 --- /dev/null +++ b/eval/code/repos/taskrunner/go.mod @@ -0,0 +1,3 @@ +module github.com/eval-org/taskrunner + +go 1.22 diff --git a/eval/code/repos/taskrunner/runner/runner.go b/eval/code/repos/taskrunner/runner/runner.go new file mode 100644 index 00000000..05f0d05c --- /dev/null +++ b/eval/code/repos/taskrunner/runner/runner.go @@ -0,0 +1,74 @@ +package runner + +import ( + "fmt" + "time" + + "github.com/eval-org/taskrunner/config" +) + +// Task represents a unit of work to execute. +type Task struct { + Name string + Fn func() error +} + +// Runner executes tasks according to the provided configuration. +type Runner struct { + cfg config.Config + tasks []Task +} + +// New creates a Runner with the given configuration. +func New(cfg config.Config) *Runner { + return &Runner{cfg: cfg} +} + +// Register adds a task to the runner. +func (r *Runner) Register(t Task) { + r.tasks = append(r.tasks, t) +} + +// Run executes all registered tasks with retry and timeout logic. +// It uses cfg.MaxRetries, cfg.Timeout, and cfg.Workers. +// Note: cfg.VerboseLogging is not checked anywhere — this is dead config. +func (r *Runner) Run() error { + sem := make(chan struct{}, r.cfg.Workers) + errs := make(chan error, len(r.tasks)) + + for _, task := range r.tasks { + sem <- struct{}{} + go func(t Task) { + defer func() { <-sem }() + errs <- r.runWithRetry(t) + }(task) + } + + for range r.tasks { + if err := <-errs; err != nil { + return err + } + } + return nil +} + +func (r *Runner) runWithRetry(t Task) error { + timeout := time.Duration(r.cfg.Timeout) * time.Second + var lastErr error + + for attempt := 0; attempt <= r.cfg.MaxRetries; attempt++ { + done := make(chan error, 1) + go func() { done <- t.Fn() }() + + select { + case err := <-done: + if err == nil { + return nil + } + lastErr = err + case <-time.After(timeout): + lastErr = fmt.Errorf("task %s timed out after %v", t.Name, timeout) + } + } + return fmt.Errorf("task %s failed after %d retries: %w", t.Name, r.cfg.MaxRetries, lastErr) +} From ae74c639a90e71af66847f6238a1139fad7acf41 Mon Sep 17 00:00:00 2001 From: guy oron Date: Tue, 4 Aug 2026 07:09:00 +0300 Subject: [PATCH 2/9] test(eval): address PR review feedback and fix fixture bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compliance & Documentation: - Remove agent-instruction language ("the agent must" → "tracing") - Add "Human reference only" disclaimer to code_expectations - Document budget as placeholder needing CI baseline Fixture Improvements: - Remove spoiler comment from runner.go (defeats test purpose) - Rewrite input.yaml as symptom-based bug report (no grep command/answer) - Delete unreachable applyReflect code from yaml.go (25 lines) Bug Fixes: - Fix timeout retry overlap via context.Context cancellation - Add config validation (Workers/Timeout/MaxRetries bounds) - Error on unknown config keys and malformed YAML lines Addresses feedback from waynesun09 and qodo-code-review. Signed-off-by: guy oron --- docs/fix.md | 1 + .../002-dead-config-field/annotations.yaml | 9 +++-- .../cases/002-dead-config-field/input.yaml | 29 ++++++++-------- eval/code/repos/taskrunner/config/fields.go | 8 ++++- .../taskrunner/config/internal/yaml/yaml.go | 32 ++---------------- eval/code/repos/taskrunner/runner/runner.go | 33 +++++++++++-------- 6 files changed, 52 insertions(+), 60 deletions(-) diff --git a/docs/fix.md b/docs/fix.md index 185bc522..e37a9756 100644 --- a/docs/fix.md +++ b/docs/fix.md @@ -160,6 +160,7 @@ The fix agent enforces iteration caps to prevent infinite review-fix loops: If this agent needs to reach hosts beyond the defaults, see the [custom network policy guide](network-policy.md). + ## Source [`harness/fix.yaml`](../harness/fix.yaml) diff --git a/eval/code/cases/002-dead-config-field/annotations.yaml b/eval/code/cases/002-dead-config-field/annotations.yaml index fe9fde4b..9192f65f 100644 --- a/eval/code/cases/002-dead-config-field/annotations.yaml +++ b/eval/code/cases/002-dead-config-field/annotations.yaml @@ -8,9 +8,14 @@ expected_files: labels: forbidden: [] +# Code agent budgets (sandbox work + retries). +# TODO: establish baseline via CI run — values copied from 001-fix-add as +# placeholder. 002 is expected to be harder (multi-file tracing vs single-line +# fix), so these may need adjustment upward after observing real runs. max_turns: 60 max_cost_usd: 4.00 +# Human reference only; not consumed by judges (unlike triage/review quality). code_expectations: | The repo has a dead config field: Config.VerboseLogging is declared, defaulted, parsed, and tested, but never read by any consumer (runner/runner.go doesn't @@ -18,5 +23,5 @@ code_expectations: | struct definition (config.go), the Defaults() return value (config.go), the SetField() switch case (fields.go), and all test assertions (config_test.go). Tests must still pass after removal. This case tests cross-file dead-code - removal — the agent must trace symbol references across multiple files to - determine what to change, not just fix a single line. + removal — tracing symbol references across multiple files to determine what + to change, not just fixing a single line. diff --git a/eval/code/cases/002-dead-config-field/input.yaml b/eval/code/cases/002-dead-config-field/input.yaml index 133fdec0..e2c330e1 100644 --- a/eval/code/cases/002-dead-config-field/input.yaml +++ b/eval/code/cases/002-dead-config-field/input.yaml @@ -1,24 +1,25 @@ forge: github fixture: type: issue - title: "config: VerboseLogging field is parsed but never consumed" + title: "config: VerboseLogging field appears unused" body: | ## Bug Report **What happened:** - `Config.VerboseLogging` (`config/config.go`, YAML key `verbose_logging`) is declared - in the struct, defaulted in `Defaults()`, parsed via `SetField()` in `config/fields.go`, - and round-tripped in tests (`config_test.go`). No code anywhere reads the value — - `runner.Run()` never checks it, there is no conditional log output, nothing branches - on it. + We have a `VerboseLogging` field in the config that I can't find being used anywhere + in the actual runner logic. I added it a while back thinking we'd need it for debug + output, but I'm not sure we ever wired it up to anything. - **What should happen:** - The field should be removed as dead code. Remove the struct field, the default value, - the `SetField` case, and the test assertions that exercise it. + **Expected behavior:** + If it's truly unused, we should remove it to keep the config clean. But I want to + make sure I'm not missing something — maybe there's some code path that reads it + that I didn't spot? - **Steps to verify:** - 1. `grep -rn VerboseLogging` across the repo — only config/ files reference it - 2. `runner/runner.go` comments mention it but never reads it - 3. After removal, `go test ./...` should still pass + **Additional context:** + - The field exists in `config/config.go` + - YAML key is `verbose_logging` + - I see tests for it, but those might just be testing the config parsing itself + - Haven't found where the runner actually checks this value, but I might be wrong - Please remove `VerboseLogging` as dead config and keep all tests passing. + Can someone verify whether this is actually used? If not, please clean it up. + Make sure tests still pass after any changes. diff --git a/eval/code/repos/taskrunner/config/fields.go b/eval/code/repos/taskrunner/config/fields.go index 55dfbe9d..00964605 100644 --- a/eval/code/repos/taskrunner/config/fields.go +++ b/eval/code/repos/taskrunner/config/fields.go @@ -1,6 +1,10 @@ package config -import "github.com/eval-org/taskrunner/config/internal/yaml" +import ( + "fmt" + + "github.com/eval-org/taskrunner/config/internal/yaml" +) // SetField implements the configFields interface for the minimal YAML parser. func (c *Config) SetField(key, value string) error { @@ -29,6 +33,8 @@ func (c *Config) SetField(key, value string) error { return err } c.Workers = v + default: + return fmt.Errorf("unknown config key: %s", key) } return nil } diff --git a/eval/code/repos/taskrunner/config/internal/yaml/yaml.go b/eval/code/repos/taskrunner/config/internal/yaml/yaml.go index 90f3a971..0de49b68 100644 --- a/eval/code/repos/taskrunner/config/internal/yaml/yaml.go +++ b/eval/code/repos/taskrunner/config/internal/yaml/yaml.go @@ -13,14 +13,14 @@ func Unmarshal(data []byte, v interface{}) error { lines := strings.Split(string(data), "\n") kvs := make(map[string]string) re := regexp.MustCompile(`^(\w+):\s*(.+)$`) - for _, line := range lines { + for i, line := range lines { line = strings.TrimSpace(line) if line == "" || strings.HasPrefix(line, "#") { continue } m := re.FindStringSubmatch(line) if m == nil { - continue + return fmt.Errorf("line %d: malformed YAML (expected 'key: value'): %q", i+1, line) } kvs[m[1]] = m[2] } @@ -39,33 +39,7 @@ func applyToStruct(kvs map[string]string, v interface{}) error { } return nil } - return applyReflect(kvs, v) -} - -func applyReflect(kvs map[string]string, v interface{}) error { - type yamlField struct { - Name string - Set func(string) error - } - - cfg, ok := v.(interface { - YAMLFields() []struct { - Key string - Set func(string) error - } - }) - if !ok { - return fmt.Errorf("target does not implement YAMLFields or configFields") - } - - for _, f := range cfg.YAMLFields() { - if val, exists := kvs[f.Key]; exists { - if err := f.Set(val); err != nil { - return fmt.Errorf("setting %s: %w", f.Key, err) - } - } - } - return nil + return fmt.Errorf("target does not implement configFields interface") } // ParseBool parses a YAML boolean string. diff --git a/eval/code/repos/taskrunner/runner/runner.go b/eval/code/repos/taskrunner/runner/runner.go index 05f0d05c..488cc17d 100644 --- a/eval/code/repos/taskrunner/runner/runner.go +++ b/eval/code/repos/taskrunner/runner/runner.go @@ -1,6 +1,7 @@ package runner import ( + "context" "fmt" "time" @@ -10,7 +11,7 @@ import ( // Task represents a unit of work to execute. type Task struct { Name string - Fn func() error + Fn func(context.Context) error } // Runner executes tasks according to the provided configuration. @@ -20,8 +21,17 @@ type Runner struct { } // New creates a Runner with the given configuration. -func New(cfg config.Config) *Runner { - return &Runner{cfg: cfg} +func New(cfg config.Config) (*Runner, error) { + if cfg.Workers < 1 { + return nil, fmt.Errorf("workers must be >= 1, got %d", cfg.Workers) + } + if cfg.Timeout < 1 { + return nil, fmt.Errorf("timeout must be >= 1, got %d", cfg.Timeout) + } + if cfg.MaxRetries < 0 { + return nil, fmt.Errorf("max_retries must be >= 0, got %d", cfg.MaxRetries) + } + return &Runner{cfg: cfg}, nil } // Register adds a task to the runner. @@ -31,7 +41,6 @@ func (r *Runner) Register(t Task) { // Run executes all registered tasks with retry and timeout logic. // It uses cfg.MaxRetries, cfg.Timeout, and cfg.Workers. -// Note: cfg.VerboseLogging is not checked anywhere — this is dead config. func (r *Runner) Run() error { sem := make(chan struct{}, r.cfg.Workers) errs := make(chan error, len(r.tasks)) @@ -57,18 +66,14 @@ func (r *Runner) runWithRetry(t Task) error { var lastErr error for attempt := 0; attempt <= r.cfg.MaxRetries; attempt++ { - done := make(chan error, 1) - go func() { done <- t.Fn() }() + ctx, cancel := context.WithTimeout(context.Background(), timeout) + err := t.Fn(ctx) + cancel() - select { - case err := <-done: - if err == nil { - return nil - } - lastErr = err - case <-time.After(timeout): - lastErr = fmt.Errorf("task %s timed out after %v", t.Name, timeout) + if err == nil { + return nil } + lastErr = err } return fmt.Errorf("task %s failed after %d retries: %w", t.Name, r.cfg.MaxRetries, lastErr) } From 407b98d72f52b5c13b4b1e0d626e8b85dbc6a1e5 Mon Sep 17 00:00:00 2001 From: guy oron Date: Sun, 9 Aug 2026 11:10:35 +0300 Subject: [PATCH 3/9] =?UTF-8?q?test(eval):=20address=20review=20feedback?= =?UTF-8?q?=20=E2=80=94=20budget=20baseline,=20tests,=20code=5Fexpectation?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Budget: - Replace unverifiable baseline-30 reference with Round 4 A/B run data - Cite workflow runs 30729219887 / 30740388070 (#5808, same pattern) - Fix "well under" overclaim — reword to "expect lower cost per run" Tests: - Add runner_test.go: config validation rejection + happy-path Run() - Add config_test.go: malformed YAML line + unknown key error paths Documentation: - Extend code_expectations to mention embedded YAML literal in TestLoad Signed-off-by: guy oron Co-Authored-By: Claude Opus 4.6 Signed-off-by: guy oron --- .../002-dead-config-field/annotations.yaml | 13 ++++-- .../repos/taskrunner/config/config_test.go | 24 ++++++++++ .../repos/taskrunner/runner/runner_test.go | 45 +++++++++++++++++++ 3 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 eval/code/repos/taskrunner/runner/runner_test.go diff --git a/eval/code/cases/002-dead-config-field/annotations.yaml b/eval/code/cases/002-dead-config-field/annotations.yaml index 9192f65f..0fd3a1b0 100644 --- a/eval/code/cases/002-dead-config-field/annotations.yaml +++ b/eval/code/cases/002-dead-config-field/annotations.yaml @@ -9,9 +9,12 @@ labels: forbidden: [] # Code agent budgets (sandbox work + retries). -# TODO: establish baseline via CI run — values copied from 001-fix-add as -# placeholder. 002 is expected to be harder (multi-file tracing vs single-line -# fix), so these may need adjustment upward after observing real runs. +# Reference: Round 4 A/B benchmark (2026-08-02, guyoron1/fullsend). +# Upstream #5808 (same dead-config-removal pattern as this case) completed +# in 75–79 turns / $3.85–$4.02 against the full fullsend codebase (~50k LOC). +# This fixture is ~200 LOC (vs ~50k LOC), so expect lower cost per run. +# Workflow runs: 30729219887 (Path A), 30740388070 (Path B). +# See: guyoron1/fullsend/docs/ab-benchmark-round4/ max_turns: 60 max_cost_usd: 4.00 @@ -24,4 +27,6 @@ code_expectations: | SetField() switch case (fields.go), and all test assertions (config_test.go). Tests must still pass after removal. This case tests cross-file dead-code removal — tracing symbol references across multiple files to determine what - to change, not just fixing a single line. + to change, not just fixing a single line. The raw YAML literal in TestLoad + (verbose_logging: true) must also be removed — SetField rejects unknown keys, + so leaving it breaks Load(). diff --git a/eval/code/repos/taskrunner/config/config_test.go b/eval/code/repos/taskrunner/config/config_test.go index e17e0cee..2043a078 100644 --- a/eval/code/repos/taskrunner/config/config_test.go +++ b/eval/code/repos/taskrunner/config/config_test.go @@ -77,3 +77,27 @@ func TestLoadPartial(t *testing.T) { t.Errorf("VerboseLogging = %v, want false (default)", cfg.VerboseLogging) } } + +func TestLoadMalformedLine(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte("not a valid line\n"), 0644); err != nil { + t.Fatal(err) + } + _, err := Load(path) + if err == nil { + t.Error("Load() = nil error for malformed YAML, want error") + } +} + +func TestLoadUnknownKey(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte("bogus_key: 1\n"), 0644); err != nil { + t.Fatal(err) + } + _, err := Load(path) + if err == nil { + t.Error("Load() = nil error for unknown key, want error") + } +} diff --git a/eval/code/repos/taskrunner/runner/runner_test.go b/eval/code/repos/taskrunner/runner/runner_test.go new file mode 100644 index 00000000..dfb8f86c --- /dev/null +++ b/eval/code/repos/taskrunner/runner/runner_test.go @@ -0,0 +1,45 @@ +package runner + +import ( + "context" + "testing" + + "github.com/eval-org/taskrunner/config" +) + +func TestNewRejectsInvalidConfig(t *testing.T) { + cases := []struct { + name string + cfg config.Config + }{ + {"zero workers", config.Config{Workers: 0, Timeout: 1, MaxRetries: 0}}, + {"negative timeout", config.Config{Workers: 1, Timeout: -1, MaxRetries: 0}}, + {"negative retries", config.Config{Workers: 1, Timeout: 1, MaxRetries: -1}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := New(tc.cfg) + if err == nil { + t.Errorf("New(%+v) = nil error, want error", tc.cfg) + } + }) + } +} + +func TestRunHappyPath(t *testing.T) { + r, err := New(config.Config{Workers: 1, Timeout: 5, MaxRetries: 0}) + if err != nil { + t.Fatal(err) + } + called := false + r.Register(Task{Name: "test", Fn: func(ctx context.Context) error { + called = true + return nil + }}) + if err := r.Run(); err != nil { + t.Fatal(err) + } + if !called { + t.Error("task was not executed") + } +} From 69dfc0e0fbdabbdfcca5cf696b9d96a1c728b9ca Mon Sep 17 00:00:00 2001 From: guy oron Date: Wed, 12 Aug 2026 06:47:11 +0300 Subject: [PATCH 4/9] test(eval): remove field-enumeration spoiler from runner doc comment Reword the Run() doc comment so it no longer lists the exact subset of used config fields, which let an agent identify the dead field by elimination without cross-file tracing. Signed-off-by: guy oron --- eval/code/repos/taskrunner/runner/runner.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eval/code/repos/taskrunner/runner/runner.go b/eval/code/repos/taskrunner/runner/runner.go index 488cc17d..5e666571 100644 --- a/eval/code/repos/taskrunner/runner/runner.go +++ b/eval/code/repos/taskrunner/runner/runner.go @@ -40,7 +40,7 @@ func (r *Runner) Register(t Task) { } // Run executes all registered tasks with retry and timeout logic. -// It uses cfg.MaxRetries, cfg.Timeout, and cfg.Workers. +// It processes tasks concurrently based on the runner's configuration. func (r *Runner) Run() error { sem := make(chan struct{}, r.cfg.Workers) errs := make(chan error, len(r.tasks)) From f3b759295abb9080db8738050d060553c6169d64 Mon Sep 17 00:00:00 2001 From: guy oron Date: Thu, 13 Aug 2026 10:54:45 +0300 Subject: [PATCH 5/9] test(eval): cover runner retry/timeout, ground budget, note 002 in eval.yaml Addresses remaining review feedback on #617: - runner_test.go: add retry-until-success and per-attempt-timeout tests so runWithRetry's context.WithTimeout logic has coverage (was untested). - 002 annotations.yaml: reframe budget as a deliberate ceiling bounded by 001's observed baseline (35 turns/$2.12) and #5808's upper reference, dropping the "expect lower" claim that contradicted the near-ceiling numbers; flag the case-specific CI baseline as follow-up. - eval.yaml: update the timeout rationale to acknowledge 002 now shares the 1700s window with a heavier 60-turn/$4 budget, and note its headroom is an open risk to revisit once a real run exists. Signed-off-by: guy oron --- .../002-dead-config-field/annotations.yaml | 20 +++++++---- eval/code/eval.yaml | 10 ++++-- .../repos/taskrunner/runner/runner_test.go | 36 +++++++++++++++++++ 3 files changed, 57 insertions(+), 9 deletions(-) diff --git a/eval/code/cases/002-dead-config-field/annotations.yaml b/eval/code/cases/002-dead-config-field/annotations.yaml index 0fd3a1b0..83b4b9b7 100644 --- a/eval/code/cases/002-dead-config-field/annotations.yaml +++ b/eval/code/cases/002-dead-config-field/annotations.yaml @@ -8,13 +8,19 @@ expected_files: labels: forbidden: [] -# Code agent budgets (sandbox work + retries). -# Reference: Round 4 A/B benchmark (2026-08-02, guyoron1/fullsend). -# Upstream #5808 (same dead-config-removal pattern as this case) completed -# in 75–79 turns / $3.85–$4.02 against the full fullsend codebase (~50k LOC). -# This fixture is ~200 LOC (vs ~50k LOC), so expect lower cost per run. -# Workflow runs: 30729219887 (Path A), 30740388070 (Path B). -# See: guyoron1/fullsend/docs/ab-benchmark-round4/ +# Code agent budgets (sandbox work + retries). A deliberate ceiling, not a +# prediction — this case has no observed run of its own yet, so the numbers +# are bounded by two reference points rather than measured, and should be +# re-derived from this case's first CI run. +# Lower bound: 001-fix-add (same harness, trivial fixture) observed up to +# 35 turns / $2.12 (CI run 30166455238). This is a harder, cross-file task, +# so expect it to sit above that. +# Upper bound: #5808 (same dead-config-removal pattern, ~50k-LOC codebase) +# ran 75–79 turns / $3.85–$4.02 in the Round 4 A/B benchmark (2026-08-02, +# guyoron1/fullsend; workflow runs 30729219887 / 30740388070). This fixture +# is ~200 LOC, so expect it to stay below that. +# 60 turns / $4.00 sits between the two, matching 001's ceiling for a task +# one step harder. See: guyoron1/fullsend/docs/ab-benchmark-round4/ max_turns: 60 max_cost_usd: 4.00 diff --git a/eval/code/eval.yaml b/eval/code/eval.yaml index 75d262f1..489f881a 100644 --- a/eval/code/eval.yaml +++ b/eval/code/eval.yaml @@ -65,8 +65,14 @@ execution: # timeout envelope; matches eval/fix/eval.yaml's value instead (see # execution.timeout above). # - # This fixture is a 2-line arithmetic bug and completes in well under a - # minute in practice, so this number essentially never fires. If a + # Two cases now share this window: 001-fix-add (a 2-line arithmetic bug + # that completes in well under a minute) and 002-dead-config-field (a + # cross-file dead-config removal with a larger 60-turn / $4.00 budget). + # Neither is expected to approach 1700s in practice — the code agent's + # own per-iteration budget (2100s, harness/code.yaml) bounds each run + # well before this backstop — so this number should still essentially + # never fire. 002 has no observed CI runtime yet, so treat its headroom + # as an open risk to revisit once a real run exists. If a # genuine hang did occur: a single-iteration hang gets caught here and # fullsend still writes a partial metrics.json before returning (see # writeMetricsJSON in run.go's error path), giving an inconclusive but diff --git a/eval/code/repos/taskrunner/runner/runner_test.go b/eval/code/repos/taskrunner/runner/runner_test.go index dfb8f86c..047efb37 100644 --- a/eval/code/repos/taskrunner/runner/runner_test.go +++ b/eval/code/repos/taskrunner/runner/runner_test.go @@ -2,6 +2,8 @@ package runner import ( "context" + "errors" + "sync/atomic" "testing" "github.com/eval-org/taskrunner/config" @@ -43,3 +45,37 @@ func TestRunHappyPath(t *testing.T) { t.Error("task was not executed") } } + +func TestRunRetriesUntilSuccess(t *testing.T) { + r, err := New(config.Config{Workers: 1, Timeout: 5, MaxRetries: 3}) + if err != nil { + t.Fatal(err) + } + var attempts int32 + r.Register(Task{Name: "flaky", Fn: func(ctx context.Context) error { + if atomic.AddInt32(&attempts, 1) < 3 { + return errors.New("transient") + } + return nil + }}) + if err := r.Run(); err != nil { + t.Fatalf("Run() = %v, want nil after retries", err) + } + if got := atomic.LoadInt32(&attempts); got != 3 { + t.Errorf("attempts = %d, want 3", got) + } +} + +func TestRunTimeoutCancelsAttempt(t *testing.T) { + r, err := New(config.Config{Workers: 1, Timeout: 1, MaxRetries: 0}) + if err != nil { + t.Fatal(err) + } + r.Register(Task{Name: "hang", Fn: func(ctx context.Context) error { + <-ctx.Done() + return ctx.Err() + }}) + if err := r.Run(); err == nil { + t.Fatal("Run() = nil, want error from timed-out task") + } +} From ea546156508b96524f3bfdb63767bf42e6b6e33a Mon Sep 17 00:00:00 2001 From: guy oron Date: Thu, 13 Aug 2026 17:24:49 +0300 Subject: [PATCH 6/9] test(eval): correct budget/timeout comments, drop stray docs change Fix two inaccurate justification comments flagged in review and remove an unrelated whitespace change: - annotations.yaml: the budget comment merged two different 001-fix-add CI runs into one citation (35 turns from run 30166455238, $2.12 from run 29424512121). Cite both runs' real numbers separately and drop the #5808-derived upper bound; state plainly this case's budget is an unmeasured ceiling to re-derive from its first CI run. - eval.yaml: the EVAL_TIMEOUT rationale claimed the 2100s per-iteration budget bounds the 1700s outer timeout, which is backwards (2100 > 1700). Clarify that the 1700s outer backstop is the binding limit. - docs/fix.md: revert an accidental blank-line insertion unrelated to this eval case. Signed-off-by: guy oron --- docs/fix.md | 1 - .../002-dead-config-field/annotations.yaml | 19 ++++++------------- eval/code/eval.yaml | 12 ++++++------ 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/docs/fix.md b/docs/fix.md index e37a9756..185bc522 100644 --- a/docs/fix.md +++ b/docs/fix.md @@ -160,7 +160,6 @@ The fix agent enforces iteration caps to prevent infinite review-fix loops: If this agent needs to reach hosts beyond the defaults, see the [custom network policy guide](network-policy.md). - ## Source [`harness/fix.yaml`](../harness/fix.yaml) diff --git a/eval/code/cases/002-dead-config-field/annotations.yaml b/eval/code/cases/002-dead-config-field/annotations.yaml index 83b4b9b7..1eb5fd6d 100644 --- a/eval/code/cases/002-dead-config-field/annotations.yaml +++ b/eval/code/cases/002-dead-config-field/annotations.yaml @@ -8,19 +8,12 @@ expected_files: labels: forbidden: [] -# Code agent budgets (sandbox work + retries). A deliberate ceiling, not a -# prediction — this case has no observed run of its own yet, so the numbers -# are bounded by two reference points rather than measured, and should be -# re-derived from this case's first CI run. -# Lower bound: 001-fix-add (same harness, trivial fixture) observed up to -# 35 turns / $2.12 (CI run 30166455238). This is a harder, cross-file task, -# so expect it to sit above that. -# Upper bound: #5808 (same dead-config-removal pattern, ~50k-LOC codebase) -# ran 75–79 turns / $3.85–$4.02 in the Round 4 A/B benchmark (2026-08-02, -# guyoron1/fullsend; workflow runs 30729219887 / 30740388070). This fixture -# is ~200 LOC, so expect it to stay below that. -# 60 turns / $4.00 sits between the two, matching 001's ceiling for a task -# one step harder. See: guyoron1/fullsend/docs/ab-benchmark-round4/ +# Code agent budgets (sandbox work + retries). Unmeasured ceiling: this case +# has no CI run of its own yet — re-derive from its first run. Values mirror +# 001-fix-add's ceiling (60 turns / $4.00) and eval.yaml's max_budget_usd. +# For reference, 001 (a trivial fixture) observed 12 turns / $2.12 (CI run +# 29424512121) and 35 turns / $0.98 (CI run 30166455238); this cross-file +# task is harder, so its ceiling is set no lower. max_turns: 60 max_cost_usd: 4.00 diff --git a/eval/code/eval.yaml b/eval/code/eval.yaml index 489f881a..484f7dad 100644 --- a/eval/code/eval.yaml +++ b/eval/code/eval.yaml @@ -68,12 +68,12 @@ execution: # Two cases now share this window: 001-fix-add (a 2-line arithmetic bug # that completes in well under a minute) and 002-dead-config-field (a # cross-file dead-config removal with a larger 60-turn / $4.00 budget). - # Neither is expected to approach 1700s in practice — the code agent's - # own per-iteration budget (2100s, harness/code.yaml) bounds each run - # well before this backstop — so this number should still essentially - # never fire. 002 has no observed CI runtime yet, so treat its headroom - # as an open risk to revisit once a real run exists. If a - # genuine hang did occur: a single-iteration hang gets caught here and + # Neither is expected to approach 1700s in practice: these small fixtures + # finish far under it. Note the 1700s outer backstop — not the larger + # 2100s per-iteration agent budget — is the binding limit, so a genuine + # hang is caught by 1700s first. 002 has no observed CI runtime yet, so + # treat its headroom as an open risk to revisit once a real run exists. + # If a genuine hang did occur: a single-iteration hang gets caught here and # fullsend still writes a partial metrics.json before returning (see # writeMetricsJSON in run.go's error path), giving an inconclusive but # readable "metrics.json not found"-adjacent judge failure; a From 2705386a232ad745f18f1fd19e23338f71ef4fce Mon Sep 17 00:00:00 2001 From: guy oron Date: Sun, 16 Aug 2026 06:35:30 +0300 Subject: [PATCH 7/9] test(eval): add removed_symbols judge to verify fix content in PR diff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Judges previously verified only that the PR touched the expected files — a PR that touches all three files but does an incomplete or wrong removal still passed. Implements the reviewer-suggested diff-content check: - capture-fixture.sh now snapshots each PR's unified diff to output/pr-.diff (with a diff_fetch_failed marker on failure, so a missing diff is distinguishable from capture never running) - new annotation-driven removed_symbols judge: every symbol a case declares must appear only in deletion lines of the diff — a survivor in an added or context line, or a symbol never deleted at all, fails - 002-dead-config-field declares VerboseLogging + verbose_logging; cases without removed_symbols (001) pass trivially - eval.yaml description updated: diff content is now inspected when declared; still no judge runs the fixture's tests Verified: all six judge snippets compile via the harness's exec wrapper; 10-scenario simulation (clean removal, survivor context line, re-added symbol, hunk-header-only mention, no declaration, fetch failure, missing diff, no/closed PRs, missing state) all behave as intended; shellcheck and eval/lint-cases.sh pass. Signed-off-by: guy oron --- .../002-dead-config-field/annotations.yaml | 9 +++ eval/code/eval.yaml | 69 +++++++++++++++++-- eval/scripts/capture-fixture.sh | 28 ++++++-- 3 files changed, 98 insertions(+), 8 deletions(-) diff --git a/eval/code/cases/002-dead-config-field/annotations.yaml b/eval/code/cases/002-dead-config-field/annotations.yaml index 1eb5fd6d..4773c5f7 100644 --- a/eval/code/cases/002-dead-config-field/annotations.yaml +++ b/eval/code/cases/002-dead-config-field/annotations.yaml @@ -8,6 +8,15 @@ expected_files: labels: forbidden: [] +# Consumed by eval.yaml's removed_symbols judge: each symbol must appear +# only in deletion lines of the captured PR diff. Both the Go identifier +# and its YAML key must vanish — a fix that drops the struct field but +# leaves the SetField case, a test assertion, or the raw YAML literal in +# TestLoad would keep one of these in a non-deletion line and fail. +removed_symbols: + - VerboseLogging + - verbose_logging + # Code agent budgets (sandbox work + retries). Unmeasured ceiling: this case # has no CI run of its own yet — re-derive from its first run. Values mirror # 001-fix-add's ceiling (60 turns / $4.00) and eval.yaml's max_budget_usd. diff --git a/eval/code/eval.yaml b/eval/code/eval.yaml index 484f7dad..9e98dabd 100644 --- a/eval/code/eval.yaml +++ b/eval/code/eval.yaml @@ -2,10 +2,13 @@ name: code-eval description: > Functional test of the fullsend code agent pipeline (pre → sandbox → post). Validates that the post-script opens a PR touching the expected files for - a small issue — an end-to-end pipeline guard, not a correctness check. - No judge inspects the PR's diff content or runs the fixture's tests - against it (see annotations.yaml: "Primary signal is pr_created"), so a PR - that opens but contains a cosmetic or outright wrong fix still passes. + a small issue — an end-to-end pipeline guard, not a full correctness + check. Cases may declare removed_symbols in annotations.yaml; the + removed_symbols judge then verifies those symbols appear only in deletion + lines of the captured PR diff. Beyond that, no judge runs the fixture's + tests against the PR, so a fix that compiles but misbehaves can still + pass — and cases with no removed_symbols get file-touch checking only + (see annotations.yaml: "Primary signal is pr_created"). Acts as a regression guard for the pipeline when sandbox GitHub access is read-only (reads + local commits still work; write/push stays on the runner). @@ -183,6 +186,62 @@ judges: return False, f"Expected files missing from PRs: {missing} (changed: {sorted(changed)})" return True, f"All expected files present: {expected}" + - name: removed_symbols + description: > + Content-level check for removal cases: every symbol listed in + annotations.removed_symbols must appear in at least one deletion line + of the captured PR diff (output/pr-.diff, written by + capture-fixture.sh) and in no added or context line — i.e. the symbol + is gone from every hunk the fix touched. Passes trivially when a case + declares no removed_symbols. Diff-scoped only: a symbol surviving in + a file the PR never touched is invisible here (expected_files covers + the known declaration sites), and no judge runs the fixture's tests. + check: | + import json + symbols = outputs.get("annotations", {}).get("removed_symbols") or [] + if not symbols: + return True, "No removed_symbols declared" + raw = outputs["files"].get("output/fixture-state.json") + if not raw: + return False, "fixture-state.json not found — capture-fixture.sh did not run or failed" + state = json.loads(raw) + prs = [p for p in (state.get("pull_requests") or []) + if str(p.get("state", "")).upper() in ("OPEN", "MERGED")] + if not prs: + return False, "No open/merged PR to inspect" + failed = [p.get("number") for p in prs if p.get("diff_fetch_failed")] + if failed: + return False, f"Could not fetch diff for PR(s): {failed}" + diff_lines = [] + for pr in prs: + chunk = outputs["files"].get(f"output/pr-{pr.get('number')}.diff") + if chunk is None: + return False, f"output/pr-{pr.get('number')}.diff not captured" + diff_lines.extend(chunk.splitlines()) + # Skip diff metadata: file headers, hunk headers (whose trailing + # function context can legitimately mention the symbol), and index + # lines. Everything else is a deletion ("-"), addition ("+"), or + # unchanged context line — the symbol may only appear in deletions. + meta = ("--- ", "+++ ", "diff ", "index ", "@@") + problems = [] + for sym in symbols: + deleted = 0 + survivors = 0 + for line in diff_lines: + if sym not in line or line.startswith(meta): + continue + if line.startswith("-"): + deleted += 1 + else: + survivors += 1 + if survivors: + problems.append(f"{sym}: present in {survivors} non-deletion diff line(s)") + elif not deleted: + problems.append(f"{sym}: no deletion lines in diff") + if problems: + return False, "Symbols not fully removed: " + "; ".join(problems) + return True, f"All declared symbols removed cleanly: {symbols}" + # forbidden_labels / max_turns / max_cost below are shared verbatim with # eval/fix/eval.yaml and eval/review/eval.yaml — update all three if changing. - name: forbidden_labels @@ -243,6 +302,8 @@ thresholds: min_pass_rate: 1.0 expected_files: min_pass_rate: 1.0 + removed_symbols: + min_pass_rate: 1.0 forbidden_labels: min_pass_rate: 1.0 max_turns: diff --git a/eval/scripts/capture-fixture.sh b/eval/scripts/capture-fixture.sh index 160d0e57..adc6c0f9 100755 --- a/eval/scripts/capture-fixture.sh +++ b/eval/scripts/capture-fixture.sh @@ -60,6 +60,21 @@ fetch_pr_files() { return 1 } +# Best-effort gh pr diff, written to output/pr-.diff so content-level +# judges (removed_symbols in eval/code/eval.yaml) can inspect what the PR +# actually changed, not just which files it touched. On persistent failure +# returns non-zero so callers can record diff_fetch_failed instead of a +# missing file being indistinguishable from "capture never ran". +fetch_pr_diff() { + local num="$1" + local diff + if diff=$(retry_cmd gh pr diff "$num" --repo "$EPHEMERAL_REPO"); then + printf '%s\n' "$diff" > "${OUTPUT_DIR}/pr-${num}.diff" + return 0 + fi + return 1 +} + # Resolve branch tip SHA via git refs API, polling if still at baseline. # Poll up to 6 times with linear backoff (~21s total, within 60s after_each timeout). # Prefer refs API over PR headRefOid — the latter can lag briefly after post-fix push. @@ -157,14 +172,19 @@ case "${FIXTURE_TYPE}" in while IFS= read -r pr; do [[ -z "$pr" ]] && continue num=$(printf '%s' "$pr" | jq -r '.number') + diff_failed=false + if ! fetch_pr_diff "$num"; then + echo "WARNING: gh pr diff failed for PR #${num}; marking diff_fetch_failed" >&2 + diff_failed=true + fi if files=$(fetch_pr_files "$num"); then - pr_lines+=("$(printf '%s' "$pr" | jq -c --argjson files "$files" \ - '. + {head: .headRefName, base: .baseRefName, files: $files, files_fetch_failed: false} + pr_lines+=("$(printf '%s' "$pr" | jq -c --argjson files "$files" --argjson diff_failed "$diff_failed" \ + '. + {head: .headRefName, base: .baseRefName, files: $files, files_fetch_failed: false, diff_fetch_failed: $diff_failed} | del(.headRefName, .baseRefName)')") else echo "WARNING: gh pr view failed for PR #${num}; marking files_fetch_failed" >&2 - pr_lines+=("$(printf '%s' "$pr" | jq -c \ - '. + {head: .headRefName, base: .baseRefName, files: null, files_fetch_failed: true} + pr_lines+=("$(printf '%s' "$pr" | jq -c --argjson diff_failed "$diff_failed" \ + '. + {head: .headRefName, base: .baseRefName, files: null, files_fetch_failed: true, diff_fetch_failed: $diff_failed} | del(.headRefName, .baseRefName)')") fi done < <(printf '%s' "$prs_json" | jq -c '.[]') From 14cd11df4707fd433c5e9887dc2a1e22c1e8cbc3 Mon Sep 17 00:00:00 2001 From: guy oron Date: Sun, 16 Aug 2026 09:00:08 +0300 Subject: [PATCH 8/9] test(eval): scrub captured PR diffs before artifact upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit capture-fixture.sh now writes output/pr-.diff for the removed_symbols judge, but scrub-eval-results.sh only masks and leak-verifies files whose suffix is in TEXT_SUFFIXES — so a captured diff carrying whatever the agent committed (e.g. a tokened remote URL) would have been uploaded unscrubbed, bypassing the fail-closed leak check. Add .diff to TEXT_SUFFIXES and cover it in scrub-eval-results-test.sh (test filename is now parameterizable); verified the new test fails without the TEXT_SUFFIXES change. Signed-off-by: guy oron --- eval/scripts/scrub-eval-results-test.sh | 14 ++++++++++++-- eval/scripts/scrub-eval-results.sh | 5 ++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/eval/scripts/scrub-eval-results-test.sh b/eval/scripts/scrub-eval-results-test.sh index 078908a8..c95a7fc6 100644 --- a/eval/scripts/scrub-eval-results-test.sh +++ b/eval/scripts/scrub-eval-results-test.sh @@ -20,10 +20,11 @@ run_test() { local test_name="$1" local input_content="$2" local expected_content="$3" + local filename="${4:-output.log}" local test_dir="${TMPDIR}/${test_name}" mkdir -p "${test_dir}" - printf '%s' "${input_content}" > "${test_dir}/output.log" + printf '%s' "${input_content}" > "${test_dir}/${filename}" local exit_code=0 bash "${SCRUB_SCRIPT}" "${test_dir}" > /dev/null 2>&1 || exit_code=$? @@ -35,7 +36,7 @@ run_test() { fi local actual - actual="$(cat "${test_dir}/output.log")" + actual="$(cat "${test_dir}/${filename}")" if [[ "${actual}" != "${expected_content}" ]]; then echo "FAIL: ${test_name}" @@ -114,6 +115,15 @@ Token is realsecret123" \ The quick brown fox jumps over a lazy dog Token is ***" +# --- Captured PR diffs (.diff) are scrubbed like any text artifact --- + +run_test "diff-file-token-redacted" \ + "diff --git a/config b/config ++url = https://x-access-token:ghs_abcdefghijklmnopqrstuv@github.com/o/r" \ + "diff --git a/config b/config ++url = https://x-access-token:***@github.com/o/r" \ + "pr-1.diff" + # --- Summary --- if [[ ${FAILURES} -gt 0 ]]; then diff --git a/eval/scripts/scrub-eval-results.sh b/eval/scripts/scrub-eval-results.sh index 7bbb030d..922067e2 100755 --- a/eval/scripts/scrub-eval-results.sh +++ b/eval/scripts/scrub-eval-results.sh @@ -33,7 +33,10 @@ import sys from pathlib import Path ROOTS = [Path(p) for p in os.environ["EVAL_SCRUB_ROOTS"].splitlines() if p] -TEXT_SUFFIXES = {".log", ".txt", ".json", ".jsonl", ".yaml", ".yml", ".md"} +# .diff: captured PR diffs (capture-fixture.sh writes output/pr-.diff); +# they can carry whatever the agent committed, so they must be scrubbed and +# leak-verified like any other text artifact before upload. +TEXT_SUFFIXES = {".log", ".txt", ".json", ".jsonl", ".yaml", ".yml", ".md", ".diff"} # Actions masks everything to end-of-line (not only \S+). ADD_MASK_RE = re.compile(r"::add-mask::(.+)$", re.MULTILINE) # Reject trivially short mask values that would over-redact (e.g. single From 14bb716db23f3eba8cff0092afeb6e58a10b54b7 Mon Sep 17 00:00:00 2001 From: guy oron Date: Tue, 18 Aug 2026 07:01:46 +0300 Subject: [PATCH 9/9] test(eval): correlate removed_symbols with per-file deletion sites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The judge counted deletions and survivors across the whole diff, so a symbol deleted in any one file satisfied it. For 002 that meant config.go alone could carry the check: a fix that dropped the struct field but left the raw `verbose_logging: true` literal in TestLoad still passed, even though SetField rejects unknown keys and Load() then breaks. That line never appears in the diff at all when the agent doesn't touch it, so it contributed neither a deletion nor a survivor. annotations.removed_symbols now maps each symbol to the files it must disappear from, and the judge requires a deletion per declared file. The missing edit becomes a failure instead of an invisible gap. Also in the judge: - Bucket lines by file and treat only ' ', '+' and '-' as content. The old skip-list enumerated metadata prefixes and missed `rename from|to`, `old|new mode`, `similarity index` and `Binary files`, so a rename to a path containing the symbol read as a survivor. - Match on word boundaries, so renaming a field to VerboseLoggingEnabled no longer counts as the old symbol surviving. - Exempt comment-only lines, so a changelog note may name the symbol. Context lines still count as survivors: that is what catches a literal left behind inside a hunk the fix did touch. - Reject the old list schema explicitly rather than silently accepting it. - Document that the judge reads the issue-fixture `pull_requests` shape, so reuse in a pull_request-type suite needs that branch to emit a diff first. capture-fixture.sh: - Only fetch the PR diff when the case declares removed_symbols. It ran for every issue-type capture, adding an API call and failure surface to cases like 001 that never read the artifact. Unknown case dir still captures. - Poll for the diff before giving up. retry_cmd's ~3s of backoff lands right after PR creation, when the API may still be replicating, and this judge runs at min_pass_rate 1.0 — a transient miss failed a correct fix. Both behaviours are covered by tests wired into `make script-test`: removed-symbols-judge-test.py extracts the shipped judge body from eval.yaml and drives it through 11 diffs (including the partial-removal case above), and capture-fixture-test.sh pins the capture gate's fallback. Signed-off-by: guy oron --- Makefile | 2 + .../002-dead-config-field/annotations.yaml | 25 ++- eval/code/eval.yaml | 110 +++++++---- eval/scripts/capture-fixture-test.sh | 71 +++++++ eval/scripts/capture-fixture.sh | 38 +++- eval/scripts/removed-symbols-judge-test.py | 183 ++++++++++++++++++ 6 files changed, 383 insertions(+), 46 deletions(-) create mode 100755 eval/scripts/capture-fixture-test.sh create mode 100755 eval/scripts/removed-symbols-judge-test.py diff --git a/Makefile b/Makefile index bae5833c..c12aa094 100644 --- a/Makefile +++ b/Makefile @@ -61,5 +61,7 @@ script-test: $(call run-timed,bash .github/scripts/select-eval-agents-test.sh) $(call run-timed,python3 scripts/process-fix-result-test.py) $(call run-timed,bash eval/scripts/scrub-eval-results-test.sh) + $(call run-timed,bash eval/scripts/capture-fixture-test.sh) + $(call run-timed,python3 eval/scripts/removed-symbols-judge-test.py) test: script-test diff --git a/eval/code/cases/002-dead-config-field/annotations.yaml b/eval/code/cases/002-dead-config-field/annotations.yaml index 4773c5f7..2faafe06 100644 --- a/eval/code/cases/002-dead-config-field/annotations.yaml +++ b/eval/code/cases/002-dead-config-field/annotations.yaml @@ -8,14 +8,25 @@ expected_files: labels: forbidden: [] -# Consumed by eval.yaml's removed_symbols judge: each symbol must appear -# only in deletion lines of the captured PR diff. Both the Go identifier -# and its YAML key must vanish — a fix that drops the struct field but -# leaves the SetField case, a test assertion, or the raw YAML literal in -# TestLoad would keep one of these in a non-deletion line and fail. +# Consumed by eval.yaml's removed_symbols judge: each symbol must have a +# deletion line in EVERY file listed under it, and must not survive in any +# added or context line of the diff. Both the Go identifier and its YAML key +# must vanish from all three sites. +# +# Per-file rather than a bare symbol list on purpose: a global "deleted at +# least once" count is already satisfied by config.go alone, so a fix that +# drops the struct field but leaves the raw `verbose_logging: true` literal +# in TestLoad would still pass — and that literal breaks Load(), since +# SetField rejects unknown keys. Naming the sites makes each one required. removed_symbols: - - VerboseLogging - - verbose_logging + VerboseLogging: + - config/config.go + - config/fields.go + - config/config_test.go + verbose_logging: + - config/config.go + - config/fields.go + - config/config_test.go # Code agent budgets (sandbox work + retries). Unmeasured ceiling: this case # has no CI run of its own yet — re-derive from its first run. Values mirror diff --git a/eval/code/eval.yaml b/eval/code/eval.yaml index 9e98dabd..79cb2876 100644 --- a/eval/code/eval.yaml +++ b/eval/code/eval.yaml @@ -3,11 +3,12 @@ description: > Functional test of the fullsend code agent pipeline (pre → sandbox → post). Validates that the post-script opens a PR touching the expected files for a small issue — an end-to-end pipeline guard, not a full correctness - check. Cases may declare removed_symbols in annotations.yaml; the - removed_symbols judge then verifies those symbols appear only in deletion - lines of the captured PR diff. Beyond that, no judge runs the fixture's - tests against the PR, so a fix that compiles but misbehaves can still - pass — and cases with no removed_symbols get file-touch checking only + check. Cases may declare removed_symbols in annotations.yaml, mapping each + symbol to the files it must disappear from; the removed_symbols judge then + requires a deletion line per declared file in the captured PR diff and + fails if the symbol survives anywhere in it. Beyond that, no judge runs + the fixture's tests against the PR, so a fix that compiles but misbehaves + can still pass — and cases with no removed_symbols get file-touch only (see annotations.yaml: "Primary signal is pr_created"). Acts as a regression guard for the pipeline when sandbox GitHub access is read-only (reads + local commits still work; write/push stays on the @@ -188,19 +189,33 @@ judges: - name: removed_symbols description: > - Content-level check for removal cases: every symbol listed in - annotations.removed_symbols must appear in at least one deletion line - of the captured PR diff (output/pr-.diff, written by - capture-fixture.sh) and in no added or context line — i.e. the symbol - is gone from every hunk the fix touched. Passes trivially when a case - declares no removed_symbols. Diff-scoped only: a symbol surviving in - a file the PR never touched is invisible here (expected_files covers - the known declaration sites), and no judge runs the fixture's tests. + Content-level check for removal cases. annotations.removed_symbols maps + each symbol to the files it must disappear from; the judge requires a + deletion line for that symbol in every listed file of the captured PR + diff (output/pr-.diff, written by capture-fixture.sh), and fails if + the symbol survives in any added or context line. Per-file because a + global "deleted somewhere" count is satisfied by the first file alone, + which would let a partial removal pass. Passes trivially when a case + declares no removed_symbols. Matching is word-boundary aware, so a + renamed identifier that merely contains the symbol is not a survivor, + and comment-only lines are exempt so a changelog-style note may mention + it. Diff-scoped: a symbol surviving in a file the PR never touched is + invisible here (naming the file makes its absence a failure instead), + and no judge runs the fixture's tests. Reads state["pull_requests"], + which only capture-fixture.sh's issue-fixture branch writes — reusing + this judge in a pull_request-type suite needs that branch to emit an + equivalent diff artifact first. check: | import json - symbols = outputs.get("annotations", {}).get("removed_symbols") or [] + import re + symbols = outputs.get("annotations", {}).get("removed_symbols") or {} if not symbols: return True, "No removed_symbols declared" + if not isinstance(symbols, dict): + return False, ( + "removed_symbols must map each symbol to the files it must be " + f"deleted from, got {type(symbols).__name__}" + ) raw = outputs["files"].get("output/fixture-state.json") if not raw: return False, "fixture-state.json not found — capture-fixture.sh did not run or failed" @@ -212,35 +227,60 @@ judges: failed = [p.get("number") for p in prs if p.get("diff_fetch_failed")] if failed: return False, f"Could not fetch diff for PR(s): {failed}" - diff_lines = [] + # Bucket diff lines by the file they belong to. Only ' ', '+' and '-' + # start content lines; everything else is metadata (diff/index/@@, + # rename from|to, old|new mode, similarity index, Binary files ...), + # which must never count as a survivor. The "+++ b/" header is + # itself a "+" line, so headers are consumed before that test. + per_file = {} + current = None for pr in prs: chunk = outputs["files"].get(f"output/pr-{pr.get('number')}.diff") if chunk is None: return False, f"output/pr-{pr.get('number')}.diff not captured" - diff_lines.extend(chunk.splitlines()) - # Skip diff metadata: file headers, hunk headers (whose trailing - # function context can legitimately mention the symbol), and index - # lines. Everything else is a deletion ("-"), addition ("+"), or - # unchanged context line — the symbol may only appear in deletions. - meta = ("--- ", "+++ ", "diff ", "index ", "@@") - problems = [] - for sym in symbols: - deleted = 0 - survivors = 0 - for line in diff_lines: - if sym not in line or line.startswith(meta): + for line in chunk.splitlines(): + if line.startswith("+++ "): + path = line[4:].strip().split("\t")[0] + current = None if path == "/dev/null" else re.sub(r"^b/", "", path) + continue + if line.startswith("--- "): continue - if line.startswith("-"): - deleted += 1 - else: - survivors += 1 + if not line or line[0] not in " +-": + continue + per_file.setdefault(current, []).append(line) + + def mentions(sym, line): + # Word-boundary: VerboseLoggingEnabled must not match VerboseLogging. + return re.search(r"\b" + re.escape(sym) + r"\b", line) is not None + + def is_comment(line): + body = line[1:].strip() + return body.startswith(("//", "#", "*")) + + problems = [] + for sym, required_files in sorted(symbols.items()): + if isinstance(required_files, str): + required_files = [required_files] + survivors = [] + for path, lines in per_file.items(): + for line in lines: + if line.startswith("-") or not mentions(sym, line): + continue + if is_comment(line): + continue + survivors.append(path or "") + break if survivors: - problems.append(f"{sym}: present in {survivors} non-deletion diff line(s)") - elif not deleted: - problems.append(f"{sym}: no deletion lines in diff") + problems.append(f"{sym}: survives in {sorted(set(survivors))}") + continue + missing = [f for f in (required_files or []) + if not any(l.startswith("-") and mentions(sym, l) + for l in per_file.get(f, []))] + if missing: + problems.append(f"{sym}: no deletion line in {missing}") if problems: return False, "Symbols not fully removed: " + "; ".join(problems) - return True, f"All declared symbols removed cleanly: {symbols}" + return True, f"All declared symbols removed cleanly from their declared files: {sorted(symbols)}" # forbidden_labels / max_turns / max_cost below are shared verbatim with # eval/fix/eval.yaml and eval/review/eval.yaml — update all three if changing. diff --git a/eval/scripts/capture-fixture-test.sh b/eval/scripts/capture-fixture-test.sh new file mode 100755 index 00000000..5f79d6a0 --- /dev/null +++ b/eval/scripts/capture-fixture-test.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# capture-fixture-test.sh — Test capture-fixture.sh's PR-diff capture gate. +# +# The gate decides whether the extra `gh pr diff` call runs. Getting its +# fallback backwards is silent: cases that need output/pr-.diff would fail +# their content judge for a missing artifact. Only the gate is exercised here — +# the rest of capture-fixture.sh needs live GitHub state. +# +# Usage: +# bash eval/scripts/capture-fixture-test.sh + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +CAPTURE_SCRIPT="${SCRIPT_DIR}/capture-fixture.sh" + +if [[ ! -f "$CAPTURE_SCRIPT" ]]; then + echo "FAIL: capture-fixture.sh not found at ${CAPTURE_SCRIPT}" >&2 + exit 1 +fi + +# Source just the gate: the script itself requires live fixture env and exits. +gate_src="$(sed -n '/^case_wants_pr_diff() {/,/^}/p' "$CAPTURE_SCRIPT")" +if [[ -z "$gate_src" ]]; then + echo "FAIL: case_wants_pr_diff not found in capture-fixture.sh" >&2 + exit 1 +fi +eval "$gate_src" + +TMP_ROOT="$(mktemp -d)" +trap 'rm -rf "$TMP_ROOT"' EXIT +failures=0 + +# assert_gate (CASE_SOURCE_DIR preset) +assert_gate() { + local expect="$1" desc="$2" actual="capture" + case_wants_pr_diff || actual="skip" + if [[ "$actual" == "$expect" ]]; then + echo "ok: ${desc} → ${actual}" + else + echo "FAIL: ${desc} — expected ${expect}, got ${actual}" >&2 + failures=$((failures + 1)) + fi +} + +# Unknown case dir must capture: never withhold an artifact a judge may need. +CASE_SOURCE_DIR="" assert_gate capture "CASE_SOURCE_DIR unset" +CASE_SOURCE_DIR="${TMP_ROOT}/missing" assert_gate capture "CASE_SOURCE_DIR points nowhere" + +no_symbols="${TMP_ROOT}/no-symbols" +mkdir -p "$no_symbols" +printf 'state: open\nexpected_files:\n - calc.py\n' > "${no_symbols}/annotations.yaml" +CASE_SOURCE_DIR="$no_symbols" assert_gate skip "case declares no removed_symbols" + +with_symbols="${TMP_ROOT}/with-symbols" +mkdir -p "$with_symbols" +printf 'state: open\nremoved_symbols:\n VerboseLogging:\n - config/config.go\n' \ + > "${with_symbols}/annotations.yaml" +CASE_SOURCE_DIR="$with_symbols" assert_gate capture "case declares removed_symbols" + +commented="${TMP_ROOT}/commented" +mkdir -p "$commented" +printf 'state: open\n# removed_symbols: not declared, just discussed\n' \ + > "${commented}/annotations.yaml" +CASE_SOURCE_DIR="$commented" assert_gate skip "removed_symbols only mentioned in a comment" + +if [[ $failures -gt 0 ]]; then + echo "FAIL: ${failures} capture-fixture gate test(s) failed" >&2 + exit 1 +fi +echo "All capture-fixture tests passed" diff --git a/eval/scripts/capture-fixture.sh b/eval/scripts/capture-fixture.sh index adc6c0f9..2634469b 100755 --- a/eval/scripts/capture-fixture.sh +++ b/eval/scripts/capture-fixture.sh @@ -13,6 +13,10 @@ # # Required env (set by harness): # CASE_WORKSPACE — path to the case workspace +# +# Optional env (set by harness): +# CASE_SOURCE_DIR — original case directory; read to decide whether the case +# needs a PR diff captured. Missing means "capture it". set -euo pipefail CASE_WORKSPACE="${CASE_WORKSPACE:?CASE_WORKSPACE is required}" @@ -67,14 +71,38 @@ fetch_pr_files() { # missing file being indistinguishable from "capture never ran". fetch_pr_diff() { local num="$1" - local diff + local diff attempt if diff=$(retry_cmd gh pr diff "$num" --repo "$EPHEMERAL_REPO"); then printf '%s\n' "$diff" > "${OUTPUT_DIR}/pr-${num}.diff" return 0 fi + # retry_cmd's ~3s of backoff lands immediately after PR creation, exactly + # when the API may still be replicating. Poll a little longer before giving + # up: removed_symbols runs at min_pass_rate 1.0, so a transient miss here + # fails an otherwise-correct fix. Mirrors resolve_head_sha's readiness poll; + # worst case ~10s more, well inside the 60s after_each timeout. + echo "WARNING: gh pr diff not ready for PR #${num}; polling..." >&2 + for attempt in 1 2 3 4; do + sleep $((attempt)) + if diff=$(gh pr diff "$num" --repo "$EPHEMERAL_REPO" 2>/dev/null); then + printf '%s\n' "$diff" > "${OUTPUT_DIR}/pr-${num}.diff" + return 0 + fi + done return 1 } +# Only cases declaring removed_symbols consume output/pr-.diff, so the +# extra `gh pr diff` call and its failure surface are skipped for the rest +# (001-fix-add declares none). Defaults to capturing when the annotations +# cannot be read — a judge must never fail for want of an artifact this +# script decided on its own to skip. +case_wants_pr_diff() { + local annotations="${CASE_SOURCE_DIR:-}/annotations.yaml" + [[ -n "${CASE_SOURCE_DIR:-}" && -f "$annotations" ]] || return 0 + grep -qE '^[[:space:]]*removed_symbols[[:space:]]*:' "$annotations" +} + # Resolve branch tip SHA via git refs API, polling if still at baseline. # Poll up to 6 times with linear backoff (~21s total, within 60s after_each timeout). # Prefer refs API over PR headRefOid — the latter can lag briefly after post-fix push. @@ -173,9 +201,11 @@ case "${FIXTURE_TYPE}" in [[ -z "$pr" ]] && continue num=$(printf '%s' "$pr" | jq -r '.number') diff_failed=false - if ! fetch_pr_diff "$num"; then - echo "WARNING: gh pr diff failed for PR #${num}; marking diff_fetch_failed" >&2 - diff_failed=true + if case_wants_pr_diff; then + if ! fetch_pr_diff "$num"; then + echo "WARNING: gh pr diff failed for PR #${num}; marking diff_fetch_failed" >&2 + diff_failed=true + fi fi if files=$(fetch_pr_files "$num"); then pr_lines+=("$(printf '%s' "$pr" | jq -c --argjson files "$files" --argjson diff_failed "$diff_failed" \ diff --git a/eval/scripts/removed-symbols-judge-test.py b/eval/scripts/removed-symbols-judge-test.py new file mode 100755 index 00000000..be991464 --- /dev/null +++ b/eval/scripts/removed-symbols-judge-test.py @@ -0,0 +1,183 @@ +#!/usr/bin/env python3 +# removed-symbols-judge-test.py — Behaviour tests for eval/code/eval.yaml's +# removed_symbols judge. +# +# The judge is Python embedded in YAML, so it has no import site of its own. +# This test extracts the shipped check body straight from eval.yaml and runs it +# against synthetic diffs, which keeps the test from drifting from the code and +# needs no YAML parser (CI installs neither pyyaml nor ruamel). +# +# Usage: +# python3 eval/scripts/removed-symbols-judge-test.py + +import json +import os +import sys +import textwrap + +REPO_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) +EVAL_YAML = os.path.join(REPO_ROOT, "eval", "code", "eval.yaml") + +SYMBOLS = { + "VerboseLogging": ["config/config.go", "config/fields.go", "config/config_test.go"], + "verbose_logging": ["config/config.go", "config/fields.go", "config/config_test.go"], +} + + +def load_judge(path): + """Return the removed_symbols check body as a callable taking `outputs`.""" + with open(path, encoding="utf-8") as handle: + lines = handle.read().splitlines() + + try: + start = lines.index(" - name: removed_symbols") + except ValueError: + sys.exit(f"FAIL: no removed_symbols judge found in {path}") + + body, collecting, indent = [], False, None + for line in lines[start:]: + if not collecting: + if line.strip() == "check: |": + collecting = True + continue + if line.strip() and indent is None: + indent = len(line) - len(line.lstrip()) + if line.strip() and (len(line) - len(line.lstrip())) < indent: + break + body.append(line) + + if not body: + sys.exit(f"FAIL: removed_symbols judge in {path} has no check body") + + source = "def _check(outputs):\n" + textwrap.indent(textwrap.dedent("\n".join(body)), " ") + namespace = {} + exec(source, namespace) # noqa: S102 - executing our own shipped judge is the point + return namespace["_check"] + + +def hunk(path, *lines): + return "\n".join([ + f"diff --git a/{path} b/{path}", + "index 1111111..2222222 100644", + f"--- a/{path}", + f"+++ b/{path}", + "@@ -1,6 +1,4 @@", + *lines, + ]) + + +DELETE_CONFIG = hunk( + "config/config.go", + '-\tVerboseLogging bool `yaml:"verbose_logging"`', + " \tName string", +) +DELETE_FIELDS = hunk( + "config/fields.go", + '-\tcase "verbose_logging":', + "-\t\tc.VerboseLogging = v", + '-\tcase "name":', +) +DELETE_TEST = hunk( + "config/config_test.go", + "-verbose_logging: true", + "-\tif cfg.VerboseLogging != true {", + ' \tif cfg.Name != "x" {', +) +COMPLETE = "\n".join([DELETE_CONFIG, DELETE_FIELDS, DELETE_TEST]) + + +def outputs_for(diff, symbols=None, pr_state=None): + if symbols is None: + symbols = SYMBOLS + if pr_state is None: + pr_state = {"number": 7, "state": "OPEN", "diff_fetch_failed": False} + return { + "annotations": {"removed_symbols": symbols}, + "files": { + "output/fixture-state.json": json.dumps({"pull_requests": [pr_state]}), + "output/pr-7.diff": diff, + }, + } + + +CASES = [ + ("complete removal across every declared file", outputs_for(COMPLETE), True), + # The partial-removal gap: config_test.go is touched, but not on any line + # mentioning the symbol, so its raw `verbose_logging: true` literal survives + # outside the diff entirely. A global "deleted somewhere" count passes this. + ("partial removal leaves a declared file untouched", + outputs_for("\n".join([ + DELETE_CONFIG, DELETE_FIELDS, + hunk("config/config_test.go", "-\tfoo := 1", " \tbar := 2"), + ])), False), + ("symbol survives on an added line", + outputs_for("\n".join([ + DELETE_CONFIG, DELETE_FIELDS, + hunk("config/config_test.go", "-verbose_logging: true", + "+\tcfg.VerboseLogging = true"), + ])), False), + ("symbol survives on an unchanged context line", + outputs_for("\n".join([ + DELETE_CONFIG, DELETE_FIELDS, + hunk("config/config_test.go", "-verbose_logging: true", + " \tif cfg.VerboseLogging != true {"), + ])), False), + # Metadata lines are not content: a rename to a path containing the symbol + # must not read as a survivor. + ("rename, mode and binary metadata naming the symbol", + outputs_for("\n".join([ + COMPLETE, + "diff --git a/config/VerboseLogging.go b/config/VerboseLoggingHandler.go", + "similarity index 95%", + "rename from config/VerboseLogging.go", + "rename to config/VerboseLoggingHandler.go", + "old mode 100644", + "new mode 100755", + "Binary files a/VerboseLogging.bin and b/x.bin differ", + ])), True), + ("renamed identifier that merely contains the symbol", + outputs_for("\n".join([ + COMPLETE, + hunk("config/other.go", "+\tVerboseLoggingEnabled bool", "+\tverbose_logging_v2 := 1"), + ])), True), + ("comment-only lines may still mention the symbol", + outputs_for("\n".join([ + COMPLETE, + hunk("CHANGELOG.md", "+# Removed unused VerboseLogging", + "+// drop verbose_logging", "+ * VerboseLogging is gone"), + ])), True), + ("no removed_symbols declared passes trivially", + outputs_for(DELETE_CONFIG, symbols={}), True), + # The pre-per-file schema is a bare list; accepting it silently would + # reinstate the gap this judge closes. + ("legacy list schema is rejected rather than ignored", + outputs_for(DELETE_CONFIG, symbols=["VerboseLogging"]), False), + ("diff fetch failure is surfaced", + outputs_for(COMPLETE, pr_state={"number": 7, "state": "OPEN", "diff_fetch_failed": True}), + False), + ("no open or merged PR to inspect", + outputs_for(COMPLETE, pr_state={"number": 7, "state": "CLOSED"}), False), +] + + +def main(): + check = load_judge(EVAL_YAML) + failures = 0 + for name, outputs, expect_pass in CASES: + passed, message = check(outputs) + ok = passed is expect_pass + if not ok: + failures += 1 + print(f"FAIL: {name} — expected {'pass' if expect_pass else 'fail'}, " + f"got {'pass' if passed else 'fail'}: {message}") + else: + print(f"ok: {name}") + if failures: + print(f"FAIL: {failures}/{len(CASES)} removed_symbols judge behaviours wrong") + return 1 + print(f"All removed_symbols judge tests passed ({len(CASES)} cases)") + return 0 + + +if __name__ == "__main__": + sys.exit(main())