sdk: Forward decisionContext on permission replies across languages - #2294
Open
aymenfurter wants to merge 9 commits into
Open
sdk: Forward decisionContext on permission replies across languages#2294aymenfurter wants to merge 9 commits into
aymenfurter wants to merge 9 commits into
Conversation
The runtime emits `auto_approval_decision` telemetry only when a client supplies an explicit `decisionContext` alongside its permission reply. The generated wire types already carry the optional field, but the hand-written reply path built a fixed three-key JSON literal and had no way for a PermissionHandler to attribute its decision. Add `PermissionResult::AttributedDecision` plus a `with_context` builder, and forward the context as a top-level sibling of `result`. When no context is supplied the emitted params are byte-identical to before, so legacy behavior is preserved exactly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Permission handlers can now attach optional provenance describing how and where a decision was reached. The SDK forwards it to the runtime as a sibling of `result` -- never nested inside it -- so auto-approval decisions made programmatically can be attributed. The wire schema and every language's generated types already accepted the field; only the hand-written reply paths never populated it. No schema, codegen, or protocol version change is required. Fully additive: handlers returning a plain decision emit a payload byte-identical to before, with no `decisionContext` key at all. No-result suppression is preserved in every language. Per CONTRIBUTING.md, the feature is implemented in sync across all six SDKs: - Rust: PermissionResult::AttributedDecision + with_context() - Node: AttributedPermissionResult + withDecisionContext() - Python: AttributedPermissionResult + with_decision_context() - Go: AttributedPermissionResult + WithDecisionContext() - .NET: PermissionDecision.WithContext() - Java: PermissionRequestResult.withContext() Each language gains focused unit tests asserting the sibling placement, the byte-identical legacy payload, replace-not-nest on re-application, and preserved no-result suppression. Node and Rust add end-to-end coverage against a CLI carrying the runtime-side support. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for #2294 · sonnet46 53.2 AIC · ⌖ 5.69 AIC · ⊞ 6.6K
Java accepted null as a "clear" operation while .NET rejects it and the other SDKs disallow it at the type level. Since null is Java's default, an uninitialized variable would have silently dropped the context -- producing exactly the unattributed telemetry this feature removes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional permission-decision provenance forwarding across all six SDKs while preserving the legacy wire shape when absent.
Changes:
- Adds language-specific APIs for attaching
decisionContext. - Forwards context beside
resultin permission RPCs. - Adds unit and E2E coverage plus compatibility documentation.
Show a summary per file
| File | Description |
|---|---|
test/snapshots/permissions/should_honor_a_decision_annotated_with_decisioncontext.yaml |
Adds shared permission E2E fixture. |
rust/tests/e2e/permissions.rs |
Tests attributed rejection end to end. |
rust/src/types.rs |
Re-exports generated context types. |
rust/src/session.rs |
Builds attributed permission RPC parameters. |
rust/src/handler.rs |
Adds attributed permission results. |
python/test_permission_decision_context.py |
Tests Python serialization behavior. |
python/copilot/session.py |
Adds and forwards attributed results. |
python/copilot/__init__.py |
Exports the new Python API. |
nodejs/test/e2e/permissions.e2e.test.ts |
Verifies live RPC shape and behavior. |
nodejs/test/client.test.ts |
Tests Node.js attribution handling. |
nodejs/src/types.ts |
Defines attributed result helpers and types. |
nodejs/src/session.ts |
Forwards context in permission replies. |
nodejs/src/index.ts |
Exports the new Node.js API. |
java/src/test/java/com/github/copilot/rpc/PermissionRequestResultDecisionContextTest.java |
Tests Java result serialization. |
java/src/main/java/com/github/copilot/rpc/PermissionRequestResult.java |
Stores optional decision context. |
java/src/main/java/com/github/copilot/CopilotSession.java |
Passes context to the generated RPC. |
go/types.go |
Exposes generated decision-context types. |
go/session.go |
Unwraps and forwards attributed decisions. |
go/permissions.go |
Adds the Go attribution wrapper. |
go/permission_context_test.go |
Tests raw Go JSON-RPC output. |
dotnet/test/Unit/ClientSessionLifetimeTests.cs |
Tests .NET forwarding and omission. |
dotnet/src/Session.cs |
Passes context through the RPC client. |
dotnet/src/PermissionDecision.cs |
Adds fluent context attachment. |
docs/troubleshooting/compatibility.md |
Documents optional attribution support. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 5
- Review effort level: Balanced
stephentoub
reviewed
Aug 7, 2026
stephentoub
reviewed
Aug 7, 2026
Go embedded the decision interface in AttributedPermissionResult, which promotes the interface methods to the value type. A handler returning `*WithDecisionContext(...)` therefore satisfied rpc.PermissionDecision but slipped past the pointer-only type assertion: the wrapper itself was sent as `result` and the context was silently dropped. Both the unwrap in the session and the replace-not-nest check now accept either form, with regression tests that fail against the pointer-only code. Rust PermissionResult gains #[non_exhaustive], matching the convention used throughout this crate, so downstream exhaustive matches keep compiling as variants are added. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
This comment has been minimized.
This comment has been minimized.
The hand-written .NET SDK has no other fluent `With*` builders, so adding one here introduced a pattern that exists nowhere else in the surface. Java and Rust keep their fluent forms because those match long-standing convention in each of those SDKs. Callers now set the public `DecisionContext` property through an object initializer, which is what the class documentation already recommends for richer decisions. The wire format is unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
The Go, Node, and Python helpers were named `WithDecisionContext` and friends, a shape none of those SDKs use. In Go a `WithX` function conventionally builds a functional option rather than decorating a value, and there were no `With` functions in the package at all. Node and Python had no `with`-prefixed helper either. Each now follows the constructor naming its own SDK already uses: `NewAttributedPermissionResult` alongside `NewCanvasError`, `createAttributedPermissionResult` alongside `createCanvas`, and `create_attributed_permission_result` alongside `create_session_fs_adapter`. The Node wrapper also gains a `kind: "attributed"` discriminant so it is narrowed the same way as every other union in that SDK, instead of by testing for the presence of a property. Java and Rust keep their fluent methods, which match long-standing convention in each of those SDKs. Behavior and wire format are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
The pointer/value type switch was duplicated verbatim in NewAttributedPermissionResult and the session permission dispatch. Embedding an interface promotes its methods to the value type too, so both forms satisfy rpc.PermissionDecision and both must be unwrapped -- missing the value case is what produced the bug caught in review. Fold both copies into splitAttribution so that hazard is stated and handled in exactly one place. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
This comment has been minimized.
This comment has been minimized.
stephentoub
reviewed
Aug 8, 2026
| * if {@code context} is {@code null} | ||
| * @since 1.3.0 | ||
| */ | ||
| public PermissionRequestResult withContext(PermissionDecisionContext context) { |
Author
There was a problem hiding this comment.
Renamed to setDecisionContext to match the getter and the other setters here, and dropped the requireNonNull since null now means "no context" everywhere else.
The Java SDK uses setX for mutators (589 of them); withX appears twice and both return a copy rather than mutating in place. withContext was the odd one out on both counts, and did not match its own getter or the sibling setKind/setRules/setFeedback on this class. Also drop the requireNonNull. The other setters here do not null-check, and null now means "no context" in every other SDK, so throwing made Java the outlier rather than the consistent one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
This comment has been minimized.
This comment has been minimized.
Hand-written Rust here has 157 enum variants: 89 unit, 35 tuple with exactly one payload, and 33 struct-style. Every variant carrying two or more values uses the struct form, so a two-payload tuple was the only one of its kind. Name the payloads instead. Construction and both read sites now say which value they mean rather than relying on position. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
Contributor
✅ Cross-SDK Consistency ReviewThis PR adds Feature parity: All SDKs ✅
Key consistency properties verified ✅
Tests coverage ✅All SDKs include unit tests covering: context present, context absent (key set verification), re-application replacing not nesting, and no-result with context. Node and Rust additionally have E2E coverage verifying the outgoing wire shape. The changes are well-structured and maintain strong cross-language consistency.
|
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.
Why
The runtime records attribution for an auto-approval decision only when the client supplies an
explicit
decisionContextalongside its permission reply. The wire schema and the runtime sidealready support this, and the generated types in this repo already carry the field.
The SDKs' hand-written permission-reply paths never populated it, so every host that answers
permissions through an SDK produces decisions the runtime cannot attribute. There is currently
no way for a permission handler to supply the context at all.
What
Let a permission handler attach optional provenance, forwarded to the runtime as a sibling of
resultonsession.permissions.handlePendingPermissionRequest. Purely additive across allsix SDKs — existing handlers compile and behave unchanged:
createAttributedPermissionResult(result, ctx)copilot.create_attributed_permission_result(result, ctx)copilot.NewAttributedPermissionResult(result, ctx)new PermissionDecisionApproveOnce { DecisionContext = ctx }PermissionRequestResult.approveOnce().setDecisionContext(ctx)PermissionResult::approve_once().with_context(ctx)Each helper is named after the conventions already present in that SDK rather than a single
cross-language spelling. The decision types are generated in Node, Python, Go and Rust, so they
cannot carry a hand-written field and attribution is applied by wrapping; .NET's generated class
is
partialand Java's is hand-written, so both take a member directly.decisionContextdescribes how a decision was reached, not what it was, so it is never nestedinside
result. With no context supplied the emitted JSON is byte-identical to today's — threekeys, and no
decisionContextkey rather thandecisionContext: null.No schema change and no generated code touched: the gap was purely in the hand-written layer.
Testing
no-result decision, and re-applying context replacing rather than nesting
1.0.79-5);CI runs the repo-pinned
^1.0.79-6Python 11 tests; Go tests with vet/gofmt clean; .NET 198 unit tests with
dotnet formatcleanthrough method promotion and must be unwrapped like the pointer form
Note for reviewers: the CLI does not validate this field — a nested
decisionContext, or aninvalid
outcomeenum, are both accepted silently. E2E therefore cannot verify the wire shape.The Node E2E test asserts the outgoing RPC params directly and was mutation-tested (nesting the
key and dropping it each correctly fail); Rust shape coverage lives in the
permission_response_paramsunit test, which catches the nesting mutation.Release note
Backward-compatible and opt-in. Hosts that do not attach context are unaffected.