agentHost: graduate the legacy-settings managed-permissions bridge - #333290
Open
joshspicer wants to merge 3 commits into
Open
agentHost: graduate the legacy-settings managed-permissions bridge#333290joshspicer wants to merge 3 commits into
joshspicer wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 3
New issues introduced by this change (3)
| Severity | Finding |
|---|---|
 |
src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts — The deprecation text says this value is ignored, but the adjacent description still says “When… |
|
src/vs/platform/agentHost/common/agentHostManagedSettings.ts — This semantic update leaves the repository's managed-settings guidance stale:… |
|
src/vs/platform/agentHost/test/common/agentHostManagedSettings.test.ts — The default-on coverage stops at the resolver, while agentHostProtocolClient.test.ts:1214-1217… |
What changed in this PR
Graduates the legacy-settings permissions bridge to default enforcement following the runtime fix referenced by #332011.
Changes:
- Removes the experimental opt-in gate.
- Deprecates the now-ignored setting.
- Adds default-on and explicit-false tests.
| File | Description |
|---|---|
agentHostManagedSettings.ts |
Makes permission mapping unconditional. |
chat.shared.contribution.ts |
Deprecates the compatibility setting. |
agentHostManagedSettings.test.ts |
Updates bridge behavior tests. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
joshspicer
force-pushed
the
joshspicer/graduate-legacy-settings-bridge
branch
from
August 31, 2026 18:09
8d85ef1 to
6ee5415
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 1
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
 |
