Skip to content

feat(openemr-cmd): wrap pre-commit tooling, add prek-in-container dispatch#718

Open
bradymiller wants to merge 9 commits into
openemr:masterfrom
bradymiller:devops-wt-sync-dev-tools-openemrcmd
Open

feat(openemr-cmd): wrap pre-commit tooling, add prek-in-container dispatch#718
bradymiller wants to merge 9 commits into
openemr:masterfrom
bradymiller:devops-wt-sync-dev-tools-openemrcmd

Conversation

@bradymiller
Copy link
Copy Markdown
Member

Summary

Adds in-container access to every pre-commit hook from OpenEMR's .pre-commit-config.yaml so developers can git commit from the host without installing PHP, Node, Python, codespell, pre-commit, or actionlint locally. The openemr flex container becomes the execution environment; openemr-cmd is the bridge.

  • openemr-cmd wrappers for each pre-commit composer script: cps (codespell), ccc (conventional-commits-check), crc (require-checker), cv (composer-validate), cn (composer-normalize), cnf (composer-normalize-fix), cck (composer-checks), cq (code-quality). First five wired into clean-sweep so the existing aggregate stays comprehensive.
  • openemr-cmd prek <args>: passthrough to in-container pre-commit. Substitutes actionlint-dockeractionlint-system at invocation time (DinD isn't available from inside the container); the repo's .pre-commit-config.yaml is untouched and host-side prek/pre-commit usage continues to work unchanged. cwd-aware container resolution (wt_resolve_container_from_cwd) so a commit fired from worktree foo always dispatches to foo's container.
  • openemr-cmd pi / pu (prek-install / prek-uninstall): manage host-side .git/hooks/{pre-commit,commit-msg} shims. One install on the primary repo covers every linked worktree (current and future) via gitcommondir. Refuses to clobber non-managed hooks without --force. Runtime safeguard rejects invocation via -d <container> or worktree exec with a clear error.
  • Container packages relocated: chromium, chromium-chromedriver, py3-codespell, plus the new pre-commit and actionlint, now installed at top of openemr.sh under DEVELOPER_TOOLS=yes gate (idempotent via apk info -e guards). Previously inside NEED_COMPOSER_BUILD=true, which meant they didn't survive docker compose down --keep-volumes && up (vendor preserved → block skipped → packages missing on the recreated container).
  • Bumps openemr-cmd VERSION to 1.0.31. Fixes a stale openemr-cmd-h filter line count (was 9, now 23).

End-to-end workflow this unlocks

For a contributor with only Docker installed on their host:

openemr-cmd worktree add my-feature -b              # one-time per feature
openemr-cmd pi                                       # one-time per clone
# edit files
git add path/to/file.php
git commit -m "feat: ..."
# → host hook fires → openemr-cmd prek run --hook-stage pre-commit
# → cwd-aware resolution dispatches to my-feature's container
# → pre-commit runs inside container against staged files
# → commit aborts on any failure, succeeds otherwise

Manual invocation paths remain:

  • openemr-cmd prek run --all-files — full-codebase check
  • openemr-cmd cq — full composer code-quality suite (direct composer path, bypasses prek)
  • openemr-cmd worktree exec <branch> prek run --all-files — target a specific worktree from anywhere

Test plan

openemr-cmd smoke checks

  • openemr-cmd --version reports 1.0.31
  • openemr-cmd --help shows the pre-commit-management: section with prek, pi/prek-install, pu/prek-uninstall entries, plus the eight new short-form wrappers under php-management
  • openemr-cmd-h php displays all 23 php-management entries (was truncated before due to stale filter count)

Per-tool wrappers (run inside any DEVELOPER_TOOLS=yes container; openemr-cmd from host)

  • openemr-cmd cps → runs codespell, no crash on warnings
  • openemr-cmd cv → reports composer.json validation
  • openemr-cmd cn → dry-run normalization, reports diff or "already normalized"
  • openemr-cmd crc → composer-require-checker output
  • openemr-cmd cq → runs the full composer code-quality suite

prek dispatcher

  • openemr-cmd prek --version reports the in-container pre-commit version
  • openemr-cmd prek run --all-files executes the full hook suite; actionlint runs without DinD
  • openemr-cmd prek run phpstan runs the single hook
  • Inside the openemr container, /tmp/openemr-cmd-pre-commit-config.yaml shows actionlint-system (not -docker)
  • From inside worktree foo, openemr-cmd prek run dispatches to foo's container (verify via docker ps while it runs)

Hook installation

  • openemr-cmd pi from the primary repo writes pre-commit and commit-msg shims to <primary>/.git/hooks/
  • Both shim files contain the __openemr_cmd_prek_hook__ marker
  • git commit from a worktree fires the shim and dispatches into that worktree's container
  • git commit from the primary fires the shim and dispatches into the primary's container
  • openemr-cmd pi refuses to overwrite a non-managed pre-existing hook; pi --force backs it up to *.bak.<timestamp> and installs
  • openemr-cmd pu removes only marker-bearing hooks; skips non-managed ones with a notice

Runtime safeguards

  • openemr-cmd -d <any-container> pi exits 1 with "host-only command" error (does not enter container)
  • openemr-cmd worktree exec <branch> pi produces the same error transitively
  • Same for pu/prek-uninstall

Container package persistence (the NEED_COMPOSER_BUILD fix)

  • After openemr-cmd worktree down --keep-volumes <branch> && openemr-cmd worktree up <branch>, all of chromium-browser, codespell, pre-commit, and actionlint resolve to installed binaries inside the new container
  • On subsequent container starts with packages already present, no apk install runs (verify the apk info -e short-circuit)

Compatibility

  • Host-based prek install followed by git commit still works for users who haven't run openemr-cmd pi (no change to host workflow)
  • CI continues to pass; in particular composer code-quality and shellcheck don't regress

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings May 13, 2026 16:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends openemr-cmd to act as a host-side bridge into the Flex container for pre-commit tooling, enabling contributors to run the repository’s pre-commit hooks (and related composer checks) without installing the underlying runtimes locally. It also adjusts the Flex container startup so required dev-only packages persist across container recreation scenarios.

Changes:

  • Add openemr-cmd prek dispatcher (in-container pre-commit) plus host-only prek-install/prek-uninstall hook shims; add short wrapper commands for several composer-based checks.
  • Extend the Flex devtools entrypoint to expose new composer commands and include key ones in clean-sweep.
  • Move installation of dev-only Alpine packages (chromium, codespell, pre-commit, actionlint) earlier in openemr.sh under the DEVELOPER_TOOLS=yes gate; update openemr-cmd-h filter count and bump openemr-cmd version.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
utilities/openemr-cmd/openemr-cmd-h Updates php-management help filter line count to reflect additional commands.
utilities/openemr-cmd/openemr-cmd Adds worktree-cwd-aware container resolution, pre-commit dispatch (prek), host hook install/uninstall commands, new PHP/composer wrappers, and bumps version.
docker/openemr/flex/utilities/devtools Adds devtools commands for composer-based pre-commit checks and wires key ones into clean-sweep.
docker/openemr/flex/openemr.sh Installs dev-only Alpine packages early (idempotent) to avoid missing tooling after container recreation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread utilities/openemr-cmd/openemr-cmd Outdated
cfg=/tmp/openemr-cmd-pre-commit-config.yaml
if [ -f .pre-commit-config.yaml ]; then
sed "s/actionlint-docker/actionlint-system/g" .pre-commit-config.yaml > "${cfg}"
exec pre-commit "$@" --config "${cfg}"
Comment thread utilities/openemr-cmd/openemr-cmd Outdated
Comment on lines +1907 to +1910
cfg=/tmp/openemr-cmd-pre-commit-config.yaml
if [ -f .pre-commit-config.yaml ]; then
sed "s/actionlint-docker/actionlint-system/g" .pre-commit-config.yaml > "${cfg}"
exec pre-commit "$@" --config "${cfg}"
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

Comment thread utilities/openemr-cmd/openemr-cmd Outdated
Comment on lines +189 to +192
docker ps \
--filter "label=com.docker.compose.project=openemr-${slug}" \
--filter "label=com.docker.compose.service=openemr" \
--format "{{.ID}}" | head -n 1
Comment on lines +894 to +906
#!/bin/sh
# openemr-cmd-managed commit-msg hook -- do not edit by hand
# ${PREK_HOOK_MARKER}
exec openemr-cmd prek run --hook-stage commit-msg --commit-msg-filename "\$1"
HOOKEOF
;;
*)
cat > "${target}" <<HOOKEOF
#!/bin/sh
# openemr-cmd-managed pre-commit hook -- do not edit by hand
# ${PREK_HOOK_MARKER}
exec openemr-cmd prek run --hook-stage pre-commit
HOOKEOF
Comment thread utilities/openemr-cmd/openemr-cmd Outdated
return 1
fi
local backup
backup="${target}.bak.$(date +%s)"
bradymiller and others added 3 commits May 14, 2026 23:00
…patch

Adds in-container access to every pre-commit hook from .pre-commit-config.yaml
so developers can `git commit` from the host without installing PHP, Node,
Python, codespell, pre-commit, or actionlint locally. The openemr flex
container becomes the execution environment; openemr-cmd is the bridge.

devtools subcommands and openemr-cmd wrappers for each pre-commit composer
script: codespell (cps), conventional-commits-check (ccc), require-checker
(crc), composer-validate (cv), composer-normalize (cn), composer-normalize-fix
(cnf), composer-checks (cck), code-quality (cq). First five are also wired
into clean-sweep so the existing aggregate stays comprehensive.

prek dispatcher (`openemr-cmd prek <args>`) is a passthrough to in-container
pre-commit. Substitutes actionlint-docker -> actionlint-system at invocation
time so the actionlint hook works without DinD; the repo's
.pre-commit-config.yaml is untouched and host-side prek/pre-commit usage is
unaffected. Resolves the target container via cwd
(wt_resolve_container_from_cwd) so a commit fired from worktree foo
dispatches to foo's container, not whichever worktree container happens to
be running.

prek-install (pi) and prek-uninstall (pu) manage host-side
.git/hooks/{pre-commit,commit-msg} shims. Linked worktrees share gitcommondir,
so a single install on the primary covers every current and future worktree.
Both refuse to clobber non-managed hooks without --force and reject
invocation via '-d <container>' or 'worktree exec' with a clear error.

