Skip to content

Commit bc0e50f

Browse files
krukowCopilot
andcommitted
fix: address Copilot Code Review feedback
- Fix docstring: :resolved-by-hook? → :resolved-by-hook (matches actual key) - Replace Thread/sleep with :on-event latch for deterministic negative assertion in resolvedByHook=true test - Assert .await return value on rpc-latch for fail-fast on timeout Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 5cc7e1d commit bc0e50f

2 files changed

Lines changed: 13 additions & 4 deletions

File tree

src/github/copilot_sdk/client.clj

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@
285285
session.permissions.handlePendingPermissionRequest RPC method.
286286
When the handler returns :no-result, the RPC call is skipped
287287
so the extension does not answer this permission request.
288-
When :resolved-by-hook? is true, the runtime already resolved
288+
When :resolved-by-hook is true, the runtime already resolved
289289
this permission via a permissionRequest hook — skip the handler
290290
entirely (the event is still published to subscribers)."
291291
[client session-id event]

test/github/copilot_sdk/integration_test.clj

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,14 +1036,20 @@
10361036
(testing "v3 permission.requested with resolvedByHook=true skips handler entirely"
10371037
(let [handler-called? (atom false)
10381038
requests (atom [])
1039+
;; Use a latch to wait for the event to be delivered to the session
1040+
event-latch (java.util.concurrent.CountDownLatch. 1)
10391041
_ (mock/set-request-hook! *mock-server*
10401042
(fn [method params]
10411043
(swap! requests conj {:method method :params params})))
10421044
session (sdk/create-session *test-client*
10431045
{:on-permission-request
10441046
(fn [_request _ctx]
10451047
(reset! handler-called? true)
1046-
{:kind :approved})})
1048+
{:kind :approved})
1049+
:on-event
1050+
(fn [event]
1051+
(when (= :copilot/permission.requested (:type event))
1052+
(.countDown event-latch)))})
10471053
session-id (sdk/session-id session)]
10481054
;; Force protocol v3
10491055
(swap! (:state *test-client*) assoc :negotiated-protocol-version 3)
@@ -1055,7 +1061,9 @@
10551061
:permissionRequest {:permissionKind "shell"
10561062
:fullCommandText "echo test"}
10571063
:resolvedByHook true})
1058-
(Thread/sleep 500)
1064+
;; Wait for the event to be delivered (proves routing completed)
1065+
(is (.await event-latch 5 java.util.concurrent.TimeUnit/SECONDS)
1066+
"timed out waiting for permission.requested event delivery")
10591067
;; Handler should NOT be called
10601068
(is (false? @handler-called?)
10611069
"permission handler should not be invoked when resolvedByHook is true")
@@ -1089,7 +1097,8 @@
10891097
:permissionRequest {:permissionKind "shell"
10901098
:fullCommandText "echo test"}
10911099
:resolvedByHook false})
1092-
(.await rpc-latch 5 java.util.concurrent.TimeUnit/SECONDS)
1100+
(is (.await rpc-latch 5 java.util.concurrent.TimeUnit/SECONDS)
1101+
"timed out waiting for handlePendingPermissionRequest RPC")
10931102
(is (true? @handler-called?)
10941103
"permission handler should be invoked when resolvedByHook is false")
10951104
(is (= 1 (count (filter #(= "session.permissions.handlePendingPermissionRequest"

0 commit comments

Comments
 (0)