policy: gate Copilot on a fresh managed-settings fetch - #331878
Closed
joshspicer wants to merge 1 commit into
Closed
policy: gate Copilot on a fresh managed-settings fetch#331878joshspicer wants to merge 1 commit into
joshspicer wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the shared contract for fail-closed managed-settings freshness without enabling enforcement.
Changes:
- Defines freshness states, failure categories, and account/provider/endpoint scoping.
- Resolves refresh policy across native MDM, server, and file channels.
- Adds unit coverage for resolution and freshness helpers.
Show a summary per file
| File | Description |
|---|---|
managedSettingsFreshness.ts |
Defines the freshness contract and helpers. |
copilotManagedSettings.ts |
Adds channel-aware refresh-control resolution. |
managedSettingsFreshness.test.ts |
Tests freshness states and scope matching. |
copilotManagedSettings.test.ts |
Tests channel precedence and malformed values. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
Mohammad javad Dianat (dianatofficial)
left a comment
There was a problem hiding this comment.
Solid PR. Tests cover the important paths.
When an enterprise sets `forceRemoteSettingsRefresh`, VS Code must fetch managed settings fresh before enabling Copilot agent functionality, and block rather than fall back to cached policy when that fetch fails. Mirrors the existing HTTP 466 "client update required" fail-closed path: `DefaultAccountProvider` publishes `managedSettingsRefreshBlocked`, and `AccountPolicyGateContribution` ORs it into the same context key and `setForceHidden` call the 466 gate already drives. Sign-in stays reachable so a user can recover. Notable behaviours: - A fresh 404 means "no policy file configured" and satisfies the requirement. Blocking on it would lock out every org without a policy file. - The requirement is persisted outside the policy cache, which expires hourly and is dropped on a failed fetch — otherwise the gate would silently reopen after an hour offline. - The server channel is recorded per account, since the server delivers the control per organization. Native MDM and the on-disk file are machine-wide and read live, so precedence is applied at read time and a signed-out machine carrying a local flag is still gated. - The managed-settings fetch is restricted to the evaluated account's sessions. `request` otherwise falls through to the next session on 401/404, letting another organization answer for — and have its settings cached under — this account. - Cache-bypassing retries are throttled, as a refresh fires on every window focus. Inert when the flag is unset, which is covered by tests. Refs microsoft/vscode-internalbacklog#8825 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
force-pushed
the
agents/vscode-internalbacklog-issue-8825-planning
branch
from
August 24, 2026 18:04
3995c58 to
8bfeec4
Compare
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.
Makes
forceRemoteSettingsRefresha genuine fail-closed startup gate: when an enterprise sets it, VS Code fetches managed settings fresh before enabling Copilot agent functionality, and blocks rather than falling back to cached policy when that fetch fails.Tracking: microsoft/vscode-internalbacklog#8825
Note
This replaces the two previous commits on this branch (a contract-only
managedSettingsFreshness.tsstate machine). That approach designed a general contract before it had a consumer; this is the behaviour itself, built on the pattern already in the codebase.Approach
VS Code already has a working fail-closed gate of exactly this shape — the HTTP 466 "client update required" path. This copies it rather than inventing a parallel mechanism:
managedSettingsCompatibilityErroronIDefaultAccountServicemanagedSettingsRefreshBlockedonDidChangeManagedSettingsCompatibilityErroronDidChangeManagedSettingsRefreshBlockedAccountPolicyGateContribution.updatePolicyGateState()setForceHiddenSign-in stays reachable throughout, so a user can always recover.
shouldForceRemoteSettingsRefresh()(dead code since 661f18f — it had no production caller) becomesresolveForceRemoteSettingsRefresh(), which additionally consults the file channel and reports the winning channel. It returnsundefinedwhen no channel sets the key, so an explicit managedfalseis distinguishable from an absent value.Behaviour worth reviewing
A fresh 404 satisfies the requirement. It means "no policy file configured". Blocking on it would permanently lock out every org that has no policy file.
The requirement outlives the policy cache. The cache is honoured for one hour and a failed fetch returns
managedSettings: undefined— dropping the restrictions and the flag itself. A naive sticky gate would silently reopen after an hour offline, so the requirement is persisted separately underdefaultAccount.managedSettingsRefreshRequired.Scope follows the delivery channel. The server delivers the control per organization, so it is recorded per account — one account's response must never speak for another's. Native MDM and the on-disk file are machine-wide and read live on every evaluation, so precedence is applied when the requirement is read (which is what lets a native MDM
falseoverride a servertrue). A machine-widelocalhint covers only the startup window in which those channels are still loading.Because the local channels are read live rather than behind an authenticated fetch, a signed-out machine carrying an MDM- or file-delivered flag is gated on first launch — one of the gaps called out during planning.
Migration. The flag already lives in users' cached server bags today, so a requirement recorded by a previous build is recovered from there on first run; without it, every flagged org would get one ungated offline launch on upgrade.
Retries are throttled (60s), because a refresh fires on every window focus and an offline client would otherwise issue a request per alt-tab. Skipping a retry inside the window never reopens the gate.
Behaviour change to confirm⚠️
The managed-settings fetch is now restricted to the evaluated account's sessions.
request()otherwise falls through to the next session on 401/404, which is a cross-account fallback: another organization could answer for your account, and its settings were merged and cached under your account.This fixes a permanent-lockout path (MDM sets the flag → server 404s for account A → account B always answers first → A's satisfying 404 is discarded and A is gated forever). But it is not bit-identical for unflagged users: in a multi-account setup with an expired primary token, VS Code today fetches managed settings with a different account's token and applies that org's policy to you. I judged "more correct" beats "bit-identical" here, but it deserves an explicit ack.
Inertness
The feature is inert when the flag isn't set — that's essentially all users — and this is asserted, not assumed:
no fetch is blocked when the refresh flag is not set anywhere— an unflagged org is never gated, even with the endpoint downa failed fetch never escalates from another account cached settings— the policy cache is never consulted for gate decisions, so an unflagged user cannot be blocked by a polluted cacheScope
Not included, deliberately:
cache-control: no-cache; the transport isfetchanddisableCache: trueis already passed..github/skills/policy-and-managed-settings.remoteAgentHostProtocolClient.tssends{}to non-local hosts).Validation
defaultAccount.test.tsand 2 inaccountPolicyGateContribution.test.ts, covering first launch, cached flag, MDM/file-delivered flag, explicit managedfalse, successful refresh, each failure class, account switching, migration and retry throttlingnpm run typecheck-client,npm run valid-layers-check, eslint — all cleanTests were run directly under mocha: this environment can't build the native modules the Electron harness needs (
npm installfails on an authenticated NuGet feed with a 401, unrelated to this change), so please let CI confirm.This change went through seven rounds of adversarial review, which surfaced eleven fail-open paths in a gate whose only job is to fail closed — several only becoming visible once an earlier fix was in place. The tests named above encode each of them.