Non-fatal chown fallback for macOS bind mounts (complement to #1307)#1537
Non-fatal chown fallback for macOS bind mounts (complement to #1307)#1537ztech-gthb wants to merge 2 commits into
Conversation
Complements coleam00#1307. On macOS Docker Desktop with VirtioFS bind mounts, host UIDs (e.g. 501) don't map to appuser (1001) and chown fails — the script then exit 1s and the container crash-loops. Now treats chown failure as a warning, falls back to running as root, and exports IS_SANDBOX=1 so ClaudeProvider skips its UID-0 check (we're still sandboxed by Docker).
📝 WalkthroughWalkthroughThe entrypoint now attempts ChangesRoot-Privilege Fallback Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-entrypoint.sh`:
- Line 17: The chown invocation currently silences stderr with `2>/dev/null`,
losing the real OS error; change the `if chown -Rh appuser:appuser /.archon
2>/dev/null; then` pattern to capture stderr into a variable (e.g.
`output=$(chown -Rh appuser:appuser /.archon 2>&1)`) and use the command's exit
status to decide the branch, then include that captured `output` in the warning
message so the exact chown error (like "Operation not permitted" or "Read-only
file system") is logged.
- Around line 17-23: The script currently unconditionally exports IS_SANDBOX=1
on any chown failure, silently bypassing the root-safety guard in
ClaudeProvider; change this so IS_SANDBOX is only set when the user explicitly
opted in (e.g., check if an opt-in env var is already set like IS_SANDBOX=1 or a
new explicit flag SANDBOX_FORCE=1) instead of auto-exporting it on failure.
Update the chown block around the chown -Rh appuser:appuser /.archon check and
the RUNNER assignment so that on failure you print the warning and leave
IS_SANDBOX alone unless the opt-in env var is present; if you need to support
macOS VirtioFS convenience, document/require users to set the opt-in variable
before starting the container rather than assigning IS_SANDBOX=1 automatically.
Ensure references to RUNNER and IS_SANDBOX in docker-entrypoint.sh remain
consistent with this opt-in behavior so ClaudeProvider's getProcessUid() guard
is not silently bypassed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
… opt-in (CodeRabbit review)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docker-entrypoint.sh (1)
27-33: Consider documentingARCHON_ALLOW_ROOT_FALLBACK=1in user-facing docs.The new env var only appears in the
exit 1error message. Users encountering the failure for the first time on macOS won't know to set it unless they read the error carefully. A one-liner in the README (or thedocker-compose.ymlexample as a commented-out variable) would make the macOS path discoverable without digging through container logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-entrypoint.sh` around lines 27 - 33, Add a short user-facing note about ARCHON_ALLOW_ROOT_FALLBACK=1 so macOS users can discover the workaround without reading container logs: update the README and the example docker-compose.yml to mention ARCHON_ALLOW_ROOT_FALLBACK (what it does, when to set it) and show a commented-out example line; also briefly document the resulting IS_SANDBOX behavior and security implications so users understand that setting ARCHON_ALLOW_ROOT_FALLBACK exports IS_SANDBOX=1 and allows the entrypoint to continue as root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docker-entrypoint.sh`:
- Around line 27-33: Add a short user-facing note about
ARCHON_ALLOW_ROOT_FALLBACK=1 so macOS users can discover the workaround without
reading container logs: update the README and the example docker-compose.yml to
mention ARCHON_ALLOW_ROOT_FALLBACK (what it does, when to set it) and show a
commented-out example line; also briefly document the resulting IS_SANDBOX
behavior and security implications so users understand that setting
ARCHON_ALLOW_ROOT_FALLBACK exports IS_SANDBOX=1 and allows the entrypoint to
continue as root.
|
Superseded by [follow-up PR — number to fill] — #1518 changed the chown block in incompatible ways and removed the silent IS_SANDBOX-fallback this PR introduced. The follow-up adds an explicit ARCHON_ALLOW_ROOT_FALLBACK opt-in across both /.archon and /home/appuser chowns, which is now the cleaner shape on top of #1518's base. |
Summary
chown -Rh appuser:appuser /.archonfails because the host controls ownership and host UIDs (e.g. 501) don't map to container'sappuser(1001). The script thenexit 1s and the container crash-loops.chownis now treated as a warning. Container falls back to running as root, withIS_SANDBOX=1exported soClaudeProviderskips its UID-0 safety check (we're still inside Docker — sandboxed in the meaningful sense).safe.directory-loop (already fixed by fix(docker): register safe.directory for all repos on bind-mount restart #1307), Linux behaviour (chown succeeds → unchanged path), Kubernetes/non-root behaviour (id != 0 → unchanged path), or the security model on Linux.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
Label Snapshot
risk: lowsize: XScorecore:docker-entrypointChange Metadata
bugcoreLinked Issue
safe.directoryissue but not thischownfailure mode.Validation Evidence (required)
bun run type-check,bun run lint,bun run test— touched file is a shell script, not TS; lint-staged was a no-op for.sh.Security Impact (required)
exit 1vs continuing).chownitself is unchanged; only the failure handling differs.IS_SANDBOX=1mirrors how Claude SDK itself handles known-sandboxed contexts; the container is still isolated by Docker.Compatibility / Migration
chownsucceeds are unchanged. Only the previously-fatal failure path is altered.IS_SANDBOX=1is exported in the fallback path. Users who already setIS_SANDBOXexplicitly are not overridden ifchownsucceeds.Human Verification (required)
gh_auth.status_okandcleanup_completedlog lines.:5174is reachable.--user-flag invocation (theelsebranch is unchanged structurally; behaviour-equivalent to pre-PR).Side Effects / Blast Radius (required)
chownto fail (highly unusual) would now silently get root-execution instead of exit. Mitigated by the explicitWARNING:log line.docker compose logs.IS_SANDBOX=1is observable in process env.Rollback Plan (required)
git revert <sha>). One-file change, no DB/state to migrate.ERROR: Failed to fix ownershipmessage — same symptom as before this PR.Risks and Mitigations
chownfails for an unrelated reason (filesystem error, capability missing) would now log a warning and run as root instead of failing fast.IS_SANDBOX=1skips aClaudeProviderUID safety check that exists for a reason.Summary by CodeRabbit