Skip to content

fix(quality): a missing composer gate script must fail, not pass vacuously - #123

Merged
rubenvdlinde merged 1 commit into
mainfrom
fix/121-missing-composer-script-must-fail
Aug 2, 2026
Merged

fix(quality): a missing composer gate script must fail, not pass vacuously#123
rubenvdlinde merged 1 commit into
mainfrom
fix/121-missing-composer-script-must-fail

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Closes #121.

The hole

composer <script> exits 1 for two very different things:

  • the script ran and returned 1, and
  • Command "phpcs" is not defined. — there is no such script.

The phpcs leg maps exit 1 to success (PHPCS warnings are non-blocking by
policy), so any repo with no phpcs composer script got a permanently green
PHP Quality (phpcs) tick
. A gate's absence and a gate's success were
indistinguishable in the checks list.

Before

phpcs)
  if [ "${{ inputs.enable-phpcs }}" != "true" ]; then
    echo "PHPCS is disabled — skipping."
    exit 0
  fi
  # Exit code 0 = clean, 1 = warnings only, 2 = errors found.
  # We treat warnings as non-blocking (exit 1 → success).
  RC=0
  composer phpcs || RC=$?
  if [ "$RC" -eq 1 ]; then
    echo "::warning::PHPCS found warnings but no errors — passing."
    exit 0
  fi
  exit $RC
  ;;

The comment describes phpcs's exit codes; RC holds composer's.

After

A require_script helper reads the declaration from composer.json (via jq,
already used 46× in this workflow) before the script is run, on every PHP
quality leg:

require_script() {
  tool="$1"
  hint="$2"
  if [ ! -f composer.json ]; then
    echo "::error::The '$tool' gate is enabled but there is no composer.json in the repository root."
    exit 1
  fi
  if ! jq -e --arg s "$tool" '(.scripts // {}) | has($s)' composer.json > /dev/null 2>&1; then
    echo "::error::The '$tool' gate is enabled but composer.json declares no '$tool' script. Add it under \"scripts\", or turn the gate off with $hint."
    exit 1
  fi
}

Deliberately not an if [ -f vendor/bin/<tool> ] guard — a file-presence
guard that skips is the same silent-pass defect in a new costume. (Also, a
bare phpmd/psalm in a composer script resolves fine without vendor/bin/,
because Composer prepends vendor/bin to PATH.)

lint, phpmd, psalm, phpstan and phpmetrics already went red on a
missing script — but with the opaque Command "phpmd" is not defined. They now
get an actionable message naming the input that turns the gate off. Their
red/green verdict is unchanged.

Proof — three live CI cells on ConductionNL/petstore

Throwaway branch feature/dotgithub-121-proof. One variable changes per cell.

Cell composer.json reusable ref PHP Quality (phpcs) Run
A has phpcs this branch (fixed) success 30766494556
B no phpcs this branch (fixed) failure 30766612959
C no phpcs @main (unfixed) success — the bug 30766687154

Cell C, @main, verbatim — the vacuous pass reproduced live:

Did you mean one of these?
    phpcs-DELIBERATELY-RENAMED-FOR-ISSUE-121-PROOF Runs the phpcs-DELIB...
    phpcs:fix                                      Runs the phpcs:fix s...
    phpcs:output                                   Runs the phpcs:outpu....
##[warning]PHPCS found warnings but no errors — passing.
##[group]Run mkdir -p quality-results
echo "success" > quality-results/phpcs.txt

Cell B, this branch, same repo state — now red with an actionable message:

##[error]The 'phpcs' gate is enabled but composer.json declares no 'phpcs' script. Add it under "scripts", or turn the gate off with the enable-phpcs input.
##[error]Process completed with exit code 1.

Cell A, this branch, real script present — still green:

FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
...
Time: 634ms; Memory: 10MB

→ job conclusion success, echo "success" > quality-results/phpcs.txt.

C→B is the discriminator: identical repo state, only the reusable-workflow ref
differs, and the verdict flips. A→B holds the ref fixed and flips only the
script's existence.

Blast radius — exactly one repo, one leg

