diff --git a/.github/skills/policy-and-managed-settings/github-managed-settings.md b/.github/skills/policy-and-managed-settings/github-managed-settings.md index b61ded7183a50..b69ae015e51bc 100644 --- a/.github/skills/policy-and-managed-settings/github-managed-settings.md +++ b/.github/skills/policy-and-managed-settings/github-managed-settings.md @@ -82,6 +82,8 @@ the schema's nested | Schema property (path) | Type in schema | Composition (`x-composition.strategy`) | |------------------------|----------------|----------------------------------------| | `permissions.disableBypassPermissionsMode` | string enum `"disable"` | most-restrictive-wins (sticky once set) | +| `model` | string (`auto`, a model family name, or a full model id) | — | +| `permissions.model` | string (legacy location for `model`) | — | | `forceRemoteSettingsRefresh` | boolean | MDM wins; controls the server cache rather than a configuration setting | | `enabledPlugins` | `{ "PLUGIN@MARKETPLACE": boolean }` | deny-wins (false beats true; enterprise denials immutable) | | `extraKnownMarketplaces` | `{ name: { source, autoUpdate? } }`, source `github` \| `git` \| `directory` | most-restrictive-wins (higher layer is the complete allowlist); explicit `autoUpdate` overrides the client's global plugin auto-update setting for that marketplace | @@ -112,6 +114,15 @@ Note the schema's `x-composition` describes the **server/runtime** layering acro enterprise/org/user. Inside VS Code the bag has already been collapsed to a single projected `ManagedSettingsData` before a `policy.value()` callback ever sees it. +> **Multi-key precedence (`model`).** The channel merge in `pickManagedSettings` only resolves the +> *same* key across delivery channels; it does not know that top-level `model` supersedes the legacy +> nested `permissions.model`. That cross-key precedence is resolved in the policy's `value()` +> callback (`managedModelValue` in `copilotManagedSettings.ts`), which reads the top-level key first +> and falls back to the legacy key (treating a blank value as unset). Because it is key-level, a +> non-empty top-level `model` wins even when `permissions.model` was supplied by a +> higher-precedence channel. The `ChatDefaultModel` policy declares **both** keys in its +> `managedSettings` so native MDM watches each and projection keeps them. + ## Declaring a managed setting on a policy A policy that should be driven by a managed-settings key declares two things on its @@ -234,6 +245,8 @@ Constants (also in `copilotManagedSettings.ts`): | `GITHUB_COPILOT_WIN32_POLICY_NAME` | `GitHubCopilot` (productName for the watcher) | | `GITHUB_COPILOT_MACOS_BUNDLE_ID` | `com.github.copilot` (CFPreferences app id) | | `COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY` | `permissions.disableBypassPermissionsMode` | +| `COPILOT_TOP_LEVEL_MODEL_KEY` | `model` (canonical; wins over the legacy nested key) | +| `COPILOT_MODEL_KEY` | `permissions.model` (legacy; retained for original-schema deployments) | | `COPILOT_ENABLED_PLUGINS_KEY` | `enabledPlugins` | | `COPILOT_EXTRA_MARKETPLACES_KEY` | `extraKnownMarketplaces` | | `COPILOT_STRICT_MARKETPLACES_KEY` | `strictKnownMarketplaces` | diff --git a/src/vs/platform/policy/common/copilotManagedSettings.ts b/src/vs/platform/policy/common/copilotManagedSettings.ts index 92fcd5c71469e..c8c981ae19757 100644 --- a/src/vs/platform/policy/common/copilotManagedSettings.ts +++ b/src/vs/platform/policy/common/copilotManagedSettings.ts @@ -73,13 +73,18 @@ export const COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_CONFIG = 'chat.mcp.allowMana export const COPILOT_ALLOW_MANAGED_HOOKS_ONLY_CONFIG = 'chat.hooks.allowManagedOnly'; /** - * Managed-settings key for the default chat model (carried as a plain string: `auto`, a model - * family name, or a full model id). Nested under `permissions` in the managed-settings schema - * (alongside {@link COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY}), so it flattens to the dot-path - * `permissions.model` in the normalized bag — the key policy `value()` callbacks must read. + * Legacy managed-settings key for the default chat model, nested under `permissions` so it flattens + * to `permissions.model`. Retained for original-schema deployments; superseded by the top-level + * {@link COPILOT_TOP_LEVEL_MODEL_KEY}, which wins when both are present (see {@link managedModelValue}). */ export const COPILOT_MODEL_KEY = 'permissions.model'; +/** + * Canonical top-level managed-settings key for the default chat model (flattens to the bag key + * `model`). Supersedes the legacy nested {@link COPILOT_MODEL_KEY} when both are present. + */ +export const COPILOT_TOP_LEVEL_MODEL_KEY = 'model'; + /** * Enterprise OTel managed-settings keys. These are the scalar leaves of the canonical * `telemetry` block from the cross-client managed-settings schema (see the CLI @@ -149,24 +154,24 @@ export function shouldForceRemoteSettingsRefresh(nativeMdm: ManagedSettingsData let managedModelValueCallback: ((policyData: IPolicyData) => ManagedSettingValue | undefined) | undefined; +/** Trim a managed-settings model value, treating a blank/whitespace-only string as unset. */ +function normalizeModelValue(value: ManagedSettingValue | undefined): string | undefined { + const trimmed = typeof value === 'string' ? value.trim() : undefined; + return trimmed ? trimmed : undefined; +} + /** - * `value` callback for the default-chat-model managed setting ({@link COPILOT_MODEL_KEY}). Like - * {@link managedSettingValue} it locks the setting to the managed value and otherwise falls through - * to the user's own value, but it additionally trims the string and treats a blank/whitespace-only - * value as "unset" (returns `undefined`) — an admin clearing the field must not lock the setting to - * an empty string. The model-specific normalization lives here, alongside the other managed-settings - * handling, rather than inline at the policy declaration, so every managed-settings control is wired - * the same way. - * - * Memoized (single key) so repeated calls return the SAME function reference, matching the - * reference-identity contract {@link managedSettingValue} relies on for `isSamePolicyDefinition`. + * `value` callback for the default-chat-model managed setting: resolves the top-level + * {@link COPILOT_TOP_LEVEL_MODEL_KEY} first, falling back to the legacy nested {@link COPILOT_MODEL_KEY} + * (each trimmed, blank treated as unset), so the top-level value wins when both are present. Memoized + * so repeated calls return the same reference, matching the identity contract {@link managedSettingValue} + * relies on for `isSamePolicyDefinition`. */ export function managedModelValue(): (policyData: IPolicyData) => ManagedSettingValue | undefined { if (!managedModelValueCallback) { managedModelValueCallback = policyData => { - const model = policyData.managedSettings?.[COPILOT_MODEL_KEY]; - const trimmed = typeof model === 'string' ? model.trim() : undefined; - return trimmed ? trimmed : undefined; + const topLevel = normalizeModelValue(policyData.managedSettings?.[COPILOT_TOP_LEVEL_MODEL_KEY]); + return topLevel ?? normalizeModelValue(policyData.managedSettings?.[COPILOT_MODEL_KEY]); }; } return managedModelValueCallback; diff --git a/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts b/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts index 41d3260f8610c..864fa4986268c 100644 --- a/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts +++ b/src/vs/platform/policy/test/common/copilotManagedSettings.test.ts @@ -7,7 +7,7 @@ import assert from 'assert'; import { IStringDictionary } from '../../../../base/common/collections.js'; import { IPolicyData } from '../../../../base/common/defaultAccount.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; -import { collectManagedSettingsDefinitions, COPILOT_FORCE_REMOTE_SETTINGS_REFRESH_KEY, hasManagedSettingsDefinitions, managedSettingValue, projectManagedSettings, pickManagedSettings, shouldForceRemoteSettingsRefresh } from '../../common/copilotManagedSettings.js'; +import { collectManagedSettingsDefinitions, COPILOT_FORCE_REMOTE_SETTINGS_REFRESH_KEY, COPILOT_MODEL_KEY, COPILOT_TOP_LEVEL_MODEL_KEY, hasManagedSettingsDefinitions, managedModelValue, managedSettingValue, projectManagedSettings, pickManagedSettings, shouldForceRemoteSettingsRefresh } from '../../common/copilotManagedSettings.js'; import { PolicyDefinition } from '../../common/policy.js'; suite('Copilot managed settings projection', () => { @@ -74,6 +74,38 @@ suite('Copilot managed settings projection', () => { ); }); + test('managedModelValue prefers the top-level key, falls back to the legacy nested key', () => { + const value = managedModelValue(); + assert.deepStrictEqual( + { + bothPresent: value({ managedSettings: { [COPILOT_TOP_LEVEL_MODEL_KEY]: 'opus', [COPILOT_MODEL_KEY]: 'gemini' } } as IPolicyData), + topLevelOnly: value({ managedSettings: { [COPILOT_TOP_LEVEL_MODEL_KEY]: 'opus' } } as IPolicyData), + legacyOnly: value({ managedSettings: { [COPILOT_MODEL_KEY]: 'gemini' } } as IPolicyData), + neither: value({ managedSettings: { 'other.key': 'x' } } as IPolicyData), + noBag: value({} as IPolicyData), + }, + { bothPresent: 'opus', topLevelOnly: 'opus', legacyOnly: 'gemini', neither: undefined, noBag: undefined }, + ); + }); + + test('managedModelValue trims values and treats a blank top-level value as unset (falls through to legacy)', () => { + const value = managedModelValue(); + assert.deepStrictEqual( + { + trimsTopLevel: value({ managedSettings: { [COPILOT_TOP_LEVEL_MODEL_KEY]: ' opus ' } } as IPolicyData), + trimsLegacy: value({ managedSettings: { [COPILOT_MODEL_KEY]: ' gemini ' } } as IPolicyData), + blankTopLevelFallsBack: value({ managedSettings: { [COPILOT_TOP_LEVEL_MODEL_KEY]: ' ', [COPILOT_MODEL_KEY]: 'gemini' } } as IPolicyData), + bothBlank: value({ managedSettings: { [COPILOT_TOP_LEVEL_MODEL_KEY]: ' ', [COPILOT_MODEL_KEY]: ' ' } } as IPolicyData), + nonString: value({ managedSettings: { [COPILOT_TOP_LEVEL_MODEL_KEY]: 42 } } as IPolicyData), + }, + { trimsTopLevel: 'opus', trimsLegacy: 'gemini', blankTopLevelFallsBack: 'gemini', bothBlank: undefined, nonString: undefined }, + ); + }); + + test('managedModelValue returns the same memoized callback (stable reference identity)', () => { + assert.strictEqual(managedModelValue(), managedModelValue()); + }); + test('forceRemoteSettingsRefresh uses native MDM over the cached server value', () => { assert.deepStrictEqual({ serverTrue: shouldForceRemoteSettingsRefresh(undefined, { [COPILOT_FORCE_REMOTE_SETTINGS_REFRESH_KEY]: true }), diff --git a/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts b/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts index c519bd4723dad..b21aba0b8330c 100644 --- a/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts +++ b/src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts @@ -15,7 +15,7 @@ import { InMemoryFileSystemProvider } from '../../../files/common/inMemoryFilesy import { NullLogService } from '../../../log/common/log.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../base/test/common/utils.js'; import { runWithFakedTimers } from '../../../../base/test/common/timeTravelScheduler.js'; -import { COPILOT_ALLOW_MANAGED_HOOKS_ONLY_KEY, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_KEY, COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY, COPILOT_ENABLED_PLUGINS_KEY, COPILOT_EXTRA_MARKETPLACES_KEY, COPILOT_MODEL_KEY, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY, managedModelValue, normalizeManagedSettings, RawManagedSettingsData } from '../../common/copilotManagedSettings.js'; +import { COPILOT_ALLOW_MANAGED_HOOKS_ONLY_KEY, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_KEY, COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY, COPILOT_ENABLED_PLUGINS_KEY, COPILOT_EXTRA_MARKETPLACES_KEY, COPILOT_MODEL_KEY, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY, COPILOT_TOP_LEVEL_MODEL_KEY, managedModelValue, normalizeManagedSettings, RawManagedSettingsData } from '../../common/copilotManagedSettings.js'; import { FileManagedSettingsService } from '../../common/fileManagedSettingsService.js'; import { FileManagedSettingsChannelClient } from '../../common/fileManagedSettingsIpc.js'; @@ -129,6 +129,29 @@ suite('normalizeManagedSettings', () => { assert.strictEqual(managedModelValue()({ managedSettings: result }), 'auto'); }); + test('carries the top-level model setting as the `model` bag key', () => { + const result = normalizeManagedSettings({ + model: 'auto' + }); + assert.deepStrictEqual(result, { + 'model': 'auto' + }); + assert.strictEqual(COPILOT_TOP_LEVEL_MODEL_KEY, 'model'); + assert.strictEqual(managedModelValue()({ managedSettings: result }), 'auto'); + }); + + test('keeps top-level and legacy model keys distinct, with the top-level value winning', () => { + const result = normalizeManagedSettings({ + model: 'opus', + permissions: { model: 'gemini' } + }); + assert.deepStrictEqual(result, { + 'model': 'opus', + 'permissions.model': 'gemini' + }); + assert.strictEqual(managedModelValue()({ managedSettings: result }), 'opus'); + }); + test('handles empty object', () => { assert.deepStrictEqual(normalizeManagedSettings({}), {}); }); diff --git a/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts b/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts index 14dc8575ce0f5..2c18149c9f716 100644 --- a/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts +++ b/src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts @@ -19,7 +19,7 @@ import { AgentHostMapLegacySettingsToManagedSettingsSettingId } from '../../../. import { DEFAULT_LOCAL_TRANSCRIPTION_MODEL } from '../../../../platform/localTranscription/common/localTranscription.js'; import { AgentNetworkFilterService, IAgentNetworkFilterService } from '../../../../platform/networkFilter/common/networkFilterService.js'; import { AgentNetworkDomainSettingId } from '../../../../platform/networkFilter/common/settings.js'; -import { COPILOT_ALLOWED_MCP_SERVERS_KEY, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_KEY, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_KEY, COPILOT_DENIED_MCP_SERVERS_KEY, COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY, COPILOT_ENABLED_PLUGINS_KEY, COPILOT_EXTRA_MARKETPLACES_KEY, COPILOT_MODEL_KEY, COPILOT_STRICT_MARKETPLACES_KEY, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY, managedModelValue, managedSettingValue } from '../../../../platform/policy/common/copilotManagedSettings.js'; +import { COPILOT_ALLOWED_MCP_SERVERS_KEY, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_HOOKS_ONLY_KEY, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_CONFIG, COPILOT_ALLOW_MANAGED_MCP_SERVERS_ONLY_KEY, COPILOT_DENIED_MCP_SERVERS_KEY, COPILOT_DISABLE_BYPASS_PERMISSIONS_MODE_KEY, COPILOT_ENABLED_PLUGINS_KEY, COPILOT_EXTRA_MARKETPLACES_KEY, COPILOT_MODEL_KEY, COPILOT_STRICT_MARKETPLACES_KEY, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_CONFIG, COPILOT_STRICT_PLUGIN_ONLY_CUSTOMIZATION_KEY, COPILOT_TOP_LEVEL_MODEL_KEY, managedModelValue, managedSettingValue } from '../../../../platform/policy/common/copilotManagedSettings.js'; import { AgentSandboxEnabledValue, AgentSandboxSettingId } from '../../../../platform/sandbox/common/settings.js'; import { ChatSessionArchiveActionWordingSettingId } from '../../../../platform/chat/common/sessionArchiveActions.js'; import { registerEditorFeature } from '../../../../editor/common/editorFeatures.js'; @@ -661,6 +661,7 @@ configurationRegistry.registerConfiguration({ value: managedModelValue(), managedSettings: { [COPILOT_MODEL_KEY]: { type: 'string' }, + [COPILOT_TOP_LEVEL_MODEL_KEY]: { type: 'string' }, }, localization: { description: { diff --git a/src/vs/workbench/services/accounts/browser/managedSettings.ts b/src/vs/workbench/services/accounts/browser/managedSettings.ts index f36a506e68709..0ba88214bf378 100644 --- a/src/vs/workbench/services/accounts/browser/managedSettings.ts +++ b/src/vs/workbench/services/accounts/browser/managedSettings.ts @@ -30,7 +30,18 @@ export type IManagedMcpServerMatcher = export interface IManagedSettingsResponse { readonly permissions?: { readonly disableBypassPermissionsMode?: string; + /** + * Legacy location for the default chat model. Retained for deployments authored against + * the original schema; the top-level {@link IManagedSettingsResponse.model} wins when both + * are present. + */ + readonly model?: string; }; + /** + * Default chat model (`auto`, a model family name, or a full model id). Canonical top-level + * location in the current schema; supersedes the legacy nested `permissions.model`. + */ + readonly model?: string; readonly enabledPlugins?: Record; readonly extraKnownMarketplaces?: Record