[dead-code] chore: remove dead functions — 5 functions removed#43807
Conversation
Remove unreachable functions identified by deadcode static analysis: - NewMultiSelectForm (pkg/console/prompt_form.go) - NewTextForm (pkg/console/prompt_form.go) - SetProcessEnvLookup (pkg/workflow/process_env_lookup.go) - PolicyCompiler.Compile (pkg/intent/policy.go) - PolicyCondition.Matches (pkg/intent/policy.go) Also removed private helpers applyFailClosedDefaults, safestDefaultPolicy, mergePolicy and package-level vars that were exclusively used by the dead Compile/Matches methods, and deleted pkg/intent/policy_test.go which exclusively tested the removed functions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
|
🧠 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. This is a dead-code removal PR (754 lines deleted) that only removes production code and associated tests. Test Quality Sentinel analysis not applicable. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43807 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Removes several unused helper APIs and their associated tests to reduce surface area in pkg/workflow, pkg/console, and pkg/intent.
Changes:
- Removed unused console form wrappers (
NewMultiSelectForm,NewTextForm) and pruned their tests. - Removed configurable process env lookup setter and tests that depended on it.
- Removed unused intent policy compilation/matching logic and deleted the dedicated test suite.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/process_env_lookup.go | Removes SetProcessEnvLookup and logging from env lookup helper. |
| pkg/workflow/github_cli_test.go | Deletes tests that depended on the removed env-lookup override. |
| pkg/workflow/features_test.go | Deletes tests/imports tied to the removed env-lookup override. |
| pkg/intent/policy.go | Removes policy compilation + condition matching implementations, leaving type definitions. |
| pkg/intent/policy_test.go | Deletes tests that exclusively covered the removed policy compiler logic. |
| pkg/console/prompt_form.go | Removes unused NewMultiSelectForm / NewTextForm wrappers. |
| pkg/console/prompt_form_test.go | Removes assertions for the deleted form wrapper constructors. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/intent/policy.go:65
- The PolicyCompiler doc comment now claims this type “compiles” rules into an ExecutionPolicy, but the Compile method (and helpers) were removed in this PR. Please update the comment to reflect the current state (legacy placeholder / compilation removed) so exported docs don’t describe non-existent behavior. Also note that specs/intent-attribution-agent-governance.md still references PolicyCompiler.Compile().
// PolicyCompiler compiles a set of rules into an ExecutionPolicy for a given intent.
// Rules are sorted by scope precedence (organization > repository > intent > workflow)
// before merging; within the same scope, declaration order is preserved.
//
// WARNING: the compiled policy is advisory only. Runtime enforcement is not yet
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Low
| import ( | ||
| "os" | ||
| "sync" | ||
|
|
||
| "github.com/github/gh-aw/pkg/logger" | ||
| ) |
There was a problem hiding this comment.
The changes are a clean, deletion-only dead-code removal. All 5 removed functions had no remaining callers in production code, and the accompanying tests that exclusively tested those functions were correctly removed. The processEnvLookupMu mutex is still properly used by the remaining lookupProcessEnv function. PolicyCompiler and its types remain intact; only the unconnected Compile method was removed. Build and test verification is documented in the PR description. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 37.2 AIC · ⌖ 6.35 AIC · ⊞ 4.9K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — commenting on two follow-on cleanups the automated removal left behind.
📋 Key Themes & Highlights
Key Themes
- Stale docstring (
PolicyCompiler): the struct-level comment still describesCompile()scope-precedence behaviour that no longer exists. - Dead synchronisation (
process_env_lookup.go):processEnvLookupMuand theprocessEnvLookupindirection variable have no remaining writer now thatSetProcessEnvLookupis gone; they can be simplified to a directos.LookupEnvcall.
Positive Highlights
- ✅ Pure deletion with zero additions — cleanest kind of dead-code PR.
- ✅ Every removed function had its exclusive tests removed with it; no test coverage was left dangling.
- ✅ CI verified (
go build,go vet,go test) before opening the PR. - ✅ The largest removal (
PolicyCondition.Matches,PolicyCompiler.Compile) was backed by 368 lines of dedicated tests — the whole suite went with the code, keeping the test suite honest.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 65.5 AIC · ⌖ 5.31 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -128,207 +68,3 @@ func (c PolicyCondition) Matches(record IntentRecord, repo RepositoryContext) bo | |||
| type PolicyCompiler struct { | |||
There was a problem hiding this comment.
[/codebase-design] Stale docstring: PolicyCompiler still claims it "compiles a set of rules" and describes scope-precedence sorting behaviour — but Compile() was just deleted. A future reader will search in vain for the method this comment describes.
💡 Suggested update
Replace the docstring to reflect the struct's current state as a data holder:
// PolicyCompiler holds the rules that will be used to compile an ExecutionPolicy.
// Rules are evaluated with scope precedence (organization > repository > intent > workflow).
//
// WARNING: the compiled policy is advisory only. Runtime enforcement is not yet
// wired to the orchestrator.
type PolicyCompiler struct {
Rules []PolicyRule
}Or, if Compile is not coming back soon, simply remove the scope-precedence description entirely since there is no runtime behaviour to document.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 50a35be by updating the PolicyCompiler doc comment in pkg/intent/policy.go so it no longer describes the removed compile behavior.
| type envLookupFunc func(string) (string, bool) | ||
|
|
||
| var ( | ||
| processEnvLookupMu sync.RWMutex |
There was a problem hiding this comment.
[/codebase-design] Dead synchronisation overhead: processEnvLookupMu (a sync.RWMutex) and the processEnvLookup variable now serve no purpose beyond wrapping os.LookupEnv. SetProcessEnvLookup — the only writer — was deleted in this PR, so the mutex can never be acquired for writing and the indirection layer adds cognitive overhead for no safety benefit.
💡 Suggested simplification
Remove the mutex and var entirely; lookupProcessEnv becomes a thin wrapper:
func lookupProcessEnv(key string) string {
value, _ := os.LookupEnv(key)
return value
}This is now equivalent in behaviour, removes a synchronisation primitive with no writers, and makes the file trivially understandable.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 50a35be by removing the leftover mutex/function indirection and calling os.LookupEnv directly in pkg/workflow/process_env_lookup.go.
There was a problem hiding this comment.
Non-blocking observations only — no blocking issues found
The dead code removal is mechanically correct: no dangling callers remain in production code, imports are cleaned up, and tests for the removed functions are properly removed. Build and vet verification is confirmed per the PR description.
Review themes
Two follow-on cleanups are flagged (medium severity, non-blocking):
-
process_env_lookup.go—processEnvLookupMu,processEnvLookup, and theenvLookupFunctype are now stranded: no writer path exists afterSetProcessEnvLookupwas removed. The mutex is acquired on every env lookup for zero benefit. -
pkg/intent/policy.go—PolicyCompilerandPolicyConditionare exported structs whose entire behavioural surface (Compile/Matchesmethods) was removed. The doc comments still describe functionality that no longer exists. No external callers were found, so these types are effectively dead exports.
Neither issue blocks merge; both would be clean follow-up PRs.
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 · 194 AIC · ⌖ 6.57 AIC · ⊞ 5.5K
Comment /review to run again
| type envLookupFunc func(string) (string, bool) | ||
|
|
||
| var ( | ||
| processEnvLookupMu sync.RWMutex |
There was a problem hiding this comment.
Stranded mutex and indirection: processEnvLookupMu now has zero writers — it will only ever be RLocked, making it a no-op guard and dead indirection.
💡 Suggested clean-up
With SetProcessEnvLookup removed, the processEnvLookupMu, processEnvLookup, and envLookupFunc type declaration are all stranded. Every env lookup incurs a read-lock for no benefit. The whole file can collapse to:
package workflow
import "os"
func lookupProcessEnv(key string) string {
value, _ := os.LookupEnv(key)
return value
}This is a follow-on cleanup — not a blocking issue — but shipping dead synchronization primitives will confuse future readers into thinking concurrent writes are expected.
There was a problem hiding this comment.
Fixed in 50a35be by simplifying pkg/workflow/process_env_lookup.go down to a direct os.LookupEnv wrapper.
| @@ -128,207 +68,3 @@ func (c PolicyCondition) Matches(record IntentRecord, repo RepositoryContext) bo | |||
| type PolicyCompiler struct { | |||
There was a problem hiding this comment.
Empty exported struct with misleading doc comment: PolicyCompiler is left exported as a struct with a Rules []PolicyRule field but no methods — its Compile implementation was the entire value proposition, now gone.
💡 Context
The remaining struct and its doc comment ("compiles a set of rules into an ExecutionPolicy") now describe behaviour that no longer exists in the package. PolicyCompiler{Rules: [...]}. has no callable methods — it is a dead configuration struct.
PolicyCondition has the same issue: it describes match-condition semantics but its Matches method was removed. The field documentation (e.g. "Labels are matched as flat strings") on the struct fields is also gone with the method, but the struct itself remains exported.
If PolicyCompiler is genuinely dead (no external callers confirmed), it should be removed in a follow-up, or at minimum the doc comment should be corrected so it doesn't claim to "compile" anything. Leaving it exported invites third-party consumers to depend on a no-op type.
There was a problem hiding this comment.
Addressed in 50a35be by correcting the PolicyCompiler doc comment to describe the remaining data-holder role instead of the removed compile behavior.
|
@copilot please run the
|
…60706-3981ef9c9b9eadab 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>
Addressed in 50a35be: simplified |
|
@copilot please run the
|
No further code changes were needed in this pass. The previously requested review feedback is already addressed in |
|
🎉 This pull request is included in a new release. Release: |
Summary
Removes 5 dead exported functions and their supporting private helpers across 3 packages. No callers existed for any removed symbol.
Changes by package
pkg/consoleNewMultiSelectForm[T]NewTextFormCorresponding test assertions in
TestPromptWrappersReturnNonNilFormsremoved.pkg/intentPolicyCondition.MatchesPolicyCompiler.CompilePrivate helpers removed alongside:
applyFailClosedDefaults,safestDefaultPolicy,mergePolicy, and theautonomyOrder,writeScopeOrder,scopePriorityOrderlookup maps.PolicyCompilerdoc comment updated to reflect its current role (data container only, not an active compiler).pkg/intent/policy_test.godeleted entirely (368 lines, 6 test functions).pkg/workflowSetProcessEnvLookuplookupProcessEnvsimplified to callos.LookupEnvdirectly. Mutex (sync.RWMutex),envLookupFunctype, and logger var removed. Two test functions that exercised the hook (TestIsFeatureEnabledUsesConfiguredProcessEnvLookup,TestExecGHUsesConfiguredProcessEnvLookup) deleted.Files changed
pkg/console/prompt_form.go— 2 functions removedpkg/console/prompt_form_test.go— 2 test cases removedpkg/intent/policy.go— 4 functions + 4 package vars removed; doc updatedpkg/intent/policy_test.go— deleted (368 lines)pkg/workflow/features_test.go— 1 test function removedpkg/workflow/github_cli_test.go— 1 test function removedpkg/workflow/process_env_lookup.go— simplified to directos.LookupEnvcallCommits
9a6a695chore: remove dead functions — 5 functions removed50a35bechore: address follow-up dead-code review feedback