mirror of
https://github.com/microsoft/vscode.git
synced 2026-06-29 10:56:24 +01:00
Josh Spicer
a4ce08f735
* Refactor Copilot managed-settings for maintainability
Centralize structured (object/array) managed-setting handling behind a
single descriptor table so adding a key touches one place, consolidate the
duplicated equality helpers onto `equals`, and add shared
`hasManagedSettingsDefinitions` and `managedSettingValue` helpers. Strictly
behavior-preserving.
Incorporates a 3-model maintainability review:
- `adaptManagedSettings` builds the scalar remainder via `{ ...response }`
plus delete (CopyDataProperties) instead of for..in + assignment, so a
server-sent own `__proto__` key cannot trigger the inherited setter. This
matches the original `...rest` semantics; adds a regression test.
- `managedSettingValue` is memoized per key so its policy-definition
reference identity is real rather than incidental to the call site.
- Corrected JSDoc and skill docs that overstated `responseField` as
compiler-checked; it is a hand-maintained union backstopped by tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Clarify why structured managed-settings keys must declare type: 'string'
The bag-carrying `type` is load-bearing, not cosmetic: `projectManagedSettings`
gates each value with `typeof value === type` and drops mismatches, and the
native MDM watcher reads the registry/plist value as that type. Spell out that
omitting it (or declaring the object/array type) makes a structured key fail
projection and silently never apply.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address PR review: allocation-free empty check, __proto__ test, doc accuracy
- hasManagedSettingsDefinitions: reuse the allocation-free isEmptyObject
helper instead of Object.keys(...).length (the bot's only valid nit).
- Add a primitive `__proto__` regression test proving a server-sent
`{"__proto__": true}` scalar is dropped, never pollutes the result
(disproves the reviewer's prototype-pollution concern).
- Fix github-managed-settings.md: omitting `type` or declaring
`'object'`/`'array'` is a compile error (the field is required and
constrained to `'string' | 'number' | 'boolean'`), not a runtime drop;
only `'number'`/`'boolean'` compile-but-drop-at-runtime.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Surface managed-settings source in Policy Diagnostics
Centralize the server-over-MDM precedence into a shared selectManagedSettings
helper (plus a ManagedSettingsSource union) and reuse it in both
AccountPolicyService and the Policy Diagnostics report, so the report can never
drift from the source that policy evaluation actually applies.
Rewrite the diagnostics "Managed Settings" section to:
- show the Active source (GitHub Server API / Native MDM / None)
- break down each channel (server fetch status + raw response, native MDM bag)
- label the raw response as the last *successful* fetch, so a later failed
fetch (e.g. a 404) no longer looks like it contradicts an empty effective bag
- compute the true effective bag via the shared projectManagedSettings
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix mock policy server "Generate example" not persisting
The "Generate example" button filled the editor and the localStorage draft but
never called debouncedSave(), so the generated body was never POSTed to
/api/state and the endpoint kept serving the empty preset. Add the missing
debouncedSave() to match applyPreset() and the editor input handler.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Strip prose from Policy Diagnostics and collect managed-settings parse errors
The Developer: Policy Diagnostics "Managed Settings" section now renders
data only (tables and JSON blocks, no explanatory paragraphs).
It also collects non-fatal parsing/normalization warnings from every stage
of the managed-settings pipeline, jsonc-style (accumulate, never throw), and
surfaces them in a new "Parse Errors" section:
- adapt: re-runs adaptManagedSettings on the raw server response
- project: re-runs projectManagedSettings against the declared policy keys
- parse: re-parses JSON-payload string values with the jsonc parser
This explains why a key is silently dropped. For example a server
extraKnownMarketplaces entry with source "github" but no "repo" now shows
the "requires \"repo\"" warning instead of just vanishing from the bag.
Adds a focused test for that github-without-repo normalization case.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Tighten Policy Diagnostics managed-settings rendering (review follow-up)
Code-quality pass on the managed-settings diagnostics section:
- Extract a jsonBlock() helper for the repeated fenced-JSON rendering
(4 call sites collapsed).
- Parse only the known JSON-payload keys (enabledPlugins,
strictKnownMarketplaces, extraKnownMarketplaces) instead of a
leading-brace heuristic. This mirrors what PolicyConfiguration actually
parses, avoids mis-sniffing scalar values, and catches malformed payloads
that don't start with a brace.
- Unify the raw-response guard on isObject() so the printed raw response and
the adapt-stage warning harvest use one predicate.
- Drop the defensive object copy in projectManagedSettings(); it is read-only,
so normalize undefined with `?? {}` instead of spreading.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Fix native MDM availability in Policy Diagnostics; tidy table headers
The diagnostics report showed "Native MDM | Available | no (desktop only)"
even on desktop. ICopilotManagedSettingsService was registered only in the
electron-main process and hand-plumbed into AccountPolicyService, but never
placed in the renderer service collection, so the report's
accessor.get(ICopilotManagedSettingsService) always threw and mislabeled the
channel as unavailable.
Register the CopilotManagedSettingsChannelClient (the renderer's handle to the
main-process service) in the service collection in both desktop.main.ts and
sessions.main.ts. The diagnostics now resolves it on desktop and Agents windows
and reports real native MDM availability and values; web still has no native
channel and correctly reports unavailable.
Also tidy the report builder: extract a PROPERTY_VALUE_TABLE_HEADER constant for
the five repeated two-column table headers, and drop the now-misleading
"(desktop only)" annotation on the availability row.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>