From e534b2c741acc484ec7097292b5ea2b0624ba1fa Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 10 Apr 2026 12:38:30 +0200 Subject: [PATCH 01/25] Share secrets between Code and Agents app via macOS Keychain Add a shared keychain service that stores secrets directly in the macOS Keychain, allowing Code and its embedded Agents app to share auth tokens without re-authentication. Architecture: - ISharedKeychainService (common interface) with ISharedKeychainMainService running in the Electron main process, exposed to renderer via IPC - SharedKeychainMainService wraps @vscode/macos-keychain native addon - NativeSecretStorageService now writes to both the shared keychain and the legacy safeStorage+SQLite pipeline (for rollback safety) - On read, shared keychain is tried first, falling back to legacy Product configuration: - darwinSharedKeychainServiceName: per-flavor service name for data isolation between Stable/Insiders/Exploration - Access group auto-detected from entitlements by the native addon Key design decisions: - Shared keychain only used when type is 'persisted' (not in-memory) - BaseSecretStorageService refactored to expose protected _doGet/_doSet/ _doDelete/_doGetKeys for use by subclasses within sequencer tasks - Native addon is an optional dependency (macOS-only) Files: - build/azure-pipelines/darwin/app-entitlements.plist (keychain-access-groups) - src/vs/platform/secrets/common/sharedKeychainService.ts (interface) - src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts (impl) - src/vs/workbench/services/secrets/electron-browser/sharedKeychainService.ts (IPC proxy) - src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts (wiring) Issue: #308028 --- build/.moduleignore | 8 + .../darwin/app-entitlements.plist | 4 + package-lock.json | 26 ++ package.json | 1 + product.json | 1 + secrets-sharing-impl.md | 216 +++++++++++++ secrets-sharing-spec.md | 288 ++++++++++++++++++ src/typings/macos-keychain.d.ts | 15 + src/vs/base/common/product.ts | 1 + src/vs/code/electron-main/app.ts | 9 + src/vs/platform/secrets/common/secrets.ts | 140 +++++---- .../secrets/common/sharedKeychainService.ts | 25 ++ .../sharedKeychainMainService.ts | 93 ++++++ src/vs/sessions/sessions.desktop.main.ts | 1 + .../electron-browser/secretStorageService.ts | 53 +++- .../electron-browser/sharedKeychainService.ts | 9 + src/vs/workbench/workbench.desktop.main.ts | 1 + 17 files changed, 831 insertions(+), 60 deletions(-) create mode 100644 secrets-sharing-impl.md create mode 100644 secrets-sharing-spec.md create mode 100644 src/typings/macos-keychain.d.ts create mode 100644 src/vs/platform/secrets/common/sharedKeychainService.ts create mode 100644 src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts create mode 100644 src/vs/workbench/services/secrets/electron-browser/sharedKeychainService.ts diff --git a/build/.moduleignore b/build/.moduleignore index f83624f893c37..d199042ac937d 100644 --- a/build/.moduleignore +++ b/build/.moduleignore @@ -159,6 +159,14 @@ vsda/** @vscode/policy-watcher/index.d.ts !@vscode/policy-watcher/build/Release/vscode-policy-watcher.node +@vscode/macos-keychain/build/** +@vscode/macos-keychain/src/** +@vscode/macos-keychain/test/** +@vscode/macos-keychain/binding.gyp +@vscode/macos-keychain/README.md +@vscode/macos-keychain/index.d.ts +!@vscode/macos-keychain/build/Release/keychainNative.node + @vscode/windows-ca-certs/**/* !@vscode/windows-ca-certs/package.json !@vscode/windows-ca-certs/**/*.node diff --git a/build/azure-pipelines/darwin/app-entitlements.plist b/build/azure-pipelines/darwin/app-entitlements.plist index 4073eafcf560d..db62570f361b9 100644 --- a/build/azure-pipelines/darwin/app-entitlements.plist +++ b/build/azure-pipelines/darwin/app-entitlements.plist @@ -10,5 +10,9 @@ com.apple.security.automation.apple-events + keychain-access-groups + + $(TeamIdentifierPrefix)com.microsoft.vscode.shared-secrets + diff --git a/package-lock.json b/package-lock.json index 2ad7243503ff5..9ee7a88b350e5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -170,6 +170,7 @@ "yaserver": "^0.4.0" }, "optionalDependencies": { + "@vscode/macos-keychain": "microsoft/vscode-macos-keychain#85ade16af6da2125eb1d31241002a8a2e5ced710", "windows-foreground-love": "0.6.1" } }, @@ -3835,6 +3836,31 @@ "url": "https://github.com/sponsors/isaacs" } }, + "node_modules/@vscode/macos-keychain": { + "version": "0.0.1", + "resolved": "git+ssh://git@github.com/microsoft/vscode-macos-keychain.git#85ade16af6da2125eb1d31241002a8a2e5ced710", + "integrity": "sha512-/BkWuiBBh8jqk0m0Oy6uj+l4wfLQTD25auuzbri4ejP0gA96Tmmx4sh06CJTo7glNIcnnmiZQulqdQv1lCAqfQ==", + "hasInstallScript": true, + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "dependencies": { + "bindings": "^1.5.0", + "node-addon-api": "^8.2.0" + } + }, + "node_modules/@vscode/macos-keychain/node_modules/node-addon-api": { + "version": "8.7.0", + "resolved": "https://registry.npmjs.org/node-addon-api/-/node-addon-api-8.7.0.tgz", + "integrity": "sha512-9MdFxmkKaOYVTV+XVRG8ArDwwQ77XIgIPyKASB1k3JPq3M8fGQQQE3YpMOrKm6g//Ktx8ivZr8xo1Qmtqub+GA==", + "license": "MIT", + "optional": true, + "engines": { + "node": "^18 || ^20 || >= 21" + } + }, "node_modules/@vscode/native-watchdog": { "version": "1.4.6", "resolved": "https://registry.npmjs.org/@vscode/native-watchdog/-/native-watchdog-1.4.6.tgz", diff --git a/package.json b/package.json index 7c9f41e5797db..06c8075a71a29 100644 --- a/package.json +++ b/package.json @@ -264,6 +264,7 @@ "url": "https://github.com/microsoft/vscode/issues" }, "optionalDependencies": { + "@vscode/macos-keychain": "microsoft/vscode-macos-keychain#85ade16af6da2125eb1d31241002a8a2e5ced710", "windows-foreground-love": "0.6.1" } } diff --git a/product.json b/product.json index 09a0a3e633144..2ccaad558b6b0 100644 --- a/product.json +++ b/product.json @@ -25,6 +25,7 @@ "win32TunnelServiceMutex": "vscodeoss-tunnelservice", "win32TunnelMutex": "vscodeoss-tunnel", "darwinBundleIdentifier": "com.visualstudio.code.oss", + "darwinSharedKeychainServiceName": "com.visualstudio.code.oss.shared-secrets", "darwinProfileUUID": "47827DD9-4734-49A0-AF80-7E19B11495CC", "darwinProfilePayloadUUID": "CF808BE7-53F3-46C6-A7E2-7EDB98A5E959", "linuxIconName": "code-oss", diff --git a/secrets-sharing-impl.md b/secrets-sharing-impl.md new file mode 100644 index 0000000000000..5e1f848a1816a --- /dev/null +++ b/secrets-sharing-impl.md @@ -0,0 +1,216 @@ +# Secrets Sharing Implementation Notes + +**Spec**: `secrets-sharing-spec.md` (in this repo) +**Issue**: https://github.com/microsoft/vscode/issues/308028 +**PR**: https://github.com/microsoft/vscode-macos-keychain/pull/1 +**Date started**: 9 April 2026 + +## What's been done + +### Phase 1, Step 1: macOS native Node addon (`@vscode/macos-keychain`) + +**Location**: `/Users/alex/src/vscode-macos-keychain/` (separate repo, https://github.com/microsoft/vscode-macos-keychain) +**Branch**: `alexdima/keychain-addon` +**Status**: Addon built, compiles cleanly, 11/11 tests passing. Draft PR open. + +The addon wraps the macOS Security framework (`SecItemAdd`, `SecItemCopyMatching`, `SecItemUpdate`, `SecItemDelete`) with `kSecAttrAccessGroup` support for cross-app keychain sharing. + +#### Files created + +``` +vscode-macos-keychain/ + package.json # @vscode/macos-keychain, deps: bindings + node-addon-api, os: ["darwin"] + package-lock.json + binding.gyp # node-gyp config, links Security + CoreFoundation + index.js # JS wrapper using `bindings` package (with platform guard) + index.d.ts # TypeScript type definitions + src/ + main.cc # Node-API entry point (Napi:: C++ wrapper) + keychain.h # Header: CFRef RAII wrapper + function declarations + keychain.cc # Core implementation: buildBaseQuery, set/get/delete/list + test/ + test.js # mocha test suite (11 tests) + azure-pipelines.yml # Azure DevOps CI/CD + npm publishing pipeline + .github/workflows/ci.yml # GitHub Actions CI (macOS, Node 22.x) + .npmignore # Excludes CI, meta files from published package + .vscode/settings.json # Branch protection config + LICENSE # MIT (from upstream repo template) + README.md # Project docs with API, usage, entitlements, testing + CODE_OF_CONDUCT.md # From upstream repo template + SECURITY.md # From upstream repo template + SUPPORT.md # From upstream repo template + .gitignore # Minimal: /node_modules, /build (matches policy-watcher) +``` + +#### API + +```typescript +keychainSet(service: string, account: string, value: string, accessGroup: string): void +keychainGet(service: string, account: string, accessGroup: string): string | undefined +keychainDelete(service: string, account: string, accessGroup: string): boolean +keychainList(service: string, accessGroup: string): string[] +``` + +All string arguments reject embedded NUL bytes. This is intentional: the native CoreFoundation string conversion used by the addon treats NUL as a terminator, so allowing embedded NUL would let distinct logical keys alias the same Keychain item. + +## Decisions and rationale + +### 1. Template: `@vscode/policy-watcher` (not `native-keymap`) + +We surveyed ALL native Node addons used by VS Code: + +| Package | macOS native? | API style | Loader | +|---------|:---:|---|---| +| `native-keymap` (`node-native-keymap`) | Yes (.mm) | Raw `napi_*` C API | Manual `require('./build/Release/...')` | +| `@vscode/policy-watcher` | Yes (C++) | `node-addon-api` (C++ NAPI wrapper) | `bindings` package | +| `@vscode/spdlog` | Yes (cross-platform C++) | `node-addon-api` | `bindings` | +| `@vscode/windows-mutex` | No (Windows) | `node-addon-api` | `bindings` | +| `@vscode/deviceid` | No (Windows) | Raw NAPI | Manual | +| `@vscode/windows-process-tree` | No (Windows) | — | — | +| `node-pty` | Yes | — | Too complex for reference | + +**Decision**: Use `@vscode/policy-watcher` as the primary template because: +- It uses the modern `node-addon-api` C++ wrapper (cleaner than raw `napi_*`) +- It has macOS-specific native code in `src/macos/` +- It uses the `bindings` package for loading (standard pattern) +- It's a Microsoft project with the same license/header conventions +- `native-keymap` is older: raw C NAPI, manual `require('./build/Release/...')` loading, Objective-C++ `.mm` files + +### 2. `node-addon-api` version: `^8.2.0` (not `7.1.0`) + +VS Code's `package.json` pins `node-addon-api` at `7.1.0`, but `@vscode/policy-watcher` (the newest addon) uses `^8.2.0`. We followed the newer addon's lead. This may need alignment when the addon is added to VS Code's dependencies. + +### 3. `Napi::Boolean` must be fully qualified + +**Problem discovered during build**: `return Boolean::New(env, deleted)` caused a compilation error: + +``` +error: reference to 'Boolean' is ambiguous +``` + +macOS `MacTypes.h` defines `typedef unsigned char Boolean;` which conflicts with `Napi::Boolean`. The file has `using namespace Napi;` so both are in scope. + +**Fix**: Use `Napi::Boolean::New(env, deleted)` instead of `Boolean::New(env, deleted)`. This only affects `Boolean` — `String`, `Array`, `Value`, etc. don't have conflicts with macOS system headers. + +### 4. `kSecUseDataProtectionKeychain` availability warning + +**Problem discovered during build**: The compiler warns: + +``` +'kSecUseDataProtectionKeychain' is only available on macOS 10.15 or newer [-Wunguarded-availability-new] +``` + +This is because node-gyp defaults `MACOSX_DEPLOYMENT_TARGET` to `10.7`, and `kSecUseDataProtectionKeychain` was introduced in 10.15. + +**Attempted fixes**: +1. `"MACOSX_DEPLOYMENT_TARGET": "10.15"` in xcode_settings — **did not work** because node-gyp uses `make` on macOS (not Xcode), so xcode_settings for deployment target are ignored by the makefile generator. +2. `"CLANG_WARN_UNGUARDED_AVAILABILITY": "NO"` in xcode_settings — **did not work** for the same reason. +3. `"-Wno-unguarded-availability-new"` in `WARNING_CFLAGS` (xcode_settings) — **worked**. The `WARNING_CFLAGS` xcode_setting DOES get passed through to the compiler via the makefile. + +**Final fix**: Added `-Wno-unguarded-availability-new` to both `cflags` (for GCC/Clang direct builds) and `WARNING_CFLAGS` (for xcode_settings pass-through). Also kept `MACOSX_DEPLOYMENT_TARGET: "10.15"` for documentation purposes even though node-gyp overrides it. + +VS Code already requires macOS 10.15+, so the availability guard is unnecessary. + +### 5. Access group is optional (empty string = skip) + +**Problem discovered during testing**: All keychain operations fail with `-34018` (`errSecMissingEntitlement`) when `kSecUseDataProtectionKeychain = true` is set but the app isn't signed with the `keychain-access-groups` entitlement. This means you **cannot test the addon locally** with a real access group using plain `node`. + +**Solution**: When `accessGroup` is an **empty string**, `buildBaseQuery()` skips both `kSecAttrAccessGroup` and `kSecUseDataProtectionKeychain`. Items go to the app's default keychain, which works without entitlements. This enables local development and CI testing. + +When `accessGroup` is non-empty (production), both attributes are set, requiring proper entitlements. This is the intended code path inside signed VS Code/Agents builds. + +### 6. Set is upsert (add-then-update pattern) + +`keychainSet` first calls `SecItemAdd`. If that returns `errSecDuplicateItem`, it falls back to building a new search query + `SecItemUpdate`. This two-step pattern is the standard approach for macOS Keychain — there is no "upsert" API. + +Note: The update path builds a **new** search query (without `kSecValueData`) and a separate attributes dict (with only `kSecValueData`). This is required by `SecItemUpdate` — it does not accept `kSecValueData` in the query dictionary. + +### 7. `CFRef` RAII wrapper for CoreFoundation objects + +CoreFoundation objects must be `CFRelease`d. Rather than tracking releases manually (error-prone, especially with early returns and exceptions), we use a `CFRef` template class that calls `CFRelease` in its destructor. It supports move semantics. When setting `CFRef`-managed objects into CF dictionaries, we use `.get()` so the RAII wrapper's destructor balances the create, while `CFDictionarySetValue` (with `kCFTypeDictionaryValueCallBacks`) retains its own +1 reference. + +**Note**: An earlier version used `.release()` to transfer ownership into dictionaries, which caused a refcount leak — `CFDictionarySetValue` retains the value, but `.release()` also gave up RAII ownership, leaving a dangling +1. Fixed by switching to `.get()`. + +### 8. Error messages include human-readable text and OSStatus code (no account names) + +All error paths use `SecCopyErrorMessageString` to get a human-readable description and also include the numeric OSStatus code. Example: `"Keychain set failed: A required entitlement isn't present. (-34018)"`. Account and service names are intentionally **omitted** from error messages to prevent them from leaking into telemetry or log files. + +### 9. Secrets stored as plaintext in Keychain (not encrypted separately) + +Per the spec: macOS Keychain `kSecClassGenericPassword` does NOT have the blob size limitations that Windows CredMan has. The keychain itself IS the secure storage — no need for an additional encryption layer (unlike the current safeStorage + SQLite approach). Values are stored as `kSecValueData` (`CFData` from UTF-8 bytes) directly. + +### 10. Test cleanup uses beforeEach/afterEach with try-catch + +Tests clean up keychain items in both `beforeEach` and `afterEach` to ensure idempotency even if a previous test run crashed. The `try-catch` handles the case where items don't exist. + +### 11. Platform guard in index.js + `"os": ["darwin"]` in package.json + +Added `if (process.platform !== 'darwin') throw ...` at the top of `index.js` for a clear error message at import time. Also added `"os": ["darwin"]` to `package.json` so `npm install` fails fast on non-macOS. The existing VS Code addons don't use either pattern, but this module is macOS-only by design so both are appropriate. + +### 12. Null check for updateAttrs dictionary + +Added a null check after `CFDictionaryCreateMutable` for the `updateAttrs` dict in the update path of `keychainSet`, consistent with the existing check in `buildBaseQuery`. + +### 13. Security hardening (from security audit) + +A dedicated security audit identified and fixed the following: + +- **CF ownership fix**: Fixed CoreFoundation reference counting in `buildBaseQuery` — changed `.release()` to `.get()` for values set into dictionaries, since `CFDictionarySetValue` retains. The original `.release()` left a dangling +1 refcount on every keychain call. +- **Secret zeroing**: A `secureClear()` utility uses a `volatile char*` pointer to zero `std::string` buffers containing secrets immediately after use, preventing the compiler from optimizing away the zeroing. Applied to the `value` parameter in `keychainSet` and the return value in `keychainGet`. +- **Build hardening flags**: Added `-D_FORTIFY_SOURCE=2` (runtime buffer overflow checks) and `-Wformat -Wformat-security` (format string vulnerability warnings) alongside the existing `-fstack-protector-strong`. +- **Error message sanitization**: Removed account/service names from error messages to avoid leaking them into telemetry or log files. The caller already knows these values. +- **Input length bounds**: Values are capped at 100 KB, and service/account/accessGroup strings at 1 KB. Apple does not document hard keychain item size limits, but the Data Protection keychain (SQLite-backed) is known to work reliably up to ~100 KB. These bounds provide predictable error messages instead of opaque OS-level failures. +- **NUL byte rejection**: All string arguments are validated to reject embedded NUL bytes, which could cause silent truncation when passed to CoreFoundation `CFStringCreateWithCString`. + +### 14. CI/CD and project meta files + +`azure-pipelines.yml` follows the `@vscode/policy-watcher` pattern: uses `microsoft/vscode-engineering` pipeline templates for npm package build/test/publish. Only tests on macOS (unlike cross-platform addons). GitHub Actions CI (`.github/workflows/ci.yml`) runs on `macos-latest` with Node 22.x. `.npmignore` excludes CI files, meta docs, and build artifacts from the published package. + +### 15. Git rebase gotcha: `--ours`/`--theirs` are swapped + +During `git rebase origin/main`, conflict resolution with `git checkout --ours README.md` took the **upstream** version, not ours. This is because rebase replays our commits onto the upstream, making the upstream `HEAD` the "ours" and our commit the "theirs". The README was silently lost and discovered later. Lesson: during rebase conflicts, use `--theirs` to keep your own changes. + +## Things NOT done yet (from the spec's Phase 1 plan) + +### Step 2: Entitlements +Add `keychain-access-groups` entitlement to both Code and Agents app builds: +- Location: `build/darwin/` entitlements files +- Value: `$(TeamIdentifierPrefix)com.microsoft.vscode.shared-secrets` +- Both Code.app and the nested Agents .app need the same group + +### Step 3: `ISharedSecretStorageService` in VS Code +New service that: +- On write: stores in shared keychain via addon + optionally keeps old pipeline in sync +- On read: reads from shared keychain, falls back to old safeStorage+SQLite +- On delete: deletes from both +- Files involved: `src/vs/platform/secrets/`, `src/vs/workbench/services/secrets/` + +### Step 4: Migration logic +One-time migration in Code on startup: +- Read all `secret://`-prefixed keys from SQLite (IStorageService) +- Decrypt each with existing safeStorage (IEncryptionService) +- Write plaintext to shared keychain via addon +- Must be idempotent/re-entrant +- Store migration-complete flag + +### Step 5: Wire up the Agents app +The Agents app (`src/vs/sessions/sessions.desktop.main.ts`) should use the shared keychain for secrets. It reads from the shared keychain directly — no migration needed from the Agents side. + +### Step 6: Testing with real auth tokens +Test with large Microsoft account refresh tokens (the ones that exceed Windows CredMan's ~2.5KB limit). macOS Keychain should handle them fine but needs verification. + +## Open questions (from spec, still open) + +1. **Naming convention for keychain items**: What `kSecAttrService` value? Spec suggests `"com.microsoft.vscode.shared-secrets"` — needs to be stable across versions. +2. **Entitlement signing**: Does adding `keychain-access-groups` require CI/CD signing process changes? +3. **Notification/sync**: Should Code notify Agents in real-time when secrets change? Or read-on-demand? +4. **Scope**: Share ALL secrets or only auth-related ones? + +## Environment notes + +- Built and tested on macOS (Darwin 25.4.0, arm64) +- Node.js v22.22.1 +- node-gyp v11.2.0 (bundled with npm) +- `node-addon-api` v8.3.1 (resolved from `^8.2.0`) +- Compilation uses `make` (not Xcode), which affects how xcode_settings are applied +- GPG commit signing fails inside the VS Code agent sandbox (can't access `~/.gnupg` or gpg-agent) — must use unsandboxed execution for `git commit -S` and `git push` diff --git a/secrets-sharing-spec.md b/secrets-sharing-spec.md new file mode 100644 index 0000000000000..a00f58f9656a1 --- /dev/null +++ b/secrets-sharing-spec.md @@ -0,0 +1,288 @@ +# Secrets Sharing Between VS Code and Agents App + +**Issue**: https://github.com/microsoft/vscode/issues/308028 +**Related**: https://github.com/microsoft/vscode/issues/308353 +**Assignees**: alexdima, deepak1556 +**Stakeholders**: Tyler Leonhardt (auth team), Kai (requested the work) + +## Problem Statement + +The Agents app is a standalone application bundled inside VS Code. When a user is already authenticated in VS Code (e.g., signed into GitHub, Microsoft account), they must re-authenticate in the Agents app because the two apps have completely isolated secret stores. This is a friction point for onboarding — users expect to be signed in automatically. + +## Current Architecture + +### How secrets are stored today + +The secret storage pipeline has three layers: + +#### Layer 1: Encryption via Electron's `safeStorage` + +**File**: `src/vs/platform/encryption/electron-main/encryptionMainService.ts` + +`EncryptionMainService` wraps Electron's `safeStorage` API: +- `safeStorage.encryptString(value)` — encrypts a string using an OS-managed encryption key +- `safeStorage.decryptString(buffer)` — decrypts it back + +On macOS, Electron stores the encryption key in the **macOS Keychain** under the app's own name (derived from `CFBundleName`). On macOS, `getKeyStorageProvider()` returns `KnownStorageProvider.keychainAccess`. + +The encrypted values are JSON-serialized `Buffer` objects (e.g., `{"data": "..."}`). + +#### Layer 2: Secret storage service (encrypt-then-store in SQLite) + +**File**: `src/vs/platform/secrets/common/secrets.ts` + +`BaseSecretStorageService` stores secrets with a `secret://` key prefix in `IStorageService` (a SQLite database scoped to `StorageScope.APPLICATION`): + +- **`set(key, value)`**: Encrypts `value` via `IEncryptionService.encrypt()`, then stores the ciphertext in SQLite under the key `secret://`. +- **`get(key)`**: Reads the ciphertext from SQLite under `secret://`, then decrypts via `IEncryptionService.decrypt()`. +- **`delete(key)`**: Removes the entry from SQLite. +- **`keys()`**: Lists all `secret://`-prefixed keys from SQLite. + +The desktop override is `NativeSecretStorageService` in `src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts`, which adds user notifications for cases where encryption is unavailable. + +#### Layer 3: Extension API + +Extensions use `vscode.ExtensionContext.secrets.store(key, value)` / `.get(key)`, which delegates to `ISecretStorageService`. Example: `extensions/github-authentication/src/common/keychain.ts`. + +### How the two apps are deployed + +On macOS, the Agents app is a **nested `.app` bundle** inside the main Code `.app`: + +``` +Code.app/ + Contents/ + Applications/ + .app ← separate bundle, own bundle ID, own Dock icon +``` + +This is configured in `build/gulpfile.vscode.ts` using metadata from `product.json`'s `embedded` key (typed as `EmbeddedProductInfo` in `build/lib/embeddedType.ts`): + +```typescript +export type EmbeddedProductInfo = { + nameShort: string; + nameLong: string; + applicationName: string; + dataFolderName: string; // ← separate data directory + darwinBundleIdentifier: string; // ← separate bundle ID + urlProtocol: string; + // ... win32 fields +}; +``` + +When the Agents `.app` launches, it runs its **own Electron main process** with `process.isEmbeddedApp = true`. It has its own Dock icon, bundle identifier, URL protocol, and data folder. See `src/vs/code/electron-main/app.ts` — when `isEmbeddedApp` is true, it calls `openAgentsWindow()` which opens a `BrowserWindow` loading `sessions.html` instead of `workbench.html`. + +The Agents window can also be opened within Code's process via the `--agents` flag (development/insider only), in which case it's just another `BrowserWindow` in the same Electron main process. + +### Why secrets are currently isolated (two barriers) + +#### Barrier 1: Encryption key isolation (macOS) + +Electron's `safeStorage` stores its encryption key in the macOS Keychain under the **app's own `CFBundleName`**. Code's keychain item has a name like `"Code"` or `"Code - Insiders"`, while the Agents app would have its own name (e.g., `"Agents"`). These are **separate keychain items with separate encryption keys**. The Agents app cannot read Code's encryption key, and vice versa. The `safeStorage` API has no concept of shared access groups. + +#### Barrier 2: Storage database isolation (all platforms) + +Each app has its own `dataFolderName` (from the `embedded` product config), so the SQLite databases where encrypted secrets live are in **completely separate directories** (e.g., `~/Library/Application Support/Code/` vs `~/Library/Application Support/Agents/`). Even if the two apps could use the same encryption key, they wouldn't find each other's stored ciphertext. + +### How the Agents app currently uses secrets + +The Agents app (`src/vs/sessions/`) imports the same `NativeSecretStorageService` as Code: +- `src/vs/sessions/sessions.desktop.main.ts` imports `../workbench/services/secrets/electron-browser/secretStorageService.js` + +So it goes through the exact same encrypt-then-store-in-SQLite pipeline, but with its own encryption key and its own SQLite database. + +## Team Discussion Summary + +### Tyler Leonhardt (auth team) confirmed: + +1. The current-state analysis above is correct. +2. He prefers a "shared keychain" concept over a backdoor into VS Code's secrets. +3. **Critical warning for Windows**: Windows CredMan has a **~2.5 KB blob size limit** per credential. A single Microsoft auth refresh token already exceeds this limit. This is why Electron stores only the small encryption key in the credential store and puts the actual (large) encrypted secrets in SQLite. **Do not attempt to store secrets directly in CredMan on Windows.** + +### deepak1556 (Electron/platform team) recommended: + +1. **macOS**: Use Apple's [Keychain Access Groups](https://developer.apple.com/documentation/security/sharing-access-to-keychain-items-among-a-collection-of-apps) — the official mechanism for sharing keychain items between apps signed by the same team. Since Electron's `safeStorage` API has no concept of access groups, **build a native Node.js addon** (Objective-C) rather than waiting on Electron/Chromium changes. +2. **Windows**: DPAPI is user-scoped, not app-scoped — any process running as the same user can already encrypt/decrypt with the same key. The only barrier is the separate SQLite databases. However, app-bound encryption (per-app isolation) is a concept from Chromium that could complicate this in the future; we don't use it today. + +## Proposed Solution (macOS first) + +### Design Principles + +1. **Don't change how Code stores secrets for existing users** — millions of installations have secrets in the current format (safeStorage + SQLite). The new mechanism must coexist with the old one. +2. **Migration must be re-entrant** — if we find a critical problem, we can roll back. Re-running migration should be safe (idempotent). +3. **macOS first, then Windows/Linux** — macOS is the most complex case (true app-level keychain isolation). Windows and Linux are simpler. +4. **Native addon, no Electron changes required** — deepak1556's recommendation. Keeps us in control of the timeline. + +### macOS: Native Node Addon with Shared Keychain Access Group + +#### Why store directly in Keychain on macOS? + +macOS Keychain (`kSecClassGenericPassword`) does **not** have the blob size limitations that Windows CredMan has. Large values (auth tokens, etc.) can be stored directly as keychain items. This eliminates the need for a separate SQLite database for shared secrets, which is a major simplification — no need to coordinate database locations or cross-read another app's SQLite file. + +#### Keychain access groups (Apple mechanism) + +From [Apple's documentation](https://developer.apple.com/documentation/security/sharing-access-to-keychain-items-among-a-collection-of-apps): + +- Every app has a default access group equal to its **app ID** (`$(TeamID).$(BundleID)`), making its keychain items private by default. +- Apps signed by the same team can share keychain items by adding a common **keychain access group** to both apps' entitlements via the `keychain-access-groups` entitlement. +- Each keychain item belongs to exactly one access group (set via `kSecAttrAccessGroup` when creating the item). +- When searching, you can specify `kSecAttrAccessGroup` to limit the search, or omit it to search all groups your app belongs to. + +#### Entitlements + +Both `Code.app` and `Agents.app` need the same keychain access group in their entitlements: + +```xml +keychain-access-groups + + $(TeamIdentifierPrefix)com.microsoft.vscode.shared-secrets + +``` + +Adding a keychain access group to entitlements is **backward-compatible** — it does not affect existing keychain items stored under the app's default access group. Existing safeStorage-managed items remain accessible. + +#### Native addon API + +Build a native Node.js addon (C/Objective-C) that wraps the macOS Security framework: + +``` +// Conceptual API: +keychainSet(service: string, account: string, value: string, accessGroup: string): void +keychainGet(service: string, account: string, accessGroup: string): string | undefined +keychainDelete(service: string, account: string, accessGroup: string): void +keychainList(service: string, accessGroup: string): string[] // returns account names +``` + +Internally uses `SecItemAdd`, `SecItemCopyMatching`, `SecItemUpdate`, `SecItemDelete` with: +- `kSecClass`: `kSecClassGenericPassword` +- `kSecAttrService`: A agreed-upon service name, e.g. `"com.microsoft.vscode.shared-secrets"` +- `kSecAttrAccount`: The secret key (e.g., `"github.auth"`) +- `kSecAttrAccessGroup`: `".com.microsoft.vscode.shared-secrets"` +- `kSecValueData`: The secret value (UTF-8 encoded) +- `kSecUseDataProtectionKeychain`: `true` (required for access groups on macOS) + +#### Service architecture + +A new `ISharedSecretStorageService` (or an alternative `ISecretStorageService` implementation) that: + +1. On **write**: Stores the secret directly in macOS Keychain using the native addon with the shared access group. Also notifies the old `ISecretStorageService` pipeline to keep the local copy in sync (for rollback safety). +2. On **read**: Reads from the shared keychain via the native addon. Falls back to the old pipeline if not found (pre-migration secrets). +3. On **delete**: Deletes from both the shared keychain and the old pipeline. + +#### Migration + +A one-time, re-entrant migration runs in **both** Code and the Agents app on startup: + +**In Code (the primary migration source)**: +1. Read all `secret://`-prefixed keys from `IStorageService` (SQLite). +2. For each key, decrypt using the existing `safeStorage`-based `IEncryptionService`. +3. Write the decrypted value to the shared keychain using the native addon. +4. Mark the migration as complete in a storage flag. + +**Re-entrancy**: The migration can be run multiple times safely. Writing to the keychain is idempotent (if the item already exists with the same value, it's a no-op or update). If a critical issue is found, the old storage still has all secrets untouched — we can remove the shared keychain code path and fall back. + +**In the Agents app**: Reads from the shared keychain directly. No migration needed from the Agents side — it just needs to find the secrets Code put there. + +### Data flow (macOS, after migration) + +``` +Extension calls context.secrets.get("github.auth") + → ISecretStorageService.get("github.auth") + → [new path] Native addon: SecItemCopyMatching( + service: "com.microsoft.vscode.shared-secrets", + account: "github.auth", + accessGroup: ".com.microsoft.vscode.shared-secrets" + ) + → Returns plaintext secret directly from keychain + → (no SQLite, no encrypt/decrypt needed — keychain IS the secure storage) +``` + +### Comparison: old vs. new (macOS) + +| Aspect | Old (safeStorage + SQLite) | New (shared keychain) | +|--------|---------------------------|----------------------| +| Encryption key | Per-app, in Keychain, managed by Electron | Not needed — Keychain itself is the secure store | +| Secret storage | Encrypted ciphertext in per-app SQLite | Plaintext in Keychain, protected by OS | +| Cross-app access | Impossible | Via shared `keychain-access-groups` entitlement | +| Size limits | None (SQLite) | None on macOS Keychain | +| Native code needed | No (Electron handles it) | Yes (native Node addon) | + +## Windows Strategy (future work) + +**Important constraint from Tyler**: Windows CredMan has a ~2.5 KB limit per credential — a single Microsoft auth refresh token exceeds this. This is why Electron chose to store only the encryption key (small) in the OS credential store, rather than the actual secrets. **Do not store secrets directly in CredMan.** + +### Current Windows state (from deepak1556) + +On Windows, when using `safeStorage` today, the DPAPI encryption keys are stored to a **file** under the user data directory after base64 encoding. There is **no protection between app processes for the same user on Windows** — any app running as the same user can read this file. This also applies to Windows Credential Manager (same process and user scope as DPAPI). This is why Chrome had to invent their own app-bound encryption to isolate secrets per app. + +### Likely approach: Cross-read Code's storage + +Since both the encryption key file and the SQLite database are just files under the user data directory, the Agents app can: + +1. **Read Code's DPAPI encryption key file** from Code's user data directory (the key is base64-encoded in a file, readable by any same-user process). +2. **Read Code's SQLite database** to find `secret://`-prefixed entries. +3. **Decrypt the ciphertext** using the encryption key — or equivalently, use `safeStorage.decryptString()` if within the same Electron process, since DPAPI is user-scoped. + +The Agents app needs to know Code's `dataFolderName` path, which is available from the parent app's `product.json` or can be derived from the `embedded` configuration. + +**Note**: Chromium has app-bound encryption on Windows that isolates secrets per app, but VS Code does not use this today. If it's adopted in the future, the cross-reading approach would break and would need revisiting. Deepak will provide more details when work begins on Windows. + +## Linux Strategy (future work) + +Similar to Windows — keyring services (gnome-keyring, kwallet) are user-scoped, not app-scoped. The Agents app should be able to cross-read Code's SQLite database and decrypt with its own `safeStorage` call. The same `dataFolderName` cross-referencing approach applies. + +## Files Involved + +| File | Role | +|------|------| +| `src/vs/platform/encryption/common/encryptionService.ts` | `IEncryptionService` interface, `KnownStorageProvider` enum | +| `src/vs/platform/encryption/electron-main/encryptionMainService.ts` | `EncryptionMainService` — wraps `electron.safeStorage` | +| `src/vs/platform/secrets/common/secrets.ts` | `BaseSecretStorageService` — encrypt-then-store in SQLite | +| `src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts` | `NativeSecretStorageService` — desktop override with notifications | +| `src/vs/sessions/sessions.desktop.main.ts` | Agents app entry point — imports secretStorageService | +| `src/vs/code/electron-main/app.ts` | Main process — `isEmbeddedApp` handling, `registerEmbeddedAppWithLaunchServices` | +| `build/lib/embeddedType.ts` | `EmbeddedProductInfo` type (bundle ID, data folder, etc.) | +| `build/gulpfile.vscode.ts` | macOS packaging — `darwinMiniAppName`, `darwinMiniAppBundleIdentifier`, etc. | +| `extensions/github-authentication/src/common/keychain.ts` | Example extension using `context.secrets` | + +## Implementation Plan + +### Phase 1: macOS shared keychain (starting here) + +1. **Native addon**: Create a macOS-specific native Node.js addon that wraps `SecItemAdd`/`SecItemCopyMatching`/`SecItemUpdate`/`SecItemDelete` with access group support. + - Location: `src/vs/platform/encryption/electron-main/darwin/` (or similar) + - Must handle: UTF-8 encoding, error codes, item-not-found vs. real errors, updating existing items + +2. **Entitlements**: Add `keychain-access-groups` entitlement to both Code and Agents app builds. + - Location: `build/darwin/` entitlements files + +3. **New service**: Create a shared-keychain-backed `ISecretStorageService` implementation for macOS. + - Reads/writes directly to Keychain with the shared access group + - Falls back to old safeStorage+SQLite path if keychain read fails + +4. **Migration logic**: In Code, on startup, migrate existing secrets to the shared keychain. + - Read all `secret://` keys from SQLite + - Decrypt each with safeStorage + - Write plaintext to shared keychain via native addon + - Store a "migration complete" flag + - Must be idempotent / re-entrant + +5. **Testing**: Test with real auth tokens (Microsoft account refresh tokens are large — this was the CredMan problem on Windows, but macOS Keychain should handle it fine). Test both directions: Code writes, Agents reads; verify sign-in state is shared. + +### Phase 2: Windows (future) + +- Evaluate the approaches listed above with deepak1556 +- Likely: cross-read Code's SQLite + same DPAPI key +- Must avoid CredMan for secret values (size limit) + +### Phase 3: Linux (future) + +- Similar to Windows approach +- Test with gnome-keyring and kwallet backends + +## Open Questions + +1. **Naming convention for keychain items**: What `kSecAttrService` value to use? Needs to be stable across versions. +2. **Entitlement signing**: Does adding `keychain-access-groups` require any changes to the CI/CD signing process? +3. **App-bound encryption on Windows**: If Chromium/Electron adopts this in the future, what's the fallback plan? +4. **Notification/sync**: When Code writes a new secret, should the Agents app be notified in real-time? Or is read-on-demand sufficient? +5. **Scope**: Should we share ALL secrets, or only auth-related ones? Sharing all is simpler but may have unintended consequences. diff --git a/src/typings/macos-keychain.d.ts b/src/typings/macos-keychain.d.ts new file mode 100644 index 0000000000000..d10bdab322e19 --- /dev/null +++ b/src/typings/macos-keychain.d.ts @@ -0,0 +1,15 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +// Type declarations for @vscode/macos-keychain. +// The package is an optional dependency (macOS-only native addon), so types +// are duplicated here to ensure TypeScript compilation succeeds on all platforms. + +declare module '@vscode/macos-keychain' { + export function keychainSet(service: string, account: string, value: string): void; + export function keychainGet(service: string, account: string): string | undefined; + export function keychainDelete(service: string, account: string): boolean; + export function keychainList(service: string): string[]; +} diff --git a/src/vs/base/common/product.ts b/src/vs/base/common/product.ts index 32e0693eacf12..a5fc11f97759e 100644 --- a/src/vs/base/common/product.ts +++ b/src/vs/base/common/product.ts @@ -221,6 +221,7 @@ export interface IProductConfiguration { readonly 'editSessions.store'?: Omit; readonly darwinUniversalAssetId?: string; readonly darwinBundleIdentifier?: string; + readonly darwinSharedKeychainServiceName?: string; readonly profileTemplatesUrl?: string; readonly commonlyUsedSettings?: string[]; diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts index 188b0b7543134..e865bc32ddfc5 100644 --- a/src/vs/code/electron-main/app.ts +++ b/src/vs/code/electron-main/app.ts @@ -37,6 +37,8 @@ import { DiagnosticsMainService, IDiagnosticsMainService } from '../../platform/ import { DialogMainService, IDialogMainService } from '../../platform/dialogs/electron-main/dialogMainService.js'; import { IEncryptionMainService } from '../../platform/encryption/common/encryptionService.js'; import { EncryptionMainService } from '../../platform/encryption/electron-main/encryptionMainService.js'; +import { ISharedKeychainMainService } from '../../platform/secrets/common/sharedKeychainService.js'; +import { SharedKeychainMainService } from '../../platform/secrets/electron-main/sharedKeychainMainService.js'; import { NativeBrowserElementsMainService, INativeBrowserElementsMainService } from '../../platform/browserElements/electron-main/nativeBrowserElementsMainService.js'; import { ipcBrowserViewChannelName } from '../../platform/browserView/common/browserView.js'; import { ipcBrowserViewGroupChannelName } from '../../platform/browserView/common/browserViewGroup.js'; @@ -1087,6 +1089,9 @@ export class CodeApplication extends Disposable { // Encryption services.set(IEncryptionMainService, new SyncDescriptor(EncryptionMainService)); + // Shared Keychain + services.set(ISharedKeychainMainService, new SyncDescriptor(SharedKeychainMainService)); + // Browser Elements services.set(INativeBrowserElementsMainService, new SyncDescriptor(NativeBrowserElementsMainService, undefined, false /* proxied to other processes */)); @@ -1252,6 +1257,10 @@ export class CodeApplication extends Disposable { const encryptionChannel = ProxyChannel.fromService(accessor.get(IEncryptionMainService), disposables); mainProcessElectronServer.registerChannel('encryption', encryptionChannel); + // Shared Keychain + const sharedKeychainChannel = ProxyChannel.fromService(accessor.get(ISharedKeychainMainService), disposables); + mainProcessElectronServer.registerChannel('sharedKeychain', sharedKeychainChannel); + // Browser Elements const browserElementsChannel = ProxyChannel.fromService(accessor.get(INativeBrowserElementsMainService), disposables); mainProcessElectronServer.registerChannel('browserElements', browserElementsChannel); diff --git a/src/vs/platform/secrets/common/secrets.ts b/src/vs/platform/secrets/common/secrets.ts index ee75762fe2281..6cccd13679fa9 100644 --- a/src/vs/platform/secrets/common/secrets.ts +++ b/src/vs/platform/secrets/common/secrets.ts @@ -64,74 +64,98 @@ export class BaseSecretStorageService extends Disposable implements ISecretStora } get(key: string): Promise { - return this._sequencer.queue(key, async () => { - const storageService = await this.resolvedStorageService; - - const fullKey = this.getKey(key); - this._logService.trace('[secrets] getting secret for key:', fullKey); - const encrypted = storageService.get(fullKey, StorageScope.APPLICATION); - if (!encrypted) { - this._logService.trace('[secrets] no secret found for key:', fullKey); - return undefined; - } + return this._sequencer.queue(key, () => this._doGet(key)); + } - try { - this._logService.trace('[secrets] decrypting gotten secret for key:', fullKey); - // If the storage service is in-memory, we don't need to decrypt - const result = this._type === 'in-memory' - ? encrypted - : await this._encryptionService.decrypt(encrypted); - this._logService.trace('[secrets] decrypted secret for key:', fullKey); - return result; - } catch (e) { - this._logService.error(e); - this.delete(key); - return undefined; - } - }); + /** + * Read from the safeStorage+SQLite pipeline without going through the sequencer. + * Must only be called from within a sequencer-queued task for the same key. + */ + protected async _doGet(key: string): Promise { + const storageService = await this.resolvedStorageService; + + const fullKey = this.getKey(key); + this._logService.trace('[secrets] getting secret for key:', fullKey); + const encrypted = storageService.get(fullKey, StorageScope.APPLICATION); + if (!encrypted) { + this._logService.trace('[secrets] no secret found for key:', fullKey); + return undefined; + } + + try { + this._logService.trace('[secrets] decrypting gotten secret for key:', fullKey); + // If the storage service is in-memory, we don't need to decrypt + const result = this._type === 'in-memory' + ? encrypted + : await this._encryptionService.decrypt(encrypted); + this._logService.trace('[secrets] decrypted secret for key:', fullKey); + return result; + } catch (e) { + this._logService.error(e); + this.delete(key); + return undefined; + } } set(key: string, value: string): Promise { - return this._sequencer.queue(key, async () => { - const storageService = await this.resolvedStorageService; - - this._logService.trace('[secrets] encrypting secret for key:', key); - let encrypted; - try { - // If the storage service is in-memory, we don't need to encrypt - encrypted = this._type === 'in-memory' - ? value - : await this._encryptionService.encrypt(value); - } catch (e) { - this._logService.error(e); - throw e; - } - const fullKey = this.getKey(key); - this._logService.trace('[secrets] storing encrypted secret for key:', fullKey); - storageService.store(fullKey, encrypted, StorageScope.APPLICATION, StorageTarget.MACHINE); - this._logService.trace('[secrets] stored encrypted secret for key:', fullKey); - }); + return this._sequencer.queue(key, () => this._doSet(key, value)); + } + + /** + * Write to the safeStorage+SQLite pipeline without going through the sequencer. + * Must only be called from within a sequencer-queued task for the same key. + */ + protected async _doSet(key: string, value: string): Promise { + const storageService = await this.resolvedStorageService; + + this._logService.trace('[secrets] encrypting secret for key:', key); + let encrypted; + try { + // If the storage service is in-memory, we don't need to encrypt + encrypted = this._type === 'in-memory' + ? value + : await this._encryptionService.encrypt(value); + } catch (e) { + this._logService.error(e); + throw e; + } + const fullKey = this.getKey(key); + this._logService.trace('[secrets] storing encrypted secret for key:', fullKey); + storageService.store(fullKey, encrypted, StorageScope.APPLICATION, StorageTarget.MACHINE); + this._logService.trace('[secrets] stored encrypted secret for key:', fullKey); } delete(key: string): Promise { - return this._sequencer.queue(key, async () => { - const storageService = await this.resolvedStorageService; - - const fullKey = this.getKey(key); - this._logService.trace('[secrets] deleting secret for key:', fullKey); - storageService.remove(fullKey, StorageScope.APPLICATION); - this._logService.trace('[secrets] deleted secret for key:', fullKey); - }); + return this._sequencer.queue(key, () => this._doDelete(key)); + } + + /** + * Delete from the safeStorage+SQLite pipeline without going through the sequencer. + * Must only be called from within a sequencer-queued task for the same key. + */ + protected async _doDelete(key: string): Promise { + const storageService = await this.resolvedStorageService; + + const fullKey = this.getKey(key); + this._logService.trace('[secrets] deleting secret for key:', fullKey); + storageService.remove(fullKey, StorageScope.APPLICATION); + this._logService.trace('[secrets] deleted secret for key:', fullKey); } keys(): Promise { - return this._sequencer.queue('__keys__', async () => { - const storageService = await this.resolvedStorageService; - this._logService.trace('[secrets] fetching keys of all secrets'); - const allKeys = storageService.keys(StorageScope.APPLICATION, StorageTarget.MACHINE); - this._logService.trace('[secrets] fetched keys of all secrets'); - return allKeys.filter(key => key.startsWith(this._storagePrefix)).map(key => key.slice(this._storagePrefix.length)); - }); + return this._sequencer.queue('__keys__', () => this._doGetKeys()); + } + + /** + * List all secret keys from the safeStorage+SQLite pipeline without going through the sequencer. + * Must only be called from within a sequencer-queued task. + */ + protected async _doGetKeys(): Promise { + const storageService = await this.resolvedStorageService; + this._logService.trace('[secrets] fetching keys of all secrets'); + const allKeys = storageService.keys(StorageScope.APPLICATION, StorageTarget.MACHINE); + this._logService.trace('[secrets] fetched keys of all secrets'); + return allKeys.filter(key => key.startsWith(this._storagePrefix)).map(key => key.slice(this._storagePrefix.length)); } private async initialize(): Promise { diff --git a/src/vs/platform/secrets/common/sharedKeychainService.ts b/src/vs/platform/secrets/common/sharedKeychainService.ts new file mode 100644 index 0000000000000..f99485e790011 --- /dev/null +++ b/src/vs/platform/secrets/common/sharedKeychainService.ts @@ -0,0 +1,25 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { createDecorator } from '../../instantiation/common/instantiation.js'; + +/** + * Provides shared keychain access between Code and the embedded Agents app + * via a macOS keychain access group. On non-macOS platforms the implementation + * is a no-op (returns undefined/empty for all operations). + */ +export const ISharedKeychainService = createDecorator('sharedKeychainService'); + +export interface ISharedKeychainService { + readonly _serviceBrand: undefined; + get(key: string): Promise; + set(key: string, value: string): Promise; + delete(key: string): Promise; + keys(): Promise; +} + +export const ISharedKeychainMainService = createDecorator('sharedKeychainMainService'); + +export interface ISharedKeychainMainService extends ISharedKeychainService { } diff --git a/src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts b/src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts new file mode 100644 index 0000000000000..ba68f7aa3da67 --- /dev/null +++ b/src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts @@ -0,0 +1,93 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { isMacintosh } from '../../../base/common/platform.js'; +import { ISharedKeychainMainService } from '../common/sharedKeychainService.js'; +import { ILogService } from '../../log/common/log.js'; +import { IProductService } from '../../product/common/productService.js'; + +type KeychainModule = typeof import('@vscode/macos-keychain'); + +export class SharedKeychainMainService implements ISharedKeychainMainService { + declare readonly _serviceBrand: undefined; + + private _modulePromise: Promise | undefined; + private readonly serviceName: string; + private readonly enabled: boolean; + + constructor( + @IProductService productService: IProductService, + @ILogService private readonly logService: ILogService, + ) { + this.enabled = isMacintosh && !!productService.darwinSharedKeychainServiceName; + this.serviceName = productService.darwinSharedKeychainServiceName ?? ''; + } + + private getModule(): Promise { + if (!this._modulePromise) { + this._modulePromise = import('@vscode/macos-keychain'); + } + return this._modulePromise; + } + + async get(key: string): Promise { + if (!this.enabled) { + return undefined; + } + try { + const mod = await this.getModule(); + const value = mod.keychainGet(this.serviceName, key); + this.logService.trace('[SharedKeychainMainService] get:', key, value !== undefined ? '(found)' : '(not found)'); + return value; + } catch (err) { + this.logService.error('[SharedKeychainMainService] get failed:', key, err); + return undefined; + } + } + + async set(key: string, value: string): Promise { + if (!this.enabled) { + return; + } + try { + const mod = await this.getModule(); + mod.keychainSet(this.serviceName, key, value); + this.logService.trace('[SharedKeychainMainService] set:', key); + } catch (err) { + this.logService.error('[SharedKeychainMainService] set failed:', key, err); + throw err; + } + } + + async delete(key: string): Promise { + if (!this.enabled) { + return false; + } + try { + const mod = await this.getModule(); + const deleted = mod.keychainDelete(this.serviceName, key); + this.logService.trace('[SharedKeychainMainService] delete:', key, deleted ? '(deleted)' : '(not found)'); + return deleted; + } catch (err) { + this.logService.error('[SharedKeychainMainService] delete failed:', key, err); + return false; + } + } + + async keys(): Promise { + if (!this.enabled) { + return []; + } + try { + const mod = await this.getModule(); + const result = mod.keychainList(this.serviceName); + this.logService.trace('[SharedKeychainMainService] keys: found', result.length, 'entries'); + return result; + } catch (err) { + this.logService.error('[SharedKeychainMainService] keys failed:', err); + return []; + } + } +} diff --git a/src/vs/sessions/sessions.desktop.main.ts b/src/vs/sessions/sessions.desktop.main.ts index ea4d6930de8a4..2b24603fa6bba 100644 --- a/src/vs/sessions/sessions.desktop.main.ts +++ b/src/vs/sessions/sessions.desktop.main.ts @@ -59,6 +59,7 @@ import '../workbench/services/encryption/electron-browser/encryptionService.js'; import '../workbench/services/imageResize/electron-browser/imageResizeService.js'; import '../workbench/services/browserElements/electron-browser/browserElementsService.js'; import '../workbench/services/secrets/electron-browser/secretStorageService.js'; +import '../workbench/services/secrets/electron-browser/sharedKeychainService.js'; import '../workbench/services/localization/electron-browser/languagePackService.js'; import '../workbench/services/telemetry/electron-browser/telemetryService.js'; import '../workbench/services/extensions/electron-browser/extensionHostStarter.js'; diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index 228328d59bb1a..5595d9cae1a92 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -15,7 +15,8 @@ import { ILogService } from '../../../../platform/log/common/log.js'; import { INotificationService, IPromptChoice } from '../../../../platform/notification/common/notification.js'; import { IOpenerService } from '../../../../platform/opener/common/opener.js'; import { BaseSecretStorageService, ISecretStorageService } from '../../../../platform/secrets/common/secrets.js'; -import { IStorageService } from '../../../../platform/storage/common/storage.js'; +import { ISharedKeychainService } from '../../../../platform/secrets/common/sharedKeychainService.js'; +import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; import { IJSONEditingService } from '../../configuration/common/jsonEditing.js'; export class NativeSecretStorageService extends BaseSecretStorageService { @@ -26,6 +27,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { @IOpenerService private readonly _openerService: IOpenerService, @IJSONEditingService private readonly _jsonEditingService: IJSONEditingService, @INativeEnvironmentService private readonly _environmentService: INativeEnvironmentService, + @ISharedKeychainService private readonly _sharedKeychainService: ISharedKeychainService, @IStorageService storageService: IStorageService, @IEncryptionService encryptionService: IEncryptionService, @ILogService logService: ILogService @@ -38,6 +40,21 @@ export class NativeSecretStorageService extends BaseSecretStorageService { ); } + override get(key: string): Promise { + return this._sequencer.queue(key, async () => { + debugger; + if (this.type === 'persisted') { + // Try shared keychain first (no-op on non-macOS) + const value = await this._sharedKeychainService.get(key); + if (value !== undefined) { + return value; + } + } + // Fall back to old safeStorage+SQLite pipeline + return this._doGet(key); + }); + } + override set(key: string, value: string): Promise { this._sequencer.queue(key, async () => { await this.resolvedStorageService; @@ -46,10 +63,42 @@ export class NativeSecretStorageService extends BaseSecretStorageService { this._logService.trace('[NativeSecretStorageService] Notifying user that secrets are not being stored on disk.'); await this.notifyOfNoEncryptionOnce(); } + }); + return this._sequencer.queue(key, async () => { + debugger; + if (this.type === 'persisted') { + // Write to shared keychain (no-op on non-macOS) + await this._sharedKeychainService.set(key, value); + } + // Also write to legacy pipeline + await this._doSet(key, value); + }); + } + override delete(key: string): Promise { + return this._sequencer.queue(key, async () => { + debugger; + if (this.type === 'persisted') { + // Delete from shared keychain (no-op on non-macOS) + await this._sharedKeychainService.delete(key); + } + // Delete from legacy pipeline + await this._doDelete(key); }); + } - return super.set(key, value); + override keys(): Promise { + return this._sequencer.queue('__keys__', async () => { + debugger; + if (this.type === 'persisted') { + // Merge keys from both sources (shared returns [] on non-macOS) + const sharedKeys = await this._sharedKeychainService.keys(); + const legacyKeys = await this._doGetKeys(); + const merged = new Set([...sharedKeys, ...legacyKeys]); + return [...merged]; + } + return this._doGetKeys(); + }); } private notifyOfNoEncryptionOnce = createSingleCallFunction(() => this.notifyOfNoEncryption()); diff --git a/src/vs/workbench/services/secrets/electron-browser/sharedKeychainService.ts b/src/vs/workbench/services/secrets/electron-browser/sharedKeychainService.ts new file mode 100644 index 0000000000000..7930df4d69341 --- /dev/null +++ b/src/vs/workbench/services/secrets/electron-browser/sharedKeychainService.ts @@ -0,0 +1,9 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import { registerMainProcessRemoteService } from '../../../../platform/ipc/electron-browser/services.js'; +import { ISharedKeychainService } from '../../../../platform/secrets/common/sharedKeychainService.js'; + +registerMainProcessRemoteService(ISharedKeychainService, 'sharedKeychain'); diff --git a/src/vs/workbench/workbench.desktop.main.ts b/src/vs/workbench/workbench.desktop.main.ts index 03a9d807a0667..93cdd143f8393 100644 --- a/src/vs/workbench/workbench.desktop.main.ts +++ b/src/vs/workbench/workbench.desktop.main.ts @@ -60,6 +60,7 @@ import './services/encryption/electron-browser/encryptionService.js'; import './services/imageResize/electron-browser/imageResizeService.js'; import './services/browserElements/electron-browser/browserElementsService.js'; import './services/secrets/electron-browser/secretStorageService.js'; +import './services/secrets/electron-browser/sharedKeychainService.js'; import './services/localization/electron-browser/languagePackService.js'; import './services/telemetry/electron-browser/telemetryService.js'; import './services/extensions/electron-browser/extensionHostStarter.js'; From a76b95248e53a8c1d5577afd2851345383b0cb8b Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 10 Apr 2026 13:12:19 +0200 Subject: [PATCH 02/25] Address review feedback --- .../electron-main/sharedKeychainMainService.ts | 1 - .../electron-browser/secretStorageService.ts | 16 ++++++---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts b/src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts index ba68f7aa3da67..3ebeb172d97e2 100644 --- a/src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts +++ b/src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts @@ -57,7 +57,6 @@ export class SharedKeychainMainService implements ISharedKeychainMainService { this.logService.trace('[SharedKeychainMainService] set:', key); } catch (err) { this.logService.error('[SharedKeychainMainService] set failed:', key, err); - throw err; } } diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index 5595d9cae1a92..3f2e02b4920e3 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -16,7 +16,7 @@ import { INotificationService, IPromptChoice } from '../../../../platform/notifi import { IOpenerService } from '../../../../platform/opener/common/opener.js'; import { BaseSecretStorageService, ISecretStorageService } from '../../../../platform/secrets/common/secrets.js'; import { ISharedKeychainService } from '../../../../platform/secrets/common/sharedKeychainService.js'; -import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; +import { IStorageService } from '../../../../platform/storage/common/storage.js'; import { IJSONEditingService } from '../../configuration/common/jsonEditing.js'; export class NativeSecretStorageService extends BaseSecretStorageService { @@ -42,8 +42,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override get(key: string): Promise { return this._sequencer.queue(key, async () => { - debugger; - if (this.type === 'persisted') { + if (this.type !== 'in-memory') { // Try shared keychain first (no-op on non-macOS) const value = await this._sharedKeychainService.get(key); if (value !== undefined) { @@ -65,8 +64,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { } }); return this._sequencer.queue(key, async () => { - debugger; - if (this.type === 'persisted') { + if (this.type !== 'in-memory') { // Write to shared keychain (no-op on non-macOS) await this._sharedKeychainService.set(key, value); } @@ -77,8 +75,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override delete(key: string): Promise { return this._sequencer.queue(key, async () => { - debugger; - if (this.type === 'persisted') { + if (this.type !== 'in-memory') { // Delete from shared keychain (no-op on non-macOS) await this._sharedKeychainService.delete(key); } @@ -87,10 +84,9 @@ export class NativeSecretStorageService extends BaseSecretStorageService { }); } - override keys(): Promise { + override async keys(): Promise { return this._sequencer.queue('__keys__', async () => { - debugger; - if (this.type === 'persisted') { + if (this.type !== 'in-memory') { // Merge keys from both sources (shared returns [] on non-macOS) const sharedKeys = await this._sharedKeychainService.keys(); const legacyKeys = await this._doGetKeys(); From d0433224a85f44c58cc8631b69276e791d24265c Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 10 Apr 2026 15:24:20 +0200 Subject: [PATCH 03/25] Add one-time migration of legacy secrets to shared keychain On first secret operation, migrate all existing secrets from the legacy safeStorage+SQLite pipeline into the shared macOS Keychain. This ensures the Agents app can read secrets that were stored before the shared keychain was introduced. - Migration is lazy (triggered on first get/set/delete/keys) - Guarded by a 'sharedKeychain.migrationDone' storage flag - Idempotent: keychain writes are upserts, re-running is safe - Best-effort per key: individual failures don't block the rest - Skipped when type is 'in-memory' - Also: make set() in SharedKeychainMainService best-effort (log, don't throw) --- .../electron-browser/secretStorageService.ts | 53 ++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index 3f2e02b4920e3..5509c13b3f17b 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -16,11 +16,15 @@ import { INotificationService, IPromptChoice } from '../../../../platform/notifi import { IOpenerService } from '../../../../platform/opener/common/opener.js'; import { BaseSecretStorageService, ISecretStorageService } from '../../../../platform/secrets/common/secrets.js'; import { ISharedKeychainService } from '../../../../platform/secrets/common/sharedKeychainService.js'; -import { IStorageService } from '../../../../platform/storage/common/storage.js'; +import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; import { IJSONEditingService } from '../../configuration/common/jsonEditing.js'; +const MIGRATION_STORAGE_KEY = 'sharedKeychain.migrationDone'; + export class NativeSecretStorageService extends BaseSecretStorageService { + private _migrationPromise: Promise | undefined; + constructor( @INotificationService private readonly _notificationService: INotificationService, @IDialogService private readonly _dialogService: IDialogService, @@ -40,9 +44,53 @@ export class NativeSecretStorageService extends BaseSecretStorageService { ); } + /** + * One-time migration: copies all secrets from the legacy safeStorage+SQLite + * pipeline into the shared keychain. Idempotent — guarded by a storage flag + * and keychain writes are upserts. + */ + private _ensureMigration(): Promise { + if (!this._migrationPromise) { + this._migrationPromise = this._doMigration(); + } + return this._migrationPromise; + } + + private async _doMigration(): Promise { + if (this.type === 'in-memory') { + return; + } + + const storageService = await this.resolvedStorageService; + if (storageService.get(MIGRATION_STORAGE_KEY, StorageScope.APPLICATION) === '1') { + this._logService.trace('[NativeSecretStorageService] shared keychain migration already done'); + return; + } + + this._logService.trace('[NativeSecretStorageService] starting shared keychain migration'); + + const legacyKeys = await this._doGetKeys(); + let migrated = 0; + for (const key of legacyKeys) { + try { + const value = await this._doGet(key); + if (value !== undefined) { + await this._sharedKeychainService.set(key, value); + migrated++; + } + } catch (err) { + this._logService.error('[NativeSecretStorageService] migration failed for key:', key, err); + } + } + + storageService.store(MIGRATION_STORAGE_KEY, '1', StorageScope.APPLICATION, StorageTarget.MACHINE); + this._logService.trace(`[NativeSecretStorageService] shared keychain migration complete: ${migrated}/${legacyKeys.length} secrets migrated`); + } + override get(key: string): Promise { return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory') { + await this._ensureMigration(); // Try shared keychain first (no-op on non-macOS) const value = await this._sharedKeychainService.get(key); if (value !== undefined) { @@ -65,6 +113,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { }); return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory') { + await this._ensureMigration(); // Write to shared keychain (no-op on non-macOS) await this._sharedKeychainService.set(key, value); } @@ -76,6 +125,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override delete(key: string): Promise { return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory') { + await this._ensureMigration(); // Delete from shared keychain (no-op on non-macOS) await this._sharedKeychainService.delete(key); } @@ -87,6 +137,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override async keys(): Promise { return this._sequencer.queue('__keys__', async () => { if (this.type !== 'in-memory') { + await this._ensureMigration(); // Merge keys from both sources (shared returns [] on non-macOS) const sharedKeys = await this._sharedKeychainService.keys(); const legacyKeys = await this._doGetKeys(); From 3beb9b9f53d45de8e08f3acfb14e167365affd46 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 10 Apr 2026 16:35:38 +0200 Subject: [PATCH 04/25] update the current implementation --- secrets-sharing-impl.md | 294 +++++++++++++++++++--------------------- 1 file changed, 143 insertions(+), 151 deletions(-) diff --git a/secrets-sharing-impl.md b/secrets-sharing-impl.md index 5e1f848a1816a..74aca2a666e5e 100644 --- a/secrets-sharing-impl.md +++ b/secrets-sharing-impl.md @@ -2,215 +2,207 @@ **Spec**: `secrets-sharing-spec.md` (in this repo) **Issue**: https://github.com/microsoft/vscode/issues/308028 -**PR**: https://github.com/microsoft/vscode-macos-keychain/pull/1 +**VS Code PR**: https://github.com/microsoft/vscode/pull/308990 **Date started**: 9 April 2026 +## Current status: Phase 1 (macOS) — feature-complete, testing in progress + +All Phase 1 steps from the spec are implemented. The end-to-end flow (Code writes secrets → Agents reads them) works locally. + ## What's been done -### Phase 1, Step 1: macOS native Node addon (`@vscode/macos-keychain`) +### Step 1: macOS native Node addon (`@vscode/macos-keychain`) -**Location**: `/Users/alex/src/vscode-macos-keychain/` (separate repo, https://github.com/microsoft/vscode-macos-keychain) -**Branch**: `alexdima/keychain-addon` -**Status**: Addon built, compiles cleanly, 11/11 tests passing. Draft PR open. +**Repo**: https://github.com/microsoft/vscode-macos-keychain +**PR #1** (merged): Initial addon with access group support +**PR #2** (open): Auto-detect access group from entitlements +**PR #3** (open): Fix build on non-macOS CI -The addon wraps the macOS Security framework (`SecItemAdd`, `SecItemCopyMatching`, `SecItemUpdate`, `SecItemDelete`) with `kSecAttrAccessGroup` support for cross-app keychain sharing. +The addon wraps the macOS Security framework (`SecItemAdd`, `SecItemCopyMatching`, `SecItemUpdate`, `SecItemDelete`). -#### Files created +#### API (after PR #2) -``` -vscode-macos-keychain/ - package.json # @vscode/macos-keychain, deps: bindings + node-addon-api, os: ["darwin"] - package-lock.json - binding.gyp # node-gyp config, links Security + CoreFoundation - index.js # JS wrapper using `bindings` package (with platform guard) - index.d.ts # TypeScript type definitions - src/ - main.cc # Node-API entry point (Napi:: C++ wrapper) - keychain.h # Header: CFRef RAII wrapper + function declarations - keychain.cc # Core implementation: buildBaseQuery, set/get/delete/list - test/ - test.js # mocha test suite (11 tests) - azure-pipelines.yml # Azure DevOps CI/CD + npm publishing pipeline - .github/workflows/ci.yml # GitHub Actions CI (macOS, Node 22.x) - .npmignore # Excludes CI, meta files from published package - .vscode/settings.json # Branch protection config - LICENSE # MIT (from upstream repo template) - README.md # Project docs with API, usage, entitlements, testing - CODE_OF_CONDUCT.md # From upstream repo template - SECURITY.md # From upstream repo template - SUPPORT.md # From upstream repo template - .gitignore # Minimal: /node_modules, /build (matches policy-watcher) +```typescript +keychainSet(service: string, account: string, value: string): void +keychainGet(service: string, account: string): string | undefined +keychainDelete(service: string, account: string): boolean +keychainList(service: string): string[] ``` -#### API +The `accessGroup` parameter was removed from the public API. Instead, `keychainInit()` (called automatically at module load) reads the process's `keychain-access-groups` entitlement via `SecTaskCopyValueForEntitlement` and caches the first access group found. If no entitlement exists (unsigned/dev builds), items go to the app's default keychain. -```typescript -keychainSet(service: string, account: string, value: string, accessGroup: string): void -keychainGet(service: string, account: string, accessGroup: string): string | undefined -keychainDelete(service: string, account: string, accessGroup: string): boolean -keychainList(service: string, accessGroup: string): string[] +### Step 2: Entitlements + +Added `keychain-access-groups` to `build/azure-pipelines/darwin/app-entitlements.plist`: + +```xml +keychain-access-groups + + $(TeamIdentifierPrefix)com.microsoft.vscode.shared-secrets + ``` -All string arguments reject embedded NUL bytes. This is intentional: the native CoreFoundation string conversion used by the addon treats NUL as a terminator, so allowing embedded NUL would let distinct logical keys alias the same Keychain item. +Both Code.app and its embedded Agents.app use the same entitlements file (via `getEntitlementsForFile()` in `build/darwin/sign.ts`), so both get the shared access group. -## Decisions and rationale +### Step 3: ISharedKeychainService -### 1. Template: `@vscode/policy-watcher` (not `native-keymap`) +Follows the standard Electron main process IPC service pattern (like `IEncryptionService`): -We surveyed ALL native Node addons used by VS Code: +| Layer | File | Role | +|-------|------|------| +| Common interface | `src/vs/platform/secrets/common/sharedKeychainService.ts` | `ISharedKeychainService` + `ISharedKeychainMainService` decorators and interfaces | +| Main process impl | `src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts` | `SharedKeychainMainService` — wraps native addon, no-op on non-macOS | +| Main process registration | `src/vs/code/electron-main/app.ts` | `services.set(ISharedKeychainMainService, ...)` + `ProxyChannel.fromService(...)` + `registerChannel('sharedKeychain', ...)` | +| Renderer proxy | `src/vs/workbench/services/secrets/electron-browser/sharedKeychainService.ts` | `registerMainProcessRemoteService(ISharedKeychainService, 'sharedKeychain')` | -| Package | macOS native? | API style | Loader | -|---------|:---:|---|---| -| `native-keymap` (`node-native-keymap`) | Yes (.mm) | Raw `napi_*` C API | Manual `require('./build/Release/...')` | -| `@vscode/policy-watcher` | Yes (C++) | `node-addon-api` (C++ NAPI wrapper) | `bindings` package | -| `@vscode/spdlog` | Yes (cross-platform C++) | `node-addon-api` | `bindings` | -| `@vscode/windows-mutex` | No (Windows) | `node-addon-api` | `bindings` | -| `@vscode/deviceid` | No (Windows) | Raw NAPI | Manual | -| `@vscode/windows-process-tree` | No (Windows) | — | — | -| `node-pty` | Yes | — | Too complex for reference | +**Design decisions**: +- Service is registered on all platforms; the main process implementation is a no-op on non-macOS (`enabled = isMacintosh && !!productService.darwinSharedKeychainServiceName`) +- `set()` is best-effort (logs error, does not throw) — callers also write to the legacy pipeline for rollback safety, so shared keychain failures should not break secret persistence +- `get()`/`delete()`/`keys()` are also best-effort (return `undefined`/`false`/`[]` on error) -**Decision**: Use `@vscode/policy-watcher` as the primary template because: -- It uses the modern `node-addon-api` C++ wrapper (cleaner than raw `napi_*`) -- It has macOS-specific native code in `src/macos/` -- It uses the `bindings` package for loading (standard pattern) -- It's a Microsoft project with the same license/header conventions -- `native-keymap` is older: raw C NAPI, manual `require('./build/Release/...')` loading, Objective-C++ `.mm` files +### Step 4: NativeSecretStorageService changes -### 2. `node-addon-api` version: `^8.2.0` (not `7.1.0`) +`src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts`: -VS Code's `package.json` pins `node-addon-api` at `7.1.0`, but `@vscode/policy-watcher` (the newest addon) uses `^8.2.0`. We followed the newer addon's lead. This may need alignment when the addon is added to VS Code's dependencies. +**Base class refactoring**: `BaseSecretStorageService` now exposes protected `_doGet`/`_doSet`/`_doDelete`/`_doGetKeys` methods that perform the actual safeStorage+SQLite operations without going through the sequencer. This allows subclasses to call them from within their own sequencer-queued tasks without deadlocking (since `SequencerByKey` would deadlock if the same key is queued from within a queued task). -### 3. `Napi::Boolean` must be fully qualified +**NativeSecretStorageService overrides** (all guarded by `this.type !== 'in-memory'`): +- `get()`: tries shared keychain first, falls back to `_doGet()` (legacy pipeline) +- `set()`: writes to shared keychain, then `_doSet()` (dual-write for rollback safety) +- `delete()`: deletes from shared keychain, then `_doDelete()` +- `keys()`: merges results from shared keychain and `_doGetKeys()` -**Problem discovered during build**: `return Boolean::New(env, deleted)` caused a compilation error: +### Step 5: Migration -``` -error: reference to 'Boolean' is ambiguous -``` +One-time, lazy migration runs on the first secret operation: -macOS `MacTypes.h` defines `typedef unsigned char Boolean;` which conflicts with `Napi::Boolean`. The file has `using namespace Napi;` so both are in scope. +1. `_ensureMigration()` is called at the start of every `get`/`set`/`delete`/`keys` (when `type !== 'in-memory'`) +2. Reads all `secret://`-prefixed keys from SQLite via `_doGetKeys()` +3. Decrypts each with safeStorage via `_doGet()` +4. Writes plaintext to shared keychain via `_sharedKeychainService.set()` +5. Stores `sharedKeychain.migrationDone = '1'` in `StorageScope.APPLICATION` +6. Subsequent calls skip migration (cached promise + storage flag) -**Fix**: Use `Napi::Boolean::New(env, deleted)` instead of `Boolean::New(env, deleted)`. This only affects `Boolean` — `String`, `Array`, `Value`, etc. don't have conflicts with macOS system headers. +**Properties**: +- Idempotent: keychain writes are upserts, storage flag prevents re-runs across sessions +- Best-effort per key: individual failures don't block the rest +- Safe with multiple windows: concurrent migrations do redundant but harmless work (all write the same values) +- Skipped for in-memory mode -### 4. `kSecUseDataProtectionKeychain` availability warning +### Step 6: Agents app wiring -**Problem discovered during build**: The compiler warns: +`src/vs/sessions/sessions.desktop.main.ts` imports both: +- `../workbench/services/secrets/electron-browser/secretStorageService.js` (NativeSecretStorageService) +- `../workbench/services/secrets/electron-browser/sharedKeychainService.js` (IPC proxy) -``` -'kSecUseDataProtectionKeychain' is only available on macOS 10.15 or newer [-Wunguarded-availability-new] -``` +The Agents app reads from the shared keychain via the same `NativeSecretStorageService` overrides. No separate migration needed — it reads whatever Code wrote. + +### Product configuration + +**`darwinSharedKeychainServiceName`** — the `kSecAttrService` value that groups keychain items. Per-flavor to isolate secrets between Stable/Insiders/Exploration: -This is because node-gyp defaults `MACOSX_DEPLOYMENT_TARGET` to `10.7`, and `kSecUseDataProtectionKeychain` was introduced in 10.15. +| Flavor | Value | +|--------|-------| +| Code OSS | `com.visualstudio.code.oss.shared-secrets` (set in `product.json`) | +| Code Stable | Set in internal product.json (e.g. `com.microsoft.vscode.shared-secrets`) | +| Code Insiders | Set in internal product.json (e.g. `com.microsoft.vscode-insiders.shared-secrets`) | -**Attempted fixes**: -1. `"MACOSX_DEPLOYMENT_TARGET": "10.15"` in xcode_settings — **did not work** because node-gyp uses `make` on macOS (not Xcode), so xcode_settings for deployment target are ignored by the makefile generator. -2. `"CLANG_WARN_UNGUARDED_AVAILABILITY": "NO"` in xcode_settings — **did not work** for the same reason. -3. `"-Wno-unguarded-availability-new"` in `WARNING_CFLAGS` (xcode_settings) — **worked**. The `WARNING_CFLAGS` xcode_setting DOES get passed through to the compiler via the makefile. +This field is at the **top level** of product.json, NOT in the `embedded` section — so both Code and its embedded Agents app within the same flavor get the same value. The `embedded` overlay (in `build/gulpfile.vscode.ts`) only copies fields listed in `IEmbeddedProductConfiguration`. -**Final fix**: Added `-Wno-unguarded-availability-new` to both `cflags` (for GCC/Clang direct builds) and `WARNING_CFLAGS` (for xcode_settings pass-through). Also kept `MACOSX_DEPLOYMENT_TARGET: "10.15"` for documentation purposes even though node-gyp overrides it. +**`darwinKeychainAccessGroup`** — removed from product.json. The native addon auto-detects it from the process's entitlements at module load time. This avoids keeping the team ID prefix in sync between the entitlements plist and product.json. -VS Code already requires macOS 10.15+, so the availability guard is unnecessary. +### Other files changed -### 5. Access group is optional (empty string = skip) +| File | Change | +|------|--------| +| `package.json` | Added `@vscode/macos-keychain` to `optionalDependencies` (macOS-only) | +| `build/.moduleignore` | Added entries for `@vscode/macos-keychain` (keep only `.node` binary) | +| `src/typings/macos-keychain.d.ts` | Type declarations for cross-platform compilation | +| `src/vs/base/common/product.ts` | Added `darwinSharedKeychainServiceName` to `IProductConfiguration` | +| `src/vs/workbench/workbench.desktop.main.ts` | Import shared keychain service registration | +| `src/vs/sessions/sessions.desktop.main.ts` | Import shared keychain service registration | -**Problem discovered during testing**: All keychain operations fail with `-34018` (`errSecMissingEntitlement`) when `kSecUseDataProtectionKeychain = true` is set but the app isn't signed with the `keychain-access-groups` entitlement. This means you **cannot test the addon locally** with a real access group using plain `node`. +## Decisions and rationale -**Solution**: When `accessGroup` is an **empty string**, `buildBaseQuery()` skips both `kSecAttrAccessGroup` and `kSecUseDataProtectionKeychain`. Items go to the app's default keychain, which works without entitlements. This enables local development and CI testing. +### 1. Template: `@vscode/policy-watcher` (not `native-keymap`) -When `accessGroup` is non-empty (production), both attributes are set, requiring proper entitlements. This is the intended code path inside signed VS Code/Agents builds. +Used `@vscode/policy-watcher` as the template for the native addon because it uses modern `node-addon-api` (C++ NAPI wrapper), `bindings` package for loading, and has macOS-specific native code. -### 6. Set is upsert (add-then-update pattern) +### 2. Access group auto-detection (PR #2) -`keychainSet` first calls `SecItemAdd`. If that returns `errSecDuplicateItem`, it falls back to building a new search query + `SecItemUpdate`. This two-step pattern is the standard approach for macOS Keychain — there is no "upsert" API. +Initially the `accessGroup` was a JS parameter passed through to the native code. This required keeping the team ID prefix (e.g. `UBF8T346G9.com.microsoft.vscode.shared-secrets`) in product.json, which was error-prone and would diverge from the entitlements plist. -Note: The update path builds a **new** search query (without `kSecValueData`) and a separate attributes dict (with only `kSecValueData`). This is required by `SecItemUpdate` — it does not accept `kSecValueData` in the query dictionary. +**Solution**: The native addon calls `SecTaskCopyValueForEntitlement` at module init to read the `keychain-access-groups` entitlement from its own process. This makes the access group an implementation detail — JS never needs to know the team ID prefix. -### 7. `CFRef` RAII wrapper for CoreFoundation objects +### 3. IPC service pattern (not direct import) -CoreFoundation objects must be `CFRelease`d. Rather than tracking releases manually (error-prone, especially with early returns and exceptions), we use a `CFRef` template class that calls `CFRelease` in its destructor. It supports move semantics. When setting `CFRef`-managed objects into CF dictionaries, we use `.get()` so the RAII wrapper's destructor balances the create, while `CFDictionarySetValue` (with `kCFTypeDictionaryValueCallBacks`) retains its own +1 reference. +The native addon runs in the **main process** (via `SharedKeychainMainService`), exposed to renderer windows via `ProxyChannel`/`registerMainProcessRemoteService`. This follows the established pattern for native services (like `IEncryptionService`). An earlier attempt to load the addon directly in the `electron-browser` layer failed layering checks — the `electron-browser` tsconfig doesn't include `node/` files. -**Note**: An earlier version used `.release()` to transfer ownership into dictionaries, which caused a refcount leak — `CFDictionarySetValue` retains the value, but `.release()` also gave up RAII ownership, leaving a dangling +1. Fixed by switching to `.get()`. +### 4. Dual-write for rollback safety -### 8. Error messages include human-readable text and OSStatus code (no account names) +`set()` writes to both the shared keychain AND the legacy safeStorage+SQLite pipeline. This means if we discover a critical issue with the shared keychain, we can remove the new code path and all secrets are still in the old storage. The legacy pipeline can be removed in a future release once the shared keychain is proven stable. -All error paths use `SecCopyErrorMessageString` to get a human-readable description and also include the numeric OSStatus code. Example: `"Keychain set failed: A required entitlement isn't present. (-34018)"`. Account and service names are intentionally **omitted** from error messages to prevent them from leaking into telemetry or log files. +### 5. Best-effort shared keychain operations -### 9. Secrets stored as plaintext in Keychain (not encrypted separately) +All `SharedKeychainMainService` methods catch errors and return safe defaults (`undefined`/`false`/`[]`). This prevents shared keychain failures (e.g. missing entitlements in dev builds, addon load failures on non-macOS CI) from breaking existing secret functionality. -Per the spec: macOS Keychain `kSecClassGenericPassword` does NOT have the blob size limitations that Windows CredMan has. The keychain itself IS the secure storage — no need for an additional encryption layer (unlike the current safeStorage + SQLite approach). Values are stored as `kSecValueData` (`CFData` from UTF-8 bytes) directly. +### 6. `CFRef` RAII wrapper for CoreFoundation objects -### 10. Test cleanup uses beforeEach/afterEach with try-catch +CoreFoundation objects must be `CFRelease`d. The addon uses a `CFRef` template class for RAII. When setting into CF dictionaries, uses `.get()` (not `.release()`) so the RAII wrapper's destructor balances the create while `CFDictionarySetValue` retains its own +1 reference. An earlier version using `.release()` caused refcount leaks. -Tests clean up keychain items in both `beforeEach` and `afterEach` to ensure idempotency even if a previous test run crashed. The `try-catch` handles the case where items don't exist. +### 7. Security hardening in native addon -### 11. Platform guard in index.js + `"os": ["darwin"]` in package.json +- **Secret zeroing**: `secureClear()` uses a `volatile char*` to zero `std::string` buffers +- **Build flags**: `-D_FORTIFY_SOURCE=2`, `-Wformat`, `-Wformat-security`, `-fstack-protector-strong` +- **Error sanitization**: account/service names omitted from error messages +- **Input bounds**: values capped at 100 KB, strings at 1 KB +- **NUL byte rejection**: all string arguments validated -Added `if (process.platform !== 'darwin') throw ...` at the top of `index.js` for a clear error message at import time. Also added `"os": ["darwin"]` to `package.json` so `npm install` fails fast on non-macOS. The existing VS Code addons don't use either pattern, but this module is macOS-only by design so both are appropriate. +### 8. Platform guard in SharedKeychainMainService -### 12. Null check for updateAttrs dictionary +The service checks `isMacintosh && !!productService.darwinSharedKeychainServiceName` at construction time. If either is false, all methods are no-ops. This means: +- On Windows/Linux: no keychain operations attempted +- On macOS without `darwinSharedKeychainServiceName` in product.json: no keychain operations (but this shouldn't happen in practice since Code OSS sets it) -Added a null check after `CFDictionaryCreateMutable` for the `updateAttrs` dict in the update path of `keychainSet`, consistent with the existing check in `buildBaseQuery`. +### 9. `type !== 'in-memory'` guard in NativeSecretStorageService -### 13. Security hardening (from security audit) +Shared keychain operations are skipped when `this.type === 'in-memory'` (encryption unavailable). The type can be `'unknown'` during initialization before encryption availability is determined — shared keychain operations proceed for both `'persisted'` and `'unknown'` states. -A dedicated security audit identified and fixed the following: +## Testing strategy -- **CF ownership fix**: Fixed CoreFoundation reference counting in `buildBaseQuery` — changed `.release()` to `.get()` for values set into dictionaries, since `CFDictionarySetValue` retains. The original `.release()` left a dangling +1 refcount on every keychain call. -- **Secret zeroing**: A `secureClear()` utility uses a `volatile char*` pointer to zero `std::string` buffers containing secrets immediately after use, preventing the compiler from optimizing away the zeroing. Applied to the `value` parameter in `keychainSet` and the return value in `keychainGet`. -- **Build hardening flags**: Added `-D_FORTIFY_SOURCE=2` (runtime buffer overflow checks) and `-Wformat -Wformat-security` (format string vulnerability warnings) alongside the existing `-fstack-protector-strong`. -- **Error message sanitization**: Removed account/service names from error messages to avoid leaking them into telemetry or log files. The caller already knows these values. -- **Input length bounds**: Values are capped at 100 KB, and service/account/accessGroup strings at 1 KB. Apple does not document hard keychain item size limits, but the Data Protection keychain (SQLite-backed) is known to work reliably up to ~100 KB. These bounds provide predictable error messages instead of opaque OS-level failures. -- **NUL byte rejection**: All string arguments are validated to reject embedded NUL bytes, which could cause silent truncation when passed to CoreFoundation `CFStringCreateWithCString`. +### Unit tests +- Run existing `BaseSecretStorageService` tests to verify the `_doGet`/`_doSet`/`_doDelete`/`_doGetKeys` refactoring didn't break anything: + ```bash + ./scripts/test.sh --run src/vs/platform/secrets/test/common/secrets.test.ts + ``` -### 14. CI/CD and project meta files +### Compilation check +```bash +npm run compile-check-ts-native +``` -`azure-pipelines.yml` follows the `@vscode/policy-watcher` pattern: uses `microsoft/vscode-engineering` pipeline templates for npm package build/test/publish. Only tests on macOS (unlike cross-platform addons). GitHub Actions CI (`.github/workflows/ci.yml`) runs on `macos-latest` with Node 22.x. `.npmignore` excludes CI files, meta docs, and build artifacts from the published package. +### Manual E2E flow +1. Launch Code OSS: `./scripts/code.sh` +2. Sign in to GitHub/Microsoft +3. Verify Keychain Access shows entries under `com.visualstudio.code.oss.shared-secrets` +4. Launch Agents: `./scripts/code.sh --agents --user-data-dir=$HOME/.vscode-oss-sessions-dev --extensions-dir=$HOME/.vscode-oss-sessions-dev/extensions` +5. Verify the Agents app is signed in without re-authentication -### 15. Git rebase gotcha: `--ours`/`--theirs` are swapped +### Edge cases +- Cold start with no prior secrets (no migration needed) +- Restart after migration (flag set, migration skips) +- Delete a secret in Code → verify it's gone from both keychain and SQLite +- Multiple windows restoring simultaneously (benign concurrent migration) -During `git rebase origin/main`, conflict resolution with `git checkout --ours README.md` took the **upstream** version, not ours. This is because rebase replays our commits onto the upstream, making the upstream `HEAD` the "ours" and our commit the "theirs". The README was silently lost and discovered later. Lesson: during rebase conflicts, use `--theirs` to keep your own changes. +## Open questions (from spec) -## Things NOT done yet (from the spec's Phase 1 plan) +1. ~~**Naming convention for keychain items**~~ → Resolved: `darwinSharedKeychainServiceName` in product.json, per-flavor +2. **Entitlement signing**: Does adding `keychain-access-groups` to `app-entitlements.plist` require CI/CD signing process changes? Needs verification with the build team. +3. **Notification/sync**: When Code writes a new secret, should the Agents app be notified in real-time? Currently read-on-demand (lazy). +4. **Scope**: Currently shares ALL secrets. May want to filter to auth-related only in the future. -### Step 2: Entitlements -Add `keychain-access-groups` entitlement to both Code and Agents app builds: -- Location: `build/darwin/` entitlements files -- Value: `$(TeamIdentifierPrefix)com.microsoft.vscode.shared-secrets` -- Both Code.app and the nested Agents .app need the same group - -### Step 3: `ISharedSecretStorageService` in VS Code -New service that: -- On write: stores in shared keychain via addon + optionally keeps old pipeline in sync -- On read: reads from shared keychain, falls back to old safeStorage+SQLite -- On delete: deletes from both -- Files involved: `src/vs/platform/secrets/`, `src/vs/workbench/services/secrets/` - -### Step 4: Migration logic -One-time migration in Code on startup: -- Read all `secret://`-prefixed keys from SQLite (IStorageService) -- Decrypt each with existing safeStorage (IEncryptionService) -- Write plaintext to shared keychain via addon -- Must be idempotent/re-entrant -- Store migration-complete flag - -### Step 5: Wire up the Agents app -The Agents app (`src/vs/sessions/sessions.desktop.main.ts`) should use the shared keychain for secrets. It reads from the shared keychain directly — no migration needed from the Agents side. - -### Step 6: Testing with real auth tokens -Test with large Microsoft account refresh tokens (the ones that exceed Windows CredMan's ~2.5KB limit). macOS Keychain should handle them fine but needs verification. - -## Open questions (from spec, still open) - -1. **Naming convention for keychain items**: What `kSecAttrService` value? Spec suggests `"com.microsoft.vscode.shared-secrets"` — needs to be stable across versions. -2. **Entitlement signing**: Does adding `keychain-access-groups` require CI/CD signing process changes? -3. **Notification/sync**: Should Code notify Agents in real-time when secrets change? Or read-on-demand? -4. **Scope**: Share ALL secrets or only auth-related ones? - -## Environment notes - -- Built and tested on macOS (Darwin 25.4.0, arm64) -- Node.js v22.22.1 -- node-gyp v11.2.0 (bundled with npm) -- `node-addon-api` v8.3.1 (resolved from `^8.2.0`) -- Compilation uses `make` (not Xcode), which affects how xcode_settings are applied -- GPG commit signing fails inside the VS Code agent sandbox (can't access `~/.gnupg` or gpg-agent) — must use unsandboxed execution for `git commit -S` and `git push` +## Phase 2 (Windows) and Phase 3 (Linux) — future work + +Not started. See spec for approach: +- **Windows**: Cross-read Code's SQLite + same DPAPI key (user-scoped, not app-scoped). Must avoid CredMan for secret values (2.5KB size limit). +- **Linux**: Similar to Windows — keyring services are user-scoped. From d41f5aff3c3324f4964c6368f1b6118877e14ad7 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 10 Apr 2026 18:30:14 +0200 Subject: [PATCH 05/25] restrict shared keychain to CROSS_APP_SHARED_SECRET_KEYS --- src/vs/platform/secrets/common/secrets.ts | 8 ++++ .../electron-browser/secretStorageService.ts | 37 ++++++++++--------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/vs/platform/secrets/common/secrets.ts b/src/vs/platform/secrets/common/secrets.ts index 6cccd13679fa9..e2f2ff82b32c4 100644 --- a/src/vs/platform/secrets/common/secrets.ts +++ b/src/vs/platform/secrets/common/secrets.ts @@ -27,6 +27,14 @@ export interface ISecretStorageService extends ISecretStorageProvider { readonly onDidChangeSecret: Event; } +/** + * Secret keys that are shared between Code and the Agents app + * via the macOS shared keychain. + */ +export const CROSS_APP_SHARED_SECRET_KEYS: readonly string[] = [ + '{"extensionId":"vscode.github-authentication","key":"github.auth"}', +]; + export class BaseSecretStorageService extends Disposable implements ISecretStorageService { declare readonly _serviceBrand: undefined; diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index 5509c13b3f17b..814a9dccc4b42 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -14,7 +14,7 @@ import { InstantiationType, registerSingleton } from '../../../../platform/insta import { ILogService } from '../../../../platform/log/common/log.js'; import { INotificationService, IPromptChoice } from '../../../../platform/notification/common/notification.js'; import { IOpenerService } from '../../../../platform/opener/common/opener.js'; -import { BaseSecretStorageService, ISecretStorageService } from '../../../../platform/secrets/common/secrets.js'; +import { BaseSecretStorageService, CROSS_APP_SHARED_SECRET_KEYS, ISecretStorageService } from '../../../../platform/secrets/common/secrets.js'; import { ISharedKeychainService } from '../../../../platform/secrets/common/sharedKeychainService.js'; import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; import { IJSONEditingService } from '../../configuration/common/jsonEditing.js'; @@ -69,27 +69,25 @@ export class NativeSecretStorageService extends BaseSecretStorageService { this._logService.trace('[NativeSecretStorageService] starting shared keychain migration'); - const legacyKeys = await this._doGetKeys(); - let migrated = 0; - for (const key of legacyKeys) { + for (const sharedKey of CROSS_APP_SHARED_SECRET_KEYS) { try { - const value = await this._doGet(key); + const value = await this._doGet(sharedKey); if (value !== undefined) { - await this._sharedKeychainService.set(key, value); - migrated++; + await this._sharedKeychainService.set(sharedKey, value); + this._logService.trace('[NativeSecretStorageService] shared keychain migration: migrated', sharedKey); } } catch (err) { - this._logService.error('[NativeSecretStorageService] migration failed for key:', key, err); + this._logService.error('[NativeSecretStorageService] migration failed for:', sharedKey, err); } } storageService.store(MIGRATION_STORAGE_KEY, '1', StorageScope.APPLICATION, StorageTarget.MACHINE); - this._logService.trace(`[NativeSecretStorageService] shared keychain migration complete: ${migrated}/${legacyKeys.length} secrets migrated`); + this._logService.trace('[NativeSecretStorageService] shared keychain migration complete'); } override get(key: string): Promise { return this._sequencer.queue(key, async () => { - if (this.type !== 'in-memory') { + if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { await this._ensureMigration(); // Try shared keychain first (no-op on non-macOS) const value = await this._sharedKeychainService.get(key); @@ -112,7 +110,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { } }); return this._sequencer.queue(key, async () => { - if (this.type !== 'in-memory') { + if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { await this._ensureMigration(); // Write to shared keychain (no-op on non-macOS) await this._sharedKeychainService.set(key, value); @@ -124,7 +122,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override delete(key: string): Promise { return this._sequencer.queue(key, async () => { - if (this.type !== 'in-memory') { + if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { await this._ensureMigration(); // Delete from shared keychain (no-op on non-macOS) await this._sharedKeychainService.delete(key); @@ -136,15 +134,18 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override async keys(): Promise { return this._sequencer.queue('__keys__', async () => { + const legacyKeys = await this._doGetKeys(); if (this.type !== 'in-memory') { await this._ensureMigration(); - // Merge keys from both sources (shared returns [] on non-macOS) - const sharedKeys = await this._sharedKeychainService.keys(); - const legacyKeys = await this._doGetKeys(); - const merged = new Set([...sharedKeys, ...legacyKeys]); - return [...merged]; + // Include any cross-app shared keys present in the shared keychain + for (const sharedKey of CROSS_APP_SHARED_SECRET_KEYS) { + const sharedValue = await this._sharedKeychainService.get(sharedKey); + if (sharedValue !== undefined && !legacyKeys.includes(sharedKey)) { + legacyKeys.push(sharedKey); + } + } } - return this._doGetKeys(); + return legacyKeys; }); } From 11b7c37c8b7930a084fa0200b3a5f2348f991bc9 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 10 Apr 2026 18:31:17 +0200 Subject: [PATCH 06/25] kick off shared keychain migration eagerly in constructor --- .../electron-browser/secretStorageService.ts | 22 +++++-------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index 814a9dccc4b42..81ccbb5355b1d 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -23,7 +23,7 @@ const MIGRATION_STORAGE_KEY = 'sharedKeychain.migrationDone'; export class NativeSecretStorageService extends BaseSecretStorageService { - private _migrationPromise: Promise | undefined; + private readonly _migrationPromise: Promise; constructor( @INotificationService private readonly _notificationService: INotificationService, @@ -42,18 +42,8 @@ export class NativeSecretStorageService extends BaseSecretStorageService { encryptionService, logService ); - } - /** - * One-time migration: copies all secrets from the legacy safeStorage+SQLite - * pipeline into the shared keychain. Idempotent — guarded by a storage flag - * and keychain writes are upserts. - */ - private _ensureMigration(): Promise { - if (!this._migrationPromise) { - this._migrationPromise = this._doMigration(); - } - return this._migrationPromise; + this._migrationPromise = this._doMigration(); } private async _doMigration(): Promise { @@ -88,7 +78,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override get(key: string): Promise { return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - await this._ensureMigration(); + await this._migrationPromise; // Try shared keychain first (no-op on non-macOS) const value = await this._sharedKeychainService.get(key); if (value !== undefined) { @@ -111,7 +101,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { }); return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - await this._ensureMigration(); + await this._migrationPromise; // Write to shared keychain (no-op on non-macOS) await this._sharedKeychainService.set(key, value); } @@ -123,7 +113,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override delete(key: string): Promise { return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - await this._ensureMigration(); + await this._migrationPromise; // Delete from shared keychain (no-op on non-macOS) await this._sharedKeychainService.delete(key); } @@ -136,7 +126,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { return this._sequencer.queue('__keys__', async () => { const legacyKeys = await this._doGetKeys(); if (this.type !== 'in-memory') { - await this._ensureMigration(); + await this._migrationPromise; // Include any cross-app shared keys present in the shared keychain for (const sharedKey of CROSS_APP_SHARED_SECRET_KEYS) { const sharedValue = await this._sharedKeychainService.get(sharedKey); From 569fdfb04cc2f6e23e8c4da318577d98727e300d Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 10 Apr 2026 18:32:45 +0200 Subject: [PATCH 07/25] update @vscode/macos-keychain to 0.0.1 --- package-lock.json | 7 ++++--- package.json | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index 4d7837a3df597..95415ef8436ef 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,6 +25,7 @@ "@vscode/codicons": "^0.0.46-1", "@vscode/deviceid": "^0.1.1", "@vscode/iconv-lite-umd": "0.7.1", + "@vscode/macos-keychain": "0.0.1", "@vscode/native-watchdog": "^1.4.6", "@vscode/policy-watcher": "^1.3.2", "@vscode/proxy-agent": "^0.41.0", @@ -170,7 +171,7 @@ "yaserver": "^0.4.0" }, "optionalDependencies": { - "@vscode/macos-keychain": "microsoft/vscode-macos-keychain#85ade16af6da2125eb1d31241002a8a2e5ced710", + "@vscode/macos-keychain": "^0.0.1", "windows-foreground-love": "0.6.1" } }, @@ -3838,8 +3839,8 @@ }, "node_modules/@vscode/macos-keychain": { "version": "0.0.1", - "resolved": "git+ssh://git@github.com/microsoft/vscode-macos-keychain.git#85ade16af6da2125eb1d31241002a8a2e5ced710", - "integrity": "sha512-/BkWuiBBh8jqk0m0Oy6uj+l4wfLQTD25auuzbri4ejP0gA96Tmmx4sh06CJTo7glNIcnnmiZQulqdQv1lCAqfQ==", + "resolved": "https://registry.npmjs.org/@vscode/macos-keychain/-/macos-keychain-0.0.1.tgz", + "integrity": "sha512-8R5eKUZRoRUJvmoKgPrXFlEpBg6n8XKq0jyA85DLDuO1ZMbGuKsu2KsUCl7jWm06+h0ajZXUF0Z7dkk6j4IguA==", "hasInstallScript": true, "license": "MIT", "optional": true, diff --git a/package.json b/package.json index 087b96edf0353..bf66d66826d61 100644 --- a/package.json +++ b/package.json @@ -264,7 +264,7 @@ "url": "https://github.com/microsoft/vscode/issues" }, "optionalDependencies": { - "@vscode/macos-keychain": "microsoft/vscode-macos-keychain#85ade16af6da2125eb1d31241002a8a2e5ced710", + "@vscode/macos-keychain": "^0.0.1", "windows-foreground-love": "0.6.1" } } From a1c8f52231604a1ba9fdc969b9f485310b249973 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Sat, 11 Apr 2026 01:33:12 +0200 Subject: [PATCH 08/25] Use provisioning profile for keychain access groups when available During signing, check for build/darwin/distribution.provisionprofile. If present, use it as the provisioning profile and keep the keychain-access-groups entitlement in app-entitlements.plist. If not present (e.g. OSS builds), strip the keychain-access-groups section from a temp copy of the entitlements plist to avoid signing failures. The shared keychain still works via the app's default keychain without access-group isolation. --- build/darwin/sign.ts | 38 ++++++++++++++++++++++++++++++++++---- package.json | 2 +- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/build/darwin/sign.ts b/build/darwin/sign.ts index dc411d5e1fe8d..7437f70c8ef47 100644 --- a/build/darwin/sign.ts +++ b/build/darwin/sign.ts @@ -22,7 +22,9 @@ function getElectronVersion(): string { return target; } -function getEntitlementsForFile(filePath: string): string { +const provisioningProfilePath = path.join(baseDir, 'darwin', 'distribution.provisionprofile'); + +function getEntitlementsForFile(filePath: string, tempDir: string): string { if (filePath.includes(gpuHelperAppName)) { return path.join(baseDir, 'azure-pipelines', 'darwin', 'helper-gpu-entitlements.plist'); } else if (filePath.includes(rendererHelperAppName)) { @@ -30,7 +32,32 @@ function getEntitlementsForFile(filePath: string): string { } else if (filePath.includes(pluginHelperAppName)) { return path.join(baseDir, 'azure-pipelines', 'darwin', 'helper-plugin-entitlements.plist'); } - return path.join(baseDir, 'azure-pipelines', 'darwin', 'app-entitlements.plist'); + const entitlementsPath = path.join(baseDir, 'azure-pipelines', 'darwin', 'app-entitlements.plist'); + if (!fs.existsSync(provisioningProfilePath)) { + // Without a provisioning profile, keychain-access-groups entitlement + // will cause signing failures. Strip it from the entitlements plist. + return getStrippedEntitlements(entitlementsPath, tempDir); + } + return entitlementsPath; +} + +let _strippedEntitlementsPath: string | undefined; + +/** + * Returns a path to a copy of the entitlements plist with the + * keychain-access-groups key removed. + */ +function getStrippedEntitlements(entitlementsPath: string, tempDir: string): string { + if (!_strippedEntitlementsPath) { + const content = fs.readFileSync(entitlementsPath, 'utf8'); + const stripped = content.replace( + /\s*keychain-access-groups<\/key>\s*[\s\S]*?<\/array>/, + '' + ); + _strippedEntitlementsPath = path.join(tempDir, 'app-entitlements-stripped.plist'); + fs.writeFileSync(_strippedEntitlementsPath, stripped); + } + return _strippedEntitlementsPath; } async function retrySignOnKeychainError(fn: () => Promise, maxRetries: number = 3): Promise { @@ -82,15 +109,18 @@ async function main(buildDir?: string): Promise { ? path.resolve(appRoot, appName, 'Contents', 'Applications', `${product.embedded.nameShort}.app`, 'Contents', 'Info.plist') : undefined; + const hasProvisioningProfile = fs.existsSync(provisioningProfilePath); + const appOpts: SignOptions = { app: path.join(appRoot, appName), platform: 'darwin', optionsForFile: (filePath) => ({ - entitlements: getEntitlementsForFile(filePath), + entitlements: getEntitlementsForFile(filePath, tempDir), hardenedRuntime: true, }), preAutoEntitlements: false, - preEmbedProvisioningProfile: false, + preEmbedProvisioningProfile: hasProvisioningProfile, + provisioningProfile: hasProvisioningProfile ? provisioningProfilePath : undefined, keychain: path.join(tempDir, 'buildagent.keychain'), version: getElectronVersion(), identity, diff --git a/package.json b/package.json index bf66d66826d61..5d5e1a00c74b5 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "code-oss-dev", "version": "1.116.0", - "distro": "8c8cff4b820485d6eae5cdf5bf0e1f30f7dbb995", + "distro": "51cdcdede788f3839a9e303bf836b693821f81b6", "author": { "name": "Microsoft Corporation" }, From ec0654525d80f897bef834881dd80f6df8b9a7ba Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Sat, 11 Apr 2026 08:18:56 +0200 Subject: [PATCH 09/25] Add entitlements diagnostic dump after signing Dump the actual entitlements from the signed binary to validate whether $(TeamIdentifierPrefix) is being expanded by codesign. Hypothesis: the variable is passed literally to the entitlements plist without expansion, causing a mismatch with the provisioning profile and resulting in Killed: 9 on launch. --- build/darwin/sign.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/build/darwin/sign.ts b/build/darwin/sign.ts index 7437f70c8ef47..3d39bf681768c 100644 --- a/build/darwin/sign.ts +++ b/build/darwin/sign.ts @@ -24,6 +24,10 @@ function getElectronVersion(): string { const provisioningProfilePath = path.join(baseDir, 'darwin', 'distribution.provisionprofile'); +function hasProvisioningProfile(): boolean { + return fs.existsSync(provisioningProfilePath); +} + function getEntitlementsForFile(filePath: string, tempDir: string): string { if (filePath.includes(gpuHelperAppName)) { return path.join(baseDir, 'azure-pipelines', 'darwin', 'helper-gpu-entitlements.plist'); @@ -33,7 +37,7 @@ function getEntitlementsForFile(filePath: string, tempDir: string): string { return path.join(baseDir, 'azure-pipelines', 'darwin', 'helper-plugin-entitlements.plist'); } const entitlementsPath = path.join(baseDir, 'azure-pipelines', 'darwin', 'app-entitlements.plist'); - if (!fs.existsSync(provisioningProfilePath)) { + if (!hasProvisioningProfile()) { // Without a provisioning profile, keychain-access-groups entitlement // will cause signing failures. Strip it from the entitlements plist. return getStrippedEntitlements(entitlementsPath, tempDir); @@ -109,7 +113,7 @@ async function main(buildDir?: string): Promise { ? path.resolve(appRoot, appName, 'Contents', 'Applications', `${product.embedded.nameShort}.app`, 'Contents', 'Info.plist') : undefined; - const hasProvisioningProfile = fs.existsSync(provisioningProfilePath); + const resolvedProvisioningProfile = hasProvisioningProfile() ? provisioningProfilePath : undefined; const appOpts: SignOptions = { app: path.join(appRoot, appName), @@ -119,8 +123,8 @@ async function main(buildDir?: string): Promise { hardenedRuntime: true, }), preAutoEntitlements: false, - preEmbedProvisioningProfile: hasProvisioningProfile, - provisioningProfile: hasProvisioningProfile ? provisioningProfilePath : undefined, + preEmbedProvisioningProfile: !!resolvedProvisioningProfile, + provisioningProfile: resolvedProvisioningProfile, keychain: path.join(tempDir, 'buildagent.keychain'), version: getElectronVersion(), identity, @@ -205,6 +209,12 @@ async function main(buildDir?: string): Promise { } await retrySignOnKeychainError(() => sign(appOpts)); + + // Dump entitlements from the signed binary for diagnostic purposes + const mainBinary = path.join(appRoot, appName, 'Contents', 'MacOS', product.nameShort); + console.log(`Dumping entitlements from signed binary: ${mainBinary}`); + const entitlementsDump = await spawn('codesign', ['--display', '--entitlements', '-', '--xml', mainBinary]); + console.log(`Signed entitlements:\n${entitlementsDump}`); } if (import.meta.main) { From 2b5329ce443a4f866a75c5123fcca3d30311c28c Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Sat, 11 Apr 2026 08:21:29 +0200 Subject: [PATCH 10/25] Exclude provisioning profile from unicode hygiene check --- build/filters.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/build/filters.ts b/build/filters.ts index d4ea9c8db730d..5741d75a338ba 100644 --- a/build/filters.ts +++ b/build/filters.ts @@ -47,6 +47,7 @@ export const unicodeFilter = Object.freeze([ '!**/*.tiff', '!build/win32/**', + '!build/darwin/**/*.provisionprofile', '!extensions/markdown-language-features/notebook-out/*.js', '!extensions/markdown-math/notebook-out/**', '!extensions/mermaid-chat-features/chat-webview-out/**', From 08b85031c9767bc74a67f7a232a84dce5a290d13 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Wed, 22 Apr 2026 11:45:52 +0200 Subject: [PATCH 11/25] update package-lock.json --- package-lock.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 3f936b9c9f007..e7eec30c58b3e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,7 +25,6 @@ "@vscode/codicons": "^0.0.46-5", "@vscode/deviceid": "^0.1.1", "@vscode/iconv-lite-umd": "0.7.1", - "@vscode/macos-keychain": "0.0.1", "@vscode/native-watchdog": "^1.4.6", "@vscode/policy-watcher": "^1.3.2", "@vscode/proxy-agent": "^0.41.0", From 4772169a7dd7f4a46c0d7b0e99020043e3a340a2 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Wed, 22 Apr 2026 12:21:56 +0200 Subject: [PATCH 12/25] Adopt multiple provision profiles --- build/darwin/sign.ts | 18 +++++++++++++++--- build/filters.ts | 1 - src/vs/platform/secrets/common/secrets.ts | 8 +------- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/build/darwin/sign.ts b/build/darwin/sign.ts index 69d0bc7675a58..73667064c8d36 100644 --- a/build/darwin/sign.ts +++ b/build/darwin/sign.ts @@ -18,10 +18,11 @@ function getElectronVersion(): string { return target; } -const provisioningProfilePath = path.join(baseDir, 'darwin', 'distribution.provisionprofile'); +const mainProvisioningProfilePath = path.join(baseDir, 'darwin', 'main.provisionprofile'); +const agentsProvisioningProfilePath = path.join(baseDir, 'darwin', 'agents.provisionprofile'); function hasProvisioningProfile(): boolean { - return fs.existsSync(provisioningProfilePath); + return fs.existsSync(mainProvisioningProfilePath); } function getEntitlementsForFile(filePath: string, tempDir: string): string { @@ -109,7 +110,18 @@ async function main(buildDir?: string): Promise { ? path.resolve(appRoot, appName, 'Contents', 'Applications', `${product.embedded.nameLong}.app`, 'Contents', 'Info.plist') : undefined; - const resolvedProvisioningProfile = hasProvisioningProfile() ? provisioningProfilePath : undefined; + const resolvedProvisioningProfile = hasProvisioningProfile() ? mainProvisioningProfilePath : undefined; + + // Embed the agents provisioning profile into the embedded app bundle + // before signing, since @electron/osx-sign only supports one top-level profile. + if (product.embedded && fs.existsSync(agentsProvisioningProfilePath)) { + const embeddedAppPath = path.join(appRoot, appName, 'Contents', 'Applications', `${product.embedded.nameLong}.app`); + if (fs.existsSync(embeddedAppPath)) { + const embeddedProfileDest = path.join(embeddedAppPath, 'Contents', 'embedded.provisionprofile'); + fs.copyFileSync(agentsProvisioningProfilePath, embeddedProfileDest); + console.log(`Embedded agents provisioning profile into ${embeddedProfileDest}`); + } + } const appOpts: SignOptions = { app: path.join(appRoot, appName), diff --git a/build/filters.ts b/build/filters.ts index edc18aa5cb092..f43780b6b182f 100644 --- a/build/filters.ts +++ b/build/filters.ts @@ -48,7 +48,6 @@ export const unicodeFilter = Object.freeze([ '!**/*.provisionprofile', '!build/win32/**', - '!build/darwin/**/*.provisionprofile', '!extensions/markdown-language-features/notebook-out/*.js', '!extensions/markdown-math/notebook-out/**', '!extensions/mermaid-chat-features/chat-webview-out/**', diff --git a/src/vs/platform/secrets/common/secrets.ts b/src/vs/platform/secrets/common/secrets.ts index fe855be14fd35..95e9e29174f84 100644 --- a/src/vs/platform/secrets/common/secrets.ts +++ b/src/vs/platform/secrets/common/secrets.ts @@ -75,12 +75,6 @@ export async function writeEncryptedSecret( /** * Secret keys that should be shared between the VS Code app and the agents app. - * When the agents app starts and doesn't have these secrets, it requests them - * from VS Code via crossAppIPC. - */ -/** - * Secret keys that are shared between Code and the Agents app - * via the macOS shared keychain. */ export const CROSS_APP_SHARED_SECRET_KEYS: readonly string[] = [ '{"extensionId":"vscode.github-authentication","key":"github.auth"}', @@ -172,7 +166,6 @@ export class BaseSecretStorageService extends Disposable implements ISecretStora protected async _doSet(key: string, value: string): Promise { const storageService = await this.resolvedStorageService; - try { await writeEncryptedSecret( key, @@ -220,6 +213,7 @@ export class BaseSecretStorageService extends Disposable implements ISecretStora this._logService.trace('[secrets] fetched keys of all secrets'); return allKeys.filter(key => key.startsWith(SECRET_STORAGE_PREFIX)).map(key => key.slice(SECRET_STORAGE_PREFIX.length)); } + private async initialize(): Promise { let storageService; if (!this._useInMemoryStorage && await this._encryptionService.isEncryptionAvailable()) { From 720d141174f7a2884ca626d9887757624703b81d Mon Sep 17 00:00:00 2001 From: deepak1556 Date: Thu, 23 Apr 2026 00:09:30 +0900 Subject: [PATCH 13/25] fix: expand teamidentifier in the entitlement --- build/darwin/sign.ts | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/build/darwin/sign.ts b/build/darwin/sign.ts index 73667064c8d36..4e2f99295d795 100644 --- a/build/darwin/sign.ts +++ b/build/darwin/sign.ts @@ -25,7 +25,7 @@ function hasProvisioningProfile(): boolean { return fs.existsSync(mainProvisioningProfilePath); } -function getEntitlementsForFile(filePath: string, tempDir: string): string { +function getEntitlementsForFile(filePath: string, tempDir: string, teamId?: string): string { if (filePath.includes(' Helper (GPU).app')) { return path.join(baseDir, 'azure-pipelines', 'darwin', 'helper-gpu-entitlements.plist'); } else if (filePath.includes(' Helper (Renderer).app')) { @@ -39,6 +39,9 @@ function getEntitlementsForFile(filePath: string, tempDir: string): string { // will cause signing failures. Strip it from the entitlements plist. return getStrippedEntitlements(entitlementsPath, tempDir); } + if (teamId) { + return getExpandedEntitlements(entitlementsPath, tempDir, teamId); + } return entitlementsPath; } @@ -61,6 +64,22 @@ function getStrippedEntitlements(entitlementsPath: string, tempDir: string): str return _strippedEntitlementsPath; } +let expandedEntitlementsPath: string | undefined; + +/** + * Returns a path to a copy of the entitlements plist with + * $(TeamIdentifierPrefix) expanded to the actual team identifier. + */ +function getExpandedEntitlements(entitlementsPath: string, tempDir: string, teamId: string): string { + if (!expandedEntitlementsPath) { + const content = fs.readFileSync(entitlementsPath, 'utf8'); + const expanded = content.replace(/\$\(TeamIdentifierPrefix\)/g, teamId + '.'); + expandedEntitlementsPath = path.join(tempDir, 'app-entitlements.plist'); + fs.writeFileSync(expandedEntitlementsPath, expanded); + } + return expandedEntitlementsPath; +} + async function retrySignOnKeychainError(fn: () => Promise, maxRetries: number = 3): Promise { let lastError: Error | undefined; @@ -112,6 +131,18 @@ async function main(buildDir?: string): Promise { const resolvedProvisioningProfile = hasProvisioningProfile() ? mainProvisioningProfilePath : undefined; + let teamId: string | undefined; + if (resolvedProvisioningProfile) { + const profilePlist = await spawn('security', ['cms', '-D', '-i', resolvedProvisioningProfile]); + const teamIdMatch = /TeamIdentifier<\/key>\s*\s*(.*?)<\/string>/s.exec(profilePlist); + if (teamIdMatch) { + teamId = teamIdMatch[1]; + console.log(`Extracted TeamIdentifier from provisioning profile: ${teamId}`); + } else { + console.warn('Could not extract TeamIdentifier from provisioning profile; $(TeamIdentifierPrefix) will not be expanded'); + } + } + // Embed the agents provisioning profile into the embedded app bundle // before signing, since @electron/osx-sign only supports one top-level profile. if (product.embedded && fs.existsSync(agentsProvisioningProfilePath)) { @@ -127,7 +158,7 @@ async function main(buildDir?: string): Promise { app: path.join(appRoot, appName), platform: 'darwin', optionsForFile: (filePath) => ({ - entitlements: getEntitlementsForFile(filePath, tempDir), + entitlements: getEntitlementsForFile(filePath, tempDir, teamId), hardenedRuntime: true, }), preAutoEntitlements: false, From 6635fb691a5933e85f9d5c2a77be1017ebd5333a Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Thu, 23 Apr 2026 12:25:34 +0200 Subject: [PATCH 14/25] Re-sign without provisioning profile for tests Run the entitlements step twice in CI: 1. First with provisioning profile (keychain-access-groups) for codesign/notarize 2. Then without provisioning profile for tests (in parallel with codesign) This avoids making codesign sequential with tests while still supporting the keychain-access-groups entitlement that requires a provisioning profile. - Add --skip-provisioning-profile flag to sign.ts - Add 'Set Hardened Entitlements (for tests)' pipeline step --- .../steps/product-build-darwin-compile.yml | 10 ++++++++++ build/darwin/sign.ts | 18 +++++++++++------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/build/azure-pipelines/darwin/steps/product-build-darwin-compile.yml b/build/azure-pipelines/darwin/steps/product-build-darwin-compile.yml index a90e25aeabe36..f5972f6224d86 100644 --- a/build/azure-pipelines/darwin/steps/product-build-darwin-compile.yml +++ b/build/azure-pipelines/darwin/steps/product-build-darwin-compile.yml @@ -332,6 +332,16 @@ steps: BUILD_SOURCESDIRECTORY: $(Build.SourcesDirectory) displayName: ✍️ Codesign & Notarize + # Re-sign the app without the provisioning profile for tests. + # This strips the keychain-access-groups entitlement which requires a + # provisioning profile and is not needed for running tests. The codesign + # step reads from the archives packaged above which have the full entitlements. + - script: | + set -e + export CODESIGN_IDENTITY=$(security find-identity -v -p codesigning $(agent.tempdirectory)/buildagent.keychain | grep -oEi "([0-9A-F]{40})" | head -n 1) + DEBUG=electron-osx-sign* node build/darwin/sign.ts --skip-provisioning-profile $(agent.builddirectory) + displayName: Set Hardened Entitlements (for tests) + - ${{ if or(eq(parameters.VSCODE_RUN_ELECTRON_TESTS, true), eq(parameters.VSCODE_RUN_BROWSER_TESTS, true), eq(parameters.VSCODE_RUN_REMOTE_TESTS, true)) }}: - template: product-build-darwin-test.yml@self parameters: diff --git a/build/darwin/sign.ts b/build/darwin/sign.ts index 4e2f99295d795..3f82bd471fb70 100644 --- a/build/darwin/sign.ts +++ b/build/darwin/sign.ts @@ -25,7 +25,7 @@ function hasProvisioningProfile(): boolean { return fs.existsSync(mainProvisioningProfilePath); } -function getEntitlementsForFile(filePath: string, tempDir: string, teamId?: string): string { +function getEntitlementsForFile(filePath: string, tempDir: string, useProvisioningProfile: boolean, teamId?: string): string { if (filePath.includes(' Helper (GPU).app')) { return path.join(baseDir, 'azure-pipelines', 'darwin', 'helper-gpu-entitlements.plist'); } else if (filePath.includes(' Helper (Renderer).app')) { @@ -34,7 +34,7 @@ function getEntitlementsForFile(filePath: string, tempDir: string, teamId?: stri return path.join(baseDir, 'azure-pipelines', 'darwin', 'helper-plugin-entitlements.plist'); } const entitlementsPath = path.join(baseDir, 'azure-pipelines', 'darwin', 'app-entitlements.plist'); - if (!hasProvisioningProfile()) { + if (!useProvisioningProfile) { // Without a provisioning profile, keychain-access-groups entitlement // will cause signing failures. Strip it from the entitlements plist. return getStrippedEntitlements(entitlementsPath, tempDir); @@ -109,7 +109,7 @@ async function retrySignOnKeychainError(fn: () => Promise, maxRetries: num throw lastError; } -async function main(buildDir?: string): Promise { +async function main(buildDir?: string, skipProvisioningProfile?: boolean): Promise { const tempDir = process.env['AGENT_TEMPDIRECTORY']; const arch = process.env['VSCODE_ARCH']; const identity = process.env['CODESIGN_IDENTITY']; @@ -129,7 +129,8 @@ async function main(buildDir?: string): Promise { ? path.resolve(appRoot, appName, 'Contents', 'Applications', `${product.embedded.nameLong}.app`, 'Contents', 'Info.plist') : undefined; - const resolvedProvisioningProfile = hasProvisioningProfile() ? mainProvisioningProfilePath : undefined; + const useProvisioningProfile = !skipProvisioningProfile && hasProvisioningProfile(); + const resolvedProvisioningProfile = useProvisioningProfile ? mainProvisioningProfilePath : undefined; let teamId: string | undefined; if (resolvedProvisioningProfile) { @@ -145,7 +146,7 @@ async function main(buildDir?: string): Promise { // Embed the agents provisioning profile into the embedded app bundle // before signing, since @electron/osx-sign only supports one top-level profile. - if (product.embedded && fs.existsSync(agentsProvisioningProfilePath)) { + if (useProvisioningProfile && product.embedded && fs.existsSync(agentsProvisioningProfilePath)) { const embeddedAppPath = path.join(appRoot, appName, 'Contents', 'Applications', `${product.embedded.nameLong}.app`); if (fs.existsSync(embeddedAppPath)) { const embeddedProfileDest = path.join(embeddedAppPath, 'Contents', 'embedded.provisionprofile'); @@ -158,7 +159,7 @@ async function main(buildDir?: string): Promise { app: path.join(appRoot, appName), platform: 'darwin', optionsForFile: (filePath) => ({ - entitlements: getEntitlementsForFile(filePath, tempDir, teamId), + entitlements: getEntitlementsForFile(filePath, tempDir, useProvisioningProfile, teamId), hardenedRuntime: true, }), preAutoEntitlements: false, @@ -257,7 +258,10 @@ async function main(buildDir?: string): Promise { } if (import.meta.main) { - main(process.argv[2]).catch(async err => { + const args = process.argv.slice(2); + const skipProvisioningProfile = args.includes('--skip-provisioning-profile'); + const buildDir = args.filter(a => !a.startsWith('--'))[0]; + main(buildDir, skipProvisioningProfile).catch(async err => { console.error(err); const tempDir = process.env['AGENT_TEMPDIRECTORY']; if (tempDir) { From 1cb20cf2311769ce6eff0ad0259729f7a865dcd6 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Thu, 23 Apr 2026 13:03:10 +0200 Subject: [PATCH 15/25] Skip plist modifications when re-signing for tests The plutil -insert calls fail on the second sign pass because the keys already exist from the first pass. Skip plist modifications when --skip-provisioning-profile is set since they are not needed. --- build/darwin/sign.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/build/darwin/sign.ts b/build/darwin/sign.ts index 3f82bd471fb70..fc53479855cac 100644 --- a/build/darwin/sign.ts +++ b/build/darwin/sign.ts @@ -172,7 +172,8 @@ async function main(buildDir?: string, skipProvisioningProfile?: boolean): Promi // Only overwrite plist entries for x64 and arm64 builds, // universal will get its copy from the x64 build. - if (arch !== 'universal') { + // Skip when re-signing (skipProvisioningProfile) since entries already exist. + if (arch !== 'universal' && !skipProvisioningProfile) { await spawn('plutil', [ '-insert', 'NSAppleEventsUsageDescription', From dfb67da92cbaa9b28aa2e0eb289f608704a4cfbb Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 24 Apr 2026 15:58:47 +0200 Subject: [PATCH 16/25] Move shared keychain migration from renderer to main process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace crossAppIPC-based secret handshake with direct shared keychain writes in the main process: - MacOSCrossAppSecretSharing now reads safeStorage+SQLite and writes to shared keychain via SharedKeychainMainService (no crossAppIPC needed) - Code.app migrates on startup; Agents app spawns Code.app once if keychain is incomplete - NativeSecretStorageService no longer does migration — just reads/writes shared keychain for cross-app keys --- src/vs/code/electron-main/app.ts | 2 +- .../macOSCrossAppSecretSharing.ts | 272 +++++------------- .../electron-browser/secretStorageService.ts | 41 +-- 3 files changed, 69 insertions(+), 246 deletions(-) diff --git a/src/vs/code/electron-main/app.ts b/src/vs/code/electron-main/app.ts index f31253f480c0c..738f69e33c602 100644 --- a/src/vs/code/electron-main/app.ts +++ b/src/vs/code/electron-main/app.ts @@ -1275,12 +1275,12 @@ export class CodeApplication extends Disposable { this._register(new MacOSCrossAppSecretSharing( accessor.get(IStorageMainService), accessor.get(IEncryptionMainService), + accessor.get(ISharedKeychainMainService), accessor.get(IStateService), this.logService, this.environmentMainService, accessor.get(ILaunchMainService), this.lifecycleMainService, - crossAppIPCService, )); } diff --git a/src/vs/platform/secrets/electron-main/macOSCrossAppSecretSharing.ts b/src/vs/platform/secrets/electron-main/macOSCrossAppSecretSharing.ts index 6dfea9bc15a96..c95e639f0729c 100644 --- a/src/vs/platform/secrets/electron-main/macOSCrossAppSecretSharing.ts +++ b/src/vs/platform/secrets/electron-main/macOSCrossAppSecretSharing.ts @@ -5,75 +5,49 @@ import { execFile } from 'child_process'; import { dirname } from '../../../base/common/path.js'; -import { Disposable, DisposableStore } from '../../../base/common/lifecycle.js'; +import { Disposable } from '../../../base/common/lifecycle.js'; import { ILogService } from '../../log/common/log.js'; import { IEncryptionMainService } from '../../encryption/common/encryptionService.js'; import { IStorageMainService } from '../../storage/electron-main/storageMainService.js'; -import { CROSS_APP_SHARED_SECRET_KEYS, secretStorageKey, readEncryptedSecret, writeEncryptedSecret } from '../common/secrets.js'; +import { CROSS_APP_SHARED_SECRET_KEYS, readEncryptedSecret } from '../common/secrets.js'; import { IStateService } from '../../state/node/state.js'; import { INodeProcess, isMacintosh } from '../../../base/common/platform.js'; import { IStorageMain } from '../../storage/electron-main/storageMain.js'; import { IEnvironmentMainService } from '../../environment/electron-main/environmentMainService.js'; import { ILaunchMainService } from '../../launch/electron-main/launchMainService.js'; import { ILifecycleMainService } from '../../lifecycle/electron-main/lifecycleMainService.js'; -import { ICrossAppIPCService } from '../../crossAppIpc/electron-main/crossAppIpcService.js'; +import { ISharedKeychainMainService } from '../common/sharedKeychainService.js'; -const MIGRATION_STATE_KEY = 'crossAppSecretSharing.migrationDone'; - -/** - * Message types exchanged between apps over crossAppIPC for secret sharing. - */ -const enum CrossAppSecretMessageType { - /** Agents → Host: Request secrets */ - SecretRequest = 'secrets/request', - /** Host → Agents: Response with secrets */ - SecretResponse = 'secrets/response', - /** Agents → Host: Confirms secrets were stored, both sides mark migration done */ - SecretAck = 'secrets/ack', -} - -interface CrossAppSecretMessage { - type: CrossAppSecretMessageType; - data?: Record; -} +const MIGRATION_STATE_KEY = 'sharedKeychain.migrationDone'; +const HOST_SPAWN_STATE_KEY = 'sharedKeychain.hostSpawnDone'; /** * Coordinates one-time secret migration between the VS Code app and the - * agents app using Electron's crossAppIPC (macOS only). + * agents app via the macOS shared keychain (macOS only). * - * **Demand-driven**: Only the agents app initiates migration. If it - * detects that migration hasn't been done yet, it: - * 1. Waits for the crossAppIPC connection (managed by ICrossAppIPCService). - * 2. Spawns Code.app with `--share-secrets-with-agents-app`, which - * either starts Code.app fresh or (if already running) forwards - * the arg to the existing instance via the node IPC socket. - * 3. Code.app creates its own crossAppIPC connection when it sees - * the arg, and the two connect. - * 4. Agents app sends `SecretRequest` → Code.app responds with - * `SecretResponse` → Agents app sends `SecretAck`. - * 5. Both sides mark migration as done. Code.app quits if it was - * launched solely for this purpose. + * Each app migrates its own secrets from safeStorage+SQLite into the + * shared keychain on startup. The agents app also spawns Code.app + * (once) with `--share-secrets-with-agents-app` to trigger Code's + * migration if the shared keychain doesn't yet contain all expected + * keys. * - * Security: crossAppIPC uses code-signature verification (Mach ports - * on macOS) — the kernel authenticates both endpoints. No secrets are - * ever in process args, files, or network. + * After migration, both apps read from and write to the shared keychain + * for cross-app secret keys (via {@link NativeSecretStorageService}). */ export class MacOSCrossAppSecretSharing extends Disposable { private readonly isEmbeddedApp: boolean; private readonly applicationStorage: IStorageMain; - private _onHostMigrationComplete: (() => void) | undefined; - private readonly hostHandshakeListeners = this._register(new DisposableStore()); constructor( storageMainService: IStorageMainService, private readonly encryptionMainService: IEncryptionMainService, + private readonly sharedKeychainMainService: ISharedKeychainMainService, private readonly stateService: IStateService, private readonly logService: ILogService, environmentMainService: IEnvironmentMainService, launchMainService: ILaunchMainService, lifecycleMainService: ILifecycleMainService, - private readonly crossAppIPCService: ICrossAppIPCService, ) { super(); this.isEmbeddedApp = !!(process as INodeProcess).isEmbeddedApp; @@ -87,143 +61,96 @@ export class MacOSCrossAppSecretSharing extends Disposable { lifecycleMainService: ILifecycleMainService, ): void { if (this.isEmbeddedApp) { - // Agents app: initiate migration if needed + // Agents app: migrate own secrets + spawn Code.app if needed this.initializeAsAgentsApp(); } else if (environmentMainService.args['share-secrets-with-agents-app']) { - // Code.app launched fresh with --share-secrets-with-agents-app: - // respond to the agents app's request, then quit if no other reason to stay + // Code.app launched with --share-secrets-with-agents-app: + // migrate secrets to shared keychain, then quit if no other reason to stay const hasOtherArgs = environmentMainService.args._.length > 0 || environmentMainService.args['folder-uri'] || environmentMainService.args['file-uri']; - this.initializeAsHostApp(hasOtherArgs ? undefined : () => { - this.logService.info('[CrossAppSecretSharing] Host app was launched for migration only, quitting'); - lifecycleMainService.quit(); + this.migrateSecrets().then(() => { + if (!hasOtherArgs) { + this.logService.info('[CrossAppSecretSharing] Host app was launched for migration only, quitting'); + lifecycleMainService.quit(); + } }); } else { - // Code.app already running: listen for --share-secrets-with-agents-app - // forwarded from a second instance via the launch service + // Code.app normal startup: migrate own secrets + this.migrateSecrets(); + // Also respond to spawn requests from the agents app this._register(launchMainService.onDidRequestShareSecrets(() => { - this.initializeAsHostApp(); + this.migrateSecrets(); })); } } private async initializeAsAgentsApp(): Promise { - if (!isMacintosh || !this.isEmbeddedApp) { + if (!isMacintosh) { return; } - if (this.isMigrationDone()) { - this.logService.trace('[CrossAppSecretSharing] Migration already done, skipping'); - return; - } + // Migrate own secrets (if any) to shared keychain + await this.migrateSecrets(); - // Wait for storage to be ready before we start — handleSecretResponse - // will write secrets into applicationStorage. - await this.applicationStorage.whenInit; - - if (!this.crossAppIPCService.initialized) { - this.logService.info('[CrossAppSecretSharing] crossAppIPC not initialized, skipping migration'); + // If we've already spawned Code.app before, don't do it again + if (this.stateService.getItem(HOST_SPAWN_STATE_KEY, false)) { return; } - this.logService.info('[CrossAppSecretSharing] Migration needed, starting...'); - - // Listen for connection — when connected, request secrets - this._register(this.crossAppIPCService.onDidConnect(isServer => { - this.logService.info(`[CrossAppSecretSharing] Connected (isServer=${isServer}), requesting secrets from host app`); - this.crossAppIPCService.sendMessage({ type: CrossAppSecretMessageType.SecretRequest }); - })); - - // Listen for messages - this._register(this.crossAppIPCService.onDidReceiveMessage(msg => { - const secretMsg = msg as CrossAppSecretMessage; - if (secretMsg?.type === CrossAppSecretMessageType.SecretResponse) { - this.handleSecretResponse(secretMsg.data ?? {}); + // Check if the shared keychain has all expected keys + let needsHostMigration = false; + for (const key of CROSS_APP_SHARED_SECRET_KEYS) { + if (await this.sharedKeychainMainService.get(key) === undefined) { + needsHostMigration = true; + break; } - })); - - // If already connected (e.g. service was initialized before storage was ready), - // send the request immediately. - if (this.crossAppIPCService.connected) { - this.logService.info(`[CrossAppSecretSharing] Already connected (isServer=${this.crossAppIPCService.isServer}), requesting secrets from host app`); - this.crossAppIPCService.sendMessage({ type: CrossAppSecretMessageType.SecretRequest }); } - // Spawn Code.app with --share-secrets-with-agents-app - this.spawnHostApp(); + if (needsHostMigration) { + this.logService.info('[CrossAppSecretSharing] Shared keychain incomplete, spawning host app'); + this.spawnHostApp(); + } - // Timeout: if migration doesn't complete within 30s, give up - setTimeout(() => { - if (!this.isMigrationDone()) { - this.logService.warn('[CrossAppSecretSharing] Migration timed out'); - } - }, 30_000); + // Mark that we've attempted the host spawn (don't retry on next startup) + this.stateService.setItem(HOST_SPAWN_STATE_KEY, true); } - private async initializeAsHostApp(onComplete?: () => void): Promise { - if (!isMacintosh || this.isEmbeddedApp) { - onComplete?.(); + /** + * Migrates this app's secrets from safeStorage+SQLite to the shared keychain. + * Idempotent — skips if already done. + */ + private async migrateSecrets(): Promise { + if (!isMacintosh) { return; } - if (this.isMigrationDone()) { + if (this.stateService.getItem(MIGRATION_STATE_KEY, false)) { this.logService.trace('[CrossAppSecretSharing] Migration already done, skipping'); - onComplete?.(); return; } - // Wait for application storage to be fully initialized before - // checking for secrets — storage may still be in-memory at this - // point during early startup. await this.applicationStorage.whenInit; - if (!this.hasAnySharedSecrets()) { - this.logService.trace('[CrossAppSecretSharing] No shared secrets to share, skipping'); - onComplete?.(); - return; - } - - if (!this.crossAppIPCService.initialized) { - this.logService.info('[CrossAppSecretSharing] crossAppIPC not initialized'); - onComplete?.(); - return; - } - - this._onHostMigrationComplete = onComplete; - - this.logService.info('[CrossAppSecretSharing] Host app responding to secret sharing request'); - - // Dispose previous listeners if initializeAsHostApp is called again - // (e.g. via repeated onDidRequestShareSecrets events). - this.hostHandshakeListeners.clear(); - - // Listen for messages from the agents app - this.hostHandshakeListeners.add(this.crossAppIPCService.onDidReceiveMessage(msg => { - const secretMsg = msg as CrossAppSecretMessage; - if (secretMsg?.type === CrossAppSecretMessageType.SecretRequest) { - this.handleSecretRequest(); - } else if (secretMsg?.type === CrossAppSecretMessageType.SecretAck) { - this.handleSecretAck(); - } - })); - - // If disconnected before ack, still allow the host to quit - this.hostHandshakeListeners.add(this.crossAppIPCService.onDidDisconnect(() => { - this._onHostMigrationComplete?.(); - this._onHostMigrationComplete = undefined; - })); - } - - private isMigrationDone(): boolean { - return this.stateService.getItem(MIGRATION_STATE_KEY, false); - } + this.logService.info('[CrossAppSecretSharing] Starting shared keychain migration'); - private hasAnySharedSecrets(): boolean { for (const key of CROSS_APP_SHARED_SECRET_KEYS) { - if (this.applicationStorage.get(secretStorageKey(key)) !== undefined) { - return true; + try { + const decrypted = await readEncryptedSecret( + key, + (fullKey) => this.applicationStorage.get(fullKey), + (value) => this.encryptionMainService.decrypt(value), + this.logService, + ); + if (decrypted !== undefined) { + await this.sharedKeychainMainService.set(key, decrypted); + this.logService.trace('[CrossAppSecretSharing] Migrated key to shared keychain:', key); + } + } catch (err) { + this.logService.error('[CrossAppSecretSharing] Failed to migrate key:', key, err); } } - return false; + + this.stateService.setItem(MIGRATION_STATE_KEY, true); + this.logService.info('[CrossAppSecretSharing] Migration complete'); } private spawnHostApp(): void { @@ -247,69 +174,4 @@ export class MacOSCrossAppSecretSharing extends Disposable { }); child.unref(); } - - private async handleSecretRequest(): Promise { - this.logService.info('[CrossAppSecretSharing] Host app handling secret request'); - - const secrets: Record = {}; - - for (const key of CROSS_APP_SHARED_SECRET_KEYS) { - try { - const decrypted = await readEncryptedSecret( - key, - (fullKey) => this.applicationStorage.get(fullKey), - (value) => this.encryptionMainService.decrypt(value), - this.logService, - ); - if (decrypted !== undefined) { - secrets[key] = decrypted; - } - } catch (err) { - this.logService.error('[CrossAppSecretSharing] Failed to read secret for key:', key, err); - } - } - - this.crossAppIPCService.sendMessage({ type: CrossAppSecretMessageType.SecretResponse, data: secrets }); - this.logService.info('[CrossAppSecretSharing] Sent secrets response with', Object.keys(secrets).length, 'keys'); - } - - private async handleSecretResponse(secrets: Record): Promise { - this.logService.info('[CrossAppSecretSharing] Agents app received', Object.keys(secrets).length, 'secrets'); - - for (const [key, value] of Object.entries(secrets)) { - if (!CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - this.logService.warn('[CrossAppSecretSharing] Ignoring unexpected key:', key); - continue; - } - - try { - await writeEncryptedSecret( - key, - value, - (fullKey, encrypted) => this.applicationStorage.set(fullKey, encrypted), - (v) => this.encryptionMainService.encrypt(v), - this.logService, - ); - } catch (err) { - this.logService.error('[CrossAppSecretSharing] Failed to store secret for key:', key, err); - } - } - - this.stateService.setItem(MIGRATION_STATE_KEY, true); - this.logService.info('[CrossAppSecretSharing] Migration complete'); - - // Tell the host app migration is done so it can also record it. - // Don't close here — let the host close first after receiving the ack. - this.crossAppIPCService.sendMessage({ type: CrossAppSecretMessageType.SecretAck }); - } - - private handleSecretAck(): void { - this.stateService.setItem(MIGRATION_STATE_KEY, true); - this.logService.info('[CrossAppSecretSharing] Host app received ack, migration complete on both sides'); - - const onComplete = this._onHostMigrationComplete; - this._onHostMigrationComplete = undefined; - - onComplete?.(); - } } diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index 81ccbb5355b1d..c8d4199b5bd0e 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -16,15 +16,11 @@ import { INotificationService, IPromptChoice } from '../../../../platform/notifi import { IOpenerService } from '../../../../platform/opener/common/opener.js'; import { BaseSecretStorageService, CROSS_APP_SHARED_SECRET_KEYS, ISecretStorageService } from '../../../../platform/secrets/common/secrets.js'; import { ISharedKeychainService } from '../../../../platform/secrets/common/sharedKeychainService.js'; -import { IStorageService, StorageScope, StorageTarget } from '../../../../platform/storage/common/storage.js'; +import { IStorageService } from '../../../../platform/storage/common/storage.js'; import { IJSONEditingService } from '../../configuration/common/jsonEditing.js'; -const MIGRATION_STORAGE_KEY = 'sharedKeychain.migrationDone'; - export class NativeSecretStorageService extends BaseSecretStorageService { - private readonly _migrationPromise: Promise; - constructor( @INotificationService private readonly _notificationService: INotificationService, @IDialogService private readonly _dialogService: IDialogService, @@ -42,43 +38,11 @@ export class NativeSecretStorageService extends BaseSecretStorageService { encryptionService, logService ); - - this._migrationPromise = this._doMigration(); - } - - private async _doMigration(): Promise { - if (this.type === 'in-memory') { - return; - } - - const storageService = await this.resolvedStorageService; - if (storageService.get(MIGRATION_STORAGE_KEY, StorageScope.APPLICATION) === '1') { - this._logService.trace('[NativeSecretStorageService] shared keychain migration already done'); - return; - } - - this._logService.trace('[NativeSecretStorageService] starting shared keychain migration'); - - for (const sharedKey of CROSS_APP_SHARED_SECRET_KEYS) { - try { - const value = await this._doGet(sharedKey); - if (value !== undefined) { - await this._sharedKeychainService.set(sharedKey, value); - this._logService.trace('[NativeSecretStorageService] shared keychain migration: migrated', sharedKey); - } - } catch (err) { - this._logService.error('[NativeSecretStorageService] migration failed for:', sharedKey, err); - } - } - - storageService.store(MIGRATION_STORAGE_KEY, '1', StorageScope.APPLICATION, StorageTarget.MACHINE); - this._logService.trace('[NativeSecretStorageService] shared keychain migration complete'); } override get(key: string): Promise { return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - await this._migrationPromise; // Try shared keychain first (no-op on non-macOS) const value = await this._sharedKeychainService.get(key); if (value !== undefined) { @@ -101,7 +65,6 @@ export class NativeSecretStorageService extends BaseSecretStorageService { }); return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - await this._migrationPromise; // Write to shared keychain (no-op on non-macOS) await this._sharedKeychainService.set(key, value); } @@ -113,7 +76,6 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override delete(key: string): Promise { return this._sequencer.queue(key, async () => { if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - await this._migrationPromise; // Delete from shared keychain (no-op on non-macOS) await this._sharedKeychainService.delete(key); } @@ -126,7 +88,6 @@ export class NativeSecretStorageService extends BaseSecretStorageService { return this._sequencer.queue('__keys__', async () => { const legacyKeys = await this._doGetKeys(); if (this.type !== 'in-memory') { - await this._migrationPromise; // Include any cross-app shared keys present in the shared keychain for (const sharedKey of CROSS_APP_SHARED_SECRET_KEYS) { const sharedValue = await this._sharedKeychainService.get(sharedKey); From f7c2ced6757439ceb0c5f61442ea169e37db79c1 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Fri, 24 Apr 2026 16:15:53 +0200 Subject: [PATCH 17/25] Add isMacintosh guards before using the shared keychain service Co-authored-by: Copilot --- .../secrets/electron-browser/secretStorageService.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index c8d4199b5bd0e..99daf30051e4c 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import { createSingleCallFunction } from '../../../../base/common/functional.js'; -import { isLinux } from '../../../../base/common/platform.js'; +import { isLinux, isMacintosh } from '../../../../base/common/platform.js'; import Severity from '../../../../base/common/severity.js'; import { localize } from '../../../../nls.js'; import { IDialogService } from '../../../../platform/dialogs/common/dialogs.js'; @@ -42,7 +42,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override get(key: string): Promise { return this._sequencer.queue(key, async () => { - if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { + if (isMacintosh && this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { // Try shared keychain first (no-op on non-macOS) const value = await this._sharedKeychainService.get(key); if (value !== undefined) { @@ -64,7 +64,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { } }); return this._sequencer.queue(key, async () => { - if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { + if (isMacintosh && this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { // Write to shared keychain (no-op on non-macOS) await this._sharedKeychainService.set(key, value); } @@ -75,7 +75,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override delete(key: string): Promise { return this._sequencer.queue(key, async () => { - if (this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { + if (isMacintosh && this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { // Delete from shared keychain (no-op on non-macOS) await this._sharedKeychainService.delete(key); } @@ -87,7 +87,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override async keys(): Promise { return this._sequencer.queue('__keys__', async () => { const legacyKeys = await this._doGetKeys(); - if (this.type !== 'in-memory') { + if (isMacintosh && this.type !== 'in-memory') { // Include any cross-app shared keys present in the shared keychain for (const sharedKey of CROSS_APP_SHARED_SECRET_KEYS) { const sharedValue = await this._sharedKeychainService.get(sharedKey); From efe36617103d9ce54d3702ca8a13e068a7596daa Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Sun, 26 Apr 2026 09:59:32 +0300 Subject: [PATCH 18/25] Remove spec --- secrets-sharing-impl.md | 208 ----------------------------- secrets-sharing-spec.md | 288 ---------------------------------------- 2 files changed, 496 deletions(-) delete mode 100644 secrets-sharing-impl.md delete mode 100644 secrets-sharing-spec.md diff --git a/secrets-sharing-impl.md b/secrets-sharing-impl.md deleted file mode 100644 index 74aca2a666e5e..0000000000000 --- a/secrets-sharing-impl.md +++ /dev/null @@ -1,208 +0,0 @@ -# Secrets Sharing Implementation Notes - -**Spec**: `secrets-sharing-spec.md` (in this repo) -**Issue**: https://github.com/microsoft/vscode/issues/308028 -**VS Code PR**: https://github.com/microsoft/vscode/pull/308990 -**Date started**: 9 April 2026 - -## Current status: Phase 1 (macOS) — feature-complete, testing in progress - -All Phase 1 steps from the spec are implemented. The end-to-end flow (Code writes secrets → Agents reads them) works locally. - -## What's been done - -### Step 1: macOS native Node addon (`@vscode/macos-keychain`) - -**Repo**: https://github.com/microsoft/vscode-macos-keychain -**PR #1** (merged): Initial addon with access group support -**PR #2** (open): Auto-detect access group from entitlements -**PR #3** (open): Fix build on non-macOS CI - -The addon wraps the macOS Security framework (`SecItemAdd`, `SecItemCopyMatching`, `SecItemUpdate`, `SecItemDelete`). - -#### API (after PR #2) - -```typescript -keychainSet(service: string, account: string, value: string): void -keychainGet(service: string, account: string): string | undefined -keychainDelete(service: string, account: string): boolean -keychainList(service: string): string[] -``` - -The `accessGroup` parameter was removed from the public API. Instead, `keychainInit()` (called automatically at module load) reads the process's `keychain-access-groups` entitlement via `SecTaskCopyValueForEntitlement` and caches the first access group found. If no entitlement exists (unsigned/dev builds), items go to the app's default keychain. - -### Step 2: Entitlements - -Added `keychain-access-groups` to `build/azure-pipelines/darwin/app-entitlements.plist`: - -```xml -keychain-access-groups - - $(TeamIdentifierPrefix)com.microsoft.vscode.shared-secrets - -``` - -Both Code.app and its embedded Agents.app use the same entitlements file (via `getEntitlementsForFile()` in `build/darwin/sign.ts`), so both get the shared access group. - -### Step 3: ISharedKeychainService - -Follows the standard Electron main process IPC service pattern (like `IEncryptionService`): - -| Layer | File | Role | -|-------|------|------| -| Common interface | `src/vs/platform/secrets/common/sharedKeychainService.ts` | `ISharedKeychainService` + `ISharedKeychainMainService` decorators and interfaces | -| Main process impl | `src/vs/platform/secrets/electron-main/sharedKeychainMainService.ts` | `SharedKeychainMainService` — wraps native addon, no-op on non-macOS | -| Main process registration | `src/vs/code/electron-main/app.ts` | `services.set(ISharedKeychainMainService, ...)` + `ProxyChannel.fromService(...)` + `registerChannel('sharedKeychain', ...)` | -| Renderer proxy | `src/vs/workbench/services/secrets/electron-browser/sharedKeychainService.ts` | `registerMainProcessRemoteService(ISharedKeychainService, 'sharedKeychain')` | - -**Design decisions**: -- Service is registered on all platforms; the main process implementation is a no-op on non-macOS (`enabled = isMacintosh && !!productService.darwinSharedKeychainServiceName`) -- `set()` is best-effort (logs error, does not throw) — callers also write to the legacy pipeline for rollback safety, so shared keychain failures should not break secret persistence -- `get()`/`delete()`/`keys()` are also best-effort (return `undefined`/`false`/`[]` on error) - -### Step 4: NativeSecretStorageService changes - -`src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts`: - -**Base class refactoring**: `BaseSecretStorageService` now exposes protected `_doGet`/`_doSet`/`_doDelete`/`_doGetKeys` methods that perform the actual safeStorage+SQLite operations without going through the sequencer. This allows subclasses to call them from within their own sequencer-queued tasks without deadlocking (since `SequencerByKey` would deadlock if the same key is queued from within a queued task). - -**NativeSecretStorageService overrides** (all guarded by `this.type !== 'in-memory'`): -- `get()`: tries shared keychain first, falls back to `_doGet()` (legacy pipeline) -- `set()`: writes to shared keychain, then `_doSet()` (dual-write for rollback safety) -- `delete()`: deletes from shared keychain, then `_doDelete()` -- `keys()`: merges results from shared keychain and `_doGetKeys()` - -### Step 5: Migration - -One-time, lazy migration runs on the first secret operation: - -1. `_ensureMigration()` is called at the start of every `get`/`set`/`delete`/`keys` (when `type !== 'in-memory'`) -2. Reads all `secret://`-prefixed keys from SQLite via `_doGetKeys()` -3. Decrypts each with safeStorage via `_doGet()` -4. Writes plaintext to shared keychain via `_sharedKeychainService.set()` -5. Stores `sharedKeychain.migrationDone = '1'` in `StorageScope.APPLICATION` -6. Subsequent calls skip migration (cached promise + storage flag) - -**Properties**: -- Idempotent: keychain writes are upserts, storage flag prevents re-runs across sessions -- Best-effort per key: individual failures don't block the rest -- Safe with multiple windows: concurrent migrations do redundant but harmless work (all write the same values) -- Skipped for in-memory mode - -### Step 6: Agents app wiring - -`src/vs/sessions/sessions.desktop.main.ts` imports both: -- `../workbench/services/secrets/electron-browser/secretStorageService.js` (NativeSecretStorageService) -- `../workbench/services/secrets/electron-browser/sharedKeychainService.js` (IPC proxy) - -The Agents app reads from the shared keychain via the same `NativeSecretStorageService` overrides. No separate migration needed — it reads whatever Code wrote. - -### Product configuration - -**`darwinSharedKeychainServiceName`** — the `kSecAttrService` value that groups keychain items. Per-flavor to isolate secrets between Stable/Insiders/Exploration: - -| Flavor | Value | -|--------|-------| -| Code OSS | `com.visualstudio.code.oss.shared-secrets` (set in `product.json`) | -| Code Stable | Set in internal product.json (e.g. `com.microsoft.vscode.shared-secrets`) | -| Code Insiders | Set in internal product.json (e.g. `com.microsoft.vscode-insiders.shared-secrets`) | - -This field is at the **top level** of product.json, NOT in the `embedded` section — so both Code and its embedded Agents app within the same flavor get the same value. The `embedded` overlay (in `build/gulpfile.vscode.ts`) only copies fields listed in `IEmbeddedProductConfiguration`. - -**`darwinKeychainAccessGroup`** — removed from product.json. The native addon auto-detects it from the process's entitlements at module load time. This avoids keeping the team ID prefix in sync between the entitlements plist and product.json. - -### Other files changed - -| File | Change | -|------|--------| -| `package.json` | Added `@vscode/macos-keychain` to `optionalDependencies` (macOS-only) | -| `build/.moduleignore` | Added entries for `@vscode/macos-keychain` (keep only `.node` binary) | -| `src/typings/macos-keychain.d.ts` | Type declarations for cross-platform compilation | -| `src/vs/base/common/product.ts` | Added `darwinSharedKeychainServiceName` to `IProductConfiguration` | -| `src/vs/workbench/workbench.desktop.main.ts` | Import shared keychain service registration | -| `src/vs/sessions/sessions.desktop.main.ts` | Import shared keychain service registration | - -## Decisions and rationale - -### 1. Template: `@vscode/policy-watcher` (not `native-keymap`) - -Used `@vscode/policy-watcher` as the template for the native addon because it uses modern `node-addon-api` (C++ NAPI wrapper), `bindings` package for loading, and has macOS-specific native code. - -### 2. Access group auto-detection (PR #2) - -Initially the `accessGroup` was a JS parameter passed through to the native code. This required keeping the team ID prefix (e.g. `UBF8T346G9.com.microsoft.vscode.shared-secrets`) in product.json, which was error-prone and would diverge from the entitlements plist. - -**Solution**: The native addon calls `SecTaskCopyValueForEntitlement` at module init to read the `keychain-access-groups` entitlement from its own process. This makes the access group an implementation detail — JS never needs to know the team ID prefix. - -### 3. IPC service pattern (not direct import) - -The native addon runs in the **main process** (via `SharedKeychainMainService`), exposed to renderer windows via `ProxyChannel`/`registerMainProcessRemoteService`. This follows the established pattern for native services (like `IEncryptionService`). An earlier attempt to load the addon directly in the `electron-browser` layer failed layering checks — the `electron-browser` tsconfig doesn't include `node/` files. - -### 4. Dual-write for rollback safety - -`set()` writes to both the shared keychain AND the legacy safeStorage+SQLite pipeline. This means if we discover a critical issue with the shared keychain, we can remove the new code path and all secrets are still in the old storage. The legacy pipeline can be removed in a future release once the shared keychain is proven stable. - -### 5. Best-effort shared keychain operations - -All `SharedKeychainMainService` methods catch errors and return safe defaults (`undefined`/`false`/`[]`). This prevents shared keychain failures (e.g. missing entitlements in dev builds, addon load failures on non-macOS CI) from breaking existing secret functionality. - -### 6. `CFRef` RAII wrapper for CoreFoundation objects - -CoreFoundation objects must be `CFRelease`d. The addon uses a `CFRef` template class for RAII. When setting into CF dictionaries, uses `.get()` (not `.release()`) so the RAII wrapper's destructor balances the create while `CFDictionarySetValue` retains its own +1 reference. An earlier version using `.release()` caused refcount leaks. - -### 7. Security hardening in native addon - -- **Secret zeroing**: `secureClear()` uses a `volatile char*` to zero `std::string` buffers -- **Build flags**: `-D_FORTIFY_SOURCE=2`, `-Wformat`, `-Wformat-security`, `-fstack-protector-strong` -- **Error sanitization**: account/service names omitted from error messages -- **Input bounds**: values capped at 100 KB, strings at 1 KB -- **NUL byte rejection**: all string arguments validated - -### 8. Platform guard in SharedKeychainMainService - -The service checks `isMacintosh && !!productService.darwinSharedKeychainServiceName` at construction time. If either is false, all methods are no-ops. This means: -- On Windows/Linux: no keychain operations attempted -- On macOS without `darwinSharedKeychainServiceName` in product.json: no keychain operations (but this shouldn't happen in practice since Code OSS sets it) - -### 9. `type !== 'in-memory'` guard in NativeSecretStorageService - -Shared keychain operations are skipped when `this.type === 'in-memory'` (encryption unavailable). The type can be `'unknown'` during initialization before encryption availability is determined — shared keychain operations proceed for both `'persisted'` and `'unknown'` states. - -## Testing strategy - -### Unit tests -- Run existing `BaseSecretStorageService` tests to verify the `_doGet`/`_doSet`/`_doDelete`/`_doGetKeys` refactoring didn't break anything: - ```bash - ./scripts/test.sh --run src/vs/platform/secrets/test/common/secrets.test.ts - ``` - -### Compilation check -```bash -npm run compile-check-ts-native -``` - -### Manual E2E flow -1. Launch Code OSS: `./scripts/code.sh` -2. Sign in to GitHub/Microsoft -3. Verify Keychain Access shows entries under `com.visualstudio.code.oss.shared-secrets` -4. Launch Agents: `./scripts/code.sh --agents --user-data-dir=$HOME/.vscode-oss-sessions-dev --extensions-dir=$HOME/.vscode-oss-sessions-dev/extensions` -5. Verify the Agents app is signed in without re-authentication - -### Edge cases -- Cold start with no prior secrets (no migration needed) -- Restart after migration (flag set, migration skips) -- Delete a secret in Code → verify it's gone from both keychain and SQLite -- Multiple windows restoring simultaneously (benign concurrent migration) - -## Open questions (from spec) - -1. ~~**Naming convention for keychain items**~~ → Resolved: `darwinSharedKeychainServiceName` in product.json, per-flavor -2. **Entitlement signing**: Does adding `keychain-access-groups` to `app-entitlements.plist` require CI/CD signing process changes? Needs verification with the build team. -3. **Notification/sync**: When Code writes a new secret, should the Agents app be notified in real-time? Currently read-on-demand (lazy). -4. **Scope**: Currently shares ALL secrets. May want to filter to auth-related only in the future. - -## Phase 2 (Windows) and Phase 3 (Linux) — future work - -Not started. See spec for approach: -- **Windows**: Cross-read Code's SQLite + same DPAPI key (user-scoped, not app-scoped). Must avoid CredMan for secret values (2.5KB size limit). -- **Linux**: Similar to Windows — keyring services are user-scoped. diff --git a/secrets-sharing-spec.md b/secrets-sharing-spec.md deleted file mode 100644 index a00f58f9656a1..0000000000000 --- a/secrets-sharing-spec.md +++ /dev/null @@ -1,288 +0,0 @@ -# Secrets Sharing Between VS Code and Agents App - -**Issue**: https://github.com/microsoft/vscode/issues/308028 -**Related**: https://github.com/microsoft/vscode/issues/308353 -**Assignees**: alexdima, deepak1556 -**Stakeholders**: Tyler Leonhardt (auth team), Kai (requested the work) - -## Problem Statement - -The Agents app is a standalone application bundled inside VS Code. When a user is already authenticated in VS Code (e.g., signed into GitHub, Microsoft account), they must re-authenticate in the Agents app because the two apps have completely isolated secret stores. This is a friction point for onboarding — users expect to be signed in automatically. - -## Current Architecture - -### How secrets are stored today - -The secret storage pipeline has three layers: - -#### Layer 1: Encryption via Electron's `safeStorage` - -**File**: `src/vs/platform/encryption/electron-main/encryptionMainService.ts` - -`EncryptionMainService` wraps Electron's `safeStorage` API: -- `safeStorage.encryptString(value)` — encrypts a string using an OS-managed encryption key -- `safeStorage.decryptString(buffer)` — decrypts it back - -On macOS, Electron stores the encryption key in the **macOS Keychain** under the app's own name (derived from `CFBundleName`). On macOS, `getKeyStorageProvider()` returns `KnownStorageProvider.keychainAccess`. - -The encrypted values are JSON-serialized `Buffer` objects (e.g., `{"data": "..."}`). - -#### Layer 2: Secret storage service (encrypt-then-store in SQLite) - -**File**: `src/vs/platform/secrets/common/secrets.ts` - -`BaseSecretStorageService` stores secrets with a `secret://` key prefix in `IStorageService` (a SQLite database scoped to `StorageScope.APPLICATION`): - -- **`set(key, value)`**: Encrypts `value` via `IEncryptionService.encrypt()`, then stores the ciphertext in SQLite under the key `secret://`. -- **`get(key)`**: Reads the ciphertext from SQLite under `secret://`, then decrypts via `IEncryptionService.decrypt()`. -- **`delete(key)`**: Removes the entry from SQLite. -- **`keys()`**: Lists all `secret://`-prefixed keys from SQLite. - -The desktop override is `NativeSecretStorageService` in `src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts`, which adds user notifications for cases where encryption is unavailable. - -#### Layer 3: Extension API - -Extensions use `vscode.ExtensionContext.secrets.store(key, value)` / `.get(key)`, which delegates to `ISecretStorageService`. Example: `extensions/github-authentication/src/common/keychain.ts`. - -### How the two apps are deployed - -On macOS, the Agents app is a **nested `.app` bundle** inside the main Code `.app`: - -``` -Code.app/ - Contents/ - Applications/ - .app ← separate bundle, own bundle ID, own Dock icon -``` - -This is configured in `build/gulpfile.vscode.ts` using metadata from `product.json`'s `embedded` key (typed as `EmbeddedProductInfo` in `build/lib/embeddedType.ts`): - -```typescript -export type EmbeddedProductInfo = { - nameShort: string; - nameLong: string; - applicationName: string; - dataFolderName: string; // ← separate data directory - darwinBundleIdentifier: string; // ← separate bundle ID - urlProtocol: string; - // ... win32 fields -}; -``` - -When the Agents `.app` launches, it runs its **own Electron main process** with `process.isEmbeddedApp = true`. It has its own Dock icon, bundle identifier, URL protocol, and data folder. See `src/vs/code/electron-main/app.ts` — when `isEmbeddedApp` is true, it calls `openAgentsWindow()` which opens a `BrowserWindow` loading `sessions.html` instead of `workbench.html`. - -The Agents window can also be opened within Code's process via the `--agents` flag (development/insider only), in which case it's just another `BrowserWindow` in the same Electron main process. - -### Why secrets are currently isolated (two barriers) - -#### Barrier 1: Encryption key isolation (macOS) - -Electron's `safeStorage` stores its encryption key in the macOS Keychain under the **app's own `CFBundleName`**. Code's keychain item has a name like `"Code"` or `"Code - Insiders"`, while the Agents app would have its own name (e.g., `"Agents"`). These are **separate keychain items with separate encryption keys**. The Agents app cannot read Code's encryption key, and vice versa. The `safeStorage` API has no concept of shared access groups. - -#### Barrier 2: Storage database isolation (all platforms) - -Each app has its own `dataFolderName` (from the `embedded` product config), so the SQLite databases where encrypted secrets live are in **completely separate directories** (e.g., `~/Library/Application Support/Code/` vs `~/Library/Application Support/Agents/`). Even if the two apps could use the same encryption key, they wouldn't find each other's stored ciphertext. - -### How the Agents app currently uses secrets - -The Agents app (`src/vs/sessions/`) imports the same `NativeSecretStorageService` as Code: -- `src/vs/sessions/sessions.desktop.main.ts` imports `../workbench/services/secrets/electron-browser/secretStorageService.js` - -So it goes through the exact same encrypt-then-store-in-SQLite pipeline, but with its own encryption key and its own SQLite database. - -## Team Discussion Summary - -### Tyler Leonhardt (auth team) confirmed: - -1. The current-state analysis above is correct. -2. He prefers a "shared keychain" concept over a backdoor into VS Code's secrets. -3. **Critical warning for Windows**: Windows CredMan has a **~2.5 KB blob size limit** per credential. A single Microsoft auth refresh token already exceeds this limit. This is why Electron stores only the small encryption key in the credential store and puts the actual (large) encrypted secrets in SQLite. **Do not attempt to store secrets directly in CredMan on Windows.** - -### deepak1556 (Electron/platform team) recommended: - -1. **macOS**: Use Apple's [Keychain Access Groups](https://developer.apple.com/documentation/security/sharing-access-to-keychain-items-among-a-collection-of-apps) — the official mechanism for sharing keychain items between apps signed by the same team. Since Electron's `safeStorage` API has no concept of access groups, **build a native Node.js addon** (Objective-C) rather than waiting on Electron/Chromium changes. -2. **Windows**: DPAPI is user-scoped, not app-scoped — any process running as the same user can already encrypt/decrypt with the same key. The only barrier is the separate SQLite databases. However, app-bound encryption (per-app isolation) is a concept from Chromium that could complicate this in the future; we don't use it today. - -## Proposed Solution (macOS first) - -### Design Principles - -1. **Don't change how Code stores secrets for existing users** — millions of installations have secrets in the current format (safeStorage + SQLite). The new mechanism must coexist with the old one. -2. **Migration must be re-entrant** — if we find a critical problem, we can roll back. Re-running migration should be safe (idempotent). -3. **macOS first, then Windows/Linux** — macOS is the most complex case (true app-level keychain isolation). Windows and Linux are simpler. -4. **Native addon, no Electron changes required** — deepak1556's recommendation. Keeps us in control of the timeline. - -### macOS: Native Node Addon with Shared Keychain Access Group - -#### Why store directly in Keychain on macOS? - -macOS Keychain (`kSecClassGenericPassword`) does **not** have the blob size limitations that Windows CredMan has. Large values (auth tokens, etc.) can be stored directly as keychain items. This eliminates the need for a separate SQLite database for shared secrets, which is a major simplification — no need to coordinate database locations or cross-read another app's SQLite file. - -#### Keychain access groups (Apple mechanism) - -From [Apple's documentation](https://developer.apple.com/documentation/security/sharing-access-to-keychain-items-among-a-collection-of-apps): - -- Every app has a default access group equal to its **app ID** (`$(TeamID).$(BundleID)`), making its keychain items private by default. -- Apps signed by the same team can share keychain items by adding a common **keychain access group** to both apps' entitlements via the `keychain-access-groups` entitlement. -- Each keychain item belongs to exactly one access group (set via `kSecAttrAccessGroup` when creating the item). -- When searching, you can specify `kSecAttrAccessGroup` to limit the search, or omit it to search all groups your app belongs to. - -#### Entitlements - -Both `Code.app` and `Agents.app` need the same keychain access group in their entitlements: - -```xml -keychain-access-groups - - $(TeamIdentifierPrefix)com.microsoft.vscode.shared-secrets - -``` - -Adding a keychain access group to entitlements is **backward-compatible** — it does not affect existing keychain items stored under the app's default access group. Existing safeStorage-managed items remain accessible. - -#### Native addon API - -Build a native Node.js addon (C/Objective-C) that wraps the macOS Security framework: - -``` -// Conceptual API: -keychainSet(service: string, account: string, value: string, accessGroup: string): void -keychainGet(service: string, account: string, accessGroup: string): string | undefined -keychainDelete(service: string, account: string, accessGroup: string): void -keychainList(service: string, accessGroup: string): string[] // returns account names -``` - -Internally uses `SecItemAdd`, `SecItemCopyMatching`, `SecItemUpdate`, `SecItemDelete` with: -- `kSecClass`: `kSecClassGenericPassword` -- `kSecAttrService`: A agreed-upon service name, e.g. `"com.microsoft.vscode.shared-secrets"` -- `kSecAttrAccount`: The secret key (e.g., `"github.auth"`) -- `kSecAttrAccessGroup`: `".com.microsoft.vscode.shared-secrets"` -- `kSecValueData`: The secret value (UTF-8 encoded) -- `kSecUseDataProtectionKeychain`: `true` (required for access groups on macOS) - -#### Service architecture - -A new `ISharedSecretStorageService` (or an alternative `ISecretStorageService` implementation) that: - -1. On **write**: Stores the secret directly in macOS Keychain using the native addon with the shared access group. Also notifies the old `ISecretStorageService` pipeline to keep the local copy in sync (for rollback safety). -2. On **read**: Reads from the shared keychain via the native addon. Falls back to the old pipeline if not found (pre-migration secrets). -3. On **delete**: Deletes from both the shared keychain and the old pipeline. - -#### Migration - -A one-time, re-entrant migration runs in **both** Code and the Agents app on startup: - -**In Code (the primary migration source)**: -1. Read all `secret://`-prefixed keys from `IStorageService` (SQLite). -2. For each key, decrypt using the existing `safeStorage`-based `IEncryptionService`. -3. Write the decrypted value to the shared keychain using the native addon. -4. Mark the migration as complete in a storage flag. - -**Re-entrancy**: The migration can be run multiple times safely. Writing to the keychain is idempotent (if the item already exists with the same value, it's a no-op or update). If a critical issue is found, the old storage still has all secrets untouched — we can remove the shared keychain code path and fall back. - -**In the Agents app**: Reads from the shared keychain directly. No migration needed from the Agents side — it just needs to find the secrets Code put there. - -### Data flow (macOS, after migration) - -``` -Extension calls context.secrets.get("github.auth") - → ISecretStorageService.get("github.auth") - → [new path] Native addon: SecItemCopyMatching( - service: "com.microsoft.vscode.shared-secrets", - account: "github.auth", - accessGroup: ".com.microsoft.vscode.shared-secrets" - ) - → Returns plaintext secret directly from keychain - → (no SQLite, no encrypt/decrypt needed — keychain IS the secure storage) -``` - -### Comparison: old vs. new (macOS) - -| Aspect | Old (safeStorage + SQLite) | New (shared keychain) | -|--------|---------------------------|----------------------| -| Encryption key | Per-app, in Keychain, managed by Electron | Not needed — Keychain itself is the secure store | -| Secret storage | Encrypted ciphertext in per-app SQLite | Plaintext in Keychain, protected by OS | -| Cross-app access | Impossible | Via shared `keychain-access-groups` entitlement | -| Size limits | None (SQLite) | None on macOS Keychain | -| Native code needed | No (Electron handles it) | Yes (native Node addon) | - -## Windows Strategy (future work) - -**Important constraint from Tyler**: Windows CredMan has a ~2.5 KB limit per credential — a single Microsoft auth refresh token exceeds this. This is why Electron chose to store only the encryption key (small) in the OS credential store, rather than the actual secrets. **Do not store secrets directly in CredMan.** - -### Current Windows state (from deepak1556) - -On Windows, when using `safeStorage` today, the DPAPI encryption keys are stored to a **file** under the user data directory after base64 encoding. There is **no protection between app processes for the same user on Windows** — any app running as the same user can read this file. This also applies to Windows Credential Manager (same process and user scope as DPAPI). This is why Chrome had to invent their own app-bound encryption to isolate secrets per app. - -### Likely approach: Cross-read Code's storage - -Since both the encryption key file and the SQLite database are just files under the user data directory, the Agents app can: - -1. **Read Code's DPAPI encryption key file** from Code's user data directory (the key is base64-encoded in a file, readable by any same-user process). -2. **Read Code's SQLite database** to find `secret://`-prefixed entries. -3. **Decrypt the ciphertext** using the encryption key — or equivalently, use `safeStorage.decryptString()` if within the same Electron process, since DPAPI is user-scoped. - -The Agents app needs to know Code's `dataFolderName` path, which is available from the parent app's `product.json` or can be derived from the `embedded` configuration. - -**Note**: Chromium has app-bound encryption on Windows that isolates secrets per app, but VS Code does not use this today. If it's adopted in the future, the cross-reading approach would break and would need revisiting. Deepak will provide more details when work begins on Windows. - -## Linux Strategy (future work) - -Similar to Windows — keyring services (gnome-keyring, kwallet) are user-scoped, not app-scoped. The Agents app should be able to cross-read Code's SQLite database and decrypt with its own `safeStorage` call. The same `dataFolderName` cross-referencing approach applies. - -## Files Involved - -| File | Role | -|------|------| -| `src/vs/platform/encryption/common/encryptionService.ts` | `IEncryptionService` interface, `KnownStorageProvider` enum | -| `src/vs/platform/encryption/electron-main/encryptionMainService.ts` | `EncryptionMainService` — wraps `electron.safeStorage` | -| `src/vs/platform/secrets/common/secrets.ts` | `BaseSecretStorageService` — encrypt-then-store in SQLite | -| `src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts` | `NativeSecretStorageService` — desktop override with notifications | -| `src/vs/sessions/sessions.desktop.main.ts` | Agents app entry point — imports secretStorageService | -| `src/vs/code/electron-main/app.ts` | Main process — `isEmbeddedApp` handling, `registerEmbeddedAppWithLaunchServices` | -| `build/lib/embeddedType.ts` | `EmbeddedProductInfo` type (bundle ID, data folder, etc.) | -| `build/gulpfile.vscode.ts` | macOS packaging — `darwinMiniAppName`, `darwinMiniAppBundleIdentifier`, etc. | -| `extensions/github-authentication/src/common/keychain.ts` | Example extension using `context.secrets` | - -## Implementation Plan - -### Phase 1: macOS shared keychain (starting here) - -1. **Native addon**: Create a macOS-specific native Node.js addon that wraps `SecItemAdd`/`SecItemCopyMatching`/`SecItemUpdate`/`SecItemDelete` with access group support. - - Location: `src/vs/platform/encryption/electron-main/darwin/` (or similar) - - Must handle: UTF-8 encoding, error codes, item-not-found vs. real errors, updating existing items - -2. **Entitlements**: Add `keychain-access-groups` entitlement to both Code and Agents app builds. - - Location: `build/darwin/` entitlements files - -3. **New service**: Create a shared-keychain-backed `ISecretStorageService` implementation for macOS. - - Reads/writes directly to Keychain with the shared access group - - Falls back to old safeStorage+SQLite path if keychain read fails - -4. **Migration logic**: In Code, on startup, migrate existing secrets to the shared keychain. - - Read all `secret://` keys from SQLite - - Decrypt each with safeStorage - - Write plaintext to shared keychain via native addon - - Store a "migration complete" flag - - Must be idempotent / re-entrant - -5. **Testing**: Test with real auth tokens (Microsoft account refresh tokens are large — this was the CredMan problem on Windows, but macOS Keychain should handle it fine). Test both directions: Code writes, Agents reads; verify sign-in state is shared. - -### Phase 2: Windows (future) - -- Evaluate the approaches listed above with deepak1556 -- Likely: cross-read Code's SQLite + same DPAPI key -- Must avoid CredMan for secret values (size limit) - -### Phase 3: Linux (future) - -- Similar to Windows approach -- Test with gnome-keyring and kwallet backends - -## Open Questions - -1. **Naming convention for keychain items**: What `kSecAttrService` value to use? Needs to be stable across versions. -2. **Entitlement signing**: Does adding `keychain-access-groups` require any changes to the CI/CD signing process? -3. **App-bound encryption on Windows**: If Chromium/Electron adopts this in the future, what's the fallback plan? -4. **Notification/sync**: When Code writes a new secret, should the Agents app be notified in real-time? Or is read-on-demand sufficient? -5. **Scope**: Should we share ALL secrets, or only auth-related ones? Sharing all is simpler but may have unintended consequences. From 8a565ba4cc275f5351581f98e422cd1d3f01ce91 Mon Sep 17 00:00:00 2001 From: Alex Dima Date: Sun, 26 Apr 2026 10:00:09 +0300 Subject: [PATCH 19/25] Tweak comments --- .../secrets/electron-browser/secretStorageService.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts index 99daf30051e4c..8763ad91df807 100644 --- a/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts +++ b/src/vs/workbench/services/secrets/electron-browser/secretStorageService.ts @@ -43,7 +43,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override get(key: string): Promise { return this._sequencer.queue(key, async () => { if (isMacintosh && this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - // Try shared keychain first (no-op on non-macOS) + // Try shared keychain first const value = await this._sharedKeychainService.get(key); if (value !== undefined) { return value; @@ -65,7 +65,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { }); return this._sequencer.queue(key, async () => { if (isMacintosh && this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - // Write to shared keychain (no-op on non-macOS) + // Write to shared keychain await this._sharedKeychainService.set(key, value); } // Also write to legacy pipeline @@ -76,7 +76,7 @@ export class NativeSecretStorageService extends BaseSecretStorageService { override delete(key: string): Promise { return this._sequencer.queue(key, async () => { if (isMacintosh && this.type !== 'in-memory' && CROSS_APP_SHARED_SECRET_KEYS.includes(key)) { - // Delete from shared keychain (no-op on non-macOS) + // Delete from shared keychain await this._sharedKeychainService.delete(key); } // Delete from legacy pipeline From 40cc75ec3aab0e613bac71af82f0fcc1325abc96 Mon Sep 17 00:00:00 2001 From: ulugbekna Date: Sun, 26 Apr 2026 18:06:39 +0200 Subject: [PATCH 20/25] nes: support neighbor files to be included in NES prompt Co-authored-by: Copilot --- .../node/nextEditProviderTelemetry.ts | 6 + .../vscode-node/similarFilesContext.ts | 106 +++++++++---- .../src/extension/test/node/services.ts | 18 +-- .../src/extension/xtab/common/lineRange.ts | 16 ++ .../extension/xtab/common/promptCrafting.ts | 11 +- .../xtab/common/recentFilesForPrompt.ts | 80 +++++++++- .../xtab/common/similarFilesContextService.ts | 31 ++++ .../src/extension/xtab/node/xtabProvider.ts | 28 +++- .../test/common/recentFilesForPrompt.spec.ts | 147 +++++++++++++++++- .../copilot/src/lib/node/chatLibMain.ts | 12 +- .../common/configurationService.ts | 2 + .../common/dataTypes/xtabPromptOptions.ts | 21 +++ .../common/statelessNextEditProvider.ts | 27 ++++ 13 files changed, 441 insertions(+), 64 deletions(-) create mode 100644 extensions/copilot/src/extension/xtab/common/lineRange.ts diff --git a/extensions/copilot/src/extension/inlineEdits/node/nextEditProviderTelemetry.ts b/extensions/copilot/src/extension/inlineEdits/node/nextEditProviderTelemetry.ts index 3cb27c243d3c5..274916b0c72e2 100644 --- a/extensions/copilot/src/extension/inlineEdits/node/nextEditProviderTelemetry.ts +++ b/extensions/copilot/src/extension/inlineEdits/node/nextEditProviderTelemetry.ts @@ -1115,6 +1115,9 @@ export class TelemetrySender implements IDisposable { "promptCharCount": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Number of characters in the prompt", "isMeasurement": true }, "nDiffsInPrompt": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Number of diffs included in the prompt", "isMeasurement": true }, "diffTokensInPrompt": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Number of tokens consumed by diffs in the prompt", "isMeasurement": true }, + "nNeighborSnippetsComputed": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Total number of neighbor (similar files) snippets computed before budget filtering", "isMeasurement": true }, + "nNeighborSnippetsInPrompt": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Number of neighbor (similar files) snippets actually included in the prompt", "isMeasurement": true }, + "neighborSnippetIndicesInPrompt": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "JSON-encoded array of original input indices (ascending) of neighbor snippets included in the prompt" }, "hadLowLogProbSuggestion": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Whether the suggestion had low log probability", "isMeasurement": true }, "nEditsSuggested": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Number of edits suggested", "isMeasurement": true }, "hasNextEdit": { "classification": "SystemMetaData", "purpose": "FeatureInsight", "comment": "Whether next edit provider returned an edit (if an edit was previously rejected, this field is false)", "isMeasurement": true }, @@ -1177,6 +1180,7 @@ export class TelemetrySender implements IDisposable { xtabAggressivenessLevel, userAggressivenessSetting, modelConfig, + neighborSnippetIndicesInPrompt: telemetry.neighborSnippetIndicesInPrompt, }, { requestN, @@ -1237,6 +1241,8 @@ export class TelemetrySender implements IDisposable { xtabUserHappinessScore, nDiffsInPrompt: telemetry.nDiffsInPrompt, diffTokensInPrompt: telemetry.diffTokensInPrompt, + nNeighborSnippetsComputed: telemetry.nNeighborSnippetsComputed, + nNeighborSnippetsInPrompt: telemetry.nNeighborSnippetsInPrompt, } ); } diff --git a/extensions/copilot/src/extension/inlineEdits/vscode-node/similarFilesContext.ts b/extensions/copilot/src/extension/inlineEdits/vscode-node/similarFilesContext.ts index 8591ebd4e59e6..bfd4836049a7b 100644 --- a/extensions/copilot/src/extension/inlineEdits/vscode-node/similarFilesContext.ts +++ b/extensions/copilot/src/extension/inlineEdits/vscode-node/similarFilesContext.ts @@ -10,8 +10,12 @@ import { TelemetryWithExp } from '../../completions-core/vscode-node/lib/src/tel import { ICompletionsTextDocumentManagerService } from '../../completions-core/vscode-node/lib/src/textDocumentManager'; import { DocumentInfoWithOffset } from '../../completions-core/vscode-node/prompt/src/prompt'; import { getSimilarSnippets } from '../../completions-core/vscode-node/prompt/src/snippetInclusion/similarFiles'; +import { SnippetWithProviderInfo } from '../../completions-core/vscode-node/prompt/src/snippetInclusion/snippets'; import { ICopilotInlineCompletionItemProviderService } from '../../completions/common/copilotInlineCompletionItemProviderService'; -import { ISimilarFilesContextService } from '../../xtab/common/similarFilesContextService'; +import { LineRange0Based } from '../../xtab/common/lineRange'; +import { INeighborFileSnippet, ISimilarFilesContextService } from '../../xtab/common/similarFilesContextService'; + +type RankedSnippet = SnippetWithProviderInfo & { relativePath?: string }; export class SimilarFilesContextService implements ISimilarFilesContextService { @@ -23,39 +27,13 @@ export class SimilarFilesContextService implements ISimilarFilesContextService { async compute(uri: string, languageId: string, source: string, cursorOffset: number): Promise { try { - const completionsInstaService = this._copilotService.getOrCreateInstantiationService(); - const telemetryData = TelemetryWithExp.createEmptyConfigForTesting(); - - const { docs } = await completionsInstaService.invokeFunction( - accessor => NeighborSource.getNeighborFilesAndTraits(accessor, uri, languageId, telemetryData) - ); - - const promptOptions = completionsInstaService.invokeFunction(getPromptOptions, telemetryData, languageId); - const similarFilesOptions = - promptOptions.similarFilesOptions || - completionsInstaService.invokeFunction(getSimilarFilesOptions, telemetryData, languageId); - - const tdm = completionsInstaService.invokeFunction(accessor => accessor.get(ICompletionsTextDocumentManagerService)); - const relativePath = tdm.getRelativePath({ uri }); - - const docInfo: DocumentInfoWithOffset = { - uri, - source, - languageId, - offset: cursorOffset, - relativePath, - }; - - const snippets = (await getSimilarSnippets( - docInfo, - Array.from(docs.values()), - similarFilesOptions, - )) - .filter(s => s.snippet.length > 0) - .sort((a, b) => a.score - b.score); - + const result = await this._gatherSnippets(uri, languageId, source, cursorOffset); + if (!result) { + return undefined; + } + const { neighborFileCount, snippets } = result; return JSON.stringify({ - neighborFileCount: docs.size, + neighborFileCount, snippets: snippets.map(s => ({ score: s.score, startLine: s.startLine, @@ -68,4 +46,66 @@ export class SimilarFilesContextService implements ISimilarFilesContextService { return undefined; } } + + async getSnippetsForPrompt(uri: string, languageId: string, source: string, cursorOffset: number): Promise { + try { + const result = await this._gatherSnippets(uri, languageId, source, cursorOffset); + if (!result) { + return undefined; + } + const { snippets, relativePathToUri } = result; + return snippets.map(s => ({ + uri: (s.relativePath && relativePathToUri.get(s.relativePath)) ?? uri, + relativePath: s.relativePath, + snippet: s.snippet, + lineRange: new LineRange0Based(s.startLine, s.endLine), + score: s.score, + })); + } catch { + return undefined; + } + } + + private async _gatherSnippets(uri: string, languageId: string, source: string, cursorOffset: number): Promise<{ neighborFileCount: number; snippets: RankedSnippet[]; relativePathToUri: Map } | undefined> { + const completionsInstaService = this._copilotService.getOrCreateInstantiationService(); + const telemetryData = TelemetryWithExp.createEmptyConfigForTesting(); + + const { docs } = await completionsInstaService.invokeFunction( + accessor => NeighborSource.getNeighborFilesAndTraits(accessor, uri, languageId, telemetryData) + ); + + const promptOptions = completionsInstaService.invokeFunction(getPromptOptions, telemetryData, languageId); + const similarFilesOptions = + promptOptions.similarFilesOptions || + completionsInstaService.invokeFunction(getSimilarFilesOptions, telemetryData, languageId); + + const tdm = completionsInstaService.invokeFunction(accessor => accessor.get(ICompletionsTextDocumentManagerService)); + const relativePath = tdm.getRelativePath({ uri }); + + const docInfo: DocumentInfoWithOffset = { + uri, + source, + languageId, + offset: cursorOffset, + relativePath, + }; + + const neighborDocs = Array.from(docs.values()); + const relativePathToUri = new Map(); + for (const doc of neighborDocs) { + if (doc.relativePath) { + relativePathToUri.set(doc.relativePath, doc.uri); + } + } + + const snippets = (await getSimilarSnippets( + docInfo, + neighborDocs, + similarFilesOptions, + )) + .filter(s => s.snippet.length > 0) + .sort((a, b) => a.score - b.score); + + return { neighborFileCount: docs.size, snippets, relativePathToUri }; + } } diff --git a/extensions/copilot/src/extension/test/node/services.ts b/extensions/copilot/src/extension/test/node/services.ts index e7afea1809f0d..5948f64620bb4 100644 --- a/extensions/copilot/src/extension/test/node/services.ts +++ b/extensions/copilot/src/extension/test/node/services.ts @@ -10,6 +10,8 @@ import { IChatHookService } from '../../../platform/chat/common/chatHookService' import { IChatMLFetcher } from '../../../platform/chat/common/chatMLFetcher'; import { ISessionTranscriptService, NullSessionTranscriptService } from '../../../platform/chat/common/sessionTranscriptService'; import { MockChatMLFetcher } from '../../../platform/chat/test/common/mockChatMLFetcher'; +import { ISessionStore } from '../../../platform/chronicle/common/sessionStore'; +import { SessionStore } from '../../../platform/chronicle/node/sessionStore'; import { IDiffService } from '../../../platform/diff/common/diffService'; import { DiffServiceImpl } from '../../../platform/diff/node/diffServiceImpl'; import { EmbeddingType, IEmbeddingsComputer } from '../../../platform/embeddings/common/embeddingsComputer'; @@ -54,13 +56,13 @@ import { SyncDescriptor } from '../../../util/vs/platform/instantiation/common/d import { ILanguageModelServer } from '../../agents/node/langModelServer'; import { MockLanguageModelServer } from '../../agents/node/test/mockLanguageModelServer'; import { IClaudeRuntimeDataService } from '../../chatSessions/claude/common/claudeRuntimeDataService'; +import { IClaudeSessionStateService } from '../../chatSessions/claude/common/claudeSessionStateService'; import { IClaudeToolPermissionService } from '../../chatSessions/claude/common/claudeToolPermissionService'; import { ClaudeCodeModels, IClaudeCodeModels } from '../../chatSessions/claude/node/claudeCodeModels'; import { IClaudeCodeSdkService } from '../../chatSessions/claude/node/claudeCodeSdkService'; import { ClaudeRuntimeDataService } from '../../chatSessions/claude/node/claudeRuntimeDataService'; -import { IClaudePluginService } from '../../chatSessions/claude/node/claudeSkills'; -import { IClaudeSessionStateService } from '../../chatSessions/claude/common/claudeSessionStateService'; import { ClaudeSessionStateService } from '../../chatSessions/claude/node/claudeSessionStateService'; +import { IClaudePluginService } from '../../chatSessions/claude/node/claudeSkills'; import { MockClaudeCodeSdkService } from '../../chatSessions/claude/node/test/mockClaudeCodeSdkService'; import { MockClaudeToolPermissionService } from '../../chatSessions/claude/node/test/mockClaudeToolPermissionService'; import { CommandServiceImpl, ICommandService } from '../../commands/node/commandService'; @@ -84,9 +86,7 @@ import { ToolGroupingService } from '../../tools/common/virtualTools/toolGroupin import '../../tools/node/allTools'; import { TestToolsService } from '../../tools/node/test/testToolsService'; import { TestToolEmbeddingsComputer } from '../../tools/test/node/virtualTools/testVirtualTools'; -import { ISimilarFilesContextService } from '../../xtab/common/similarFilesContextService'; -import { ISessionStore } from '../../../platform/chronicle/common/sessionStore'; -import { SessionStore } from '../../../platform/chronicle/node/sessionStore'; +import { ISimilarFilesContextService, NullSimilarFilesContextService } from '../../xtab/common/similarFilesContextService'; export interface ISimulationModelConfig { chatModel?: string; @@ -180,14 +180,6 @@ class NullClaudePluginService implements IClaudePluginService { } } -class NullSimilarFilesContextService implements ISimilarFilesContextService { - declare readonly _serviceBrand: undefined; - - async compute(): Promise { - return undefined; - } -} - class NullChatHookService implements IChatHookService { declare readonly _serviceBrand: undefined; diff --git a/extensions/copilot/src/extension/xtab/common/lineRange.ts b/extensions/copilot/src/extension/xtab/common/lineRange.ts new file mode 100644 index 0000000000000..495d93f1035bb --- /dev/null +++ b/extensions/copilot/src/extension/xtab/common/lineRange.ts @@ -0,0 +1,16 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +/** + * A 0-based line range where `startLine` is inclusive and `endLineExcl` is exclusive. + */ +export class LineRange0Based { + constructor( + /** 0-based, inclusive. */ + readonly startLine: number, + /** 0-based, exclusive. */ + readonly endLineExcl: number, + ) { } +} diff --git a/extensions/copilot/src/extension/xtab/common/promptCrafting.ts b/extensions/copilot/src/extension/xtab/common/promptCrafting.ts index cb734872293a5..314265d2d6f6a 100644 --- a/extensions/copilot/src/extension/xtab/common/promptCrafting.ts +++ b/extensions/copilot/src/extension/xtab/common/promptCrafting.ts @@ -17,7 +17,8 @@ import { OffsetRange } from '../../../util/vs/editor/common/core/ranges/offsetRa import { getEditDiffHistory } from './diffHistoryForPrompt'; import { LintErrors } from './lintErrors'; import { countTokensForLines, toUniquePath } from './promptCraftingUtils'; -import { getRecentCodeSnippets } from './recentFilesForPrompt'; +import { AppendNeighborFileSnippetsResult, getRecentCodeSnippets } from './recentFilesForPrompt'; +import { INeighborFileSnippet } from './similarFilesContextService'; import { PromptTags } from './tags'; import { CurrentDocument } from './xtabCurrentDocument'; @@ -35,6 +36,7 @@ export class PromptPieces { public readonly lintErrors: LintErrors, public readonly computeTokens: (s: string) => number, public readonly opts: PromptOptions, + public readonly neighborSnippets?: readonly INeighborFileSnippet[], ) { } } @@ -43,14 +45,15 @@ export interface UserPromptResult { readonly prompt: string; readonly nDiffsInPrompt: number; readonly diffTokensInPrompt: number; + readonly neighborSnippetsResult: AppendNeighborFileSnippetsResult | undefined; } export function getUserPrompt(promptPieces: PromptPieces): UserPromptResult { - const { activeDoc, xtabHistory, taggedCurrentDocLines, areaAroundCodeToEdit, langCtx, aggressivenessLevel, lintErrors, computeTokens, opts } = promptPieces; + const { activeDoc, xtabHistory, taggedCurrentDocLines, areaAroundCodeToEdit, langCtx, aggressivenessLevel, lintErrors, computeTokens, opts, neighborSnippets } = promptPieces; const currentFileContent = taggedCurrentDocLines.join('\n'); - const { codeSnippets: recentlyViewedCodeSnippets, documents: docsInPrompt } = getRecentCodeSnippets(activeDoc, xtabHistory, langCtx, computeTokens, opts); + const { codeSnippets: recentlyViewedCodeSnippets, documents: docsInPrompt, neighborSnippetsResult } = getRecentCodeSnippets(activeDoc, xtabHistory, langCtx, computeTokens, opts, neighborSnippets); docsInPrompt.add(activeDoc.id); // Add active document to the set of documents in prompt @@ -125,7 +128,7 @@ ${PromptTags.EDIT_HISTORY.end}`; const trimmedPrompt = prompt.trim(); - return { prompt: trimmedPrompt, nDiffsInPrompt, diffTokensInPrompt }; + return { prompt: trimmedPrompt, nDiffsInPrompt, diffTokensInPrompt, neighborSnippetsResult }; } function wrapInBackticks(content: string) { diff --git a/extensions/copilot/src/extension/xtab/common/recentFilesForPrompt.ts b/extensions/copilot/src/extension/xtab/common/recentFilesForPrompt.ts index 902c3d3a2ad69..0684435ec3a7d 100644 --- a/extensions/copilot/src/extension/xtab/common/recentFilesForPrompt.ts +++ b/extensions/copilot/src/extension/xtab/common/recentFilesForPrompt.ts @@ -16,6 +16,7 @@ import { OffsetRange } from '../../../util/vs/editor/common/core/ranges/offsetRa import { StringText } from '../../../util/vs/editor/common/core/text/abstractText'; import { expandRangeToPageRange } from './promptCrafting'; import { countTokensForLines, toUniquePath } from './promptCraftingUtils'; +import { INeighborFileSnippet } from './similarFilesContextService'; import { PromptTags } from './tags'; export function getRecentCodeSnippets( @@ -24,7 +25,8 @@ export function getRecentCodeSnippets( langCtx: LanguageContextResponse | undefined, computeTokens: (code: string) => number, opts: PromptOptions, -): { codeSnippets: string; documents: Set } { + neighborSnippets?: readonly INeighborFileSnippet[], +): { codeSnippets: string; documents: Set; neighborSnippetsResult: AppendNeighborFileSnippetsResult | undefined } { const { includeViewedFiles, nDocuments, clippingStrategy } = opts.recentlyViewedDocuments; @@ -44,9 +46,15 @@ export function getRecentCodeSnippets( appendLanguageContextSnippets(langCtx, snippets, opts.languageContext.maxTokens, computeTokens, opts.recentlyViewedDocuments.includeLineNumbers); } + let neighborSnippetsResult: AppendNeighborFileSnippetsResult | undefined; + if (opts.neighborFiles.enabled && neighborSnippets && neighborSnippets.length > 0) { + neighborSnippetsResult = appendNeighborFileSnippets(neighborSnippets, snippets, docsInPrompt, opts.neighborFiles.maxTokens, computeTokens, opts.recentlyViewedDocuments.includeLineNumbers); + } + return { codeSnippets: snippets.join('\n\n'), documents: docsInPrompt, + neighborSnippetsResult, }; } @@ -319,6 +327,76 @@ function appendLanguageContextSnippets( } } +/** + * Result of appending neighbor-file snippets, used for telemetry. + */ +export interface AppendNeighborFileSnippetsResult { + /** Total snippets considered (input array length). */ + readonly nComputed: number; + /** Snippets actually included in the prompt. */ + readonly nIncluded: number; + /** + * Original input indices (ascending) of snippets included in the prompt. + * E.g. `[3, 4, 6]` means snippets at original indices 3, 4 and 6 were included + * (snippet at index 5 was skipped — too large or duplicate doc — and snippets + * at 0, 1 and 2 were skipped because the budget ran out). + */ + readonly includedIndices: readonly number[]; +} + +/** + * Append Completions-style neighbor-file snippets (Jaccard-ranked) to the snippets array. + * + * Snippets are pre-clipped by Completions (each is a fixed-window match around + * the cursor in a neighbor file) and arrive ordered with the highest-scoring + * snippet last. We select greedily from highest score downward so the best + * snippets reserve budget first, skipping any whose file is already represented + * in {@link docsInPrompt} (avoids duplicating recently-edited or recently-viewed + * files) and any snippet that would exceed the remaining token budget. Selected + * snippets are then appended in score-ascending order so the highest-scoring + * snippet ends up closest to the current file in the prompt. + */ +export function appendNeighborFileSnippets( + neighborSnippets: readonly INeighborFileSnippet[], + snippets: string[], + docsInPrompt: Set, + tokenBudget: number, + computeTokens: (code: string) => number, + includeLineNumbers: xtabPromptOptions.IncludeLineNumbersOption, +): AppendNeighborFileSnippetsResult { + const selected: { snippet: INeighborFileSnippet; originalIndex: number }[] = []; + // Iterate from highest score (last) to lowest (first) so the best snippets reserve budget first. + for (let i = neighborSnippets.length - 1; i >= 0; i--) { + const neighborSnippet = neighborSnippets[i]; + const documentId = DocumentId.create(neighborSnippet.uri); + if (docsInPrompt.has(documentId)) { + continue; + } + const potentialBudget = tokenBudget - computeTokens(neighborSnippet.snippet); + if (potentialBudget < 0) { + continue; + } + selected.push({ snippet: neighborSnippet, originalIndex: i }); + docsInPrompt.add(documentId); + tokenBudget = potentialBudget; + } + // Reverse so the highest-scoring snippet is appended last (closest to the current file). + for (let i = selected.length - 1; i >= 0; i--) { + const neighborSnippet = selected[i].snippet; + snippets.push(formatCodeSnippet( + DocumentId.create(neighborSnippet.uri), + neighborSnippet.snippet.split(/\r?\n/), + { truncated: false, includeLineNumbers, startLineOffset: neighborSnippet.lineRange.startLine }, + )); + } + const includedIndices = selected.map(s => s.originalIndex).sort((a, b) => a - b); + return { + nComputed: neighborSnippets.length, + nIncluded: selected.length, + includedIndices, + }; +} + /** * Clip a file without visible ranges by taking pages from the start until budget is exhausted. * diff --git a/extensions/copilot/src/extension/xtab/common/similarFilesContextService.ts b/extensions/copilot/src/extension/xtab/common/similarFilesContextService.ts index 4b7d3e05dfd4c..fd1bb28137205 100644 --- a/extensions/copilot/src/extension/xtab/common/similarFilesContextService.ts +++ b/extensions/copilot/src/extension/xtab/common/similarFilesContextService.ts @@ -4,6 +4,19 @@ *--------------------------------------------------------------------------------------------*/ import { createServiceIdentifier } from '../../../util/common/services'; +import { LineRange0Based } from './lineRange'; + +/** + * A neighbor-file snippet selected via Jaccard similarity, ready to be + * embedded into the recently_viewed_code_snippets section of the prompt. + */ +export interface INeighborFileSnippet { + readonly uri: string; + readonly relativePath: string | undefined; + readonly snippet: string; + readonly lineRange: LineRange0Based; + readonly score: number; +} export interface ISimilarFilesContextService { readonly _serviceBrand: undefined; @@ -13,6 +26,24 @@ export interface ISimilarFilesContextService { * @returns JSON-serialized telemetry payload, or `undefined` on any error. Never throws. */ compute(uri: string, languageId: string, source: string, cursorOffset: number): Promise; + + /** + * Computes neighbor-file snippets (Jaccard-ranked) intended for inclusion in the prompt. + * @returns Snippets ordered with best (highest scores) last, or `undefined` on any error. Never throws. + */ + getSnippetsForPrompt(uri: string, languageId: string, source: string, cursorOffset: number): Promise; } export const ISimilarFilesContextService = createServiceIdentifier('ISimilarFilesContextService'); + +export class NullSimilarFilesContextService implements ISimilarFilesContextService { + declare readonly _serviceBrand: undefined; + + async compute(): Promise { + return undefined; + } + + async getSnippetsForPrompt(): Promise { + return undefined; + } +} diff --git a/extensions/copilot/src/extension/xtab/node/xtabProvider.ts b/extensions/copilot/src/extension/xtab/node/xtabProvider.ts index e5196bcc2669f..291cc330fdf77 100644 --- a/extensions/copilot/src/extension/xtab/node/xtabProvider.ts +++ b/extensions/copilot/src/extension/xtab/node/xtabProvider.ts @@ -357,6 +357,20 @@ export class XtabProvider implements IStatelessNextEditProvider { return new NoNextEditReason.GotCancelled('afterLanguageContextAwait'); } + const neighborSnippets = promptOptions.neighborFiles.enabled + ? await raceCancellation( + raceTimeout( + this.similarFilesContextService.getSnippetsForPrompt(activeDocument.id.uri, activeDocument.languageId, activeDocument.documentAfterEdits.value, currentDocument.cursorOffset), + delaySession.getDebounceTime() + ), + cancellationToken, + ) + : undefined; + + if (cancellationToken.isCancellationRequested) { + return new NoNextEditReason.GotCancelled('afterNeighborSnippetsAwait'); + } + const lintErrors = new LintErrors(activeDocument.id, currentDocument, this.langDiagService, request.xtabEditHistory); const promptPieces = new PromptPieces( @@ -371,13 +385,19 @@ export class XtabProvider implements IStatelessNextEditProvider { aggressivenessLevel, lintErrors, XtabProvider.computeTokens, - promptOptions + promptOptions, + neighborSnippets, ); - const { prompt: userPrompt, nDiffsInPrompt, diffTokensInPrompt } = getUserPrompt(promptPieces); + const { prompt: userPrompt, nDiffsInPrompt, diffTokensInPrompt, neighborSnippetsResult } = getUserPrompt(promptPieces); telemetry.setNDiffsInPrompt(nDiffsInPrompt); telemetry.setDiffTokensInPrompt(diffTokensInPrompt); + if (neighborSnippetsResult) { + telemetry.setNNeighborSnippetsComputed(neighborSnippetsResult.nComputed); + telemetry.setNNeighborSnippetsInPrompt(neighborSnippetsResult.nIncluded); + telemetry.setNeighborSnippetIndicesInPrompt(neighborSnippetsResult.includedIndices); + } const responseFormat = xtabPromptOptions.ResponseFormat.fromPromptingStrategy(promptOptions.promptingStrategy); @@ -1408,6 +1428,10 @@ export class XtabProvider implements IStatelessNextEditProvider { maxTokens: this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabLanguageContextMaxTokens, this.expService), traitPosition: this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabLanguageContextTraitsPosition, this.expService), }), + neighborFiles: { + enabled: this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabIncludeNeighborFiles, this.expService), + maxTokens: this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabNeighborFilesMaxTokens, this.expService), + }, diffHistory: { nEntries: this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabDiffNEntries, this.expService), maxTokens: this.configService.getExperimentBasedConfig(ConfigKey.TeamInternal.InlineEditsXtabDiffMaxTokens, this.expService), diff --git a/extensions/copilot/src/extension/xtab/test/common/recentFilesForPrompt.spec.ts b/extensions/copilot/src/extension/xtab/test/common/recentFilesForPrompt.spec.ts index 00d4c588fb669..10a54c40a39fe 100644 --- a/extensions/copilot/src/extension/xtab/test/common/recentFilesForPrompt.spec.ts +++ b/extensions/copilot/src/extension/xtab/test/common/recentFilesForPrompt.spec.ts @@ -12,7 +12,9 @@ import { splitLines } from '../../../../util/vs/base/common/strings'; import { StringEdit } from '../../../../util/vs/editor/common/core/edits/stringEdit'; import { OffsetRange } from '../../../../util/vs/editor/common/core/ranges/offsetRange'; import { StringText } from '../../../../util/vs/editor/common/core/text/abstractText'; -import { buildCodeSnippetsUsingPagedClipping, computeFocalPageCost, historyEntriesToCodeSnippet, selectFocalRangesWithinSpanCap } from '../../common/recentFilesForPrompt'; +import { LineRange0Based } from '../../common/lineRange'; +import { appendNeighborFileSnippets, buildCodeSnippetsUsingPagedClipping, computeFocalPageCost, historyEntriesToCodeSnippet, selectFocalRangesWithinSpanCap } from '../../common/recentFilesForPrompt'; +import { INeighborFileSnippet } from '../../common/similarFilesContextService'; function nLines(n: number): StringText { return new StringText(new Array(n).fill(0).map((_, i) => `${i + 1}`).join('\n')); @@ -825,3 +827,146 @@ suite('historyEntriesToCodeSnippet', () => { ]); }); }); + +suite('appendNeighborFileSnippets', () => { + + function makeNeighbor(uri: string, snippet: string, startLine: number, score = 0.5): INeighborFileSnippet { + const lineCount = snippet.split(/\r?\n/).length; + return { uri, relativePath: uri, snippet, lineRange: new LineRange0Based(startLine, startLine + lineCount), score }; + } + + test('appends snippets formatted like recent files', () => { + const snippets: string[] = []; + const docsInPrompt = new Set(); + appendNeighborFileSnippets( + [makeNeighbor('file:///src/n1.ts', 'a\nb', 5)], + snippets, + docsInPrompt, + /*tokenBudget*/ 100, + computeTokens, + IncludeLineNumbersOption.WithSpaceAfter, + ); + expect(snippets).toMatchInlineSnapshot(` + [ + "<|recently_viewed_code_snippet|> + code_snippet_file_path: /src/n1.ts + 5| a + 6| b + <|/recently_viewed_code_snippet|>", + ] + `); + expect(docsInPrompt.has(DocumentId.create('file:///src/n1.ts'))).toBe(true); + }); + + test('skips oversize snippets but keeps trying smaller ones', () => { + const snippets: string[] = []; + const docsInPrompt = new Set(); + // First snippet ~= 8 tokens (32 chars / 4); second ~= 1 token. Budget 5 only allows the second. + appendNeighborFileSnippets( + [ + makeNeighbor('file:///src/big.ts', 'b'.repeat(32), 0), + makeNeighbor('file:///src/small.ts', 'aa', 0), + ], + snippets, + docsInPrompt, + /*tokenBudget*/ 5, + computeTokens, + IncludeLineNumbersOption.None, + ); + expect(snippets).toHaveLength(1); + expect(snippets[0]).toContain('/src/small.ts'); + }); + + test('skips snippets whose document is already in the prompt', () => { + const dupDoc = DocumentId.create('file:///src/dup.ts'); + const docsInPrompt = new Set([dupDoc]); + const snippets: string[] = []; + appendNeighborFileSnippets( + [ + makeNeighbor('file:///src/dup.ts', 'duplicated', 0), + makeNeighbor('file:///src/fresh.ts', 'fresh', 0), + ], + snippets, + docsInPrompt, + /*tokenBudget*/ 100, + computeTokens, + IncludeLineNumbersOption.None, + ); + expect(snippets).toHaveLength(1); + expect(snippets[0]).toContain('/src/fresh.ts'); + }); + + test('no-op when no snippets provided', () => { + const snippets: string[] = []; + const docsInPrompt = new Set(); + const result = appendNeighborFileSnippets([], snippets, docsInPrompt, 100, computeTokens, IncludeLineNumbersOption.None); + expect(snippets).toEqual([]); + expect(result).toEqual({ nComputed: 0, nIncluded: 0, includedIndices: [] }); + }); + + test('selects highest-score snippets first when budget is tight', () => { + const snippets: string[] = []; + const docsInPrompt = new Set(); + // Each snippet ~= 8 tokens (32 chars / 4); budget 10 only fits one. + // The highest-score one (last in input) must win. + appendNeighborFileSnippets( + [ + makeNeighbor('file:///src/low.ts', 'l'.repeat(32), 0, /*score*/ 0.1), + makeNeighbor('file:///src/high.ts', 'h'.repeat(32), 0, /*score*/ 0.9), + ], + snippets, + docsInPrompt, + /*tokenBudget*/ 10, + computeTokens, + IncludeLineNumbersOption.None, + ); + expect(snippets).toHaveLength(1); + expect(snippets[0]).toContain('/src/high.ts'); + }); + + test('places highest-score snippet last (closest to current file)', () => { + const snippets: string[] = []; + const docsInPrompt = new Set(); + appendNeighborFileSnippets( + [ + makeNeighbor('file:///src/low.ts', 'low', 0, /*score*/ 0.1), + makeNeighbor('file:///src/mid.ts', 'mid', 0, /*score*/ 0.5), + makeNeighbor('file:///src/high.ts', 'high', 0, /*score*/ 0.9), + ], + snippets, + docsInPrompt, + /*tokenBudget*/ 100, + computeTokens, + IncludeLineNumbersOption.None, + ); + expect(snippets).toHaveLength(3); + expect(snippets[0]).toContain('/src/low.ts'); + expect(snippets[1]).toContain('/src/mid.ts'); + expect(snippets[2]).toContain('/src/high.ts'); + }); + + test('reports included indices, skipping oversize and budget-exhausted entries', () => { + // Indices: 0 (small low score), 1 (small low score), 2 (small low score), 3 (small mid), 4 (small mid), 5 (huge), 6 (small high) + // Budget allows ~4 small snippets (each ~1 token, big takes ~16 tokens, budget=4). + // Iteration order: 6, 5 (skipped — too big), 4, 3, 2, 1 (skipped — out of budget), 0 (skipped — out of budget). + const snippets: string[] = []; + const docsInPrompt = new Set(); + const result = appendNeighborFileSnippets( + [ + makeNeighbor('file:///src/i0.ts', 'a', 0, /*score*/ 0.1), + makeNeighbor('file:///src/i1.ts', 'b', 0, /*score*/ 0.2), + makeNeighbor('file:///src/i2.ts', 'c', 0, /*score*/ 0.3), + makeNeighbor('file:///src/i3.ts', 'd', 0, /*score*/ 0.4), + makeNeighbor('file:///src/i4.ts', 'e', 0, /*score*/ 0.5), + makeNeighbor('file:///src/i5.ts', 'h'.repeat(64), 0, /*score*/ 0.6), + makeNeighbor('file:///src/i6.ts', 'g', 0, /*score*/ 0.9), + ], + snippets, + docsInPrompt, + /*tokenBudget*/ 4, + computeTokens, + IncludeLineNumbersOption.None, + ); + expect(result).toEqual({ nComputed: 7, nIncluded: 4, includedIndices: [2, 3, 4, 6] }); + }); +}); diff --git a/extensions/copilot/src/lib/node/chatLibMain.ts b/extensions/copilot/src/lib/node/chatLibMain.ts index 4dd64f67f70b0..2ed011e55bc69 100644 --- a/extensions/copilot/src/lib/node/chatLibMain.ts +++ b/extensions/copilot/src/lib/node/chatLibMain.ts @@ -55,7 +55,7 @@ import { LlmNESTelemetryBuilder, NextEditProviderTelemetryBuilder, TelemetrySend import { INextEditResult } from '../../extension/inlineEdits/node/nextEditResult'; import { IPowerService, NullPowerService } from '../../extension/power/common/powerService'; import { ChatMLFetcherImpl } from '../../extension/prompt/node/chatMLFetcher'; -import { ISimilarFilesContextService } from '../../extension/xtab/common/similarFilesContextService'; +import { ISimilarFilesContextService, NullSimilarFilesContextService } from '../../extension/xtab/common/similarFilesContextService'; import { XtabProvider } from '../../extension/xtab/node/xtabProvider'; import { IAuthenticationService } from '../../platform/authentication/common/authentication'; import { ICopilotTokenManager } from '../../platform/authentication/common/copilotTokenManager'; @@ -104,8 +104,8 @@ import { resolveOTelConfig } from '../../platform/otel/common/otelConfig'; import { IOTelService } from '../../platform/otel/common/otelService'; import { IProxyModelsService } from '../../platform/proxyModels/common/proxyModelsService'; import { ProxyModelsService } from '../../platform/proxyModels/node/proxyModelsService'; -import { NullRequestLogger } from '../../platform/requestLogger/node/nullRequestLogger'; import { IRequestLogger } from '../../platform/requestLogger/common/requestLogger'; +import { NullRequestLogger } from '../../platform/requestLogger/node/nullRequestLogger'; import { ISimulationTestContext, NulSimulationTestContext } from '../../platform/simulationTestContext/common/simulationTestContext'; import { ISnippyService, NullSnippyService } from '../../platform/snippy/common/snippyService'; import { IExperimentationService, TreatmentsChangeEvent } from '../../platform/telemetry/common/nullExperimentationService'; @@ -484,14 +484,6 @@ class OverridableConfigurationService extends DefaultsOnlyConfigurationService { } } -class NullSimilarFilesContextService implements ISimilarFilesContextService { - declare readonly _serviceBrand: undefined; - - async compute(): Promise { - return undefined; - } -} - class NullEndpointProvider implements IEndpointProvider { declare readonly _serviceBrand: undefined; readonly onDidModelsRefresh = VsEvent.None; diff --git a/extensions/copilot/src/platform/configuration/common/configurationService.ts b/extensions/copilot/src/platform/configuration/common/configurationService.ts index e50b1be311b76..bf69080313428 100644 --- a/extensions/copilot/src/platform/configuration/common/configurationService.ts +++ b/extensions/copilot/src/platform/configuration/common/configurationService.ts @@ -832,6 +832,8 @@ export namespace ConfigKey { export const InlineEditsXtabEarlyCursorLineDivergenceCancellation = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.earlyCursorLineDivergenceCancellation', ConfigType.ExperimentBased, EarlyDivergenceCancellationMode.Off, EarlyDivergenceCancellationMode.VALIDATOR); export const InlineEditsXtabLanguageContextEnabled = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.languageContext.enabled', ConfigType.ExperimentBased, xtabPromptOptions.DEFAULT_OPTIONS.languageContext.enabled); export const InlineEditsXtabLanguageContextMaxTokens = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.languageContext.maxTokens', ConfigType.ExperimentBased, xtabPromptOptions.DEFAULT_OPTIONS.languageContext.maxTokens); + export const InlineEditsXtabIncludeNeighborFiles = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.neighborFiles.enabled', ConfigType.ExperimentBased, xtabPromptOptions.DEFAULT_OPTIONS.neighborFiles.enabled); + export const InlineEditsXtabNeighborFilesMaxTokens = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.neighborFiles.maxTokens', ConfigType.ExperimentBased, xtabPromptOptions.DEFAULT_OPTIONS.neighborFiles.maxTokens); export const InlineEditsXtabMaxMergeConflictLines = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.maxMergeConflictLines', ConfigType.ExperimentBased, undefined); export const InlineEditsXtabOnlyMergeConflictLines = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.onlyMergeConflictLines', ConfigType.ExperimentBased, false); export const InlineEditsXtabAggressivenessLevel = defineTeamInternalSetting('chat.advanced.inlineEdits.xtabProvider.aggressivenessLevel', ConfigType.ExperimentBased, undefined); diff --git a/extensions/copilot/src/platform/inlineEdits/common/dataTypes/xtabPromptOptions.ts b/extensions/copilot/src/platform/inlineEdits/common/dataTypes/xtabPromptOptions.ts index 0dee4e8cf4d98..c7811693883cd 100644 --- a/extensions/copilot/src/platform/inlineEdits/common/dataTypes/xtabPromptOptions.ts +++ b/extensions/copilot/src/platform/inlineEdits/common/dataTypes/xtabPromptOptions.ts @@ -51,6 +51,22 @@ export type LanguageContextOptions = { readonly traitPosition: 'before' | 'after'; }; +/** + * Options for including Completions-style neighbor file snippets (Jaccard-ranked) + * into the recently_viewed_code_snippets section of the prompt. + */ +export type NeighborFilesOptions = { + readonly enabled: boolean; + readonly maxTokens: number; +}; + +export namespace NeighborFilesOptions { + export const VALIDATOR: IValidator> = vObj({ + 'enabled': vBoolean(), + 'maxTokens': vNumber(), + }); +} + export type DiffHistoryOptions = { readonly nEntries: number; readonly maxTokens: number; @@ -238,6 +254,7 @@ export type PromptOptions = { readonly pagedClipping: PagedClipping; readonly recentlyViewedDocuments: RecentlyViewedDocumentsOptions; readonly languageContext: LanguageContextOptions; + readonly neighborFiles: NeighborFilesOptions; readonly diffHistory: DiffHistoryOptions; readonly includePostScript: boolean; readonly lintOptions: LintOptions | undefined; @@ -351,6 +368,10 @@ export const DEFAULT_OPTIONS: PromptOptions = { maxTokens: 2000, traitPosition: 'after', }, + neighborFiles: { + enabled: false, + maxTokens: 1000, + }, diffHistory: { nEntries: 25, maxTokens: 1000, diff --git a/extensions/copilot/src/platform/inlineEdits/common/statelessNextEditProvider.ts b/extensions/copilot/src/platform/inlineEdits/common/statelessNextEditProvider.ts index 3e27f7701fae3..f43503bab05f6 100644 --- a/extensions/copilot/src/platform/inlineEdits/common/statelessNextEditProvider.ts +++ b/extensions/copilot/src/platform/inlineEdits/common/statelessNextEditProvider.ts @@ -414,6 +414,12 @@ export interface IStatelessNextEditTelemetry { readonly nDiffsInPrompt: number | undefined; readonly diffTokensInPrompt: number | undefined; + /* neighbor (similar files) snippets info */ + readonly nNeighborSnippetsComputed: number | undefined; + readonly nNeighborSnippetsInPrompt: number | undefined; + /** JSON-encoded array of original input indices of snippets included in the prompt. */ + readonly neighborSnippetIndicesInPrompt: string | undefined; + /* lint errors info */ readonly lintErrors: string | undefined; @@ -506,6 +512,9 @@ export class StatelessNextEditTelemetryBuilder { cursorJumpResponse: this._cursorJumpResponse, nDiffsInPrompt: this._nDiffsInPrompt, diffTokensInPrompt: this._diffTokensInPrompt, + nNeighborSnippetsComputed: this._nNeighborSnippetsComputed, + nNeighborSnippetsInPrompt: this._nNeighborSnippetsInPrompt, + neighborSnippetIndicesInPrompt: this._neighborSnippetIndicesInPrompt, lintErrors: this._lintErrors, terminalOutput: this._terminalOutput, similarFilesContext: this._similarFilesContext, @@ -703,6 +712,24 @@ export class StatelessNextEditTelemetryBuilder { return this; } + private _nNeighborSnippetsComputed: number | undefined; + public setNNeighborSnippetsComputed(n: number): this { + this._nNeighborSnippetsComputed = n; + return this; + } + + private _nNeighborSnippetsInPrompt: number | undefined; + public setNNeighborSnippetsInPrompt(n: number): this { + this._nNeighborSnippetsInPrompt = n; + return this; + } + + private _neighborSnippetIndicesInPrompt: string | undefined; + public setNeighborSnippetIndicesInPrompt(indices: readonly number[]): this { + this._neighborSnippetIndicesInPrompt = JSON.stringify(indices); + return this; + } + private _lintErrors: string | undefined; public setLintErrors(lintErrors: string): this { this._lintErrors = lintErrors; From d603ad485d534b9debd2c27a3b6268c80bffee0d Mon Sep 17 00:00:00 2001 From: Bhavya U Date: Sun, 26 Apr 2026 09:57:52 -0700 Subject: [PATCH 21/25] Add 'last two messages' cache breakpoint strategy for Messages API (#312472) * Add 'last two messages' cache breakpoint strategy for Messages API Adds an experiment-gated alternative to the heuristic-based cache breakpoint placement. Places cache_control on the last two merged Anthropic messages instead of using prompt-tsx markers and addCacheBreakpoints. Gated behind AnthropicCacheBreakpointsLastTwoMessages (default false, onExp). * Address PR review: handle pre-marked tail messages in addLastTwoMessagesCacheControl * Remove 'advanced' tag from cacheBreakpoints.lastTwoMessages setting --- extensions/copilot/package.json | 9 + extensions/copilot/package.nls.json | 1 + .../src/extension/intents/node/agentIntent.ts | 11 +- .../common/configurationService.ts | 2 + .../src/platform/endpoint/node/messagesApi.ts | 117 ++++++- .../endpoint/test/node/messagesApi.spec.ts | 329 +++++++++++++++++- 6 files changed, 449 insertions(+), 20 deletions(-) diff --git a/extensions/copilot/package.json b/extensions/copilot/package.json index 79dd3b42ff176..f7a77f670bb02 100644 --- a/extensions/copilot/package.json +++ b/extensions/copilot/package.json @@ -3815,6 +3815,15 @@ "clear-both" ] }, + "github.copilot.chat.anthropic.cacheBreakpoints.lastTwoMessages": { + "type": "boolean", + "default": false, + "markdownDescription": "%github.copilot.config.anthropic.cacheBreakpoints.lastTwoMessages%", + "tags": [ + "experimental", + "onExp" + ] + }, "github.copilot.chat.responsesApiReasoningSummary": { "type": "string", "default": "detailed", diff --git a/extensions/copilot/package.nls.json b/extensions/copilot/package.nls.json index 5eb1a8178da2d..1c64b31759544 100644 --- a/extensions/copilot/package.nls.json +++ b/extensions/copilot/package.nls.json @@ -342,6 +342,7 @@ "copilot.toolSet.web.description": "Fetch information from the web", "github.copilot.config.useMessagesApi": "Use the Messages API instead of the Chat Completions API when supported.", "github.copilot.config.anthropic.contextEditing.mode": "Select the context editing mode for Anthropic models. Automatically manages conversation context as it grows, helping optimize costs and stay within context window limits.\n\n- `off`: Context editing is disabled.\n- `clear-thinking`: Clears thinking blocks while preserving tool uses.\n- `clear-tooluse`: Clears tool uses while preserving thinking blocks.\n- `clear-both`: Clears both thinking blocks and tool uses.\n\n**Note**: This is an experimental feature. Context editing may cause additional cache rewrites. Enable with caution.", + "github.copilot.config.anthropic.cacheBreakpoints.lastTwoMessages": "Use the 'last two messages' cache breakpoint strategy instead of heuristic-based placement for Anthropic Messages API.", "github.copilot.config.useResponsesApi": "Use the Responses API instead of the Chat Completions API when supported. Enables reasoning and reasoning summaries.\n\n**Note**: This is an experimental feature that is not yet activated for all users.\n\n**Important**: URL API path resolution for custom OpenAI-compatible and Azure models is independent of this setting and fully determined by `url` property of `#github.copilot.chat.customOAIModels#` or `#github.copilot.chat.azureModels#` respectively.", "github.copilot.config.responsesApiReasoningSummary": "Sets the reasoning summary style used for the Responses API. Requires `#github.copilot.chat.useResponsesApi#`.", "github.copilot.config.responsesApiContextManagement.enabled": "Enables context management for the Responses API. Requires `#github.copilot.chat.useResponsesApi#`.", diff --git a/extensions/copilot/src/extension/intents/node/agentIntent.ts b/extensions/copilot/src/extension/intents/node/agentIntent.ts index 853eb96833503..96f947cfb9938 100644 --- a/extensions/copilot/src/extension/intents/node/agentIntent.ts +++ b/extensions/copilot/src/extension/intents/node/agentIntent.ts @@ -448,6 +448,11 @@ export class AgentIntentInvocation extends EditCodeIntentInvocation implements I this.logService.debug(`[Agent] rendering with budget=${safeBudget} (baseBudget: ${baseBudget}, toolTokens: ${toolTokens}, totalTools: ${tools?.length ?? 0}, toolSearchEnabled: ${toolSearchEnabled}), summarizationEnabled=${summarizationEnabled}`); let result: RenderPromptResult; + // When the "last two messages" cache breakpoint strategy is enabled, + // suppress prompt-tsx and heuristic cache breakpoints — messagesApi.ts + // will place breakpoints on the last two merged messages instead. + const useLastTwoMessagesCacheBPs = isAnthropicFamily(this.endpoint) + && this.configurationService.getExperimentBasedConfig(ConfigKey.AnthropicCacheBreakpointsLastTwoMessages, this.expService); const props: AgentPromptProps = { endpoint, promptContext: { @@ -458,7 +463,7 @@ export class AgentIntentInvocation extends EditCodeIntentInvocation implements I } }, location: this.location, - enableCacheBreakpoints: summarizationEnabled, + enableCacheBreakpoints: summarizationEnabled && !useLastTwoMessagesCacheBPs, ...this.extraPromptProps, customizations: this._resolvedCustomizations }; @@ -722,7 +727,9 @@ export class AgentIntentInvocation extends EditCodeIntentInvocation implements I } } - addCacheBreakpoints(result.messages); + if (!useLastTwoMessagesCacheBPs) { + addCacheBreakpoints(result.messages); + } if (this.request.command === 'error') { // Should trigger a 400 diff --git a/extensions/copilot/src/platform/configuration/common/configurationService.ts b/extensions/copilot/src/platform/configuration/common/configurationService.ts index bf69080313428..7c7033a50da56 100644 --- a/extensions/copilot/src/platform/configuration/common/configurationService.ts +++ b/extensions/copilot/src/platform/configuration/common/configurationService.ts @@ -909,6 +909,8 @@ export namespace ConfigKey { /** Use the Messages API instead of Chat Completions when supported */ export const UseAnthropicMessagesApi = defineSetting('chat.anthropic.useMessagesApi', ConfigType.ExperimentBased, true); + /** Use "last two messages" cache breakpoint strategy instead of heuristic-based placement */ + export const AnthropicCacheBreakpointsLastTwoMessages = defineSetting('chat.anthropic.cacheBreakpoints.lastTwoMessages', ConfigType.ExperimentBased, false); /** Context editing mode for Anthropic Messages API. 'off' disables context editing. */ export const AnthropicContextEditingMode = defineSetting<'off' | 'clear-thinking' | 'clear-tooluse' | 'clear-both'>('chat.anthropic.contextEditing.mode', ConfigType.ExperimentBased, 'off'); /** Configure reasoning summary style sent to Responses API */ diff --git a/extensions/copilot/src/platform/endpoint/node/messagesApi.ts b/extensions/copilot/src/platform/endpoint/node/messagesApi.ts index fc3c6469535bb..5d7fb7613a75a 100644 --- a/extensions/copilot/src/platform/endpoint/node/messagesApi.ts +++ b/extensions/copilot/src/platform/endpoint/node/messagesApi.ts @@ -181,11 +181,25 @@ export function createMessagesRequestBody(accessor: ServicesAccessor, options: I const validToolNames = finalTools.length > 0 ? new Set(finalTools.map(t => t.name)) : undefined; const messagesResult = rawMessagesToMessagesAPI(options.messages, toolSearchEnabled ? validToolNames : undefined); + // "Last two messages" cache breakpoint strategy: place cache_control on the last + // two merged messages. This is gated behind an experiment and replaces the + // heuristic-based addCacheBreakpoints (which runs upstream in the prompt builder). + // Run before addToolsAndSystemCacheControl: shifting markers placed first, + // static markers fill the remainder. When the experiment is on we count slots + // once and thread the budget through both functions to avoid a redundant walk. + const useLastTwoMessages = configurationService.getExperimentBasedConfig(ConfigKey.AnthropicCacheBreakpointsLastTwoMessages, experimentationService); + let precomputedSlots: number | undefined; + if (useLastTwoMessages) { + precomputedSlots = maxCacheBreakpoints - countExistingMessageAndSystemCacheControl(messagesResult); + if (precomputedSlots > 0) { + precomputedSlots -= addLastTwoMessagesCacheControl(messagesResult, precomputedSlots); + } + } + // Add cache_control to the last tool and last system block so the stable tools+system // prefix is cached across turns. Per the Anthropic docs, cache prefixes are created in // order: tools → system → messages, and a max of 4 cache_control blocks is allowed. - // Count existing cache_control in messages+system first to stay within the limit. - addToolsAndSystemCacheControl(finalTools, messagesResult); + addToolsAndSystemCacheControl(finalTools, messagesResult, precomputedSlots); // Guard: The Anthropic Messages API requires the conversation to end with a user message. // A trailing assistant message is treated as a prefill request, which is not supported @@ -478,23 +492,16 @@ function contentBlockSupportsCacheControl(block: ContentBlockParam): block is Ex const maxCacheBreakpoints = 4; /** - * Optionally adds cache_control to the tools and system prefix when there are spare - * slots available (i.e. existing breakpoints < max). The last non-deferred tool is - * marked first if possible, and the last system block is marked only while slots remain. - * Message-level cache breakpoints are never evicted because they already implicitly - * cache the tools+system prefix (Anthropic cache hierarchy: tools → system → messages) - * and cover more content. + * Counts existing cache_control markers across system blocks and messages. + * Does not count tool-level cache_control — tools are managed separately by + * addToolsAndSystemCacheControl. */ -export function addToolsAndSystemCacheControl( - tools: AnthropicMessagesTool[], - messagesResult: { messages: MessageParam[]; system?: TextBlockParam[] }, -): void { - // Count existing cache_control in messages and system - let existingCount = 0; +function countExistingMessageAndSystemCacheControl(messagesResult: { messages: MessageParam[]; system?: TextBlockParam[] }): number { + let count = 0; if (messagesResult.system) { for (const block of messagesResult.system) { if (block.cache_control) { - existingCount++; + count++; } } } @@ -502,13 +509,30 @@ export function addToolsAndSystemCacheControl( if (Array.isArray(msg.content)) { for (const block of msg.content) { if (typeof block === 'object' && 'cache_control' in block && block.cache_control) { - existingCount++; + count++; } } } } + return count; +} - let slotsAvailable = maxCacheBreakpoints - existingCount; +/** + * Optionally adds cache_control to the tools and system prefix when there are spare + * slots available (i.e. existing breakpoints < max). The last non-deferred tool is + * marked first if possible, and the last system block is marked only while slots remain. + * Message-level cache breakpoints are never evicted because they already implicitly + * cache the tools+system prefix (Anthropic cache hierarchy: tools → system → messages) + * and cover more content. + */ +export function addToolsAndSystemCacheControl( + tools: AnthropicMessagesTool[], + messagesResult: { messages: MessageParam[]; system?: TextBlockParam[] }, + slotsAvailable?: number, +): void { + if (slotsAvailable === undefined) { + slotsAvailable = maxCacheBreakpoints - countExistingMessageAndSystemCacheControl(messagesResult); + } if (slotsAvailable <= 0) { return; } @@ -534,6 +558,65 @@ export function addToolsAndSystemCacheControl( } } +/** + * Adds cache_control to the last two distinct messages in the conversation. + * This implements a simpler "shifting breakpoint" strategy: the last two messages + * always carry cache breakpoints, which naturally advances as the conversation grows. + * Combined with addToolsAndSystemCacheControl (which handles tools + system), + * this gives 4 breakpoints: 2 static (tools/system) + 2 shifting (last two messages). + * + * If a trailing message already carries a cache_control marker, it counts toward the + * "two distinct messages" target and no additional marker is added — protecting the + * intent against any upstream code that may have placed markers before this runs. + * + * Returns the number of new cache_control markers added (0–2). + */ +export function addLastTwoMessagesCacheControl( + messagesResult: { messages: MessageParam[]; system?: TextBlockParam[] }, + slotsAvailable?: number, +): number { + if (slotsAvailable === undefined) { + slotsAvailable = maxCacheBreakpoints - countExistingMessageAndSystemCacheControl(messagesResult); + } + if (slotsAvailable <= 0) { + return 0; + } + + // Walk messages in reverse, marking the last cacheable content block of the + // last two distinct messages. A message that already has a cache_control + // marker counts toward the target without a new marker being added. + const messages = messagesResult.messages; + let markedCount = 0; + let added = 0; + for (let i = messages.length - 1; i >= 0 && slotsAvailable > 0 && markedCount < 2; i--) { + const msg = messages[i]; + if (!Array.isArray(msg.content) || msg.content.length === 0) { + continue; + } + + const alreadyMarked = msg.content.some(b => + typeof b === 'object' && 'cache_control' in b && b.cache_control + ); + if (alreadyMarked) { + markedCount++; + continue; + } + + // Find the last block in this message that supports cache_control + for (let j = msg.content.length - 1; j >= 0; j--) { + const block = msg.content[j]; + if (typeof block === 'object' && contentBlockSupportsCacheControl(block)) { + block.cache_control = { type: 'ephemeral' }; + slotsAvailable--; + markedCount++; + added++; + break; + } + } + } + return added; +} + export async function processResponseFromMessagesEndpoint( instantiationService: IInstantiationService, telemetryService: ITelemetryService, diff --git a/extensions/copilot/src/platform/endpoint/test/node/messagesApi.spec.ts b/extensions/copilot/src/platform/endpoint/test/node/messagesApi.spec.ts index 798481a0466d5..787d79bc37d40 100644 --- a/extensions/copilot/src/platform/endpoint/test/node/messagesApi.spec.ts +++ b/extensions/copilot/src/platform/endpoint/test/node/messagesApi.spec.ts @@ -15,7 +15,7 @@ import { AnthropicMessagesTool, CUSTOM_TOOL_SEARCH_NAME } from '../../../network import { IChatEndpoint, ICreateEndpointBodyOptions } from '../../../networking/common/networking'; import { IToolDeferralService } from '../../../networking/common/toolDeferralService'; import { createPlatformServices } from '../../../test/node/services'; -import { addToolsAndSystemCacheControl, buildToolInputSchema, createMessagesRequestBody, rawMessagesToMessagesAPI } from '../../node/messagesApi'; +import { addLastTwoMessagesCacheControl, addToolsAndSystemCacheControl, buildToolInputSchema, createMessagesRequestBody, rawMessagesToMessagesAPI } from '../../node/messagesApi'; function assertContentArray(content: MessageParam['content']): ContentBlockParam[] { expect(Array.isArray(content)).toBe(true); @@ -665,6 +665,333 @@ suite('buildToolInputSchema', function () { }); }); +suite('addLastTwoMessagesCacheControl', function () { + + function makeMessages(...msgs: MessageParam[]): MessageParam[] { + return msgs; + } + + function makeTool(name: string, deferred = false): AnthropicMessagesTool { + return { + name, + description: `${name} tool`, + input_schema: { type: 'object', properties: {}, required: [] }, + ...(deferred ? { defer_loading: true } : {}), + }; + } + + function getCacheControl(block: ContentBlockParam): { type: string } | undefined { + return 'cache_control' in block ? (block as { cache_control?: { type: string } }).cache_control : undefined; + } + + function countAllCacheControl(messages: MessageParam[], system?: TextBlockParam[]): number { + let count = 0; + if (system) { + for (const block of system) { + if (block.cache_control) { + count++; + } + } + } + for (const msg of messages) { + if (Array.isArray(msg.content)) { + for (const block of msg.content) { + if (typeof block === 'object' && 'cache_control' in block && block.cache_control) { + count++; + } + } + } + } + return count; + } + + test('marks last two messages in a normal agentic loop', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'edit my file' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'calling tool' }, { type: 'tool_use', id: 'toolu_1', name: 'read_file', input: {} }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'tool_result', tool_use_id: 'toolu_1', content: [{ type: 'text', text: 'file contents' }] }] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + const assistantContent = messages[1].content as ContentBlockParam[]; + expect(getCacheControl(assistantContent[assistantContent.length - 1])).toEqual({ type: 'ephemeral' }); + + const toolResult = (messages[2].content as ContentBlockParam[])[0] as ToolResultBlockParam; + expect(toolResult.cache_control).toEqual({ type: 'ephemeral' }); + + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toBeUndefined(); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('marks last two messages in plain chat', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'hello' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'hi there' }] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl((messages[1].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('handles single message', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'hello' }] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(countAllCacheControl(messages)).toBe(1); + }); + + test('handles empty messages array', function () { + const messagesResult = { messages: [] as MessageParam[] }; + + addLastTwoMessagesCacheControl(messagesResult); + + expect(messagesResult.messages).toHaveLength(0); + }); + + test('skips thinking and redacted_thinking blocks', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'hello' }] as ContentBlockParam[] }, + { + role: 'assistant', content: [ + { type: 'thinking', thinking: 'hmm', signature: 'sig' }, + { type: 'text', text: 'response' }, + ] as ContentBlockParam[] + }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + const assistantContent = messages[1].content as ContentBlockParam[]; + expect(getCacheControl(assistantContent[0])).toBeUndefined(); + expect(getCacheControl(assistantContent[1])).toEqual({ type: 'ephemeral' }); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('respects max breakpoint count when some already exist', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'a', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'b', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'text', text: 'c', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'd' }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'text', text: 'e' }] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + // 3 existing + 1 new = 4 total + expect(countAllCacheControl(messages)).toBe(4); + expect(getCacheControl((messages[4].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + // Second-to-last should NOT get one — would exceed 4 + expect(getCacheControl((messages[3].content as ContentBlockParam[])[0])).toBeUndefined(); + }); + + test('does nothing when all 4 slots are occupied', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'a', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'b', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'text', text: 'c', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'd', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'text', text: 'e' }] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + expect(getCacheControl((messages[4].content as ContentBlockParam[])[0])).toBeUndefined(); + expect(countAllCacheControl(messages)).toBe(4); + }); + + test('treats trailing message with existing cache_control as already marked', function () { + // Regression: prior code would walk past a pre-marked tail message and + // add two new markers to earlier messages, ending up with 3 distinct + // marked messages instead of 2. + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'a' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'b' }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'text', text: 'c' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'd', cache_control: { type: 'ephemeral' } }] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + const added = addLastTwoMessagesCacheControl(messagesResult); + + expect(added).toBe(1); + expect(getCacheControl((messages[3].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl((messages[2].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl((messages[1].content as ContentBlockParam[])[0])).toBeUndefined(); + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toBeUndefined(); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('does not add a second marker to a message that already has one on a non-last block', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'a' }] as ContentBlockParam[] }, + { + role: 'assistant', content: [ + { type: 'text', text: 'first', cache_control: { type: 'ephemeral' } }, + { type: 'text', text: 'second' }, + ] as ContentBlockParam[] + }, + ); + const messagesResult = { messages }; + + const added = addLastTwoMessagesCacheControl(messagesResult); + + // Last message already counts as marked; only the prior message gets a new marker. + expect(added).toBe(1); + const assistantContent = messages[1].content as ContentBlockParam[]; + expect(getCacheControl(assistantContent[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl(assistantContent[1])).toBeUndefined(); + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('marks assistant-with-tool-calls as fork point', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'do stuff' }] as ContentBlockParam[] }, + { + role: 'assistant', content: [ + { type: 'text', text: 'I will call tools' }, + { type: 'tool_use', id: 'toolu_a', name: 'tool_a', input: {} }, + { type: 'tool_use', id: 'toolu_b', name: 'tool_b', input: {} }, + ] as ContentBlockParam[] + }, + { + role: 'user', content: [ + { type: 'tool_result', tool_use_id: 'toolu_a', content: [{ type: 'text', text: 'result a' }] }, + { type: 'tool_result', tool_use_id: 'toolu_b', content: [{ type: 'text', text: 'result b' }] }, + ] as ContentBlockParam[] + }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + const assistantContent = messages[1].content as ContentBlockParam[]; + expect(getCacheControl(assistantContent[2])).toEqual({ type: 'ephemeral' }); + + const userContent = messages[2].content as ContentBlockParam[]; + expect(getCacheControl(userContent[1])).toEqual({ type: 'ephemeral' }); + + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('counts system block breakpoints toward the limit', function () { + const system: TextBlockParam[] = [ + { type: 'text', text: 'system', cache_control: { type: 'ephemeral' } }, + ]; + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'a' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'b' }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'text', text: 'c' }] as ContentBlockParam[] }, + ); + const messagesResult = { messages, system }; + + addLastTwoMessagesCacheControl(messagesResult); + + // 1 system + 2 message breakpoints = 3 total + expect(countAllCacheControl(messages, system)).toBe(3); + expect(getCacheControl((messages[1].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl((messages[2].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + }); + + test('skips tail message with empty content and marks two prior', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'hello' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'response' }] as ContentBlockParam[] }, + { role: 'user', content: [] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl((messages[1].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('skips thinking-only tail message and marks two prior', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'hello' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'first response' }] as ContentBlockParam[] }, + { + role: 'assistant', content: [ + { type: 'thinking', thinking: 'deep thought', signature: 'sig' }, + { type: 'redacted_thinking', data: 'redacted' }, + ] as ContentBlockParam[] + }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + // Thinking-only message has no cacheable blocks — skip it + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl((messages[1].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('skips empty middle message and still finds two cacheable', function () { + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'hello' }] as ContentBlockParam[] }, + { role: 'assistant', content: [] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'text', text: 'follow up' }] as ContentBlockParam[] }, + ); + const messagesResult = { messages }; + + addLastTwoMessagesCacheControl(messagesResult); + + // Last message + first message (middle is empty, skipped) + expect(getCacheControl((messages[2].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(getCacheControl((messages[0].content as ContentBlockParam[])[0])).toEqual({ type: 'ephemeral' }); + expect(countAllCacheControl(messages)).toBe(2); + }); + + test('round-trip with addToolsAndSystemCacheControl produces exactly 4 markers', function () { + const tools = [makeTool('read_file'), makeTool('edit_file')]; + const system: TextBlockParam[] = [{ type: 'text', text: 'You are a helpful assistant.' }]; + const messages = makeMessages( + { role: 'user', content: [{ type: 'text', text: 'edit my file' }] as ContentBlockParam[] }, + { role: 'assistant', content: [{ type: 'text', text: 'calling tool' }, { type: 'tool_use', id: 'toolu_1', name: 'read_file', input: {} }] as ContentBlockParam[] }, + { role: 'user', content: [{ type: 'tool_result', tool_use_id: 'toolu_1', content: [{ type: 'text', text: 'file contents' }] }] as ContentBlockParam[] }, + ); + const messagesResult = { messages, system }; + + // Call both in the same order as createMessagesRequestBody + addLastTwoMessagesCacheControl(messagesResult); + addToolsAndSystemCacheControl(tools, messagesResult); + + // 2 message breakpoints + 1 tool + 1 system = 4 + let totalCount = countAllCacheControl(messages, system); + for (const tool of tools) { + if (tool.cache_control) { + totalCount++; + } + } + expect(totalCount).toBe(4); + + // Verify positions + const assistantContent = messages[1].content as ContentBlockParam[]; + expect(getCacheControl(assistantContent[assistantContent.length - 1])).toEqual({ type: 'ephemeral' }); + expect(((messages[2].content as ContentBlockParam[])[0] as ToolResultBlockParam).cache_control).toEqual({ type: 'ephemeral' }); + expect(tools[1].cache_control).toEqual({ type: 'ephemeral' }); + expect(system[0].cache_control).toEqual({ type: 'ephemeral' }); + }); +}); + describe('createMessagesRequestBody reasoning effort', () => { let disposables: DisposableStore; let instantiationService: IInstantiationService; From bccdfa8bb564789ef5764dac24240d8b72d60287 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Sun, 26 Apr 2026 11:33:33 -0700 Subject: [PATCH 22/25] copilot agent type cleanup (#312611) --- .../node/copilot/copilotAgentSession.ts | 43 ++++++++----------- .../node/copilot/copilotPluginConverters.ts | 22 ++++++---- .../test/node/copilotAgentSession.test.ts | 19 ++++++-- 3 files changed, 47 insertions(+), 37 deletions(-) diff --git a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts index 23cc1ce9131e4..4555cb5bf7ac2 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotAgentSession.ts @@ -32,6 +32,13 @@ import { buildPendingEditContentUri } from './pendingEditContentStore.js'; const COPILOT_HOME_DIRECTORY = '.copilot'; const SESSION_STATE_DIRECTORY = join(COPILOT_HOME_DIRECTORY, 'session-state'); +type UserInputHandler = NonNullable; +type UserInputRequest = Parameters[0]; +type UserInputResponse = Awaited>; +type SessionHooks = NonNullable; +type PreToolUseHookInput = Parameters>[0]; +type PostToolUseHookInput = Parameters>[0]; + function getCopilotCLISessionStateDir(userHome: string): string { const xdgHome = process.env['XDG_STATE_HOME']; return xdgHome ? join(xdgHome, SESSION_STATE_DIRECTORY) : join(userHome, SESSION_STATE_DIRECTORY); @@ -58,28 +65,15 @@ export interface IActiveClientSnapshot { */ export type SessionWrapperFactory = (callbacks: { readonly onPermissionRequest: (request: ITypedPermissionRequest) => Promise; - readonly onUserInputRequest: (request: IUserInputRequest, invocation: { sessionId: string }) => Promise; + readonly onUserInputRequest: (request: UserInputRequest, invocation: { sessionId: string }) => Promise; readonly hooks: { - readonly onPreToolUse: (input: { toolName: string; toolArgs: unknown }) => Promise; - readonly onPostToolUse: (input: { toolName: string; toolArgs: unknown }) => Promise; + readonly onPreToolUse: (input: PreToolUseHookInput) => Promise; + readonly onPostToolUse: (input: PostToolUseHookInput) => Promise; }; // eslint-disable-next-line @typescript-eslint/no-explicit-any readonly clientTools: Tool[]; }) => Promise; -/** Matches the SDK's `UserInputRequest` which is not re-exported from the package entry point. */ -interface IUserInputRequest { - question: string; - choices?: string[]; - allowFreeform?: boolean; -} - -/** Matches the SDK's `UserInputResponse` which is not re-exported from the package entry point. */ -interface IUserInputResponse { - answer: string; - wasFreeform: boolean; -} - /** * Options for constructing a {@link CopilotAgentSession}. */ @@ -245,15 +239,14 @@ export class CopilotAgentSession extends Disposable { } const textContent = result.content - ?.filter(c => c.type === 'text') - .map(c => (c as { text: string }).text) + ?.filter(c => c.type === ToolResultContentType.Text) + .map(c => c.text) .join('\n') ?? ''; const binaryResults = result.content - ?.filter(c => c.type === 'embeddedResource') + ?.filter(c => c.type === ToolResultContentType.EmbeddedResource) .map(c => { - const embedded = c as { data: string; contentType: string }; - return { data: embedded.data, mimeType: embedded.contentType, type: embedded.contentType }; + return { data: c.data, mimeType: c.contentType, type: c.contentType }; }); if (result.success) { @@ -537,9 +530,9 @@ export class CopilotAgentSession extends Disposable { * respond via {@link respondToUserInputRequest}. */ async handleUserInputRequest( - request: IUserInputRequest, + request: UserInputRequest, _invocation: { sessionId: string }, - ): Promise { + ): Promise { const questionPreview = request.question.substring(0, 100); try { const requestId = generateUuid(); @@ -615,7 +608,7 @@ export class CopilotAgentSession extends Disposable { return false; } - private async _handlePreToolUse(input: { toolName: string; toolArgs: unknown }): Promise { + private async _handlePreToolUse(input: PreToolUseHookInput): Promise { try { if (isEditTool(input.toolName)) { const filePath = getEditFilePath(input.toolArgs); @@ -629,7 +622,7 @@ export class CopilotAgentSession extends Disposable { } } - private async _handlePostToolUse(input: { toolName: string; toolArgs: unknown }): Promise { + private async _handlePostToolUse(input: PostToolUseHookInput): Promise { try { if (isEditTool(input.toolName)) { const filePath = getEditFilePath(input.toolArgs); diff --git a/src/vs/platform/agentHost/node/copilot/copilotPluginConverters.ts b/src/vs/platform/agentHost/node/copilot/copilotPluginConverters.ts index 4be688fba030d..61e93e5a6c1e3 100644 --- a/src/vs/platform/agentHost/node/copilot/copilotPluginConverters.ts +++ b/src/vs/platform/agentHost/node/copilot/copilotPluginConverters.ts @@ -12,6 +12,12 @@ import type { IMcpServerDefinition, INamedPluginResource, IParsedHookCommand, IP import { dirname } from '../../../../base/common/path.js'; type SessionHooks = NonNullable; +type PreToolUseHookInput = Parameters>[0]; +type PostToolUseHookInput = Parameters>[0]; +type UserPromptSubmittedHookInput = Parameters>[0]; +type SessionStartHookInput = Parameters>[0]; +type SessionEndHookInput = Parameters>[0]; +type ErrorOccurredHookInput = Parameters>[0]; // --------------------------------------------------------------------------- // MCP servers @@ -226,8 +232,8 @@ const HOOK_TYPE_TO_SDK_KEY: Record = { export function toSdkHooks( hookGroups: readonly IParsedHookGroup[], editTrackingHooks?: { - readonly onPreToolUse: (input: { toolName: string; toolArgs: unknown }) => Promise; - readonly onPostToolUse: (input: { toolName: string; toolArgs: unknown }) => Promise; + readonly onPreToolUse: (input: PreToolUseHookInput) => Promise; + readonly onPostToolUse: (input: PostToolUseHookInput) => Promise; }, ): SessionHooks { // Group all commands by SDK handler key @@ -247,7 +253,7 @@ export function toSdkHooks( // Pre-tool-use handler const preToolCommands = commandsByKey.get('onPreToolUse'); if (preToolCommands?.length || editTrackingHooks) { - hooks.onPreToolUse = async (input: { toolName: string; toolArgs: unknown }) => { + hooks.onPreToolUse = async (input: PreToolUseHookInput) => { await editTrackingHooks?.onPreToolUse(input); return runHookCommands(preToolCommands, input); }; @@ -256,7 +262,7 @@ export function toSdkHooks( // Post-tool-use handler const postToolCommands = commandsByKey.get('onPostToolUse'); if (postToolCommands?.length || editTrackingHooks) { - hooks.onPostToolUse = async (input: { toolName: string; toolArgs: unknown }) => { + hooks.onPostToolUse = async (input: PostToolUseHookInput) => { await editTrackingHooks?.onPostToolUse(input); return runHookCommands(postToolCommands, input); }; @@ -265,7 +271,7 @@ export function toSdkHooks( // User-prompt-submitted handler const promptCommands = commandsByKey.get('onUserPromptSubmitted'); if (promptCommands?.length) { - hooks.onUserPromptSubmitted = async (input: { prompt: string }) => { + hooks.onUserPromptSubmitted = async (input: UserPromptSubmittedHookInput) => { const stdin = JSON.stringify(input); for (const cmd of promptCommands) { try { @@ -280,7 +286,7 @@ export function toSdkHooks( // Session-start handler const startCommands = commandsByKey.get('onSessionStart'); if (startCommands?.length) { - hooks.onSessionStart = async (input: { source: string }) => { + hooks.onSessionStart = async (input: SessionStartHookInput) => { const stdin = JSON.stringify(input); for (const cmd of startCommands) { try { @@ -295,7 +301,7 @@ export function toSdkHooks( // Session-end handler const endCommands = commandsByKey.get('onSessionEnd'); if (endCommands?.length) { - hooks.onSessionEnd = async (input: { reason: string }) => { + hooks.onSessionEnd = async (input: SessionEndHookInput) => { const stdin = JSON.stringify(input); for (const cmd of endCommands) { try { @@ -310,7 +316,7 @@ export function toSdkHooks( // Error-occurred handler const errorCommands = commandsByKey.get('onErrorOccurred'); if (errorCommands?.length) { - hooks.onErrorOccurred = async (input: { error: string }) => { + hooks.onErrorOccurred = async (input: ErrorOccurredHookInput) => { const stdin = JSON.stringify(input); for (const cmd of errorCommands) { try { diff --git a/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts b/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts index a59fafd93268f..93b8331262de3 100644 --- a/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts +++ b/src/vs/platform/agentHost/test/node/copilotAgentSession.test.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. See License.txt in the project root for license information. *--------------------------------------------------------------------------------------------*/ -import type { CopilotSession, SessionEvent, SessionEventPayload, SessionEventType, ToolResultObject, TypedSessionEventHandler } from '@github/copilot-sdk'; +import type { CopilotSession, SessionEvent, SessionEventPayload, SessionEventType, Tool, ToolResultObject, TypedSessionEventHandler } from '@github/copilot-sdk'; import assert from 'assert'; import { DeferredPromise } from '../../../../base/common/async.js'; import { Emitter } from '../../../../base/common/event.js'; @@ -83,7 +83,7 @@ class CapturingLogService extends NullLogService { * {@link ToolResultObject} — which is what {@link CopilotAgentSession}'s * handler implementation actually returns. */ -function invokeClientToolHandler(tool: { name: string; handler: (args: any, invocation: any) => unknown }, toolCallId: string): Promise { +function invokeClientToolHandler(tool: Pick, toolCallId: string): Promise { return Promise.resolve(tool.handler({}, { sessionId: 'test-session-1', toolCallId, @@ -882,7 +882,12 @@ suite('CopilotAgentSession', () => { }; await assert.rejects( - capturedCallbacks.current!.hooks.onPreToolUse({ toolName: 'edit', toolArgs: { path: '/tmp/file.ts' } }), + capturedCallbacks.current!.hooks.onPreToolUse({ + timestamp: 0, + cwd: '/tmp', + toolName: 'edit', + toolArgs: { path: '/tmp/file.ts' }, + }), /pre tool boom/, ); @@ -903,7 +908,13 @@ suite('CopilotAgentSession', () => { }; await assert.rejects( - capturedCallbacks.current!.hooks.onPostToolUse({ toolName: 'edit', toolArgs: { path: '/tmp/file.ts' } }), + capturedCallbacks.current!.hooks.onPostToolUse({ + timestamp: 0, + cwd: '/tmp', + toolName: 'edit', + toolArgs: { path: '/tmp/file.ts' }, + toolResult: { textResultForLlm: '', resultType: 'success' }, + }), /post tool boom/, ); From fe0c770380f783e49ffb6bff6ae30c73f424ab56 Mon Sep 17 00:00:00 2001 From: Martin Aeschlimann Date: Sun, 26 Apr 2026 21:42:09 +0200 Subject: [PATCH 23/25] promptValidator: log error (#311899) Co-authored-by: Copilot --- .../common/promptSyntax/languageProviders/promptValidator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/vs/workbench/contrib/chat/common/promptSyntax/languageProviders/promptValidator.ts b/src/vs/workbench/contrib/chat/common/promptSyntax/languageProviders/promptValidator.ts index f44449e2ddc66..e803859df9d5d 100644 --- a/src/vs/workbench/contrib/chat/common/promptSyntax/languageProviders/promptValidator.ts +++ b/src/vs/workbench/contrib/chat/common/promptSyntax/languageProviders/promptValidator.ts @@ -172,8 +172,8 @@ export class PromptValidator { const loc = this.labelService.getUriLabel(resolved); report(toMarker(localize('promptValidator.fileNotFound', "File '{0}' not found at '{1}'.", ref.content, loc), ref.range, MarkerSeverity.Warning)); } - } catch { - this.logger.warn(`Error checking existence of file reference '${ref.content}' resolved to '${resolved.toString()}' in prompt file '${promptAST.uri.toString()}'`); + } catch (e) { + this.logger.warn(`Error checking existence of file reference '${ref.content}' resolved to '${resolved.toString()}' in prompt file '${promptAST.uri.toString()}': ${e.message}`); } })()); } From 1d8e3cbec61558b318734e60b42c1d908a30253e Mon Sep 17 00:00:00 2001 From: Justin Chen <54879025+justschen@users.noreply.github.com> Date: Sun, 26 Apr 2026 13:36:33 -0700 Subject: [PATCH 24/25] round trip permission levels to extension (#312479) * round trip permission levels to extension * address some comments --- .../claudeChatSessionContentProvider.ts | 10 +++ .../copilotCLIChatSessionsContribution.ts | 43 ++++++++++- .../claudeChatSessionContentProvider.spec.ts | 14 ++++ .../copilotCLIChatSessionParticipant.spec.ts | 74 ++++++++++++++++++- .../browser/claudePermissionModePicker.ts | 13 +++- .../browser/permissionPicker.ts | 15 +++- .../browser/widget/input/chatInputPart.ts | 5 ++ 7 files changed, 167 insertions(+), 7 deletions(-) diff --git a/extensions/copilot/src/extension/chatSessions/vscode-node/claudeChatSessionContentProvider.ts b/extensions/copilot/src/extension/chatSessions/vscode-node/claudeChatSessionContentProvider.ts index 4cd6a9c44e2f8..15e13c50c8e92 100644 --- a/extensions/copilot/src/extension/chatSessions/vscode-node/claudeChatSessionContentProvider.ts +++ b/extensions/copilot/src/extension/chatSessions/vscode-node/claudeChatSessionContentProvider.ts @@ -89,6 +89,16 @@ export class ClaudeChatSessionContentProvider extends Disposable implements vsco // #region Chat Participant Handler + provideHandleOptionsChange(resource: vscode.Uri, updates: ReadonlyArray, _token: vscode.CancellationToken): void { + const sessionId = ClaudeSessionUri.getSessionId(resource); + for (const update of updates) { + const value = update.value; + if (update.optionId === PERMISSION_MODE_OPTION_ID && value && isPermissionMode(value)) { + this.sessionStateService.setPermissionModeForSession(sessionId, value); + } + } + } + createHandler(): ChatExtendedRequestHandler { return async (request: vscode.ChatRequest, context: vscode.ChatContext, stream: vscode.ChatResponseStream, token: vscode.CancellationToken): Promise => { const { chatSessionContext } = context; diff --git a/extensions/copilot/src/extension/chatSessions/vscode-node/copilotCLIChatSessionsContribution.ts b/extensions/copilot/src/extension/chatSessions/vscode-node/copilotCLIChatSessionsContribution.ts index 4c7d156c7e8b9..e650294489cb8 100644 --- a/extensions/copilot/src/extension/chatSessions/vscode-node/copilotCLIChatSessionsContribution.ts +++ b/extensions/copilot/src/extension/chatSessions/vscode-node/copilotCLIChatSessionsContribution.ts @@ -58,6 +58,7 @@ import { convertReferenceToVariable } from '../copilotcli/vscode-node/copilotCLI import { clearChangesCacheForAffectedSessions } from './chatSessionRepositoryTracker'; const REPOSITORY_OPTION_ID = 'repository'; +const PERMISSION_LEVEL_OPTION_ID = 'permissionLevel'; const _sessionWorktreeIsolationCache = new Map(); const BRANCH_OPTION_ID = 'branch'; @@ -578,6 +579,7 @@ export class CopilotCLIChatSessionContentProvider extends Disposable implements private _currentSessionId: string | undefined; private _selectedRepoForBranches: { repoUri: URI; headBranchName: string | undefined } | undefined; private _displayedOptionIds = new Set(); + private readonly _activeSessionsById = new Map(); /** * ID of the last used folder in an untitled workspace (for defaulting selection). */ @@ -1076,7 +1078,10 @@ export class CopilotCLIChatSessionContentProvider extends Disposable implements const wasBranchOptionShow = !!this._selectedRepoForBranches; let triggerProviderOptionsChange = false; for (const update of updates) { - if (update.optionId === REPOSITORY_OPTION_ID && typeof update.value === 'string' && this.sessionItemProvider.isNewSession(sessionId)) { + if (update.optionId === PERMISSION_LEVEL_OPTION_ID) { + const level = typeof update.value === 'string' ? update.value : undefined; + this._getActiveSessionForResourceId(sessionId)?.setPermissionLevel(level); + } else if (update.optionId === REPOSITORY_OPTION_ID && typeof update.value === 'string' && this.sessionItemProvider.isNewSession(sessionId)) { const folder = vscode.Uri.file(update.value); if (isEqual(folder, this._selectedRepoForBranches?.repoUri)) { continue; @@ -1184,6 +1189,29 @@ export class CopilotCLIChatSessionContentProvider extends Disposable implements } } + private _getActiveSessionForResourceId(sessionId: string): ICopilotCLISession | undefined { + return this._activeSessionsById.get(this.sessionItemProvider.untitledSessionIdMapping.get(sessionId) ?? sessionId) + ?? this._activeSessionsById.get(sessionId); + } + + trackActiveSession(resourceSessionId: string, session: ICopilotCLISession): void { + this._activeSessionsById.set(resourceSessionId, session); + this._activeSessionsById.set(session.sessionId, session); + } + + untrackActiveSession(resourceSessionId: string | undefined, session: ICopilotCLISession | undefined, hasPendingRequests: boolean): void { + if (!session || hasPendingRequests) { + return; + } + + if (resourceSessionId && this._activeSessionsById.get(resourceSessionId) === session) { + this._activeSessionsById.delete(resourceSessionId); + } + if (this._activeSessionsById.get(session.sessionId) === session) { + this._activeSessionsById.delete(session.sessionId); + } + } + } function toRepositoryOptionItem(repository: RepoContext | Uri, isDefault: boolean = false): ChatSessionProviderOptionItem { @@ -1348,7 +1376,9 @@ export class CopilotCLIChatSessionParticipant extends Disposable { const disposables = new DisposableStore(); let sessionId: string | undefined = undefined; let sessionParentId: string | undefined = undefined; + let sessionPermissionLevel: string | undefined = undefined; let sdkSessionId: string | undefined = undefined; + let activeSession: ICopilotCLISession | undefined; try { const initialOptions = chatSessionContext?.initialSessionOptions; @@ -1365,6 +1395,8 @@ export class CopilotCLIChatSessionParticipant extends Disposable { _sessionBranch.set(sessionId, value); } else if (opt.optionId === ISOLATION_OPTION_ID && value) { _sessionIsolation.set(sessionId, value as IsolationMode); + } else if (opt.optionId === PERMISSION_LEVEL_OPTION_ID && value) { + sessionPermissionLevel = value; } else if (opt.optionId === PARENT_SESSION_OPTION_ID && value) { sessionParentId = value; } @@ -1453,7 +1485,7 @@ export class CopilotCLIChatSessionParticipant extends Disposable { }; const newBranch = (isUntitled && request.prompt && this.branchNameGenerator) ? this.branchNameGenerator.generateBranchName(fakeContext, token) : undefined; - const sessionResult = await this.getOrCreateSession(request, chatSessionContext, stream, { model, agent, newBranch, sessionParentId }, disposables, token); + const sessionResult = await this.getOrCreateSession(request, chatSessionContext, stream, { model, agent, newBranch, sessionParentId, permissionLevel: sessionPermissionLevel }, disposables, token); const session = sessionResult.session; if (session) { disposables.add(session); @@ -1472,6 +1504,8 @@ export class CopilotCLIChatSessionParticipant extends Disposable { } sdkSessionId = session.object.sessionId; + activeSession = session.object; + this.contentProvider.trackActiveSession(sessionId, activeSession); const modeInstructions = this.createModeInstructions(request); this.chatSessionMetadataStore.updateRequestDetails(sessionId, [{ vscodeRequestId: request.id, agentId: agent?.name ?? '', modeInstructions }]).catch(ex => this.logService.error(ex, 'Failed to update request details')); @@ -1565,6 +1599,7 @@ export class CopilotCLIChatSessionParticipant extends Disposable { } } } + this.contentProvider.untrackActiveSession(sessionId, activeSession, sdkSessionId ? this.pendingRequestBySession.has(sdkSessionId) : false); if (chatSessionContext?.chatSessionItem.resource) { this.sessionItemProvider.notifySessionsChange(); } @@ -1831,7 +1866,7 @@ export class CopilotCLIChatSessionParticipant extends Disposable { } } - private async getOrCreateSession(request: vscode.ChatRequest, chatSessionContext: vscode.ChatSessionContext, stream: vscode.ChatResponseStream, options: { model: { model: string; reasoningEffort?: string } | undefined; agent: SweCustomAgent | undefined; newBranch?: Promise; sessionParentId?: string }, disposables: DisposableStore, token: vscode.CancellationToken): Promise<{ session: IReference | undefined; trusted: boolean }> { + private async getOrCreateSession(request: vscode.ChatRequest, chatSessionContext: vscode.ChatSessionContext, stream: vscode.ChatResponseStream, options: { model: { model: string; reasoningEffort?: string } | undefined; agent: SweCustomAgent | undefined; newBranch?: Promise; sessionParentId?: string; permissionLevel?: string }, disposables: DisposableStore, token: vscode.CancellationToken): Promise<{ session: IReference | undefined; trusted: boolean }> { const { resource } = chatSessionContext.chatSessionItem; const existingSessionId = this.sessionItemProvider.untitledSessionIdMapping.get(SessionIdForCLI.parse(resource)); const id = existingSessionId ?? SessionIdForCLI.parse(resource); @@ -1872,7 +1907,7 @@ export class CopilotCLIChatSessionParticipant extends Disposable { void this.workspaceFolderService.trackSessionWorkspaceFolder(session.object.sessionId, sessionWorkingDirectory.fsPath, session.object.workspace.repositoryProperties); } disposables.add(session.object.attachStream(stream)); - const permissionLevel = request.permissionLevel; + const permissionLevel = request.permissionLevel ?? options.permissionLevel; session.object.setPermissionLevel(permissionLevel); return { session, trusted }; diff --git a/extensions/copilot/src/extension/chatSessions/vscode-node/test/claudeChatSessionContentProvider.spec.ts b/extensions/copilot/src/extension/chatSessions/vscode-node/test/claudeChatSessionContentProvider.spec.ts index 84e648b0121bc..f3afe3ac0d893 100644 --- a/extensions/copilot/src/extension/chatSessions/vscode-node/test/claudeChatSessionContentProvider.spec.ts +++ b/extensions/copilot/src/extension/chatSessions/vscode-node/test/claudeChatSessionContentProvider.spec.ts @@ -1104,6 +1104,20 @@ describe('ChatSessionContentProvider', () => { expect(getGroup(state, 'permissionMode')!.selected?.id).toBe('default'); }); + it('live permission option changes update session state', async () => { + const mocks = createDefaultMocks(); + const { provider, accessor: localAccessor } = createProviderWithServices(store, [workspaceFolderUri], mocks); + const sessionStateService = localAccessor.get(IClaudeSessionStateService); + const setPermissionSpy = vi.spyOn(sessionStateService, 'setPermissionModeForSession'); + + provider.provideHandleOptionsChange(createClaudeSessionUri('live-session'), [ + { optionId: 'permissionMode', value: 'plan' } + ], CancellationToken.None); + + expect(setPermissionSpy).toHaveBeenCalledWith('live-session', 'plan'); + expect(sessionStateService.getPermissionModeForSession('live-session')).toBe('plan'); + }); + it('external permission change syncs into a previousInputState-restored pipeline', async () => { const mocks = createDefaultMocks(); const { accessor: localAccessor } = createProviderWithServices(store, [workspaceFolderUri], mocks); diff --git a/extensions/copilot/src/extension/chatSessions/vscode-node/test/copilotCLIChatSessionParticipant.spec.ts b/extensions/copilot/src/extension/chatSessions/vscode-node/test/copilotCLIChatSessionParticipant.spec.ts index 24f16b7055438..5dc299b1ee599 100644 --- a/extensions/copilot/src/extension/chatSessions/vscode-node/test/copilotCLIChatSessionParticipant.spec.ts +++ b/extensions/copilot/src/extension/chatSessions/vscode-node/test/copilotCLIChatSessionParticipant.spec.ts @@ -47,7 +47,7 @@ import { getWorkingDirectory, IWorkspaceInfo } from '../../common/workspaceInfo' import { IChatDelegationSummaryService } from '../../copilotcli/common/delegationSummaryService'; import { type CopilotCLIModelInfo, type ICopilotCLIModels, type ICopilotCLISDK } from '../../copilotcli/node/copilotCli'; import { CopilotCLIPromptResolver } from '../../copilotcli/node/copilotcliPromptResolver'; -import { CopilotCLISession, CopilotCLISessionInput } from '../../copilotcli/node/copilotcliSession'; +import { CopilotCLISession, CopilotCLISessionInput, ICopilotCLISession } from '../../copilotcli/node/copilotcliSession'; import { CopilotCLISessionService, CopilotCLISessionWorkspaceTracker, ICopilotCLISessionService } from '../../copilotcli/node/copilotcliSessionService'; import { ICopilotCLIMCPHandler } from '../../copilotcli/node/mcpHandler'; import { MockCliSdkSession, MockCliSdkSessionManager, MockSkillLocations, NullCopilotCLIAgents, NullICopilotCLIImageSupport } from '../../copilotcli/node/test/testHelpers'; @@ -250,6 +250,7 @@ function createChatContext(sessionId: string, isUntitled: boolean, ...requests: class TestCopilotCLISession extends CopilotCLISession { public requests: Array<{ input: CopilotCLISessionInput; attachments: Attachment[]; model: { model: string; reasoningEffort?: string } | undefined; authInfo: NonNullable; token: vscode.CancellationToken }> = []; + public permissionLevel: string | undefined; public static nextHandleRequestResult: Promise | undefined; public static handleRequestHook: ((request: { id: string; toolInvocationToken: vscode.ChatParticipantToolToken; sessionResource?: vscode.Uri }, input: CopilotCLISessionInput) => Promise) | undefined; public static statusOverride?: vscode.ChatSessionStatus; @@ -263,6 +264,10 @@ class TestCopilotCLISession extends CopilotCLISession { } return TestCopilotCLISession.nextHandleRequestResult ?? Promise.resolve(); } + override setPermissionLevel(level: string | undefined): void { + this.permissionLevel = level; + super.setPermissionLevel(level); + } } @@ -405,6 +410,8 @@ describe('CopilotCLIChatSessionParticipant.handleRequest', () => { override notifySessionOptionsChange = vi.fn((_resource: vscode.Uri, _updates: ReadonlyArray<{ optionId: string; value: string | vscode.ChatSessionProviderOptionItem }>): void => { // tracked by vi.fn }); + override trackActiveSession = vi.fn(); + override untrackActiveSession = vi.fn(); }(); folderRepositoryManager = new CopilotCLIFolderRepositoryManager( worktree, @@ -468,6 +475,71 @@ describe('CopilotCLIChatSessionParticipant.handleRequest', () => { expect(cliSessions[0].requests[0]).toEqual({ input: { prompt: 'Say hi' }, attachments: [], model: { model: 'base' }, authInfo, token }); }); + it('uses permissionLevel from initial session options', async () => { + const request = new TestChatRequest('Say hi'); + const context = createChatContext('temp-new', true, request); + (context.chatSessionContext as { initialSessionOptions?: ReadonlyArray<{ optionId: string; value: string }> }).initialSessionOptions = [{ optionId: 'permissionLevel', value: 'autopilot' }]; + const stream = new MockChatResponseStream(); + const token = disposables.add(new CancellationTokenSource()).token; + + await participant.createHandler()(request, context, stream, token); + + expect(cliSessions.length).toBe(1); + expect(cliSessions[0].permissionLevel).toBe('autopilot'); + }); + + it('applies live permissionLevel option changes to an active session', async () => { + const provider = Object.create(CopilotCLIChatSessionContentProvider.prototype) as CopilotCLIChatSessionContentProvider; + (provider as unknown as { sessionItemProvider: CopilotCLIChatSessionItemProvider }).sessionItemProvider = itemProvider; + (provider as unknown as { _activeSessionsById: Map })._activeSessionsById = new Map(); + const activeSession = { + sessionId: 'sdk-session', + setPermissionLevel: vi.fn(), + } as unknown as ICopilotCLISession; + itemProvider.untitledSessionIdMapping.set('untitled-session', activeSession.sessionId); + provider.trackActiveSession('untitled-session', activeSession); + + await provider.provideHandleOptionsChange(Uri.parse('copilotcli:/untitled-session'), [ + { optionId: 'permissionLevel', value: 'autopilot' } + ], disposables.add(new CancellationTokenSource()).token); + + expect(activeSession.setPermissionLevel).toHaveBeenCalledWith('autopilot'); + }); + + it('scopes live permissionLevel changes to the targeted session', async () => { + const provider = Object.create(CopilotCLIChatSessionContentProvider.prototype) as CopilotCLIChatSessionContentProvider; + (provider as unknown as { sessionItemProvider: CopilotCLIChatSessionItemProvider }).sessionItemProvider = itemProvider; + (provider as unknown as { _activeSessionsById: Map })._activeSessionsById = new Map(); + const sessionA = { sessionId: 'sdk-a', setPermissionLevel: vi.fn() } as unknown as ICopilotCLISession; + const sessionB = { sessionId: 'sdk-b', setPermissionLevel: vi.fn() } as unknown as ICopilotCLISession; + itemProvider.untitledSessionIdMapping.set('resource-a', sessionA.sessionId); + itemProvider.untitledSessionIdMapping.set('resource-b', sessionB.sessionId); + provider.trackActiveSession('resource-a', sessionA); + provider.trackActiveSession('resource-b', sessionB); + + await provider.provideHandleOptionsChange(Uri.parse('copilotcli:/resource-b'), [ + { optionId: 'permissionLevel', value: 'autopilot' } + ], disposables.add(new CancellationTokenSource()).token); + + expect(sessionB.setPermissionLevel).toHaveBeenCalledWith('autopilot'); + expect(sessionA.setPermissionLevel).not.toHaveBeenCalled(); + }); + + it('clears permissionLevel on an active session when option value is undefined', async () => { + const provider = Object.create(CopilotCLIChatSessionContentProvider.prototype) as CopilotCLIChatSessionContentProvider; + (provider as unknown as { sessionItemProvider: CopilotCLIChatSessionItemProvider }).sessionItemProvider = itemProvider; + (provider as unknown as { _activeSessionsById: Map })._activeSessionsById = new Map(); + const activeSession = { sessionId: 'sdk-session', setPermissionLevel: vi.fn() } as unknown as ICopilotCLISession; + itemProvider.untitledSessionIdMapping.set('untitled-session', activeSession.sessionId); + provider.trackActiveSession('untitled-session', activeSession); + + await provider.provideHandleOptionsChange(Uri.parse('copilotcli:/untitled-session'), [ + { optionId: 'permissionLevel', value: undefined } + ], disposables.add(new CancellationTokenSource()).token); + + expect(activeSession.setPermissionLevel).toHaveBeenCalledWith(undefined); + }); + it('uses worktree workingDirectory when isolation is enabled for a new untitled session', async () => { const worktreeProperties = { autoCommit: true, diff --git a/src/vs/sessions/contrib/copilotChatSessions/browser/claudePermissionModePicker.ts b/src/vs/sessions/contrib/copilotChatSessions/browser/claudePermissionModePicker.ts index 99ee2d015f208..a11a3fe97e37d 100644 --- a/src/vs/sessions/contrib/copilotChatSessions/browser/claudePermissionModePicker.ts +++ b/src/vs/sessions/contrib/copilotChatSessions/browser/claudePermissionModePicker.ts @@ -15,6 +15,7 @@ import { ActionListItemKind, IActionListDelegate, IActionListItem, IActionListOp import { ISessionsManagementService } from '../../../services/sessions/common/sessionsManagement.js'; import { ISessionsProvidersService } from '../../../services/sessions/browser/sessionsProvidersService.js'; import { CopilotChatSessionsProvider } from './copilotChatSessionsProvider.js'; +import { IChatSessionsService } from '../../../../workbench/contrib/chat/common/chatSessionsService.js'; const PERMISSION_MODE_OPTION_ID = 'permissionMode'; @@ -56,6 +57,7 @@ export class ClaudePermissionModePicker extends Disposable { @IActionWidgetService private readonly actionWidgetService: IActionWidgetService, @ISessionsManagementService private readonly sessionsManagementService: ISessionsManagementService, @ISessionsProvidersService private readonly sessionsProvidersService: ISessionsProvidersService, + @IChatSessionsService private readonly chatSessionsService: IChatSessionsService, ) { super(); } @@ -140,7 +142,16 @@ export class ClaudePermissionModePicker extends Disposable { } const provider = this.sessionsProvidersService.getProvider(session.providerId); if (provider instanceof CopilotChatSessionsProvider) { - provider.getSession(session.sessionId)?.setOption?.(PERMISSION_MODE_OPTION_ID, { id: mode.id, name: mode.label }); + const chatSession = provider.getSession(session.sessionId); + if (!chatSession) { + return; + } + const option = { id: mode.id, name: mode.label }; + if (chatSession.setOption) { + chatSession.setOption(PERMISSION_MODE_OPTION_ID, option); + } else { + this.chatSessionsService.setSessionOption(chatSession.resource, PERMISSION_MODE_OPTION_ID, option); + } } } diff --git a/src/vs/sessions/contrib/copilotChatSessions/browser/permissionPicker.ts b/src/vs/sessions/contrib/copilotChatSessions/browser/permissionPicker.ts index 135d30996ecbc..1d499583f6d2d 100644 --- a/src/vs/sessions/contrib/copilotChatSessions/browser/permissionPicker.ts +++ b/src/vs/sessions/contrib/copilotChatSessions/browser/permissionPicker.ts @@ -23,6 +23,9 @@ import { MarkdownString } from '../../../../base/common/htmlContent.js'; import { IOpenerService } from '../../../../platform/opener/common/opener.js'; import { URI } from '../../../../base/common/uri.js'; import { CopilotChatSessionsProvider } from '../../copilotChatSessions/browser/copilotChatSessionsProvider.js'; +import { IChatSessionsService } from '../../../../workbench/contrib/chat/common/chatSessionsService.js'; + +const PERMISSION_LEVEL_OPTION_ID = 'permissionLevel'; /** * Strategy for the per-provider parts of {@link PermissionPicker}: how to read @@ -348,6 +351,7 @@ export class CopilotPermissionPickerDelegate extends Disposable implements IPerm constructor( @ISessionsManagementService private readonly _sessionsManagementService: ISessionsManagementService, @ISessionsProvidersService private readonly _sessionsProvidersService: ISessionsProvidersService, + @IChatSessionsService private readonly _chatSessionsService: IChatSessionsService, ) { super(); } @@ -359,7 +363,16 @@ export class CopilotPermissionPickerDelegate extends Disposable implements IPerm } const provider = this._sessionsProvidersService.getProvider(session.providerId); if (provider instanceof CopilotChatSessionsProvider) { - provider.getSession(session.sessionId)?.setPermissionLevel(level); + const chatSession = provider.getSession(session.sessionId); + if (!chatSession) { + return; + } + if (chatSession.setOption) { + chatSession.setPermissionLevel(level); + chatSession.setOption(PERMISSION_LEVEL_OPTION_ID, level); + } else { + this._chatSessionsService.setSessionOption(chatSession.resource, PERMISSION_LEVEL_OPTION_ID, level); + } } } } diff --git a/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts b/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts index 56b30bea3d506..5089f6c6bb8ce 100644 --- a/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts +++ b/src/vs/workbench/contrib/chat/browser/widget/input/chatInputPart.ts @@ -140,6 +140,7 @@ const INPUT_EDITOR_LINE_HEIGHT = 20; const INPUT_EDITOR_PADDING = { compact: { top: 2, bottom: 2 }, default: { top: 12, bottom: 12 } }; const CachedLanguageModelsKey = 'chat.cachedLanguageModels.v2'; const CHAT_INPUT_PICKER_COLLAPSE_WIDTH = 480; +const PERMISSION_LEVEL_OPTION_ID = 'permissionLevel'; export interface IChatInputStyles { overlayBackground: string; @@ -821,6 +822,10 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge this._currentPermissionLevel.set(level, undefined); this.permissionLevelKey.set(level); this.permissionWidget?.refresh(); + const sessionResource = this.getCurrentSessionResource(); + if (sessionResource) { + this.chatSessionsService.setSessionOption(sessionResource, PERMISSION_LEVEL_OPTION_ID, level); + } this._syncInputStateToModel(); } From 15d2d8cc8718c2c39a81cef81b7be8da3aed29c9 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Sun, 26 Apr 2026 14:54:00 -0700 Subject: [PATCH 25/25] Fix agent host agents in vscode (#312628) * chat: register in-place action for programmatic chat session contributions The autorun that registers `openNewChatSessionInPlace.` actions filters `_contributions` by `_contributionDisposables.has(...)`. Only extension-contributed providers were getting added to that map (via `_evaluateAvailability`), so programmatically-registered contributions (local + remote agent hosts) had no in-place action and the session-type picker in VS Code threw "command not found" when switching to the local Copilot CLI agent host. Mark programmatic registrations as active in `registerChatSessionContribution` so they participate in the autorun. Also disambiguate the agent-host displayName in VS Code by suffixing "- Agent Host", since the extension-host Copilot CLI harness uses the same "Copilot CLI" label. The Agents window keeps the original displayName. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * chat: refresh hasCanDelegateProviders context key on programmatic register/unregister Programmatic chat session contributions bypass _evaluateAvailability, which was the only path that called _updateHasCanDelegateProvidersContextKey. Update the context key directly in registerChatSessionContribution and its dispose so UI gated on ChatContextKeys.hasCanDelegateProviders stays in sync. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../agentHost/agentHostChatContribution.ts | 10 +++++++++- .../browser/chatSessions/chatSessions.contribution.ts | 9 +++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostChatContribution.ts b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostChatContribution.ts index 6f1c9568a0f90..03568600148b8 100644 --- a/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostChatContribution.ts +++ b/src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/agentHostChatContribution.ts @@ -152,6 +152,14 @@ export class AgentHostContribution extends Disposable implements IWorkbenchContr const agentId = sessionType; const vendor = sessionType; + // In the Agents app, the agent-host displayName is unambiguous because + // only agent-host sessions exist there. In VS Code, the same picker + // also lists the extension-host harness with the same displayName + // (e.g. "Copilot CLI"), so suffix with "- Agent Host" to disambiguate. + const displayName = this._isSessionsWindow + ? agent.displayName + : localize('agentHost.displayName', "{0} - Agent Host", agent.displayName); + // Chat session contribution. // In the Agents app, hide the delegation picker for local agent host // sessions (matches behavior of remote agent host sessions). In VS Code, @@ -159,7 +167,7 @@ export class AgentHostContribution extends Disposable implements IWorkbenchContr store.add(this._chatSessionsService.registerChatSessionContribution({ type: sessionType, name: agentId, - displayName: agent.displayName, + displayName, description: agent.description, canDelegate: true, requiresCustomModels: true, diff --git a/src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts b/src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts index e6cbe34da226f..22e8a9a45d48d 100644 --- a/src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts +++ b/src/vs/workbench/contrib/chat/browser/chatSessions/chatSessions.contribution.ts @@ -810,10 +810,19 @@ export class ChatSessionsService extends Disposable implements IChatSessionsServ } this._contributions.set(contribution.type, { contribution, extension: undefined }); + // Programmatically-registered contributions are always considered + // available; mark them as such so the autorun in the constructor + // registers the in-place "New {0} Session" action for them. Without + // this, types like `agent-host-copilotcli` (registered by the local + // agent host) have no `openNewChatSessionInPlace.` command. + this._contributionDisposables.set(contribution.type, new DisposableStore()); + this._updateHasCanDelegateProvidersContextKey(); this._onDidChangeAvailability.fire(); return toDisposable(() => { this._contributions.delete(contribution.type); + this._contributionDisposables.deleteAndDispose(contribution.type); + this._updateHasCanDelegateProvidersContextKey(); this._onDidChangeAvailability.fire(); }); }