Skip to content

chore(cleanup): remove dead code and relocate test-only seams #491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SantiagoDePolonia merged 1 commit into from
Jul 6, 2026
Merged

chore(cleanup): remove dead code and relocate test-only seams #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions internal/admin/dashboard/dashboard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,8 @@ type Handler struct {
basePath string
}

// New creates a new dashboard handler with parsed templates and static file server.
func New() (*Handler, error) {
return NewWithBasePath("/")
}

// NewWithBasePath creates a dashboard handler for an app mounted under basePath.
// It parses templates and sets up the static file server.
func NewWithBasePath(basePath string) (*Handler, error) {
basePath = config.NormalizeBasePath(basePath)
assetVersions, err := buildFrontendAssetVersions()
Expand Down
54 changes: 27 additions & 27 deletions internal/admin/dashboard/dashboard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,19 @@ import (
)

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")
}
}
Comment on lines 13 to 21

@coderabbitai coderabbitai Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 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.


func TestIndex_ReturnsHTML(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)
}

e := echo.New()
Expand Down Expand Up @@ -115,9 +115,9 @@ func TestIndex_UsesBasePathForGeneratedURLs(t *testing.T) {
}

func TestStatic_ServesCSS(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)
}

e := echo.New()
Expand All @@ -138,9 +138,9 @@ func TestStatic_ServesCSS(t *testing.T) {
}

func TestStatic_ServesJS(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)
}

e := echo.New()
Expand All @@ -161,9 +161,9 @@ func TestStatic_ServesJS(t *testing.T) {
}

func TestStatic_ServesModuleJS(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)
}

e := echo.New()
Expand All @@ -184,9 +184,9 @@ func TestStatic_ServesModuleJS(t *testing.T) {
}

func TestStatic_ServesProvidersModuleJS(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)
}

e := echo.New()
Expand All @@ -207,9 +207,9 @@ func TestStatic_ServesProvidersModuleJS(t *testing.T) {
}

func TestStatic_ServesVirtualModelsModuleJS(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)
}

e := echo.New()
Expand All @@ -230,9 +230,9 @@ func TestStatic_ServesVirtualModelsModuleJS(t *testing.T) {
}

func TestStatic_ServesWorkflowsModuleJS(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)
}

e := echo.New()
Expand All @@ -253,9 +253,9 @@ func TestStatic_ServesWorkflowsModuleJS(t *testing.T) {
}

func TestStatic_ServesGuardrailsModuleJS(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)
}

e := echo.New()
Expand All @@ -276,9 +276,9 @@ func TestStatic_ServesGuardrailsModuleJS(t *testing.T) {
}

func TestStatic_ServesFavicon(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)
}

e := echo.New()
Expand All @@ -299,9 +299,9 @@ func TestStatic_ServesFavicon(t *testing.T) {
}

func TestStatic_NotFound(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)
}

e := echo.New()
Expand All @@ -322,9 +322,9 @@ func TestStatic_NotFound(t *testing.T) {
// page must load every script, style, and font from the embedded /admin/static
// tree, never from a CDN or remote font host.
func TestIndex_HasNoExternalResources(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)
}

e := echo.New()
Expand Down Expand Up @@ -358,9 +358,9 @@ func TestIndex_HasNoExternalResources(t *testing.T) {
// TestStatic_ServesVendoredAssets confirms the vendored libraries and font
// files are embedded and served, so the dashboard renders without network access.
func TestStatic_ServesVendoredAssets(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)
}

paths := []string{
Expand Down
7 changes: 0 additions & 7 deletions internal/admin/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,6 @@ func WithTagging(service *tagging.Service) Option {
}
}

// WithGuardrailsRegistry enables listing valid guardrail references for workflow authoring.
func WithGuardrailsRegistry(registry guardrails.Catalog) Option {
return func(h *Handler) {
h.guardrails = registry
}
}

