Skip to content

fix: gate phpcs job on reviewdog result, not phpcs whole-file exit code#24

Merged
WayneRocha merged 2 commits into
mainfrom
fix/phpcs-reviewdog-exit-code
Jul 2, 2026
Merged

fix: gate phpcs job on reviewdog result, not phpcs whole-file exit code#24
WayneRocha merged 2 commits into
mainfrom
fix/phpcs-reviewdog-exit-code

Conversation

@WayneRocha

@WayneRocha WayneRocha commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Problem

After #22 and #23, the phpcs job still fails on PRs that introduce no new violations.

The Run reviewdog step ends with:

exit $PHPCS_EXIT_CODE

So the job's pass/fail is driven by phpcs's exit code. But phpcs scans the whole changed file, so its exit code reflects every issue in that file — including pre-existing ones on lines the PR never touched. reviewdog, by contrast, runs with -filter-mode=added and only reports/fails on issues introduced on the PR's added lines.

The two disagree, and the job exits on the phpcs code.

Reproduction — the-events-calendar#5687 "clean" test (bf090fc)

From the job log:

mapfile -t FILES <<< "src/functions/views/provider.php"
##[error]Process completed with exit code 2.
  • phpcs scanned provider.php, found pre-existing issues → exited 2.
  • Those issues are not on the PR's added lines, so reviewdog filtered them out → no inline comment ("No bot display").
  • Then exit $PHPCS_EXIT_CODE ran exit 2job failed anyway.

#22/#23 focused on faithfully preserving the phpcs exit code — which is exactly the value that should not gate the job.

Fix

  • Run phpcs with --report=checkstyle and pipe straight to reviewdog (-f=checkstyle), dropping the jq transform.
  • Exit with reviewdog's exit code, not phpcs's. -filter-mode=added -fail-level=any means reviewdog exits 1 only when it reports a violation on a line this PR changed.
  • Keep a guard: if phpcs produces no checkstyle output and exits non-zero, it genuinely failed to run (bad standard, parse error, …) → fail the job with phpcs's code.
set +e
CHECKSTYLE_REPORT=$(vendor/bin/phpcs --report=checkstyle -q "${FILES[@]}")
PHPCS_EXIT_CODE=$?
set -e

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

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 "$REVIEWDOG_EXIT_CODE"

Expected behavior across the-events-calendar#5687 test commits

Commit Introduces new violations? reviewdog comment Job
Test 1 / Test 2 yes posted fails
Test 3 (clean) no (pre-existing only) none passes

Test PR

the-events-calendar#5687 repointed to @fix/phpcs-reviewdog-exit-code to re-run all three scenarios.

Created by claude 🤖

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 <noreply@anthropic.com>
WayneRocha added a commit to the-events-calendar/the-events-calendar that referenced this pull request Jul 2, 2026
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@WayneRocha WayneRocha merged commit e3407b0 into main Jul 2, 2026
@WayneRocha WayneRocha deleted the fix/phpcs-reviewdog-exit-code branch July 2, 2026 15:33
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