Conversation
…et KDF (Fixes #2187) Follow-up to PR #2178 (#1986). PR #2178 hardened SecureStore's fallback encryption with a machine-secret root of trust and a versioned AES-256-GCM envelope (v:1 legacy host/user KDF, v:2 machine-secret KDF), but three sibling encrypted file stores bypassed SecureStore and still derived keys from hard-coded constants plus non-secret host/user metadata. This migrates the live and reachable siblings onto the same root of trust via a new shared codec, preserves backward-compatible reads of existing files, and makes rotation/downgrade failures fail closed. Phase 1 - shared codec (packages/storage): - Add secure-store/envelope-codec.ts exposing encryptEnvelopeString, decryptEnvelopeString, and readEnvelopeVersion. It is a thin wrapper over the existing envelope.ts primitives (scrypt + AES-256-GCM, layout [salt][iv][authTag][ciphertext]) and introduces no new crypto parameters. Centralizes v:1/v:2 selection, the anti-downgrade guard, and fail-closed decrypt (EnvelopeCodecError). - Export the codec from the barrel and add a deep sub-path export (./storage/envelope-codec.js) so core and mcp can import it without crossing the import-boundary guard. Phase 2 - ToolKeyStorage live .key fallback (packages/core): - Route saveToFile/getFromFile through the codec: new writes are v:2 envelopes when the machine secret is available; legacy iv:authTag:ciphertext files still decrypt; a v:2 file with a missing/rotated secret fails closed instead of being misread as "no key configured"; unrecognized content fails closed. Phase 3 - FileTokenStorage MCP fallback (packages/mcp): - Route loadTokens/saveTokens through the codec with the same v:2/legacy/ anti-downgrade/fail-closed semantics, mapping codec errors to the existing "Token file corrupted" behavior. Phase 4 - deprecate the dead store: - Mark FileTokenStore (file-token-store.ts) @deprecated; it has no production instantiation and is retained only for public-API/backward-compatible reads. Tests: - New behavioral tests (real temp-dir files, injected machine-secret loaders) prove v:2 writes use the secret root of trust, legacy data stays readable, and downgrade/rotation failures fail closed across all touched stores. - Update the three vitest storageExportToSource alias maps for the new sub-path export.
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
No description provided. |
Contributor
|
Warning Review limit reached
More reviews will be available in 10 minutes and 33 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a shared versioned envelope codec, updates machine-secret handling, and switches ToolKeyStorage and FileTokenStorage to envelope-based writes while preserving legacy hex-colon reads and tightening permissions. ChangesEnvelope-backed storage hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/core/src/tools/tool-key-storage.ts`:
- Line 234: The key write path in tool-key-storage’s save logic only sets mode
on creation, so existing .key files may keep broader permissions after being
overwritten. Update the write flow around the fs.writeFile call in the key
persistence method to explicitly apply a restrictive chmod on filePath after
writing, ensuring migrated or updated keys remain private.
- Around line 219-233: In the tool-key storage write path, the existing envelope
version check is too permissive because tool-key-storage.ts treats a null result
from readEnvelopeVersion(existing) as if no prior file existed. Update the logic
around the existingVersion handling in this flow so malformed or tampered
envelopes fail closed instead of being overwritten; only pass a version into
encryptEnvelopeString when the parsed version is valid, and otherwise reject the
write or surface an error rather than downgrading an existing file.
In `@packages/mcp/src/auth/token-storage/file-token-storage.ts`:
- Around line 132-145: The legacy decrypt path in `FileTokenStorage` still lets
raw crypto errors escape from `this.decrypt(data)` for malformed
`iv:authTag:ciphertext` input. Update the catch in the legacy plaintext load
flow to normalize every exception from `this.decrypt(data)` to `Token file
corrupted`, and keep the existing behavior for the current encrypted format
separate if needed. Add a regression test around `FileTokenStorage` for
malformed legacy content (for example invalid IV or auth tag lengths) to verify
it now fails closed with `Token file corrupted`.
In `@packages/storage/src/secure-store/envelope-codec.ts`:
- Around line 121-133: The default loader path in
defaultMachineSecretLoader()/resolveLoader currently uses getMachineSecret() for
both encrypt and decrypt, which can generate a new machine secret during v:2
reads. Split the behavior so decryptEnvelopeString() only loads an existing
secret and fails closed when missing or rotated, while generate-on-miss remains
limited to write/encrypt paths; update the loader selection in resolveLoader to
use the non-generating path for decrypt and preserve the existing write
behavior.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f60593e5-9867-4ffb-9a60-3cfae6bdad65
📒 Files selected for processing (13)
packages/cli/vitest.config.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/file-token-store.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/providers/vitest.config.tspackages/settings/vitest.config.tspackages/storage/package.jsonpackages/storage/src/index.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/storage/src/secure-store/envelope-codec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (macOS)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: Lint (Javascript)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/mcp/src/auth/file-token-store.tspackages/storage/src/index.tspackages/settings/vitest.config.tspackages/cli/vitest.config.tspackages/providers/vitest.config.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/mcp/src/auth/file-token-store.tspackages/storage/src/index.tspackages/settings/vitest.config.tspackages/cli/vitest.config.tspackages/providers/vitest.config.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/mcp/src/auth/file-token-store.tspackages/storage/src/index.tspackages/settings/vitest.config.tspackages/cli/vitest.config.tspackages/providers/vitest.config.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/mcp/src/auth/file-token-store.tspackages/storage/src/index.tspackages/settings/vitest.config.tspackages/cli/vitest.config.tspackages/providers/vitest.config.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/mcp/src/auth/file-token-store.tspackages/storage/src/index.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-02-16T16:11:07.481Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1434
File: packages/core/src/tools/delete_line_range.ts:204-254
Timestamp: 2026-02-16T16:11:07.481Z
Learning: Identify duplicated LSP diagnostics collection logic across packages/core/src/tools/*.ts. In reviews, flag the common block (checkFile, filter by includeSeverities, limit by maxDiagnosticsPerFile, format with <diagnostics> tags) that is replicated in six files (ast-edit.ts, delete_line_range.ts, insert_at_line.ts, edit.ts, write-file.ts, apply-patch.ts). Recommend extracting into a shared helper (e.g., collectLspDiagnosticsBlock) and ensure it handles Promise.race timeout and uses the correct severities label instead of a hardcoded "LSP errors". This guideline applies to all files in that directory and similar tools unless explicitly excluded.
Applied to files:
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.ts
📚 Learning: 2026-06-24T07:45:19.981Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2146
File: packages/core/src/tools-adapters/CoreSubagentServiceAdapter.ts:299-300
Timestamp: 2026-06-24T07:45:19.981Z
Learning: In this repo, follow the "unnecessary-condition" lint policy: if a value is already typed as non-optional (e.g., `SubagentManager.loadSubagent(...)` returns `SubagentConfig`, not `SubagentConfig | undefined`), do not add defensive conditional guards like `loaded ? ... : undefined` before passing the value into helpers (e.g., `toToolsSubagentConfig(loaded)`). Passing the non-optional value directly is the correct pattern; adding such branches is considered dead code and should be avoided so the lint passes.
Applied to files:
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.ts
🪛 ast-grep (0.44.0)
packages/mcp/src/auth/token-storage/file-token-storage.behavior.test.ts
[warning] 102-102: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tokenFilePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 205-205: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tokenFilePath, legacyContent, { mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 224-224: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tokenFilePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 241-241: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tokenFilePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/mcp/src/auth/token-storage/file-token-storage.ts
[warning] 101-101: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(this.tokenFilePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 167-167: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(this.tokenFilePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/core/src/tools/tool-key-storage.test.ts
[warning] 359-359: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 465-465: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(filePath, legacyContent, { mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 486-488: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(filePath, 'this-is-not-a-valid-key-file', {
mode: 0o600,
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 515-515: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(filePath, bogusLegacy, { mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 537-537: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 552-552: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/core/src/tools/tool-key-storage.ts
[warning] 220-220: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 233-233: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(filePath, envelopeJson, { mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 253-253: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(filePath, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (2)
packages/core/src/tools/tool-key-storage.ts (1)
32-37: LGTM!Also applies to: 96-110, 132-149, 160-171, 237-293
packages/core/src/tools/tool-key-storage.test.ts (1)
17-25: LGTM!Also applies to: 35-35, 82-116, 351-367, 406-561
Contributor
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. |
Remediates CodeRabbit findings on the #2187 hardening: - envelope-codec: a v:2 decrypt must never generate a new machine secret on a miss. Thread a generateIfMissing flag through the default loader so encrypt may mint/persist a root of trust but decrypt fails closed (CORRUPT) when the secret is gone, rather than minting a fresh secret that cannot decrypt the existing envelope. - machine-secret: add generateIfMissing (default true) to MachineSecretOptions. When false, getMachineSecret only loads an existing secret (keyring -> file) and returns null without generating/persisting. A read-only miss is not negatively cached, so it cannot poison a later generating write for the same source. - tool-key-storage / file-token-storage: writeFile's mode only applies on creation, so overwriting a pre-existing file left looser permissions intact. Explicitly chmod 0o600 after every write on POSIX (no-op on Windows) so a credential file is never left group/world-readable. - file-token-storage: normalize every legacy hex-colon decrypt failure to a single 'Token file corrupted' error so raw crypto error details never leak. Tests: - machine-secret: read-only (generateIfMissing:false) loads existing keyring/ file secrets, returns null without persisting on a miss, and a read miss does not poison a later generating call. - file-token-storage / tool-key-storage: overwriting a loose-mode file tightens it back to 0o600; a malformed legacy token file fails closed as corrupted.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/mcp/src/auth/token-storage/file-token-storage.ts (1)
160-186: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTighten existing permissions before writing the new token envelope.
Line 180 writes fresh token data while a pre-existing loose-mode file remains readable until Lines 184-185 complete; a crash or local read race can leave the new envelope exposed under the old permissions.
Proposed hardening
// Detect an existing envelope version for anti-downgrade protection. // Non-envelope (legacy) files and missing files yield null. let existingVersion: number | null = null; + let fileExists = false; try { const existing = await fs.readFile(this.tokenFilePath, 'utf-8'); + fileExists = true; existingVersion = readEnvelopeVersion(existing); } catch (error: unknown) { const err = error as NodeJS.ErrnoException; if (err.code !== 'ENOENT') { throw error; @@ const encrypted = await encryptEnvelopeString(json, this.serviceName, { ...this.codecOptions, existingEnvelopeVersion: existingVersion, }); + if (fileExists && process.platform !== 'win32') { + await fs.chmod(this.tokenFilePath, 0o600); + } await fs.writeFile(this.tokenFilePath, encrypted, { mode: 0o600 }); // writeFile's `mode` only applies on creation; overwriting a pre-existing🤖 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/mcp/src/auth/token-storage/file-token-storage.ts` around lines 160 - 186, The token write path in file-token-storage’s write flow leaves a pre-existing file under its old permissions until after fs.writeFile completes, so tighten permissions before the new envelope is written. In the code around existingVersion/encryptEnvelopeString/writeFile, change the update sequence so any existing tokenFilePath is chmod’d to 0o600 before writing the new encrypted envelope, while still keeping the post-write chmod on POSIX as a safeguard.packages/core/src/tools/tool-key-storage.ts (1)
230-238: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTighten existing permissions before writing the new key.
The current order writes the new secret while a pre-existing loose-mode file is still group/world-readable until Line 238 runs; a crash or local read race before
chmodleaves the fresh key exposed.Proposed hardening
let existingVersion: number | null = null; + let fileExists = false; try { const existing = await fs.readFile(filePath, 'utf-8'); + fileExists = true; existingVersion = readEnvelopeVersion(existing); } catch (error) { const err = error as NodeJS.ErrnoException; if (err.code !== 'ENOENT') { throw error; } } const envelopeJson = await encryptEnvelopeString(key, KEYCHAIN_SERVICE, { ...this.codecOptions, existingEnvelopeVersion: existingVersion, }); + if (fileExists) { + await this.chmodIfPosix(filePath); + } await fs.writeFile(filePath, envelopeJson, { mode: 0o600 }); // writeFile's `mode` only applies when the file is created; overwriting a // pre-existing file with looser permissions leaves them unchanged. Tighten🤖 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/core/src/tools/tool-key-storage.ts` around lines 230 - 238, The key file is being overwritten in `tool-key-storage.ts` while any existing loose permissions remain in place until after the write completes. Update the write flow in the key storage path that uses `encryptEnvelopeString`, `fs.writeFile`, and `chmodIfPosix` so the destination file is tightened before the new secret is written, then persist the envelope and re-apply the restrictive mode after writing if needed. Use the existing `chmodIfPosix` helper to ensure a pre-existing file is never left group/world-readable during the overwrite.
🤖 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.
Outside diff comments:
In `@packages/core/src/tools/tool-key-storage.ts`:
- Around line 230-238: The key file is being overwritten in
`tool-key-storage.ts` while any existing loose permissions remain in place until
after the write completes. Update the write flow in the key storage path that
uses `encryptEnvelopeString`, `fs.writeFile`, and `chmodIfPosix` so the
destination file is tightened before the new secret is written, then persist the
envelope and re-apply the restrictive mode after writing if needed. Use the
existing `chmodIfPosix` helper to ensure a pre-existing file is never left
group/world-readable during the overwrite.
In `@packages/mcp/src/auth/token-storage/file-token-storage.ts`:
- Around line 160-186: The token write path in file-token-storage’s write flow
leaves a pre-existing file under its old permissions until after fs.writeFile
completes, so tighten permissions before the new envelope is written. In the
code around existingVersion/encryptEnvelopeString/writeFile, change the update
sequence so any existing tokenFilePath is chmod’d to 0o600 before writing the
new encrypted envelope, while still keeping the post-write chmod on POSIX as a
safeguard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f66ac69a-7448-44e4-80aa-9a2712ff9e9e
📒 Files selected for processing (8)
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/machine-secret.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: E2E Test (Linux) - sandbox:docker
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (macOS)
- GitHub Check: Lint (Javascript)
- GitHub Check: CodeQL
- GitHub Check: Interactive UI (tmux)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/machine-secret.tspackages/core/src/tools/tool-key-storage.test.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/machine-secret.tspackages/core/src/tools/tool-key-storage.test.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/machine-secret.tspackages/core/src/tools/tool-key-storage.test.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/machine-secret.tspackages/core/src/tools/tool-key-storage.test.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/storage/src/secure-store/machine-secret.tspackages/core/src/tools/tool-key-storage.test.tspackages/storage/src/secure-store/envelope-codec.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.ts
📚 Learning: 2026-02-16T16:11:07.481Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1434
File: packages/core/src/tools/delete_line_range.ts:204-254
Timestamp: 2026-02-16T16:11:07.481Z
Learning: Identify duplicated LSP diagnostics collection logic across packages/core/src/tools/*.ts. In reviews, flag the common block (checkFile, filter by includeSeverities, limit by maxDiagnosticsPerFile, format with <diagnostics> tags) that is replicated in six files (ast-edit.ts, delete_line_range.ts, insert_at_line.ts, edit.ts, write-file.ts, apply-patch.ts). Recommend extracting into a shared helper (e.g., collectLspDiagnosticsBlock) and ensure it handles Promise.race timeout and uses the correct severities label instead of a hardcoded "LSP errors". This guideline applies to all files in that directory and similar tools unless explicitly excluded.
Applied to files:
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.ts
📚 Learning: 2026-06-24T07:45:19.981Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2146
File: packages/core/src/tools-adapters/CoreSubagentServiceAdapter.ts:299-300
Timestamp: 2026-06-24T07:45:19.981Z
Learning: In this repo, follow the "unnecessary-condition" lint policy: if a value is already typed as non-optional (e.g., `SubagentManager.loadSubagent(...)` returns `SubagentConfig`, not `SubagentConfig | undefined`), do not add defensive conditional guards like `loaded ? ... : undefined` before passing the value into helpers (e.g., `toToolsSubagentConfig(loaded)`). Passing the non-optional value directly is the correct pattern; adding such branches is considered dead code and should be avoided so the lint passes.
Applied to files:
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.ts
🪛 ast-grep (0.44.0)
packages/storage/src/secure-store/machine-secret.test.ts
[warning] 601-601: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tempFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 628-630: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tempFilePath, existing.toString('base64'), {
mode: 0o600,
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/mcp/src/auth/token-storage/file-token-storage.behavior.test.ts
[warning] 273-273: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tokenFilePath, malformedLegacy, { mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/core/src/tools/tool-key-storage.test.ts
[warning] 381-381: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(filePath, 'placeholder', { mode: 0o644 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (9)
packages/core/src/tools/tool-key-storage.ts (2)
215-233: Existing malformed key files are still treated as overwrite-safe.
readEnvelopeVersion(existing) === nullstill collapses malformed/unsupported envelopes with legacy or missing files, so this is the same unresolved anti-downgrade/fail-closed concern from the previous review.
105-110: LGTM!Also applies to: 127-192, 247-311
packages/storage/src/secure-store/envelope-codec.ts (1)
90-103: LGTM!Also applies to: 128-152, 166-211, 226-301
packages/storage/src/secure-store/machine-secret.ts (1)
60-68: LGTM!Also applies to: 149-187, 224-229
packages/storage/src/secure-store/machine-secret.test.ts (1)
559-672: LGTM!packages/core/src/tools/tool-key-storage.test.ts (1)
370-395: LGTM!packages/mcp/src/auth/token-storage/file-token-storage.ts (1)
29-58: LGTM!Also applies to: 61-151, 189-264
packages/mcp/src/auth/token-storage/file-token-storage.test.ts (1)
30-88: LGTM!packages/mcp/src/auth/token-storage/file-token-storage.behavior.test.ts (1)
259-310: LGTM!
Follow-up review pass on the #2187 hardening. No behavior-changing crypto changes; tightens completeness of the fail-closed contract, hardens the permission-tightening path, makes legacy detection stricter, removes a small duplication, and strengthens test assertions. Source: - envelope-codec: complete the fail-closed contract on a v:2 decrypt. Wrap the machine-secret loader() call so a rejected loader is normalized to EnvelopeCodecError(CORRUPT) instead of leaking a raw exception, and move scryptAsync inside the decrypt try so a KDF failure also fails closed. Extract a shared parseEnvelope() helper so decryptEnvelopeString and readEnvelopeVersion can never drift in how they recognize a valid envelope. - tool-key-storage / file-token-storage: if the post-write chmod 0o600 fails, unlink the just-written file and throw a descriptive error so a secret is never left on disk with over-permissive modes (distinct from a write failure). - tool-key-storage: tighten legacy hex-colon detection to the exact legacy shape (32-hex IV, 32-hex auth tag, non-empty hex ciphertext) via a single anchored pattern, so short/garbage `a:b:c` content fails closed as unrecognized rather than being routed to a decrypt attempt. - file-token-storage: lazily derive the legacy KDF key only when a legacy file is actually read (writes and envelope reads no longer pay the scrypt cost), and document the anti-downgrade read as best-effort defense-in-depth (TOCTOU). Tests: - envelope-codec: assert the structured CORRUPT code rather than brittle message text; add a rejecting-loader fail-closed test and a readEnvelopeVersion('') null edge case. - tool-key-storage: assert a generic rejection instead of Node's raw crypto message; document the frozen legacy KDF spec; add length-validation regression tests; drop redundant mkdir calls covered by beforeEach. - machine-secret: add a read-only (generateIfMissing:false) corrupt-file test proving it fails closed (null) and leaves the file untouched. - file-token-storage: decrypt written envelopes to assert merge-on-update and removal-on-delete; rename the misleading anti-downgrade behavior test and tighten a server-list assertion to exact membership.
The previous open-code-review pass introduced two changes to
decryptEnvelopeString that contradicted the canonical
SecureStore.readFallbackFileAtPath contract this codec is documented to
mirror, and broke the file-token-storage behavior test that encodes the
intended design:
- A try/catch around the v:2 machine-secret loader relabeled ANY loader
rejection as EnvelopeCodecError('CORRUPT'). A loader rejection signals a
transient infrastructure fault (e.g. a temporarily broken keyring), not an
unreadable envelope, so the original error must propagate unchanged. Only a
loader that resolves to null (secret genuinely unavailable) fails closed as
CORRUPT.
- Moving scryptAsync inside the decrypt try block would likewise mask an
unexpected KDF fault (e.g. resource exhaustion) as data corruption.
Both are reverted so the codec matches readFallbackFileAtPath exactly: the
loader call and KDF derivation stay outside the try, and only a genuine
authentication/decrypt failure fails closed as CORRUPT. The corresponding
unit test now asserts the correct behavior (an unexpected loader rejection
propagates unchanged and is not an EnvelopeCodecError), mirroring the mcp
file-token-storage behavior test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/mcp/src/auth/token-storage/file-token-storage.ts (1)
204-223: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAvoid overwriting the live token file before permission tightening succeeds.
Line 204 replaces the target in place before Line 210 proves the mode can be tightened. If the existing file is still
0644, the new credentials stay readable under that mode untilchmodruns, and achmodfailure then deletes the only copy at Lines 216-223. Write to a temp file created with0o600, tighten/verify that path, then rename atomically over the target so this hardening path doesn't introduce credential loss.🤖 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/mcp/src/auth/token-storage/file-token-storage.ts` around lines 204 - 223, The file write flow in FileTokenStorage is still overwriting the live token file before permission tightening is confirmed. Update the write path in the token persistence logic around the fs.writeFile/fs.chmod sequence so credentials are first written to a temporary file created with 0o600, then chmod/verify succeeds on that temp file, and only then atomically rename it over the final tokenFilePath; keep the existing error handling and cleanup behavior tied to the file write method in FileTokenStorage.
🤖 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/core/src/tools/tool-key-storage.ts`:
- Around line 322-327: The legacy format check in isLegacyHexColonFormat is too
strict because it rejects valid old records with an empty ciphertext after the
second colon. Update the regex or equivalent validation in tool-key-storage.ts
so it still requires the two 32-hex segments and the two separators, but allows
the ciphertext portion to be empty; keep the function name
isLegacyHexColonFormat as the place to adjust backward-compatible parsing.
In `@packages/storage/src/secure-store/envelope-codec.ts`:
- Around line 333-334: readEnvelopeVersion currently depends on full
parseEnvelope validation, so tampered v:2 envelopes can be treated as null and
later downgraded. Update readEnvelopeVersion in envelope-codec.ts to detect and
preserve the envelope version marker independently of KDF/crypto metadata
validation, returning 2 whenever the v:2 marker is present even if parseEnvelope
fails. Keep parseEnvelope for full decoding, but make existingEnvelopeVersion
derive from the version marker so the anti-downgrade guard still triggers.
---
Outside diff comments:
In `@packages/mcp/src/auth/token-storage/file-token-storage.ts`:
- Around line 204-223: The file write flow in FileTokenStorage is still
overwriting the live token file before permission tightening is confirmed.
Update the write path in the token persistence logic around the
fs.writeFile/fs.chmod sequence so credentials are first written to a temporary
file created with 0o600, then chmod/verify succeeds on that temp file, and only
then atomically rename it over the final tokenFilePath; keep the existing error
handling and cleanup behavior tied to the file write method in FileTokenStorage.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6921f413-52e3-4831-a5a0-61757fc482fe
📒 Files selected for processing (8)
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/storage/src/secure-store/envelope-codec.tspackages/storage/src/secure-store/machine-secret.test.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/mcp/src/auth/token-storage/file-token-storage.behavior.test.tspackages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-02-16T16:11:07.481Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1434
File: packages/core/src/tools/delete_line_range.ts:204-254
Timestamp: 2026-02-16T16:11:07.481Z
Learning: Identify duplicated LSP diagnostics collection logic across packages/core/src/tools/*.ts. In reviews, flag the common block (checkFile, filter by includeSeverities, limit by maxDiagnosticsPerFile, format with <diagnostics> tags) that is replicated in six files (ast-edit.ts, delete_line_range.ts, insert_at_line.ts, edit.ts, write-file.ts, apply-patch.ts). Recommend extracting into a shared helper (e.g., collectLspDiagnosticsBlock) and ensure it handles Promise.race timeout and uses the correct severities label instead of a hardcoded "LSP errors". This guideline applies to all files in that directory and similar tools unless explicitly excluded.
Applied to files:
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.ts
📚 Learning: 2026-06-24T07:45:19.981Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2146
File: packages/core/src/tools-adapters/CoreSubagentServiceAdapter.ts:299-300
Timestamp: 2026-06-24T07:45:19.981Z
Learning: In this repo, follow the "unnecessary-condition" lint policy: if a value is already typed as non-optional (e.g., `SubagentManager.loadSubagent(...)` returns `SubagentConfig`, not `SubagentConfig | undefined`), do not add defensive conditional guards like `loaded ? ... : undefined` before passing the value into helpers (e.g., `toToolsSubagentConfig(loaded)`). Passing the non-optional value directly is the correct pattern; adding such branches is considered dead code and should be avoided so the lint passes.
Applied to files:
packages/core/src/tools/tool-key-storage.test.tspackages/core/src/tools/tool-key-storage.ts
🪛 ast-grep (0.44.0)
packages/storage/src/secure-store/machine-secret.test.ts
[warning] 649-651: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(tempFilePath, 'not-a-valid-32-byte-secret', {
mode: 0o600,
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 661-661: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tempFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
packages/core/src/tools/tool-key-storage.test.ts
[warning] 586-586: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(filePath, 'aa:bb:cc', { mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 610-612: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(filePath, ${shortIv}:${authTag}:2222, {
mode: 0o600,
})
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (7)
packages/storage/src/secure-store/envelope-codec.ts (1)
155-176: LGTM!Also applies to: 254-323
packages/storage/src/secure-store/envelope-codec.test.ts (1)
255-263: LGTM!Also applies to: 279-306, 357-362
packages/storage/src/secure-store/machine-secret.test.ts (1)
643-664: LGTM!packages/core/src/tools/tool-key-storage.ts (1)
238-252: LGTM!Also applies to: 261-311
packages/core/src/tools/tool-key-storage.test.ts (1)
102-136: LGTM!Also applies to: 569-622
packages/mcp/src/auth/token-storage/file-token-storage.behavior.test.ts (1)
218-218: LGTM!Also applies to: 230-234, 324-328
packages/mcp/src/auth/token-storage/file-token-storage.test.ts (1)
21-49: LGTM!Also applies to: 245-256, 322-328
Collaborator
Author
|
Valid — fixed in the follow-up commit. The legacy |
…ward-compat read CodeRabbit review on PR #2188 flagged that isLegacyHexColonFormat required a non-empty hex ciphertext (`+` quantifier). Because the legacy AES-256-GCM encrypt path is a stream cipher (ciphertext length == plaintext length), a legacy file that stored an empty key serializes as `iv:authTag:` with an empty third part. The stricter pattern rejected those genuine legacy files as 'unrecognized format' instead of reading them back compatibly. Relax the ciphertext quantifier to `*` while keeping the exact 32-hex IV and 32-hex auth-tag length checks, so short/garbage `a:b:c` content is still not misclassified as legacy. Add a regression test proving an empty-key legacy file round-trips to ''. The companion CodeRabbit suggestion to make readEnvelopeVersion tolerate tampered crypto metadata was declined (with rationale on the PR): it rests on an incorrect threat model, would diverge from the canonical SecureStore.readExistingEnvelopeVersion it mirrors, and would prevent re-saving over a genuinely corrupt file.
…rdening Second open-code-review pass on the #2187 hardening. Completes the fail-closed/permission contracts and strengthens test coverage; no behavior-changing crypto. Source: - file-token-storage: restructure loadTokens to classify content as versioned-envelope, exact legacy hex-colon shape, or neither. Add a private isLegacyHexColonFormat guard (mirroring ToolKeyStorage) so short/garbage a:b:c content fails closed as "Token file corrupted" instead of being passed into the crypto API with an invalid-length IV. - file-token-storage: preserve the structured EnvelopeCodecError as `cause` on the "Token file corrupted" error so callers/debuggers can still distinguish UNAVAILABLE vs CORRUPT and recover the remediation hint. - file-token-storage / tool-key-storage: if the post-write chmod 0o600 fails AND the cleanup unlink also fails, no longer throw a message that falsely claims the file was removed. Track the unlink outcome and throw a branched, accurate message; tool-key-storage additionally logs the path and unlink error via debugLogger.warn so an operator can manually remove the over-permissive secret. - tool-key-storage: saveKeyfilesMap now calls chmodIfPosix after writeFile. writeFile's `mode` only applies on creation, so overwriting a pre-existing keyfiles.json left its prior (possibly group/world-readable) permissions intact; tighten explicitly on POSIX, mirroring saveToFile. - a2a-server tsconfig: add ES2022.Error to `lib`. a2a-server type-checks mcp source directly, and the new Error(..., { cause }) overload requires the ES2022.Error lib (already present in mcp and 9 sibling packages). Tests: - file-token-storage: add an expired-token read test proving the storage layer returns expired credentials without filtering; decrypt the written envelope to assert the persisted credential map on create; add chmod-failure path tests for both the unlink-succeeds and unlink-also-fails branches; document the legacy-KDF helper's frozen magic strings against the production getLegacyEncryptionKey. - envelope-codec: add a v:1 cross-service isolation test (decrypt under a different serviceName fails CORRUPT); add a readEnvelopeVersion v:1 test; add an anti-downgrade happy-path test (existing v:2 + available secret overwrites as v:2). - machine-secret: add a read-only (generateIfMissing:false) test proving a rejecting keyring loader with no existing file fails closed (null) and writes no file.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/tools/tool-key-storage.ts (1)
383-392: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winClean up
keyfiles.jsonwhen chmod hardening fails.Line 392 throws on chmod failure but leaves the just-written
keyfiles.jsonin place with its previous broad permissions. MirrorsaveToFilehere so plaintext keyfile paths are not left group/world-readable after a failed hardening step.Proposed hardening
await fs.writeFile(this.keyfilesJsonPath, JSON.stringify(map, null, 2), { mode: 0o600, }); @@ - await this.chmodIfPosix(this.keyfilesJsonPath); + try { + await this.chmodIfPosix(this.keyfilesJsonPath); + } catch (chmodError) { + let unlinkFailed = false; + try { + await fs.unlink(this.keyfilesJsonPath); + } catch { + unlinkFailed = true; + } + const detail = + chmodError instanceof Error ? chmodError.message : String(chmodError); + throw new Error( + unlinkFailed + ? `keyfiles.json was written but permissions could not be tightened to 0o600, and the over-permissive file could not be removed: ${detail}` + : `keyfiles.json was written but permissions could not be tightened to 0o600; the file was removed: ${detail}`, + ); + }🤖 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/core/src/tools/tool-key-storage.ts` around lines 383 - 392, The chmod hardening in saveKeyfilesMap can fail after keyfiles.json has already been written, leaving the plaintext path map on disk with its existing permissions. Update saveKeyfilesMap to mirror saveToFile by catching chmodIfPosix failures, removing the newly written keyfilesJsonPath on error, and rethrowing so the failure is not silently ignored.
🤖 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/mcp/src/auth/token-storage/file-token-storage.test.ts`:
- Around line 300-350: These chmod failure cases in the file-token-storage tests
are POSIX-only and should not run on Windows because the production path in
setCredentials skips chmod when process.platform is win32. Guard or skip the two
affected tests so they only execute on non-Windows platforms, keeping the
expectations around chmod, unlink cleanup, and the “permissions could not be
restricted” / “could not be removed” errors tied to the same setCredentials
flow.
---
Outside diff comments:
In `@packages/core/src/tools/tool-key-storage.ts`:
- Around line 383-392: The chmod hardening in saveKeyfilesMap can fail after
keyfiles.json has already been written, leaving the plaintext path map on disk
with its existing permissions. Update saveKeyfilesMap to mirror saveToFile by
catching chmodIfPosix failures, removing the newly written keyfilesJsonPath on
error, and rethrowing so the failure is not silently ignored.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb0418e5-6363-4f54-87d1-b093fae13c1e
📒 Files selected for processing (6)
packages/a2a-server/tsconfig.jsonpackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/storage/src/secure-store/machine-secret.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: E2E Test (Linux) - sandbox:none
- GitHub Check: E2E Test (Linux) - sandbox:docker
⚠️ CI failures not shown inline (4)
GitHub Actions: LLxprt Code CI / 7_Lint (Javascript).txt: Harden sibling encrypted file stores with machine-secret KDF (Fixes #2187)
Conclusion: failure
##[group]Run npm run format
�[36;1mnpm run format�[0m
�[36;1m# Check for changes, excluding project-plans directory�[0m
�[36;1mgit diff --exit-code -- . ':!project-plans/'�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
ACTIONLINT_VERSION: 1.7.7
SHELLCHECK_VERSION: 0.11.0
YAMLLINT_VERSION: 1.35.1
##[endgroup]
> `@vybestack/llxprt-code`@0.10.0 format
> prettier --experimental-cli --write .
.llxprt/LLXPRT.md
warning: in the working copy of 'packages/vscode-ide-companion/NOTICES.txt', CRLF will be replaced by LF the next time Git touches it
diff --git a/.llxprt/LLXPRT.md b/.llxprt/LLXPRT.md
index 2760baf67..f1271dd7b 100644
--- a/.llxprt/LLXPRT.md
+++ b/.llxprt/LLXPRT.md
@@ -2,5 +2,5 @@
- CRITICAL: NEVER delete, remove, or modify the .llxprt/ directory or any of its contents (LLXPRT.md, settings.json, skills/, commands/). This directory contains project memories, settings, and skills that are version-controlled. Do not run rm, git clean, git checkout, or any other command that would remove these files. If git status shows .llxprt files as modified, leave them alone — they are supposed to be there. "Clean workspace" means only: checkout main and pull from origin. It does NOT mean deleting files.
- Before checking in code changes from the main project directory, run: npm run test, npm run lint, npm run typecheck, npm run format, npm run build, and node scripts/start.js --profile-load ollamakimi "write me a haiku and nothing else"; fix any errors, ensure code is formatted before pushing/creating PRs, and always use gh for PRs/issues/comments (never webfetch).
-- When user requests to address GitHub issue `#NUM`: 1) Checkout main and pull latest from origin (do NOT delete any files or run git clean — just git checkout main && git pull), 2) Create branch "issueNUM", 3) Use gh to pull issue and comments, 4) Research issue in codebase using description/comments as starting point, 5) Create test-first plan following dev-docs/RULE...
GitHub Actions: LLxprt Code CI / Lint (GitHub Actions): Harden sibling encrypted file stores with machine-secret KDF (Fixes #2187)
Conclusion: failure
##[group]Run actionlint \
�[36;1mactionlint \�[0m
�[36;1m -color \�[0m
�[36;1m -format "{{range \$err := .}}::error file={{\$err.Filepath}},line={{\$err.Line}},col={{\$err.Column}}::{{\$err.Filepath}}@{{\$err.Line}} {{\$err.Message}}%0A\`\`\`%0A{{replace \$err.Snippet \"\\\\n\" \"%0A\"}}%0A\`\`\`\\n{{end}}" \�[0m
GitHub Actions: LLxprt Code CI / Lint (Javascript): Harden sibling encrypted file stores with machine-secret KDF (Fixes #2187)
Conclusion: failure
##[group]Run npm run format
�[36;1mnpm run format�[0m
�[36;1m# Check for changes, excluding project-plans directory�[0m
�[36;1mgit diff --exit-code -- . ':!project-plans/'�[0m
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
ACTIONLINT_VERSION: 1.7.7
SHELLCHECK_VERSION: 0.11.0
YAMLLINT_VERSION: 1.35.1
##[endgroup]
> `@vybestack/llxprt-code`@0.10.0 format
> prettier --experimental-cli --write .
.llxprt/LLXPRT.md
warning: in the working copy of 'packages/vscode-ide-companion/NOTICES.txt', CRLF will be replaced by LF the next time Git touches it
diff --git a/.llxprt/LLXPRT.md b/.llxprt/LLXPRT.md
index 2760baf67..f1271dd7b 100644
--- a/.llxprt/LLXPRT.md
+++ b/.llxprt/LLXPRT.md
@@ -2,5 +2,5 @@
- CRITICAL: NEVER delete, remove, or modify the .llxprt/ directory or any of its contents (LLXPRT.md, settings.json, skills/, commands/). This directory contains project memories, settings, and skills that are version-controlled. Do not run rm, git clean, git checkout, or any other command that would remove these files. If git status shows .llxprt files as modified, leave them alone — they are supposed to be there. "Clean workspace" means only: checkout main and pull from origin. It does NOT mean deleting files.
- Before checking in code changes from the main project directory, run: npm run test, npm run lint, npm run typecheck, npm run format, npm run build, and node scripts/start.js --profile-load ollamakimi "write me a haiku and nothing else"; fix any errors, ensure code is formatted before pushing/creating PRs, and always use gh for PRs/issues/comments (never webfetch).
-- When user requests to address GitHub issue `#NUM`: 1) Checkout main and pull latest from origin (do NOT delete any files or run git clean — just git checkout main && git pull), 2) Create branch "issueNUM", 3) Use gh to pull issue and comments, 4) Research issue in codebase using description/comments as starting point, 5) Create test-first plan following dev-docs/RULE...
GitHub Actions: LLxprt Code CI / 6_Lint (GitHub Actions).txt: Harden sibling encrypted file stores with machine-secret KDF (Fixes #2187)
Conclusion: failure
##[group]Run actionlint \
�[36;1mactionlint \�[0m
�[36;1m -color \�[0m
�[36;1m -format "{{range \$err := .}}::error file={{\$err.Filepath}},line={{\$err.Line}},col={{\$err.Column}}::{{\$err.Filepath}}@{{\$err.Line}} {{\$err.Message}}%0A\`\`\`%0A{{replace \$err.Snippet \"\\\\n\" \"%0A\"}}%0A\`\`\`\\n{{end}}" \�[0m
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-06-11T05:52:47.561Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1991
File: packages/ide-integration/tsconfig.json:6-6
Timestamp: 2026-06-11T05:52:47.561Z
Learning: In the vybestack/llxprt-code monorepo, preserve the repo-wide TypeScript tsconfig convention that each workspace package’s `compilerOptions.lib` intentionally includes both `"DOM"` and `"DOM.Iterable"`. This is required for DOM-typed globals like `fetch`/`Response` used by packages (e.g., `ide-client.ts`). During code review, avoid suggesting removal of these DOM libs from any package tsconfig; if a package’s tsconfig is missing them, add them rather than removing them.
Applied to files:
packages/a2a-server/tsconfig.json
📚 Learning: 2026-02-06T15:52:42.315Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1305
File: scripts/generate-keybindings-doc.ts:1-5
Timestamp: 2026-02-06T15:52:42.315Z
Learning: In reviews of vybestack/llxprt-code, do not suggest changing existing copyright headers from 'Google LLC' to 'Vybestack LLC' for files that originated from upstream. Preserve upstream copyrights in files that came from upstream, and only apply 'Vybestack LLC' copyright on newly created, original LLxprt files. If a file is clearly LLxprt-original, it may carry the Vybestack header; if it is upstream-originated, keep the original sponsor header.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-03-31T02:12:43.093Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1854
File: packages/core/src/core/subagentRuntimeSetup.test.ts:77-84
Timestamp: 2026-03-31T02:12:43.093Z
Learning: In this codebase, tool declarations should follow the single required contract `parametersJsonSchema`; do not ask to preserve or reintroduce the legacy `parameters` fallback field. Reviewers should not flag assertions/checks for missing `parameters` or suggest backward-compatibility behavior for `parameters`. Schema converters/providers are expected to error if `parametersJsonSchema` is absent instead of falling back to `parameters`.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-10T18:18:08.545Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:156-156
Timestamp: 2026-06-10T18:18:08.545Z
Learning: In this repo, ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is set to fail loops that contain more than 1 `break`/`continue` total per loop (or both present). When a loop violates this (e.g., it contains a `break` and a `continue`, or has multiple `break`s/`continue`s), the code will not lint unless the violating line includes `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop`. In code reviews, do not suggest removing these `eslint-disable-next-line` directives (use refactoring only if it eliminates the underlying >1 break/continue pattern).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-10T18:18:09.253Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1983
File: packages/policy/src/policy-engine.ts:263-263
Timestamp: 2026-06-10T18:18:09.253Z
Learning: In this repository, the ESLint rule `sonarjs/too-many-break-or-continue-in-loop` is configured to allow at most 1 `break`/`continue` per loop (it is stricter than the SonarJS default). During code review, treat `// eslint-disable-next-line sonarjs/too-many-break-or-continue-in-loop` on loops with 2+ `break`/`continue` as intentional and do not suggest removing or changing those directives. Only consider a change if the rule is violated without an appropriate intentional disable.
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-06-19T17:16:56.523Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2108
File: packages/agents/src/api/agentImpl.ts:1047-1079
Timestamp: 2026-06-19T17:16:56.523Z
Learning: When the fake-provider test seam is active in vybestack/llxprt-code, `process.env.LLXPRT_FAKE_RESPONSES` is set to a fixture file path ending in a `.jsonl` (not to the string `'1'` or any other boolean-like value). In code, detect the seam by checking `process.env.LLXPRT_FAKE_RESPONSES !== undefined` (and/or that it is a non-empty string), rather than using `process.env.LLXPRT_FAKE_RESPONSES === '1'`. Update any callers of the env var accordingly (see `packages/providers/src/composition/providerManagerInstance.ts` and harness usages).
Applied to files:
packages/storage/src/secure-store/machine-secret.test.tspackages/storage/src/secure-store/envelope-codec.test.tspackages/mcp/src/auth/token-storage/file-token-storage.tspackages/core/src/tools/tool-key-storage.tspackages/mcp/src/auth/token-storage/file-token-storage.test.ts
📚 Learning: 2026-02-16T16:11:07.481Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 1434
File: packages/core/src/tools/delete_line_range.ts:204-254
Timestamp: 2026-02-16T16:11:07.481Z
Learning: Identify duplicated LSP diagnostics collection logic across packages/core/src/tools/*.ts. In reviews, flag the common block (checkFile, filter by includeSeverities, limit by maxDiagnosticsPerFile, format with <diagnostics> tags) that is replicated in six files (ast-edit.ts, delete_line_range.ts, insert_at_line.ts, edit.ts, write-file.ts, apply-patch.ts). Recommend extracting into a shared helper (e.g., collectLspDiagnosticsBlock) and ensure it handles Promise.race timeout and uses the correct severities label instead of a hardcoded "LSP errors". This guideline applies to all files in that directory and similar tools unless explicitly excluded.
Applied to files:
packages/core/src/tools/tool-key-storage.ts
📚 Learning: 2026-06-24T07:45:19.981Z
Learnt from: acoliver
Repo: vybestack/llxprt-code PR: 2146
File: packages/core/src/tools-adapters/CoreSubagentServiceAdapter.ts:299-300
Timestamp: 2026-06-24T07:45:19.981Z
Learning: In this repo, follow the "unnecessary-condition" lint policy: if a value is already typed as non-optional (e.g., `SubagentManager.loadSubagent(...)` returns `SubagentConfig`, not `SubagentConfig | undefined`), do not add defensive conditional guards like `loaded ? ... : undefined` before passing the value into helpers (e.g., `toToolsSubagentConfig(loaded)`). Passing the non-optional value directly is the correct pattern; adding such branches is considered dead code and should be avoided so the lint passes.
Applied to files:
packages/core/src/tools/tool-key-storage.ts
🪛 ast-grep (0.44.0)
packages/storage/src/secure-store/machine-secret.test.ts
[warning] 621-621: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(tempFilePath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🔇 Additional comments (7)
packages/a2a-server/tsconfig.json (1)
5-5: LGTM!packages/storage/src/secure-store/machine-secret.test.ts (1)
607-625: LGTM!packages/storage/src/secure-store/envelope-codec.test.ts (1)
233-261: LGTM!Also applies to: 362-405
packages/core/src/tools/tool-key-storage.ts (1)
132-348: LGTM!packages/mcp/src/auth/token-storage/file-token-storage.ts (2)
82-178: LGTM!Also applies to: 197-254
156-159: 🎯 Functional CorrectnessNo tsconfig change needed for
Error.causepackages/mcp/tsconfig.jsonalready includesES2022.Error, sonew Error(message, { cause })is supported.packages/mcp/src/auth/token-storage/file-token-storage.test.ts (1)
81-89: LGTM!Also applies to: 162-185, 226-231
The root format script used 'prettier --experimental-cli --write .', but the
experimental CLI does not auto-load .prettierignore the way the stable CLI
does. As a result 'npm run format' reformatted ignored files (e.g.
.llxprt/LLXPRT.md, which is listed in .prettierignore), and CI's formatter
check ('npm run format' followed by 'git diff --exit-code') failed on any
code-bearing PR even though the offending file was never touched by the branch.
Adding an explicit '--ignore-path .prettierignore' restores the intended
behavior: the experimental CLI now skips ignored paths, matching the sibling
'format:check' script (plain 'prettier --check .') which already honors the
ignore file. Verified that the full-repo format run no longer modifies any
ignored files and that the CI formatter check is clean.
The two chmod-failure tests in file-token-storage.test.ts mock fs.chmod to reject and expect setCredentials to reject. Production only tightens permissions on POSIX platforms (chmod is skipped when process.platform === 'win32'), so on Windows the promise would resolve and these tests would fail. Guard both with it.skipIf(process.platform === 'win32'), matching the existing convention in tool-key-storage.test.ts and elsewhere in the repo.
# Conflicts: # package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2187. Follow-up to PR #2178 (#1986).
PR #2178 hardened
SecureStore's fallback encryption with a machine-secret root of trust and a versioned AES-256-GCM envelope (v:1 = legacy host/user-metadata KDF, v:2 = machine-secret-backed KDF). However, three sibling encrypted file stores bypassedSecureStoreentirely and still derived AES-256-GCM keys from hard-coded constants plus non-secret host/user metadata — i.e. confidentiality depended on filesystem permissions and predictable metadata rather than a high-entropy secret.This PR migrates the live and reachable siblings onto the same root of trust through a new shared codec, preserves backward-compatible reads of existing files, and makes rotation/downgrade failures fail closed.
What changed
Phase 1 — Shared envelope codec (
packages/storage)secure-store/envelope-codec.tsexposingencryptEnvelopeString,decryptEnvelopeString, andreadEnvelopeVersion.envelope.tsprimitives (deriveV1KdfInput,deriveV2KdfInput,isValidEnvelope,scryptAsync,SCRYPT_PARAMS,SALT_LEN) — no new crypto parameters. Same on-disk layout[salt][iv][authTag][ciphertext], scrypt + AES-256-GCM.SecureStore: v:1/v:2 version selection, the anti-downgrade guard (never overwrite an existing v:2 with a weaker v:1 when the machine secret is unavailable), and fail-closed decrypt (EnvelopeCodecError)../storage/envelope-codec.js) socoreandmcpcan import it without crossing the import-boundary guard.Phase 2 —
ToolKeyStoragelive.keyfallback (packages/core) — prioritizedThis is the only path live in production today (used whenever the OS keyring is unavailable), so it is addressed first.
saveToFile/getFromFilenow route through the codec. New writes are v:2 envelopes when the machine secret is available.iv:authTag:ciphertext.keyfiles still decrypt (positively recognized via a strict hex-colon matcher).Phase 3 —
FileTokenStorageMCP fallback (packages/mcp)loadTokens/saveTokensroute through the codec with the same v:2 / legacy-read / anti-downgrade / fail-closed semantics, mapping codec failures to the existingToken file corruptedbehavior.Phase 4 — Deprecate the dead store
FileTokenStore(file-token-store.ts) is marked@deprecated. It has no production instantiation and is retained only for public-API compatibility and backward-compatible reads, directing consumers toHybridTokenStorage/FileTokenStorage.Tests
hex:hex:hexstring),vitest.config.tsstorageExportToSourcealias maps for the new sub-path export.Verification
npm run format,npm run typecheck,npm run lint,npm run lint:eslint-guard,npm run build— all green.node scripts/start.js --profile-load ollamakimi) — green.Note on pre-existing flakes
A handful of
packages/core/src/utils/filesearch/tests (crawler.test.ts,fileSearch.directory.test.ts) are flaky under worker co-location due to a module-global cache shared between two test files. This is pre-existing and unrelated to this change — it reproduces identically on a pristineorigin/mainworktree with none of these changes applied, andmainCI is green. Not touched here.