ci(shellcheck): drop SC2164 disable + guard all 25 cd sites#150
Merged
bradymiller merged 2 commits intoJun 29, 2026
Merged
Conversation
PR B of the ratchet cleanup. SC2164 flags `cd $X` without `|| exit` (or `|| return` inside a function) -- silent cd failures leave the script running in the wrong directory and any subsequent `rm -fr`, `git pull`, `tar -czf`, or similar operates somewhere it shouldn't. Same shape that caused the SC2115 footguns (openemr#145) but harder to spot because the wrong directory is downstream of the failure. All 25 sites guarded: - demo_build.sh (9 sites): top-level cd inside the demosGo loop body. `|| exit 1` -- the surrounding `for demo in ...` is the only retry point, and entering the next iteration with the wrong $PWD would scramble per-iteration paths. - docker/scripts/startFarm.sh (6 sites): repo update steps (`cd ~/<repo>; git pull; cd ~/`). `|| exit 1` -- if the repo dir is missing, the subsequent `git pull` would silently pull into $HOME or wherever cd left us. - docker/scripts/restartFarm.sh (6 sites): same pattern. - docker/scripts/refreshWebsite.sh (2 sites): same pattern. - docker/scripts/demoLibrary.source (2 sites): inside `snapshotDemo()` function, so `|| return 1` to bubble the failure up to the caller. Verification: - `shellcheck --check-sourced --external-sources` against every *.sh and *.source in the repo (excluding fixtures) -- clean. - `./tools/build-tests/test.sh` -- 7/7 PASS (Phase 1 dry-run suite catches behavioral surprises -- the cd guards run before every wrapped command, so a regression where cd silently failed would now fail loudly). - `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS. Disables remaining: SC1090, SC2006, SC2034, SC2086, SC2155 (5 of the original 15 from before this cleanup series). Refs: openemr#146 (parent issue), openemr#149 (prior shellcheck cleanup PR A) 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
ChangesFail-fast
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.
Actionable comments posted: 2
🤖 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 `@docker/scripts/restartFarm.sh`:
- Around line 33-36: The optional wkhtmltopdf update in restartFarm.sh is
currently fatal because the cd into the wkhtmltopdf-openemr checkout exits the
whole script when the repo is missing. Update this block so it only runs the git
fetch/pull when the wkhtmltopdf-openemr directory exists, and otherwise skips
the update and continues the farm restart. Keep the existing restart flow intact
by adjusting the conditional around the cd/git commands in restartFarm.sh.
In `@docker/scripts/startFarm.sh`:
- Around line 69-72: The optional wkhtmltopdf update step in startFarm.sh is
currently aborting the entire farm startup when the checkout is missing. Make
the update block non-blocking by checking for the presence of the
~/wkhtmltopdf-openemr directory before running the git fetch/pull, and skip that
step cleanly if it is absent. Keep the main startup flow unchanged so the
optional repo remains truly optional.
🪄 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: e1c45bce-d57d-4dfc-a860-9737e520f4a1
📒 Files selected for processing (6)
.shellcheckrcdemo_build.shdocker/scripts/demoLibrary.sourcedocker/scripts/refreshWebsite.shdocker/scripts/restartFarm.shdocker/scripts/startFarm.sh
💤 Files with no reviewable changes (1)
- .shellcheckrc
Rabbit-flagged regression in this PR. The previous code at startFarm.sh:68 and restartFarm.sh:32 had a comment marking `wkhtmltopdf-openemr` as optional. The unguarded `cd ~/...` silently failed when the directory was absent, so the subsequent `git pull` failed too -- but the script kept going. That silent failure WAS the "skip if absent" semantic. Adding `|| exit 1` (the SC2164 fix) turned the documented-optional repo into a hard requirement: a fresh host without the wkhtmltopdf checkout would exit before starting any docker containers. The build-tests suite (openemr#147/openemr#148) only covers demo_build.sh, so this regression wouldn't surface in CI -- the docker/scripts/* orchestration scripts run on the production EC2 host and were explicitly out-of-scope when openemr#146 was planned. Rabbit's review was the only layer that caught this. Fix: wrap both sites with `if [ -d ~/wkhtmltopdf-openemr ]` guards. Keeps the cd-failure-is-fatal semantic (so a stale dir without git history still exits loudly), but skips the block entirely when the optional repo isn't there. Refs: openemr#146 (issue), openemr#150 (this PR) Assisted-by: Claude Code
bradymiller
added a commit
that referenced
this pull request
Jun 30, 2026
…rune) (#151) * ci(shellcheck): drop SC1090 + SC2034 (source directives + dead-code prune) PR C1 of the ratchet cleanup. Closes two more disables for ~30 min of work; the bulk SC2086 audit is split into PR C2 (covered files) and PR C3 (uncovered files) so reviewers can size them independently. SC1090 (9 sites): all 9 docker/scripts/*.sh drivers source `~/demo_farm_openemr/docker/scripts/demoLibrary.source`. ShellCheck can't follow a `~`-relative path at lint time; the standard fix is a `# shellcheck source=docker/scripts/demoLibrary.source` directive above the source line. No behavior change. SC2034 (7 sites, audited each): demo_build.sh: - PASSWORDRESETSCRIPT (L241), GITTRANS (L243), FINALWEB (L321 + L326), passResetAuto (L463 + L465): all 4 variables were only referenced inside a commented-out 4-line dead block at the end of the demosGo loop body (`#if $passResetAuto; then ... #nohup php -f ${PASSWORDRESETSCRIPT} ${FINALWEB} 300 ${passReset} ...`). Deleted both the variables and the dead comment block -- the latter has been disabled long enough that the variables it references read as live, which is more misleading than helpful. If anyone wants to re-enable the auto-password-reset feature, the 4 lines aren't load- bearing history. tools/auto-derive/derive.sh: - CUR_REPO (L671): part of the documented CUR_* parser family (declared alongside siblings at L624, `# CUR_REPO` row in the schema comment at L612). Not currently consumed but kept for future derive logic. Inline `disable=SC2034` with rationale at the assignment site. - seen_base (L690, L696): marker written but never checked -- the surrounding alias-collection loop iterates `CUR_ORDER` which has no duplicates, so the dedup safeguard the marker was meant to provide is a no-op. Deleted. - subdomain (L843, L844): computed inside synth_master_row but never referenced. Vestigial -- the description string is what the function actually emits. Deleted along with the now- orphaned base_cluster local that fed it. Verification: - `shellcheck --check-sourced --external-sources` repo-wide (excluding fixtures) -- clean. - `./tools/build-tests/test.sh` -- 7/7 PASS. - `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS. Disables remaining after this PR: SC2006, SC2086, SC2155 (the trio in PR C2 / C3). Refs: #146 (parent), #149 + #150 (prior ratchet PRs) Assisted-by: Claude Code * fix: restore password-reset machinery (disabled-pending-fix, not dead) User-confirmed walk-back of part of the previous commit. The deletion of PASSWORDRESETSCRIPT, FINALWEB, passResetAuto, and the commented-out re-enable block was wrong -- this code is feature-toggled off, not truly dead, and the git history makes that explicit: 4e3a2ba (Dec 2022): "Temporarily disable the automated password reset feature in the demo farm since it is breaking the demos. Need to fix it at some point. See openemr/openemr#5991 for details." a393d37 (Dec 2022): "fix bug in prior commit. need to completely comment out the if block to temporarily stop the password reset mechanism" Corroborating evidence the feature is still expected to come back: - set_pass.php (7KB CLI script with modes 1-4) is preserved in the repo root. - ip_map_branch.txt's pass_reset column (col 16) is still actively populated for production rows -- demo.openemr.io (rows five / five_a / five_b) carry pass_reset=4 ("reset all official users on 6.0.0+"), encoding intent in the data even though the daemon isn't running. - The deleted variables are the entry points -- restoring them keeps re-enabling a one-line uncomment instead of a rewrite. Reverts: - PASSWORDRESETSCRIPT assignment (with inline SC2034 disable + openemr/openemr#5991 reference) - FINALWEB assignments in both demosGo branches (same treatment) - passResetAuto if/else block (same treatment) - The commented-out `#if $passResetAuto; then #nohup ... #fi` block, with a sharper header note that explicitly cites the issue and tells future maintainers why each piece is still here. Does NOT revert: - GITTRANS deletion: genuinely unused, never referenced anywhere, no disable-pending-fix history attached. - tools/auto-derive/derive.sh changes (CUR_REPO inline disable, seen_base + subdomain + base_cluster deletion) -- those audits stand. Goldens unchanged (the restored assignments are pure variable sets, no wrapped commands, no action-log emission). Refs: openemr/openemr#5991, #146 (parent), #151 (this PR) Assisted-by: Claude Code * docs(demo_build): align pass_reset comment with set_pass.php's real modes Rabbit-flagged in PR #151 review. The inline comment on the password- reset block I restored said `(if 1, then admin, if 2, then all)` -- that was the original 2018 documentation, never updated when modes 3 + 4 were added in 2021's openemr/openemr#5991 work. set_pass.php's actual $mode arg: 1 = just admin, OpenEMR <6.0.0 2 = all official users, <6.0.0 3 = just admin, OpenEMR 6.0.0+ 4 = all official users, 6.0.0+ Production demos at demo.openemr.io (rows five/five_a/five_b) all carry pass_reset=4, so the ip_map data and set_pass.php agree -- only the demo_build.sh comment was stale. Since the whole point of restoring these variables (instead of deleting them as I'd originally proposed) is to keep the future re-enable a one-line uncomment, the comment that the future re-enabler will read should match the code they'll be calling. No runtime change (the feature is still disabled per a393d37). Refs: openemr/openemr#5991, #146 (parent), #151 (this PR) Assisted-by: Claude Code
bradymiller
added a commit
that referenced
this pull request
Jun 30, 2026
PR C2 of the ratchet cleanup. Drops the two big disables for the
files covered by the build-tests + auto-derive fixture suites
(demo_build.sh, derive.sh). The 80 violations in
docker/scripts/demoLibrary.source are deferred to PR C3 with a
file-level shellcheck directive (so this PR can drop the rc
disables cleanly without C3 work blocking on this).
What changed:
demo_build.sh:
- SC2006 (19 sites): mechanical `\`cmd\`` -> `$(cmd)`.
- SC2086 (153 sites): shellcheck auto-fix applied via `-f diff`
to quote every unquoted variable. Reverted the 3 sites that
auto-fix wrongly quoted `$rpassparam` -- those are
intentional word-splitting (empty when no $mrp -> zero args;
"-p<pass>" when set -> one arg; quoting "" would pass an
empty positional arg that mariadb would misinterpret).
Inline `disable=SC2086` with rationale at each.
- SC2002 (13 sites): once SC2086 was cleaned up, shellcheck
surfaced "useless cat" warnings that were previously masked
at the same code positions. The pattern
`cat "$GITDEMOFARMMAP" | grep "$IPADDRESS" | ...`
becomes `grep "$IPADDRESS" "$GITDEMOFARMMAP" | ...` --
identical behavior, one less process per column lookup.
tools/auto-derive/derive.sh:
- SC2086 (1 site, L561): `$FLEX_ALPINES` in
`printf '%s\n' $FLEX_ALPINES | grep | sort` is intentional
word-splitting (space-separated list -> one printf arg each
-> one line of output each). Inline disable.
tools/build-tests/test.sh:
- SC2086 (1 site, L199): same shape as $rpassparam -- $EXTRA_ARGS
in `bash $DEMO_BUILD --dry-run $EXTRA_ARGS` is intentional
word-splitting (empty -> zero args, set -> multiple args).
Inline disable.
docker/scripts/demoLibrary.source:
- File-level `# shellcheck disable=SC2086,SC2006,SC2155` at the
top, with a comment explaining the C2/C3 split. C3 will fix
the 72 SC2086 + 4 SC2006 + 4 SC2155 sites individually and
remove this directive.
Verification:
- `shellcheck --check-sourced --external-sources` repo-wide
(excluding fixtures) -- clean.
- `./tools/build-tests/test.sh` -- 7/7 PASS.
- `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS.
Disables remaining in .shellcheckrc after this PR: SC2155 only. C3
will drop that (4 sites, all in demoLibrary.source) along with the
file-level directive above.
Refs: #146 (parent), #149 + #150 + #151 (prior ratchet PRs)
Assisted-by: Claude Code
bradymiller
added a commit
that referenced
this pull request
Jul 1, 2026
#156) PR C3a of the ratchet cleanup. Closes out SC2006 (rc-wide) and SC2155 (file-level) for docker/scripts/demoLibrary.source. SC2086 in this file is C3b's bulk SC2086 audit; the file-level disable is trimmed but not removed. SC2006 (4 sites, mechanical): - L91, L162, L284: local MYSQLIP=`docker inspect ...` - L285: local currentTime=`date "+..."` All converted to $(...) form. SC2155 (4 sites, surfaced after SC2006 fix): Same code positions as the SC2006 fix above. Once the backticks became $(...), shellcheck's "local x=$(cmd) masks return value" finding became visible. Same masked-warning pattern as the SC2002 fold-in we hit in C2 (cat-in-grep mask). Fix: split into two statements (declare separately, then assign). Now if `docker inspect` or `date` fails, the assignment carries the real exit code instead of being masked by `local`'s exit code. Note on the rc estimate: .shellcheckrc said "(4 violations)" for SC2155 but with SC2006 still active, shellcheck reported 0 SC2155 violations directly. The 4 became visible only after C3a's SC2006 fix, which is why this commit "closes out" both checks together rather than handling SC2155 as a separate concern. Side note: the rc estimate matched the actual count exactly (4) -- the comment was right, the count just wasn't reachable until SC2006 was cleaned. .shellcheckrc: - SC2155 dropped (the only remaining rc disable -- file is now clean except for the file-level SC2086 directive that C3b will retire). docker/scripts/demoLibrary.source file-level directive: - Trimmed from `disable=SC2086,SC2006,SC2155` to `disable=SC2086`. C3b will remove the directive entirely once the 72 SC2086 sites are individually fixed. Verification: - shellcheck repo-wide -- clean. - build-tests dry-run suite -- 7/7 PASS. - auto-derive fixture suite -- 8/8 PASS. State after this PR: .shellcheckrc is empty of disables (only external-sources=true remains). The file-level disable on demoLibrary.source is the last remaining scoped ratchet. Refs: #146 (parent), #149 + #150 + #151 + #152 (prior ratchet PRs) Assisted-by: Claude Code
bradymiller
added a commit
to bradymiller/demo_farm_openemr
that referenced
this pull request
Jul 1, 2026
PR C3b of the ratchet cleanup. Closes the file-level SC2086 disable
for docker/scripts/demoLibrary.source via shellcheck's `-f diff`
auto-fix (filtered with `-e SC2006,SC2155` so the diff covers only
SC2086 hunks; C3a owns the SC2006 + SC2155 work and is in flight
as a parallel PR).
Auto-fix changes (72 sites):
Pattern is uniformly mechanical quoting -- `${1}` -> `"${1}"`,
`$MYSQLIP` -> `"$MYSQLIP"`, etc. Most are inside docker / mysql /
mysqladmin command lines, where each `${...}` is a single arg
(cluster name, password, image string) -- safe to quote without
changing behavior. No intentional-word-split sites in this file
(unlike $rpassparam in demo_build.sh and $EXTRA_ARGS in test.sh
which needed inline disables in C2).
File-level disable on demoLibrary.source:
- Trimmed from `disable=SC2086,SC2006,SC2155` to `disable=SC2006`.
(The 4 SC2006 backtick sites + the 4 SC2155 sites they mask
are still here pending C3a's land.) Once C3a lands and is
merged, the directive becomes empty and can be removed entirely.
Verification:
- `shellcheck --check-sourced --external-sources` repo-wide -- clean.
- `./tools/build-tests/test.sh` -- 7/7 PASS.
- `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS.
Coordination with C3a:
Both PRs touch the same file-level `# shellcheck disable=...` line.
Whichever lands second will rebase and re-apply its directive
edit (or it'll be a trivial 3-way merge -- both PRs trim entries
from the same comma-separated list).
State after both C3a and C3b land:
- .shellcheckrc: zero `disable=` lines (just `external-sources=true`)
- demoLibrary.source file-level directive: removed entirely
- Full repo is shellcheck-clean.
Refs: openemr#146 (parent), openemr#149 + openemr#150 + openemr#151 + openemr#152 (prior ratchet PRs), openemr#156 (C3a)
Assisted-by: Claude Code
bradymiller
added a commit
that referenced
this pull request
Jul 1, 2026
PR C3b of the ratchet cleanup. Closes the file-level SC2086 disable
for docker/scripts/demoLibrary.source via shellcheck's `-f diff`
auto-fix (filtered with `-e SC2006,SC2155` so the diff covers only
SC2086 hunks; C3a owns the SC2006 + SC2155 work and is in flight
as a parallel PR).
Auto-fix changes (72 sites):
Pattern is uniformly mechanical quoting -- `${1}` -> `"${1}"`,
`$MYSQLIP` -> `"$MYSQLIP"`, etc. Most are inside docker / mysql /
mysqladmin command lines, where each `${...}` is a single arg
(cluster name, password, image string) -- safe to quote without
changing behavior. No intentional-word-split sites in this file
(unlike $rpassparam in demo_build.sh and $EXTRA_ARGS in test.sh
which needed inline disables in C2).
File-level disable on demoLibrary.source:
- Trimmed from `disable=SC2086,SC2006,SC2155` to `disable=SC2006`.
(The 4 SC2006 backtick sites + the 4 SC2155 sites they mask
are still here pending C3a's land.) Once C3a lands and is
merged, the directive becomes empty and can be removed entirely.
Verification:
- `shellcheck --check-sourced --external-sources` repo-wide -- clean.
- `./tools/build-tests/test.sh` -- 7/7 PASS.
- `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS.
Coordination with C3a:
Both PRs touch the same file-level `# shellcheck disable=...` line.
Whichever lands second will rebase and re-apply its directive
edit (or it'll be a trivial 3-way merge -- both PRs trim entries
from the same comma-separated list).
State after both C3a and C3b land:
- .shellcheckrc: zero `disable=` lines (just `external-sources=true`)
- demoLibrary.source file-level directive: removed entirely
- Full repo is shellcheck-clean.
Refs: #146 (parent), #149 + #150 + #151 + #152 (prior ratchet PRs), #156 (C3a)
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
PR B of the ShellCheck ratchet cleanup. Drops
SC2164from.shellcheckrcafter guarding all 25cdsites in the repo.The bug class:
cd $Xwithout|| exit(or|| returnin a function) silently fails and leaves the script running in the wrong directory. Any subsequentrm -fr,git pull,tar -czfthen operates somewhere it shouldn't. Same class of footgun as the SC2115 unguarded-rmcases #145 cleaned up, but harder to spot because the wrong directory is downstream of the failure rather than at the call site.Sites guarded
demo_build.shdocker/scripts/startFarm.shdocker/scripts/restartFarm.shdocker/scripts/refreshWebsite.shdocker/scripts/demoLibrary.sourceVerification
shellcheck --check-sourced --external-sourceson every*.sh/*.source(excluding fixtures) — clean../tools/build-tests/test.sh— 7/7 PASS../tools/auto-derive/fixtures-and-tests/test.sh— 8/8 PASS.The dry-run suite is the load-bearing safety net here: the cd guards run before every wrapped command in
demo_build.sh, so a regression that introduced a silently-failing cd would now exit loudly during the test rather than running the test with broken state.State of
.shellcheckrcafter this PR5 disables remaining (was 15 before the cleanup series began with #147):
SC2086(1255 sites) — PR C, the big one. Per-site eyeball audit because some uses intentionally rely on word splitting.SC2006(88 backticks →$()),SC2155(4 masked exit statuses),SC2034(7 apparently unused),SC1090(9 non-constantsource) — fold into PR C since the SC2086 audit touches every line anyway.🤖 Generated with Claude Code
Summary by CodeRabbit