Container packages (actionlint, pre-commit, py3-codespell, plus the existing
chromium/chromium-chromedriver) now installed at top of openemr.sh under the
DEVELOPER_TOOLS=yes gate. Previously these lived inside the NEED_COMPOSER_BUILD
branch, so they did not survive `docker compose down --keep-volumes && up`
(vendor preserved -> block skipped -> packages missing). Moved out and
guarded with `apk info -e` so they re-install on container recreation but
are no-ops on subsequent starts.

Bumps openemr-cmd VERSION to 1.0.31. Updates openemr-cmd-h's php-management
filter line count from a stale 9 to 23 so all entries display.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…atch

* SC2312 (line 185): process substitution masked jq's return value;
  add explicit '|| true' so the helper's empty-state handling is
  unambiguous to readers and to shellcheck.

* SC2155 (line 885): split 'local backup="..."' so the assignment's
  exit code isn't masked by the 'local' builtin.

* SC2310 (four sites in cmd_prek_install / cmd_prek_uninstall):
  drop 'func || exit 1' and 'if ! func' patterns (both of which
  disable set -e inside the called function) in favor of bare
  assignments that propagate the function's exit code through the
  script's set -euo pipefail.

* --config placement (Copilot): pre-commit parses '--config' at the
  top-level parser. Trailing placement (after the subcommand) gave
  "unrecognized arguments". Move to precede "$@".

* /tmp config race (Copilot): parallel openemr-cmd prek invocations
  in the same container could clobber each other's substituted
  config mid-run. Use mktemp for a unique path per invocation and
  trap-clean up on EXIT/INT/TERM.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docker ps in wt_resolve_container_from_cwd swallows daemon-unreachable
  errors (2>/dev/null) and forces a zero exit (|| true) so the caller can
  see an empty result rather than having 'set -o pipefail' kill the script
  before the dispatcher's own "no running openemr container" handling can
  run.

* Hook shims bake in the absolute path to the openemr-cmd binary that
  installed them ($0 resolved via realpath, with command -v fallback).
  Git frequently invokes hooks with a reduced PATH (especially from GUI
  clients), so 'exec openemr-cmd ...' could fail when openemr-cmd lives
  in a user-local bindir like ~/bin or ~/.local/bin.

* Backup filename for clobbered hooks uses mktemp instead of date +%s,
  so rapid 'prek-install --force' re-runs within the same second don't
  overwrite each other's backups. mktemp is portable across macOS BSD
  and Linux GNU coreutils (date +%s%N is GNU-only).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bradymiller bradymiller force-pushed the devops-wt-sync-dev-tools-openemrcmd branch from 480537a to 0904563 Compare May 15, 2026 06:01
Comment thread docker/openemr/flex/utilities/devtools Outdated
composer phpstan-baseline
fi

if [ "$1" = "codespell" ] || [ "$1" = "clean-sweep" ]; then
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest case/esac for this section, instead of if. There also really ought to be a way to avoid duplicating that cd. A shell function perhaps?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done on both fronts (ending up converting entire 'CODE QUALITY & STYLING' section to case/esac)

Comment thread utilities/openemr-cmd/openemr-cmd Outdated

