chore(cleanup): remove dead code and relocate test-only seams#491
Merged
Conversation
Systematic dead-code sweep driven by golang.org/x/tools/cmd/deadcode (binary and -test modes) and staticcheck U1000. Net -2,148 lines. Removed: - The unused responsecache stream-reconstruction subsystem (stream_cache_chat.go, stream_cache_responses.go, and the dead half of stream_cache.go), superseded by verbatim raw-SSE replay, plus the tests that only exercised it. - Dead thin wrappers, migrating their test call sites to the live variants: workflows.NewCompiler, dashboard.New, gateway.ResolveRequestModel, server resolveRequestModel, DetermineBatchExecutionSelection(WithAuthorizer), ensureTranslatedRequestWorkflow, doOpenAICompatibleFileIDRequest, and the batchrewrite RecordPreparation/CleanupSuperseded* family (the guardrails harness now mirrors production via RecordResult/CleanupFile). - Dead functions together with their dead tests: observability.GetMetrics/HealthCheck, core.HasNonTextContent, core.NormalizeModelSelector, the DecodedBatchItemRequest accessors, BatchItemModelSelector, auditlog.DefaultConfig, conversationstore.WithUnboundedRetention, responsecache guardrails-hash and cache-skip wrappers, and the unused MockEmbedder test type. Relocated production-dead but test-used seams into test files: server NewHandler/newHandler/AuthMiddleware/WorkflowResolution( WithResolver)/GetProviderType/handleStreamingResponse (new seams_test.go), vecstore_map.go, NewRedisModelCacheWithStore, failover.NewResolver, admin.WithGuardrailsRegistry, derivedEnvNames. Deliberately kept: the ext package public extension API, the provider NewWithHTTPClient constructor family (contract tests and cross-provider delegation), and cross-package test seams (cache.MapStore, llmclient.DefaultConfig, ResetMetrics, NewResponseCacheMiddlewareWithStore, memory-store retention options). deadcode -test now reports only the intentional ext API; staticcheck U1000 (including tests) is clean; all packages pass, and contract/ integration suites compile under their build tags. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR removes numerous legacy convenience/wrapper functions across the codebase in favor of their explicit, more parameterized counterparts (e.g., authorizer-aware, feature-cap-aware, base-path-aware variants). Callers and tests are updated to use the new explicit APIs, and several unused helper functions and streaming-cache reconstruction logic are deleted. ChangesWrapper API Removal and Caller Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/dashboard/dashboard_test.go`:
- Around line 13-21: The test name is stale after the constructor rename:
TestNew now only covers NewWithBasePath, so rename the test to match the actual
API it exercises. Update the test identifier in dashboard_test.go from TestNew
to something aligned with NewWithBasePath, and keep the assertions against
NewWithBasePath and its returned handler/error behavior.
In `@internal/workflows/service_test.go`:
- Line 364: Create a shared package-level test helper for the repeated compiler
setup in service_test.go, since NewCompilerWithFeatureCaps(nil,
core.DefaultWorkflowFeatures()) is duplicated across many test cases. Add a
small helper such as newDefaultTestCompiler() near the other test utilities, and
replace each direct construction at the Service test call sites with that
helper. Keep the helper returning the same Compiler value so all tests continue
using the default workflow features consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1866e84-4ca0-4553-95b0-1dc721e52a59
📒 Files selected for processing (54)
internal/admin/dashboard/dashboard.gointernal/admin/dashboard/dashboard_test.gointernal/admin/handler.gointernal/admin/handler_guardrails_test.gointernal/admin/handler_workflows_test.gointernal/auditlog/auditlog.gointernal/batchrewrite/helpers.gointernal/batchrewrite/helpers_test.gointernal/cache/modelcache/redis.gointernal/cache/modelcache/redis_test.gointernal/conversationstore/store_memory.gointernal/core/batch_semantic.gointernal/core/batch_semantic_test.gointernal/core/chat_content.gointernal/core/chat_content_test.gointernal/core/semantic.gointernal/core/semantic_canonical.gointernal/core/semantic_canonical_test.gointernal/embedding/embedding_test.gointernal/failover/resolver.gointernal/failover/resolver_test.gointernal/gateway/batch_selection.gointernal/gateway/edge_cases_test.gointernal/gateway/request_model_resolution.gointernal/guardrails/provider_harness_test.gointernal/observability/metrics.gointernal/observability/metrics_test.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/file_adapter_openai_compat.gointernal/providers/file_adapter_openai_compat_test.gointernal/responsecache/handle_request_test.gointernal/responsecache/semantic.gointernal/responsecache/semantic_test.gointernal/responsecache/stream_cache.gointernal/responsecache/stream_cache_chat.gointernal/responsecache/stream_cache_responses.gointernal/responsecache/vecstore_map_test.gointernal/server/auth.gointernal/server/handlers.gointernal/server/http_test.gointernal/server/model_validation.gointernal/server/model_validation_test.gointernal/server/request_model_resolution.gointernal/server/request_model_resolution_test.gointernal/server/seams_test.gointernal/server/translated_inference_service.gointernal/server/workflow_helpers.gointernal/server/workflow_helpers_test.gointernal/server/workflow_policy_test.gointernal/workflows/compiler.gointernal/workflows/compiler_test.gointernal/workflows/service_test.gotests/e2e/setup_test.go
💤 Files with no reviewable changes (26)
- internal/server/request_model_resolution.go
- internal/responsecache/stream_cache_chat.go
- internal/responsecache/stream_cache_responses.go
- internal/providers/config.go
- internal/auditlog/auditlog.go
- internal/cache/modelcache/redis.go
- internal/server/workflow_helpers.go
- internal/admin/handler.go
- internal/server/translated_inference_service.go
- internal/core/chat_content.go
- internal/providers/file_adapter_openai_compat.go
- internal/core/semantic_canonical.go
- internal/gateway/batch_selection.go
- internal/gateway/request_model_resolution.go
- internal/core/semantic_canonical_test.go
- internal/embedding/embedding_test.go
- internal/workflows/compiler.go
- internal/batchrewrite/helpers_test.go
- internal/core/batch_semantic.go
- internal/observability/metrics_test.go
- internal/observability/metrics.go
- internal/server/model_validation_test.go
- internal/batchrewrite/helpers.go
- internal/core/chat_content_test.go
- internal/responsecache/handle_request_test.go
- internal/responsecache/stream_cache.go
Comment on lines
13
to
21
| func TestNew(t *testing.T) { | ||
| h, err := New() | ||
| h, err := NewWithBasePath("/") | ||
| if err != nil { | ||
| t.Fatalf("New() returned error: %v", err) | ||
| t.Fatalf("NewWithBasePath() returned error: %v", err) | ||
| } | ||
| if h == nil { | ||
| t.Fatal("New() returned nil handler") | ||
| t.Fatalf("NewWithBasePath() returned nil handler") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Stale test name after constructor rename.
TestNew now exercises NewWithBasePath exclusively, but the test name still references the removed New() constructor.
♻️ Suggested rename
-func TestNew(t *testing.T) {
+func TestNewWithBasePath(t *testing.T) {
h, err := NewWithBasePath("/")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestNew(t *testing.T) { | |
| h, err := New() | |
| h, err := NewWithBasePath("/") | |
| if err != nil { | |
| t.Fatalf("New() returned error: %v", err) | |
| t.Fatalf("NewWithBasePath() returned error: %v", err) | |
| } | |
| if h == nil { | |
| t.Fatal("New() returned nil handler") | |
| t.Fatalf("NewWithBasePath() returned nil handler") | |
| } | |
| } | |
| func TestNewWithBasePath(t *testing.T) { | |
| h, err := NewWithBasePath("/") | |
| if err != nil { | |
| t.Fatalf("NewWithBasePath() returned error: %v", err) | |
| } | |
| if h == nil { | |
| t.Fatalf("NewWithBasePath() returned nil handler") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/admin/dashboard/dashboard_test.go` around lines 13 - 21, The test
name is stale after the constructor rename: TestNew now only covers
NewWithBasePath, so rename the test to match the actual API it exercises. Update
the test identifier in dashboard_test.go from TestNew to something aligned with
NewWithBasePath, and keep the assertions against NewWithBasePath and its
returned handler/error behavior.
| } | ||
|
|
||
| service, err := NewService(store, NewCompiler(nil)) | ||
| service, err := NewService(store, NewCompilerWithFeatureCaps(nil, core.DefaultWorkflowFeatures())) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider extracting a shared test helper for the repeated compiler construction.
NewCompilerWithFeatureCaps(nil, core.DefaultWorkflowFeatures()) is repeated verbatim ~16 times across this file. A small package-level test helper would reduce duplication and centralize future updates.
♻️ Suggested helper
func newDefaultTestCompiler() Compiler {
return NewCompilerWithFeatureCaps(nil, core.DefaultWorkflowFeatures())
}Then replace call sites, e.g.:
- service, err := NewService(store, NewCompilerWithFeatureCaps(nil, core.DefaultWorkflowFeatures()))
+ service, err := NewService(store, newDefaultTestCompiler())Also applies to: 396-396, 453-453, 509-509, 556-556, 590-590, 808-808, 933-933, 1005-1005, 1046-1046, 1092-1092, 1127-1127, 1214-1214, 1269-1269, 1327-1327, 1394-1394, 1452-1452
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/workflows/service_test.go` at line 364, Create a shared
package-level test helper for the repeated compiler setup in service_test.go,
since NewCompilerWithFeatureCaps(nil, core.DefaultWorkflowFeatures()) is
duplicated across many test cases. Add a small helper such as
newDefaultTestCompiler() near the other test utilities, and replace each direct
construction at the Service test call sites with that helper. Keep the helper
returning the same Compiler value so all tests continue using the default
workflow features consistently.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Systematic dead-code sweep driven by
golang.org/x/tools/cmd/deadcode(both from thecmd/binaries and with-test) andstaticcheck -checks U1000(including test files). Net −2,148 lines across 54 files. After this PR,deadcode -test ./...reports only the intentionally-publicextextension API, and staticcheck U1000 is clean.No user-visible behavior changes: everything removed was unreachable from the production binaries, and every test that covered live logic through a removed wrapper was migrated to the production variant instead of deleted.
What's removed
Dead streaming-cache reconstruction subsystem (~1,500 lines).
internal/responsecachestill carried the pre-redesign machinery that parsed cached SSE events and rebuilt synthetic streams (stream_cache_chat.go,stream_cache_responses.go, and the dead half ofstream_cache.go). Production replays raw SSE bytes verbatim (writeCachedResponse); nothing calledreconstructStreamingResponse/renderCached*Streamoutside ~475 lines of tests that tested only the dead code. Both deleted.Dead thin wrappers — deleted, with test call sites migrated to the live variants they delegated to:
workflows.NewCompiler→NewCompilerWithFeatureCaps(~23 sites)dashboard.New→NewWithBasePath("/")gateway.ResolveRequestModel+ serverresolveRequestModel→ResolveRequestModelWithAuthorizerDetermineBatchExecutionSelection/...WithAuthorizer→...WithAuthorizerAndInputFileResolverensureTranslatedRequestWorkflow→...WithAuthorizerdoOpenAICompatibleFileIDRequest→...WithPreparerRecordPreparation/CleanupFileFromRouter/CleanupSupersededFile*— the guardrails test harness now mirrors production orchestration via the liveRecordResult/CleanupFileDead functions plus their dead tests:
observability.GetMetrics/HealthCheck(+PrometheusMetrics),core.HasNonTextContent,core.NormalizeModelSelector, the fourDecodedBatchItemRequestaccessors,BatchItemModelSelector(its table tests retargeted onto the liveBatchItemRequestedModelSelector),auditlog.DefaultConfig,conversationstore.WithUnboundedRetention, responsecache'sGuardrailsHashFromContext/WithGuardrailsHash/ShouldSkipExactCache/ShouldSkipAllCache, and the unusedMockEmbeddertest type.Production-dead but test-used seams moved into test files (so the shipped binary no longer carries them, without churning ~250 test call sites): server's
NewHandler/newHandler/AuthMiddleware/WorkflowResolution(WithResolver)/GetProviderType/handleStreamingResponseinto a newinternal/server/seams_test.go; plusvecstore_map.go→vecstore_map_test.go,modelcache.NewRedisModelCacheWithStore,failover.NewResolver,admin.WithGuardrailsRegistry, andproviders.derivedEnvNames.Deliberately kept (still flagged by deadcode, not dead)
ext/registry.gopackage-level wrappers andRegistrymutators — the documented public extension API for external embedders; static analysis can't see out-of-repo consumers.NewWithHTTPClientconstructors — pinned by the-tags=contractsuite and by cross-provider production delegation (opencodego embeds anthropic; OpenAI-compatible providers embed openai).cache.MapStore,llmclient.DefaultConfig,observability.ResetMetrics,NewResponseCacheMiddlewareWithStore, and the memory-store retention options.Verification
go build ./..., fullgo test ./...: 61 packages ok, 0 failures (pre-commit also ranmake test-raceandmake lint).go vet -tags=contract ./tests/contract/and-tags=integration ./tests/integration/...compile clean.deadcode ./cmd/...down from 129 flagged functions to 41 (all in the deliberate-keep list above);deadcode -test ./...down from 12 to the 4extAPI functions.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
no-storerequests.Refactor