Skip to content

chore(ci): make lint.yml drop-into-existing-repo friendly#13

Merged
topcoder1 merged 3 commits into
mainfrom
chore/lint-shellcheck-and-deps
May 1, 2026
Merged

chore(ci): make lint.yml drop-into-existing-repo friendly#13
topcoder1 merged 3 commits into
mainfrom
chore/lint-shellcheck-and-deps

Conversation

@topcoder1
Copy link
Copy Markdown
Owner

Summary

Two fixes for the `install-lint.sh` fanout caught by the first real-world target (whois-api-llc/wxa-jake-ai#181).

Problems

  1. actionlint default surfaced N pre-existing shellcheck warnings. The shellcheck integration in actionlint defaults to severity=info, which means dropping the workflow into a repo with N existing `run:` blocks fires N info-level warnings (SC2086 quote-variables, SC2034 unused-variable, etc.) and turns the install PR red. Forcing every new repo to clean up shellcheck noise before shipping a single PR is hostile.

  2. prettier --check failed in repos with custom prettier configs that reference plugins (wxa-jake-ai uses `prettier-plugin-svelte`). The lint job ran prettier without installing the target's deps, so the plugin couldn't resolve.

Fixes

Change Default Override
Disable actionlint's shellcheck integration OFF `run_shellcheck: true`
Run `npm ci` (or `npm install`) before prettier when package.json present ON `install_node_deps: false`

Real GHA bugs (typos in `uses:`, undefined steps refs, deprecated runners) are caught by actionlint's native checks regardless. Shellcheck-on-bash is a separate concern best opted into per repo once the codebase is normalized.

After this lands

wxa-jake-ai#181 and wxa_vpn#180 will pass actionlint on the next CI run. Pre-existing markdown drift (separate concern) still needs a `prettier --write` pass per repo or `--no-prettier` install option.

🤖 Generated with Claude Code

Two fixes for the install-lint.sh fanout caught by the first real-world
target (whois-api-llc/wxa-jake-ai PR #181):

1. actionlint default: shellcheck OFF.
   actionlint's shellcheck integration defaults to severity=info, which
   means dropping the workflow into a repo with N existing `run:` blocks
   surfaces N info-level style warnings (SC2086 quote-variables etc.)
   and fails the install PR. Forcing every new repo to clean up
   shellcheck noise before they can ship a single PR is hostile.
   Real GHA bugs (typos in `uses:`, undefined steps refs, deprecated
   runners) are caught by actionlint's native checks — those still run.
   New input `run_shellcheck` (default false) lets normalized repos
   opt in.

2. prettier installs target deps when a package.json exists.
   Repos with custom prettier configs (e.g. wxa-jake-ai's
   prettier-plugin-svelte) need their plugins to resolve. We now run
   `npm ci` (or `npm install` if no lockfile) before prettier when
   package.json is present, then invoke the project's prettier with
   its plugins. Falls back to npx prettier@3 for markdown-only repos.
   New input `install_node_deps` (default true) for explicit control.

Both inputs default to the "least surprise on first install" choice.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Use the project's prettier (with its plugins) when node_modules is
# populated; otherwise fall back to a one-off prettier@3 install.
run: |
GLOB="${{ inputs.markdown_glob || '**/*.md' }}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Script injection via inputs.markdown_glob

${{ inputs.markdown_glob }} is interpolated directly into the shell script body. A caller that passes a value containing shell metacharacters — e.g. **/*.md"; malicious_cmd; echo " — would execute arbitrary code in the runner. This is the injection pattern GitHub's security-hardening guide explicitly warns against.

Fix: bind the expression to an environment variable at the step level and reference that inside run:

      - name: prettier --check ${{ inputs.markdown_glob || '**/*.md' }}
        env:
          GLOB: ${{ inputs.markdown_glob || '**/*.md' }}
        run: |
          if [ -x node_modules/.bin/prettier ]; then
            node_modules/.bin/prettier --check "$GLOB"
          else
            npx --yes prettier@3 --check "$GLOB"
          fi
        shell: bash

With the env-var approach, the expression value is never parsed by the shell as code — it's handed in through the environment.

@claude
Copy link
Copy Markdown

claude Bot commented May 1, 2026

Flagged 1 issue inline — script injection risk at line 121 where inputs.markdown_glob is interpolated directly into the shell body; fix is to bind it to an env var at the step level.

topcoder1 and others added 2 commits May 1, 2026 10:03
`github.event_name` in a reusable workflow returns the caller's
triggering event (e.g. 'pull_request'), NOT 'workflow_call'. So
the previous condition

  if: ${{ github.event_name != 'workflow_call' || inputs.run_X }}

was always true in both self-test AND reusable-call contexts —
inputs.run_actionlint and inputs.run_prettier were silently ignored.
Verified: wxa-jake-ai#181 set run_prettier: false but the prettier
job still ran.

Use `inputs == null` to distinguish self-test (no workflow_call ⇒
inputs is null) from reusable invocation. New condition:

  if: ${{ inputs == null || inputs.run_X }}

  - self-test (pull_request): inputs is null → first clause true → run
  - workflow_call run_X: false: inputs is object, run_X false → skip
  - workflow_call run_X: true: inputs is object, run_X true → run

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous fix used `inputs == null` to distinguish self-test
(pull_request) from workflow_call. Wrong — GHA initializes the
inputs context as `{}` (empty object) on non-workflow_call triggers,
not null. So `inputs == null` was always false and both jobs got
skipped on self-test.

Use `inputs.<field> == null` instead — that's null on self-test
(no schema-default applied) and a real value on workflow_call
(default-filled or caller-provided).

Also corrected the run_shellcheck branch logic that was inverted in
the previous commit, and applied the same null-check pattern to the
nested install_node_deps condition in the prettier job.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread .github/workflows/lint.yml Outdated
# node_modules to resolve them. Run `npm ci` (preferred, exact lockfile)
# or fall back to `npm install` if no lockfile.
- name: Install target repo's deps (for prettier plugins)
if: ${{ (github.event_name != 'workflow_call' || inputs.install_node_deps) && hashFiles('**/package.json') != '' }}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: install_node_deps: false is never respected when called via workflow_call.

The condition github.event_name != 'workflow_call' is always true for reusable workflows — the PR itself documents this in the actionlint job comment (lines 68–71): github.event_name in a reusable workflow always returns the caller's event (e.g. pull_request), never 'workflow_call'. So inputs.install_node_deps is never reached, and npm install runs unconditionally whenever package.json is present, even when the caller sets install_node_deps: false.

The fix should use the same pattern as the job-level if: conditions in this PR:

Suggested change
if: ${{ (github.event_name != 'workflow_call' || inputs.install_node_deps) && hashFiles('**/package.json') != '' }}
if: ${{ (inputs == null || inputs.install_node_deps) && hashFiles('**/package.json') != '' }}

@claude
Copy link
Copy Markdown

claude Bot commented May 1, 2026

Flagged 1 issue inline — the install_node_deps: false opt-out is never applied when called via workflow_call because github.event_name is always the caller's event, not 'workflow_call'; the step condition should use inputs == null || inputs.install_node_deps (same pattern the job-level if: conditions correctly use).

@claude
Copy link
Copy Markdown

claude Bot commented May 1, 2026

No issues found. Logic for null-input detection, shellcheck opt-in conditional, npm caching guard, and --ignore-scripts security practice all look correct.

@topcoder1 topcoder1 merged commit df76988 into main May 1, 2026
3 checks passed
@topcoder1 topcoder1 deleted the chore/lint-shellcheck-and-deps branch May 1, 2026 17:10
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