-
Notifications
You must be signed in to change notification settings - Fork 16
test(eval): add 003-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
14bb716
f5dea5c
0b42b80
9160864
35f1b62
0a39bd7
6d658f2
4bcc8e2
37bbe07
f16bb61
c2f55e9
09eee85
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,58 @@ | ||
| state: open | ||
|
|
||
| expected_files: | ||
| - config/config.go | ||
| - config/fields.go | ||
| - config/config_test.go | ||
|
|
||
| labels: | ||
| forbidden: [] | ||
|
|
||
| # Consumed by eval.yaml's removed_symbols judge: for each symbol, each file | ||
| # maps to the minimum number of deletion lines that must name the symbol, | ||
| # and the symbol 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. | ||
| # | ||
| # Counts rather than a bare file list on purpose: one deletion line per | ||
| # file is satisfied by the struct field alone, while the `VerboseLogging: | ||
| # false,` in Defaults() sits 11 lines away — outside git's 3-line context, | ||
| # so it is neither deleted nor visible as a survivor, and the tree no | ||
| # longer compiles. The counts are the lines that name the symbol today | ||
| # (`grep -n` in eval/code/repos/taskrunner/config/), excluding the doc | ||
| # comment above the field, which a fix may legitimately leave or drop. | ||
| removed_symbols: | ||
| VerboseLogging: | ||
| config/config.go: 2 # struct field, Defaults() | ||
| config/fields.go: 1 # c.VerboseLogging = v | ||
| config/config_test.go: 6 # TestDefaults, TestLoad, TestLoadPartial (2 each) | ||
| verbose_logging: | ||
| config/config.go: 1 # yaml tag on the struct field | ||
| config/fields.go: 1 # case "verbose_logging" | ||
| config/config_test.go: 1 # raw literal in TestLoad | ||
|
|
||
| # Code agent budgets (sandbox work + retries). Unmeasured ceiling, and to be | ||
| # precise about what "unmeasured" means here: the eval harness has never | ||
| # executed this case at all. Every Functional Tests run on the branch that | ||
| # adds it was gated off by check-e2e-authorization.sh (external contributor, | ||
| # no ok-to-test label), so nothing below is derived from a run of this | ||
| # fixture — the numbers are a ceiling, not an observation. Re-derive from | ||
| # the first real run, which needs a maintainer to apply ok-to-test. | ||
| # 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 |
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 — 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) addseval/code/cases/002-push-back-on-nonsense/annotations.yaml,input.yamland areposymlink, and also modifieseval/code/eval.yaml. Because the two case directories have different names, git merges both cleanly and main ends up with two distinct002-cases; meanwhile the overlappingeval/code/eval.yamledit (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.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.
Renumbered to
003-dead-config-fieldin 0b42b80 (dir, the two comment mentions in eval.yaml, PR title/body). Numbering gaps already exist in this tree (eval/fix/caseshas 001 and 003), andcapture-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-treereports no conflicts at 816b89b).