Skip to content

PR version sync doubles every required check; make version-only commits cheap #37

Description

@darksidemilk

Summary

Every push to a working-1.6 pull request runs the full required-check suite twice. The second run exists only to re-validate a bot commit that changes a single string constant.

This is not a runaway loop. The circuit breakers in fogproject-pr-regen.yml hold — no pull request has ever carried two consecutive bot commits, and the "Assert regeneration converged" step has never fired. It is a fixed 2x amplification, and the cause is that the pre-merge version sync writes a commit to the PR head, which re-fires all seven required checks.

Measured on 2026-08-27 against live run history.

Findings

1. All of the churn is the version sync. None of it is regen.

Seven bot commits sampled across #1415, #1416, #1419, #1424, #1425 and #1426. Every one touches exactly one file:

$ gh api repos/FOGProject/fogproject/commits/<sha> --jq '[.files[].filename]|join(", ")'

e8cd2dcd7 -> packages/web/src/Base/System.php
5d7b22052 -> packages/web/src/Base/System.php
c73c61d   -> packages/web/src/Base/System.php
8383fc7   -> packages/web/src/Base/System.php
4420c10   -> packages/web/lib/fog/system.class.php
a805a5b   -> packages/web/lib/fog/system.class.php
6fb159d   -> packages/web/lib/fog/system.class.php

Zero were PSR2 or gettext corrections. The regeneration half of fogproject-pr-regen.yml is, in practice, a no-op — the pre-commit hook is doing its job. Only the version step ever produces a commit.

2. Seven required checks re-run on each version-only commit

Verified on e8cd2dcd7:

$ gh api repos/FOGProject/fogproject/commits/e8cd2dcd7/check-runs

fogproject / tests (PHP 7.4)               success
fogproject / tests (PHP 8.3)               success
fogproject / vendor matches composer.lock  success
fogproject / schema on mariadb:10.5        success
fogproject / schema on mariadb:11.8        success
fogproject / schema on mysql:8.0           success
fogproject / release pins resolve          success

All seven are required by the working-1.6 ruleset, with strict_required_status_checks_policy: true (gh api repos/FOGProject/fogproject/rules/branches/working-1.6).

None of them can be affected by a change to FOG_VERSION.

3. The cost

Roughly 2.4–2.8 minutes per Tests run.

PR contributor pushes bot commits Tests runs today if the bot commit were free
#1426 1 1 2 1
#1416 2 (+1 base merge) 2 4 2
#1415 8 8 16 8

#1415's eight version numbers — 4189, 4191, 4193, 4196, 4198, 4200, 4212, 4219 — were each obsolete within minutes of being written. Only the last one mattered.

4. An extra round-trip when the base moves

#1416's actual history:

399bdd1  Tom Elliott         push
d54eefb  Tom Elliott         push
49b52dd  fog-workflows[bot]  Version Sync: ... 4190
a1ae945  Tom Elliott         Merge remote-tracking branch 'origin/working-1.6'
19dc96e  fog-workflows[bot]  Version Sync: ... 4195

Four cycles for one pull request. strict forces the branch update, and fogproject-pr-regen.yml's "Does this branch contain the tip of its base?" gate skips the version step rather than updating the branch itself — so the correction needs a second, human-triggered round.

What we want

The version commit should not re-run the tests. Two approaches that sound right and are not:

  • Post-merge sync instead (reverting to sync-generated-files.yml owning the version). Rejected: it moves the working-1.6 tip a second time per merge, and under strict that re-stales every open pull request 1–3 minutes after you have just updated it. That inhibits continuous development more than the double-testing does.
  • Push with GITHUB_TOKEN, or [skip ci]. Both already documented as traps at the top of fogproject-pr-regen.yml: a push that produces no run leaves the new SHA with no checks, and the PR deadlocks on "Expected — waiting for status" forever.

Options

Option 1 — Fast-path the required jobs (recommended)

A version-only bot commit provably cannot affect any of the seven checks. Make the jobs recognise that and report genuine success cheaply, rather than trying to avoid the run.

Add a cheap gate job to fogproject-tests.yml (shallow checkout) that outputs trivial=true only when all of these hold:

  • the head commit is authored by the App bot;
  • exactly one file changed;
  • that file is packages/web/src/Base/System.php or packages/web/lib/fog/system.class.php;
  • every changed line matches define('FOG_VERSION', ...) or define('FOG_CHANNEL', ...).

