fix(openemr-cmd): auto-chown bind mount via container before worktree remove (#833)

## 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 -R`
before 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:

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`.
3. Best-effort: stderr suppressed, `|| true` swallows 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).
4. Probe runs as before; usually passes now.
5. Destructive ops proceed.

## 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`)

| Scenario | Assertion |
|---|---|
| Container running | docker.log shows `exec -u root <id> chown -R
<uid>:<gid> /var/www/.../openemr` + "Auto-chowning bind mount via
container" message |
| No container running | chown step skipped silently (no exec
invocation, no log line); remove still completes |
| Chown returns non-zero | attempt logged, failure ignored, probe +
destructive ops continue, remove still succeeds |

## Test plan

- [x] Full hermetic bats suite — 293 tests, 0 failures
- [x] `shellcheck utilities/openemr-cmd/openemr-cmd` clean
- [ ] CI: BATS openemr-cmd (ubuntu-22.04) green
- [ ] CI: BATS openemr-cmd (macos-14) green
- [ ] CI: ShellCheck / actionlint green
- [ ] CI: e2e jobs green (the lifecycle job's A5 probe step should still
pass — the auto-chown runs first but the probe step intentionally
re-creates the container-uid state, so the probe contract is unchanged)
- [ ] CodeRabbit review

Assisted-by: Claude Code

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* `openemr-cmd worktree remove` now attempts an auto-chown inside the
running app container (when available) to help prevent
permission-related removal failures.
  * Updated `openemr-cmd --version` to **1.0.48**.
* **Bug Fixes**
* Removal continues to succeed even if the container auto-chown attempt
fails.
* Improved failure guidance with two recovery options: start the stack
and retry, or manually run `sudo chown -R` on the worktree directory.
* **Tests**
* Expanded BATS coverage for auto-chown behavior: confirmation abort,
container running, container not running, and container chown errors.
* Updated test helper behavior to source the correct portion of the
command script.
* **Chores**
* Adjusted CI workflow concurrency to cancel in-progress runs only for
pull requests.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Code <noreply@anthropic.com>

Author: bradymiller

.github/workflows/test-bats-openemr-cmd-real-docker.yml

@@ -50,11 +50,19 @@ on:
 permissions:
   contents: read
 
-# Multiple in-flight runs would race on docker daemon resources (image
-# pull, container names, port allocation). Serialize per ref.
+# On PR pushes: cancel any in-flight run for the same ref so the new SHA's
+# workflow registers as a check immediately. This keeps the All Checks
+# Passed rollup honest — without it, a queued workflow doesn't register
+# its jobs as checks on the new SHA, and the rollup (which waits for
+# whatever checks exist) goes green prematurely. On master pushes: keep
+# the in-flight run so sequential master history isn't lost. Matches the
+# pattern in vendored-contracts-self-test.yml.
+#
+# Cross-run docker daemon contention isn't a concern here: each workflow
+# run gets its own ephemeral GitHub-hosted runner with its own daemon.
 concurrency:
   group: ${{ github.workflow }}-${{ github.ref }}
-  cancel-in-progress: false
+  cancel-in-progress: ${{ github.event_name == 'pull_request' }}
 
 jobs:
   worktree-lifecycle-e2e:

tests/bats/openemr-cmd/helpers.bash

@@ -79,7 +79,7 @@ STUB
 # If the script is restructured, bump this constant — the test
 # 'OC_SCRIPT_FUNCS_END points at end of function defs' in helpers_pure.bats
 # will fail loudly when it drifts.
-OC_SCRIPT_FUNCS_END=1741
+OC_SCRIPT_FUNCS_END=1786
 
 # Source ONLY the function definitions of openemr-cmd into the current shell.
 # Caller is responsible for setting OPENEMR_ROOT (and WT_STATE_FILE / others

tests/bats/openemr-cmd/remove_graceful.bats

