Skip to content

fix(auto-derive): printf format starting with '-' breaks bash builtin#142

Merged
bradymiller merged 1 commit into
openemr:masterfrom
bradymiller:fix-printf-dash-option
Jun 28, 2026
Merged

fix(auto-derive): printf format starting with '-' breaks bash builtin#142
bradymiller merged 1 commit into
openemr:masterfrom
bradymiller:fix-printf-dash-option

Conversation

@bradymiller

@bradymiller bradymiller commented Jun 28, 2026

Copy link
Copy Markdown
Member

Summary

Scheduled reconciliation broke today on its first non-empty-diff run with:

```
/home/runner/work/_temp/.../sh: line 21: printf: - : invalid option
printf: usage: printf [-v var] format [arguments]
##[error]Process completed with exit code 2.
```

Reproducer: https://github.com/openemr/demo_farm_openemr/actions/runs/28315987703

Root cause

The reconcile job's `Build PR body` step has:

```bash
printf '- Force-push fresh reconciliation tomorrow ...\n'
printf '- Close this PR automatically ...\n\n'
```

Bash's `printf` builtin parses a leading `-` in the format string as an option flag attempt (`-F`, `-o`, etc.), and aborts with "invalid option" when the option isn't recognized. The string content is treated as the format, not as the value of a `%s`.

Why it slipped past PR #138 review

Prior scheduled runs hit the no-diff early-exit path (workflow closes any open reconciliation PR and returns) before reaching the body-building step. Today's first run that found a real diff (the rel-704/rel-800/rel-810 col-3 deltas + 8.1.1 description bump) exercised the broken code path.

Fix

Pass the bullet content as a `%s` value instead of as the format string:

```bash
printf '%s\n' '- Force-push fresh reconciliation tomorrow ...'
printf '%s\n\n' '- Close this PR automatically ...'
```

Added an inline comment explaining the trap so future edits don't reintroduce it.

Audit

Grepped all `printf` calls in the workflow — only these 2 lines have format strings starting with `-`. Other printfs start with letters, backticks, or `%`, so no others triggered.

Test plan

  • CI passes
  • Next scheduled run (or manual `workflow_dispatch` mode=reconcile) opens the reconciliation PR cleanly instead of failing in Build PR body

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved formatting for the PR body’s Behavior section so bullet points render consistently.
    • Fixed an issue where leading dashes could be interpreted incorrectly and removed an extra trailing newline in the generated text.

The reconcile job's "Build PR body" step ran `printf '- Force-push ...'`
which bash's printf builtin parses as: option '- ' (invalid). Job
exits with "printf: - : invalid option" before the PR body finishes
rendering, breaking the entire scheduled reconciliation.

Triggered by today's first scheduled run that actually hit a non-empty
diff path (prior runs hit no-diff early-exit and never reached this
step). Fix is to pass the bullet text as %s value instead of as the
format string, so the leading '-' isn't parse-tested as an option.

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

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2b9215bf-6a45-4998-a879-845128607382

📥 Commits

Reviewing files that changed from the base of the PR and between 83fd965 and 4148d4f.

📒 Files selected for processing (1)
  • .github/workflows/derive-ip-map.yml

📝 Walkthrough

Walkthrough

In .github/workflows/derive-ip-map.yml, two printf calls that render "Behavior" bullet points in the auto-generated PR body are updated from format-string-embedded newlines to printf '%s\n' argument-based form, preventing the leading - from being interpreted as a format option.

derive-ip-map workflow: PR body printf fix

Layer / File(s) Summary
Fix printf format for PR body bullets
.github/workflows/derive-ip-map.yml
Replaces printf "- ...\n" with printf '%s\n' "- ..." for the two Behavior bullet lines, and adjusts trailing newline handling on the final bullet with an explanatory comment.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

Poem

A dash at the start gave old printf a fright,
It thought it a flag in the dead of the night!
But '%s\n' said the rabbit, "just pass it as data!"
No more mangled bullets — we'll format them straighter.
🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the bash printf leading-dash bug fixed in the workflow.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@bradymiller
bradymiller merged commit d75e424 into openemr:master Jun 28, 2026
4 checks passed
bradymiller added a commit to bradymiller/demo_farm_openemr that referenced this pull request Jun 28, 2026
…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
bradymiller added a commit that referenced this pull request Jun 29, 2026
…d.sh (#146) (#147)

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

Phase 1 of issue #146: per-PR regression coverage that catches the
PR #142 (printf '-' dash) and PR #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: #146

Assisted-by: Claude Code

* feat(build-tests): address rabbit review + add capsule-path-traversal 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 #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: #146 (issue), #147 (this PR)

Assisted-by: Claude Code

* fix(build-tests): rabbit-review-2 fixes for test.sh harness

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: #146 (issue), #147 (this PR)

Assisted-by: Claude Code

* fix(build-tests): redaction false-positive on --unsafe-perm

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: #146 (issue), #147 (this PR)

Assisted-by: Claude Code

* fix(build-tests): extend redaction + fix fixture-extras path docstring

Address two rabbit findings (PR #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: #146 (issue), #147 (this PR), #148 (Phase 2)

Assisted-by: Claude Code
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.

1 participant