src/vs/platform/agentHost/test/common/agentHostManagedSettings.test.ts — This default-on test only verifies the VS Code DTO. It cannot catch the regression that originally… |
Pre-existing issues (3)
| Severity | Finding |
|---|---|
|
src/vs/platform/agentHost/test/common/agentHostManagedSettings.test.ts — The default-on coverage stops at the resolver, while agentHostProtocolClient.test.ts:1214-1217… View comment |
|
src/vs/platform/agentHost/common/agentHostManagedSettings.ts — This semantic update leaves the repository's managed-settings guidance stale:… View comment |
|
src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts — The deprecation text says this value is ignored, but the adjacent description still says “When… View comment |
Suppressed comments (1)
src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts:1617
- The new deprecation text says the value is ignored, but the adjacent setting description still says mapping happens “When enabled.” Both are displayed as user-facing setting documentation, so existing users now receive contradictory guidance. Update the description to describe the always-on bridge without conditioning it on this boolean.
markdownDeprecationMessage: nls.localize('chat.agentHost.copilot.mapLegacySettingsToManagedSettings.deprecated', "This setting is no longer used. Supported legacy settings are always mapped to Copilot SDK managed settings, so this value is ignored."),
The bridge from legacy VS Code settings to the Copilot SDK's per-session managedSettings.permissions shipped behind the experimental opt-in chat.agentHost.copilot.mapLegacySettingsToManagedSettings because the runtime treated any managed rule from any source as activating the global managed policy, so one narrow restriction forced unmatched shell/read/write/URL/factory requests to prompt. github/copilot-agent-runtime#16249 makes client/session-injected managed permissions non-activating, which removes that broadening. The bundled runtime VS Code spawns contains the fix, so the bridge becomes the default enforcement path. The setting is deprecated and its value is now ignored rather than honored as an opt-out: the mappings only ever contribute administrator-configured restrictions, so honoring an explicit false would let a user switch off an enterprise policy. Also drops the now-inert opt-in fixture from the per-tool auto-approval tests added in #333264, so no test implies the gate still influences the result. Refs #332011 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
joshspicer
force-pushed
the
joshspicer/graduate-legacy-settings-bridge
branch
from
August 31, 2026 18:22
6ee5415 to
6898071
Compare
joshspicer
marked this pull request as ready for review
August 31, 2026 18:31
Contributor
There was a problem hiding this comment.
Copilot review overview
Review tier: Balanced
Findings: 2
New issues introduced by this change (1)
| Severity | Finding |
|---|---|
|
src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts — The setting is now ignored for both true and false, but the Settings UI still says mapping… |
Pre-existing issues (4)
| Severity | Finding |
|---|---|
|
src/vs/platform/agentHost/test/common/agentHostManagedSettings.test.ts — This default-on test only verifies the VS Code DTO. It cannot catch the regression that originally… View comment |
|
src/vs/platform/agentHost/test/common/agentHostManagedSettings.test.ts — The default-on coverage stops at the resolver, while agentHostProtocolClient.test.ts:1214-1217… View comment |
|
src/vs/platform/agentHost/common/agentHostManagedSettings.ts — This semantic update leaves the repository's managed-settings guidance stale:… View comment |
|
src/vs/workbench/contrib/chat/browser/chat.shared.contribution.ts — The deprecation text says this value is ignored, but the adjacent description still says “When… View comment |
Follow-up to the gate graduation, from PR review: - The managed-settings skill guidance still listed the false-by-default experimental gate as a bridge invariant, which would instruct future work to restore the removed gate. Updated in sdk-runtime-policy.md, and in the two further places carrying the same claim (legacy-permission-policy.md and SKILL.md). - The protocol-boundary tests still supplied the deprecated opt-in to their forwarding fixtures, so they did not prove a local connection forwards restrictions without it. Removed, along with the now-unused import. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The bridge is now unconditional, so the gating setting had no effect while still presenting a checkbox in Settings whose description claimed mapping happens "when enabled". A user with an existing `false` value was told the bridge was disabled while restrictions were in fact enforced. Remove the setting from the configuration registry and drop the now-unused constant. The resolver already ignores the value, so behaviour is unchanged; this only stops Settings from advertising a control that does nothing. The unit test keeps the regression guard using the raw setting id, so a stale settings.json entry still cannot switch off an administrator restriction. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Henning Dieterichs (hediet)
approved these changes
Aug 31, 2026
joshspicer
enabled auto-merge (squash)
August 31, 2026 18:59
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.
Graduates the legacy-settings →
managedSettings.permissionsbridge from its experimental opt-in to the default enforcement path.Refs #332011. Bridge shipped in #331415; runtime blocker github/copilot-sdk-internal#243, fixed by github/copilot-agent-runtime#16249.
Why this was gated
The runtime treated any managed rule from any source as activating the global managed policy, so one narrow restriction (a single denied domain, one terminal
falserule) turned into session-wide prompting for unmatched shell/read/write/URL/factory requests.github/copilot-agent-runtime#16249 makes client/session-injected managed permissions non-activating:
deny/ask/disableBypassPermissionsModestill bind, but they no longer flip unmatched families to "default ask". Server/MDM layers keep today's activating lockdown. Behavior-only and source-based, so VS Code sends nothing new on the wire.It also fully covers #243 rather than partially: the runtime split the conflated flag into
rules_active(client rules still authoritative) anddefault_ask_active(external layers only). That satisfies #243's first three acceptance criteria; the fourth is this PR.Verifying the bundled runtime contains #16249
This needed care, because the bump was reverted and re-landed: #332803 bumped it, #332951 reverted it in full after an unrelated session-restore hang (#332885), and #333198 has since re-landed it.
mainnow pins@github/copilot1.0.82-0 and@github/copilot-sdk1.0.13-preview.2.Verified by tag containment on the runtime merge commit — the only reliable check here:
9caaa2f)cli-1.0.80divergedcli-1.0.81-0divergedcli-1.0.81-10behindcli-1.0.82-0(current pin)behindWorth flagging for anyone re-checking this: binary symbol presence is not a valid test. Grepping the 1.0.81-0 runtime for
_defaultAskActive— the internal key #16249 introduces — returns a match, but that release does not contain the fix. The string ships ahead of the logic. Version-number ordering is also unsafe, since the-Nsuffixes are not ordered by merge date. Only tag containment answered this correctly.The re-bump also retires a second hazard. Under the older pins there were two runtimes on disk, including a
1.0.80nested under@github/copilot-sdkwithout the fix, and only the explicitresolveCopilotCliPath()override incopilotAgent.tsguaranteed the fixed one enforced. Now thatcopilot-sdk@1.0.13-preview.2depends on@github/copilot@^1.0.82-0, that copy dedupes to the same version, so both the explicit path and ordinary module resolution land on a fixed runtime.Behaviour for users who set the experimental setting
chat.agentHost.copilot.mapLegacySettingsToManagedSettingshas been removed from the settings registry. It no longer appears in Settings, and its value is ignored wherever it still exists on disk.true→ no change; now the default.false→ ignored, the bridge applies. A leftover entry insettings.jsonwill read as an unknown setting and can be deleted.Ignoring an explicit
falseis deliberate. Every mapping contributes only restrictions, sourced from policy/global enterprise settings, so honoringfalseas a permanent opt-out would let an ordinary user switch off an administrator-configureddeny. The escape hatch remains the underlying legacy settings.Changes
agentHostManagedSettings.ts— drop the gate check; replace the stale "contributing any rule makes the managed policy active" JSDoc with the non-activating semantics; drop the setting frommanagedPermissionsConfigurationIds; document the constant as a deprecated no-op.chat.shared.contribution.ts— remove the setting registration entirely, so Settings no longer advertises a checkbox whose description claims mapping happens "when enabled" while the value does nothing.chat.tools.eligibleForAutoApprovalto managed settings #333264 and the protocol-boundary forwarding tests); replace the "bridge disabled" test with default-on coverage; keep a guard, using the raw setting id, that a stalesettings.jsonentry cannot switch off a mapped restriction..github/skills/policy-and-managed-settings/— the guidance listed the false-by-default gate as a bridge invariant in three places, which would have instructed future work to restore it. Updated.chat.tools.eligibleForAutoApproval(#333264) landed after this branch and is now default-on too. It's registeredpolicyOnly, so only an enterprise policy value can trigger it — no user preference can lock bypass mode by default.Not changed, per the issue: no
allowlist is contributed, host-sideSessionPermissionManagerstays (still used by Claude/Codex and remote hosts), and expressiveness-gap mappings remain SDK-side work.