Skip to content

feat: add handling for additional directories in session permissions - #2235

Draft
DonJayamanne wants to merge 3 commits into
mainfrom
don/top-wallaby
Draft

feat: add handling for additional directories in session permissions#2235
DonJayamanne wants to merge 3 commits into
mainfrom
don/top-wallaby

Conversation

@DonJayamanne

Copy link
Copy Markdown
Contributor

Follow up PR for #2180 (comment)

Copilot AI review requested due to automatic review settings August 3, 2026 19:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Node.js workaround to restore additional-directory permissions after session resume.

Changes:

  • Re-adds configured paths after session.resume.
  • Extends unit coverage for the permission RPC.
Show a summary per file
File Description
nodejs/src/client.ts Adds resumed-session permission paths.
nodejs/test/client.test.ts Verifies permission-path RPC dispatch.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread nodejs/src/client.ts
Comment thread nodejs/test/client.test.ts
@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings August 3, 2026 20:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (2)

rust/src/session.rs:1288

  • This new resume-time RPC is not exercised by the Rust tests. The existing rust/src/types.rs coverage only checks that additionalDirectories is serialized into session.resume, so it would not catch this follow-up call being omitted or using the wrong session/path. Add a resume_session RPC-harness test that supplies additional directories and asserts the emitted session.permissions.paths.add requests.
        for path in additional_directories {
            self.call(
                rpc_methods::SESSION_PERMISSIONS_PATHS_ADD,
                Some(serde_json::json!({
                    "sessionId": session_id,
                    "path": path,
                })),
            )
            .await?;

java/src/main/java/com/github/copilot/CopilotClient.java:845

  • There is no Java test covering this post-resume chain; the current SessionRequestBuilderTest only verifies additionalDirectories in the resume payload. Add a recording-runtime test that resumes with one or more directories and asserts session.permissions.paths.add receives each path and the active session ID, including the re-keyed ID when the server returns a different one.
                        CompletableFuture<Void> additionalDirectories = CompletableFuture.completedFuture(null);
                        if (config.getAdditionalDirectories() != null) {
                            for (String path : config.getAdditionalDirectories()) {
                                additionalDirectories = additionalDirectories
                                        .thenCompose(v -> session.getRpc().permissions.paths
                                                .add(new SessionPermissionsPathsAddParams(session.getSessionId(), path))
                                                .thenApply(ignored -> null));
  • Files reviewed: 11/11 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@github-actions

This comment has been minimized.

Copilot AI review requested due to automatic review settings August 3, 2026 20:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (7)

java/src/main/java/com/github/copilot/CopilotClient.java:845

  • The add response has a nullable success field, but thenApply(ignored -> null) treats both false and missing success as successful. That can return a resumed session without the configured permission. Validate Boolean.TRUE.equals(result.success()) and fail the chain otherwise.
                                additionalDirectories = additionalDirectories
                                        .thenCompose(v -> session.getRpc().permissions.paths
                                                .add(new SessionPermissionsPathsAddParams(session.getSessionId(), path))
                                                .thenApply(ignored -> null));

nodejs/src/client.ts:1867

  • The add RPC reports logical failure via its success field, but the result is discarded here. A valid { success: false } response therefore lets resumeSession succeed even though the configured directory is still disallowed. Check the flag and reject the resume when it is false.
            for (const path of config.additionalDirectories ?? []) {
                await session.rpc.permissions.paths.add({ path });

python/copilot/client.py:3294

  • permissions.paths.add can complete normally with success == false, but this ignores that status and returns a resumed session without the requested permission. Treat a false result as a resume failure.
            for path in additional_directories or []:
                await session.rpc.permissions.paths.add(PermissionPathsAddParams(path))

dotnet/src/Client.cs:1441

  • The RPC's Success result is discarded. If the runtime rejects a path by returning Success = false, this method still returns successfully although AdditionalDirectories was not applied. Inspect the result and throw so the existing catch block removes the session.
            foreach (var path in config.AdditionalDirectories ?? [])
            {
                await session.Rpc.Permissions.Paths.AddAsync(path, cancellationToken).ConfigureAwait(false);

go/client.go:1343

  • This checks only transport/deserialization errors. The RPC result also has a Success field, so { "success": false } currently returns a session whose requested directory was not added. Fail and unregister the session on that result as well.
	for _, path := range config.AdditionalDirectories {
		if _, err := session.RPC.Permissions.Paths().Add(ctx, &rpc.PermissionPathsAddParams{Path: path}); err != nil {

rust/src/session.rs:1282

  • This raw call verifies only RPC transport success and never deserializes PermissionsPathsAddResult.success. A { "success": false } response therefore allows resume to complete while the additional directory remains disallowed. Deserialize the result and return an SDK error when the flag is false.
            self.call(
                rpc_methods::SESSION_PERMISSIONS_PATHS_ADD,

java/src/main/java/com/github/copilot/CopilotClient.java:841

  • Java currently tests only that additionalDirectories is serialized into the resume request; no test exercises this new post-resume RPC. Add a RecordingRuntime-style resume test that verifies each path is sent to session.permissions.paths.add and that a false/failed response makes the resume fail.

This issue also appears on line 842 of the same file.

                        CompletableFuture<Void> additionalDirectories = CompletableFuture.completedFuture(null);
                        if (config.getAdditionalDirectories() != null) {
                            for (String path : config.getAdditionalDirectories()) {
  • Files reviewed: 12/12 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR updates all six SDK implementations with the same feature — calling session.permissions.paths.add for each entry in additionalDirectories after a resumeSession call. The feature is consistently implemented across:

SDK File Notes
Node.js/TypeScript nodejs/src/client.ts ✅ Uses generated RPC wrapper that auto-injects sessionId
Python python/copilot/client.py ✅ Passes PermissionPathsAddParams(path)
Go go/client.go ✅ Cleans up session from map on error
.NET dotnet/src/Client.cs ✅ Propagates exceptions naturally
Java java/src/main/java/.../CopilotClient.java ✅ Chained CompletableFuture
Rust rust/src/session.rs ✅ Sequential with error propagation

Error handling is consistent: all SDKs propagate errors from paths.add — Go additionally removes the session from the active sessions map on failure, which is a Go-idiomatic cleanup step.

API structure is properly parallel across languages, respecting each language's naming conventions and async patterns.

No consistency gaps found. 🎉

Generated by SDK Consistency Review Agent for #2235 · sonnet46 32.7 AIC · ⌖ 8.22 AIC · ⊞ 6.6K ·

@DonJayamanne
DonJayamanne marked this pull request as ready for review August 3, 2026 21:20
@DonJayamanne
DonJayamanne requested a review from a team as a code owner August 3, 2026 21:20
@DonJayamanne
DonJayamanne marked this pull request as draft August 3, 2026 21:20
@DonJayamanne
DonJayamanne marked this pull request as ready for review August 3, 2026 21:59
@DonJayamanne
DonJayamanne enabled auto-merge August 3, 2026 21:59
@SteveSandersonMS
SteveSandersonMS marked this pull request as draft August 4, 2026 13:58
auto-merge was automatically disabled August 4, 2026 13:58

Pull request was converted to draft

@SteveSandersonMS

Copy link
Copy Markdown
Contributor

I thought this was a runtime bug, and if so, should be fixed in the underlying runtime. Or am I misunderstanding?

Setting this to draft to make sure we don't merge by mistake. If I've got this wrong, please clarify!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants