Conversation
The container sandbox bind-mounted the host global config directory at the legacy /home/node/.llxprt destination, but the CLI inside the container resolves its config directory through Storage to /home/node/.config/llxprt-code. Nothing was mounted where the CLI looks, so every sandboxed launch saw an empty configuration and re-ran the first-time setup wizard, re-prompted folder trust, and could not see profiles, subagents, prompts, commands, policies, hooks or global memory. Mount the host config directory at path parity and pin all four canonical roots with LLXPRT_CONFIG_HOME / LLXPRT_DATA_HOME / LLXPRT_CACHE_HOME / LLXPRT_LOG_HOME so in-container resolution no longer depends on the container HOME. All four must be pinned: data, cache and log fall back to LLXPRT_CONFIG_HOME, so setting config alone would redirect container-local state into the mounted host directory. Data, cache and log stay container-local; mounting the data directory would push raw OAuth and provider credentials across the sandbox boundary, which is what #2946 removed. The legacy destination also made the in-container startup migration treat the mount as a legacy directory and copy the whole config into throwaway paths on every start. That exposed a second defect: categorizeEntry routed welcomeConfig.json, trustedFolders.json and skills/ to the data directory even though the application reads all three from the config directory. That is a host bug too - migrating from a legacy ~/.llxprt silently dropped the welcome marker, trusted-folder decisions and user skills. Add them to CONFIG_ENTRIES and bump the migration marker so already-migrated users re-place them.
- Revert MIGRATION_MARKER_VERSION 2→1: the bump re-ran the whole legacy copy pass and recreated every entry the user deleted, including oauth_creds.json (deleted on logout), which would silently re-authenticate a logged-out user. Replaced the stale-marker test with one pinning the actual safety property: a current marker means a deleted canonical entry is never recreated. - Resolve the config dir dynamically via Storage.getGlobalConfigDir() so the runtime legacy-fallback (cli.tsx sets LLXPRT_CONFIG_HOME at runtime) is honoured by the mount, not the module-load-time constant. - Pin the ephemeral data/cache/log roots from the real container HOME inside the entrypoint (not as host --env), so custom sandbox images whose default user home is not /home/node are not forced to mkdir under it. Filter those roots from SANDBOX_ENV so they cannot shadow the entrypoint export. - Collapse the async shouldUseCurrentUserInSandbox wrapper (identical sync semantics) and update both call sites. - Pass containerHome to mountGitConfigFiles instead of the hard-coded value; drop the defensive existsSync/mkdir (the startup lifecycle guarantees it); add the :z SELinux shared label to the config mount under podman. - Define explicit top-level skills/ precedence over tmp/skills/ via a deterministic sort (no-overwrite copy semantics make first-writer win). - Rewrite the inert/tautological tests and add the invariants the change relies on (HOME agreement, current-user branch, dynamic config dir, single CONFIG_HOME under SANDBOX_ENV, Windows path translation, entrypoint roots).
github-actions
Bot
added
the
maintainer:e2e:ok
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR updates legacy path migration and aligns container sandbox mounts, environment variables, home resolution, entrypoint paths, SSH-agent setup, tests, and documentation with canonical platform directories. ChangesSandbox paths and legacy migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
WalkthroughThis PR changes 16 file(s).
Changes
Magnitude🎯 2 (M) RelatedNo related items found. Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/cli/src/utils/sandbox-containers.test.ts`:
- Around line 549-556: Update volumeDestinations to parse Windows host paths
before extracting the container destination: handle a drive-letter prefix such
as C:\ so the destination is taken from the separator after the host path, while
preserving existing Unix volume and options parsing behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e14cb50-47d9-40b2-ab25-2e9e945bdf74
⛔ Files ignored due to path filters (1)
project-plans/issue-3081-sandbox-canonical-config-mount.mdis excluded by!project-plans/**
📒 Files selected for processing (15)
docs/sandbox.mddocs/tutorials/sandbox-setup.mdpackages/cli/src/config/pathMigration.profileRepair.test.tspackages/cli/src/config/pathMigration.test.tspackages/cli/src/config/pathMigration.tspackages/cli/src/utils/sandbox-containers.test.tspackages/cli/src/utils/sandbox-containers.tspackages/cli/src/utils/sandbox-entrypoint.tspackages/cli/src/utils/sandbox-env.tspackages/cli/src/utils/sandbox-exec.tspackages/cli/src/utils/sandbox-ssh.test.tspackages/cli/src/utils/sandbox-ssh.tspackages/cli/test-bun/pathMigration.issue3081.bun.tspackages/cli/test-bun/sandbox-env.bun.tsscripts/bun-test-manifest.ts
OpenCodeReview — automatic reviews suspendedAutomatic OCR reviews are suspended for this PR after 2 of 2 automatic reviews. To get more reviews you can:
|
|
Addressed the Windows volume-source parsing finding in commit 5460833. The test helper now skips the drive-letter colon before extracting the container destination, preserves Unix and optional volume-label parsing, and has a focused Windows-source regression test. The sandbox container suite passes 41/41. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Addressed the quoted os-release finding in commit ee3ce84. Sandbox user auto-detection now parses ID and ID_LIKE values, strips optional single or double quotes, and matches complete whitespace-delimited ID_LIKE tokens. Added direct Bun coverage for quoted/unquoted Debian, Ubuntu, and Debian-derived values; 20/20 sandbox-env tests pass. |
|
Evaluated this filesystem-stub finding against the source and did not change the tests. shouldUseCurrentUserInSandbox reads only process.env, os.platform(), and fs.readFileSync('/etc/os-release'); it does not call os.homedir(), resolve XDG paths, or read any other file. Both stubs are intentionally path-conditional and delegate every non-os-release read to the real readFileSync, which is narrower and more deterministic than a broad fake HOME setup. The suggested broader stub would reduce coverage and would not exercise any actual current path. |
|
Addressed both duplicate migration-safety findings in commit 8ef3f49. The test now invokes the real runStartupMigrationWithPath orchestrator after asserting the marker gate, then verifies the deleted oauth_creds.json remains absent. performMigration itself intentionally does not read the marker; runStartupMigrationWithPath is the production authority that composes shouldMigrate with the copy pass. The focused migration suite passes 5/5. |
1 participant
TLDR
Fixes the container sandbox path mismatch that made every sandboxed LLxprt launch look like a fresh install. The host global config directory was mounted at the legacy dot-llxprt destination, while the in-container CLI resolves config through the OS-standard path. The sandbox now mounts the canonical config directory at path parity, pins
LLXPRT_CONFIG_HOMEto the mount, and keeps data/cache/log roots container-local via entrypoint exports derived from the real containerHOME.Also corrects fresh legacy migrations so
welcomeConfig.json,trustedFolders.json, and userskills/are routed to the config directory where the application reads them.Dive Deeper
Container sandbox
Storage.getGlobalConfigDir(), so runtime migration fallback and explicit host overrides are honored.LLXPRT_CONFIG_HOMEto it.LLXPRT_DATA_HOME,LLXPRT_CACHE_HOME, andLLXPRT_LOG_HOMEfrom the real container$HOMEinside the trusted entrypoint. This is required because those roots otherwise fall back toLLXPRT_CONFIG_HOME, which would redirect ephemeral state into the host config mount. Resolving them container-side also supports custom images whose default home is not/home/node.SANDBOX_ENVattempts to override any of the four canonical roots before image/network/SSH side effects.:zshared SELinux label for the writable config bind mount.Legacy migration
Fresh/unmarked migrations now route these entries to config rather than data:
welcomeConfig.jsontrustedFolders.jsonskills/The migration marker remains version 1. A full marker bump was rejected because re-running the entire copy pass could resurrect deliberately deleted credentials, hooks, commands, profiles, and
.envfiles. Recovery of entries already misplaced by a completed v1 migration is tracked separately in #3085 as a bounded data-to-config reconciliation.Top-level
skills/deterministically wins over the historicaltmp/skills/source on same-name collisions.Review and verification
HOMEhandling, Windows path translation, reserved-root validation ordering, Podman labeling, migration categorization, marker safety, and skills precedence.Reviewer Test Plan
Run the complete validation suite:
Inspect generated container args via the focused suite:
Launch a Docker or Podman sandbox twice from a configured host install. Confirm:
HOME, not under the mounted config root.Set
SANDBOX_ENV=LLXPRT_CONFIG_HOME=/tmp/overrideand confirm startup fails before image/network/SSH setup with the reserved-key error.Testing Matrix
macOS Docker and Podman bind-mount checks passed. Windows and Linux behavior is covered by the platform-focused Bun tests; no physical Windows/Linux host was used for this PR. Seatbelt is unaffected by this container-only mount change and already resolves canonical roots directly through
Storage.Linked issues / bugs
Fixes #3081
Follow-up recovery for already-completed v1 migrations: #3085
Related security boundary: #2957
Summary by CodeRabbit
New Features
Bug Fixes
Documentation