Map managed permission policies into Agent Host sessions - #328082
Map managed permission policies into Agent Host sessions#328082joshspicer wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Maps enterprise permission policies into Copilot Agent Host session startup settings and restart detection.
Changes:
- Derives managed runtime permissions exclusively from policy values.
- Forwards permissions during session creation/resumption.
- Restarts sessions when managed permissions change and adds coverage.
Show a summary per file
| File | Description |
|---|---|
agentHostSchema.ts |
Defines managed permission schema and derivation. |
remoteAgentHostProtocolClient.ts |
Forwards policy-derived permissions. |
copilotAgent.ts |
Adds permissions to restart snapshots. |
copilotSessionLauncher.ts |
Passes permissions to the Copilot SDK. |
agentHostSchema.test.ts |
Tests derivation and validation. |
remoteAgentHostProtocolClient.test.ts |
Tests policy forwarding. |
copilotAgent.test.ts |
Tests restart detection. |
copilotSessionLauncher.test.ts |
Tests SDK configuration forwarding. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Medium
0eab96e to
df202b5
Compare
|
Base:
|
71e9d23 to
cd28635
Compare
87a2d11 to
cd28635
Compare
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- remote/package-lock.json: Generated file
Suppressed comments (1)
src/vs/platform/agentHost/browser/remoteAgentHostProtocolClient.ts:1509
- This contradicts the PR's “Logs no raw permission rules” guarantee. The value is sent inside the
dispatchActionframe, whileAhpJsonlLogger.logserializes the complete frame (common/ahpJsonlLogger.ts:89-107) andAgentService.dispatchActionalso traces the complete action (node/agentService.ts:2577-2579). With AHP logging or trace logging enabled,Shell(*)—and any future path/domain rules—will therefore be written to exported logs. RedactmanagedPermissionsat both logging boundaries while preserving the wire payload.
this._dispatchRootConfig({ [AgentHostManagedPermissionsConfigKey]: permissions ?? {} });
- Files reviewed: 11/13 changed files
- Comments generated: 1
- Review effort level: Balanced
7d10d67 to
7951784
Compare
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- remote/package-lock.json: Generated file
Suppressed comments (1)
src/vs/platform/agentHost/common/agentService.ts:670
- The added
client/mixedprovenance cannot be returned by the current diagnostics implementation.CopilotAgent.getManagedSettingsDiagnostics()calls the account-scoped runtimegetManagedSettings()API, whose SDK contract explicitly excludes session-local client injection; that provenance is available only on the session managed-settings-resolved event. As a result, the mapped policy remains absent from diagnostics despite the PR's stated behavior. Capture the latest per-session resolved event (or query a session-aware API) and expose that snapshot instead.
readonly source: 'server' | 'device' | 'client' | 'mixed' | 'none';
readonly serverManaged: boolean;
readonly clientManaged?: boolean;
- Files reviewed: 11/13 changed files
- Comments generated: 0 new
- Review effort level: Balanced
7951784 to
804ccda
Compare
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- remote/package-lock.json: Generated file
Suppressed comments (2)
src/vs/platform/agentHost/node/agentService.ts:2735
- This cleanup races with the per-client dispatch queue. A root-config action that arrives behind an asynchronous chat/attachment action is deferred by
_clientDispatchQueues; if the final transport closes first, this method removes the current entry, but the deferred action later calls_setClientManagedPermissionsand recreates a contribution for the disconnected client. That orphan can keep restrictions active indefinitely. Serialize removal after already-queued dispatches (while ordering a later reconnect after the removal), or invalidate deferred managed-permission writes with a connection generation.
removeClientManagedPermissions(clientId: string): void {
if (!this._managedPermissionsByClient.delete(clientId)) {
return;
}
this._configurationService.publishRootTransientValues({
src/vs/platform/agentHost/common/agentHostSchema.ts:420
- This comment is now incorrect: the renderer deliberately forwards
{}, notundefined, andgetRootValuetherefore returns the sentinel until consumers normalize it. Please describe the sentinel/normalization path so future changes do not accidentally remove the required wire-level clear.
// Intentionally NO `default`: the key follows omit semantics. When no
// restrictive policy applies the renderer forwards `undefined`, so
// `getRootValue` stays `undefined` and the launcher omits `managedSettings`
// entirely rather than forwarding an empty (and misleading) object.
- Files reviewed: 17/19 changed files
- Comments generated: 1
- Review effort level: Balanced
| } | ||
|
|
||
| removeClientManagedPermissions(clientId: string): void { | ||
| this._clientDispatchGenerations.set(clientId, (this._clientDispatchGenerations.get(clientId) ?? 0) + 1); |
There was a problem hiding this comment.
AI Review: removeClientManagedPermissions invalidates the entire client dispatch generation, so every older queued action returns before reduction, not just the stale managed-permissions RootConfigChanged. The new test demonstrates this by blocking seq 1 ChatTurnStarted on attachment I/O and then expecting only seq 3 after the generation bump, which silently drops the already accepted user turn. Please scope invalidation to pending managed-permissions updates (or explicitly reject affected actions) so disconnect-grace expiry cannot discard unrelated queued client actions.
Synthesize a client-agnostic managedSettings.permissions object from VS Code's legacy enterprise Copilot/agent policies and forward it through the Agent Host root config to the Copilot SDK at session create/resume. Derive exclusively from IConfigurationService.inspect(...).policyValue, using only rule boundaries the runtime supports: - managed chat.tools.global.autoApprove === false -> disableBypassPermissionsMode: 'disable' - managed chat.tools.terminal.enableAutoApprove === false -> ask: ['Shell(*)'] Per-tool eligibility (chat.tools.eligibleForAutoApproval) is intentionally not mapped: the runtime rejects generic Tool(...) rules, so there is no supported boundary to express it, and malformed/unknown rules reject session startup. Network/sandbox policies are out of scope. The derived snapshot participates in restart detection so a policy change refreshes the session before the next turn. The published @github/copilot-sdk (^1.0.8) does not yet expose managedSettings, so a precise additive local type mirrors the existing local-type precedent (ICopilotRuntimeManagedSettingsSdk) until the SDK publishes the field. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c111c62a-eff3-4ff6-bb8a-8436a1b4babe
The `managedPermissions` root config key follows omit semantics: it is absent
when no restrictive enterprise policy applies. Its schema property carried a
`default: {}`, which is seeded into stored root config values by
`registerProviderConfiguration` and would let the launcher forward an empty
(and misleading) `managedSettings` even with no policy. Remove the default so
the key stays `undefined` and `managedSettings` is omitted entirely.
Also remove the stale `chat.tools.eligibleForAutoApproval` reference from the
`AgentHostManagedPermissionsConfigKey` doc comment (eligibility is no longer
synthesized after the runtime-contract correction).
Add a launcher test asserting neither create nor resume config carries
`managedSettings` when the root value is unset.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c111c62a-eff3-4ff6-bb8a-8436a1b4babe
Clear removed policy through the merge-based root config, restart peer chats on policy changes, and update rebased launcher tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d5d4d699-33e2-4a55-9d48-57d2e483dd3d
Bump @github/copilot-sdk to 1.0.10-preview.0 in the desktop and remote dependency roots and regenerate both lockfiles. Replace the temporary managedSettings session-config shim with the published package-root types. Convert VS Code's readonly common-layer permission snapshot at the SDK boundary while preserving omitted keys, and expose the runtime's client/mixed managed-settings provenance in diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c111c62a-eff3-4ff6-bb8a-8436a1b4babe
Aggregate managed permission restrictions per connected client, keep them transient, and redact policy rules from protocol and trace logs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c111c62a-eff3-4ff6-bb8a-8436a1b4babe
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c111c62a-eff3-4ff6-bb8a-8436a1b4babe
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c111c62a-eff3-4ff6-bb8a-8436a1b4babe
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c111c62a-eff3-4ff6-bb8a-8436a1b4babe
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d5d4d699-33e2-4a55-9d48-57d2e483dd3d
Emit the runtime's kind-only Shell rule for terminal managed policy, validate the supported managed permission grammar in policy diagnostics, and distinguish client injection from the provider account/device baseline. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
574e9ca to
241afeb
Compare
Limit the legacy bridge to its two exact restrictive outputs, use root-schema feature detection instead of protocol-version inference, and consolidate overlapping tests while preserving disconnect, refresh, and diagnostics coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply managed bypass restrictions to the live SDK permission mode immediately when root policy changes, rather than waiting for session refresh on the next send. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Starts to map VS Code's legacy enterprise permission policies into the client-agnostic
managedSettings.permissionsobject forwarded through the Agent Host root config to the Copilot runtime at SDK session startup (create and resume).The runtime support (github/copilot-agent-runtime#14000) and public SDK types (github/copilot-sdk#2139) are merged. This PR consumes
@github/copilot-sdk@^1.0.10-preview.0and its required@github/copilot@1.0.79-6runtime from both desktop and remote dependency roots.Scope is permissions-only. No sandbox-floor logic, network policy mapping, or generic per-tool eligibility.
Policy mapping
Derived exclusively from
IConfigurationService.inspect(...).policyValue; ordinary user/workspace values are never read:chat.tools.global.autoApprove=== falsedisableBypassPermissionsMode: "disable"chat.tools.terminal.enableAutoApprove=== falseask: ["Shell"]When no restrictive policy applies, nothing is forwarded to the SDK runtime. The renderer uses an empty-object clear sentinel so the value survives JSON serialization; the host normalizes
{}to no policy.Not mapped (by design):
chat.tools.eligibleForAutoApprovalbecause the runtime rejects genericTool(...)rules. Permissive (true) policy is not mapped toallow, and network/sandbox policies remain out of scope.Behavior
enableManagedSettings: trueand forwards identical managed permissions on session create and resume.ManagedSettings/ManagedSettingsPermissionstypes; the temporary SDK shim is removed.IManagedPermissionsclient-agnostic and converts readonly arrays at the Node SDK boundary while preserving omitted keys.{}cannot clear another client's enterprise restriction.client/mixedsources and optionalclientManagedflag; session-scoped runtime resolution can now report client-injected policy.managedPermissionsat both AHP JSONL and Agent Service trace-log boundaries without mutating the wire/reducer payload.Shellrule and constrains the bridge schema to the two exact values it can synthesize; Developer: Policy Diagnostics shows the resulting client injection separately from the runtime account/device baseline.Fail-closed compatibility
Protocol version alone cannot prove that an Agent Host recognizes this VS Code-specific root configuration. Support is detected from the host's advertised root-config schema:
managedPermissionsproperty before accepting the connection.Dependency update
@github/copilot-sdk:^1.0.10-preview.0@github/copilot:1.0.79-6Current diff: 24 files, +998/−423. Lockfile regeneration accounts for most deletions.
Validation
Development validation performed during this PR:
npm run typecheck-clientnpm run transpile-clientnode build/azure-pipelines/common/validatePackageLocks.ts origin/main./scripts/test.sh src/vs/platform/agentHost/test/common/ahpJsonlLogger.test.ts src/vs/platform/agentHost/test/common/agentHostSchema.test.ts src/vs/platform/agentHost/test/electron-browser/remoteAgentHostProtocolClient.test.ts src/vs/platform/agentHost/test/node/agentService.test.ts src/vs/platform/agentHost/test/node/protocolServerHandler.test.ts src/vs/platform/agentHost/test/node/copilotSessionLauncher.test.ts src/vs/platform/agentHost/test/node/copilotAgent.test.ts— 604 passing, 17 pending on the recorded runThe latest compatibility follow-up also adds coverage in
remoteAgentHostProtocolClient.test.tsandremoteAgentHostService.test.ts.Latest canonical-rule/diagnostics follow-up:
npm run typecheck-client./scripts/test.sh --run src/vs/platform/agentHost/test/common/agentHostSchema.test.ts --run src/vs/platform/agentHost/test/electron-browser/remoteAgentHostProtocolClient.test.ts --run src/vs/platform/agentHost/test/node/agentService.test.ts --run src/vs/platform/agentHost/test/node/copilotSessionLauncher.test.ts— 328 passing, 17 pendingask: ["Shell"]requires human approval even with Allow All enabled;deny: ["Shell"]overrides clientask: ["Shell"]without prompting.Latest reduction/bug-bash pass:
npm run typecheck-clientnode build/azure-pipelines/common/validatePackageLocks.ts origin/mainnpm run valid-layers-checknpm run precommitModel council follow-up: