test(eval): add 003-dead-config-field functional test case

[guyoron1]

Summary

• Adds a second code eval case testing cross-file dead-code removal in a Go project
• Fixture: Config.VerboseLogging is declared, defaulted, parsed, and tested — but never read by any consumer
• Agent must trace references across 3 files (config.go, fields.go, config_test.go) and remove them while keeping tests passing
• Adds a content-level removed_symbols judge (annotation-driven, reusable): each declared symbol needs a deletion line in every declared file of the captured PR diff and must not survive on any added/context line of a non-doc file. capture-fixture.sh snapshots output/pr-<num>.diff (gated on the case declaring removed_symbols) and scrub-eval-results.sh masks .diff like other text artifacts.
• Pattern inspired by internal/config: defaults.auto_merge is parsed but never consumed fullsend#5808 — a structurally identical dead-config issue (RepoDefaults.AutoMerge declared, defaulted, parsed, tested, never consumed). In A/B benchmarking (Round 4, 30 upstream issues, 13 overlapping pairs where both paths produced PRs), #5808 produced byte-for-byte identical diffs across Path A and Path B.

Why this case

001-fix-add tests a single-line arithmetic bug. This case tests multi-file symbol tracing — the agent needs to identify that VerboseLogging is dead code by confirming no consumer reads it, then remove it from the struct definition, defaults, field setter, and test assertions across 3 files. Numbered 003 because open #682 adds 002-push-back-on-nonsense.

Structure

eval/code/cases/003-dead-config-field/
├── annotations.yaml    # expected_files, removed_symbols (symbol -> files), budget (60 turns, $4)
├── input.yaml          # issue fixture
└── repo -> ../../repos/taskrunner

eval/code/repos/taskrunner/
├── go.mod
├── README.md
├── config/
│   ├── config.go           # struct + Defaults() — has VerboseLogging
│   ├── fields.go           # SetField() switch — has VerboseLogging case
│   ├── config_test.go      # 3 tests asserting VerboseLogging
│   └── internal/yaml/yaml.go
└── runner/
    └── runner.go           # uses cfg.MaxRetries/Timeout/Workers — never VerboseLogging

eval/scripts/
├── removed-symbols-judge-test.py   # runs the shipped judge body from eval.yaml over 18 synthetic diffs
└── capture-fixture-test.sh         # the diff-capture gate (5 cases)

Test plan (measured, head 0b42b80)

• go test ./... in eval/code/repos/taskrunner: ok config, ok runner (yaml pkg has no tests)
• bash eval/lint-cases.sh code: OK: all cases pass lint checks
• python3 eval/scripts/removed-symbols-judge-test.py: 18/18 pass; the 6 cases added in 0b42b80 fail against the previous judge body (negative-checked)
• bash eval/scripts/capture-fixture-test.sh, bash eval/scripts/scrub-eval-results-test.sh: pass
• Not run: end-to-end fullsend eval code — functional tests and Script tests are approval-gated on this fork PR and have not yet executed (needs ok-to-test)

A/B benchmark source

Round 4 A/B benchmark (2026-08-02) on guyoron1/fullsend. 30 upstream issues × 2 config paths, 13 overlapping pairs where both produced PRs, blind LLM judge (Opus 4.6).

• Quality evaluation (13 pairs)
• Comparison data
• Detailed judge scoring
• #5808 agent output — identical PRs from both paths: Path A PR #813, Path B PR #855

Related: fullsend-ai/fullsend#5808

[github-actions]

Functional tests did not run

Functional tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

[qodo-code-review]

PR Summary by Qodo

eval(code): add 002 dead-config-field functional test case

🧪 Tests ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Add a new eval/code case exercising cross-file dead config removal in Go.
• Provide a fixture repo where Config.VerboseLogging is parsed and tested but unused.
• Define expected touched files, budget, and issue text to guide the agent workflow.

Diagram

graph TD
  Case["Eval case 002"] --> Anno["annotations.yaml"]
  Case --> Input["input.yaml"]
  Case --> Repo["taskrunner fixture"] --> Config["config package"] --> Tests["config tests"]
  Repo --> Runner["runner package"] --> Config

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Extend the existing 001-fix-add fixture

• ➕ Fewer fixture repos/cases to maintain
• ➕ Less duplicated scaffolding (go.mod/README/parser)
• ➖ Mixes concerns (arithmetic bug + dead-field removal) and reduces case isolation
• ➖ Harder to benchmark specific reasoning capability regressions

2. Use a smaller fixture without a custom YAML parser

• ➕ Less code to read; faster agent and reviewer iteration
• ➕ Focuses purely on symbol tracing/removal
• ➖ Less realistic cross-file tracing (struct/defaults/parser/tests) that mirrors real repos
• ➖ Fewer touchpoints to validate full removal (defaults + parsing + tests)

Recommendation: Keep the new standalone 002 case with its own fixture repo: it cleanly isolates the multi-file dead-code-removal task and provides realistic cross-file references (struct, defaults, parser switch, tests) without adding unrelated complexity.

Files changed (10) +373 / -0

Tests (7) +366 / -0

annotations.yamlAdd case expectations and budget for 002 dead-field scenario +22/-0

Add case expectations and budget for 002 dead-field scenario

• Defines expected modified files (config.go/fields.go/config_test.go), run budget (turns/cost), and the success criteria describing dead-field removal across files.

eval/code/cases/002-dead-config-field/annotations.yaml

input.yamlAdd issue fixture describing VerboseLogging as dead config +24/-0

Add issue fixture describing VerboseLogging as dead config

• Introduces an issue-style prompt that instructs the agent to verify VerboseLogging is never consumed and remove it while keeping Go tests passing.

eval/code/cases/002-dead-config-field/input.yaml

config.goDefine Config with VerboseLogging and Defaults/Load behavior +47/-0

Define Config with VerboseLogging and Defaults/Load behavior

• Adds the fixture's core configuration struct including the dead VerboseLogging field, plus Defaults() and YAML-based Load().

eval/code/repos/taskrunner/config/config.go

config_test.goAdd tests asserting VerboseLogging defaulting and YAML round-trip +79/-0

Add tests asserting VerboseLogging defaulting and YAML round-trip

• Introduces three tests that validate Defaults(), full Load(), and partial Load() behavior, including explicit assertions on VerboseLogging to be removed by the agent.

eval/code/repos/taskrunner/config/config_test.go

fields.goImplement YAML field parsing via SetField switch +34/-0

Implement YAML field parsing via SetField switch

• Adds SetField() to parse flat YAML keys, including a verbose_logging case that sets Config.VerboseLogging.

eval/code/repos/taskrunner/config/fields.go

yaml.goAdd minimal flat YAML unmarshaller and basic parsers +86/-0

Add minimal flat YAML unmarshaller and basic parsers

• Provides a small YAML parser used by the fixture config loader, including ParseBool/ParseInt helpers and a SetField-based application path.

eval/code/repos/taskrunner/config/internal/yaml/yaml.go

runner.goAdd runner that consumes config but never reads VerboseLogging +74/-0

Add runner that consumes config but never reads VerboseLogging

• Implements a concurrent task runner using MaxRetries/Timeout/Workers and explicitly notes VerboseLogging is dead (never consumed), forming the basis of the evaluation task.

eval/code/repos/taskrunner/runner/runner.go

