fix(auth): share OS credentials across runtimes - #3423
Conversation
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Keep the published bin entry Windows-shim-safe while replacing Node with Bun before CLI initialization, so scriptless global installs retain secure keychain access. Fixes LET-9257. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Probe the discovered Bun executable before Node 22 replaces itself, preserving the Node fallback for corrupt or incompatible runtimes and covering both recursion guards. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Replace launcher runtime switching with platform-native secure-storage backends that preserve existing Bun Keychain, Credential Manager, and Secret Service entries under Node. Isolate credential tests from live user entries and prove the shared storage identity in both directions. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Integrate the latest channel access-control and schedule-runner changes without rewriting the existing PR branch, so the runtime-independent credential fix is reviewed against current main. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Install secret-tool on Linux CI runners after unlocking GNOME Keyring so the required Bun-to-Node interoperability test reaches the same command boundary used in production. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Replace the native Keychain wrapper with the error-preserving macOS security boundary, and migrate restricted legacy Bun entries through their owning Bun runtime before Node reads them. Keep new macOS writes headless-compatible without putting secret values in process arguments, prove the actual Node process boundary, and normalize empty-value deletion across runtimes. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Keep read, write, and delete operations on the owning Bun boundary whenever Bun is available, rather than assuming unrestricted read access transfers mutation ownership to Node. Exercise the lifecycle across a fresh Node process and construct the fake Darwin backend explicitly on Linux CI. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
| const bunPath = getBunExecutablePath(runtime); | ||
| if (!bunPath) return null; | ||
|
|
||
| const result = await runSecretCommand( |
There was a problem hiding this comment.
This helper runs bun -e from the current project directory with the full inherited env plus the keychain locator and, for set, the secret payload on stdin. Bun honors project-local config/preloads for bun -e, so simply running Letta Code inside a repo with a bunfig.toml preload can execute repo-controlled code inside the credential bridge process. That is broader than the mod threat model: users expect installed mods to run code, but not arbitrary cwd Bun config during auth/keychain migration. Please run this bridge from a trusted neutral cwd and disable/sanitize Bun project config/preloads and Bun-affecting env before spawning.
There was a problem hiding this comment.
Addressed in ba6474b8. The Bun Keychain bridge now runs from a freshly created empty temporary directory and strips Bun project/config/preload environment variables before spawning. The regression test verifies that repo-local bunfig.toml configuration is not consulted and the bridge does not inherit the caller’s working directory.
— Overlord (agent-c2adbf5c-8419-4211-8cd8-3740db164974)
|
Fixed both review findings in
Validation:
|
|
Converted to a draft after getting too many signin prompts, though this may have been fixed by ba6474b. |
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Hydrate API and refresh tokens once per process, make availability checks structural, and bypass secure storage entirely when LETTA_API_KEY is authoritative. Bound transient read recovery and isolate credential tests so normal startup no longer amplifies Keychain access or risks live credentials. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Route LCD account mutations through the awaited credential-storage path so failures cannot redact or orphan credentials before a successful keyring operation. Keep ordinary updates and deletes non-hydrating while preserving rollback access when route ownership changes. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Keep hydrated keyring values redacted during synchronous metadata saves and delay route cleanup until credential deletion succeeds. This prevents plaintext credential re-persistence and preserves related account state across keyring failures. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Resolve the conflict on the source-size baseline while preserving awaited credential writes, keychain redaction, and listener channel secret paths. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Preserve main deletion order by removing local account state before surfacing keyring cleanup failures. The regression now verifies account, route, target, pairing, and multi-secret cleanup behavior. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Carry secure channel account writes through the new process-boundary protocol while retaining the current gateway and listener refactor.
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Preserve secure-reference metadata across in-process account mutations and roll back multi-field keyring writes when any credential update fails. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
Letta Code (agent-c2adbf5c-8419-4211-8cd8-3740db164974) Serialize each channel credential transaction through its shared account file and roll back only completed keyring writes so failed updates cannot clobber concurrent successes. 👾 Generated with [Letta Code](https://letta.com) Co-Authored-By: Letta Code <noreply@letta.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
cpacker
left a comment
There was a problem hiding this comment.
Summary
The underlying fix here is correct and clearly needed, and the platform work is careful. My concerns are about packaging and rollout sequencing, not correctness — specifically that this bundles five independent concerns and performs an irreversible credential-format transition in a single deploy.
Recommending we restructure this into a stack before landing any part of it. Details below.
What's good
- The compatibility-proof approach is the right one. Bidirectional read/write tests between
Bun.secretsand each Node backend, pinning target format, schema, persistence, and encoding, is exactly how you defend against undocumented-format drift. The macOS test that builds the bundle, runs it against a legacy restricted entry, then launches a second fresh process correctly targets the ACL-and-process-restart boundary an in-process test can't reach. - Secrets never touch argv. Values go over stdin everywhere; the macOS
setdeliberately keeps-wlast and refuses multiline rather than falling back to argv. Output is byte-bounded and subprocesses time out. bun -eruns in a freshmkdtempcwd withBUN_CONFIG/BUN_OPTIONSstripped — prevents a projectbunfig.tomlor env preload from injecting code into the credential helper. Easy to miss.- Making
isKeychainAvailable()structural is correct. Probing by reading a live credential was causing the very Keychain access it was trying to detect. - The self-disclosed risk section is unusually honest, including the
allowUnrestrictedAccesstradeoff (any process running as the same user can now read the API key without a prompt, where previously it was ACL-bound tobun). That tradeoff is real and unavoidable for headless cross-runtime sharing, and it's correctly surfaced rather than buried. - Layer placement,
@/imports, kebab-case, namedexport function, size-baseline ratchet, and isolated-test registration all conform to repo conventions.
Main concern: the Bun→Node flip can log out existing macOS users
The dangerous interaction is removing the polyglot shebang combined with macOS Keychain ACLs.
Bun.secrets.set() defaults to an ACL restricted to the writing executable. Every currently-authenticated macOS user who installed with Bun present has a Keychain item owned by the bun binary. After this PR they run under Node.
| Scenario | Outcome |
|---|---|
macOS, was on Bun, bun on this process's PATH |
✅ Works — Node spawns bun -e <helper>, Bun is the ACL owner |
macOS, was on Bun, bun NOT on this process's PATH |
security find-generic-password against a restricted item → GUI auth prompt, or exit 51 headless → read fails → treated as unauthenticated |
| Already on Node (any OS) | ✅ Net improvement — keychain now works where it didn't |
Linux without libsecret-tools |
✅ isAvailable() correctly false; settings-file fallback holds |
The reason I'd call row 2 a real risk rather than an edge case: findExecutableOnPath (src/utils/secret-backends.ts) consults only env.PATH. Bun installs to ~/.bun/bin, which is added by shell profiles — and listeners started from launchd, systemd, a GUI-launched process, or cron get a minimal PATH that does not include it. So the trigger isn't "the user uninstalled Bun," it's "this particular process doesn't have Bun on its PATH" — which is exactly the listener case this PR is trying to fix.
The failure is also quiet and slow. Each failed read waits on the 10s SECRET_COMMAND_TIMEOUT_MS, and SecureTokenCache.hydrateOnce gives up permanently after the second failure:
if (loaded.complete || this.failedHydrations > 0) {
this.hydrated = true; // stops retrying for the process lifetime
} else {
this.failedHydrations += 1;
}Two 10s hangs, then silently unauthenticated until restart.
There is no proactive ACL migration. allowUnrestrictedAccess: true is only applied on writes. Legacy restricted items get rewritten as unrestricted only when a token write happens to occur — which needs Bun on PATH at that moment. A user who doesn't re-auth is never migrated.
What I'd want before this ships
- Ship the ACL migration under Bun first. Release the backends with the shebang flip reverted. Under Bun, proactively rewrite existing items with
allowUnrestrictedAccess: true. Let that reach adoption, then flip to Node in a later release. This turns the risky scenario into a no-op because items are already unrestricted. - Widen Bun discovery beyond
PATH— check~/.bun/bin/bunand$BUN_INSTALL/bin/bunexplicitly. - Never let a Keychain read block on a GUI prompt. A modal Keychain dialog appearing twice on CLI startup is its own incident. Drop the timeout well below 10s for the
securityfallback. - Confirm the recovery path.
LETTA_SKIP_KEYCHAIN_CHECK=1forces the settings-file fallback and is a usable kill switch — but only for users who have a settings-file token. Anyone on the Keychain has to re-runletta login. Support should know that's the answer.
Structural: five concerns in one PR
| # | Concern | Files | ~Lines |
|---|---|---|---|
| 1 | Node OS-credential backends | secret-backends.ts + tests, secrets.ts |
~1,850 |
| 2 | Remove polyglot shebang / build patch | postinstall-patches.js, build.js |
~40 |
| 3 | Single-flight token cache + LETTA_API_KEY short-circuits |
secure-token-cache.ts, settings-manager.ts |
~200 |
| 4 | Channel secret write serialization + rollback | accounts.ts, service-accounts.ts, protocol-* |
~780 |
| 5 | Test service-name namespacing | secrets.ts, isolated-unit-tests.json |
~100 |
(4) doesn't belong here. Per-channel write locking and multi-field credential rollback fix a concurrent-writer clobbering bug, not a runtime-availability bug. It's ~780 lines of transactional logic reviewable entirely on its own, and bundling it means a revert of the auth change also reverts that fix. Same for (3) — a legitimate perf improvement, but it changes credential precedence (below) and deserves its own review.
Suggested sequence: (1)+(5) → (3) → (4), with (2) last and separately, since (2) is the only piece that changes behavior for already-working users.
Specific findings
1. Credential precedence silently changed. getSettingsWithSecureTokens:
// before: keychain wins; process.env not consulted here
!secureTokens.apiKey && baseSettings.env?.LETTA_API_KEY ? ... : secureTokens.apiKey
// after: process.env wins over keychain
process.env.LETTA_API_KEY ?? secureTokens.apiKey ?? baseSettings.env?.LETTA_API_KEYThis aligns with src/index.ts:860, which already preferred process.env, so it's a consistency fix rather than a bug. But a user with a stale or differently-scoped LETTA_API_KEY exported in their shell and an active Cloud login will silently switch identity. Worth a changelog line at minimum.
2. getSecretBackend() returning non-null doesn't mean usable. On Linux it always returns a linux-secret-service backend even with no secret-tool and no D-Bus; only isAvailable() knows. All current callers gate on isKeychainAvailable() first so this is correct today, but getBackendOrThrow() reads as a usability guarantee it doesn't provide. Consider returning null when the backend can't function.
3. Windows: C# is recompiled on every credential operation. Add-Type -TypeDefinition invokes the C# compiler per PowerShell spawn — realistically 1–3s per get/set/delete. Two reads at startup means a multi-second delay before Windows users are authenticated. The single-flight cache limits it to once per process, but worth measuring; batching both token reads into one request would halve it.
4. The Bun helper's restore path drops the unrestricted ACL. In BUN_MACOS_KEYCHAIN_HELPER_SCRIPT, the rollback is Bun.secrets.set({ ...locator, value: previousValue }) with no allowUnrestrictedAccess: true — so a failed replace silently converts an unrestricted item back to restricted, re-creating the exact condition this PR exists to avoid.
5. add-generic-password -U -A may not reset an existing item's ACL. -U updates in place; whether -A is reapplied to an existing ACL isn't guaranteed. If it isn't, the Node-without-Bun path can never self-heal a legacy item. Worth an explicit test: create a restricted item, update it via the security path, verify a different binary can then read it.
6. Non-reentrant lock with no guard. runSerializedChannelSecretOperation isn't reentrant. Today no locked body calls back into hydrateChannelAccountSecrets / getChannelAccountWithSecrets, so it's correct — but a future caller adding one self-deadlocks forever with no timeout and no error. A dev-mode "already holding lock for this channel" assertion would make that a loud failure instead of a hang.
7. SecureTokenCache.merge never removes. if (tokens.apiKey) this.tokens.apiKey = ... means a token deleted by another process is never dropped from the cache. Documented as intentional and clear() covers the in-process delete path — noting as a known sharp edge, not a defect.
8. No minimum Bun version check. allowUnrestrictedAccess is passed unconditionally. On an older Bun that ignores the option, items stay restricted and the user silently lands in the broken scenario. A version gate, or a post-write read-back verification, would catch this.
9. flushPendingChannelSecretWrites is now an empty no-op still called at three sites. Fine as a compatibility shim, but if it's only for tests, prefer deleting it and updating the tests in the same change rather than leaving a misleading name.
|
Following up on my review — I've split this into a reviewable stack, and I want to correct the framing of my earlier comment. On the diagnosis: it's right, and more definitively than this PR argues. I traced the mechanism. I also over-weighted the deploy risk. The population my macOS ACL finding would newly break is largely the population already broken today, and the recovery path degrades to the settings-file fallback rather than bricking. It's worth fixing before the runtime flip, but it isn't the incident-level risk I framed it as. The stack
The useful property: #3655 alone fixes both reported symptoms without flipping any runtime. Node processes get real OS credential storage, and Bun writes opt into Two things found while splitting1. This branch is stale against 2. One addition that isn't from this PR: #3656 includes a fix for a regression these backends would otherwise introduce on Linux. Migrating into the Secret Service and deleting the file fallback strands every process without Suggest converting this to draft and keeping it as the reference implementation until the stack lands, rather than closing it — the multi-platform CI evidence and the format-matching details here are the expensive part to reproduce. |
Problem
Letta Code stores Cloud credentials with
Bun.secrets, but the published CLI and listeners can run under Node. Node therefore treated secure storage as unavailable even when the same OS credential already existed, causing global installs and listeners to lose authentication.This fixes credential availability at the storage boundary instead of changing the launcher runtime. Credential tests now use unique service names and never inspect live
letta-codeentries.Approach
src/utils/secrets.tsnow selects aSecretBackendat runtime:Bun.secrets.allowUnrestrictedAccessmode. When Bun is absent, Node uses the built-in/usr/bin/securityboundary directly. This avoids GUI permission prompts while keeping a standalone Node backend for new installations.${service}/${name}, usernamename, raw UTF-8 bytes, and enterprise persistence.secret-toolthrough stdin using Bun's exact Secret Service attributes:service,account, andxdg:schema=com.oven-sh.bun.Secret.Secret values are never placed in process arguments. Command output is bounded and subprocesses time out after ten seconds. Missing OS services still use the existing settings fallback.
Credential reads are also bounded: backend availability checks are structural and never inspect a credential, API and refresh tokens hydrate through one single-flight cache per process, and
LETTA_API_KEYskips secure storage entirely. A failed hydration gets one later retry for transient recovery, then stops until reload or restart.The earlier Node-to-Bun launcher bootstrap has been removed. The published entrypoint remains an ordinary Node shebang, and the final implementation adds no native npm dependency.
Compatibility proof
Tests write, read, and delete the same randomly named OS entries in both directions between
Bun.secretsand the explicit Node backend. Platform CI requires this interoperability rather than silently skipping it.The macOS regression test additionally builds the backend for Node, launches it against a legacy restricted Bun entry, then launches a fresh second Node process to read and delete both legacy and newly written entries. That covers the real Keychain application-ownership and process-restart boundaries; the earlier in-process test alone could not.
Validation on this branch:
bun run check— 12/12 checks pass.get,get) for API and refresh tokens, zero stderr.LETTA_API_KEY— zero Keychain/helper operations, zero stderr.Risk and limits
The in-process cache intentionally does not poll for credential changes made by another process; Letta Code writes update it directly, and reload or restart rehydrates from secure storage.
The main risk is matching Bun's undocumented platform storage identity exactly. Tests pin target/schema, persistence, encoding, and bidirectional behavior to catch drift.
On macOS, cross-runtime headless access deliberately uses a per-user rather than per-executable Keychain ACL. Credentials remain encrypted by Keychain, but other processes running as the same user can invoke the same credential APIs. This is the same tradeoff exposed by Bun's
allowUnrestrictedAccessoption and is required for Node and Bun listeners to share credentials without GUI prompts.The macOS
securityprompt interface cannot safely accept multiline values through stdin. On systems without Bun, new multiline writes fail rather than exposing a secret in argv. Existing multiline Bun entries, and new multiline writes when Bun is available, remain intact. If the Bun executable that owns a legacy entry is removed, replaced, or missing fromPATH, Node can still read unrestricted entries but mutation of an older restricted item may require reauthentication.Manual validation
LETTA_API_KEYandLETTA_BASE_URLunset; no Keychain or sign-in prompt appeared.