Add managed permission settings to session startup - #2139
Conversation
| // Allow lists operations permitted without prompting. Every declared allow | ||
| // list across managed layers must admit an operation for it to be allowed. | ||
| Allow []string `json:"allow,omitempty"` |
135b25a to
3676a65
Compare
There was a problem hiding this comment.
Review details
Files not reviewed (2)
- go/rpc/zrpc.go: Generated file
- go/rpc/zrpc_encoding.go: Generated file
Suppressed comments (5)
go/types.go:1485
- An explicitly empty
allowlist is not equivalent to omittingallow: by this field's own contract, every present allow-list must admit an operation, so[]is a deny-all constraint.omitemptydrops that value and therefore removes the host's restriction, weakening the managed policy. Preserve the nil-versus-empty distinction (for example with a pointer slice or custom marshaling) and update the test that currently asserts omission.
Allow []string `json:"allow,omitempty"`
rust/src/types.rs:1763
- This public field documents a single legal literal but accepts and serializes any string. That defeats the typed contract and lets invalid policy reach the runtime. Use a public enum for
disable(the generated protocol already definesDisableBypassPermissionsMode) rather thanString.
pub disable_bypass_permissions_mode: Option<String>,
go/types.go:1478
- The contract permits only the
"disable"literal, but*stringaccepts and forwards arbitrary values. Expose a dedicated typed value/constant (the generated RPC package already hasrpc.DisableBypassPermissionsMode) so callers cannot accidentally construct an invalid managed policy.
DisableBypassPermissionsMode *string `json:"disableBypassPermissionsMode,omitempty"`
java/src/main/java/com/github/copilot/rpc/SessionConfig.java:108
- The Java generated RPC surface was not regenerated for this schema addition.
CopilotClient.getRpc().sessions.open(...)publicly consumes generatedSessionOpenOptions, but that record still has nomanagedSettingsfield or generated managed-settings types, so this feature is unavailable through Java's typed RPC API while the other generated mirrors include it. Regenerate Java RPC sources from the updated schema rather than hand-editing them.
private ManagedSettings managedSettings;
dotnet/src/Types.cs:3012
- This property claims a single legal
"disable"value but is an unrestricted string, so the new typed API accepts invalid policy and only fails later at runtime. Model it as a serialized enum/value type, consistent with other closed string-valued options inTypes.cs.
[JsonPropertyName("disableBypassPermissionsMode")]
public string? DisableBypassPermissionsMode { get; set; }
- Files reviewed: 25/30 changed files
- Comments generated: 2
- Review effort level: Balanced
| type SessionManagedPermissions struct { | ||
| // Permission rules that allow matching operations unless another managed source, deny, or | ||
| // ask rule restricts them. | ||
| Allow []string `json:"allow,omitzero"` |
| t.Run("omits empty permission arrays (omitempty idiom)", func(t *testing.T) { | ||
| // Go's `omitempty` drops both nil and empty slices; an empty rule list | ||
| // is semantically equivalent to no rules for that key. |
3676a65 to
5411c88
Compare
There was a problem hiding this comment.
Review details
Files not reviewed (2)
- go/rpc/zrpc.go: Generated file
- go/rpc/zrpc_encoding.go: Generated file
Suppressed comments (7)
rust/src/wire.rs:331
- This optional field is serialized as
"managedSettings": nullon every resume when unset because it is missing the neighboringskip_serializing_ifattribute. That violates the startup option's omission contract and may cause schema-validating runtimes to reject normal resumes.
pub managed_settings: Option<crate::types::ManagedSettings>,
go/types.go:1489
omitemptydrops a non-nil emptyAllowslice, but these states are not equivalent: an explicitly present empty allow list admits no operation under the documented intersection semantics, while an omitted list imposes no constraint. This can silently broaden an injected enterprise policy. Preserve non-nil empty slices (for example with Go 1.24'somitzero, as the generated RPC type does) and update the serialization test accordingly.
Allow []string `json:"allow,omitempty"`
go/client_test.go:3507
- This test codifies an unsafe equivalence for
allow: an explicit empty allow list admits nothing, whereas omittingallowcontributes no restriction. Update the test to require"allow": []after changing serialization to preserve non-nil empty slices.
// Go's `omitempty` drops both nil and empty slices; an empty rule list
// is semantically equivalent to no rules for that key.
java/src/main/java/com/github/copilot/rpc/ManagedSettingsPermissions.java:69
@returnis currently parsed as part of therulesparameter text rather than as a Javadoc block tag. Move it to a separate line.
* ask rules; @return this policy
java/src/main/java/com/github/copilot/rpc/ManagedSettingsPermissions.java:83
@returnis embedded in the parameter description, leaving the fluent setter's return value undocumented in generated Javadoc. Use a separate block tag.
* allow rules; @return this policy
java/src/main/java/com/github/copilot/rpc/ManagedSettingsPermissions.java:55
@returnis embedded in the@paramdescription, so generated Javadoc does not document the method's return value. Put it on its own block-tag line.
This issue also appears in the following locations of the same file:
- line 69
- line 83
* deny rules; @return this policy
java/src/main/java/com/github/copilot/rpc/ManagedSettings.java:27
- The inline
@returntext is part of the parameter description, not a Javadoc return tag. Split it onto its own block-tag line so the public fluent API is documented correctly.
* managed permission policy; @return this settings object
- Files reviewed: 26/31 changed files
- Comments generated: 1
- Review effort level: Balanced
| pub enable_managed_settings: Option<bool>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub is_experimental_mode: Option<bool>, | ||
| pub managed_settings: Option<crate::types::ManagedSettings>, |
5411c88 to
d599be9
Compare
…/resume
Add an optional per-session `managedSettings` field (permissions-only
contract) across all six language SDKs, alongside the existing
`enableManagedSettings` boolean. Hosts can inject enterprise permission
policy at session startup via:
managedSettings.permissions = {
disableBypassPermissionsMode?: "disable",
deny?: string[],
ask?: string[],
allow?: string[],
}
Semantics: startup-only (not persisted), must be re-supplied on resume,
composes restrictively with runtime-managed settings, and older runtimes
fail closed. Wired through hand-written wire types at both create and
resume in Node, Python, Go, .NET, Rust, and Java, plus tests, docs, and
a CHANGELOG entry. Generated RPC mirror types regenerated from the
runtime schema (TS/Python/Go/Rust; C# unaffected as it does not mirror
SessionOpenOptions). No SDK protocol bump.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Treat direct managedSettings injection as a managed session in every language SDK and document the compatible-runtime requirement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d5d4d699-33e2-4a55-9d48-57d2e483dd3d
Restore the managed-settings RPC definitions after rebasing onto the latest generated schema. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d5d4d699-33e2-4a55-9d48-57d2e483dd3d
Keep explicit empty Go rule arrays, omit unset Rust settings, and expose managed settings through the generated Java RPC surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d5d4d699-33e2-4a55-9d48-57d2e483dd3d
d599be9 to
a078848
Compare
There was a problem hiding this comment.
Review details
Files not reviewed (2)
- go/rpc/zrpc.go: Generated file
- go/rpc/zrpc_encoding.go: Generated file
Suppressed comments (7)
rust/src/types.rs:1786
ManagedSettingsexplicitly documents that it “currently” contains only permissions, so new managed-setting sections are expected. Without#[non_exhaustive], adding the next section will break downstream Rust struct literals; this also differs from the extensible public config structs elsewhere in this file (for exampleGitHubMcpToolConfigandSessionConfig).
rust/src/types.rs:1758- This new public policy struct is intended to evolve, but unlike the repository's other extensible Rust configuration types (for example
Tool,GitHubMcpToolConfig, andSessionConfig), it is exhaustive. Adding another permission field later would therefore be a source-breaking change for downstream struct literals. Mark it#[non_exhaustive]before publishing the type.
This issue also appears on line 1786 of the same file.
rust/src/types.rs:1763
- The wire contract permits only the literal
"disable", but this public field accepts any string, so invalid policy values compile and fail only when starting a session. Model this as a single-variant serialized enum (as the generatedDisableBypassPermissionsModetype does) so the SDK API cannot construct unsupported values.
go/types.go:1485 - The contract accepts only
"disable", but*stringallows callers to send arbitrary values and discover the error only at session startup. Use a named string type with aDisableBypassPermissionsModeDisableconstant, consistent withSectionOverrideActionandToolDefer, so the public API exposes the supported value explicitly.
DisableBypassPermissionsMode *string `json:"disableBypassPermissionsMode,omitempty"`
dotnet/src/Types.cs:3012
- This is a single-literal protocol field, but exposing it as
stringpermits unsupported values that fail only when the runtime validates session startup. Use a JSON string enum containingDisable, as done for other constrained SDK options such asCopilotToolDefer, to keep invalid policy values out of the public API.
[JsonPropertyName("disableBypassPermissionsMode")]
public string? DisableBypassPermissionsMode { get; set; }
dotnet/test/Unit/ClientSessionLifetimeTests.cs:511
- This test reaches into a private field by reflection, coupling it to an implementation detail rather than the SDK's public behavior. Exercise a permission request through the configured handler and assert the public
PermissionInvocation.ManagedSettingsEnabledvalue instead, so the test remains valid if session internals are refactored.
var managedField = typeof(CopilotSession).GetField("_managedSettingsEnabled", BindingFlags.Instance | BindingFlags.NonPublic)
?? throw new InvalidOperationException("Managed settings field was not found.");
Assert.True((bool)managedField.GetValue(session)!);
java/src/test/java/com/github/copilot/ManagedSettingsTest.java:54
- This test bypasses the public API with
getDeclaredField/setAccessible, making it depend on the private field name. Verify the managed flag through the public permission-handler invocation context instead; that tests the externally observable safeguard and avoids reflection-based access to session internals.
var field = CopilotSession.class.getDeclaredField("managedSettingsEnabled");
field.setAccessible(true);
assertEquals(true, field.getBoolean(session));
- Files reviewed: 26/35 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Files not reviewed (2)
- go/rpc/zrpc.go: Generated file
- go/rpc/zrpc_encoding.go: Generated file
Suppressed comments (14)
java/src/test/java/com/github/copilot/ManagedSettingsTest.java:54
- This test reaches package-private configuration code and then reflects into a private session field. Verify the safeguard through the public permission-handler behavior instead; otherwise internal refactors can break the test without changing the API contract.
SessionRequestBuilder.configureSession(session, new SessionConfig().setManagedSettings(settings));
var field = CopilotSession.class.getDeclaredField("managedSettingsEnabled");
field.setAccessible(true);
assertEquals(true, field.getBoolean(session));
rust/src/types.rs:1758
- This public struct is exhaustive even though the permission contract is explicitly additive. Adding another permission field later would break downstream struct literals and exhaustive patterns; mark it
#[non_exhaustive]now while the type is new.
rust/src/types.rs:1763 - The public contract restricts this value to the literal
"disable", butOption<String>accepts and serializes any value. Use a dedicated enum/newtype so invalid policies are rejected by the Rust type system rather than only by the runtime.
dotnet/src/Types.cs:3012 - The property accepts any string although the contract has a single
"disable"value. Model it as a nullable JSON string enum, consistent with the enum-backed mode properties elsewhere in this file, so invalid policy values cannot be serialized.
public string? DisableBypassPermissionsMode { get; set; }
rust/src/types.rs:1786
- This public top-level settings struct is exhaustive although its documentation says it currently carries only permissions. Future managed-settings siblings would therefore require a breaking Rust release; mark the new type
#[non_exhaustive].
go/types.go:1485 - This exposes an unrestricted string even though the wire contract permits only
"disable". Define a public named string type and constant (as this file does forToolDeferandSectionOverrideAction) so callers cannot accidentally pass an ordinary string and the API reflects the permissions schema.
DisableBypassPermissionsMode *string `json:"disableBypassPermissionsMode,omitempty"`
java/src/main/java/com/github/copilot/rpc/ManagedSettingsPermissions.java:40
- Although this setter validates at runtime, the public API still exposes the schema's literal as an arbitrary
String. Use an enum value (the same pattern asAgentMode) so unsupported policy values are not representable and callers get compile-time guidance.
public ManagedSettingsPermissions setDisableBypassPermissionsMode(String value) {
java/src/main/java/com/github/copilot/rpc/ManagedSettingsPermissions.java:55
@returnis embedded in the parameter description, so Javadoc does not recognize a return tag for this fluent public method. Put it on its own block-tag line.
* deny rules; @return this policy
java/src/main/java/com/github/copilot/rpc/ManagedSettingsPermissions.java:69
@returnis embedded in the parameter description, so the generated Javadoc loses the return contract. Put it on a separate block-tag line.
* ask rules; @return this policy
java/src/main/java/com/github/copilot/rpc/ManagedSettingsPermissions.java:83
@returnis embedded in the parameter description, so the generated Javadoc loses the return contract. Put it on a separate block-tag line.
* allow rules; @return this policy
java/src/main/java/com/github/copilot/rpc/ManagedSettings.java:27
@returnis part of the@paramprose here rather than a Javadoc block tag. Split it onto its own line so the fluent return value appears correctly in generated API documentation.
* managed permission policy; @return this settings object
java/src/test/java/com/github/copilot/ManagedSettingsTest.java:29
- This verifies serialization through the package-private request builder instead of the public client API. Exercise
createSession/resumeSessionagainst the Java test server so the test also covers the public forwarding path and does not depend on internals.
This issue also appears on line 50 of the same file.
var create = SessionRequestBuilder.buildCreateRequest(
new SessionConfig().setEnableManagedSettings(true).setManagedSettings(managedSettings),
"managed-create");
var resume = SessionRequestBuilder.buildResumeRequest("managed-resume",
new ResumeSessionConfig().setEnableManagedSettings(true).setManagedSettings(managedSettings));
java/src/test/java/com/github/copilot/ManagedSettingsTest.java:22
- The PR calls out explicit empty arrays—especially
allow: []—as security-relevant, but the Java test only serializes non-empty lists. Add a public-path assertion that an emptyallowlist remains[]rather than being omitted.
.setAllow(List.of("Read(**)"));
dotnet/test/Unit/ClientSessionLifetimeTests.cs:511
- This test reflects into
_managedSettingsEnabled, so it does not verify the behavior through the public .NET API and is coupled to a private field name. Trigger a permission request through the fake server and assert the publicApproveAllbehavior instead.
var managedField = typeof(CopilotSession).GetField("_managedSettingsEnabled", BindingFlags.Instance | BindingFlags.NonPublic)
?? throw new InvalidOperationException("Managed settings field was not found.");
Assert.True((bool)managedField.GetValue(session)!);
- Files reviewed: 26/35 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Why
SDK hosts need a typed, cross-language way to inject enterprise permission policy at session startup, independent of the runtime's server/device managed-settings fetch path.
Public API
Create and resume configuration in Node, Python, Go, .NET, Rust, and Java now expose the same permissions-only object:
Each SDK uses native language naming while serializing the same camelCase JSON. Generated RPC mirrors are updated for Node, Python, Go, Rust, and Java; .NET's generated RPC model does not mirror
SessionOpenOptions, so its high-level wire type remains handwritten.How it works
enableManagedSettingsremains independent and may be combined with direct injection.managedSettingsmarks the session managed in every SDK, so permissive built-in handlers such asapproveAllcannot bypass enterprise restrictions even when self-fetch is disabled.allow: [], which means no operation is admitted; it is not equivalent to an absent allowlist.null.Compatibility and rollout
Older runtimes may ignore the additive field. Hosts must not rely on injected policy until they ship a runtime whose schema and enforcement include
managedSettings.This PR should publish only after
github/copilot-agent-runtime#14000is released. Downstream hosts such as VS Code must then bump to the published SDK/runtime pair before removing temporary type shims or enabling enforcement.Validation
go vet