Port upstream PR #1428: Client Mode (Empty) — multitenancy hardening#114
Merged
Conversation
…preset Source-qualified tool filter constructors (builtin/mcp/custom + builtins vector form) plus isolated-builtins / isolated for parity with upstream BuiltInTools.Isolated. Bare "*" is rejected at construction time. Adds fdef specs in instrument.clj for every public fn so integration tests with instrumentation enabled catch contract violations. Mirrors upstream nodejs/src/toolSet.ts from PR github/copilot-sdk#1428 github/copilot-sdk#1428 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add :mode #{:copilot-cli :empty} client option (default :copilot-cli)
mirroring upstream PR github/copilot-sdk#1428.
In :empty mode the SDK:
- Requires at least one tenant-scoped storage root at construction time
(:copilot-home, :session-fs, :cli-url, or :is-child-process?) so the
spawned CLI never falls back to the user's home directory.
- Forces COPILOT_DISABLE_KEYTAR=1 on the spawned CLI via the
cli-env-overrides :overrides slot so the caller cannot accidentally
re-enable the host keychain.
- Requires every create-session / resume-session call (sync + async) to
supply :available-tools; an empty vector is legitimate, the key just
has to be present so silently-empty filters cannot happen.
- Spreads 9 safe defaults UNDER caller session config (caller always
wins): :enable-session-telemetry? false,
:mcp-oauth-token-storage :in-memory, :skip-embedding-retrieval true,
:embedding-cache-storage :in-memory,
:enable-on-demand-instruction-discovery false,
:enable-file-hooks false, :enable-host-git-operations false,
:enable-session-store false, :enable-skills false.
- Normalizes :system-message so environment_context is stripped unless
the app has taken control of it (mirrors upstream
getSystemMessageConfigForMode): no system-message emits
{:mode customize :sections {:environment_context {:action remove}}};
:append is promoted to :customize preserving content; :customize
without an env-context override gets one added; :replace passes
through unchanged. :copilot-cli mode keeps legacy behavior.
- After session.create / session.resume succeeds, issues a follow-up
session.options.update RPC carrying four overridable feature flags
(:skip-custom-instructions true, :custom-agents-local-only true,
:coauthor-enabled false, :manage-schedule-enabled false) plus
:installed-plugins []. In :copilot-cli mode only flags the caller
explicitly set are forwarded; an empty patch skips the RPC entirely.
On failure the SDK disconnects and removes the half-configured
session before rethrowing. Wired into create-session,
resume-session, <create-session, <resume-session.
Both modes always emit :tool-filter-precedence "excluded" on
session.create / session.resume so the ordering between
:available-tools and :excluded-tools is deterministic regardless of
CLI version, and reject bare "*" in :available-tools / :excluded-tools
at the SDK boundary (matches upstream resolveToolFilterOptions).
Adds 23 new integration tests covering validation, env-var overrides,
wire payload mode-defaults, system-message normalization, and the
session.options.update RPC (including async path + cleanup-on-failure).
339 tests / 1578 assertions / 0 failures.
Upstream: github/copilot-sdk#1428
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- doc/reference/API.md: add :mode client option to constructor table,
add 4 new session-config flags (skip-custom-instructions,
custom-agents-local-only, coauthor-enabled, manage-schedule-enabled)
to the session-config table, and add two new sections under Advanced
Usage:
- 'Client Mode (Empty)' — runnable example, mode semantics, the 9
config defaults, system-message normalization, options.update
flags, and the always-emit tool-filter-precedence guarantee.
- 'Tool Sets' — github.copilot-sdk.tool-set API surface (builtin /
mcp / custom / builtins / isolated) with the bare-* rejection
rationale.
- CHANGELOG.md: new [Unreleased] section for upstream PR #1428,
remove the matching 'deferred from round 6' bullet (no longer
deferred — ported here).
- examples/empty_mode.clj: BYOK-based runnable example showing temp
copilot-home + in-memory session-fs + tool-set/isolated. Excluded
from run-all-examples.sh because empty mode disables the local
keychain and the example requires OPENAI_API_KEY or
ANTHROPIC_API_KEY.
- examples/README.md: example 20 entry, prerequisites note.
- doc/index.md: bump example count to 20.
Upstream: github/copilot-sdk#1428
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Ports upstream PR #1428 to add a hardened multitenancy posture via :mode :empty, plus tool-filter constructors and the post-create session.options.update orchestration, while preserving default :copilot-cli behavior.
Changes:
- Add
:mode #{:copilot-cli :empty}client option, with:emptyenforcing tenant-scoped storage, keychain disablement, safe session defaults, system-message normalization, and required:available-tools. - Add
session.options.updatefollow-up RPC (sync + async) to apply mode-derived option flags (and forcedinstalledPlugins []in:empty), including cleanup-on-failure. - Introduce
github.copilot-sdk.tool-sethelpers/presets, plus comprehensive unit/integration tests and docs/changelog updates.
Show a summary per file
| File | Description |
|---|---|
src/github/copilot_sdk/client.clj |
Implements :mode :empty behavior, tool-filter validation, system-message normalization, always-emitted toolFilterPrecedence, and session.options.update orchestration. |
src/github/copilot_sdk/specs.clj |
Adds ::client-mode and session-config keys for the new session.options.update flags. |
src/github/copilot_sdk/process.clj |
Forces COPILOT_DISABLE_KEYTAR=1 in spawned env for :mode :empty. |
src/github/copilot_sdk/tool_set.clj |
Adds source-qualified tool filter constructors and isolated preset (parity with upstream). |
src/github/copilot_sdk/instrument.clj |
Registers fdefs for the new tool-set public functions and ensures namespace loading. |
test/github/copilot_sdk/integration_test.clj |
Adds constructor/validation, wire-shape, system-message normalization, and options.update (sync/async/failure) tests. |
test/github/copilot_sdk/tool_set_test.clj |
Unit tests for tool-set validation and presets. |
test/github/copilot_sdk/process_test.clj |
Tests env override behavior for COPILOT_DISABLE_KEYTAR under :mode :empty. |
test/github/copilot_sdk/mock_server.clj |
Adds session.options.update mock response. |
doc/reference/API.md |
Documents client mode :empty, tool sets, and new session-config option keys forwarded via session.options.update. |
doc/index.md |
Updates example count reference. |
examples/empty_mode.clj |
Adds an example demonstrating :mode :empty with BYOK + isolated tools. |
examples/README.md |
Documents the new example and updates run-all-examples exclusions. |
script/validate_docs.clj |
Includes github.copilot-sdk.tool-set in doc validation load list. |
CHANGELOG.md |
Adds an Unreleased section entry summarizing the new mode, tool-set namespace, and options-update behavior. |
Copilot's findings
- Files reviewed: 15/15 changed files
- Comments generated: 2
The async <apply-session-options-update! ran cleanup-failed-options-update! directly inside its go block on RPC failure. That cleanup calls session/disconnect!, which uses blocking proto/send-request! (5s timeout) and other side effects. Blocking work inside a go block can starve the core.async dispatch threadpool and stall unrelated async flows. Offload the cleanup to async/thread and <! it from the go block before delivering the Throwable, matching the existing convention used in session.clj for blocking user-handler work. Also adds: - A regression test (test-empty-mode-options-update-async-failure-cleans-up-session) asserting the async path yields a Throwable and removes the half-configured session from the registry after an options.update failure. - A regression test (test-empty-mode-system-message-customize-no-sections-key) covering :customize without a :sections key — locks in the existing (contains? nil ...) → false → add :sections semantics so the behavior cannot regress. Addresses Copilot Code Review feedback on PR #114. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ports upstream github/copilot-sdk PR #1428 — "Multitenancy hardening: Client Mode (Empty)" — that was deferred from round 6 (copilot-community-sdk/copilot-sdk-clojure#113). This is the dedicated follow-up sync round for that one PR.
Adds the
:mode #{:copilot-cli :empty}client option (default:copilot-cli— historical behavior preserved).:emptymode is the hardened multitenancy posture for SaaS hosts that run sessions on behalf of multiple users and must isolate sessions from local machine state.This is a non-breaking addition.
:copilot-climode is the default, and the only behavioral change for that mode is that:tool-filter-precedence "excluded"is now always emitted onsession.create/session.resumeso the ordering between:available-toolsand:excluded-toolsis deterministic across CLI versions. Bare"*"in tool filters is rejected at the SDK boundary in both modes (mirrors upstreamresolveToolFilterOptions).Session plan:
~/.copilot/session-state/b794e17b-2c5a-42ed-90fe-63b823312966/plan.md(local; key decisions captured in this PR description and the per-step checkpoints).What
:emptymode does:copilot-home,:session-fs,:cli-url, or:is-child-process?so the CLI never falls back to the user's home directoryCOPILOT_DISABLE_KEYTAR=1on the spawned CLI (via thecli-env-overrides :overridesslot — caller cannot override)create-session/resume-session(sync + async) must supply:available-tools([]is legitimate — the key just has to be present so silently-empty filters cannot happen):enable-session-telemetry? false,:mcp-oauth-token-storage :in-memory,:skip-embedding-retrieval true,:embedding-cache-storage :in-memory,:enable-on-demand-instruction-discovery false,:enable-file-hooks false,:enable-host-git-operations false,:enable-session-store false,:enable-skills falseenvironment_contextunless the app has taken control of it: no:system-message→{:mode customize :sections {:environment_context {:action remove}}};:appendis promoted to:customizepreserving content + env-context removed;:customizewithout env-context override gets one added;:customizewith explicit env-context override is left untouched;:replacepasses through unchangedsession.create/session.resumesucceeds, issues a follow-upsession.options.updatewith four overridable feature flags (:skip-custom-instructions true,:custom-agents-local-only true,:coauthor-enabled false,:manage-schedule-enabled false) plus:installed-plugins []. On RPC failure the SDK disconnects + removes the half-configured session before rethrowingIn
:copilot-climode thesession.options.updateRPC is only sent when the caller explicitly set one of the four new flags; if the resulting patch is empty the RPC is skipped entirely.New public API surface
:mode #{:copilot-cli :empty}(default:copilot-cli).session.options.update, NOTsession.create)::skip-custom-instructions(boolean):custom-agents-local-only(boolean):coauthor-enabled(boolean):manage-schedule-enabled(boolean)github.copilot-sdk.tool-set: source-qualified tool filter constructors (builtin/mcp/custom/builtins) plus theisolated-builtins/isolatedpreset (parity with upstreamBuiltInTools.Isolated). Bare"*"rejected at construction time.See
doc/reference/API.md→ Client Mode (Empty) and Tool Sets for full reference docs.Design notes
Why one mode flag and not nine
Upstream PR #1428 introduces a single
modeenum so multi-tenant hosts can opt into the entire hardened posture (env-var override, config defaults, system-message normalization, options.update flags, validation) with one knob. The Clojure port preserves this — the per-feature flags exist (:enable-skills,:skip-embedding-retrieval, etc., from round 6) but:mode :emptyis the one-line opt-in.session.options.updateis a SEPARATE RPC, not asession.createfieldThe four feature flags do NOT ride on
session.create— they ship in a follow-upsession.options.updateRPC that the SDK issues after the session is registered. This mirrors upstream and means:session.createsucceeds butsession.options.updatefails, the SDK MUST clean up (else we leak a half-configured session).cleanup-failed-options-update!calls bothsession/disconnect!(CLI-side cleanup viasession.destroy) ANDsession/remove-session!(in-memory registry cleanup); both are idempotent.<apply-session-options-update!) returns a channel that yields:okon success (including the no-op case) or aThrowableon failure (after cleanup), matching the existing async error convention.:copilot-climode + no flags set), the RPC is skipped entirely.:available-toolsrequired, but[]is legitimatevalidate-empty-mode-session-requirements!uses(contains? config :available-tools)— NOTseq. An explicit empty vector[]means "no tools" and is legitimate; only an absent key is rejected. This mirrors upstream'sresolveToolFilterOptionswhere the distinction between "undefined" and "empty list" is meaningful.Always-emit
:tool-filter-precedenceBoth modes now always send
toolFilterPrecedence: "excluded"onsession.createandsession.resume. This is the only behavioral change to:copilot-climode, and it makes the precedence between:available-toolsand:excluded-toolsdeterministic regardless of CLI version (older CLIs might have defaulted differently).::client-modenot::mode::specs/modewas already defined at line 899 formessage-options(#{:enqueue :immediate}). The new spec is named::specs/client-modeand validated in::client-optionsvia a predicate (mirrors the existing::remote-session-modepattern further down).Validation
bb test(unit + integration): 339 tests / 1578 assertions / 0 failures / 0 errors (+ 18 new tests this round across constructor validation, env-var override, wire payload mode-defaults, system-message normalization, andsession.options.updateRPC including async path + cleanup-on-failure)bb validate-docs: cleanbb jar: succeedsCOPILOT_E2E_TESTS=true bb test: 2 flaky E2E timeouts (test-e2e-blob-attachment,test-e2e-list-sessions) — both pre-existing model-latency timeouts onmain, neither exercises:mode :emptyor any new code path. Not regressions.examples/empty_mode.clj: compile-checked. Not included inrun-all-examples.shbecause empty mode disables the local keychain and the example requires a BYOK API key (matches howbyok_provider.cljandmcp_local_server.cljare handled — seeexamples/README.md).Commits
feat(tool-set): add github.copilot-sdk.tool-set namespace + isolated presetfeat(client): implement client mode :empty (multitenancy hardening)docs: document client mode :empty + empty_mode exampleEach commit individually builds; commits 1 and 2 each pass tests at HEAD.