@@ -165,10 +165,14 @@ setup_full_worktree() {
     chmod 0700 "${sub}" 2>/dev/null || true
 
     assert_failure
-    # Clear, actionable error mentioning the unwritable dir + chown hint.
+    # Clear, actionable error mentioning the unwritable dir + BOTH
+    # recovery options (start-stack auto-chown OR manual sudo chown).
     assert_output --partial "is not writable by you"
-    assert_output --partial "sudo chown -R"
     assert_output --partial "${sub}"
+    # Option 1: start the stack first, auto-chown handles it.
+    assert_output --partial "openemr-cmd worktree up"
+    # Option 2: manual host-side sudo chown.
+    assert_output --partial "sudo chown -R"
     # Nothing destructive ran: no compose down, no rm of dir, state intact.
     if grep -F -e " down " "${STUB_DIR}/docker.log" >/dev/null 2>&1; then
         cat "${STUB_DIR}/docker.log"
@@ -202,6 +206,156 @@ setup_full_worktree() {
     assert_output "false"
 }
 
+# --- Auto-chown via container before destruction --------------------------
+# When the openemr container is still running, cmd_worktree_remove asks it
+# (as root, in-container) to chown the bind-mounted worktree back to the
+# host uid/gid BEFORE the destructive ops. This eliminates the manual
+# `sudo chown -R` step that users would otherwise hit when the container
+# wrote root-owned files (e.g. after drid). The A5 probe remains the
+# safety net for cases where the chown wasn't possible.
+
+@test "remove: aborted at the prompt leaves NO side effects (no auto-chown, no destruction)" {
+    # The auto-chown sequence runs AFTER the user's y/N confirmation.
+    # If the user aborts, the container's ownership of bind-mounted files
+    # must stay exactly as it was — chowning to the host uid would leave
+    # apache (inside the container) unable to write to its own files on
+    # hosts where host-uid != container-apache-uid (uid 1000), effectively
+    # breaking the stack for continued use.
+    setup_full_worktree feature-rm-abort -b
+    local wt_dir
+    wt_dir=$(jq -r '.["feature-rm-abort"].dir' "${STATE_FILE}")
+    : > "${STUB_DIR}/docker.log"
+    # Pipe 'n' to the prompt to abort; DOCKER_PS_OUTPUT is set so that
+    # IF the auto-chown step were misplaced before the prompt, the chown
+    # invocation would be recorded — its absence below proves the reorder.
+    run bash -c "echo n | env \
+        PATH='${STUB_DIR}:${PATH}' \
+        OPENEMR_ROOT='${TMP_OPENEMR_ROOT}' \
+        WORKTREE_PARENT='${TMP_WT_PARENT}' \
+        DOCKER_PS_OUTPUT='fake-openemr-container-id' \
+        '${SCRIPT}' worktree remove feature-rm-abort"
+    assert_success
+    assert_output --partial "Aborted."
+    # The auto-chown banner must NOT have fired.
+    refute_output --partial "Auto-chowning bind mount via container"
+    # The chown docker exec must NOT have been recorded.
+    if grep -F "exec -u root" "${STUB_DIR}/docker.log" >/dev/null 2>&1; then
+        cat "${STUB_DIR}/docker.log"
+        fail "auto-chown fired before the prompt; aborted remove left side effects"
+    fi
+    # And nothing destructive: dir still on disk, state entry still present.
+    [[ -d "${wt_dir}" ]] || fail "worktree dir gone after aborted remove"
+    run jq -r 'has("feature-rm-abort")' "${STATE_FILE}"
+    assert_output "true"
+}
+
+@test "remove: when container is running, docker exec chown is invoked with host uid:gid" {
+    setup_full_worktree feature-rm-autochown -b
+    local wt_dir
+    wt_dir=$(jq -r '.["feature-rm-autochown"].dir' "${STATE_FILE}")
+    : > "${STUB_DIR}/docker.log"
+    # DOCKER_PS_OUTPUT is what the stub returns for any `docker ps ...`
+    # query; the openemr-cmd auto-chown step uses that filter to find the
+    # worktree's openemr container.
+    local uid gid
+    uid=$(id -u)
+    gid=$(id -g)
+    run bash -c "echo y | env \
+        PATH='${STUB_DIR}:${PATH}' \
+        OPENEMR_ROOT='${TMP_OPENEMR_ROOT}' \
+        WORKTREE_PARENT='${TMP_WT_PARENT}' \
+        DOCKER_PS_OUTPUT='fake-openemr-container-id' \
+        '${SCRIPT}' worktree remove feature-rm-autochown"
+    assert_success
+    # User-facing message confirms the auto-chown ran.
+    assert_output --partial "Auto-chowning bind mount via container"
+    assert_output --partial "uid=${uid}"
+    # The recorded docker invocation is the chown with the right shape:
+    # exec -u root <id> chown -R <uid>:<gid> /var/www/.../openemr
+    grep -Fq "exec -u root fake-openemr-container-id chown -R ${uid}:${gid} /var/www/localhost/htdocs/openemr" \
+        "${STUB_DIR}/docker.log" \
+        || { cat "${STUB_DIR}/docker.log"; fail "expected docker exec chown invocation not recorded"; }
+    # And the core outcome: the worktree directory itself is gone.
+    [[ ! -e "${wt_dir}" ]] || fail "worktree directory still exists: ${wt_dir}"
+}
+
+@test "remove: when container is NOT running, auto-chown step is skipped silently" {
+    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"
+    # DOCKER_PS_OUTPUT empty → the chown lookup returns nothing → step skipped.
+    run bash -c "echo y | env \
+        PATH='${STUB_DIR}:${PATH}' \
+        OPENEMR_ROOT='${TMP_OPENEMR_ROOT}' \
+        WORKTREE_PARENT='${TMP_WT_PARENT}' \
+        '${SCRIPT}' worktree remove feature-rm-no-container"
+    assert_success
+    # No "Auto-chowning" message; no chown invocation in log.
+    refute_output --partial "Auto-chowning bind mount via container"
+    if grep -F "exec -u root" "${STUB_DIR}/docker.log" >/dev/null 2>&1; then
+        cat "${STUB_DIR}/docker.log"
+        fail "auto-chown ran despite no container being detected"
+    fi
+    # And remove still succeeded — state entry gone, dir gone.
+    run jq -r 'has("feature-rm-no-container")' "${STATE_FILE}"
+    assert_output "false"
+    [[ ! -e "${wt_dir}" ]] || fail "worktree directory still exists: ${wt_dir}"
+}
+
+@test "remove: auto-chown failure is non-fatal — probe is still the safety net" {
+    # Use a stub variant where `docker exec` returns non-zero (simulating a
+    # chown failure inside the container). The remove flow should still
+    # complete: chown failure is logged + ignored, probe finds no
+    # unwritable dirs (since the fixture doesn't have any), destructive
+    # ops proceed.
+    local failing_stub
+    failing_stub=$(oc_mktempdir)
+    cat > "${failing_stub}/docker" <<'STUB'
+#!/bin/sh
+echo "$@" >> STUBLOG
+# Plugin probe must succeed so check_docker_compose_install picks compose.
+if [ "$1" = "compose" ] && [ "$#" = "1" ]; then exit 0; fi
+# `ps` queries return the fake container id so the auto-chown reaches exec.
+case " $* " in
+    *' ps '*) echo 'fake-id-but-exec-will-fail' ;;
+esac
+# Fail `docker exec ...` so the auto-chown returns non-zero. The script's
+# `|| true` should swallow this and continue.
+if [ "$1" = "exec" ]; then exit 17; fi
+exit 0
+STUB
+    sed -i.bak "s|STUBLOG|${failing_stub}/docker.log|g" "${failing_stub}/docker" 2>/dev/null \
+        || sed -i "" "s|STUBLOG|${failing_stub}/docker.log|g" "${failing_stub}/docker"
+    rm -f "${failing_stub}/docker.bak"
+    : > "${failing_stub}/docker.log"
+    chmod +x "${failing_stub}/docker"
+
+    setup_full_worktree feature-rm-chown-fails -b
+    local wt_dir
+    wt_dir=$(jq -r '.["feature-rm-chown-fails"].dir' "${STATE_FILE}")
+    # Re-record the docker log against the failing stub (setup_full_worktree
+    # used the original STUB_DIR for the `add`; the remove below uses the
+    # failing stub).
+    run bash -c "echo y | env \
+        PATH='${failing_stub}:${PATH}' \
+        OPENEMR_ROOT='${TMP_OPENEMR_ROOT}' \
+        WORKTREE_PARENT='${TMP_WT_PARENT}' \
+        '${SCRIPT}' worktree remove feature-rm-chown-fails"
+    assert_success
+    # The chown invocation WAS attempted (proves the failure wasn't a
+    # silent skip), even though it returned non-zero.
+    grep -Fq "exec -u root fake-id-but-exec-will-fail chown" "${failing_stub}/docker.log" \
+        || { cat "${failing_stub}/docker.log"; fail "auto-chown attempt not recorded"; }
+    # Remove still succeeded — state gone, dir gone.
+    run jq -r 'has("feature-rm-chown-fails")' "${STATE_FILE}"
+    assert_output "false"
+    [[ ! -e "${wt_dir}" ]] || fail "worktree directory still exists: ${wt_dir}"
+    rm -rf "${failing_stub}"
+}
+
+# --- existing tamper guard ------------------------------------------------
+
 @test "remove: when state dir is outside WORKTREE_PARENT, validation refuses early (no destructive action)" {
     # Tamper with the state file so dir points OUTSIDE WORKTREE_PARENT.
     # wt_validate_dir (invoked inside wt_compose_cmd before any docker or

utilities/openemr-cmd/openemr-cmd

@@ -18,7 +18,7 @@ set -euo pipefail
 #################################################################################################
 
 # Increment the version when modify script
-VERSION="1.0.47"
+VERSION="1.0.48"
 
 # ============================================================================
 # WORKTREE STATE MANAGEMENT
@@ -835,16 +835,60 @@ cmd_worktree_remove() {
         return 0
     fi
 
-    # Probe for permission issues BEFORE any destructive op. The openemr
-    # container writes files into the bind-mounted worktree as a non-host
-    # uid (root/apache), which makes subdirectories unwritable for the host
-    # user. If we proceed without catching that, we'd:
+    echo "This will remove worktree '${branch}' at ${dir}."
+    if ! ${purge}; then echo "  --keep-volumes: Docker volumes will be preserved."; fi
+    read -rp "Continue? [y/N] " confirm
+    [[ "${confirm}" =~ ^[Yy]$ ]] || { echo "Aborted."; exit 0; }
+
+    # Best-effort auto-chown: when the openemr container is still running,
+    # ask it (as root) to chown the bind-mounted worktree back to the host
+    # uid/gid before we try to remove anything. The container writes files
+    # as root during install / drid / other admin paths; on hosts where
+    # the operator's uid != the container's apache uid, those files become
+    # unreadable + unremovable from the host side. Doing the chown from
+    # INSIDE the container (where it has root) sidesteps the manual
+    # `sudo chown -R` step that users would otherwise hit. The probe below
+    # is still the safety net — it catches anything chown missed (container
+    # not running, root-owned files outside the openemr workdir, etc.).
+    #
+    # Runs AFTER the user's y/N confirmation, not before. The chown is
+    # observable side effect for hosts where host-uid != container-apache-uid
+    # (the bind mount's ownership changes to the host user; apache in the
+    # container then cannot write to those files). On abort, that side
+    # effect would leave the stack effectively broken for continued use.
+    # Confirming first means abort genuinely leaves no trace.
+    local slug=""
+    slug=$(wt_slug "${branch}")
+    local autochown_id=""
+    autochown_id=$(docker ps \
+        --filter "label=com.docker.compose.project=openemr-${slug}" \
+        --filter "label=com.docker.compose.service=openemr" \
+        --format '{{.ID}}' 2>/dev/null | head -n 1 || true)
+    if [[ -n "${autochown_id}" ]]; then
+        # Capture uid/gid once (shellcheck SC2312: don't mask command
+        # substitution return values inside an expanded string).
+        local host_uid host_gid
+        host_uid=$(id -u)
+        host_gid=$(id -g)
+        wt_info "Auto-chowning bind mount via container (uid=${host_uid} gid=${host_gid})"
+        # `|| true`: chown failures are non-fatal; the probe below catches
+        # any unwritable dirs the chown couldn't fix.
+        docker exec -u root "${autochown_id}" \
+            chown -R "${host_uid}:${host_gid}" /var/www/localhost/htdocs/openemr \
+            2>/dev/null || true
+    fi
+
+    # Probe for permission issues before the destructive ops. After the
+    # auto-chown step above, this usually passes — but it remains as the
+    # safety net for: (a) container wasn't running, (b) root-owned files
+    # outside /var/www/localhost/htdocs/openemr, (c) chown itself failed.
+    # If we proceed without catching that, we'd:
     #   1. purge the compose volumes (wt_compose_cmd down --volumes),
     #   2. let `git worktree remove --force` unregister the worktree, and
     #   3. fail mid-rm with EACCES — leaving partial state on disk + a
     #      state entry pointing at a no-longer-registered worktree, which
     #      a chown+retry can't recover from (wt_validate_dir rejects the
-    #      now-orphan dir). Bail early with a clear chown hint instead.
+    #      now-orphan dir). Bail with a clear chown hint instead.
     # `[[ -w ${d} ]]` uses access(2) for the current uid; we walk every
     # subdir because `rm` needs write+execute on each PARENT to unlink
     # children. Not using `find -writable` (GNU-only — BSD/macOS lacks it).
@@ -860,15 +904,16 @@ cmd_worktree_remove() {
         # releases the lock — no manual release needed.
         wt_die "Cannot remove worktree '${branch}': directory '${unwritable}' is not writable by you.
 This usually means the docker container wrote files into the worktree as a
-different uid (root/apache). Restore ownership and retry:
-    sudo chown -R \"\$(id -u):\$(id -g)\" '${dir}'"
+different uid (root/apache). Two ways to recover:
+  (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}'"
     fi
 
-    echo "This will remove worktree '${branch}' at ${dir}."
-    if ! ${purge}; then echo "  --keep-volumes: Docker volumes will be preserved."; fi
-    read -rp "Continue? [y/N] " confirm
-    [[ "${confirm}" =~ ^[Yy]$ ]] || { echo "Aborted."; exit 0; }
-
     wt_info "Stopping Docker stack"
     if ${purge}; then
         wt_compose_cmd "${branch}" down --volumes 2>/dev/null