fix(openemr-cmd): auto-chown bind mount via container before worktree remove#833
Merged
bradymiller merged 5 commits intoJun 25, 2026
Merged
Conversation
… remove The A5 permission probe from openemr#826 catches container-uid files cleanly, but it still requires the user to run a manual `sudo chown -R` before the remove can proceed. The container itself has root, so it can chown the bind-mounted worktree back to the host uid/gid for us before we tear it down — eliminating the manual step in the common case. Sequence in cmd_worktree_remove (between the dir-exists check and the A5 probe): 1. Find the worktree's openemr container by compose project label (same pattern as cmd_worktree_exec). 2. If a container is running, run `docker exec -u root <id> chown -R <host-uid>:<host-gid> /var/www/localhost/htdocs/openemr`. Best-effort: stderr suppressed, `|| true` swallows non-zero exits — the A5 probe is the safety net for anything chown couldn't fix (container not running, root-owned files outside the openemr workdir, chown itself failed). 3. Probe runs as before; usually passes now. 4. Destructive ops proceed. Tests (tests/bats/openemr-cmd/remove_graceful.bats): - container-running case: docker.log shows `exec -u root <id> chown -R <uid>:<gid> /var/www/.../openemr` with the host's actual uid/gid + the "Auto-chowning bind mount via container" log line. - no-container case: chown step skipped silently (no exec invocation, no log line), remove still completes. - chown-failure case: a stub variant returns 17 on `docker exec`; the script logs the attempt + ignores the failure + lets the probe + destructive ops continue. Remove still succeeds. OC_SCRIPT_FUNCS_END bumped 1741 → 1773 to track the new function-defs end. VERSION 1.0.47 → 1.0.48. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 ChangesWorktree remove auto-chown
Workflow concurrency behavior
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🧹 Nitpick comments (1)
tests/bats/openemr-cmd/remove_graceful.bats (1)
213-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the worktree directory is removed in these new paths.
The new cases prove the chown behavior, but they do not consistently verify the command’s core outcome: the worktree directory is gone. Capture the registered
dirbefore removal and assert[[ ! -e "${wt_dir}" ]]afterassert_success.Example assertion pattern
setup_full_worktree feature-rm-no-container -b + local wt_dir + wt_dir=$(jq -r '.["feature-rm-no-container"].dir' "${STATE_FILE}") : > "${STUB_DIR}/docker.log" @@ run jq -r 'has("feature-rm-no-container")' "${STATE_FILE}" assert_output "false" + [[ ! -e "${wt_dir}" ]] || fail "worktree directory still exists: ${wt_dir}"Also applies to: 239-258, 260-306
🤖 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 `@tests/bats/openemr-cmd/remove_graceful.bats` around lines 213 - 237, The new remove_graceful Bats cases around worktree removal verify auto-chown behavior but do not assert the main outcome that the worktree directory is actually deleted. In each affected test block, capture the registered worktree directory path from the setup/registration step (the dir returned or stored by the remove flow) and, after assert_success, add a filesystem assertion that the directory no longer exists, using the same wt_dir variable pattern consistently across the remove cases.
🤖 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 838-873: The auto-chown and permission-check logic in openemr-cmd
should run only after the user confirms they want to proceed, not before the
confirmation prompt. Reorder the flow around the prompt so the confirmation
happens first, then the existing auto-chown block using autochown_id and the
permission probe execute immediately before wt_compose_cmd down, preserving the
current safeguards without changing ownership or container state if the user
aborts.
---
Nitpick comments:
In `@tests/bats/openemr-cmd/remove_graceful.bats`:
- Around line 213-237: The new remove_graceful Bats cases around worktree
removal verify auto-chown behavior but do not assert the main outcome that the
worktree directory is actually deleted. In each affected test block, capture the
registered worktree directory path from the setup/registration step (the dir
returned or stored by the remove flow) and, after assert_success, add a
filesystem assertion that the directory no longer exists, using the same wt_dir
variable pattern consistently across the remove cases.
🪄 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: 3f9a0709-3d7a-4799-8875-66ec077bed9a
📒 Files selected for processing (3)
tests/bats/openemr-cmd/helpers.bashtests/bats/openemr-cmd/remove_graceful.batsutilities/openemr-cmd/openemr-cmd
…o-chown tests CodeRabbit nit on openemr#833: the new auto-chown test cases verified the chown behavior + state-entry cleanup, but didn't assert the core remove outcome — that the worktree directory on disk is actually gone. Add `[[ ! -e "${wt_dir}" ]]` after each `assert_success` in the three new cases (container-running, container-not-running, chown-fails). Capture `wt_dir` from the state file before the remove runs, so the assertion compares against the canonical registered path. The actionable comment on the script flow (reorder so auto-chown + probe run after the user confirms) was deliberately not addressed: moving the probe after the prompt is a UX regression — asking "Continue? [y/N]" only to fail goes against the probe's whole point. The chown itself is reversibly idempotent (it restores host ownership, matching user expectations of files under their own worktree dir) so requiring confirmation first adds friction without proportional safety benefit. Co-Authored-By: Claude Code <noreply@anthropic.com>
…e effects on abort) Per user review on openemr#833: on hosts where host-uid != container-apache-uid (uid 1000) — common on macOS Docker Desktop, on Linux enterprise envs where dev users get non-1000 uids, etc. — running the container chown BEFORE the user's confirmation prompt would leave the stack effectively broken if the user then aborted with 'N'. The bind mount's files would be owned by the host user; apache inside the container could no longer write to its own files. "Reversibly idempotent" was wrong: only true when host-uid == 1000. Reorder so the auto-chown + permission probe both run AFTER the y/N confirmation. If the user aborts, no state changes — chown wasn't attempted, no docker exec was issued, no files mutated. Trade-off vs the prior "probe before prompt" position: the probe now fires after the user types 'y', so a remove that's going to fail on unwritable dirs no longer fails until after the confirmation. In practice this is one wasted 'y' keystroke vs. the alternative of silently breaking the stack on abort — clear win for the latter. New test (tests/bats/openemr-cmd/remove_graceful.bats): - "aborted at the prompt leaves NO side effects" — pipes 'n' to the prompt with DOCKER_PS_OUTPUT set; asserts no "Auto-chowning" log, no `exec -u root` in docker.log, dir still on disk, state entry still present. If auto-chown were misplaced before the prompt again, the chown invocation would be recorded and this test would fail — making the contract a permanent regression guard. OC_SCRIPT_FUNCS_END bumped 1773 → 1780 to track the additional comment lines. Co-Authored-By: Claude Code <noreply@anthropic.com>
…OR sudo chown) Per user review on openemr#833: the probe's chown hint only mentioned the manual host-side `sudo chown -R` recovery. With auto-chown now running from inside the container, there's a second (often easier) recovery option: start the stack first, then retry remove — the auto-chown step will fix permissions without needing sudo on the host. Updated message lists both: (1) Start the stack first; the auto-chown step will fix permissions from inside the container, then re-run remove: openemr-cmd worktree up '${branch}' openemr-cmd worktree remove '${branch}' (2) Or restore ownership manually on the host (requires sudo): sudo chown -R \"\$(id -u):\$(id -g)\" '${dir}' openemr-cmd worktree remove '${branch}' Strengthened the existing probe-refuses test to assert BOTH options appear (the `openemr-cmd worktree up` line + the `sudo chown -R` line). Catches future drift where one of the recovery paths gets silently dropped from the message. OC_SCRIPT_FUNCS_END bumped 1780 → 1786 for the additional lines. Co-Authored-By: Claude Code <noreply@anthropic.com>
…ure rollup-green)
Before: the e2e workflow used `cancel-in-progress: false`, so pushing
a new commit during an in-flight run queued the new run behind the
old. The new SHA's workflow doesn't register its jobs as checks until
it actually starts — so during the queue window, the All Checks
Passed rollup (which uses lewagon/wait-on-check-action against
"whatever checks exist on this SHA") saw only ShellCheck + BATS +
actionlint pass, no e2e checks present, declared green prematurely.
After: `cancel-in-progress: ${{ github.event_name == 'pull_request' }}`.
On PR pushes, supersede the old run so the new SHA's workflow starts
immediately and its jobs register as checks. On master pushes, keep
the prior behavior (don't lose sequential master history). Matches
the existing pattern in vendored-contracts-self-test.yml.
The original comment about "docker daemon resource contention" was
the wrong rationale — each workflow run gets its own ephemeral
runner with its own daemon, no cross-run state.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Merged
5 tasks
bradymiller
added a commit
that referenced
this pull request
Jun 25, 2026
…v/auto-chown coverage (#834) ## Summary Stocktake on the e2e jobs found two issues: 1. **All four job titles drifted from what they actually run.** The lifecycle job's title undersold a job that already tests exec, set-env refusal, restart, and the probe contract. The standard-stack title omitted `dn` + `prek`. The functional round-trip title omitted `drid` — the step that mirrors the user's real ops flow. 2. **The worktree-lifecycle job didn't exercise several core user-facing operations** (`regen`, `set-env` env-switch, `list` status rendering) and didn't validate the auto-chown remove path added in #833 — it only validated the manual-chown fallback (which is now the secondary recovery path). ## Title changes | Job | Old | New | |---|---|---| | worktree-lifecycle | `e2e — add --start → healthy → down → remove` | `e2e — full worktree lifecycle (add → restart → set-env → regen → auto-chown remove)` | | worktree-multi-concurrent | (unchanged — already accurate) | — | | nonworktree-lifecycle | `e2e — standard stack — up → healthy → exec → down` | `e2e — standard stack (up → exec → dn → prek → down)` | | functional-roundtrip | `e2e — functional round-trip — irp + bs + gc + pc + rs` | `e2e — functional round-trip (drid → irp → bs → gc → pc → rs)` | Header comment block updated to match. ## Lifecycle job — new coverage After the existing restart-cycle smoke, the job now also runs: - **`worktree list` status `running`** — verifies the list rendering against a live up stack. - **`worktree regen`** — stomp `.env`, regen, `diff` against pre-stomp snapshot. Confirms `wt_write_env` is deterministic and restores byte-for-byte. - **`worktree list` status `stopped`** — verifies status detection after `down --keep-volumes`. - **`worktree set-env easy → easy-light`** (with stack down) — state `env` field updated, `easy-light/.env` + override written. - **`worktree up` + healthy + smoke on easy-light** — proves the env-switch produced a workable stack on the new env. ## Lifecycle job — auto-chown remove (replaces the manual-chown sequence) With the stack still up after the env-switch, the job runs `worktree remove test-e2e` with no host-side chown. The auto-chown step (from #833) should detect the running container, exec into it as root to chown the bind mount, and let remove succeed cleanly. Asserts the `Auto-chowning bind mount via container` banner appears AND the probe's `is not writable by you` hint does **not** — proves auto-chown handled the root-owned files. ### Dropped from this job (rationale below) - **Probe-refuses-when-container-down test** — covered hermetically in `tests/bats/openemr-cmd/remove_graceful.bats`. The bats fixture uses real `/tmp/...` paths, so probe message rendering against real paths is still validated; we just no longer need a 5-min e2e cycle to re-prove it. - **Manual chown + remove workaround** — no longer the recommended path now that auto-chown handles it. The bats probe-refuses test exercises the message rendering that would direct a user to the manual path when needed. ## Cost Lifecycle job timeout 35 → 45 min for the third healthy-poll window (post-env-switch). In practice the second + third healthy waits are fast (~3-5 min) since the base layer is cached, but each is bounded at 15 min. ## Test plan - [x] YAML loads cleanly - [ ] CI: all existing checks (BATS / ShellCheck / actionlint / CodeRabbit) green - [ ] CI: lifecycle job's new steps all pass - [ ] CI: auto-chown banner appears in the remove step output - [ ] CI: All Checks Passed rollup waits for e2e correctly (the #833 concurrency fix is live on master now) Assisted-by: Claude Code <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded end-to-end coverage for the worktree lifecycle with explicit `worktree list` state checks and a fuller up/down sequence. * Strengthened environment regeneration validation by verifying `.env` stomp/restore behavior and byte-level consistency, including compose-file presence after switching `easy` → `easy-light`. * Updated removal validation to run `worktree remove` while the stack is still running, confirming auto-ownership behavior and expected messaging. * Increased the worktree lifecycle end-to-end timeout to support longer flows, and refreshed test job naming and workflow header descriptions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Code <noreply@anthropic.com>
Merged
3 tasks
bradymiller
added a commit
to bradymiller/openemr-devops
that referenced
this pull request
Jun 25, 2026
The openemr container's apache install chowns the bind-mounted worktree to uid=1000 during startup; the runner (different uid) then can't write the probe file or `git add` to update the worktree's index. Add the same one-shot host-side chown the lifecycle job uses (after healthy is confirmed, before any host-side writes). The `worktree remove` auto-chown from openemr#833 handles the symmetric case at teardown. Assisted-by: Claude Code
bradymiller
added a commit
to bradymiller/openemr-devops
that referenced
this pull request
Jun 25, 2026
…e-shadow dirs Two failures from the prior CI run: TEST 2 of prek e2e (pre-commit reject via codespell typo) passed the commit when it should have rejected. Root cause: codespell skips dotfiles by default. The probe file `.openemr-prek-probe.txt` (with leading dot) was silently excluded; codespell ran but had nothing to check, so "teh" was never flagged. Renamed to `openemr-prek-probe.txt` and documented the gotcha inline. Multi-3's teardown failed at the first remove with "ccdaservice/ packages/oe-cqm-service/node_modules is not writable by you". Root cause: openemr#833's auto-chown via `docker exec` chowns files INSIDE the bind mount, but cannot reach the host-side dirs that docker creates as root when first mounting named volumes OVER bind-mount paths. The lifecycle job sidesteps this with a sudo chown during the env- switch flow; multi-3 has no natural hook for it. Added a pre-remove sudo chown step for each of the three worktree dirs. Auto-chown still runs (handles any container-written files in the small window between the sudo chown and the remove) — the two paths are complementary. The longer-term fix for both contributor and CI ergonomics is the HOST_UID-passthrough work tracked against the openemr container — once apache inside the container adopts the runner's uid, none of these chown dances are needed anywhere. Assisted-by: Claude Code
bradymiller
added a commit
that referenced
this pull request
Jun 25, 2026
…-concurrent to 3 envs (#836) ## Summary Two e2e additions to `test-bats-openemr-cmd-real-docker.yml` covering gaps surfaced during the stocktake in #834. ### `prek-workflow-e2e` (new job) Drives the `prek-install` host-hook contract end-to-end via **real `git commit`** from inside a worktree, rather than invoking the hook scripts directly. Spins up a worktree on the default `easy` env, runs `openemr-cmd prek-install`, then runs three commits: 1. **commit-msg reject** — clean file + bad message → `conventional-commits` rejects at commit-msg stage 2. **pre-commit reject** — codespell-typo file + good message → `codespell` rejects at pre-commit stage before commit-msg ever runs 3. **happy path** — clean file + good message → both stages pass, commit lands, HEAD advances, file in the new tree Catches regressions in shim wiring, `prek_hooks_dir`'s git-common-dir resolution, cwd-aware container routing, and hook exit-code propagation that a direct hook invocation would miss. Cleans up via `worktree remove` (auto-chown from #833) + `prek-uninstall` + hygiene check. ### `worktree-multi-concurrent-e2e` (expanded 2 → 3 stacks) One worktree per env (`easy` / `easy-light` / `easy-redis`) running side-by-side. Gives `easy-redis` its only e2e coverage in this workflow and pressures the docker daemon under realistic developer-workflow parallelism. Distinct-offsets check, 3-way healthy poll, 3-way HTTP smoke, 3×3 exec cross-talk matrix, 3-way teardown via auto-chown. Timeout bumped 30 → 40 min for the third parallel boot. ## Test plan - [ ] All five jobs in `test-bats-openemr-cmd-real-docker.yml` pass on the PR - [ ] `prek-workflow-e2e` shows the three commit attempts produce the expected reject/reject/accept outcomes - [ ] `worktree-multi-concurrent-e2e` confirms three distinct openemr container IDs and clean teardown 🤖 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** * Strengthened end-to-end workflow validation for multi-environment runs, including improved health checks, port verification, isolation/no-cross-talk checks, and more complete teardown validation. * Enhanced failure diagnostics to surface issues more clearly when additional environments fail to initialize or cleanup properly. * **Tests** * Expanded automated workflow coverage with parallel multi-environment execution and additional commit hook/“prek” behavior checks (reject invalid messages, allow valid ones) plus hygiene verification. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
bradymiller
added a commit
that referenced
this pull request
Jun 26, 2026
…probe (#838) ## 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_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`. After `compose down --volumes`, those empty root-owned dirs reappear on the host and trip the writability probe on the next `worktree 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_add` and `cmd_worktree_regen` so both fresh and env-switched worktrees are covered. Idempotent (mkdir -p is safe on existing dirs, including the ones already checked into the repo like `public/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 — 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. ### Coverage matrix | Ownership case | Handler | |---|---| | Container-written files in bind mount (apache uid) | `#833` auto-chown via `docker exec` | | Empty docker-daemon-created mount-point dirs (root) | Pre-create (option 1) | | Pre-create drift / future new volumes | Probe loosening (option 2) | | Non-empty unwritable dirs | Probe still fails (correct — rm would too) | The 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 commit `cf99562`) 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 - [ ] New bats file `tests/bats/openemr-cmd/mountpoint_precreate.bats` exercises 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) - [ ] OC_SCRIPT_FUNCS_END bumped 1786 → 1861 to track the new function's landing position - [ ] CI's multi-3 e2e job passes without the sudo-chown workaround that was just removed - [ ] Other e2e jobs (lifecycle, prek, functional, nonworktree) still pass — pre-create is additive, doesn't change existing behavior 🤖 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** * Enhanced worktree setup to pre-create named-volume webroot mount-point directories from compose targets, and to repeat the same behavior during `worktree regen`. * **Bug Fixes** * Updated worktree removal to allow removing empty, non-writable directories when the parent is writable; non-empty non-writable directories still block removal. * **Tests** * Added BATS coverage for pre-create behavior (idempotent regen, symlink-safe traversal, webroot-only creation, and correct handling of bind mounts) and the adjusted removal permission checks. * **CI** * Streamlined teardown by removing a prior host-side ownership workaround and relying on existing auto-ownership behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
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
Follow-on to #826. The A5 permission probe catches container-uid files cleanly, but still requires the user to run a manual
sudo chown -Rbefore retry. The container itself has root, so it can do the chown for us before tear-down — eliminating the manual step in the common case.Change
In
cmd_worktree_remove, between the dir-exists check and the A5 probe:cmd_worktree_exec).docker exec -u root <id> chown -R <host-uid>:<host-gid> /var/www/localhost/htdocs/openemr.|| trueswallows non-zero exits. The probe is the safety net for anything chown couldn't fix (container not running, root-owned files outside the openemr workdir, chown itself failed).Why now
This was filed as the natural next step after a user hit the manual-chown requirement during cleanup of an in-flight worktree. The probe surfaced the issue cleanly, but the user had to context-switch into a sudo-capable terminal to fix it. With the container itself doing the chown, the most common case (root-owned files inside the openemr workdir) becomes invisible to the user.
Tests (
tests/bats/openemr-cmd/remove_graceful.bats)exec -u root <id> chown -R <uid>:<gid> /var/www/.../openemr+ "Auto-chowning bind mount via container" messageTest plan
shellcheck utilities/openemr-cmd/openemr-cmdcleanAssisted-by: Claude Code
Summary by CodeRabbit
openemr-cmd worktree removenow attempts an auto-chown inside the running app container (when available) to help prevent permission-related removal failures.openemr-cmd --versionto 1.0.48.sudo chown -Ron the worktree directory.