Address PR review comments

- nodejs/dotnet/go: best-effort disconnect/destroy session when the
  post-create options.update RPC fails so empty mode never leaves a
  runtime session alive with permissive defaults. Logic lives inside
  updateSessionOptionsForMode so call sites stay one line.
- python: same cleanup already happens; harden it so a failing
  disconnect doesn't mask the original error.
- python _normalize_tool_filter: reject bare str so passing
  'builtin:bash' fails fast instead of silently splitting into chars.
- nodejs client.ts: fix doc comment claiming append-mode is rejected
  in empty mode (we promote it to customize).
- go toolset.go: drop misleading 'implicit conversion' wording.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: SteveSandersonMS

dotnet/src/Client.cs

@@ -677,15 +677,34 @@ private async Task UpdateSessionOptionsForModeAsync(CopilotSession session, Sess
 
         if (!hasAnyPatch) return;
 
+        try
+        {
 #pragma warning disable GHCP001
-        await session.Rpc.Options.UpdateAsync(
-            skipCustomInstructions: skipCustomInstructions,
-            customAgentsLocalOnly: customAgentsLocalOnly,
-            coauthorEnabled: coauthorEnabled,
-            manageScheduleEnabled: manageScheduleEnabled,
-            installedPlugins: installedPlugins,
-            cancellationToken: cancellationToken).ConfigureAwait(false);
+            await session.Rpc.Options.UpdateAsync(
+                skipCustomInstructions: skipCustomInstructions,
+                customAgentsLocalOnly: customAgentsLocalOnly,
+                coauthorEnabled: coauthorEnabled,
+                manageScheduleEnabled: manageScheduleEnabled,
+                installedPlugins: installedPlugins,
+                cancellationToken: cancellationToken).ConfigureAwait(false);
 #pragma warning restore GHCP001
+        }
+        catch
+        {
+            // The runtime session exists but the post-create options
+            // patch failed — best-effort destroy so we don't leak it
+            // (in empty mode it would otherwise stay alive with
+            // permissive defaults).
+            try
+            {
+                await session.DisposeAsync().ConfigureAwait(false);
+            }
+            catch
+            {
+                // Swallow: original error is what the caller needs.
+            }
+            throw;
+        }
     }
 
     /// <summary>

go/client.go

@@ -777,14 +777,7 @@ func (c *Client) CreateSession(ctx context.Context, config *SessionConfig) (*Ses
 		CoauthorEnabled:        config.CoauthorEnabled,
 		ManageScheduleEnabled:  config.ManageScheduleEnabled,
 	}); err != nil {
-		// In empty mode, refuse to expose a session whose safe-defaults
-		// patch was rejected: tear it down so callers never get a
-		// permissive session.
-		_ = session.Disconnect()
-		c.sessionsMux.Lock()
-		delete(c.sessions, sessionID)
-		c.sessionsMux.Unlock()
-		return nil, fmt.Errorf("failed to apply mode-specific session options: %w", err)
+		return nil, err
 	}
 
 	return session, nil
@@ -1002,11 +995,7 @@ func (c *Client) ResumeSessionWithOptions(ctx context.Context, sessionID string,
 		CoauthorEnabled:        config.CoauthorEnabled,
 		ManageScheduleEnabled:  config.ManageScheduleEnabled,
 	}); err != nil {
-		_ = session.Disconnect()
-		c.sessionsMux.Lock()
-		delete(c.sessions, sessionID)
-		c.sessionsMux.Unlock()
-		return nil, fmt.Errorf("failed to apply mode-specific session options: %w", err)
+		return nil, err
 	}
 
 	return session, nil

go/mode_empty.go

@@ -193,8 +193,17 @@ func (c *Client) updateSessionOptionsForMode(ctx context.Context, session *Sessi
 	if !hasAny {
 		return nil
 	}
-	_, err := session.RPC.Options.Update(ctx, patch)
-	return err
+	if _, err := session.RPC.Options.Update(ctx, patch); err != nil {
+		// The runtime session exists but the post-create options patch
+		// failed — best-effort disconnect so we don't leak it (in empty
+		// mode it would otherwise keep running with permissive defaults).
+		_ = session.Disconnect()
+		c.sessionsMux.Lock()
+		delete(c.sessions, session.SessionID)
+		c.sessionsMux.Unlock()
+		return fmt.Errorf("failed to apply mode-specific session options: %w", err)
+	}
+	return nil
 }
 
 // optBackInFields is the subset of SessionConfig / ResumeSessionConfig shared

go/toolset.go

@@ -39,9 +39,9 @@ const (
 // built-in, even if an MCP server or custom-agent extension happens to
 // register a tool with the same wire name.
 //
-// ToolSet's zero value is ready to use. ToolSet implements implicit conversion
-// to []string via [ToolSet.ToSlice], and the [SessionConfig] fields accept
-// []string directly; pass tools as `(&ToolSet{}).AddBuiltIn(...).ToSlice()`.
+// ToolSet's zero value is ready to use. Convert to []string via [ToolSet.ToSlice]
+// before passing to [SessionConfig] fields, e.g.
+// `(&ToolSet{}).AddBuiltIn(...).ToSlice()`.
 type ToolSet struct {
 	items []string
 }

nodejs/src/client.ts

@@ -899,8 +899,11 @@ export class CopilotClient {
     /**
      * Returns the systemMessage config to use, adjusted for the current mode.
      * In empty mode we ensure the environment_context section is removed
-     * unless the app has already taken control of it; append mode is rejected
-     * because it would leave environment info in the prompt.
+     * unless the app has already taken control of it. `append` (and
+     * unspecified) mode is promoted to `customize` so we can also strip
+     * environment_context; the caller's `content` is preserved verbatim
+     * because the runtime appends it as additional instructions in both
+     * customize and append modes.
      */
     private getSystemMessageConfigForMode(
         supplied: SystemMessageConfig | undefined
@@ -966,8 +969,22 @@ export class CopilotClient {
             if (config.manageScheduleEnabled !== undefined)
                 patch.manageScheduleEnabled = config.manageScheduleEnabled;
         }
-        if (Object.keys(patch).length > 0) {
+        if (Object.keys(patch).length === 0) {
+            return;
+        }
+        try {
             await session.rpc.options.update(patch);
+        } catch (e) {
+            // The runtime session exists but the post-create options
+            // patch failed — best-effort disconnect so we don't leak
+            // it (in empty mode it would otherwise keep running with
+            // permissive defaults).
+            try {
+                await session.disconnect();
+            } catch {
+                // Swallow: original error is the one the caller needs.
+            }
+            throw e;
         }
     }
 

python/copilot/_mode.py

@@ -105,11 +105,21 @@ def __len__(self) -> int:
 
 
 def _normalize_tool_filter(value: Any) -> list[str] | None:
-    """Accept ``ToolSet``, ``list[str]``, or ``None``; return a list or ``None``."""
+    """Accept ``ToolSet``, ``list[str]``, or ``None``; return a list or ``None``.
+
+    Reject plain ``str`` explicitly — ``list("foo")`` would silently shred it
+    into characters, sending an invalid tool filter list on the wire.
+    """
     if value is None:
         return None
     if isinstance(value, ToolSet):
         return value.to_list()
+    if isinstance(value, str):
+        raise TypeError(
+            "tool filter must be a ToolSet or list[str], not str. "
+            'Pass a single-element list (e.g. ["builtin:bash"]) or a '
+            "ToolSet (e.g. ToolSet().add_builtin('bash'))."
+        )
     return list(value)
 
 

python/copilot/client.py

@@ -3283,7 +3283,10 @@ async def _apply_post_create_options_patch(
         except BaseException:
             with self._sessions_lock:
                 self._sessions.pop(session.session_id, None)
-            await session.disconnect()
+            try:
+                await session.disconnect()
+            except BaseException:
+                pass
             raise
 
     async def _set_session_fs_provider(self) -> None: