Skip to content

Commit 7a590cf

Browse files
Aymen FurterAymen Furter
authored andcommitted
Preserve Rust permission handler compatibility
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
1 parent 277bb6c commit 7a590cf

5 files changed

Lines changed: 206 additions & 64 deletions

File tree

rust/src/handler.rs

Lines changed: 84 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515
//! [`Tool::with_handler`](crate::types::Tool::with_handler) on entries passed to
1616
//! [`SessionConfig::with_tools`](crate::types::SessionConfig::with_tools).
1717
18+
use std::sync::Arc;
19+
1820
use async_trait::async_trait;
1921
use serde::{Deserialize, Serialize};
2022

@@ -38,28 +40,35 @@ use crate::types::{
3840
/// approve-for-session, approve-permanently, user-not-available, …) or
3941
/// [`PermissionResult::NoResult`], which tells the SDK to suppress its
4042
/// response so another connected client can answer instead.
41-
#[non_exhaustive]
43+
///
44+
/// ```
45+
/// use github_copilot_sdk::handler::PermissionResult;
46+
///
47+
/// fn is_decision(result: PermissionResult) -> bool {
48+
/// match result {
49+
/// PermissionResult::Decision(_) => true,
50+
/// PermissionResult::NoResult => false,
51+
/// }
52+
/// }
53+
/// ```
4254
#[derive(Debug, Clone)]
4355
pub enum PermissionResult {
4456
/// Send a permission decision on the wire.
4557
Decision(PermissionDecision),
46-
/// Send a permission decision annotated with the context describing how
47-
/// and where it was reached, so the runtime can attribute
48-
/// auto-approval telemetry to the responding surface.
49-
///
50-
/// The context is informational only — it never changes permission
51-
/// behavior.
52-
AttributedDecision {
53-
/// The decision to send on the wire.
54-
decision: PermissionDecision,
55-
/// Context describing how and where the decision was reached.
56-
context: PermissionDecisionContext,
57-
},
5858
/// Decline to respond to this request, allowing another connected
5959
/// client to answer instead. The SDK suppresses the response.
6060
NoResult,
6161
}
6262

63+
/// A permission result with optional context describing how it was reached.
64+
#[derive(Debug, Clone)]
65+
pub struct AttributedPermissionResult {
66+
/// The permission result.
67+
pub result: PermissionResult,
68+
/// Context describing how and where the decision was reached.
69+
pub context: Option<PermissionDecisionContext>,
70+
}
71+
6372
impl PermissionResult {
6473
/// Approve this single request.
6574
pub fn approve_once() -> Self {
@@ -92,8 +101,7 @@ impl PermissionResult {
92101
/// Attach provenance describing how and where this decision was made,
93102
/// so the runtime can attribute auto-approval telemetry.
94103
///
95-
/// Applying this to an already-attributed decision replaces the
96-
/// previous context. It is a no-op on [`PermissionResult::NoResult`].
104+
/// It is a no-op on [`PermissionResult::NoResult`].
97105
///
98106
/// ```rust,no_run
99107
/// # use github_copilot_sdk::handler::PermissionResult;
@@ -108,16 +116,71 @@ impl PermissionResult {
108116
/// surface: PermissionDecisionSurface::Sdk,
109117
/// });
110118
/// ```
111-
pub fn with_context(self, context: PermissionDecisionContext) -> Self {
112-
match self {
113-
Self::Decision(decision) | Self::AttributedDecision { decision, .. } => {
114-
Self::AttributedDecision { decision, context }
115-
}
116-
Self::NoResult => Self::NoResult,
119+
pub fn with_context(self, context: PermissionDecisionContext) -> AttributedPermissionResult {
120+
let context = match self {
121+
Self::Decision(_) => Some(context),
122+
Self::NoResult => None,
123+
};
124+
AttributedPermissionResult {
125+
result: self,
126+
context,
127+
}
128+
}
129+
}
130+
131+
impl AttributedPermissionResult {
132+
/// Replace the context describing how this decision was reached.
133+
pub fn with_context(mut self, context: PermissionDecisionContext) -> Self {
134+
if matches!(self.result, PermissionResult::Decision(_)) {
135+
self.context = Some(context);
136+
}
137+
self
138+
}
139+
}
140+
141+
impl From<PermissionResult> for AttributedPermissionResult {
142+
fn from(result: PermissionResult) -> Self {
143+
Self {
144+
result,
145+
context: None,
117146
}
118147
}
119148
}
120149

150+
/// Handler for permission requests that also reports how the decision was made.
151+
#[async_trait]
152+
pub trait AttributedPermissionHandler: Send + Sync + 'static {
153+
/// Resolve a permission request and report how it was decided.
154+
async fn handle(
155+
&self,
156+
session_id: SessionId,
157+
request_id: RequestId,
158+
data: PermissionRequestData,
159+
) -> AttributedPermissionResult;
160+
}
161+
162+
struct UnattributedHandler(Arc<dyn PermissionHandler>);
163+
164+
#[async_trait]
165+
impl AttributedPermissionHandler for UnattributedHandler {
166+
async fn handle(
167+
&self,
168+
session_id: SessionId,
169+
request_id: RequestId,
170+
data: PermissionRequestData,
171+
) -> AttributedPermissionResult {
172+
PermissionHandler::handle(&*self.0, session_id, request_id, data)
173+
.await
174+
.into()
175+
}
176+
}
177+
178+
pub(crate) fn attributed(
179+
handler: Arc<dyn PermissionHandler>,
180+
) -> Arc<dyn AttributedPermissionHandler> {
181+
Arc::new(UnattributedHandler(handler))
182+
}
183+
121184
impl From<PermissionDecision> for PermissionResult {
122185
fn from(value: PermissionDecision) -> Self {
123186
Self::Decision(value)

rust/src/permission.rs

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ use std::sync::Arc;
1616

1717
use async_trait::async_trait;
1818

19-
use crate::handler::{PermissionHandler, PermissionResult, permission_handler_failure};
19+
use crate::handler::{
20+
AttributedPermissionHandler, PermissionHandler, PermissionResult, permission_handler_failure,
21+
};
2022
use crate::types::{PermissionRequestData, RequestId, SessionId};
2123

2224
/// Return a [`PermissionHandler`] that approves requests when managed settings
@@ -93,12 +95,16 @@ impl std::fmt::Debug for Policy {
9395
/// `requestPermission: false`).
9496
pub(crate) fn resolve_handler(
9597
handler: Option<Arc<dyn PermissionHandler>>,
98+
attributed_handler: Option<Arc<dyn AttributedPermissionHandler>>,
9699
policy: Option<Policy>,
97-
) -> Option<Arc<dyn PermissionHandler>> {
98-
match (handler, policy) {
99-
(_, Some(policy)) => Some(Arc::new(PolicyHandler { policy })),
100-
(Some(h), None) => Some(h),
101-
(None, None) => None,
100+
) -> Option<Arc<dyn AttributedPermissionHandler>> {
101+
match (handler, attributed_handler, policy) {
102+
(_, _, Some(policy)) => Some(crate::handler::attributed(Arc::new(PolicyHandler {
103+
policy,
104+
}))),
105+
(_, Some(h), None) => Some(h),
106+
(Some(h), None, None) => Some(crate::handler::attributed(h)),
107+
(None, None, None) => None,
102108
}
103109
}
104110

@@ -227,12 +233,13 @@ mod tests {
227233
}
228234
}
229235
let resolved =
230-
resolve_handler(Some(Arc::new(AlwaysApprove)), Some(Policy::DenyAll)).unwrap();
236+
resolve_handler(Some(Arc::new(AlwaysApprove)), None, Some(Policy::DenyAll)).unwrap();
231237
// Policy wins -- the AlwaysApprove handler is discarded.
232238
assert!(matches!(
233239
resolved
234240
.handle(SessionId::from("s"), RequestId::new("1"), data())
235-
.await,
241+
.await
242+
.result,
236243
PermissionResult::Decision(crate::types::PermissionDecision::Reject(_))
237244
));
238245
}
@@ -251,17 +258,18 @@ mod tests {
251258
PermissionResult::approve_once()
252259
}
253260
}
254-
let resolved = resolve_handler(Some(Arc::new(H)), None).unwrap();
261+
let resolved = resolve_handler(Some(Arc::new(H)), None, None).unwrap();
255262
assert!(matches!(
256263
resolved
257264
.handle(SessionId::from("s"), RequestId::new("1"), data())
258-
.await,
265+
.await
266+
.result,
259267
PermissionResult::Decision(crate::types::PermissionDecision::ApproveOnce(_))
260268
));
261269
}
262270

263271
#[test]
264272
fn resolve_handler_with_neither_returns_none() {
265-
assert!(resolve_handler(None, None).is_none());
273+
assert!(resolve_handler(None, None, None).is_none());
266274
}
267275
}

rust/src/session.rs

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ use crate::generated::session_events::{
2020
SessionCanvasClosedData, SessionErrorData, SessionEventType,
2121
};
2222
use crate::handler::{
23-
AutoModeSwitchHandler, AutoModeSwitchResponse, ElicitationHandler, ExitPlanModeHandler,
24-
McpAuthHandler, McpAuthRequest, McpAuthResult, PermissionHandler, PermissionResult,
23+
AttributedPermissionHandler, AutoModeSwitchHandler, AutoModeSwitchResponse, ElicitationHandler,
24+
ExitPlanModeHandler, McpAuthHandler, McpAuthRequest, McpAuthResult, PermissionResult,
2525
UserInputHandler, UserInputResponse,
2626
};
2727
use crate::hooks::SessionHooks;
@@ -56,7 +56,7 @@ const TOOL_SEARCH_TOOL_NAME: &str = "tool_search_tool";
5656
/// are derived from these fields.
5757
#[derive(Clone)]
5858
pub(crate) struct SessionHandlers {
59-
pub permission: Option<Arc<dyn PermissionHandler>>,
59+
pub permission: Option<Arc<dyn AttributedPermissionHandler>>,
6060
pub managed_settings_enabled: bool,
6161
pub elicitation: Option<Arc<dyn ElicitationHandler>>,
6262
pub mcp_auth: Option<Arc<dyn McpAuthHandler>>,
@@ -902,6 +902,7 @@ impl Client {
902902

903903
let permission_handler = crate::permission::resolve_handler(
904904
runtime.permission_handler.take(),
905+
runtime.attributed_permission_handler.take(),
905906
runtime.permission_policy.take(),
906907
);
907908
let handlers = SessionHandlers {
@@ -1175,6 +1176,7 @@ impl Client {
11751176

11761177
let permission_handler = crate::permission::resolve_handler(
11771178
runtime.permission_handler.take(),
1179+
runtime.attributed_permission_handler.take(),
11781180
runtime.permission_policy.take(),
11791181
);
11801182
let handlers = SessionHandlers {
@@ -1580,15 +1582,14 @@ fn permission_request_data(
15801582
fn notification_permission_payload(result: &PermissionResult) -> Option<Value> {
15811583
match result {
15821584
PermissionResult::NoResult => None,
1583-
PermissionResult::Decision(decision)
1584-
| PermissionResult::AttributedDecision { decision, .. } => Some(
1585+
PermissionResult::Decision(decision) => Some(
15851586
serde_json::to_value(decision).expect("serializing permission decision should succeed"),
15861587
),
15871588
}
15881589
}
15891590

15901591
/// Build the full `session.permissions.handlePendingPermissionRequest`
1591-
/// params for a [`PermissionResult`].
1592+
/// params for an attributed permission result.
15921593
///
15931594
/// `decisionContext` is a sibling of `result` and is only present when the
15941595
/// handler attributed the decision — omitting it preserves legacy behavior.
@@ -1597,15 +1598,15 @@ fn notification_permission_payload(result: &PermissionResult) -> Option<Value> {
15971598
fn permission_response_params(
15981599
session_id: &SessionId,
15991600
request_id: &RequestId,
1600-
result: &PermissionResult,
1601+
result: &crate::handler::AttributedPermissionResult,
16011602
) -> Option<Value> {
1602-
let result_value = notification_permission_payload(result)?;
1603+
let result_value = notification_permission_payload(&result.result)?;
16031604
let mut params = serde_json::json!({
16041605
"sessionId": session_id,
16051606
"requestId": request_id,
16061607
"result": result_value,
16071608
});
1608-
if let PermissionResult::AttributedDecision { context, .. } = result {
1609+
if let Some(context) = &result.context {
16091610
params["decisionContext"] =
16101611
serde_json::to_value(context).expect("serializing decision context should succeed");
16111612
}
@@ -2641,7 +2642,7 @@ mod tests {
26412642
let params = permission_response_params(
26422643
&SessionId::from("session-1"),
26432644
&RequestId::from("permission-1"),
2644-
&PermissionResult::approve_once(),
2645+
&PermissionResult::approve_once().into(),
26452646
)
26462647
.unwrap();
26472648
assert_eq!(
@@ -2686,18 +2687,17 @@ mod tests {
26862687
permission_response_params(
26872688
&SessionId::from("session-1"),
26882689
&RequestId::from("permission-1"),
2689-
&PermissionResult::NoResult,
2690+
&PermissionResult::NoResult.into(),
26902691
)
26912692
.is_none()
26922693
);
26932694
}
26942695

26952696
#[test]
26962697
fn with_context_is_a_no_op_on_no_result() {
2697-
assert!(matches!(
2698-
PermissionResult::no_result().with_context(attribution_context()),
2699-
PermissionResult::NoResult
2700-
));
2698+
let result = PermissionResult::no_result().with_context(attribution_context());
2699+
assert!(matches!(result.result, PermissionResult::NoResult));
2700+
assert!(result.context.is_none());
27012701
}
27022702

27032703
#[test]

0 commit comments

Comments
 (0)