[dead-code] chore: remove dead functions — 3 functions removed#43564
Conversation
Remove unreachable functions identified by the deadcode static analyzer: - cli.SetProcessEnvLookup: no non-test callers - cli.lookupEnvOk: no non-test callers - console.newColorProfileWriter: no non-test callers (both build variants) Delete the exclusive test files that only tested the removed functions. Live helpers (lookupEnv, stderrWriter) and their backing variables are preserved as they remain in active production use. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43564 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 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. Both changed test files (pkg/cli/process_env_lookup_test.go, pkg/console/colorprofile_writer_test.go) were DELETED as part of dead-code removal. Test Quality Sentinel skipped. |
There was a problem hiding this comment.
Pull request overview
Removes recently-identified dead code paths and their dedicated tests, simplifying the CLI and console color writer helpers while keeping the remaining stderr writer behavior intact.
Changes:
- Removed
SetProcessEnvLookupandlookupEnvOkfrompkg/cli/process_env_lookup.go. - Removed
newColorProfileWriterfrom bothpkg/consolebuild variants. - Removed tests that exclusively exercised the removed helpers.
Show a summary per file
| File | Description |
|---|---|
| pkg/console/colorprofile_writer.go | Removes unused newColorProfileWriter helper from non-wasm console build. |
| pkg/console/colorprofile_writer_wasm.go | Removes unused newColorProfileWriter helper from wasm console build. |
| pkg/console/colorprofile_writer_test.go | Deletes tests that only covered the removed console helper. |
| pkg/cli/process_env_lookup.go | Removes unused env-lookup override/setter helpers, leaving lookupEnv. |
| pkg/cli/process_env_lookup_test.go | Deletes tests that only covered the removed CLI helpers. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Low
| func stderrWriter() io.Writer { | ||
| return colorwriter.Stderr() | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — no issues found. This is a clean dead-code removal.
📋 Analysis Summary
What was verified
- No remaining callers: grep across all Go files confirms
SetProcessEnvLookup(pkg/cli),lookupEnvOk, andnewColorProfileWriterhave zero call-sites after this change. TheSetProcessEnvLookupreferences inpkg/workflow/are a distinct function in a separate package — unrelated. - Deleted tests were exclusively for removed functions: Both test files contained only tests for the functions being removed; there is no regression from deleting them.
- Dual build-variant consistency: Both
colorprofile_writer.goandcolorprofile_writer_wasm.goare updated symmetrically. - Build and vet verified by the author (
go build ./...,go vet ./...,make fmt).
Positive Highlights
- ✅ No production logic changed — purely subtractive
- ✅ Symmetric removal across build variants (non-wasm and wasm)
- ✅ Test files removed rather than left with dead test helpers
- ✅ PR description is precise and includes a verification checklist
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 24.2 AIC · ⌖ 6.53 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Clean dead-code removal. Verified no remaining callers for SetProcessEnvLookup (pkg/cli), lookupEnvOk, or newColorProfileWriter in the codebase. The pkg/workflow SetProcessEnvLookup references are a separate function and unaffected. Test files are correctly deleted alongside their subjects. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 15 AIC · ⌖ 6 AIC · ⊞ 4.9K
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · 103.9 AIC · ⌖ 7.55 AIC · ⊞ 5.4K
Comment /review to run again
| } | ||
|
|
||
| func lookupEnv(key string) string { | ||
| processEnvLookupMu.RLock() |
There was a problem hiding this comment.
Dead lock overhead: processEnvLookupMu.RLock() now protects permanently immutable state and can never be contested.
💡 Explanation
SetProcessEnvLookup was the only writer of processEnvLookup. Now that it has been removed, processEnvLookup is assigned exactly once at init and can never change. Every call to lookupEnv still acquires (and releases) a read lock for no reason — the mutex provides zero protection and adds unnecessary overhead on each env lookup.
The correct follow-up is to simplify the entire file:
func lookupEnv(key string) string {
value, _ := os.LookupEnv(key)
return value
}Or at minimum remove the processEnvLookupMu/processEnvLookup indirection entirely and call os.LookupEnv directly. Leaving a read-locked call to an effectively-constant function pointer is a code smell that will confuse future readers trying to understand why synchronisation exists here.
Functions Removed
SetProcessEnvLookuppkg/cli/process_env_lookup.golookupEnvOkpkg/cli/process_env_lookup.gonewColorProfileWriterpkg/console/colorprofile_writer.go(both build variants)Tests Removed
pkg/cli/process_env_lookup_test.go(exclusively tested the removed functions)pkg/console/colorprofile_writer_test.go(exclusively testednewColorProfileWriter)Verification
go build ./...go vet ./...go vet -tags=integration ./...make fmthttps://github.com/github/gh-aw/actions/runs/28744384357