[dead-code] chore: remove dead functions — 1 function removed#43212
Conversation
Remove HasModelPricingResolver from Compiler since it has no callers outside test files. Remove its two exclusive test functions that tested nothing else. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43212 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. All test changes are pure deletions (removal of two test functions for the dead HasModelPricingResolver function). Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Removes a dead exported helper (Compiler.HasModelPricingResolver) from the workflow compiler API and deletes the CLI tests that depended on that helper.
Changes:
- Removed
(*workflow.Compiler).HasModelPricingResolverfrompkg/workflow/compiler_types.go. - Removed two tests in
pkg/cli/compile_compiler_setup_test.gothat asserted models.dev pricing resolver registration via that helper.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_types.go | Removes the HasModelPricingResolver method from Compiler. |
| pkg/cli/compile_compiler_setup_test.go | Removes tests that validated models.dev pricing resolver setup behavior via the removed method. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
| } | ||
| } | ||
|
|
||
| // TestSetupRepositoryContext_ValidScheduleSeedLocksSlug verifies that when |
There was a problem hiding this comment.
Covered by commit 8c41a0a: I restored behavioral coverage in pkg/cli/compile_compiler_setup_test.go for both default resolver wiring and DisableModelsDevLookup skip behavior in createAndConfigureCompiler.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM
Dead code removal is clean and complete:
HasModelPricingResolver()had no callers in the codebase — safe to remove.- The two deleted tests only exercised this dead getter; they provide no coverage value once the function is gone.
- The underlying field (
modelPricingResolver), its setter, and all real usage paths remain intact.
No actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14 AIC · ⌖ 6.09 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — this is a clean, well-executed dead code removal. Approving.
📋 Key Themes & Highlights
Key Themes
- Clean removal:
HasModelPricingResolver()had zero remaining call sites — confirmed bygo buildandgo vet - Tests correctly removed: The two tests that validated the deleted method are properly removed alongside it
Positive Highlights
- ✅ Automated dead-code analysis correctly identified a predicate with no callers
- ✅
SetModelPricingResolver(the setter) and the underlyingmodelPricingResolverfield remain — the capability is preserved, only the unnecessary public predicate was removed - ✅ Verification checklist complete (
go build,go vet,make fmt) - ✅ 0 additions / 19 deletions — pure shrinkage, no regression risk
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 28.9 AIC · ⌖ 5.58 AIC · ⊞ 6.6K
Comment /matt to run again
There was a problem hiding this comment.
Non-blocking observation: test coverage gap on DisableModelsDevLookup
The dead-code removal is correct — HasModelPricingResolver has no remaining callers and the production change is clean. One concern worth addressing before or shortly after merge: the two deleted tests were the only coverage for the DisableModelsDevLookup conditional in compile_compiler_setup.go. That branch is now invisible to the test suite.
🔎 Code quality review by PR Code Quality Reviewer · 60 AIC · ⌖ 5.03 AIC · ⊞ 5.4K
Comment /review to run again
| @@ -8,20 +8,6 @@ | |||
| "github.com/github/gh-aw/pkg/workflow" | |||
There was a problem hiding this comment.
Test coverage for DisableModelsDevLookup branch is now zero: the two removed tests were the only safety net verifying that createAndConfigureCompiler wires (or skips) SetModelPricingResolver based on DisableModelsDevLookup.
💡 Why this matters and suggested fix
The conditional at compile_compiler_setup.go:119-125:
if config.DisableModelsDevLookup {
// no resolver registered
} else {
compiler.SetModelPricingResolver(FindOrFetchModelPricing)
}...is now completely untested at the CLI setup level. If the condition is accidentally inverted, the else branch dropped, or the wrong resolver wired in a future refactor, nothing will catch it.
Options:
- Keep a package-internal (unexported) helper instead of deleting the method entirely:
// in compiler_types.go — unexported, tests-only visibility via same package
func (c *Compiler) hasModelPricingResolver() bool {
return c.modelPricingResolver != nil
}- Replace the deleted tests with ones that inspect resolver behaviour indirectly (compile a minimal workflow that needs pricing and assert the resolver is/isn't called).
- At minimum, add a comment in the test file noting that this branch is intentionally not covered at the unit level.
There was a problem hiding this comment.
Addressed in 8c41a0a with restored CLI setup tests, and documented the remaining reflective coupling note in 0a49da8. The DisableModelsDevLookup branch now has explicit test coverage again.
|
@copilot run pr-finisher skill |
Ran the pr-finisher pass on this PR. No additional code changes were needed on top of |
|
@copilot please run the Run: https://github.com/github/gh-aw/actions/runs/28671435759
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented and pushed coverage restoration for |
…ver-437451664ba8954e
|
@copilot please run the
|
Ran a fresh pr-finisher pass on current HEAD
CI on this branch head remains maintainer-triggered; please re-run required workflows after this pass. |
pelikhan
deleted the
chore/dead-code-removal-hasmodelpricingresolver-437451664ba8954e
branch
Summary
Removes the dead exported method
HasModelPricingResolver()fromworkflow.Compilerand replaces its test-only usage with a package-private reflection helper that preserves behavioral coverage without re-exporting internal state.Changes
pkg/workflow/compiler_types.goHasModelPricingResolver() boolfrom*Compiler.modelPricingResolverfield) with no production caller.pkg/cli/compile_compiler_setup_test.goreflectimport.hasModelPricingResolver(compiler *workflow.Compiler) boolthat usesreflect.ValueOfto inspect the privatemodelPricingResolverfield — preserving the same assertions without requiring a public API.TestCreateAndConfigureCompiler_RegistersModelPricingResolverByDefaultandTestCreateAndConfigureCompiler_SkipsModelPricingResolverWhenDisabledto call the new helper instead of the removed method.workflow.Compiler.API Surface
(*Compiler).HasModelPricingResolver()pkg/workflowMotivation
Dead exported functions add noise to the public API surface, can be mistakenly relied upon by callers, and create maintenance burden. The method existed solely to support two test assertions; inlining the check via reflection keeps the coverage intact without widening the API.
Test Impact
No tests removed. Both compiler-setup tests continue to assert the same invariants via the reflection helper. No behavioural change.