[dead-code] chore: remove dead functions — 5 functions removed - #55418
Conversation
Remove unreachable functions identified by deadcode analyzer: - scanfindings.Sort, scanfindings.CountAtLeast - workflow.mcpGatewayMountsUseRunnerToolCache - agentdrain.Miner.Train - intent.ResolveRisk Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
changed the title
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #55418 does not have the 'implementation' label and has only 5 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
![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 — changes approved.
📋 Key Themes & Highlights
Key Themes
- Clean API reduction: removing exported dead symbols (
Sort,CountAtLeast,ResolveRisk,Miner.Train) shrinks the public surface, which is a net positive for navigability. - Safe migration: the only non-trivial change is replacing raw-string
Miner.Traincall sites in tests with structuredTrainEventcalls — the rewrite is correct and consistent. - Tooling-backed: deadcode verification via
deadcode ./cmd/... ./internal/tools/...gives high confidence these symbols are truly unreachable.
Positive Highlights
- ✅ Every removed function has its exclusive tests removed too — no orphaned test coverage.
- ✅
TestClustersandTestConcurrencycorrectly migrated toTrainEventrather than simply deleted. - ✅ PR description is thorough: lists each function, its package, and confirms build/vet/fmt/test passed.
- ✅
mcpGatewayMountsUseRunnerToolCacheremoved along with its mount-escape edge-case tests — the logic remains in the still-usedcollectMCPServerConfiguredMounts.
Minor Observation
Sort and CountAtLeast in pkg/scanfindings are documented as providing stable, deterministic ordering and severity thresholding. These are generally useful utilities. If there is a planned scanner integration that might need them, parking them in an internal helper package rather than deleting them could save re-work — but this is informational only and does not block merging.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 18.2 AIC · ⌖ 9.79 AIC · ⊞ 7.6K
Comment /matt to run again
There was a problem hiding this comment.
Clean dead-code removal. All 5 functions deleted alongside their tests, and the one surviving test migrated from Train to TrainEvent. No remaining callers. LGTM.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 15.9 AIC · ⌖ 8.95 AIC · ⊞ 6.2K
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
REQUEST_CHANGES
The code deletions themselves are mostly straightforward, but this PR leaves the repository's published package specs behind in multiple places. Removing exported symbols while keeping them documented as public API is a merge-blocking maintenance bug because it teaches users and automation to depend on functions that no longer exist.
Blocking themes
pkg/agentdrain/README.mdstill documents(*Miner).Trainpkg/scanfindings/README.mdstill documents and demonstratesSort/CountAtLeastpkg/intent/README.mdstill documentsResolveRisk- the attempted
grumpy-codersub-agent did not run in this environment (grumpy-coderexecutable missing), so this review is based on direct analysis only
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 15.2 AIC · ⌖ 7.07 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Removes five functions identified as unused, deletes their tests, and migrates remaining miner tests to TrainEvent.
Changes:
- Removes scan-finding helpers, risk resolution, raw-line training, and an MCP mount helper.
- Deletes corresponding unit and API-contract tests.
- Migrates remaining miner tests from
TraintoTrainEvent.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_setup_gateway.go |
Removes the unused mount-cache detector. |
pkg/workflow/mcp_gateway_mount_policy_test.go |
Removes its dedicated tests. |
pkg/scanfindings/scanfindings.go |
Removes sorting and counting APIs. |
pkg/scanfindings/scanfindings_test.go |
Removes associated API tests. |
pkg/intent/governance.go |
Removes risk classification. |
pkg/intent/governance_formal_test.go |
Removes formal risk invariants. |
pkg/agentdrain/spec_test.go |
Removes the raw-line training contract test. |
pkg/agentdrain/miner.go |
Removes Miner.Train. |
pkg/agentdrain/miner_test.go |
Removes training tests and migrates callers. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Balanced
| @@ -169,38 +167,6 @@ func Render(w io.Writer, findings []Finding) { | |||
| } | |||
| } | |||
|
|
|||
| @@ -42,20 +42,6 @@ func NewMiner(cfg Config) (*Miner, error) { | |||
| }, nil | |||
| } | |||
|
|
|||
| @@ -18,45 +18,6 @@ var ErrToolDenied = errors.New("intent: tool denied by policy") | |||
| // AllowedTools is non-nil (restricted) and does not contain the requested tool. | |||
| var ErrToolNotAllowed = errors.New("intent: tool not allowed by policy") | |||
|
|
|||
| // Formal test suite derived from specs/intent-attribution-agent-governance.md, | ||
| // focusing on the Risk classification (ResolveRisk) and Enforcement | ||
| // (Authorizer.AuthorizeTool) sections, plus fail-closed policy compilation for | ||
| // unlinked/ambiguous attribution. Each test corresponds to a named predicate or | ||
| // invariant in the behavioral coverage map. | ||
|
|
||
| // TestResolveRisk_ExplicitOverride (P1/P2 — RiskExplicitOverride) | ||
| // Invariant: an explicit intent.Risk always wins over derived rules, even with | ||
| // conflicting domains/priority that would otherwise resolve differently. | ||
| func TestResolveRisk_ExplicitOverride(t *testing.T) { | ||
| rec := intent.IntentRecord{ | ||
| Risk: "low", | ||
| Domains: []string{"security", "production"}, | ||
| Priority: "critical", | ||
| } | ||
| assert.Equal(t, "low", intent.ResolveRisk(rec), | ||
| "P1/P2: explicit risk must win over derived rules") | ||
| } | ||
|
|
||
| // TestResolveRisk_SecurityCriticalIsHigh (P3 — RiskSecurityCriticalHigh) | ||
| // Invariant: domains contains security AND priority == critical => high. | ||
| func TestResolveRisk_SecurityCriticalIsHigh(t *testing.T) { | ||
| rec := intent.IntentRecord{ | ||
| Domains: []string{"security"}, | ||
| Priority: "critical", | ||
| } | ||
| assert.Equal(t, "high", intent.ResolveRisk(rec), | ||
| "P3: security+critical must resolve to high") | ||
| } | ||
|
|
||
| // TestResolveRisk_ProductionIsHigh (P4 — RiskProductionHigh) | ||
| // Invariant: domains contains production => high, independent of priority. | ||
| func TestResolveRisk_ProductionIsHigh(t *testing.T) { | ||
| cases := []string{"", "low", "critical", "unrecognized"} | ||
| for _, priority := range cases { | ||
| t.Run("priority="+priority, func(t *testing.T) { | ||
| rec := intent.IntentRecord{ | ||
| Domains: []string{"production"}, | ||
| Priority: priority, | ||
| } | ||
| assert.Equal(t, "high", intent.ResolveRisk(rec), | ||
| "P4: production domain must resolve to high regardless of priority") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestResolveRisk_InfrastructureIsMedium (P5 — RiskInfrastructureMedium) | ||
| // Invariant: domains contains infrastructure => medium. | ||
| func TestResolveRisk_InfrastructureIsMedium(t *testing.T) { | ||
| rec := intent.IntentRecord{Domains: []string{"infrastructure"}} | ||
| assert.Equal(t, "medium", intent.ResolveRisk(rec), | ||
| "P5: infrastructure domain must resolve to medium") | ||
| } | ||
|
|
||
| // TestResolveRisk_DocumentationIsLow (P6 — RiskDocumentationLow) | ||
| // Invariant: domains contains documentation => low. | ||
| func TestResolveRisk_DocumentationIsLow(t *testing.T) { | ||
| rec := intent.IntentRecord{Domains: []string{"documentation"}} | ||
| assert.Equal(t, "low", intent.ResolveRisk(rec), | ||
| "P6: documentation domain must resolve to low") | ||
| } | ||
|
|
||
| // TestResolveRisk_UnknownDefault (P7 — RiskUnknownDefault) | ||
| // Invariant: no matching rule (empty, unrecognized domain, security without | ||
| // critical priority) => unknown. | ||
| func TestResolveRisk_UnknownDefault(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| rec intent.IntentRecord | ||
| }{ | ||
| {"empty", intent.IntentRecord{}}, | ||
| {"unrecognized_domain", intent.IntentRecord{Domains: []string{"marketing"}}}, | ||
| {"security_without_critical", intent.IntentRecord{Domains: []string{"security"}, Priority: "low"}}, | ||
| {"security_no_priority", intent.IntentRecord{Domains: []string{"security"}}}, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| assert.Equal(t, "unknown", intent.ResolveRisk(tc.rec), | ||
| "P7: non-matching input must resolve to unknown") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // TestResolveRisk_PrecedenceOrder (P8 — RiskPrecedenceOrder) | ||
| // Invariant: security+critical takes precedence when multiple domains overlap. | ||
| func TestResolveRisk_PrecedenceOrder(t *testing.T) { | ||
| rec := intent.IntentRecord{ | ||
| Domains: []string{"documentation", "infrastructure", "production", "security"}, | ||
| Priority: "critical", | ||
| } | ||
| assert.Equal(t, "high", intent.ResolveRisk(rec), | ||
| "P8: security+critical must take precedence over other overlapping domains") | ||
| } | ||
| // focusing on Enforcement (Authorizer.AuthorizeTool) sections, plus fail-closed | ||
| // policy compilation for unlinked/ambiguous attribution. Each test corresponds | ||
| // to a named predicate or invariant in the behavioral coverage map. |
|
🎉 This pull request is included in a new release. Release: |
Summary
Removed 5 unreachable functions identified by
deadcode ./cmd/... ./internal/tools/..., plus their exclusive tests.Functions Removed
SortCountAtLeastmcpGatewayMountsUseRunnerToolCacheMiner.TrainResolveRiskTests Removed
TestSort,TestSortOrdersBySeverityWithinSameLocation,TestCountAtLeast(pkg/scanfindings)TestMcpGatewayMountsUseRunnerToolCache(pkg/workflow)TestTrain,TestTrainEmptyLine(pkg/agentdrain),TestSpec_PublicAPI_Miner_Train(pkg/agentdrain) — other Train call sites rewritten to useTrainEventTestResolveRisk_*(7 tests) andTestEdgeCase_EmptyDomainsAndPriority(pkg/intent)Verification
go build ./...go vet ./...go vet -tags=integration ./...make fmtgo test ./pkg/scanfindings/... ./pkg/workflow/... ./pkg/agentdrain/... ./pkg/intent/...Run: https://github.com/github/gh-aw/actions/runs/32738169288