All 19 consumers of this workflow in the org were swept for the six gate scripts
in their default-branch composer.json:

Repo Effect
openregister, hermiq, openbuild, openbuilt, pipelinq, doriath, opencatalogi, decidesk, zaakafhandelapp, scholiq, portaliq, planix, nextcloud-app-template, larpingapp, petstore, shillinq, deskdesk, mydash all six scripts declared — no change
app-versions PHP Quality (phpcs) flips green → red

app-versions uses php-cs-fixer, not phpcs, and declares no phpcs, phpmd,
phpstan or phpmetrics script while leaving enable-phpcs at its true
default. Its phpmd / phpstan / phpmetrics legs are already red for
exactly this reason; only phpcs was being swallowed. Turning that tick red is
the point of this change — but it is the one CI status in the fleet that moves.
Fix on that side is one line: enable-phpcs: false in
app-versions/.github/workflows/code-quality.yml (see ConductionNL/versioniq#118).

Not touched

global-settings/ is unmodified, so no VERSION bump is required.

…ously

`composer <script>` exits 1 both when the script runs and returns 1 and when
the script does not exist ("Command \"phpcs\" is not defined."). The phpcs leg
maps exit 1 to success to keep PHPCS warnings non-blocking, so any repo with no
`phpcs` composer script got a green "PHP Quality (phpcs)" tick forever — a
gate's absence and a gate's success were indistinguishable in the checks list.

Assert the script is declared in composer.json (via jq) before running it, on
every PHP quality leg. The other legs already went red on a missing script, but
with the opaque `Command "phpmd" is not defined.`; they now get an actionable
message naming the input that turns the gate off.

Deliberately not an `[ -f vendor/bin/<tool> ]` guard — a file-presence guard
that skips reintroduces the same silent-pass defect.

Closes #121
@rubenvdlinde
rubenvdlinde merged commit 9d9f884 into main Aug 2, 2026
4 checks passed
rubenvdlinde added a commit that referenced this pull request Aug 21, 2026
This workflow ends by opening a pull request that brings the integration
branch up to the version it just tagged. Merging that pull request is a push
to the integration branch, which triggers this workflow again, which bumps
the version again, which opens another pull request.

The bump commit carries `[skip ci]`, but the MERGE COMMIT that actually lands
on the branch does not — so the marker never gets a chance to work.

Measured on opencatalogi, 2026-08-20, over its last 100 development commits:

    45  chore(release): version bumps
    46  merges of release/* branches
     9  actual work

91% of the branch's history was the workflow reacting to itself, one cycle
about every ten minutes, and the base version never advanced: every one of
those releases was 1.0.9-unstable.<timestamp>.

It hides well. Each run is a legitimate release of a real commit, so the only
symptom is that the branch never settles — which reads as "someone is
committing a lot" rather than as a defect. The damage shows up elsewhere: a
development branch that moves every ten minutes cancels its own in-flight CI
by concurrency, and opencatalogi's dev→beta pull request had its E2E and
Hydra Gates jobs cancelled repeatedly for exactly that reason. A cancelled
job yields no verdict, so the promotion could not be judged at all.

The guard skips the job when the triggering push IS the bump landing, in
either merge style: a merge commit names the `release/v*` head branch, and a
squash produces `chore(release): <version> (#123)`. Nothing is lost — those
pushes contain only appinfo/info.xml (and openapi.json where present), so
re-releasing them would build a byte-identical artefact under a new number.

`workflow_dispatch` is never skipped, so a human can still force a release by
hand, including of a commit this guard would refuse.

Verified with a truth table over REAL commit messages from opencatalogi's
history, asserting both directions rather than only the skip: the two bump
shapes and the squash shape skip, while `feat/rotterdam-demo-rig`, a plain
`test(federation):` commit and a `chore/nc-vue-2.8.2` dependency bump all
still release. A guard that only proved it can skip would be indisttinguishable
from one that skips everything.

Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
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.

quality.yml: a missing phpcs composer script passes VACUOUSLY (composer exit 1 read as 'warnings only')

1 participant