Fail closed: anything unexpected yields false.

Every existing job then gains needs: gate, and each expensive step gets if: needs.gate.outputs.trivial != 'true'.

Do not put the condition at job level. A skipped job reports the skipped conclusion, which does not satisfy a required status check — the pull request would deadlock exactly like the GITHUB_TOKEN case above. The jobs must run and succeed with their steps no-op'd, so the check name and a real success conclusion still attach to the SHA.

Known rough edge: the schema job declares job-level services: with three DB images, and service containers start before steps run — so conditional steps alone leave ~30–40s of container startup. Either accept that, or move DB startup into a step (docker run) so it can be skipped too. Reasonable as a follow-up rather than a blocker.

Conservative variant: fast-path only vendor, schema and pins — the three that provably never read the constant — and let the PHP suite run for real. Less saving, but no need to reason about tests/autoload.test.php, which does grep FOG_VERSION out of System.php.

Result: roughly N cycles of real work per pull request. Independent of Option 2, and composes with it.

Option 2 — Defer the version sync to a merge-time signal

A new workflow file on working-1.6 only, triggered by on: pull_request: types: [auto_merge_enabled, labeled]. Development pushes run tests once; one prepare pass at the end merges the base in if stale, regenerates, computes the version, and pushes a single commit.

  • Reduces 2N to N+1. It does not reach N — the final SHA always pays one extra full cycle, because required checks must attach to the new SHA.
  • Needs nothing on stable. pull_request and all of its activity types are read from the merge of head into base, so a file living on working-1.6 is consulted; pull_request_target, schedule and workflow_dispatch are the default-branch-only ones. allow_auto_merge is already true on the repo.
  • Would also fix finding 4 if the prepare pass merges the base in itself — but that requires relaxing the one-consecutive-bot-commit breaker, since re-preparing after the base moves again would legitimately trip it. Rebound it by content rather than by author.

Option 3 — Ruleset bypass plus programmatic merge

Grant the App a ruleset bypass; the prepare pass pushes the version commit and merges immediately, asserting the version-only delta is safe rather than re-proving it. Simplest to implement, but blunt — it removes the checks as a gate on the final SHA entirely. Listed for completeness; not recommended.

Option 4 — Retire the committed constant (deferred)

The only option that removes the version commit entirely. Bigger change, but the investigation turned up that its cost is currently much lower than it looks, and that is worth recording before the window closes:

  • utils/FOGUpdater/fogupdater.sh resolves a base branch from the update channel (Beta -> working-1.6, trunk -> dev-branch, Patches -> stable), reads raw.githubusercontent.com/.../System.php, and then verifyPayload() re-reads it from the downloaded branch tarball and demands an exact match. It never reads a PR head — it has no concept of one.
  • That raw-URL protocol is only days old (34150bc79, 2026-08-26, which replaced the SourceForge / fogproject.org/version/index.php path), and utils/ has only been installed to $fogprogramdir since 666f9385c, 2026-08-01. The deployed population bound to this protocol is very small right now, and only grows.
  • Shape if pursued: publish a version manifest out-of-band; generate a gitignored Version.php at install time (from git when .git is present, else from the shipped manifest); point fogupdater.sh and verifyPayload() at the manifest. Also touches bin/installfog.sh:88, bin/updatefog.sh:223, lib/common/utils.sh:66, tests/autoload.test.php and tests/fogupdater-update-source.test.sh.

Recommendation

Option 1, optionally with Option 2 later if the remaining single extra cycle per PR is still worth removing.

Option 1 attacks the actual cost rather than the commit, needs no ruleset change, keeps every check reporting honestly on every SHA, and does not touch stable or the default branch of either repo. The schema services caveat is the one loose end and can land separately.

Files involved

  • .github/workflows/fogproject-tests.yml (this repo) — the four job definitions behind the seven required contexts; where Option 1 lands.
  • .github/workflows/fogproject-pr-regen.yml (this repo) — the version sync, the marker-commit math, the up-to-date gate, the circuit breakers.
  • .github/workflows/tests.yml on fogproject working-1.6 — the stub carrying sync_version: true and the eligibility policy.
  • .githooks/lib/fog-version.sh on fogproject — the version formula. Stays there; not to be duplicated here.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions