feat(virtualmodels): load-balanced virtual models (round-robin + cost) with IaC config#433
Merged
Merged
Conversation
…t strategies Light up the multi-target redirect path the virtual_models store already persisted. A redirect with one target stays a plain alias; several targets are load balanced by strategy: - round_robin (default): rotate across targets, honoring per-target weight - cost: route to the cheapest catalog-priced available target, falling back to declared order when none are priced Unavailable targets are skipped, so a redirect serves while any target is live. Round-robin position lives on the Service and survives snapshot refreshes. Also adds infrastructure-as-code configuration: declare virtual models under virtual_models: in config.yaml or the VIRTUAL_MODELS env var (JSON; env merges over YAML per source). Declarative entries are validated at startup, override admin-store rows of the same source, and are read-only in the dashboard. The admin API and dashboard editor gain a multi-target list (model + weight) and a strategy selector; managed (config) virtual models render read-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughVirtual-model config now supports declarative YAML and environment overlays, multi-target redirects with routing strategies, managed overlay precedence, and dashboard read-only handling for config-owned entries. Audit logs now format redirected model names through a helper, and several hot-path perf ceilings were tightened. ChangesVirtual models feature
Audit log model display
Perf guard thresholds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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: 5
🤖 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/virtual-models.js`:
- Around line 709-729: The load-balanced alias editor is dropping the first
target’s weight because openVirtualModelEditAlias only keeps a qualified model
string for the primary target while discarding lbTargets[0].weight. Preserve the
primary target’s weight on vmForm (similar to how toggleAliasRow round-trips
target weights) and make submitVirtualModelForm reattach that weight when
building the payload, or add a primary-target weight field so it can be edited
and saved correctly.
In `@internal/admin/dashboard/templates/model-table-body.html`:
- Line 47: The “Config” badge in model-table-body.html is bound to row.managed,
but buildDisplayModels() only populates managed on alias rows, so real model
rows never show the badge even when their access override is config-managed.
Update the model-row creation path in buildDisplayModels() in virtual-models.js
to propagate the override’s managed flag onto real model rows as well, keeping
the existing rowIsManaged() behavior unchanged and ensuring the template’s
x-show="row.managed" renders consistently.
In `@internal/admin/handler_virtualmodels.go`:
- Around line 174-201: The target parsing in the virtual model handler is
incorrectly treating a non-empty req.Targets list with no usable entries as an
access policy, which silently drops redirect intent. In the logic that builds
targets from req.Targets and falls back to req.TargetModel, update the behavior
in the handler that returns virtualmodels.Targets so that if req.Targets is
present but all entries are blank/invalid after trimming and parsing, it returns
an invalid request error instead of nil/empty results. Use the existing
ParseModelSelector and NewInvalidRequestError flow to locate and report the
problem before the target_model fallback is considered.
In `@internal/virtualmodels/service.go`:
- Around line 110-119: The isManagedSource guard only trims source values, but
config.mergeVirtualModels keys sources using lowercased trimmed strings, so
casing differences can bypass ownership checks. Update Service.isManagedSource
to normalize source comparisons the same way as the config source keying logic,
and use the same normalization for model.Source before comparing so managed
sources are consistently detected during merge.
In `@internal/virtualmodels/validation.go`:
- Around line 47-49: The validation error in the multi-target handling inside
the validation logic uses the legacy field name instead of the per-entry target
field. Update the error text in the `target.Model == ""` check within the
`VirtualModel` validation path to reference each target’s `model` field (not
`target_model`), so the message matches the `targets[]` input shape and is clear
to callers.
🪄 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: f0b45b13-e7a8-430b-bce8-90d9c2d0ee40
📒 Files selected for processing (27)
.env.templateCLAUDE.mdconfig/config.example.yamlconfig/config.goconfig/virtualmodels.goconfig/virtualmodels_test.godocs/adr/0008-virtual-models.mddocs/features/virtual-models.mdxinternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/modules/dashboard-layout.test.cjsinternal/admin/dashboard/static/js/modules/virtual-models.jsinternal/admin/dashboard/static/js/modules/virtual-models.test.cjsinternal/admin/dashboard/templates/model-table-body.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/handler_virtualmodels.gointernal/admin/handler_virtualmodels_test.gointernal/virtualmodels/balancer.gointernal/virtualmodels/balancer_test.gointernal/virtualmodels/config.gointernal/virtualmodels/config_overlay_test.gointernal/virtualmodels/factory.gointernal/virtualmodels/resolve.gointernal/virtualmodels/service.gointernal/virtualmodels/service_test.gointernal/virtualmodels/snapshot.gointernal/virtualmodels/types.gointernal/virtualmodels/validation.go
Comment on lines
+110
to
+119
| // isManagedSource reports whether source is owned by a declarative config row. | ||
| func (s *Service) isManagedSource(source string) bool { | ||
| source = strings.TrimSpace(source) | ||
| for _, model := range s.configModels { | ||
| if strings.TrimSpace(model.Source) == source { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'func virtualModelKey|ToLower|TrimSpace' config/virtualmodels.go
rg -nP '\b(Source|source)\b.*(ToLower|TrimSpace)' internal/virtualmodels/*.go
ast-grep run --pattern 'strings.ToLower($_)' --lang go internal/virtualmodels/snapshot.goRepository: ENTERPILOT/GoModel
Length of output: 928
Normalize isManagedSource with the same keying as config sources. config.mergeVirtualModels lowercases and trims source keys, but this guard only trims. A managed source with different casing can slip past the ownership check and survive the merge.
🤖 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/service.go` around lines 110 - 119, The
isManagedSource guard only trims source values, but config.mergeVirtualModels
keys sources using lowercased trimmed strings, so casing differences can bypass
ownership checks. Update Service.isManagedSource to normalize source comparisons
the same way as the config source keying logic, and use the same normalization
for model.Source before comparing so managed sources are consistently detected
during merge.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Preserve the primary target's weight when editing a load-balanced redirect in the dashboard; add an editable weight field for it (was reset to 1 on save). - Show the "Config" badge for managed access policies on real model rows by keying it off rowIsManaged() instead of an alias-only flag. - Reject an admin upsert whose targets[] has no usable model instead of silently demoting the redirect to an access policy. - Clarify the empty-target validation message for the targets[] shape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed the review feedback in d6b9e84. Summary of each finding:
All Go tests, |
Source is the primary key on every store backend, so changing it now goes through a dedicated rename path instead of being locked in the UI. - Service.Rename(old, new): stores under the new source and deletes the old row with validation, refresh, and rollback; refuses to clobber an existing source; blocks managed (config-owned) rows. - Admin PUT accepts old_source and routes to Rename when it differs, preserving the enabled flag from the old row. - Dashboard unlocks Source for redirects/aliases and adds an "Edit redirect" action on model rows masked by a redirect, so a masking redirect can be renamed/repointed (model-access pencil hidden there). - Audit-log model pill shows "requested -> provider/resolved" when a redirect was applied, and widens to fit. - Prune round-robin counters on refresh so deleted/renamed aliases do not leak; validate declarative redirect targets on refresh. - Tighten hot-path perf guard ceilings to ~5% above the new baselines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/virtual-models.js (1)
1052-1067: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRoute masked-model toggles through the redirect path.
Masked concrete-model rows are now the only visible row for redirects that shadow a real model, but this branch still sends them to
toggleModelRow(). That payload is just{ source, enabled, user_paths }, and the backend treats a no-target upsert as a policy, so flipping the toggle can replace the redirect instead of enabling/disabling it.🤖 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/virtual-models.js` around lines 1052 - 1067, The toggle flow in toggleRowEnabled currently sends masked concrete-model rows into toggleModelRow(), which can overwrite a redirect policy instead of toggling it. Update the branching logic in toggleRowEnabled to detect masked-model rows and route them through the redirect/alias toggle path used by toggleAliasRow() or the existing redirect-handling helper, while keeping normal concrete models on toggleModelRow(). Ensure the decision uses the row’s identifying fields so masked rows are treated as redirects consistently.
🤖 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/virtual-models.js`:
- Around line 1439-1448: The rename collision check in the virtual-models flow
is using case-insensitive alias matching, which conflicts with the
case-sensitive source lookup used by the backend. Update the rename path in the
virtual model form logic so it checks collisions against the exact source string
only, using the existing source-based lookup in the rename branch instead of
findExistingAliasByName(), and keep the existing backend-override check
consistent with that exact-match behavior.
- Around line 681-682: The managed-delete check is only applied in
rowRedirectCanRemove(), so standalone alias rows can still be removed through
aliasRowCanRemove() even when alias.managed is true. Update the alias removal
path to use the same managed guard as redirect rows, and ensure both
rowRedirectCanRemove() and aliasRowCanRemove() block destructive actions for
managed entries.
In `@internal/admin/handler_virtualmodels.go`:
- Around line 156-163: The enabled-state carry-forward in the virtual model
update flow is using the raw request source before alias normalization, so
policy aliases and rename fallbacks can miss the stored row and overwrite a
disabled entry as enabled. Fix this in the VirtualModels handler path by
normalizing the lookup key before reading from h.virtualModels, or by moving the
Enabled carry-forward into Service.normalizeForUpsert so the lookup always uses
the canonical source; make sure both the source-based lookup and the old_source
fallback use the same normalized key.
In `@internal/virtualmodels/service.go`:
- Around line 299-323: The Rename flow in virtualmodels.Service is matching
oldSource too strictly against current.bySource, so policy-style selectors like
gpt-4o fail even when they should resolve to an existing canonical source.
Normalize oldSource with the same policy canonicalization used by
normalizePolicyInput, or resolve it against the snapshot before the lookup in
Rename, and keep the managed-source and exact-equality checks working on the
normalized value.
---
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/virtual-models.js`:
- Around line 1052-1067: The toggle flow in toggleRowEnabled currently sends
masked concrete-model rows into toggleModelRow(), which can overwrite a redirect
policy instead of toggling it. Update the branching logic in toggleRowEnabled to
detect masked-model rows and route them through the redirect/alias toggle path
used by toggleAliasRow() or the existing redirect-handling helper, while keeping
normal concrete models on toggleModelRow(). Ensure the decision uses the row’s
identifying fields so masked rows are treated as redirects consistently.
🪄 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: fdd7105d-2b4c-4625-ac26-f444a88ff816
📒 Files selected for processing (16)
internal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/dashboard-display.test.cjsinternal/admin/dashboard/static/js/modules/virtual-models.jsinternal/admin/dashboard/static/js/modules/virtual-models.test.cjsinternal/admin/dashboard/templates/model-table-body.htmlinternal/admin/dashboard/templates/page-audit-logs.htmlinternal/admin/dashboard/templates/page-models.htmlinternal/admin/handler_virtualmodels.gointernal/admin/handler_virtualmodels_test.gointernal/virtualmodels/balancer.gointernal/virtualmodels/balancer_test.gointernal/virtualmodels/config_overlay_test.gointernal/virtualmodels/rename_test.gointernal/virtualmodels/service.gotests/perf/hotpath_test.go
…ence - Add a "Rename or repoint a redirect" section covering the editable Source, one-step rename, the collision/shadow rules, and editing a shadowing redirect from the model row. - Regenerate the admin API reference (swag) so the virtual-model upsert body documents old_source alongside the load-balancing targets/strategy fields and the managed view flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…very refresh validateManagedRedirects ran inside refreshLocked, so the background refresh ticker re-applied the catalog-support gate to declarative config redirects on every tick. A transient provider-catalog gap (a target model briefly missing from the upstream list) would abort the snapshot swap and freeze every virtual model until the model reappeared. The declarative config set is installed once at startup and never changes, so move the check to an explicit ValidateManagedConfig() step the factory runs once after the initial Refresh. Background refreshes no longer validate, so an unavailable managed target is simply skipped at resolve time like any other redirect target and the snapshot keeps updating. Also simplify the write path: Upsert/Rename/Delete each hand-rolled the same store-write + refresh + rollback dance. Extract commitRefresh/restore so the rollback logic (and its rollback-failed wrapping) lives once, and each method ends in a single commitRefresh call. Fix two inaccurate balancer comments: weighted rotation is a plain rotation only when every weight is 1, and the cost fallback/tie-break is the first/earlier supported target, not the first declared one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/virtualmodels/config_overlay_test.go (1)
120-135: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an invalid
targets[]case.These cases only exercise
targetshorthand. Add at least one multi-target entry with one invalid/self/cross-redirect target so the per-target managed validation path is covered directly. As per coding guidelines,**/*_test.go: “Add or update tests for behavior changes”.🤖 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/config_overlay_test.go` around lines 120 - 135, The table-driven tests in config_overlay_test.go only cover the single-target shorthand path, so add at least one new case using the targets[] form with an invalid/self/cross-redirect entry to exercise the per-target managed validation logic directly. Update the existing VirtualModelConfig test set near the "self target", "unknown target", and "virtual model target" cases so the new case is validated alongside them. Make sure the added case asserts the expected failure for the invalid target while keeping the valid target behavior unchanged.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/virtualmodels/service.go`:
- Around line 477-488: The rollback logic in Service.restore stops on the first
store error, which can leave a multi-row rename only partially restored. Update
restore to iterate through every entry in prior and keep applying s.store.Delete
for nil rows and s.store.Upsert for non-nil rows even after an error, while
treating a not-found delete for an already-absent nil prior row as success. Use
the restore method and the s.store.Delete / s.store.Upsert calls as the key
places to change, and return an error only after all touched rows have been
attempted.
---
Outside diff comments:
In `@internal/virtualmodels/config_overlay_test.go`:
- Around line 120-135: The table-driven tests in config_overlay_test.go only
cover the single-target shorthand path, so add at least one new case using the
targets[] form with an invalid/self/cross-redirect entry to exercise the
per-target managed validation logic directly. Update the existing
VirtualModelConfig test set near the "self target", "unknown target", and
"virtual model target" cases so the new case is validated alongside them. Make
sure the added case asserts the expected failure for the invalid target while
keeping the valid target behavior unchanged.
🪄 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: cd2523c8-253a-45b7-8513-4965cce46617
📒 Files selected for processing (4)
internal/virtualmodels/balancer.gointernal/virtualmodels/config_overlay_test.gointernal/virtualmodels/factory.gointernal/virtualmodels/service.go
…naged UI - restore() is now best-effort: it attempts every touched row and joins the errors, and treats deleting an already-absent (nil-prior) row as success, so a failed two-row rename rollback never leaves the other row unrepaired. - The rename collision pre-check matches the source exactly (case-sensitive), mirroring the case-sensitive store key, so a case-only rename (e.g. "Smart" -> "smart") is no longer wrongly rejected as already existing. - aliasRowCanRemove() now hides the delete action for config-managed alias rows, matching rowRedirectCanRemove() and keeping managed entries read-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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/virtual-models.js (1)
1391-1393: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the editor delete path for managed virtual models.
submitVirtualModelFormblocks managed edits, butdeleteVirtualModel()can still issue aDELETEfor the same managed editor state. Add the same read-only guard before confirming/sending the request.Proposed fix
async deleteVirtualModel() { + if (this.vmFormManaged) { + this.vmFormError = 'This virtual model is managed by configuration and cannot be removed here.'; + return; + } const source = String(this.vmForm.source || this.vmFormOriginalSource || '').trim(); if (!source || !this.vmFormHasExisting) { return; }Also applies to: 1533-1551
🤖 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/virtual-models.js` around lines 1391 - 1393, The delete flow for managed virtual models is missing the same read-only guard used in submitVirtualModelForm. Update deleteVirtualModel() to check vmFormManaged before prompting for confirmation or issuing the DELETE request, and surface the same vmFormError message so managed editor state cannot be deleted from the form.
🤖 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/virtual-models.js`:
- Around line 1391-1393: The delete flow for managed virtual models is missing
the same read-only guard used in submitVirtualModelForm. Update
deleteVirtualModel() to check vmFormManaged before prompting for confirmation or
issuing the DELETE request, and surface the same vmFormError message so managed
editor state cannot be deleted from the form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a074c1c0-2a2b-4a94-b9ad-e75418747b17
📒 Files selected for processing (6)
internal/admin/dashboard/static/js/modules/virtual-models.jsinternal/admin/dashboard/static/js/modules/virtual-models.test.cjsinternal/virtualmodels/balancer.gointernal/virtualmodels/config_overlay_test.gointernal/virtualmodels/factory.gointernal/virtualmodels/service.go
submitVirtualModelForm already refuses to act on a managed (config-owned) virtual model, but deleteVirtualModel had no matching guard. The editor's Remove button is hidden for managed entries and the backend rejects managed deletes, so this is defense-in-depth/consistency: mirror the same vmFormManaged check and error message before issuing the DELETE. Extend the managed read-only test to cover the delete path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 (2)
internal/admin/dashboard/static/js/modules/virtual-models.js (2)
1039-1050: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t mark inherited policies as config-owned rows.
row.access.overridecan be inherited from a global/provider policy, buttoggleModelRow()would writerowAccessSelector(row), not that inherited source. This blocks legitimate child overrides under a managed parent. Check the managed policy for the row’s own selector instead.Suggested fix
- return Boolean((row.access && row.access.override && row.access.override.managed) - || (row.masking_alias && row.masking_alias.managed)); + const selector = this.rowAccessSelector(row); + const ownPolicy = selector ? this.findModelOverrideView(selector) : null; + return Boolean((ownPolicy && ownPolicy.managed) + || (row.masking_alias && row.masking_alias.managed));🤖 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/virtual-models.js` around lines 1039 - 1050, The row ownership check in rowIsManaged is too broad because row.access.override may come from an inherited global/provider policy rather than the row’s own selector. Update rowIsManaged to determine managed status from the row’s actual selector source, matching what toggleModelRow and rowAccessSelector operate on, and keep alias/masking_alias handling unchanged. Use the existing rowIsManaged and rowAccessSelector logic in virtual-models.js to ensure only config-owned rows are marked managed.
1088-1092: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStrip weights when toggling cost-balanced aliases too.
The save path removes weights for
strategy === 'cost', but the toggle path writes them back. That makes enable/disable a data-shape-changing write path for cost balancers and conflicts with the form serialization contract. Update the toggle payload and the test expectation around Line 1600.Suggested fix
if (lbTargets.length > 1) { - payload.targets = lbTargets.map((target) => - this.targetEntry(this.qualifyTarget(target), target.weight)); payload.strategy = alias.strategy || 'round_robin'; + payload.targets = payload.strategy === 'cost' + ? lbTargets.map((target) => ({ model: this.qualifyTarget(target) })) + : lbTargets.map((target) => + this.targetEntry(this.qualifyTarget(target), target.weight));🤖 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/virtual-models.js` around lines 1088 - 1092, The toggle path in the alias save flow is still writing target weights back for cost-balanced aliases, which conflicts with the serialization behavior used elsewhere. Update the payload-building logic around the alias target handling in virtual-models.js so that the toggle path strips weights when alias.strategy is cost, matching the existing save-path contract. Also adjust the related test expectation near the alias toggle test so it asserts weight-less targets for cost-balanced aliases.
🤖 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/virtual-models.js`:
- Around line 1039-1050: The row ownership check in rowIsManaged is too broad
because row.access.override may come from an inherited global/provider policy
rather than the row’s own selector. Update rowIsManaged to determine managed
status from the row’s actual selector source, matching what toggleModelRow and
rowAccessSelector operate on, and keep alias/masking_alias handling unchanged.
Use the existing rowIsManaged and rowAccessSelector logic in virtual-models.js
to ensure only config-owned rows are marked managed.
- Around line 1088-1092: The toggle path in the alias save flow is still writing
target weights back for cost-balanced aliases, which conflicts with the
serialization behavior used elsewhere. Update the payload-building logic around
the alias target handling in virtual-models.js so that the toggle path strips
weights when alias.strategy is cost, matching the existing save-path contract.
Also adjust the related test expectation near the alias toggle test so it
asserts weight-less targets for cost-balanced aliases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 878c22bb-0532-48b8-a7f1-e40618617b14
📒 Files selected for processing (2)
internal/admin/dashboard/static/js/modules/virtual-models.jsinternal/admin/dashboard/static/js/modules/virtual-models.test.cjs
…aliases The editor save path drops per-target weights for the cost strategy (weight only biases round-robin), but the enable/disable toggle round-tripped them back, making toggle a data-shape-changing write that conflicts with the save serialization contract. Apply the same cost->weight-less rule in toggleAliasRow. Update the toggle test that encoded the old behavior and add a round-robin variant asserting weights are still preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SantiagoDePolonia
added a commit
that referenced
this pull request
Jun 29, 2026
) * fix(virtualmodels): don't gate IaC startup on catalog availability Declarative virtual models (VIRTUAL_MODELS / config.yaml) were validated at startup against the model catalog, which loads asynchronously and is empty on a cold cache. ValidateManagedConfig required every managed redirect target to be catalog-supported, so a brand-new deployment with valid IaC virtual models and no warm cache aborted startup with "target model not found". Separate the two concerns the check conflated: - Structural invariants (valid selector, no self-/cross-redirect target) are pure properties of the declaration, so they still fail startup loudly (validateRedirectStructure). - Catalog availability is runtime state, already handled by skipping unavailable targets at resolve time, so it is no longer a startup gate. The admin write path keeps it (firstUnsupportedTarget) since it runs against a warm catalog and an unknown target there is a caller mistake. A managed redirect with an unknown target now boots and is simply unavailable until the catalog provides it, consistent with the resolve-time skip and the background refresh that already tolerates a transient catalog gap. Tests: cold-catalog startup no longer aborts and resolves once warm; admin upsert still rejects an unsupported target; structural invalids still abort. Adds the standalone tests/e2e/test-iac-virtualmodels.sh IaC harness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(e2e): add release scenarios for load balancing, throughput, and cache analytics Cover the features shipped since v0.1.44 that the matrix had no behavioral coverage of (only the single-target alias path via S13/S24/S31): - S118-S125 load-balanced virtual models (#433): round-robin, weighted, cost, rename via old_source, plus negatives (unknown strategy, unknown target, rename of a non-existent source). - S126-S128 token throughput (#434): window shape across granularities, negative granularity handling, and live-traffic reflection. - S129-S132 cache analytics (#428): cache_mode on the usage summary, cache overview availability gating, and locally-cached token accounting from an exact-cache hit. Each scenario is self-contained ($QA_SUFFIX-scoped, cleans up after itself) and validated through run-release-e2e.sh. IaC virtual-model behavior needs gateways booted with custom config, so it lives in the standalone tests/e2e/test-iac-virtualmodels.sh harness instead of this running-stack matrix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
Virtual model redirects can now point at multiple targets and load balance across them. The
virtual_modelsstore already persistedtargets/strategy/weight(anticipated by ADR 0008) but validation rejected multi-target redirects — this lights up that path end-to-end.A redirect with one target stays a plain alias (fully backward compatible). With several targets it load balances by strategy:
round_robin(default) — rotate across targets, honoring per-targetweight(weight 2 = double the share).cost— route to the cheapest catalog-priced available target (input + output per-Mtok), falling back to declared order when none are priced.Targets the gateway cannot currently serve are skipped, so a redirect keeps working while any one target is live. Round-robin position lives on the
Serviceand survives the periodic snapshot refresh.Infrastructure-as-code
Virtual models can be declared declaratively, for operators who manage config as code:
virtual_models:inconfig.yaml, orVIRTUAL_MODELSenv var (a JSON array; env merges over YAML, winning persource).Declarative entries are validated at startup (bad config fails fast), override admin-store rows with the same
source, and are read-only in the dashboard (aConfigbadge). No DB schema change — they're an in-memory managed overlay.User-visible changes
PUT /admin/virtual-models) acceptstargets[]+strategy; the singletarget_modelfield still works as shorthand.GET /v1/modelsstill lists a load-balanced redirect as one model (representative available target).Scope / design notes
round_robin,cost. Unknown strategy / missing / self-referential targets are rejected with clear errors.Tests
go test ./...,go vet,golangci-lint,make test-race, and the dashboard JS suite all pass.Docs
docs/features/virtual-models.mdx,.env.template,config/config.example.yaml,CLAUDE.md, and ADR 0008 updated.🤖 Generated with Claude Code
Summary by CodeRabbit
config.yamlor viaVIRTUAL_MODELS, including multi-target redirect load balancing (round-robin with optional weights, or lowest-cost routing).old_source, plus multi-target updates usingtargetsandstrategy.