feat: add handling for additional directories in session permissions - #2235
feat: add handling for additional directories in session permissions#2235DonJayamanne wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
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
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
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.rscoverage only checks thatadditionalDirectoriesis serialized intosession.resume, so it would not catch this follow-up call being omitted or using the wrong session/path. Add aresume_sessionRPC-harness test that supplies additional directories and asserts the emittedsession.permissions.paths.addrequests.
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
SessionRequestBuilderTestonly verifiesadditionalDirectoriesin the resume payload. Add a recording-runtime test that resumes with one or more directories and assertssession.permissions.paths.addreceives 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
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review details
Suppressed comments (7)
java/src/main/java/com/github/copilot/CopilotClient.java:845
- The add response has a nullable
successfield, butthenApply(ignored -> null)treats bothfalseand missing success as successful. That can return a resumed session without the configured permission. ValidateBoolean.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
successfield, but the result is discarded here. A valid{ success: false }response therefore letsresumeSessionsucceed 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.addcan complete normally withsuccess == 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
Successresult is discarded. If the runtime rejects a path by returningSuccess = false, this method still returns successfully althoughAdditionalDirectorieswas 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
Successfield, 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
additionalDirectoriesis serialized into the resume request; no test exercises this new post-resume RPC. Add aRecordingRuntime-style resume test that verifies each path is sent tosession.permissions.paths.addand 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
Cross-SDK Consistency Review ✅This PR updates all six SDK implementations with the same feature — calling
Error handling is consistent: all SDKs propagate errors from API structure is properly parallel across languages, respecting each language's naming conventions and async patterns. No consistency gaps found. 🎉
|
Follow up PR for #2180 (comment)