From a7d3799de502e4598b3510c5a95b52c625a19ed2 Mon Sep 17 00:00:00 2001 From: WayneRocha Date: Thu, 2 Jul 2026 10:53:17 -0300 Subject: [PATCH 1/2] fix: gate phpcs job on reviewdog result, not phpcs whole-file exit code The reviewdog step ended with `exit $PHPCS_EXIT_CODE`, so the job pass/fail was driven by phpcs's exit code. But phpcs scans the entire changed file, so its exit code reflects every issue in that file - including pre-existing ones on lines the PR never touched. reviewdog, meanwhile, runs with -filter-mode=added and only reports/fails on issues introduced on the PR's added lines. Result: a PR that introduces no new violations but touches a file with pre-existing issues gets no inline comment (reviewdog filtered them out) yet still fails the job with the stale phpcs exit code (e.g. exit 2). This is the false positive seen in the-events-calendar#5687 "clean" test. Switch phpcs to --report=checkstyle piped to reviewdog and exit with reviewdog's code instead. A guard still fails the job when phpcs produces no report at all (i.e. it genuinely failed to run). Co-Authored-By: Claude Opus 4.8 --- .github/workflows/phpcs.yml | 45 ++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/.github/workflows/phpcs.yml b/.github/workflows/phpcs.yml index ab91115..c1700bf 100644 --- a/.github/workflows/phpcs.yml +++ b/.github/workflows/phpcs.yml @@ -78,27 +78,36 @@ jobs: env: REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.access-token }} run: | - # Read changed files into array - preserves filenames with spaces, avoids xargs exit code translation + # Read changed files into array - preserves filenames with spaces mapfile -t FILES <<< "${{ env.CHANGED_FILES }}" - # Run phpcs - || captures real exit code without letting bash -e kill the step early - PHPCS_EXIT_CODE=0 - JSON_REPORT=$(vendor/bin/phpcs --report=json -q "${FILES[@]}") || PHPCS_EXIT_CODE=$? + # Run phpcs over the changed files, capturing its checkstyle report. + # IMPORTANT: phpcs's own exit code reflects EVERY issue in the whole + # file, including pre-existing ones on lines this PR never touched, so + # it must NOT gate the job - reviewdog does that below. Wrapping in + # `set +e` keeps `bash -e` from killing the step before reviewdog runs. + set +e + CHECKSTYLE_REPORT=$(vendor/bin/phpcs --report=checkstyle -q "${FILES[@]}") + PHPCS_EXIT_CODE=$? + set -e - # Check if phpcs produced a JSON report - if [ -z "$JSON_REPORT" ]; then - echo "No JSON report generated by phpcs" - exit $PHPCS_EXIT_CODE + # phpcs prints a document even when the code is clean, so + # empty output combined with a non-zero code means phpcs itself failed + # to run (bad standard, parse error, ...). That is a real failure. + if [ -z "$CHECKSTYLE_REPORT" ] && [ "$PHPCS_EXIT_CODE" -ne 0 ]; then + echo "phpcs failed to run (exit code $PHPCS_EXIT_CODE)" + exit "$PHPCS_EXIT_CODE" fi - # Validate the JSON - if ! echo "$JSON_REPORT" | jq empty; then - echo "Invalid JSON" - exit 1 - fi - - # Process JSON and run reviewdog - echo "$JSON_REPORT" | jq -r ' .files | to_entries[] | .key as $path | .value.messages[] as $msg | "\($path):\($msg.line):\($msg.column):`\($msg.source)`
\($msg.message)" ' | reviewdog -efm="%f:%l:%c:%m" -name="phpcs" -filter-mode="added" -fail-level=any -reporter=github-pr-review + # Feed the report to reviewdog. -filter-mode=added limits reporting to + # lines changed by this PR and -fail-level=any makes reviewdog exit 1 + # only when it actually reports such an issue. The job result is thus + # driven by NEW violations, not phpcs's whole-file exit code. + REVIEWDOG_EXIT_CODE=0 + echo "$CHECKSTYLE_REPORT" \ + | reviewdog -f=checkstyle -name="phpcs" -filter-mode="added" -fail-level=any -reporter=github-pr-review \ + || REVIEWDOG_EXIT_CODE=$? - # Exit with the original phpcs exit code - exit $PHPCS_EXIT_CODE + # 0 when the PR introduced no new violations, 1 when it did (inline + # comments have already been posted by reviewdog). + exit "$REVIEWDOG_EXIT_CODE" From a3ad745378e812e015ea2f661afc1a2e83a08f1c Mon Sep 17 00:00:00 2001 From: WayneRocha Date: Thu, 2 Jul 2026 11:20:47 -0300 Subject: [PATCH 2/2] docs: trim phpcs step comments to the essential why Co-Authored-By: Claude Opus 4.8 --- .github/workflows/phpcs.yml | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/.github/workflows/phpcs.yml b/.github/workflows/phpcs.yml index c1700bf..ec1dc0b 100644 --- a/.github/workflows/phpcs.yml +++ b/.github/workflows/phpcs.yml @@ -78,36 +78,27 @@ jobs: env: REVIEWDOG_GITHUB_API_TOKEN: ${{ secrets.access-token }} run: | - # Read changed files into array - preserves filenames with spaces mapfile -t FILES <<< "${{ env.CHANGED_FILES }}" - # Run phpcs over the changed files, capturing its checkstyle report. - # IMPORTANT: phpcs's own exit code reflects EVERY issue in the whole - # file, including pre-existing ones on lines this PR never touched, so - # it must NOT gate the job - reviewdog does that below. Wrapping in - # `set +e` keeps `bash -e` from killing the step before reviewdog runs. + # phpcs's exit code covers the whole file (incl. pre-existing issues on + # untouched lines), so it must not gate the job. set +e stops bash -e + # from killing the step on that non-zero exit. set +e CHECKSTYLE_REPORT=$(vendor/bin/phpcs --report=checkstyle -q "${FILES[@]}") PHPCS_EXIT_CODE=$? set -e - # phpcs prints a document even when the code is clean, so - # empty output combined with a non-zero code means phpcs itself failed - # to run (bad standard, parse error, ...). That is a real failure. + # No report + non-zero exit means phpcs itself failed to run. if [ -z "$CHECKSTYLE_REPORT" ] && [ "$PHPCS_EXIT_CODE" -ne 0 ]; then echo "phpcs failed to run (exit code $PHPCS_EXIT_CODE)" exit "$PHPCS_EXIT_CODE" fi - # Feed the report to reviewdog. -filter-mode=added limits reporting to - # lines changed by this PR and -fail-level=any makes reviewdog exit 1 - # only when it actually reports such an issue. The job result is thus - # driven by NEW violations, not phpcs's whole-file exit code. + # reviewdog gates the job: -filter-mode=added + -fail-level=any make it + # exit 1 only for violations on lines this PR changed. REVIEWDOG_EXIT_CODE=0 echo "$CHECKSTYLE_REPORT" \ | reviewdog -f=checkstyle -name="phpcs" -filter-mode="added" -fail-level=any -reporter=github-pr-review \ || REVIEWDOG_EXIT_CODE=$? - # 0 when the PR introduced no new violations, 1 when it did (inline - # comments have already been posted by reviewdog). exit "$REVIEWDOG_EXIT_CODE"