Skip to content

Remove unreachable functions identified by deadcode analysis#5355

Merged
ChrisJBurns merged 4 commits into
mainfrom
code-dead-code-analysis-and-removal-potential
May 21, 2026
Merged

Remove unreachable functions identified by deadcode analysis#5355
ChrisJBurns merged 4 commits into
mainfrom
code-dead-code-analysis-and-removal-potential

Conversation

@ChrisJBurns
Copy link
Copy Markdown
Collaborator

@ChrisJBurns ChrisJBurns commented May 20, 2026

Summary

Strips 100 unreachable functions across production and test helpers, identified via golang.org/x/tools/cmd/deadcode -test ./.... These are mostly speculative API surface (builder options, functional options, convenience constructors) and stale test scaffolding that no caller exercises.

Why: Reduces maintenance burden and surface area. Several of the removed builder/option helpers also conflict with the go-style.md guideline that public functions should have at least one concrete caller. Dead test scaffolding misleads future contributors into thinking the harness is more featureful than it is.

Approach: Ran deadcode -test ./... (which considers all packages and tests as roots — the strictest, most accurate view) and removed everything it flagged outside cmd/thv/app/ui/. The UI package is intentionally retained pending a possible TUI revival.

Type of change

  • Refactoring (no behavior change)

Large PR Justification

This PR is 38 files / ~1,176 LOC, which exceeds the project's 10-file / 400-LOC PR guideline (.claude/rules/pr-creation.md). The size is justified because:

  1. Single logical change. Every change is "delete a function that no caller exercises." There is no mix of feature work, refactoring, or bug fixes — splitting would produce N PRs of identical character with no additional review signal.
  2. Each deletion is independent. No removal depends on another. Reviewers can scan the file-level diff and approve or reject each entry on its own merits.
  3. Splitting adds churn, not safety. A split by package would create ~25 small PRs, each requiring its own review cycle, CI run, and rebase against main — with the same verification (task build + task test) repeated each time. The risk profile is identical to landing once.
  4. Mechanical, reflection-blind tooling, fully verified. The list was produced by deadcode -test ./..., the strictest mode, then verified end-to-end with task build and task test (full unit suite, exit 0). Any false positive from reflection-blind analysis would have surfaced as a compile or test failure.
  5. Pure deletion diff. No new code, no API additions, no behavior changes. The blast radius is limited to "code that nothing called is gone."

If reviewers prefer a split, I'm happy to break it apart by top-level package (pkg/api, pkg/json, pkg/state, pkg/vmcp/composer, cmd/thv-operator/test-integration, test/e2e, test/integration) — flag it and I'll re-cut.

Changes