// WithGuardrailService enables full guardrail definition administration endpoints.
func WithGuardrailService(service *guardrails.Service) Option {
return func(h *Handler) {
Expand Down
4 changes: 2 additions & 2 deletions internal/admin/handler_guardrails_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ func TestDeleteGuardrailRejectsActiveWorkflowReference(t *testing.T) {
},
},
}
planService, err := workflows.NewService(planStore, workflows.NewCompiler(guardrailService))
planService, err := workflows.NewService(planStore, workflows.NewCompilerWithFeatureCaps(guardrailService, core.DefaultWorkflowFeatures()))
if err != nil {
t.Fatalf("workflows.NewService() error = %v", err)
}
Expand Down Expand Up @@ -408,7 +408,7 @@ func TestDeleteGuardrailIgnoresDisabledWorkflowGuardrailRefs(t *testing.T) {
},
},
}
planService, err := workflows.NewService(planStore, workflows.NewCompiler(guardrailService))
planService, err := workflows.NewService(planStore, workflows.NewCompilerWithFeatureCaps(guardrailService, core.DefaultWorkflowFeatures()))
if err != nil {
t.Fatalf("workflows.NewService() error = %v", err)
}
Expand Down
11 changes: 10 additions & 1 deletion internal/admin/handler_workflows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ import (
"gomodel/internal/workflows"
)

// WithGuardrailsRegistry enables listing valid guardrail references for
// workflow authoring. Test-only seam: production wires the full guardrail
// service via WithGuardrailService.
func WithGuardrailsRegistry(registry guardrails.Catalog) Option {
return func(h *Handler) {
h.guardrails = registry
}
}

