internal/modeldata/merger_test.go:376

• This test duplicates cost-calculation logic via calculateCostHelper instead of testing the canonical usage.CalculateCost implementation. This duplication can drift over time and make tests pass while production behavior changes; consider importing gomodel/internal/usage in the test and exercising usage.CalculateCost directly (or moving cost calculation to a small shared package to keep a single implementation).

func TestCalculateCost(t *testing.T) {
	tests := []struct {
		name        string
		input       int
		output      int
		pricing     *core.ModelPricing
		wantInput   *float64
		wantOutput  *float64
		wantTotal   *float64
	}{
		{
			name:    "nil pricing",
			input:   1000,
			output:  500,
			pricing: nil,
		},
		{
			name:   "basic pricing",
			input:  1000000,
			output: 500000,
			pricing: &core.ModelPricing{
				Currency:      "USD",
				InputPerMtok:  ptr(2.50),
				OutputPerMtok: ptr(10.00),
			},
			wantInput:  ptr(2.50),
			wantOutput: ptr(5.00),
			wantTotal:  ptr(7.50),
		},
		{
			name:   "zero tokens",
			input:  0,
			output: 0,
			pricing: &core.ModelPricing{
				Currency:      "USD",
				InputPerMtok:  ptr(2.50),
				OutputPerMtok: ptr(10.00),
			},
			wantInput:  ptr(0.0),
			wantOutput: ptr(0.0),
			wantTotal:  ptr(0.0),
		},
		{
			name:   "partial pricing (input only)",
			input:  1000000,
			output: 500000,
			pricing: &core.ModelPricing{
				Currency:     "USD",
				InputPerMtok: ptr(2.50),
			},
			wantInput: ptr(2.50),
		},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			// CalculateCost is in the usage package, but we test the logic inline
			in, out, total := calculateCostHelper(tt.input, tt.output, tt.pricing)

			assertPtrFloat(t, "input", in, tt.wantInput)
			assertPtrFloat(t, "output", out, tt.wantOutput)
			assertPtrFloat(t, "total", total, tt.wantTotal)
		})
	}
}

// calculateCostHelper intentionally mirrors usage.CalculateCost for testing within
// this package. It is duplicated here (rather than imported) to avoid import cycles
// between modeldata and usage. The canonical implementation lives in
// usage.CalculateCost — keep both in sync when changing cost calculation logic.
func calculateCostHelper(inputTokens, outputTokens int, pricing *core.ModelPricing) (input, output, total *float64) {
	if pricing == nil {
		return nil, nil, nil
	}
	if pricing.InputPerMtok != nil {
		v := float64(inputTokens) * *pricing.InputPerMtok / 1_000_000
		input = &v
	}
	if pricing.OutputPerMtok != nil {
		v := float64(outputTokens) * *pricing.OutputPerMtok / 1_000_000
		output = &v
	}
	if input != nil && output != nil {
		v := *input + *output
		total = &v
	}
	return
}

internal/providers/registry.go:548

• StartBackgroundRefresh cancellation won't stop an in-flight model list refresh because refreshModelList uses context.Background() instead of deriving from the refresh loop's ctx. Consider passing a parent ctx into refreshModelList (and using it in context.WithTimeout) so calling the returned cancel func reliably stops both provider refresh and model-list refresh work.

func (r *ModelRegistry) StartBackgroundRefresh(interval time.Duration, modelListURL string, modelListTimeout time.Duration) func() {
	ctx, cancel := context.WithCancel(context.Background())

	go func() {
		ticker := time.NewTicker(interval)
		defer ticker.Stop()

		for {
			select {
			case <-ctx.Done():
				return
			case <-ticker.C:
				refreshCtx, refreshCancel := context.WithTimeout(context.Background(), 30*time.Second)
				if err := r.Refresh(refreshCtx); err != nil {
					slog.Warn("background model refresh failed", "error", err)
				} else {
					// Save to cache after successful refresh
					if err := r.SaveToCache(refreshCtx); err != nil {
						slog.Warn("failed to save models to cache after refresh", "error", err)
					}
				}
				refreshCancel()

				// Also refresh model list if configured
				if modelListURL != "" {
					r.refreshModelList(modelListURL, modelListTimeout)
				}
			}
		}
	}()

	return cancel
}

