Expose managed approval requirement on permission requests - #2080
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
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
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
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 |
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 (2)
java/src/main/java/com/github/copilot/RpcHandlerDispatcher.java:223
- Suppressing a response here leaves the protocol-v2
permission.requestJSON-RPC call permanently unresolved. Unlike thepermission.requestedevent path, this callback has no permissionrequestIdexposed to the host (only the transport ID is available here), so the request cannot later be completed throughhandlePendingPermissionRequest; the tool call simply hangs. KeepNO_RESULThandling protocol-aware rather than applying the v3/event abstention behavior to this direct-request path.
if (PermissionRequestResultKind.NO_RESULT.getValue().equalsIgnoreCase(result.getKind())) {
return;
python/copilot/session.py:377
- This new built-in return path conflicts with
PermissionNoResult's public docstring above, which says the sentinel is only meaningful for protocol-v1, is rejected by v2, and causes the SDK to raiseValueError. The current event handler instead suppresses the response, and the README now tells hosts to rely on that behavior for managed requests; update the API docstring so users are not told the newly recommended result is unsupported.
if request.managed_approval_required is True:
return PermissionNoResult()
- Files reviewed: 33/36 changed files
- Comments generated: 1
- Review effort level: Medium
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
Suppressed comments (2)
java/README.md:70
- This snapshot coordinate is already out of sync with the module version:
java/pom.xml:10declares1.0.10-preview.1-SNAPSHOT. Copying this README example will therefore request the previous preview (and may fail if that snapshot is not published). Keep the example aligned with the POM.
implementation 'com.github:copilot-sdk-java:1.0.10-preview.0-SNAPSHOT'
dotnet/src/PermissionRequest.cs:17
- This base property is hidden by separate
ManagedApprovalRequiredproperties already generated onPermissionRequestShell,Write,Read, andUrl(for exampleGenerated/SessionEvents.cs:6959-6962). Polymorphic deserialization populates the derived property, so code that receives the handler's declaredPermissionRequesttype and readsrequest.ManagedApprovalRequiredsees the base property's defaultnulleven when the wire value istrue. The field needs a single storage location (for example, generate it on the base type and stop generating duplicate derived properties) so the advertised request-level API works without variant-specific casts.
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
[JsonPropertyName("managedApprovalRequired")]
public bool? ManagedApprovalRequired { get; set; }
- Files reviewed: 42/45 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Review details
Files not reviewed (3)
- go/rpc/zrpc.go: Generated file
- go/rpc/zsession_encoding.go: Generated file
- go/rpc/zsession_events.go: Generated file
Suppressed comments (2)
java/README.md:70
- The documented snapshot coordinate does not match this module's current Maven version:
java/pom.xml:10declares1.0.10-preview.1-SNAPSHOT. Users copying this dependency will request a different (and potentially unpublished) artifact instead of the current development snapshot.
implementation 'com.github:copilot-sdk-java:1.0.10-preview.0-SNAPSHOT'
go/rpc/zsession_encoding.go:25
- The regenerated Go output is no longer
gofmt-formatted (these struct fields would be column-aligned bygofmt), and the same formatting loss repeats throughout this file. Please rungofmtafter generation and ensure the generator's formatter step does not silently skip this output; otherwise this PR introduces a large unrelated generated diff and future regeneration will remain noisy.
- Files reviewed: 42/51 changed files
- Comments generated: 2
- Review effort level: Balanced
Co-authored-by: Copilot <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
Suppressed comments (5)
dotnet/src/PermissionHandlers.cs:33
- These variants inherit the new base
ManagedApprovalRequiredproperty, but this arm ignores it and always reports false. A flagged MCP/custom-tool/etc. request therefore gets auto-approved despite the property's documented contract that automatic approval must be bypassed. The runtime's initial selector scope is narrower, but the public request type andApproveAllmust still honor metadata when it is present.
PermissionRequestMcp
or PermissionRequestMemory
or PermissionRequestCustomTool
or PermissionRequestHook
or PermissionRequestExtensionManagement
java/README.md:70
- This snapshot coordinate is stale:
java/pom.xml:10declares1.0.10-preview.1-SNAPSHOT, so copying this README example requests a different build. Keep the documented version aligned with the module version.
implementation 'com.github:copilot-sdk-java:1.0.10-preview.0-SNAPSHOT'
nodejs/src/session.ts:1130
- This catch also covers
handlePendingPermissionRequest, so an RPC send failure is logged as a handler failure even when the handler completed successfully. Use a message that includes response delivery (or split the try blocks) so operational logs identify the failing stage accurately.
console.error("Permission handler failed", {
sessionId: this.sessionId,
requestId,
error,
python/copilot/session.py:2162
- The enclosing try includes the response RPC at lines 2143-2149, so this message blames the handler when sending an otherwise valid decision fails. Use a stage-neutral message or separate handler and transport error handling.
dotnet/src/Session.cs:966 - This catch also handles failures from
HandlePendingPermissionRequestAsync, so it can report “Permission handler failed” after the handler succeeded and only response delivery failed. Make the log message cover both stages or split their exception handling.
_logger.LogError(ex, "Permission handler failed. SessionId={SessionId}, RequestId={RequestId}", SessionId, requestId);
- Files reviewed: 43/47 changed files
- Comments generated: 1
- Review effort level: Balanced
joshspicer
left a comment
There was a problem hiding this comment.
Addressed current .NET review feedback in bd1f880.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Run status: HEAD |
There was a problem hiding this comment.
Review details
Files not reviewed (1)
- go/rpc/zsession_events.go: Generated file
Suppressed comments (1)
python/copilot/session.py:2561
- On the supported legacy permission callback path, this
RuntimeErroris caught by_handle_permission_requestand logged only at DEBUG (session.py:2577-2584) before returningUserNotAvailable. With normal logging,approve_alltherefore does not “fail loudly” when managed settings are enabled, contrary to the PR contract. Log that caught exception at error level (as the event-dispatch path now does) so the host can detect the misconfiguration.
- Files reviewed: 42/46 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
Suppressed comments (14)
go/README.md:692
- This paragraph still says
ApproveAllapproves ordinary requests in managed sessions. The required behavior is to fail on the first dispatch whenever managed settings are enabled; request-levelRequiresManagedApproval()abstention applies only as a fallback when that session signal is absent.
`ApproveAll` leaves requests with `RequiresManagedApproval()` pending and approves ordinary requests automatically.
python/test_managed_permissions.py:44
- Because this invocation explicitly sets
managed_settings_enabledto true,approve_allmust raise before consultingmanaged_approval_required. ExpectingPermissionNoResulthere locks in behavior that contradicts the managed-session fail-loud contract; the no-result fallback should be tested only with the session flag absent.
nodejs/src/types.ts:1153 approveAlldiscards the session flag, so an ordinary request is still auto-approved whenmanagedSettingsEnabledis true. This contradicts the documented/PR contract that the built-in handler must fail on the first permission dispatch in a managed session, allowing a misconfigured host to continue silently instead of surfacing that it needs a human-facing handler.
export const approveAll: PermissionHandler = (request, invocation) => {
void invocation;
if ("managedApprovalRequired" in request && request.managedApprovalRequired === true) {
return { kind: "no-result" };
}
return { kind: "approve-once" };
go/permissions.go:16
ApproveAllignores the session-level managed-settings signal and only checks the individual request. It therefore auto-approves an ordinary request in a managed session instead of failing loudly on the first dispatch as the PR contract requires, leaving hosts without a human approval flow silently misconfigured.
ApproveAll: func(request PermissionRequest, invocation PermissionInvocation) (rpc.PermissionDecision, error) {
if request.RequiresManagedApproval() {
return &rpc.PermissionDecisionNoResult{}, nil
}
return &rpc.PermissionDecisionApproveOnce{}, nil
scripts/codegen/csharp.ts:900
- Emitting
newcreates a second independent property on each derived request. Polymorphic deserialization populates the derived slot, but custom handlers receivePermissionRequestand a genericrequest.ManagedApprovalRequiredreads the null base slot, so they can miss a managed ask and auto-approve it. Generate one shared property across the hierarchy rather than hiding the base property.
python/copilot/session.py:383 approve_allnever checksmanaged_settings_enabled, so it auto-approves ordinary requests even in a managed session. The public documentation and PR contract say this built-in handler must raise on the first permission dispatch when managed settings are enabled, otherwise a host without a human approval UI can remain silently misconfigured.
java/README.md:70- This dependency version does not match the module version declared in
java/pom.xml(1.0.10-preview.1-SNAPSHOT). The quick-start snippet now directs users to a different snapshot than the one this source tree builds and publishes; keep the README coordinate synchronized with the POM.
implementation 'com.github:copilot-sdk-java:1.0.10-preview.1-SNAPSHOT'
go/session.go:70
- The new session flag is stored here but is never copied into
PermissionInvocation(which still contains onlySessionID), so neitherApproveAllnor a custom handler can observe that managed settings are enabled. The Go path therefore cannot implement the required first-dispatch failure; propagate this state through the invocation or wrap the built-in handler with session state.
managedSettings bool
python/test_managed_permissions.py:37
- This test codifies behavior that contradicts the documented contract:
approve_allmust raise on the first permission dispatch whenever managed settings are enabled, rather than approving an ordinary request. Update it to expect the failure so the regression in the built-in handler is caught.
This issue also appears on line 40 of the same file.
go/README.md:224
- This documents request-level abstention as the complete
ApproveAllbehavior, but the PR contract requires the built-in handler to fail on the first permission dispatch whenever managed settings are enabled. Document that managed sessions require a custom human-facing handler, while retaining request-level abstention only as a defense-in-depth fallback when the session signal is absent.
This issue also appears on line 692 of the same file.
- `OnPermissionRequest` (PermissionHandlerFunc): Optional handler called before each tool execution to approve or deny it. When nil, permission requests are emitted as events and left pending for manual resolution. `copilot.PermissionHandler.ApproveAll` approves ordinary requests and leaves managed requests pending. Custom handlers can inspect `RequiresManagedApproval()` for human-facing confirmation logic. See [Permission Handling](#permission-handling) section.
nodejs/test/session-event-types.test.ts:176
- This test asserts the opposite of the managed-session contract: with
managedSettingsEnabled: true,approveAllmust fail on the first dispatch even when the individual request is ordinary. As written, the test locks in the silent auto-approval bug instead of detecting it.
it("approves ordinary requests and leaves managed requests pending", () => {
expect(
approveAll(
{
kind: "url",
url: "https://api.example.com/data",
intention: "Fetch ordinary data",
},
{ sessionId: "session-1", managedSettingsEnabled: true }
)
).toEqual({ kind: "approve-once" });
nodejs/test/client.test.ts:38
- Passing
managedSettingsEnabled: truemust makeapproveAllthrow before considering request-level metadata. This expectation instead preserves the old abstention behavior and would fail once the required managed-session guard is implemented.
it("leaves managed requests pending even when managed settings are enabled", () => {
expect(
approveAll(
{ ...request, managedApprovalRequired: true },
{ ...invocation, managedSettingsEnabled: true }
nodejs/test/session-event-types.test.ts:182
- This second assertion also uses a managed-session invocation, so the required outcome is a loud failure, not
{ kind: "no-result" }. Request-level abstention is only the defense-in-depth fallback when the session flag is absent.
expect(
approveAll(
{
kind: "url",
url: "https://api.example.com/data",
dotnet/src/PermissionHandlers.cs:34
- These known variants inherit the public
ManagedApprovalRequiredproperty, but this branch hard-codes them tofalse. A request such as MCP carryingmanagedApprovalRequired: trueis therefore auto-approved when the session-level flag is absent, even though every other SDK exposes the metadata uniformly and the fallback is supposed to bypass automatic approval. Read the shared flag for every known variant; only truly unknown/raw requests need unconditional fail-closed behavior.
return request.GetType() == typeof(PermissionRequest)
&& request.Kind is not ("shell"
or "write"
or "read"
or "mcp"
- Files reviewed: 41/45 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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
Suppressed comments (1)
scripts/codegen/python.ts:2847
- Regenerating with this augmentation inserts the new optional dataclass field alphabetically before existing optional fields (for example,
PermissionRequestCustomTool.managed_approval_requirednow precedestool_call_id). Existing positional construction such asPermissionRequestCustomTool(desc, name, args, tool_call_id)silently binds the ID to the new boolean field and later fails serialization. Preserve the existing positional parameter order by emitting this new field after all existing fields (or otherwise provide a compatibility-preserving constructor).
- Files reviewed: 42/46 changed files
- Comments generated: 1
- Review effort level: Balanced
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
Suppressed comments (1)
dotnet/README.md:812
- This sample only abstains for the four currently emitted managed selector variants, even though
ManagedApprovalRequirednow lives on thePermissionRequestbase and can be set on every variant. A handler copied from the docs would therefore auto-approve an MCP/custom/future request carrying the managed flag. Check the base property directly so the documented defense-in-depth rule applies to all requests.
if (request is PermissionRequestShell { ManagedApprovalRequired: true }
or PermissionRequestWrite { ManagedApprovalRequired: true }
or PermissionRequestRead { ManagedApprovalRequired: true }
or PermissionRequestUrl { ManagedApprovalRequired: true })
- Files reviewed: 42/46 changed files
- Comments generated: 1
- Review effort level: Balanced
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
Suppressed comments (1)
scripts/codegen/python.ts:2262
- This reorders an already-public positional argument on every class whose schema already contained
managedApprovalRequired(for example,PermissionRequestReadchanges its fourth argument frommanaged_approval_requiredtorequest_sandbox_bypass, as shown in the generated diff). The compatibility test only covers a newly augmented variant, so existing positional calls can now silently bind values to the wrong fields. Append the field only for definitions where this PR injected it, while retaining the prior order for Read/Shell/URL/Write and the prompt request classes.
- Files reviewed: 42/46 changed files
- Comments generated: 1
- Review effort level: Balanced
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>
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 fail loudly on the first permission dispatch when managed settings are enabled, surfacing hosts that lack a human-facing permission flow. When the session-level signal is absent, they retain a defense-in-depth request-level fallback: ordinary requests are approved, while requests carrying managed-approval metadata are left unanswered for explicit human resolution. Custom handlers must inspect the request-level managed flag before applying automatic approval.
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 codegenmvn verify, focused permission/dispatcher tests, Spotless, Checkstyle, and Java docs validation