test(openemr-cmd): bats permission-case coverage — probe matrix + #836 reproducer#839
Merged
bradymiller merged 2 commits intoJun 26, 2026
Conversation
…nemr#836 reproducer Adds 4 bats cases to mountpoint_precreate.bats covering permission scenarios that were previously script-logic-only (no hermetic tests pinning the behavior): 1. Empty non-writable + non-writable parent → probe fails. The probe- loosening rule "empty + parent writable = rmdir-able" requires BOTH conditions; this 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 openemr#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. Pinning the bug as a regression guard against future drift. No production code changes — pure scaffolding. 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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds four ChangesWorktree remove probe coverage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
tests/bats/openemr-cmd/mountpoint_precreate.bats (1)
264-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test to match the exercised mode.
The body intentionally switched from
0700to0000, but the@testname and leading comment still advertise a0700case. That makes CI output misleading when this scenario fails.🤖 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/mountpoint_precreate.bats` around lines 264 - 279, Update the test case name and the leading comment in mountpoint_precreate.bats to match the actual permissions scenario being exercised. The current test body uses the unreadable 0000 case, so rename the `@test` description and adjust nearby commentary to reflect 0000 rather than 0700, keeping the intent aligned with the probe-unreadable scenario.
🤖 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.
Nitpick comments:
In `@tests/bats/openemr-cmd/mountpoint_precreate.bats`:
- Around line 264-279: Update the test case name and the leading comment in
mountpoint_precreate.bats to match the actual permissions scenario being
exercised. The current test body uses the unreadable 0000 case, so rename the
`@test` description and adjust nearby commentary to reflect 0000 rather than 0700,
keeping the intent aligned with the probe-unreadable scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3aea6304-c317-45fd-b95f-90a1e63e614d
📒 Files selected for processing (1)
tests/bats/openemr-cmd/mountpoint_precreate.bats
… exercises CodeRabbit caught a stale name: the test was titled "mode 0700" but the body uses chmod 0000 (since 0700 keeps owner rwx and would never trip the probe-fail branch for the test user who owns the dir). The name mismatch would make CI output misleading on regression. Rename to "mode 0000" and rewrite the leading comment to explain the 0700 vs 0000 distinction inline — preserves the design rationale that prompted the switch. 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
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:Empty non-writable + non-writable parent → probe fails. The probe-loosening rule from fix(openemr-cmd): pre-create volume mount-point dirs + loosen remove probe #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.
Unreadable dir (mode 0000) → probe fails conservatively. When we can't
[[ -r && -x ]]the dir, we can't verify emptiness viafind; the probe must treat it as blocking (rm could silently leave content behind otherwise).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.
Reproducer for the multi-3 CI failure in test(openemr-cmd): e2e — prek install + real git commit; expand multi-concurrent to 3 envs #836: stage
ccdaservice/packages/oe-cqm-service/node_modulesas 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.ymlcovers the cause-side end-to-end. The two are complementary.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
worktree removewhen mount points or directories are not writable.