chat: enforce managed settings client compatibility - #329688
Conversation
Send VS Code and Copilot runtime identities when fetching managed settings, fail closed on compatibility rejections, and block queued or future chat work until a fresh compatible response succeeds.\n\nAdd concise policy diagnostics and configurable 404/466 responses to the mock policy server.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Robo (@deepak1556)Matched files:
|
Expose only a managed-settings request channel from Electron main and reject unrelated request commands or call sites.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds managed-settings compatibility enforcement and update-required UX across desktop, remote, chat, and Agents surfaces.
Changes:
- Reports VS Code/Copilot runtime versions and handles HTTP 466 fail-closed.
- Blocks new/queued chat work and surfaces update guidance.
- Extends diagnostics, tests, and the mock policy server.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts |
Extends account-service fixture. |
src/vs/workbench/services/request/electron-browser/requestService.ts |
Routes explicit User-Agent requests through main-process IPC. |
src/vs/workbench/services/request/browser/requestService.ts |
Routes explicit User-Agent requests through remote services. |
src/vs/workbench/services/policies/test/browser/multiplexPolicyService.test.ts |
Updates account-provider mock. |
src/vs/workbench/services/policies/test/browser/accountPolicyService.test.ts |
Tests compatibility policy gating. |
src/vs/workbench/services/policies/common/accountPolicyService.ts |
Adds compatibility gate state. |
src/vs/workbench/services/policies/browser/accountPolicyGateContribution.ts |
Adds context, telemetry, and notification UX. |
src/vs/workbench/services/accounts/test/browser/managedSettings.test.ts |
Tests compatibility response parsing. |
src/vs/workbench/services/accounts/test/browser/defaultAccount.test.ts |
Tests 404/466 and recovery behavior. |
src/vs/workbench/services/accounts/browser/managedSettings.ts |
Parses compatibility errors. |
src/vs/workbench/services/accounts/browser/defaultAccount.ts |
Negotiates, persists, and publishes compatibility state. |
src/vs/workbench/contrib/chat/test/common/chatService/chatService.test.ts |
Tests chat rejection and queue clearing. |
src/vs/workbench/contrib/chat/common/chatService/chatServiceImpl.ts |
Blocks future and queued chat requests. |
src/vs/workbench/browser/actions/developerActions.ts |
Expands policy diagnostics. |
src/vs/sessions/test/web.test.ts |
Updates Sessions account mock. |
src/vs/sessions/contrib/policyBlocked/test/browser/sessionsPolicyBlocked.fixture.ts |
Adds update-required visual fixture. |
src/vs/sessions/contrib/policyBlocked/browser/sessionsPolicyBlocked.ts |
Renders update-required overlay. |
src/vs/sessions/contrib/policyBlocked/browser/policyBlocked.contribution.ts |
Selects the compatibility overlay. |
src/vs/platform/defaultAccount/common/defaultAccount.ts |
Defines compatibility service contracts. |
src/vs/editor/standalone/browser/standaloneServices.ts |
Updates standalone account stub. |
src/vs/editor/contrib/inlineCompletions/test/browser/utils.ts |
Updates inline-completion test service. |
src/vs/code/electron-main/app.ts |
Registers the request IPC channel. |
scripts/mock-policy-server/server.ts |
Supports configurable response statuses. |
scripts/mock-policy-server/README.md |
Documents compatibility-response testing. |
scripts/mock-policy-server/public/index.html |
Adds the response-status input. |
scripts/mock-policy-server/public/app.ts |
Handles status editing and persistence. |
scripts/mock-policy-server/endpoints.ts |
Adds 200, 404, and 466 presets. |
Review details
- Files reviewed: 29/29 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (readHeader(options.headers, 'User-Agent')) { | ||
| if (!connection) { | ||
| throw new Error('Cannot send an explicit User-Agent without a remote request service'); | ||
| } | ||
| return this._makeRemoteRequest(connection, options, token); | ||
| } |
| if (readHeader(options.headers, 'User-Agent')) { | ||
| return this.nodeRequestService.request(options, token); | ||
| } |
Screenshot ChangesBase: Added (2)
|
Dmitriy Vasyura (dmitrivMS)
left a comment
Dmitriy Vasyura (dmitrivMS)
left a comment
There was a problem hiding this comment.
AI Review: Reviewed managed-settings compatibility enforcement and request boundaries.
|
|
||
| mainProcessElectronServer.registerChannel( | ||
| MANAGED_SETTINGS_REQUEST_CHANNEL, | ||
| new RequestChannel(accessor.get(IRequestService), options => options.callSite === MANAGED_SETTINGS_REQUEST_CALL_SITE) |
There was a problem hiding this comment.
AI Review: callSite comes from renderer-supplied request options, so it cannot authorize this channel. A renderer that knows this constant can send an arbitrary URL, method, headers, and body through the main-process request service, bypassing browser CORS and forbidden-header restrictions. Please constrain the trusted-side contract to the managed-settings request (including origin/path, method, headers/body, and redirects), or expose a dedicated operation whose endpoint is not renderer-controlled.
| return Iterable.some(models, model => model.requestInProgress.read(reader)); | ||
| }); | ||
|
|
||
| this._register(this.defaultAccountService.onDidChangeManagedSettingsCompatibilityError(error => { |
There was a problem hiding this comment.
AI Review: This only reacts to future compatibility-error events. If a cached 466 state is loaded before ChatService is created, no rejection runs; later Agent Host hydration can restore and auto-drain its server-authoritative queued prompts while the client is blocked. Please enforce the current compatibility state during startup/hydration and cover delayed ChatService construction with an Agent Host queue test.
| this.reportManagedSettingsOutcome(status, rateLimitBackoffActive); | ||
| return { kind: 'noSettings' }; | ||
| } | ||
| if (status === 466) { |
There was a problem hiding this comment.
AI Review: Browser-only clients are described as excluded from compatibility negotiation and do not send the compatibility User-Agent, but this branch still treats every 466 (including a malformed response) as update-required. The mock server also makes this reachable without checking the header. Please gate 466 handling to negotiation-capable paths, unless the backend contract that 466 is impossible without that header is documented and tested.
| const fetchedAt = defaultAccountService.managedSettingsFetchedAt; | ||
| content += `| Last successful fetch | ${fetchedAt ? new Date(fetchedAt).toLocaleString() : '*n/a*'} |\n`; | ||
| content += `| Last fetch | ${fetchStatus === null ? '*never*' : `\`${fetchStatus}\`${fetchedAt ? ` at ${new Date(fetchedAt).toLocaleString()}` : ''}`} |\n`; | ||
| content += `| User-Agent | \`vscode/${productService.version} copilot-runtime/${productService.copilotVersions?.runtime ?? 'unknown'}\` |\n`; |
There was a problem hiding this comment.
AI Review: This diagnostics row can report a User-Agent that was never sent: browser-only clients omit the header, and when the runtime version is unavailable the request builder omits the runtime segment instead of sending copilot-runtime/unknown. That can mislead compatibility investigations. Please derive this display from the same request-building/capability logic, or explicitly report that the header was not sent.
| if (this.currentReason === options.reason && options.reason !== SessionsBlockedReason.AccountPolicyGate) { | ||
| if (this.currentReason === options.reason | ||
| && options.reason !== SessionsBlockedReason.AccountPolicyGate | ||
| && options.reason !== SessionsBlockedReason.ManagedSettingsUpdateRequired) { |
There was a problem hiding this comment.
AI Review: Excluding this static reason from the same-reason early return recreates the modal whenever compatibility metadata changes, even though the overlay does not render that metadata. Clearing and rebuilding it resets keyboard focus and can repeat the screen-reader announcement without any visible benefit. Please retain the existing overlay while this reason is unchanged, or update only user-visible content in place.
Summary
mock-policy-serverValidation
npm run typecheck-clientnpm run valid-layers-checkNotes
Browser-only vscode.dev is intentionally excluded from compatibility negotiation because browser APIs cannot set
User-Agent; desktop and remote Node-capable request paths participate. Exact intra-turn model/tool atomic boundaries remain owned by the corresponding@github/copilotruntime implementation.