Fix permission denial keyword types and pin examples to gpt-5.2#18
Fix permission denial keyword types and pin examples to gpt-5.2#18
Conversation
Default permission-denial responses in session.clj and client.clj
returned :kind as a string ("denied-no-approval-rule-...") while
the spec (::permission-result-kind) and approve-all both use keywords.
This inconsistency meant users inspecting responses in Clojure saw
mixed types depending on whether a handler was configured.
Changes:
- session.clj: 3 denial paths now return keyword :kind
- client.clj: unknown-session denial now returns keyword :kind
- integration_test.clj: updated assertion to expect keyword
Also includes upstream PR #509 sync (deny-by-default permissions):
- requestPermission always true on wire
- approve-all convenience handler
- Integration tests for permission model
- Documentation and changelog updates
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Examples that relied on the CLI default model or used gpt-4.1/gpt-4o now explicitly specify gpt-5.2 for consistent behavior. - helpers_query.clj: add session-config with model, pass to all - metadata_api.clj: with-session and switch-model! now use gpt-5.2 - multi_agent.clj: researcher sessions changed from gpt-4.1 to gpt-5.2 BYOK provider examples (claude-sonnet-4, llama3) are intentionally unchanged as they demonstrate provider-specific models. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR syncs upstream permission-handling behavior (deny-by-default) while fixing internal :kind type consistency for permission-denial results, and pins example code to the gpt-5.2 model for determinism.
Changes:
- Make default permission-denial
:kindvalues keywords (matching specs andapprove-all) and addapprove-allhelper + docs/changelog for deny-by-default permissions. - Force
requestPermissionto betrueon create/resume wire params and expand integration tests around the new permission model. - Update examples/docs to explicitly use
gpt-5.2and document deny-by-default permissions +approve-all.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/github/copilot_sdk/session.clj |
Uses keyword :kind in default permission-denial paths. |
src/github/copilot_sdk/client.clj |
Uses keyword :kind for unknown-session denial; always sends :request-permission true; adds approve-all. |
src/github/copilot_sdk/instrument.clj |
Adds spec/instrumentation for client/approve-all. |
src/github/copilot_sdk.clj |
Re-exports approve-all from client. |
test/github/copilot_sdk/integration_test.clj |
Adds/updates tests for deny-by-default permission model and keyword :kind. |
test/github/copilot_sdk/mock_server.clj |
Adds a helper intended to inject permission requests in tests. |
examples/helpers_query.clj |
Pins helper-based queries to gpt-5.2. |
examples/metadata_api.clj |
Pins session model to gpt-5.2 in the metadata demo (including model switching section). |
examples/multi_agent.clj |
Pins multi-agent example model to gpt-5.2. |
examples/mcp_local_server.clj |
Adds :on-permission-request copilot/approve-all to satisfy deny-by-default for MCP tools. |
examples/README.md |
Updates snippets to use gpt-5.2 and documents deny-by-default permissions + approve-all. |
doc/reference/API.md |
Documents deny-by-default permissions and adds approve-all reference section. |
README.md |
Notes deny-by-default permissions in the “Advanced Usage” bullet. |
CHANGELOG.md |
Adds breaking-change note for deny-by-default permissions and documents approve-all + keyword :kind fix. |
Comments suppressed due to low confidence (1)
src/github/copilot_sdk/session.clj:172
handle-permission-request!passes the incomingrequestmap through to the user handler unchanged. In this PR's integration tests,:permission-kindis a string ("shell"), while::specs/permission-requestrequires keyword kinds. Consider normalizingrequestbefore invoking the handler (e.g., convert string kinds to keywords) so handler inputs match the documented/spec’d shape andapprove-allcan be safely instrumented.
{:result {:kind :denied-no-approval-rule-and-could-not-request-from-user}}
(try
(let [result (handler request {:session-id session-id})
;; If handler returns a channel, await it
result (if (channel? result)
(<!! result)
result)]
| (defn inject-permission-request! | ||
| "Inject a permission request from the mock server. | ||
| Simulates the CLI asking for permission to execute a tool. | ||
| Returns a channel that delivers the permission response." | ||
| [server session-id permission-request] | ||
| (let [request-id (generate-id (:message-id server)) | ||
| response-ch (chan 1)] | ||
| ;; We need to read the response after sending the request. | ||
| ;; Use the on-request hook to capture the response. | ||
| (write-message (:writer server) | ||
| {:jsonrpc "2.0" | ||
| :id request-id | ||
| :method "permission.request" | ||
| :params {:sessionId session-id | ||
| :permissionRequest permission-request}}) | ||
| ;; Return request-id so caller can match the response | ||
| request-id)) |
There was a problem hiding this comment.
inject-permission-request!'s docstring says it returns a channel delivering the permission response, but the function currently returns only request-id and never uses response-ch (also no mechanism to capture the client's JSON-RPC response). Either implement response capture (e.g., track pending request IDs and deliver responses on a channel) or update the docstring/signature and remove the unused local.
| :ret nil?) | ||
|
|
||
| (s/fdef github.copilot-sdk.client/approve-all | ||
| :args (s/cat :request ::specs/permission-request |
There was a problem hiding this comment.
The new approve-all fdef requires :request to conform to ::specs/permission-request, which currently requires keyword :permission-kind. However, wire-level permission requests may carry string kinds (and tests in this PR construct them as strings). Either relax the fdef to accept the wire shape as well, or ensure permission requests are normalized to keyword kinds before calling the handler; otherwise instrumentation can fail when users pass github.copilot-sdk.client/approve-all as :on-permission-request.
| :args (s/cat :request ::specs/permission-request | |
| :args (s/cat :request (s/or :normalized ::specs/permission-request | |
| :wire map?) |
| (copilot/with-session [session client {:model "gpt-5.2"}] | ||
| ;; Query with gpt-5.2 | ||
| (println " Query: 'What is 2+2? Answer briefly.'") | ||
| (println (str " Response: " (h/query "What is 2+2? Answer briefly." :session session))) | ||
|
|
||
| ;; Try model introspection (requires CLI support) | ||
| (try | ||
| (let [current (copilot/get-current-model session)] | ||
| (println (str "\n Current model: " current)) | ||
| (copilot/switch-model! session "gpt-4o") | ||
| (copilot/switch-model! session "gpt-5.2") | ||
| (println (str " Switched to: " (copilot/get-current-model session))) |
There was a problem hiding this comment.
This section is labeled “Dynamic Model Switching”, but the session is created with :model "gpt-5.2" and then switch-model! is called with the same model, so it doesn't actually demonstrate switching. Either start the session without an explicit model and switch to gpt-5.2, or switch to a different known-good model while keeping the initial model pinned for the earlier queries.
- Add prominent warning at top of MCP overview about deny-by-default permissions - Add new 'Permission Handling' section with approve-all and custom handler examples - Update all MCP code examples to include :on-permission-request copilot/approve-all - Updated both doc/mcp/overview.md and doc/mcp/debugging.md - Updated CHANGELOG.md with documentation improvements This addresses documentation gap where MCP examples would fail due to permission denials without guidance on the deny-by-default model introduced in PR #18. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add Step 5 covering deny-by-default permission model - Show approve-all usage for development/trusted environments - Demonstrate custom permission handler implementation - Link to permission_bash.clj example and API reference - Renumber subsequent steps (Step 5->6, Step 6->7) Updates CHANGELOG.md with documentation changes. Addresses documentation gap identified in PR #18 review - the getting-started guide now properly introduces users to permission handling, which is critical for sessions using bash, file operations, or other restricted tools. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add two missing topics to doc/getting-started.md: - Permission model: explain deny-by-default, show approve-all usage, link to Permission Handling section in API Reference - :client-name option: show how to identify an application in session config Both features were added in merged PRs (#18 and #21) but were not reflected in the getting-started tutorial. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Two changes bundled together from code review findings:
1. Fix permission denial
:kindtype consistencyDefault permission-denial responses in
session.cljandclient.cljreturned:kindas a string ("denied-no-approval-rule-and-could-not-request-from-user"), while the spec (::permission-result-kind) andapprove-allboth use keywords. This created an internal type mismatch that could confuse users inspecting responses.Changes:
session.clj: 3 denial paths now return keyword:kindclient.clj: unknown-session denial now returns keyword:kindintegration_test.clj: updated assertion to expect keywordAlso includes upstream PR #509 sync (deny-by-default permissions):
requestPermissionalwaystrueon wireapprove-allconvenience handler2. Pin all examples to gpt-5.2 model
Examples that relied on the CLI default model or used
gpt-4.1/gpt-4onow explicitly specifygpt-5.2for consistent behavior across environments.Changed:
helpers_query.clj,metadata_api.clj,multi_agent.cljUnchanged: BYOK provider examples (
claude-sonnet-4,llama3) — intentionally provider-specific.Validation
bb test: 83 tests, 246 assertions, 0 failures ✅helpers-query/run,metadata-api/run,multi-agent/run: all pass ✅