-
Notifications
You must be signed in to change notification settings - Fork 12
test(eval): add 002-dead-config-field functional test case #617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8dbe7df
ae74c63
407b98d
69dfc0e
f3b7592
ea54615
2705386
14cd11d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| state: open | ||
|
|
||
| expected_files: | ||
| - config/config.go | ||
| - config/fields.go | ||
| - config/config_test.go | ||
|
|
||
| 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. | ||
| # 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM — Budget copied from 001-fix-add with no observed baseline, and case shipped without an end-to-end harness run
Suggestion: Run the case at least once through the real fullsend eval code harness (or CI) before merging, and add an annotations.yaml comment documenting the observed turns/cost baseline and chosen headroom multiplier, matching the convention in 001-fix-add/annotations.yaml. If CI access is genuinely blocking pre-merge, say so explicitly and treat the unchecked test-plan item as a required follow-up rather than optional. |
||
|
|
||
| # 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 | ||
| 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 — tracing symbol references across multiple files to determine what | ||
| 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(). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| forge: github | ||
| fixture: | ||
| type: issue | ||
| title: "config: VerboseLogging field appears unused" | ||
| body: | | ||
| ## Bug Report | ||
|
|
||
| **What happened:** | ||
| 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. | ||
|
|
||
| **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? | ||
|
|
||
| **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 | ||
|
|
||
| Can someone verify whether this is actually used? If not, please clean it up. | ||
| Make sure tests still pass after any changes. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| ../../repos/taskrunner |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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). | ||
|
|
@@ -65,9 +68,15 @@ 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 | ||
| # genuine hang did occur: a single-iteration hang gets caught here and | ||
| # 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: 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 | ||
|
|
@@ -177,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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM — removed_symbols judge is schema-incompatible with the pull_request fixture-type branch if ever reused Verified against current capture-fixture.sh (head 14cd11d): the removed_symbols judge reads Suggestion: Either document in the judge description that removed_symbols only supports issue-type fixtures today, or extend capture-fixture.sh's pull_request branch to also populate an equivalent diff artifact / diff_fetch_failed flag before this pattern is copy-pasted into another suite. |
||
| 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-<num>.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 ", "@@") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM — removed_symbols judge's diff-metadata skip-list omits rename/mode-change header lines Verified: Suggestion: Add the rename/mode-change/binary line prefixes to the |
||
| problems = [] | ||
| for sym in symbols: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HIGH — removed_symbols judge aggregates deletions/survivors across the whole diff, so a partial/incomplete removal can still pass Verified against current eval.yaml (head 14cd11d): the removed_symbols check counts Suggestion: Correlate each removed_symbols deletion requirement with the specific expected_files declaration site(s) (e.g., parse the diff per-file and require a deletion in each file expected to contain the symbol), or diff the final file contents against a known-good post-fix reference instead of relying on a floating "at least one deletion anywhere" check. |
||
| deleted = 0 | ||
| survivors = 0 | ||
| for line in diff_lines: | ||
| if sym not in line or line.startswith(meta): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM — removed_symbols uses unanchored substring matching, risking false-fails on comments/renamed identifiers Verified: Suggestion: Use word-boundary-aware matching (e.g. regex |
||
| 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 | ||
|
|
@@ -237,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: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # taskrunner | ||
|
|
||
| A minimal task runner that reads a YAML config and executes registered tasks. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM — code_expectations omits that config_test.go's embedded raw YAML literal must also be edited, not just Go-level assertions annotations.yaml's Suggestion: Extend |
||
| 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) | ||
| } | ||
| } | ||
|
|
||
| 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") | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package config | ||
|
|
||
| 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 { | ||
| 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 | ||
| default: | ||
| return fmt.Errorf("unknown config key: %s", key) | ||
| } | ||
| return nil | ||
| } | ||
|
qodo-code-review[bot] marked this conversation as resolved.
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MEDIUM — The eval harness has never actually executed this case — every prior "Functional Tests" run was gated off as unauthorized
annotations.yaml's own comment (lines 11-16) already self-flags the max_turns/max_cost budget as an "Unmeasured ceiling... this case has no CI run of its own yet." Live verification at head 4ab7533 confirms why:
gh pr checks 617and the gate job log for the latest run (31710102745, 2026-08-13T14:25) show.github/scripts/check-e2e-authorization.shprintingauthorized=false reason=unauthorizedwith the warning "Functional tests did not run (reason: unauthorized). External contributors need a maintainer to apply the ok-to-test label after the latest push." The "functional-tests" job itself reports DETECT_RESULT=skipped / TESTS_RESULT=skipped. Checking the full run history for this branch (11 pushes over 2026-08-03 through 2026-08-13), every single "Functional Tests" workflow run completed in 5-21 seconds and shows the identical unauthorized/skip pattern — the coding-agent harness (setup-fixture.sh's repo copy, the code agent solving the issue, and the expected_files/pr_created/max_turns/max_cost judges) has never actually run end-to-end for this case at any point in the PR's life, despite 4+ rounds of prior review iterating on the budget numbers as if a real baseline existed. This is a distinct, actionable fact beyond the already-posted "budget is an unmeasured guess" comments: it identifies the specific, currently-active CI gate blocking validation and the concrete remedy.Suggestion: Have a maintainer apply the
ok-to-testlabel (per CONTRIBUTING.md) to trigger a real functional-tests run before merge, so the budget/expected_files/harness wiring for this case is validated at least once rather than merged as an entirely unexercised configuration.