Highlights:

  • pkg/api/server.go — drop 7 unused ServerBuilder.With* options and the standalone Serve helper
  • pkg/json/any.go — drop NewData / MustParse / FromRawExtension / MapFromRawExtension convenience constructors
  • pkg/state/runconfig.go — drop ReadRunConfigJSON, LoadRunConfigOfType, LoadRunConfigWithFunc, and the now-orphaned RunConfigReadJSONFunc type
  • pkg/vmcp/composer/workflow_errors.go — drop unused WorkflowError struct + methods (keep package-level sentinel errors and ValidationError)
  • pkg/container/runtime/registry.go — drop unused package-level registry accessors (GetRegisteredRuntime, IsRuntimeRegistered, RegisteredRuntimes)
  • pkg/{labels,networking,auth,client,k8s,runner,server,transport,vmcp}/... — assorted one-off unused funcs (GetGroup/SetGroup, IsIPv6Available, NewTokenValidatorConfig, FindRegisteredClientConfigs, NewDynamicClient, IsAvailable, WithMiddlewareConfig, CreatePermissionProfileFile, ReadServerInfo, NewMCPPinger, NewBackendDiscovererWithManager, etc.)
  • cmd/thv-operator/test-integration/mcp-registry/* — drop ~30 unused builder/helper methods (WithLabel, WaitForSyncCompletion, PatchRegistry, timing helpers, etc.)
  • test/e2e/* and test/integration/authserver/helpers/* — drop unused mock/helper functions and option setters
  • Removed imports orphaned by the deletions

Statistics: 38 files changed, 1,176 lines removed.

Test plan

  • task build passes
  • task test passes (exit 0, full unit suite)
  • task lint-fix clean (CI confirmed after gci fix in follow-up commit)
  • Re-ran deadcode -test ./...: 23 unreachable funcs remain, all in cmd/thv/app/ui/ (intentionally kept)
  • E2E tests — not run locally; CI will exercise them

Special notes for reviewers

  • cmd/thv/app/ui/ (help.go, spinner.go, parts of styles.go) was deliberately excluded — looks like an aborted TUI redesign that may be revived. A follow-up decision is needed on whether to revive or remove that package.
  • deadcode is reflection-blind. The build + test suite passing is the verification that these removals are safe; if any reviewer suspects a removed function is called via reflection or generated code, please flag it.

🤖 Generated with Claude Code

Strips 100 unreachable functions across production and test helpers,
identified via `golang.org/x/tools/cmd/deadcode -test ./...`. These
are mostly speculative API surface (builder options, functional
options, convenience constructors) and stale test scaffolding that
no caller exercises.

Highlights:
- pkg/api/server.go: drop 7 unused ServerBuilder.With* options and
  the standalone Serve helper
- pkg/json/any.go: drop NewData/MustParse/FromRawExtension/
  MapFromRawExtension convenience constructors
- pkg/state/runconfig.go: drop ReadRunConfigJSON, LoadRunConfigOfType,
  LoadRunConfigWithFunc, and the now-orphaned RunConfigReadJSONFunc
  type
- pkg/vmcp/composer/workflow_errors.go: drop unused WorkflowError
  type; keep package-level sentinels and ValidationError
- Trim unused test helpers in cmd/thv-operator/test-integration/
  mcp-registry/, test/e2e/, and test/integration/authserver/helpers/
- Remove imports orphaned by deletions

Excludes cmd/thv/app/ui/ dead code (kept intentionally, may be
revived for a TUI). ~1,176 LOC removed across 38 files. `task build`
and `task test` pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label May 20, 2026
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels May 20, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels May 20, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.63%. Comparing base (bd73817) to head (55f1ef0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5355      +/-   ##
==========================================
+ Coverage   68.37%   68.63%   +0.25%     
==========================================
  Files         624      624              
  Lines       63442    63244     -198     
==========================================
+ Hits        43381    43407      +26     
+ Misses      16820    16596     -224     
  Partials     3241     3241              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels May 20, 2026
@github-actions github-actions Bot dismissed their stale review May 20, 2026 22:15

Large PR justification has been provided. Thank you!

@github-actions
Copy link
Copy Markdown
Contributor

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

@ChrisJBurns ChrisJBurns merged commit 5540b3d into main May 21, 2026
47 checks passed
@ChrisJBurns ChrisJBurns deleted the code-dead-code-analysis-and-removal-potential branch May 21, 2026 13:05
reyortiz3 added a commit that referenced this pull request May 22, 2026
* Restore ServerBuilder.WithMiddleware and WithRoute

These two methods are the only public API available to ApplyServerExtensions
hooks (the enterprise overlay uses them to inject session-auth middleware
and mount an /enterprise sub-router). #5355 dropped them as unreachable
because the only callers live behind //go:build enterprise, which upstream
CI does not exercise.

Add a unit test that exercises both methods so future deadcode passes see
them as live, and add doc comments explaining the extension-point contract.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Reword comments to describe the public extension contract

Reframe the doc comments and test rationale around the public extension
point itself ("ApplyServerExtensions consumers", "downstream repositories")
rather than the internal identity of any particular consumer. The behavior
of the methods is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants