Skip to content

policy: resolve managed settings per-key across delivery channels#323780

Merged
joshspicer merged 3 commits into
mainfrom
agents/managed-settings-per-key-precedence
Jul 1, 2026
Merged

policy: resolve managed settings per-key across delivery channels#323780
joshspicer merged 3 commits into
mainfrom
agents/managed-settings-per-key-precedence

Conversation

@joshspicer

Copy link
Copy Markdown
Member

What

Managed settings previously used a single authoritative source: the first non-empty delivery channel (native MDM > server > file) won wholesale and the others were ignored entirely. This switches to per-key precedence — the same order is honored, but resolved key-by-key:

  • A key set/locked by a higher-precedence channel still cannot be overwritten by a lower one.
  • A key a higher channel leaves unset is now filled in by a lower channel.

How

  • Centralized merge — replaced selectManagedSettings with pickManagedSettings() in copilotManagedSettings.ts. It walks MANAGED_SETTINGS_CHANNELS highest-first and returns:
    • values — the effective merged bag,
    • resolutions — per-key provenance (winning source + every channel's contribution, winner first),
    • activeSources — channels that won at least one key, in precedence order.
    • The merged bag is built with Object.fromEntries so an untrusted __proto__ key can't corrupt its prototype chain.
  • AccountPolicyService.getPolicyData consumes the picker; activeSources.length === 0 replaces the old source === 'none' short-circuit.
  • Policy Diagnostics report reworked to make what came from where, what's effective, and why explicit: plural "active sources", a per-key Resolution table (effective value, winning source, and per-channel values with overridden ones struck through), and per-key policy attribution.

Tests

  • New dedicated pickManagedSettings unit suite: per-key resolution + provenance, mid-tier (server) winning when native is absent, distinct keys winning from distinct channels, falsy-but-present values winning, undefined-hole fall-through, precedence-ordered activeSources, fresh-object guarantee, empty/all-empty, and a prototype-pollution guard.
  • New end-to-end test in accountPolicyService.test.ts: native MDM wins one key and the file wins another — both reach policy evaluation (per-key fill-down across the full stack).

Notes for reviewers

  • Precedence order and within-channel merge semantics (e.g. enabledPlugins deny-wins) are unchanged.
  • The prototype-pollution guard fixes a subtle hygiene regression the new test surfaced: bracket assignment (values[key] = …) invoked the __proto__ setter on the returned bag; Object.fromEntries (define-property semantics) does not. The global prototype was never at risk and downstream reads only declared keys, so no functional bug shipped.
  • Docs updated: .github/skills/add-policy/SKILL.md and github-managed-settings.md.
  • Validation: ESLint clean, tsgo typecheck clean. The repo's gulp hygiene hook and the live Mocha suite couldn't run in this worktree (missing build deps / no out/ build); the per-key algorithm was additionally verified against a faithful functional port.

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Managed settings previously used a single authoritative source: the first
non-empty delivery channel (native MDM > server > file) won wholesale and the
others were ignored entirely. Switch to per-key precedence: the same order is
honored, but resolved key-by-key. A key locked by a higher-precedence channel
still cannot be overwritten, while keys a higher channel leaves unset are now
filled in by a lower channel.

Centralize the resolution in a new pickManagedSettings() (replacing
selectManagedSettings) that returns the merged bag, per-key provenance, and the
active sources, so policy evaluation and the Policy Diagnostics report share one
implementation. Build the merged bag with Object.fromEntries so an untrusted
__proto__ key cannot corrupt its prototype chain.

Rework the Policy Diagnostics report to show every contributing source, a per-key
Resolution table (effective value, winning source, and struck-through overrides),
and per-key policy attribution.

Add unit coverage for the per-key merge (provenance, fill-down, falsy values,
ordering, prototype-pollution guard) plus an end-to-end test proving two keys can
win from two different channels and both reach policy evaluation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 30, 2026 20:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request updates VS Code’s Copilot managed-settings handling so that precedence is resolved per key across delivery channels (native MDM → server → file), and upgrades Policy Diagnostics to explain per-key provenance and effective values.

Changes:

  • Replaces the “single authoritative bag” selection logic with pickManagedSettings() that merges managed settings key-by-key and records per-key resolutions + active sources.
  • Updates AccountPolicyService and Developer: Policy Diagnostics to consume the new per-key resolution output and report per-key attribution.
  • Adds new unit tests for pickManagedSettings() plus an end-to-end test validating that different keys can win from different channels.
Show a summary per file
File Description
src/vs/workbench/services/policies/test/browser/accountPolicyService.test.ts Adds an end-to-end test validating per-key fill-down across native MDM + file managed settings.
src/vs/workbench/services/policies/common/accountPolicyService.ts Switches policy evaluation from whole-bag selection to per-key resolution (pickManagedSettings).
src/vs/workbench/browser/actions/developerActions.ts Reworks Policy Diagnostics to display plural active sources and a per-key resolution table + attribution.
src/vs/platform/policy/test/common/copilotManagedSettings.test.ts Adds a dedicated test suite for per-key resolution behavior and provenance.
src/vs/platform/policy/common/copilotManagedSettings.ts Introduces ManagedSettingsChannel, MANAGED_SETTINGS_CHANNELS, resolution/provenance types, and pickManagedSettings().
.github/skills/add-policy/SKILL.md Updates policy skill documentation to reflect per-key managed-settings precedence.
.github/skills/add-policy/github-managed-settings.md Updates managed-settings documentation to reflect the new per-key merge/precedence model.

Review details

  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread src/vs/platform/policy/common/copilotManagedSettings.ts
Address PR feedback: switch pickManagedSettings from `for...in` to a guarded
`Object.keys` loop so only own enumerable properties are visited (managed-settings
bags are untrusted input) and absent channels are skipped explicitly.

Note: `for...in` over an undefined bag was already a no-op (never threw), so this is
a robustness/clarity improvement rather than a crash fix.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

  • Files reviewed: 7/7 changed files
  • Comments generated: 0 new
  • Review effort level: Low

@joshspicer
joshspicer marked this pull request as ready for review July 1, 2026 20:22
Drop two redundant cases (distinct-keys and standalone activeSources ordering,
both already covered by the headline and empty/absent tests) and fold the
non-contributing-middle-channel ordering check into the empty/absent snapshot.
Verified against the live node unit runner (all green).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@joshspicer
joshspicer merged commit 5a270e6 into main Jul 1, 2026
29 checks passed
@joshspicer
joshspicer deleted the agents/managed-settings-per-key-precedence branch July 1, 2026 20:55
@vs-code-engineering vs-code-engineering Bot added this to the 1.128.0 milestone Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants