Fix permission handler kinds in SDK docs and samples (#1133)#1315
Fix permission handler kinds in SDK docs and samples (#1133)#1315stephentoub wants to merge 2 commits into
Conversation
The SDKs distinguish two permission types that look similar but mean different things: * PermissionDecision (present tense) is what an onPermissionRequest handler returns: approve-once, approve-for-session, approve-for-location, approve-permanently, reject, user-not-available, no-result. * PermissionResult (past tense) is what shows up in permission.completed session events: approved, denied-by-rules, denied-interactively-by-user, etc. The READMEs, shared docs, and several test scenario samples were documenting and returning past-tense PermissionResult strings from onPermissionRequest handlers. That is wrong: those handlers must return PermissionDecision. This change updates every handler-return reference to use valid PermissionDecision kinds, while leaving event-payload documentation (which is legitimately past-tense) untouched. Files touched include the Node, Python, Go, and .NET READMEs, the .NET PermissionRequestResult.Kind XML doc, nodejs/docs/examples.md, the shared docs/ markdown (hooks, skills, image-input, steering-and-queueing, custom-agents, getting-started, microsoft-agent-framework), and the Python + TypeScript sample programs under test/scenarios/. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates SDK documentation and sample programs to use current permission decision kinds for onPermissionRequest handlers, aligning examples with the JSON-RPC permission decision contract rather than permission completion event result strings.
Changes:
- Replaces past-tense permission handler kinds like
"approved"with decision kinds like"approve-once"/"reject". - Updates SDK README permission-kind tables for Node.js, Python, Go, and .NET.
- Adjusts scenario samples and shared docs snippets to reflect the valid permission decision values.
Show a summary per file
| File | Description |
|---|---|
test/scenarios/tools/virtual-filesystem/typescript/src/index.ts |
Updates TypeScript virtual filesystem permission approval kind. |
test/scenarios/tools/virtual-filesystem/python/main.py |
Updates Python virtual filesystem permission approval kind. |
test/scenarios/tools/skills/typescript/src/index.ts |
Updates TypeScript skills permission approval kind. |
test/scenarios/tools/skills/python/main.py |
Updates Python skills permission approval kind. |
test/scenarios/callbacks/user-input/typescript/src/index.ts |
Updates TypeScript user-input scenario permission kind. |
test/scenarios/callbacks/user-input/python/main.py |
Updates Python user-input scenario permission kind. |
test/scenarios/callbacks/permissions/typescript/src/index.ts |
Updates TypeScript permissions scenario approval kind. |
test/scenarios/callbacks/permissions/README.md |
Updates permissions scenario prose and examples. |
test/scenarios/callbacks/permissions/python/main.py |
Updates Python permissions scenario approval kind. |
test/scenarios/callbacks/hooks/typescript/src/index.ts |
Updates TypeScript hooks scenario permission kind. |
test/scenarios/callbacks/hooks/python/main.py |
Updates Python hooks scenario permission kind. |
python/README.md |
Corrects Python permission handler examples and kind table. |
nodejs/README.md |
Corrects Node.js permission handler examples and decision table. |
nodejs/docs/examples.md |
Updates Node.js custom permission logic example. |
go/README.md |
Uses canonical Go permission constants and documents aliases. |
dotnet/src/Types.cs |
Updates XML docs for .NET permission request result kinds. |
dotnet/README.md |
Uses canonical .NET permission constants and documents aliases. |
docs/integrations/microsoft-agent-framework.md |
Updates TypeScript integration permission snippets. |
docs/getting-started.md |
Updates Python getting-started permission snippet. |
docs/features/steering-and-queueing.md |
Updates steering/queueing permission snippets. |
docs/features/skills.md |
Updates skills permission snippets. |
docs/features/image-input.md |
Updates image-input permission snippets. |
docs/features/hooks.md |
Updates hooks permission snippets across SDK examples. |
docs/features/custom-agents.md |
Updates custom-agent permission snippets. |
Copilot's findings
Comments suppressed due to low confidence (4)
docs/features/hooks.md:276
- This Python snippet returns a raw dict from
on_permission_request, but the SDK expectsPermissionRequestResultand accessesresult.kind. Readers copying this sample will getuser-not-availablebehavior instead of approval.
on_permission_request=lambda req, inv: {"kind": "approve-once"},
docs/features/hooks.md:629
- This Python snippet returns a raw dict from
on_permission_request, but the SDK expectsPermissionRequestResultand accessesresult.kind. Readers copying this sample will getuser-not-availablebehavior instead of approval.
on_permission_request=lambda req, inv: {"kind": "approve-once"},
docs/features/hooks.md:726
- This Python snippet returns a raw dict from
on_permission_request, but the SDK expectsPermissionRequestResultand accessesresult.kind. Readers copying this sample will getuser-not-availablebehavior instead of approval.
on_permission_request=lambda req, inv: {"kind": "approve-once"},
docs/features/hooks.md:959
- This Python snippet returns a raw dict from
on_permission_request, but the SDK expectsPermissionRequestResultand accessesresult.kind. Readers copying this sample will getuser-not-availablebehavior instead of approval.
on_permission_request=lambda req, inv: {"kind": "approve-once"},
- Files reviewed: 24/24 changed files
- Comments generated: 7
This comment has been minimized.
This comment has been minimized.
Python permission request handlers return PermissionRequestResult objects, not raw dictionaries. Update the docs and scenario samples that were still using dict-shaped examples so readers and scenario runs get the intended approve-once behavior instead of falling back to user-not-available. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅This PR fixes a documentation bug where past-tense What was checked:
One noted difference that is by design: No cross-SDK consistency issues found in this PR.
|
|
@SteveSandersonMS / @patniko , the SDK docs/samples diverge here significantly from the current runtime behavior. Before merging this to match the SDK to the runtime, I want to double-check that's the right direction for the fix? |
Why
The SDK docs and several sample programs documented past-tense
PermissionResultstrings (such as{ kind: "approved" }and{ kind: "denied-interactively-by-user" }) as return values fromonPermissionRequesthandlers. Following those examples produces handlers that don't satisfy the SDK type contract: handlers must return aPermissionDecision(present tense), and past-tense kinds belong to thePermissionResultshape that the runtime emits inpermission.completedsession events.Closes #1133.
What changed
Every
onPermissionRequesthandler example in the repo now returns a validPermissionDecisionkind for its SDK. Thepermission.completedevent-payload documentation (which is legitimately past-tense) is intentionally untouched.SDK surfaces
nodejs/README.md,python/README.md,go/README.md,dotnet/README.md: handler example and "Permission Result Kinds" table now list only the kinds each SDK actually accepts. Go and .NET tables use the canonical non-deprecated constants and add a note pointing at the deprecated past-tense aliases.dotnet/src/Types.cs: XML doc onPermissionRequestResult.Kindrewritten to list the four valid kinds with<see cref>links.nodejs/docs/examples.md: "Custom permission logic" sample updated.Shared
docs/markdowndocs/getting-started.md,docs/integrations/microsoft-agent-framework.md, anddocs/features/{hooks,skills,image-input,steering-and-queueing,custom-agents}.md: TypeScript, Python, and Go handler snippets switched to present-tense kinds.Test scenarios
test/scenarios/callbacks/{permissions,hooks,user-input}/*andtest/scenarios/tools/{skills,virtual-filesystem}/*: Python and TypeScript sample programs updated. The permissions scenario README's prose, table, and "Key Insight" lines were updated to match.Per-SDK kind sets used
PermissionDecisionunion plusno-result(with the v1/v2 caveat noted in the table).PermissionRequestResultKindLiteral:approve-once,reject,user-not-available,no-result.PermissionRequestResultKindApproved/Rejected/UserNotAvailable/NoResult(canonical constants whose wire values areapprove-once/reject/ etc.).PermissionRequestResultKind.Approved/Rejected/UserNotAvailable/NoResult(same wire values).Notes for reviewers
PermissionRequestResultKind.Approvedmember name is kept as-is even though its wire value is"approve-once"; that's a deliberate .NET API design choice and not part of the bug.docs/features/streaming-events.mdand the e2e test assertions that checkevent.data.result.kindare intentionally left past-tense - they describePermissionResult, notPermissionDecision.*/generated/*andgo/rpc/,dotnet/src/Generated/is left alone; both types are already represented correctly there.Validation
just validate-docsextracted 164 TypeScript and 43 C# snippets fromdocs/; all pass. (Python and Go validators failed for unrelated environment reasons in this worktree, not because of these edits; the editedmain.pysamples all parse withast.parse.)dotnet build src/GitHub.Copilot.SDK.csproj: 0 warnings, 0 errors.nodejs/src/types.ts,nodejs/src/generated/rpc.ts,python/copilot/session.py,go/types.go, anddotnet/src/Types.csand found no remaining inconsistencies.