diff --git a/cmd/gomodel/docs/docs.go b/cmd/gomodel/docs/docs.go index 89295320..4f03af8e 100644 --- a/cmd/gomodel/docs/docs.go +++ b/cmd/gomodel/docs/docs.go @@ -2835,6 +2835,9 @@ const docTemplate = `{ "$ref": "#/definitions/virtualmodels.View" } }, + "204": { + "description": "No-op access policy removed" + }, "400": { "description": "Bad Request", "schema": { diff --git a/docs/features/virtual-models.mdx b/docs/features/virtual-models.mdx index 55548b8e..14259ac9 100644 --- a/docs/features/virtual-models.mdx +++ b/docs/features/virtual-models.mdx @@ -27,6 +27,13 @@ Create and manage both in the admin dashboard at `Models -> New virtual model`. Fill **Target model** to make a redirect; leave it empty to make an access policy on the **Source** selector. +GoModel does not persist empty, ineffective access policies, whether saved from +the dashboard or through the admin API. If a saved policy has no target, user +paths, or description and its enabled state matches the inherited/default +access state, GoModel removes the stored row. It retains an otherwise empty +policy when that row still overrides a disabled default or an inherited +user-path restriction. + The rest of this page covers redirects. ## Use stable names diff --git a/docs/openapi.json b/docs/openapi.json index 7e4130fe..4fffc7be 100644 --- a/docs/openapi.json +++ b/docs/openapi.json @@ -3761,6 +3761,9 @@ } } }, + "204": { + "description": "No-op access policy removed" + }, "400": { "description": "Bad Request", "content": { diff --git a/internal/admin/dashboard/static/css/dashboard.css b/internal/admin/dashboard/static/css/dashboard.css index 419d33fe..efbf80e8 100644 --- a/internal/admin/dashboard/static/css/dashboard.css +++ b/internal/admin/dashboard/static/css/dashboard.css @@ -2260,6 +2260,26 @@ td.col-price { color: var(--text-muted); } +.model-redirect-remove-btn { + appearance: none; + margin-left: 4px; + padding: 0; + border: 0; + background: none; + color: var(--danger); + font-size: 11px; + cursor: pointer; +} + +.model-redirect-remove-btn:hover:not(:disabled) { + text-decoration: underline; +} + +.model-redirect-remove-btn:disabled { + opacity: 0.45; + cursor: default; +} + .provider-group-row td { background: color-mix(in srgb, var(--accent) 6%, var(--bg)); padding-top: 12px; @@ -2324,6 +2344,24 @@ td.col-price { border-color: color-mix(in srgb, var(--accent) 55%, var(--border)); } +.model-kind-icon { + display: inline-flex; + align-items: center; + justify-content: center; + width: 24px; + height: 24px; + flex: 0 0 24px; + border: 1px solid color-mix(in srgb, var(--accent) 55%, var(--border)); + border-radius: 999px; + background: var(--bg); + color: var(--accent); +} + +.model-kind-icon-svg { + width: 14px; + height: 14px; +} + .inline-link { padding: 0; border: none; @@ -6567,6 +6605,16 @@ body.conversation-drawer-open { max-width: 220px; } +.auth-key-actions-cell { + white-space: nowrap; +} + +.auth-key-row-actions { + display: inline-flex; + align-items: center; + gap: 6px; +} + .auth-key-status-badge { display: inline-block; padding: 2px 10px; diff --git a/internal/admin/dashboard/static/js/modules/dashboard-layout.test.cjs b/internal/admin/dashboard/static/js/modules/dashboard-layout.test.cjs index cfdbccc6..6b7e55f3 100644 --- a/internal/admin/dashboard/static/js/modules/dashboard-layout.test.cjs +++ b/internal/admin/dashboard/static/js/modules/dashboard-layout.test.cjs @@ -624,6 +624,34 @@ test("auth key expirations render as a UTC date with the full UTC timestamp in t assert.match(paginationBtnWithIconRule, /gap:\s*8px/); }); +test("auth key row actions stay inline and use titled icon buttons", () => { + const indexTemplate = readDashboardTemplateSource(); + const css = readFixture("../../css/dashboard.css"); + const powerIconTemplate = readFixture("../../../templates/power-icon.html"); + + assert.match(indexTemplate, /<\/th>/); + assert.match( + indexTemplate, + /class="auth-key-actions-cell"[\s\S]*class="auth-key-row-actions"[\s\S]*class="table-action-btn table-icon-btn"[\s\S]*:aria-label="'Edit labels for API key ' \+ key\.name"[\s\S]*:title="'Edit labels for API key ' \+ key\.name"[\s\S]*@click="openAuthKeyLabelsEditor\(key\)"[\s\S]*{{template "edit-icon"}}/, + ); + assert.match( + indexTemplate, + /class="table-action-btn table-action-btn-danger table-icon-btn"[\s\S]*:disabled="authKeyDeactivatingID === key\.id"[\s\S]*:aria-label="authKeyDeactivatingID === key\.id \? 'Deactivating API key ' \+ key\.name : 'Deactivate API key ' \+ key\.name"[\s\S]*:title="authKeyDeactivatingID === key\.id \? 'Deactivating API key ' \+ key\.name : 'Deactivate API key ' \+ key\.name"[\s\S]*@click="deactivateAuthKey\(key\)"[\s\S]*{{template "power-icon"}}/, + ); + assert.match( + powerIconTemplate, + /{{define "power-icon"}}[\s\S]*class="table-icon-svg"[\s\S]*<\/path>[\s\S]*{{end}}/, + ); + + const actionCellRule = readCSSRule(css, ".auth-key-actions-cell"); + assert.match(actionCellRule, /white-space:\s*nowrap/); + + const rowActionsRule = readCSSRule(css, ".auth-key-row-actions"); + assert.match(rowActionsRule, /display:\s*inline-flex/); + assert.match(rowActionsRule, /align-items:\s*center/); + assert.match(rowActionsRule, /gap:\s*6px/); +}); + test("workflow guardrail warning links directly to the top-level guardrails page", () => { const indexTemplate = readDashboardTemplateSource(); @@ -1251,7 +1279,7 @@ test("alias rows use a shared icon-only edit action", () => { assert.match( modelTableTemplate, - /class="table-action-btn table-action-btn-danger table-icon-btn"[\s\S]*x-show="virtualModelsAvailable && aliasRowCanRemove\(row\)"[\s\S]*@click="removeAliasRow\(row\)"[\s\S]*{{template "trash-icon"}}[\s\S]*class="table-action-btn table-icon-btn"[\s\S]*:aria-label="'Edit alias ' \+ row\.alias\.name"[\s\S]*@click="openVirtualModelEditAlias\(row\.alias\)"[\s\S]*{{template "edit-icon"}}/, + /class="table-action-btn table-action-btn-danger table-icon-btn"[\s\S]*x-show="virtualModelsAvailable && aliasRowCanRemove\(row\)"[\s\S]*@click="removeAliasRow\(row\)"[\s\S]*{{template "trash-icon"}}[\s\S]*class="table-action-btn table-icon-btn table-action-btn-active"[\s\S]*:aria-label="'Edit alias ' \+ row\.alias\.name"[\s\S]*@click="openVirtualModelEditAlias\(row\.alias\)"[\s\S]*{{template "edit-icon"}}/, ); assert.match( modelTableTemplate, @@ -1263,12 +1291,31 @@ test("alias rows use a shared icon-only edit action", () => { ); assert.match( modelTableTemplate, - /Redirects to <\/span>/, + /Redirects to <\/span>[\s\S]*class="model-redirect-remove-btn mono"[\s\S]*x-show="virtualModelsAvailable && rowRedirectCanRemove\(row\)"[\s\S]*@click="removeRedirectRow\(row\)">\[remove\]<\/button>/, + ); + assert.match( + modelTableTemplate, + /class="model-kind-icon" x-show="row\.is_alias"[\s\S]*aria-label="Virtual model"[\s\S]*class="model-kind-icon-svg"/, + ); + assert.match( + modelTableTemplate, + /class="model-kind-icon" x-show="!row\.is_alias && row\.masking_alias"[\s\S]*aria-label="Redirect"[\s\S]*class="model-kind-icon-svg"/, ); assert.match( modelTableTemplate, - /x-show="modelPricingOverridesAvailable"[\s\S]*@click="openModelPricingOverrideEdit\(row\)"[\s\S]*{{template "dollar-icon"}}[\s\S]*class="table-action-btn table-action-btn-danger table-icon-btn"[\s\S]*x-show="virtualModelsAvailable && rowRedirectCanRemove\(row\)"[\s\S]*@click="removeRedirectRow\(row\)"[\s\S]*{{template "trash-icon"}}/, + /x-show="modelPricingOverridesAvailable"[\s\S]*@click="openModelPricingOverrideEdit\(row\)"[\s\S]*{{template "dollar-icon"}}/, + ); + assert.match( + modelTableTemplate, + /@click="openRateLimitInspectorForModel\(row\)"[\s\S]*{{template "gauge-icon"}}[\s\S]*class="table-action-btn table-icon-btn table-action-btn-active"[\s\S]*x-show="virtualModelsAvailable && row\.masking_alias && row\.masking_alias\.name"[\s\S]*@click="openVirtualModelEditAlias\(row\.masking_alias\)"[\s\S]*{{template "edit-icon"}}/, + ); + assert.doesNotMatch( + modelTableTemplate, + /class="table-action-btn table-action-btn-danger table-icon-btn"[\s\S]*@click="removeRedirectRow\(row\)"/, ); + const redirectRemoveRule = readCSSRule(css, ".model-redirect-remove-btn"); + assert.match(redirectRemoveRule, /font-size:\s*11px/); + assert.match(redirectRemoveRule, /color:\s*var\(--danger\)/); assert.doesNotMatch(css, /\.data-table tr\.masked-model-row td/); assert.match(indexTemplate, /{{template "model-table-body" \.}}/); assert.match( diff --git a/internal/admin/dashboard/static/js/modules/virtual-models.js b/internal/admin/dashboard/static/js/modules/virtual-models.js index 37229fcc..91d642c1 100644 --- a/internal/admin/dashboard/static/js/modules/virtual-models.js +++ b/internal/admin/dashboard/static/js/modules/virtual-models.js @@ -80,7 +80,6 @@ is_alias: false, alias: null, access, - kind_badge: '', masking_alias: maskingAlias, has_virtual_model: Boolean(maskingAlias || (access && access.override)), alias_state_class: '', @@ -117,7 +116,6 @@ is_alias: true, alias, access: null, - kind_badge: 'Virtual Model', masking_alias: null, source_model_exists: Boolean(sourceModel), has_virtual_model: true, @@ -488,14 +486,19 @@ } } + const virtualRows = []; const groups = new Map(); for (const row of rows) { + if (row && row.is_alias) { + virtualRows.push(row); + continue; + } const providerName = String(row && row.provider_name || '').trim(); const providerType = String(row && row.provider_type || '').trim(); - const key = providerName || providerType || 'unassigned'; + const key = 'provider-group:' + (providerName || providerType || 'unassigned'); if (!groups.has(key)) { groups.set(key, { - key: 'provider-group:' + key, + key, provider_name: providerName, provider_type: providerType, display_name: this.providerGroupDisplayName(providerName, providerType), @@ -516,7 +519,7 @@ group.rows.push(row); } - return Array.from(groups.values()) + const result = Array.from(groups.values()) .map((group) => { const access = this.providerGroupAccess(group.provider_name, group.provider_type, overridesBySelector); return { @@ -527,6 +530,21 @@ }; }) .sort((a, b) => String(a.display_name || '').localeCompare(String(b.display_name || ''))); + if (virtualRows.length > 0) { + result.unshift({ + key: 'virtual-model-group', + is_virtual_models: true, + provider_name: '', + provider_type: '', + display_name: 'Virtual models', + type_label: '', + rows: virtualRows, + access: { selector: '' }, + access_summary: '', + item_count_label: this.providerGroupItemCountLabel(virtualRows) + }); + } + return result; }, providerGroupDisplayName(providerName, providerType) { @@ -735,19 +753,6 @@ return classes.join(' '); }, - // rowVirtualBadge returns the small badge label shown on a real model row - // that carries a virtual model (empty for plain rows and alias rows, which - // already show their own Virtual Model badge). - rowVirtualBadge(row) { - if (!row || row.is_alias || !row.has_virtual_model) { - return ''; - } - if (row.masking_alias) { - return 'Redirect'; - } - return 'Override'; - }, - aliasRowCanRemove(row) { return Boolean(row && row.is_alias && row.alias && row.alias.name && !row.alias.managed); }, @@ -1281,36 +1286,59 @@ if (!source) { return; } - await this.removeVirtualModelSource(source, row.key, 'Remove the virtual model alias "' + source + '"?'); + await this.mutateVirtualModelRow({ + rowKey: row.key, + confirmMessage: 'Remove the virtual model alias "' + source + '"?', + method: 'DELETE', + payload: { source }, + operation: 'virtual model', + failureMessage: 'Failed to remove virtual model.', + notice: 'Virtual model removed.', + ignoreNotFound: true + }); }, async removeRedirectRow(row) { if (!this.rowRedirectCanRemove(row) || this.rowDeletingKey) { return; } - const source = String(row.masking_alias.name || '').trim(); + const alias = row.masking_alias; + const source = String(alias.name || '').trim(); if (!source) { return; } - await this.removeVirtualModelSource(source, row.key, 'Remove the redirect for "' + source + '"?'); + await this.mutateVirtualModelRow({ + rowKey: row.key, + confirmMessage: 'Remove the redirect for "' + source + '"? Other virtual model settings will be preserved.', + method: 'PUT', + payload: { + source, + user_paths: Array.isArray(alias.user_paths) ? alias.user_paths : [], + description: String(alias.description || '').trim(), + enabled: alias.enabled !== false + }, + operation: 'virtual model redirect', + failureMessage: 'Failed to remove redirect.', + notice: 'Redirect removed. Other virtual model settings were preserved.' + }); }, - async removeVirtualModelSource(source, rowKey, confirmMessage) { + async mutateVirtualModelRow(options) { if (this.rowDeletingKey) { return; } - if (!this.confirmAction(confirmMessage)) { + if (!this.confirmAction(options.confirmMessage)) { return; } - this.rowDeletingKey = rowKey; + this.rowDeletingKey = options.rowKey; this.aliasError = ''; this.aliasNotice = ''; try { const request = this.adminRequestOptions({ - method: 'DELETE', - body: JSON.stringify({ source }) + method: options.method, + body: JSON.stringify(options.payload) }); const res = await fetch('/admin/virtual-models', request); if (res.status === 503) { @@ -1318,15 +1346,15 @@ this.aliasError = 'Virtual models feature is unavailable.'; return; } - if (res.status !== 404) { - const handled = this.handleFetchResponse(res, 'virtual model', request); + if (!(options.ignoreNotFound && res.status === 404)) { + const handled = this.handleFetchResponse(res, options.operation, request); if (typeof this.isStaleAuthFetchResult === 'function' && this.isStaleAuthFetchResult(handled)) { return; } if (!handled) { this.aliasError = res.status === 401 ? 'Authentication required.' - : await this.aliasResponseMessage(res, 'Failed to remove virtual model.'); + : await this.aliasResponseMessage(res, options.failureMessage); return; } } @@ -1334,10 +1362,10 @@ await Promise.all([this.fetchModels(), this.fetchVirtualModels()]); this.syncDisplayModels(); - this.aliasNotice = 'Virtual model removed.'; + this.aliasNotice = options.notice; } catch (e) { - console.error('Failed to delete virtual model:', e); - this.aliasError = 'Failed to remove virtual model.'; + console.error(options.failureMessage, e); + this.aliasError = options.failureMessage; } finally { this.rowDeletingKey = ''; } @@ -1593,12 +1621,15 @@ : await this.aliasResponseMessage(res, 'Failed to save virtual model.'); return; } + const policyPruned = !isRedirect && res.status === 204; this.setVirtualModelsAvailable(true); await Promise.all([this.fetchModels(), this.fetchVirtualModels()]); this.syncDisplayModels(); this.closeVirtualModelForm(); - this.aliasNotice = isRedirect ? 'Alias saved.' : 'Model access saved.'; + this.aliasNotice = isRedirect + ? 'Alias saved.' + : (policyPruned ? 'Model access reset to inherited/default.' : 'Model access saved.'); } catch (e) { console.error('Failed to save virtual model:', e); this.vmFormError = 'Failed to save virtual model.'; diff --git a/internal/admin/dashboard/static/js/modules/virtual-models.test.cjs b/internal/admin/dashboard/static/js/modules/virtual-models.test.cjs index 8af1f0d2..6229e223 100644 --- a/internal/admin/dashboard/static/js/modules/virtual-models.test.cjs +++ b/internal/admin/dashboard/static/js/modules/virtual-models.test.cjs @@ -798,6 +798,33 @@ test('submitVirtualModelForm sends a policy payload when target_model is empty', assert.equal(Object.prototype.hasOwnProperty.call(body, 'target_model'), false); }); +test('submitVirtualModelForm reports when an empty policy is pruned', async() => { + const module = createAliasesModule({ + context: { + fetch: async() => ({ ok: true, status: 204, json: async() => ({}) }) + } + }); + stubRequests(module); + module.fetchModels = async() => {}; + module.fetchVirtualModels = async() => {}; + module.syncDisplayModels = () => {}; + module.vmFormOpen = true; + module.vmFormMode = 'edit'; + module.vmForm = { + source: 'openai/gpt-4o', + target_model: '', + targets: [], + user_paths: '', + description: '', + enabled: true + }; + + await module.submitVirtualModelForm(); + + assert.equal(module.vmFormOpen, false); + assert.equal(module.aliasNotice, 'Model access reset to inherited/default.'); +}); + test('deleteVirtualModel removes the editor source', async() => { const requests = []; const module = createAliasesModule({ @@ -832,7 +859,6 @@ test('displayRowClass renders real models carrying a virtual model as alias-like access: { effective_enabled: true } }; assert.equal(module.displayRowClass(overrideRow), 'alias-row is-valid'); - assert.equal(module.rowVirtualBadge(overrideRow), 'Override'); const redirectRow = { is_alias: false, @@ -841,16 +867,13 @@ test('displayRowClass renders real models carrying a virtual model as alias-like access: { effective_enabled: true } }; assert.equal(module.displayRowClass(redirectRow), 'alias-row is-valid masked-model-row'); - assert.equal(module.rowVirtualBadge(redirectRow), 'Redirect'); assert.equal(module.rowRedirectCanRemove(redirectRow), true); const plainRow = { is_alias: false, has_virtual_model: false, access: { effective_enabled: true } }; assert.equal(module.displayRowClass(plainRow), ''); - assert.equal(module.rowVirtualBadge(plainRow), ''); const aliasRow = { is_alias: true, alias: { enabled: true, valid: true } }; assert.equal(module.displayRowClass(aliasRow), 'alias-row is-valid'); - assert.equal(module.rowVirtualBadge(aliasRow), ''); assert.equal(module.aliasRowCanRemove({ is_alias: true, source_model_exists: true, @@ -939,16 +962,13 @@ test('buildDisplayModels combines source-backed redirects with the concrete mode const nestedTargetAlias = module.displayModels.find((row) => row.key === 'alias:anthropic/claude-fable-5'); const nestedTargetModel = module.displayModels.find((row) => row.key === 'model:openrouter/anthropic/claude-fable-5'); - assert.equal(aliasOnly.kind_badge, 'Virtual Model'); assert.equal(aliasOnly.source_model_exists, false); assert.equal(module.aliasRowCanRemove(aliasOnly), true); assert.equal(modelBackedAliasRow, undefined); assert.equal(modelBacked.masking_alias.name, 'gpt-4o'); - assert.equal(module.rowVirtualBadge(modelBacked), 'Redirect'); assert.equal(module.rowRedirectCanRemove(modelBacked), true); assert.equal(nestedTargetAlias, undefined); assert.equal(nestedTargetModel.masking_alias.name, 'anthropic/claude-fable-5'); - assert.equal(module.rowVirtualBadge(nestedTargetModel), 'Redirect'); assert.equal(module.rowRedirectCanRemove(nestedTargetModel), true); // The masking redirect on a real-model row is editable through the alias @@ -1002,7 +1022,7 @@ test('removeAliasRow confirms and deletes an alias-only virtual model', async() assert.equal(module.rowDeletingKey, ''); }); -test('removeRedirectRow confirms and deletes a source-backed redirect', async() => { +test('removeRedirectRow confirms and removes only a source-backed redirect', async() => { const requests = []; const confirms = []; const fetched = []; @@ -1029,16 +1049,28 @@ test('removeRedirectRow confirms and deletes a source-backed redirect', async() key: 'model:openai/gpt-4o', is_alias: false, display_name: 'openai/gpt-4o', - masking_alias: { name: 'openai/gpt-4o' } + masking_alias: { + name: 'openai/gpt-4o', + description: 'Team model', + user_paths: ['/team'], + enabled: true + } }); - assert.deepEqual(confirms, ['Remove the redirect for "openai/gpt-4o"?']); + assert.deepEqual(confirms, [ + 'Remove the redirect for "openai/gpt-4o"? Other virtual model settings will be preserved.' + ]); assert.equal(requests.length, 1); assert.equal(requests[0].url, '/admin/virtual-models'); - assert.equal(requests[0].request.method, 'DELETE'); - assert.deepEqual(JSON.parse(requests[0].request.body), { source: 'openai/gpt-4o' }); + assert.equal(requests[0].request.method, 'PUT'); + assert.deepEqual(JSON.parse(requests[0].request.body), { + source: 'openai/gpt-4o', + user_paths: ['/team'], + description: 'Team model', + enabled: true + }); assert.deepEqual(fetched, ['models', 'virtual', 'sync']); - assert.equal(module.aliasNotice, 'Virtual model removed.'); + assert.equal(module.aliasNotice, 'Redirect removed. Other virtual model settings were preserved.'); assert.equal(module.rowDeletingKey, ''); }); @@ -1154,6 +1186,56 @@ test('filteredDisplayModelGroups groups rows by provider_name and applies provid assert.deepEqual(Array.from(backup.access.user_paths), ['/non-existing']); }); +test('filteredDisplayModelGroups keeps alias-only virtual models in a first group', () => { + const module = createAliasesModule(); + module.models = [ + { + provider_name: 'alpha', + provider_type: 'test', + selector: 'alpha/model-a', + model: { id: 'model-a', object: 'model' } + }, + { + provider_name: 'zulu', + provider_type: 'test', + selector: 'zulu/model-z', + model: { id: 'model-z', object: 'model' } + } + ]; + module.aliases = [ + { + name: 'smart', + target_provider: 'zulu', + target_model: 'model-z', + enabled: true, + valid: true + }, + { + name: 'model-a', + target_provider: 'zulu', + target_model: 'model-z', + enabled: true, + valid: true + } + ]; + module.virtualModelsAvailable = true; + module.modelFilter = ''; + module.activeCategory = 'all'; + module.syncDisplayModels(); + + const groups = module.filteredDisplayModelGroups; + + assert.equal(groups[0].key, 'virtual-model-group'); + assert.equal(groups[0].display_name, 'Virtual models'); + assert.deepEqual(Array.from(groups[0].rows, (row) => row.display_name), ['smart']); + assert.equal(groups[0].provider_name, ''); + assert.equal(groups[0].access.selector, ''); + assert.deepEqual(Array.from(groups, (group) => group.display_name), ['Virtual models', 'alpha', 'zulu']); + + const overriddenModel = groups[1].rows.find((row) => row.display_name === 'alpha/model-a'); + assert.equal(overriddenModel.masking_alias.name, 'model-a'); +}); + test('filteredDisplayModelGroups lets provider-wide overrides replace global paths', () => { const module = createAliasesModule(); module.models = [ diff --git a/internal/admin/dashboard/templates/model-table-body.html b/internal/admin/dashboard/templates/model-table-body.html index 0f42affa..696566c3 100644 --- a/internal/admin/dashboard/templates/model-table-body.html +++ b/internal/admin/dashboard/templates/model-table-body.html @@ -50,8 +50,18 @@
- - + + + + + + Config
@@ -59,6 +69,12 @@
Redirects to +
@@ -85,7 +101,7 @@ @click="removeAliasRow(row)"> {{template "trash-icon"}} - - - + - + +
+ + +
diff --git a/internal/admin/dashboard/templates/power-icon.html b/internal/admin/dashboard/templates/power-icon.html new file mode 100644 index 00000000..4817b24e --- /dev/null +++ b/internal/admin/dashboard/templates/power-icon.html @@ -0,0 +1,6 @@ +{{define "power-icon"}} + +{{end}} diff --git a/internal/admin/handler_virtualmodels.go b/internal/admin/handler_virtualmodels.go index 44d7a9d9..33d7c02f 100644 --- a/internal/admin/handler_virtualmodels.go +++ b/internal/admin/handler_virtualmodels.go @@ -71,6 +71,7 @@ func (h *Handler) ListVirtualModels(c *echo.Context) error { // @Security BearerAuth // @Param virtual_model body upsertVirtualModelRequest true "Virtual model definition" // @Success 200 {object} virtualmodels.View +// @Success 204 "No-op access policy removed" // @Failure 400 {object} core.GatewayError // @Failure 401 {object} core.GatewayError // @Failure 502 {object} core.GatewayError diff --git a/internal/admin/handler_virtualmodels_test.go b/internal/admin/handler_virtualmodels_test.go index d6c3bbff..e1686957 100644 --- a/internal/admin/handler_virtualmodels_test.go +++ b/internal/admin/handler_virtualmodels_test.go @@ -149,6 +149,7 @@ func newVMHandler(t *testing.T, items ...virtualmodels.VirtualModel) *Handler { t.Helper() catalog := newVMTestCatalog() catalog.add("openai/gpt-4o", "openai") + catalog.add("openai/gpt-4o-mini", "openai") service := newVMService(t, catalog, newVMTestStore(items...), true) return NewHandler(nil, nil, WithVirtualModels(service)) } @@ -262,6 +263,63 @@ func TestUpsertAndDeleteRedirectVirtualModel(t *testing.T) { } } +func TestUpsertVirtualModelRemovesRedirectAndPreservesPolicyFields(t *testing.T) { + h := newVMHandler(t) + e := echo.New() + + putBody := `{"source":"gpt-4o","target_model":"openai/gpt-4o","description":"Team model","user_paths":["/team"],"enabled":true}` + putReq := httptest.NewRequest(http.MethodPut, "/admin/virtual-models", bytes.NewBufferString(putBody)) + putReq.Header.Set("Content-Type", "application/json") + putRec := httptest.NewRecorder() + if err := h.UpsertVirtualModel(e.NewContext(putReq, putRec)); err != nil { + t.Fatalf("UpsertVirtualModel() error = %v", err) + } + + policyBody := `{"source":"gpt-4o","description":"Team model","user_paths":["/team"],"enabled":true}` + policyReq := httptest.NewRequest(http.MethodPut, "/admin/virtual-models", bytes.NewBufferString(policyBody)) + policyReq.Header.Set("Content-Type", "application/json") + policyRec := httptest.NewRecorder() + if err := h.UpsertVirtualModel(e.NewContext(policyReq, policyRec)); err != nil { + t.Fatalf("UpsertVirtualModel(policy) error = %v", err) + } + if policyRec.Code != http.StatusOK { + t.Fatalf("policy status = %d, want 200 body=%s", policyRec.Code, policyRec.Body.String()) + } + + vm, ok := h.virtualModels.Get("gpt-4o") + if !ok { + t.Fatal("policy replacement removed the virtual model") + } + if vm.IsRedirect() { + t.Fatalf("policy replacement left targets %#v", vm.Targets) + } + if vm.Description != "Team model" || !vm.Enabled { + t.Fatalf("policy replacement changed fields: %#v", vm) + } + if len(vm.UserPaths) != 1 || vm.UserPaths[0] != "/team" { + t.Fatalf("policy replacement user paths = %#v, want [/team]", vm.UserPaths) + } +} + +func TestUpsertVirtualModelEmptyEditDropsNoopRecord(t *testing.T) { + h := newVMHandler(t, redirectVM("gpt-4o", "openai/gpt-4o", true)) + e := echo.New() + + putBody := `{"source":"gpt-4o","enabled":true}` + putReq := httptest.NewRequest(http.MethodPut, "/admin/virtual-models", bytes.NewBufferString(putBody)) + putReq.Header.Set("Content-Type", "application/json") + putRec := httptest.NewRecorder() + if err := h.UpsertVirtualModel(e.NewContext(putReq, putRec)); err != nil { + t.Fatalf("UpsertVirtualModel(empty edit) error = %v", err) + } + if putRec.Code != http.StatusNoContent { + t.Fatalf("put status = %d, want 204 body=%s", putRec.Code, putRec.Body.String()) + } + if _, ok := h.virtualModels.Get("gpt-4o"); ok { + t.Fatal("empty edit retained a no-op virtual model") + } +} + func TestUpsertVirtualModelRenamesViaOldSource(t *testing.T) { h := newVMHandler(t, redirectVM("smart", "openai/gpt-4o", false)) e := echo.New() @@ -341,6 +399,41 @@ func TestUpsertPolicyVirtualModelAcceptsEmptyUserPaths(t *testing.T) { } } +func TestUpsertRedirectVirtualModelReplacesAccessPolicy(t *testing.T) { + h := newVMHandler(t, virtualmodels.VirtualModel{ + Source: "openai/gpt-4o", + ProviderName: "openai", + Model: "gpt-4o", + UserPaths: []string{"/team"}, + Enabled: true, + }) + e := echo.New() + + body := `{"source":"openai/gpt-4o","target_model":"openai/gpt-4o-mini","description":"fallback","enabled":true}` + req := httptest.NewRequest(http.MethodPut, "/admin/virtual-models", bytes.NewBufferString(body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + if err := h.UpsertVirtualModel(e.NewContext(req, rec)); err != nil { + t.Fatalf("UpsertVirtualModel(policy to redirect) error = %v", err) + } + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 body=%s", rec.Code, rec.Body.String()) + } + var view virtualmodels.View + if err := json.Unmarshal(rec.Body.Bytes(), &view); err != nil { + t.Fatalf("decode response: %v", err) + } + if view.Kind != virtualmodels.KindRedirect || len(view.Targets) != 1 { + t.Fatalf("view = %#v, want one-target redirect", view) + } + if len(view.UserPaths) != 0 { + t.Fatalf("view.UserPaths = %v, want none after full replacement", view.UserPaths) + } + if got := view.Targets[0].Provider + "/" + view.Targets[0].Model; got != "openai/gpt-4o-mini" { + t.Fatalf("target = %q, want openai/gpt-4o-mini", got) + } +} + func TestUpsertVirtualModelPreservesEnabledWhenOmitted(t *testing.T) { h := newVMHandler(t, redirectVM("smart", "openai/gpt-4o", false)) e := echo.New() diff --git a/internal/virtualmodels/rename_test.go b/internal/virtualmodels/rename_test.go index 5f06284a..f65a6ec0 100644 --- a/internal/virtualmodels/rename_test.go +++ b/internal/virtualmodels/rename_test.go @@ -77,6 +77,29 @@ func TestService_RenamePreservesDisabledState(t *testing.T) { } } +func TestService_RenameToNoopPolicyDeletesOldSource(t *testing.T) { + t.Parallel() + svc := newTestService(t) + ctx := context.Background() + + if err := svc.Upsert(ctx, VirtualModel{ + Source: "fast", + Targets: []Target{{Provider: "openai", Model: "gpt-4o"}}, + Enabled: true, + }); err != nil { + t.Fatalf("Upsert(redirect) error = %v", err) + } + if err := svc.Rename(ctx, "fast", VirtualModel{Source: "gpt-4o", Enabled: true}); err != nil { + t.Fatalf("Rename(no-op policy) error = %v", err) + } + if _, ok := svc.Get("fast"); ok { + t.Fatal("Rename(no-op policy) retained the old source") + } + if _, ok := svc.Get("gpt-4o"); ok { + t.Fatal("Rename(no-op policy) stored the new no-op source") + } +} + func TestService_RenameRejectsExistingTarget(t *testing.T) { t.Parallel() svc := newTestService(t) diff --git a/internal/virtualmodels/service.go b/internal/virtualmodels/service.go index b0c7caa0..70566e93 100644 --- a/internal/virtualmodels/service.go +++ b/internal/virtualmodels/service.go @@ -11,6 +11,7 @@ import ( "time" "github.com/enterpilot/gomodel/internal/core" + "github.com/enterpilot/gomodel/internal/modelselectors" ) // Service is the single native engine over the virtual_models store. It serves @@ -266,8 +267,10 @@ func (s *Service) redirectViewResolution(vm VirtualModel) (resolved, providerTyp return resolved, providerType, valid } -// Upsert validates and stores one virtual model, then refreshes the in-memory -// snapshot with rollback on refresh failure. +// Upsert validates and stores one virtual model, replacing any existing row at +// the same source even when its kind changes. A policy with no metadata whose +// access state is identical to what it would inherit is deleted instead of +// persisting a redundant row. func (s *Service) Upsert(ctx context.Context, vm VirtualModel) error { if s == nil { return fmt.Errorf("virtual models service is required") @@ -285,17 +288,44 @@ func (s *Service) Upsert(ctx context.Context, vm VirtualModel) error { defer s.refreshMu.Unlock() current := s.snapshot() - if err := s.ensureSourceKind(current, normalized.Source, normalized.IsRedirect()); err != nil { + previous, canonical, existed := current.lookupCanonicalSource(normalized.Source) + if existed { + normalized.Source = canonical + if normalized.CreatedAt.IsZero() { + normalized.CreatedAt = previous.CreatedAt + } + if previous.Managed { + return managedSourceError(canonical) + } + } + + baseRows := current.rows() + if existed { + baseRows = removeRow(baseRows, canonical) + } + redundant, err := s.policyIsRedundant(normalized, baseRows) + if err != nil { return err } + if redundant { + if !existed { + return nil + } + if err := s.store.Delete(ctx, canonical); err != nil { + if errors.Is(err, ErrNotFound) { + return ErrNotFound + } + return fmt.Errorf("delete redundant virtual model policy: %w", err) + } + return s.commitRefresh(ctx, map[string]*VirtualModel{canonical: &previous}) + } if err := s.validateRedirectTarget(current, normalized); err != nil { return err } - if _, err := buildSnapshot(upsertRow(current.rows(), normalized), s.defaultEnabled); err != nil { + if _, err := buildSnapshot(upsertRow(baseRows, normalized), s.defaultEnabled); err != nil { return fmt.Errorf("validate virtual models: %w", err) } - previous, existed := current.bySource[normalized.Source] if err := s.store.Upsert(ctx, normalized); err != nil { return fmt.Errorf("upsert virtual model: %w", err) } @@ -342,10 +372,24 @@ func (s *Service) Rename(ctx context.Context, oldSource string, vm VirtualModel) if _, taken := current.bySource[normalized.Source]; taken { return newValidationError(fmt.Sprintf("virtual model %q already exists; choose a different source", normalized.Source), nil) } + baseRows := removeRow(current.rows(), oldSource) + redundant, err := s.policyIsRedundant(normalized, baseRows) + if err != nil { + return err + } + if redundant { + if err := s.store.Delete(ctx, oldSource); err != nil { + if errors.Is(err, ErrNotFound) { + return ErrNotFound + } + return fmt.Errorf("delete no-op renamed policy: %w", err) + } + return s.commitRefresh(ctx, map[string]*VirtualModel{oldSource: &previous}) + } if err := s.validateRedirectTarget(current, normalized); err != nil { return err } - rows := upsertRow(removeRow(current.rows(), oldSource), normalized) + rows := upsertRow(baseRows, normalized) if _, err := buildSnapshot(rows, s.defaultEnabled); err != nil { return fmt.Errorf("validate virtual models: %w", err) } @@ -404,25 +448,63 @@ func (s *Service) Delete(ctx context.Context, source string) error { return s.commitRefresh(ctx, map[string]*VirtualModel{source: &previous}) } +func (s *Service) policyIsRedundant(policy VirtualModel, fallbackRows []VirtualModel) (bool, error) { + if policy.IsRedirect() { + return false, nil + } + fallback, err := buildSnapshot(fallbackRows, s.defaultEnabled) + if err != nil { + return false, fmt.Errorf("validate inherited virtual models: %w", err) + } + return policyIsNoop(policy, fallback), nil +} + +// policyIsNoop reports whether an otherwise empty policy changes effective +// access compared with the snapshot that would remain without that policy. +func policyIsNoop(policy VirtualModel, fallback snapshot) bool { + if policy.IsRedirect() || len(policy.UserPaths) > 0 || strings.TrimSpace(policy.Description) != "" { + return false + } + + matches := func(enabled bool, userPaths []string) bool { + return policy.Enabled == enabled && len(userPaths) == 0 + } + selector := core.ModelSelector{Provider: policy.ProviderName, Model: policy.Model} + + switch scopeKindFor(policy.Source, policy.ProviderName, policy.Model) { + case modelselectors.ScopeGlobal: + return matches(fallback.defaultEnable, nil) + case modelselectors.ScopeProvider: + state := fallback.effectiveState(selector) + if !matches(state.Enabled, state.UserPaths) { + return false + } + // Provider-wide policies outrank model-wide policies. Removing one can + // therefore expose any model-wide rule for this provider. + for _, modelPolicy := range fallback.modelWide { + if !matches(modelPolicy.Enabled, modelPolicy.UserPaths) { + return false + } + } + return true + default: + state := fallback.effectiveState(selector) + return matches(state.Enabled, state.UserPaths) + } +} + func (s *Service) normalizeForUpsert(vm VirtualModel) (VirtualModel, error) { if vm.IsRedirect() { normalized, _, err := normalizeRedirect(vm) return normalized, err } - return normalizePolicyInput(s.catalog, vm) -} - -// ensureSourceKind rejects an upsert that would clobber an existing row of the -// other kind. Source is a single namespace. -func (s *Service) ensureSourceKind(current snapshot, source string, wantRedirect bool) error { - existing, ok := current.bySource[source] - if !ok { - return nil - } - if existing.IsRedirect() == wantRedirect { - return nil + normalized, err := normalizePolicyInput(s.catalog, vm) + if err != nil { + return VirtualModel{}, err } - return crossKindError(source, wantRedirect) + normalized.Strategy = "" + normalized.Description = strings.TrimSpace(normalized.Description) + return normalized, nil } // validateRedirectTarget enforces redirect rules for an admin write: the diff --git a/internal/virtualmodels/service_test.go b/internal/virtualmodels/service_test.go index 26b855c7..ab267f1d 100644 --- a/internal/virtualmodels/service_test.go +++ b/internal/virtualmodels/service_test.go @@ -110,30 +110,35 @@ func TestService_EnabledPolicyEmptyUserPathsAllowsAll(t *testing.T) { } } -func TestService_RejectsCrossKindClobber(t *testing.T) { +func TestService_UpsertReplacesRedirectWithPolicy(t *testing.T) { t.Parallel() svc := newTestService(t) ctx := context.Background() - if err := svc.Upsert(ctx, VirtualModel{Source: "gpt-fast", Targets: []Target{{Provider: "openai", Model: "gpt-4o"}}, Enabled: true}); err != nil { + if err := svc.Upsert(ctx, VirtualModel{ + Source: "gpt-fast", + Targets: []Target{{Provider: "openai", Model: "gpt-4o"}}, + Description: "Team model", + UserPaths: []string{"/team"}, + Enabled: true, + }); err != nil { t.Fatalf("Upsert(redirect) error = %v", err) } - // A policy with the same source must be rejected, not silently clobber it. - err := svc.Upsert(ctx, VirtualModel{Source: "gpt-fast", UserPaths: []string{"/team"}}) - if err == nil { - t.Fatalf("Upsert(policy over redirect) error = nil, want rejection") - } - if !IsValidationError(err) { - t.Fatalf("Upsert(policy over redirect) error = %v, want validation error", err) + if err := svc.Upsert(ctx, VirtualModel{ + Source: "gpt-fast", + Description: "Team model", + UserPaths: []string{"/team"}, + Enabled: true, + }); err != nil { + t.Fatalf("Upsert(policy over redirect) error = %v", err) } - // The redirect must survive intact. - got, getErr := svc.store.Get(ctx, "gpt-fast") - if getErr != nil { - t.Fatalf("store.Get() error = %v", getErr) + got, ok := svc.Get("gpt-fast") + if !ok { + t.Fatal("Get(gpt-fast) = missing") } - if !got.IsRedirect() { - t.Fatalf("redirect was clobbered: %#v", got) + if got.IsRedirect() || got.Description != "Team model" || len(got.UserPaths) != 1 || got.UserPaths[0] != "/team" { + t.Fatalf("replacement policy = %#v", got) } } @@ -494,6 +499,85 @@ func TestService_DeleteMissingReturnsErrNotFound(t *testing.T) { } } +func TestService_UpsertPrunesOnlyRedundantPolicies(t *testing.T) { + t.Parallel() + ctx := context.Background() + + t.Run("matching default", func(t *testing.T) { + svc := newTestService(t) + if err := svc.Upsert(ctx, VirtualModel{ + Source: "openai/gpt-4o", + Description: "temporary note", + Enabled: true, + }); err != nil { + t.Fatalf("Upsert(existing policy) error = %v", err) + } + if err := svc.Upsert(ctx, VirtualModel{Source: "openai/gpt-4o", Enabled: true}); err != nil { + t.Fatalf("Upsert() error = %v", err) + } + if _, ok := svc.Get("openai/gpt-4o"); ok { + t.Fatal("Upsert() stored a policy matching the default") + } + }) + + t.Run("different from default", func(t *testing.T) { + svc, err := NewService(newSQLiteVMStore(t), testCatalog(), false) + if err != nil { + t.Fatalf("NewService() error = %v", err) + } + if err := svc.Upsert(ctx, VirtualModel{Source: "openai/gpt-4o", Enabled: true}); err != nil { + t.Fatalf("Upsert() error = %v", err) + } + if _, ok := svc.Get("openai/gpt-4o"); !ok { + t.Fatal("Upsert() dropped an explicit enable over a disabled default") + } + }) + + t.Run("overrides inherited paths", func(t *testing.T) { + svc := newTestService(t) + if err := svc.Upsert(ctx, VirtualModel{Source: "/", UserPaths: []string{"/team"}, Enabled: true}); err != nil { + t.Fatalf("Upsert(global policy) error = %v", err) + } + if err := svc.Upsert(ctx, VirtualModel{Source: "openai/gpt-4o", Enabled: true}); err != nil { + t.Fatalf("Upsert() error = %v", err) + } + if _, ok := svc.Get("openai/gpt-4o"); !ok { + t.Fatal("Upsert() dropped an allow-all policy overriding inherited paths") + } + }) +} + +func TestService_UpsertReplacesPolicyWithRedirect(t *testing.T) { + t.Parallel() + catalog := testCatalog() + catalog.supported["openai/gpt-4o-mini"] = core.Model{ID: "openai/gpt-4o-mini", Object: "model", OwnedBy: "openai"} + svc, err := NewService(newSQLiteVMStore(t), catalog, true) + if err != nil { + t.Fatalf("NewService() error = %v", err) + } + ctx := context.Background() + if err := svc.Upsert(ctx, VirtualModel{ + Source: "openai/gpt-4o", + UserPaths: []string{"/team"}, + Enabled: true, + }); err != nil { + t.Fatalf("Upsert(policy) error = %v", err) + } + + err = svc.Upsert(ctx, VirtualModel{ + Source: "openai/gpt-4o", + Targets: []Target{{Provider: "openai", Model: "gpt-4o-mini"}}, + Enabled: true, + }) + if err != nil { + t.Fatalf("Upsert(redirect over policy) error = %v", err) + } + vm, ok := svc.Get("openai/gpt-4o") + if !ok || !vm.IsRedirect() { + t.Fatalf("Upsert() result = %#v, found=%v; want redirect", vm, ok) + } +} + func TestService_ResolveUpsertEnabled(t *testing.T) { t.Parallel() svc := newTestService(t) diff --git a/internal/virtualmodels/validation.go b/internal/virtualmodels/validation.go index a91b0120..b6f24cbf 100644 --- a/internal/virtualmodels/validation.go +++ b/internal/virtualmodels/validation.go @@ -177,11 +177,3 @@ func selectorString(providerName, model string) string { func scopeKindFor(selector, providerName, model string) modelselectors.ScopeKind { return modelselectors.ScopeKindFor(selector, providerName, model) } - -func crossKindError(source string, wantRedirect bool) error { - other := "an access policy" - if !wantRedirect { - other = "an alias" - } - return newValidationError(fmt.Sprintf("source %q is already used by %s", source, other), nil) -}