feat(build-tests): add Phase 1 dry-run regression suite for demo_build.sh (#146) by bradymiller · Pull Request #147 · openemr/demo_farm_openemr

bradymiller · 2026-06-28T22:22:07Z

Summary

Phase 1 of #146 — per-PR regression coverage for demo_build.sh.

demo_build.sh --dry-run skips side-effecting commands (composer, npm, mariadb, apk, git clone, curl, httpd, postfix, stunnel, system-path writes) but emits a structured ACTION: log line for each one through three new helpers: run_action (argv), run_action_sh (pipes/redirects), run_action_capture (with stub value for $(curl …) style capture). Pure parsing/branching code still runs, so the action log captures what the script intended to do.

tools/build-tests/test.sh runs each fixture's ip_map_branch.txt through demo_build.sh --dry-run, normalizes the captured action log (replaces the throwaway work-dir path with the literal token <WORK>), and diffs against the golden in expected/action-log.txt. Mirrors the tools/auto-derive/fixtures-and-tests/ pattern that #135 introduced.

Credential redaction in the emitter covers -p<pass> (mariadb root flag), rootpass=<pass> (InstallerAuto arg), Authorization: token <key> (composer github API probe), and --auth github-oauth.github.com <key> (composer config). Dry-run stubs don't get touched (<DRY_RUN_GITHUB_KEY> becomes \<…\> after %q, which dodges the [A-Za-z0-9_]+ body match); Phase 2 live runs would otherwise leak the real token to ACTION_LOG.

Scenarios

Seven fixtures, picked from real production rows where possible to exercise the actual branching surface of the script:

Scenario	Row(s)	Coverage
`branch-pinned-master`	`two`	master clone, udt=1 (dev translations), ppapi=1 (portal/API globals), random theme (ThemeTwo)
`tag-pinned-with-data`	`five`	v8_0_0_3 tag clone, demo data SQL, demoDataUpgrade (sed transforms + ALTER TABLE/DATABASE on `$v_major≥6`), random theme (ThemeOne)
`release-packageserve`	`six`	v7_0_4 tag, `sp=1` → exercises the entire ~80-line packageServe block (a second composer/npm/phing pass + tar/zip/md5sum packaging)
`up-for-grabs-fork`	`four_a`	non-canonical fork URL (juggernautsei/openemr.git) + non-canonical branch + demo data
`light-reset`	`two_a` + positional arg	`lightReset=true` path, single-subdemo, skips post-loop block
`multi-demo`	`seven`/`seven_a`/`seven_b`	`DOCKERNUMBERDEMOS=3` loop iteration
`capsule-path-traversal`	synthetic `malcap`	fail-expected — synthetic row with `capsule="../etc"` exercises the case statement guard PR #145 added (`''

CI

.github/workflows/build-tests.yml runs the suite on every PR touching demo_build.sh, tools/build-tests/**, or the workflow itself. Wall time: seconds.

Two .shellcheckrc entries added (SC3043 local, SC3050 printf %q) — same rationale as the five existing SC3xxx disables: the script carries a stale #!/bin/sh shebang while using bash features throughout, and the new helpers extend that gap by two more bashisms. Consistent with the ratchet's existing posture.

What's NOT in this PR

Live end-to-end execution. That's Phase 2 (PR feat(build-tests): Phase 2 — live end-to-end integration tests (#146) #148, stacked on this one) — actually runs demo_build.sh against a real mariadb + openemr flex container in CI matrix-parallel, and asserts the resulting container survives a healthcheck plus its action log matches a per-fixture golden.

Test plan

./tools/build-tests/test.sh — all 7 PASS locally
Mutation test: deliberately stripped the capsule path-traversal guard → only that fixture FAILed (negative test of the new fail-expected harness path)
shellcheck demo_build.sh tools/build-tests/test.sh — clean
bash -n demo_build.sh — clean
Production-call surface unchanged: with no --dry-run and no ACTION_LOG env var, helpers append to /dev/null and exec the wrapped command — behavior is byte-identical to pre-PR for the docker entrypoint callers
CI: all checks green (ShellCheck, build-tests dry-run, Derive ip_map test, CodeRabbit)

Refs: #146 (issue), #135 (fixture pattern this mirrors)

🤖 Generated with Claude Code

coderabbitai · 2026-06-28T22:22:27Z

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds dry-run support and action logging to demo_build.sh, adds a fixture-driven harness to compare dry-run logs, and includes scenario fixtures, expected outputs, a workflow, and ShellCheck suppressions.

Changes

Dry-run testing infrastructure

Layer / File(s)	Summary
run_action wrappers and CLI parsing `demo_build.sh`	Adds dry-run and light-reset parsing, `ACTION_LOG` handling, environment-style defaults, and the `run_action`, `run_action_sh`, and `run_action_capture` helpers.
Instrumented demo_build.sh operations `demo_build.sh`	Routes clone, copy, permission, dependency, installer, database, capsule, swagger, packaging, service-startup, and hold-open steps through the action helpers.
Fixture test runner `tools/build-tests/test.sh`	Introduces `tools/build-tests/test.sh` with fixture discovery, work-tree setup, action-log normalization, per-fixture dry-run execution, golden update or diffing, and summary reporting.
Fixture scenarios and goldens `tools/build-tests/fixtures/branch-pinned-master/`, `tools/build-tests/fixtures/light-reset/`, `tools/build-tests/fixtures/multi-demo/`, `tools/build-tests/fixtures/release-packageserve/`, `tools/build-tests/fixtures/tag-pinned-with-data/`, `tools/build-tests/fixtures/capsule-path-traversal/`, `tools/build-tests/fixtures/up-for-grabs-fork/*`	Adds fixture `inputs.env`, `ip_map_branch.txt`, and expected output files for the scenario set.
CI workflow and ShellCheck config `.github/workflows/build-tests.yml`, `.shellcheckrc`	Adds the path-filtered pull_request workflow with manual dispatch support and suppresses the new ShellCheck warnings.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Add regression testing for demo_build.sh + orchestration scripts #146: This PR directly implements the dry-run regression harness described in that issue, adding the wrappers, fixture test runner, and CI workflow.

Possibly related PRs

openemr/demo_farm_openemr#145: Overlaps the same demo_build.sh capsule/path-handling areas now routed through dry-run helpers.

Poem

🐇 I hop through logs with careful feet,
Dry-run trails make tests complete.
Fixture rows in tidy lines,
Match the goldens, branch by branch and pine.
No side effects, just moonlit hops,
While CI keeps watch and never stops.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 72.73% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: adding a dry-run regression suite for demo_build.sh.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

…d.sh Phase 1 of issue openemr#146: per-PR regression coverage that catches the PR openemr#142 (printf '-' dash) and PR openemr#145 (SC2115 rm -fr footguns) class of bug at PR time in seconds, rather than waiting for the nightly host reset to surface them. Three pieces: - demo_build.sh: additive --dry-run flag + ACTION_LOG emitter (run_action / run_action_sh / run_action_capture helpers). With no flag and no ACTION_LOG env, helpers degrade to plain exec -- production behavior is unchanged. - tools/build-tests/: fixture-test harness mirroring the tools/auto-derive/fixtures-and-tests/ pattern. 6 scenarios cover branch-pinned/tag-pinned/release-packageserve/fork-URL/ light-reset/multi-demo paths. - .github/workflows/build-tests.yml: runs the suite on every PR that touches demo_build.sh or tools/build-tests/**. Phase 2 (separate PR) will add a slower nightly job that actually executes demo_build.sh end-to-end against a compose stack, reusing the same fixtures + action-log emitter, and asserting the dry-run log matches what live execution produces -- the integrity check that keeps Phase 1 honest. Refs: openemr#146 Assisted-by: Claude Code

coderabbitai

Actionable comments posted: 4

🤖 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 @.github/workflows/build-tests.yml:
- Around line 14-23: The workflow trigger in build-tests is inconsistent with
the intent comment: pull_request path filtering in the workflow does not include
the derive-ip-map workflow file, so PRs that change it will not run this suite.
Update the on.pull_request.paths in the build-tests workflow to include
.github/workflows/derive-ip-map.yml, or remove the comment if that trigger is no
longer intended, keeping the paths list aligned with the workflow’s actual
behavior.

In `@demo_build.sh`:
- Around line 60-64: ACTION_LOG currently writes raw argv/shell text from the
logging helpers, which can persist secrets such as GitHub tokens, database
password flags, and rootpass values. Update the action logging paths in the
relevant logging helper blocks to redact known credential patterns before
appending to ACTION_LOG, or reject action logging when real secrets are present.
Make sure the fix is applied consistently across the helper that emits
ACTION_LOG so all affected logging sites use the same sanitization behavior.
- Around line 733-737: Guard the real cd steps in the package build flow so
dry-run does not try to enter directories that were never created. Update the
openemr build path and the similar package-serve path that use cd after
run_action/run_action_sh (for example the openemr/vendor check block and the
FILESSERVEDIR sequence) to skip or conditionally avoid cd when dry-run is
active, using the existing build helpers and the surrounding package build logic
as the reference points.
- Around line 73-77: The run_action_sh helper is executing shell-form strings
with eval, which can re-parse env-derived values and allow unintended command
execution. Refactor run_action_sh to accept a command plus positional arguments
and invoke it directly without eval, then update the call sites that pass
OPENEMR, GIT, and TMPDIR-derived paths to build fixed argument lists instead of
shell strings.

🪄 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: 4885dd46-03d9-4344-925d-87333afed7d3

📥 Commits

Reviewing files that changed from the base of the PR and between 000d131 and 61b3b07.

📒 Files selected for processing (24)

.github/workflows/build-tests.yml
.shellcheckrc
demo_build.sh
tools/build-tests/fixtures/branch-pinned-master/expected/action-log.txt
tools/build-tests/fixtures/branch-pinned-master/inputs.env
tools/build-tests/fixtures/branch-pinned-master/ip_map_branch.txt
tools/build-tests/fixtures/light-reset/expected/action-log.txt
tools/build-tests/fixtures/light-reset/inputs.env
tools/build-tests/fixtures/light-reset/ip_map_branch.txt
tools/build-tests/fixtures/multi-demo/expected/action-log.txt
tools/build-tests/fixtures/multi-demo/inputs.env
tools/build-tests/fixtures/multi-demo/ip_map_branch.txt
tools/build-tests/fixtures/release-packageserve/expected/action-log.txt
tools/build-tests/fixtures/release-packageserve/inputs.env
tools/build-tests/fixtures/release-packageserve/ip_map_branch.txt
tools/build-tests/fixtures/tag-pinned-with-data/expected/action-log.txt
tools/build-tests/fixtures/tag-pinned-with-data/extra/git/demo_farm_openemr/pieces/demo_5_0_0_5.sql
tools/build-tests/fixtures/tag-pinned-with-data/inputs.env
tools/build-tests/fixtures/tag-pinned-with-data/ip_map_branch.txt
tools/build-tests/fixtures/up-for-grabs-fork/expected/action-log.txt
tools/build-tests/fixtures/up-for-grabs-fork/extra/git/demo_farm_openemr/pieces/demo_5_0_0_5.sql
tools/build-tests/fixtures/up-for-grabs-fork/inputs.env
tools/build-tests/fixtures/up-for-grabs-fork/ip_map_branch.txt
tools/build-tests/test.sh

coderabbitai · 2026-06-28T22:28:20Z

+run_action_sh () {
+  printf 'ACTION_SH: %s\n' "$1" >> "$ACTION_LOG"
+  if ! $dryRun; then
+    eval "$1"
+  fi


🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash set -euo pipefail dryRun=false ACTION_LOG=/dev/null run_action_sh () { printf 'ACTION_SH: %s\n' "$1" >> "$ACTION_LOG" if ! $dryRun; then eval "$1" fi } rm -f /tmp/coderabbit-eval-poc OPENEMR='$(touch /tmp/coderabbit-eval-poc)' run_action_sh "printf '%s\n' $OPENEMR >/dev/null" test -f /tmp/coderabbit-eval-poc && echo "reproduced eval reparse"

Repository: openemr/demo_farm_openemr

Length of output: 187

🏁 Script executed:

#!/bin/bash set -euo pipefail # Locate the script and inspect the relevant regions. git ls-files demo_build.sh wc -l demo_build.sh sed -n '60,90p' demo_build.sh printf '\n---\n' sed -n '450,485p' demo_build.sh printf '\n---\n' sed -n '720,745p' demo_build.sh # Show every call to run_action_sh so the risk can be scoped precisely. rg -n 'run_action_sh\s*\(' demo_build.sh

Repository: openemr/demo_farm_openemr

Length of output: 3809

🏁 Script executed:

#!/bin/bash set -euo pipefail # Show how the script gets its inputs and whether they can be influenced externally. sed -n '1,140p' demo_build.sh printf '\n--- run_action_sh callsites ---\n' rg -n -C 2 'run_action_sh\s*\(' demo_build.sh printf '\n--- variable origins ---\n' rg -n '^(OPENEMR|GIT|GITMAIN|TMPDIR|GITBRANCH|demo|packageServe|dryRun|ACTION_LOG)\b' demo_build.sh

Repository: openemr/demo_farm_openemr

Length of output: 5339

Avoid eval for shell-form actions

run_action_sh re-parses its string with eval, and the call sites feed in env-derived paths like OPENEMR, GIT, and TMPDIR. A value containing shell syntax or command substitution will execute in live mode. Pass fixed commands with positional parameters instead of building shell strings.

🧰 Tools

🪛 ast-grep (0.44.0)

[error] 75-75: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "$1"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(eval-on-variable-bash)

🤖 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 `@demo_build.sh` around lines 73 - 77, The run_action_sh helper is executing shell-form strings with eval, which can re-parse env-derived values and allow unintended command execution. Refactor run_action_sh to accept a command plus positional arguments and invoke it directly without eval, then update the call sites that pass OPENEMR, GIT, and TMPDIR-derived paths to build fixed argument lists instead of shell strings.

Source: Linters/SAST tools

… fixture Rabbit-flagged fixes: - .github/workflows/build-tests.yml: drop the stale "includes derive-ip-map.yml" comment that didn't match the actual paths filter (and didn't need to -- changes to that workflow file don't affect build behavior). - demo_build.sh: add a _emit_action_log helper that redacts `-p<password>` (mariadb root pass flag) and `rootpass=<password>` from action-log lines before append. Best-effort; documented that ACTION_LOG should not be enabled in production without further review. Phase 2 live-run work will extend this with composer auth tokens and Authorization headers when those become real values rather than dry-run stubs. - demo_build.sh: document the trust assumption on run_action_sh (wrapped strings come from ip_map_branch.txt + docker env, not user input). Eval is intentional for pipes/redirects; refactor is out of scope for this PR. - demo_build.sh: unwrap two `mkdir` calls in the packageServe block so subsequent `cd` commands work in dry-run, where the intervening rsync is skipped. Same pattern already used for $WEB/log and $OPENEMR earlier in the script. New fixture: - capsule-path-traversal: synthetic ip_map row with `capsule` column = "../etc", exercising the case statement guard PR openemr#145 added (demo_build.sh lines ~617-622). Pins the rejection so a future regression that drops the guard fails CI -- otherwise it would silently re-enable path traversal in dry code, since no production row currently sets `capsule`. - tools/build-tests/test.sh: extends harness with the `expected/fail.txt` pattern (mirrors auto-derive harness): substring check on stderr/stdout + assert non-zero exit. All 7 fixtures pass. Verified negative test: stripping the case guard from demo_build.sh fails ONLY capsule-path-traversal. Refs: openemr#146 (issue), openemr#147 (this PR) Assisted-by: Claude Code

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tools/build-tests/test.sh (1)

230-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Fail this fixture explicitly when the action log is missing.

If demo_build.sh regresses and exits 0 without creating ACTION_LOG, normalize_action_log will trip set -e and abort the entire harness, so later fixtures never run and you lose the summary. Guard the file before normalizing/diffing and return a per-fixture failure instead.

Suggested fix

+    if [[ ! -f "$action_log" ]]; then
+        echo "  FAIL: missing action log $action_log"
+        echo "  stdout (tail):"
+        tail -n 20 "$work/script.stdout" | sed 's/^/    | /'
+        echo "  stderr (tail):"
+        tail -n 20 "$work/script.stderr" | sed 's/^/    | /'
+        return 1
+    fi
+
     normalize_action_log "$work" "$action_log"

🤖 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 `@tools/build-tests/test.sh` at line 230, The fixture currently calls
normalize_action_log on action_log unconditionally, so a missing ACTION_LOG can
abort the whole harness instead of failing just this case. In
tools/build-tests/test.sh, add an explicit existence check for the action_log
before normalize_action_log and the subsequent diff, and make the current
fixture return a failure status with a clear message when it is missing. Use the
existing test harness flow around normalize_action_log and action_log so later
fixtures still run and the summary is preserved.

🤖 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 `@tools/build-tests/test.sh`:
- Around line 209-215: The failure check in the test helper uses a piped `cat
... | grep -qF` pattern, which can trip `pipefail` and cause a false negative
when the substring exists. Update the error-substring check in `test.sh` to have
`grep -qF` read the stdout/stderr files directly, and keep the surrounding
failure logging intact so the `expected_msg` branch only fails on a real miss.

---

Outside diff comments:
In `@tools/build-tests/test.sh`:
- Line 230: The fixture currently calls normalize_action_log on action_log
unconditionally, so a missing ACTION_LOG can abort the whole harness instead of
failing just this case. In tools/build-tests/test.sh, add an explicit existence
check for the action_log before normalize_action_log and the subsequent diff,
and make the current fixture return a failure status with a clear message when
it is missing. Use the existing test harness flow around normalize_action_log
and action_log so later fixtures still run and the summary is preserved.

🪄 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: 87486744-4b31-4657-9421-6157dba57b1c

📥 Commits

Reviewing files that changed from the base of the PR and between 4c9afb6 and 94cde98.

📒 Files selected for processing (12)

.github/workflows/build-tests.yml
demo_build.sh
tools/build-tests/fixtures/branch-pinned-master/expected/action-log.txt
tools/build-tests/fixtures/capsule-path-traversal/expected/fail.txt
tools/build-tests/fixtures/capsule-path-traversal/inputs.env
tools/build-tests/fixtures/capsule-path-traversal/ip_map_branch.txt
tools/build-tests/fixtures/light-reset/expected/action-log.txt
tools/build-tests/fixtures/multi-demo/expected/action-log.txt
tools/build-tests/fixtures/release-packageserve/expected/action-log.txt
tools/build-tests/fixtures/tag-pinned-with-data/expected/action-log.txt
tools/build-tests/fixtures/up-for-grabs-fork/expected/action-log.txt
tools/build-tests/test.sh

💤 Files with no reviewable changes (1)

.github/workflows/build-tests.yml

✅ Files skipped from review due to trivial changes (3)

tools/build-tests/fixtures/capsule-path-traversal/expected/fail.txt
tools/build-tests/fixtures/capsule-path-traversal/inputs.env
tools/build-tests/fixtures/release-packageserve/expected/action-log.txt

🚧 Files skipped from review as they are similar to previous changes (6)

tools/build-tests/fixtures/multi-demo/expected/action-log.txt
tools/build-tests/fixtures/tag-pinned-with-data/expected/action-log.txt
tools/build-tests/fixtures/branch-pinned-master/expected/action-log.txt
tools/build-tests/fixtures/up-for-grabs-fork/expected/action-log.txt
tools/build-tests/fixtures/light-reset/expected/action-log.txt
demo_build.sh

Two findings from rabbit's second review pass: - Fail-expected check used `cat $stdout $stderr | grep -qF`, which under `set -o pipefail` (enabled at the top of the script) can flip a real PASS into a false FAIL: grep closes stdin on the first match, cat dies on SIGPIPE, pipeline exits non-zero. Read the files directly with `grep -qF -- $msg $stdout $stderr` — no pipe, no false negative. - normalize_action_log was called unconditionally even when the script produced no action log. If a future regression makes demo_build.sh exit 0 without writing ACTION_LOG, `sed -i` on the missing file would trip `set -e` and abort the whole harness mid-run, killing later fixtures and the summary. Added an explicit `[[ -f $action_log ]]` guard that fails just the affected fixture with a clear message and lets the run finish. Refs: openemr#146 (issue), openemr#147 (this PR) Assisted-by: Claude Code

The `-p<pass>` redaction regex matched anywhere in a line, including inside `--unsafe-perm` (npm flag), turning legitimate command args into `--unsafe-p<REDACTED>` in the action log. Surfaced by an in-progress Phase 2 smoke run — first time live mode exercised the npm install branch end-to-end. Anchor the pattern to start-of-line or whitespace so it only matches the standalone mariadb `-p<password>` flag. Regen the 6 goldens that contained the false-positive form. Refs: openemr#146 (issue), openemr#147 (this PR) Assisted-by: Claude Code

Address two rabbit findings (PR openemr#148, applies to Phase 1): * Latent secret leak in live mode: _emit_action_log only redacted -p<pass> and rootpass=<pass>. The script also emits the github api token via `curl -H "Authorization: token $GITHUB_KEY_COMPOSER"` (rate-limit probe) and `composer config --auth github-oauth.github.com $KEY`. In dry-run those values are stubbed (placeholders have <> which %q-escapes to \<\>, dodging the new pattern's [A-Za-z0-9_]+ body match), but Phase 2 live runs with a real /home/openemr/ github-key mounted would otherwise commit the raw token to ACTION_LOG. Added two more sed patterns to cover both shapes. * test.sh docstring example was stale: the harness does `cp -a "$extra_dir/." "$work/"` which mirrors the work-tree layout. Fixtures need to put version.php at extra/web/openemr/version.php, not extra/openemr/version.php (which silently lands at the wrong root and never overrides $WEB/openemr/version.php). Phase 1 goldens are unchanged — the new redaction patterns don't fire against the dry-run stubs. Refs: openemr#146 (issue), openemr#147 (this PR), openemr#148 (Phase 2) Assisted-by: Claude Code

Stacked on openemr#147 (Phase 1 dry-run suite); GitHub auto-retargets to master when Phase 1 lands. tools/build-tests/integration/run-scenario.sh stands up mariadb:10.6 + openemr/openemr:flex-3.22-php-8.2 on an isolated docker network, runs demo_build.sh for real (composer install, npm install, webpack themes, openemr install, mariadb populate, apache start), then asserts: 1. "Demo install script is complete" marker fires within the 15-min timeout (i.e., the full happy path including the slow phases dry-run skips). 2. Every expected database exists in mariadb with a `globals` table (sanity check that InstallerAuto actually ran). 3. Captured action log matches the fixture's golden (expected/live-action-log.txt), after normalizing per-run- varying values: mariadb container hostname -> <MARIADB_HOST>, randomly-picked CSS theme -> <LIVE_RANDOM_THEME>. The -p<pass> / rootpass=<pass> redaction inherits from Phase 1's helper. run-all.sh wraps the 3 live scenarios. The 4 remaining Phase 1 fixtures stay dry-run-only (cost/signal rationale documented inline): - light-reset: same composer/npm work as branch-pinned-master with no incremental signal (the lightReset gate is purely control-flow, already covered by action-log diff) - multi-demo: 3x the composer/npm cost to prove loop iteration that action-log diff already proves - capsule-path-traversal: fail-expected, exits before any side effect - up-for-grabs-fork: depends on a community fork's branch state (flaky-by-design, not our regression to catch) Real demo data SQL from $REPO_ROOT/pieces/ is overlaid onto the fixture's pieces/ at run time (Phase 1 fixtures carry empty markers for the SQL since dry-run wraps the mariadb load; live needs actual content -- without it the demo-data DROP+RELOAD cycle wipes the schema InstallerAuto installed). CI workflow: build-tests-live.yml. - During this PR's development: on: pull_request + workflow_dispatch so we get per-push feedback while iterating. - Before merge: switch to on: schedule (cron '0 8 * * *', 1h after the auto-derive reconcile job) and drop pull_request. The workflow's comment block flags this explicitly so it can't be forgotten. What's deliberately deferred to follow-ups: - actions/cache for ~/.composer/cache + ~/.npm (would shave ~10 min off nightly). - Direct diff against Phase 1 dry-run goldens (needs Phase 1 goldens redesigned with abstract path tokens; non-trivial scope creep). - GitHub auth for composer (unauthenticated 60/hour limit is fine for one nightly run). Refs: openemr#146 (issue), openemr#147 (Phase 1) Assisted-by: Claude Code

…#148) * feat(build-tests): Phase 2 -- live end-to-end integration tests Stacked on #147 (Phase 1 dry-run suite); GitHub auto-retargets to master when Phase 1 lands. tools/build-tests/integration/run-scenario.sh stands up mariadb:10.6 + openemr/openemr:flex-3.22-php-8.2 on an isolated docker network, runs demo_build.sh for real (composer install, npm install, webpack themes, openemr install, mariadb populate, apache start), then asserts: 1. "Demo install script is complete" marker fires within the 15-min timeout (i.e., the full happy path including the slow phases dry-run skips). 2. Every expected database exists in mariadb with a `globals` table (sanity check that InstallerAuto actually ran). 3. Captured action log matches the fixture's golden (expected/live-action-log.txt), after normalizing per-run- varying values: mariadb container hostname -> <MARIADB_HOST>, randomly-picked CSS theme -> <LIVE_RANDOM_THEME>. The -p<pass> / rootpass=<pass> redaction inherits from Phase 1's helper. run-all.sh wraps the 3 live scenarios. The 4 remaining Phase 1 fixtures stay dry-run-only (cost/signal rationale documented inline): - light-reset: same composer/npm work as branch-pinned-master with no incremental signal (the lightReset gate is purely control-flow, already covered by action-log diff) - multi-demo: 3x the composer/npm cost to prove loop iteration that action-log diff already proves - capsule-path-traversal: fail-expected, exits before any side effect - up-for-grabs-fork: depends on a community fork's branch state (flaky-by-design, not our regression to catch) Real demo data SQL from $REPO_ROOT/pieces/ is overlaid onto the fixture's pieces/ at run time (Phase 1 fixtures carry empty markers for the SQL since dry-run wraps the mariadb load; live needs actual content -- without it the demo-data DROP+RELOAD cycle wipes the schema InstallerAuto installed). CI workflow: build-tests-live.yml. - During this PR's development: on: pull_request + workflow_dispatch so we get per-push feedback while iterating. - Before merge: switch to on: schedule (cron '0 8 * * *', 1h after the auto-derive reconcile job) and drop pull_request. The workflow's comment block flags this explicitly so it can't be forgotten. What's deliberately deferred to follow-ups: - actions/cache for ~/.composer/cache + ~/.npm (would shave ~10 min off nightly). - Direct diff against Phase 1 dry-run goldens (needs Phase 1 goldens redesigned with abstract path tokens; non-trivial scope creep). - GitHub auth for composer (unauthenticated 60/hour limit is fine for one nightly run). Refs: #146 (issue), #147 (Phase 1) Assisted-by: Claude Code * fix(build-tests): --image CLI flag wins over fixture INTEGRATION_IMAGE Rabbit-flagged: the fixture source step (`source inputs.env`) unconditionally clobbered the parsed CLI --image value with the fixture's INTEGRATION_IMAGE, so the advertised flag was effectively ignored whenever a fixture set the env var. Track whether --image came from the CLI; let the fixture value override only when the flag was not passed. Lets callers A/B-test a scenario against a different image (e.g., to bisect a flex tag regression) without editing the fixture. Refs: #146 (issue), #148 (this PR) Assisted-by: Claude Code * fix(build-tests): drop unused PHP_VERSION_ABBR source in run-scenario.sh Rabbit-flagged. The fixture inputs.env carries PHP_VERSION_ABBR because Phase 1 dry-run needs it (the script writes to /etc/php\${ABBR}/conf.d/ which has no flex image to provide a value). Phase 2 live runs inside the openemr flex container which bakes in the right value, and run-scenario.sh never passes it via --env -- so the default-and-source dance here was a contract the runner didn't actually have. Comment block explains why the var is kept in fixtures despite not being read here. Refs: #146, #148 Assisted-by: Claude Code * ci(build-tests): parallelize live integration via matrix strategy Wall time drops from ~25 min (sequential) to ~8-10 min (slowest of 3 cells). Each scenario runs on its own ubuntu-24.04 runner, so: - Composer/npm/webpack/InstallerAuto across the three scenarios run truly in parallel rather than serialized. - Each cell gets a fresh VM, so no leftover state (docker layer cache, mariadb data dir, /tmp clutter) from scenario N can affect scenario N+1. - One failing cell does NOT cancel its siblings (fail-fast: false). Debugging is easier when you can see every result. Cost: ~5 extra GitHub Actions minutes per run (3 cells x 10 min vs 1 runner x 25 min). Worth it during PR development where wall time matters; still fine post-merge when this becomes nightly. run-all.sh stays as the local-execution wrapper -- now annotated with a "MUST stay in sync with workflow matrix" comment so anyone adding/removing a scenario updates both places. Refs: #146 (issue), #148 (this PR) Assisted-by: Claude Code * ci(build-tests): add daily 08:00 UTC schedule alongside pull_request The matrix conversion dropped wall time to ~10 min (slowest cell), which is now tractable as blocking per-PR CI. Per-PR catches the "PR-introduced regression" class; schedule catches the "upstream image bump on master" class that no PR triggers. Both paths are cheap on a low-volume repo, and they cover different failure modes. 08:00 UTC is 1h after derive-ip-map's daily reconcile (07:00) so the two scheduled flows don't race on the same window. Drops the pre-merge "TODO swap pull_request for schedule" note that was premised on a 25-min sequential wall time -- that constraint is gone, so per-PR can stay. Refs: #146 (issue), #148 (this PR) Assisted-by: Claude Code

…s) (#149) PR A of the shellcheck ratchet cleanup, following the analysis in issue #146. The build-tests suites landed in #147/#148 are the regression net that makes this safe to do in one PR. Disables dropped (9): - SC3014, SC3028, SC3030, SC3037, SC3043, SC3050, SC3054 (7 POSIX-sh warnings) -- all caused by demo_build.sh carrying a stale `#!/bin/sh` shebang while using bash features (arrays, `[[`, `==`, `RANDOM`, `local`, `printf %q`) throughout. Production already invokes via `bash -c demo_build.sh` from docker/scripts/demoLibrary.source, so the runtime call shape doesn't change. - SC1001 (1 violation, demo_build.sh:205): `\=` is a regular `=` in this context (the backslash in `cut -d \= -f 2` was a no-op). - SC2046 (1 violation, demo_build.sh:205): unquoted `$()` in `collect_var` (also covered SC2005 -- useless `echo $(cmd)`). Refactored to drop the unnecessary `echo` wrapper; the inner pipeline already produces the value the caller captures via `$(collect_var ...)`. - SC2048 (1 violation, demo_build.sh:314): `${demosGo[*]}` → `"${demosGo[@]}"` for the for-loop. Same behavior for entries without spaces (all current entries are "empty"/"a"/.../"i"); canonical bash array iteration. - SC2005 (1 violation, demo_build.sh:205): covered above. - SC2016 (2 violations): literal `$v_major` (test.sh:120) and literal `$1` (derive.sh:1330) inside single-quoted printf formats -- intentional, the `$` characters are emitted into output. Inline `# shellcheck disable=SC2016` per site. - SC2001 (3 violations, in 2 files): `echo "$var" | sed 's/^/...'` indent-prefix idiom. Inline `disable=SC2001` per site rather than rewriting to awk -- there are 14 other `sed 's/^/...'` sites in the same files reading from cmd/file output (not flagged), and rewriting only the 3 flagged sites would split the codebase between two idioms. Matching comments at each site explain the choice. - SC2129 (0 violations): rc comment overcounted; no actual flags. - SC2196 (0 violations): same -- the 2 `egrep` calls flagged earlier were inside demo_build.sh strings passed to mariadb-dump | egrep, not on bare lines shellcheck inspects. Disables retained for follow-up PRs (per the issue #146 ratchet plan): SC1090, SC2006, SC2034, SC2086, SC2155, SC2164. 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 regression suite catches behavioral surprises from the cleanup). - `./tools/auto-derive/fixtures-and-tests/test.sh` -- 8/8 PASS. Refs: #146 (parent issue closed but discussion is the plan source), #139 (workflow), #145 (prior SC2115 ratchet step). Assisted-by: Claude Code

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

* ci(shellcheck): drop SC2164 disable + guard all 25 cd sites 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 (#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: #146 (parent issue), #149 (prior shellcheck cleanup PR A) Assisted-by: Claude Code * fix(scripts): preserve optional-repo semantic on wkhtmltopdf-openemr 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 (#147/#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 #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: #146 (issue), #150 (this PR) Assisted-by: Claude Code

The README hadn't been touched to reflect several years of demo_farm work. Rewritten to match current reality: Workflow badges added at top: - ShellCheck (post-cleanup: rc has zero disable= lines) - build-tests (dry-run) -- per-PR fixture suite from #147 - build-tests (live integration) -- nightly + per-PR matrix from #148 - Derive ip_map (reconcile) -- auto-derive bot from #135 ip_map_branch.txt column docs: - Converted from bullet list to table for readability. - Marked `branch_tag` (col 13) as deprecated -- matches the header annotation, the demo_build.sh comment ("col 13 (branch_tag) is deprecated; ... `git clone --branch` works for both"), and the fact that every production row now carries `0` there. - `fun_stuff`: documented actual code behavior. `1` = 36-theme picker, `2` = 6-theme picker, any other non-zero value falls through to the 36-theme picker via the `else` branch. Previously the README said "1 for <6.0, 2 for 6.0+" which reads like a version gate but is really a picker-selection. - `pass_reset`: documented that set_pass.php actually supports modes 1-4 (3/4 are 6.0.0+ variants), production uses 4, AND the mechanism is currently disabled pending openemr/openemr#5991. Column values are kept aligned with intended re-enable. - `capsule`: added the path-traversal guard mention (added in #145) as a footnote-style note. New sections: - "Regression tests" -- describes the dry-run + live-integration suites from #147/#148 and the auto-derive fixtures. - "Auto-derive bot" -- describes the daily reconcile flow from #135 including the release-targets-changed cross-repo dispatch. - "ShellCheck" -- notes the ratchet cleanup completion (PRs #149 through #157) and the current-clean state. Kept intact: - "UP FOR GRABS" onboarding steps (5-step process; still current). - "How to restart the demo farm after a host reboot" section (order + scripts still current). Trimmed the "watch this YouTube video 15 times" line since it dated the page. Intro paragraph rewritten to name the actual URL structure (demo.openemr.io etc.) and describe what a cluster/subdemo is, rather than the previous single-line description. License section preserved. Assisted-by: Claude Code

bradymiller force-pushed the feat/issue-146-phase1-dry-run branch from 61b3b07 to 4c9afb6 Compare June 28, 2026 22:26

coderabbitai Bot reviewed Jun 28, 2026

View reviewed changes

Comment thread tools/build-tests/test.sh Outdated

bradymiller added 2 commits June 28, 2026 23:07

bradymiller mentioned this pull request Jun 29, 2026

feat(build-tests): Phase 2 — live end-to-end integration tests (#146) #148

Merged

5 tasks

bradymiller merged commit 91477d1 into openemr:master Jun 29, 2026
6 checks passed

This was referenced Jun 29, 2026

Add regression testing for demo_build.sh + orchestration scripts #146

Closed

ci(shellcheck): drop 9 disables (shebang flip + trivial-category fixes) #149

Merged

bradymiller mentioned this pull request Jun 29, 2026

ci(shellcheck): drop SC2164 disable + guard all 25 cd sites #150

Merged

This was referenced Jun 30, 2026

bug(demo_build): grep "$IPADDRESS" substring-matches sibling rows (two/two_a, five/five_a, etc.) #154

Closed

docs(readme): modernize + add workflow status badges #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat(build-tests): add Phase 1 dry-run regression suite for demo_build.sh (#146)#147

feat(build-tests): add Phase 1 dry-run regression suite for demo_build.sh (#146)#147
bradymiller merged 5 commits into
openemr:masterfrom
bradymiller:feat/issue-146-phase1-dry-run

bradymiller commented Jun 28, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jun 28, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Jun 28, 2026

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

bradymiller commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Scenarios

CI

What's NOT in this PR

Test plan

Uh oh!

coderabbitai Bot commented Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot Jun 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bradymiller commented Jun 28, 2026 •

edited

Loading

coderabbitai Bot commented Jun 28, 2026 •

edited

Loading