Files
FOSSBilling/.github/workflows/code-quality-check.yml
Adam Daley 5f6397cb12 Fix and harden the Code Quality & Styling automation (#4206)
* 🎨 Apply pending Rector/PHP-CS-Fixer fixes

Drift left over from the auto-cs-fix.yml run that failed to open its
PR due to an expired BOT_TOKEN.

* 🐛 Fix code quality workflow and add PR-level style check

- Rotate the sweep's push credential from BOT_TOKEN (expired) to
  FOSSBILLING_BOT_TOKEN, a fresh fine-grained PAT scoped to just this
  repo (contents, pull-requests, issues: read/write).
- Factor the shared Rector/PHP-CS-Fixer setup out of auto-cs-fix.yml
  into a local composite action, .github/actions/php-quality-setup.
- Add code-quality-check.yml: a read-only dry-run check that runs on
  every pull request, including forks, scoped to just the files each
  PR changes so pre-existing drift on main can't fail an unrelated
  PR. Complements the scheduled sweep, which now only needs to catch
  drift from rule/tool-version changes on code no open PR touches.

* 🎨 Replace peter-evans/create-pull-request with gh CLI

Removes the third-party Action dependency for opening the sweep's PR
in favor of the officially-maintained gh CLI, which ships preinstalled
on GitHub-hosted runners:

- gh auth setup-git wires GH_TOKEN into git's credential helper, so
  plain git push/fetch authenticate without a hand-rolled
  http.extraheader (what silently broke with the old BOT_TOKEN).
- chore/cs-fix is fully regenerated every run, so there's no need to
  diff against existing remote content — force-with-lease push plus
  gh pr list/create covers create-or-update.
- Also more portable: the same script runs unchanged outside GitHub
  Actions' action-invocation model (other CI, cron, a terminal),
  whereas a third-party Action cannot.
- Top-level permissions trimmed to contents: read — nothing in the
  job uses the implicit GITHUB_TOKEN anymore.

* 🔒 Pin 1password/load-secrets-action to v5.0.1 by SHA

Was floating on the mutable @v4 tag, inconsistent with every other
third-party action in this repo (all pinned by commit SHA), and a
version behind — v5.0.1 is current as of last week and includes
1Password's own CI hardening. No breaking changes for our usage
(OP_SERVICE_ACCOUNT_TOKEN path is untouched; v5.0.0 only adds an
alternative OIDC auth method).

* 🎨 Remove unused create_pr operation output

Set in both branches but never read — the job summary only uses
.outputs.url, and the same created-vs-updated info is already in the
step's own log output.

* 🐛 Fix two Rector rules that generate code breaking Pest

CI on PR #4206 caught these — I'd only run php -l (syntax check) on
the earlier drift-cleanup commit, not the actual test suite, and both
slipped through:

- AddArrowFunctionReturnTypeRector typed Pest expect()-chain closures
  as their IDE-only @mixin stub (Pest\Mixins\Expectation) instead of
  the real runtime class (Pest\Expectation) — throws a TypeError.
- ArrowFunctionDelegatingCallToFirstClassCallableRector converted a
  Pest ->with() dataset closure to a first-class callable reference,
  which breaks Pest's Closure::bindTo() lazy-evaluation step (no scope
  to rebind on a closure derived from a plain function).

Both only misfire against Pest's dynamic patterns in tests/, so
they're skipped there specifically rather than repo-wide — src/ keeps
the benefit. Reverts the two bad transformations and re-verifies with
the full Pest suite this time, not just php -l.

* 🎨 Apply Rector fixes for drift introduced by #4205

RecastingRemovalRector: three redundant (string) casts, plus one
PHP 8.3 typed-const/literal-return-type modernization in
CacheFactory.php — all from files #4205 touched after this branch
forked from main.

Verified with the full suite this time (2812 tests, 10979 assertions)
and PHPStan, not just php -l.

* 🎨 Fix import order in rector.php

Caught by the new PR check itself — it passes rector.php as one of
the changed files, so PHP-CS-Fixer processes it directly (path-mode
override), unlike the scheduled sweep whose Finder never scans
repo-root config files at all.

* 🔒 Fix script-injection risk and stale fetch ref from CodeRabbit review

- code-quality-check.yml: the changed-file list was interpolated via
  ${{ steps.changed_files.outputs.files }} directly into a run: script.
  That's literal text substitution before bash parses anything, so a
  PHP filename containing shell metacharacters (or just a space) could
  break argument parsing or execute as a command. Fixed by writing the
  NUL-delimited path list to a file and reading it back into a real
  bash array with mapfile in every step that needs it — only a plain
  boolean (has_php_files) now crosses the ${{ }} boundary. Verified:
  a space-containing filename was silently split into two broken
  arguments under the old design; the array-based version preserves
  it as one element.
