Expose managed approval requirement on permission requests - #2080
Expose managed approval requirement on permission requests#2080joshspicer wants to merge 36 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Exposes managed approval requirements to TypeScript permission handlers.
Changes:
- Adds optional
managedApprovalRequiredmetadata. - Adds package-root type coverage.
Show a summary per file
| File | Description |
|---|---|
nodejs/src/types.ts |
Overlays managed approval metadata onto permission requests. |
nodejs/test/session-event-types.test.ts |
Verifies the field is publicly importable. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Medium
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 18f1bbc1-6001-43e2-b293-724505087f6a
13c3d37 to
03184d2
Compare
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
nodejs/src/types.ts:1108
- This overlay only updates the standalone type and
PermissionHandler; the publicSessionEvent/PermissionRequestedDatatypes still come from the generated schema, wherepermissionRequestlacks this property. Consequently, an event-only host (the documented path whenonPermissionRequestis omitted) cannot compileevent.data.permissionRequest.managedApprovalRequiredeven though the runtime sends it. Please overlay thepermission.requestedevent payload as well so every public permission-request surface exposes the metadata.
export type PermissionRequest = GeneratedPermissionRequest & {
readonly managedApprovalRequired?: boolean;
};
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Medium
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: c7f00b84-b0a7-4cdf-aca9-ffd49737f26e
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
nodejs/src/types.ts:1145
- The behavior change leaves the public README inaccurate:
nodejs/README.md:39,:133, and:856-866still describeapproveAllas allowing every request/tool call. With a managed request it now returnsno-result, so following those docs can leave execution pending until the consumer explicitly resolves the permission. Please update the permission-handling docs and examples to describe this exception and how hosts should resolve it.
export const approveAll: PermissionHandler = (request) =>
request.managedApprovalRequired ? { kind: "no-result" } : { kind: "approve-once" };
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Medium
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
nodejs/README.md:869
- The immediately following custom-handler example still returns
approve-oncefor every non-shell request without checkingmanagedApprovalRequired. Copying it therefore auto-approves managed Read, Edit, or Domain asks, contradicting the human-approval requirement documented here. Update that example to route flagged requests through a human confirmation flow (or leave them unanswered).
For requests with `managedApprovalRequired: true`, `approveAll` returns `{ kind: "no-result" }`. The request remains pending and the host must present a human-facing confirmation flow to resolve it explicitly.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Medium
|
Linked end-to-end validation completed against SDK head Cross-layer audit of the latest runtime session and AHP transcript confirmed:
The assistant's final table overstated downstream tool success for two approved Domain asks (HTTP redirect/404) and mislabeled the |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- go/rpc/zsession_events.go: Generated file
Comments suppressed due to low confidence (7)
scripts/codegen/go.ts:1
- Hard-coding a type-name special case in the Go generator makes the output sensitive to naming changes and obscures why the method exists. If possible, drive this from schema metadata / codegen options (or isolate it in a small post-processing hook), so the behavior is explicit and less likely to break when types are renamed or reused.
rust/src/session.rs:1 managed_approval_requiredis extracted explicitly but only applied in theErr(_)fallback. To ensure the managed-approval flag is preserved even if deserialization succeeds but the field isn’t populated (e.g., due to rename/config mismatches), consider explicitly assigningdata.managed_approval_required = data.managed_approval_required.or(managed_approval_required)in theOk(mut data)path.
python/copilot/session.py:350- The docstring states that legacy direct callbacks cannot abstain, but the legacy callback path (
_handle_permission_request) still returns whatever the handler returns without enforcing this constraint. Either enforce this (e.g., translatePermissionNoResultinto a concrete decision or raise a clear error in the legacy path) or soften the documentation to match the actual behavior.
class PermissionNoResult:
"""Sentinel that leaves an event-dispatched permission request unanswered.
During event-based permission dispatch, the SDK suppresses its response so
another connected client, such as a human-facing host, can answer the pending
request. Legacy direct callbacks require a concrete decision and cannot abstain.
"""
go/permissions.go:19
- Go error strings should be lowercase and not start with a capital letter. Consider changing the message to start with
approveAll/approve all(and keeping it consistent with other languages if desired).
ApproveAll: func(_ PermissionRequest, invocation PermissionInvocation) (rpc.PermissionDecision, error) {
if invocation.ManagedSettingsEnabled {
return nil, errors.New("ApproveAll cannot be used when managed settings are enabled")
}
return &rpc.PermissionDecisionApproveOnce{}, nil
},
nodejs/src/session.ts:710
- Using
console.errorin a library will unconditionally write to stderr and can be noisy/uncontrolled for SDK consumers. Consider routing this through the same logging mechanism used elsewhere in the package (or making it opt-in via a logger callback) so callers can control verbosity and destinations.
} catch (error) {
if (this.disconnected) {
return;
}
console.error("Permission handler failed", {
sessionId: this.sessionId,
requestId,
error,
});
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:31
- Creating a dedicated
new ObjectMapper()here can diverge from the rest of the SDK’s Jackson configuration (modules, naming strategy, inclusion rules), which can lead to subtle conversion differences. Prefer reusing the SDK’s configured mapper (if there is one) or initialize this mapper to match the project-wide settings (e.g.,JsonMapper.builder().../findAndAddModules()as appropriate).
@JsonIgnoreProperties(ignoreUnknown = true)
public class PermissionRequest {
private static final ObjectMapper MAPPER = new ObjectMapper();
@JsonProperty("kind")
private String kind;
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:56
- Creating a dedicated
new ObjectMapper()here can diverge from the rest of the SDK’s Jackson configuration (modules, naming strategy, inclusion rules), which can lead to subtle conversion differences. Prefer reusing the SDK’s configured mapper (if there is one) or initialize this mapper to match the project-wide settings (e.g.,JsonMapper.builder().../findAndAddModules()as appropriate).
public static PermissionRequest fromJsonValue(Object value) {
if (value instanceof PermissionRequest request) {
return request;
}
return MAPPER.convertValue(value, PermissionRequest.class);
}
- Files reviewed: 37/38 changed files
- Comments generated: 2
- Review effort level: Low
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- go/rpc/zsession_events.go: Generated file
Comments suppressed due to low confidence (4)
scripts/codegen/go.ts:1
- Hard-coding
typeName === \"PermissionRequest\"in the Go codegen makes the generator brittle (rename-sensitive and non-discoverable for future unions needing similar extensions). Consider driving this from schema/metadata (e.g., a per-union config list of extra interface methods) or a predicate based on the union’s discriminator/source file, so the behavior stays correct if the type name changes and remains reusable.
go/permissions.go:19 - The Go
ApproveAllhelper currently ignores request-level managed-approval metadata (RequiresManagedApproval()), so a request explicitly marked as requiring human approval could still be auto-approved if the session flag is absent/mis-set. Recommend also checkingrequest.RequiresManagedApproval()and returning a no-result decision (or an error) to avoid bypassing managed policy. Additionally (optional), Go error strings conventionally start with a lowercase letter (e.g., "approveAll cannot be used...") for consistency with Go tooling and style.
ApproveAll: func(_ PermissionRequest, invocation PermissionInvocation) (rpc.PermissionDecision, error) {
if invocation.ManagedSettingsEnabled {
return nil, errors.New("ApproveAll cannot be used when managed settings are enabled")
}
return &rpc.PermissionDecisionApproveOnce{}, nil
},
nodejs/src/types.ts:1135
approveAllignores request-levelmanagedApprovalRequired. For defense-in-depth, consider checkingrequest.managedApprovalRequired === true(for variants that carry it) and returning{ kind: \"no-result\" }(or throwing) so the SDK cannot automatically approve requests that the runtime marks as requiring an explicit human decision—even if the session-level managed-settings flag is missing or incorrect.
export const approveAll: PermissionHandler = (_request, invocation) => {
if (invocation.managedSettingsEnabled) {
throw new Error("approveAll cannot be used when managed settings are enabled");
}
return { kind: "approve-once" };
};
python/copilot/session.py:383
PermissionHandler.approve_allonly gates on the session-levelmanaged_settings_enabled. To prevent accidental auto-approval of requests explicitly markedmanaged_approval_required(defense-in-depth), consider also checkinggetattr(request, \"managed_approval_required\", False) is Trueand returningPermissionNoResult()(event-based dispatch) or a conservative denial for legacy callbacks.
def approve_all(
request: PermissionRequest, invocation: PermissionInvocation
) -> PermissionRequestResult:
if invocation["managed_settings_enabled"]:
raise RuntimeError("approve_all cannot be used when managed settings are enabled")
return PermissionDecisionApproveOnce()
- Files reviewed: 37/38 changed files
- Comments generated: 0 new
- Review effort level: Low
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- go/rpc/zsession_events.go: Generated file
Comments suppressed due to low confidence (6)
scripts/codegen/go.ts:1
- This generator change hard-codes a specific union name (
"PermissionRequest") to inject an interface method, which is brittle and easy to miss if the schema name changes or if a second managed-aware union is introduced. Prefer driving this off schema metadata (e.g., union/variant contains amanagedApprovalRequiredfield) or a generator option list, so the behavior is declarative and less likely to silently diverge from the schema.
python/copilot/session.py:384 PermissionHandler.approve_allwill raiseKeyErrorif a caller invokes it directly with the previously-valid invocation shape (e.g.,{"session_id": "..."}), since it now indexesinvocation["managed_settings_enabled"]. To keep this helper backward-compatible, use a default (e.g.,invocation.get("managed_settings_enabled", False)) and consider makingmanaged_settings_enabledoptional in theTypedDict(e.g.,total=False) while still populating it fromCopilotSession.
request: PermissionRequest, invocation: PermissionInvocation
) -> PermissionRequestResult:
if invocation["managed_settings_enabled"]:
raise RuntimeError("approve_all cannot be used when managed settings are enabled")
if getattr(request, "managed_approval_required", False) is True:
return PermissionNoResult()
nodejs/src/types.ts:1125
- Making
managedSettingsEnabledrequired in the exportedPermissionHandlerinvocation type is a breaking change for consumers who implemented handlers as(request, invocation: { sessionId: string }) => .... Consider makingmanagedSettingsEnabledoptional with a default offalseinapproveAll(and when constructing invocations), preserving source compatibility while still providing the new signal to updated handlers.
export type PermissionHandler = (
request: PermissionRequest,
invocation: { sessionId: string; managedSettingsEnabled: boolean }
) => Promise<PermissionRequestResult> | PermissionRequestResult;
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:28
fromJsonValueuses a new standaloneObjectMapperinstance, which can diverge from the SDK’s configured mapper (modules, coercions, naming strategies, features) used elsewhere. To avoid inconsistent conversions, reuse the SDK’s canonicalObjectMapper(if one exists in the library) or accept anObjectMapperparameter so callers can supply the same configuration used to deserialize the event.
private static final ObjectMapper MAPPER = new ObjectMapper();
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:56
fromJsonValueuses a new standaloneObjectMapperinstance, which can diverge from the SDK’s configured mapper (modules, coercions, naming strategies, features) used elsewhere. To avoid inconsistent conversions, reuse the SDK’s canonicalObjectMapper(if one exists in the library) or accept anObjectMapperparameter so callers can supply the same configuration used to deserialize the event.
public static PermissionRequest fromJsonValue(Object value) {
if (value instanceof PermissionRequest request) {
return request;
}
return MAPPER.convertValue(value, PermissionRequest.class);
}
go/permissions.go:17
- The error message uses
approveAll(lower camelCase), but the public Go API isApproveAll. Aligning the message with the exported identifier (e.g., “ApproveAll cannot be used…”) makes it clearer for Go users and matches the naming used in docs.
if invocation.ManagedSettingsEnabled {
return nil, errors.New("approveAll cannot be used when managed settings are enabled")
}
- Files reviewed: 37/38 changed files
- Comments generated: 1
- Review effort level: Low
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- go/rpc/zsession_events.go: Generated file
Comments suppressed due to low confidence (5)
scripts/codegen/go.ts:1
- This codegen special-case hardcodes a single union name ("PermissionRequest"), which makes the generator brittle and easy to forget when schemas/types evolve. A more maintainable (and less API-breaking) approach in Go would be to avoid extending the generated
PermissionRequestinterface and instead provide a helper function (e.g.,RequiresManagedApproval(request PermissionRequest) bool) implemented via a type-switch (and Raw JSON fallback), keeping codegen schema-driven and minimizing public surface area changes.
rust/src/types.rs:1 permission_request_data()now setsPermissionRequestData.extrato the full event payload (includingrequestIdand possibly the nestedpermissionRequestobject), not just the request params object. The doc comment aboveextrashould be updated to match the new semantics so downstream consumers know what shape to expect inextra.
nodejs/src/session.ts:710- Using
console.errorin a library can create noisy/unstructured output for consumers and is hard to integrate with existing logging/telemetry. Prefer routing this through the project’s established logging mechanism (if present), or exposing it via a configurable logger callback so hosts can control formatting, redaction, and destinations.
} catch (error) {
if (this.disconnected) {
return;
}
console.error("Permission handler failed", {
sessionId: this.sessionId,
requestId,
error,
});
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:31
- Introducing a new
ObjectMapperinstance here risks diverging from whatever configuration/modules the SDK uses elsewhere (dates, naming strategies, unknown-field handling, polymorphism), and adds another mapper to maintain. Consider reusing the SDK’s shared/configured mapper (or a centralized mapper factory) forconvertValue, so event conversions behave consistently with the rest of the SDK.
private static final ObjectMapper MAPPER = new ObjectMapper();
@JsonProperty("kind")
private String kind;
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:56
- Introducing a new
ObjectMapperinstance here risks diverging from whatever configuration/modules the SDK uses elsewhere (dates, naming strategies, unknown-field handling, polymorphism), and adds another mapper to maintain. Consider reusing the SDK’s shared/configured mapper (or a centralized mapper factory) forconvertValue, so event conversions behave consistently with the rest of the SDK.
public static PermissionRequest fromJsonValue(Object value) {
if (value instanceof PermissionRequest request) {
return request;
}
return MAPPER.convertValue(value, PermissionRequest.class);
}
- Files reviewed: 37/38 changed files
- Comments generated: 1
- Review effort level: Low
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- go/rpc/zsession_events.go: Generated file
Comments suppressed due to low confidence (7)
rust/src/types.rs:1
managed_settings_enabledis#[serde(default)]but will serialize asfalseby default, which can change JSON output for any codepaths that serializePermissionRequestData(even if the field is conceptually internal). Consider addingskip_serializing_iffor thefalsecase (via a smallis_falsehelper) to avoid emitting the field unless it’s true.
scripts/codegen/go.ts:1- This hard-codes a type-name string special-case into the Go code generator, which is brittle and easy to miss when the schema evolves (e.g., renames, additional unions with similar needs). Consider driving this via schema metadata/config (e.g., a per-type feature flag), or generating the method based on the presence of
managedApprovalRequiredacross union variants rather than a literaltypeNamecheck.
python/copilot/session.py:373 - Making
managed_settings_enabledrequired inPermissionInvocation(and narrowing the handler signature to that TypedDict) is a typing-breaking change for users who annotated handlers as acceptingdict[str, str]or a TypedDict without this key. To keep type-checking compatibility, consider making the TypedDict non-total (or markingmanaged_settings_enabledas optional) while continuing to always pass the key at runtime.
class PermissionInvocation(TypedDict):
session_id: str
managed_settings_enabled: bool
_PermissionHandlerFn = Callable[
[PermissionRequest, PermissionInvocation],
PermissionRequestResult | Awaitable[PermissionRequestResult],
]
python/copilot/session.py:1465
- The
CopilotSession.__init__signature addsmanaged_settings_enabled, but the constructor docstring/Args in the surrounding context does not document this new parameter. Please update the docstring to describe what it does and when consumers should expect it to be set (even if the constructor is internal).
self,
session_id: str,
client: Any,
workspace_path: os.PathLike[str] | str | None = None,
managed_settings_enabled: bool = False,
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:28
- Creating a standalone
new ObjectMapper()here risks diverging from any SDK-wide ObjectMapper configuration (modules, naming strategies, coercion rules, etc.) used elsewhere, and makes behavior harder to control. Prefer reusing the SDK’s configured mapper (or injecting one), or at least building the mapper with the same configuration defaults used across the project.
private static final ObjectMapper MAPPER = new ObjectMapper();
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:56
- Creating a standalone
new ObjectMapper()here risks diverging from any SDK-wide ObjectMapper configuration (modules, naming strategies, coercion rules, etc.) used elsewhere, and makes behavior harder to control. Prefer reusing the SDK’s configured mapper (or injecting one), or at least building the mapper with the same configuration defaults used across the project.
public static PermissionRequest fromJsonValue(Object value) {
if (value instanceof PermissionRequest request) {
return request;
}
return MAPPER.convertValue(value, PermissionRequest.class);
}
go/rpc/permission_request_managed_approval.go:78
- If unmarshalling fails, this returns
false, which can causeApproveAllto auto-approve a request even though the SDK couldn’t inspect managed-approval metadata (undermining the defense-in-depth goal described elsewhere in this PR). Consider a conservative fallback of returningtrueon unmarshal errors (or otherwise ensuring unknown/invalid shapes don’t get silently approved).
func (r RawPermissionRequest) RequiresManagedApproval() bool {
var metadata struct {
ManagedApprovalRequired *bool `json:"managedApprovalRequired"`
}
return json.Unmarshal(r.Raw, &metadata) == nil && managedApprovalRequired(metadata.ManagedApprovalRequired)
}
- Files reviewed: 37/38 changed files
- Comments generated: 1
- Review effort level: Low
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- go/rpc/zsession_events.go: Generated file
Comments suppressed due to low confidence (6)
scripts/codegen/go.ts:1
- This hard-coded
typeName === \"PermissionRequest\"special-case makes Go codegen brittle (renames or additional unions needing the same method will require more ad-hoc branching). Prefer driving this from schema/metadata (e.g., a generator feature flag for that union) or centralizing the special-case behind a clearly named helper/constant so it’s discoverable and easier to extend.
rust/src/session.rs:1 managed_approval_requiredis computed unconditionally but only used in theErr(_)fallback. Either (a) move the extraction into the error branch to avoid unnecessary JSON field lookups, or (b) also apply it in theOk(mut data)branch as a defense-in-depth fallback (e.g., populate it when deserialization succeeds but the field is missing/unmapped).
python/copilot/session.py:367- This changes the public type contract for
invocationfromdict[str, str]to aTypedDictwith required keys, which can break static type-checking for existing user handlers (and for any internal call sites that pass onlysession_id). Consider makingmanaged_settings_enabledoptional (e.g.,TypedDict, total=Falsewithmanaged_settings_enabledabsent by default, or usingNotRequired[bool]) and documenting the key as best-effort metadata.
class PermissionInvocation(TypedDict):
session_id: str
managed_settings_enabled: bool
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:28
- Using a new default
ObjectMapper()here may diverge from the SDK’s configured Jackson behavior (modules, naming strategy, coercion settings), which can yield subtle conversion differences vs other serialization/deserialization paths. Prefer reusing the project’s shared/configured mapper (or constructing viaJsonMapper.builder().findAndAddModules()with the same configuration) to keep event-value conversion consistent.
private static final ObjectMapper MAPPER = new ObjectMapper();
java/src/main/java/com/github/copilot/rpc/PermissionRequest.java:56
- Using a new default
ObjectMapper()here may diverge from the SDK’s configured Jackson behavior (modules, naming strategy, coercion settings), which can yield subtle conversion differences vs other serialization/deserialization paths. Prefer reusing the project’s shared/configured mapper (or constructing viaJsonMapper.builder().findAndAddModules()with the same configuration) to keep event-value conversion consistent.
public static PermissionRequest fromJsonValue(Object value) {
if (value instanceof PermissionRequest request) {
return request;
}
return MAPPER.convertValue(value, PermissionRequest.class);
}
nodejs/src/session.ts:710
- Logging directly to
console.errorfrom a library can be noisy and may unintentionally expose sensitive details (theerrorobject, identifiers) in production logs. Prefer routing this through an injectable logger / existing debug facility (or at least gating behind a debug flag) while still returning the safeuser-not-availablefallback.
} catch (error) {
if (this.disconnected) {
return;
}
console.error("Permission handler failed", {
sessionId: this.sessionId,
requestId,
error,
});
- Files reviewed: 37/38 changed files
- Comments generated: 1
- Review effort level: Low
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Why
Enterprise-managed permission
askrules must be answered by a person. SDK hosts need to distinguish those requests from ordinary permission prompts that client settings may auto-approve.This is the public SDK contract for:
The initial runtime scope supports managed Shell, Read, Edit, and Domain selectors. Tool and MCP selectors are deferred.
What
Exposes optional managed-approval metadata on public permission request and
permission.requestedevent paths in every SDK:managedApprovalRequiredmanaged_approval_requiredManagedApprovalRequiredandRequiresManagedApproval()ManagedApprovalRequiredmanaged_approval_requiredgetManagedApprovalRequired()andPermissionRequest.fromJsonValue(...)for generated event valuesBuilt-in approve-all handlers continue approving requests when managed settings are disabled. When managed settings are enabled for a session, they fail loudly on the first permission dispatch so hosts without a human-facing permission UI do not silently leave managed asks hanging. Custom handlers can inspect the request-level managed flag to obtain an explicit human decision before approving affected requests.
Python, Go, and Rust codegen augment the pinned CLI schema until the runtime schema update ships in the repository's published CLI dependency. Regeneration is deterministic and the committed generated diff contains only the managed approval field.
Validation
go test ./...,golangci-lint run ./..., and Go docs validation-D warnings, nightly rustfmt, and reproducible codegen; the full replay E2E run had two unrelated snapshot mismatchesmvn verify, focused permission/dispatcher tests, Spotless, Checkstyle, and Java docs validation