fix(docker): probe the configured state dir before /workspace for UID/GID detection (#7027) - #7034
fix(docker): probe the configured state dir before /workspace for UID/GID detection (#7027)#7034jorgejiro wants to merge 1 commit into
Conversation
…/GID (nesquena#7027) UID/GID auto-detection probed /workspace before the configured HERMES_WEBUI_STATE_DIR. In a stock single-container image none of the priority-1 candidates exist, but /workspace does — owned by the image's build-time 1024:1024. Detection therefore returned the image's own owner, which carries no information about the host, while the one directory whose owner *is* the host UID by definition — the state-dir bind mount — was never probed. With a host-owned state mount and no explicit WANTED_UID the container remapped to 1024, failed its own state-dir writability check, and restart-looped. The log line made this expensive to debug: 1024 is also the fallback default, so "Auto-detected workspace UID: 1024" read as if detection had found nothing. - probe ${HERMES_WEBUI_STATE_DIR:-/app/data} first, for both UID and GID - keep /workspace as a lower-priority signal (unchanged for setups that actually bind-mount it), and keep the hermes-home probes from nesquena#668 - stop treating an explicitly supplied 1024 as "unset": the sentinel and a valid UID were the same number, so an operator who deliberately ran as 1024 got it overwritten by detection. The explicit/detected origin is persisted next to the value because `su` drops the environment when the script re-enters as the runtime user, so the second pass would otherwise see an explicit choice as a detected one. Tests: tests/test_7027_state_dir_uid_probe.py runs the real resolution block under bash with `stat` stubbed, covering the non-1024 state mount, the explicit 1024 override (including across the privilege drop), and the pre-existing /workspace + hermes-home fallback paths. A new state-dir-uid job in the Docker smoke workflow boots a real container on a host-owned state mount and gates on /health. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
| Filename | Overview |
|---|---|
| docker_init.bash | Prioritizes the state directory and persists value origins correctly, but independent UID/GID probing can combine owners from different mounts. |
| tests/test_7027_state_dir_uid_probe.py | Adds focused source and behavioral tests for probe precedence, explicit 1024 handling, privilege re-entry, and existing fallbacks. |
| .github/workflows/docker-smoke.yml | Adds end-to-end startup gates for state-dir detection and explicit 1024, while introducing additional mutable action references. |
| docs/docker.md | Documents the failure symptom, cause, corrected probe order, explicit-value behavior, and workaround for older images. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Root docker_init] --> B{WANTED UID/GID supplied?}
B -->|Yes| C[Mark explicit]
B -->|No| D[Read persisted values and sources]
D --> E{Detection allowed?}
E -->|Yes| F[Probe state dir]
F --> G[Probe hermes-home candidates]
G --> H[Probe workspace]
H --> I[Fallback 1024]
C --> J[Persist values and sources]
F --> J
G --> J
H --> J
I --> J
J --> K[Apply groupmod and usermod]
K --> L[Re-enter init as hermeswebui]
L --> D
Comments Outside Diff (1)
-
docker_init.bash, line 122-130 (link)If a separately mounted state directory has a non-root UID but root GID, the UID loop selects that directory while the GID loop skips it and selects a later probe. This creates an artificial UID:GID pair matching neither mount, producing incorrect host-visible ownership and potentially inaccessible shared-home files.
Knowledge Base Used: Bootstrap and deployment
Reviews (1): Last reviewed commit: "fix(docker): probe the configured state ..." | Re-trigger Greptile
| - uses: actions/checkout@v4 | ||
|
|
||
| - uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: Restore Docker image from cache | ||
| uses: docker/build-push-action@v6 |
There was a problem hiding this comment.
Workflow actions use mutable tags
The added job executes actions through mutable major-version tags, so an upstream tag change alters trusted CI code without a repository change. Pinning these references to full commit SHAs prevents tag movement from affecting the Docker build; this job's read-only token and lack of secrets limit the current exposure.
How this was verified: The new job references @v4, @v3, and @v6 while the workflow grants only contents: read and does not pass secrets.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks — two notes on the automated review, both now also reflected in the PR description. UID/GID probes can diverge (P2). Real, but it predates this PR rather than being introduced by it: on Mutable action tags. The new job reuses the same tags as the existing jobs in this workflow ( One thing I could not verify myself: the |
🔬 Exact-head gate — SAFE TO SHIP (queued for maintainer merge)Thanks @jorgejiro — well-reasoned fix for #7027. The entrypoint couldn't distinguish an operator's explicit Full gate — SAFE TO SHIP (head
Backend/entrypoint only, no visible surface, ships with a substantive test + extended CI smoke coverage. Queuing (Release-manager overnight gate — queued for maintainer merge, not autonomously tagged.) |
…xperimental) (#7090) * feat: add GLM-5.3 to Z.AI model list Add glm-5.3 as the newest zai entry in _PROVIDER_MODELS and a matching zai/glm-5.3 entry in _FALLBACK_MODELS, and bump the Z.AI onboarding default_model from glm-5.1 to glm-5.3 (Z.ai's current flagship; legacy GLM-5.2/5.1 requests are routed to GLM-5.3 per docs.z.ai). Reasoning gating needs no change: _zai_glm_classification() treats GLM >= 5.2 as effort-ladder capable, so glm-5.3 is already covered and pinned by tests/test_zai_reasoning_effort_gating.py. New regression coverage in tests/test_glm_5_3_catalog.py: catalog presence, newest-first ordering, fallback entry, onboarding default, full reasoning_effort ladder, and the get_available_models() payload. * test: restore _cfg_fingerprint in catalog test fixture Review follow-up (#7017): the isolation fixture snapshot restored cfg, _cfg_mtime, and _cfg_path but left _cfg_fingerprint pointing at the temporary config loaded by the payload test. api/config.py uses that fingerprint to distinguish in-memory overrides from changed files (config.py:371), so a stale value could make later same-process tests skip reloading a changed config. Snapshot and restore it like the rest. * fix: keep Z.AI onboarding default at glm-5.1 until direct API serves GLM-5.3 Review follow-up (#7017): GLM-5.3 is live on Z.ai's Coding Plan endpoint only; the direct api.z.ai endpoint the zai provider uses still lists the GLM-5.3 API as coming soon. Defaulting new direct-API users onto glm-5.3 would fail their first message, so the catalog addition stays (opt-in) and the default stays glm-5.1. Bump the default in a follow-up once the direct endpoint serves GLM-5.3. * fix(docker): probe the configured state dir before /workspace for UID/GID (#7027) UID/GID auto-detection probed /workspace before the configured HERMES_WEBUI_STATE_DIR. In a stock single-container image none of the priority-1 candidates exist, but /workspace does — owned by the image's build-time 1024:1024. Detection therefore returned the image's own owner, which carries no information about the host, while the one directory whose owner *is* the host UID by definition — the state-dir bind mount — was never probed. With a host-owned state mount and no explicit WANTED_UID the container remapped to 1024, failed its own state-dir writability check, and restart-looped. The log line made this expensive to debug: 1024 is also the fallback default, so "Auto-detected workspace UID: 1024" read as if detection had found nothing. - probe ${HERMES_WEBUI_STATE_DIR:-/app/data} first, for both UID and GID - keep /workspace as a lower-priority signal (unchanged for setups that actually bind-mount it), and keep the hermes-home probes from #668 - stop treating an explicitly supplied 1024 as "unset": the sentinel and a valid UID were the same number, so an operator who deliberately ran as 1024 got it overwritten by detection. The explicit/detected origin is persisted next to the value because `su` drops the environment when the script re-enters as the runtime user, so the second pass would otherwise see an explicit choice as a detected one. Tests: tests/test_7027_state_dir_uid_probe.py runs the real resolution block under bash with `stat` stubbed, covering the non-1024 state mount, the explicit 1024 override (including across the privilege drop), and the pre-existing /workspace + hermes-home fallback paths. A new state-dir-uid job in the Docker smoke workflow boots a real container on a host-owned state mount and gates on /health. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Release batch B: Docker UID fix + GLM-5.3 catalog (experimental) Two independently gate-passed contributor PRs, rebased fresh onto master and re-gated as a combined stage (Codex SAFE TO SHIP; full suite green except the 8 pre-existing approval-test failures that fail identically on clean origin/master — CI green on the same commit; tracked separately for a fix). - #7027 (@jorgejiro) probe configured state-dir before /workspace for Docker UID/GID; persist explicit marker so a supplied 1024 survives root->su re-entry (fixes the single-container restart loop) (#7034) - #7017 (@rh-id) add GLM-5.3 to the Z.AI model list (onboarding default stays glm-5.1) Co-authored-by: jorgejiro <jorgejiro@users.noreply.github.com> Co-authored-by: rh-id <rh-id@users.noreply.github.com> --------- Co-authored-by: Ruby Hartono <58564005+rh-id@users.noreply.github.com> Co-authored-by: Jorge <jorgejiro@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: n <a@n> Co-authored-by: jorgejiro <jorgejiro@users.noreply.github.com> Co-authored-by: rh-id <rh-id@users.noreply.github.com>
|
Shipped in experimental release exp-v0.52.230 (batched with the GLM-5.3 catalog add). Merged onto master via batch release PR #7090 — your commit content is in master and live on the experimental channel now. Thanks @jorgejiro for the fix. 🤖 Closed by the release-manager agent. |
Fixes #7027.
Implements the fix shape agreed in #7027 (comment).
Thinking Path
usermod/workspace— present and owned1024:1024in every stock image — was probed while the configured state directory, which is a bind mount by definition, was not1024as "unset"What Changed
docker_init.bash${HERMES_WEBUI_STATE_DIR:-/app/data}, ahead of the hermes-home candidates./workspacestays exactly where it was, as the priority-2 signal, for setups that genuinely bind-mount it. The hermes-home probes from Docker image: Permission denied writing to shared /opt/data volume when hermes-agent and hermes-webui run in separate containers #668 are untouched.[ -z "${WANTED_UID+x}" ] || [ "${WANTED_UID}" = "1024" ], so the fallback sentinel and a legitimate UID were the same number and an operator's explicit1024was overwritten by detection.explicit/detected) is persisted to$itdirnext to the value itself.docker_init.bashruns twice — once as root, thenexec sure-enters it ashermeswebuiwith the environment dropped — so without persisting the origin, the second pass reads1024back from the file and cannot tell an operator's explicit choice from the fallback. Re-detecting there would leaveWANTED_UIDdisagreeing with the user the script is already running as, which is a hard startup failure. A detected1024stays re-detectable on later runs, which is what the sentinel was for.tests/test_7027_state_dir_uid_probe.py(new) — 18 tests, described under Verification..github/workflows/docker-smoke.yml— newstate-dir-uidjob, described under Verification.docs/docker.md— new troubleshooting entry #9 with the symptom, the cause, and the full probe order;#7027added to Related issues.Why It Matters
The failure is a restart loop with an error that names the consequence rather than the cause:
Nothing in it points at UID detection, and because
1024is also the fallback, the precedingAuto-detected workspace UID: 1024line reads as though detection had found nothing — so the natural next step is to investigate why detection found nothing, when in fact it succeeded and picked the wrong source. The state directory is the one path that is a bind mount by definition in a single-container deploy, so its owner is always the host identity the container needs to match.Verification
Behavioural tests —
tests/test_7027_state_dir_uid_probe.pyextracts the real UID/GID resolution block fromdocker_init.bashand runs it under bash withstatstubbed, so ownership can be simulated without root. This follows the existing pattern intests/test_docker_env_readonly_vars.py; source-level assertions alone cannot catch ordering or shell-quoting regressions inside the block.The reproduction test is bound to the shape pinned in the issue — host directory owned
1001:1001bind-mounted as the state dir, stock/workspaceat1024:1024, no explicit IDs — rather than a fixture rebuilt from my reading of it. Coverage:/workspace(the issue)/workspaceHERMES_WEBUI_STATE_DIRunset → default pathWANTED_UID=1024+ 1001-owned mounts1024across thesuprivilege drop/workspaceowned 501/workspace1024$HERMES_HOMEprobeReverting
docker_init.bashalone turns 10 of the 18 red, including both behavioural anchors (state_mount_beats_stock_workspace,explicit_1024_is_not_overwritten). The 8 that still pass without the fix are the ones pinning previously shipped behaviour — that is the intended split.Full suite —
./scripts/test.shon Python 3.11, run twice: once on this branch, once onmasterwith an otherwise identical environment. The failure sets are byte-identical (319 failing ids on both), so this branch introduces no regressions; it adds exactly 18 passes (12903 vs 12885).Those 312 are environmental on my machine, not pre-existing repo breakage: 224 of them are
FileNotFoundError: [Errno 2] No such file or directory: 'node'— I have no Node installed, and the JS-evaluating tests need it. CI has it.tests/test_compression_phantom_barrier.pywas excluded from both runs becauseplaywrightis not in my local venv; it is unrelated to this change.Docker-adjacent suites specifically (
test_issue569_579.py,test_docker_docs_and_readonly.py,test_issue2237_docker_chown_git_objects.py,test_issue1908_docker_hardening.py,test_docker_env_readonly_vars.py, and the rest that readdocker_init.bash): 108 passed.scripts/ruff_lint.pyclean,scripts/critical_markdown_check.pyclean ondocs/docker.md,bash -n docker_init.bashclean, and everyrun:block in the modified workflow parses underbash -n.Startup health — the product gate named in the issue comment is
/health, which no source-level invariant can prove, so it is a newstate-dir-uidjob indocker-smoke.ymlrather than a pytest assertion. It boots a real container the way the issue does —HERMES_WEBUI_STATE_DIRbind-mounted from a host directorychowned to1001:1001, noWANTED_UID, and deliberately no/workspacemount so the image's own1024-owned/workspaceis the competing signal — then requires/healthto return 200, the log to name the state dir as the detection source,id -u hermeswebuito actually be 1001, and the startup log to be free of the error signatures. A second step covers the explicit-1024half; it additionally mounts a 1001-owned hermes-home, because without that the pre-fix script would land on 1024 anyway via/workspaceand the step would pass for the wrong reason.What I could not verify myself: I have no Docker daemon on this machine, so that job has never executed on my side — it is verified by syntax only. Docker is owned by the runner, not by this repo's test suite, so please treat the CI run on this PR as the authoritative evidence for it, and I will iterate on the job if it comes back red.
Contract Routing
Task type: bug fix in container startup, with a public-doc update
Touched areas:
docker_init.bashUID/GID resolution,docs/docker.md, Docker smoke workflowRelevant public docs:
AGENTS.mdCONTRIBUTING.mddocs/CONTRACTS.mddocs/docker.mdScope boundaries: probe order and explicit-value handling only. No change to
usermod/groupmod, the chown walk, the read-only-root branch, or any compose file.Evidence needed before claiming done: bash-level regressions for the reported shape plus the pre-existing fallbacks, and a real container reaching
/healthon a host-owned state mount.Contract Change
WANTED_UID/WANTED_GIDequal to1024were treated as unset and re-derived by auto-detection, regardless of whether an operator had set them; detection ranked/workspaceabove the configured state directory.1024; the configured state directory ranks first,/workspaceremains the next signal.docs/docker.md(new §9 documenting the full order),tests/test_7027_state_dir_uid_probe.py.tests/test_issue569_579.pystill passes unmodified — the/workspaceand hermes-home probes it pins are unchanged.1024was simultaneously the sentinel and a valid identity, and the pre-fix order let an image-owned path outrank a host-owned one. Deployments that set IDs explicitly, or that rely on/workspace/hermes-home detection, keep their current behaviour.Risks / Follow-ups
/workspaceare owned by different non-root UIDs now follow the state dir instead of/workspace. That is the intended change, and it is the ownership that startup actually validates, but it is a behaviour change for anyone who was relying on the old precedence.$itdirgains two small marker files (hermeswebui_user_{uid,gid}_source). They are covered by the existingchown -R "$itdir"and written throughwrite_privtmpfile, so they inherit the same 600 mode and ownership as the value files. A container carrying an old$itdirwith no marker files reads them as absent and behaves exactly as before.0, so a probe owned1001:0can hand the UID to one directory and the GID to a later one, yielding a pair that matches neither mount. That behaviour predates this PR — the two loops have always been separate with the same!= "0"guard — and this change extends both lists identically, so it adds a candidate rather than the divergence itself. Coupling the two loops would change detection semantics for the bug: workspace file browser shows empty when volume is mounted in Docker #569/Docker image: Permission denied writing to shared /opt/data volume when hermes-agent and hermes-webui run in separate containers #668 paths as well, which is past the scope agreed on the issue; happy to open a sibling issue for it, or to fold it in here if you would rather have it in one go.actions/checkout@v4,docker/setup-buildx-action@v3,docker/build-push-action@v6). Also flagged by Greptile. I kept them consistent with the rest of the file rather than SHA-pinning only my steps; pinning the workflow is a worthwhile change but belongs in its own PR.Release notes
Docker: fix UID/GID auto-detection selecting the image's
1024owner (#7027) — single-container deploys with a bind-mountedHERMES_WEBUI_STATE_DIRowned by a non-1024host user no longer restart-loop withFailed to verify state directory. The configured state directory is now probed before/workspace, whose owner in a stock image is a build-time artifact rather than a host signal. An explicitly suppliedWANTED_UID/WANTED_GIDof1024is also no longer overwritten by auto-detection.Model Used
AI-assisted.
claude-opus-5[1m], via Claude Code