sdk: Forward decisionContext on permission replies across languages - #2294
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.
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.
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
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.
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
This comment has been minimized.
This comment has been minimized.
Add a separate attributed permission handler path so clients can forward decision context without changing the existing PermissionResult enum or PermissionHandler contract. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
|
@stephentoub I moved this PR back to draft after finding that the current Rust API change breaks existing clients. Adding I tested a backwards-compatible alternative that keeps Existing clients continue to use: impl PermissionHandler for MyHandler {
async fn handle(...) -> PermissionResult {
PermissionResult::approve_once()
}
}
SessionConfig::default()
.with_permission_handler(Arc::new(MyHandler))Copilot App would use: impl AttributedPermissionHandler for CopilotAppPermissionHandler {
async fn handle(...) -> AttributedPermissionResult {
PermissionResult::approve_once().with_context(
PermissionDecisionContext {
outcome: PermissionDecisionOutcome::PromptedUser,
source: PermissionDecisionSource::HumanResponse,
surface: PermissionDecisionSurface::CopilotApp,
},
)
}
}
let client = Client::start(ClientOptions::default()).await?;
let session = client
.create_session(
SessionConfig::default().with_attributed_permission_handler(
Arc::new(CopilotAppPermissionHandler),
),
)
.await?;Existing clients compile without changes, and context-aware clients implement only one handler method. The cost is one additional handler trait and configuration method. |
In the past, I believe @tclem has suggested we shouldn't care about such breaking changes for rust consumers. Tim? |
|
@tclem What do you think? 👀 |
Keep PermissionHandler as the single dispatch API and carry decision context through PermissionResult. Replace the ineffective rejection E2E test with an exact fake-server wire assertion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
|
We decided to make the breaking change for Rust. |
Store optional decision context on the existing Decision variant so every decision follows one semantic path and downstream matches receive a compiler-guided migration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
Resolve the Java SDK module move by placing the decision-context test under java/sdk. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 79152db2-4cc7-4777-983a-655fae5b68c9
Cross-SDK Consistency Review ✅This PR adds API surface — all six SDKs covered
Behavior consistencyAll six implementations agree on:
Minor observation — Go "Experimental" labelThe Go SDK marks TestingAll six SDKs include unit tests covering: context forwarding, omission when absent, no-result suppression, and context replacement. Node adds an E2E test; Rust adds a fake-server wire-format test. Overall the PR maintains strong cross-SDK consistency. 👍
|
Why
The runtime emits
auto_approval_decisiontelemetry only when the client sends an explicitdecisionContextwith its permission reply. The wire schema, runtime, and generated SDK types already support the field.The hand-written permission reply paths in these SDKs did not forward it. Hosts that answered permission requests through an SDK therefore could not tell the runtime whether a decision came from a person, host policy, or an automated recommendation.
What changed
Permission handlers can now attach optional decision context. The SDK sends it as a top-level sibling of
resultinsession.permissions.handlePendingPermissionRequest:createAttributedPermissionResult(result, context)copilot.create_attributed_permission_result(result, context)copilot.NewAttributedPermissionResult(result, context)DecisionContexton the permission decisionPermissionRequestResult.approveOnce().setDecisionContext(context)PermissionResult::approve_once().with_context(context)Each SDK follows its existing language conventions. Applying context twice replaces the previous context instead of nesting it. A no-result response remains suppressed.
When a handler does not supply context, the SDK sends the same legacy JSON shape with only
sessionId,requestId, andresult. It does not senddecisionContext: null.No schema or generated code changed. This PR only fills the gap in the hand-written permission reply paths.
Rust compatibility
Rust keeps
PermissionHandleras the single handler API.PermissionResult::Decisionnow contains the decision and an optional context:This is an intentional Rust source break. Clients that construct or match
PermissionResult::Decisiondirectly must use the new struct variant. The compiler identifies these sites during migration, including wildcard matches that could otherwise mistake a contextual decision forNoResult. Existing handler implementations that use the result helpers and existing session registration remain unchanged. This avoids a parallel attributed handler trait and keeps all decisions in one semantic variant.The other five SDK changes are additive.
Testing
decisionContextis besideresult, not inside it.Release note
Permission handlers can attach
decisionContextso the runtime can attribute permission decisions. This is additive for Node, Python, Go, .NET, and Java. Rust clients that construct or matchPermissionResult::Decisiondirectly must migrate from the tuple variant to the new struct variant.