Repository navigation
claude-docker: seed settings.docker.json via entrypoint copy so in-session settings writes work - #58
Conversation
…ind mount Claude Code persists settings by renaming a tmp file over settings.json. With settings.docker.json bind-mounted directly at /root/.claude/settings.json that path is a mountpoint, and rename() over a mountpoint fails with EBUSY — so every in-session settings change (effort, model, theme) errored with 'Failed to set effort level: ... EBUSY: resource busy or locked'. It also made /model silently claim to save a new default that never landed. Forward the host file read-only at a seed path (/run/claude-docker/ settings.json) instead, and have entrypoint.sh copy it to /root/.claude/settings.json at container start. The copy is a regular file, so in-session settings writes succeed; they last for that run and are re-seeded from the host file on the next start — never written back. The entrypoint copy needs an ownership dance on warm volumes: the dir/file are HOST_UID-owned from a prior run's chown walk and container root has no CAP_DAC_OVERRIDE, so take ownership (CAP_CHOWN) before writing and let the existing chown walk hand everything back. Spec: host-config-parity amended in place — the requirement's intent (container uses the dedicated settings file) is unchanged; the mandated mechanism was the bug. Smoke: all cells now mount a settings fixture at the seed path and assert the copy exists, carries the sentinel, and can be atomically replaced (the exact regression); the RO cell doubles as the no-seed cell (--settings=0).
dtump
left a comment
There was a problem hiding this comment.
Review
Overview — Correct fix for the right root cause, with the regression pinned in spec and smoke tests. The capability reasoning in the entrypoint holds up against run.sh's actual --cap-drop ALL --cap-add CHOWN,SETUID,SETGID,DAC_READ_SEARCH set (run.sh:401): the seed read works via DAC_READ_SEARCH, rm+cp instead of cp -f is genuinely required without DAC_OVERRIDE, and the chown root at entrypoint.sh:25 is handed back by the existing walk (entrypoint.sh:62). One test gap to close before merge.
Blockers
None.
Code quality (fix before merge)
- The re-seed guarantee is exercised but not asserted. The new spec scenario promises "the next container start re-seeds
settings.jsonfrom the host file" (spec.md:72), implemented by the unconditionalrm+cp(entrypoint.sh:26-27) — but the warm cell reruns with the same fixture content, so a naive re-implementation like[ -f settings.json ] || cpwould pass every cell: pass 2 finds the sentinel left by pass 1. Fix shape: insmoke.sh(warm branch, ~line 272), overwrite${TMPROOT}/settings.docker.jsonin place with a second sentinel (e.g.SMOKE-SENTINEL-SETTINGS-V2) between the cold and warm passes — truncate-in-place keeps the inode, so the bind mount sees the new content — and pass the expected sentinel via env (e.g.EXPECT_SETTINGS_SENTINEL) instead of hardcoding it atassert-in-container.sh:327. The warm pass then proves the seed overwrote the previous copy, which is the exact behavioral contract the spec added.
Follow-ups (track as issues)
- The
.git/configoverlay very likely has the same bug this PR diagnosed.run.sh:347bind-mounts the overlay as a single-file mount (not:ro), and the comment atrun.sh:324-326claims container-sidegit config/git remote add"need to succeed" with writes landing in the overlay. But git writes config the same way Claude Code writes settings: create.git/config.lock, thenrename(2)it over.git/config— rename over a file mountpoint →EBUSY, no fallback in git. If confirmed, the overlay needs the same seed-copy treatment (or the comment and README trade-off text need correcting). Not blocking: pre-existing, a different feature with its own spec (container-git-config-overlay), and it needs an empirical repro first. - Uncovered spec path: "no seed + warm volume leaves a previously persisted
settings.jsonas-is" (spec.md:58, acknowledged atassert-in-container.sh:310-311— the no-seed cell is cold-only). Cheap to fold into the same smoke change as the code-quality item above, otherwise track it; the primary regression is covered.
Strengths
- The regression test tests the mechanism, not the symptom:
assert-in-container.sh:333-342performs the actual tmp-file rename Claude Code does, so any future return to a mountpoint (or an ownership regression that breaks the rename) fails CI in nine of ten cells, including warm, uid-0, and ephemeral. - The ownership-dance comment (
entrypoint.sh:18-22) states the non-obvious constraint (noCAP_DAC_OVERRIDE, whyrm+cp) rather than narrating the code. - Spec amended in place with intent preserved and the broken mechanism superseded; all three doc touchpoints (README:73, run.sh help text, spec) say the same thing.
- Reusing cell 9 for no-seed keeps the matrix at ten cells, documented at both call site (
ci.yml:201-205) and assertion site.
Summary
Land the one smoke-test improvement (assert the warm pass re-seeds with fresh content), open the .git/config-overlay EBUSY issue, then ship.
…s smoke cell Address PR #58 review: the warm cell reran with the same fixture content, so the spec's re-seed guarantee was exercised but never asserted — a `[ -f ] || cp` re-implementation of the entrypoint seed block would have passed every cell on pass 1's leftovers. - smoke.sh rewrites the fixture IN PLACE (inode preserved, so the :ro bind mount sees it) with a V2 sentinel between the cold and warm passes, and passes the expected sentinel via EXPECT_SETTINGS_SENTINEL instead of hardcoding it in assert-in-container.sh. - The warm cell gains a third pass with no seed mounted (EXPECT_SETTINGS=keep): a previously persisted settings.json must survive untouched — covers the spec's "No settings file" warm-volume path the review flagged as untested. Verified with a stubbed docker driver (3 passes, correct mounts/env, fixture inode stable across the rewrite) and a scenario matrix over check_settings (stale-V1-on-warm fails, keep/lost/leak/absent behave).
|
Review addressed in e4123bf. Code-quality 1 (re-seed asserted, not just exercised) — done as suggested: Follow-up 2 (no seed + warm volume keeps persisted settings) — folded in, since it rides the same plumbing: the warm cell gains a third pass with the seed mount dropped and Follow-up 1 ( Verification (no docker on this machine; the full matrix runs in CI): drove |
dtump
left a comment
There was a problem hiding this comment.
Re-reviewed after e4123bf — all findings resolved, no new ones. Ready to merge.
- Re-seed assertion (code quality): fixed as prescribed. The warm cell now rewrites the fixture in place with a V2 sentinel between passes (inode preserved, so the
:robind mount sees it) and asserts the warm pass carries V2 — a[ -f ] || cpre-implementation of the seed block can no longer pass on pass 1's leftovers. Sentinel plumbing viaEXPECT_SETTINGS_SENTINELis fail-closed (unset → explicit FAIL). - No-seed warm-volume path (follow-up 2): covered beyond what was asked — a third
EXPECT_SETTINGS=keeppass asserts the seed is absent at/runand the persistedsettings.jsonsurvives untouched and stays atomically replaceable. Sound because/runis container-scoped while/root/.claudeis the named volume, so the pass genuinely exercises the entrypoint's skip branch against persisted state. .git/configoverlay EBUSY (follow-up 1): tracked as #60.
Also checked: per-pass env injection has no duplicate -e flags, the empty-array guard matches the bash-3.2-safe idiom used elsewhere (macOS checklist unaffected), the stderr-WARN caveat update is correct (chown walk never touches /run), and ci.yml's cell inventory matches the new three-pass cell 5. CI green on this head, including the full smoke matrix.
Problem
Changing effort (or any setting) inside the container fails:
Claude Code persists settings atomically: write
settings.json.tmp.*, thenrename()oversettings.json. Becauserun.shbind-mountedsettings.docker.jsondirectly at/root/.claude/settings.json, that path is a mountpoint, and the kernel refuses to rename over a mountpoint →EBUSY. This is inherent to single-file bind mounts — dropping:rowould not help. Side effect:/modelclaims "saved as your default for new sessions" while the write silently failed, and an effort reset on model switch becomes impossible to undo.Fix
Forward the host file read-only at a seed path (
/run/claude-docker/settings.json, outside the persistent volumes) and haveentrypoint.shcopy it to/root/.claude/settings.jsonat container start:settings.docker.jsonexists, nothing is seeded and whatever a previous run persisted in the home volume is left alone (preserves behavior for users who never usedsettings.docker.json).Ownership subtlety: on warm volumes
/root/.claudeand a prior copy are HOST_UID-owned, and container root has noCAP_DAC_OVERRIDE— the entrypoint takes ownership (CAP_CHOWN) before writing; the existing chown walk hands everything back to HOST_UID.Spec
host-config-parityamended in place (no OpenSpec change dir): the requirement's intent — the container uses your dedicated settings file — is unchanged; the previously mandated mechanism (direct bind mount at the real path) was the bug. Added a scenario pinning the rename-over-settings.json behavior.Tests
--settings=0): entrypoint must start cleanly without a forwarded settings file.Verified locally: seed-block simulation (overwrite stale copy, 0600 perms, rename-over succeeds, absent-seed no-op) and
bash -non all touched scripts; the full container matrix runs in CI. Requires an image rebuild to pick up the newentrypoint.sh.Note: intentionally excludes the local
pins/claude-code.envbump (kept out of this PR).