Documentation (1) +3 / -0

README.mdDocument the taskrunner fixture purpose +3/-0

Document the taskrunner fixture purpose

• Adds a minimal README describing the fixture as a YAML-configured task runner.

eval/code/repos/taskrunner/README.md

Other (2) +4 / -0

repoWire 002 case to the taskrunner fixture repo +1/-0

Wire 002 case to the taskrunner fixture repo

• Adds a repo pointer (symlink target) so the eval harness runs against eval/code/repos/taskrunner.

eval/code/cases/002-dead-config-field/repo

go.modAdd Go module definition for the taskrunner fixture +3/-0

Add Go module definition for the taskrunner fixture

• Defines module path github.com/eval-org/taskrunner and Go 1.22 toolchain target for the fixture repo.

eval/code/repos/taskrunner/go.mod

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Agent instructions in code_expectations ✓ Resolved 📜 Skill insight ⛨ Security

Description

The new code_expectations text includes explicit agent-directed instructions (e.g., "the agent
must ..."), which matches disallowed prompt/instruction patterns in config values. This can enable
prompt-injection-style behavior if these files are ever consumed by agent runtimes or tooling.

Code

eval/code/cases/002-dead-config-field/annotations.yaml[R20-22]

+  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.

Relevance

●●● Strong

Repo previously accepted removing agent-instruction-like fixture text for compliance (PR #381).

PR-#381

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1538322 prohibits agent-instruction/prompt-injection-like patterns in config
values. The added code_expectations text explicitly instructs an agent ("the agent must trace
..."), which falls under the rule’s failure criteria.

eval/code/cases/002-dead-config-field/annotations.yaml[20-22]
Skill: code-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`eval/code/cases/002-dead-config-field/annotations.yaml` includes agent-directed instruction language in `code_expectations` (e.g., `the agent must ...`), which violates the policy against agent-instruction patterns in comments/string literals/config values.

## Issue Context
This text can be reworded to describe the scenario and success criteria without addressing an agent or providing imperative instructions.

## Fix Focus Areas
- eval/code/cases/002-dead-config-field/annotations.yaml[14-22]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Nonpositive workers breaks Run ✓ Resolved 🐞 Bug ☼ Reliability

Description

Runner.Run() writes to sem before starting the worker goroutine; if cfg.Workers == 0, sem is
unbuffered and the first send blocks forever (deadlock). If cfg.Workers < 0, `make(chan struct{},
r.cfg.Workers)` panics, crashing the process.

Code

eval/code/repos/taskrunner/runner/runner.go[R39-42]

+	for _, task := range r.tasks {
+		sem <- struct{}{}
+		go func(t Task) {
+			defer func() { <-sem }()

Relevance

●● Moderate

No prior evidence of worker-count validation enforcement; taskrunner runner code is new fixture
content.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

sem is created with capacity r.cfg.Workers and the loop immediately performs sem <- struct{}{}
before launching the goroutine that eventually does <-sem; with capacity 0, that first send cannot
proceed.

eval/code/repos/taskrunner/runner/runner.go[35-44]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Run()` uses `cfg.Workers` as a channel capacity and sends into the channel before any goroutine can receive. `Workers==0` deadlocks; `Workers<0` panics during channel creation.

## Issue Context
`Workers` is configured via YAML and is not validated anywhere.

## Fix Focus Areas
- eval/code/repos/taskrunner/runner/runner.go[35-44]
- eval/code/repos/taskrunner/config/config.go[35-47]

## Suggested fix
- Validate config either in `config.Load()` or in `runner.New()`/`Run()`:
 - If `Workers < 1`, return a clear error (e.g., `fmt.Errorf("workers must be >= 1")`).
 - (Optional) also validate `Timeout > 0` and `MaxRetries >= 0` to avoid surprising behavior.
- Only construct the semaphore after validation passes.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Timeout retries overlap ✓ Resolved 🐞 Bug ≡ Correctness

Description

runner.runWithRetry() starts each attempt in a goroutine and, on timeout, immediately begins the
next attempt without cancelling the previous one, so the same task can run concurrently multiple
times and duplicate side effects. This also defeats the intended semantics of a per-attempt timeout
because timed-out attempts keep running in the background.

Code

eval/code/repos/taskrunner/runner/runner.go[R59-62]

+	for attempt := 0; attempt <= r.cfg.MaxRetries; attempt++ {
+		done := make(chan error, 1)
+		go func() { done <- t.Fn() }()
+

Relevance

●● Moderate

No repo history on cancelling timed-out retries; taskrunner runner code appears new fixture content.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code starts a goroutine for t.Fn() per attempt and the timeout branch does not cancel or wait
for that goroutine before looping, so a timeout necessarily allows the prior attempt to continue
while the next attempt starts.

eval/code/repos/taskrunner/runner/runner.go[55-71]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`runWithRetry()` spawns `t.Fn()` in a goroutine and on timeout immediately retries, leaving the timed-out goroutine running. This can cause overlapping attempts and duplicated side effects.

## Issue Context
`Task.Fn` has no cancellation mechanism, so the runner cannot stop work on timeout.

## Fix Focus Areas
- eval/code/repos/taskrunner/runner/runner.go[55-74]

## Suggested fix
- Change `Task.Fn` to accept `context.Context` (or similar cancellation signal).
- In `runWithRetry`, create a `context.WithTimeout` per attempt, call `t.Fn(ctx)` (no extra goroutine needed if `Fn` blocks honoring ctx), and `cancel()` on completion/timeout.
- Alternatively (if you must keep `func() error`), document/enforce that tasks must be idempotent and tolerate overlap, but this is weaker than true cancellation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Config typos silently ignored ✓ Resolved 🐞 Bug ☼ Reliability

Description

The custom YAML parser silently skips non-empty lines that don’t match its key: value regex, and
Config.SetField has no default case (always returns nil), so malformed lines and unknown keys are
accepted without error and defaults are used unexpectedly. This makes misconfiguration hard to
detect and can lead to unexpected runtime behavior.

Code

eval/code/repos/taskrunner/config/fields.go[R31-34]

+		c.Workers = v
+	}
+	return nil
+}

Relevance

●● Moderate

No historical evidence enforcing strict YAML/unknown-key errors; taskrunner fixture repo not present
historically.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Unmarshal explicitly continues on regex mismatch (dropping malformed lines), and SetField’s switch
has no default while the function always returns nil, so unknown keys cannot surface as errors.

eval/code/repos/taskrunner/config/internal/yaml/yaml.go[12-27]
eval/code/repos/taskrunner/config/fields.go[6-34]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Config parsing currently fails open: malformed lines are dropped and unknown keys are ignored with no error. This can hide typos and produce unexpected defaults.

## Issue Context
`yaml.Unmarshal()` continues when a non-empty line doesn't match the regex, and `Config.SetField()` returns nil even when `key` isn't recognized.

## Fix Focus Areas
- eval/code/repos/taskrunner/config/internal/yaml/yaml.go[12-27]
- eval/code/repos/taskrunner/config/fields.go[6-34]

## Suggested fix
- In `yaml.Unmarshal`, when a non-empty/non-comment line doesn't match the supported `key: value` format, return an error that includes the line content (and ideally line number).
- In `Config.SetField`, add a `default:` case that returns an error for unknown keys (e.g., `fmt.Errorf("unknown config key: %s", key)`).
- Update tests (or add new ones) to assert that unknown/malformed config fails fast.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

Tip of the day

💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[waynesun09]

MEDIUM — Judges only verify file-touch, not correctness, for a case whose entire premise is correctness (eval/code/eval.yaml, not modified by this PR)

eval.yaml's own top-level description already documents that "No judge inspects the PR's diff content or runs the fixture's tests against it," and the wired-up judges (pr_created, expected_files, forbidden_labels, max_turns, max_cost) confirm this: expected_files only checks that the PR's changed-file set is a superset of config/config.go, config/fields.go, config/config_test.go — it never verifies VerboseLogging was actually removed correctly, that a stale SetField case wasn't left behind, or that go test ./... still passes. Unlike 001-fix-add/annotations.yaml, which explicitly labels its code_expectations as "Human reference only; not consumed by judges (unlike triage/review quality)", 002's annotations.yaml code_expectations is written with stronger correctness-implying language ("Tests must still pass after removal... the agent must trace symbol references across multiple files") and omits that disclaimer, making it easy for a reader to overestimate what a green run actually verifies for this harder, 3-file case.

Suggestion: Add the same "Human reference only; not consumed by judges" disclaimer to 002's code_expectations for consistency with 001, and/or add a judge that checks out the PR branch and runs go build ./... && go test ./... (and ideally greps for the removed symbol) so a PR that touches all three expected files but does an incomplete/incorrect removal doesn't still pass.

[waynesun09]

HIGH — Fixture spoils the answer the eval case claims to test for

The PR frames this case as testing genuine multi-file symbol tracing ("the agent needs to identify that VerboseLogging is dead code by confirming no consumer reads it"), but this line contains an explicit comment directly above Run(): "Note: cfg.VerboseLogging is not checked anywhere — this is dead config." input.yaml (lines 19-24) compounds this: step 1 of "Steps to verify" tells the agent to run grep -rn VerboseLogging and claims "only config/ files reference it" — which is itself factually wrong, since runner.go also matches that grep (in the spoiler comment) — and step 2 then explicitly states "runner/runner.go comments mention it but never reads it", followed by an explicit instruction: "Please remove VerboseLogging as dead config and keep all tests passing." Between the runner.go comment and the issue body handing over the exact grep command, the (partially incorrect) verification narrative, and the literal conclusion/fix instruction, the agent is given the answer rather than having to discover it via cross-file tracing — defeating the stated purpose of the eval case, which will mostly measure whether the agent follows an explicit instruction rather than performs dead-code tracing.

Suggestion: Remove or reword the spoiler comment in runner.go so it doesn't name VerboseLogging as dead, and rewrite input.yaml's issue body to describe symptoms/behavior only (e.g., "this field seems unused, can someone confirm and clean it up") without the exact grep command, the pre-stated conclusion, or the explicit "please remove" instruction. Also fix the factually incorrect claim in step 1 that only config/ files reference VerboseLogging.

[waynesun09]

MEDIUM — Unreachable applyReflect fallback ships as unscoped dead code inside a dead-code-detection fixture

applyToStruct() (this line) always finds that *Config satisfies the local configFields interface via its pointer-receiver SetField method, so applyReflect() (lines 45-69) — including its YAMLFields() interface check — is unreachable dead code; nothing in the repo implements YAMLFields. The local yamlField struct type declared inside applyReflect is also never referenced anywhere in the function body. None of this is mentioned in annotations.yaml's expected_files (only config/config.go, config/fields.go, config/config_test.go are listed) or in code_expectations, so it's unclear whether it's an intentional distractor or an oversight. In a fixture whose entire premise is "find and remove dead code," shipping additional real dead code that's out of the graded scope risks an agent reasonably generalizing the issue and touching this file (diverging from expected_files and affecting scoring), or confusing what "dead code" means in this exercise.

Suggestion: Either delete the unreachable applyReflect/YAMLFields/yamlField scaffolding (simplify yaml.go to just the SetField-based path used by the fixture), or, if it's an intentional decoy for scope discipline, say so explicitly in annotations.yaml/code_expectations so scoring and future maintainers can account for it.

[waynesun09]

MEDIUM — Budget copied from 001-fix-add with no observed baseline, and case shipped without an end-to-end harness run

max_turns: 60 / max_cost_usd: 4.00 are byte-identical to 001-fix-add's values, but 001-fix-add/annotations.yaml documents its numbers with actual observed CI baselines ("12 turns / $2.12 (CI run 29424512121), then 35 turns / $0.98 (CI run 30166455238)") and explains the chosen headroom multiplier. This annotations.yaml has no such comment at all. This PR's own test plan leaves "End-to-end fullsend eval code run (requires CI)" unchecked — the case has only been validated by go test ./... and eval/lint-cases.sh code, never by actually driving an agent through the real fullsend eval code harness — yet the PR justifies its design partly by citing an unrelated prior benchmark (fullsend#5808, N=13 pairs on a structurally different, single-file fixture) as if it generalizes to this new cross-file case. Given the PR itself describes 002 as meaningfully harder than 001 (multi-file tracing across 3 files vs. a one-line arithmetic fix), reusing 001's untouched budget without any baseline or real run is an unverified guess presented as final configuration.

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.

[waynesun09]

MEDIUM — PR description cites an unverifiable/apparently-fabricated "N=13 byte-for-byte identical fixes" benchmarking statistic

The PR body states as settled fact: "Pattern inspired by fullsend-ai/fullsend#5808, which produced byte-for-byte identical fixes across different model configurations in A/B benchmarking (N=13 pairs)". Fetching issue #5808 (body + all comments, including the triage-agent and prioritize-agent bot comments) directly from the live repo shows it is a real, structurally-similar dead-config-field report (RepoDefaults.AutoMerge) with a request to remove the dead field, but it contains no A/B benchmarking data, no mention of 13 model-configuration pairs, and no "byte-for-byte identical" comparison of any kind.

This is a distinct claim from the existing inline comment on annotations.yaml:16, which questions whether #5808's benchmark generalizes to this harder 3-file case — this finding is about the specific N=13/byte-for-byte statistic itself appearing to be fabricated or sourced from something not linked/verifiable anywhere in the cited issue, not about whether it's the right precedent to cite.

Suggestion: Either link the actual source of the N=13 A/B benchmark data (a run, dashboard, or discussion), or soften the claim to something verifiable, e.g. "this pattern is representative of real dead-code-removal issues we've seen (see #5808)" without the unverifiable statistic.

[waynesun09]

MEDIUM — Budget comment now cites a fabricated/unverifiable "baseline-30" benchmark and nonexistent commit SHA

The latest commit replaced the prior bare budget line with: "Reference: baseline-30 code-phase analytics (30 runs, claude-opus-4-6, commit f96750b) — median $2.29 / 27 turns, P75 $3.76 / 42 turns, P90 $4.66 / 61 turns." Verified git cat-file -t f96750b returns "Not a valid object name" — the cited commit does not exist in this repo. A repo-wide search for "baseline-30" matches only this one line — there is no dashboard, report, or artifact anywhere backing that label. This is a new problem introduced by this commit (distinct from the earlier "no comment at all" feedback, which this commit was addressing) — the content added to fix that gap is itself unverifiable. Contrast with 001-fix-add/annotations.yaml's convention of citing concrete, clickable CI run IDs (e.g. "CI run 29424512121").

Suggestion: Either link to the actual baseline-30 report/dashboard/run artifact (matching the concrete-CI-run-ID convention used in 001-fix-add), or soften the claim to avoid citing an unresolvable commit SHA and an unlinked analytics run, e.g. "budget set near the higher end of typical code-agent runs for multi-file tasks; revisit after first CI run with case-specific data."

[guyoron1]

Heyya !
Thanks for the review
Fixed:

• Replaced "baseline-30 / commit f96750b" budget reference with actual Round 4 run data (Path A run, Path B run).
• PR description now links to published benchmark data: quality eval, scoring, and the actual identical PRs from both paths: #813 vs #855.
• Spoiler, input.yaml, applyReflect, disclaimer already fixed in 68f3f70.

Open follow-ups: test coverage for runner validation / error-path fixes, code_expectations should mention embedded YAML literal, per-case correctness judge needs eval harness changes.

[waynesun09]

MEDIUM — New runner.go validation and per-attempt-timeout/cancellation logic has zero test coverage

The latest "Address PR review feedback and fix fixture bugs" commit added real behavioral logic as bug fixes: New() (here, lines 24-35) now validates Workers>=1 / Timeout>=1 / MaxRetries>=0, and runWithRetry() (lines 64-79) now uses context.WithTimeout per attempt instead of the previous overlapping-goroutine retry logic. Verified go test ./... on this fixture at PR head reports github.com/eval-org/taskrunner/runner [no test files]. Also verified no CI workflow or Makefile in this repo runs go test against this fixture at all. This logic was added after the earlier review round that flagged the underlying bugs (now resolved) and isn't covered by any existing thread — the fix itself has never been exercised by any automated test.

Suggestion: Add a small runner_test.go covering New() rejecting Workers<1/Timeout<1/MaxRetries<0, and one happy-path Run() test exercising the retry/timeout behavior. Consider wiring go test ./... for eval/code/repos/* fixtures into CI or eval/lint-cases.sh so future edits to fixture logic can't silently regress.

[waynesun09]

MEDIUM — Config error-path bug fixes (malformed YAML line, unknown config key) shipped with no regression test

The same bug-fix commit changed Unmarshal to return an error on a malformed line (here, previously silently skipped) and added a default: case in fields.go's SetField (line 36-38) to error on unknown config keys (previously always returned nil) — both explicitly framed in the commit message as responses to the earlier "Config typos silently ignored" finding (now marked resolved). Verified config_test.go at PR head still contains only TestDefaults, TestLoad, and TestLoadPartial — none feeds a malformed line or an unknown key through Load()/Unmarshal()/SetField(). The fix is asserted in the commit message but never exercised by the test suite.

Suggestion: Add two small test cases: one asserting Load() returns a non-nil error for a config file containing a malformed line (e.g. not a valid line), and one asserting an error for an unrecognized key (e.g. bogus_key: 1).

[waynesun09]

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 code_expectations says the agent must remove VerboseLogging from "the struct definition, the Defaults() return value, the SetField() switch case, and all test assertions" but never mentions that the raw embedded YAML string in TestLoad's content variable (here: verbose_logging: true) also needs to be edited. This matters given the concurrent fix in yaml.go/fields.go: since SetField now has a default: case returning fmt.Errorf("unknown config key: %s", key), leaving verbose_logging: true in the raw content while removing the Go-level field/case/assertions would make Load() fail with "unknown config key: verbose_logging" at test time. Because no judge actually runs go test against the resulting PR, an agent could plausibly ship this exact incomplete removal, touch all three expected_files, and still pass the eval.

Suggestion: Extend code_expectations to explicitly call out that the fixture's embedded raw YAML in config_test.go must be updated too (not just Go-level struct/assertions), and/or treat this as supporting evidence for adding a real correctness judge rather than relying solely on file-touch matching.

[waynesun09]

Review-only pass (no approval/changes-requested; not self-assigning).

HIGH — DCO check failing, blocks merge

The DCO check-run on the current head (f95f7d0) reports conclusion=action_required: "There are 2 commits incorrectly signed off." Confirmed via the commits API: commit 4846372 ("eval(code): ground 002 budget comment in baseline-30 analytics") has no Signed-off-by trailer, and commit f95f7d0 ("eval(code): ground 002 budget in Round 4 #5808 run data") also has no Signed-off-by trailer — while the branch's first two commits (4774e27, 68f3f70) do carry one. This is a currently-failing, blocking check not addressed by any existing review comment.

Suggestion: rebase and add sign-off to the two unsigned commits (e.g. git rebase HEAD~2 --signoff then force-push with lease), or squash the branch into signed-off commit(s) before merge.

[waynesun09]

MEDIUM — Budget still unvalidated against this fixture, and its own stated math contradicts the "well under" framing

This third revision cites a new, independently-verified source (Round 4 A/B benchmark, workflow runs 30729219887 / 30740388070, docs/ab-benchmark-round4/ in the linked fork) — the earlier fabricated "baseline-30"/nonexistent-SHA problem is resolved. However two issues remain:

1. The comment states upstream #5808 (a ~50k-LOC codebase) took 75–79 turns / $3.85–$4.02, and concludes this ~200-LOC fixture's budget "should be well under those numbers" — yet max_turns: 60 is only ~20% below the low end (75), and max_cost_usd: 4.00 sits at the very top of the cited range, not under it at all. That directly contradicts the comment's own framing.
2. The budget is still an unmeasured guess for this fixture — the PR's own end-to-end fullsend eval code test-plan checkbox remains unchecked, and the generalization drawn (dead-config removal being "highly deterministic") comes from a single tied pair on a structurally different, much larger codebase, not a run of this case.

Suggestion: either run this case once through the real harness and cite the observed turns/cost (matching 001-fix-add's convention of citing concrete CI run IDs for its own fixture), or lower max_cost_usd meaningfully below $3.85 (and/or max_turns further below 75) so the values actually match the "well under" claim, rather than reusing near-ceiling numbers from a 250x-larger reference codebase.

[waynesun09]

Review-only pass (no approval/changes-requested; not self-assigning).

[waynesun09]

HIGH — Residual doc comment still lets an agent identify the dead field by elimination, without cross-file tracing

A prior review round flagged an explicit spoiler comment ("Note: cfg.VerboseLogging is not checked anywhere — this is dead config") directly above Run(), and that exact line was removed (input.yaml was also rewritten to remove the grep hint and the pre-stated conclusion — both confirmed fixed at head). However, the line immediately above where that spoiler used to sit — "It uses cfg.MaxRetries, cfg.Timeout, and cfg.Workers." (this line) — was never touched and has been present since the very first commit. Verified at head: config/config.go's Config struct has exactly four fields (MaxRetries, Timeout, VerboseLogging, Workers), and this doc comment names exactly the three that are actually used. An agent only needs to open config.go, see the four fields, and compare against this one doc comment to identify VerboseLogging as unused by elimination — without reading any function body, fields.go, or config_test.go. This defeats the case's stated purpose ("this case tests cross-file dead-code removal — tracing symbol references across multiple files"), the same way the already-removed explicit spoiler did, just one inferential step removed. This is a distinct, previously-unflagged defect (no existing PR comment mentions this specific line), revealing that the earlier fix for the spoiler issue was incomplete.

Suggestion: Reword the comment so it doesn't enumerate the exact subset of used config fields, e.g. "Run executes all registered tasks with retry and timeout logic based on the runner's configuration." Let the agent discover which fields are actually referenced by reading the function bodies.

[waynesun09]

Review-only pass (no approval/changes-requested; not self-assigning).

MEDIUM — Shared eval/code/eval.yaml timeout comment is now stale for the heavier 002 case (eval/code/eval.yaml, not modified by this PR)

eval/code/eval.yaml is not touched by this PR, but PR 617 adds a new case (002-dead-config-field) that shares this file's execution.timeout: 1800 / EVAL_TIMEOUT: "1700" hard-kill window. The existing justifying comment block explicitly reasons about why 1800s is safe by saying the fixture "is a 2-line arithmetic bug and completes in well under a minute in practice, so this number essentially never fires" — that framing describes 001-fix-add only (these comments were added in commits predating this PR, all in the context of the original 001 fixture). Now that 002 shares the same eval.yaml with a materially larger budget (max_turns: 60, max_cost_usd: 4.00 vs 001's much smaller observed baseline) and a cross-file symbol-tracing task description, the comment's "essentially never fires" safety claim is no longer accurate for every case sharing this harness, and the PR doesn't acknowledge or re-justify this for 002.

Suggestion: Update the eval.yaml comment to acknowledge it now covers a second, heavier case, and confirm (or re-derive) that 1800s/1700s still gives adequate headroom for 002's 60-turn/$4 budget — or note explicitly that this remains an open risk to revisit once 002 has an observed CI runtime.

[waynesun09]

Review-only pass (no approval/changes-requested; not self-assigning).

[waynesun09]

MEDIUM — Budget comment mismatches turns and cost from two different 001-fix-add CI runs

The latest commit (ec5f406, "ground 002 budget comment in baseline-30 analytics"/"note 002 in eval.yaml") rewrote the lower-bound citation to: "Lower bound: 001-fix-add (same harness, trivial fixture) observed up to 35 turns / $2.12 (CI run 30166455238)." Checked against 001-fix-add's own annotations.yaml (eval/code/cases/001-fix-add/annotations.yaml on main, unchanged), the real data is: CI run 29424512121 = 12 turns / $2.12, and CI run 30166455238 = 35 turns / $0.98. The new text takes the turn count from one run (30166455238) and the cost from a different run (29424512121), then attributes the combined, non-existent data point to a single run ID that actually cost $0.98, not $2.12. This is a brand-new mistake introduced in the PR's most recent (current HEAD) commit, postdating every existing review comment on this file, and is the third time this same comment block has shipped inaccurate/unverifiable sourcing (previously: a fabricated "baseline-30/commit f96750b" reference, and a "well under" framing that contradicted its own cited numbers, both already flagged and partly addressed in earlier rounds).

Suggestion: Cite each run's real numbers separately and correctly, e.g. "35 turns (CI run 30166455238) and $2.12 (CI run 29424512121)" — don't merge two different runs' peak values into a single run-ID citation.

[waynesun09]

MEDIUM — New EVAL_TIMEOUT justification is logically backwards (2100s cannot bound a 1700s timeout)

Commit ec5f406 (current HEAD) rewrote the EVAL_TIMEOUT=1700 rationale to state that "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." This is backwards: 2100s is larger than the 1700s outer timeout, so the per-iteration budget cannot bound anything below 1700s — if a legitimate run (especially 002-dead-config-field's newly-added, heavier 60-turn/$4.00 budget) approaches even half of its 2100s per-iteration allowance, the 1700s harness-level EVAL_TIMEOUT fires first and kills it, contradicting the very next clause's "should still essentially never fire" conclusion. The same paragraph then admits "002 has no observed CI runtime yet, so treat its headroom as an open risk to revisit once a real run exists" — i.e., the confident claim and the acknowledged uncertainty sit side by side unreconciled. This text is new in the HEAD commit (added specifically to respond to a prior review round about 002 sharing this timeout window), so it postdates and is not covered by any existing PR comment.

Suggestion: Fix the reasoning (a larger per-iteration budget does not bound a smaller outer timeout) — e.g. clarify that the outer 1700s is the actual backstop and per-iteration 2100s is irrelevant to it, or that in practice runs finish well under budget for unrelated reasons. If 002 genuinely needs more headroom, consider raising EVAL_TIMEOUT or splitting timeout windows per case, and revisit the number once a real 002 CI run exists.

[waynesun09]

Review-only pass (no approval/changes-requested; not self-assigning).

[waynesun09]

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 617 and the gate job log for the latest run (31710102745, 2026-08-13T14:25) show .github/scripts/check-e2e-authorization.sh printing authorized=false reason=unauthorized with 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-test label (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.

[waynesun09]

Review-only pass (no approval/changes-requested; not self-assigning). Findings focus on eval.yaml's removed_symbols judge and capture-fixture.sh's supporting diff-fetch logic.

[waynesun09]

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 deleted/survivors per symbol across ALL diff lines from every touched hunk, with no correlation to which file/occurrence was deleted. It only requires "at least one deletion line across the whole diff" and "zero non-deletion lines mentioning the symbol in any touched hunk". Because VerboseLogging/verbose_logging is legitimately deleted in config.go's struct field+yaml tag and fields.go's SetField case regardless, deleted is already non-zero from those alone. If the agent misses the third required edit — the raw verbose_logging: true literal in config_test.go's TestLoad string (which annotations.yaml's own code_expectations now explicitly calls out: "SetField rejects unknown keys, so leaving it breaks Load()") — that untouched line never appears in the diff at all (unified diff only shows lines inside changed hunks/context windows), so it contributes neither a deletion nor a survivor. The judge would report "All declared symbols removed cleanly" and expected_files would also pass (config_test.go was touched elsewhere), so a PR that ships a broken Load() could pass the entire eval at min_pass_rate 1.0. No judge runs the fixture's tests to catch this.

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.

[waynesun09]

MEDIUM — removed_symbols judge's diff-metadata skip-list omits rename/mode-change header lines

Verified: meta = ("--- ", "+++ ", "diff ", "index ", "@@") is the only skip-list for non-content diff lines. Unified/git diff output also emits rename from <path>, rename to <path>, old mode <mode>, new mode <mode>, similarity index NN%, and Binary files ... differ lines for renamed/mode-changed/binary files, none of which are in the skip list. A future removal case whose PR renames a file to a path containing the removed symbol (e.g. rename to config/VerboseLoggingHandler.go) would have that line misclassified as a non-deletion "survivor" line and fail the case even though the symbol was legitimately removed elsewhere.

Suggestion: Add the rename/mode-change/binary line prefixes to the meta tuple, or restrict the content-line check to only lines starting with ' ', '+', or '-' rather than trying to enumerate every non-content prefix.

[waynesun09]

MEDIUM — removed_symbols uses unanchored substring matching, risking false-fails on comments/renamed identifiers

Verified: if sym not in line or line.startswith(meta): continue is a plain substring test with no word-boundary or comment-awareness. This means: (1) a legitimate explanatory comment/commit-message-style line like // Removed unused VerboseLogging in an added or context line would count as a "survivor" and fail an otherwise-correct removal; (2) renaming the field to something like VerboseLoggingEnabled would false-fail because VerboseLogging is a substring of the new identifier; (3) any unchanged context line in a touched hunk that happens to mention the symbol name (e.g. in a docstring) also counts as a survivor. This is stricter than the actual intent and can penalize correct fixes.

Suggestion: Use word-boundary-aware matching (e.g. regex \bVerboseLogging\b) and/or restrict the survivor check to added lines only (line.startswith('+')), letting unchanged context lines mention the symbol without failing the case.

[waynesun09]

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 state.get("pull_requests"), a nested array populated only by capture-fixture.sh's issue-fixture-type branch (lines 145-214). The pull_request fixture-type branch (lines 216-315, used by human-authored-PR fixtures like eval/fix's case type) writes the PR's fields flat at the top level of fixture-state.json, never sets diff_fetch_failed, and never calls fetch_pr_diff/writes output/pr-<num>.diff at all (fetch_pr_diff is only invoked inside the issue case's while-loop at line 176). If removed_symbols is ever copied into a pull_request-type eval suite (e.g. eval/fix or eval/review) as-is, it will silently return "No open/merged PR to inspect" for every run regardless of what the PR actually contains.

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.

[waynesun09]

MEDIUM — fetch_pr_diff runs unconditionally for every PR in the shared issue-fixture capture path, even for cases that don't declare removed_symbols

Verified: inside the issue fixture-type branch's per-PR loop, fetch_pr_diff "$num" is called unconditionally for every PR found, regardless of whether the case's annotations.yaml declares removed_symbols. capture-fixture.sh's after_each hook is wired into eval/code/eval.yaml generically (not gated per-case), and only 002-dead-config-field currently declares removed_symbols (001-fix-add does not). This adds an extra gh pr diff API call (with its own 3-attempt retry_cmd backoff and possible WARNING logs on transient failure) to every issue-type PR capture that doesn't need the diff content, adding latency and failure surface for no benefit on cases like 001-fix-add.

Suggestion: Gate the fetch_pr_diff call on whether the case declares removed_symbols (e.g. via an env var passed from the harness, since annotations are already available to hooks), so the extra API call only fires for cases that actually consume the diff artifact.

[waynesun09]

MEDIUM — Transient gh pr diff failure hard-fails removed_symbols (min_pass_rate 1.0) even when the underlying fix is correct

Verified: unlike the pull_request branch's resolve_head_sha, which explicitly polls up to 6 times with escalating backoff to handle push-propagation lag before trusting a SHA, the issue-branch's fetch_pr_diff has no equivalent readiness poll — it relies solely on retry_cmd's 3 attempts (~3s of backoff) immediately after PR creation. If gh pr diff fails after those retries (e.g., due to a brief GitHub API replication delay right after the PR is opened), diff_fetch_failed is set true and the removed_symbols judge (min_pass_rate: 1.0 in eval.yaml's thresholds) hard-fails the entire eval outright — even when pr_created and expected_files already passed, i.e., even when the agent's fix was correct.

Suggestion: Either add a short readiness poll/retry specifically for the diff fetch (reusing or adapting the existing SHA-polling pattern), or have removed_symbols treat a diff-fetch failure as inconclusive/soft-fail rather than a hard min_pass_rate-1.0 failure when the other content-agnostic judges already passed.

[guyoron1]

Heyaaa ! thanks for the review 🙏

All six are fixed in 14bb716 — reproduced each one first, they all held.

• HIGH, aggregate deletions — removed_symbols is now a mapping of symbol → the files it must be deleted from, so every site is required on its own. The missed verbose_logging: true literal in TestLoad now fails the case instead of slipping through on config.go's deletion.
• Metadata prefixes — took your alternative: bucket lines per file and treat only ' ', '+', '-' as content. rename/mode/binary fall out for free.
• Unanchored matching — word boundaries now, so VerboseLoggingEnabled no longer reads as the old symbol.
• pull_request fixture — documented in the judge description; reuse there needs that branch to emit a diff artifact first.
• Unconditional fetch_pr_diff — gated on the case declaring removed_symbols (read from CASE_SOURCE_DIR). Unknown dir still captures, and I checked no other suite reads output/pr-*.diff.
• Transient fetch failure — added a readiness poll like resolve_head_sha (~10s worst case, budget peaks ~19s against the 60s timeout). Left the eventual failure hard, since silently passing a content check on missing evidence seemed worse — happy to flip it if you'd rather it be inconclusive.

One deliberate difference: you offered "survivors on added lines only", and I kept context lines but exempted comment-only lines instead. Dropping context would reopen the HIGH finding from the other side — a literal left inside a hunk the fix did touch shows up as context. The comment exemption still covers all three of your false-fail examples.

Fair hit on my verification last round, so it's pinned in the repo this time — both wired into make script-test:

• eval/scripts/removed-symbols-judge-test.py — runs the actual judge body extracted from eval.yaml (no YAML dep, CI installs none) across 11 diffs, including the partial-removal case above
• eval/scripts/capture-fixture-test.sh — the capture gate, including the unknown-dir fallback

Also merged main to clear the conflict with check-rollup-result-test.sh.

Still can't run the case itself — functional tests and Script tests are both approval-gated on this fork PR, so ok-to-test after this push would be much appreciated 🙏

[waynesun09]

Review sweep: 4 findings on the new removed_symbols judge internals and on case numbering.

[waynesun09]

MEDIUM — is_comment's bare * prefix exempts real code, letting a survivor slip past the judge

is_comment() (eval.yaml:256-258) returns True for any diff line whose body starts with //, #, or *. A bare leading * is not a comment marker in Go — it is a pointer dereference. Empirically driven against the shipped judge body extracted from head f5dea5c (real annotations, real per-file diff), an otherwise-complete removal plus an added runner.go line +\t*VerboseLogging = true returns:

(True, "All declared symbols removed cleanly from their declared files: ['VerboseLogging', 'verbose_logging']")

The symbol plainly survives in executable code and the judge passes. This is a false-negative in the exact anti-cheat the judge was added for (commit 14bb716 "correlate removed_symbols with per-file deletion sites"), and the new code is not covered by any existing review thread. eval/scripts/removed-symbols-judge-test.py only exercises //, #, and * continuation forms, so the gap is untested.

Suggestion: Drop bare * from the prefix tuple (a C-style block-comment continuation cannot be distinguished from a Go pointer deref by prefix alone), or gate the heuristic on file extension so .go uses //, .py/.yaml use #, and doc files are exempted wholesale. Add a *ptr = value survivor case to removed-symbols-judge-test.py asserting it is NOT treated as a comment.

[guyoron1]

Fixed in 0b42b80. Reproduced first: +\t*VerboseLogging = true in runner.go passed on f5dea5c. Comment prefixes are now keyed by extension (// for .go/.js/.ts/.c/.rs..., # for .py/.sh/.yaml/.toml/.mk...); bare * is gone, and both a block-comment continuation and an unknown extension get no exemption — fail closed, documented in the judge description. Pinned in removed-symbols-judge-test.py: "pointer deref is not a comment", "block-comment continuation fails closed", "comment marker from another language is not exempt", "unknown extension gets no comment exemption" — all four fail against the previous judge body.

[waynesun09]

MEDIUM — Survivor scan covers every file in the diff, so documenting the removal fails a correct fix at min_pass_rate 1.0

The survivor loop (eval.yaml:265-272) iterates per_file.items() — every path in the captured PR diff — not just the files declared for that symbol, and the only escape hatch is the ///#/* prefix check. The fixture ships eval/code/repos/taskrunner/README.md and removed_symbols is wired at min_pass_rate: 1.0 (eval.yaml:345-346), so a correct removal that also documents itself turns Functional Tests red. Empirically confirmed against the shipped judge at head f5dea5c:

• complete removal + +- Removed the unused verbose_logging option (VerboseLogging field). in README.md → (False, "Symbols not fully removed: VerboseLogging: survives in ['README.md']; verbose_logging: survives in ['README.md']")
• complete removal + +The verbose_logging key is no longer supported. in README.md → (False, "... verbose_logging: survives in ['README.md']")

The - markdown bullet is the most common list marker and is the one form the exemption omits, which is inconsistent with the judge's own description (eval.yaml:201-202: "comment-only lines are exempt so a changelog-style note may mention it"). This is a residual gap in the fix for the earlier thread at eval.yaml:231 (unanchored substring matching): that fix added is_comment and word boundaries but did not scope the scan or widen the marker list.

Suggestion: Scope the survivor scan to the files declared for that symbol (or to source-code extensions), rather than every path in the diff. If a global scan is genuinely wanted, allow-list doc paths (README/CHANGELOG/docs/**) and widen the marker list to -, >, --, ;, <!--. Encode the --bullet and prose-doc cases in removed-symbols-judge-test.py so the chosen narrowness is a tested decision.

[guyoron1]

Fixed in 0b42b80. Reproduced both README lines on f5dea5c. Went with your first option plus the doc allow-list: documentation files (*.md, *.markdown, *.rst, *.txt, *.adoc, docs/**) are exempt from the survivor scan wholesale, so - bullets / prose / > need no marker list. Deliberately did NOT narrow the scan to the declared files only — your other finding on this sweep (*VerboseLogging = true in an undeclared runner.go) is exactly the survivor that would disappear, so every non-doc file in the diff is still scanned. Tests: "README bullet and prose may mention the symbol" (pass) and "survivor in an undeclared yaml fixture" (fail, config/testdata/full.yaml context line) so the chosen width is a tested decision.

[waynesun09]

MEDIUM — Whole-file deletions are bucketed under None, producing a false "no deletion line" failure

The bucketing loop keys lines off the +++ b/<path> header and sets current = None when that header is +++ /dev/null (eval.yaml:242-245). Git emits +++ /dev/null for a whole-file deletion, so every - line of a deleted file lands in per_file[None] and never matches a declared path; the --- a/<path> line that carries the real identity is skipped unconditionally at line 246. Empirically confirmed against the shipped judge at head f5dea5c with a diff that deletes config/config_test.go outright (both symbols on removed lines):

(False, "Symbols not fully removed: VerboseLogging: no deletion line in ['config/config_test.go']; verbose_logging: no deletion line in ['config/config_test.go']")

Any survivor in that bucket is also reported as <unknown file> (line 271). This cannot fire on 002's intended fix (all three declared files must survive), but eval.yaml:190-207 documents removed_symbols as a reusable annotation-driven primitive for other removal cases, where deleting a now-empty file is the natural fix. current is also never reset at a diff --git boundary, so a mode-only block with no +++ header inherits the previous file's identity.

Suggestion: Track the last --- a/<path> value and, when +++ is /dev/null, set current to that path (stripping the a/ prefix) — a whole-file deletion removes every symbol in it and should satisfy the requirement. Also reset current = None on each diff --git line. Add a whole-file-deletion scenario to removed-symbols-judge-test.py's CASES.

[guyoron1]

Fixed in 0b42b80. Reproduced the no deletion line in [config/config_test.go] on a deleted file at f5dea5c. The --- a/<path> is now tracked and used as the identity when +++ is /dev/null, and diff --git resets both so a header-less block cannot inherit the previous file. Test: "whole-file deletion satisfies the declared file" (fails against the previous body). Kept the <unknown file> label for the remaining None bucket since it can now only come from a malformed diff.

[waynesun09]

MEDIUM — Case ordinal 002 collides with open PR #682, which adds a second 002- case and edits the same eval.yaml block

Verified live: open PR #682 ("feat(#677): add needs_input pushback for the code agent", branch feat/677-code-needs-input, state OPEN) adds eval/code/cases/002-push-back-on-nonsense/annotations.yaml, input.yaml and a repo symlink, and also modifies eval/code/eval.yaml. Because the two case directories have different names, git merges both cleanly and main ends up with two distinct 002- cases; meanwhile the overlapping eval/code/eval.yaml edit (both PRs rewrite the top-level description block that documents removed_symbols / case behaviour) will conflict for whichever PR lands second. Not raised on any existing thread.

Suggestion: Renumber this case to 003-dead-config-field, or coordinate with #682 on who keeps 002, and rebase onto whichever lands first so the eval.yaml description edits are reconciled deliberately rather than through conflict resolution.

[guyoron1]

Renumbered to 003-dead-config-field in 0b42b80 (dir, the two comment mentions in eval.yaml, PR title/body). Numbering gaps already exist in this tree (eval/fix/cases has 001 and 003), and capture-fixture.sh/tests key on the annotations, not the directory name, so nothing else moves. The eval.yaml description-block overlap with #682 is real but small and outside the judge body — happy to rebase onto whichever lands first; the branch is currently a clean merge against main (git merge-tree reports no conflicts at 816b89b).

[guyoron1]

Hwyaaaa !

• Bare * as comment — prefixes are now per-extension (// Go-likes, # shell/python/yaml/...). Block-comment continuations and unknown extensions get no exemption and fail closed.
• Survivor scan vs docs — documentation (*.md/*.rst/*.txt/*.adoc, docs/**) exempt wholesale; every other file in the diff is still scanned, so the undeclared-runner.go survivor from your first finding stays caught.
• Whole-file deletions — --- a/ path is used when +++ is /dev/null; diff --git resets identity.
• 002 collision — renumbered to 003-dead-config-field; clean merge against current main.

removed-symbols-judge-test.py went 11 → 18 cases; the 6 new behaviours fail against the previous body (negative-checked by running the new test file over git show HEAD:eval/code/eval.yaml). PR body refreshed to describe the judge and a measured test plan.

Still approval-gated on this fork for functional tests + Script tests — ok-to-test after this push would be much appreciated : ))

[waynesun09]

Review-only pass (no approval/changes-requested; not self-assigning). 5 findings on the 0b42b80 judge rework (doc allow-list, per-file schema, diff header parsing) and on the now-two-case timeout budget.

[waynesun09]

MEDIUM — is_doc exempts real source files: any path containing a docs segment escapes the survivor scan entirely

The doc allow-list added in 0b42b80 (the fix for the README false-failure thread) applies the path test before any extension test: return p.endswith(DOC_SUFFIXES) or p.startswith("docs/") or "/docs/" in p. Any file whose path merely contains a docs segment — including Go, Python, YAML sources — is exempted from the survivor scan wholesale. Reproduced against the shipped judge body extracted from head 0b42b80: a diff that correctly deletes the field from config/config.go but adds +\tcfg.VerboseLogging = true to pkg/docs/gen.go returns (True, "All declared symbols removed cleanly from their declared files: ['VerboseLogging']").

The judge's own description contradicts this in two places: it promises docs/** (a path glob, implemented here as an unanchored substring) and claims "Unknown extensions get no comment exemption ... fail closed" — but the path branch bypasses the extension logic entirely, so a source file under any docs directory gets a blanket exemption rather than failing closed. This is a defect in the new fix's implementation, not in its declared intent (documentation files exempt); a survivor in a source file is exactly what the same round's reply said must keep failing.

Secondary: .txt is in DOC_SUFFIXES, so config/testdata/golden.txt with +VerboseLogging: true also passes (verified) — but .txt was explicitly listed as intentional in the fix reply, so only the scoping is worth revisiting.

Suggestion: Make the path exemption extension-aware — skip it for any path whose extension is in COMMENT_PREFIXES (a known source type) even under a docs directory, and match docs as a real path segment (p.split('/') membership) rather than a substring, or anchor it to a top-level docs/ prefix only. Add judge test cases for pkg/docs/gen.go and a docs-dir YAML/Python file.

[waynesun09]

MEDIUM — Null or empty per-symbol file list silently disables the per-file deletion requirement; a scalar crashes the judge

The judge validates the top-level shape (isinstance(symbols, dict)) and the test suite deliberately rejects the pre-per-file bare-list schema so "accepting it silently would reinstate the gap this judge closes" — but the per-symbol value is never validated. missing = [f for f in (required_files or [])] iterates nothing when the value is None or [].

Verified against the shipped judge body at head 0b42b80: removed_symbols: {VerboseLogging: null} and {VerboseLogging: []} both return (True, "All declared symbols removed cleanly ...") on a diff containing no deletion of the symbol at all — silently degrading to survivor-only checking, i.e. exactly the gap the per-file schema was introduced to close, reachable via a YAML typo (key written with the file list omitted or mis-indented). Because the survivor scan still runs, it is a partial no-op rather than an obvious pass, which makes it harder to notice at min_pass_rate: 1.0.

Secondary, also verified: {VerboseLogging: 5} raises TypeError: 'int' object is not iterable inside the check body, and {VerboseLogging: {a: 1}} iterates dict keys and reports no deletion line in ['a'].

Suggestion: After the isinstance(symbols, dict) guard, validate each value the same way the legacy list schema is already rejected: accept a non-empty str or non-empty list[str], otherwise return False, f"{sym}: removed_symbols entry must list at least one file". Add a negative test alongside the existing "legacy list schema is rejected rather than ignored" case.

[waynesun09]

MEDIUM — In-hunk content lines beginning with -- / ++ are consumed as file headers, in both fail-closed and fail-open directions

The bucketing loop tests line.startswith("--- ") / line.startswith("+++ ") at any position in the stream, with no hunk-range tracking, and the accompanying comment asserts the invariant "Only ' ', '+' and '-' start content lines; everything else is metadata" — which is false for content whose own text begins with -- or ++.

Both directions reproduced against the shipped judge body at head 0b42b80:

• fail-closed — a deleted SQL/Lua/Haskell comment line -- VerboseLogging column serialises as --- VerboseLogging column, is consumed as a header and never counted as a deletion, so a correct removal returns (False, "VerboseLogging: no deletion line in ['db/migrate.sql']").
• fail-open — inside config.go's hunk, an added line whose text begins with ++ (+++ b/NOTES.md) is consumed as a +++ header and rebinds current to a doc path mid-hunk, so the immediately following survivor +\tcfg.VerboseLogging = true lands in the doc bucket and is exempted: the judge returns (True, "All declared symbols removed cleanly ...").

This is the inverse of the already-posted metadata-skip-list finding (metadata counted as content); here content is counted as metadata. Zero impact on the current Go fixture, but the judge is advertised as reusable and the file-identity logic was just reworked in 0b42b80.

Suggestion: Only honour --- /+++ as headers when not inside a hunk — parse the @@ -a,b +c,d @@ counts and treat lines as content until both are exhausted, or at minimum require the header to immediately follow diff --git / index / old mode / new mode / similarity index. Add judge test cases for a deleted -- line and an added ++ line.

[waynesun09]

MEDIUM — New two-case timeout paragraph is false at job level: 2 x (before_each + 1700s + after_each) exceeds the 45-minute CI cap

The comment block rewritten in 0b42b80 adds: "Two cases now share this window ... 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." That is true per case and false for the job.

Verified: execution.timeout: 1800 and execution.parallelism: 1 are per-case (the pinned harness resolves timeout_s once from config.execution.timeout and passes it into each _run_case; effective parallelism is forced to 1), and .github/workflows/functional-tests.yml:207 caps the whole matrix leg at timeout-minutes: 45 (2700s) with one leg per agent, not per case. Two cases now cost up to 2 x (before_each 120 + EVAL_TIMEOUT 1700 + after_each 60+30) = 3820s against a 2700s cap. A hang in the first case consumes 1910s; job setup (podman alone routinely ~190s per this file's own text) leaves roughly 300s for the second case, so the second case is killed by the GitHub Actions job timeout well before its own 1700s backstop fires.

The same block concedes a job-level timeout yields "no artifacts at all", so the documented graceful-degradation ladder (partial metrics.json plus a readable judge failure) is unreachable for a hang in either case. The derivation the block cites — that 1700s was chosen because the real envelope "cannot fit inside this CI job's 45-minute cap" — was computed for one case and was not updated for two.

Suggestion: Restore real headroom rather than only annotating it as "an open risk": lower EVAL_TIMEOUT/execution.timeout so 2 x (before_each + EVAL_TIMEOUT + after_each) + setup fits under 2700s, or split the cases into separate matrix legs (one case per job), or raise timeout-minutes on the functional-tests job. Then correct the "binding limit" sentence so the stated derivation matches the new case count.

[waynesun09]

MEDIUM — Per-file check requires only one deletion line, so a second occurrence of the symbol in the same file outside the diff hunk still passes

The per-file map added to close the "deleted somewhere" gap only requires any(l.startswith('-') and mentions(sym, l)) per declared file — one deletion line satisfies a file no matter how many occurrences it holds — and the survivor scan only sees lines the diff actually captured.

In this fixture the two sites in config.go are 11 lines apart (the struct field VerboseLogging with its yaml:"verbose_logging" tag at line 19, VerboseLogging: false, in Defaults() at line 30), further than git's default 3 lines of context, so a fix that deletes only the struct field leaves the Defaults() occurrence outside every hunk: it is neither a required deletion nor a visible survivor. Verified against the shipped judge body at head 0b42b80 — a diff deleting only the struct-field line returns (True, "All declared symbols removed cleanly from their declared files: ['VerboseLogging']"), even though the resulting tree does not compile. The same shape repeats in config_test.go (TestDefaults ~line 17, TestLoad ~line 28, TestLoadPartial ~lines 76-77).

annotations.yaml's rationale overstates what the schema buys — "Naming the sites makes each one required" — it makes each file required, not each site; and the judge description's caveat only disclaims files the PR never touched, not untouched regions of touched files. No judge runs the fixture's tests, so a non-compiling partial removal has no other backstop at min_pass_rate: 1.0.

Suggestion: Extend the annotation schema to enumerate expected deletion sites per file (or a minimum deletion count per file, e.g. config/config.go: 2) and have the per-file check require every declared site / the declared count rather than a single match. Reading full file contents is not an option here — the judge only sees the captured diff artifact.