type workflowTestStore struct {
versions []workflows.Version
}
Expand Down Expand Up @@ -192,7 +201,7 @@ func newWorkflowHandler(t *testing.T, store workflows.Store, registry *guardrail
func newWorkflowHandlerWithModelRegistry(t *testing.T, store workflows.Store, modelRegistry *providers.ModelRegistry, guardrailRegistry *guardrails.Registry) *Handler {
t.Helper()

service, err := workflows.NewService(store, workflows.NewCompiler(guardrailRegistry))
service, err := workflows.NewService(store, workflows.NewCompilerWithFeatureCaps(guardrailRegistry, core.DefaultWorkflowFeatures()))
if err != nil {
t.Fatalf("NewService() error = %v", err)
}
Expand Down
14 changes: 0 additions & 14 deletions internal/auditlog/auditlog.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,17 +369,3 @@ type Config struct {
// When true, only /v1/chat/completions, /v1/responses, /v1/embeddings, /v1/files, and /v1/batches are logged
OnlyModelInteractions bool
}

// DefaultConfig returns a Config with sensible defaults
func DefaultConfig() Config {
return Config{
Enabled: false,
LogBodies: false,
LogAudioBodies: false,
LogHeaders: false,
BufferSize: 1000,
FlushInterval: 5 * time.Second,
RetentionDays: 30,
OnlyModelInteractions: true,
}
}
62 changes: 0 additions & 62 deletions internal/batchrewrite/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,6 @@ type FileDeleter interface {
DeleteFile(ctx context.Context, providerType, id string) (*core.FileDeleteResponse, error)
}

// FileRouter resolves a routed native file provider lazily.
type FileRouter func() (core.NativeFileRoutableProvider, error)

// RecordPreparation stores request-scoped rewrite metadata for persistence and
// later cleanup.
func RecordPreparation(ctx context.Context, original, rewritten *core.BatchRequest) {
if ctx == nil || original == nil || rewritten == nil {
return
}
metadata := core.GetBatchPreparationMetadata(ctx)
if metadata == nil {
return
}
metadata.RecordInputFileRewrite(original.InputFileID, rewritten.InputFileID)
}

// RecordResult stores rewrite metadata produced by an explicit batch preparer.
func RecordResult(ctx context.Context, result *core.BatchRewriteResult) {
if ctx == nil || result == nil {
Expand Down Expand Up @@ -65,52 +49,6 @@ func CleanupFile(ctx context.Context, files FileDeleter, providerType, fileID, l
return true
}

// CleanupFileFromRouter resolves the native file API only when there is a file
// id to delete.
func CleanupFileFromRouter(ctx context.Context, router FileRouter, providerType, fileID, logMessage string, attrs ...any) bool {
fileID = strings.TrimSpace(fileID)
if router == nil || fileID == "" {
return false
}
files, err := router()
if err != nil {
return false
}
return CleanupFile(ctx, files, providerType, fileID, logMessage, attrs...)
}

// CleanupSupersededFileFromRouter deletes a local rewrite artifact only when a
// later rewrite has replaced it in the request-scoped batch metadata.
func CleanupSupersededFileFromRouter(ctx context.Context, router FileRouter, providerType, fileID, logMessage string, attrs ...any) bool {
if !ShouldCleanupSupersededFile(ctx, fileID) {
return false
}
return CleanupFileFromRouter(ctx, router, providerType, fileID, logMessage, attrs...)
}

// CleanupSupersededFile deletes a local rewrite artifact only when a later
// rewrite has replaced it in the request-scoped batch metadata.
func CleanupSupersededFile(ctx context.Context, files FileDeleter, providerType, fileID, logMessage string, attrs ...any) bool {
if !ShouldCleanupSupersededFile(ctx, fileID) {
return false
}
return CleanupFile(ctx, files, providerType, fileID, logMessage, attrs...)
}

// ShouldCleanupSupersededFile reports whether fileID is a temporary rewrite
// artifact that has been superseded by a later rewrite stage.
func ShouldCleanupSupersededFile(ctx context.Context, fileID string) bool {
fileID = strings.TrimSpace(fileID)
if fileID == "" {
return false
}
metadata := core.GetBatchPreparationMetadata(ctx)
if metadata == nil {
return false
}
return strings.TrimSpace(metadata.RewrittenInputFileID) != fileID
}

// MergeEndpointHints returns a fresh map containing left hints overwritten by
// right hints. It preserves nil when both inputs are empty.
func MergeEndpointHints(left, right map[string]string) map[string]string {
Expand Down
32 changes: 0 additions & 32 deletions internal/batchrewrite/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,6 @@ func (d *recordingDeleter) DeleteFile(_ context.Context, providerType, id string
return &core.FileDeleteResponse{ID: id, Deleted: true}, nil
}

func TestRecordPreparation(t *testing.T) {
metadata := &core.BatchPreparationMetadata{}
ctx := core.WithBatchPreparationMetadata(context.Background(), metadata)

RecordPreparation(ctx, &core.BatchRequest{InputFileID: " file_original "}, &core.BatchRequest{InputFileID: " file_rewritten "})

if metadata.OriginalInputFileID != "file_original" {
t.Fatalf("OriginalInputFileID = %q, want file_original", metadata.OriginalInputFileID)
}
if metadata.RewrittenInputFileID != "file_rewritten" {
t.Fatalf("RewrittenInputFileID = %q, want file_rewritten", metadata.RewrittenInputFileID)
}
}

func TestRecordResult(t *testing.T) {
metadata := &core.BatchPreparationMetadata{}
ctx := core.WithBatchPreparationMetadata(context.Background(), metadata)
Expand Down Expand Up @@ -79,24 +65,6 @@ func TestCleanupFileReturnsFalseOnDeleteError(t *testing.T) {
}
}

func TestCleanupSupersededFile(t *testing.T) {
metadata := &core.BatchPreparationMetadata{RewrittenInputFileID: "file_current"}
ctx := core.WithBatchPreparationMetadata(context.Background(), metadata)
deleter := &recordingDeleter{}

if CleanupSupersededFile(ctx, deleter, "openai", "file_current", "") {
t.Fatal("CleanupSupersededFile deleted current file")
}
if !CleanupSupersededFile(ctx, deleter, "openai", "file_old", "") {
t.Fatal("CleanupSupersededFile returned false for superseded file")
}

want := []deleteCall{{providerType: "openai", fileID: "file_old"}}
if !reflect.DeepEqual(deleter.calls, want) {
t.Fatalf("calls = %#v, want %#v", deleter.calls, want)
}
}

func TestMergeEndpointHints(t *testing.T) {
left := map[string]string{"a": "/v1/chat/completions", "b": "/v1/responses"}
right := map[string]string{"b": "/v1/chat/completions", "c": "/v1/embeddings"}
Expand Down
11 changes: 0 additions & 11 deletions internal/cache/modelcache/redis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,6 @@ func NewRedisModelCache(cfg RedisModelCacheConfig) (Cache, error) {
return &redisModelCache{store: store, key: key, ttl: ttl, owned: true}, nil
}

// NewRedisModelCacheWithStore creates a Cache from an existing Store (for testing).
func NewRedisModelCacheWithStore(store cache.Store, key string, ttl time.Duration) Cache {
if key == "" {
key = DefaultRedisKey
}
if ttl == 0 {
ttl = cache.DefaultRedisTTL
}
return &redisModelCache{store: store, key: key, ttl: ttl, owned: false}
}

type redisModelCache struct {
store cache.Store
key string
Expand Down
Loading