- auto-cs-fix.yml: pin the fetch to update the actual remote-tracking
  ref (refs/remotes/origin/chore/cs-fix), not just FETCH_HEAD, so
  --force-with-lease always has a real baseline to compare against.

* 🔒 Add option-terminator before changed-file paths

A filename beginning with '-' would otherwise be parsed as a Rector/
PHP-CS-Fixer CLI option rather than a path. Not currently reachable
given rector.php/.php-cs-fixer.dist.php scope to src/ and tests/ (a
repo-relative diff path always has that prefix), but zero-cost to
harden regardless. Verified the composed composer script -- ... -- ...
invocation forwards the second terminator correctly.

* 🎨 Apply Rector/CS-Fixer fixes for drift introduced by #4204

readonly on Registrar_Rdap::$httpClient (verified: only ever assigned
in the constructor), PHP 8.3 typed consts, redundant @param mixed
removal, pre-increment style. Verified with the full suite (2854
tests, 11055 assertions) and PHPStan.
2026-08-25 22:16:58 +01:00

91 lines
3.6 KiB
YAML

name: Code Quality & Styling Check
on:
pull_request:
branches: main
types: [opened, reopened, synchronize]
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
cancel-in-progress: true
permissions:
contents: read
jobs:
check-cs:
name: Rector & PHP-CS-Fixer
runs-on: ubuntu-latest
timeout-minutes: 15
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
with:
persist-credentials: false
# Full history is needed so the merge-base diff below can reach
# the PR's base commit — the default shallow clone won't have it.
fetch-depth: 0
- name: Set Up PHP Quality Tools
uses: ./.github/actions/php-quality-setup
with:
php-version: 8.3
# The path list is written to a file, not a $GITHUB_OUTPUT string —
# ${{ }} is literal text substitution into the run: script before bash
# ever parses it, so interpolating a filename there is a script
# injection vector (and breaks on spaces regardless). Only the boolean
# below crosses the step boundary via outputs; every step that needs
# the actual paths reads them back into a real array with mapfile.
- name: Determine Changed PHP Files
id: changed_files
env:
BASE_SHA: ${{ github.event.pull_request.base.sha }}
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
run: |
# Triple-dot diffs against the merge-base, not the base branch's
# current tip, so a PR isn't blamed for drift on main it never
# touched — matches what GitHub's own "Files changed" tab shows.
git diff -z --name-only --diff-filter=ACMR "$BASE_SHA...$HEAD_SHA" -- '*.php' > "$RUNNER_TEMP/changed-php-files.txt"
if [ -s "$RUNNER_TEMP/changed-php-files.txt" ]; then
echo "has_php_files=true" >> "$GITHUB_OUTPUT"
echo "Changed PHP files:"
xargs -0 -n1 printf ' %s\n' < "$RUNNER_TEMP/changed-php-files.txt"
else
echo "has_php_files=false" >> "$GITHUB_OUTPUT"
echo "No changed PHP files in this pull request — skipping style/quality checks."
fi
# Scoped to just the files this PR touches, not the whole repo — see
# https://cs.symfony.com/doc/usage.html#using-php-cs-fixer-on-ci. This
# also means pre-existing drift on main (only ever cleaned up by the
# scheduled sweep in auto-cs-fix.yml) can't fail an unrelated PR.
- name: Run Rector (dry-run)
id: rector
if: steps.changed_files.outputs.has_php_files == 'true'
run: |
mapfile -d '' -t files < "$RUNNER_TEMP/changed-php-files.txt"
composer rector:check -- --no-progress-bar -- "${files[@]}"
# --config is required here: PHP-CS-Fixer only auto-discovers
# .php-cs-fixer.dist.php when given a single (or zero) path argument,
# and refuses to run against multiple explicit paths without it.
- name: Run PHP-CS-Fixer (dry-run)
id: php_cs_fixer
if: steps.changed_files.outputs.has_php_files == 'true'
run: |
mapfile -d '' -t files < "$RUNNER_TEMP/changed-php-files.txt"
composer cs:check -- --config=.php-cs-fixer.dist.php -- "${files[@]}"
- name: Write Job Summary
if: always()
run: |
cat >> "${GITHUB_STEP_SUMMARY}" <<'EOF'
## Code Quality and Styling Check
| Item | Value |
| --- | --- |
| PHP version | `8.3` |
| Rector | `${{ steps.rector.outcome }}` |
| PHP-CS-Fixer | `${{ steps.php_cs_fixer.outcome }}` |
EOF