fix(admin): polish dashboard actions and clarify telemetry labels#215
Merged
SantiagoDePolonia merged 7 commits intomainfrom Apr 7, 2026
Merged
fix(admin): polish dashboard actions and clarify telemetry labels#215SantiagoDePolonia merged 7 commits intomainfrom
SantiagoDePolonia merged 7 commits intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds provider-name tracking and separates requested vs resolved model fields across routing, execution, logging, storage, and the admin dashboard; updates DB schemas/migrations (Mongo/Postgres/SQLite); augments provider resolution APIs; and implements dashboard asset URL versioning plus UI/CSS/JS/template changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router.ResolveModel
participant Registry as ModelRegistry
participant Provider as Provider (executor)
participant Exec as ExecutionPlan
participant Usage as UsageLogger
participant AuditDB as AuditStore/DB
Client->>Router: request with requested selector
Router->>Registry: lookup/resolve provider name
Registry-->>Router: resolved qualified selector + providerName
Router->>Provider: invoke with providerType and model
Provider-->>Exec: produce ExecutionPlan (includes ProviderName)
Exec->>Usage: log usage (includes ProviderName)
Exec->>AuditDB: write audit entry (RequestedModel, ResolvedModel, ProviderName)
AuditDB-->>Client: ack/write result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/auditlog/stream_wrapper.go (1)
83-103:⚠️ Potential issue | 🟡 MinorAdd
ProviderNameto the streaming entry copy.The
ProviderNamefield is enriched and populated in the entry beforeCreateStreamEntryis called (viaEnrichEntryWithResolvedRoutein the streaming handler). Since it's a first-class field inLogEntrythat captures the concrete provider instance name alongside the canonicalProvidertype, it should be copied to the streaming entry for audit consistency.Suggested change
entryCopy := &LogEntry{ ID: baseEntry.ID, Timestamp: baseEntry.Timestamp, DurationNs: baseEntry.DurationNs, RequestedModel: baseEntry.RequestedModel, ResolvedModel: baseEntry.ResolvedModel, Provider: baseEntry.Provider, ProviderName: baseEntry.ProviderName, // Add this line AliasUsed: baseEntry.AliasUsed, ExecutionPlanVersionID: baseEntry.ExecutionPlanVersionID, CacheType: baseEntry.CacheType, StatusCode: baseEntry.StatusCode, RequestID: baseEntry.RequestID, AuthKeyID: baseEntry.AuthKeyID, AuthMethod: baseEntry.AuthMethod, ClientIP: baseEntry.ClientIP, Method: baseEntry.Method, Path: baseEntry.Path, UserPath: baseEntry.UserPath, Stream: true, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auditlog/stream_wrapper.go` around lines 83 - 103, The streaming copy of the audit log (entryCopy created as a *LogEntry in CreateStreamEntry) is missing the ProviderName field; update the struct literal that builds entryCopy to include ProviderName: baseEntry.ProviderName so the enriched ProviderName set earlier (via EnrichEntryWithResolvedRoute in the streaming handler) is preserved in streaming entries for audit consistency.internal/usage/reader_sqlite.go (1)
89-105: 🧹 Nitpick | 🔵 TrivialNormalize provider-name predicates here as well.
GetUsageByModelalready groups on the normalized provider-name expression, butGetUsageLogstill filters/searches the rawprovider_namecolumn. Using the same expression in both places would keep SQLite behavior aligned with the normalizedProviderNamereturned to the UI.♻️ Suggested refactor
func (r *SQLiteReader) GetUsageLog(ctx context.Context, params UsageLogParams) (*UsageLogResult, error) { limit, offset := clampLimitOffset(params.Limit, params.Offset) conditions, args, err := sqliteUsageConditions(params.UsageQueryParams) if err != nil { return nil, err } + normalizedProviderNameExpr := usageGroupedProviderNameSQL("provider_name", "provider") if params.Model != "" { conditions = append(conditions, "model = ?") args = append(args, params.Model) } if params.Provider != "" { - conditions = append(conditions, "(provider = ? OR provider_name = ?)") + conditions = append(conditions, "(provider = ? OR "+normalizedProviderNameExpr+" = ?)") args = append(args, params.Provider, params.Provider) } if params.Search != "" { - conditions = append(conditions, "(model LIKE ? ESCAPE '\\' OR provider LIKE ? ESCAPE '\\' OR provider_name LIKE ? ESCAPE '\\' OR request_id LIKE ? ESCAPE '\\' OR provider_id LIKE ? ESCAPE '\\')") + conditions = append(conditions, "(model LIKE ? ESCAPE '\\' OR provider LIKE ? ESCAPE '\\' OR "+normalizedProviderNameExpr+" LIKE ? ESCAPE '\\' OR request_id LIKE ? ESCAPE '\\' OR provider_id LIKE ? ESCAPE '\\')") s := "%" + escapeLikeWildcards(params.Search) + "%" args = append(args, s, s, s, s, s) }🤖 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 89 - 105, The provider_name predicates in sqliteUsageConditions/getter that build conditions for GetUsageLog must use the same normalized expression used in GetUsageByModel instead of the raw provider_name column; update the equality predicate "(provider = ? OR provider_name = ?)" and the LIKE predicate that includes "provider_name LIKE ?" to use the normalized provider-name expression (the same SQL expression/string used when grouping in GetUsageByModel), and ensure the corresponding args still pass the unmodified params.Provider or the escaped search string as before so filtering/search behavior remains consistent with the normalized ProviderName returned to the UI.internal/usage/reader_postgresql.go (1)
89-106: 🧹 Nitpick | 🔵 TrivialNormalize provider-name predicates the same way you normalize grouping/display.
GetUsageByModelalready usesusageGroupedProviderNameSQL(...), butGetUsageLogstill filters/searches the rawprovider_namecolumn. Reusing the normalized expression here keeps filtering aligned with theProviderNamethe dashboard receives.♻️ Suggested refactor
func (r *PostgreSQLReader) GetUsageLog(ctx context.Context, params UsageLogParams) (*UsageLogResult, error) { limit, offset := clampLimitOffset(params.Limit, params.Offset) conditions, args, argIdx, err := pgUsageConditions(params.UsageQueryParams, 1) if err != nil { return nil, err } + normalizedProviderNameExpr := usageGroupedProviderNameSQL("provider_name", "provider") if params.Model != "" { conditions = append(conditions, fmt.Sprintf("model = $%d", argIdx)) args = append(args, params.Model) argIdx++ } if params.Provider != "" { - conditions = append(conditions, fmt.Sprintf("(provider = $%d OR provider_name = $%d)", argIdx, argIdx+1)) - args = append(args, params.Provider, params.Provider) - argIdx += 2 + conditions = append(conditions, fmt.Sprintf("(provider = $%d OR %s = $%d)", argIdx, normalizedProviderNameExpr, argIdx)) + args = append(args, params.Provider) + argIdx++ } if params.Search != "" { s := "%" + escapeLikeWildcards(params.Search) + "%" - conditions = append(conditions, fmt.Sprintf("(model ILIKE $%d ESCAPE '\\' OR provider ILIKE $%d ESCAPE '\\' OR provider_name ILIKE $%d ESCAPE '\\' OR request_id ILIKE $%d ESCAPE '\\' OR provider_id ILIKE $%d ESCAPE '\\')", argIdx, argIdx, argIdx, argIdx, argIdx)) + conditions = append(conditions, fmt.Sprintf("(model ILIKE $%d ESCAPE '\\' OR provider ILIKE $%d ESCAPE '\\' OR %s ILIKE $%d ESCAPE '\\' OR request_id ILIKE $%d ESCAPE '\\' OR provider_id ILIKE $%d ESCAPE '\\')", argIdx, argIdx, normalizedProviderNameExpr, argIdx, argIdx, argIdx)) args = append(args, s) argIdx++ }🤖 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 89 - 106, GetUsageLog currently filters/searches the raw provider_name column while GetUsageByModel and the dashboard use the normalized provider name; update the search/filter construction in pgUsageConditions/GetUsageLog to use the same normalized SQL expression by replacing occurrences of provider_name with the usageGroupedProviderNameSQL(...) expression (the same helper used by GetUsageByModel), ensure the ILIKE and ESCAPE '\\' comparisons wrap that expression, and adjust argument placeholders/argIdx and usage of escapeLikeWildcards(params.Search) so the normalized provider-name is searched consistently with the dashboard/grouping logic.internal/server/request_model_resolution.go (1)
69-74:⚠️ Potential issue | 🟡 MinorPreserve
ResolvedSelector.ProviderwhenGetProviderNamecannot resolve it.
resolvedSelector.Provideris already concrete at this point. Passing""as the fallback means any resolver/provider pair withoutcore.ProviderNameResolverwill emit a blankProviderName, and the new audit/usage field loses the value you just resolved.♻️ Proposed fix
- ProviderName: resolvedProviderName(provider, resolvedSelector, ""), + ProviderName: resolvedProviderName(provider, resolvedSelector, resolvedSelector.Provider),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/request_model_resolution.go` around lines 69 - 74, When building the core.RequestModelResolution, don't pass an empty fallback into resolvedProviderName; instead use the already-resolved selector provider value so a provider without core.ProviderNameResolver still yields a concrete ProviderName. Locate the creation of core.RequestModelResolution (the struct literal returning Requested, ResolvedSelector, ProviderType, ProviderName, AliasApplied) and change the resolvedProviderName call (resolvedProviderName(provider, resolvedSelector, "")) to use resolvedSelector.Provider (or another non-empty fallback derived from resolvedSelector) so ResolvedSelector.Provider is preserved in ProviderName when GetProviderName cannot resolve it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/admin/dashboard/dashboard.go`:
- Around line 71-86: Add a short comment documenting why the SHA-256 sum is
truncated to the first 6 bytes in buildAssetVersions: explain that sum[:6]
yields a 12-hex-character (≈48-bit) fingerprint chosen for cache-busting
(collision risk acceptable for this use) and that it balances uniqueness vs. URL
length; place the comment immediately above the versions[normalizedPath]
assignment or at the top of the buildAssetVersions function so future
maintainers see why sum[:6] is used.
In `@internal/admin/dashboard/static/css/dashboard.css`:
- Around line 2703-2706: The mobile override only changes width so the icon
buttons keep the base height/min-width of 32px; update the rule for
.alias-actions-cell .table-icon-btn and .aliases-editor-header .table-icon-btn
to also set height: 36px and min-width: 36px (and optionally adjust
line-height/alignment if needed) so the icon-only controls actually grow to the
intended touch-friendly size.
In `@internal/admin/dashboard/templates/index.html`:
- Line 630: The models filter input (the element with x-model="modelFilter" and
classes "filter-input models-filter-input") lacks an accessible name; add one by
either adding an explicit label element tied to the input (give the input a
unique id and a <label for="...">like "Filter models by provider, model, alias,
or owner"</label>) or by adding an aria-label/aria-labelledby attribute (e.g.,
aria-label="Filter models by provider, model, alias, or owner") so screen
readers retain the control's purpose after a value is entered.
In `@internal/auditlog/store_postgresql.go`:
- Around line 259-270: The ALTER TABLE statement in renamePostgreSQLAuditColumn
builds SQL with fmt.Sprintf using table/column names which can allow SQL
injection; update the function to quote/escape identifiers using pgx.Identifier
(or pgxpool's safe identifier quoting) when constructing the ALTER TABLE ...
RENAME COLUMN ... statement and any other dynamic identifier uses (ensure
postgresqlColumnExists calls remain compatible), e.g., build the identifier list
via pgx.Identifier{tableName}.Sanitize()/Join or pgx.Identifier(...).Sanitize
equivalent so the table and column names are safely quoted instead of
interpolated directly.
In `@internal/providers/router_test.go`:
- Around line 41-68: The test helper newTestRegistryWithModels directly mutates
the internal maps models and modelsByProvider; instead populate registry via its
public API: call RegisterProviderWithNameAndType for each provider and then add
models through a public registry method (e.g., add or expose a
RegisterModel/RegisterModelInfo on ModelRegistry) rather than assigning
registry.models or registry.modelsByProvider directly; update the helper to use
that public method (or add one like RegisterModel(info *ModelInfo) on
ModelRegistry) so tests exercise the same registration logic and avoid coupling
to internal fields such as models, modelsByProvider, and the ModelInfo layout.
In `@internal/server/handlers_test.go`:
- Around line 405-422: The GetProviderName helper currently strips a parsed
selector's qualifier and falls back to the bare model, allowing inputs like
"wrong-provider/gpt-4o-mini" to match; change it so that when
core.ParseModelSelector(model, "") succeeds and selector.Provider != "" you must
first lookup m.providerNames[selector.QualifiedModel()] and if that lookup fails
return "" immediately (do not set model = selector.Model and do not fall back);
only allow the bare-model fallback path when parsing fails or when the selector
does not contain a Provider (i.e., selector.Provider == "") so that legitimate
slashes that are part of the model ID can still be handled.
In `@internal/server/translated_inference_service.go`:
- Around line 682-691: qualifyModelWithProvider currently avoids prefixing if
the model contains any "/" which incorrectly treats slashed model IDs as already
qualified; change the logic in qualifyModelWithProvider so that after trimming
it only skips adding providerName when providerName is empty OR model already
has the provider prefix (i.e., strings.HasPrefix(model, providerName+"/"));
otherwise return providerName + "/" + model; keep the existing empty-model guard
and trimming steps and update the conditional that checks for "/" to the
HasPrefix check to ensure only true provider-qualified IDs are left unmodified.
- Around line 770-772: The fallback telemetry and logging currently use
selector.Provider (or the un-resolved provider) which can be a
provider-name-qualified selector; instead compute the canonical provider type
once via providerTypeForSelector(selector, providerTypeFromPlan(plan)) (e.g.,
store it in a variable like resolvedProviderType) and use that
resolvedProviderType everywhere you populate the canonical provider field,
telemetry, and the slog.Warn message; also ensure you use
resolvedProviderName(s.provider, selector, providerNameFromPlan(plan)) for the
provider name where applicable and apply the same change in both fallback loops
(the occurrences around the first fallback and the second fallback).
In `@internal/usage/store_sqlite.go`:
- Around line 16-20: Update the stale inline comment for maxEntriesPerBatch: the
current calculation maxSQLiteParams / columnsPerUsageEntry (999 / 18) yields 55,
not 58, so change the comment to reflect "55 entries" (referencing the constants
maxSQLiteParams, columnsPerUsageEntry, and maxEntriesPerBatch) so the comment
matches the actual schema column count.
---
Outside diff comments:
In `@internal/auditlog/stream_wrapper.go`:
- Around line 83-103: The streaming copy of the audit log (entryCopy created as
a *LogEntry in CreateStreamEntry) is missing the ProviderName field; update the
struct literal that builds entryCopy to include ProviderName:
baseEntry.ProviderName so the enriched ProviderName set earlier (via
EnrichEntryWithResolvedRoute in the streaming handler) is preserved in streaming
entries for audit consistency.
In `@internal/server/request_model_resolution.go`:
- Around line 69-74: When building the core.RequestModelResolution, don't pass
an empty fallback into resolvedProviderName; instead use the already-resolved
selector provider value so a provider without core.ProviderNameResolver still
yields a concrete ProviderName. Locate the creation of
core.RequestModelResolution (the struct literal returning Requested,
ResolvedSelector, ProviderType, ProviderName, AliasApplied) and change the
resolvedProviderName call (resolvedProviderName(provider, resolvedSelector, ""))
to use resolvedSelector.Provider (or another non-empty fallback derived from
resolvedSelector) so ResolvedSelector.Provider is preserved in ProviderName when
GetProviderName cannot resolve it.
In `@internal/usage/reader_postgresql.go`:
- Around line 89-106: GetUsageLog currently filters/searches the raw
provider_name column while GetUsageByModel and the dashboard use the normalized
provider name; update the search/filter construction in
pgUsageConditions/GetUsageLog to use the same normalized SQL expression by
replacing occurrences of provider_name with the usageGroupedProviderNameSQL(...)
expression (the same helper used by GetUsageByModel), ensure the ILIKE and
ESCAPE '\\' comparisons wrap that expression, and adjust argument
placeholders/argIdx and usage of escapeLikeWildcards(params.Search) so the
normalized provider-name is searched consistently with the dashboard/grouping
logic.
In `@internal/usage/reader_sqlite.go`:
- Around line 89-105: The provider_name predicates in
sqliteUsageConditions/getter that build conditions for GetUsageLog must use the
same normalized expression used in GetUsageByModel instead of the raw
provider_name column; update the equality predicate "(provider = ? OR
provider_name = ?)" and the LIKE predicate that includes "provider_name LIKE ?"
to use the normalized provider-name expression (the same SQL expression/string
used when grouping in GetUsageByModel), and ensure the corresponding args still
pass the unmodified params.Provider or the escaped search string as before so
filtering/search behavior remains consistent with the normalized ProviderName
returned to the UI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 48d52183-4e6d-4a73-9259-0a15c3722e6d
📒 Files selected for processing (73)
internal/admin/dashboard/dashboard.gointernal/admin/dashboard/dashboard_test.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/audit-list.jsinternal/admin/dashboard/static/js/modules/audit-list.test.jsinternal/admin/dashboard/static/js/modules/charts.jsinternal/admin/dashboard/static/js/modules/charts.test.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.jsinternal/admin/dashboard/static/js/modules/execution-plans-layout.test.jsinternal/admin/dashboard/static/js/modules/execution-plans.jsinternal/admin/dashboard/static/js/modules/execution-plans.test.jsinternal/admin/dashboard/static/js/modules/usage.jsinternal/admin/dashboard/templates/edit-icon.htmlinternal/admin/dashboard/templates/execution-plan-chart.htmlinternal/admin/dashboard/templates/index.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/x-icon.htmlinternal/admin/handler.gointernal/admin/handler_test.gointernal/aliases/provider.gointernal/auditlog/auditlog.gointernal/auditlog/auditlog_test.gointernal/auditlog/logger.gointernal/auditlog/middleware.gointernal/auditlog/middleware_test.gointernal/auditlog/reader.gointernal/auditlog/reader_mongodb.gointernal/auditlog/reader_mongodb_test.gointernal/auditlog/reader_postgresql.gointernal/auditlog/reader_sqlite.gointernal/auditlog/reader_sqlite_boundary_test.gointernal/auditlog/store_mongodb.gointernal/auditlog/store_postgresql.gointernal/auditlog/store_postgresql_test.gointernal/auditlog/store_sqlite.gointernal/auditlog/store_sqlite_test.gointernal/auditlog/stream_wrapper.gointernal/core/interfaces.gointernal/core/request_model_resolution.gointernal/providers/config.gointernal/providers/config_test.gointernal/providers/registry.gointernal/providers/router.gointernal/providers/router_test.gointernal/responsecache/usage_hit.gointernal/server/execution_plan_helpers_test.gointernal/server/handlers_test.gointernal/server/internal_chat_completion_executor.gointernal/server/internal_chat_completion_executor_test.gointernal/server/model_validation_test.gointernal/server/native_batch_support.gointernal/server/passthrough_service.gointernal/server/passthrough_support.gointernal/server/request_model_resolution.gointernal/server/request_model_resolution_test.gointernal/server/translated_inference_service.gointernal/server/translated_inference_service_test.gointernal/usage/cache_type.gointernal/usage/reader.gointernal/usage/reader_helpers.gointernal/usage/reader_mongodb.gointernal/usage/reader_mongodb_grouping_test.gointernal/usage/reader_mongodb_test.gointernal/usage/reader_postgresql.gointernal/usage/reader_sqlite.gointernal/usage/reader_sqlite_boundary_test.gointernal/usage/store_mongodb.gointernal/usage/store_postgresql.gointernal/usage/store_postgresql_test.gointernal/usage/store_sqlite.gointernal/usage/stream_observer.gointernal/usage/usage.go
💤 Files with no reviewable changes (1)
- internal/admin/dashboard/static/js/modules/audit-list.js
Comment on lines
+2703
to
+2706
| .alias-actions-cell .table-icon-btn, | ||
| .aliases-editor-header .table-icon-btn { | ||
| width: 36px; | ||
| } |
Contributor
There was a problem hiding this comment.
The mobile override never increases the icon-button height.
This rule only changes width, so .table-icon-btn keeps the base height: 32px and min-width: 32px. The new icon-only controls stay undersized on touch devices even though this block is clearly trying to bump them to 36px.
♻️ Proposed fix
.alias-actions-cell .table-icon-btn,
.aliases-editor-header .table-icon-btn {
width: 36px;
+ min-width: 36px;
+ height: 36px;
}📝 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
| .alias-actions-cell .table-icon-btn, | |
| .aliases-editor-header .table-icon-btn { | |
| width: 36px; | |
| } | |
| .alias-actions-cell .table-icon-btn, | |
| .aliases-editor-header .table-icon-btn { | |
| width: 36px; | |
| min-width: 36px; | |
| height: 36px; | |
| } |
🤖 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 2703 - 2706,
The mobile override only changes width so the icon buttons keep the base
height/min-width of 32px; update the rule for .alias-actions-cell
.table-icon-btn and .aliases-editor-header .table-icon-btn to also set height:
36px and min-width: 36px (and optionally adjust line-height/alignment if needed)
so the icon-only controls actually grow to the intended touch-friendly size.
Comment on lines
+259
to
+270
| func renamePostgreSQLAuditColumn(ctx context.Context, pool *pgxpool.Pool, tableName, from, to string) error { | ||
| fromExists, err := postgresqlColumnExists(ctx, pool, tableName, from) | ||
| if err != nil || !fromExists { | ||
| return err | ||
| } | ||
| toExists, err := postgresqlColumnExists(ctx, pool, tableName, to) | ||
| if err != nil || toExists { | ||
| return err | ||
| } | ||
| _, err = pool.Exec(ctx, fmt.Sprintf("ALTER TABLE %s RENAME COLUMN %s TO %s", tableName, from, to)) | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider parameterizing table/column names to prevent SQL injection.
While the current callers use hardcoded strings, renamePostgreSQLAuditColumn constructs SQL using fmt.Sprintf with table and column names. If these parameters ever come from untrusted sources, this could be vulnerable. Consider using pgx.Identifier for safe quoting.
🛡️ Suggested safer approach
func renamePostgreSQLAuditColumn(ctx context.Context, pool *pgxpool.Pool, tableName, from, to string) error {
fromExists, err := postgresqlColumnExists(ctx, pool, tableName, from)
if err != nil || !fromExists {
return err
}
toExists, err := postgresqlColumnExists(ctx, pool, tableName, to)
if err != nil || toExists {
return err
}
- _, err = pool.Exec(ctx, fmt.Sprintf("ALTER TABLE %s RENAME COLUMN %s TO %s", tableName, from, to))
+ // Note: pgx.Identifier provides safe quoting for identifiers
+ query := fmt.Sprintf("ALTER TABLE %s RENAME COLUMN %s TO %s",
+ pgx.Identifier{tableName}.Sanitize(),
+ pgx.Identifier{from}.Sanitize(),
+ pgx.Identifier{to}.Sanitize())
+ _, err = pool.Exec(ctx, query)
return err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auditlog/store_postgresql.go` around lines 259 - 270, The ALTER
TABLE statement in renamePostgreSQLAuditColumn builds SQL with fmt.Sprintf using
table/column names which can allow SQL injection; update the function to
quote/escape identifiers using pgx.Identifier (or pgxpool's safe identifier
quoting) when constructing the ALTER TABLE ... RENAME COLUMN ... statement and
any other dynamic identifier uses (ensure postgresqlColumnExists calls remain
compatible), e.g., build the identifier list via
pgx.Identifier{tableName}.Sanitize()/Join or pgx.Identifier(...).Sanitize
equivalent so the table and column names are safely quoted instead of
interpolated directly.
Comment on lines
+41
to
+68
| func newTestRegistryWithModels(entries ...registryModelEntry) *ModelRegistry { | ||
| registry := NewModelRegistry() | ||
| for _, entry := range entries { | ||
| registry.RegisterProviderWithNameAndType(entry.provider, entry.providerName, entry.providerType) | ||
| } | ||
|
|
||
| registry.models = make(map[string]*ModelInfo) | ||
| registry.modelsByProvider = make(map[string]map[string]*ModelInfo) | ||
| for _, entry := range entries { | ||
| info := &ModelInfo{ | ||
| Model: core.Model{ | ||
| ID: entry.modelID, | ||
| Object: "model", | ||
| }, | ||
| Provider: entry.provider, | ||
| ProviderName: entry.providerName, | ||
| ProviderType: entry.providerType, | ||
| } | ||
| if _, ok := registry.modelsByProvider[entry.providerName]; !ok { | ||
| registry.modelsByProvider[entry.providerName] = make(map[string]*ModelInfo) | ||
| } | ||
| registry.modelsByProvider[entry.providerName][entry.modelID] = info | ||
| if _, exists := registry.models[entry.modelID]; !exists { | ||
| registry.models[entry.modelID] = info | ||
| } | ||
| } | ||
| return registry | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test utility directly manipulates registry internals.
The newTestRegistryWithModels helper bypasses normal registration flow by directly assigning to internal models and modelsByProvider maps. While this enables fast, isolated testing, it creates coupling to internal structure. Consider whether RegisterProviderWithNameAndType alone could populate the needed state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/providers/router_test.go` around lines 41 - 68, The test helper
newTestRegistryWithModels directly mutates the internal maps models and
modelsByProvider; instead populate registry via its public API: call
RegisterProviderWithNameAndType for each provider and then add models through a
public registry method (e.g., add or expose a RegisterModel/RegisterModelInfo on
ModelRegistry) rather than assigning registry.models or
registry.modelsByProvider directly; update the helper to use that public method
(or add one like RegisterModel(info *ModelInfo) on ModelRegistry) so tests
exercise the same registration logic and avoid coupling to internal fields such
as models, modelsByProvider, and the ModelInfo layout.
Comment on lines
+405
to
+422
| func (m *mockProvider) GetProviderName(model string) string { | ||
| selector, err := core.ParseModelSelector(model, "") | ||
| if err == nil && selector.Provider != "" { | ||
| if m.providerNames != nil { | ||
| if providerName, ok := m.providerNames[selector.QualifiedModel()]; ok { | ||
| return providerName | ||
| } | ||
| } | ||
| model = selector.Model | ||
| } | ||
|
|
||
| if m.providerNames != nil { | ||
| if providerName, ok := m.providerNames[model]; ok { | ||
| return providerName | ||
| } | ||
| } | ||
| return "" | ||
| } |
Contributor
There was a problem hiding this comment.
Don't let qualified-selector misses fall back to the bare model.
After ParseModelSelector succeeds, this helper strips the qualifier and retries against the unqualified model. That makes inputs like wrong-provider/gpt-4o-mini resolve successfully, so routing/provider-name regressions can still pass these tests. Only keep the bare-model fallback for cases where the slash can genuinely be part of the model ID; otherwise return "" on qualifier mismatches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/handlers_test.go` around lines 405 - 422, The GetProviderName
helper currently strips a parsed selector's qualifier and falls back to the bare
model, allowing inputs like "wrong-provider/gpt-4o-mini" to match; change it so
that when core.ParseModelSelector(model, "") succeeds and selector.Provider !=
"" you must first lookup m.providerNames[selector.QualifiedModel()] and if that
lookup fails return "" immediately (do not set model = selector.Model and do not
fall back); only allow the bare-model fallback path when parsing fails or when
the selector does not contain a Provider (i.e., selector.Provider == "") so that
legitimate slashes that are part of the model ID can still be handled.
Comment on lines
770
to
772
| providerType := s.providerTypeForSelector(selector, providerTypeFromPlan(plan)) | ||
| providerName := resolvedProviderName(s.provider, selector, providerNameFromPlan(plan)) | ||
| slog.Warn("primary model attempt failed, trying fallback", |
Contributor
There was a problem hiding this comment.
Fallback telemetry can write provider_name into the canonical provider field.
Once a fallback selector is already provider-name-qualified, providerTypeForSelector() returns that configured name. The non-streaming path is usually corrected by resp.Provider, but the streaming path has no response object to overwrite it, so audit/usage/pricing can record openai_primary where this PR expects openai.
♻️ Representative fix
Apply the same lookup in both fallback loops so the canonical type comes from the resolved selector, not selector.Provider.
- providerType := s.providerTypeForSelector(selector, providerTypeFromPlan(plan))
+ providerType := providerTypeFromPlan(plan)
+ if s.provider != nil {
+ if resolved := strings.TrimSpace(s.provider.GetProviderType(selector.QualifiedModel())); resolved != "" {
+ providerType = resolved
+ }
+ }Also applies to: 858-860
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/translated_inference_service.go` around lines 770 - 772, The
fallback telemetry and logging currently use selector.Provider (or the
un-resolved provider) which can be a provider-name-qualified selector; instead
compute the canonical provider type once via providerTypeForSelector(selector,
providerTypeFromPlan(plan)) (e.g., store it in a variable like
resolvedProviderType) and use that resolvedProviderType everywhere you populate
the canonical provider field, telemetry, and the slog.Warn message; also ensure
you use resolvedProviderName(s.provider, selector, providerNameFromPlan(plan))
for the provider name where applicable and apply the same change in both
fallback loops (the occurrences around the first fallback and the second
fallback).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 31-32: The "Run linter" step that runs "make lint" fails because
golangci-lint is not installed on the runner; add a prior step to install or set
up golangci-lint (e.g., use the golangci-lint GitHub Action or download/install
via curl) before the step named "Run linter" so that the "make lint" command can
succeed; ensure the new step runs before the "Run linter" step and is invoked on
the same job so the "make lint" target can find the golangci-lint binary.
In `@internal/server/request_model_resolution.go`:
- Around line 68-104: The code re-calls core.NewRequestedModelSelector inside
resolveExecutionSelector after resolver.ResolveModel, which is redundant because
the caller resolveRequestModel already normalized requested; remove the extra
core.NewRequestedModelSelector(...) assignment in the if resolver != nil block
and instead update the requested RequestedModelSelector to reflect
resolvedSelector.QualifiedModel() without re-invoking NewRequestedModelSelector
(e.g., assign the field(s) or construct a RequestedModelSelector directly),
leaving resolver.ResolveModel, provider.(RequestModelResolver) handling, and the
final Normalize() logic unchanged.
In `@Makefile`:
- Line 93: The lint-fix target currently runs "golangci-lint run --fix ./cmd/...
./config/... ./internal/..." and misses the same file sets the lint target
checks (so fixes don't cover tagged test trees); update the lint-fix command to
use the exact same path globs as the lint target (replace the current path list
in the "golangci-lint run --fix ..." invocation with the same arguments used by
the lint target) so both targets operate on an identical scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a81ad0de-aad4-4114-95bd-ada0fdc741be
📒 Files selected for processing (9)
.github/workflows/test.yml.pre-commit-config.yamlMakefileinternal/server/request_model_resolution.gointernal/server/translated_inference_service.gointernal/server/translated_inference_service_test.gointernal/usage/store_sqlite.gotests/e2e/auditlog_test.gotests/integration/dbassert/auditlog.go
Comment on lines
+68
to
+104
| func resolveExecutionSelector( | ||
| provider core.RoutableProvider, | ||
| resolver RequestModelResolver, | ||
| requested core.RequestedModelSelector, | ||
| ) (core.ModelSelector, bool, error) { | ||
| requested = core.NewRequestedModelSelector(requested.Model, requested.ProviderHint) | ||
|
|
||
| var ( | ||
| resolvedSelector core.ModelSelector | ||
| aliasApplied bool | ||
| err error | ||
| ) | ||
|
|
||
| if resolver != nil { | ||
| resolvedSelector, aliasApplied, err = resolver.ResolveModel(requested) | ||
| if err != nil { | ||
| return core.ModelSelector{}, false, err | ||
| } | ||
| requested = core.NewRequestedModelSelector(resolvedSelector.QualifiedModel(), "") | ||
| } | ||
|
|
||
| if providerResolver, ok := provider.(RequestModelResolver); ok { | ||
| var providerChanged bool | ||
| resolvedSelector, providerChanged, err = providerResolver.ResolveModel(requested) | ||
| if err != nil { | ||
| return core.ModelSelector{}, false, err | ||
| } | ||
| return resolvedSelector, aliasApplied || providerChanged, nil | ||
| } | ||
|
|
||
| if resolvedSelector != (core.ModelSelector{}) { | ||
| return resolvedSelector, aliasApplied, nil | ||
| } | ||
|
|
||
| resolvedSelector, err = requested.Normalize() | ||
| return resolvedSelector, aliasApplied, err | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the redundant NewRequestedModelSelector call.
Line 73 re-normalizes requested with core.NewRequestedModelSelector, but the caller resolveRequestModel already normalizes it at line 38. This is harmless since NewRequestedModelSelector is idempotent, but consider removing it for clarity.
♻️ Optional cleanup
func resolveExecutionSelector(
provider core.RoutableProvider,
resolver RequestModelResolver,
requested core.RequestedModelSelector,
) (core.ModelSelector, bool, error) {
- requested = core.NewRequestedModelSelector(requested.Model, requested.ProviderHint)
-
var (📝 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 resolveExecutionSelector( | |
| provider core.RoutableProvider, | |
| resolver RequestModelResolver, | |
| requested core.RequestedModelSelector, | |
| ) (core.ModelSelector, bool, error) { | |
| requested = core.NewRequestedModelSelector(requested.Model, requested.ProviderHint) | |
| var ( | |
| resolvedSelector core.ModelSelector | |
| aliasApplied bool | |
| err error | |
| ) | |
| if resolver != nil { | |
| resolvedSelector, aliasApplied, err = resolver.ResolveModel(requested) | |
| if err != nil { | |
| return core.ModelSelector{}, false, err | |
| } | |
| requested = core.NewRequestedModelSelector(resolvedSelector.QualifiedModel(), "") | |
| } | |
| if providerResolver, ok := provider.(RequestModelResolver); ok { | |
| var providerChanged bool | |
| resolvedSelector, providerChanged, err = providerResolver.ResolveModel(requested) | |
| if err != nil { | |
| return core.ModelSelector{}, false, err | |
| } | |
| return resolvedSelector, aliasApplied || providerChanged, nil | |
| } | |
| if resolvedSelector != (core.ModelSelector{}) { | |
| return resolvedSelector, aliasApplied, nil | |
| } | |
| resolvedSelector, err = requested.Normalize() | |
| return resolvedSelector, aliasApplied, err | |
| } | |
| func resolveExecutionSelector( | |
| provider core.RoutableProvider, | |
| resolver RequestModelResolver, | |
| requested core.RequestedModelSelector, | |
| ) (core.ModelSelector, bool, error) { | |
| var ( | |
| resolvedSelector core.ModelSelector | |
| aliasApplied bool | |
| err error | |
| ) | |
| if resolver != nil { | |
| resolvedSelector, aliasApplied, err = resolver.ResolveModel(requested) | |
| if err != nil { | |
| return core.ModelSelector{}, false, err | |
| } | |
| requested = core.NewRequestedModelSelector(resolvedSelector.QualifiedModel(), "") | |
| } | |
| if providerResolver, ok := provider.(RequestModelResolver); ok { | |
| var providerChanged bool | |
| resolvedSelector, providerChanged, err = providerResolver.ResolveModel(requested) | |
| if err != nil { | |
| return core.ModelSelector{}, false, err | |
| } | |
| return resolvedSelector, aliasApplied || providerChanged, nil | |
| } | |
| if resolvedSelector != (core.ModelSelector{}) { | |
| return resolvedSelector, aliasApplied, nil | |
| } | |
| resolvedSelector, err = requested.Normalize() | |
| return resolvedSelector, aliasApplied, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/server/request_model_resolution.go` around lines 68 - 104, The code
re-calls core.NewRequestedModelSelector inside resolveExecutionSelector after
resolver.ResolveModel, which is redundant because the caller resolveRequestModel
already normalized requested; remove the extra
core.NewRequestedModelSelector(...) assignment in the if resolver != nil block
and instead update the requested RequestedModelSelector to reflect
resolvedSelector.QualifiedModel() without re-invoking NewRequestedModelSelector
(e.g., assign the field(s) or construct a RequestedModelSelector directly),
leaving resolver.ResolveModel, provider.(RequestModelResolver) handling, and the
final Normalize() logic unchanged.
| # Run linter with auto-fix | ||
| lint-fix: | ||
| golangci-lint run --fix ./... | ||
| golangci-lint run --fix ./cmd/... ./config/... ./internal/... |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Keep lint-fix scope aligned with lint.
Line 93 only auto-fixes core paths, so make lint-fix && make lint can still fail on tagged test trees.
Proposed patch
lint-fix:
golangci-lint run --fix ./cmd/... ./config/... ./internal/...
+ golangci-lint run --fix --build-tags=e2e ./tests/e2e/...
+ golangci-lint run --fix --build-tags=integration ./tests/integration/...
+ golangci-lint run --fix --build-tags=contract ./tests/contract/...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` at line 93, The lint-fix target currently runs "golangci-lint run
--fix ./cmd/... ./config/... ./internal/..." and misses the same file sets the
lint target checks (so fixes don't cover tagged test trees); update the lint-fix
command to use the exact same path globs as the lint target (replace the current
path list in the "golangci-lint run --fix ..." invocation with the same
arguments used by the lint target) so both targets operate on an identical
scope.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/manage-release-e2e-stack.sh`:
- Around line 125-142: The loop in wait_for_health assigns attempt but never
uses it (triggering SC2034); change the for-loop to use a conventional unused
variable name (e.g., replace "for attempt in $(seq 1 30);" with "for _ in $(seq
1 30);") so the loop intent is clear and shellcheck stops reporting an unused
variable in the wait_for_health function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7aaaa719-6df6-42c2-97bd-e87519ddf63f
📒 Files selected for processing (3)
tests/e2e/manage-release-e2e-stack.shtests/e2e/release-e2e-scenarios.mdtests/e2e/run-release-e2e.sh
Comment on lines
+125
to
+142
| wait_for_health() { | ||
| local gateway="$1" | ||
| local port="$2" | ||
| local log_file | ||
| local attempt | ||
|
|
||
| log_file="$(gateway_log_file "$gateway")" | ||
| for attempt in $(seq 1 30); do | ||
| if curl -fsS "http://localhost:$port/health" >/dev/null 2>&1; then | ||
| return 0 | ||
| fi | ||
| sleep 1 | ||
| done | ||
|
|
||
| echo "failed to start $gateway on port $port" >&2 | ||
| [[ -f "$log_file" ]] && tail -n 40 "$log_file" >&2 | ||
| exit 1 | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused loop variable attempt.
The variable is assigned but never read, which triggers shellcheck SC2034. Consider using _ as the conventional "unused" variable name.
🔧 Suggested fix
- for attempt in $(seq 1 30); do
+ for _ in $(seq 1 30); do📝 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
| wait_for_health() { | |
| local gateway="$1" | |
| local port="$2" | |
| local log_file | |
| local attempt | |
| log_file="$(gateway_log_file "$gateway")" | |
| for attempt in $(seq 1 30); do | |
| if curl -fsS "http://localhost:$port/health" >/dev/null 2>&1; then | |
| return 0 | |
| fi | |
| sleep 1 | |
| done | |
| echo "failed to start $gateway on port $port" >&2 | |
| [[ -f "$log_file" ]] && tail -n 40 "$log_file" >&2 | |
| exit 1 | |
| } | |
| wait_for_health() { | |
| local gateway="$1" | |
| local port="$2" | |
| local log_file | |
| local attempt | |
| log_file="$(gateway_log_file "$gateway")" | |
| for _ in $(seq 1 30); do | |
| if curl -fsS "http://localhost:$port/health" >/dev/null 2>&1; then | |
| return 0 | |
| fi | |
| sleep 1 | |
| done | |
| echo "failed to start $gateway on port $port" >&2 | |
| [[ -f "$log_file" ]] && tail -n 40 "$log_file" >&2 | |
| exit 1 | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 132-132: attempt appears unused. Verify use (or export if used externally).
(SC2034)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/manage-release-e2e-stack.sh` around lines 125 - 142, The loop in
wait_for_health assigns attempt but never uses it (triggering SC2034); change
the for-loop to use a conventional unused variable name (e.g., replace "for
attempt in $(seq 1 30);" with "for _ in $(seq 1 30);") so the loop intent is
clear and shellcheck stops reporting an unused variable in the wait_for_health
function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR bundles the dashboard polish work on
feat/ui-polishingwith the follow-up telemetry fixes needed to make audit and usage labels unambiguous and consistent.What Changed
Dashboard and admin UX
Edit/Deleterow actions with shared icon-only buttons and extracted the icon templatesClearbuttonrequested_modelexplicitly and keepresolved_modelas the executed selectorprovider_nameover provider type wherever the UI is presenting a human-facing provider labelTelemetry model and provider semantics
provideras the canonical provider type used for routing, pricing, and filteringprovider_nameon usage and audit records so the admin UI no longer has to reconstruct provider display labels heuristicallymodelfield torequested_modelend to end, while keepingresolved_modelas the executed selectorRouting and provider resolution
Correctness fixes
provider_namecoalesce to the canonicalproviderbefore grouping; this prevents duplicate provider/model rows with split totalsprovider_nameNotes
modelfiltering for audit log reads, but the explicit field exposed now isrequested_modelTesting
go test ./internal/usage ./internal/auditlog ./internal/admin ./internal/servernode --test internal/admin/dashboard/static/js/modules/audit-list.test.js internal/admin/dashboard/static/js/modules/dashboard-layout.test.js internal/admin/dashboard/static/js/modules/execution-plans-layout.test.js internal/admin/dashboard/static/js/modules/execution-plans.test.jsgo unit test,go mod tidy, hot-path performance guard checks, andgolangci-lintSummary by CodeRabbit
New Features
Improvements