fix(docker): ensure ironclaw runtime home exists - #1918
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the Dockerfile to establish a home directory for the ironclaw user and sets the working directory to that location. It also introduces a new test suite to verify these Dockerfile configurations. Review feedback identifies a potential runtime issue where the application might fail to find migrations due to the changed working directory and suggests improving the robustness of the new tests by using token-based matching instead of fragile string containment.
| RUN useradd -m -u 1000 -s /bin/bash ironclaw \ | ||
| && mkdir -p /home/ironclaw/.ironclaw \ | ||
| && chown -R ironclaw:ironclaw /home/ironclaw | ||
| WORKDIR /home/ironclaw |
There was a problem hiding this comment.
The WORKDIR is set to /home/ironclaw, but the application migrations are copied to /app/migrations (line 72). If the ironclaw binary expects to find the migrations directory in its current working directory (e.g., via a relative path like ./migrations), it will fail to locate them. Consider either setting the WORKDIR to /app or ensuring the application is configured to use the absolute path /app/migrations to avoid runtime errors during database initialization.
| assert!( | ||
| dockerfile.contains("useradd -m -u 1000 -s /bin/bash ironclaw"), | ||
| "runtime image must create the ironclaw user with a home directory", | ||
| ); | ||
| assert!( | ||
| dockerfile.contains("ENV HOME=/home/ironclaw"), | ||
| "runtime image must set HOME to /home/ironclaw for ~/.ironclaw state", | ||
| ); | ||
| assert!( | ||
| dockerfile.contains("WORKDIR /home/ironclaw"), | ||
| "runtime image must start in the ironclaw home directory", | ||
| ); | ||
| assert!( | ||
| dockerfile.contains("mkdir -p /home/ironclaw/.ironclaw"), | ||
| "runtime image must pre-create ~/.ironclaw before dropping privileges", | ||
| ); |
There was a problem hiding this comment.
These assertions are highly sensitive to the exact string content of the Dockerfile. To improve robustness and maintainability, use token-based or word-boundary checks instead of simple substring containment to avoid false positives. Additionally, separate checks for distinct conditions (like username and flags) to improve code clarity and robustness, as per repository guidelines.
References
- When detecting commands or keywords in a string, use token-based or word-boundary checks instead of simple substring containment to avoid false positives.
- Separate checks for distinct conditions to improve code clarity and robustness.
…-home-directory-6ua # Conflicts: # Dockerfile
zmanian
left a comment
There was a problem hiding this comment.
LGTM. Clean, minimal fix for a real runtime issue.
Dockerfile changes: Correct. Setting HOME explicitly, pre-creating ~/.ironclaw, and chown-ing before USER ironclaw is the right approach. The useradd -m -d flags are consistent. Idempotent -- safe to rebuild.
Gemini's migration concern is a false positive: migrations use embed_migrations!("migrations") (refinery macro) which embeds SQL at compile time into the binary. The WORKDIR change has no effect on migration discovery at runtime. /app/migrations is only needed during the build stage.
Test (tests/dockerfile_runtime_home.rs): Reasonable regression guard for a Dockerfile invariant. The string-matching approach is brittle by nature, but for this purpose (catching accidental removal of the home dir setup) it is adequate. Gemini's suggestion to use "token-based matching" is over-engineering for a Dockerfile text check.
Snapshot diff: Whitespace-only change (trailing space removed). No functional impact.
One minor observation (non-blocking): the PR drops -s /bin/bash from the useradd call that exists on staging. The default shell will be /bin/sh or whatever is in /etc/default/useradd. This is fine for a container runtime (the ironclaw process doesn't need an interactive shell), but worth noting in case anyone expects bash inside the container for debugging.
(cherry picked from commit 13852ff)
Summary
HOME=/home/ironclawin the production runtime image/home/ironclaw/.ironclawand switchWORKDIRto the ironclaw home before dropping privilegesTesting
rustfmt --check tests/dockerfile_runtime_home.rsrustc --test tests/dockerfile_runtime_home.rs -o /tmp/dockerfile_runtime_home_test && /tmp/dockerfile_runtime_home_testNotes
cargo test --test dockerfile_runtime_homewas attempted, but this machine ran out of disk during workspace compilation before the test binary was built.Fixes #1899