Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis large PR implements a repository-wide migration from legacy ChangesOS-Standard Path Migration
Estimated code review effort: 5 (Critical) | ~180 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #2646Issue AlignmentStrong alignment with #2606. The PR establishes Side Effects
Code QualityWell-structured and defensive. The shared Tests and CoverageCoverage impact: Increase. Substantial behavioral tests added across storage, auth locks, memory reconciliation, prompt backup/publishing collisions, path delegation, and the legacy-path guard (including RED/GREEN self-test and fixture-based negatives). Tests exercise real filesystem concurrency and edge cases rather than mocking implementation details. One caveat: some suites are conditionally skipped when Bun is unavailable, which may reduce coverage in non-Bun environments. VerdictReady — the implementation is comprehensive, well-tested, and closely tracks the issue requirements. Before merging, confirm that all remaining active |
# Conflicts: # .github/workflows/ci.yml # docs/reference/profiles.md # scripts/check-agents-api-surface.mjs # scripts/tests/check-agents-api-surface.test.ts # scripts/tmux-script.approval-ui.json # scripts/tmux-script.issue2208-newlines.fake.json # scripts/tmux-script.slash-autocomplete.json
Contributor
OpenCodeReview — PR #2646
OCR stderr excerptOCR preview stderr excerpt |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/debug-logging.md (1)
127-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the documented debug-log filename format.
This section says logs are named
llxprt-debug-<PID>.jsonl, but the troubleshooting text at Line 271 still saysdebug-YYYY-MM-DD.jsonl. Keep one filename format throughout the guide.🤖 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 `@docs/debug-logging.md` around lines 127 - 131, Update the troubleshooting reference to use the documented llxprt-debug-<PID>.jsonl filename format instead of debug-YYYY-MM-DD.jsonl, keeping the debug-log filename consistent throughout the guide.packages/a2a-server/src/config/config.ts (1)
320-346: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
~/.envbehind the canonical config fallback.For a normal workspace under
$HOME, Lines 333-335 select~/.envduring parent traversal, so Lines 342-345 never consider the canonical config.env. Stop the upward scan before evaluatinghomeDir, then check canonical config and finallyhomeDir/.env. Add a regression with a workspace nested beneath the injected home directory.Proposed fix
function findEnvFile(startDir: string, homeDir: string): string | null { let currentDir = path.resolve(startDir); let parentDir = path.resolve(startDir); + const resolvedHomeDir = path.resolve(homeDir); do { currentDir = parentDir; + if (currentDir === resolvedHomeDir) { + break; + } // prefer llxprt-specific .env under LLXPRT_CONFIG_DIR🤖 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/a2a-server/src/config/config.ts` around lines 320 - 346, Update findEnvFile so the upward traversal stops before evaluating homeDir, preventing ~/.env from being selected during the scan. Preserve the existing order after traversal: check Storage.getGlobalConfigDir()/.env first, then homeDir/.env; add a regression covering a workspace nested beneath the injected home directory.packages/core/src/utils/memoryDiscovery.ts (1)
443-501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAsymmetric exclusion in
findUpwardLlxprtFiles: direct-in-dir candidate isn't excluded like the.llxprt-nested one.The canonical/legacy exclusion check (lines 488-496) is only applied to the
.llxprt/<filename>candidate (llxprtDirPath). The direct<currentDir>/<filename>candidate (directPath, lines 468-478) has no equivalent guard, unlikesearchUpwardForLlxprtMdwhich filters both candidate types throughexcludedPaths. If a JIT traversal'scurrentDirever equals the canonical global memory dir (or~/.llxprt), the direct candidate can resurface the global memory file, relying entirely on the caller's separatealreadyLoadedPathsdedup as the only safety net.🐛 Suggested fix
const directPath = path.join(currentDir, filename); checks.push( (async () => { try { await fs.access(directPath, fsSync.constants.R_OK); + if ( + path.resolve(currentDir) === canonicalGlobalMemoryDir || + path.resolve(currentDir) === legacyGlobalLlxprtDir + ) { + return null; + } return directPath; } catch { return null; } })(), );🤖 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/utils/memoryDiscovery.ts` around lines 443 - 501, Update the directPath candidate handling in findUpwardLlxprtFiles to exclude files whose containing directory is canonicalGlobalMemoryDir or legacyGlobalLlxprtDir, matching the existing llxprtDirPath guard. Return null for those excluded direct candidates while preserving discovery for all other directories.
🟡 Minor comments (23)
docs/architecture/message-bus-architecture.md-753-754 (1)
753-754: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify the fenced-block language.
Add
textto this output-only code fence so markdownlint passes.-``` +```text Testing policy: /absolute/path/to/my-policy.toml🤖 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 `@docs/architecture/message-bus-architecture.md` around lines 753 - 754, Specify the language on the output-only fenced code block containing “Testing policy: /absolute/path/to/my-policy.toml” by using the text fence marker, preserving the block’s content unchanged.Source: Linters/SAST tools
docs/cli/context-dumping.md-85-88 (1)
85-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefine
DUMPSbefore using it.The comment documents
DUMPS=<cache>/dumpsbut never assigns the variable, so the following commands resolve to an invalid path unless users infer and perform an undocumented setup step.Suggested fix
-# DUMPS=<cache>/dumps — LLxprt's dumps directory (see Application Directories) +DUMPS="${LLXPRT_CACHE_HOME:-${LLXPRT_CONFIG_HOME:-$HOME/.cache/llxprt-code}}/dumps" jq '.request.body' "$DUMPS/YOUR_DUMP.json" > /tmp/body.json🤖 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 `@docs/cli/context-dumping.md` around lines 85 - 88, Update the command example near the DUMPS comment to assign the DUMPS environment variable to the documented cache dumps directory before jq and curl reference "$DUMPS". Preserve the existing dump-file commands and path convention.docs/reference/profiles.md-220-220 (1)
220-220: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify the fenced-block language.
Add
textto the directory-tree fence so the documentation passes MD040.Suggested fix
-``` +```text <config>/profiles/🤖 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 `@docs/reference/profiles.md` at line 220, Specify the text language on the directory-tree fenced code block in the profiles documentation by changing its opening fence to use text, while leaving the block contents unchanged.Source: Linters/SAST tools
docs/tools/mcp-server.md-13-13 (1)
13-13: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the MCP scope in the CLI example.
The command shown omits
--scope, whose documented default isprojectat Line 576. It therefore writes to project settings, not the usersettings.json. Add--scope userto the example or change the sentence to describe project-scope behavior.🤖 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 `@docs/tools/mcp-server.md` at line 13, Correct the MCP CLI example and its accompanying description so they agree on scope: add the explicit user scope option to the command when describing updates to user settings, or revise the text to accurately describe the documented project-scope default.docs/debug-logging.md-199-209 (1)
199-209: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake shell path examples follow the centralized cross-platform resolver.
The examples hardcode Linux defaults and do not consistently honor the documented override/fallback behavior, so users on other platforms or with only
LLXPRT_CONFIG_HOMEset can inspect the wrong files.
docs/debug-logging.md#L199-L209: include theLLXPRT_CONFIG_HOMEfallback and clearly label Linux-only commands, or provide platform-specific/resolver-backed commands.docs/tools/memory.md#L112-L113: label the command as Linux-specific or replace its fallback with a platform-aware config-directory resolution.🤖 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 `@docs/debug-logging.md` around lines 199 - 209, Update the shell examples in docs/debug-logging.md (lines 199-209) to follow the centralized cross-platform log-directory resolution, including LLXPRT_CONFIG_HOME as a fallback; otherwise clearly label the commands as Linux-only. Apply the same platform-awareness requirement to the command in docs/tools/memory.md (lines 112-113), either by using the resolver-backed config directory or explicitly labeling it Linux-specific.docs/migration/approval-mode-to-policies.md-251-261 (1)
251-261: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake every example truly absolute.
The text correctly says not to use
~or relative paths, but the Linux and Windows examples immediately use~/.config/...and%APPDATA%\.... Replace them with concrete absolute examples such as/home/yourname/.config/...andC:\Users\yourname\AppData\Roaming\....🤖 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 `@docs/migration/approval-mode-to-policies.md` around lines 251 - 261, Update the Linux and Windows path examples in the migration documentation to use concrete absolute paths, replacing the `~` and `%APPDATA%` placeholders with `/home/yourname/.config/...` and `C:\Users\yourname\AppData\Roaming\...`; keep the macOS example and the surrounding absolute-path guidance unchanged.docs/hooks/creating-custom-hooks.md-244-245 (1)
244-245: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStore rate-limit state under the canonical log/state directory.
Path.home() / '.llxprt-rate-limit-state.json'bypassesLLXPRT_LOG_HOME, the config fallback, and platform defaults. Use the resolved log/state directory so this tutorial remains consistent with the migration contract.🤖 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 `@docs/hooks/creating-custom-hooks.md` around lines 244 - 245, Update the state file assignment in the custom hooks example to derive its path from the resolved canonical log/state directory rather than directly from Path.home(). Reuse the existing resolution flow honoring LLXPRT_LOG_HOME, its config fallback, and platform defaults, while preserving the rate-limit state filename.docs/hooks/creating-custom-hooks.md-143-150 (1)
143-150: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake audit-log fallback resolution platform-correct.
Both snippets hard-code the Linux default even though the surrounding documentation describes OS-specific canonical locations.
docs/hooks/creating-custom-hooks.md#L143-L150: use the platform-specific log/state default or label the snippet Linux-only.docs/hooks/index.md#L185-L196: apply the same correction to the quick-start audit example.🤖 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 `@docs/hooks/creating-custom-hooks.md` around lines 143 - 150, Make audit-log fallback resolution platform-correct in the snippets at docs/hooks/creating-custom-hooks.md lines 143-150 and docs/hooks/index.md lines 185-196: use the documented OS-specific canonical log/state default after LLXPRT_LOG_HOME and LLXPRT_CONFIG_HOME, or explicitly label each snippet as Linux-only. Keep the existing LOG_DIR, mkdir, and LOG_FILE flow consistent across both examples.docs/gemini-cli-tips.md-96-104 (1)
96-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssign
CONFIG_DIRinstead of placing the assignment in a comment.The copy commands currently use an unset
CONFIG_DIR, so they resolve to/LLXPRT.mdand/settings.jsonon typical shells. Make the example executable:-# Example on Linux: CONFIG_DIR="${LLXPRT_CONFIG_HOME:-$HOME/.config/llxprt-code}" +CONFIG_DIR="${LLXPRT_CONFIG_HOME:-$HOME/.config/llxprt-code}"🤖 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 `@docs/gemini-cli-tips.md` around lines 96 - 104, Update the shell example before the cp commands to actually assign CONFIG_DIR using the documented LLXPRT_CONFIG_HOME fallback, rather than leaving the assignment only in a comment. Keep both copy commands using this variable so they target the resolved configuration directory.packages/auth/src/__tests__/keyring-token-store.lock-behavior.test.ts-185-194 (1)
185-194: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNull-byte lockDir assertion is too broad
packages/auth/src/__tests__/keyring-token-store.lock-behavior.test.ts:185-194fs.mkdir(...'\0')throwsTypeError [ERR_INVALID_ARG_VALUE]with a message about null bytes; it does not includelock,directory, orerror, sorejects.toThrow(/lock|directory|error/i)is brittle. Assert oncodeornameinstead.🤖 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/auth/src/__tests__/keyring-token-store.lock-behavior.test.ts` around lines 185 - 194, Update the rejection assertion in the “propagates unexpected write errors from lockDir creation” test to inspect the thrown error’s stable code or name for the null-byte path failure, rather than matching the message against /lock|directory|error/i. Keep the test focused on the TypeError/ERR_INVALID_ARG_VALUE produced by the invalid lockDir.packages/cli/src/config/memoryReconciliation.test.ts-119-145 (1)
119-145: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlso gate this filesystem-permission test off Windows.
chmod(file, 0o000)does not remove read access on Windows, soreconcileGlobalMemorysucceeds andexpect(result.error).toBe(true)fails there. The currentit.skipIf(os.userInfo().uid === 0)only skips for root (uid-1on Windows won't match). Add awin32guard so the error-path contract stays enforceable only where mode bits are honored.Based on learnings, prefer platform-gating Windows-specific skips via `it.skipIf(process.platform === 'win32')`.Proposed gate
- it.skipIf(os.userInfo().uid === 0)( + it.skipIf(os.userInfo().uid === 0 || process.platform === 'win32')( 'returns error and no marker when source is unreadable, and leaves the source untouched',🤖 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/cli/src/config/memoryReconciliation.test.ts` around lines 119 - 145, Update the skip condition for the unreadable-source test in the `it.skipIf` block to also skip when `process.platform === 'win32'`, while preserving the existing root-user guard. Keep the test’s permission-error assertions unchanged for platforms where chmod mode bits are honored.Source: Learnings
scripts/telemetry_utils.js-30-40 (1)
30-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the fallback on the shared resolver contract.
If the import fails, this path skips
LLXPRT_LOG_HOME/LLXPRT_CONFIG_HOMEand falls back to a Linux-only directory. Reuse the shared resolver logic here so the telemetry path still honors the same precedence on the error path.🤖 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 `@scripts/telemetry_utils.js` around lines 30 - 40, Update the fallback in the telemetry initialization around resolveGlobalLogDir so it reuses the shared resolver contract and continues honoring LLXPRT_LOG_HOME/LLXPRT_CONFIG_HOME precedence when the dynamic import fails. Avoid hardcoding the Linux-only path.join fallback; use the existing shared resolution logic or equivalent configuration-aware fallback.scripts/tests/nightly-bun-native-smoke.test.js-30-37 (1)
30-37: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject fractional values that floor to zero.
0.5passes the positivity check but producesHARNESS_TIMEOUT_MS = 0. Validate after flooring so every accepted override remains positive.const parsed = Number(raw); if (!Number.isFinite(parsed) || parsed <= 0) return DEFAULT; - return Math.floor(parsed); + const timeout = Math.floor(parsed); + return timeout > 0 ? timeout : DEFAULT;🤖 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 `@scripts/tests/nightly-bun-native-smoke.test.js` around lines 30 - 37, Update resolveHarnessTimeoutMs so the parsed timeout is floored before validation, and reject any resulting value less than or equal to zero. Preserve the default timeout for undefined, empty, non-finite, non-positive, or sub-unit fractional overrides, while returning positive integer overrides.scripts/genai-enclave/file-discovery.ts-121-191 (1)
121-191: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
-zrename pairs inscripts/genai-enclave/file-discovery.ts
git status --porcelain --untracked-files=all -zemits renames as two NUL-delimited tokens (R.. new\0old\0). The current loop parses the trailingoldtoken as if it were a status record, which can misclassify paths whose leading characters resemble a status code. Skip the paired field explicitly instead of splitting every token throughparseStatusEntry().🤖 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 `@scripts/genai-enclave/file-discovery.ts` around lines 121 - 191, The status parser in collectStatusSets must handle -z rename records as paired NUL-delimited tokens: when parseStatusEntry identifies a rename, consume and skip the following old-path token instead of parsing it as a separate status entry. Preserve deleted-path and untracked-path collection for the actual status record, and retain parseStatusEntry’s rename path handling.packages/cli/src/config/extension.test.ts-136-143 (1)
136-143: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comment contradicts the rest of the file.
This comment says Storage resolves user extensions under
<tempHome>/.llxprt/extensions, but theEXTENSIONS_DIRECTORY_NAMEcomment at Lines 109-112 (and the same pattern inextension.skills.test.ts,rootAwareManagement.test.ts,update.test.ts) states fixtures land at<tempHome>/extensionswith no.llxprtcomponent. One of these is stale; givenStorage.getUserExtensionsDir()returns<dataHome>/extensionsperextension.ts, the.llxprt-prefixed wording here appears to be leftover from the pre-migration layout.✏️ Suggested fix
- // Redirect all LLxprt category dirs to the temp home so Storage resolves - // user extensions under <tempHome>/.llxprt/extensions (config falls back - // to data via the compat chain). This mirrors how the existing suite - // laid out fixtures under tempHome/.llxprt/extensions. + // Redirect all LLxprt category dirs to the temp home so + // Storage.getUserExtensionsDir() resolves under <tempHome>/extensions.🤖 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/cli/src/config/extension.test.ts` around lines 136 - 143, Update the setup comment above the ENV_KEYS loop to describe the current fixture layout under <tempHome>/extensions, removing the stale <tempHome>/.llxprt/extensions wording and references to the old compatibility layout. Keep the environment redirection logic unchanged.packages/tools/src/__tests__/memory-tool.test.ts-66-67 (1)
66-67: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the storage fake isolated from the real home directory.
Returning
os.homedir()/.llxprtpreserves the legacy global path and can direct the fake’s real filesystem calls at a developer’s state. Use an isolated temporary directory, ideally with distinct canonical memory/data subdirectories.🤖 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/tools/src/__tests__/memory-tool.test.ts` around lines 66 - 67, Update the storage fake’s writeFile implementation to use an isolated temporary directory instead of the real home directory or legacy .llxprt path. Create distinct canonical memory and data subdirectories within that temporary location, and ensure the fake’s filesystem operations use those paths.packages/agents/src/core/TodoContinuationService.todoops.test.ts-14-20 (1)
14-20: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep a focused assertion for resolver propagation.
The fake
TodoStoreonly returns methods, so these tests now pass even ifTodoContinuationServicestops forwardingdataDirResolver. Retain one targeted assertion that the created store receives the injected resolver; this is the migration contract, not incidental mock behavior.Also applies to: 94-94, 243-246, 259-262, 275-278
🤖 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/agents/src/core/TodoContinuationService.todoops.test.ts` around lines 14 - 20, Retain a focused constructor assertion in the TodoContinuationService tests that verifies the fake TodoStore receives the injected dataDirResolver. Keep the existing outcome-based assertions and avoid asserting unrelated constructor arguments or invocation details; apply the same resolver-propagation coverage to the corresponding test cases.packages/tools/src/tools/memoryTool.test.ts-66-67 (1)
66-67: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReplace legacy global-home fixtures with isolated canonical paths.
Both test fixtures still model
~/.llxprtas a global directory, conflicting with the new category contract and risking access to real developer state.
packages/tools/src/tools/memoryTool.test.ts#L66-L67: return isolated temporary memory/data paths instead ofpath.join(os.homedir(), '.llxprt').packages/tools/src/__tests__/interface-contracts.test.ts#L355-L368: replace/home/user/.llxprtwith a canonical config fixture or neutral temporary path.🤖 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/tools/src/tools/memoryTool.test.ts` around lines 66 - 67, Replace the getGlobalMemoryDir and getGlobalDataDir fixtures in packages/tools/src/tools/memoryTool.test.ts:66-67 with isolated temporary paths, avoiding os.homedir() and the real ~/.llxprt directory. In packages/tools/src/__tests__/interface-contracts.test.ts:355-368, replace /home/user/.llxprt with the canonical config fixture or a neutral temporary path so both tests use isolated, non-global locations.packages/agents/src/api/__tests__/session.spec.ts-90-94 (1)
90-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve the cleanup root through
Storage.When no override is set, this selects legacy
~/.llxprt, whileStorageselects the platform log directory. Cleanup can therefore miss the artifacts it created. UseStorage.getGlobalLogDir()here rather than duplicating environment resolution.Based on PR objective:
Storageis the canonical authority for global paths.🤖 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/agents/src/api/__tests__/session.spec.ts` around lines 90 - 94, Update the cleanup-root resolution in the test helper to use Storage.getGlobalLogDir() as the fallback instead of join(homedir(), '.llxprt') and duplicated environment-variable handling. Preserve the existing tmp/hash subpath construction so cleanup targets the artifacts created by Storage.packages/agents/src/api/__tests__/capabilityGaps.integration.spec.ts-48-52 (1)
48-52: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail when cleanup exceeds the bound.
The timeout currently resolves, so a hung
built.cleanup()passes teardown and can leak resources into subsequent tests. Reject on timeout so the stuck cleanup is surfaced.Proposed fix
await Promise.race([ built.cleanup(), - new Promise<void>((resolve) => setTimeout(resolve, 5000)), + new Promise<never>((_, reject) => + setTimeout(() => reject(new Error('Agent cleanup timed out')), 5000), + ), ]);🤖 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/agents/src/api/__tests__/capabilityGaps.integration.spec.ts` around lines 48 - 52, Update the cleanup timeout in the teardown flow around built.cleanup() so the Promise.race rejects when the 5-second bound is exceeded instead of resolving. Preserve successful cleanup behavior while surfacing a hung cleanup as a failing test.shell-scripts/llxprt-paths.test.sh-54-57 (1)
54-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEarly "node not found" exit bypasses the failure check.
This exits 0 unconditionally, even if the
llxprt_is_abs_overrideassertions above (lines 46-50) already recorded failures in$failures.run-shell-tests.shonly checks this script's exit code, so a real regression here could be masked as a PASS whenevernodehappens to be unavailable.🐛 Proposed fix: honor `$failures` before the early exit
if ! command -v node >/dev/null 2>&1; then echo "node not found; skipping category resolution tests" + if [ "$failures" -gt 0 ]; then + echo "" + echo "$failures assertion(s) FAILED" + exit 1 + fi exit 0 fi🤖 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 `@shell-scripts/llxprt-paths.test.sh` around lines 54 - 57, Update the node-availability early-exit branch in llxprt-paths.test.sh to return the accumulated failures status instead of unconditionally exiting successfully. Preserve the skip message and ensure assertions recorded in failures, including llxprt_is_abs_override checks, propagate through the script’s exit code when node is unavailable.shell-scripts/cache-baseline-destructive.test.sh-26-29 (1)
26-29: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnquoted
rm -rftarget.
rm -rf $_TMP_DIRS_TO_CLEANUPrelies on word-splitting; if any collected path ever contains whitespace/glob chars it could delete unintended paths.🛡️ Proposed fix: quote each path individually
_cleanup_tmp_dirs() { - [ -n "$_TMP_DIRS_TO_CLEANUP" ] && rm -rf $_TMP_DIRS_TO_CLEANUP + for _d in $_TMP_DIRS_TO_CLEANUP; do + [ -n "$_d" ] && rm -rf -- "$_d" + done }🤖 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 `@shell-scripts/cache-baseline-destructive.test.sh` around lines 26 - 29, Update _cleanup_tmp_dirs to remove each collected temporary-directory path with individually quoted arguments, avoiding unquoted expansion and accidental word splitting or glob expansion. Preserve the existing cleanup behavior for all tracked paths.Source: Linters/SAST tools
shell-scripts/codex-oauth.sh-243-244 (1)
243-244: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winGate the credential warning in
shell-scripts/codex-oauth.sh:243-244. Send it to stderr and wrap it inCODEX_VERBOSEso normal output stays clean and the credential path isn’t disclosed by default.🤖 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 `@shell-scripts/codex-oauth.sh` around lines 243 - 244, Update the credential warning immediately after the chmod command so it only runs when CODEX_VERBOSE is enabled and sends the message to stderr; preserve the existing warning text while preventing AUTH_DIR disclosure during normal execution.
🧹 Nitpick comments (9)
packages/auth/src/keyring-token-store.ts (1)
158-200: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring contradicts the implementation. The comment states
tryCreateLockreturnsfalse(no throw) onEEXIST, butfs.open(lockPath, 'wx', ...)throwsEEXISThere — that error is whatacquireLockcatches to decide to keep polling. A maintainer trusting this doc could remove theEEXISTcatch inacquireLock, turning every contended acquisition into a thrown error instead of a poll. Recommend aligning the comment with actual control flow (EEXIST propagates and is handled by the caller).🤖 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/auth/src/keyring-token-store.ts` around lines 158 - 200, Update the docstring above tryCreateLock to state that EEXIST propagates from fs.open and is handled by acquireLock, while other errors also propagate; remove the inaccurate claim that EEXIST returns false. Keep the documented successful ownership and close-warning behavior unchanged.packages/a2a-server/vitest.config.ts (1)
106-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPreserve Vitest’s default excludes
test.excludereplaces Vitest’s built-in list. SpreadconfigDefaults.excludehere if you still want the default.git,cypress, and config-file exclusions.🤖 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/a2a-server/vitest.config.ts` around lines 106 - 116, Update the Vitest configuration’s test.exclude setting to include configDefaults.exclude along with the existing node_modules and dist patterns, preserving Vitest’s default exclusions such as .git, cypress, and configuration files. Import or reuse the standard configDefaults symbol in the vitest.config.ts configuration.scripts/legacy-paths/ast-scanner.ts (1)
41-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTypo:
DOT_LLPRT_PATTERN→DOT_LLXPRT_PATTERN.Missing the
X. Cosmetic only, but worth fixing for readability since this constant is referenced throughout the file.🤖 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 `@scripts/legacy-paths/ast-scanner.ts` at line 41, Rename the constant DOT_LLPRT_PATTERN to DOT_LLXPRT_PATTERN and update every reference to it throughout ast-scanner.ts, preserving the existing regular expression and behavior.scripts/check-legacy-paths.ts (2)
596-683: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelf-test doesn't cover all defined patterns.
unix-home-usersandwindows-drive-homeare defined inLEGACY_PATTERNS(scripts/legacy-paths/config.tslines 74-88) but have no correspondingRED_CASESentry here, so--self-testnever verifies they actually fire. Given the self-test's stated purpose (RED/GREEN validation of every pattern), this is a blind spot for two patterns.➕ Suggested additions to RED_CASES
+ { + name: 'explicit absolute home /Users/<name>/.llxprt', + sample: "const f = '/Users/alice/.llxprt/settings.json';", + expectPattern: 'unix-home-users', + }, + { + name: 'Windows drive-letter home C:\\Users\\<name>\\.llxprt', + sample: 'set CONFIG=C:\\Users\\alice\\.llxprt\\settings.json', + expectPattern: 'windows-drive-home', + },🤖 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 `@scripts/check-legacy-paths.ts` around lines 596 - 683, Add RED_CASES entries for the uncovered LEGACY_PATTERNS symbols unix-home-users and windows-drive-home in the self-test definitions, using representative samples that trigger each pattern and matching expectPattern values. Keep the existing RED/GREEN validation flow unchanged so --self-test verifies every defined pattern.
383-393: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
packages/is walked once per extension-specific glob.
SCANNED_TREEShas 7 separatepackages/*/src/**/*.EXTentries that all resolve to the samebaseDir(packages/), socollectCandidatestriggers up to 7 full recursive walks ofpackages/per run before deduplicating into theSet. A cache keyed bybaseDirincollectFromGlobTree/walkDirwould avoid the repeated I/O.🤖 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 `@scripts/check-legacy-paths.ts` around lines 383 - 393, Update collectFromGlobTree and its walkDir traversal to cache results by baseDir, so repeated extension-specific entries in SCANNED_TREES reuse the same packages/ walk instead of performing duplicate recursive I/O. Preserve collectCandidates’ existing Set deduplication and error behavior while ensuring each baseDir is traversed at most once per run.scripts/legacy-path-allowlist.json (1)
2-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
LLXPRT_DIRalternative in this allowlist entry.The bare
LLXPRT_DIRalternative (no anchoring tohomedir()/~/) suppresses any future line instorage.tsmerely containing that substring, wider than the "narrow allowlist" principle documented incheck-legacy-paths.ts. Consider scoping it closer to the actual migration-helper construction (e.g. requiringhomedir()/~/nearby) to avoid masking new violations added later to the same file.🤖 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 `@scripts/legacy-path-allowlist.json` around lines 2 - 6, Update the allowlist entry for storage.ts by narrowing the pattern used for the getLegacyLlxprtDir migration helper: remove the standalone LLXPRT_DIR alternative and require it to appear in the legacy path construction context alongside homedir() or the ~/.llxprt form. Preserve coverage for the existing migration-only helper without allowing unrelated future references in storage.ts.packages/a2a-server/src/config/extension.userdirs.test.ts (1)
49-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated Storage-env-redirect boilerplate into a shared test helper.
Six test files independently reimplement the same "snapshot
process.env, redirectLLXPRT_*_HOMEto a temp dir, restore inafterEach" pattern. It has already drifted (some files redirect onlyLLXPRT_DATA_HOME/LLXPRT_CONFIG_HOME, others redirect all four category vars; some guard with try/catch, some don't), which will only get worse as more tests copy it.
packages/a2a-server/src/config/extension.userdirs.test.ts#L49-L54: promote this version (with the beforeEach-before-createHarness ordering guard) into a shared helper, e.g.withRedirectedStorageEnv(dir), in a shared test-utils module.packages/a2a-server/src/config/extension.compat.test.ts#L52-L57: replace the localENV_KEYS/SAVED_ENVblock with the shared helper.packages/a2a-server/src/config/extension.test.ts#L47-L52: replace the localENV_KEYS/SAVED_ENVblock with the shared helper.packages/cli/src/config/extension.part2.test.ts#L106-L117: replace the localENV_KEYS/SAVED_ENVblock with the shared helper.packages/cli/src/config/extension.part3.test.ts#L108-L119: replace the localENV_KEYS/SAVED_ENVblock (and its try/catch restoration) with the shared helper.packages/cli/src/config/extension.part4.test.ts#L120-L136: replace the localENV_KEYS/SAVED_ENVblock (and its try/catch restoration) with the shared helper.🤖 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/a2a-server/src/config/extension.userdirs.test.ts` around lines 49 - 54, Extract the complete four-key environment snapshot, temporary-directory redirection, and restoration lifecycle from packages/a2a-server/src/config/extension.userdirs.test.ts into a shared test helper such as withRedirectedStorageEnv(dir), preserving its beforeEach-before-createHarness ordering guard. Replace each local ENV_KEYS/SAVED_ENV implementation in packages/a2a-server/src/config/extension.compat.test.ts (lines 52-57), packages/a2a-server/src/config/extension.test.ts (lines 47-52), packages/cli/src/config/extension.part2.test.ts (lines 106-117), packages/cli/src/config/extension.part3.test.ts (lines 108-119), and packages/cli/src/config/extension.part4.test.ts (lines 120-136) with the shared helper, removing their duplicated try/catch restoration logic.packages/tools/src/tools/todo-read.ts (1)
84-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate read-fallback-then-throw logic across
todo-read.tsandtodo-write.ts. Both files independently implement "tryreadTodos, elsegetTodos, else throw a wired-store-missing error" with near-identical error message construction.
packages/tools/src/tools/todo-read.ts#L84-L106: extractreadTodosFromStore's body into a shared helper (e.g.resolveTodosFromStore(store, toolName, sessionId, agentId)).packages/tools/src/tools/todo-write.ts#L216-L239: replacereadOldTodoswith a call to the same shared helper instead of re-implementing the fallback/throw logic.🤖 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/tools/src/tools/todo-read.ts` around lines 84 - 106, Extract the shared readTodos/getTodos fallback and wired-store-missing error from readTodosFromStore in packages/tools/src/tools/todo-read.ts#L84-L106 into a reusable helper such as resolveTodosFromStore, preserving the sessionId, agentId, and tool-name context in the error. Update readTodosFromStore to delegate to that helper, and replace readOldTodos in packages/tools/src/tools/todo-write.ts#L216-L239 with the same helper call; both sites should use the centralized implementation.packages/core/src/tools-adapters/CoreStorageServiceAdapter.ts (1)
32-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared
coreStorageServiceAdaptersingleton isn't actually used by the productionMemoryTool/Todo wiring.CoreStorageServiceAdapter.tsdocuments the singleton as required soMemoryTool,memoryDiscovery,prompts, and the CLI memory command all agree on one directory-resolving instance, buttoolRegistryFactory.tsconstructs a separateCoreStorageServiceAdapterinstance and wires that intoMemoryTooland the Todo resolver instead. The class is stateless today (both instances just forward to staticStoragecalls), so there's no current behavioral bug, but the documented single-source-of-truth guarantee is not actually honored for the production factory path, and future instance-level test seams (e.g.vi.spyOn(coreStorageServiceAdapter, ...)) wouldn't affect this wiring.
packages/core/src/tools-adapters/CoreStorageServiceAdapter.ts#L32-L42: keep the singleton export as-is; it's the intended composition-root instance.packages/core/src/config/toolRegistryFactory.ts#L350-L354: import and use the exportedcoreStorageServiceAdaptersingleton instead ofnew CoreStorageServiceAdapter()for wiringMemoryTooland the Todo resolver.🤖 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-adapters/CoreStorageServiceAdapter.ts` around lines 32 - 42, Use the exported coreStorageServiceAdapter singleton from CoreStorageServiceAdapter.ts for production wiring in packages/core/src/config/toolRegistryFactory.ts lines 350-354, replacing the separately constructed CoreStorageServiceAdapter used by MemoryTool and the Todo resolver; leave the singleton declaration unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6441328a-ecfa-48e8-91bf-e98b06f833cd
⛔ Files ignored due to path filters (9)
dev-docs/cherrypicking.mdis excluded by!dev-docs/**dev-docs/profileandsettings.mdis excluded by!dev-docs/**project-plans/issue2606.mdis excluded by!project-plans/**scripts/tmux-script.approval-ui.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.blank-live-shell-output.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.issue2208-newlines.fake.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.shellmode-esc-cancel-kills.llxprt.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.shellmode-esc-cancel.llxprt.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.slash-autocomplete.jsonis excluded by!scripts/tmux-script.*.json
📒 Files selected for processing (237)
.github/workflows/ci.ymldocs/EMOJI-FILTER.mddocs/architecture/message-bus-architecture.mddocs/checkpointing.mddocs/cli/commands.mddocs/cli/configuration.mddocs/cli/context-dumping.mddocs/cli/enterprise.mddocs/cli/profiles.mddocs/cli/providers.mddocs/cli/sandbox-profiles.mddocs/cli/skills.mddocs/cli/themes.mddocs/cli/tutorials/skills-getting-started.mddocs/debug-logging.mddocs/disabled-tools.mddocs/extension.mddocs/gemini-cli-tips.mddocs/hooks/api-reference.mddocs/hooks/best-practices.mddocs/hooks/creating-custom-hooks.mddocs/hooks/index.mddocs/index.mddocs/local-models.mddocs/migration/approval-mode-to-policies.mddocs/oauth-setup.mddocs/policy-configuration.mddocs/prompt-configuration.mddocs/recipes/claude-pro-workflow.mddocs/recipes/free-tier-setup.mddocs/recipes/index.mddocs/reference/application-directories.mddocs/reference/ephemerals.mddocs/reference/profiles.mddocs/sandbox.mddocs/settings-and-profiles.mddocs/subagents.mddocs/telemetry-privacy.mddocs/telemetry.mddocs/tools/index.mddocs/tools/mcp-server.mddocs/tools/memory.mddocs/troubleshooting.mddocs/tutorials/sandbox-setup.mddocs/zed-integration.mdpackage.jsonpackages/a2a-server/bun-preload-storage-isolation.tspackages/a2a-server/package.jsonpackages/a2a-server/src/config/config.env.test.tspackages/a2a-server/src/config/config.tspackages/a2a-server/src/config/extension.compat.test.tspackages/a2a-server/src/config/extension.test.tspackages/a2a-server/src/config/extension.tspackages/a2a-server/src/config/extension.userdirs.test.tspackages/a2a-server/src/config/settings.test.tspackages/a2a-server/src/config/settings.tspackages/a2a-server/src/storage-isolation.bun.test.tspackages/a2a-server/vitest.config.tspackages/agents/src/api/__tests__/capabilityGaps.integration.spec.tspackages/agents/src/api/__tests__/session.spec.tspackages/agents/src/api/__tests__/tsconfig.providersSourceMapping.spec.tspackages/agents/src/api/control/authState.tspackages/agents/src/app-services/profiles.tspackages/agents/src/compression/__tests__/compression-retry-provider-hardlimit.test.tspackages/agents/src/core/TodoContinuationService.complexity.test.tspackages/agents/src/core/TodoContinuationService.postturn.test.tspackages/agents/src/core/TodoContinuationService.reminders.test.tspackages/agents/src/core/TodoContinuationService.todoops.test.tspackages/agents/src/core/TodoContinuationService.tspackages/agents/src/core/__tests__/todoContinuation.characterization.test.tspackages/agents/src/core/client.tspackages/agents/src/core/subagentToolProcessing.tspackages/agents/src/tools/task.tspackages/agents/tsconfig.jsonpackages/auth/src/__tests__/keyring-token-store.di.test.tspackages/auth/src/__tests__/keyring-token-store.lock-behavior.test.tspackages/auth/src/__tests__/oauth-errors.spec.tspackages/auth/src/__tests__/token-store.refresh-race.spec.tspackages/auth/src/__tests__/token-store.spec.tspackages/auth/src/keyring-token-store.tspackages/auth/src/oauth-errors.tspackages/auth/src/proxy/proxy-token-store.tspackages/auth/src/token-store.tspackages/cli/src/config/extension.part2.test.tspackages/cli/src/config/extension.part3.test.tspackages/cli/src/config/extension.part4.test.tspackages/cli/src/config/extension.skills.test.tspackages/cli/src/config/extension.test.tspackages/cli/src/config/extension.tspackages/cli/src/config/extensions/rootAwareManagement.test.tspackages/cli/src/config/extensions/rootAwareUninstallIdentity.test.tspackages/cli/src/config/extensions/update.test.tspackages/cli/src/config/logging/loggingConfig.test.tspackages/cli/src/config/memoryReconciliation.concurrent.test.tspackages/cli/src/config/memoryReconciliation.test.tspackages/cli/src/config/memoryReconciliation.tspackages/cli/src/config/migrationTypes.tspackages/cli/src/config/pathMigration.overrideValidity.test.tspackages/cli/src/config/pathMigration.profileRepair.concurrency.test.tspackages/cli/src/config/pathMigration.tspackages/cli/src/config/reconcileLock.test.tspackages/cli/src/config/reconcileLock.tspackages/cli/src/config/settings.env.test.tspackages/cli/src/config/settings.tspackages/cli/src/config/settingsSchema.tspackages/cli/src/config/yargsOptions.help.test.tspackages/cli/src/config/yargsOptions.tspackages/cli/src/integration-tests/__tests__/oauth-buckets.integration.spec.tspackages/cli/src/integration-tests/compression-todo.integration.test.tspackages/cli/src/integration-tests/oauth-timing.integration.test.tspackages/cli/src/integration-tests/todo-continuation.integration.test.tspackages/cli/src/ui/commands/memoryCommand.tspackages/cli/src/ui/contexts/TodoProvider.tsxpackages/cli/src/utils/sandbox-macos-permissive-closed.sbpackages/cli/src/utils/sandbox-macos-permissive-open.sbpackages/cli/src/utils/sandbox-macos-permissive-proxied.sbpackages/cli/src/utils/sandbox-macos-restrictive-closed.sbpackages/cli/src/utils/sandbox-macos-restrictive-open.sbpackages/cli/src/utils/sandbox-macos-restrictive-proxied.sbpackages/cli/src/utils/sandbox-seatbelt.test.tspackages/cli/src/utils/sandbox-seatbelt.tspackages/cli/test/integration/auth-e2e.integration.test.tspackages/cli/test/providers/providerAliases.test.tspackages/cli/test/ui/commands/authCommand-logout.test.tspackages/core/src/__tests__/root-barrel-exports.test.tspackages/core/src/auth-factories.fallback.test.tspackages/core/src/auth-factories.lockdir.test.tspackages/core/src/auth-factories.tspackages/core/src/config/subagentManager.tspackages/core/src/config/toolRegistryFactory.tspackages/core/src/config/types.tspackages/core/src/core/prompts.coreMemory.test.tspackages/core/src/core/prompts.tspackages/core/src/policy/persistence.test.tspackages/core/src/prompt-config/defaults/index.tspackages/core/src/prompt-config/index.tspackages/core/src/prompt-config/installer/backup-operations.collision.test.tspackages/core/src/prompt-config/installer/backup-operations.test.tspackages/core/src/prompt-config/installer/backup-operations.tspackages/core/src/prompt-config/installer/file-writer.tspackages/core/src/prompt-config/installer/prompt-installer.backup-cleanup.test.tspackages/core/src/prompt-config/installer/prompt-installer.backup-unique.test.tspackages/core/src/prompt-config/prompt-installer.backup-hash.test.tspackages/core/src/prompt-config/prompt-installer.test-helpers.tspackages/core/src/prompt-config/prompt-installer.test.tspackages/core/src/prompt-config/prompt-installer.tspackages/core/src/tools-adapters/CoreStorageServiceAdapter.tspackages/core/src/tools-adapters/CoreTodoServiceAdapter.tspackages/core/src/tools-adapters/index.tspackages/core/src/utils/memoryDiscovery.roundtrip.test.tspackages/core/src/utils/memoryDiscovery.subfunctions.test.tspackages/core/src/utils/memoryDiscovery.test.tspackages/core/src/utils/memoryDiscovery.tspackages/providers/src/__tests__/LoadBalancingProvider.failover.retryable.test.tspackages/providers/src/auth/__tests__/auth-flow-orchestrator.spec.tspackages/providers/src/auth/__tests__/codex-oauth-provider.test.tspackages/providers/src/auth/__tests__/proactive-renewal-manager.spec.tspackages/providers/src/auth/__tests__/token-access-coordinator.spec.tspackages/providers/src/auth/auth-flow-orchestrator.tspackages/providers/src/auth/migration.tspackages/providers/src/auth/oauth-manager-initialization.spec.tspackages/providers/src/auth/oauth-manager.auth-lock.spec.tspackages/providers/src/auth/oauth-manager.concurrency.spec.tspackages/providers/src/auth/oauth-manager.refresh-race.spec.tspackages/providers/src/auth/proactive-renewal-manager.tspackages/providers/src/auth/proxy/__tests__/credential-proxy-server.test.tspackages/providers/src/auth/proxy/__tests__/e2e-credential-flow.test.tspackages/providers/src/auth/proxy/__tests__/integration.test.tspackages/providers/src/auth/proxy/__tests__/oauth-exchange.spec.tspackages/providers/src/auth/proxy/__tests__/oauth-initiate.spec.tspackages/providers/src/auth/proxy/__tests__/oauth-poll.spec.tspackages/providers/src/auth/proxy/__tests__/refresh-coordinator.test.tspackages/providers/src/auth/proxy/__tests__/refresh-flow.spec.tspackages/providers/src/auth/token-access-coordinator.tspackages/settings/src/settings/registry/registry-entries-2.tspackages/settings/src/storage/__tests__/Storage.test.tspackages/storage/package.jsonpackages/storage/src/config/path-resolver.test.tspackages/storage/src/config/path-resolver.tspackages/storage/src/config/storage.test.tspackages/storage/src/config/storage.tspackages/storage/src/secure-store/secure-store.fallback.test.tspackages/storage/src/secure-store/secure-store.tspackages/storage/tsconfig.jsonpackages/telemetry/src/debug/MockConfigurationManager.tspackages/tools/src/__tests__/filesystem-tools.test.tspackages/tools/src/__tests__/interface-contracts.test.tspackages/tools/src/__tests__/memory-tool.test.tspackages/tools/src/__tests__/neutral-types.test.tspackages/tools/src/__tests__/todo-contract.test-d.tspackages/tools/src/index.tspackages/tools/src/interfaces/IStorageService.tspackages/tools/src/tools/memoryTool.test.tspackages/tools/src/tools/memoryTool.tspackages/tools/src/tools/todo-pause.tspackages/tools/src/tools/todo-read.tspackages/tools/src/tools/todo-store-injection.test.tspackages/tools/src/tools/todo-store-single-resolve.test.tspackages/tools/src/tools/todo-store.tspackages/tools/src/tools/todo-write.tsschemas/settings.schema.jsonscripts/bun-test-manifest.tsscripts/check-agents-api-surface.mjsscripts/check-genai-enclave.tsscripts/check-legacy-paths.tsscripts/genai-enclave/file-discovery.tsscripts/legacy-path-allowlist.jsonscripts/legacy-paths/ast-scanner.tsscripts/legacy-paths/config.tsscripts/lint-all.shscripts/run-lint.tsscripts/run-shell-tests.shscripts/run_bun_tests.tsscripts/sandbox_command.tsscripts/telemetry.tsscripts/telemetry_utils.jsscripts/tests/check-agents-api-surface.test.tsscripts/tests/codex-credential-scripts.test.tsscripts/tests/genai-enclave-guard-deleted-files.test.tsscripts/tests/genai-enclave-guard-helpers.tsscripts/tests/legacy-paths-ast-scanner.test.tsscripts/tests/legacy-paths-guard-helpers.tsscripts/tests/legacy-paths-guard.test.tsscripts/tests/legacy-paths-scanned-trees-validation.test.tsscripts/tests/nightly-bun-native-smoke.test.jsscripts/tests/telemetry-user-settings-path.test.tsscripts/tests/telemetry-utils-path.test.tsscripts/verify-oauth-integration.shshell-scripts/cache-baseline-destructive.test.shshell-scripts/cache-baseline-test.shshell-scripts/codex-call.shshell-scripts/codex-models.shshell-scripts/codex-oauth.shshell-scripts/issue489-acceptance-test.shshell-scripts/llxprt-paths.shshell-scripts/llxprt-paths.test.shtsconfig.scripts.json
💤 Files with no reviewable changes (3)
- packages/providers/src/auth/migration.ts
- packages/providers/src/auth/oauth-manager.concurrency.spec.ts
- packages/providers/src/auth/oauth-manager.auth-lock.spec.ts
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)
docs/settings-and-profiles.md (1)
19-19: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a language to this fenced block.
This violates MD040; use
textfor the interactive command example.Proposed fix
-``` +```text🤖 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 `@docs/settings-and-profiles.md` at line 19, Update the fenced code block in the settings-and-profiles documentation to specify the text language, using ```text for the interactive command example so it satisfies MD040.Source: Linters/SAST tools
🤖 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 `@scripts/tests/check-agents-api-surface.test.ts`:
- Around line 100-105: Update resolveBuildTimeoutMs to reject timeout overrides
whose floored numeric value is below 1, preventing fractional values such as 0.9
from becoming a zero-millisecond timeout. Preserve the existing flooring
behavior for valid positive values.
---
Outside diff comments:
In `@docs/settings-and-profiles.md`:
- Line 19: Update the fenced code block in the settings-and-profiles
documentation to specify the text language, using ```text for the interactive
command example so it satisfies MD040.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0020141d-658a-4270-810f-50d55a9d6090
⛔ Files ignored due to path filters (3)
scripts/tmux-script.approval-ui.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.issue2208-newlines.fake.jsonis excluded by!scripts/tmux-script.*.jsonscripts/tmux-script.slash-autocomplete.jsonis excluded by!scripts/tmux-script.*.json
📒 Files selected for processing (12)
.github/workflows/ci.ymldocs/cli/configuration.mddocs/cli/profiles.mddocs/reference/ephemerals.mddocs/reference/profiles.mddocs/settings-and-profiles.mddocs/troubleshooting.mddocs/zed-integration.mdpackage.jsonschemas/settings.schema.jsonscripts/check-agents-api-surface.mjsscripts/tests/check-agents-api-surface.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- schemas/settings.schema.json
- docs/cli/profiles.md
- docs/zed-integration.md
- docs/reference/ephemerals.md
- docs/reference/profiles.md
- .github/workflows/ci.yml
- docs/cli/configuration.md
- docs/troubleshooting.md
- scripts/check-agents-api-surface.mjs
- package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@shell-scripts/cache-baseline-destructive.test.sh`:
- Around line 27-30: Update _cleanup_tmp_dirs so _TMP_DIRS_TO_CLEANUP entries
are stored and iterated safely, removing each directory with a quoted expansion
instead of unquoted word splitting and glob expansion. Preserve the existing
empty-list guard and cleanup behavior.
- Around line 59-66: The behavioral tests in cache-baseline-destructive.test.sh
do not consistently validate the target script’s exit status. Update each
affected test block, including the no-opt-in, absolute-path, and pipeline cases,
to capture the cache-baseline-test.sh status independently (preferably using the
benchmark executable stub) and assert the expected result; ensure the pipeline
preserves the target command’s status rather than head’s.
In `@shell-scripts/llxprt-paths.sh`:
- Around line 85-88: Update the validation cases for _llxprt_primary and the
corresponding secondary name before eval to require the first character be a
letter or underscore, while allowing subsequent characters to be letters,
digits, or underscores. Reject values such as “0” so positional parameters
cannot be interpreted as directory overrides.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c536be78-979a-40db-91be-c38b7da895d8
📒 Files selected for processing (9)
scripts/run-shell-tests.shshell-scripts/cache-baseline-destructive.test.shshell-scripts/cache-baseline-test.shshell-scripts/codex-call.shshell-scripts/codex-models.shshell-scripts/codex-oauth.shshell-scripts/issue489-acceptance-test.shshell-scripts/llxprt-paths.shshell-scripts/llxprt-paths.test.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- shell-scripts/issue489-acceptance-test.sh
- shell-scripts/cache-baseline-test.sh
- scripts/run-shell-tests.sh
- shell-scripts/codex-call.sh
- shell-scripts/codex-oauth.sh
- shell-scripts/codex-models.sh
- shell-scripts/llxprt-paths.test.sh
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. |
17 tasks
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.
TLDR
Completes the remaining OS-standard global-path migration by making Storage the single authority for config, data, cache, and runtime-state paths. This moves active OAuth fallback storage and locks, global memory, user extensions, A2A state, sandbox grants, telemetry, and maintainer scripts onto the canonical path contract while preserving workspace-local .llxprt behavior and bounded legacy migration inputs.
The change also adds behavioral reconciliation for memory previously written to the data directory, safety-first advisory locks, atomic no-overwrite prompt backups, and repository guards that prevent legacy global path algorithms from returning.
Dive Deeper
Canonical path authority
OAuth and credential safety
Migration and reconciliation
Extensions, A2A, and sandboxing
Backups, tools, and maintainability
Reviewer Test Plan
Testing Matrix
Local verification completed successfully:
Linked issues / bugs
Fixes #2606
Summary by CodeRabbit
~/.llxprtpaths with application-directory locations and overrides.