// refreshModelList fetches the model list and re-enriches all models.
func (r *ModelRegistry) refreshModelList(url string, timeout time.Duration) {
	fetchCtx, cancel := context.WithTimeout(context.Background(), timeout)
	defer cancel()

	list, raw, err := modeldata.Fetch(fetchCtx, url, timeout)
	if err != nil {

internal/providers/init.go:107

• The model list fetch goroutine is created with context.Background(), so shutdown/caller cancellation won't stop the fetch early (it only stops via timeout). Consider deriving the fetch context from the Init(ctx, ...) argument so service shutdown cancels the best-effort background fetch promptly.

	// Fetch model list in background (best-effort, non-blocking)
	modelListURL := result.Config.Cache.ModelList.URL
	modelListTimeout := time.Duration(result.Config.Cache.ModelList.Timeout) * time.Second
	if modelListTimeout <= 0 {
		modelListTimeout = 30 * time.Second
	}
	if modelListURL != "" {
		go func() {
			fetchCtx, cancel := context.WithTimeout(context.Background(), modelListTimeout)
			defer cancel()

			list, raw, err := modeldata.Fetch(fetchCtx, modelListURL, modelListTimeout)
			if err != nil {
				slog.Warn("failed to fetch model list", "url", modelListURL, "error", err)
				return
			}
			if list == nil {
				return
			}

internal/providers/registry.go:454

• EnrichModels holds r.mu for the entire enrichment and calls modeldata.Enrich, which currently logs via slog.Info. Doing logging (and potentially other non-trivial work) while holding the write lock can increase contention for callers like ListModels; consider having Enrich return counts and logging after releasing the lock (or lowering the log level / moving logging out of Enrich).

// EnrichModels re-applies model list metadata to all currently registered models.
// Call this after SetModelList to update existing models with the new metadata.
// Holds the write lock for the entire operation to prevent races with concurrent
// readers (e.g. ListModels) that may read Model.Metadata.
func (r *ModelRegistry) EnrichModels() {
	r.mu.Lock()
	defer r.mu.Unlock()

	if r.modelList == nil || len(r.models) == 0 {
		return
	}

	providerTypes := make(map[core.Provider]string, len(r.providerTypes))
	for k, v := range r.providerTypes {
		providerTypes[k] = v
	}

	accessor := &registryAccessor{models: r.models, providerTypes: providerTypes}
	modeldata.Enrich(accessor, r.modelList)
}

internal/usage/extractor.go:61

• Cost calculation is introduced here, but the existing usage extractor tests don't exercise the pricing path (and therefore won't catch regressions in CalculateCost or the InputCost/OutputCost/TotalCost assignment). Add/extend tests in the usage package to pass pricing and assert cost fields, including partial pricing cases.

// CalculateCost computes input, output, and total costs from token counts and pricing.
// Returns nil pointers when pricing info is unavailable.
func CalculateCost(inputTokens, outputTokens int, pricing *core.ModelPricing) (input, output, total *float64) {
	if pricing == nil {
		return nil, nil, nil
	}
	if pricing.InputPerMtok != nil {
		v := float64(inputTokens) * *pricing.InputPerMtok / 1_000_000
		input = &v
	}
	if pricing.OutputPerMtok != nil {
		v := float64(outputTokens) * *pricing.OutputPerMtok / 1_000_000
		output = &v
	}
	if input != nil && output != nil {
		v := *input + *output
		total = &v
	}
	return
}

// ExtractFromChatResponse extracts usage data from a ChatResponse.
// It normalizes the usage data into a UsageEntry and preserves raw extended data.
// If pricing is provided, cost fields are calculated.
func ExtractFromChatResponse(resp *core.ChatResponse, requestID, provider, endpoint string, pricing ...*core.ModelPricing) *UsageEntry {
	if resp == nil {
		return nil
	}

	entry := &UsageEntry{
		ID:           uuid.New().String(),
		RequestID:    requestID,
		ProviderID:   resp.ID,
		Timestamp:    time.Now().UTC(),
		Model:        resp.Model,
		Provider:     provider,
		Endpoint:     endpoint,
		InputTokens:  resp.Usage.PromptTokens,
		OutputTokens: resp.Usage.CompletionTokens,
		TotalTokens:  resp.Usage.TotalTokens,
	}

	// Preserve raw extended usage data if available (defensive copy to avoid races)
	if len(resp.Usage.RawUsage) > 0 {
		entry.RawData = cloneRawData(resp.Usage.RawUsage)
	}

	// Calculate costs if pricing is provided
	if len(pricing) > 0 && pricing[0] != nil {
		entry.InputCost, entry.OutputCost, entry.TotalCost = CalculateCost(entry.InputTokens, entry.OutputTokens, pricing[0])
	}

internal/cache/cache.go (1)

14-18: ⚠️ Potential issue | 🟡 Minor

Add a Go doc comment for ModelListData.
Exported fields should have a brief doc comment.

✍️ Suggested fix

 type ModelCache struct {
 	Version       int                    `json:"version"`
 	UpdatedAt     time.Time              `json:"updated_at"`
 	Models        map[string]CachedModel `json:"models"`
+	// ModelListData holds the raw JSON from the external model registry.
 	ModelListData json.RawMessage        `json:"model_list_data,omitempty"`
 }

As per coding guidelines, **/*.go: Update in-code documentation (Go doc comments on exported types, functions, interfaces) when changing public APIs, adding new exported symbols, or modifying function signatures/behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cache/cache.go` around lines 14 - 18, Add a one-line Go doc comment
for the exported field ModelListData on the Cache struct describing its purpose
and format (e.g., "ModelListData holds raw JSON for the list of models, used to
cache model list responses.") — place the comment immediately above the
ModelListData field in the Cache struct so it follows Go doc conventions for
exported fields.

internal/modeldata/enricher_test.go (1)

31-46: The ptr() helper is accessible from merger_test.go within the same package.

In Go, all *_test.go files in the same package compile together during go test, so the ptr[T any]() generic helper defined at line 10 of merger_test.go is accessible here. The code will compile and run correctly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/enricher_test.go` around lines 31 - 46, The reviewer notes
that the generic helper ptr[T any]() used in TestEnrich_MatchedAndUnmatched is
already defined in merger_test.go and is available to this test; confirm that
ptr is defined in merger_test.go and do not re-declare it here—either leave the
usage as-is or, if a duplicate ptr exists in this file, remove the duplicate and
add a short comment referencing merger_test.go's ptr to avoid future confusion
(check TestEnrich_MatchedAndUnmatched, ptr, and merger_test.go to locate and
resolve).

internal/core/types.go (1)

78-83: Document the new Metadata field.

Add a Go doc comment so the exported field’s purpose is discoverable in generated docs.

📝 Suggested doc comment

 type Model struct {
 	ID       string         `json:"id"`
 	Object   string         `json:"object"`
 	OwnedBy  string         `json:"owned_by"`
 	Created  int64          `json:"created"`
+	// Metadata holds enriched information from the external model registry.
 	Metadata *ModelMetadata `json:"metadata,omitempty"`
 }

As per coding guidelines: **/*.go: Update in-code documentation (Go doc comments on exported types, functions, interfaces) when changing public APIs, adding new exported symbols, or modifying function signatures/behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/types.go` around lines 78 - 83, Add a Go doc comment above the
exported Metadata field in the struct (Metadata *ModelMetadata
`json:"metadata,omitempty"`) describing its purpose and when it is populated,
referencing the ModelMetadata type; ensure the comment is a proper sentence
starting with "Metadata" so it appears in generated Go docs and follows project
doc style for exported fields.

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

internal/usage/cost.go:143

• CalculateGranularCost will add a caveat for any RawData key ending in "_tokens"/"_count" that isn't in providerMappings. Since buildRawUsageFromDetails can populate keys like "prompt_text_tokens", "prompt_image_tokens", and prediction token breakdowns (which are typically just a breakdown of prompt/completion tokens, not an extra billable category), this will incorrectly mark cost calculation as incomplete and may confuse users. Consider explicitly excluding known breakdown-only fields from caveat generation (or marking them as mapped-with-no-extra-cost), and/or tightening isTokenField to a per-provider allowlist of additive token fields.
  internal/admin/handler.go:243
• UsageLog assumes GetUsageLog always returns a non-nil result when err is nil, and dereferences result.Entries unconditionally. To avoid a panic if an implementation returns (nil, nil), add a nil check (e.g., initialize an empty UsageLogResult) before accessing result.Entries.

	result, err := h.usageReader.GetUsageLog(c.Request().Context(), params)
	if err != nil {
		return handleError(c, err)
	}

	if result.Entries == nil {
		result.Entries = []usage.UsageLogEntry{}
	}

config/config.go:272

• PR description mentions adding a ModelListConfig with URL + timeout, but the config struct only includes URL and the fetch logic uses hard-coded 45s timeouts in providers/init.go and providers/registry.go. Either update the PR description, or add a configurable timeout field (with env/yaml support) and plumb it through the fetch/refresh paths so operators can tune it.

// ModelListConfig holds configuration for fetching the external model metadata registry.
type ModelListConfig struct {
	// URL is the HTTP(S) URL to fetch models.json from (empty = disabled)
	URL string `yaml:"url" env:"MODEL_LIST_URL"`
}

internal/core/types.go (1)

96-101: ⚠️ Potential issue | 🟡 Minor

Add a doc comment for Model.Metadata.

Line 100 exposes Metadata without a Go doc comment.

✍️ Suggested fix

 type Model struct {
 	ID       string         `json:"id"`
 	Object   string         `json:"object"`
 	OwnedBy  string         `json:"owned_by"`
 	Created  int64          `json:"created"`
+	// Metadata holds enriched information from the external model registry.
 	Metadata *ModelMetadata `json:"metadata,omitempty"`
 }

As per coding guidelines, "Use Go doc comments on exported types, functions, and interfaces to document public APIs".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/types.go` around lines 96 - 101, The exported field
Model.Metadata lacks a Go doc comment; add a one-line Go doc comment directly
above the Metadata field on the Model struct that explains what metadata
contains and when it may be nil (e.g., "Metadata contains optional model
metadata such as training data or tags; may be nil"). Reference the
Model.Metadata field and the ModelMetadata type in the comment so users can
understand the purpose and optional nature of the metadata.

internal/cache/cache.go (1)

14-17: ⚠️ Potential issue | 🟡 Minor

Add a doc comment for ModelListData.

Line 17 introduces an exported field without a Go doc comment; add a brief description of the raw registry JSON it stores.

✍️ Suggested fix

 type ModelCache struct {
 	Version       int                    `json:"version"`
 	UpdatedAt     time.Time              `json:"updated_at"`
 	Models        map[string]CachedModel `json:"models"`
+	// ModelListData stores the raw JSON payload fetched from the external model registry.
 	ModelListData json.RawMessage        `json:"model_list_data,omitempty"`
 }

As per coding guidelines, "Use Go doc comments on exported types, functions, and interfaces to document public APIs".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cache/cache.go` around lines 14 - 17, Add a Go doc comment for the
exported field ModelListData to describe what it holds; specifically, above the
ModelListData json.RawMessage field (the exported field in the cache struct
alongside Version, UpdatedAt, and Models), add a short sentence stating it
contains the raw registry JSON for the model list (e.g., "ModelListData contains
the raw registry JSON for available models") so the public API is documented.

internal/usage/reader_mongodb.go (1)

99-100: ⚠️ Potential issue | 🟡 Minor

Add a Go doc comment for GetUsageByModel.

Line 99 introduces an exported method without a Go doc comment.

✍️ Suggested fix

+// GetUsageByModel aggregates usage totals grouped by model and provider.
 func (r *MongoDBReader) GetUsageByModel(ctx context.Context, params UsageQueryParams) ([]ModelUsage, error) {

As per coding guidelines, "Use Go doc comments on exported types, functions, and interfaces to document public APIs".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_mongodb.go` around lines 99 - 100, Add a Go doc comment
for the exported method GetUsageByModel on MongoDBReader that begins with the
method name and briefly describes its behavior, expected parameters and return
values (e.g., what UsageQueryParams represents and that it returns a slice of
ModelUsage or an error); place the comment directly above the GetUsageByModel
declaration so it documents the public API.

internal/providers/init.go (1)

91-115: context.Background() ignores the Init context's cancellation — derive from ctx instead.

If the caller cancels ctx during shutdown (e.g., the application is shutting down while the registry is still initializing), this goroutine continues fetching for up to 45 seconds. Using ctx as the parent aligns the fetch lifetime with the initialization phase.

🛡️ Proposed fix

-			fetchCtx, cancel := context.WithTimeout(context.Background(), 45*time.Second)
+			fetchCtx, cancel := context.WithTimeout(ctx, 45*time.Second)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/init.go` around lines 91 - 115, The goroutine that calls
modeldata.Fetch currently creates a timeout from context.Background(), which
ignores the surrounding Init context cancellation; change the parent context
used to derive the timeout so it uses the outer ctx (i.e., use
context.WithTimeout(ctx, 45*time.Second) and keep the defer cancel()) so the
fetch (modeldata.Fetch) and subsequent registry.SetModelList /
registry.EnrichModels / registry.SaveToCache calls are aborted if the Init
context is canceled.

internal/admin/dashboard/static/css/dashboard.css (1)

839-843: ⚠️ Potential issue | 🟡 Minor

Stylelint font-family-name-quotes error on td.col-price.

Same issue already flagged in the previous review — 'Menlo' and 'Consolas' should be unquoted (single-word family names must not be quoted).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/static/css/dashboard.css` around lines 839 - 843,
The CSS for selector td.col-price is using quoted single-word family names which
triggers stylelint's font-family-name-quotes rule; update the font-family
declaration in td.col-price to keep the quoted multi-word family ('SF Mono') but
remove quotes around single-word families so it reads: 'SF Mono', Menlo,
Consolas, monospace (i.e., unquote Menlo and Consolas).

internal/modeldata/fetcher.go (1)

20-20: 🧹 Nitpick | 🔵 Trivial

Reusing http.Client for connection pooling is still unaddressed.

Each call to Fetch creates a fresh &http.Client{}, bypassing connection pooling and TLS session reuse. For the current use-case (background refresh on startup) this is low-risk but the previous suggestion to share a package-level or injected client still stands.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/fetcher.go` at line 20, The Fetch implementation currently
creates a new http.Client on each call (client := &http.Client{}), preventing
connection pooling and TLS session reuse; change Fetch to use a shared client
instead—either add a package-level variable (e.g., var httpClient =
&http.Client{}) or accept an injected *http.Client on the fetcher struct (e.g.,
add a Client field on the fetcher type and use f.Client in Fetch) and replace
the local client creation with that shared/injected client so connections and
TLS sessions are reused across calls.

internal/usage/reader_sqlite.go (1)

58-58: Add Go doc comments for GetUsageByModel and GetUsageLog.

Same issue as the PostgreSQL reader — both new exported methods lack doc comments. As per coding guidelines: **/*.go: Use Go doc comments on exported types, functions, and interfaces to document public APIs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_sqlite.go` at line 58, Add Go doc comments for the
exported methods on SQLiteReader: document GetUsageByModel and GetUsageLog with
brief descriptions of what they do, their parameters, and return values (e.g.,
mention ctx and params UsageQueryParams and that they return []ModelUsage and
error). Place the comments immediately above the function declarations using the
standard Go doc format starting with the function name (e.g., "GetUsageByModel
..." and "GetUsageLog ...") so they appear in godoc and satisfy the package's
documentation guidelines.

internal/usage/reader_postgresql.go (1)

58-58: Add Go doc comments for GetUsageByModel and GetUsageLog.

Both new exported methods lack doc comments. As per coding guidelines: **/*.go: Use Go doc comments on exported types, functions, and interfaces to document public APIs.

✍️ Suggested doc comments

+// GetUsageByModel returns aggregated input/output token usage and costs grouped by model and provider.
 func (r *PostgreSQLReader) GetUsageByModel(ctx context.Context, params UsageQueryParams) ([]ModelUsage, error) {

+// GetUsageLog returns a paginated list of individual usage log entries with optional filtering.
 func (r *PostgreSQLReader) GetUsageLog(ctx context.Context, params UsageLogParams) (*UsageLogResult, error) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` at line 58, Add Go doc comments for the
exported methods GetUsageByModel and GetUsageLog on the PostgreSQLReader
receiver: write short, clear sentences starting with the method name that
describe what each method does, its primary inputs (e.g., context.Context and
UsageQueryParams for GetUsageByModel) and what it returns (a slice of ModelUsage
and error, or usage logs and error). Place the comments immediately above the
function declarations for GetUsageByModel and GetUsageLog and follow Go doc
style (start with the function name and be concise).

internal/admin/dashboard/templates/index.html (2)

510-511: Empty state still doesn't account for category filtering.

When a user selects a category that has no models (e.g., "video") and modelFilter is empty, both existing empty-state paragraphs are hidden — leaving a blank area. A previous review flagged this same gap.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 510 - 511, The
empty-state logic hides both messages when a category is selected but
modelFilter is empty; update the x-show conditions so the "No models match your
filter." paragraph is shown whenever filteredModels.length === 0 (and not
loading/authError), regardless of modelFilter, and/or add a new paragraph that
triggers when filteredModels.length === 0 && selectedCategory (or
categoryFilter) is set; reference the template variables models, filteredModels,
modelFilter and the category selection variable (e.g., selectedCategory) and
change the x-show expressions to include filteredModels.length === 0 && !loading
&& !authError (and/or selectedCategory) so a clear empty-state appears when a
chosen category has no models.

---

366-392: "All" category view shows text-generation pricing columns for non-text models.

When activeCategory === 'all', the text_generation table layout is displayed for all models, including image, audio, and utility models that don't have input_per_mtok/output_per_mtok pricing. These rows will show empty cells in the pricing columns, which may confuse users.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 366 - 392, The
table currently shows text-generation pricing columns for every row when
activeCategory === 'all'; update the per-row pricing cell visibility so those
columns only render when the model is actually a text-generation model. Change
the pricing <td> elements (the ones using formatPrice and the cached column) to
include an additional condition like (activeCategory === 'text_generation' ||
(m.model.metadata?.modes ?? []).includes('text_generation')) in their x-show, so
only rows whose m.model.metadata.modes contains 'text_generation' (or when the
whole view is the text_generation category) display input_per_mtok,
output_per_mtok and cached_input_per_mtok; keep using formatPrice for rendering.

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

internal/usage/extractor_test.go (1)

203-341: Pricing branch in ExtractFrom remains untested.*

Still missing tests that pass pricing into ExtractFromChatResponse/ExtractFromResponsesResponse to assert InputCost/OutputCost/TotalCost/Caveat population. This was already noted in an earlier review.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/extractor_test.go` around lines 203 - 341, Tests for
ExtractFromChatResponse and ExtractFromResponsesResponse are missing assertions
that pricing passed into those functions populates InputCost, OutputCost,
TotalCost, and Caveat fields; add new test cases (or extend existing ones like
TestExtractFromChatResponse_WithPromptTokensDetails and
TestExtractFromResponsesResponse_WithDetails) that call ExtractFromChatResponse
and ExtractFromResponsesResponse with a non-nil pricing object (set input_cost,
output_cost, total_cost and a caveat string) and assert the returned entry has
entry.InputCost, entry.OutputCost, entry.TotalCost and entry.Caveat set to the
expected values; ensure you also cover precedence behavior if RawUsage/pricing
conflicts with details as needed.

internal/usage/cost_test.go (1)

37-54: OpenAI cached/reasoning expectations still assume additive pricing.

Cached/reasoning tokens are subsets of base counts, so base token costs should exclude them before applying special rates. This was already flagged earlier; the test still encodes the additive model.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/cost_test.go` around lines 37 - 54, The test
TestCalculateGranularCost_OpenAI_CachedAndReasoning assumes cached/reasoning
token charges are additive; instead update the expectations to treat
cached_tokens and reasoning_tokens as subsets of the base counts so base rates
exclude those tokens before applying special rates. Specifically, when calling
CalculateGranularCost(500_000, 300_000, rawData, "openai", pricing), compute
input base charge using (500_000 - cached_tokens) at InputPerMtok and
cached_tokens at CachedInputPerMtok, and compute output base charge using
(300_000 - reasoning_tokens) at OutputPerMtok and reasoning_tokens at
ReasoningOutputPerMtok, then update the assertCostNear expected values
accordingly so the test matches CalculateGranularCost's intended subtraction
behavior.

internal/cache/cache.go (1)

11-18: ⚠️ Potential issue | 🟡 Minor

Add a doc comment for ModelListData.

This exported field needs a short description of what it stores and when it’s populated.

📝 Suggested doc comment

 type ModelCache struct {
 	Version       int                    `json:"version"`
 	UpdatedAt     time.Time              `json:"updated_at"`
 	Models        map[string]CachedModel `json:"models"`
+	// ModelListData holds raw JSON from the external model registry.
 	ModelListData json.RawMessage        `json:"model_list_data,omitempty"`
 }

As per coding guidelines: "Update in-code Go doc comments on exported types, functions, and interfaces when changing public APIs, adding new exported symbols, or modifying function signatures/behavior."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/cache/cache.go` around lines 11 - 18, Add a Go doc comment for the
exported field ModelListData on the ModelCache type: describe that ModelListData
holds raw JSON of the original model list response (or metadata) used to
populate Models, specify its format (json.RawMessage), and note when it is
populated (e.g., populated when a full model list is fetched or cached) and that
it may be omitted/empty if not stored. Update the comment immediately above the
ModelListData field in the ModelCache struct so tools like godoc surface the
description.

internal/core/types.go (1)

94-101: ⚠️ Potential issue | 🟡 Minor

Add a doc comment for Model.Metadata.

A brief comment improves API clarity for the new exported field.

📝 Suggested doc comment

 type Model struct {
 	ID       string         `json:"id"`
 	Object   string         `json:"object"`
 	OwnedBy  string         `json:"owned_by"`
 	Created  int64          `json:"created"`
+	// Metadata holds enriched information from the external model registry.
 	Metadata *ModelMetadata `json:"metadata,omitempty"`
 }

As per coding guidelines: "Update in-code Go doc comments on exported types, functions, and interfaces when changing public APIs, adding new exported symbols, or modifying function signatures/behavior."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/types.go` around lines 94 - 101, Add a Go doc comment for the
exported Model.Metadata field to explain its purpose and when it can be nil;
update the struct declaration for type Model to include a short sentence above
or on the Metadata field referencing the ModelMetadata type (e.g., "Metadata
provides optional model-specific metadata; may be nil.") so the exported field
is documented for consumers and matches Go doc conventions for Model and
ModelMetadata.

internal/usage/cost.go (2)

93-196: ⚠️ Potential issue | 🔴 Critical

Avoid charging breakdown tokens twice.

Breakdown fields (cached/reasoning/audio tokens) are subsets of the base counts; the current flow applies the base rate to full inputTokens/outputTokens and then adds specialized cost on top, effectively double-charging those subsets. Subtract breakdown counts from the base before applying base rates (or deduct base cost for those subsets) and then add specialized pricing.

🐛 One way to adjust base costs after breakdowns

-    // Base input cost
-    if pricing.InputPerMtok != nil {
-        inputCost += float64(inputTokens) * *pricing.InputPerMtok / 1_000_000
-        hasInput = true
-    }
-
-    // Base output cost
-    if pricing.OutputPerMtok != nil {
-        outputCost += float64(outputTokens) * *pricing.OutputPerMtok / 1_000_000
-        hasOutput = true
-    }
+    adjustedInputTokens := inputTokens
+    adjustedOutputTokens := outputTokens

     // Apply provider-specific mappings
     if mappings, ok := providerMappings[providerType]; ok {
         for _, m := range mappings {
             count := extractInt(rawData, m.rawDataKey)
             if count == 0 {
                 continue
             }
             mappedKeys[m.rawDataKey] = true
+            if isBreakdownToken(m.rawDataKey) && m.unit == unitPerMtok {
+                switch m.side {
+                case sideInput:
+                    adjustedInputTokens -= count
+                    if adjustedInputTokens < 0 {
+                        adjustedInputTokens = 0
+                    }
+                case sideOutput:
+                    adjustedOutputTokens -= count
+                    if adjustedOutputTokens < 0 {
+                        adjustedOutputTokens = 0
+                    }
+                }
+            }

             rate := m.pricingField(pricing)
             if rate == nil {
                 continue // Base rate covers this token type; no adjustment needed
             }
             ...
         }
     }
+
+    // Base input cost (after breakdown adjustments)
+    if pricing.InputPerMtok != nil {
+        inputCost += float64(adjustedInputTokens) * *pricing.InputPerMtok / 1_000_000
+        hasInput = true
+    }
+
+    // Base output cost (after breakdown adjustments)
+    if pricing.OutputPerMtok != nil {
+        outputCost += float64(adjustedOutputTokens) * *pricing.OutputPerMtok / 1_000_000
+        hasOutput = true
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/cost.go` around lines 93 - 196, CalculateGranularCost is
double-charging breakdown token subsets because it applies base rates to full
inputTokens/outputTokens and then also adds provider-specific breakdown costs;
fix by subtracting mapped breakdown counts (e.g., cached, reasoning, audio) from
the base counts before computing the base cost: iterate
providerMappings[providerType] (and informationalFields) using extractInt to sum
counts per side (input/output), subtract those sums from
inputTokens/outputTokens (clamp at zero), then compute base input/output cost
using pricing.InputPerMtok/pricing.OutputPerMtok on the adjusted counts and
proceed to add specialized costs from each mapping as now they represent only
the incremental charge. Ensure you still mark mappedKeys and preserve PerRequest
addition and caveat logic.

---

43-68: ⚠️ Potential issue | 🟠 Major

Guard against double-counting when multiple RawData keys map to the same pricing field.

cached_tokens and prompt_cached_tokens (and the xAI equivalents) both map to CachedInputPerMtok. If both keys appear in rawData, the cost will be applied twice. Consider a dedupe guard or document mutual exclusivity to prevent double-charging.

🛡️ Suggested documentation guard

-// providerMappings defines the per-provider RawData key to pricing field mappings.
+// providerMappings defines the per-provider RawData key to pricing field mappings.
+// NOTE: Alternative key variants (e.g., "cached_tokens" vs "prompt_cached_tokens")
+// are mutually exclusive in practice — only one form should appear per response.
 var providerMappings = map[string][]tokenCostMapping{

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/cost.go` around lines 43 - 68, providerMappings contains
multiple entries that point different rawDataKey values (e.g., "cached_tokens"
and "prompt_cached_tokens" or their xai equivalents) to the same pricing field
(CachedInputPerMtok), which can double-count cost; update the cost-calculation
logic that iterates over providerMappings/tokenCostMapping to track which
pricingField (use the pricingField function identity or a derived unique key
like field name) has already been applied for a given rawData set and skip
subsequent mappings for the same pricing field when the earlier mapping was
used, ensuring each pricingField (e.g., CachedInputPerMtok,
ReasoningOutputPerMtok, InputPerImage, CacheWritePerMtok) is only charged once
per record; add a short comment near providerMappings and the calculation loop
(the code that reads rawData and applies tokenCostMapping) documenting why the
guard exists.

internal/modeldata/fetcher_test.go (1)

1-149: Missing test for the 10 MB response-body size cap.

The len(raw) > maxBodySize guard in fetcher.go remains untested. This was noted in a previous review cycle.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/fetcher_test.go` around lines 1 - 149, Add a unit test
that verifies the maxBodySize guard in Fetch rejects responses larger than 10MB:
create a new test (e.g., TestFetch_BodySizeLimit) that starts an httptest.Server
whose handler writes >maxBodySize bytes (use the same maxBodySize constant
name), call Fetch(ctx, server.URL) and assert it returns a non-nil error and
does not return a large raw body; reference Fetch and maxBodySize so the test
fails if the len(raw) > maxBodySize check is missing or broken.

internal/usage/reader_postgresql.go (4)

183-185: json.Unmarshal error silently discarded.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` around lines 183 - 185, The call to
json.Unmarshal on rawDataJSON currently discards errors; change the code in the
function handling row decoding (where rawDataJSON and e.RawData are used) to
capture the error returned by json.Unmarshal, and handle it appropriately (for
example, return the error up the call stack or log it with context). Replace "_
= json.Unmarshal(...)" with "if err := json.Unmarshal(...); err != nil { /*
return or log with context including rawDataJSON and the affected entity e */ }"
so parsing failures are not silently ignored.

---

62-108: ⚠️ Potential issue | 🟠 Major

GetUsageByModel has the same missing COALESCE on costCols; missing Go doc comment.

1. Missing COALESCE: Historical records inserted before cost tracking was added will have NULL cost columns. SUM(input_cost) over a group where every row has NULL costs returns SQL NULL, causing a scan failure identical to the GetSummary issue. Apply the same fix:

🐛 Proposed fix

-	costCols := `, SUM(input_cost), SUM(output_cost), SUM(total_cost)`
+	costCols := `, COALESCE(SUM(input_cost), 0), COALESCE(SUM(output_cost), 0), COALESCE(SUM(total_cost), 0)`

2. Missing doc comment: GetUsageByModel is an exported method added without a Go doc comment. As per coding guidelines, exported symbols require documentation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` around lines 62 - 108, GetUsageByModel
fails to COALESCE cost aggregates and lacks a Go doc comment; update the
costCols constant used in GetUsageByModel to wrap SUM(input_cost),
SUM(output_cost), SUM(total_cost) with COALESCE(...) so the query returns 0
instead of SQL NULL (fix the same way as GetSummary), and add a proper Go doc
comment for the exported GetUsageByModel method describing its behavior and
parameters; locate the costCols variable and the GetUsageByModel function (which
scans into ModelUsage and uses r.pool.Query / rows.Scan) and make the COALESCE
change and add the doc comment above the function signature.

---

167-167: append(args, limit, offset) may alias the args backing array.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` at line 167, The current dataArgs :=
append(args, limit, offset) can alias the original args backing array; to fix
it, create a new slice and copy args into it before appending limit and offset
(e.g., allocate a new slice of length len(args)+2, copy args, then set the last
two entries to limit and offset) so modifications to dataArgs won't mutate args;
update the code around the dataArgs construction (the variables dataArgs, args,
limit, offset) accordingly.

---

147-151: Unescaped LIKE wildcards in search (%, _ treated as SQL pattern chars).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` around lines 147 - 151, The search term
is used raw in ILIKE so literal % and _ in params.Search become SQL wildcards;
escape backslashes, percent and underscore before wrapping with % and adding to
args (e.g. escaped := strings.ReplaceAll(...)) and then append "%" + escaped +
"%" to args; update the SQL fragment built with conditions (the fmt.Sprintf that
uses argIdx) to include an ESCAPE clause (e.g. "ILIKE $%d ESCAPE '\\'") so the
DB treats backslash-escaped characters literally, and ensure argIdx is
incremented as before. Use the existing variables (params.Search, argIdx,
conditions, args) in reader_postgresql.go to locate and apply this change.

internal/providers/init.go (1)

91-116: context.Background() ignores Init ctx cancellation — use ctx instead.

Already flagged in a prior review cycle; the fix has not been applied. Deriving the timeout from ctx ensures the fetch goroutine respects shutdown cancellation.

-            fetchCtx, cancel := context.WithTimeout(context.Background(), 45*time.Second)
+            fetchCtx, cancel := context.WithTimeout(ctx, 45*time.Second)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/init.go` around lines 91 - 116, The goroutine that fetches
the model list uses context.WithTimeout(context.Background(), 45*time.Second)
which ignores the parent Init ctx; change it to derive from the provided ctx
(i.e., use context.WithTimeout(ctx, 45*time.Second)) so the fetch goroutine
respects Init cancellation, keep the cancel() defer, and ensure the closure
captures the outer ctx used by the Init function (refer to the anonymous
goroutine where fetchCtx, cancel := context.WithTimeout(...) is created and to
registry.SaveToCache / modeldata.Fetch calls).

internal/modeldata/enricher_test.go (2)

31-42: ptr() helper is undefined in this file.
Tests won’t compile unless the helper is added here or shared from another test file.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/enricher_test.go` around lines 31 - 42, The test
TestEnrich_MatchedAndUnmatched references an undefined helper ptr() causing
compilation failures; add a small local helper in the test file (or import it
from a shared test utility) that returns a pointer to the given literal type
used (e.g., func ptr[T any](v T) *T { return &v }) so the ModelList/ModelEntry
initialization (and any uses of ptr for ContextWindow, InputPerMtok,
OutputPerMtok) compiles and the test runs.

---

127-128: Guard against nil InputPerMtok before dereference.
This can panic if pricing is partially populated.

🛡️ Suggested null-safe assertion

-	if meta.Pricing == nil || *meta.Pricing.InputPerMtok != 2.50 {
-		t.Error("expected pricing from base model via reverse lookup")
-	}
+	if meta.Pricing == nil {
+		t.Error("expected non-nil pricing from base model via reverse lookup")
+	} else if meta.Pricing.InputPerMtok == nil || *meta.Pricing.InputPerMtok != 2.50 {
+		t.Errorf("InputPerMtok = %v, want 2.50", meta.Pricing.InputPerMtok)
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/enricher_test.go` around lines 127 - 128, Guard against
nil InputPerMtok before dereferencing in the test: update the assertion that
currently checks meta.Pricing and *meta.Pricing.InputPerMtok so it first ensures
meta.Pricing.InputPerMtok != nil (in addition to meta.Pricing != nil) and only
then dereferences it to compare to 2.50; locate the failing assertion around
meta.Pricing and InputPerMtok in enricher_test.go and change the conditional to
perform a null-safe check and comparison.

internal/modeldata/merger.go (2)

38-44: Reverse-lookup recursion can loop without a guard.
A cyclic mapping in providerModelByActualID could recurse indefinitely; add a depth/visited guard or resolve inline.

🛡️ Example guard

-			reverseKey := providerType + "/" + modelID
-			if compositeKey, ok := list.providerModelByActualID[reverseKey]; ok {
-				return Resolve(list, providerType, compositeKey[len(providerType)+1:])
+			reverseKey := providerType + "/" + modelID
+			if compositeKey, ok := list.providerModelByActualID[reverseKey]; ok {
+				canonicalModelID := compositeKey[len(providerType)+1:]
+				if canonicalModelID != modelID {
+					return Resolve(list, providerType, canonicalModelID)
+				}
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/merger.go` around lines 38 - 44, The reverse-lookup in
Resolve (when model == nil && pm == nil) uses providerModelByActualID and calls
Resolve recursively with compositeKey, which can infinite-loop on cyclic
mappings; modify Resolve (or add an internal helper) to accept a small recursion
depth counter or a visited set (e.g., visited map[string]bool keyed by
providerType+"/"+modelID) and check it before recursing, returning a nil/err
when a cycle is detected; update the call site that currently builds reverseKey
and calls Resolve(list, providerType, compositeKey[len(providerType)+1:]) to
mark the key visited (or increment depth) and pass it through so cycles are
detected and recursion stops.

---

69-79: Provider-level capabilities are dropped.
ProviderModelEntry.Capabilities isn’t applied, so provider-specific overrides are lost.

🛠️ Example merge

 	if pm != nil {
@@
 		if pm.Pricing != nil {
 			meta.Pricing = pm.Pricing
 		}
+		if pm.Capabilities != nil {
+			if meta.Capabilities == nil {
+				meta.Capabilities = map[string]bool{}
+			}
+			for k, v := range pm.Capabilities {
+				meta.Capabilities[k] = v
+			}
+		}
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/merger.go` around lines 69 - 79, The merge ignores
provider model capabilities so any ProviderModelEntry.Capabilities overrides are
dropped; in the merger where pm and meta are merged (variables pm and meta in
internal/modeldata/merger.go), add a non-nil check for pm.Capabilities and
assign meta.Capabilities = pm.Capabilities when present so provider-specific
capabilities override the base (i.e., alongside the existing checks for
ContextWindow, MaxOutputTokens, and Pricing).

internal/modeldata/fetcher.go (1)

20-21: Consider reusing an http.Client for connection pooling.
Creating a new client per fetch prevents reuse; a shared or injected client is more efficient if fetch frequency increases.

♻️ Possible refactor

+var httpClient = &http.Client{}
+
 func Fetch(ctx context.Context, url string) (*ModelList, []byte, error) {
@@
-	client := &http.Client{}
+	client := httpClient

To gauge whether pooling would help, you can verify how often Fetch is invoked:

#!/bin/bash
# Find call sites of modeldata.Fetch
rg -n 'modeldata\.Fetch' --type=go

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/fetcher.go` around lines 20 - 21, The code creates a new
http.Client inside Fetch (client := &http.Client{}), which prevents connection
pooling; change Fetch to reuse a shared client by either adding a Client
*http.Client field to the fetcher struct (or adding a constructor that accepts a
client) and use that field instead of creating a new client, or declare a
package-level httpClient variable initialized once and use it in Fetch; update
any constructors or callers (e.g., NewFetcher or Fetch) to supply/inject the
shared client so requests can reuse connections.

internal/admin/handler.go (2)

225-234: Enforce a default and max limit for usage logs.
Without bounds, callers can request unbounded rows (or 0 meaning “no limit”), which can lead to heavy reads.

🔒 Suggested guardrails

+const (
+	defaultUsageLogLimit = 100
+	maxUsageLogLimit     = 1000
+)
+
 	if l := c.QueryParam("limit"); l != "" {
 		if parsed, err := strconv.Atoi(l); err == nil && parsed > 0 {
-			params.Limit = parsed
+			if parsed > maxUsageLogLimit {
+				parsed = maxUsageLogLimit
+			}
+			params.Limit = parsed
 		}
 	}
+	if params.Limit == 0 {
+		params.Limit = defaultUsageLogLimit
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/handler.go` around lines 225 - 234, The query parsing for
"limit" in internal/admin/handler.go must enforce a sensible default and an
upper bound to prevent unbounded reads: when handling the "limit" param (the
existing block that reads c.QueryParam("limit") and assigns params.Limit), treat
missing or non-positive values as DefaultUsageLogsLimit, and clamp any parsed
positive value to a MaxUsageLogsLimit; add or use constants like
DefaultUsageLogsLimit and MaxUsageLogsLimit and apply the clamp before assigning
params.Limit. Keep the "offset" handling but ensure it still validates
non-negative values (params.Offset) as before.

---

182-203: Swagger annotations are missing for the new admin handlers.
These endpoints won’t appear in generated docs without full @Summary/@Tags/@Param/@Success/@Failure/@Router blocks.

Also applies to: 205-246, 277-284

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/handler.go` around lines 182 - 203, Add full Swagger/OpenAPI
annotations to the new admin handler functions so they appear in generated docs:
update the UsageByModel function by adding `@Summary`, `@Tags` (e.g., Admin), `@Param`
entries for query/path parameters, `@Success` (200) response schema for
[]usage.ModelUsage, `@Failure` entries for common errors, and a `@Router`
declaration for GET /admin/api/v1/usage/models; apply the same annotation
pattern to the other new admin handlers in this file (the handlers immediately
following UsageByModel) so each endpoint has complete
`@Summary/`@Tags/@Param/@Success/@Failure/@Router blocks.

internal/admin/dashboard/static/css/dashboard.css (1)

839-842: Fix stylelint font-family-name-quotes in .col-price.
Menlo and Consolas should be unquoted to satisfy the linter.

🧹 Suggested fix

-    font-family: 'SF Mono', 'Menlo', 'Consolas', monospace;
+    font-family: 'SF Mono', Menlo, Consolas, monospace;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/static/css/dashboard.css` around lines 839 - 842,
Update the .col-price rule to satisfy stylelint's font-family-name-quotes by
unquoting generic font family identifiers: keep 'SF Mono' quoted (it contains a
space) but remove the quotes around Menlo and Consolas in the font-family list
in the td.col-price selector so it reads something like "font-family: 'SF Mono',
Menlo, Consolas, monospace;".

internal/providers/registry_test.go (1)

669-671: Avoid hardcoding the category count.
Use len(core.AllCategories()) so the test stays correct if categories change.

♻️ Suggested fix

-	if len(counts) != 7 {
-		t.Fatalf("expected 7 category counts, got %d", len(counts))
+	if want := len(core.AllCategories()); len(counts) != want {
+		t.Fatalf("expected %d category counts (len(core.AllCategories())), got %d", want, len(counts))
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry_test.go` around lines 669 - 671, The test
currently hardcodes the expected category count (7) when checking len(counts);
replace that literal with the dynamic value len(core.AllCategories()) and update
the assertion message accordingly (e.g., compute expected :=
len(core.AllCategories()) and use expected in the t.Fatalf) so the test stays
correct when categories change; locate the check around the variable counts in
registry_test.go and modify it to compare against len(core.AllCategories()) (and
use the expected variable in the error string).

internal/usage/reader_sqlite.go (3)

110-110: ⚠️ Potential issue | 🟡 Minor

Add a Go doc comment for GetUsageLog.

This exported method lacks a doc comment.

✍️ Proposed fix

+// GetUsageLog returns a paginated list of usage log entries with filtering options.
func (r *SQLiteReader) GetUsageLog(ctx context.Context, params UsageLogParams) (*UsageLogResult, error) {

As per coding guidelines: "Update in-code Go doc comments on exported types, functions, and interfaces when changing public APIs, adding new exported symbols, or modifying function signatures/behavior".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_sqlite.go` at line 110, Add a Go doc comment for the
exported method GetUsageLog on SQLiteReader: place a sentence or two immediately
above the function declaration starting with "GetUsageLog" (e.g., "GetUsageLog
...") describing what the method does, its receiver (SQLiteReader), what the
params (UsageLogParams) represent and what the returned *UsageLogResult and
error mean; ensure the comment follows Go doc style (starts with the function
name) and is concise.

---

62-62: ⚠️ Potential issue | 🟡 Minor

Add a Go doc comment for GetUsageByModel.

This exported method lacks a doc comment.

✍️ Proposed fix

+// GetUsageByModel aggregates input/output tokens and costs grouped by model and provider.
func (r *SQLiteReader) GetUsageByModel(ctx context.Context, params UsageQueryParams) ([]ModelUsage, error) {

As per coding guidelines: "Update in-code Go doc comments on exported types, functions, and interfaces when changing public APIs, adding new exported symbols, or modifying function signatures/behavior".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_sqlite.go` at line 62, Add a Go doc comment above the
exported method GetUsageByModel on type SQLiteReader describing what the method
does (e.g., "GetUsageByModel returns usage metrics grouped by model"), mention
the meaning of the params parameter (UsageQueryParams) and that it returns a
slice of ModelUsage and an error, and ensure the comment starts with
"GetUsageByModel" to follow Go doc conventions; place the comment immediately
above the func declaration "func (r *SQLiteReader) GetUsageByModel(ctx
context.Context, params UsageQueryParams) ([]ModelUsage, error)".

---

175-181: ⚠️ Potential issue | 🟡 Minor

Handle timestamp parse failures explicitly.

If all parse attempts fail, the timestamp remains the zero value, which leaks into the UI. Consider logging or surfacing a parse failure indicator instead of silently falling back.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_sqlite.go` around lines 175 - 181, The timestamp
parsing chain silently leaves e.Timestamp as the zero value if all parses fail;
add an explicit else branch after the last time.Parse that captures the final
parse error, logs a warning (including the raw ts and the parse errors), and
mark the event with an explicit parse-failure flag (e.g., set a new bool field
like e.TimestampParseFailed = true or populate e.TimestampParseError) so the UI
can detect and show a parse-failure indicator instead of silently displaying the
zero time; update the Event struct (or equivalent) to hold that flag/field and
propagate it where timestamps are rendered.

internal/usage/reader_mongodb.go (2)

99-99: ⚠️ Potential issue | 🟡 Minor

Add a Go doc comment for GetUsageByModel.

This exported method lacks a doc comment.

✍️ Proposed fix

+// GetUsageByModel aggregates input/output tokens and costs grouped by model and provider.
func (r *MongoDBReader) GetUsageByModel(ctx context.Context, params UsageQueryParams) ([]ModelUsage, error) {

As per coding guidelines: "Update in-code Go doc comments on exported types, functions, and interfaces when changing public APIs, adding new exported symbols, or modifying function signatures/behavior".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_mongodb.go` at line 99, Add a proper Go doc comment for
the exported method GetUsageByModel on type MongoDBReader: include a one-line
summary of what the method does, mention the meaning of the ctx context.Context
and params UsageQueryParams parameters, and describe the returned []ModelUsage
slice and error; ensure the comment begins with "GetUsageByModel" to follow Go
doc conventions and lives immediately above the function declaration for
GetUsageByModel.

---

179-179: ⚠️ Potential issue | 🟡 Minor

Add a Go doc comment for GetUsageLog.

This exported method lacks a doc comment.

✍️ Proposed fix

+// GetUsageLog returns a paginated list of usage log entries with filtering options.
func (r *MongoDBReader) GetUsageLog(ctx context.Context, params UsageLogParams) (*UsageLogResult, error) {

As per coding guidelines: "Update in-code Go doc comments on exported types, functions, and interfaces when changing public APIs, adding new exported symbols, or modifying function signatures/behavior".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_mongodb.go` at line 179, Add a Go doc comment for the
exported method GetUsageLog on type MongoDBReader: write a concise sentence
starting with "GetUsageLog ..." that describes what the method does, mention key
parameters (ctx and params) and what it returns (*UsageLogResult, error), and
include any important behavior or error conditions callers should know about
(e.g., context cancellation, DB lookup semantics). Place the comment immediately
above the GetUsageLog function declaration to satisfy Go documentation
conventions.

internal/admin/dashboard/templates/index.html (4)

316-318: ⚠️ Potential issue | 🟡 Minor

Guard pagination range when offset exceeds total.

If the current page is empty, the start can exceed the end. Clamp the start value.

✍️ Proposed fix

-<span class="pagination-info" x-text="'Showing ' + (usageLog.offset + 1) + '-' + Math.min(usageLog.offset + usageLog.limit, usageLog.total) + ' of ' + usageLog.total"></span>
+<span class="pagination-info" x-text="'Showing ' + Math.min(usageLog.offset + 1, usageLog.total) + '-' + Math.min(usageLog.offset + usageLog.limit, usageLog.total) + ' of ' + usageLog.total"></span>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 316 - 318, Clamp
the pagination start so it never exceeds total when usageLog.offset is past the
end: update the x-text expression in the pagination-info span that currently
builds "'Showing ' + (usageLog.offset + 1) + '-' + Math.min(usageLog.offset +
usageLog.limit, usageLog.total) + ' of ' + usageLog.total" to compute start =
Math.min(usageLog.offset + 1, usageLog.total) and keep end =
Math.min(usageLog.offset + usageLog.limit, usageLog.total) (both using the
usageLog object), so the displayed range is "'Showing ' + start + '-' + end + '
of ' + usageLog.total".

---

153-239: 🧹 Nitpick | 🔵 Trivial

Extract duplicated date-picker markup into a template partial.

The overview and usage pages now share identical date-picker blocks; a partial would reduce drift.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 153 - 239, There
are two identical date-picker blocks duplicated across overview and usage pages;
extract the entire date-picker markup (the container div with class
"date-picker" including its buttons, calendars, templates and dp-cursor-hint)
into a reusable template partial (e.g., _date-picker.html) and replace both
occurrences with an include/partial import. Ensure the partial preserves the
Alpine bindings and references used by the surrounding component
(datePickerOpen, toggleDatePicker(), dateRangeLabel(), selectedPreset,
selectPreset(), calendarTitle(), calendarDays(), prevMonth(), nextMonth(),
isCurrentMonth(), isToday(), isRangeStart(), isRangeEnd(), isInRange(),
isFutureDay(), selectCalendarDay(), onCalendarMouseMove(),
onCalendarMouseLeave(), cursorHint, selectingDate, selectionHint()) so behavior
remains unchanged after replacing the duplicated markup with the include.

---

499-500: ⚠️ Potential issue | 🟡 Minor

Add an empty state when a category has no models.

Filtering by category can yield zero results even when models exist overall.

✍️ Proposed fix

 <p class="empty-state" x-show="models.length === 0 && !loading && !authError && !modelFilter">No models registered.</p>
 <p class="empty-state" x-show="models.length > 0 && filteredModels.length === 0 && modelFilter">No models match your filter.</p>
+<p class="empty-state" x-show="models.length > 0 && filteredModels.length === 0 && !modelFilter && activeCategory !== 'all'">No models in this category.</p>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 499 - 500, Add a
third empty-state paragraph to handle the case where there are models overall
but none match the currently selected category: render a message like "No models
in this category." with an x-show that evaluates models.length > 0 &&
filteredModels.length === 0 && <categoryFilterVar>; use the actual category
filter variable name present in the template (e.g., categoryFilter or
selectedCategory) instead of <categoryFilterVar> so the condition only triggers
for category filtering and not for overall empty or text-filter empty states.

---

355-377: ⚠️ Potential issue | 🟡 Minor

Hide text-only pricing columns when category = all.

“All” includes non-text models, so text-specific MTok columns become misleading. Consider rendering those columns only for the text_generation category (or swap to a generic pricing view).

✍️ One possible adjustment

-<th class="col-price">Input $/MTok</th>
-<th class="col-price">Output $/MTok</th>
-<th class="col-price" x-show="activeCategory === 'text_generation'">Cached $/MTok</th>
+<th class="col-price" x-show="activeCategory === 'text_generation'">Input $/MTok</th>
+<th class="col-price" x-show="activeCategory === 'text_generation'">Output $/MTok</th>
+<th class="col-price" x-show="activeCategory === 'text_generation'">Cached $/MTok</th>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 355 - 377, The
text-specific pricing columns (Input $/MTok, Output $/MTok and Cached $/MTok)
are currently shown for "all" which is misleading; update the header THs and the
corresponding TDs inside the template x-for so they only render when
activeCategory === 'text_generation' (e.g., add x-show="activeCategory ===
'text_generation'" to the two <th class="col-price"> for Input/Output and to
both <td class="col-price"
x-text="formatPrice(m.model.metadata?.pricing?.input_per_mtok)" and the output
TD), leaving the cached column's existing x-show as-is, so the table rows
(template x-for="m in filteredModels") and headers stay aligned when
activeCategory !== 'text_generation'.

internal/admin/dashboard/static/js/dashboard.js (2)

631-634: ⚠️ Potential issue | 🟡 Minor

Reset usage-log pagination when the query changes (offset carry-over bug).

fetchUsagePage() is called after date-range changes (and on navigation), but it calls fetchUsageLog(false) which retains the previous offset. That can easily yield empty pages for the new query. This was previously flagged and still applies.

Proposed fix

 async fetchUsagePage() {
-    await Promise.all([this.fetchModelUsage(), this.fetchUsageLog(false)]);
+    await Promise.all([this.fetchModelUsage(), this.fetchUsageLog(true)]);
     this.renderDonutChart();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/static/js/dashboard.js` around lines 631 - 634,
fetchUsagePage calls fetchUsageLog(false) and therefore preserves the previous
pagination offset, causing empty pages after date-range or query changes; update
fetchUsagePage to reset pagination before fetching (either call
fetchUsageLog(true) or explicitly set this.usageLog.offset = 0 /
this.usageLog.page = 1) so the usage log always starts at the first page when
the query/date-range changes; refer to fetchUsagePage and fetchUsageLog to
implement the change.

---

691-701: ⚠️ Potential issue | 🔴 Critical

Fix Biome lint errors: forEach callbacks must not implicitly return.

Biome flags m => set.add(...) because Set.prototype.add() returns the set (non-void). This is likely pipeline-blocking given the lint severity shown. This was previously flagged and still applies.

Proposed fix

 usageLogModelOptions() {
     const set = new Set();
-    this.modelUsage.forEach(m => set.add(m.model));
+    this.modelUsage.forEach(m => { set.add(m.model); });
     return [...set].sort();
 }

 usageLogProviderOptions() {
     const set = new Set();
-    this.modelUsage.forEach(m => set.add(m.provider));
+    this.modelUsage.forEach(m => { set.add(m.provider); });
     return [...set].sort();
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/static/js/dashboard.js` around lines 691 - 701, The
arrow callbacks in usageLogModelOptions and usageLogProviderOptions implicitly
return values because they use m => set.add(...), which Biome flags; change the
callbacks to not return anything by either replacing the forEach with a for...of
loop over this.modelUsage or using a block-bodied callback like m => {
set.add(m.model); } (and similarly for provider) so Set.add is called but no
value is implicitly returned; update both functions (usageLogModelOptions and
usageLogProviderOptions) accordingly.

internal/providers/registry.go (2)

204-227: ⚠️ Potential issue | 🟡 Minor

Preserve cached raw ModelListData even when parsing fails.

Right now r.modelListRaw is only set when Parse() succeeds. If the cached schema changes and parsing fails, the next SaveToCache() drops the raw payload, preventing a future re-parse after a code upgrade.
This matches an earlier review concern and still applies in the current code.

Proposed fix

 r.mu.Lock()
 r.models = newModels
-	if list != nil {
-		r.modelList = list
-		r.modelListRaw = modelCache.ModelListData
-	}
+	// Preserve raw bytes regardless of parse success so future versions can re-parse.
+	if len(modelCache.ModelListData) > 0 {
+		r.modelListRaw = modelCache.ModelListData
+	}
+	if list != nil {
+		r.modelList = list
+	}
 r.mu.Unlock()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry.go` around lines 204 - 227, The code currently
sets r.modelListRaw only when modeldata.Parse(modelCache.ModelListData)
succeeds, which allows SaveToCache() to drop the raw payload if parsing fails;
change the logic so r.modelListRaw is assigned modelCache.ModelListData
regardless of Parse() success (keep r.modelList set only when Parse returns a
parsed list), i.e., after attempting modeldata.Parse(...) always preserve
modelCache.ModelListData into r.modelListRaw so future re-parsing is possible
while still only assigning r.modelList when parsed succeeds.

---

489-500: ⚠️ Potential issue | 🟡 Minor

Avoid blank UI labels when core.AllCategories() adds a new category.

DisplayName: categoryDisplayNames[cat] will become "" for any category not in the map. This was previously flagged and is still present.

Proposed fix

 	for _, cat := range allCategories {
 		count := counts[cat]
 		if cat == core.CategoryAll {
 			count = len(r.models)
 		}
+		name := categoryDisplayNames[cat]
+		if name == "" {
+			name = fmt.Sprint(cat)
+		}
 		result = append(result, CategoryCount{
 			Category:    cat,
-			DisplayName: categoryDisplayNames[cat],
+			DisplayName: name,
 			Count:       count,
 		})
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry.go` around lines 489 - 500, When building the
CategoryCount list inside the loop over core.AllCategories(), guard the lookup
into categoryDisplayNames so you don't insert an empty DisplayName for unknown
categories: check for existence (e.g. val, ok := categoryDisplayNames[cat]) and
if missing fall back to a sensible default such as cat.String() or the category
identifier, then use that value when constructing the CategoryCount; keep the
existing logic for core.CategoryAll/counts and the same fields (CategoryCount,
categoryDisplayNames, counts, r.models).

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

internal/usage/reader_sqlite.go (2)

158-161: ⚠️ Potential issue | 🟠 Major

Cost columns in GetUsageLog SELECT lack COALESCE for historical rows.

Historical rows inserted before this PR will have NULL in input_cost, output_cost, and total_cost. Depending on the Go type of the scan targets, this may cause scan errors.

Also, dataArgs := append(args, limit, offset) can alias args' backing array — a known Go footgun flagged in a prior review.

♻️ Proposed fixes

 	dataQuery := `SELECT id, request_id, provider_id, timestamp, model, provider, endpoint,
-		input_tokens, output_tokens, total_tokens, input_cost, output_cost, total_cost, raw_data, costs_calculation_caveat
+		input_tokens, output_tokens, total_tokens, COALESCE(input_cost, 0), COALESCE(output_cost, 0), COALESCE(total_cost, 0), raw_data, COALESCE(costs_calculation_caveat, '')
 		FROM usage` + where + ` ORDER BY timestamp DESC LIMIT ? OFFSET ?`
-	dataArgs := append(args, limit, offset)
+	dataArgs := make([]interface{}, len(args), len(args)+2)
+	copy(dataArgs, args)
+	dataArgs = append(dataArgs, limit, offset)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_sqlite.go` around lines 158 - 161, GetUsageLog's
dataQuery selects input_cost, output_cost, and total_cost without COALESCE which
causes NULLs for historic rows to error on scanning; change the SELECT in
dataQuery to wrap those three columns with COALESCE(..., 0) (referencing the
dataQuery variable and the column names input_cost, output_cost, total_cost) so
scans always get numeric zeros instead of NULL. Also avoid aliasing args'
backing array when building dataArgs (referencing dataArgs, args, limit, offset)
by creating a new slice (e.g., make/copy or append to a nil slice) and then
adding limit and offset instead of directly doing dataArgs := append(args,
limit, offset).

---

179-185: ⚠️ Potential issue | 🟡 Minor

Timestamp parse failure silently leaves e.Timestamp as zero value.

If none of the three formats match, e.Timestamp remains 0001-01-01T00:00:00Z, which will surface as confusing data in the UI. Consider logging a warning (consistent with the JSON unmarshal warning on line 188) when all parse attempts fail.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_sqlite.go` around lines 179 - 185, The timestamp parse
block leaves e.Timestamp as the zero value if none of the three time.Parse
attempts succeed; modify the parse logic in reader_sqlite.go (the block that
sets e.Timestamp from ts) to detect when all parses fail and emit a warning
similar to the JSON unmarshal warning on line 188 — include the original ts
value and context (e.g., "failed to parse timestamp for entry") in the log so
callers can diagnose bad data while leaving e.Timestamp unchanged.

internal/usage/reader_postgresql.go (3)

167-171: ⚠️ Potential issue | 🟠 Major

GetUsageLog cost columns lack COALESCE, and append may alias args.

Two issues previously flagged that remain:

1. input_cost, output_cost, total_cost are not wrapped in COALESCE (unlike costs_calculation_caveat on the same line), so NULL from historical rows will cause scan errors.
2. dataArgs := append(args, limit, offset) can mutate args' backing array.

🐛 Proposed fix

 	dataQuery := fmt.Sprintf(`SELECT id, request_id, provider_id, timestamp, model, provider, endpoint,
-		input_tokens, output_tokens, total_tokens, input_cost, output_cost, total_cost, raw_data, COALESCE(costs_calculation_caveat, '')
+		input_tokens, output_tokens, total_tokens, COALESCE(input_cost, 0), COALESCE(output_cost, 0), COALESCE(total_cost, 0), raw_data, COALESCE(costs_calculation_caveat, '')
 		FROM "usage"%s ORDER BY timestamp DESC LIMIT $%d OFFSET $%d`, where, argIdx, argIdx+1)
-	dataArgs := append(args, limit, offset)
+	dataArgs := make([]interface{}, len(args), len(args)+2)
+	copy(dataArgs, args)
+	dataArgs = append(dataArgs, limit, offset)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` around lines 167 - 171, In GetUsageLog
change the SELECT in dataQuery to wrap input_cost, output_cost, total_cost with
COALESCE(..., 0) (or another appropriate default) just like
costs_calculation_caveat to avoid NULL scan errors, and avoid mutating the
original args when building dataArgs by creating a new slice (e.g. copy args
into a new slice or use append on an empty slice) before appending limit and
offset so args' backing array isn't aliased.

---

33-33: ⚠️ Potential issue | 🟠 Major

GetSummary cost columns still lack COALESCE — flagged in prior review.

SUM(input_cost), SUM(output_cost), and SUM(total_cost) return NULL when no rows match. pgx v5 will error on scanning NULL into a non-nullable float64.

🐛 Proposed fix

-	costCols := `, SUM(input_cost), SUM(output_cost), SUM(total_cost)`
+	costCols := `, COALESCE(SUM(input_cost), 0), COALESCE(SUM(output_cost), 0), COALESCE(SUM(total_cost), 0)`

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` at line 33, GetSummary's costCols uses
SUM(input_cost), SUM(output_cost), SUM(total_cost) which can be NULL and will
cause pgx v5 scan errors into float64; update the costCols string in
reader_postgresql.go (the GetSummary query builder) to wrap each SUM with
COALESCE(..., 0) — e.g. COALESCE(SUM(input_cost), 0), COALESCE(SUM(output_cost),
0), COALESCE(SUM(total_cost), 0) — so the query returns zeros instead of NULLs
and scans safely into non-nullable floats.

---

72-72: ⚠️ Potential issue | 🟠 Major

GetUsageByModel cost columns also lack COALESCE.

Same issue — SUM(input_cost) returns NULL for groups with all-NULL cost values (historical pre-pricing rows).

🐛 Proposed fix

-	costCols := `, SUM(input_cost), SUM(output_cost), SUM(total_cost)`
+	costCols := `, COALESCE(SUM(input_cost), 0), COALESCE(SUM(output_cost), 0), COALESCE(SUM(total_cost), 0)`

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/reader_postgresql.go` at line 72, GetUsageByModel's costCols
currently uses SUM(input_cost), SUM(output_cost), SUM(total_cost) which can
return NULL for groups with all-NULL values; update the costCols string in
reader_postgresql.go to wrap each SUM with COALESCE (e.g.,
COALESCE(SUM(input_cost), 0), COALESCE(SUM(output_cost), 0),
COALESCE(SUM(total_cost), 0)) so zero is returned instead of NULL and downstream
handling in GetUsageByModel remains consistent.

internal/core/types.go (1)

68-92: LGTM on the new detail types and Usage expansion.

PromptTokensDetails and CompletionTokensDetails are clean breakdowns with appropriate doc comments. The pointer fields on Usage correctly handle optionality.

The RawUsage map[string]any concern was already raised in a prior review.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/core/types.go` around lines 68 - 92, Change the RawUsage field on
the Usage struct from a generic map[string]any to a raw JSON payload to avoid
loose typing and preserve unknown provider fields: update the Usage definition
(symbol Usage and field RawUsage) to use json.RawMessage (encoding/json) instead
of map[string]any, keep the json tag, and add the encoding/json import; this
preserves the raw provider usage blob and defers unmarshalling to callers who
know the schema.

internal/usage/cost.go (1)

99-162: ⚠️ Potential issue | 🔴 Critical

Critical: Specialized token costs are still added on top of full base costs (double-counting).

This was flagged in a prior review and remains unaddressed. For OpenAI/xAI/Gemini, cached tokens and reasoning tokens are subsets of prompt_tokens/completion_tokens. The base cost (lines 112-121) charges the full token count, then lines 128-162 add specialized token costs on top — charging those tokens twice at different rates instead of replacing the base rate with the specialized rate.

Example with OpenAI, 500k prompt tokens (200k cached), InputPerMtok=$2.50, CachedInputPerMtok=$1.25:

• Current: 500k × $2.50/M + 200k × $1.25/M = $1.50
• Correct: (500k − 200k) × $2.50/M + 200k × $1.25/M = $1.00

The fix should subtract the specialized count from the base before applying the base rate, or equivalently, add the delta (specialized_rate − base_rate) × count / 1M for per-Mtok items.

Note: Anthropic's cache_read_input_tokens and cache_creation_input_tokens are separate line items (not subsets of input_tokens), so they should remain additive. A per-provider isSubset flag on the mapping would cleanly express this.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/usage/cost.go` around lines 99 - 162, CalculateGranularCost
currently double-counts subset token types (cached/reasoning) because the base
per-Mtok cost (pricing.InputPerMtok/pricing.OutputPerMtok) is applied to the
full token totals and specialized mapping costs are then added on top; update
the providerMappings/mapping handling so mappings can declare isSubset (boolean)
and, for mappings whose isSubset==true and whose unit==unitPerMtok and whose
pricing field (rate) != nil, apply only the delta between the specialized rate
and the base rate (i.e. delta = (*rate - *baseRate) * count / 1_000_000) or
equivalently subtract base_rate*count/1_000_000 from inputCost/outputCost before
adding the specialized cost; ensure this logic references CalculateGranularCost,
providerMappings, the mapping struct (add isSubset), appliedFields, and
pricing.InputPerMtok/pricing.OutputPerMtok, and keep Anthropic mappings with
isSubset=false so their tokens remain additive.

internal/modeldata/enricher_test.go (1)

31-42: ⚠️ Potential issue | 🟠 Major

Confirm ptr() helper is in the same package; tests won’t compile otherwise.

ptr(...) is used but not defined in this file. If the helper only exists in a modeldata_test package, these tests will fail to compile. Ensure ptr is defined in package modeldata or add a local helper here.

🛠️ Minimal local helper (if needed)

+func ptr[T any](v T) *T { return &v }

Run the following to confirm helper visibility:

#!/bin/bash
rg -n 'func ptr' internal/modeldata
rg -n '^package ' internal/modeldata/*_test.go

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/modeldata/enricher_test.go` around lines 31 - 42, Tests use the
ptr(...) helper but it's not defined in this package, causing compilation
failures; add a small helper function named ptr in the same package as the tests
(modeldata) or declare a local helper at top of
internal/modeldata/enricher_test.go so ptr[T any](v T) *T is visible to
TestEnrich_MatchedAndUnmatched and other tests referencing ptr, ensuring the
symbol is in package modeldata (not modeldata_test) or duplicated locally in the
test file.

internal/admin/dashboard/static/js/dashboard.js (1)

691-699: ⚠️ Potential issue | 🟡 Minor

Fix Biome lint: forEach callbacks should not return values.

Biome flags implicit returns from forEach callbacks here. Wrap the bodies in braces.

🛠️ Suggested fix

-            this.modelUsage.forEach(m => set.add(m.model));
+            this.modelUsage.forEach(m => { set.add(m.model); });
@@
-            this.modelUsage.forEach(m => set.add(m.provider));
+            this.modelUsage.forEach(m => { set.add(m.provider); });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/static/js/dashboard.js` around lines 691 - 699, The
Biome lint flags implicit returns from the arrow callbacks in
usageLogModelOptions() and usageLogProviderOptions(): change the forEach
callbacks so they use block bodies instead of implicit returns — e.g., in
usageLogModelOptions() replace this.modelUsage.forEach(m => set.add(m.model))
with a block callback that calls set.add(m.model) inside braces, and do the
analogous change in usageLogProviderOptions() for set.add(m.provider); this
removes the implicit return and satisfies the lint rule while keeping the same
behavior.

internal/providers/registry.go (2)

596-600: 🧹 Nitpick | 🔵 Trivial

Clarify that registryAccessor may wrap live maps.
The current comment still implies a snapshot only.

✏️ Suggested wording

-// registryAccessor implements modeldata.ModelInfoAccessor for a snapshot of models.
+// registryAccessor implements modeldata.ModelInfoAccessor over a models map;
+// callers must ensure appropriate synchronization when passing the live registry map.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry.go` around lines 596 - 600, The comment for
registryAccessor incorrectly implies it is a snapshot; update the documentation
on the registryAccessor type to clarify that it may wrap live maps (models and
providerTypes) and therefore can reflect concurrent changes rather than being an
immutable snapshot; reference the type name registryAccessor and its fields
models and providerTypes and the interface modeldata.ModelInfoAccessor so
readers know the wrapper may point at live map instances rather than a frozen
copy.

---

204-226: ⚠️ Potential issue | 🟡 Minor

Preserve raw model-list bytes even when parsing fails.
Dropping raw bytes prevents a later re-parse after schema fixes.

🛠️ Suggested fix

 r.mu.Lock()
 r.models = newModels
-if list != nil {
-	r.modelList = list
-	r.modelListRaw = modelCache.ModelListData
-}
+if len(modelCache.ModelListData) > 0 {
+	r.modelListRaw = modelCache.ModelListData
+}
+if list != nil {
+	r.modelList = list
+}
 r.mu.Unlock()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/providers/registry.go` around lines 204 - 226, The code currently
only assigns r.modelListRaw when parsing succeeds (list != nil), which discards
modelCache.ModelListData on parse failure; change the flow so that
r.modelListRaw is preserved whenever modelCache.ModelListData is non-empty:
either assign r.modelListRaw = modelCache.ModelListData before attempting Parse
or, inside the r.mu.Lock block, set r.modelListRaw = modelCache.ModelListData
whenever modelCache.ModelListData has content (regardless of parseErr), while
still only setting r.modelList when parsing succeeded (list != nil); update
references to the local variable list and keep the existing slog.Warn on
parseErr.

internal/admin/dashboard/templates/index.html (3)

325-326: ⚠️ Potential issue | 🟡 Minor

Add an empty state for category filters.
Currently, selecting a category with no models yields a blank area.

🧾 Suggested addition

 <p class="empty-state" x-show="models.length === 0 && !loading && !authError && !modelFilter">No models registered.</p>
 <p class="empty-state" x-show="models.length > 0 && filteredModels.length === 0 && modelFilter">No models match your filter.</p>
+<p class="empty-state" x-show="models.length > 0 && filteredModels.length === 0 && !modelFilter && activeCategory !== 'all'">No models in this category.</p>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 325 - 326, Add an
empty-state paragraph for category filtering so the UI shows a message when a
category is selected but yields no models: add a <p class="empty-state"> element
similar to the existing model-filter message that uses the same visibility logic
but checks the category filter variable (e.g., x-show="models.length > 0 &&
filteredModels.length === 0 && categoryFilter" or whatever the template uses
like selectedCategory), and set the text to something like "No models in this
category." Ensure you reference the existing symbols models and filteredModels
so it only displays when there are models overall but none match the category.

---

181-203: 🧹 Nitpick | 🔵 Trivial

Hide category-specific pricing columns when activeCategory === 'all'.
“All” includes non-text models, so the text pricing columns become misleading.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 181 - 203, The
price columns are currently shown for "all" which mixes non-text models; update
the table so Input $/MTok and Output $/MTok headers and their cells are only
rendered for text models by adding the same category check used for the cached
column. Specifically, add x-show="activeCategory === 'text_generation'" to the
<th> elements for "Input $/MTok" and "Output $/MTok" and to their corresponding
<td class="col-price" x-text="formatPrice(...)" /> rows (which reference
m.model.metadata?.pricing?.input_per_mtok and output_per_mtok) so only when
activeCategory === 'text_generation' the pricing columns (and their formatPrice
calls) are visible while filteredModels, activeCategory, and formatPrice remain
unchanged.

---

142-147: ⚠️ Potential issue | 🟡 Minor

Guard pagination range when offsets are stale.
The current math can show a start value greater than the total.

🧮 Suggested fix

-<span class="pagination-info" x-text="'Showing ' + (usageLog.offset + 1) + '-' + Math.min(usageLog.offset + usageLog.limit, usageLog.total) + ' of ' + usageLog.total"></span>
+<span class="pagination-info" x-text="'Showing ' + Math.min(usageLog.offset + 1, usageLog.total) + '-' + Math.min(usageLog.offset + usageLog.limit, usageLog.total) + ' of ' + usageLog.total"></span>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dashboard/templates/index.html` around lines 142 - 147, The
pagination display can show a start > total when offsets are stale; clamp the
displayed start and end values and ensure offset is clamped when paging. Replace
the start calculation in the span that uses usageLog.offset, usageLog.limit,
usageLog.total with a guarded expression (e.g., start = Math.min(usageLog.total,
Math.max(1, usageLog.offset + 1)) and end = Math.min(usageLog.total,
usageLog.offset + usageLog.limit)) and also update usageLogPrevPage() and
usageLogNextPage() to clamp usageLog.offset into the valid range (0..Math.max(0,
usageLog.total - usageLog.limit)) after changing pages so UI never shows an
out-of-range range.

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro