feat(virtualmodels)!: unify engine, admin API, and dashboard UI#423
Merged
Conversation
Finish the virtual-models unification begun in #416. Collapse the two composed matching engines (aliases + model access overrides) into one native engine over the single virtual_models table, behind one in-memory snapshot, and wire a single admin endpoint and dashboard surface on top. Backend - Native engine: redirect (alias) resolution and policy (access) gating now run on VirtualModel rows directly, one atomically-swapped snapshot, one refresh loop. The internal/aliases and internal/modeloverrides packages are removed; their tested matching logic was ported. Legacy tables stay one release as a rollback net, read only by a self-contained seed. - Authoritative Enabled: a policy row's Enabled governs access (disabled = off for everyone; enabled + user_paths = restricted; no row = MODELS_ENABLED_BY_DEFAULT). A single model can now be disabled. - Scoped redirects: user_paths on a redirect are enforced at resolution via the optional gateway.UserPathModelResolver, so an alias applies only to matching callers and otherwise falls through to the literal name (the use case from the closed upstream PR #387). Admin API - One GET/PUT/DELETE /admin/virtual-models endpoint replaces /admin/aliases and /admin/model-overrides. Dashboard - One virtual-model editor (Source locked + prefilled when editing an existing model, an always-present target field, user_paths, a 3-state Enabled/Restricted/Disabled switch, description), a per-row enable/disable toggle on every model, and alias-like styling for any model carrying a virtual model. The pricing and virtual-model editors share one layout. Cleanups - Pre-parse redirect targets in the snapshot (no per-request ParseModelSelector on the hot path); validate redirect user_paths instead of silently dropping them; DRY the source lookup; tighten compile-time interface assertions. See docs/adr/0008-virtual-models.md (updated). BREAKING CHANGE: the /admin/aliases and /admin/model-overrides admin endpoints are removed in favor of /admin/virtual-models; the internal aliases and modeloverrides packages are deleted. Runtime model-routing behavior is preserved; access semantics gain authoritative per-row Enabled and scoped redirects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughRemoves the legacy ChangesVirtual models unification across backend and dashboard
Sequence Diagram(s)sequenceDiagram
participant Client
participant GatewayHandler
participant ResolveExecutionSelector
participant VirtualModelsService
participant Provider
Client->>GatewayHandler: POST /v1/chat/completions (model="smart")
GatewayHandler->>ResolveExecutionSelector: ctx + requested selector
ResolveExecutionSelector->>VirtualModelsService: ResolveModelForUserPath(ctx, "smart")
VirtualModelsService->>VirtualModelsService: snapshot.resolveRedirect("smart", catalog, userPath)
VirtualModelsService-->>ResolveExecutionSelector: resolved="openai/gpt-4o", changed=true
ResolveExecutionSelector-->>GatewayHandler: resolved selector
GatewayHandler->>VirtualModelsService: ValidateModelAccess(ctx, resolved)
VirtualModelsService-->>GatewayHandler: nil (allowed)
GatewayHandler->>Provider: ChatCompletion(rewritten request, model="gpt-4o")
Provider-->>GatewayHandler: response
GatewayHandler-->>Client: 200 OK
sequenceDiagram
participant AdminUI
participant DashboardJS
participant AdminAPI
participant VirtualModelsService
participant Store
AdminUI->>DashboardJS: fetchVirtualModels()
DashboardJS->>AdminAPI: GET /admin/virtual-models
AdminAPI->>VirtualModelsService: ListViews()
VirtualModelsService-->>AdminAPI: []View (redirects + policies)
AdminAPI-->>DashboardJS: 200 []View
DashboardJS->>DashboardJS: applyVirtualModelViews(views)
AdminUI->>DashboardJS: openVirtualModelCreate()
DashboardJS->>DashboardJS: vmFormOpen=true, vmFormMode="create"
AdminUI->>DashboardJS: submitVirtualModelForm()
DashboardJS->>AdminAPI: PUT /admin/virtual-models {source, target_model, enabled}
AdminAPI->>VirtualModelsService: Upsert(ctx, VirtualModel)
VirtualModelsService->>Store: Upsert(vm)
Store-->>VirtualModelsService: nil
VirtualModelsService-->>AdminAPI: view
AdminAPI-->>DashboardJS: 200 View
DashboardJS->>DashboardJS: fetchVirtualModels() + fetchModels()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Confidence Score: 5/5This looks safe to merge.
Reviews (4): Last reviewed commit: "fix(virtualmodels): scope provider ListM..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/gateway/request_model_resolution.go (1)
215-216: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the scoped resolver in the provider-owned branch too.
The explicit resolver path is now ctx-aware, but this branch still calls
ResolveModeldirectly. When a redirect-aware provider/decorator owns resolution,user_pathsare ignored during gateway selection. ReuseresolveRequestedModel(ctx, providerResolver, requested)here and ensure redirect-aware providers implementResolveModelForUserPath.🔒 Proposed fix
if providerResolver, ok := provider.(ModelResolver); ok { - providerSelector, providerChanged, err := providerResolver.ResolveModel(requested) + providerSelector, providerChanged, err := resolveRequestedModel(ctx, providerResolver, requested) if err != nil { if resolvedSelector != (core.ModelSelector{}) { // Preserve alias targets so callers can refresh the concrete provider before retrying.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/gateway/request_model_resolution.go` around lines 215 - 216, The code in the ModelResolver type assertion branch is calling ResolveModel directly on the providerResolver, which bypasses the context-aware scoped resolution. Replace the direct call to providerResolver.ResolveModel(requested) with the scoped resolver function resolveRequestedModel(ctx, providerResolver, requested) to ensure user_paths are properly handled during gateway selection. Additionally, verify that any redirect-aware providers implementing the ModelResolver interface also implement the ResolveModelForUserPath method as required by the scoped resolver.internal/virtualmodels/seed_test.go (1)
141-145: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert collision failures leave no partial seed rows.
This test passes if
seedFromLegacyinserts one legacy row and then returns the collision error, which would leave the rollback-safety migration in a partially applied state. Check the virtual model store remains empty after the error.Test hardening
if err := seedFromLegacy(ctx, vmStore, conn); err == nil { t.Fatalf("seedFromLegacy() error = nil, want migration conflict error") } + got, err := vmStore.List(ctx) + if err != nil { + t.Fatalf("List() after failed seed error = %v", err) + } + if len(got) != 0 { + t.Fatalf("failed seed left %d virtual model row(s): %#v", len(got), got) + }As per coding guidelines, tests should cover error handling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/virtualmodels/seed_test.go` around lines 141 - 145, The test currently only verifies that seedFromLegacy returns an error, but does not check that the virtual model store remains empty after the error occurs. Add an assertion after the error check to verify that vmStore contains no rows or entries, ensuring that the collision error prevents partial seed data from being left in the system. This will properly test that the migration is atomic and leaves no partial state when encountering the collision error.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/admin/dashboard/static/js/modules/aliases.js`:
- Line 624: The logic for computing vmFormEffectiveEnabled at line 624
incorrectly treats the presence of an override as true even when
override.enabled is explicitly false. The current expression Boolean(override &&
override.enabled !== false) || defaultEnabled uses override.enabled !== false
which treats undefined as true. Instead, check if an override exists and use its
enabled value directly; if override exists and has an enabled property, use that
value; otherwise fall back to defaultEnabled. This ensures that an override with
enabled set to false is properly respected and takes precedence over the default
value.
- Around line 799-808: The toggleRowEnabled method dispatches write operations
via toggleAliasRow and toggleModelRow without checking if the feature is
currently available, which leads to repeated failing requests when the service
is in an unavailable state. Add a guard condition at the beginning of the
toggleRowEnabled method to check if virtual models are available before
proceeding with either the row.is_alias check and toggleAliasRow call or the
toggleModelRow call, and return early if the feature is unavailable.
- Around line 1052-1072: The confirmation check for overwriting existing aliases
is skipped when targetModel is empty because the condition checks both
vmFormMode !== 'edit' AND targetModel together. This allows silent overwrites of
existing redirect/policy rows in create mode with an empty target_model.
Restructure the logic to separate the checks: first verify if we are in create
mode and have a source, then check for existing aliases using
findExistingAliasByName regardless of whether targetModel is set, ensuring users
always get confirmation before overwriting an existing alias or policy
regardless of the target_model value.
In `@internal/admin/dashboard/templates/page-models.html`:
- Around line 130-132: The Save button (virtual-model-submit-btn) at line 131
only checks vmSubmitting in its disabled state, but does not account for
vmDeleting which is set by the Remove button. Update the :disabled binding on
the submit button to include vmDeleting in addition to vmSubmitting, so the Save
button is disabled whenever either a save or delete operation is in progress,
preventing concurrent submissions.
In `@internal/admin/handler_usage.go`:
- Around line 409-410: The virtual model selector resolution in the ResolveModel
method call is using an empty string for the user path parameter when
constructing the RequestedModelSelector, which prevents proper path-restricted
redirect resolution. Replace the empty string "" passed to
core.NewRequestedModelSelector with the actual user_path variable that should be
available in this handler context to ensure scoped redirects are correctly
resolved and the appropriate provider/model combination is selected.
In `@internal/virtualmodels/provider.go`:
- Around line 473-493: The redirect resolution helper functions
resolveRedirectModel, resolveRedirectRequestSelector, and
resolveRedirectRoutableSelector are currently using the unscoped
Service.ResolveModel method, which does not respect user_path scoping. Add a ctx
context.Context parameter to all three functions, and update the call to
service.ResolveModel to use service.ResolveModelForUserPath(ctx, requested)
instead. This will ensure request-time redirects are properly scoped to user
paths. Update all callers of these functions to pass the request context.
In `@internal/virtualmodels/resolve.go`:
- Around line 85-113: The exposedModelsFiltered method does not check
entry.vm.UserPaths when building the result list, allowing redirects scoped to
specific paths like /team to appear in context-free listings such as
ExposedModels. Add a condition in the exposedModelsFiltered function to skip
entries where entry.vm.UserPaths is non-empty, ensuring that scoped redirects
are not exposed when there is no user path context available to match against.
This check should occur after the Enabled check and before appending to the
result list.
In `@internal/virtualmodels/seed_test.go`:
- Around line 85-87: Add explicit assertions after the migration logic completes
in the idempotency test to verify that the Enabled semantics are properly
handled. Specifically, add assertions to confirm that the enabled alias created
via insertLegacyAlias with enabled=true remains enabled after migration, and
that the legacy override created via insertLegacyOverride becomes an enabled
restricted policy. Additionally, add a test case with a disabled legacy alias
(by inserting an alias with enabled=false) to ensure that disabled aliases are
properly handled during migration and that regressions in this case would be
caught by the test.
In `@internal/virtualmodels/seed.go`:
- Around line 52-61: The seed function currently writes aliases to the store via
store.Upsert before validating override conflicts, which means if an error
occurs during override processing, aliases are already persisted and seeding
gets skipped on retry. Restructure the logic to validate all legacy aliases and
overrides for conflicts before making any store.Upsert calls; once validation
completes successfully, then perform all write operations. If the store supports
transactions, wrap both the legacyAliasRows loop and legacyOverrideRows loop
write operations within a single transaction to ensure atomicity and prevent
partial migrations.
---
Outside diff comments:
In `@internal/gateway/request_model_resolution.go`:
- Around line 215-216: The code in the ModelResolver type assertion branch is
calling ResolveModel directly on the providerResolver, which bypasses the
context-aware scoped resolution. Replace the direct call to
providerResolver.ResolveModel(requested) with the scoped resolver function
resolveRequestedModel(ctx, providerResolver, requested) to ensure user_paths are
properly handled during gateway selection. Additionally, verify that any
redirect-aware providers implementing the ModelResolver interface also implement
the ResolveModelForUserPath method as required by the scoped resolver.
In `@internal/virtualmodels/seed_test.go`:
- Around line 141-145: The test currently only verifies that seedFromLegacy
returns an error, but does not check that the virtual model store remains empty
after the error occurs. Add an assertion after the error check to verify that
vmStore contains no rows or entries, ensuring that the collision error prevents
partial seed data from being left in the system. This will properly test that
the migration is atomic and leaves no partial state when encountering the
collision error.
🪄 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: 7b8ebd52-2d41-490a-8060-d2dc9fc32fc3
📒 Files selected for processing (66)
docs/adr/0008-virtual-models.mdinternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/aliases.test.cjsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/static/js/modules/timezone-layout.test.cjsinternal/admin/dashboard/templates/model-table-body.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/errors.gointernal/admin/handler.gointernal/admin/handler_aliases.gointernal/admin/handler_aliases_test.gointernal/admin/handler_model_overrides.gointernal/admin/handler_model_overrides_test.gointernal/admin/handler_models.gointernal/admin/handler_models_test.gointernal/admin/handler_pricing_test.gointernal/admin/handler_usage.gointernal/admin/handler_virtualmodels.gointernal/admin/handler_virtualmodels_test.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/aliases/batch_preparer.gointernal/aliases/batch_preparer_test.gointernal/aliases/factory.gointernal/aliases/provider_test.gointernal/aliases/service.gointernal/aliases/service_test.gointernal/aliases/store.gointernal/aliases/store_mongodb.gointernal/aliases/store_postgresql.gointernal/aliases/store_sqlite.gointernal/aliases/types.gointernal/app/app.gointernal/app/app_test.gointernal/app/runtime_refresh.gointernal/gateway/batch_selection.gointernal/gateway/interfaces.gointernal/gateway/request_model_resolution.gointernal/modeloverrides/batch_preparer.gointernal/modeloverrides/factory.gointernal/modeloverrides/service.gointernal/modeloverrides/service_test.gointernal/modeloverrides/store.gointernal/modeloverrides/store_mongodb.gointernal/modeloverrides/store_postgresql.gointernal/modeloverrides/store_sqlite.gointernal/modeloverrides/store_sqlite_test.gointernal/modeloverrides/types.gointernal/server/handlers_test.gointernal/server/model_validation_test.gointernal/virtualmodels/adapters.gointernal/virtualmodels/authorize.gointernal/virtualmodels/batch_preparer.gointernal/virtualmodels/factory.gointernal/virtualmodels/provider.gointernal/virtualmodels/resolve.gointernal/virtualmodels/seed.gointernal/virtualmodels/seed_legacy.gointernal/virtualmodels/seed_test.gointernal/virtualmodels/service.gointernal/virtualmodels/service_test.gointernal/virtualmodels/snapshot.gointernal/virtualmodels/types.gointernal/virtualmodels/validation.go
💤 Files with no reviewable changes (26)
- internal/aliases/provider_test.go
- internal/aliases/types.go
- internal/aliases/store_postgresql.go
- internal/modeloverrides/store_sqlite_test.go
- internal/modeloverrides/store_sqlite.go
- internal/modeloverrides/store_postgresql.go
- internal/admin/handler_aliases_test.go
- internal/modeloverrides/factory.go
- internal/aliases/store.go
- internal/admin/handler_aliases.go
- internal/aliases/batch_preparer.go
- internal/admin/handler_model_overrides_test.go
- internal/aliases/batch_preparer_test.go
- internal/virtualmodels/adapters.go
- internal/modeloverrides/batch_preparer.go
- internal/modeloverrides/types.go
- internal/aliases/service.go
- internal/aliases/store_mongodb.go
- internal/modeloverrides/service_test.go
- internal/aliases/store_sqlite.go
- internal/aliases/service_test.go
- internal/modeloverrides/store_mongodb.go
- internal/admin/handler_model_overrides.go
- internal/modeloverrides/store.go
- internal/aliases/factory.go
- internal/modeloverrides/service.go
…oped alias listing Addresses the greptile review on #423. - Refresh cadence (P1): the unified refresh loop used only the model-cache interval (default 1h), so on a multi-instance deployment a disable/restrict on one instance could take up to an hour to reach others — a regression from the policy side's prior minute cadence. Use the shorter of the model-cache and workflow intervals. - Seed safety (P1): the seed wrote legacy aliases before checking access-override source collisions, so a conflict could abort mid-write and leave virtual_models partially seeded; the len(existing) > 0 guard would then skip the seed on the next startup and never import the access overrides. Resolve all rows and detect collisions before writing anything (test now asserts an empty table after a conflict). - Scoped alias listing (P2): ExposedModelsFiltered only filtered by the target policy, so a redirect scoped to user_paths was listed in /v1/models to callers outside its scope even though resolution falls through for them. Add ExposedModelsForUserPath (hiding redirects the caller does not match) and have the server prefer it via the optional UserPathExposedModelLister. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks @greptile-apps — all three findings addressed in 8957e18:
|
|
All three fixes look solid. Here's my assessment: P1 — Refresh cadence ( P1 — Seed atomicity ( P2 — Scoped alias listing ( The |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/server/handlers.go`:
- Around line 417-423: The UserPathExposedModelLister type check in the handler
is coupled with the h.modelAuthorizer nil check, meaning scoped user-path
listing only occurs when an authorizer exists. Decouple these checks by removing
the h.modelAuthorizer != nil condition from the UserPathExposedModelLister
branch so that scoped filtering always executes when available, and modify the
authorization filter function passed to ExposedModelsForUserPath to handle the
case where h.modelAuthorizer is nil (returning true to allow all models when no
authorizer is present). Apply the same decoupling logic to the
FilteredExposedModelLister branch to ensure consistent behavior.
🪄 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: 46126531-c41b-4b00-93cd-f743d2b019eb
📒 Files selected for processing (8)
internal/server/exposed_model_lister.gointernal/server/handlers.gointernal/virtualmodels/factory.gointernal/virtualmodels/resolve.gointernal/virtualmodels/seed.gointernal/virtualmodels/seed_test.gointernal/virtualmodels/service.gointernal/virtualmodels/service_test.go
Backend - Make request-time redirect rewrites user-path aware (Major, security): the shared Provider/BatchPreparer rewrite path resolved redirects with unscoped ResolveModel, so a user_paths-scoped alias could be applied for callers outside its scope in batch and guardrail-executor requests (the main chat path already resolved scoped via the gateway). Thread ctx through the rewrite helpers and use ResolveModelForUserPath for request-time rewrites; keep unscoped ResolveModel only for inventory/projection. - Seed test now asserts migrated Enabled semantics, including a disabled legacy alias staying disabled (Enabled is authoritative). Dashboard - vmFormEffectiveEnabled now reflects the override's value (a disabled override shows "Effective now: no" even when the default is enabled), not its presence. - toggleRowEnabled bails out when virtual models are unavailable, avoiding repeated failing writes after a 503. - Create-mode overwrite confirmation runs even with an empty target, since that is a policy upsert that can otherwise silently replace an existing redirect/policy. - Save is disabled while a Remove is in progress (vmDeleting), preventing concurrent DELETE + PUT. Not changed: handler_usage.go pricing recalculation resolves an operator-supplied selector that has no request user_path, and v1 redirects have a single target, so unscoped resolution yields the correct concrete model for pricing — using the user-path resolver there would wrongly fall through to the literal name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed @coderabbitai's findings in ebcd3c0 (the two it overlapped with @greptile-apps — seed prevalidation and exposed-redirect user_path gating — were already in 8957e18): Fixed
Not changed (with reason)
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
The security fix in Everything looks good — feel free to proceed with merging. 🐇✨ |
|
The fix pass looks thorough. A few notes on each item: Confirmed valid reasoning — Two of my original findings remain open:
Everything else looks addressed. The |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/admin/dashboard/static/js/modules/aliases.js (1)
884-891: 🎯 Functional Correctness | 🟠 MajorEnable toggle silently fails when the default is disabled.
In the enable branch (
desired === true), an existing policy with emptyuser_pathsalways triggers aDELETE, reverting the source to inherited/default behavior. Whendefault_enabledisfalse(e.g.,MODELS_ENABLED_BY_DEFAULT=false, or an inherited provider/global disable), deleting the explicit policy leaves the model disabled even though the operator just toggled it on — yet line 919 still reports'Model enabled.'. OnlyPUT enabled:truereliably enables in that case.Proposed fix
- } else if (existingPolicy && existingPaths.length === 0) { - method = 'DELETE'; - payload = { source: selector }; + } else if (existingPolicy && existingPaths.length === 0 && access.default_enabled !== false) { + method = 'DELETE'; + payload = { source: selector }; } else { payload = { source: selector, enabled: true, user_paths: existingPaths }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/admin/dashboard/static/js/modules/aliases.js` around lines 884 - 891, The enable toggle fails when the default is disabled because the condition `existingPolicy && existingPaths.length === 0` triggers a DELETE operation even in the enable case (desired === true), which reverts the source to inherited/default behavior. When default_enabled is false, this leaves the model disabled despite the user toggling it on. Fix this by restructuring the conditional logic to ensure that when desired === true, you always send a PUT request with enabled:true instead of using DELETE, regardless of whether existingPaths is empty. This will explicitly enable the model and work correctly in all default_enabled scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/aliases.js`:
- Around line 884-891: The enable toggle fails when the default is disabled
because the condition `existingPolicy && existingPaths.length === 0` triggers a
DELETE operation even in the enable case (desired === true), which reverts the
source to inherited/default behavior. When default_enabled is false, this leaves
the model disabled despite the user toggling it on. Fix this by restructuring
the conditional logic to ensure that when desired === true, you always send a
PUT request with enabled:true instead of using DELETE, regardless of whether
existingPaths is empty. This will explicitly enable the model and work correctly
in all default_enabled scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49420b45-af64-42e5-a7e4-101e62b1a9a9
📒 Files selected for processing (6)
internal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/templates/page-models.htmlinternal/virtualmodels/batch_preparer.gointernal/virtualmodels/provider.gointernal/virtualmodels/seed_test.go
…toggle, seed rollback - /v1/models exposure scoping no longer requires a model authorizer (server, Major): UserPathExposedModelLister now runs even when h.modelAuthorizer is nil (passing a nil target-allow), so a user_paths-scoped redirect is still hidden from callers outside its scope instead of falling back to unscoped listing. - Dashboard enable toggle (P1): enabling a model only DELETEs a path-less policy when the default actually enables it (default_enabled != false). In a default-disabled deployment it now PUTs enabled:true so the model is really enabled instead of falling back to the disabled default while the UI claims "enabled". - Seed partial-write rollback (P1): if a write fails mid-seed (DB error, context cancellation), already-written rows are rolled back (best-effort, fresh context) so a partial seed cannot trip the len(existing) > 0 guard and skip importing the remaining access controls next startup. Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed the latest re-review findings in 81136a3:
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/virtualmodels/seed.go (1)
60-81: 🔒 Security & Privacy | 🔴 CriticalApply source normalization before collision detection to match Store.Upsert behavior.
The collision detection in
seedFromLegacy(lines 60-81) uses rawvm.Sourcevalues fromalias.toRedirect()andoverride.toPolicy(), but all threeStore.Upsertimplementations normalize the source viastrings.TrimSpace()before persisting. This creates a mismatch: an alias withSource="foo "(trailing space) and an override withSource="foo"would both pass the collision check (different raw keys), but collide on the same normalized store key"foo". The override—containing an access control—would then be silently overwritten, exactly the failure this check aims to prevent.Normalize sources in the
seenmap using the samestrings.TrimSpace()thatStore.Upsertapplies, or wrap the legacy rows withnormalizeRedirect()/normalizeStoredPolicy()before the collision check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/virtualmodels/seed.go` around lines 60 - 81, The collision detection in seedFromLegacy uses raw vm.Source values from alias.toRedirect() and override.toPolicy(), but Store.Upsert normalizes sources using strings.TrimSpace() before persisting. This mismatch allows sources with whitespace differences (like "foo " and "foo") to bypass the collision check but collide in the actual store, silently overwriting the access control override. Normalize each vm.Source using strings.TrimSpace() when adding to the seen map to match the normalization behavior in Store.Upsert, ensuring the collision detection catches all actual conflicts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/virtualmodels/seed.go`:
- Around line 60-81: The collision detection in seedFromLegacy uses raw
vm.Source values from alias.toRedirect() and override.toPolicy(), but
Store.Upsert normalizes sources using strings.TrimSpace() before persisting.
This mismatch allows sources with whitespace differences (like "foo " and "foo")
to bypass the collision check but collide in the actual store, silently
overwriting the access control override. Normalize each vm.Source using
strings.TrimSpace() when adding to the seen map to match the normalization
behavior in Store.Upsert, ensuring the collision detection catches all actual
conflicts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca965fb7-6c94-4a25-9264-a6abf1417f67
📒 Files selected for processing (4)
internal/admin/dashboard/static/js/modules/aliases.jsinternal/server/handlers.gointernal/virtualmodels/seed.gointernal/virtualmodels/seed_test.go
Contributor
|
Everything looks good — feel free to merge. 🐇✨ ✅ Action performedComments resolved. Approval is disabled; enable |
…ss scopes
Fixes the edit-form prefill for provider-wide ("{provider}/") and any policy whose
effective state was computed from override PRESENCE rather than its enabled VALUE:
providerGroupAccess.effective_enabled now honors the override's enabled flag (a
disabled provider policy reads as disabled), and the model/provider editor reads
the override's own enabled value. This also corrects the provider-group state.
Adds the same three-state switch (Enabled / Restricted / Disabled) to the provider
("{provider}/") and global ("/") rows on the models list, matching model and alias
rows. The toggle markup is extracted into one reusable {{template "access-toggle"}}
partial driven by the existing scope-agnostic rowToggle*/toggleRowEnabled helpers,
so alias rows, model rows, provider groups, and a new globalScopeRow all share it.
The redundant model-access-state-badge (and now-dead modelAccessStateText) are
removed — the toggle is the single state indicator everywhere.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the latest greptile re-review. - Provider.ListModels merged exposed redirects with the unscoped ExposedModels() projection, leaking user_paths-scoped redirect IDs to callers outside their scope (the server /v1/models path was already scoped). It has ctx, so use ExposedModelsForUserPath(core.UserPathFromContext(ctx), nil) for parity. - The legacy seed now writes atomically on the SQL backends: SQLiteStore and PostgreSQLStore implement an optional transactional UpsertAll, and the seed prefers it so a failed seed leaves the table untouched rather than partially populated (which would trip the "already populated" guard and suppress a re-import next start). MongoDB — which has no cross-backend transaction without a replica set — keeps the per-row best-effort rollback fallback. Added an UpsertAll atomicity test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed the latest re-review in 32fb69e:
On the residual MongoDB case: a fully atomic Mongo seed needs a session transaction (replica set), which isn't always available; the best-effort rollback + the fact that a seed error fails startup (so the process won't serve partial data on that run) is the pragmatic floor for this REMOVE-LATER migration. Happy to add a Mongo session-transaction path if you'd prefer. |
Contributor
Author
|
Confirmed the type question from the review on the |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/admin/dashboard/static/js/modules/aliases.js (1)
913-925: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not DELETE an exact policy when a broader policy would still restrict the row.
When enabling a path-less exact policy, this branch deletes it whenever defaults are enabled. If a provider/global policy is disabled or restricted, deleting the exact row falls back to that broader policy, so the toggle can say “Model enabled” while the row remains disabled/restricted. Keep an explicit
enabled: truepolicy unless the inherited state already allows all.Based on PR context, virtual-model policies use exact/provider/global precedence, and DELETE only removes the exact source row.
Suggested fix
const existingPaths = existingPolicy && Array.isArray(existingPolicy.user_paths) ? existingPolicy.user_paths : []; + const providerSelector = row.provider_name ? this.providerOverrideSelector(row.provider_name) : ''; + const providerPolicy = providerSelector && providerSelector !== selector + ? this.findModelOverrideView(providerSelector) + : null; + const globalPolicy = selector !== this.globalOverrideSelector() + ? this.findModelOverrideView(this.globalOverrideSelector()) + : null; + const inheritedPolicy = providerPolicy || globalPolicy; + const inheritedAllowsAll = !inheritedPolicy + || (inheritedPolicy.enabled !== false && !this.modelAccessUserPathsRestrict(inheritedPolicy.user_paths)); this.rowTogglingKey = row.key; this.aliasError = ''; this.aliasNotice = ''; let method = 'PUT'; let payload; if (desired === false) { payload = { source: selector, enabled: false, user_paths: existingPaths }; - } else if (existingPolicy && existingPaths.length === 0 && access.default_enabled !== false) { + } else if (existingPolicy && existingPaths.length === 0 && access.default_enabled !== false && inheritedAllowsAll) { // Removing a path-less policy only enables the model when the // default is on; in a default-disabled deployment we must keep an // explicit enabled policy instead of falling back to the default. method = 'DELETE'; payload = { source: selector }; } else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/admin/dashboard/static/js/modules/aliases.js` around lines 913 - 925, The DELETE condition in the path-less policy handling block only checks if access.default_enabled is not false, but it fails to account for broader policies (provider/global level) that might still restrict the row. When deleting the exact policy, if a broader policy is disabled or restricted, the row will fall back to that restricted state while the toggle incorrectly shows as enabled. Fix this by also verifying that broader policies would allow access before proceeding with the DELETE operation. If broader policies would restrict access, use the else branch instead to keep an explicit enabled: true policy for the selector source, ensuring the row remains properly enabled and not restricted by inherited policies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/virtualmodels/store_postgresql.go`:
- Line 155: The deferred transaction rollback in the anonymous function at line
155 is using the seeding context which may be canceled after Begin is called,
causing the rollback to fail and leaving connection/transaction cleanup to pgx.
Replace the seeding context parameter in the tx.Rollback(ctx) call with a fresh
context obtained from rollbackContext() to match the existing rollback pattern
used in service.go, ensuring proper cleanup even if the original context is
canceled.
In `@internal/virtualmodels/store_test.go`:
- Around line 109-116: The current test for the cancelled context in the
UpsertAll function only cancels the context before any transaction begins, which
does not actually test whether rollback occurs after a partial write succeeds.
Modify the test to add a mid-batch failure scenario where the first row in the
vms batch is successfully written but a subsequent row fails (for example, by
introducing a constraint violation or validation error on the second row), then
verify that the entire transaction is rolled back and the table remains empty.
This will ensure the test catches missing rollback logic in UpsertAll when
failures occur after partial writes.
---
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/aliases.js`:
- Around line 913-925: The DELETE condition in the path-less policy handling
block only checks if access.default_enabled is not false, but it fails to
account for broader policies (provider/global level) that might still restrict
the row. When deleting the exact policy, if a broader policy is disabled or
restricted, the row will fall back to that restricted state while the toggle
incorrectly shows as enabled. Fix this by also verifying that broader policies
would allow access before proceeding with the DELETE operation. If broader
policies would restrict access, use the else branch instead to keep an explicit
enabled: true policy for the selector source, ensuring the row remains properly
enabled and not restricted by inherited policies.
🪄 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: 61f6565f-38ef-40b5-b655-833d3e242682
📒 Files selected for processing (10)
internal/admin/dashboard/static/js/modules/aliases.jsinternal/admin/dashboard/static/js/modules/aliases.test.cjsinternal/admin/dashboard/templates/access-toggle.htmlinternal/admin/dashboard/templates/model-table-body.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/virtualmodels/provider.gointernal/virtualmodels/seed.gointernal/virtualmodels/store_postgresql.gointernal/virtualmodels/store_sqlite.gointernal/virtualmodels/store_test.go
💤 Files with no reviewable changes (1)
- internal/admin/dashboard/static/js/modules/aliases.test.cjs
…le, collapse flags - Refresh cadence: the unified store now refreshes solely on Cache.Model.Refresh- Interval (virtual models are part of the model-config plane, same cadence as the provider model list), dropping the Workflows.RefreshInterval min() mixing. - Rename the dashboard controller aliases.js -> virtual-models.js (and dashboardAliasesModule -> dashboardVirtualModelsModule, aliases.test.cjs -> virtual-models.test.cjs); update the script loader, embedded-asset test, and template reference. The module drives the unified virtual-models UI, so the "aliases" name was a misnomer. - Collapse the redundant aliasesAvailable/modelOverridesAvailable flags into the single virtualModelsAvailable (templates already used it). - Remove the now-dead model-access-state-badge CSS (the toggle replaced the badge at every scope). Inert load-balancing fields (Strategy, Target.Weight, multi-target) are kept per ADR-0008. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ch atomicity test Addresses coderabbit review on the UpsertAll work: - PostgreSQLStore.UpsertAll deferred its tx.Rollback with the seed context, so a canceled context would skip rollback and leak cleanup to pgx. Use a fresh rollbackContext() (mirrors service.go). SQLite's tx.Rollback() takes no context and is unaffected. - TestSQLiteStore_UpsertAllIsAtomic previously failed at BeginTx (pre-write), so it never proved rollback after a successful row. Replace it with a true mid-batch failure (row 1 written in the tx, row 2 fails to encode) and assert the table is left empty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed @coderabbitai's review of the UpsertAll work (19e6c09):
Also in this push (separate commit 3b16799, the requested cleanups): unified the refresh cadence on
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| aliases: [], | ||
| models: [], | ||
| modelOverrideViews: [], | ||
| virtualModelsAvailable: true, |
| aliases: [], | ||
| models: [], | ||
| modelOverrideViews: [], | ||
| virtualModelsAvailable: true, |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/virtualmodels/factory.go`:
- Around line 104-107: The comment in the virtual models factory section
references an incorrect environment variable name CACHE_MODEL_REFRESH_INTERVAL
when the actual config field uses CACHE_REFRESH_INTERVAL. Update the referenced
environment variable name in the comment that discusses the unified store
refresh cadence and operator tuning to use the correct env var name
CACHE_REFRESH_INTERVAL instead of CACHE_MODEL_REFRESH_INTERVAL to prevent
operator confusion during runtime configuration.
🪄 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: 7c7d2c28-1f9f-433f-8ee1-73dc1ccfb7a2
📒 Files selected for processing (9)
internal/admin/dashboard/dashboard_test.gointernal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/virtual-models.jsinternal/admin/dashboard/static/js/modules/virtual-models.test.cjsinternal/admin/dashboard/templates/layout.htmlinternal/virtualmodels/factory.gointernal/virtualmodels/store_postgresql.gointernal/virtualmodels/store_test.go
💤 Files with no reviewable changes (1)
- internal/admin/dashboard/static/css/dashboard.css
Comment on lines
+104
to
+107
| // Virtual models are part of the model-config plane, so the unified store | ||
| // refreshes on the model-cache cadence — the same interval the provider model | ||
| // list uses. Cross-instance staleness is therefore identical to the model | ||
| // cache's; operators tune CACHE_MODEL_REFRESH_INTERVAL for faster propagation. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the config env var name in the operator guidance comment.
Line 107 says CACHE_MODEL_REFRESH_INTERVAL, but the config field uses env:"CACHE_REFRESH_INTERVAL". This mismatch can mislead runtime tuning.
Suggested patch
- // cache's; operators tune CACHE_MODEL_REFRESH_INTERVAL for faster propagation.
+ // cache's; operators tune CACHE_REFRESH_INTERVAL for faster propagation.📝 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
| // Virtual models are part of the model-config plane, so the unified store | |
| // refreshes on the model-cache cadence — the same interval the provider model | |
| // list uses. Cross-instance staleness is therefore identical to the model | |
| // cache's; operators tune CACHE_MODEL_REFRESH_INTERVAL for faster propagation. | |
| // Virtual models are part of the model-config plane, so the unified store | |
| // refreshes on the model-cache cadence — the same interval the provider model | |
| // list uses. Cross-instance staleness is therefore identical to the model | |
| // cache's; operators tune CACHE_REFRESH_INTERVAL for faster propagation. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/virtualmodels/factory.go` around lines 104 - 107, The comment in the
virtual models factory section references an incorrect environment variable name
CACHE_MODEL_REFRESH_INTERVAL when the actual config field uses
CACHE_REFRESH_INTERVAL. Update the referenced environment variable name in the
comment that discusses the unified store refresh cadence and operator tuning to
use the correct env var name CACHE_REFRESH_INTERVAL instead of
CACHE_MODEL_REFRESH_INTERVAL to prevent operator confusion during runtime
configuration.
Contributor
|
The cleanups in 3b16799 are good housekeeping: the This PR is in excellent shape — all findings across the four rounds have been fully addressed with clear rationale for the intentional non-changes. Ready to merge. 🐇✨ ✅ Action performedFailed to resolve some comments. Please check permissions and try again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Finishes the virtual-models unification started in #416. Aliases and model access overrides are now one virtual model entity end to end: one native engine, one admin endpoint, one dashboard editor.
Backend
VirtualModelrows behind a single atomically-swapped snapshot, one refresh loop.internal/aliasesandinternal/modeloverridesare deleted (logic ported; legacy tables kept one release as a rollback net, read only by a self-contained seed).Enabled. A policy'sEnabledgoverns access — disabled = off for everyone, enabled +user_paths= restricted, no row =MODELS_ENABLED_BY_DEFAULT. A single model can now be disabled.user_pathson a redirect are enforced at resolution via the optionalgateway.UserPathModelResolver; a scoped alias applies only to matching callers and otherwise falls through to the literal model name.Admin API (breaking)
GET/PUT/DELETE /admin/virtual-modelsreplaces/admin/aliasesand/admin/model-overrides.Dashboard
user_paths, a 3-state Enabled/Restricted/Disabled switch, description.Cleanups / perf
ParseModelSelectoron the hot path).user_pathsinstead of silently dropping them (a dropped path would widen a scoped alias to everyone).Verification
go build ./...,go vet ./...,go test ./...— 57 packages green.node --test …/*.test.cjs— 318 JS tests green./v1/modelsexposure, disable/enable a real model, and the PR feat(aliases): add user_path restrictions to model aliases #387 scoped redirect (matchinguser_pathresolves to the target; others fall through tomodel_not_found).ADR
docs/adr/0008-virtual-models.mdupdated.Notes / follow-ups
docs/openapi.jsonstill lists/admin/model-overrides:make docs-openapifails on a pre-existing swag parse error ininternal/server/messages_handler.go(json.RawMessage), unrelated to this change, so the generated spec wasn't regenerated./v1/modelsexposure for a scoped redirect is unscoped (listed for everyone); resolution/enforcement is correct. Filtering the listing by per-redirectuser_pathsis a small follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
/admin/virtual-modelsendpoint for managing model redirects and policies.Documentation