while IFS=$'\t' read -r b d; do
[[ -z "${b}" ]] && continue
local d_real
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can hoist this declaration

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread utilities/openemr-cmd/openemr-cmd Outdated
[[ -z "${b}" ]] && continue
local d_real
d_real=$(realpath -m "${d}" 2>/dev/null) || continue
if [[ "${d_real}" == "${toplevel_real}" ]]; then
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend single equals. Just a style thing here, but double equals has subtle breakage when dealing with some shells. (Only time you need == in bash is in arithmetic context.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

bradymiller and others added 2 commits May 16, 2026 04:52
…ygiene

* devtools: factor out 'run_composer_at_root' helper so each new code-quality
  handler is one line instead of three. Removes nine duplicated
  'cd /var/www/localhost/htdocs/openemr' lines. The helper guards with
  '|| exit 1' so a missing webroot aborts the script rather than silently
  running composer from a wrong cwd ('devtools' has no 'set -e').

* openemr-cmd / wt_resolve_container_from_cwd: hoist 'local d_real' to the
  function's top-of-body declaration alongside the other locals (was
  redeclared on every loop iteration) and switch '==' to '=' for string
  equality inside '[[ ]]' (POSIX-standard spelling; '==' is reserved for
  arithmetic context in shell style guides).

Behavioral parity: no functional change; output of every affected command
is identical. Shellcheck count on 'devtools' drops from 53 to 49.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address kojiromike's suggestion for the full section, not just the new
handlers. Converts 13 pre-existing cascading-if blocks plus the 8 new
ones added by this PR into a uniform case/esac dispatch:

  * One do_<task>() helper per handler -- the "what does this do"
    half of dispatch is now a flat, named, greppable definition rather
    than embedded in if-test bodies. Multi-line layout (one statement
    per line in each function body) matches the prevailing style guide.
  * Two helpers at the top of the section -- run_at_openemr_root (cd +
    "$@") and run_composer_at_root (cd + composer "$@"). Replaces 13
    duplicated 'cd /var/www/localhost/htdocs/openemr' lines in the
    section with a centralized cd that guards with '|| exit 1'.
  * Single case "$1" in ... esac with 22 branches: 20 single-task
    branches (including the psr2-* / psr12-* backward-compat aliases),
    plus clean-sweep and clean-sweep-tests that enumerate exactly which
    do_* helpers compose them. The '*) ;;' default lets unmatched input
    fall through to the TESTING COMMANDS section below, where its own
    if-cascade still matches clean-sweep / clean-sweep-tests for the
    test suites (preserving the existing whole-file composition).

Behavior preserved bit-for-bit: same handlers fire on each input, same
order, same echo strings (including trailing whitespace in a few that
had it), same find/php-l redirect dance in do_php_parserror (wrapped in
sh -c to preserve the '2>&1 >&-' pattern through a subshell). The
pre-existing '}' typo in '.\/ccdaservice\/node_modules}\/*' is kept
verbatim -- separate fix if intended.

Quantitative effect: section drops ~37 lines; shellcheck findings on
the file drop from 49 to 37 (driven by collapsing 13 cd lines into
two helpers). Zero shellcheck findings on lines touched by this PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comment thread utilities/openemr-cmd/openemr-cmd Outdated
# mktemp for the substituted config: a fixed /tmp path would
# race if two prek invocations run concurrently in the same
# container (parallel CI steps, manual run alongside a hook).
cfg=$(mktemp -t openemr-cmd-pre-commit-config.XXXXXX.yaml) \
Comment on lines +204 to +207
do_php_parserror() {
echo "Generating PHP parse errors"
cd /var/www/localhost/htdocs/openemr
find . -type f \( -name "*.php" -or -name "*.inc" \) \( -not -path "./vendor/*" -and -not -path "./node_modules/*" -and -not -path "./ccdaservice/node_modules}/*" \) -exec php -d error_reporting=32767 -l {} \; 2>&1 >&- | grep "^"
fi
run_at_openemr_root sh -c 'find . -type f \( -name "*.php" -or -name "*.inc" \) \( -not -path "./vendor/*" -and -not -path "./node_modules/*" -and -not -path "./ccdaservice/node_modules}/*" \) -exec php -d error_reporting=32767 -l {} \; 2>&1 >&- | grep "^"'
}
bradymiller and others added 4 commits May 17, 2026 03:23
* mktemp template: 'mktemp -t openemr-cmd-pre-commit-config.XXXXXX.yaml'
  in the in-container prek dispatcher fails under Alpine's BusyBox mktemp,
  which requires the X-group at the *end* of the template (no trailing
  extension). Switch to full-path form 'mktemp /tmp/...XXXXXX'. No
  '.yaml' suffix needed -- pre-commit accepts any filename via --config.
  This form is also portable across GNU coreutils, BSD mktemp, and
  BusyBox.

