fix(openemr-cmd): pre-create volume mount-point dirs + loosen remove probe#838
Merged
bradymiller merged 4 commits intoJun 26, 2026
Conversation
…probe Two complementary fixes for the same root cause — defense in depth against the writability-probe failure that surfaced in openemr#836's multi-3 e2e job. (1) wt_precreate_volume_mountpoints parses each env's docker- compose.yml for volume mounts targeting paths under /var/www/localhost/htdocs/openemr/ and pre-creates the host-side dirs as the current user before docker first attaches the volumes. Without this, docker creates missing mount-point dirs on the host as root:root 0755 — those empty root-owned dirs accumulate in worktrees and trip the writability probe on the next worktree remove. Called from cmd_worktree_add and cmd_worktree_regen so both fresh worktrees and env-switched/regenerated ones are covered. Idempotent (mkdir -p safe on existing dirs). (2) cmd_worktree_remove's writability probe now accepts the case where a non-writable dir is EMPTY and its parent is writable — because rm only needs write+execute on the PARENT to rmdir an empty child, not write on the child itself. Catches any drift where pre-create misses a mount point (e.g., openemr/openemr adds a new volume and the regex doesn't update). Non-empty non-writable dirs still fail the probe since rm there would genuinely fail mid-walk. Auto-chown via docker exec (openemr#833) handles the orthogonal case of container-written files INSIDE the bind mount. Together with the two changes here, all three host-side ownership scenarios are covered: - container-written files in bind mount → openemr#833 auto-chown - empty docker-daemon-created mount-point dirs → pre-create + probe - non-empty root-owned dirs → probe still fails (correct) The remaining apache-uid mismatch (apache inside container chowns the bind mount to uid=1000) is a separate concern, tracked as the HOST_UID-passthrough work in openemr/openemr + openemr-devops. CI workaround removed: the "Pre-remove host-side chown" step in the worktree-multi-concurrent-e2e job (added in openemr#836 commit cf99562) is no longer needed — pre-create handles the volume mount-point dirs upfront, and auto-chown handles the rest. Tests: tests/bats/openemr-cmd/mountpoint_precreate.bats pins both behaviors (5 precreate cases + 2 probe-loosening cases). OC_SCRIPT_FUNCS_END bumped 1786 → 1861 to match the new function landing position. 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 (1)
📝 WalkthroughWalkthroughThe PR precreates webroot volume mount directories during ChangesOpenEMR worktree lifecycle changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
…w|ro) CI bats run revealed sed: -e expression openemr#1, char 96: unknown option to s. The s|...|...|p form parsed the | inside (rw|ro) as the s-delimiter, making the substitute invalid. The function silently failed (no paths extracted → no dirs pre-created) because the surrounding `|| true` masked the sed error. Switch to # as the delimiter so | retains its regex-alternation meaning. Verified locally with a quick sed test: the bind mount line (no subpath after openemr) is correctly excluded; volume mount lines with a subpath are correctly extracted. Assisted-by: Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@utilities/openemr-cmd/openemr-cmd`:
- Around line 651-677: The wt_precreate_volume_mountpoints helper currently
blocks textual .. but still uses mkdir -p on "${dir}/${rel_path}", which can
traverse existing symlink components inside the worktree. Update this pre-create
logic to reject or safely skip any mount path whose intermediate components
resolve through symlinks before directory creation, using the existing
wt_precreate_volume_mountpoints flow and the rel_path/host_path handling to keep
the worktree boundary intact. Ensure the fix preserves the best-effort behavior
while preventing symlinked paths from escaping the worktree.
🪄 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: a9a1edd4-1646-43da-b8a3-95fecaa6c33b
📒 Files selected for processing (4)
.github/workflows/test-bats-openemr-cmd-real-docker.ymltests/bats/openemr-cmd/helpers.bashtests/bats/openemr-cmd/mountpoint_precreate.batsutilities/openemr-cmd/openemr-cmd
… (symlink defense)
CodeRabbit caught a symlink-traversal vector in
wt_precreate_volume_mountpoints: plain `mkdir -p ${dir}/${rel_path}`
follows existing symlink components in the worktree. A malicious
branch that committed e.g. `public -> /etc` would cause us to create
`/etc/assets` on the host when bringing the stack up.
The existing wt_write_override symlink guards only cover docker/ and
the env's compose subdir — they don't extend to the volume mount
targets we now pre-create. Extend the same defense by walking
rel_path component-by-component: refuse to descend through any
symlink or non-directory file, and on any mismatch abandon that
mount target (continue to the next one) without aborting the rest
of pre-create.
Tradeoff vs `mkdir -p`: marginally more bash code, single test added
to pin the symlink-skip behavior. The performance cost is negligible
— rel_paths are 1-4 components and we only iterate the volume mount
list (~9 entries per env).
Assisted-by: Claude Code
…efactor The component-walk hardening in c3eef03 added ~21 lines to wt_precreate_volume_mountpoints, shifting the end-of-functions anchor from 1861 to 1882. The hermetic bats suite sources only the first OC_SCRIPT_FUNCS_END lines for pure-function tests; when the constant is stale, the sourced view ends mid-function and every wt_slug/wt_compose_subdir/state/lock test fails because the helpers it depends on are never defined. The "OC_SCRIPT_FUNCS_END points at end of function defs" drift test in helpers_pure.bats fires when the anchor doesn't land at end-of-functions; bumping in lockstep with the function add was missed in c3eef03. Assisted-by: Claude Code
2 tasks
bradymiller
added a commit
that referenced
this pull request
Jun 26, 2026
…reproducer (#839) ## Summary Hermetic bats coverage for the permission scenarios that have been the most active bug surface in the recent worktree-cmd PRs (#833, #836, #838). Pure test scaffolding — no production code changes. ## What's added Four cases in `tests/bats/openemr-cmd/mountpoint_precreate.bats`: 1. **Empty non-writable + non-writable parent → probe fails.** The probe-loosening rule from #838 ("empty + parent writable = rmdir-able") requires BOTH conditions; guards against accidentally accepting a case where the parent's perms would block the rmdir too. 2. **Unreadable dir (mode 0000) → probe fails conservatively.** When we can't `[[ -r && -x ]]` the dir, we can't verify emptiness via `find`; the probe must treat it as blocking (rm could silently leave content behind otherwise). 3. **Multiple unwritable dirs (one empty-removable, one non-empty) → probe fails on the non-empty one.** Early-termination correctness: the probe must evaluate each unwritable dir against the empty-removable rule rather than failing on the first unwritable it sees regardless of removability. 4. **Reproducer for the multi-3 CI failure in #836:** stage `ccdaservice/packages/oe-cqm-service/node_modules` as empty 0555 — the exact path + state that broke pre-loosening. Confirms BOTH that pre-create populates the path AND that the loosened probe accepts the simulated post-volume-purge state. Pins the bug as a regression guard against future drift. ## Why now The permission-bug class has surfaced repeatedly across recent PRs (apache uid mismatch, docker daemon root-owned mount-points, probe over-strictness). Each fix added defensive logic; this PR turns those scenarios into bats that fail loudly if any future refactor breaks the invariants. The reproducer in particular locks down the specific bug from #836 so it can't silently regress. ## What this PR can't pin The bats simulates the FILESYSTEM STATE (root-owned-empty via chmod 0555/0000) but not the CAUSE (docker daemon creating dirs as root, apache chowning bind mount). The real-docker e2e job in `test-bats-openemr-cmd-real-docker.yml` covers the cause-side end-to-end. The two are complementary. ## Test plan - [ ] BATS openemr-cmd (ubuntu-22.04) and (macos-14) pass — the new 4 tests + existing 12 in mountpoint_precreate.bats - [ ] No other workflow effects (no production code changes; the existing real-docker e2e jobs run unchanged) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of `worktree remove` when mount points or directories are not writable. * Added coverage ensuring the “empty directory” exemption is not used when the parent is also non-writable. * Added conservative behavior checks for effectively unreadable/unenterable directories, preventing unsafe verification. * Strengthened multi-candidate evaluation when multiple unwritable directories exist, ensuring the correct failure is surfaced. * Reproduced and protected a regression so removal succeeds for an empty, non-writable mount point. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 tasks
bradymiller
added a commit
to bradymiller/openemr-devops
that referenced
this pull request
Jun 26, 2026
…urce-only guard; bump VERSION Two cleanups bundled: (1) The hermetic bats suite used to source openemr-cmd's function definitions via `eval "$(head -n OC_SCRIPT_FUNCS_END script)"` with a magic line-number constant that had to be bumped every time the script grew/shrank near end-of-functions. A drift sentinel test caught stale constants but the bump itself was easy to forget — which broke CI on openemr#838 and again now on this branch, distracting from the actual review surface. Replace with a __OPENEMR_CMD_SOURCE_FUNCS_ONLY=1 env-var guard: the script returns early from sourcing right after the last function def when the flag is set; direct invocation (./openemr-cmd) is unaffected since the flag isn't in the runtime environment. Tests that want only function defs set the flag and source — no magic number, no drift, no maintenance. - openemr-cmd: source guard added before USAGE_EXIT_CODE=13 - helpers.bash: oc_source_funcs uses the env var; OC_SCRIPT_FUNCS_END constant removed - helpers_pure.bats: oc_run_in_funcs drops the funcs_end arg; the drift sentinel test replaced with two guard-behavior tests (sourcing with the flag defines funcs but skips dispatch; direct execution is unaffected by the flag) - copy_base_env.bats: oc_run_in_funcs drops the funcs_end arg - state.bats, state_lock.bats, state_lock_crash_recovery.bats, worktree_add_base.bats: inline eval-of-head replaced with source-with-flag pattern (2) Bump VERSION 1.0.48 → 1.0.49 for the HOST_UID passthrough + this refactor. Was missed in the initial openemr#840 push. Assisted-by: Claude Code
bradymiller
added a commit
that referenced
this pull request
Jun 26, 2026
…container (#840) ## Summary Completes the cross-repo HOST_UID story started in openemr/openemr#12642 (entrypoint side, already merged + new flex image SHA pinned via dependabot #12644). This PR wires the openemr-devops side: `wt_write_override` emits `HOST_UID`/`HOST_GID` env vars in the openemr service block, sourced from `id -u` / `id -g` at worktree-add / regen time. ## Result For **non-uid=1000 hosts** (CI runners at uid=1001, multi-user dev boxes, distros that start uids at 1001+, macOS via Docker Desktop): apache inside the container now writes bind-mount files with the host's uid. Host-side writes (`git commit`, IDE edits, `worktree regen`, `worktree set-env`) work without any chown dance. The uid-mismatch class of bugs that dominated #833 / #836 / #838 review cycles is eliminated end-to-end. For **uid=1000 hosts** (most Linux desktop developers): zero functional change. The env vars are emitted unconditionally because the entrypoint adoption block is a sub-millisecond no-op on uid=1000 and the consistent behavior across hosts is worth more than the savings. **Backwards compatible:** pre-HOST_UID-aware images silently ignore the env vars. ## CI workarounds removed (now obsolete) Both `sudo chown -R "$(id -u):$(id -g)"` steps in `test-bats-openemr-cmd-real-docker.yml`: 1. **`worktree-lifecycle-e2e`** "One-shot chown back to runner (enables host-side regen + set-env)" — added in #834 2. **`prek-workflow-e2e`** "One-shot chown back to runner (enables host-side writes + git commits)" — added in #836 Both replaced with `NOTE:` comments documenting what used to live there and why the chown is no longer needed — for archaeology by future contributors. ## Kept as back-compat shim The `#833` auto-chown block in `cmd_worktree_remove` stays in place. Inline comment updated to reflect its new status (no-op when the image honors HOST_UID; still useful for users on pre-HOST_UID images and for any drift the entrypoint adoption might miss). ## Tests New bats file `tests/bats/openemr-cmd/host_uid_compose.bats` pins three behaviors: 1. `wt_write_override` emits `HOST_UID` and `HOST_GID` env vars in the override file 2. Values match `id -u` / `id -g` at write time 3. Env vars are correctly nested under `services.openemr.environment` (structural check via yaml.safe_load) 4. Emitted for all three env variants (easy / easy-light / easy-redis) ## Test plan - [ ] BATS openemr-cmd (ubuntu-22.04 + macos-14) pass — new tests + existing suite - [ ] e2e — full worktree lifecycle: passes WITHOUT the sudo chown step (proves apache now adopts runner uid; regen + set-env writes work as runner) - [ ] e2e — prek install + real git commit: passes WITHOUT the sudo chown step (proves `git commit` from the worktree works as runner) - [ ] Other e2e jobs unchanged - [ ] FUNCS_END drift sentinel still passes (no new function added — wt_write_override grew inline, last function still at line 1882) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Generated OpenEMR docker-compose override files now include the current host user and group IDs (`HOST_UID`/`HOST_GID`) in the container environment. * **Bug Fixes** * Worktree lifecycle and pre-install commit workflows no longer require host-side ownership adjustment before regeneration and environment setup. * Permission recovery/chown behavior is more consistent, with backward-compatible fallback for older images or ownership drift. * **Tests** * Added BATS coverage validating `HOST_UID`/`HOST_GID` emission and stability of golden comparisons by masking host-specific values. * Improved BATS harness sourcing for hermetic function-only execution. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
Two complementary fixes for the bug surfaced in #836's multi-3 e2e job ("ccdaservice/packages/oe-cqm-service/node_modules is not writable by you"). Defense in depth against the same root cause.
(1) Pre-create volume mount-point dirs
New helper
wt_precreate_volume_mountpointsparses each env'sdocker-compose.ymlfor volume mounts targeting paths under/var/www/localhost/htdocs/openemr/and pre-creates the host-side dirs as the current user before docker first attaches the volumes.Without this: docker creates missing mount-point dirs on the host as
root:root 0755. Aftercompose down --volumes, those empty root-owned dirs reappear on the host and trip the writability probe on the nextworktree remove.With this: docker sees the dirs already exist (owned by host user) and just attaches the volume — host ownership preserved across the volume's lifetime.
Called from both
cmd_worktree_addandcmd_worktree_regenso both fresh and env-switched worktrees are covered. Idempotent (mkdir -p is safe on existing dirs, including the ones already checked into the repo likepublic/assets,sites).(2) Loosen the writability probe
cmd_worktree_remove's probe now accepts the case where a non-writable dir is empty AND its parent is writable — becausermonly needs write+execute on the PARENT to rmdir an empty child, not write on the child itself.Catches any drift where pre-create misses a mount point (e.g., openemr/openemr adds a new volume and the regex doesn't update). Non-empty non-writable dirs still fail the probe since rm there would genuinely fail mid-walk.
Coverage matrix
#833auto-chown viadocker execThe remaining apache-uid mismatch (apache inside container chowns the bind mount to uid=1000 on hosts where uid != 1000) is orthogonal and tracked separately as the HOST_UID-passthrough work.
CI workaround removed
The "Pre-remove host-side chown" step in
worktree-multi-concurrent-e2e(added in #836 commitcf99562) is no longer needed. This PR removes that step from the workflow file as part of the fix — proving the change works end-to-end in CI.Test plan
tests/bats/openemr-cmd/mountpoint_precreate.batsexercises 5 precreate cases (creates expected dirs, skips out-of-webroot mounts, skips the bind mount itself, runs from regen path, idempotent) + 2 probe-loosening cases (empty-removable passes, non-empty fails)🤖 Generated with Claude Code
Summary by CodeRabbit
worktree regen.