[dead-code] chore: remove dead functions — 5 functions removed#38640
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38640 does not have the 'implementation' label and has only 1 new line of code in business logic directories (≤100 threshold). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Removes several unused helper functions and their now-obsolete tests across the CLI, parser, and workflow compiler environment code, reducing surface area and maintenance overhead.
Changes:
- Deleted dead env-resolution helper
ResolveDefaultMaxDailyAICreditsand its unit test. - Removed unused nested-import parsing helpers and a lenient-parsing test that was exclusive to them.
- Removed the unused domain “insight” analysis API and the unused
setNonInteractiveCIEnvwrapper + corresponding tests/imports.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compilerenv/manager.go | Removes the daily-credits resolver function from compilerenv. |
| pkg/workflow/compilerenv/manager_test.go | Deletes tests that only covered the removed daily-credits resolver. |
| pkg/parser/import_bfs.go | Removes unused nested import entry parsing helpers. |
| pkg/parser/import_bfs_test.go | Removes a unit test that only covered the deleted lenient parsing helpers. |
| pkg/cli/outcome_domain_breakdown.go | Removes unused DomainInsight / AnalyzeDomainPerformance API and cleans imports. |
| pkg/cli/mcp_validation_test.go | Renames/reduces tests after removing the cmd-env wrapper; keeps coverage for env-copy helper (but currently misses a key case). |
| pkg/cli/mcp_helpers.go | Removes unused exec-based wrapper and its import; keeps env helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/cli/mcp_validation_test.go:67
- The renamed TestWithNonInteractiveCIEnv now only covers the case where CI is already present (and overridden). The removed sub-test for “adds CI when missing” still applies to withNonInteractiveCIEnv and should be retained to keep coverage for that behavior.
func TestWithNonInteractiveCIEnv(t *testing.T) {
t.Run("returns copied env with CI forced on", func(t *testing.T) {
input := []string{"CI=false", "HOME=/tmp/test-home"}
output := withNonInteractiveCIEnv(input)
pkg/workflow/compilerenv/manager.go:53
- ResolveDefaultMaxDailyAICredits is removed here, but pkg/workflow/compilerenv/spec_test.go still states that ResolveDefaultMaxDailyAICredits is “retained” (around lines 64–69). That comment will be incorrect after this PR; please update the spec test commentary to match the current API surface.
// ResolveDefaultMaxAICredits returns the resolved max AI credits default, checking
// the enterprise env var GH_AW_DEFAULT_MAX_AI_CREDITS.
// Falls back to fallback (built-in default) when the env var is unset or invalid.
//
// A value of -1 is preserved and signals that budget enforcement and token
- Files reviewed: 7/7 changed files
- Comments generated: 1
| func countTotalAttempted(breakdowns []DomainBreakdown) int { | ||
| total := 0 | ||
| for _, d := range breakdowns { | ||
| total += d.Attempted | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 13.1 AIC
| assert.NotContains(t, cmd.Env, "CI=false") | ||
| assert.Contains(t, cmd.Env, "HOME=/tmp/test-home") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Test coverage regression: the withNonInteractiveCIEnv append-fallback path (return append(env, "CI=1")) is no longer exercised by any test case.
💡 Why this matters / suggested fix
Before this PR, setNonInteractiveCIEnv wrapped withNonInteractiveCIEnv, and its now-deleted "adds CI when missing" sub-test indirectly covered the branch in withNonInteractiveCIEnv that appends CI=1 when no CI= entry is present in the slice.
The remaining sub-test only covers the replacement branch (input already contains CI=false). After this removal, the append(env, "CI=1") line in withNonInteractiveCIEnv has no direct test.
Consider adding a sub-test to maintain coverage:
t.Run("adds CI when missing", func(t *testing.T) {
input := []string{"HOME=/tmp/test-home"}
output := withNonInteractiveCIEnv(input)
assert.Equal(t, []string{"HOME=/tmp/test-home"}, input, "must not mutate input")
assert.Contains(t, output, "CI=1")
assert.Contains(t, output, "HOME=/tmp/test-home")
})
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — approving with one minor follow-up note.
📋 Key Themes & Highlights
Key Themes
- Clean deletions across the board: all 5 removed functions (
setNonInteractiveCIEnv,AnalyzeDomainPerformance+DomainInsight,parseNestedImportEntries,parseNestedImportEntry,ResolveDefaultMaxDailyAICredits) have zero callers in the repo — confirmed by exhaustive grep. - Stale spec comment:
spec_test.golines 67–68 still referenceResolveDefaultMaxDailyAICreditsas "retained"; see inline comment onmanager.go.
Positive Highlights
- ✅ Test rename (
TestSetNonInteractiveCIEnv→TestWithNonInteractiveCIEnv) correctly tracks the surviving function - ✅ Exported symbols (
AnalyzeDomainPerformance,DomainInsight) verified dead before deletion — higher-risk removals handled well - ✅ Companion test files pruned in lockstep — no orphaned test coverage
- ✅ Build and vet gates passed per PR description
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 291.3 AIC · ⌖ 13.5 AIC
| return fallback | ||
| } | ||
|
|
||
| // ResolveDefaultMaxAICredits returns the resolved max AI credits default, checking |
There was a problem hiding this comment.
[/zoom-out] Stale spec comment in a companion file needs a follow-up cleanup.
spec_test.go line 67–68 (unchanged in this PR) still reads:
"ResolveDefaultMaxDailyAICredits is retained for other compile-time uses but MUST NOT be called for the daily guardrail default path."
Now that ResolveDefaultMaxDailyAICredits has been deleted, that sentence provides incorrect guidance to the next reader. Consider patching the spec_test.go comment in a follow-up (or squashing it into this PR) to say the function has been removed and BuildDefaultMaxDailyAICreditsExpression is the only conforming API.
💡 Suggested comment update in spec_test.go
// TestSpec_DailyAICreditsGuardrail_RuntimeNotCompileTime validates AIC spec §9.3 (2):
// GH_AW_DEFAULT_MAX_DAILY_AI_CREDITS MUST be resolved at action runtime, not at
// compiler process environment lookup. BuildDefaultMaxDailyAICreditsExpression is
// the sole conforming API for this path; the compile-time resolver
// ResolveDefaultMaxDailyAICredits has been removed as dead code.
//
// T-AIC-DG-006: The compiler produces an expression, not a pre-resolved value.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Deleted Tests (Dead-Code Cleanup — Appropriate)These tests were correctly removed because the production functions they covered were deleted as dead code:
Language SupportTests analyzed:
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of modified tests are implementation tests (threshold: 30%). This is a clean dead-code removal; the single modified test (TestWithNonInteractiveCIEnv) is a rename aligning the test name with its production function.
|
@copilot run pr-finisher skill |
…nd spec Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed all three unresolved review threads in commit
|
|
@copilot review all comments and address unresolved review feedback, then summarize any remaining blockers and refresh the branch if needed.
|
test