feat(virtualmodels): unify aliases and access overrides into virtual models#416
Conversation
…models Aliases (rename -> real model) and access overrides (gate a scoped selector by user_paths) were stored, served, and surfaced separately despite being the same operator concept. The split duplicated user_path scoping and DB-migration handling and caused real bugs. Introduce a single virtual_models entity keyed by source: rows with targets are redirects (alias now, load balancing later), rows without targets are access policies. Behavior is derived from targets-presence; there is no role column, and resolution stays staged (redirect early, gate late). To avoid re-porting subtle resolution logic, the unified service composes the existing, tested aliases and modeloverrides engines over one store via role-partitioned adapters. A one-time, idempotent seed migrates legacy aliases and model_overrides rows; the legacy tables are left dormant for one release. v1 preserves today's behavior exactly; multi-target/strategy and scoped redirects are inert fields enabled by later, additive changes. See docs/adr/0008-virtual-models.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (3)
📝 WalkthroughWalkthroughIntroduces the ChangesVirtual Models Unification
Sequence Diagram(s)sequenceDiagram
participant App
participant virtualmodels as virtualmodels.New
participant seedFromLegacy
participant Service
participant redirectStore
participant policyStore
App->>virtualmodels: New(ctx, cfg, catalog)
virtualmodels->>seedFromLegacy: copy legacy aliases + overrides (if virtual_models empty)
seedFromLegacy-->>virtualmodels: seeded
virtualmodels->>Service: NewService(store, catalog)
Service->>redirectStore: NewAliasService(redirectStore)
Service->>policyStore: NewOverrideService(policyStore)
virtualmodels->>Service: Refresh()
virtualmodels-->>App: Result{Service, stopFns}
Note over App: aliasService = virtualModels.Service.Aliases()
Note over App: modelOverrideService = virtualModels.Service.Overrides()
App->>Service: Upsert(vm)
alt vm.IsRedirect()
Service->>redirectStore: ensureSourceKind + Upsert
else policy
Service->>policyStore: ensureSourceKind + Upsert
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 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: 4/5This is close, but the disabled-overrides behavior should be fixed before merging.
internal/app/app.go and the related model override config plumbing. Reviews (2): Last reviewed commit: "refactor(config)!: remove MODEL_OVERRIDE..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (a *App) modelOverrideService() refreshableService { | ||
| if a == nil || a.modelOverrides == nil || a.modelOverrides.Service == nil { | ||
| if a == nil || a.virtualModels == nil || a.virtualModels.Service == nil { | ||
| return nil | ||
| } | ||
| return a.modelOverrides.Service | ||
| return a.virtualModels.Service.Overrides() | ||
| } |
There was a problem hiding this comment.
This helper returns the composed override service whenever virtual models exist, even when Models.OverridesEnabled is false. Startup correctly leaves the gateway authorizer nil in that configuration, but /admin/runtime/refresh will still run and report a model_overrides refresh step as active. Operators can get an OK refresh result for a feature that is disabled and not wired into request handling.
| return &Result{ | ||
| Service: service, |
There was a problem hiding this comment.
The override background refresh loop starts unconditionally. With MODEL_OVERRIDES_ENABLED=false, the authorizer is not wired, but this goroutine still reads and parses policy rows forever and can log refresh errors for a disabled feature. This regresses the old disabled path, where the model-overrides subsystem was not initialized at all.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/adapters.go`:
- Around line 16-31: The ensureSourceKind function performs a separate read
operation before the upsert operations occur later in the code, creating a race
condition where concurrent writes can bypass the cross-kind clobber protection.
Move the kind enforcement logic from the separate ensureSourceKind check into
the actual upsert operations (at lines 95 and 129) by implementing a conditional
update in the backend store that validates the kind atomically during the write.
This ensures that the validation and write happen together as a single atomic
operation, preventing two concurrent requests from both passing the check and
overwriting each other.
🪄 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: 3bb01699-920e-4ca6-a645-dac8808b0ade
📒 Files selected for processing (17)
docs/adr/0008-virtual-models.mdinternal/app/app.gointernal/app/app_test.gointernal/app/runtime_refresh.gointernal/virtualmodels/adapters.gointernal/virtualmodels/factory.gointernal/virtualmodels/helpers_test.gointernal/virtualmodels/seed.gointernal/virtualmodels/seed_test.gointernal/virtualmodels/service.gointernal/virtualmodels/service_test.gointernal/virtualmodels/store.gointernal/virtualmodels/store_mongodb.gointernal/virtualmodels/store_postgresql.gointernal/virtualmodels/store_sqlite.gointernal/virtualmodels/store_test.gointernal/virtualmodels/types.go
…sabled parity - Migration: a source shared by a legacy alias and a legacy access override now fails the migration loudly instead of silently dropping the override. Dropping it could remove an access control and expose a previously gated model (provider-explicit requests bypass alias resolution). - Cross-kind clobber guard: redirect/policy adapters now share a write mutex so the check-then-write upsert/delete serialize across the two engines' locks, closing a TOCTOU where concurrent admin writes to the same source could clobber. - Disabled overrides: when MODEL_OVERRIDES_ENABLED=false the access-override engine is no longer built, refreshed, or reported by /admin/runtime/refresh, restoring parity with the old disabled path (authorizer stays nil = allow-all). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @greptile-apps — all four findings were valid and are addressed in P1 · Policy migration drops access ( P1 · Clobber guard races ( P2 · Disabled overrides still refresh / poll ( New tests cover the fail-closed migration, the disabled-overrides path, and the cross-kind guard. |
|
All four fixes verify correctly against the diff. Migration fail-closed ( Shared write mutex ( Disabled overrides — no background goroutine ( Disabled overrides — runtime refresh skip ( The implementation is correct. LGTM. |
…ca residual race Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai good catch on the multi-replica angle. The shared mutex makes the check-then-write atomic within a process (the common single-instance case), but you're right that across replicas sharing one database it's best-effort. I've chosen to document this as a known limitation rather than make it fully atomic, because:
If multi-replica admin-write safety becomes a real requirement, the conditional-upsert approach is the fix; noted in the code comment. Happy to take it on in a follow-up if maintainers prefer. Re: docstring coverage — the package, exported types, and exported funcs are documented; the 32% figure is counting unexported helpers and test funcs. |
|
Tip For best results, initiate chat on the files or code changes.
If multi-replica write safety does become a requirement down the line, the per-backend approach you described (targeting-column length check on insert, filtering update on Mongo) is a clean upgrade path without needing to restructure the schema. Worth capturing that sketch in a follow-up issue to avoid losing it. On docstring coverage — that clarification makes sense; exported surface area is what matters for downstream consumers. (´• ω •`) ♡ ✏️ Learnings added
|
…lways on After unifying aliases and access overrides, the flag had a single functional consumer and was redundant: with the default MODELS_ENABLED_BY_DEFAULT=true and no override rules, an always-on override engine is allow-all — identical to the old disabled path. Removing it drops config surface and deletes the disabled path complexity added earlier (the overridesEnabled plumbing, nil guards, and the gated background refresh / runtime-refresh step). BREAKING CHANGE: MODEL_OVERRIDES_ENABLED is removed (the YAML key is ignored). The only behavior change is the self-contradictory combination MODELS_ENABLED_BY_DEFAULT=false + MODEL_OVERRIDES_ENABLED=false, which previously allowed all models (the disabled authorizer ignored the deny default) and now correctly enforces the allowlist (deny unless a user-path access override matches). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| aliasService := app.virtualModels.Service.Aliases() | ||
| modelOverrideService := app.virtualModels.Service.Overrides() |
There was a problem hiding this comment.
This now always wires the access-override engine into the gateway. Deployments that set MODEL_OVERRIDES_ENABLED=false or models.overrides_enabled: false used to get a nil authorizer and allow-all behavior, but the latest change removes that config and still returns app.virtualModels.Service.Overrides(). If an environment has persisted access policies, requests can now be denied even though the operator explicitly disabled model overrides.
Context Used: CLAUDE.md (source)
* feat(virtualmodels)!: unify engine, admin API, and dashboard UI 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> * fix(virtualmodels): address review — refresh cadence, seed safety, scoped 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> * fix(virtualmodels,dashboard): address coderabbit review 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> * fix(virtualmodels,server): address review — exposure scoping, enable 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> * feat(dashboard): unify enable/restrict/disable toggle across all access 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> * fix(virtualmodels): scope provider ListModels + atomic SQL seed 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> * refactor(dashboard,virtualmodels): unify refresh cadence, rename module, 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> * fix(virtualmodels): fresh rollback ctx for Postgres seed tx + mid-batch 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> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
Summary
Aliases (rename a new name → a real model) and access overrides (gate a scoped selector by
user_paths) were stored, served, and surfaced as two separate subsystems despite being the same operator concept. The split duplicateduser_pathscoping and DB-migration handling and was the root cause of recent bugs.This unifies them behind one entity — the virtual model — in a single
virtual_modelstable keyed bysource:targetsis a redirect (alias today; many targets + a strategy enable load balancing later),targetsis an access policy (scoped selector gated byuser_paths).Behavior is derived from
targets-presence — there is norolecolumn — and resolution stays staged: redirect runs early (resolver), the access gate runs late (authorizer), exactly as before.See
docs/adr/0008-virtual-models.md.Approach (low-risk composition)
To avoid re-porting subtle, tested resolution logic, the unified
virtualmodels.Servicecomposes the existingaliasesandmodeloverridesengines over the single store via role-partitioned adapters (redirectStore→aliases.Store,policyStore→modeloverrides.Store). The two engines run verbatim; only storage, wiring, and the service face are unified.seedFromLegacy) copies legacyaliases+model_overridesrows intovirtual_modelson first start when empty. MarkedREMOVE-LATER.targets[]+strategycolumns are present but inert → load balancing is a later additive change with no re-migration.app.gonet −14 lines: two subsystems collapsed into one.Behavior preservation
v1 preserves today's behavior exactly:
MODEL_OVERRIDES_ENABLED=falsestill yields a nil authorizer (allow-all)./admin/aliasesand/admin/model-overridesendpoints and the current dashboard keep working unchanged (fed the composed sub-services).Correctness guards
sourceis one namespace, upserting a policy whose selector equals an existing alias name (or vice-versa) is rejected with a validation error rather than silently overwriting the other row.Service.Upsert(v1) rather than silently dropped.Tests
New
internal/virtualmodelssuite: store round-trip (all fields), migration seed (copy + idempotency + source collision → redirect wins), unified resolution / access-gating / list+delete routing, and the two correctness guards. All existingaliases,modeloverrides,app,gateway,server,adminsuites pass unchanged.go build ./...✅ ·go test ./...✅ ·gofmt✅ ·golangci-lint0 issues ✅Follow-up (not in this PR)
The unified
/admin/virtual-modelsendpoint + single "Virtual Models" dashboard form is a frontend-focused change kept out of this PR to stay low-risk; the current UI already presents both on one page and is fully functional.🤖 Generated with Claude Code
Summary by CodeRabbit