feat(openemr-cmd): HOST_UID in non-worktree mode; remove obsolete auto-chown shim#842
Merged
bradymiller merged 3 commits intoJun 26, 2026
Merged
Conversation
…o-chown shim Three related changes that complete the HOST_UID story: 1. Non-worktree `up` now exports HOST_UID/HOST_GID before invoking docker compose, so the openemr service's compose-side env block (declared in openemr/openemr#12647) gets the runner's uid for apache to adopt. Worktree mode already did this via wt_write_ override's environment block (openemr#840); this closes the gap for the non-worktree path (`openemr-cmd up` from openemr/docker/development-*). Respects user pre-exports — only auto-derives from `id -u` when unset. 2. Non-worktree `up` also pre-creates host-side dirs for named-volume mount points under the webroot bind mount, mirroring the worktree path's wt_precreate_volume_mountpoints. Without this, docker daemon creates the missing dirs on the host as root:root 0755, accumulating cruft in the base repo over time. Refactored the existing pre-create logic into precreate_volume_mountpoints_at(compose_file, bind_root) so both paths share the implementation. Idempotent — accepts existing host-owned dirs AND existing root-owned legacy cruft (mkdir-then-accept; we don't sudo-chown legacy dirs because that would require host root and surprise the user). 3. Removed the openemr#833 in-container auto-chown shim from cmd_worktree_ remove. With HOST_UID adoption now shipping universally (worktree AND non-worktree), apache writes bind-mount files as the host uid throughout — the shim's chown(host-uid → host-uid) was a permanent no-op wasting docker exec time on every remove. The probe + the pre-create + the probe-loosening (openemr#838) are the safety net for any drift cases. E2e updates: - worktree-lifecycle-e2e: removed the "auto-chown banner present" assertion (banner is gone); added an explicit bind-mount-ownership assertion (verify sites/default/sqlconf.php is owned by runner uid, proving apache adopted host uid via HOST_UID) - nonworktree-lifecycle-e2e: added the same bind-mount-ownership assertion against openemr/sites/default/sqlconf.php — proves the non-worktree path's HOST_UID export reaches the container - multi-3 + prek: updated step names + comments to reflect the new reality (no shim, no chown dance) Bats: - New host_uid_nonworktree.bats: 4 cases pinning the non-worktree HOST_UID export + pre-create behavior (export with default uid; respects pre-exports; pre-creates expected dirs; idempotent on existing dirs) VERSION bumped 1.0.49 → 1.0.50. Upgrade path for existing base-repo users (with legacy root-owned mount-point dirs): one-shot `sudo chown -R "$(id -u):$(id -g)" <openemr-checkout>` if they care about the cruft. Otherwise harmless — non-worktree mode has no rm-based operation that would trip the writability probe. Assisted-by: Claude Code
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesHost UID bind-mount flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/test-bats-openemr-cmd-real-docker.yml (1)
444-445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert GID adoption too.
These checks pin
HOST_UID, but the PR also exportsHOST_GID; adding a%g/id -gassertion catches a broken group-adoption path in the real Docker flow.Proposed check to add in both ownership probes
owner_uid=$(stat -c '%u' "${probe_file}") + owner_gid=$(stat -c '%g' "${probe_file}") runner_uid=$(id -u) + runner_gid=$(id -g) if [[ "${owner_uid}" != "${runner_uid}" ]]; then echo "FAIL: bind-mount file owned by uid=${owner_uid} (expected runner uid=${runner_uid})" echo "HOST_UID adoption is NOT working — apache may have stayed at uid=1000" ls -la "${probe_file}" exit 1 fi + if [[ "${owner_gid}" != "${runner_gid}" ]]; then + echo "FAIL: bind-mount file owned by gid=${owner_gid} (expected runner gid=${runner_gid})" + echo "HOST_GID adoption is NOT working" + ls -la "${probe_file}" + exit 1 + fiAlso applies to: 909-910
🤖 Prompt for 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. In @.github/workflows/test-bats-openemr-cmd-real-docker.yml around lines 444 - 445, The ownership probes currently verify only HOST_UID adoption, so extend the checks in the Docker workflow’s probe logic to also validate HOST_GID. Update the ownership assertions alongside the existing stat/id comparisons in the probe sections so they capture the file’s group ID with stat %g and compare it against id -g from the runner. Apply the same GID assertion in both ownership probe locations referenced by the workflow to ensure the real Docker path confirms group adoption too.
🤖 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 `@utilities/openemr-cmd/openemr-cmd`:
- Around line 954-956: The stale auto-chown recovery hint still references a
removed in-container permissions fix, so update the failure message to direct
users to perform a manual host-side chown instead of starting the stack for an
auto-chown step. Make this change in the openemr-cmd permission recovery/error
path that emits the hint, and also review any related BATS expectations or
assertions that still look for the old auto-chown banner or docker exec chown
behavior.
- Around line 2206-2213: The non-worktree bind-root resolution in the `up` flow
ignores `OPENEMR_DIR` and always precreates mountpoints under `${PWD}/../..`,
which can target the wrong checkout. Update the `bind_root_resolved` /
`precreate_volume_mountpoints_at` path handling to resolve the bind root from
the same `${OPENEMR_DIR:-../..}` value used by the compose file, while
preserving the existing cwd-based worktree check. Ensure the logic in
`openemr-cmd` uses the resolved environment-aware bind root before calling
`precreate_volume_mountpoints_at`.
---
Nitpick comments:
In @.github/workflows/test-bats-openemr-cmd-real-docker.yml:
- Around line 444-445: The ownership probes currently verify only HOST_UID
adoption, so extend the checks in the Docker workflow’s probe logic to also
validate HOST_GID. Update the ownership assertions alongside the existing
stat/id comparisons in the probe sections so they capture the file’s group ID
with stat %g and compare it against id -g from the runner. Apply the same GID
assertion in both ownership probe locations referenced by the workflow to ensure
the real Docker path confirms group adoption too.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e971af20-41f7-43cf-b9a9-3cccb5a05878
📒 Files selected for processing (3)
.github/workflows/test-bats-openemr-cmd-real-docker.ymltests/bats/openemr-cmd/host_uid_nonworktree.batsutilities/openemr-cmd/openemr-cmd
The auto-chown shim removal in the previous commit broke three bats tests in remove_graceful.bats that asserted the shim's specific behavior (banner present, docker exec invoked, failure non-fatal). Replace with one regression-guard test pinning the "no shim" contract: "remove never invokes docker exec for chown (shim removed)" — fires loudly if anyone re-introduces it. Also: - Update the "aborted at prompt" test to drop the auto-chown banner check (banner is gone regardless of abort path) - Update the probe error message in cmd_worktree_remove to remove the obsolete "start the stack first, auto-chown handles it" option; the only recovery is sudo chown now - Update the corresponding probe-test assertion to expect the simplified single-option error - Tidy stale "auto-chown" mentions in workflow job names + top comment Assisted-by: Claude Code
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 `@tests/bats/openemr-cmd/remove_graceful.bats`:
- Around line 232-238: The regression check in the remove_graceful bats tests is
too narrow because it only searches for the exact "exec -u root" fragment.
Update the check in the test logic around the docker log assertion to fail on
any docker exec invocation by matching the exec subcommand itself, so future
variants with different flags or ordering are still caught; apply the same
change in both affected test blocks, including the one referenced by the
remove_graceful test helpers.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ac353f0-e666-4831-a79d-9d1ba2392b0c
📒 Files selected for processing (3)
.github/workflows/test-bats-openemr-cmd-real-docker.ymltests/bats/openemr-cmd/remove_graceful.batsutilities/openemr-cmd/openemr-cmd
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/test-bats-openemr-cmd-real-docker.yml
- utilities/openemr-cmd/openemr-cmd
…aden bats exec-check CodeRabbit found two issues on openemr#842: (1) Non-worktree pre-create hardcoded `${PWD}/../..` as the bind mount root, ignoring the `${OPENEMR_DIR:-../..}` substitution that the compose file actually honors. If a user pointed their openemr checkout elsewhere via OPENEMR_DIR, pre-create wrote dirs to a phantom location at cwd/../.. while docker created the real mount- point dirs (under OPENEMR_DIR) as root — same bug we were trying to fix, just one level up. Fix: resolve the bind root from OPENEMR_DIR with the same default the compose uses. (2) Two bats regression-guard checks used `grep -F "exec -u root"` to detect a re-introduced shim. Too narrow — a future variant like `docker exec --tty <id> chown ...` (without `-u root`) would slip through. Broadened to match the `exec` subcommand at line start, catching any flag/user/order shape. Assisted-by: Claude Code
bradymiller
added a commit
to bradymiller/openemr
that referenced
this pull request
Jun 26, 2026
Brief mention of the openemr-cmd HOST_UID/HOST_GID auto-export in two places where contributors and agents will look: - CONTRIBUTING.md: added a sub-bullet under the openemr-cmd intro explaining that bind-mounted files apache writes are host-owned for any host uid (transparent for new checkouts; one-shot sudo chown documented for legacy checkouts with root-owned cruft). - CLAUDE.md: added a Common Gotchas entry so future agents debugging EACCES errors on bind-mount edits go straight to "did you bypass openemr-cmd?" instead of tracing it themselves. Also notes the insane env hasn't been wired up yet (older image tags don't honor HOST_UID). The auto-export landed in openemr/openemr-devops#840 (worktree side) and openemr/openemr-devops#842 (non-worktree side); the entrypoint adoption it depends on landed in openemr#12642 (image side) and openemr#12647 (compose env declaration). Assisted-by: Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
openemr-cmd upnow forwardsHOST_UID/HOST_GIDinto Docker (when not set) to preserve host ownership for non-worktree runs.HOST_UID/HOST_GIDbehavior and the revised removal flows.Cross-repo dependency
Depends on openemr/openemr#12647 (now landed). The flex image already pins HOST_UID entrypoint support (from openemr/openemr#12642 via the #12644 dependabot bump); #12647 added the compose-side env declaration so the exported HOST_UID actually reaches the openemr service. No additional image release needed — all moving pieces in place once both PRs land.
What's tested end-to-end in CI
worktree-lifecycle-e2eassertssites/default/sqlconf.phpis owned by the runner uid after install — proves apache adopted HOST_UID via wt_write_override's env block + entrypoint adoption.nonworktree-lifecycle-e2easserts the same againstopenemr/sites/default/sqlconf.php— proves run_docker_compose's export reaches the openemr service via #12647's env block + entrypoint adoption.