* do_php_parserror find exclusion: pre-existing typo
  './ccdaservice/node_modules}/*' had a stray '}', so the -not -path
  exclusion silently never matched and php -l was run on every .php file
  under ccdaservice/node_modules. Corrected to the actual directory name.
  Significantly faster php-parserror and clean-sweep runs; less noise.
  Flagged by Copilot on a line touched by the case/esac migration.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tures

Two composer scripts that were previously unreachable through openemr-cmd
get dedicated wrappers, completing the in-container coverage for the
host-zero-deps validation workflow:

* phpunit-isolated (alias 'pit') -- composer phpunit-isolated. Runs the
  PHP isolated test suite, which needs neither a database nor an
  external Docker service. Lets contributors validate isolated tests
  inside the openemr container without a host PHP toolchain.

* update-twig-fixtures (alias 'utf') -- composer update-twig-fixtures.
  Regenerates the expected-output fixture files used by the Twig
  render tests under tests/Tests/Isolated/Common/Twig/fixtures/render/.
  Mutating maintenance command (not a test): the help text and the
  in-container 'echo' both flag that fixture files are overwritten
  and the diff should be reviewed before committing.

Both wired into the existing TESTING section in devtools (if-cascade
style to match its neighbors; clean-sweep / clean-sweep-tests do not
include them since isolated tests run independently and twig-fixture
regeneration is mutating). openemr-cmd dispatches via existing
run_devtools_in_docker. openemr-cmd-h's test-management filter line
count bumped 8 -> 12 so all twelve entries display.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror the new 'phpstanvolume' named volume added to the base
docker-compose.yml files in openemr (3 environments: easy, easy-light,
easy-redis) so that each worktree's PHPStan cache lives in its own
docker-managed volume ('openemr-<slug>_phpstan') rather than colliding
across worktrees or spilling onto the bind-mounted host filesystem.

The mount itself (phpstanvolume -> tmp-phpstan in the openemr workdir)
is declared in the base compose; the override file just gives that
named volume a worktree-scoped 'name:' field, same pattern used for
the other per-worktree volumes (db, vendor, nodemodules, etc.).

Requires the openemr-side compose change to be in the worktree's
checkout. Bump VERSION to 1.0.32.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…punit-isolated to clean-sweep

Migrates the 12 cascading-if blocks in the TESTING COMMANDS section
into per-task functions and case-branches inside the existing dispatch
block, finishing the migration kojiromike initially suggested for the
code-quality section. The combined block now covers code quality,
styling, and testing under one dispatcher; the standalone TESTING
COMMANDS section is removed.

* New do_* helpers: do_unit_test, do_javascript_unit_test,
  do_jut_reports_build, do_api_test, do_e2e_test, do_fixtures_test,
  do_services_test, do_validators_test, do_controllers_test,
  do_common_test, do_phpunit_isolated, do_update_twig_fixtures.
  (The last two were previously standalone if-blocks added during the
  phpunit-isolated / update-twig-fixtures wrappers commit; now folded
  into the unified scheme.)

* clean-sweep gains do_phpunit_isolated at the end of the test block
  (12 -> 22 task list). Previously the isolated suite was excluded
  even though it's part of the project's full validation set;
  including it makes clean-sweep cover everything CI runs plus the
  isolated tests that don't require a database.

* clean-sweep-tests gains the same do_phpunit_isolated entry (11 -> 12
  task list).

* jut-reports-build and update-twig-fixtures remain explicit-only.
  jut-reports-build is a coverage-report build step, not a check;
  update-twig-fixtures is a mutating maintenance command that rewrites
  checked-in files and should never run as part of an aggregate.

* Section header renamed:
    'CODE QUALITY & STYLING COMMANDS'
    -> 'CODE QUALITY, STYLING & TESTING COMMANDS'

Mechanical side-effect: 12 more 'cd /var/www/localhost/htdocs/openemr'
lines collapse into the existing helpers, dropping shellcheck findings
on devtools from 37 to 27 (-10 SC2164 'cd should have || exit').

No new shellcheck findings on the changed lines; zero functional change
to any individual command's behavior (echo strings, env exports, and
phpunit/npm invocations preserved verbatim). The only behavior change
is the intentional addition of phpunit-isolated to the two aggregates.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants