Conversation
…2928) SecureStore could not tell "this credential does not exist" from "macOS refused / the user cancelled", so a denied Keychain prompt looked like a missing credential, the existing classifier never fired, and every subsequent read re-prompted with nothing able to stop it. Three changes, all in TypeScript: Error fidelity. classifyError moves to a new dependency-leaf module (classify-error.ts) so the adapter boundary can use it without an import cycle, and it now recognises genuine user cancellation as DENIED instead of letting it fall through to a silently degradable UNAVAILABLE. The match is deliberately narrow -- errSecUserCanceled / errSecCanceled and word-boundary "user cancel(l)ed" -- because @napi-rs/keyring accepts an AbortSignal on every method and abort/timeout text must not latch. Syscall/errno errors (EACCES, EPERM) never latch either: their messages read as "permission denied" without the keyring having refused anything. One session-level transition. keyring-session-state.ts holds the process-wide latch. It is enforced at the single real chokepoint -- createGuardedAdapter() in default-keyring-adapter.ts -- through which SecureStore, machine-secret and MCP token storage all obtain their adapter. The guard checks the session before each native call and routes every thrown native error through the shared noteKeyringError before rethrowing it unchanged, so the first DENIED/LOCKED latches the keyring off, emits exactly one stderr warning, and yields zero further native entry -- even for an adapter a consumer cached before the latch, and even when that consumer swallows the error. RUNTIME_REPLACED stays terminal and is never absorbed. An explicit opt-out. LLXPRT_DISABLE_OS_KEYRING=1 and the security.disableOsKeyring setting make the factory return null before @napi-rs/keyring is imported, so zero Keychain operations occur including for llxprt-code-machine-secret. The setting is pushed into storage at the top of finalizeConfig, ahead of profile auth wiring, because that path performs a real SecureStore read. While the keyring is disabled the fallback write path resolves the machine secret read-only and refuses to mint a replacement when a v:2 envelope exists, since a keychain-resident secret may be present but unreachable and replacing it would permanently orphan those envelopes. Minting is still allowed when there is nothing to orphan. The v:2 read path fails closed with an actionable error rather than generating a new root of trust. Read-path fidelity is deliberately not addressed here. @napi-rs/keyring discards the OSStatus in PasswordTask::compute via Rust .ok(), and 1.3.0 is already the latest release, so recovering it requires changing the native binding. Tracked in #3067.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a ChangesOS keyring session handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
WalkthroughThis PR changes 18 file(s).
Changes
Magnitude🎯 3 (L) RelatedNo related items found. Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
CI caught two consequences of adding security.disableOsKeyring: - scripts shard: generate-settings-doc's check mode failed because docs/cli/configuration.md and schemas/settings.schema.json were not regenerated. Ran npm run docs:settings; both gain only the new boolean. - cli shard: settings.test.ts asserts the fully-defaulted security block with toStrictEqual, so it needed the new key. Also addresses two review comments on the PR: - The "env var wins when both are present" wording was wrong. isOsKeyringSessionDisabled() ORs the latch, the setting and the env var, so there is no precedence -- each independently disables the keyring. Corrected the comments and rewrote the test to assert what actually holds: either path alone disables, and with neither set the session is enabled. - Wrapped the process.stderr.write spy in try/finally so a failing assertion cannot leave it attached and pollute later tests.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
env.set(undefined) read ambiguously enough that review took it for vitest's helper and flagged it as possibly assigning the literal string "undefined". It was a local helper that already deleted the key, but the ambiguity is worth removing: set() now accepts only a string, and an explicit clear() performs the delete, so passing undefined is no longer expressible.
# Conflicts: # packages/storage/src/secure-store/default-keyring-adapter.ts
|
Merged latest main, which now contains #3074 (Fixes #3020). That PR shipped the same LLXPRT_DISABLE_OS_KEYRING=1 escape hatch, so there was a genuine conflict in default-keyring-adapter.ts. Resolved by keeping one source of truth. #3074 read the env var through a local isOsKeyringDisabled() in default-keyring-adapter.ts; this branch reads the same variable in keyring-session-state.ts, where it sits alongside the security.disableOsKeyring setting and the runtime DENIED/LOCKED latch. The factory now calls isOsKeyringSessionDisabled(), which ORs all three, and #3074's local const plus helper were removed so the variable is not defined in two places. Behaviour from #3074 is preserved and now strictly extended: the env var alone still short-circuits the factory before @napi-rs/keyring is imported. Its test suite (test-bun/keyring-opt-out.bun.ts) is untouched and passes alongside this branch's suite -- 29/29 across both files. Full verification re-run on the merged head: format, typecheck, lint, lint:eslint-guard and build all exit 0; storage bun and vitest suites both green. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/storage/test-bun/secure-store.keyring-session.bun.ts (2)
223-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the
fixedMachineSecretname and doc.The function returns a new random 32-byte value on each call. The name and the doc claim a fixed, deterministic secret. Each caller stores the result in a local variable, so the tests are correct, but the doc is wrong. Rename to
randomMachineSecretand state that the value is stable per test because callers reuse the variable.♻️ Proposed fix
-/** A fixed 32-byte machine secret for deterministic v:2 envelopes. */ -function fixedMachineSecret(): Buffer { +/** + * A fresh random 32-byte machine secret. Each test holds the returned value in + * a local variable, so the v:2 envelope and its read-back use the same secret. + */ +function randomMachineSecret(): Buffer { return crypto.randomBytes(32); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/storage/test-bun/secure-store.keyring-session.bun.ts` around lines 223 - 226, Rename fixedMachineSecret to randomMachineSecret and update its documentation to describe that it generates a random 32-byte secret, with stability per test provided by callers reusing the returned local variable. Update all references to the renamed function.
631-655: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the comment with the mechanism it uses.
The comment says the test simulates a
DENIEDlatch, but the test callssetOsKeyringDisabledBySetting(true), which sets the settings opt-out flag. The assertion is still valid because the guard readsisOsKeyringSessionDisabled(). State that the test drives the shared disablement predicate through the setting, so the comment does not imply DENIED-latch coverage that the second test at line 657 provides.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/storage/test-bun/secure-store.keyring-session.bun.ts` around lines 631 - 655, Update the comments in the test around createGuardedAdapter to state that setOsKeyringDisabledBySetting(true) drives the shared isOsKeyringSessionDisabled() predicate through the settings opt-out, rather than simulating a DENIED latch. Keep the existing assertions and distinguish this coverage from the separate DENIED-latch test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/storage/bunfig.toml`:
- Line 2: Update the storage Bun manifest’s preload configuration to include
test-setup-bun-session-reset.ts alongside the existing preload entries, ensuring
scripts/run_bun_tests.ts forwards the session reset setup for every storage test
file.
In `@packages/storage/src/secure-store/secure-store.ts`:
- Around line 206-229: Move the read-only JSDoc block with requirement R3.5 from
above loadMachineSecretForWrite to immediately above loadMachineSecretForRead.
Leave the write-path JSDoc solely documenting loadMachineSecretForWrite,
preserving each method’s existing implementation and annotations.
---
Nitpick comments:
In `@packages/storage/test-bun/secure-store.keyring-session.bun.ts`:
- Around line 223-226: Rename fixedMachineSecret to randomMachineSecret and
update its documentation to describe that it generates a random 32-byte secret,
with stability per test provided by callers reusing the returned local variable.
Update all references to the renamed function.
- Around line 631-655: Update the comments in the test around
createGuardedAdapter to state that setOsKeyringDisabledBySetting(true) drives
the shared isOsKeyringSessionDisabled() predicate through the settings opt-out,
rather than simulating a DENIED latch. Keep the existing assertions and
distinguish this coverage from the separate DENIED-latch test.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d5b2f58-f6d3-4a22-94c7-cf35bbd0deef
⛔ Files ignored due to path filters (1)
project-plans/issue2928/PLAN.mdis excluded by!project-plans/**
📒 Files selected for processing (17)
docs/cli/configuration.mdpackages/cli/src/config/postConfigRuntime.tspackages/cli/src/config/settings-schema/schema-extensions.tspackages/cli/src/config/settings.test.tspackages/storage/bunfig.tomlpackages/storage/src/index.tspackages/storage/src/secure-store/classify-error.tspackages/storage/src/secure-store/default-keyring-adapter.tspackages/storage/src/secure-store/keyring-session-state.tspackages/storage/src/secure-store/provider-key-storage.test.tspackages/storage/src/secure-store/secure-store.fallback.test.tspackages/storage/src/secure-store/secure-store.tspackages/storage/test-bun/secure-store.fallback-hardening.bun.tspackages/storage/test-bun/secure-store.keyring-session.bun.tspackages/storage/test-setup-bun-session-reset.tsschemas/settings.schema.jsonscripts/bun-test-manifest-data-storage.ts
The Bun-only reset hooks were wired into packages/storage/bunfig.toml, but scripts/run_bun_tests.ts builds explicit --preload arguments from the manifest entry's preloads and never reads that file. In manifest-driven runs -- which is what npm test and CI use -- the session reset was silently dropped, so the process-wide keyring latch could leak from one test file into the next. The suites masked it because each one already resets the latch in its own beforeEach. The storage manifest entry now declares both preloads. Also moves the read-path JSDoc back onto loadMachineSecretForRead. It was orphaned above loadMachineSecretForWrite when the R3.4 machine-secret mirror was dropped, leaving two stacked blocks on one method and none on the other.
1 participant
TLDR
SecureStore could not tell "this credential does not exist" from "macOS refused / the user cancelled". Every failure collapsed into "not found", so LLxprt's existing
classifyErrornever fired, users saw a misleading "not authenticated" state, and nothing could stop it from re-prompting on every subsequent read.This delivers the two things that actually stop the storm, plus a way out:
DENIEDinstead of a silently degradableUNAVAILABLE.DENIED/LOCKEDlatches the OS keyring unusable for the process, emits exactly one warning, and guarantees zero further native Keychain entry.LLXPRT_DISABLE_OS_KEYRING=1and asecurity.disableOsKeyringsetting, honored before@napi-rs/keyringis even imported.Reviewers please look at: the placement of the latch in
createGuardedAdapter()(that choice is the crux of the PR), the deliberately narrow cancellation matching, and the machine-secret write-path guard that refuses to mint a replacement root of trust.Dive Deeper
Why the read path is not fixed here (and what is)
I read the source of the binding rather than guessing. In
Brooooooklyn/keyring-nodemain:src/async_entry.rs,PasswordTask::compute->Ok(self.inner.get_password().ok())— the.ok()discards theOSStatus.src/async_entry.rs,EntryTaskdelete ->Ok(Some(self.inner.delete_credential().is_ok()))— same collapse.src/entry.rs(syncEntry) — identical.set_password/set_secretDO propagate viamap_err(anyhow::Error::from)?.Confirmed empirically on darwin against the installed binding:
getPassword()on a missing entry returnsnull,deleteCredential()returnsfalse.@napi-rs/keyring@1.3.0is installed and is the latest published version, so upgrading cannot fix the read path.findCredentialsis not a usable disambiguator either — on macOS itsfilter_mapreadsget_generic_passwordfor every account under the service and drops failures silently, so using it would multiply prompts, which is the exact thing this issue exists to stop.Recovering read-path
OSStatustherefore requires forking/vendoring/replacing a Rust native module (toolchain plus prebuilt binaries for darwin-arm64/x64, linux-x64/arm64 gnu+musl, win32-x64/arm64). That is a dependency + build + CI change and is deliberately out of scope, tracked in #3067. It plugs in behind the same factory with no call-site changes when it lands.The write, delete-verification and probe paths do propagate errors today, which is what the latch runs on.
The latch is at the adapter boundary, not in SecureStore
The first cut put the latch in SecureStore's catch sites. That was wrong:
machine-secret.ts(readFromKeyring,persistToKeyringLocked) and MCPkeychain-token-storage.tshold adapters directly and catch their own errors, and SecureStore's ownlist()and write-verification swallow errors before classification. All of those bypassed it.It now lives in
createGuardedAdapter()indefault-keyring-adapter.ts— the one place every consumer's adapter comes from. Each wrapped method checks the session before entering native code and routes any thrown native error through the sharednoteKeyringErrorbefore rethrowing it unchanged. So the latch fires even when the immediate caller swallows the error, and a consumer holding an adapter cached from before the latch gets a typedUNAVAILABLEinstead of a prompt.classifyErrormoved to a new dependency-leaf module (classify-error.ts) purely to let the adapter use it without an import cycle back intosecure-store.ts.Classification is deliberately narrow
A process-wide latch is close to irreversible within a session, so the trigger has to be precise:
errsecusercanceled/errseccanceledand word-boundaryuser cancel(l)ed/cancel(l)ed by the user. A barecancelsubstring would also match"request cancelled due to timeout", and the binding accepts anAbortSignalon every method.EACCES,EPERM, …) never latch. Their messages read as "permission denied" without the keyring having refused anything.TIMEOUTandUNAVAILABLEdo not latch.RUNTIME_REPLACEDstays terminal, does not latch, and is never converted into the session error.Machine-secret durability
Two guards, both about not orphaning credentials:
An earlier draft mirrored the machine secret to disk on every v:2 write to make mode-switching seamless. That was removed: it put the keychain-resident root of trust on disk, was fail-open on persistence failure, could install a mismatched/stale secret, and raced other writers. The reasoning is recorded in
project-plans/issue2928/PLAN.md.Notes on collateral changes
packages/storage/test-setup-storage-isolation.tsis loaded by bothbunfig.tomlandvitest.config.ts, so the Bun-only reset hooks live in a separatetest-setup-bun-session-reset.ts. Importingbun:testin the shared file breaks Vitest collection outright.lockederror to simulate a transient failure and then asserted recovery.LOCKEDnow intentionally latches, so those were switched to non-latchingTIMEOUT/UNAVAILABLEmessages, preserving each test's original intent.Reviewer Test Plan
The opt-out (no Keychain access at all):
Then confirm no
llxprt-code-*items are touched. On macOS, watch with Console.app filtered onsecurityd, or simply confirm you get no prompts and that credentials round-trip through~/.llxprt/secure-store/. Equivalent via settings: setsecurity.disableOsKeyring: trueand restart.The prompt storm itself (the real-world reproduction, on macOS with a keychain entry whose ACL will prompt): start LLxprt and press Cancel/Escape on the first Keychain dialog during a credential write. Before this change you would keep getting dialogs; now you should get exactly one stderr warning naming the cause and remedy, and then silence for the rest of the session, with credentials served from the encrypted fallback.
Automated:
The new suite is
packages/storage/test-bun/secure-store.keyring-session.bun.ts. Worth spot-checking that the tests fail if you delete the logic: the post-latch cases use fresh counting adapters and assert zero adapter calls, so they cannot pass merely by hitting the same denied adapter again.Testing Matrix
Verified locally on macOS:
npm run typecheck,npm run lint,npm run lint:eslint-guard,npm run format,npm run buildall exit 0;packages/storagebun and vitest suites both green; smoke test viabun scripts/start.ts --profile-load stepfun-37returns normally.npm run testat the repo root has pre-existing failures on this machine that are unrelated and reproduce identically with the branch stashed: 9 image tests fail becausesharpis not installed in the project and resolves to a stale v0.33.5 in$HOMEthat lacksmetadata.autoOrient, and the agents package produces rotating 30s timeouts (a different set every run, present at baseline too). Relying on CI for the authoritative signal.Linked issues / bugs
Fixes #2928
Read-path
OSStatusfidelity is explicitly deferred and tracked in #3067. Companion to the replaced-runtime fast-fail work in #2926.Summary by CodeRabbit
New Features
LLXPRT_DISABLE_OS_KEYRING=1and takes effect after restarting.Bug Fixes
Documentation