chat: fail closed on forced managed settings refresh - #332388
Open
joshspicer wants to merge 8 commits into
Open
Conversation
Groundwork for making `forceRemoteSettingsRefresh` a real fail-closed startup gate (microsoft/vscode-internalbacklog#8825). Contract only — no behavior change, and nothing gates on freshness yet. Adds `managedSettingsFreshness.ts`, declaring the state machine shared by the fetch path, the policy gate and Policy Diagnostics so those consumers cannot drift: `NotRequired` / `Pending` / `Satisfied` / `Blocked`, the failure categories every inability-to-refresh maps to, and scoping by account + provider + endpoint so satisfaction is never transferable across accounts or GHE hosts. Replaces `shouldForceRemoteSettingsRefresh` with `resolveForceRemoteSettingsRefresh`, which resolves through `pickManagedSettings` instead of re-implementing precedence. Two fixes fall out: the file channel now participates (the old helper read only native MDM and server, silently ignoring managed-file delivery), and an explicit managed `false` is now distinguishable from an absent value, which a later change needs in order to know when the requirement may be cleared. The old helper had no production caller — it was left orphaned when 661f18f reworked the managed-settings fetch — so this is inert. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address PR feedback: `IManagedSettingsFreshness` was a bag of optional fields, so a consumer could construct `Blocked` with no failure, `Satisfied` with no scope, or attach `httpStatus`/`retryAfter` to states where they mean nothing — leaving the fetch, gate and diagnostics consumers free to drift despite the type. Models it as a discriminated union instead, so each active state requires the fields its contract defines. `Blocked` is itself a union keyed on the failure category, so a status code is required for an HTTP error, a backoff deadline for rate limiting, and neither is accepted elsewhere. `source` is now the shared `ManagedSettingsChannel` rather than `string`, and is required on the effective states, which also encodes that it is never `'none'` once a channel has supplied the control. Adds `@ts-expect-error` coverage for the three rejected shapes: the directives fail the build if any shape becomes constructible again. `isSameManagedSettingsFreshnessScope` is now a private helper with required arguments — the union guarantees a scope is present, so its undefined-tolerance was unreachable, and nothing outside this module used it. Also trims two over-long comments flagged in review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Require a fresh managed-settings response before enabling AI features when forceRemoteSettingsRefresh is effective. Preserve recovery through sign-in and retry, expose diagnostics, and cover native, server, file, failure, scope, and sign-out behavior. Related to microsoft/vscode-internalbacklog#8825. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds fail-closed managed-settings freshness enforcement across account fetching, policy gates, diagnostics, and the Agents window.
Changes:
- Introduces scoped freshness states and failure categories.
- Integrates freshness enforcement and recovery UX.
- Adds unit, fixture, and service initialization coverage.
Show a summary per file
| File | Description |
|---|---|
.github/skills/policy-and-managed-settings/github-managed-settings.md |
Documents fail-closed behavior. |
src/vs/editor/contrib/inlineCompletions/test/browser/utils.ts |
Updates account-service mock. |
src/vs/editor/standalone/browser/standaloneServices.ts |
Adds default freshness state. |
src/vs/platform/defaultAccount/common/defaultAccount.ts |
Exposes freshness API. |
src/vs/platform/policy/common/copilotManagedSettings.ts |
Resolves control precedence. |
src/vs/platform/policy/common/fileManagedSettingsIpc.ts |
Awaits initial file snapshots. |
src/vs/platform/policy/common/fileManagedSettingsService.ts |
Adds initialization barrier. |
src/vs/platform/policy/common/managedSettingsFreshness.ts |
Defines freshness state machine. |
src/vs/platform/policy/test/common/copilotManagedSettings.test.ts |
Tests control precedence. |
src/vs/platform/policy/test/common/fileManagedSettingsService.test.ts |
Tests initialization API. |
src/vs/platform/policy/test/common/managedSettingsFreshness.test.ts |
Tests freshness invariants. |
src/vs/sessions/contrib/policyBlocked/browser/policyBlocked.contribution.ts |
Routes freshness overlays. |
src/vs/sessions/contrib/policyBlocked/browser/sessionsPolicyBlocked.ts |
Adds recovery UI. |
src/vs/sessions/contrib/policyBlocked/test/browser/sessionsPolicyBlocked.fixture.ts |
Adds blocked-state fixture. |
src/vs/sessions/test/web.test.ts |
Updates account-service mock. |
src/vs/workbench/browser/actions/developerActions.ts |
Extends policy diagnostics. |
src/vs/workbench/browser/web.main.ts |
Registers null file service. |
src/vs/workbench/services/accounts/browser/defaultAccount.ts |
Implements freshness fetching and scope. |
src/vs/workbench/services/accounts/test/browser/defaultAccount.test.ts |
Covers freshness outcomes. |
src/vs/workbench/services/policies/browser/accountPolicyGateContribution.ts |
Adds notification recovery UX. |
src/vs/workbench/services/policies/common/accountPolicyService.ts |
Composes freshness into policy gate. |
src/vs/workbench/services/policies/test/browser/accountPolicyGateContribution.test.ts |
Tests notifications. |
src/vs/workbench/services/policies/test/browser/accountPolicyService.test.ts |
Tests gate enforcement. |
src/vs/workbench/services/policies/test/browser/multiplexPolicyService.test.ts |
Updates provider mock. |
src/vs/workbench/test/browser/componentFixtures/fixtureUtils.ts |
Updates fixture services. |
Review details
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 25/25 changed files
- Comments generated: 6
- Review effort level: Balanced
Re-render the Agents window when freshness failure details change and preserve startup notification deferral. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Screenshot ChangesBase: Added (2)
|
Scope cached server controls before precedence, avoid expired rate-limit poll loops, and align update-required recovery guidance across workbench and Agents window UI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Classic web initialization intentionally skips the default-account fetch. Exercise the explicit refresh path before asserting the no-token fail-closed state so the browser suite observes the same lifecycle it is validating. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ensure managed-settings messages render Code - OSS instead of an undefined product label in component screenshots. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
marked this pull request as ready for review
August 24, 2026 21:50
Alexandru Dima (alexdima)
previously approved these changes
Aug 24, 2026
Anthony Kim (anthonykim1)
previously approved these changes
Aug 24, 2026
Let the mock policy server return HTTP errors, malformed JSON, immediate disconnects, or no response until client timeout through presets, the GUI, and the control API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
dismissed stale reviews from Anthony Kim (anthonykim1) and Alexandru Dima (alexdima)
via
August 24, 2026 22:45
4551135
Alexandru Dima (alexdima)
approved these changes
Aug 24, 2026
Vritant Bhardwaj (vritant24)
approved these changes
Aug 24, 2026
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
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.
Summary
Makes
forceRemoteSettingsRefresha true fail-closed freshness control in VS Code.When the control is effective from native MDM, a cached server response, or the managed settings file, VS Code now keeps AI features unavailable until
/copilot_internal/managed_settingsreturns a fresh successful response for the current account, authentication provider, and endpoint. Cached settings may preserve existing restrictions and self-perpetuate the flag, but they cannot satisfy freshness.What changed
notRequired,pending,satisfied, and categorizedblockedstates.Runtime alignment
This is the VS Code host implementation. It does not duplicate runtime schema parsing or policy matching. It aligns the host gate with the runtime behavior landed in github/copilot-agent-runtime#16384 while preserving VS Code-owned account, workbench, and Agents-window lifecycle behavior.
Validation
npm run transpile-clientnpm run valid-layers-checkgit diff --checknpm run typecheck-clientreports only the unrelated pre-existingAgentServiceconstructor error atsrc/vs/platform/agentHost/test/node/agentService.test.ts:9986(the changed files type-check cleanly).chrome-sandboxis not configured setuid-root; the equivalent focused Node runner was used.Related to microsoft/vscode-internalbacklog#8825.