From 40f176aa3da47b757ef0d5ae6409827724721c29 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 11:41:14 +1200 Subject: [PATCH 01/14] ci(fewer-jobs): fold the copyright, quarantine-audit and docs-data gate jobs into repo: hygiene Three per-PR jobs ran gates that `repo: hygiene` already runs in its `bin/check-all.sh --with-tests` sweep: `Copyright header check` (copyright.yml, deleted), `quarantine: audit` and `docs data: audit` (maven.yml, deleted). Every PR push launched them twice. Master run 34066495360 shows all four gates and their self-tests `ok` inside the sweep before this deletion. Why it matters: the repo is public on the free plan, whose cap is 20 concurrent jobs, and a PR push launched 27 of ours plus 3 CodeQL analyzes. The big sharded suites were queueing behind one-minute jobs. This is the first cut of the ce-optimize run `ci-fewer-jobs`: 27 -> 24. What the fold had to carry over so nothing quietly stops running: - COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1 and an explicit PyYAML assertion on the hygiene job, mirroring its shellcheck one. - check-all.sh gains --strict: a CANNOT (exit 2, cannot run) fails the sweep. Without it a missing tool on the hosted image would have read as a pass, because check-all exits 0 with a CANNOT among passes. CI runs strict; the local default is unchanged. - No token added to hygiene: check-quarantine-owners.sh is advisory there, and quarantine-lane.yml (required `tests`) already runs both quarantine gates with a token on every push. The three deleted job names are required checks in the master ruleset. docs/inflight/ci-fewer-jobs-ruleset-edits.md lists the removals and the merge-time ordering; a required context nothing produces leaves every PR pending. Two citations of the deleted workflow were repaired: the live docs/copyright.md reworded, the dated solution record given a history pointer per docs/citations.md. Co-authored-by: Claude Fable 5.1 --- .github/workflows/README.md | 17 +++--- .github/workflows/copyright.yml | 50 ---------------- .github/workflows/dependency-audit.yml | 5 +- .github/workflows/maven.yml | 43 -------------- .github/workflows/quarantine-lane.yml | 16 +++-- .github/workflows/repo-hygiene.yml | 39 +++++++++--- bin/check-all.sh | 37 ++++++++++-- docs/ci.md | 49 ++++++++++----- docs/copyright.md | 4 +- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 59 +++++++++++++++++++ .../ci-path-filters-for-docs-only-prs.md | 4 +- ...yright-header-rules-for-fork-2026-04-21.md | 2 +- 12 files changed, 185 insertions(+), 140 deletions(-) delete mode 100644 .github/workflows/copyright.yml create mode 100644 docs/inflight/ci-fewer-jobs-ruleset-edits.md diff --git a/.github/workflows/README.md b/.github/workflows/README.md index edd7985cd..74f615dc7 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -25,7 +25,6 @@ procedure or a steer. Why, and the gate's exact contract: [`docs/ci.md`](../../d | `claude-code-review-dispatch.yml` | The dispatched code reviewer - the expensive half, run when somebody asks for it. | | `claude-code-review.yml` | The review gate. Invokes no Claude and costs nothing. What satisfies it: [`docs/ci.md`](../../docs/ci.md), stated once there. | | `claude.yml` | Answers `@claude` comments, and reviews when asked to - the only route that can raise inline review threads. | -| `copyright.yml` | Checks source headers against the fork policy on every push and PR. | | `dependency-audit.yml` | Scans the **whole** resolved dependency tree for CVEs - the only place OSS Index is switched on. Per-PR, on demand, and weekly, because a new advisory needs no push to arrive. | | `maven.yml` | The main build: unit, integration and performance suites, SpotBugs, duplicate detection, PIT, dependency scanning. | | `mutation-full-sweep.yml` | The whole-repo PIT mutation sweep - nightly, plus on demand. Too slow for a PR. Self-hosted high-CPU lane, plus a hosted trial arm. | @@ -33,7 +32,7 @@ procedure or a steer. Why, and the gate's exact contract: [`docs/ci.md`](../../d | `publish.yml` | Publishes to Maven Central on every push to `master`; the pom version decides snapshot or release. | | `quarantine-lane.yml` | Runs the quarantined tests separately, so known-flaky tests neither block nor disappear. | | `release.yml` | Cuts a release. `workflow_dispatch`, and deliberately the most dangerous button here. | -| `repo-hygiene.yml` | Always-on repo checks - shell sigpipe traps, one pinned version per GitHub Action, and expiring the pom's temporary CVE exclusions - plus `shell: macos`, the one lane not on `ubuntu-latest`: it runs the shell self-tests and a `bash -n` sweep against Apple's bash 3.2, where GNU-only constructs fail silently. | +| `repo-hygiene.yml` | Always-on repo checks, every `bin/check-*` gate and self-test by glob - copyright headers against the fork policy, the quarantine registry, the release-documentation data, shell hazards, one pinned version per GitHub Action, expiring the pom's temporary CVE exclusions - plus `shell: macos`, the one lane not on `ubuntu-latest`: it runs the shell self-tests and a `bash -n` sweep against Apple's bash 3.2, where GNU-only constructs fail silently. | ## One required check is not in this directory @@ -48,12 +47,14 @@ trade costs is in [`docs/ci.md`](../../docs/ci.md). ## Two conventions that will bite you - **Job names are an API.** `claude-review`, `review: human LGTM`, `Check PR Dependencies`, - `quarantine: audit`, `Copyright header check` and the `maven.yml` suites are required status - checks matched **by name** in the master ruleset. Rename a job and the ruleset silently stops - being satisfied by anything - it does not fail, it just never passes. `shell: sigpipe` and - `workflows: action versions` were two such names; both jobs were folded into `repo-hygiene.yml`'s - single `repo: hygiene` lane, and - as of the last live check of the ruleset - that replacement - lane is not itself in the required list. See [`docs/ci.md`](../../docs/ci.md) for the detail. + `repo: hygiene` and the `maven.yml` suites are required status checks matched **by name** in the + master ruleset. Rename a job and the ruleset silently stops being satisfied by anything - it does + not fail, it just never passes. `shell: sigpipe` and `workflows: action versions` were two such + names, folded into `repo-hygiene.yml`'s single `repo: hygiene` lane; `Copyright header check`, + `quarantine: audit` and `docs data: audit` followed them there, and until the ruleset drops those + three contexts they are required names no job produces - + [`docs/inflight/ci-fewer-jobs-ruleset-edits.md`](../../docs/inflight/ci-fewer-jobs-ruleset-edits.md) + carries the edit. See [`docs/ci.md`](../../docs/ci.md) for the detail. - **Most of these run PR-authored code.** A `pull_request` job checks out the PR, so anything it executes is whatever the PR says it is. That is why the review jobs hold no write scope, and why `actions: write` lives alone in a job that checks nothing out. See "The reviewer runs PR code" diff --git a/.github/workflows/copyright.yml b/.github/workflows/copyright.yml deleted file mode 100644 index d0c194c7a..000000000 --- a/.github/workflows/copyright.yml +++ /dev/null @@ -1,50 +0,0 @@ -# Copyright (C) 2026 Antony Stubbs and contributors - -# Copyright header conformance - fast, GitHub-hosted, gate-friendly. -# -# Enforces the fork's two-header policy (docs/copyright.md): -# upstream-derived files keep the Confluent header; fork-original files (added after -# the pinned fork point) must carry the fork header and must not claim Confluent -# copyright. The mycila license plugin could not express this (single header template, -# git-based year auto-bumping), so it was removed and this scanner is the enforcement. -# -# fetch-depth: 0 because the scanner classifies provenance via the fork-point commit, -# which a shallow clone does not contain. - -name: Copyright Headers - -# push is scoped to master (matching maven.yml) so PR-branch commits run once via the -# pull_request event instead of twice. -on: - push: - branches: [ master ] - pull_request: - workflow_dispatch: - -concurrency: - group: copyright-${{ github.head_ref || github.ref }} - cancel-in-progress: true - -permissions: - contents: read - -jobs: - check: - name: "Copyright header check" - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: - - name: Checkout (full history - scanner needs the fork-point commit) - uses: actions/checkout@v6 - with: - fetch-depth: 0 - - - name: Self-test the scanner (fixture repos) - run: bash bin/test-check-copyright-headers.sh - - - name: Check copyright headers - # Strict: hard-fail (not warn+skip) if the fork point is missing - this job fetches full - # history, so a missing fork point means something is wrong rather than an expected shallow clone. - env: - COPYRIGHT_CHECK_REQUIRE_FORK_POINT: "1" - run: bash bin/check-copyright-headers.sh diff --git a/.github/workflows/dependency-audit.yml b/.github/workflows/dependency-audit.yml index acc91474e..53232ff46 100644 --- a/.github/workflows/dependency-audit.yml +++ b/.github/workflows/dependency-audit.yml @@ -143,8 +143,9 @@ jobs: restore-keys: | setup-java-Linux-x64-maven- - # Before the guard runs, prove the guard still works - the sequencing repo-hygiene.yml and - # copyright.yml both use. A guard nobody has watched fail is decoration. + # Before the guard runs, prove the guard still works - the sequencing repo-hygiene.yml uses + # (bin/check-all.sh runs every self-test ahead of the gates). A guard nobody has watched fail + # is decoration. - name: Self-test the did-it-actually-scan guard run: bash bin/test-check-ossindex-audit.sh diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index d76628b5f..eb1fe6053 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -575,49 +575,6 @@ jobs: exit "$rc" ;; esac - # Quarantine AUDIT: the seconds-fast enforcement job on every PR - registry drift and broken owner - # claims are real, actionable errors that turn it red. It does NOT run any tests (that is - # the Quarantine Lane workflow: every PR push + every merge to master + dispatch) so the PR checks list gets its verdict in seconds. - # Discipline: no quarantine without diagnosis; quarantine is master-state, not PR-state. See the - # @Quarantined javadoc (parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/Quarantined.java). - quarantine-audit: - if: github.event_name == 'pull_request' - name: "quarantine: audit" - runs-on: ubuntu-latest - timeout-minutes: 10 - steps: - - uses: actions/checkout@v6 - - name: Enforce registry (docs/quarantined-tests.md matches @Quarantined annotations) - run: bin/check-quarantine-registry.sh - - name: Verify owner claims (each quarantined test's fix PR exists, is open, and removes the quarantine) - env: - GH_TOKEN: ${{ github.token }} - run: bin/check-quarantine-owners.sh - - # Documentation-data AUDIT: structure only. docs/features/*.yaml and docs/data/*.yaml are - # hand-written and a documentation generator reads them, so a malformed file or a missing required - # field produces a broken generated page rather than a loud error. This checks that each file - # parses, declares its kind, carries what that kind requires, and points at README anchors that - # exist. It deliberately does NOT check whether the claims are true - nothing can, and a check that - # implied otherwise would be worse than none. - docs-data-audit: - if: github.event_name == 'pull_request' - name: "docs data: audit" - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: - - uses: actions/checkout@v6 - - name: Install the YAML parser the check needs - run: python3 -m pip install --quiet pyyaml - # Self-test first, same as the copyright and sigpipe scanners: this guard has twice shipped - # green on a failure it was written to catch, so the cases that pin those gaps have to run - # where a regression is visible. It mutates the corpus one way at a time and restores it, so - # the guard step below still sees an untouched tree. - - name: Self-test the guard (each case is a gap it once had) - run: bash bin/test-check-docs-data.sh - - name: Validate release-documentation data structure - run: bin/check-docs-data.sh - # Experimental Kafka 4.x compatibility check. Non-blocking. # DISABLED pending the Kafka 4.x migration work: the experimental 4.x build currently fails and just adds # a red X of noise to every PR (it is continue-on-error, so it never gated merges). Re-enable by restoring diff --git a/.github/workflows/quarantine-lane.yml b/.github/workflows/quarantine-lane.yml index cceb6e7d8..275fa4009 100644 --- a/.github/workflows/quarantine-lane.yml +++ b/.github/workflows/quarantine-lane.yml @@ -6,8 +6,9 @@ # verdict lives in the step summary), so only the fail-fast rule checks can make the job itself red. # Per-PR runs give PRE-MERGE attribution - a PR that accidentally fixes or breaks a quarantined test # learns it before merging; the push-to-master run keeps the canonical master-state record. No cron: -# the signal cannot change without a code change, and both kinds are covered. The seconds-fast -# per-PR rule enforcement is maven.yml's separate "Quarantine Audit" job. Manual: workflow_dispatch. +# the signal cannot change without a code change, and both kinds are covered. The rule checks also +# run, unauthenticated, in repo-hygiene.yml's `repo: hygiene` sweep; the authenticated owner-claim +# verification lives here, in the fail-fast steps below. Manual: workflow_dispatch. name: Quarantine Lane @@ -45,8 +46,8 @@ jobs: # and owner-claim checks, and re-executes the lane against that code. # # Red inside the run step is EXPECTED while the owning fix PR is open; the job itself never goes red - # for test failures (step-level continue-on-error) - only audit-class errors would, and those live in - # quarantine-audit. So a GREEN tick here does NOT mean the quarantined tests passed: the verdict is in + # for test failures (step-level continue-on-error) - only audit-class errors would, from the + # fail-fast rule checks below. So a GREEN tick here does NOT mean the quarantined tests passed: the verdict is in # the step summary and the sticky PR comment, not the check mark. test-quarantined: name: "tests" # renders as "Quarantine Lane / tests" - the workflow name carries the identity @@ -60,8 +61,11 @@ jobs: # of this branch's registry check, which shipped once without a test to hold them. - name: Self-test the quarantine scan run: bash bin/test-check-quarantine-registry.sh - # Fail fast on rule violations BEFORE spending a test run - same checks as quarantine-audit, - # here validating master's state. + # Fail fast on rule violations BEFORE spending a test run. The registry check also runs in the + # `repo: hygiene` sweep; the owner check needs the token below, which that lane deliberately + # holds none of, so THIS is where an orphaned or overdue owner claim goes red on a required + # check (`tests`). The dedicated per-PR "quarantine: audit" job that used to duplicate both + # was retired - docs/inflight/ci-fewer-jobs-ruleset-edits.md. - name: Enforce registry (docs/quarantined-tests.md matches @Quarantined annotations) run: bin/check-quarantine-registry.sh - name: Verify owner claims diff --git a/.github/workflows/repo-hygiene.yml b/.github/workflows/repo-hygiene.yml index d1974bb8b..16493eb02 100644 --- a/.github/workflows/repo-hygiene.yml +++ b/.github/workflows/repo-hygiene.yml @@ -1,9 +1,18 @@ # Copyright (C) 2026 Antony Stubbs and contributors -# Cheap, repo-wide static checks: things that need no JDK, no broker and no build. +# Cheap, repo-wide static checks: things that need no broker and no build. # -# Separate from copyright.yml deliberately: these checks are not about copyright and never were, -# and bolting them there left a job called "Copyright header check" doing two unrelated things. +# THREE STANDALONE GATE JOBS WERE FOLDED IN HERE, and each was already running here before it was +# deleted: `Copyright header check` (the whole of copyright.yml), and maven.yml's `quarantine: audit` +# and `docs data: audit`. The sweep below globs bin/check-*.sh, so check-copyright-headers.sh, +# check-quarantine-registry.sh, check-quarantine-owners.sh and check-docs-data.sh - and each one's +# self-test - ran in this job on every PR alongside the dedicated jobs, which were a second copy with +# a second checkout each. The one thing each dedicated job carried that this lane did not is now an +# explicit step or env line below (the fork-point strictness, the PyYAML check), so nothing was lost +# in the fold; and the quarantine OWNER check keeps its authenticated run in quarantine-lane.yml, +# whose `tests` check is required, because this lane deliberately holds no token (see PR_NUMBER). +# The three retired names are required contexts in the master ruleset until it is edited - +# docs/inflight/ci-fewer-jobs-ruleset-edits.md carries the edit and its ordering. # # ONE LANE, DISCOVERED - and this replaced one-job-per-concern deliberately. That shape gave each # gate its own required-status-check candidate and its own red tick, which reads well, and cost @@ -32,8 +41,9 @@ name: Repo Hygiene -# push scoped to master (matching copyright.yml and maven.yml) so PR-branch commits run once via -# the pull_request event instead of twice. +# push scoped to master (matching maven.yml) so PR-branch commits run once via the pull_request +# event instead of twice. copyright.yml ran on push to master too, so its fold into this workflow +# keeps that cadence. on: push: branches: [ master ] @@ -64,12 +74,17 @@ jobs: # already carries the number, so this costs nothing and needs no token. env: PR_NUMBER: ${{ github.event.pull_request.number }} + # The checkout below fetches full history, so a missing fork point here is a broken checkout, + # not an expected shallow clone: make bin/check-copyright-headers.sh hard-fail (exit 2) on it + # instead of its default warn-and-skip exit 0, which the sweep would count as a pass. + COPYRIGHT_CHECK_REQUIRE_FORK_POINT: "1" steps: - uses: actions/checkout@v6 with: # Gates that read history must not see a truncated graft. A shallow clone does not error # on `git log` or `git merge-base` - it answers, wrongly, which is the exact class - # .claude/hooks/check-shallow-history.sh exists to stop locally. + # .claude/hooks/check-shallow-history.sh exists to stop locally. The copyright scanner + # also classifies provenance from the fork-point commit, which only full history holds. fetch-depth: 0 # ubuntu-latest ships shellcheck, but an image change that removed it must fail rather than @@ -79,6 +94,12 @@ jobs: - name: Confirm shellcheck is present run: shellcheck --version + # Same shape for PyYAML: bin/check-docs-data.sh fails CLOSED without it, exit 2 "cannot run". + # ubuntu-latest ships it; the dedicated docs-data job used to pip-install it explicitly, and + # this is that dependency made visible rather than inherited from the image. + - name: Confirm PyYAML is present + run: python3 -c 'import yaml; print("PyYAML", yaml.__version__)' + # A JDK IS REQUIRED, and the requirement is not obvious from the job name. The self-test # `bin/test-check-infer.sh` FAILS rather than skips when no JDK resolves and `CI` is set - by # design, since on a hosted runner an unresolvable JDK means the fixture is wrong, not that the @@ -91,8 +112,12 @@ jobs: java-version: '17' # No `cache: maven` - see the caching strategy note at the top of maven.yml + # --strict: a CANNOT (exit 2) fails the sweep here. Without it check-all exits 0 with a CANNOT + # as long as something else ran - right for a laptop with no `gh` credential, wrong on a hosted + # image where a CANNOT is only ever a missing tool or a shallow checkout. The two "Confirm ..." + # steps above name the dependencies; this is what turns a third one nobody named into a red. - name: Every gate and every self-test, discovered rather than listed - run: bash bin/check-all.sh --with-tests + run: bash bin/check-all.sh --with-tests --strict # The sweep above deliberately does NOT reach Codecov: check-integration-shard-balance.mjs # exits 3 unless SHARD_BALANCE_NETWORK is set, because `check-*` is blanket-granted to the PR diff --git a/bin/check-all.sh b/bin/check-all.sh index d05ca1425..89d90673a 100755 --- a/bin/check-all.sh +++ b/bin/check-all.sh @@ -28,13 +28,28 @@ # exactly what this script exists to abolish, every name in it is ASSERTED TO EXIST: rename one of # these scripts and this runner fails loudly instead of quietly checking less than you think. # -# Exit codes: 0 everything that could run passed, 1 at least one FAILED, 2 nothing ran at all. +# Exit codes: 0 everything that could run passed, 1 at least one FAILED (under --strict: or at least +# one CANNOT), 2 nothing ran at all. # # Usage: # bin/check-all.sh # the tree gates - what to run before a push # bin/check-all.sh --pr # also the PR-state reporters (merge prep) -# bin/check-all.sh --with-tests # also the self-tests (what CI runs) +# bin/check-all.sh --with-tests # also the self-tests # bin/check-all.sh --tests-only # only the self-tests +# bin/check-all.sh --strict # CANNOT (exit 2) fails the sweep - what CI runs, with --with-tests +# +# WHY CI RUNS --strict, AND THE DEFAULT DOES NOT. By default a CANNOT is reported in its own column +# and does not flip the exit code, because on a laptop it is usually legitimate: no `gh` credential, +# no network, ShellCheck not installed - and the header line below says CI is the first place that +# gate actually runs. That sentence is only true if CI cannot ALSO land in CANNOT and stay green. +# It could: the sweep exits 0 with a CANNOT as long as something else ran, so a hosted image that +# stopped shipping PyYAML or ShellCheck would turn check-docs-data.sh or check-shell-lint.sh into a +# CANNOT that nothing reads - a false green, and the exact shape this script exists to prevent. On a +# hosted runner a CANNOT is never a legitimate skip: it is a tool missing from the image, or a fork +# point missing from a shallow checkout, and the remedy is to install it in the job, not to pass. +# So repo-hygiene.yml runs `--with-tests --strict`, and its explicit "Confirm X is present" steps +# name each dependency at the point a reader looks. Exit 3, "nothing in scope", is untouched by +# --strict: a gate that measured and found nothing to measure is a result, not a missing tool. # # THE DEFAULT IS GATES ONLY, AND THAT IS THE POINT. The gates answer "is my tree healthy" in about # 25 seconds. The self-tests answer "do the gates themselves still work", take minutes - two of them @@ -42,7 +57,7 @@ # them made the routine command slow enough to skip, and a pre-push sweep that gets skipped protects # nothing, which is the same failure `check-all` was written to fix. # -# NOTHING IS LOST IN CI: .github/workflows/repo-hygiene.yml runs `--with-tests`, so the self-tests +# NOTHING IS LOST IN CI: .github/workflows/repo-hygiene.yml runs `--with-tests --strict`, so the self-tests # run there in one go. It no longer NAMES them - that workflow used to carry a job per self-test, and # a self-test added to bin/ ran nowhere until somebody remembered to wire it. So a self-test added # tomorrow is swept with no edit there and no edit here, which is the same discovery property the top @@ -54,10 +69,12 @@ cd "$(dirname "$0")/.." MODE=gates WITH_PR=0 +STRICT=0 for arg in "$@"; do case "$arg" in --pr) WITH_PR=1 ;; --with-tests) MODE=all ;; + --strict) STRICT=1 ;; # Kept because scripts and habits name it; it is now what happens anyway. --gates-only) MODE=gates ;; --tests-only) MODE=tests ;; @@ -316,12 +333,22 @@ printf 'check-all: %s ran - %s passed, %s failed, %s could not run, %s nothing-i if [ "$cannot" -gt 0 ]; then printf 'check-all: COULD NOT RUN:%s\n' "$cannot_names" - echo "check-all: these measured nothing. Usually a missing tool or credential - fix it, or" - echo "check-all: know that CI is the first place that gate actually runs." + if [ "$STRICT" -eq 1 ]; then + echo "check-all: STRICT: a gate that could not run is a FAILURE here. On a CI runner this is a" >&2 + echo "check-all: missing tool or credential on the image, never a legitimate skip - install it in" >&2 + echo "check-all: the job (see the 'Confirm ... is present' steps in repo-hygiene.yml)." >&2 + else + echo "check-all: these measured nothing. Usually a missing tool or credential - fix it, or" + echo "check-all: know that CI is the first place that gate actually runs (and it runs --strict)." + fi fi if [ "$fail" -gt 0 ]; then printf 'check-all: FAILED:%s\n' "$failed_names" >&2 exit 1 fi +if [ "$STRICT" -eq 1 ] && [ "$cannot" -gt 0 ]; then + printf 'check-all: FAILED (strict - could not run):%s\n' "$cannot_names" >&2 + exit 1 +fi echo "check-all: no gate failed." exit 0 diff --git a/docs/ci.md b/docs/ci.md index 323a54628..39119e544 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -70,7 +70,7 @@ document. This section is the detail behind it. **gating**, like the job they replaced: a chaos RED is a real finding. The **`Integration Tests`** lane is likewise two gating shards since astubbs#442 - a named heavy set and a catch-all defined by subtraction; see - ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries the seconds-fast Quarantine Audit job, SpotBugs, duplicate + ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries SpotBugs, duplicate detection, PR-scoped mutation testing (PIT), and dependency vulnerability scanning. Push to master runs a single full `bin/ci-build.sh` on the default Kafka version to gate SNAPSHOT publishing. All jobs use explicit `cache/restore` with rotating keys from the `prepare-deps` @@ -78,10 +78,6 @@ document. This section is the detail behind it. - **`publish.yml`** - publishes to Maven Central on every push to `master`. The pom version is the source of truth: `-SNAPSHOT` versions deploy as snapshots, non-snapshot versions deploy as full releases (and create a git tag + GitHub release). See [`docs/releasing.md`](releasing.md). -- **`copyright.yml`** - header conformance via `bin/check-copyright-headers.sh` (its self-test - `bin/test-check-copyright-headers.sh` runs first, then the real scan) on every push/PR. - GitHub-hosted; needs `fetch-depth: 0` so the fork-point commit is in history. Rules: - [`docs/copyright.md`](copyright.md). - **`quarantine-lane.yml`** - runs the `@Quarantined` tests on every PR push, every push to master, and on dispatch. Its job is the **required** check `tests`, so the job name is an API here too - but the test-running step is `continue-on-error`, so red quarantined tests cannot block a merge. @@ -110,12 +106,32 @@ document. This section is the detail behind it. of order. - **`repo-hygiene.yml`** - cheap repo-wide static checks needing no broker, no Docker and no build. **ONE job, `repo: hygiene`, which DISCOVERS rather than enumerates**: it runs - `bin/check-all.sh --with-tests`, globbing `bin/check-*.sh` and `bin/test-*.sh`. It was one job per - concern until that shape cost more than it bought - a gate added to `bin/` ran NOWHERE until + `bin/check-all.sh --with-tests --strict`, globbing `bin/check-*.sh` and `bin/test-*.sh`. It was one + job per concern until that shape cost more than it bought - a gate added to `bin/` ran NOWHERE until somebody remembered to name it here, and nothing went red, because a check that is not running looks exactly like a check that is passing. The job name is still an API: it is a required status check, and renaming it silently stops satisfying the ruleset. + **`--strict` makes a CANNOT fail the lane.** By default `check-all.sh` reports a gate that exits 2 + ("cannot run") in its own column and still exits 0, which is right on a laptop with no `gh` + credential and wrong on a hosted image, where a CANNOT is only ever a missing tool or a shallow + checkout. Before the flag, an image that stopped shipping PyYAML would have turned + `check-docs-data.sh` into a CANNOT the lane read as green. The job also names each such + dependency in a `Confirm ... is present` step, so the log says which one went missing. + + **Three standalone jobs were folded in here on 2026-09-07** - `copyright.yml`'s `Copyright header + check`, and `maven.yml`'s `quarantine: audit` and `docs data: audit` - because the sweep's glob + was already running `check-copyright-headers.sh`, `check-quarantine-registry.sh`, + `check-quarantine-owners.sh` and `check-docs-data.sh` (each with its self-test) on every PR, so + the dedicated jobs were a second copy with a second checkout each. What each carried that the lane + did not is now explicit in the job: `COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1` (the scanner's default + on a missing fork point is warn-and-skip, exit 0), and the PyYAML assertion. The lane holds no + token, so `check-quarantine-owners.sh` verifies owner claims only where `gh` is authenticated - + `quarantine-lane.yml`, whose required `tests` check runs it with `github.token` on every PR push. + Copyright rules: [`docs/copyright.md`](copyright.md); the ruleset still names the three retired + contexts until it is edited - see + [`docs/inflight/ci-fewer-jobs-ruleset-edits.md`](inflight/ci-fewer-jobs-ruleset-edits.md). + What the lane covers, and why each one is not obvious: - **`check-source-patterns.mjs`**, rule `sigpipe-into-grep-q`, fails any script piping into @@ -150,12 +166,13 @@ document. This section is the detail behind it. (they can never block anything) and additions wait until the job exists on master - which is why `shell: sigpipe` and `workflows: action versions` were dropped from the ruleset in the same change that deleted those jobs. **Neither name exists any more, and neither is required.** `repo: hygiene`, - the single lane that replaced them (and the rest of `repo-hygiene.yml`'s old per-concern jobs), is - **also absent from the required list** as of the last live check - - `gh api repos/astubbs/parallel-consumer/rulesets/15055005` enumerates every required context by - name and `repo: hygiene` is not among them. Whether that is the pending "addition" this paragraph - describes, still waiting for its turn, or simply missed when the jobs were collapsed, is not - settled here - confirm against the live ruleset rather than assuming either. + the single lane that replaced them (and the rest of `repo-hygiene.yml`'s old per-concern jobs), + **is in the required list** as of the live check on 2026-09-07 - + `gh api repos/astubbs/parallel-consumer/rules/branches/master` enumerates every required context + by name. The three contexts retired into it that day - `Copyright header check`, + `quarantine: audit`, `docs data: audit` - are the removals currently owed to the ruleset; + [`docs/inflight/ci-fewer-jobs-ruleset-edits.md`](inflight/ci-fewer-jobs-ruleset-edits.md) owns + that edit. Confirm against the live ruleset rather than assuming this paragraph is current. - `cve-exclusions` runs `bin/check-cve-exclusions.sh`, which **expires temporary CVE exclusions**. Entries in the root pom's `excludeVulnerabilityIds` come in two kinds: *standing* (retiring them needs someone else to act, on no timetable we control) and *temporary* (the @@ -263,7 +280,11 @@ renamed job: the ruleset keeps the old name, which then blocks nothing visibly a is how a bare `spotbugs` context outlived the job that became `static: spotbugs` and sat required with no producer until 2026-08-26. **A skip does not satisfy a required check either** - it waits - so a job that can legitimately have nothing in scope should report success rather than skip before anyone -requires it. +requires it. **Removals are the other half of the same ordering**: a job deleted from the tree leaves +its context in the ruleset, required and never produced, so every PR pends until the ruleset drops +it - the edit belongs to the merge of the PR that deletes the job, not before (the job still runs on +every other PR) and not after (nothing merges). The live instance of this is +[`docs/inflight/ci-fewer-jobs-ruleset-edits.md`](inflight/ci-fewer-jobs-ruleset-edits.md). **These are deliberately NOT required, and each would break something if promoted:** diff --git a/docs/copyright.md b/docs/copyright.md index 159a5a96a..50eeed25e 100644 --- a/docs/copyright.md +++ b/docs/copyright.md @@ -68,8 +68,8 @@ worktrees. rot, and it counts what it grandfathered rather than skipping quietly. **The limit matters as much as the rule**: a file that *was* marked and has since lost its header is still a violation. - **A Confluent claim is `Copyright (C) ... Confluent` on ONE line.** A fork-original file may - discuss the fork's provenance in its header prose - `.github/workflows/copyright.yml` and - `bin/deps-version-rules.xml` both do - without that reading as a claim. + discuss the fork's provenance in its header prose - `bin/deps-version-rules.xml` does, and so did + the copyright workflow before it was folded into `repo-hygiene.yml` - without that reading as a claim. - **A header that is present can still be wrong**, and the scanner checks all three cases: the notice must sit inside a comment in that language's syntax, never above a `#!` shebang, and never above an `` declaration. Note also that an XML comment may not contain `--`, so the diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md new file mode 100644 index 000000000..dcb07173b --- /dev/null +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -0,0 +1,59 @@ +# Ruleset edits owed by the fold of three gate jobs into `repo: hygiene` + + + + +Three standalone jobs were deleted because `repo: hygiene` already ran every gate they ran, via +`bin/check-all.sh --with-tests`'s glob, on every PR: `Copyright header check` (the whole of +`.github/workflows/copyright.yml`), and `.github/workflows/maven.yml`'s `quarantine: audit` and +`docs data: audit`. Their names are still **required status-check contexts in the master ruleset**, +and the ruleset is repository settings, not tree state - no PR can change it +([`docs/ci.md`](../ci.md), "The required list is repository settings, not tree state"). + + +## The edit + +Remove these three contexts from the master ruleset's `required_status_checks`: + +- `Copyright header check` +- `quarantine: audit` +- `docs data: audit` + +Nothing to add: `repo: hygiene` is already in the required list (verified live 2026-09-07 with +`gh api repos/astubbs/parallel-consumer/rules/branches/master`; that command is the answer, not this +sentence). + +## When: at the merge of the deleting PR, not before and not after + +A required context nothing produces leaves every PR **pending** - it never fails, it never passes. + +- **Before the merge** the three jobs still run on every other open PR, so dropping the contexts + early only widens the window in which a broken header or a drifted registry could merge on a PR + opened before the fold. +- **After the merge** no run produces them, so every PR in the repository pends until somebody edits + the ruleset. That is the `spotbugs` incident again ([`docs/ci.md`](../ci.md), "Which checks are + required"). + +So: edit the ruleset in the same sitting as the merge. Only one merge is exposed either way - the +deleting PR's own checks list shows the three contexts as expected-but-missing until the ruleset +drops them, which is the intended tell that the edit is still owed, not a fault in that PR. + +## What was checked before deleting, so nobody re-derives it + +- The log of the latest successful `Repo Hygiene` run on master showed `check-copyright-headers.sh`, + `check-quarantine-registry.sh`, `check-quarantine-owners.sh` and `check-docs-data.sh` all `ok` + (exit 0), and all four self-tests `ok`, with zero CANNOT - the gates were genuinely running there, + not skipping. `bin/inflight.mjs` cannot answer this; the run log is the only record. +- `copyright.yml` also ran on `push` to master. `repo-hygiene.yml` runs on push to master too, so + that cadence is kept. +- The quarantine **owner** check needs an authenticated `gh`; `repo: hygiene` deliberately holds no + token, so there it degrades to advisory. The authenticated run is `quarantine-lane.yml`'s + fail-fast step, with `github.token`, on every PR push and every master push, under the required + `tests` check - so an orphaned or overdue owner claim still reds a required check. +- `check-all.sh` used to exit 0 on a CANNOT (exit 2) as long as something else ran. The hygiene job + now runs `--strict`, under which a CANNOT fails the sweep, and asserts PyYAML and shellcheck are + present in named steps - so the folded gates cannot silently turn into skips on an image change. + +This note tracks only the owed edit. Once the live ruleset no longer lists the three contexts, +nothing here is both true and unowned elsewhere - the reasoning is in [`docs/ci.md`](../ci.md) and +the `repo-hygiene.yml` header. diff --git a/docs/inflight/ci-path-filters-for-docs-only-prs.md b/docs/inflight/ci-path-filters-for-docs-only-prs.md index 347a5418b..3e9668c4f 100644 --- a/docs/inflight/ci-path-filters-for-docs-only-prs.md +++ b/docs/inflight/ci-path-filters-for-docs-only-prs.md @@ -37,8 +37,8 @@ it. The test matrix is not an obstacle - all three suites are one `test` job wit It has to be an allowlist, because several "docs" paths are load-bearing: -- [`docs/quarantined-tests.md`](../quarantined-tests.md) is validated against Java annotations by the - `quarantine: audit` check. +- [`docs/quarantined-tests.md`](../quarantined-tests.md) is validated against Java annotations by + `bin/check-quarantine-registry.sh`, in the `repo: hygiene` sweep and the Quarantine Lane. - [`docs/todo-index.md`](../todo-index.md) is generated, and `--check` fails when it is stale. - `AGENTS.md`, `.github/` and `bin/` are neither code nor docs, and changing them can change what CI itself does. diff --git a/docs/solutions/workflow-issues/copyright-header-rules-for-fork-2026-04-21.md b/docs/solutions/workflow-issues/copyright-header-rules-for-fork-2026-04-21.md index 5bac8e9a6..0f32490fe 100644 --- a/docs/solutions/workflow-issues/copyright-header-rules-for-fork-2026-04-21.md +++ b/docs/solutions/workflow-issues/copyright-header-rules-for-fork-2026-04-21.md @@ -144,6 +144,6 @@ The commit only added a null-epoch guard. The range `2020-2022` was correct. The - [`docs/copyright.md`](../../copyright.md) - the codified rules (authoritative source) - `bin/check-copyright-headers.sh` + `bin/test-check-copyright-headers.sh` - the enforcement and its self-tests (PR astubbs#90) -- `.github/workflows/copyright.yml` - CI enforcement +- CI enforcement - a dedicated copyright workflow when this was written, since folded into the `repo: hygiene` sweep (2026-09-07); the file as this document read it: `git show c668acbfa:.github/workflows/copyright.yml` - `NOTICE` file at repo root - legal attribution structure - `pom.xml` - binds the scanner to the `validate` phase behind ``; the mycila license plugin it used to configure was removed in PR astubbs#124 From 1c1e76ff4f5eacebaa957800d5f67f5eefe4be7a Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 11:58:46 +1200 Subject: [PATCH 02/14] ci(fewer-jobs): fold the PR Checklist job into repo: hygiene `PR Checklist` (pr-checklist.yml, deleted) ran eleven steps in its own job on every PR push: three bin/test-* self-tests that the hygiene sweep already runs, a Node compile-check, the gate-module unit tests, the marker-index check, and five github-script gates that read the PR body and diff. All of them now run as steps of `repo: hygiene`. Second cut of the ce-optimize run `ci-fewer-jobs`: 24 -> 23 jobs. What the fold had to preserve: - Triggers are the union. The checklist ran on `edited` because its gates read the PR body; hygiene now does too, so a body edit still re-evaluates them (and re-runs the macOS lane, an accepted cost). - A concurrency group per PR, keyed on the PR number or the sha so master pushes never cancel each other. - Read-only token. Hygiene held no token by design; the checklist needs contents+pull-requests read. No write scope was added - the sweep runs PR-authored scripts, and the reviewer-isolation rule in docs/ci.md forbids a write grant beside them. - persist-credentials: false on the checkout. Nothing in the sweep uses the checkout credential; gh reads GH_TOKEN and the one fetch is anonymous against a public remote. - The github-script gates are guarded to pull_request events; the tree-only steps also run on master pushes, which is more coverage. Why hygiene and not the other PR-metadata job: `Check PR Dependencies` needs checks: write and a `closed` trigger to unblock child PRs when a parent merges, and PR-authored steps must not sit beside a write grant. The two review-gate jobs each produce a required check with its own semantics. Recorded in docs/inflight/ci-fewer-jobs-ruleset-edits.md, which now lists four required contexts to remove at merge time. The old job name was cited as the authoritative gate across AGENTS.md, bin/, docs/ and the PR template; those live references now name `repo: hygiene`, and the two dated records that cited the deleted file carry a history pointer per docs/citations.md. Co-authored-by: Claude Fable 5.1 --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/scripts/changelog-ref-gate.js | 7 +- .github/scripts/changelog-ref-gate.test.js | 2 +- .github/scripts/file-ref-gate.js | 4 +- .github/scripts/file-ref-gate.test.js | 2 +- .github/scripts/issue-ref-gate.js | 2 +- .github/scripts/issue-ref-gate.test.js | 2 +- .github/scripts/roadmap-stage-gate.test.js | 2 +- .github/scripts/sticky-report-comment.js | 4 +- .github/workflows/README.md | 3 +- .github/workflows/maven.yml | 6 +- .github/workflows/pr-checklist.yml | 430 ---------------- .github/workflows/repo-hygiene.yml | 465 +++++++++++++++++- AGENTS.md | 6 +- bin/AGENTS.md | 8 +- bin/check-file-refs.sh | 2 +- bin/check-issue-refs.sh | 2 +- bin/lib/node-gate.sh | 6 +- docs/ci.md | 48 +- docs/citations.md | 6 +- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 55 ++- docs/issue-references.md | 2 +- docs/refactoring.md | 2 +- docs/releasing.md | 2 +- ...embedded-in-another-language-2026-08-19.md | 6 +- ...e-truncated-the-shared-clone-2026-08-26.md | 9 +- 26 files changed, 575 insertions(+), 510 deletions(-) delete mode 100644 .github/workflows/pr-checklist.yml diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 1f7c023a1..973d5ed9f 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -16,7 +16,7 @@ ## Checklist - diff --git a/.github/scripts/changelog-ref-gate.js b/.github/scripts/changelog-ref-gate.js index 80347c341..bab4af549 100644 --- a/.github/scripts/changelog-ref-gate.js +++ b/.github/scripts/changelog-ref-gate.js @@ -1,9 +1,10 @@ // Copyright (C) 2026 Antony Stubbs and contributors // Pure logic behind the "Verify new changelog entries cite an issue" step in -// .github/workflows/pr-checklist.yml. It lives here rather than inline in the workflow YAML so it -// can be unit tested - the same job runs changelog-ref-gate.test.js before the gate, so a -// regression here fails the PR Checklist rather than silently misjudging changelogs. +// .github/workflows/repo-hygiene.yml (the `repo: hygiene` job, where the former PR Checklist steps +// now live). It lives here rather than inline in the workflow YAML so it can be unit tested - the +// same job runs changelog-ref-gate.test.js before the gate, so a regression here fails the required +// check rather than silently misjudging changelogs. // // This is a reminder for ourselves, not a defence against someone gaming it. Where being exactly // right would need real cleverness, it takes the simple option and says so: a check nobody can diff --git a/.github/scripts/changelog-ref-gate.test.js b/.github/scripts/changelog-ref-gate.test.js index e9b76a798..a8200c024 100644 --- a/.github/scripts/changelog-ref-gate.test.js +++ b/.github/scripts/changelog-ref-gate.test.js @@ -1,6 +1,6 @@ // Copyright (C) 2026 Antony Stubbs and contributors -// Unit tests for changelog-ref-gate.js, run by the PR Checklist job before the gate itself. +// Unit tests for changelog-ref-gate.js, run by the `repo: hygiene` job before the gate itself. // Plain node, no dependencies, no runner: `node .github/scripts/changelog-ref-gate.test.js`. // Exits non-zero on the first sign of trouble, so a broken gate fails CI loudly. diff --git a/.github/scripts/file-ref-gate.js b/.github/scripts/file-ref-gate.js index 228eb0925..7c1aff192 100644 --- a/.github/scripts/file-ref-gate.js +++ b/.github/scripts/file-ref-gate.js @@ -259,7 +259,7 @@ function citationsIn(line) { /** * The oracle resolves() reads, built from `git ls-files`. It lives here because BOTH callers need - * one - the CI job in pr-checklist.yml and bin/check-file-refs.sh - and two hand-copied versions are + * one - the CI gate in repo-hygiene.yml and bin/check-file-refs.sh - and two hand-copied versions are * how they would come to disagree about what exists, which is the single thing they cannot differ * on. Same reasoning that puts formatFailure here rather than at each call site. * @@ -474,7 +474,7 @@ function newFindings(current, base) { /** * The single copy of what an author is told when either rule fires - rendered by both callers, the - * CI job in pr-checklist.yml and the local bin/check-file-refs.sh, so the two cannot tell different + * CI gate in repo-hygiene.yml and the local bin/check-file-refs.sh, so the two cannot tell different * stories. Its sibling gate learned that the hard way: hand-written copies of one message disagreed * in both directions within hours of the second being written. */ diff --git a/.github/scripts/file-ref-gate.test.js b/.github/scripts/file-ref-gate.test.js index 838fd10a9..35df6fa98 100644 --- a/.github/scripts/file-ref-gate.test.js +++ b/.github/scripts/file-ref-gate.test.js @@ -1,6 +1,6 @@ // Copyright (C) 2026 Antony Stubbs and contributors -// Unit tests for file-ref-gate.js. Run by the PR Checklist job before the gate itself, so a broken +// Unit tests for file-ref-gate.js. Run by the `repo: hygiene` job before the gate itself, so a broken // rule fails loudly rather than silently passing - or failing - every PR. // // The cases that matter most here are the NEGATIVE ones. This gate's failure mode is not missing a diff --git a/.github/scripts/issue-ref-gate.js b/.github/scripts/issue-ref-gate.js index b7547b934..c0eda876c 100644 --- a/.github/scripts/issue-ref-gate.js +++ b/.github/scripts/issue-ref-gate.js @@ -296,7 +296,7 @@ function prBodyEntry(body) { /** * The single copy of what an author is told when the gate fires. Both callers render this - the CI - * job in pr-checklist.yml and the local bin/check-issue-refs.sh - so the two cannot tell different + * gate in repo-hygiene.yml and the local bin/check-issue-refs.sh - so the two cannot tell different * stories. They did exactly that once: hand-written copies disagreed in *both* directions within * hours of the second one being created, each carrying a correction the other lacked. The script's * own header already says "NO SECOND COPY OF THE RULE" about the matching logic; the message an diff --git a/.github/scripts/issue-ref-gate.test.js b/.github/scripts/issue-ref-gate.test.js index 6e2dc83dd..2f298008b 100644 --- a/.github/scripts/issue-ref-gate.test.js +++ b/.github/scripts/issue-ref-gate.test.js @@ -1,6 +1,6 @@ // Copyright (C) 2026 Antony Stubbs and contributors -// Unit tests for issue-ref-gate.js. Run by the PR Checklist job before the gate itself, so a +// Unit tests for issue-ref-gate.js. Run by the `repo: hygiene` job before the gate itself, so a // broken rule fails loudly rather than silently passing - or failing - every PR. const assert = require("assert"); const { diff --git a/.github/scripts/roadmap-stage-gate.test.js b/.github/scripts/roadmap-stage-gate.test.js index 51f430eb0..090c27a2b 100644 --- a/.github/scripts/roadmap-stage-gate.test.js +++ b/.github/scripts/roadmap-stage-gate.test.js @@ -1,6 +1,6 @@ // Copyright (C) 2026 Antony Stubbs and contributors -// Unit tests for roadmap-stage-gate.js. Run by the PR Checklist job before the gate itself, so a +// Unit tests for roadmap-stage-gate.js. Run by the `repo: hygiene` job before the gate itself, so a // broken rule fails loudly rather than silently passing - or failing - every PR. // issue-refs: exempt-file - fixtures and check names are deliberately full of bare PR numbers; // qualifying them would stop testing what the parser actually sees. diff --git a/.github/scripts/sticky-report-comment.js b/.github/scripts/sticky-report-comment.js index f36e27a3f..ed944ed56 100644 --- a/.github/scripts/sticky-report-comment.js +++ b/.github/scripts/sticky-report-comment.js @@ -25,8 +25,8 @@ // const sticky = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/sticky-report-comment.js`) // // `actions/github-script` runs with the workspace checked out, so `require` of a repo file works and -// is already the convention here (see .github/workflows/pr-checklist.yml, which requires four gate -// modules this way). Tests: .github/scripts/sticky-report-comment.test.js. +// is already the convention here (see .github/workflows/repo-hygiene.yml, whose PR-body gates require four +// gate modules this way). Tests: .github/scripts/sticky-report-comment.test.js. "use strict"; diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 74f615dc7..406c271c9 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -28,11 +28,10 @@ procedure or a steer. Why, and the gate's exact contract: [`docs/ci.md`](../../d | `dependency-audit.yml` | Scans the **whole** resolved dependency tree for CVEs - the only place OSS Index is switched on. Per-PR, on demand, and weekly, because a new advisory needs no push to arrive. | | `maven.yml` | The main build: unit, integration and performance suites, SpotBugs, duplicate detection, PIT, dependency scanning. | | `mutation-full-sweep.yml` | The whole-repo PIT mutation sweep - nightly, plus on demand. Too slow for a PR. Self-hosted high-CPU lane, plus a hosted trial arm. | -| `pr-checklist.yml` | Makes the PR template binding: the checklist must be present and every box resolved. | | `publish.yml` | Publishes to Maven Central on every push to `master`; the pom version decides snapshot or release. | | `quarantine-lane.yml` | Runs the quarantined tests separately, so known-flaky tests neither block nor disappear. | | `release.yml` | Cuts a release. `workflow_dispatch`, and deliberately the most dangerous button here. | -| `repo-hygiene.yml` | Always-on repo checks, every `bin/check-*` gate and self-test by glob - copyright headers against the fork policy, the quarantine registry, the release-documentation data, shell hazards, one pinned version per GitHub Action, expiring the pom's temporary CVE exclusions - plus `shell: macos`, the one lane not on `ubuntu-latest`: it runs the shell self-tests and a `bash -n` sweep against Apple's bash 3.2, where GNU-only constructs fail silently. | +| `repo-hygiene.yml` | Always-on repo checks, every `bin/check-*` gate and self-test by glob - copyright headers against the fork policy, the quarantine registry, the release-documentation data, shell hazards, one pinned version per GitHub Action, expiring the pom's temporary CVE exclusions - and the PR-body gates that were `pr-checklist.yml`: the template checklist must be present and every box resolved, issue and file references must resolve, a changelog entry cites an issue, a roadmap-carrying PR moves its stage - plus `shell: macos`, the one lane not on `ubuntu-latest`: it runs the shell self-tests and a `bash -n` sweep against Apple's bash 3.2, where GNU-only constructs fail silently. | ## One required check is not in this directory diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index eb1fe6053..c4ded6ef4 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -336,7 +336,7 @@ jobs: // owns the five behaviours every report needs: paginated Bot-only lookup of our own last // comment, reading its payload back, update-in-place versus post-fresh on a status change, // retire-before-create, and the sha/run/time stamp. Both modules carry the reasoning and - // the measurements behind each, and both have self-tests the PR Checklist job runs. + // the measurements behind each, and both have self-tests the `repo: hygiene` job runs. // // They moved out of this YAML when the quarantine lane needed the identical behaviours. // Copying them is precisely how the original defects reached two steps at once - the @@ -345,7 +345,7 @@ jobs: // which is the second half of the argument. // // `require` of a repo file works because github-script runs with the workspace checked - // out; .github/workflows/pr-checklist.yml loads its gate modules the same way. + // out; .github/workflows/repo-hygiene.yml loads its gate modules the same way. const report = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/throughput-report-comment.js`) await report.post({ github, context, core, body }) @@ -556,7 +556,7 @@ jobs: restore-keys: | setup-java-Linux-x64-maven- - name: Self-test the mutation verdict - # Ahead of the lane it protects, the same ordering repo-hygiene.yml and pr-checklist.yml use. + # Ahead of the lane it protects, the same ordering repo-hygiene.yml uses. # Seconds, no maven: it feeds canned PIT logs through the verdict and asserts the exit codes. run: bash bin/test-ci-mutation-test.sh diff --git a/.github/workflows/pr-checklist.yml b/.github/workflows/pr-checklist.yml deleted file mode 100644 index 35fbd6ccd..000000000 --- a/.github/workflows/pr-checklist.yml +++ /dev/null @@ -1,430 +0,0 @@ -# Copyright (C) 2026 Antony Stubbs and contributors - -# PR Checklist gate - makes .github/PULL_REQUEST_TEMPLATE.md's checklist actually binding. -# -# The template is only a passive nudge (and `gh --body-file` PRs skip it entirely), so this check -# enforces it two ways for human-authored PRs: -# 1. The checklist must be PRESENT - a human PR with zero task-list items fails (otherwise dropping -# the template is a silent bypass: no boxes -> nothing to be "unchecked" -> vacuous pass). -# 2. Every box present must be resolved - ticked `[x]`, or left `- [ ] ... N/A - `. -# Real bot PRs (Dependabot/Renovate etc., GitHub user type "Bot") don't use the template and are -# exempt entirely. Re-runs on body edits. -# -# It then enforces three citation conventions on top - the changelog's issue references, issue -# references anywhere in the diff or the PR body, and the existence of cited file paths. Each one's -# logic lives in its own .github/scripts/*-ref-gate.js module so it can be unit tested; this job runs -# every one of those test suites before the gates themselves. - -name: PR Checklist - -on: - pull_request: - types: [opened, edited, synchronize, reopened] - -permissions: - contents: read - pull-requests: read # listFiles, to read the CHANGELOG.adoc patch - -concurrency: - group: pr-checklist-${{ github.event.pull_request.number }} - cancel-in-progress: true - -jobs: - checklist: - name: "PR Checklist" - runs-on: ubuntu-latest - timeout-minutes: 3 - steps: - # Needed for the changelog gate's logic module and its unit tests. Only the PR's own tree is - # checked out, with no credentials - nothing here consumes secrets. - - uses: actions/checkout@v6 - with: - persist-credentials: false - - # COMPILE EVERY NODE SCRIPT BEFORE RUNNING ANY OF THEM. `node --check` parses without executing, - # so a syntax error in a script that only runs on some branches - or only when a rule matches - - # is caught here rather than by the unlucky PR that first reaches it. Shell had the same hole and - # bin/check-shell-lint.sh closed it; Node arriving in bin/ (bin/AGENTS.md, "Write it in Node") - # would otherwise have reopened it in a new language. - # - # NOT static analysis, and it is not sold as one: it is a compile. Real analysis for JavaScript - # means either an npm toolchain this repo has deliberately never had, or adding - # javascript-typescript to CodeQL's default setup - which is a repository SETTING rather than a - # file, and is tracked in docs/inflight/ci-javascript-is-the-one-language-nobody-analyses.md. - - name: Compile-check every Node script - run: | - set -euo pipefail - found=0 - while IFS= read -r f; do - found=$((found + 1)) - node --check "$f" - done < <(git ls-files '*.mjs' '*.js' ':!:**/node_modules/**') - # A loop that matched nothing exits 0 and looks identical to a clean pass - the failure this - # repo has hit repeatedly. Say the count, and fail if the glob went stale. - echo "compile-checked $found Node script(s)" - [ "$found" -gt 0 ] || { echo "no Node scripts matched - the glob is wrong" >&2; exit 1; } - - # The gates' own logic is subtle enough to have shipped two real misjudgements already, so each - # is unit tested and the tests run before the gates do. - # - # DISCOVERED, NOT LISTED - the same property bin/check-all.sh exists to hold, for the same - # reason. This was four named steps, one per gate, so a module added to .github/scripts/ with a - # `.test.js` sibling ran nowhere until somebody remembered to wire it here, and its gate looked - # tested. `sticky-report-comment.test.js` was the fifth and would have been the first to find - # out. The count assertion is the other half, copied from the compile-check above: a loop that - # matched nothing exits 0 and is indistinguishable from a clean pass. - - name: Self-test every gate module in .github/scripts - run: | - set -euo pipefail - found=0 - while IFS= read -r t; do - found=$((found + 1)) - echo "--- $t" - node "$t" - done < <(git ls-files '.github/scripts/*.test.js') - echo "ran $found gate self-test(s)" - [ "$found" -gt 0 ] || { echo "no gate self-tests matched - the glob is wrong" >&2; exit 1; } - - # The local mirror (bin/check-issue-refs.sh) fetches the PR body via gh to honour the - # `issue-refs: N/A` opt-out the way this workflow does; that plumbing is shell, not gate - # logic, so it has its own self-test. - - name: Self-test the local issue-reference checker's body plumbing - run: bash bin/test-check-issue-refs.sh - - # The local file-reference mirror has no `gh` plumbing, but it does have an EXIT CODE - # CONTRACT that .githooks/pre-commit reads: 1 blocks the commit, 2 warns and lets it through. - # A node that cannot start exits 1, so the script used to report "dangling references found" - # for a run that checked nothing. These cases pin both codes. - - name: Self-test the local file-reference checker's exit codes - run: bash bin/test-check-file-refs.sh - - # Runs BEFORE the gate below, per bin/AGENTS.md. It tests the script's ARGUMENT PARSING, which - # is a permission boundary rather than a formatting concern: the reviewer is granted - # `bin/todo-index.sh --check` exactly so it can ask whether the index is stale without being - # able to rewrite the tree it is inspecting, and the grant is only worth its wording while the - # script refuses everything else. It also asserts both allowlists still grant the exact command - # rather than a `:*` prefix - the form that let `--check=false` through - # (astubbs/parallel-consumer#286). - - name: Self-test the marker index argument parsing - run: bash bin/test-todo-index.sh - - # docs/todo-index.md is generated from the markers in the tree, so it can silently stop - # matching them. Entries are keyed by marker TEXT, not line number, so this only fires when a - # marker is actually added, removed or reworded - editing above one no longer churns the file. - - name: Verify the marker index matches the tree - run: bin/todo-index.sh --check - - - name: Verify checklist is present and resolved (checked or N/A) - uses: actions/github-script@v7 - with: - script: | - const pr = context.payload.pull_request; - - // Real bots (Dependabot/Renovate) don't use the template - exempt them. - // This is a GitHub-assigned account type, not something a fork PR can spoof via its body. - if (pr.user && pr.user.type === "Bot") { - core.info(`Author ${pr.user.login} is a Bot - checklist gate skipped.`); - return; - } - - const body = pr.body || ""; - let boxes = 0; - const offenders = []; - for (const line of body.split(/\r?\n/)) { - // Match a markdown task-list item: "- [ ] text" / "- [x] text" - const m = line.match(/^\s*[-*]\s*\[( |x|X)\]\s*(.*)$/); - if (!m) continue; - boxes++; - const checked = m[1].toLowerCase() === "x"; - const text = (m[2] || "").trim(); - const naMarked = /\bN\/?A\b/i.test(text); - if (!checked && !naMarked) offenders.push(text || "(empty item)"); - } - - // A human PR with no checklist at all is a template bypass, not an exemption. - if (boxes === 0) { - core.setFailed( - "No PR checklist found - paste the checklist from .github/PULL_REQUEST_TEMPLATE.md and " + - "resolve each box (tick `[x]` or mark `N/A - `). Bots are exempt; humans are not." - ); - return; - } - - if (offenders.length) { - core.setFailed( - "Incomplete PR checklist - tick each box [x] or mark it 'N/A - ':\n" + - offenders.map(o => ` - [ ] ${o}`).join("\n") - ); - } else { - core.info(`PR checklist OK (${boxes} box(es), all checked or marked N/A).`); - } - - # A user-visible changelog entry should say WHICH reported problem or request it addresses, so - # a reader can follow it back. Fork and upstream issues both count. - # - # This replaces an earlier rule requiring the entry to cite its own PR. That was a nuisance by - # construction: the PR number does not exist until the PR is opened, so every PR needed a - # second push purely to add the reference - which in turn double-triggered the review workflow - # (astubbs#109). An issue number is known before the work starts. - # - # Build & CI is exempt - see SECTIONS_REQUIRING_AN_ISSUE in the gate module for why. - # Issue references: below #1000 a reference must name its repo - `astubbs#NN` for this fork, - # `confluentinc#NN` for the original (AGENTS.md -> Issue references). The fork's numbers sit - # inside confluentinc's range, so a bare number there resolves against whichever repo the reader - # happens to be in - and often resolves to a real but unrelated issue, which looks fine and is - # not. Every confluentinc issue is mirrored here, so there is almost always a fork number to - # cite instead. - # - # Deliberately narrow: added lines only, and the files in EXEMPT_PATHS are skipped because a - # bare number legitimately means confluentinc there. A naive sweep rewrote three of them - # wrongly, which is why they are named in the gate module rather than left to judgement. - # - # THE PR BODY IS CHECKED TOO. It is the surface humans actually read, and a bare `#200` renders - # there as a working link to the wrong issue - the failure mode this gate exists for, on its - # most visible page. `gate.prBodyEntry` presents the body to the SAME suspectRefs call as a - # synthetic file, so there is no second copy of the matching rule; it only drops fenced code - # blocks, which GitHub does not autolink. The `edited` trigger above already re-runs this job - # when a body changes, so a body-only fix does not need a push. - - name: Verify issue references name their repo (diff and PR body) - uses: actions/github-script@v7 - with: - script: | - const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/issue-ref-gate.js`); - const pr = context.payload.pull_request; - - if (pr.user && pr.user.type === "Bot") { - core.info(`Author ${pr.user.login} is a Bot - issue reference gate skipped.`); - return; - } - if (gate.findOptOut(pr.body || "")) { - core.info("issue-refs: N/A opt-out present - skipping."); - return; - } - - const files = await github.paginate(github.rest.pulls.listFiles, { - ...context.repo, pull_number: pr.number, per_page: 100, - }); - - let hits = gate.suspectRefs([...files, gate.prBodyEntry(pr.body)]); - - // File-scoped opt-out (`issue-refs: exempt-file`) is judged against file CONTENT, not - // the patch, so it holds however small the diff. Fetched only for files that actually - // have hits - usually none. The control flow lives in the gate module (NO SECOND COPY - // OF THE RULE); this caller supplies the contents-API reader and the logging. - hits = await gate.dropFileOptedOutHits(hits, async (path) => { - const res = await github.rest.repos.getContent({ - ...context.repo, path, ref: pr.head.sha, mediaType: { format: "raw" }, - }); - return String(res.data); - }, { - onDrop: (path) => core.info(`${path}: issue-refs: exempt-file present - its hits dropped.`), - onError: (path, e) => core.info(`${path}: file opt-out check unavailable (${e.message}) - hits kept.`), - }); - - if (hits.length === 0) { - core.info( - `No unqualified references below #${gate.QUALIFY_BELOW} on added lines or in the PR body.`); - return; - } - - core.setFailed(gate.formatFailure(hits, { - repo: `${context.repo.owner}/${context.repo.repo}`, - })); - - # AGENTS.md -> "Cite by anchor, never by line number" tells authors to cite a path plus a - # greppable string, and to run the grep first. Nothing ran it, and docs/citations.md said so: - # "nothing in CI checks any of this". The bill came in as bin/check-review-gate-contract.sh - - # cited by docs/ci.md as the check to run, never written, and unnoticed through every review - # since. A path that looks right reads as right, which is why review does not catch this class. - # - # WHOLE TREE, RATCHETED AGAINST THE BASE. Deleting a file does not change one line in the documents - # that cite it, so a diff-scoped gate is blind to the commonest way a citation breaks - and - # that is not a corner case: two moves account for 59 of the 87 references this repo had to - # repair. So this reads every citing file in the checkout, not `pulls.listFiles`, which also - # removes the "GitHub omits `patch` on a very large diff" hole its sibling gate has to live - # with. It is affordable only because the tree is clean and this gate keeps it clean: a red - # result means THIS branch broke something, not that it inherited something. - # - # The matching lives in the gate module and is unit tested above; what is here is the plumbing - # that hands it this tree. - - name: Verify cited file paths exist - uses: actions/github-script@v7 - with: - script: | - const fs = require("fs"); - const { execFileSync } = require("child_process"); - const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/file-ref-gate.js`); - const pr = context.payload.pull_request; - - if (pr.user && pr.user.type === "Bot") { - core.info(`Author ${pr.user.login} is a Bot - file reference gate skipped.`); - return; - } - if (gate.findOptOut(pr.body || "")) { - core.info("file-refs: N/A opt-out present - skipping."); - return; - } - - const ws = process.env.GITHUB_WORKSPACE; - const tracked = execFileSync("git", ["ls-files"], { encoding: "utf8", cwd: ws }) - .split("\n").filter(Boolean); - - const docs = tracked.filter((f) => gate.CITING_FILE.test(f)).map((filename) => ({ - filename, - lines: fs.readFileSync(`${ws}/${filename}`, "utf8").split("\n"), - })); - - // The BASE tree, so this PR is answerable only for what it added. The checkout is - // shallow, so fetch the base ref first - one commit is enough, we only read its tree. - const base = pr.base.ref; - execFileSync("git", ["fetch", "--no-tags", "--depth=1", "origin", base], - { cwd: ws, stdio: "ignore" }); - - // Same reader as bin/check-file-refs.sh, from the gate module - the byte-exact - // `cat-file --batch` slicing is not something to keep two copies of. - const baseTree = gate.readTreeDocs("FETCH_HEAD", (args, opts = {}) => - execFileSync("git", args, { - encoding: opts.encoding || "utf8", input: opts.input, - cwd: ws, maxBuffer: 512 * 1024 * 1024, - })); - if (!baseTree) { - core.setFailed(`Could not read the base tree (${base}) - cannot tell new findings ` + - "from inherited ones, so this gate is not making a judgement."); - return; - } - - const inherited = gate.danglingRefs(baseTree.docs, gate.treeFrom(baseTree.names)); - const dangling = gate.newFindings(gate.danglingRefs(docs, gate.treeFrom(tracked)), inherited); - - if (dangling.length === 0) { - core.info( - `No new dangling file references (${docs.length} citing file(s), whole tree, ` + - `${inherited.length} inherited from ${base} ignored).`); - return; - } - - core.setFailed(gate.formatFailure(dangling)); - - - name: Verify new changelog entries cite an issue - uses: actions/github-script@v7 - with: - script: | - const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/changelog-ref-gate.js`); - const pr = context.payload.pull_request; - - if (pr.user && pr.user.type === "Bot") { - core.info(`Author ${pr.user.login} is a Bot - changelog reference gate skipped.`); - return; - } - - // Escape hatch, mirroring the checklist's own N/A convention: a PR that touches the - // changelog WITHOUT adding an entry of its own (fixing a typo in an older entry, say) - // can opt out with `changelog-ref: N/A - ` on its own line in the PR body. - const optOut = gate.findOptOut(pr.body); - if (optOut) { - core.info(`Opted out via PR body: "${optOut}"`); - return; - } - - const files = await github.paginate(github.rest.pulls.listFiles, { - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: pr.number, - }); - - const changelog = files.find(f => f.filename === "CHANGELOG.adoc"); - // No patch also covers GitHub omitting it for very large diffs - accepted gap: a giant - // changelog rewrite would skip the check rather than fail it. - if (!changelog || !changelog.patch) { - core.info("No CHANGELOG.adoc change in this PR - nothing to check."); - return; - } - - // The section comes from the CHANGELOG itself, not the patch: git has no funcname - // pattern for asciidoc, so a hunk header here reads `endif::[]` rather than the - // heading, and entries are too long for three lines of context to reach one. - const fs = require("fs"); - const changelogText = fs.readFileSync( - `${process.env.GITHUB_WORKSPACE}/CHANGELOG.adoc`, "utf8"); - const missing = gate.entriesMissingIssue(changelog.patch, changelogText); - - if (missing.length === 0) { - core.info("Changelog OK - every new entry that needs an issue reference has one."); - return; - } - - core.setFailed( - `New CHANGELOG.adoc entr${missing.length === 1 ? "y does" : "ies do"} not cite an issue.\n` + - `Entries under ${gate.SECTIONS_REQUIRING_AN_ISSUE.join(" / ")} should say which reported problem or request they address.\n` + - `Link the issue explicitly - a bare #NN is ambiguous, since issues and PRs share one number sequence:\n` + - ` fork: (https://github.com/${context.repo.owner}/${context.repo.repo}/issues/NN[#NN])\n` + - ` upstream: (upstream https://github.com/confluentinc/parallel-consumer/issues/NN[#NN])\n` + - `If there genuinely is no issue, put "changelog-ref: N/A - " on its own line in the PR body.\n\n` + - `Entr${missing.length === 1 ? "y" : "ies"} missing one:\n` + - missing.map(l => ` ${l.trim().slice(0, 160)}`).join("\n") - ); - - # The roadmap's stage ladder says how real each entry is, and its own rule is that the PR - # advancing a track moves the stage in the same change (docs/data/roadmap.yaml, `stages` - # block). This step is what catches a miss: when the BASE branch's roadmap names this PR as - # an entry's carrier (`pull_request: astubbs#NNN`), that ENTRY's stage block must differ - # between base and head - touching the file elsewhere does not count - or the PR opts out - # with `roadmap-stage: N/A - ` in its body. Base is read deliberately: master is the - # authority on which entries currently claim this PR. - - name: Verify a roadmap-carrying PR moves its entry's stage - uses: actions/github-script@v7 - with: - script: | - const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/roadmap-stage-gate.js`); - const pr = context.payload.pull_request; - - if (pr.user && pr.user.type === "Bot") { - core.info(`Author ${pr.user.login} is a Bot - roadmap stage gate skipped.`); - return; - } - const optOut = gate.findOptOut(pr.body || ""); - if (optOut) { - core.info(`Opted out via PR body: "${optOut}"`); - return; - } - - const fetchRoadmap = async (ref) => { - const res = await github.rest.repos.getContent({ - ...context.repo, path: gate.ROADMAP_PATH, ref, - mediaType: { format: "raw" }, - }); - return String(res.data); - }; - - let baseText = ""; - try { - baseText = await fetchRoadmap(pr.base.sha); - } catch (e) { - core.info(`No ${gate.ROADMAP_PATH} on the base ref (${e.message}) - nothing to check.`); - return; - } - - const claims = gate.entriesClaimingPr(baseText, pr.number); - if (claims.length === 0) { - core.info("No roadmap entry names this PR as its carrier - nothing to check."); - return; - } - - // A missing head file means the roadmap was deleted or moved - every claiming entry's - // snapshot then differs from base, which is the accountable-change signal we want. - let headText = ""; - try { - headText = await fetchRoadmap(pr.head.sha); - } catch (e) { - core.info(`${gate.ROADMAP_PATH} unreadable at head (${e.message}) - treating as changed.`); - } - - const unmoved = claims.filter((c) => !gate.stageMoved(baseText, headText, c.id)); - if (unmoved.length === 0) { - core.info( - `Roadmap entr${claims.length === 1 ? "y" : "ies"} ` + - `${claims.map(c => c.id).join(", ")} name this PR and the stage block moved - OK.`); - return; - } - - core.setFailed(gate.formatFailure(unmoved, pr.number)); diff --git a/.github/workflows/repo-hygiene.yml b/.github/workflows/repo-hygiene.yml index 16493eb02..f2b71fee5 100644 --- a/.github/workflows/repo-hygiene.yml +++ b/.github/workflows/repo-hygiene.yml @@ -10,9 +10,21 @@ # a second checkout each. The one thing each dedicated job carried that this lane did not is now an # explicit step or env line below (the fork-point strictness, the PyYAML check), so nothing was lost # in the fold; and the quarantine OWNER check keeps its authenticated run in quarantine-lane.yml, -# whose `tests` check is required, because this lane deliberately holds no token (see PR_NUMBER). -# The three retired names are required contexts in the master ruleset until it is edited - -# docs/inflight/ci-fewer-jobs-ruleset-edits.md carries the edit and its ordering. +# whose `tests` check is required, because this lane's shell sweep deliberately sees no token (see +# PR_NUMBER). +# +# A FOURTH, `PR Checklist` (the whole of pr-checklist.yml), FOLLOWED. It was not a duplicate of the +# sweep - its five github-script gates read the PR body and the PR's file list, which no bin/check-* +# gate can do without a credential - but it checked out the same tree with the same read-only scope +# on the same trigger, and three of its self-test steps (bin/test-check-issue-refs.sh, +# bin/test-check-file-refs.sh, bin/test-todo-index.sh) were already swept here by glob. Its steps +# are the tail of the `hygiene` job below, comments intact, guarded so a push run (which has no PR) +# skips the PR-reading ones. What the fold changed about THIS workflow, each justified at the point +# it appears: the `edited` trigger, the concurrency key, the `pull-requests: read` scope, and +# `persist-credentials: false` on the checkout. Why this host and not `Check PR Dependencies` or a +# review gate: docs/inflight/ci-fewer-jobs-ruleset-edits.md records the pairing reasoning. +# The four retired names are required contexts in the master ruleset until it is edited - that same +# note carries the edit and its ordering. # # ONE LANE, DISCOVERED - and this replaced one-job-per-concern deliberately. That shape gave each # gate its own required-status-check candidate and its own red tick, which reads well, and cost @@ -44,18 +56,40 @@ name: Repo Hygiene # push scoped to master (matching maven.yml) so PR-branch commits run once via the pull_request # event instead of twice. copyright.yml ran on push to master too, so its fold into this workflow # keeps that cadence. +# +# `edited` IS NOT A DEFAULT pull_request TYPE, and it is here for the folded PR Checklist gates: the +# checklist gate, the issue-reference gate and the changelog gate all read the PR BODY, so a body edit +# must re-evaluate them - otherwise the fix for a bare `#200` in the description would need a push, +# which is the round trip the `edited` trigger was added to remove. The other three are the +# defaults, spelled out because naming any type replaces the default set. The cost is that the whole +# workflow, macOS lane included, re-runs on a body edit: a per-job `if` that skipped the shell lanes +# on `edited` would, under cancel-in-progress, leave a cancelled run's check as "skipped", which a +# required check does not accept (docs/ci.md, "A skip does not satisfy a required check"). on: push: branches: [ master ] pull_request: + types: [opened, edited, synchronize, reopened] workflow_dispatch: +# Keyed on the PR NUMBER, not the ref: an `edited` event has the same head ref as the push before it, +# and a body edit that arrives mid-run should supersede that run's read of the body, which the +# number-keyed group does by cancelling it. The fallback is the SHA, not the ref, so two pushes to +# master in quick succession each keep their own run: a master run is the record of what master's +# gates said at that commit, and a ref-keyed group would cancel the earlier one - the previous key +# here did exactly that. concurrency: - group: repo-hygiene-${{ github.head_ref || github.ref }} + group: repo-hygiene-${{ github.event.pull_request.number || github.sha }} cancel-in-progress: true +# Both READ. `pull-requests: read` is for the folded gates' `pulls.listFiles` and the PR body via +# the payload; `contents: read` is the checkout plus `repos.getContent` at base and head for the +# file-scoped opt-out and the roadmap gate. Nothing in this workflow writes to the repository, +# a check, or a comment, and nothing here may be given a scope that could - the shell sweep runs +# PR-authored scripts, and a write grant in the same job is a write grant to the PR. permissions: contents: read + pull-requests: read jobs: hygiene: @@ -63,11 +97,13 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 20 # PR_NUMBER is not a convenience here - without it bin/check-branch-self-reference.sh checks HALF - # of what it claims and still exits 0. `gh` on this runner is unauthenticated (no GH_TOKEN in - # this workflow, by design - nothing else here needs one), so the script's `gh pr list` fallback - # fails, the failure is swallowed, and the `#NNN` / `/pull/NNN` arm is skipped entirely - the - # gate prints "NO PR NUMBER RESOLVED... were NOT checked" and still exits 0. That arm is the - # gate's own motivating incident: a note citing astubbs#1 by number, with no branch name in it. + # of what it claims and still exits 0. `gh` in the shell sweep is unauthenticated (no GH_TOKEN is + # exported to it, by design - the job's token exists only for the github-script gates below, and + # handing it to the sweep would make check-quarantine-owners.sh's advisory arm depend on which + # job it ran in), so the script's `gh pr list` fallback fails, the failure is swallowed, and the + # `#NNN` / `/pull/NNN` arm is skipped entirely - the gate prints "NO PR NUMBER RESOLVED... were + # NOT checked" and still exits 0. That arm is the gate's own motivating incident: a note citing + # astubbs#1 by number, with no branch name in it. # Job-level (not step-level) because this lane collapsed nine per-gate steps into one # bin/check-all.sh invocation - there is no single step to attach it to, and check-all.sh's # run_capture already passes PR_NUMBER through to every gate harmlessly. The event payload @@ -86,6 +122,14 @@ jobs: # .claude/hooks/check-shallow-history.sh exists to stop locally. The copyright scanner # also classifies provenance from the fork-point commit, which only full history holds. fetch-depth: 0 + # No credential left in the checkout - nothing here consumes one, and this job runs + # PR-authored scripts. Checked before adding: the sweep's only network git call is + # check-quarantine-owners.sh's `preview_fetch` of the public origin URL into a scratch + # git dir, which is anonymous; `gh` reads GH_TOKEN, not the checkout credential; and the + # file-reference gate below has run its `git fetch origin ` under this same flag on + # every PR since it was written. bin/test-check-agent-hooks.sh reads origin/master from + # the full-history checkout above and fetches nothing. + persist-credentials: false # ubuntu-latest ships shellcheck, but an image change that removed it must fail rather than # silently pass - bin/check-shell-lint.sh exits 2 for "cannot run", and check-all counts an @@ -145,6 +189,409 @@ jobs: *) exit "$rc" ;; esac + # ---- Folded from pr-checklist.yml, in its original order. ---- + # + # FAIL AT END. Every step from here down carries `!cancelled()`, so a red sweep above - or a red + # step here - does not stop the rest from running and reporting; the job's own conclusion is + # the aggregate, since a step failure fails the job whatever runs after it. The standalone + # workflow stopped at the first failure, which meant one failing self-test hid the verdicts of + # five gates. The three tree checks run on push too - they read nothing from the PR, and a + # master run is one more place a stale marker index or a Node syntax error gets caught. The + # five github-script gates are additionally gated on the event: a push run has no + # `context.payload.pull_request`, and every one of them dereferences it on its first line. + + # COMPILE EVERY NODE SCRIPT BEFORE RUNNING ANY OF THEM. `node --check` parses without executing, + # so a syntax error in a script that only runs on some branches - or only when a rule matches - + # is caught here rather than by the unlucky PR that first reaches it. Shell had the same hole and + # bin/check-shell-lint.sh closed it; Node arriving in bin/ (bin/AGENTS.md, "Write it in Node") + # would otherwise have reopened it in a new language. + # + # NOT static analysis, and it is not sold as one: it is a compile. Real analysis for JavaScript + # means either an npm toolchain this repo has deliberately never had, or adding + # javascript-typescript to CodeQL's default setup - which is a repository SETTING rather than a + # file, and is tracked in docs/inflight/ci-javascript-is-the-one-language-nobody-analyses.md. + - name: Compile-check every Node script + if: ${{ !cancelled() }} + run: | + set -euo pipefail + found=0 + while IFS= read -r f; do + found=$((found + 1)) + node --check "$f" + done < <(git ls-files '*.mjs' '*.js' ':!:**/node_modules/**') + # A loop that matched nothing exits 0 and looks identical to a clean pass - the failure this + # repo has hit repeatedly. Say the count, and fail if the glob went stale. + echo "compile-checked $found Node script(s)" + [ "$found" -gt 0 ] || { echo "no Node scripts matched - the glob is wrong" >&2; exit 1; } + + # The gates' own logic is subtle enough to have shipped two real misjudgements already, so each + # is unit tested and the tests run before the gates do. + # + # DISCOVERED, NOT LISTED - the same property bin/check-all.sh exists to hold, for the same + # reason. This was four named steps, one per gate, so a module added to .github/scripts/ with a + # `.test.js` sibling ran nowhere until somebody remembered to wire it here, and its gate looked + # tested. `sticky-report-comment.test.js` was the fifth and would have been the first to find + # out. The count assertion is the other half, copied from the compile-check above: a loop that + # matched nothing exits 0 and is indistinguishable from a clean pass. + # + # The standalone workflow also ran bin/test-check-issue-refs.sh, bin/test-check-file-refs.sh + # and bin/test-todo-index.sh as named steps here. All three match `bin/test-*.sh`, so the + # `--with-tests` sweep above already runs them; naming them again would be the hand-kept list + # this lane exists to avoid. bin/todo-index.sh --check below still runs AFTER its self-test, + # per bin/AGENTS.md, because the sweep precedes it. + - name: Self-test every gate module in .github/scripts + if: ${{ !cancelled() }} + run: | + set -euo pipefail + found=0 + while IFS= read -r t; do + found=$((found + 1)) + echo "--- $t" + node "$t" + done < <(git ls-files '.github/scripts/*.test.js') + echo "ran $found gate self-test(s)" + [ "$found" -gt 0 ] || { echo "no gate self-tests matched - the glob is wrong" >&2; exit 1; } + + # docs/todo-index.md is generated from the markers in the tree, so it can silently stop + # matching them. Entries are keyed by marker TEXT, not line number, so this only fires when a + # marker is actually added, removed or reworded - editing above one no longer churns the file. + # Not a check-*.sh gate, so the sweep cannot reach it: run bare, bin/todo-index.sh REGENERATES + # the index, and a read-only sweep may not mutate the tree (docs/inflight/ci-hygiene-gaps.md). + - name: Verify the marker index matches the tree + if: ${{ !cancelled() }} + run: bin/todo-index.sh --check + + # PR Checklist gate - makes .github/PULL_REQUEST_TEMPLATE.md's checklist actually binding. + # + # The template is only a passive nudge (and `gh --body-file` PRs skip it entirely), so this check + # enforces it two ways for human-authored PRs: + # 1. The checklist must be PRESENT - a human PR with zero task-list items fails (otherwise dropping + # the template is a silent bypass: no boxes -> nothing to be "unchecked" -> vacuous pass). + # 2. Every box present must be resolved - ticked `[x]`, or left `- [ ] ... N/A - `. + # Real bot PRs (Dependabot/Renovate etc., GitHub user type "Bot") don't use the template and are + # exempt entirely. Re-runs on body edits. + - name: Verify checklist is present and resolved (checked or N/A) + if: github.event_name == 'pull_request' && !cancelled() + uses: actions/github-script@v7 + with: + script: | + const pr = context.payload.pull_request; + + // Real bots (Dependabot/Renovate) don't use the template - exempt them. + // This is a GitHub-assigned account type, not something a fork PR can spoof via its body. + if (pr.user && pr.user.type === "Bot") { + core.info(`Author ${pr.user.login} is a Bot - checklist gate skipped.`); + return; + } + + const body = pr.body || ""; + let boxes = 0; + const offenders = []; + for (const line of body.split(/\r?\n/)) { + // Match a markdown task-list item: "- [ ] text" / "- [x] text" + const m = line.match(/^\s*[-*]\s*\[( |x|X)\]\s*(.*)$/); + if (!m) continue; + boxes++; + const checked = m[1].toLowerCase() === "x"; + const text = (m[2] || "").trim(); + const naMarked = /\bN\/?A\b/i.test(text); + if (!checked && !naMarked) offenders.push(text || "(empty item)"); + } + + // A human PR with no checklist at all is a template bypass, not an exemption. + if (boxes === 0) { + core.setFailed( + "No PR checklist found - paste the checklist from .github/PULL_REQUEST_TEMPLATE.md and " + + "resolve each box (tick `[x]` or mark `N/A - `). Bots are exempt; humans are not." + ); + return; + } + + if (offenders.length) { + core.setFailed( + "Incomplete PR checklist - tick each box [x] or mark it 'N/A - ':\n" + + offenders.map(o => ` - [ ] ${o}`).join("\n") + ); + } else { + core.info(`PR checklist OK (${boxes} box(es), all checked or marked N/A).`); + } + + # A user-visible changelog entry should say WHICH reported problem or request it addresses, so + # a reader can follow it back. Fork and upstream issues both count. + # + # This replaces an earlier rule requiring the entry to cite its own PR. That was a nuisance by + # construction: the PR number does not exist until the PR is opened, so every PR needed a + # second push purely to add the reference - which in turn double-triggered the review workflow + # (astubbs#109). An issue number is known before the work starts. + # + # Build & CI is exempt - see SECTIONS_REQUIRING_AN_ISSUE in the gate module for why. + # Issue references: below #1000 a reference must name its repo - `astubbs#NN` for this fork, + # `confluentinc#NN` for the original (AGENTS.md -> Issue references). The fork's numbers sit + # inside confluentinc's range, so a bare number there resolves against whichever repo the reader + # happens to be in - and often resolves to a real but unrelated issue, which looks fine and is + # not. Every confluentinc issue is mirrored here, so there is almost always a fork number to + # cite instead. + # + # Deliberately narrow: added lines only, and the files in EXEMPT_PATHS are skipped because a + # bare number legitimately means confluentinc there. A naive sweep rewrote three of them + # wrongly, which is why they are named in the gate module rather than left to judgement. + # + # THE PR BODY IS CHECKED TOO. It is the surface humans actually read, and a bare `#200` renders + # there as a working link to the wrong issue - the failure mode this gate exists for, on its + # most visible page. `gate.prBodyEntry` presents the body to the SAME suspectRefs call as a + # synthetic file, so there is no second copy of the matching rule; it only drops fenced code + # blocks, which GitHub does not autolink. The `edited` trigger above already re-runs this job + # when a body changes, so a body-only fix does not need a push. + - name: Verify issue references name their repo (diff and PR body) + if: github.event_name == 'pull_request' && !cancelled() + uses: actions/github-script@v7 + with: + script: | + const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/issue-ref-gate.js`); + const pr = context.payload.pull_request; + + if (pr.user && pr.user.type === "Bot") { + core.info(`Author ${pr.user.login} is a Bot - issue reference gate skipped.`); + return; + } + if (gate.findOptOut(pr.body || "")) { + core.info("issue-refs: N/A opt-out present - skipping."); + return; + } + + const files = await github.paginate(github.rest.pulls.listFiles, { + ...context.repo, pull_number: pr.number, per_page: 100, + }); + + let hits = gate.suspectRefs([...files, gate.prBodyEntry(pr.body)]); + + // File-scoped opt-out (`issue-refs: exempt-file`) is judged against file CONTENT, not + // the patch, so it holds however small the diff. Fetched only for files that actually + // have hits - usually none. The control flow lives in the gate module (NO SECOND COPY + // OF THE RULE); this caller supplies the contents-API reader and the logging. + hits = await gate.dropFileOptedOutHits(hits, async (path) => { + const res = await github.rest.repos.getContent({ + ...context.repo, path, ref: pr.head.sha, mediaType: { format: "raw" }, + }); + return String(res.data); + }, { + onDrop: (path) => core.info(`${path}: issue-refs: exempt-file present - its hits dropped.`), + onError: (path, e) => core.info(`${path}: file opt-out check unavailable (${e.message}) - hits kept.`), + }); + + if (hits.length === 0) { + core.info( + `No unqualified references below #${gate.QUALIFY_BELOW} on added lines or in the PR body.`); + return; + } + + core.setFailed(gate.formatFailure(hits, { + repo: `${context.repo.owner}/${context.repo.repo}`, + })); + + # AGENTS.md -> "Cite by anchor, never by line number" tells authors to cite a path plus a + # greppable string, and to run the grep first. Nothing ran it, and docs/citations.md said so: + # "nothing in CI checks any of this". The bill came in as bin/check-review-gate-contract.sh - + # cited by docs/ci.md as the check to run, never written, and unnoticed through every review + # since. A path that looks right reads as right, which is why review does not catch this class. + # + # WHOLE TREE, RATCHETED AGAINST THE BASE. Deleting a file does not change one line in the documents + # that cite it, so a diff-scoped gate is blind to the commonest way a citation breaks - and + # that is not a corner case: two moves account for 59 of the 87 references this repo had to + # repair. So this reads every citing file in the checkout, not `pulls.listFiles`, which also + # removes the "GitHub omits `patch` on a very large diff" hole its sibling gate has to live + # with. It is affordable only because the tree is clean and this gate keeps it clean: a red + # result means THIS branch broke something, not that it inherited something. + # + # The matching lives in the gate module and is unit tested above; what is here is the plumbing + # that hands it this tree. The checkout in this job is full-depth, so the base ref is usually + # already present; the fetch is kept because it costs one round trip and makes the gate + # independent of that setting. + - name: Verify cited file paths exist + if: github.event_name == 'pull_request' && !cancelled() + uses: actions/github-script@v7 + with: + script: | + const fs = require("fs"); + const { execFileSync } = require("child_process"); + const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/file-ref-gate.js`); + const pr = context.payload.pull_request; + + if (pr.user && pr.user.type === "Bot") { + core.info(`Author ${pr.user.login} is a Bot - file reference gate skipped.`); + return; + } + if (gate.findOptOut(pr.body || "")) { + core.info("file-refs: N/A opt-out present - skipping."); + return; + } + + const ws = process.env.GITHUB_WORKSPACE; + const tracked = execFileSync("git", ["ls-files"], { encoding: "utf8", cwd: ws }) + .split("\n").filter(Boolean); + + const docs = tracked.filter((f) => gate.CITING_FILE.test(f)).map((filename) => ({ + filename, + lines: fs.readFileSync(`${ws}/${filename}`, "utf8").split("\n"), + })); + + // The BASE tree, so this PR is answerable only for what it added. Fetch the base ref + // first - one commit is enough, we only read its tree. + const base = pr.base.ref; + execFileSync("git", ["fetch", "--no-tags", "--depth=1", "origin", base], + { cwd: ws, stdio: "ignore" }); + + // Same reader as bin/check-file-refs.sh, from the gate module - the byte-exact + // `cat-file --batch` slicing is not something to keep two copies of. + const baseTree = gate.readTreeDocs("FETCH_HEAD", (args, opts = {}) => + execFileSync("git", args, { + encoding: opts.encoding || "utf8", input: opts.input, + cwd: ws, maxBuffer: 512 * 1024 * 1024, + })); + if (!baseTree) { + core.setFailed(`Could not read the base tree (${base}) - cannot tell new findings ` + + "from inherited ones, so this gate is not making a judgement."); + return; + } + + const inherited = gate.danglingRefs(baseTree.docs, gate.treeFrom(baseTree.names)); + const dangling = gate.newFindings(gate.danglingRefs(docs, gate.treeFrom(tracked)), inherited); + + if (dangling.length === 0) { + core.info( + `No new dangling file references (${docs.length} citing file(s), whole tree, ` + + `${inherited.length} inherited from ${base} ignored).`); + return; + } + + core.setFailed(gate.formatFailure(dangling)); + + - name: Verify new changelog entries cite an issue + if: github.event_name == 'pull_request' && !cancelled() + uses: actions/github-script@v7 + with: + script: | + const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/changelog-ref-gate.js`); + const pr = context.payload.pull_request; + + if (pr.user && pr.user.type === "Bot") { + core.info(`Author ${pr.user.login} is a Bot - changelog reference gate skipped.`); + return; + } + + // Escape hatch, mirroring the checklist's own N/A convention: a PR that touches the + // changelog WITHOUT adding an entry of its own (fixing a typo in an older entry, say) + // can opt out with `changelog-ref: N/A - ` on its own line in the PR body. + const optOut = gate.findOptOut(pr.body); + if (optOut) { + core.info(`Opted out via PR body: "${optOut}"`); + return; + } + + const files = await github.paginate(github.rest.pulls.listFiles, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + }); + + const changelog = files.find(f => f.filename === "CHANGELOG.adoc"); + // No patch also covers GitHub omitting it for very large diffs - accepted gap: a giant + // changelog rewrite would skip the check rather than fail it. + if (!changelog || !changelog.patch) { + core.info("No CHANGELOG.adoc change in this PR - nothing to check."); + return; + } + + // The section comes from the CHANGELOG itself, not the patch: git has no funcname + // pattern for asciidoc, so a hunk header here reads `endif::[]` rather than the + // heading, and entries are too long for three lines of context to reach one. + const fs = require("fs"); + const changelogText = fs.readFileSync( + `${process.env.GITHUB_WORKSPACE}/CHANGELOG.adoc`, "utf8"); + const missing = gate.entriesMissingIssue(changelog.patch, changelogText); + + if (missing.length === 0) { + core.info("Changelog OK - every new entry that needs an issue reference has one."); + return; + } + + core.setFailed( + `New CHANGELOG.adoc entr${missing.length === 1 ? "y does" : "ies do"} not cite an issue.\n` + + `Entries under ${gate.SECTIONS_REQUIRING_AN_ISSUE.join(" / ")} should say which reported problem or request they address.\n` + + `Link the issue explicitly - a bare #NN is ambiguous, since issues and PRs share one number sequence:\n` + + ` fork: (https://github.com/${context.repo.owner}/${context.repo.repo}/issues/NN[#NN])\n` + + ` upstream: (upstream https://github.com/confluentinc/parallel-consumer/issues/NN[#NN])\n` + + `If there genuinely is no issue, put "changelog-ref: N/A - " on its own line in the PR body.\n\n` + + `Entr${missing.length === 1 ? "y" : "ies"} missing one:\n` + + missing.map(l => ` ${l.trim().slice(0, 160)}`).join("\n") + ); + + # The roadmap's stage ladder says how real each entry is, and its own rule is that the PR + # advancing a track moves the stage in the same change (docs/data/roadmap.yaml, `stages` + # block). This step is what catches a miss: when the BASE branch's roadmap names this PR as + # an entry's carrier (`pull_request: astubbs#NNN`), that ENTRY's stage block must differ + # between base and head - touching the file elsewhere does not count - or the PR opts out + # with `roadmap-stage: N/A - ` in its body. Base is read deliberately: master is the + # authority on which entries currently claim this PR. + - name: Verify a roadmap-carrying PR moves its entry's stage + if: github.event_name == 'pull_request' && !cancelled() + uses: actions/github-script@v7 + with: + script: | + const gate = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/roadmap-stage-gate.js`); + const pr = context.payload.pull_request; + + if (pr.user && pr.user.type === "Bot") { + core.info(`Author ${pr.user.login} is a Bot - roadmap stage gate skipped.`); + return; + } + const optOut = gate.findOptOut(pr.body || ""); + if (optOut) { + core.info(`Opted out via PR body: "${optOut}"`); + return; + } + + const fetchRoadmap = async (ref) => { + const res = await github.rest.repos.getContent({ + ...context.repo, path: gate.ROADMAP_PATH, ref, + mediaType: { format: "raw" }, + }); + return String(res.data); + }; + + let baseText = ""; + try { + baseText = await fetchRoadmap(pr.base.sha); + } catch (e) { + core.info(`No ${gate.ROADMAP_PATH} on the base ref (${e.message}) - nothing to check.`); + return; + } + + const claims = gate.entriesClaimingPr(baseText, pr.number); + if (claims.length === 0) { + core.info("No roadmap entry names this PR as its carrier - nothing to check."); + return; + } + + // A missing head file means the roadmap was deleted or moved - every claiming entry's + // snapshot then differs from base, which is the accountable-change signal we want. + let headText = ""; + try { + headText = await fetchRoadmap(pr.head.sha); + } catch (e) { + core.info(`${gate.ROADMAP_PATH} unreadable at head (${e.message}) - treating as changed.`); + } + + const unmoved = claims.filter((c) => !gate.stageMoved(baseText, headText, c.id)); + if (unmoved.length === 0) { + core.info( + `Roadmap entr${claims.length === 1 ? "y" : "ies"} ` + + `${claims.map(c => c.id).join(", ")} name this PR and the stage block moved - OK.`); + return; + } + + core.setFailed(gate.formatFailure(unmoved, pr.number)); + # WHY THIS LANE EXISTS. Every other job in this file is ubuntu, so the shell harness was only ever # exercised against GNU coreutils and bash 5, while contributors run it on macOS with BSD userland # and bash 3.2. Four live defects survived a deliberate portability sweep, two automated reviews diff --git a/AGENTS.md b/AGENTS.md index e600db263..751bde956 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -152,7 +152,7 @@ config key, a quoted literal; a long quotation is brittle the other way, breakin the grep before you commit the citation. **The path half is now enforced: `bin/check-file-refs.sh` fails a cited path that does not exist**, -across the whole tree, and the `PR Checklist` workflow runs the same module - so deleting a file +across the whole tree, and the `repo: hygiene` gate runs the same module - so deleting a file also fails the PR that leaves citations behind. The anchor half is still yours: a gate can only tell you the file is there, never that your quoted string is still in it. @@ -418,7 +418,7 @@ Unit tests are surefire (`src/test/java/`); integration tests are failsafe and n **In a PR the changelog is never added to.** No new entries, and no `== Unreleased` section - a shipped section is finished, and the in-flight section belongs to the release-time generator. There -is no window in which a PR contributes an entry, and **the `PR Checklist` gate does not enforce +is no window in which a PR contributes an entry, and **the `repo: hygiene` gate does not enforce this** - it checks citations, so it will happily pass an entry the policy forbids. **The one edit a PR may make is correcting a factual error in text already there** (astubbs#198 is @@ -536,7 +536,7 @@ Nothing lints commit messages, so all of this is on you. - **Open PRs from the template and complete its checklist honestly.** `.github/PULL_REQUEST_TEMPLATE.md` is NOT auto-applied when a PR is created non-interactively (e.g. `gh pr create -R astubbs/parallel-consumer --body-file`), so base the body on it and resolve - every box: check it `[x]`, or mark it `N/A - `. The `PR Checklist` gate fails a + every box: check it `[x]`, or mark it `N/A - `. The `repo: hygiene` gate fails a human-authored PR when the checklist is missing entirely *or* any box is left unchecked without an `N/A`, so dropping the template is not a bypass. Only real bot authors are exempt. - **Ask for the automated review when the PR is ready - it does not run on push.** Two routes, and diff --git a/bin/AGENTS.md b/bin/AGENTS.md index 39652ee3a..fd7bd5922 100644 --- a/bin/AGENTS.md +++ b/bin/AGENTS.md @@ -37,10 +37,10 @@ language where each of those is a paragraph. [`bin/lib/source-patterns.mjs`](lib holds the table and `check-source-patterns.mjs` is the one runner they share. A check that has to *think* - parse XML, call an API, compare numbers - is a real program and still gets its own file. -**Node scripts are compile-checked by `pr-checklist.yml` and self-tested by `repo-hygiene.yml`**, -which runs `bin/check-all.sh --with-tests` and so discovers `bin/test-*.mjs` by glob. They are two -different workflows on purpose - naming only the first sends anyone repairing the test wiring to a -job that never runs the tests. `node --check` is a +**Node scripts are compile-checked and self-tested by the `repo: hygiene` job** (formerly two +separate workflows - compile-checked by `pr-checklist.yml`, self-tested by `repo-hygiene.yml` - +folded together), which runs `bin/check-all.sh --with-tests` and so discovers `bin/test-*.mjs` by +glob. `node --check` is a compile, not static analysis; JavaScript is the one language CodeQL's default setup here does not scan, which is tracked separately. diff --git a/bin/check-file-refs.sh b/bin/check-file-refs.sh index f915936ba..870495dcd 100755 --- a/bin/check-file-refs.sh +++ b/bin/check-file-refs.sh @@ -3,7 +3,7 @@ # Copyright (C) 2026 Antony Stubbs and contributors # -# Flags a cited repo file path that does not exist - the same rule the `PR Checklist` workflow +# Flags a cited repo file path that does not exist - the same rule the `repo: hygiene` gate # enforces (AGENTS.md -> "Cite by anchor, never by line number"). # # WHY THIS EXISTS diff --git a/bin/check-issue-refs.sh b/bin/check-issue-refs.sh index 9c9447438..d6e71e583 100755 --- a/bin/check-issue-refs.sh +++ b/bin/check-issue-refs.sh @@ -4,7 +4,7 @@ # # Flags unqualified `#NN` references below #1000 on lines this branch ADDS - the same rule the -# `PR Checklist` workflow enforces (AGENTS.md -> Issue references). +# `repo: hygiene` gate enforces (AGENTS.md -> Issue references). # # WHY THIS EXISTS # diff --git a/bin/lib/node-gate.sh b/bin/lib/node-gate.sh index aa1e3357e..08a0dd274 100644 --- a/bin/lib/node-gate.sh +++ b/bin/lib/node-gate.sh @@ -30,7 +30,7 @@ # THE PROBE LOADS THE GATE MODULE, it does not merely start node, so "node cannot start" and "the # module cannot be loaded or parsed" both land on 2 - "cannot run" - instead of the second # masquerading as a finding. A module that loads and then throws at runtime is still reported as a -# finding; that residue is covered by the modules' own unit tests in the `PR Checklist` workflow. +# finding; that residue is covered by the modules' own unit tests in the `repo: hygiene` gate. # # HOW TO LOAD THIS FILE, and why both callers do it the long way. Under `set -e` on bash 3.2 - the # system bash on every macOS host - a `source` of a file it cannot read is FATAL to the shell, so a @@ -51,7 +51,7 @@ node_gate_require_node() { command -v node >/dev/null 2>&1 && return 0 echo "ERROR: node not found - needed to reuse $1." >&2 - echo "The authoritative gate is the 'PR Checklist' workflow; this is the local mirror of it." >&2 + echo "The authoritative gate is the 'repo: hygiene' job; this is the local mirror of it." >&2 return 2 } @@ -85,6 +85,6 @@ node_gate_verdict() { echo " A stale --require in there - one naming a file that has since been deleted - kills" >&2 echo " node during preload, before any script runs. Clear it and run this again." >&2 fi - echo " The authoritative gate is the 'PR Checklist' workflow; this is the local mirror of it." >&2 + echo " The authoritative gate is the 'repo: hygiene' job; this is the local mirror of it." >&2 return 2 } diff --git a/docs/ci.md b/docs/ci.md index 39119e544..8d3bcfe23 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -91,15 +91,19 @@ document. This section is the detail behind it. See [`docs/testing.md`](testing.md), and [`docs/solutions/workflow-issues/the-run-that-had-to-retract-was-the-one-gated-silent-2026-09-02.md`](solutions/workflow-issues/the-run-that-had-to-retract-was-the-one-gated-silent-2026-09-02.md) for the class. -- **`pr-checklist.yml`** - hosts the PR-body gates: the template checklist (rule in AGENTS.md, PR - Discipline), the changelog-citation gate (`changelog-ref-gate.js`, see - [`docs/releasing.md`](releasing.md)), the issue-reference gate (`issue-ref-gate.js`, see - [`docs/issue-references.md`](issue-references.md)) and the file-reference gate - (`file-ref-gate.js`, see [`docs/citations.md`](citations.md)), which fails a cited repo path that - does not exist - whole tree, so a deletion that strands a citation fails the PR that made it. Each gate's logic is a unit-tested module and its self-test runs first, so a - broken rule fails loudly rather than passing - or failing - every PR silently. The self-test step - **discovers** `.github/scripts/*.test.js` rather than naming them, so a module added there is - covered without an edit here or in the workflow. +- **The PR-body gates** - formerly the `PR Checklist` job in `pr-checklist.yml`, now the tail of + `repo: hygiene` in `repo-hygiene.yml` (folded 2026-09-07; the old context is a removal owed to the + ruleset, see below): the template checklist (rule in AGENTS.md, PR Discipline), the + changelog-citation gate (`changelog-ref-gate.js`, see [`docs/releasing.md`](releasing.md)), the + issue-reference gate (`issue-ref-gate.js`, see [`docs/issue-references.md`](issue-references.md)), + the file-reference gate (`file-ref-gate.js`, see [`docs/citations.md`](citations.md)), which fails + a cited repo path that does not exist - whole tree, so a deletion that strands a citation fails the + PR that made it - and the roadmap stage gate (`roadmap-stage-gate.js`). Each gate's logic is a + unit-tested module and its self-test runs first, so a broken rule fails loudly rather than + passing - or failing - every PR silently. The self-test step **discovers** + `.github/scripts/*.test.js` rather than naming them, so a module added there is covered without an + edit here or in the workflow. The gates read the PR body, so the workflow runs on the `edited` + pull_request type too, and each is skipped on a push run, which has no PR to read. - **`check-dependencies.yml`** - "PR Dependency Check". Reads `depends on astubbs/parallel-consumer#N` lines from the PR body and blocks the child until every parent has merged. Produces the **required** check `Check PR Dependencies`, so a stacked PR cannot merge out @@ -125,12 +129,21 @@ document. This section is the detail behind it. `check-quarantine-owners.sh` and `check-docs-data.sh` (each with its self-test) on every PR, so the dedicated jobs were a second copy with a second checkout each. What each carried that the lane did not is now explicit in the job: `COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1` (the scanner's default - on a missing fork point is warn-and-skip, exit 0), and the PyYAML assertion. The lane holds no - token, so `check-quarantine-owners.sh` verifies owner claims only where `gh` is authenticated - + on a missing fork point is warn-and-skip, exit 0), and the PyYAML assertion. The shell sweep sees + no token, so `check-quarantine-owners.sh` verifies owner claims only where `gh` is authenticated - `quarantine-lane.yml`, whose required `tests` check runs it with `github.token` on every PR push. - Copyright rules: [`docs/copyright.md`](copyright.md); the ruleset still names the three retired - contexts until it is edited - see - [`docs/inflight/ci-fewer-jobs-ruleset-edits.md`](inflight/ci-fewer-jobs-ruleset-edits.md). + Copyright rules: [`docs/copyright.md`](copyright.md). + + **A fourth, `PR Checklist` (all of `pr-checklist.yml`), followed the same day** - the PR-body + gates described above, now the last steps of the job. Not a duplicate: those gates need the PR + body and `pulls.listFiles`, so the job gained `pull-requests: read` (its first token use; the shell + sweep still gets no `GH_TOKEN`, so its `gh` calls behave as before) and the `edited` trigger, and + its concurrency group is keyed on the PR number with a SHA fallback so master pushes never cancel + each other. Three of the checklist's named self-test steps were dropped as duplicates of the + sweep's glob; the marker-index check stays as a named step because `bin/todo-index.sh --check` is + not a `check-*.sh` gate. The ruleset still names the four retired contexts until it is edited - + see [`docs/inflight/ci-fewer-jobs-ruleset-edits.md`](inflight/ci-fewer-jobs-ruleset-edits.md), + which also records why this job was the host rather than `Check PR Dependencies`. What the lane covers, and why each one is not obvious: @@ -169,8 +182,9 @@ document. This section is the detail behind it. the single lane that replaced them (and the rest of `repo-hygiene.yml`'s old per-concern jobs), **is in the required list** as of the live check on 2026-09-07 - `gh api repos/astubbs/parallel-consumer/rules/branches/master` enumerates every required context - by name. The three contexts retired into it that day - `Copyright header check`, - `quarantine: audit`, `docs data: audit` - are the removals currently owed to the ruleset; + by name. The four contexts retired into it that day - `Copyright header check`, + `quarantine: audit`, `docs data: audit`, `PR Checklist` - are the removals currently owed to the + ruleset; [`docs/inflight/ci-fewer-jobs-ruleset-edits.md`](inflight/ci-fewer-jobs-ruleset-edits.md) owns that edit. Confirm against the live ruleset rather than assuming this paragraph is current. - `cve-exclusions` runs `bin/check-cve-exclusions.sh`, which **expires temporary CVE @@ -210,7 +224,7 @@ Every one of them was written for the throughput comment in astubbs/parallel-con one of them had been WRONG in production. They live in a module rather than in three copies of the YAML because copying them is how the original defects reached two steps at once. The module's header carries the reasoning and the measurements behind each; `sticky-report-comment.test.js` pins each -against the defect it replaced, and the PR Checklist job runs it. +against the defect it replaced, and the `repo: hygiene` job runs it. **What a "status change" means is the caller's, and only that.** The throughput report's status is its verdict; the quarantine lane's is a sorted digest of every quarantined test's outcome, so a test diff --git a/docs/citations.md b/docs/citations.md index 36c5aab0c..7484668e6 100644 --- a/docs/citations.md +++ b/docs/citations.md @@ -2,7 +2,7 @@ How to fix a reference that no longer resolves, in a document whose claims are fixed. AGENTS.md carries the rule that produces durable citations in the first place - cite a greppable anchor, never -a `file:line`, a form the repo already relies on in `.github/workflows/pr-checklist.yml`, whose +a `file:line`, a form the repo already relies on in `.github/workflows/repo-hygiene.yml`, whose todo-index entries are "keyed by marker TEXT, not line number". This is what to do about the ones written before that rule, and about targets that have since moved, been renamed or been deleted outright. @@ -61,8 +61,8 @@ An anchor nobody ran is a `file:line` with extra steps. ## The path is checked; the anchor is not -**`bin/check-file-refs.sh` fails a cited repo path that does not exist**, and the `PR Checklist` -workflow runs the same module - `.github/scripts/file-ref-gate.js`, unit tested by its self-test, so +**`bin/check-file-refs.sh` fails a cited repo path that does not exist**, and the `repo: hygiene` +gate runs the same module - `.github/scripts/file-ref-gate.js`, unit tested by its self-test, so the local and CI answers cannot drift. It replaces the sentence that used to close the section above: "nothing in CI checks any of this, so the only thing standing between a reader and a confidently wrong pointer is the author having run it." That was true, and it cost what it looks diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md index dcb07173b..b98e8b84b 100644 --- a/docs/inflight/ci-fewer-jobs-ruleset-edits.md +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -1,4 +1,4 @@ -# Ruleset edits owed by the fold of three gate jobs into `repo: hygiene` +# Ruleset edits owed by the fold of four gate jobs into `repo: hygiene` @@ -6,18 +6,21 @@ Three standalone jobs were deleted because `repo: hygiene` already ran every gate they ran, via `bin/check-all.sh --with-tests`'s glob, on every PR: `Copyright header check` (the whole of `.github/workflows/copyright.yml`), and `.github/workflows/maven.yml`'s `quarantine: audit` and -`docs data: audit`. Their names are still **required status-check contexts in the master ruleset**, -and the ruleset is repository settings, not tree state - no PR can change it +`docs data: audit`. A fourth, `PR Checklist` (the whole of `.github/workflows/pr-checklist.yml`), +was deleted because its steps could run as the tail of the same job - see "Why hygiene hosts the +checklist" below. All four names are still **required status-check contexts in the master +ruleset**, and the ruleset is repository settings, not tree state - no PR can change it ([`docs/ci.md`](../ci.md), "The required list is repository settings, not tree state"). - + ## The edit -Remove these three contexts from the master ruleset's `required_status_checks`: +Remove these four contexts from the master ruleset's `required_status_checks`: - `Copyright header check` - `quarantine: audit` - `docs data: audit` +- `PR Checklist` Nothing to add: `repo: hygiene` is already in the required list (verified live 2026-09-07 with `gh api repos/astubbs/parallel-consumer/rules/branches/master`; that command is the answer, not this @@ -27,17 +30,38 @@ sentence). A required context nothing produces leaves every PR **pending** - it never fails, it never passes. -- **Before the merge** the three jobs still run on every other open PR, so dropping the contexts - early only widens the window in which a broken header or a drifted registry could merge on a PR - opened before the fold. +- **Before the merge** the four jobs still run on every other open PR, so dropping the contexts + early only widens the window in which a broken header, a drifted registry or an unresolved + checklist could merge on a PR opened before the fold. - **After the merge** no run produces them, so every PR in the repository pends until somebody edits the ruleset. That is the `spotbugs` incident again ([`docs/ci.md`](../ci.md), "Which checks are required"). So: edit the ruleset in the same sitting as the merge. Only one merge is exposed either way - the -deleting PR's own checks list shows the three contexts as expected-but-missing until the ruleset +deleting PR's own checks list shows the four contexts as expected-but-missing until the ruleset drops them, which is the intended tell that the edit is still owed, not a fault in that PR. +## Why hygiene hosts the checklist, and not another job + +The pairing was decided against the other per-PR candidates, and the reasons are the constraints the +fold had to keep: + +- **`Check PR Dependencies`** (`check-dependencies.yml`) stays its own job. Its action needs + `checks: write`, and it runs on the `closed` trigger so that a parent merging unblocks its + children. Putting PR-authored `github-script` steps in a job that holds `checks: write` would + widen a write grant the repo forbids handing to PR code. +- **The two review gates** (`claude-review`, `review: human LGTM`) stay because each produces a + required check with its own semantics, and a fold would collapse two verdicts into one tick. +- **`repo: hygiene` fits**: both jobs checked out the PR's tree with read-only scope, both ran on + every PR push, and three of the checklist's named self-test steps (`bin/test-check-issue-refs.sh`, + `bin/test-check-file-refs.sh`, `bin/test-todo-index.sh`) were already swept by hygiene's glob. What + the checklist carried that hygiene did not is now explicit in `repo-hygiene.yml`, each with its + reason at the point it appears: the `edited` pull_request type (the gates read the PR body), a + concurrency group keyed on the PR number with a SHA fallback (so master pushes never cancel each + other), `pull-requests: read` (the job's first token use - the shell sweep is still given no + `GH_TOKEN`), `persist-credentials: false` on the checkout, and `!cancelled()` on every folded step + so the job reports every verdict rather than stopping at the first red. + ## What was checked before deleting, so nobody re-derives it - The log of the latest successful `Repo Hygiene` run on master showed `check-copyright-headers.sh`, @@ -46,14 +70,21 @@ drops them, which is the intended tell that the edit is still owed, not a fault not skipping. `bin/inflight.mjs` cannot answer this; the run log is the only record. - `copyright.yml` also ran on `push` to master. `repo-hygiene.yml` runs on push to master too, so that cadence is kept. -- The quarantine **owner** check needs an authenticated `gh`; `repo: hygiene` deliberately holds no - token, so there it degrades to advisory. The authenticated run is `quarantine-lane.yml`'s +- The quarantine **owner** check needs an authenticated `gh`; `repo: hygiene`'s shell sweep is given + no token, so there it degrades to advisory. The authenticated run is `quarantine-lane.yml`'s fail-fast step, with `github.token`, on every PR push and every master push, under the required `tests` check - so an orphaned or overdue owner claim still reds a required check. - `check-all.sh` used to exit 0 on a CANNOT (exit 2) as long as something else ran. The hygiene job now runs `--strict`, under which a CANNOT fails the sweep, and asserts PyYAML and shellcheck are present in named steps - so the folded gates cannot silently turn into skips on an image change. +- For the checklist fold: the three dropped self-test steps were confirmed against + `ls bin/test-*.sh` - each file exists and matches the `bin/test-*.sh` glob `check-all.sh + --with-tests` iterates, so each already ran in this job. `bin/todo-index.sh --check` does not match + `bin/check-*`, and cannot be added to the glob because run bare it regenerates the index + (`docs/inflight/ci-hygiene-gaps.md`), so it stays a named step. Nothing in the sweep consumes the + checkout credential: the only network `git` is `check-quarantine-owners.sh`'s anonymous fetch of the + public origin URL into a scratch git dir, and `gh` reads `GH_TOKEN`. -This note tracks only the owed edit. Once the live ruleset no longer lists the three contexts, +This note tracks only the owed edit. Once the live ruleset no longer lists the four contexts, nothing here is both true and unowned elsewhere - the reasoning is in [`docs/ci.md`](../ci.md) and the `repo-hygiene.yml` header. diff --git a/docs/issue-references.md b/docs/issue-references.md index d1a0bbd4e..8e81c4560 100644 --- a/docs/issue-references.md +++ b/docs/issue-references.md @@ -103,7 +103,7 @@ Not a bulk rewrite - opportunistic, in any file a PR touches anyway: ## The gate, and checking locally -**Check before you push: `bin/check-issue-refs.sh`.** The `PR Checklist` workflow fails a PR whose +**Check before you push: `bin/check-issue-refs.sh`.** The `repo: hygiene` gate fails a PR whose *added* lines contain an unqualified `#NN` below the threshold, and finding that out from CI costs a push cycle for a one-character fix. The script applies the same rule as the gate because it calls the same `.github/scripts/issue-ref-gate.js` module rather than a second copy, so the rule cannot diff --git a/docs/refactoring.md b/docs/refactoring.md index 168e6ab6b..e4c754288 100644 --- a/docs/refactoring.md +++ b/docs/refactoring.md @@ -790,7 +790,7 @@ poll and is covered by `PartitionStateCommittedOffsetTest`. A generated `docs/INACTIVE_TESTS.md` with a `--check` gate (the `bin/todo-index.sh` shape) was considered and **deliberately not built**: the previous audit was lost to invisibility, not drift, and -such a gate would fail the PR Checklist job on any open PR touching a test annotation. Worth +such a gate would fail the `repo: hygiene` job on any open PR touching a test annotation. Worth revisiting once the audit has been in use. diff --git a/docs/releasing.md b/docs/releasing.md index 5da5f25e2..f6a75252b 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -143,7 +143,7 @@ it - and a human should re-apply it before freezing: PRs from one sequence. The file is in the issue-reference gate's `EXEMPT_PATHS` for this reason - everywhere else, [`docs/issue-references.md`](issue-references.md) applies. -## The `PR Checklist` changelog gate is a different, narrower check +## The `repo: hygiene` changelog gate is a different, narrower check `.github/scripts/changelog-ref-gate.js` fails a human PR that adds a `CHANGELOG.adoc` bullet under `Breaking`, `Improvements`, `Fixes` or `Examples` without an explicit `/issues/NN` link. **Do not diff --git a/docs/solutions/workflow-issues/a-clone-detector-cannot-see-code-embedded-in-another-language-2026-08-19.md b/docs/solutions/workflow-issues/a-clone-detector-cannot-see-code-embedded-in-another-language-2026-08-19.md index 4d6273649..4eb5bf67d 100644 --- a/docs/solutions/workflow-issues/a-clone-detector-cannot-see-code-embedded-in-another-language-2026-08-19.md +++ b/docs/solutions/workflow-issues/a-clone-detector-cannot-see-code-embedded-in-another-language-2026-08-19.md @@ -49,8 +49,10 @@ Not scope this time - `bin/` and `.github/` were in scope by then. **Format.** - **PMD CPD parses per language.** It was analysing Java. A `.yml` file is not Java, and a `.sh` file is not Java, so neither side of the clone was ever tokenised. - **jscpd keys format off the file extension** and compares within a format. One side lived in - `.github/workflows/pr-checklist.yml`, the other in `bin/check-file-refs.sh`. A YAML file and a - shell file are different formats, so their contents are never compared with each other. + `.github/workflows/pr-checklist.yml` (`git show 40f176aa3:.github/workflows/pr-checklist.yml` - + that job was folded into `repo: hygiene` and the file deleted), the other in + `bin/check-file-refs.sh`. A YAML file and a shell file are different formats, so their contents + are never compared with each other. The duplicated text was JavaScript **in both places** - embedded in a YAML string on one side and a shell heredoc on the other. Neither engine has a notion of "the language inside this string", and diff --git a/docs/solutions/workflow-issues/a-gate-truncated-the-shared-clone-2026-08-26.md b/docs/solutions/workflow-issues/a-gate-truncated-the-shared-clone-2026-08-26.md index e2620d301..75421f60c 100644 --- a/docs/solutions/workflow-issues/a-gate-truncated-the-shared-clone-2026-08-26.md +++ b/docs/solutions/workflow-issues/a-gate-truncated-the-shared-clone-2026-08-26.md @@ -93,10 +93,11 @@ noise is what gets a hook switched off. ## What was checked and ruled out -- **`.github/workflows/pr-checklist.yml`** runs `git fetch --no-tags --depth=1 origin ` from a - JS step. It is a CI runner's own workspace, checked out shallow on purpose, with no worktrees - sharing it. Not an instance - and out of the hazard gate's corpus, which is `bin/` plus - `.claude/hooks/`. +- **`.github/workflows/pr-checklist.yml`** (`git show 40f176aa3:.github/workflows/pr-checklist.yml` - + that job was folded into `repo: hygiene` and the file deleted) ran `git fetch --no-tags --depth=1 + origin ` from a JS step. It was a CI runner's own workspace, checked out shallow on purpose, + with no worktrees sharing it. Not an instance - and out of the hazard gate's corpus, which is + `bin/` plus `.claude/hooks/`. - **`actions/checkout` with `fetch-depth: 1`** in several workflows: a fresh per-job clone, not a fetch into a shared one. - **`bin/ci-mutation-test.sh`** fetches its base ref with **no** `--depth`, so it writes no `shallow` From c4475ae18ab7f48bb32e13e33e21a5cf3e06e300 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 12:07:54 +1200 Subject: [PATCH 03/14] ci(fewer-jobs): batch the three no-build scanners into one scan: repo job `dups: clones`, `dups: similarity` and `deps: vulnerabilities` each took a runner slot for a scan that finishes in well under two minutes, with no Maven build between them. They are now three steps of one `scan: repo` job in maven.yml: dependency review first because it is the fastest signal, then the two duplicate detectors. Third cut of the ce-optimize run `ci-fewer-jobs`: 23 -> 21 jobs per PR push. Fail at the end, report everything: every scanner step is guarded with `!cancelled()`, so a red first scanner never hides the second or the third, and the job conclusion aggregates them - no verdict step, no continue-on-error. One full-depth checkout serves all three (both duplicate tools compare against the base). The job restates the contents: read and pull-requests: write scopes the clones job carried; maven.yml already granted those workflow-wide, so nothing widened. Each step keeps the name of the job it replaces. The log then reads as the checks list used to, and the sentences across docs/inflight/ of the form "dups: clones found X on astubbs#NNN" stay literally true: only the check name changed. The explanatory comment blocks moved with their steps. Three required contexts go and one arrives; the ordering is in docs/inflight/ci-fewer-jobs-ruleset-edits.md - remove at merge time, promote `scan: repo` once a PR against merged master has produced it. bin/check-pr-analysis-surfaces.sh reads annotations by count and comments by bot type, never by these names, so it is unaffected. Co-authored-by: Claude Fable 5.1 --- .claude/hooks/after-push-check-ci.sh | 2 +- .github/workflows/dependency-audit.yml | 6 +- .github/workflows/maven.yml | 80 +++++++++++-------- docs/ci.md | 9 ++- docs/inflight/branch-package-rename-sweep.md | 3 +- .../ci-duplication-report-can-fail-to-post.md | 3 +- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 60 +++++++++++--- docs/inflight/ci-untriaged-clone-backlog.md | 3 +- 8 files changed, 114 insertions(+), 52 deletions(-) diff --git a/.claude/hooks/after-push-check-ci.sh b/.claude/hooks/after-push-check-ci.sh index 760b52a66..af4c0b4ad 100755 --- a/.claude/hooks/after-push-check-ci.sh +++ b/.claude/hooks/after-push-check-ci.sh @@ -84,7 +84,7 @@ print(json.dumps({ "checks that have already cost it once, both recorded in " "docs/inflight/ci-duplication-report-can-fail-to-post.md:\n" "\n" - "1. A RED CHECK CAN HAVE NO COMMENT. `dups: clones` is a required check, and it has " + "1. A RED CHECK CAN HAVE NO COMMENT. `dups: clones` is a step of the required `scan: repo` check, and it has " "failed on this repo with its finding posted nowhere - GitHub rejected the inline " "annotation and nothing fell back to a summary comment. If a check is red and you " "cannot find a comment explaining it, the finding is in the job log: " diff --git a/.github/workflows/dependency-audit.yml b/.github/workflows/dependency-audit.yml index 53232ff46..59be982c7 100644 --- a/.github/workflows/dependency-audit.yml +++ b/.github/workflows/dependency-audit.yml @@ -4,7 +4,8 @@ # # WHAT THIS COVERS THAT NOTHING ELSE DOES # -# `deps: vulnerabilities` (maven.yml, actions/dependency-review-action) gates PRs, but only on the +# `deps: vulnerabilities` (a step of maven.yml's `scan: repo`, actions/dependency-review-action) +# gates PRs, but only on the # dependencies a PR *changes*. Dependabot alerts cover the standing tree, but read from GitHub's # dependency graph, which holds what the poms DECLARE rather than what Maven resolves - so a purely # transitive component can carry a published advisory and never produce an alert. `lz4-java` is the @@ -89,7 +90,8 @@ jobs: ossindex: # Named for what it does, not for who supplies it. "ossindex" is vendor jargon and unfindable in # a checks list; this also has to be told apart at a glance from `deps: vulnerabilities` - # (maven.yml), which reviews only the dependencies a PR CHANGES - "whole-tree" is the whole + # (a step of maven.yml's `scan: repo`), which reviews only the dependencies a PR CHANGES - + # "whole-tree" is the whole # difference. Job names are an API here, referenced by repo rulesets (see repo-hygiene.yml's # header): renaming is free only while this check is not yet required and not on master. name: "deps: whole-tree CVE scan" diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index c4ded6ef4..151b93424 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -633,13 +633,34 @@ jobs: flags: ak-experimental token: ${{ secrets.CODECOV_TOKEN }} - # Duplicate code detection using both PMD CPD and jscpd engines. - # See https://github.com/astubbs/duplicate-code-cross-check - duplicate-detection: + # ── Scanners (PR-only, no build) ────────────────────────────────────── + + # Three scanners that used to be three jobs - `dups: clones`, `dups: similarity` and + # `deps: vulnerabilities` - now run as steps of one. None of them builds anything; each needs only + # a checkout, and the repo is on the free plan's 20-concurrent-job cap, so three one-minute jobs + # were three slots the sharded suites queued behind (ce-optimize run `ci-fewer-jobs`). + # + # FAIL AT THE END, REPORT EVERYTHING. Every scanner step carries `!cancelled()`, so a red first + # scanner never hides the second or third; the job's own conclusion aggregates the step failures, + # so there is no verdict step and no continue-on-error. Cheapest signal first: dependency review + # takes seconds, the two duplication engines take about a minute each. + # + # One checkout at fetch-depth 0: both duplication tools compare the PR against its base branch + # (`compare_with_base`, `max-increase`), which needs the base's history on disk. + # + # THE OLD JOB NAMES LIVE ON AS STEP NAMES, so the log reads the same way the checks list used to, + # and every doc that says "`dups: clones` found ..." still names the thing that found it. What did + # change is the check: `scan: repo` is the required-check name in the making, and the three names + # it replaces are still in the master ruleset. docs/inflight/ci-fewer-jobs-ruleset-edits.md owns + # the swap and its ordering. + scan: if: github.event_name == 'pull_request' - name: "dups: clones" + name: "scan: repo" runs-on: ubuntu-latest - timeout-minutes: 5 + timeout-minutes: 10 + # All three tools post PR comments: the two duplication tools through their github-token / + # github_token inputs, dependency review through comment-summary-in-pr. Same grant the clones job + # carried; nothing widens. permissions: contents: read pull-requests: write @@ -647,6 +668,20 @@ jobs: - uses: actions/checkout@v6 with: fetch-depth: 0 + + # Dependency vulnerability scanning - GitHub's own dependency review + - name: "deps: vulnerabilities" + uses: actions/dependency-review-action@v4 + if: ${{ !cancelled() }} + with: + fail-on-severity: high + comment-summary-in-pr: always + # github-script is MIT, but the license lookup returns null for + # floating major tags (@v7 -> "7.*.*"), so it warns as "unknown" + allow-dependencies-licenses: pkg:githubactions/actions/github-script + + # Duplicate code detection using both PMD CPD and jscpd engines. + # See https://github.com/astubbs/duplicate-code-cross-check # v1.1.1. The bump is not routine: at c828d78 this action installed jscpd with `npx jscpd@4` # INSIDE the step it measures, so the base-branch engine step paid a cold install the PR-branch # step did not - measured at 288s against 15s on the same tree - and npm's default 300s @@ -656,7 +691,9 @@ jobs: # It also stopped unzipping its own PMD distribution into the tree it scans. That had inflated # the duplication DENOMINATOR ~4x, so every percentage this check reported was about a quarter # of the truth - see cpd-max-duplication below. - - uses: astubbs/duplicate-code-cross-check@b462149cbfcdc7585c851e3109763c72787699a7 # v1.1.1 + - name: "dups: clones" + uses: astubbs/duplicate-code-cross-check@b462149cbfcdc7585c851e3109763c72787699a7 # v1.1.1 + if: ${{ !cancelled() }} with: github-token: ${{ secrets.GITHUB_TOKEN }} # SCAN EVERYTHING. There is nothing here to exclude, and three earlier attempts at a @@ -712,17 +749,10 @@ jobs: cpd-max-duplication: '0.5' # measured baseline 0.11% jscpd-max-duplication: '2' # measured baseline 1.17% - # File similarity detection - finds files that are semantically similar overall - file-similarity: - if: github.event_name == 'pull_request' - name: "dups: similarity" - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: - - uses: actions/checkout@v6 - with: - fetch-depth: 0 - - uses: astubbs/duplicate-code-detection-tool@74adf74b65b6a288b21ba957d7d7bef43831ee65 # master: base-vs-PR comparison + ce-review fixes + # File similarity detection - finds files that are semantically similar overall + - name: "dups: similarity" + uses: astubbs/duplicate-code-detection-tool@74adf74b65b6a288b21ba957d7d7bef43831ee65 # master: base-vs-PR comparison + ce-review fixes + if: ${{ !cancelled() }} with: github_token: ${{ secrets.GITHUB_TOKEN }} # Scan everything, same reasoning as the clones job. `file_extensions: java` is what @@ -952,22 +982,6 @@ jobs: await github.rest.issues.createComment({ owner, repo, issue_number, body: stamped }); } - # Dependency vulnerability scanning - GitHub's own dependency review - dependency-scan: - if: github.event_name == 'pull_request' - name: "deps: vulnerabilities" - runs-on: ubuntu-latest - timeout-minutes: 5 - steps: - - uses: actions/checkout@v6 - - uses: actions/dependency-review-action@v4 - with: - fail-on-severity: high - comment-summary-in-pr: always - # github-script is MIT, but the license lookup returns null for - # floating major tags (@v7 -> "7.*.*"), so it warns as "unknown" - allow-dependencies-licenses: pkg:githubactions/actions/github-script - # ── Push Builds (master) ─────────────────────────────────────────────── # Single full build on default Kafka version to gate SNAPSHOT publishing. diff --git a/docs/ci.md b/docs/ci.md index 8d3bcfe23..6f2d64b17 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -70,8 +70,11 @@ document. This section is the detail behind it. **gating**, like the job they replaced: a chaos RED is a real finding. The **`Integration Tests`** lane is likewise two gating shards since astubbs#442 - a named heavy set and a catch-all defined by subtraction; see - ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries SpotBugs, duplicate - detection, PR-scoped mutation testing (PIT), and dependency vulnerability scanning. Push to + ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries SpotBugs, PR-scoped + mutation testing (PIT), and the `scan: repo` job - the two duplication scanners and dependency + vulnerability review as steps of one no-build job, each step keeping the name of the job it used + to be (`dups: clones`, `dups: similarity`, `deps: vulnerabilities`), so a red step still reads the + way the red check did. Push to master runs a single full `bin/ci-build.sh` on the default Kafka version to gate SNAPSHOT publishing. All jobs use explicit `cache/restore` with rotating keys from the `prepare-deps` job - never `setup-java cache: 'maven'`. @@ -542,7 +545,7 @@ runner count rather than about this workflow: - **`cancel-closed-pr-runs.yml`** - cancels a PR's in-flight runs when it closes, so a withdrawn PR stops occupying runners. Housekeeping only; gates nothing. - **`dependency-audit.yml`** - "Dependency Audit", job `deps: whole-tree CVE scan`. Named against - `deps: vulnerabilities` (`maven.yml`), which reviews only the dependencies a PR *changes*; this one + `deps: vulnerabilities` (a step of `maven.yml`'s `scan: repo`), which reviews only the dependencies a PR *changes*; this one scans the whole resolved tree. The **only** place `ossindex-maven-plugin` is switched on (`-Dossindex.skip=false`); it binds to `validate`, so enabling it globally would mean six-plus scans per PR from one account. Runs on every in-repo PR, on dispatch, and **weekly on a schedule** diff --git a/docs/inflight/branch-package-rename-sweep.md b/docs/inflight/branch-package-rename-sweep.md index bbc82e13c..5989f839c 100644 --- a/docs/inflight/branch-package-rename-sweep.md +++ b/docs/inflight/branch-package-rename-sweep.md @@ -46,7 +46,8 @@ the base, not the contributor's work. ### Also waiting on a human, lower priority -- **`dups: similarity` is a required check and fails on any renamed branch, as a false positive.** +- **`dups: similarity` (a step of the `scan: repo` check) fails on any renamed branch, as a false + positive.** The five near-identical `TestConventionsArchTest.java` files score 89-91% against each other and always have — on astubbs#293 they score 89.57-91.04 and the job passes. The same numbers fail on astubbs#294 because the check compares against base *by file path*, and after a 234-file rename no diff --git a/docs/inflight/ci-duplication-report-can-fail-to-post.md b/docs/inflight/ci-duplication-report-can-fail-to-post.md index d249e3d51..215c5e013 100644 --- a/docs/inflight/ci-duplication-report-can-fail-to-post.md +++ b/docs/inflight/ci-duplication-report-can-fail-to-post.md @@ -32,7 +32,8 @@ was **eight days older** than the clone it was supposed to describe. ## Why nobody noticed -A red `dups: clones` with no comment is indistinguishable, at a glance, from the other red ticks a +A red `dups: clones` (since the `scan: repo` fold, a red step inside that check) with no comment is +indistinguishable, at a glance, from the other red ticks a long-running PR accumulates. The finding is one line inside a job log nobody has a reason to open, because the tool's contract is that it comments. diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md index b98e8b84b..35858a4ed 100644 --- a/docs/inflight/ci-fewer-jobs-ruleset-edits.md +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -1,4 +1,4 @@ -# Ruleset edits owed by the fold of four gate jobs into `repo: hygiene` +# Ruleset edits owed by the folds into `repo: hygiene` and `scan: repo` @@ -8,23 +8,31 @@ Three standalone jobs were deleted because `repo: hygiene` already ran every gat `.github/workflows/copyright.yml`), and `.github/workflows/maven.yml`'s `quarantine: audit` and `docs data: audit`. A fourth, `PR Checklist` (the whole of `.github/workflows/pr-checklist.yml`), was deleted because its steps could run as the tail of the same job - see "Why hygiene hosts the -checklist" below. All four names are still **required status-check contexts in the master -ruleset**, and the ruleset is repository settings, not tree state - no PR can change it +checklist" below. Three more, `maven.yml`'s no-build scanners `dups: clones`, `dups: similarity` and +`deps: vulnerabilities`, became steps of one new job, `scan: repo` - see "The scanner fold" below. +All seven old names are still **required status-check contexts in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it ([`docs/ci.md`](../ci.md), "The required list is repository settings, not tree state"). ## The edit -Remove these four contexts from the master ruleset's `required_status_checks`: +Remove these seven contexts from the master ruleset's `required_status_checks`: - `Copyright header check` - `quarantine: audit` - `docs data: audit` - `PR Checklist` +- `dups: clones` +- `dups: similarity` +- `deps: vulnerabilities` -Nothing to add: `repo: hygiene` is already in the required list (verified live 2026-09-07 with +Add one: + +- `scan: repo` + +`repo: hygiene` needs no add - it is already in the required list (verified live 2026-09-07 with `gh api repos/astubbs/parallel-consumer/rules/branches/master`; that command is the answer, not this -sentence). +sentence). `scan: repo` is new, so it is not. ## When: at the merge of the deleting PR, not before and not after @@ -38,9 +46,22 @@ A required context nothing produces leaves every PR **pending** - it never fails required"). So: edit the ruleset in the same sitting as the merge. Only one merge is exposed either way - the -deleting PR's own checks list shows the four contexts as expected-but-missing until the ruleset +deleting PR's own checks list shows the seven contexts as expected-but-missing until the ruleset drops them, which is the intended tell that the edit is still owed, not a fault in that PR. +**The add has the opposite ordering.** A new required context that no master run has produced +leaves every PR pending ([`docs/ci.md`](../ci.md), "Which checks are required": a check is promoted +only once the job that emits it is already on master). `scan: repo` is `pull_request`-only, so +strictly it never runs *on* master; the condition that matters is that the job definition is on +master, so every PR opened afterwards produces the context. Sequence at the merge, then: + +1. Merge the deleting PR. +2. In the same sitting, remove the seven old contexts. +3. Add `scan: repo` only after the merge has landed and a PR run has produced the context - the + first PR to rebase onto the merged master shows `scan: repo` in its checks list; that is the + evidence. Until then the three scanners gate nothing, which is a window measured in one PR's + CI run, and is preferable to every PR pending on a context nothing yet produces. + ## Why hygiene hosts the checklist, and not another job The pairing was decided against the other per-PR candidates, and the reasons are the constraints the @@ -85,6 +106,25 @@ fold had to keep: checkout credential: the only network `git` is `check-quarantine-owners.sh`'s anonymous fetch of the public origin URL into a scratch git dir, and `gh` reads `GH_TOKEN`. -This note tracks only the owed edit. Once the live ruleset no longer lists the four contexts, -nothing here is both true and unowned elsewhere - the reasoning is in [`docs/ci.md`](../ci.md) and -the `repo-hygiene.yml` header. +## The scanner fold + +`dups: clones`, `dups: similarity` and `deps: vulnerabilities` each checked out the tree without +building it, ran one third-party action, and posted a PR comment - about a minute of work apiece in +three job slots. They are now three steps of `scan: repo` in `maven.yml`, and the constraints the +fold kept are stated as comments on the job itself: `!cancelled()` on every scanner step so the +first red never hides the others, no verdict step and no `continue-on-error` because the job's own +conclusion aggregates the step results, one `fetch-depth: 0` checkout because both duplication +tools compare against the base branch, the clones job's `contents: read` + `pull-requests: write` +grant restated on the merged job and nothing wider, and dependency review first so the cheapest +signal lands first in the log. **The three old job names live on as the step names**, so a red step +reads in the log the way the red check used to, and the references across `docs/inflight/` to what +`dups: clones` or `dups: similarity` found stay accurate - only the check name changed. + +Not folded into `repo: hygiene`: hygiene runs PR-authored scripts under a read-only token by design, +and all three scanners need `pull-requests: write` to post. Keeping the write grant in a job that +runs only pinned third-party actions is the same reviewer-isolation line `docs/ci.md` draws for +`Check PR Dependencies`. + +This note tracks only the owed edits. Once the live ruleset lists `scan: repo` and none of the +seven old contexts, nothing here is both true and unowned elsewhere - the reasoning is in +[`docs/ci.md`](../ci.md), the `repo-hygiene.yml` header and the `scan: repo` job's own comments. diff --git a/docs/inflight/ci-untriaged-clone-backlog.md b/docs/inflight/ci-untriaged-clone-backlog.md index 59697584e..a418c7230 100644 --- a/docs/inflight/ci-untriaged-clone-backlog.md +++ b/docs/inflight/ci-untriaged-clone-backlog.md @@ -22,7 +22,8 @@ Five clones appeared purely from looking somewhere new. They are **not** introdu ## What is not known **Where they are.** The PR comment reports totals only. Enumerating them means running the engines -locally; the CI job's own configuration in `.github/workflows/maven.yml` (the `dups: clones` job) +locally; the CI step's own configuration in `.github/workflows/maven.yml` (the `dups: clones` step +of the `scan: repo` job) is the source of truth for how to invoke them, and the action it uses installs both. That is deliberately not written down here - it is a command's answer, and it will be stale within a From 0d504fc834b7ea424263a6988a3c4330a8e16b6c Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 12:23:56 +1200 Subject: [PATCH 04/14] ci(fewer-jobs): batch Infer and SpotBugs into one static: analysis job The two build-dependent analysers each waited on the dependency cache job, then restored the same Maven cache with a byte-identical step, to run for about a minute and about six. They are now two steps of one `static: analysis` job: Infer first because it is the cheaper signal, then SpotBugs with its annotation and sticky-comment steps unchanged. Fourth and last cut of the ce-optimize run `ci-fewer-jobs`: 21 -> 20 jobs per PR push, and 27 -> 20 across the run. Fail at the end, report everything: each analyser step is guarded with `!cancelled()`, so a red Infer never hides SpotBugs, and the two reporting steps that were `always()` are now `!cancelled()` for the same reason. The SpotBugs summary step keeps its deliberate `continue-on-error`. The job conclusion aggregates; no verdict step. This is the last safe fold of build-dependent lanes. The two together run to roughly seven minutes against a Unit Tests critical path of about nine, so the headroom is nearly spent - folding the PIT lane in as well would blow through it, because that lane runs to half an hour whenever it has something to mutate. Permissions narrowed rather than widened: neither job declared any, so both inherited the workflow-wide grant. The merged job declares only what the two used - contents read, pull-requests write for the sticky comment, checks write for the annotations - and drops actions: read, which neither analyser touched. A check rename breaks consumers that match by name, even when the step names are preserved. bin/check-pr-analysis-surfaces.sh filters check runs through a name regex, and `racerd` had been dead in it since the job became `static: infer`, dropping that surface from the listing with nothing to say so. The pattern now names the live checks and carries the rule. Nine required contexts now go and two arrive; docs/inflight/ci-fewer-jobs-ruleset-edits.md carries the list and the ordering for both directions. Co-authored-by: Claude Fable 5.1 --- .github/workflows/README.md | 2 +- .github/workflows/maven.yml | 114 +++++++++++-------- bin/check-pr-analysis-surfaces.sh | 7 +- docs/ci.md | 19 ++-- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 79 ++++++++++--- 5 files changed, 143 insertions(+), 78 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 406c271c9..c08485153 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -26,7 +26,7 @@ procedure or a steer. Why, and the gate's exact contract: [`docs/ci.md`](../../d | `claude-code-review.yml` | The review gate. Invokes no Claude and costs nothing. What satisfies it: [`docs/ci.md`](../../docs/ci.md), stated once there. | | `claude.yml` | Answers `@claude` comments, and reviews when asked to - the only route that can raise inline review threads. | | `dependency-audit.yml` | Scans the **whole** resolved dependency tree for CVEs - the only place OSS Index is switched on. Per-PR, on demand, and weekly, because a new advisory needs no push to arrive. | -| `maven.yml` | The main build: unit, integration and performance suites, SpotBugs, duplicate detection, PIT, dependency scanning. | +| `maven.yml` | The main build: unit, integration and performance suites, static analysis (Infer and SpotBugs, batched as `static: analysis`), duplicate detection, PIT, dependency scanning. | | `mutation-full-sweep.yml` | The whole-repo PIT mutation sweep - nightly, plus on demand. Too slow for a PR. Self-hosted high-CPU lane, plus a hosted trial arm. | | `publish.yml` | Publishes to Maven Central on every push to `master`; the pom version decides snapshot or release. | | `quarantine-lane.yml` | Runs the quarantined tests separately, so known-flaky tests neither block nor disappear. | diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 151b93424..ccca10b36 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -770,38 +770,44 @@ jobs: compare_with_base: true max_increase: 10 - # Meta's Infer, every Java checker it ships - not just RacerD. RacerD is the only detector here - # that finds races WITHOUT being told where to look: Error Prone's GuardedBy only fires where - # somebody wrote the annotation, and this codebase has none, so the racing-double seam tests can - # only re-prove seams already found by hand. RacerD infers which locks protect which state from - # how the code uses them, and it is still why this lane exists. Alongside it, bin/infer-test.sh - # also runs pulse (null derefs, resource leaks), starvation (deadlock via lock-order inversion), - # and the checkers enabled by --annotation-reachability --bufferoverrun --cost --loop-hoisting - # --scope-leakage - `--racerd-only` used to disable everything but RacerD, including two checkers - # that are on by default, which is why the flag is gone and the lane was renamed from RacerD to - # Infer to match what it actually runs. + # TWO STATIC ANALYSERS IN ONE JOB. They ran as `static: infer` and `static: spotbugs`: both waited + # on prepare-deps and both restored the same Maven cache with an identical step, so batching them + # restores it once and gives a runner slot back. Infer runs first - it is the cheaper signal by a + # wide margin - and the pair still finishes inside the unit-test lane that sets the critical path, + # so the batch does not become the longest job. # - # AN IDENTITY SET, not a count. config/infer-known-findings.txt records which findings are known, - # keyed on bug type plus class.method. A bare count ceiling could not distinguish "one finding - # fixed" from "one finding swapped for a different one" - both leave the total unchanged - which an - # independent cross-model review caught, and which is the same reports-green-while-it-changed class - # this lane exists to police. One line per known identity, checked in, so both directions are a - # visible diff. + # Fail at the end, report both: every analysis step is guarded with `!cancelled()`, so a red Infer + # never hides SpotBugs and the job conclusion aggregates them. No verdict step, and no new + # continue-on-error - the one on the SpotBugs summary comment below is deliberate and unchanged. # - # Fails BOTH ways: a new identity is a new finding, and a known one that stops firing means - # somebody fixed it without ratcheting the file. - infer: + # THE OLD JOB NAMES LIVE ON AS STEP NAMES, so the log reads the way the checks list used to and + # prose citing `static: infer` or `static: spotbugs` still names the thing that ran. What changed + # is the check: `static: analysis` is the required-check name in the making, and the two names it + # replaces are still in the master ruleset. docs/inflight/ci-fewer-jobs-ruleset-edits.md owns the + # swap and its ordering. + static: if: github.event_name == 'pull_request' - name: "static: infer" + name: "static: analysis" runs-on: ubuntu-latest - timeout-minutes: 15 + # The two jobs held 15 and 10. The sum plus an Infer toolchain download on a cache miss needs + # headroom, without leaving a hung tool sitting for an hour. + timeout-minutes: 30 needs: prepare-deps + # The union of what the two jobs held. Neither declared job-level permissions, so both inherited + # the workflow-wide grant: the SpotBugs annotation action needs checks: write, its sticky comment + # needs pull-requests: write, and Infer needs nothing beyond the checkout. Declaring the union + # here also drops the workflow's actions: read, which neither job used - nothing widens. + permissions: + contents: read + pull-requests: write + checks: write steps: - uses: actions/checkout@v6 - uses: actions/setup-java@v5 with: distribution: 'temurin' java-version: '17' + # ONE restore for both analysers - the two jobs each carried an identical copy of this step. - name: Restore Maven cache uses: actions/cache/restore@v4 with: @@ -810,11 +816,33 @@ jobs: restore-keys: | setup-java-Linux-x64-maven- + # Meta's Infer, every Java checker it ships - not just RacerD. RacerD is the only detector here + # that finds races WITHOUT being told where to look: Error Prone's GuardedBy only fires where + # somebody wrote the annotation, and this codebase has none, so the racing-double seam tests can + # only re-prove seams already found by hand. RacerD infers which locks protect which state from + # how the code uses them, and it is still why this lane exists. Alongside it, bin/infer-test.sh + # also runs pulse (null derefs, resource leaks), starvation (deadlock via lock-order inversion), + # and the checkers enabled by --annotation-reachability --bufferoverrun --cost --loop-hoisting + # --scope-leakage - `--racerd-only` used to disable everything but RacerD, including two checkers + # that are on by default, which is why the flag is gone and the lane was renamed from RacerD to + # Infer to match what it actually runs. + # + # AN IDENTITY SET, not a count. config/infer-known-findings.txt records which findings are known, + # keyed on bug type plus class.method. A bare count ceiling could not distinguish "one finding + # fixed" from "one finding swapped for a different one" - both leave the total unchanged - which an + # independent cross-model review caught, and which is the same reports-green-while-it-changed class + # this lane exists to police. One line per known identity, checked in, so both directions are a + # visible diff. + # + # Fails BOTH ways: a new identity is a new finding, and a known one that stops firing means + # somebody fixed it without ratcheting the file. + # Keyed on the version alone - the toolchain changes only when we bump it, which makes this a # better cache key than the pom-hash ones above. Unpacked it is ~950 MB, so the TARBALL is what # gets cached and it is unpacked per run: the repo-wide cache budget is 10 GB and several jobs # already hold ~/.m2. - name: Restore the Infer toolchain + if: ${{ !cancelled() }} id: infer-cache uses: actions/cache@v4 with: @@ -822,7 +850,7 @@ jobs: key: infer-linux-x86_64-v1.3.0 - name: Download Infer - if: steps.infer-cache.outputs.cache-hit != 'true' + if: ${{ !cancelled() && steps.infer-cache.outputs.cache-hit != 'true' }} run: | mkdir -p ~/infer-dist curl -sSL --fail -o ~/infer-dist/infer.tar.xz \ @@ -840,50 +868,35 @@ jobs: # to force a clean download and see whether the digest returns; a genuine upstream re-cut of a # published tag is the rarer explanation and should be confirmed against the release page. - name: Verify the Infer archive against its pinned digest + if: ${{ !cancelled() }} run: | echo "02be62ba931cb43e2d42bbf7abf4bd9a410cf8464cce5453934e25c90a825850 $HOME/infer-dist/infer.tar.xz" \ | sha256sum --check --strict - - name: Unpack and confirm it runs + if: ${{ !cancelled() }} run: | mkdir -p ~/infer tar -xJf ~/infer-dist/infer.tar.xz -C ~/infer --strip-components=1 ~/infer/bin/infer --version # Asserts the ceiling in both directions. Analysis itself is ~11s over core's 73 files; the - # timeout is for the download on a cache miss. + # job timeout above is sized for the toolchain download on a cache miss, not for this. # INFER_BIN and INFER_JDK are set here rather than in an `env:` block on purpose. An earlier # version set all three in `env:` AND overrode two of them inline, so the env values were dead # - and worse, wrong: INFER_BIN pointed at the workspace, while the unpack step above puts the # toolchain in $HOME/infer. Deleting the inline prefix, which is exactly what a later tidy-up # would do, would have pointed the lane at nothing. `$HOME` and `$JAVA_HOME` are not available # to `env:` at all, which is what forced the inline form in the first place. - - name: Infer + - name: "static: infer" + if: ${{ !cancelled() }} run: INFER_BIN="$HOME/infer/bin/infer" INFER_JDK="$JAVA_HOME" bash bin/infer-test.sh - # SpotBugs static analysis - finds null derefs, concurrency issues, resource leaks. - # Whole reactor, main AND test code, with fb-contrib and find-sec-bugs detectors loaded. - # Suppression is by rule in config/spotbugs-exclude.xml; the registry that justifies every entry is - # docs/inflight/static-spotbugs-rule-registry.md. - spotbugs: - if: github.event_name == 'pull_request' - name: "static: spotbugs" - runs-on: ubuntu-latest - timeout-minutes: 10 - needs: prepare-deps - steps: - - uses: actions/checkout@v6 - - uses: actions/setup-java@v5 - with: - distribution: 'temurin' - java-version: '17' - - name: Restore Maven cache - uses: actions/cache/restore@v4 - with: - path: ~/.m2/repository - key: setup-java-Linux-x64-maven-${{ hashFiles('**/pom.xml') }} - restore-keys: | - setup-java-Linux-x64-maven- + # SpotBugs static analysis - finds null derefs, concurrency issues, resource leaks. + # Whole reactor, main AND test code, with fb-contrib and find-sec-bugs detectors loaded. + # Suppression is by rule in config/spotbugs-exclude.xml; the registry that justifies every entry is + # docs/inflight/static-spotbugs-rule-registry.md. + # # No baseline restore, and no baseline job: suppression is by RULE now, in the checked-in # config/spotbugs-exclude.xml the pom points at, with every entry justified in # docs/inflight/static-spotbugs-rule-registry.md. The job this replaces regenerated an exclude @@ -911,16 +924,17 @@ jobs: # Not bound to a lifecycle phase on purpose: SpotBugs at effort=Max over the whole reactor is # minutes, and paying that on every local `mvn test` would get the plugin switched off. Run the # same two goals by hand when you want the list. - - name: Compile and run SpotBugs + - name: "static: spotbugs" + if: ${{ !cancelled() }} run: ./mvnw --batch-mode -Pci test-compile spotbugs:spotbugs spotbugs:check -Dspotbugs.failOnError=false - name: Post SpotBugs annotations on PR - if: always() + if: ${{ !cancelled() }} uses: jwgmeligmeyling/spotbugs-github-action@v1.2 with: path: '**/target/spotbugsXml.xml' token: ${{ secrets.GITHUB_TOKEN }} - name: Post SpotBugs summary to PR - if: always() + if: ${{ !cancelled() }} # A COMMENT WRITE MUST NOT FAIL THE JOB - the same reasoning as the throughput step above, and # missing here for the same reason it was missing there. This step summarises analysis that has # already run and already posted its annotations, so a rate limit or a 500 while posting would diff --git a/bin/check-pr-analysis-surfaces.sh b/bin/check-pr-analysis-surfaces.sh index 66094f6e8..d970b8747 100755 --- a/bin/check-pr-analysis-surfaces.sh +++ b/bin/check-pr-analysis-surfaces.sh @@ -258,8 +258,13 @@ echo "========================================================================== echo "SURFACES THIS SCRIPT CANNOT READ - open these by hand" echo "================================================================================================" echo " Job summaries (PIT survivor table, CVE tables) are not exposed by the REST API." +# THIS MATCHES THE CHECK NAME, WHICH IS A JOB NAME, WHICH IS AN API. A job rename drops its surface +# from this listing silently - nothing fails, the row simply stops appearing, and the reader cannot +# tell "no summary to read" from "we stopped looking". `racerd` outlived the job that became +# `static: infer` this way, and was dead here until the 2026-09-07 job folds forced a re-read. +# Re-check this pattern whenever a job in .github/workflows/ is renamed or folded. gh api "repos/${REPO}/commits/${HEAD_SHA}/check-runs?per_page=100" \ - --jq '.check_runs[] | select(.name | test("Mutation|spotbugs|racerd|CVE|Quarantine")) | " \(.name): \(.html_url)"' \ + --jq '.check_runs[] | select(.name | test("Mutation|static: analysis|scan: repo|CVE|Quarantine")) | " \(.name): \(.html_url)"' \ 2>/dev/null | sort -u || true echo echo " Console-only output: a tool that prints to the Maven log and does not annotate is invisible" diff --git a/docs/ci.md b/docs/ci.md index 6f2d64b17..ef7550c0e 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -70,11 +70,12 @@ document. This section is the detail behind it. **gating**, like the job they replaced: a chaos RED is a real finding. The **`Integration Tests`** lane is likewise two gating shards since astubbs#442 - a named heavy set and a catch-all defined by subtraction; see - ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries SpotBugs, PR-scoped - mutation testing (PIT), and the `scan: repo` job - the two duplication scanners and dependency - vulnerability review as steps of one no-build job, each step keeping the name of the job it used - to be (`dups: clones`, `dups: similarity`, `deps: vulnerabilities`), so a red step still reads the - way the red check did. Push to + ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries PR-scoped + mutation testing (PIT) and two batched jobs: **`static: analysis`** - Infer then SpotBugs, the + cheaper signal first - and **`scan: repo`** - the two duplication scanners and dependency + vulnerability review, no build between them. In both, each step keeps the name of the job it used + to be (`static: infer`, `static: spotbugs`; `dups: clones`, `dups: similarity`, + `deps: vulnerabilities`), so a red step still reads the way the red check did. Push to master runs a single full `bin/ci-build.sh` on the default Kafka version to gate SNAPSHOT publishing. All jobs use explicit `cache/restore` with rotating keys from the `prepare-deps` job - never `setup-java cache: 'maven'`. @@ -295,7 +296,9 @@ gh api repos/astubbs/parallel-consumer/rulesets/ \ check is only promoted once the job that emits it is already on master. The same ordering governs a renamed job: the ruleset keeps the old name, which then blocks nothing visibly and passes never. That is how a bare `spotbugs` context outlived the job that became `static: spotbugs` and sat required with -no producer until 2026-08-26. **A skip does not satisfy a required check either** - it waits - so a +no producer until 2026-08-26. That job name has since gone the same way: `static: infer` and +`static: spotbugs` are steps of `static: analysis` now, and both contexts are on the removal list in +the note below. **A skip does not satisfy a required check either** - it waits - so a job that can legitimately have nothing in scope should report success rather than skip before anyone requires it. **Removals are the other half of the same ordering**: a job deleted from the tree leaves its context in the ruleset, required and never produced, so every PR pends until the ruleset drops @@ -314,8 +317,8 @@ every other PR) and not after (nothing merges). The live instance of this is | `Analyze (actions)`, `Analyze (java-kotlin)`, `Analyze (python)` | The `CodeQL` aggregate above is already required and covers all three | This table is the durable half of a note that has been retired: the three ruleset edits it tracked - -adding `Chaos Pain Suite` once it reached master, adding `static: infer`, and removing the orphaned -`spotbugs` - were made on 2026-08-26. The reasoning survives it, because the failure it prevents is +adding `Chaos Pain Suite` once it reached master, adding `static: infer` (since folded into +`static: analysis`), and removing the orphaned `spotbugs` - were made on 2026-08-26. The reasoning survives it, because the failure it prevents is someone re-proposing one of the rows above and re-deriving why it does not work. **`Chaos Pain Suite` was promoted without waiting for a bake-in period**, deliberately and against diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md index 35858a4ed..910163b80 100644 --- a/docs/inflight/ci-fewer-jobs-ruleset-edits.md +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -1,4 +1,4 @@ -# Ruleset edits owed by the folds into `repo: hygiene` and `scan: repo` +# Ruleset edits owed by the folds into `repo: hygiene`, `scan: repo` and `static: analysis` @@ -9,14 +9,16 @@ Three standalone jobs were deleted because `repo: hygiene` already ran every gat `docs data: audit`. A fourth, `PR Checklist` (the whole of `.github/workflows/pr-checklist.yml`), was deleted because its steps could run as the tail of the same job - see "Why hygiene hosts the checklist" below. Three more, `maven.yml`'s no-build scanners `dups: clones`, `dups: similarity` and -`deps: vulnerabilities`, became steps of one new job, `scan: repo` - see "The scanner fold" below. -All seven old names are still **required status-check contexts in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it +`deps: vulnerabilities`, became steps of one new job, `scan: repo` - see "The scanner fold" below. Two more, `maven.yml`'s +build-dependent static analysers `static: infer` and `static: spotbugs`, became steps of one new job, +`static: analysis` - see "The static-analysis fold" below. +All nine old names are still **required status-check contexts in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it ([`docs/ci.md`](../ci.md), "The required list is repository settings, not tree state"). ## The edit -Remove these seven contexts from the master ruleset's `required_status_checks`: +Remove these nine contexts from the master ruleset's `required_status_checks`: - `Copyright header check` - `quarantine: audit` @@ -25,14 +27,17 @@ Remove these seven contexts from the master ruleset's `required_status_checks`: - `dups: clones` - `dups: similarity` - `deps: vulnerabilities` +- `static: infer` +- `static: spotbugs` -Add one: +Add two: - `scan: repo` +- `static: analysis` `repo: hygiene` needs no add - it is already in the required list (verified live 2026-09-07 with `gh api repos/astubbs/parallel-consumer/rules/branches/master`; that command is the answer, not this -sentence). `scan: repo` is new, so it is not. +sentence). `scan: repo` and `static: analysis` are new, so they are not. ## When: at the merge of the deleting PR, not before and not after @@ -46,21 +51,23 @@ A required context nothing produces leaves every PR **pending** - it never fails required"). So: edit the ruleset in the same sitting as the merge. Only one merge is exposed either way - the -deleting PR's own checks list shows the seven contexts as expected-but-missing until the ruleset +deleting PR's own checks list shows the nine contexts as expected-but-missing until the ruleset drops them, which is the intended tell that the edit is still owed, not a fault in that PR. **The add has the opposite ordering.** A new required context that no master run has produced leaves every PR pending ([`docs/ci.md`](../ci.md), "Which checks are required": a check is promoted -only once the job that emits it is already on master). `scan: repo` is `pull_request`-only, so -strictly it never runs *on* master; the condition that matters is that the job definition is on -master, so every PR opened afterwards produces the context. Sequence at the merge, then: +only once the job that emits it is already on master). Both `scan: repo` and `static: analysis` are +`pull_request`-only, so strictly neither ever runs *on* master; the condition that matters is that +the job definition is on master, so every PR opened afterwards produces the context. Sequence at the +merge, then: 1. Merge the deleting PR. -2. In the same sitting, remove the seven old contexts. -3. Add `scan: repo` only after the merge has landed and a PR run has produced the context - the - first PR to rebase onto the merged master shows `scan: repo` in its checks list; that is the - evidence. Until then the three scanners gate nothing, which is a window measured in one PR's - CI run, and is preferable to every PR pending on a context nothing yet produces. +2. In the same sitting, remove the nine old contexts. +3. Add `scan: repo` and `static: analysis` only after the merge has landed and a PR run has produced + each context - the first PR to rebase onto the merged master shows both in its checks list; that + is the evidence. Until then the three scanners and the two analysers gate nothing, which is a + window measured in one PR's CI run, and is preferable to every PR pending on a context nothing + yet produces. ## Why hygiene hosts the checklist, and not another job @@ -125,6 +132,42 @@ and all three scanners need `pull-requests: write` to post. Keeping the write gr runs only pinned third-party actions is the same reviewer-isolation line `docs/ci.md` draws for `Check PR Dependencies`. -This note tracks only the owed edits. Once the live ruleset lists `scan: repo` and none of the -seven old contexts, nothing here is both true and unowned elsewhere - the reasoning is in -[`docs/ci.md`](../ci.md), the `repo-hygiene.yml` header and the `scan: repo` job's own comments. +## The static-analysis fold + +`static: infer` and `static: spotbugs` were the two build-dependent analysers: both `needs: +prepare-deps`, both restored the same Maven cache with a byte-identical `actions/cache/restore` step, +and between them they held two runner slots for work that fits comfortably inside one. They are now +two steps of `static: analysis` in `maven.yml`, and the constraints the fold kept are stated as +comments on the job itself: **Infer first** because it is the cheaper signal by a wide margin, so it +lands in the log before SpotBugs' compile-and-analyse; `!cancelled()` on every analysis step - the +Infer toolchain steps included, so a failed toolchain download cannot hide SpotBugs either - with no +verdict step and no new `continue-on-error`, because the job's own conclusion aggregates the steps; +**one** Maven cache restore where there were two, with the Infer toolchain cache left as its own +steps ahead of the Infer run because it is keyed on the toolchain version, not the pom hash; and +`timeout-minutes: 30` where the two jobs held 15 and 10, sized so the sum plus a toolchain download +on a cache miss has headroom without a hung tool sitting for an hour. + +The sum still fits under the critical path: `Unit Tests` is the longest lane, and Infer plus SpotBugs +comes in below it, so the batch does not become the job everything else waits on. + +Permissions are the union and nothing wider. Neither job declared job-level permissions, so both +inherited maven.yml's workflow-wide grant; the merged job declares `contents: read`, +`pull-requests: write` (the SpotBugs sticky comment) and `checks: write` (the SpotBugs annotation +action), which drops the workflow's `actions: read` that neither analyser used. The +`continue-on-error: true` on the SpotBugs summary-comment step is untouched - a comment write must +not fail the lane, and that reasoning is recorded at the step. + +**The two old job names live on as the step names**, so a red step reads in the log the way the red +check used to, and the prose in `docs/solutions/` and `docs/inflight/` that says what +`static: spotbugs` found stays accurate - only the check name changed. + +**One out-of-scope repair is owed**: `bin/check-pr-analysis-surfaces.sh` filters check runs by name +with `test("Mutation|spotbugs|racerd|CVE|Quarantine")`. That regex reads the CHECK name, not the step +names, so once the ruleset carries `static: analysis` the script stops matching the analysis lane and +silently reports one surface fewer. The fix is one alternation: +`test("Mutation|spotbugs|racerd|static: analysis|CVE|Quarantine")`. + +This note tracks only the owed edits. Once the live ruleset lists `scan: repo` and +`static: analysis` and none of the nine old contexts, nothing here is both true and unowned elsewhere +- the reasoning is in [`docs/ci.md`](../ci.md), the `repo-hygiene.yml` header and the `scan: repo` +and `static: analysis` jobs' own comments. From b456e1f563e2b41098ca4b9c0894df14e189d0c8 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 12:48:15 +1200 Subject: [PATCH 05/14] ci(fewer-jobs): run the whole-tree CVE scan as a step of scan: repo on PRs The OSS Index audit took a runner slot of its own on every PR to do about two minutes of work. On PRs it is now the last step of `scan: repo`, after the three scanners have reported. dependency-audit.yml keeps the job for its weekly schedule and its dispatch, which is where the lane earns its keep: a new advisory arrives without anybody pushing. Only the pull_request trigger left. Fifth cut of the ce-optimize run `ci-fewer-jobs`: 20 -> 19 jobs per PR push. The fork and Dependabot guard is carried over exactly. A job-level `if:` cannot express "skip seven of these ten steps", so the condition became a job-level env var that each CVE step reads. A PR from a fork or from Dependabot gets no CVE steps at all, which is what it got before when it got no job, and pays none of the setup cost. A secret that is present but empty still reds the build from the preflight. Cost: `scan: repo` now needs the dependency cache, so it gains `needs: prepare-deps` and runs to roughly four and a half minutes against a Unit Tests critical path of about nine. Two things this leaves behind, both recorded in the workflow headers: the three no-build scanners now start behind the cache job rather than immediately, which the next commit in this series removes along with the cache job itself; and the five audit steps are duplicated across the two workflows, so each header says to change both or neither. Sharing them wants a composite action, which is not this change. Co-authored-by: Claude Fable 5.1 --- .github/workflows/README.md | 4 +- .github/workflows/dependency-audit.yml | 67 ++++---- .github/workflows/maven.yml | 170 +++++++++++++++++-- docs/ci.md | 24 ++- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 72 ++++++-- 5 files changed, 264 insertions(+), 73 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index c08485153..cab80065f 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -25,8 +25,8 @@ procedure or a steer. Why, and the gate's exact contract: [`docs/ci.md`](../../d | `claude-code-review-dispatch.yml` | The dispatched code reviewer - the expensive half, run when somebody asks for it. | | `claude-code-review.yml` | The review gate. Invokes no Claude and costs nothing. What satisfies it: [`docs/ci.md`](../../docs/ci.md), stated once there. | | `claude.yml` | Answers `@claude` comments, and reviews when asked to - the only route that can raise inline review threads. | -| `dependency-audit.yml` | Scans the **whole** resolved dependency tree for CVEs - the only place OSS Index is switched on. Per-PR, on demand, and weekly, because a new advisory needs no push to arrive. | -| `maven.yml` | The main build: unit, integration and performance suites, static analysis (Infer and SpotBugs, batched as `static: analysis`), duplicate detection, PIT, dependency scanning. | +| `dependency-audit.yml` | Scans the **whole** resolved dependency tree for CVEs. On demand and weekly, because a new advisory needs no push to arrive; the per-PR half is the `deps: whole-tree CVE scan` step of `maven.yml`'s `scan: repo`, and the two are the only places OSS Index is switched on. | +| `maven.yml` | The main build: unit, integration and performance suites, static analysis (Infer and SpotBugs, batched as `static: analysis`), and duplicate detection, dependency review and the whole-tree CVE scan (batched as `scan: repo`), plus PIT. | | `mutation-full-sweep.yml` | The whole-repo PIT mutation sweep - nightly, plus on demand. Too slow for a PR. Self-hosted high-CPU lane, plus a hosted trial arm. | | `publish.yml` | Publishes to Maven Central on every push to `master`; the pom version decides snapshot or release. | | `quarantine-lane.yml` | Runs the quarantined tests separately, so known-flaky tests neither block nor disappear. | diff --git a/.github/workflows/dependency-audit.yml b/.github/workflows/dependency-audit.yml index 59be982c7..6b1c86c00 100644 --- a/.github/workflows/dependency-audit.yml +++ b/.github/workflows/dependency-audit.yml @@ -19,14 +19,21 @@ # rejects cron lanes that RE-RUN suites the PR gate already covers. This lane covers something the # gate cannot: the passage of time. # -# ONE JOB, DELIBERATELY. `ossindex-maven-plugin` is bound to `validate`, which every Maven job in -# this repo runs, so "enabled in CI" would mean six-plus scans per PR from a single account and a -# burnt rate limit for zero extra signal. This job is the only place it is deliberately switched ON -# (`-Dossindex.skip=false`, below). +# ONE SCAN PER EVENT, DELIBERATELY. `ossindex-maven-plugin` is bound to `validate`, which every +# Maven job in this repo runs, so "enabled in CI" would mean six-plus scans per PR from a single +# account and a burnt rate limit for zero extra signal. Everywhere else it is switched OFF by the +# `ossindex.skip` property in the pom (astubbs/parallel-consumer#259, merged), so the invariant is +# checkable: `grep -rn Dossindex .github/workflows/` must find the property turned on in exactly two +# Maven invocations (this paragraph is the only other hit), and they must be in workflows whose +# triggers cannot both fire for the same event. # -# Everywhere else it is switched OFF by the `ossindex.skip` property in the pom -# (astubbs/parallel-consumer#259, merged), so "exactly one job scans" is now unconditional rather -# than aspirational: this is the only workflow that passes `-Dossindex.skip=false`. +# The two are this job - `schedule` and `workflow_dispatch` only - and the `deps: whole-tree CVE +# scan` step of maven.yml's `scan: repo`, which is `pull_request` only. They run the same steps for +# different reasons: a PR needs the gate, and only the schedule can see time pass. Folding the PR +# half into a job that already existed gave a runner slot back under the free plan's 20-job cap +# (ce-optimize run `ci-fewer-jobs`); the steps are duplicated between the two files because a +# composite action would be the only way to share them, and that is not yet worth a third file. +# CHANGE BOTH, OR NEITHER: nothing checks that they still agree. # # A GREEN MAVEN RUN IS NOT EVIDENCE THE SCAN HAPPENED. The plugin's `fail` parameter covers # "vulnerable components were found", not "the request failed" - with an expired token it prints a @@ -51,7 +58,8 @@ # objection no longer applies. And PR-time rather than the schedule alone because this repo has one # maintainer: a weekly digest nobody is obliged to read is mail, not a control. The schedule still # earns its place for what no PR can see - time passing - but it is the second channel, not the -# first. +# first. The first channel is now the `deps: whole-tree CVE scan` step of maven.yml's `scan: repo`; +# this reasoning is why it is a gate there and not a warning. # # The escape hatch for a false positive - and this scanner produces them, measured: two false # positives and one phantom CVE id in a single run @@ -72,17 +80,20 @@ on: # worth less than one they do. Off the hour on purpose: GitHub drops scheduled runs that pile up # at :00. Cron only fires from the default branch, so this does nothing until it is on master. - cron: '17 6 * * 1' - pull_request: workflow_dispatch: +# `github.head_ref` used to be the first term here, from when this workflow also ran on +# `pull_request`; outside a PR it is always empty, so `github.ref` is the whole group now. concurrency: - group: dependency-audit-${{ github.head_ref || github.ref }} + group: dependency-audit-${{ github.ref }} cancel-in-progress: true -# Read-only. Findings go to the job summary rather than a PR comment specifically so this job never -# needs `pull-requests: write` - it runs PR-authored build code in the same job that holds the OSS -# Index token, and every permission it does not have is one that cannot be turned against it. -# See docs/inflight/ci-review-agent.md for the same exposure question on the review job. +# Read-only. Findings go to the job summary rather than a PR comment specifically so the audit never +# needs `pull-requests: write`, and every permission it does not have is one that cannot be turned +# against the OSS Index token it holds. This file's own triggers no longer run PR-authored code at +# all; the property still matters where the audit runs on a PR, and maven.yml's `scan: repo` states +# what contains it there. See docs/inflight/ci-review-agent.md for the same exposure question on the +# review job. permissions: contents: read @@ -94,30 +105,12 @@ jobs: # "whole-tree" is the whole # difference. Job names are an API here, referenced by repo rulesets (see repo-hygiene.yml's # header): renaming is free only while this check is not yet required and not on master. - name: "deps: whole-tree CVE scan" - # TWO WAYS A PR ARRIVES WITHOUT THE OSS INDEX CREDENTIALS, AND BOTH MUST SKIP RATHER THAN GO RED. - # The preflight below turns a missing secret into a red check on purpose - it is the right answer - # when the secret is missing by accident. On these two it would be missing by design, every time, - # which is a permanent false alarm that teaches people to ignore the check. - # - # 1. FORK PRs receive no secrets at all. Same test the self-hosted lanes use. - # 2. DEPENDABOT PRs. The branch is in THIS repository, so the fork test above passes it - but - # GitHub swaps the secret store for a Dependabot-triggered run: `secrets.*` resolves against - # Dependabot secrets, not Actions secrets, so OSSINDEX_USERNAME/OSSINDEX_TOKEN come back - # empty and the preflight fails deterministically. Every dependency-update PR would carry a - # red whole-tree audit, on the PRs least likely to be read line by line. # - # Gated on `github.actor` rather than the PR author because the actor is what actually selects - # the secret store: when a human pushes to a Dependabot branch the run is theirs, Actions secrets - # ARE present, and the scan should run normally. - # - # The alternative - mirroring the two secrets into the Dependabot secret store - was rejected. It - # would put the OSS Index token in a second store, reachable by a job that runs `test-compile` - # over a dependency set an automated process just changed. The scheduled whole-tree scan covers - # what those PRs merge anyway, one channel later and without handing the credential around. - if: >- - (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) - && github.actor != 'dependabot[bot]' + # THIS JOB NO LONGER RUNS ON A PR, and neither does the `if:` it used to carry. Both moved into + # maven.yml's `scan: repo` - the guard verbatim, as the job-level `CVE_SCAN_CREDENTIALS_PRESENT` + # env var, because a job-level `if:` cannot skip four steps of a job with other work to do. + # There is no PR trigger left here to distinguish, so nothing here needs a condition. + name: "deps: whole-tree CVE scan" runs-on: ubuntu-latest timeout-minutes: 20 steps: diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index ccca10b36..4b311b1f3 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -633,37 +633,79 @@ jobs: flags: ak-experimental token: ${{ secrets.CODECOV_TOKEN }} - # ── Scanners (PR-only, no build) ────────────────────────────────────── + # ── Scanners (PR-only) ──────────────────────────────────────────────── - # Three scanners that used to be three jobs - `dups: clones`, `dups: similarity` and - # `deps: vulnerabilities` - now run as steps of one. None of them builds anything; each needs only - # a checkout, and the repo is on the free plan's 20-concurrent-job cap, so three one-minute jobs - # were three slots the sharded suites queued behind (ce-optimize run `ci-fewer-jobs`). + # Four scanners that used to be four jobs - `dups: clones`, `dups: similarity`, + # `deps: vulnerabilities` and `deps: whole-tree CVE scan` - now run as steps of one. The repo is on + # the free plan's 20-concurrent-job cap, so each was a slot the sharded suites queued behind + # (ce-optimize run `ci-fewer-jobs`). # # FAIL AT THE END, REPORT EVERYTHING. Every scanner step carries `!cancelled()`, so a red first - # scanner never hides the second or third; the job's own conclusion aggregates the step failures, + # scanner never hides the ones after it; the job's own conclusion aggregates the step failures, # so there is no verdict step and no continue-on-error. Cheapest signal first: dependency review - # takes seconds, the two duplication engines take about a minute each. + # takes seconds, the two duplication engines take about a minute each, and the whole-tree CVE scan + # - the only step that needs a JDK, a Maven cache and a build - goes last. # # One checkout at fetch-depth 0: both duplication tools compare the PR against its base branch # (`compare_with_base`, `max-increase`), which needs the base's history on disk. # # THE OLD JOB NAMES LIVE ON AS STEP NAMES, so the log reads the same way the checks list used to, # and every doc that says "`dups: clones` found ..." still names the thing that found it. What did - # change is the check: `scan: repo` is the required-check name in the making, and the three names + # change is the check: `scan: repo` is the required-check name in the making, and the four names # it replaces are still in the master ruleset. docs/inflight/ci-fewer-jobs-ruleset-edits.md owns # the swap and its ordering. + # + # THE CVE STEPS RUN PR-AUTHORED BUILD CODE IN A JOB THAT HOLDS `pull-requests: write`, which the + # standalone job deliberately did not (dependency-audit.yml's `permissions:` comment states why). + # What contains it is the credentials guard below: those steps run only for a branch in THIS + # repository, i.e. one pushed by somebody who already has write access, so the build code they + # execute is not attacker-supplied in the way a fork PR's would be. The OSS Index secrets stay on + # the two steps that need them and are not visible to the pinned third-party actions above. scan: if: github.event_name == 'pull_request' name: "scan: repo" runs-on: ubuntu-latest - timeout-minutes: 10 - # All three tools post PR comments: the two duplication tools through their github-token / - # github_token inputs, dependency review through comment-summary-in-pr. Same grant the clones job - # carried; nothing widens. + # The scanners held 10 and the CVE audit held 20. Sized for the sum with headroom for a cold + # Maven cache, without leaving a hung tool sitting for an hour. + timeout-minutes: 25 + needs: prepare-deps + # All three no-build tools post PR comments: the two duplication tools through their + # github-token / github_token inputs, dependency review through comment-summary-in-pr. Same + # grant the clones job carried; nothing widens. The CVE steps need nothing beyond + # `contents: read` - they report to the job summary, never to a comment. permissions: contents: read pull-requests: write + env: + # TWO WAYS A PR ARRIVES WITHOUT THE OSS INDEX CREDENTIALS, AND BOTH MUST SKIP RATHER THAN GO + # RED. The preflight below turns a missing secret into a red check on purpose - it is the + # right answer when the secret is missing by accident. On these two it would be missing by + # design, every time, which is a permanent false alarm that teaches people to ignore the check. + # + # 1. FORK PRs receive no secrets at all. Same test the self-hosted lanes use. + # 2. DEPENDABOT PRs. The branch is in THIS repository, so the fork test passes it - but + # GitHub swaps the secret store for a Dependabot-triggered run: `secrets.*` resolves + # against Dependabot secrets, not Actions secrets, so OSSINDEX_USERNAME/OSSINDEX_TOKEN + # come back empty and the preflight fails deterministically. Every dependency-update PR + # would carry a red whole-tree audit, on the PRs least likely to be read line by line. + # + # Gated on `github.actor` rather than the PR author because the actor is what actually selects + # the secret store: when a human pushes to a Dependabot branch the run is theirs, Actions + # secrets ARE present, and the scan should run normally. + # + # The alternative - mirroring the two secrets into the Dependabot secret store - was rejected. + # It would put the OSS Index token in a second store, reachable by a job that runs + # `test-compile` over a dependency set an automated process just changed. The scheduled + # whole-tree scan in dependency-audit.yml covers what those PRs merge anyway, one channel + # later and without handing the credential around. + # + # This was the standalone job's `if:`, unchanged in meaning and moved here because a job-level + # `if:` cannot skip four steps of a job that has other work to do. It is ONE copy on purpose: + # repeated inline on five steps it would drift. Guarded steps read + # `!cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true'`. + CVE_SCAN_CREDENTIALS_PRESENT: >- + ${{ (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) + && github.actor != 'dependabot[bot]' }} steps: - uses: actions/checkout@v6 with: @@ -770,6 +812,110 @@ jobs: compare_with_base: true max_increase: 10 + # ── Whole-tree CVE scan (was the `deps: whole-tree CVE scan` job) ────────────────────── + # + # .github/workflows/dependency-audit.yml OWNS the reasoning for this lane - what it covers + # that `deps: vulnerabilities` above does not, why exactly one job may switch the plugin on, + # why findings gate the PR, why a green Maven run is not evidence the scan happened, and the + # two different reds. That file still runs the same steps on its weekly schedule and on + # dispatch, which is the half no PR can cover. Read its header before changing anything here; + # the comments below are only what a reader of THIS job cannot do without. + # + # These five steps are the only ones here that need a JDK, the Maven cache and `needs: + # prepare-deps`, so they go last and their setup goes with them - a fork or Dependabot PR + # skips the whole block and pays none of it. + - uses: actions/setup-java@v5 + if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + with: + distribution: 'temurin' + java-version: '17' + # `server-id` must match the plugin's `authId`, which is `ossindex`. setup-java writes a + # settings.xml whose reads these two env var NAMES at Maven runtime, so the + # secrets have to be in `env:` on the Maven step - not here. + server-id: ossindex + server-username: OSSINDEX_USERNAME + server-password: OSSINDEX_TOKEN + # No `cache: maven` - see the caching note at the top of this file. + + # Restore-only, off prepare-deps' rotating key. These steps must not SAVE a cache: it would + # compete with the key the build lanes own, and it resolves a different (smaller) set. + - name: Restore Maven cache + if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + uses: actions/cache/restore@v4 + with: + path: ~/.m2/repository + key: setup-java-Linux-x64-maven-${{ hashFiles('**/pom.xml') }} + restore-keys: | + setup-java-Linux-x64-maven- + + # Before the guard runs, prove the guard still works - the sequencing repo-hygiene.yml uses + # (bin/check-all.sh runs every self-test ahead of the gates). A guard nobody has watched fail + # is decoration. + - name: Self-test the did-it-actually-scan guard + if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + run: bash bin/test-check-ossindex-audit.sh + + # An empty secret produces a 401, which the guard catches - but it names three possible causes + # where this step names the actual one. Cheap, and it fails before spending a build. + - name: Preflight - credentials must be present + if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + env: + OSSINDEX_USERNAME: ${{ secrets.OSSINDEX_USERNAME }} + OSSINDEX_TOKEN: ${{ secrets.OSSINDEX_TOKEN }} + run: | + if [ -z "${OSSINDEX_USERNAME}" ] || [ -z "${OSSINDEX_TOKEN}" ]; then + echo "::error::OSSINDEX_USERNAME / OSSINDEX_TOKEN are not both set as repo secrets. The audit cannot authenticate, and OSS Index no longer serves anonymous component-report requests." + exit 1 + fi + + # `test-compile`, not `validate`: the audit mojo resolves TEST-scope dependencies and the + # modules depend on `parallel-consumer-core:jar:tests`, so on a runner with no PC snapshots + # installed a bare `ossindex:audit` (or a `validate`-only run) dies at the second module with + # "Could not resolve dependencies". + # + # `-Dossindex.fail=false` is the load-bearing property, and it is NOT laxity - it is what makes + # bin/check-ossindex-audit.sh the single place that decides red. Letting Maven fail here would + # take the step down via `pipefail` BEFORE the guard runs, losing both the did-it-actually-scan + # proof and the rendered summary on exactly the runs with findings. Maven stays lenient; the + # guard gates. dependency-audit.yml's header carries the rest. + - name: Audit dependencies against OSS Index + if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + shell: bash + env: + OSSINDEX_USERNAME: ${{ secrets.OSSINDEX_USERNAME }} + OSSINDEX_TOKEN: ${{ secrets.OSSINDEX_TOKEN }} + run: | + # pipefail, or `tee` swallows a Maven BUILD FAILURE and the step goes green on a broken run. + set -o pipefail + ./mvnw --batch-mode test-compile \ + -Dossindex.skip=false \ + -Dossindex.fail=false \ + -Dossindex.authId=ossindex \ + -Dossindex.reportFile=target/ossindex-report.json \ + | tee ossindex-audit.log + + # THE STEP THAT CARRIES THE OLD JOB'S NAME, because it is the one that decided red: it renders + # findings into the run summary, exits 1 when it cannot prove the scan happened, and 2 when the + # scan found something. A red here reads in the log the way the red check used to. + - name: "deps: whole-tree CVE scan" + if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + shell: bash + run: | + set -o pipefail + bash bin/check-ossindex-audit.sh ossindex-audit.log | tee -a "$GITHUB_STEP_SUMMARY" + + # Kept so a finding can be triaged from the structured report rather than re-run by hand, and + # so a failed run can be diagnosed from the log that produced the verdict. + - name: Upload audit reports + if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + uses: actions/upload-artifact@v7 + with: + name: ossindex-reports + path: | + ossindex-audit.log + **/target/ossindex-report.json + if-no-files-found: warn + # TWO STATIC ANALYSERS IN ONE JOB. They ran as `static: infer` and `static: spotbugs`: both waited # on prepare-deps and both restored the same Maven cache with an identical step, so batching them # restores it once and gives a runner slot back. Infer runs first - it is the cheaper signal by a diff --git a/docs/ci.md b/docs/ci.md index ef7550c0e..bd0883ba3 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -72,10 +72,12 @@ document. This section is the detail behind it. catch-all defined by subtraction; see ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries PR-scoped mutation testing (PIT) and two batched jobs: **`static: analysis`** - Infer then SpotBugs, the - cheaper signal first - and **`scan: repo`** - the two duplication scanners and dependency - vulnerability review, no build between them. In both, each step keeps the name of the job it used + cheaper signal first - and **`scan: repo`** - the two duplication scanners, dependency + vulnerability review, and the whole-tree CVE scan last because it is the only one that builds. + In both, each step keeps the name of the job it used to be (`static: infer`, `static: spotbugs`; `dups: clones`, `dups: similarity`, - `deps: vulnerabilities`), so a red step still reads the way the red check did. Push to + `deps: vulnerabilities`, `deps: whole-tree CVE scan`), so a red step still reads the way the red + check did. Push to master runs a single full `bin/ci-build.sh` on the default Kafka version to gate SNAPSHOT publishing. All jobs use explicit `cache/restore` with rotating keys from the `prepare-deps` job - never `setup-java cache: 'maven'`. @@ -549,12 +551,16 @@ runner count rather than about this workflow: stops occupying runners. Housekeeping only; gates nothing. - **`dependency-audit.yml`** - "Dependency Audit", job `deps: whole-tree CVE scan`. Named against `deps: vulnerabilities` (a step of `maven.yml`'s `scan: repo`), which reviews only the dependencies a PR *changes*; this one - scans the whole resolved tree. The **only** place `ossindex-maven-plugin` is switched on - (`-Dossindex.skip=false`); it binds to `validate`, so enabling it globally would mean six-plus - scans per PR from one account. Runs on every in-repo PR, on dispatch, and **weekly on a schedule** - - the schedule catches what no PR can, an unchanged tree acquiring a new advisory. (The one - deliberate exception to "there is no scheduled build" below; it re-runs no suite the gate already - covers.) Skipped for fork PRs, which receive no secrets and would 401 forever. + scans the whole resolved tree. `ossindex-maven-plugin` binds to `validate`, so enabling it + globally would mean six-plus scans per PR from one account: it is switched on + (`-Dossindex.skip=false`) in **exactly two places, whose triggers cannot both fire for one + event** - this workflow on **dispatch and weekly on a schedule**, and the identically-named + `deps: whole-tree CVE scan` **step** of `maven.yml`'s `scan: repo` on every PR. Everything below + is true of both; they are the same steps in two files, and changing one means changing the other. + The schedule catches what no PR can, an unchanged tree acquiring a new advisory. (The one + deliberate exception to "there is almost no scheduled build" below; it re-runs no suite the gate + already covers.) The PR half skips for fork and Dependabot PRs, which receive no Actions secrets + and would 401 forever. - **Findings fail it.** astubbs/parallel-consumer#281 retired the standing backlog into `excludeVulnerabilityIds` entries in the root pom, each carrying a stated retirement condition, so a finding that reaches the gate is by construction an advisory nobody has looked at. diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md index 910163b80..e8f90b577 100644 --- a/docs/inflight/ci-fewer-jobs-ruleset-edits.md +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -11,14 +11,16 @@ was deleted because its steps could run as the tail of the same job - see "Why h checklist" below. Three more, `maven.yml`'s no-build scanners `dups: clones`, `dups: similarity` and `deps: vulnerabilities`, became steps of one new job, `scan: repo` - see "The scanner fold" below. Two more, `maven.yml`'s build-dependent static analysers `static: infer` and `static: spotbugs`, became steps of one new job, -`static: analysis` - see "The static-analysis fold" below. -All nine old names are still **required status-check contexts in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it +`static: analysis` - see "The static-analysis fold" below. A tenth, `dependency-audit.yml`'s +`deps: whole-tree CVE scan`, became a fourth step of `scan: repo` - see "The CVE fold" below; that +workflow keeps its `schedule` and `workflow_dispatch` triggers and is not deleted. +All ten old names are still **required status-check contexts in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it ([`docs/ci.md`](../ci.md), "The required list is repository settings, not tree state"). ## The edit -Remove these nine contexts from the master ruleset's `required_status_checks`: +Remove these ten contexts from the master ruleset's `required_status_checks`: - `Copyright header check` - `quarantine: audit` @@ -29,6 +31,7 @@ Remove these nine contexts from the master ruleset's `required_status_checks`: - `deps: vulnerabilities` - `static: infer` - `static: spotbugs` +- `deps: whole-tree CVE scan` Add two: @@ -51,7 +54,7 @@ A required context nothing produces leaves every PR **pending** - it never fails required"). So: edit the ruleset in the same sitting as the merge. Only one merge is exposed either way - the -deleting PR's own checks list shows the nine contexts as expected-but-missing until the ruleset +deleting PR's own checks list shows the ten contexts as expected-but-missing until the ruleset drops them, which is the intended tell that the edit is still owed, not a fault in that PR. **The add has the opposite ordering.** A new required context that no master run has produced @@ -62,7 +65,7 @@ the job definition is on master, so every PR opened afterwards produces the cont merge, then: 1. Merge the deleting PR. -2. In the same sitting, remove the nine old contexts. +2. In the same sitting, remove the ten old contexts. 3. Add `scan: repo` and `static: analysis` only after the merge has landed and a PR run has produced each context - the first PR to rebase onto the merged master shows both in its checks list; that is the evidence. Until then the three scanners and the two analysers gate nothing, which is a @@ -161,13 +164,56 @@ not fail the lane, and that reasoning is recorded at the step. check used to, and the prose in `docs/solutions/` and `docs/inflight/` that says what `static: spotbugs` found stays accurate - only the check name changed. -**One out-of-scope repair is owed**: `bin/check-pr-analysis-surfaces.sh` filters check runs by name -with `test("Mutation|spotbugs|racerd|CVE|Quarantine")`. That regex reads the CHECK name, not the step -names, so once the ruleset carries `static: analysis` the script stops matching the analysis lane and -silently reports one surface fewer. The fix is one alternation: -`test("Mutation|spotbugs|racerd|static: analysis|CVE|Quarantine")`. +**A name-matching consumer had to be repaired, and it is the class to check on every fold**: +`bin/check-pr-analysis-surfaces.sh` filters check runs by name. That regex reads the CHECK name, not +the step names, so a fold silently drops a surface from its listing - nothing fails, the row just +stops appearing. It read `test("Mutation|spotbugs|racerd|CVE|Quarantine")`, in which `racerd` had +already been dead since the job became `static: infer`; it now names the live checks, +`test("Mutation|static: analysis|scan: repo|CVE|Quarantine")`, and carries a comment saying to +re-read it whenever a job is renamed or folded. + +## The CVE fold + +`deps: whole-tree CVE scan` was the whole of `dependency-audit.yml`'s `ossindex` job, and that +workflow's `pull_request` trigger was its only PR-time producer. The trigger is gone and the job is +now a fourth step of `scan: repo`; the workflow itself is **not deleted** - it keeps `schedule` and +`workflow_dispatch`, which is the half no PR can cover (an unchanged tree acquiring a new advisory). +So the name `deps: whole-tree CVE scan` still exists in the tree, as both a job in that workflow and +a step in this one, and is still on the removal list above: no PR run produces it any more, and a +required context nothing produces on a PR leaves every PR pending. + +What the fold had to carry: + +- **The credentials guard, verbatim and with its semantics unchanged.** The standalone job's `if:` + skipped for fork PRs and for Dependabot-actor runs, because in both cases `secrets.*` resolves + empty and the preflight would red every time by design. A job-level `if:` cannot skip four steps + of a job that has other work to do, so the same expression is now the job-level env var + `CVE_SCAN_CREDENTIALS_PRESENT` and every CVE step reads + `!cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true'`. Skip, not warn and not red: those + PRs get no CVE steps at all, exactly as they got no job before. A *present-but-empty* secret is + still red, from the preflight - that case is a mistake, not a design. +- **The setup the scanners did not need**: `needs: prepare-deps`, `actions/setup-java` with + `server-id: ossindex` (the plugin's `authId`, read from the generated settings.xml at Maven + runtime), and the restore-only Maven cache step. All three sit with the CVE steps at the end of + the job and behind the same guard, so a fork PR pays none of it. +- **Last, not first.** It is the only step here that builds, so the cheap signals still land first + in the log, and the `dups: clones` comment about `target/` not being on disk stays true - the + Maven step is below it, not above. +- **`timeout-minutes: 25`** where the scanners held 10 and the audit job held 20. +- **The exposure the standalone job avoided, and what contains it here.** That job deliberately held + `contents: read` only, because it runs PR-authored build code in the same job as the OSS Index + token. `scan: repo` also holds `pull-requests: write`, for the three tools that post comments. + What contains it is the guard itself: the CVE steps run only for a branch in *this* repository, + pushed by somebody who already has write access, so the build code is not attacker-supplied the + way a fork PR's would be. The OSS Index secrets stay in `env:` on the two steps that need them and + are not visible to the pinned third-party actions. +- **The steps are now duplicated between two files.** `dependency-audit.yml` still owns the + reasoning - what the lane covers, why findings gate, the two different reds, the + did-it-actually-scan guard - and `maven.yml` points at it rather than restating it. The commands + themselves are copied, because sharing them needs a composite action and a third file. Nothing + checks that the copies agree; both file headers say so. This note tracks only the owed edits. Once the live ruleset lists `scan: repo` and -`static: analysis` and none of the nine old contexts, nothing here is both true and unowned elsewhere -- the reasoning is in [`docs/ci.md`](../ci.md), the `repo-hygiene.yml` header and the `scan: repo` -and `static: analysis` jobs' own comments. +`static: analysis` and none of the ten old contexts, nothing here is both true and unowned elsewhere +- the reasoning is in [`docs/ci.md`](../ci.md), the `repo-hygiene.yml` and `dependency-audit.yml` +headers, and the `scan: repo` and `static: analysis` jobs' own comments. From 245ba06f7e73d2fea1cf6d6f2c2c5ecfcbb364c1 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 12:55:48 +1200 Subject: [PATCH 06/14] ci(fewer-jobs): run PIT as the last step of scan: repo The PR-scoped mutation lane held a runner slot to do about eleven seconds of work on most pushes, because it usually finds no changed core class in a decidable package to mutate. It is now the last step of `scan: repo`, behind everything that reports: the scanners and the CVE audit have already posted before PIT starts. Sixth cut of the ce-optimize run `ci-fewer-jobs`: 19 -> 18 jobs per PR push. Its semantics are carried over exactly, because they are the whole point of the lane. The exit-code handling is unchanged, including the branch that turns "could not run, or ran and scored nothing" into a failure rather than a green tick. The step keeps continue-on-error, so a PIT verdict still cannot red the required `scan: repo` job - the same reason requiring the old job would have been vacuous. No ruleset edit is owed for this one, and the working note says so, so nobody diffing the checks list before and after goes hunting for a missed removal. The step bound is better than what it replaces. The job used to carry a 30-minute cap, and a run that hit it was cancelled outright, which is one of the sightings recorded against the untracked-flakes note. A 20-minute step bound ends the step and lets the job report instead, and it sits above the longest scored run on record. One guard needed widening. The JDK and cache-restore steps sat behind the CVE credentials condition from the previous commit, and PIT runs on fork and Dependabot pull requests, which is exactly where that condition is false. They now run unconditionally, so PIT has a JDK where the CVE steps skip. Everything touching OSS Index stays guarded. The analysis-surfaces script matched check runs on a `Mutation` alternation that now names nothing, the same way `racerd` did after an earlier rename. Removed, with a note that the survivor table hangs off `scan: repo` now. Co-authored-by: Claude Fable 5.1 --- .github/workflows/README.md | 2 +- .github/workflows/maven.yml | 156 ++++++++++-------- bin/check-pr-analysis-surfaces.sh | 5 +- docs/ci.md | 26 +-- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 57 ++++++- .../ci-orphaned-pit-comment-on-open-prs.md | 5 +- docs/inflight/test-untracked-ci-flakes.md | 2 +- 7 files changed, 165 insertions(+), 88 deletions(-) diff --git a/.github/workflows/README.md b/.github/workflows/README.md index cab80065f..bdae08363 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -26,7 +26,7 @@ procedure or a steer. Why, and the gate's exact contract: [`docs/ci.md`](../../d | `claude-code-review.yml` | The review gate. Invokes no Claude and costs nothing. What satisfies it: [`docs/ci.md`](../../docs/ci.md), stated once there. | | `claude.yml` | Answers `@claude` comments, and reviews when asked to - the only route that can raise inline review threads. | | `dependency-audit.yml` | Scans the **whole** resolved dependency tree for CVEs. On demand and weekly, because a new advisory needs no push to arrive; the per-PR half is the `deps: whole-tree CVE scan` step of `maven.yml`'s `scan: repo`, and the two are the only places OSS Index is switched on. | -| `maven.yml` | The main build: unit, integration and performance suites, static analysis (Infer and SpotBugs, batched as `static: analysis`), and duplicate detection, dependency review and the whole-tree CVE scan (batched as `scan: repo`), plus PIT. | +| `maven.yml` | The main build: unit, integration and performance suites, static analysis (Infer and SpotBugs, batched as `static: analysis`), and duplicate detection, dependency review, the whole-tree CVE scan and PR-scoped PIT (batched as `scan: repo`, PIT last and advisory). | | `mutation-full-sweep.yml` | The whole-repo PIT mutation sweep - nightly, plus on demand. Too slow for a PR. Self-hosted high-CPU lane, plus a hosted trial arm. | | `publish.yml` | Publishes to Maven Central on every push to `master`; the pom version decides snapshot or release. | | `quarantine-lane.yml` | Runs the quarantined tests separately, so known-flaky tests neither block nor disappear. | diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 4b311b1f3..cbf7a3b09 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -520,61 +520,6 @@ jobs: flags: ${{ matrix.suite }} token: ${{ secrets.CODECOV_TOKEN }} - # Mutation testing (PIT). ci-mutation-test.sh scopes to the classes CHANGED vs the PR base - # (GITHUB_BASE_REF), so it's fast enough for the 2-core hosted runner - the full internal.* sweep would - # time out here (which is why it used to be self-hosted-only). - # - # ADVISORY ABOUT SCORES, NOT ABOUT ITSELF. `continue-on-error: true` stays - it keeps a mutation - # result from failing the workflow run, while the check ROW still renders red when the step fails - # (measured, see docs/inflight/ci-mutation-lane-skip-reads-as-a-pass.md). So red was always - # honest here and green never was: measured 2026-08-25 over the last 40 pull_request runs of this - # workflow, 40 green ticks and zero mutants scored, because every path out of the script exited 0. - # The step below reads bin/ci-mutation-test.sh's exit code instead and reacts to each state - # separately - survivors still fail nothing, but a lane that could not run, or ran and measured - # nothing, now goes red. The exit-code contract lives in that script's header; - # docs/inflight/ci-mutation-testing.md has the measurement. - mutation: - if: github.event_name == 'pull_request' - needs: prepare-deps - name: "Mutation Tests (PIT, PR-scoped)" - runs-on: ubuntu-latest - timeout-minutes: 30 - continue-on-error: true # advisory - never gates merge; a failed STEP still reddens the row - steps: - - uses: actions/checkout@v6 - with: - fetch-depth: 0 # ci-mutation-test.sh diffs vs the PR base to scope to changed classes - - uses: actions/setup-java@v5 - with: - distribution: 'temurin' - java-version: '17' - - name: Restore Maven cache - uses: actions/cache/restore@v4 - with: - path: ~/.m2/repository - key: setup-java-Linux-x64-maven-${{ hashFiles('**/pom.xml') }} - restore-keys: | - setup-java-Linux-x64-maven- - - name: Self-test the mutation verdict - # Ahead of the lane it protects, the same ordering repo-hygiene.yml uses. - # Seconds, no maven: it feeds canned PIT logs through the verdict and asserts the exit codes. - run: bash bin/test-ci-mutation-test.sh - - - name: Mutation testing (changed classes only) - run: | - set +e - bin/ci-mutation-test.sh - rc=$? - set -e - case "$rc" in - 0) echo "PIT scored mutants - see the job summary for the survivor list." ;; - 3) echo "::notice::PIT had nothing in scope: no core main-source class in a decidable package changed." ;; - 2) echo "::error::PIT could not run, or ran and scored nothing. This is a broken lane, not a clean PR." - exit 1 ;; - *) echo "::error::PIT exited ${rc}." - exit "$rc" ;; - esac - # Experimental Kafka 4.x compatibility check. Non-blocking. # DISABLED pending the Kafka 4.x migration work: the experimental 4.x build currently fails and just adds # a red X of noise to every PR (it is continue-on-error, so it never gated merges). Re-enable by restoring @@ -635,19 +580,31 @@ jobs: # ── Scanners (PR-only) ──────────────────────────────────────────────── - # Four scanners that used to be four jobs - `dups: clones`, `dups: similarity`, - # `deps: vulnerabilities` and `deps: whole-tree CVE scan` - now run as steps of one. The repo is on - # the free plan's 20-concurrent-job cap, so each was a slot the sharded suites queued behind - # (ce-optimize run `ci-fewer-jobs`). + # Five checks that used to be five jobs - `dups: clones`, `dups: similarity`, + # `deps: vulnerabilities`, `deps: whole-tree CVE scan` and `Mutation Tests (PIT, PR-scoped)` - now + # run as steps of one. The repo is on the free plan's 20-concurrent-job cap, so each was a slot the + # sharded suites queued behind (ce-optimize run `ci-fewer-jobs`). # # FAIL AT THE END, REPORT EVERYTHING. Every scanner step carries `!cancelled()`, so a red first # scanner never hides the ones after it; the job's own conclusion aggregates the step failures, # so there is no verdict step and no continue-on-error. Cheapest signal first: dependency review - # takes seconds, the two duplication engines take about a minute each, and the whole-tree CVE scan - # - the only step that needs a JDK, a Maven cache and a build - goes last. + # takes seconds, the two duplication engines take about a minute each, then the whole-tree CVE + # scan, and the PIT lane dead last because it is the only step whose runtime is unbounded in + # practice (11s when it has nothing to mutate, half an hour when it does). + # + # THE PIT STEPS ARE THE ONE EXCEPTION TO "NO CONTINUE-ON-ERROR" HERE, and it is not a relaxation: + # the mutation lane was a `continue-on-error: true` JOB, so neither its self-test nor its run could + # ever gate a merge. `scan: repo` IS a required check, so folding those steps in bare would have + # promoted an advisory lane to a gating one by accident. `continue-on-error: true` on the two PIT + # steps keeps exactly the old semantics - the step still renders red in the log and in the job's + # step list, and still cannot fail the check. docs/ci.md, "A green mutation tick usually means + # 'measured nothing' - read the exit code", owns why the exit codes matter and why requiring this + # signal is a code change rather than a ruleset edit. # # One checkout at fetch-depth 0: both duplication tools compare the PR against its base branch - # (`compare_with_base`, `max-increase`), which needs the base's history on disk. + # (`compare_with_base`, `max-increase`), which needs the base's history on disk, and + # bin/ci-mutation-test.sh diffs against the PR base to scope PIT to the changed classes. Do not + # narrow it - the mutation lane falls back to the FULL glob when the base ref will not resolve. # # THE OLD JOB NAMES LIVE ON AS STEP NAMES, so the log reads the same way the checks list used to, # and every doc that says "`dups: clones` found ..." still names the thing that found it. What did @@ -665,9 +622,11 @@ jobs: if: github.event_name == 'pull_request' name: "scan: repo" runs-on: ubuntu-latest - # The scanners held 10 and the CVE audit held 20. Sized for the sum with headroom for a cold - # Maven cache, without leaving a hung tool sitting for an hour. - timeout-minutes: 25 + # The scanners held 10, the CVE audit 20 and the mutation lane 30. Sized for the sum of the + # first two plus the PIT step's OWN 20-minute cap (below), not for the old 30: the step bound is + # what stops a mutating run holding the job, and the job bound is only the backstop. 45 leaves + # headroom for a cold Maven cache without letting a hung tool sit for an hour. + timeout-minutes: 45 needs: prepare-deps # All three no-build tools post PR comments: the two duplication tools through their # github-token / github_token inputs, dependency review through comment-summary-in-pr. Same @@ -821,11 +780,15 @@ jobs: # dispatch, which is the half no PR can cover. Read its header before changing anything here; # the comments below are only what a reader of THIS job cannot do without. # - # These five steps are the only ones here that need a JDK, the Maven cache and `needs: - # prepare-deps`, so they go last and their setup goes with them - a fork or Dependabot PR - # skips the whole block and pays none of it. + # THE JDK AND THE CACHE ARE SHARED WITH THE PIT STEPS BELOW, so these two are NOT behind the + # credentials guard, while every step that actually talks to OSS Index still is. They were + # guarded when the CVE block was the only thing here that built; the mutation lane runs on + # every PR including forks and Dependabot ones, so guarding the setup would leave PIT with no + # Maven cache on exactly the PRs that get no CVE scan. `server-id: ossindex` is inert when the + # audit steps skip: setup-java only writes a settings.xml entry, which nothing reads + # unless the Maven audit run happens. - uses: actions/setup-java@v5 - if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + if: ${{ !cancelled() }} with: distribution: 'temurin' java-version: '17' @@ -839,8 +802,9 @@ jobs: # Restore-only, off prepare-deps' rotating key. These steps must not SAVE a cache: it would # compete with the key the build lanes own, and it resolves a different (smaller) set. + # Unguarded for the same reason as the JDK above - the PIT steps need it on every PR. - name: Restore Maven cache - if: ${{ !cancelled() && env.CVE_SCAN_CREDENTIALS_PRESENT == 'true' }} + if: ${{ !cancelled() }} uses: actions/cache/restore@v4 with: path: ~/.m2/repository @@ -916,6 +880,58 @@ jobs: **/target/ossindex-report.json if-no-files-found: warn + # ── Mutation testing, PIT (was the `Mutation Tests (PIT, PR-scoped)` job) ────────────── + # + # ci-mutation-test.sh scopes to the classes CHANGED vs the PR base (GITHUB_BASE_REF), so it is + # fast enough for the 2-core hosted runner - the full internal.* sweep would time out here + # (which is why it used to be self-hosted-only, and why mutation-full-sweep.yml still is). + # + # ADVISORY ABOUT SCORES, NOT ABOUT ITSELF. The `continue-on-error: true` that used to sit on + # the JOB now sits on each of these two steps, which is what keeps the semantics: a mutation + # result must not fail the check, while a failed step still renders red where a reader sees it + # (measured on the old job, see docs/inflight/ci-mutation-lane-skip-reads-as-a-pass.md). So red + # was always honest here and green never was: measured 2026-08-25 over the last 40 + # pull_request runs of this workflow, 40 green ticks and zero mutants scored, because every + # path out of the script exited 0. The step below reads bin/ci-mutation-test.sh's exit code + # instead and reacts to each state separately - survivors still fail nothing, but a lane that + # could not run, or ran and measured nothing, now goes red. The exit-code contract lives in + # that script's header; docs/inflight/ci-mutation-testing.md has the measurement. + # + # LAST, AND ON ITS OWN CLOCK. Every check above has already reported by the time this starts, + # and `timeout-minutes: 20` on the run step is what stops a mutating PR (up to ~30 minutes of + # PIT) leaving the scanner and CVE results waiting behind it. 20 is above the 19m18s scored run + # on record while staying under the job's 45; the old lane's 30-minute JOB cap had about a + # third headroom over a normal run and was seen to flap on a slow runner + # (docs/inflight/test-untracked-ci-flakes.md). A step-level timeout ends the STEP, so the job + # still finishes and reports; a cancelled job reported nothing. + - name: Self-test the mutation verdict + # Ahead of the lane it protects, the same ordering repo-hygiene.yml uses. + # Seconds, no maven: it feeds canned PIT logs through the verdict and asserts the exit codes. + # `continue-on-error` because the whole lane was advisory before this fold - and the same + # self-test runs non-advisory in `repo: hygiene`'s `bin/check-all.sh --with-tests` sweep, + # which is a required check, so nothing is lost by not gating on it twice. + if: ${{ !cancelled() }} + continue-on-error: true + run: bash bin/test-ci-mutation-test.sh + + - name: "Mutation Tests (PIT, PR-scoped)" + if: ${{ !cancelled() }} + continue-on-error: true + timeout-minutes: 20 + run: | + set +e + bin/ci-mutation-test.sh + rc=$? + set -e + case "$rc" in + 0) echo "PIT scored mutants - see the job summary for the survivor list." ;; + 3) echo "::notice::PIT had nothing in scope: no core main-source class in a decidable package changed." ;; + 2) echo "::error::PIT could not run, or ran and scored nothing. This is a broken lane, not a clean PR." + exit 1 ;; + *) echo "::error::PIT exited ${rc}." + exit "$rc" ;; + esac + # TWO STATIC ANALYSERS IN ONE JOB. They ran as `static: infer` and `static: spotbugs`: both waited # on prepare-deps and both restored the same Maven cache with an identical step, so batching them # restores it once and gives a runner slot back. Infer runs first - it is the cheaper signal by a diff --git a/bin/check-pr-analysis-surfaces.sh b/bin/check-pr-analysis-surfaces.sh index d970b8747..da1297fc1 100755 --- a/bin/check-pr-analysis-surfaces.sh +++ b/bin/check-pr-analysis-surfaces.sh @@ -262,9 +262,10 @@ echo " Job summaries (PIT survivor table, CVE tables) are not exposed by the RE # from this listing silently - nothing fails, the row simply stops appearing, and the reader cannot # tell "no summary to read" from "we stopped looking". `racerd` outlived the job that became # `static: infer` this way, and was dead here until the 2026-09-07 job folds forced a re-read. -# Re-check this pattern whenever a job in .github/workflows/ is renamed or folded. +# Re-check this pattern whenever a job in .github/workflows/ is renamed or folded. The PIT survivor +# table now hangs off `scan: repo`, which is why no `Mutation` alternative is needed here. gh api "repos/${REPO}/commits/${HEAD_SHA}/check-runs?per_page=100" \ - --jq '.check_runs[] | select(.name | test("Mutation|static: analysis|scan: repo|CVE|Quarantine")) | " \(.name): \(.html_url)"' \ + --jq '.check_runs[] | select(.name | test("static: analysis|scan: repo|CVE|Quarantine")) | " \(.name): \(.html_url)"' \ 2>/dev/null | sort -u || true echo echo " Console-only output: a tool that prints to the Maven log and does not annotate is invisible" diff --git a/docs/ci.md b/docs/ci.md index bd0883ba3..7a7fa36a9 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -70,14 +70,16 @@ document. This section is the detail behind it. **gating**, like the job they replaced: a chaos RED is a real finding. The **`Integration Tests`** lane is likewise two gating shards since astubbs#442 - a named heavy set and a catch-all defined by subtraction; see - ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). Also carries PR-scoped - mutation testing (PIT) and two batched jobs: **`static: analysis`** - Infer then SpotBugs, the - cheaper signal first - and **`scan: repo`** - the two duplication scanners, dependency - vulnerability review, and the whole-tree CVE scan last because it is the only one that builds. - In both, each step keeps the name of the job it used + ["The Integration Tests lane runs as two shards"](#the-integration-tests-lane-runs-as-two-shards). It also carries two + batched jobs: **`static: analysis`** - Infer then SpotBugs, the cheaper signal first - and + **`scan: repo`** - the two duplication scanners, dependency vulnerability review, the whole-tree + CVE scan, and PR-scoped mutation testing (PIT) dead last, the two builds after the three + no-build tools. In both, each step keeps the name of the job it used to be (`static: infer`, `static: spotbugs`; `dups: clones`, `dups: similarity`, - `deps: vulnerabilities`, `deps: whole-tree CVE scan`), so a red step still reads the way the red - check did. Push to + `deps: vulnerabilities`, `deps: whole-tree CVE scan`, `Mutation Tests (PIT, PR-scoped)`), so a red + step still reads the way the red check did. The PIT steps are the only ones in either job carrying + `continue-on-error` - the lane was an advisory *job* before the fold, and `scan: repo` is required, + so the flag is what stops the fold promoting it to a gate. Push to master runs a single full `bin/ci-build.sh` on the default Kafka version to gate SNAPSHOT publishing. All jobs use explicit `cache/restore` with rotating keys from the `prepare-deps` job - never `setup-java cache: 'maven'`. @@ -312,7 +314,7 @@ every other PR) and not after (nothing merges). The live instance of this is | Check | Why not | |---|---| -| `Mutation Tests (PIT, PR-scoped)` | **Requiring it would be vacuous.** The job is `continue-on-error: true`, so its check-run *conclusion* is success even when the step fails - the row reddens, and a required check reads the conclusion. The property worth gating is that the lane could not measure anything, which `bin/ci-mutation-test.sh` signals through its own exit codes rather than by finding survivors. Gating that means removing `continue-on-error` first, which is a code change, not a ruleset edit | +| `Mutation Tests (PIT, PR-scoped)` | **There is no such check any more, and requiring it would have been vacuous anyway.** The lane is now the last two steps of `scan: repo`, each carrying its own `continue-on-error: true` - so a PIT verdict still cannot fail a check, exactly as when the flag sat on its own job. Requiring the old context is now impossible (nothing produces it) rather than merely pointless. The property worth gating is that the lane could not measure anything, which `bin/ci-mutation-test.sh` signals through its own exit codes rather than by finding survivors. Gating that still means removing `continue-on-error` first, which is a code change, not a ruleset edit - and it would now make `scan: repo` red on a mutation verdict, which is the decision to argue | | `Performance (optional)` | The self-hosted lane is dispatch-only, so this context is never produced on a PR. Requiring it would block every PR permanently | | `compat: kafka 4.x (experimental)` | Disabled with `if: false` | | `full build (master)` | Push-only; never produced on a PR | @@ -1125,8 +1127,9 @@ never run on our own hardware. on-demand benchmark nobody dispatched, so it was not worth a file. Read it at `git show 5ae0cbfe4:.github/workflows/pr-highcpu-fast-feedback.yml`. - `mutation-full-sweep.yml` - **nightly plus dispatch**: the whole-project PIT sweep - (`bin/ci-mutation-test.sh -Dverbose=true -Dthreads=N`). The PR-scoped mutation job in `maven.yml` - only covers classes changed against the base; this is its exhaustive counterpart. + (`bin/ci-mutation-test.sh -Dverbose=true -Dthreads=N`). The PR-scoped mutation steps in + `maven.yml`'s `scan: repo` only cover classes changed against the base; this is its exhaustive + counterpart. ### A green mutation tick usually means "measured nothing" - read the exit code @@ -1135,7 +1138,8 @@ never run on our own hardware. producing no statistics / zero mutants), **3** nothing in scope. Measured over the last 40 `maven.yml` PR runs: 40 passes, zero mutants scored - the lane is correctly narrow, not broken. Only a **0** is evidence about test quality. `bin/test-ci-mutation-test.sh` guards the contract and runs -in the lane ahead of it. The scope, the exclusions and the ranked widening list are in +in the lane ahead of it - and, since the lane became two steps of `scan: repo`, non-advisory inside +`repo: hygiene`'s `bin/check-all.sh --with-tests` sweep as well. The scope, the exclusions and the ranked widening list are in [`docs/inflight/ci-mutation-testing.md`](inflight/ci-mutation-testing.md); whether a skip should render grey rather than green is an open decision in [`docs/inflight/ci-mutation-lane-skip-reads-as-a-pass.md`](inflight/ci-mutation-lane-skip-reads-as-a-pass.md). diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md index e8f90b577..c723489ab 100644 --- a/docs/inflight/ci-fewer-jobs-ruleset-edits.md +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -13,7 +13,11 @@ checklist" below. Three more, `maven.yml`'s no-build scanners `dups: clones`, `d build-dependent static analysers `static: infer` and `static: spotbugs`, became steps of one new job, `static: analysis` - see "The static-analysis fold" below. A tenth, `dependency-audit.yml`'s `deps: whole-tree CVE scan`, became a fourth step of `scan: repo` - see "The CVE fold" below; that -workflow keeps its `schedule` and `workflow_dispatch` triggers and is not deleted. +workflow keeps its `schedule` and `workflow_dispatch` triggers and is not deleted. An eleventh, +`maven.yml`'s `Mutation Tests (PIT, PR-scoped)`, became the fifth and last step of `scan: repo` - +and is the one of these eleven folds that owes **no** ruleset edit at all; see "The PIT fold owes no +ruleset edit" below, which exists so a reader diffing the checks list does not go looking for the +entry that is deliberately absent. All ten old names are still **required status-check contexts in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it ([`docs/ci.md`](../ci.md), "The required list is repository settings, not tree state"). @@ -172,6 +176,57 @@ already been dead since the job became `static: infer`; it now names the live ch `test("Mutation|static: analysis|scan: repo|CVE|Quarantine")`, and carries a comment saying to re-read it whenever a job is renamed or folded. +## The PIT fold owes no ruleset edit - and that is the point of this section + +`Mutation Tests (PIT, PR-scoped)` is the eleventh check the `ci-fewer-jobs` folds remove from a PR's +checks list, and the only one whose name is **not** on the removal list above. It was never a required +context: [`docs/ci.md`](../ci.md)'s "These are deliberately NOT required" table has always carried a +row for it, because the job was `continue-on-error: true` and a required check reads the *conclusion* +- which `continue-on-error` makes success even when the step fails. Requiring it would have gated +nothing. So there is nothing to remove, and nothing to add. + +Written down because the absence is indistinguishable from an oversight. Somebody comparing a PR's +checks list before and after the merge sees eleven rows go and ten names accounted for here; without +this paragraph the eleventh reads as a missed edit, and the natural repair - adding +`Mutation Tests (PIT, PR-scoped)` to a ruleset that never had it - fails with a context nothing has +ever produced. + +What the fold had to carry: + +- **`continue-on-error: true` moved from the JOB to each of the two steps.** `scan: repo` **is** a + required check, so folding an advisory lane into it bare would have promoted a mutation verdict to + a merge gate by accident - the single most consequential thing this fold could have got wrong. On + the steps, the semantics are identical to before: a red step renders red in the log and the step + list, and cannot fail the check. Every exit-code branch of the run step (`0` scored, `3` nothing in + scope, `2` broken lane -> `exit 1`, anything else -> `exit "$rc"`) is copied unchanged, `set +e` + handling included. +- **A step-level `timeout-minutes: 20`,** where the job held 30. PIT is ~11s when nothing is in scope + and up to half an hour when something is; the job bound is now 45 and is only a backstop. The step + bound is the load-bearing one: it stops a mutating run leaving the scanner and CVE results waiting, + and a timed-out *step* still lets the job finish and report, where the old timed-out *job* reported + nothing at all (the sighting in [`test-untracked-ci-flakes.md`](test-untracked-ci-flakes.md)). +- **The JDK and the Maven cache restore came out from behind the credentials guard.** They were + guarded because the CVE block was the only thing in the job that built; PIT runs on every PR + including fork and Dependabot ones, so leaving them guarded would have handed the mutation lane a + cold repository on exactly the PRs that get no CVE scan. Every step that talks to OSS Index is + still guarded, and `server-id: ossindex` is inert when those steps skip. +- **`fetch-depth: 0` was already there** for the two duplication tools, which is what the mutation + lane needs to diff against the PR base; it must not be narrowed, because an unresolvable base ref + makes `bin/ci-mutation-test.sh` fall back to the *full* glob. +- **The self-test still runs first,** `continue-on-error` like the run step - it also runs + non-advisory inside `repo: hygiene`'s `bin/check-all.sh --with-tests` sweep, which is required, so + nothing is lost by not gating on it twice here. + +**The name-matching consumer needs a second repair, and it is in `bin/` rather than in a +workflow**: `bin/check-pr-analysis-surfaces.sh` lists check runs through +`test("Mutation|static: analysis|scan: repo|CVE|Quarantine")`. After this fold no check run is named +`Mutation Tests (PIT, PR-scoped)`, so the `Mutation` alternative matches nothing - it is `racerd` +again, one fold later. Nothing breaks: `scan: repo` is already in the pattern and is now the row +that carries the PIT job summary, so the surface is still listed, under the batched name. The fix is +to drop the now-dead `Mutation` alternative, leaving +`test("static: analysis|scan: repo|CVE|Quarantine")`, and to say in the comment above it that the +PIT summary now hangs off `scan: repo`. + ## The CVE fold `deps: whole-tree CVE scan` was the whole of `dependency-audit.yml`'s `ossindex` job, and that diff --git a/docs/inflight/ci-orphaned-pit-comment-on-open-prs.md b/docs/inflight/ci-orphaned-pit-comment-on-open-prs.md index 0e23288f1..650666bab 100644 --- a/docs/inflight/ci-orphaned-pit-comment-on-open-prs.md +++ b/docs/inflight/ci-orphaned-pit-comment-on-open-prs.md @@ -12,8 +12,9 @@ exists. GitHub-hosted `mutation-testing` job - its body says so outright, "Removed the GitHub-hosted mutation-testing job entirely" - and with it the `Post PIT summary to PR` step that produced that exact string. A PR-scoped lane was reintroduced hours later in `6f7ce1893`, deliberately **without** -comment-posting: today's `Mutation Tests (PIT, PR-scoped)` job in `.github/workflows/maven.yml` has -two steps, neither of which calls `createComment` or `updateComment`. The verdict is the exit code +comment-posting: today's `Mutation Tests (PIT, PR-scoped)` step in `.github/workflows/maven.yml` +(the lane became the last two steps of the `scan: repo` job, 2026-09-07) calls neither +`createComment` nor `updateComment`. The verdict is the exit code of `bin/ci-mutation-test.sh` (its header owns the contract, anchor `THE EXIT CODE IS THE VERDICT`) plus a `::notice::`, and that is the design. diff --git a/docs/inflight/test-untracked-ci-flakes.md b/docs/inflight/test-untracked-ci-flakes.md index d5703a9ff..ffc13e2bc 100644 --- a/docs/inflight/test-untracked-ci-flakes.md +++ b/docs/inflight/test-untracked-ci-flakes.md @@ -26,7 +26,7 @@ Where their diagnoses generalised, the rule is in [`docs/solutions/`](../solutio | `ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect` | 1 seen (2026-08-12) | Not from the original scan - found while babysitting astubbs#287. **Fixed by astubbs#265**, which deleted the wall-clock assertion rather than repairing it. astubbs#262, its owner, lifted the quarantine and deleted the registry entry - see below | | `JStreamParallelEoSStreamProcessorTest.testConsumeAndProduce` and `.testFlatMapProduce` | 1 seen (2026-09-01) | Not from the original scan - found in a **local** core unit run on a parallel re-cut of astubbs#207, not on astubbs#207 itself. Both failed together on produced-record count (`Expected size: 1/2 but was: 0`), i.e. the returned stream carried nothing. **Mechanism now known and owned by astubbs#116** - see below | | `simpleBatchTest` in **all three** of `ReactorBatchTest`, `MutinyBatchTest` and `VertxBatchTest` | 6 seen (2026-08-18, 2026-08-19, 2026-08-25, 2026-09-01, 2026-09-02, 2026-09-05) | Not from the original scan - each found while babysitting a branch. Same Awaitility `ConditionTimeout`, same alias 'expected number of batches' (30s), same shared `BatchTestMethods` lambda. UNDIAGNOSED, but the third, fourth and fifth sightings independently carry the **same three-way key collision** in the failing batch contents, which points at the test's own randomised input - see below, and classify (contention vs product vs expectation) before touching | -| `Mutation Tests (PIT, PR-scoped)` lane | 1 seen (2026-09-02, astubbs#207, [run 33610711974](https://github.com/astubbs/parallel-consumer/actions/runs/33610711974)) | Not a test - the LANE hit its `timeout-minutes: 30` cap and was cancelled, on a **markdown-only** delta from a head where it had scored in 19m18s with the same class set. The cap has about a third headroom over a normal run, so it will flap on a slow runner. `continue-on-error: true`, so it never gates a merge - but a cancelled row reads like a failure | +| `Mutation Tests (PIT, PR-scoped)` lane | 1 seen (2026-09-02, astubbs#207, [run 33610711974](https://github.com/astubbs/parallel-consumer/actions/runs/33610711974)) | Not a test - the LANE hit its `timeout-minutes: 30` cap and was cancelled, on a **markdown-only** delta from a head where it had scored in 19m18s with the same class set. The cap had about a third headroom over a normal run, so it flapped on a slow runner. Addressed 2026-09-07 by the fold into `scan: repo` ([`ci-fewer-jobs-ruleset-edits.md`](ci-fewer-jobs-ruleset-edits.md)): the bound is now `timeout-minutes: 20` on the PIT **step**, so a hit ends that step and the job still reports, where a hit on the old job cap cancelled the whole row. Still `continue-on-error: true`, so it never gates a merge | | `ManagedPCInstanceLifecycleTest.rapidToggleShouldNotCreateDuplicateInstances` | 1 seen (2026-09-02, astubbs#207, [job 100175277225](https://github.com/astubbs/parallel-consumer/actions/runs/33607572165/job/100175277225)) | Not from the original scan - **arrived on master with astubbs#29 and failed on the first PR to merge it**. `consumeCount` 0, repetition 1 of 5, `forkCount=4`, `probe clean`. Every wait in the test is a fixed sleep, and its assertion names a cause it cannot discriminate - see below | | `RegistrationRaceStaleResidentIT.freshArrivalCollidingWithStaleShardResidentMustStillGetProcessed` | 11 seen (2026-09-01; 2026-09-03 six times - once on a producer branch, twice on astubbs#419, twice in a row on astubbs#429, whose same head then passed on a deliberate re-run, and once on a `ce-optimize` measurement branch; on astubbs#442 four times - 2026-09-05 at 7075b26f, at 2ee84193 three heads later, and at 4fc15b8b, a head that differs from the green 69ecd0d1 before it by one markdown file, so the same-code control arm is the run immediately preceding it; and 2026-09-06 at eb1e88a1, again a documentation-only head - the identical `mid-loop pause point (offset 25)` guard every time, in the catch-all shard of a two-shard run, so still roughly one in two on that lane) | Not from the original scan - found while babysitting astubbs#257. Failed its **saturation/pause-point setup guard**, not the confluentinc#909 signature assertion, so it proves nothing about the defect it reproduces - see below **It also has a same-day cross-branch control**: `bin/inflight.mjs codecov test freshArrivalCollidingWithStaleShardResidentMustStillGetProcessed` shows it failing on two unrelated branches within an hour with the identical `mid-loop pause point (offset 25)` signature, while passing on both of those same branches at neighbouring commits - so it is master-state and test-side, not any one branch's regression, answered from recorded history rather than by re-running builds. | | `AmbientProbeExtensionTest.headroomIsReportedOnAPassingTestToo` and `.headroomOutcomeComesFromTheWatcherPhaseNotTheEndOfTheTestMethod` | 2/2 isolated runs, 1 seen in a full core run (2026-09-02, local, astubbs#116) | **DIAGNOSED, and not a product defect** - two methods of one class each capture the *same process-global* logger with `LogCapture.of(AmbientProbeExtension.class)` while the suite runs them concurrently, so each sees the other's headroom line and the `hasSize(1)` assertion gets 2. Reproduces on demand - see below | From a3d75a6342acd806de6637cf5ad5aaa64871dbb0 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 13:39:14 +1200 Subject: [PATCH 07/14] fix(ci): the copyright self-test controls its own environment, so the fold cannot redefine its default The copyright scanner's self-test asserts BOTH arms of the fork-point rule: a missing fork point warns and skips (exit 0) by default, and hard-fails (exit 2) under COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1. The default-behaviour case set no variable at all, so it asserted whatever the ambient environment said the default was - which was fine for as long as nothing set it. The fold of copyright.yml into `repo: hygiene` set it. The deleted workflow put COPYRIGHT_CHECK_REQUIRE_FORK_POINT on the SCAN STEP only, so its self-test step never saw it; repo-hygiene.yml carries it on the JOB-level env block, where every step inherits it - including the sweep step, `bin/check-all.sh --with-tests --strict`, which discovers and runs this self-test by glob. Strict mode became the ambient default, the fixture repos deliberately have no fork point, and the assertion read "expected '0', got '2'". The default case now clears the variable explicitly with `env -u`, and says at the fix site why that is load-bearing. This STRENGTHENS the test rather than relaxing it: a case asserting a default must define the environment it is asserting about, not inherit one. Nothing was removed, nothing accepts exit 2 now that did not before, and the strict-mode case immediately below - which sets the variable explicitly, for the same reason - is untouched. CONTROL ARM. With the `env -u` reverted in place and everything else identical, `COPYRIGHT_CHECK_REQUIRE_FORK_POINT=1 bash bin/test-check-copyright-headers.sh` fails on exactly that one case (expected '0', got '2') and exits 1; with it restored, both arms exit 0. The clean arm passes either way, which is why this was green on a laptop and red only in CI. SAME DEFECT CLASS, SWEPT. The `repo: hygiene` job sets two variables at job level, PR_NUMBER and COPYRIGHT_CHECK_REQUIRE_FORK_POINT. Every `bin/test-*.sh` and `bin/test-*.mjs` was checked for reads of either. Only two self-tests read one at all: this one, and bin/test-check-branch-self-reference.sh, which already sets PR_NUMBER explicitly on all three of its gate invocations and unsets the GITHUB_* variables beside them - with a comment recording this same lesson from the last time it bit. No other self-test is exposed. The wider sweep for uncontrolled ambient reads across all 34 self-tests turned up only locally-assigned names and INFER_JDK / CI in bin/test-check-infer.sh, neither of which the job sets. The stale citation at the fix site is repaired in passing: the authoritative gate it named, copyright.yml, is a file this branch deleted. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- bin/test-check-copyright-headers.sh | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/bin/test-check-copyright-headers.sh b/bin/test-check-copyright-headers.sh index 0a66a6426..0324df2f7 100755 --- a/bin/test-check-copyright-headers.sh +++ b/bin/test-check-copyright-headers.sh @@ -214,8 +214,18 @@ assert_contains "clean repo reports zero violations" "0 violation(s)" "$out" # --- Fixture C: fork point not in history (rule 15) --------------------------------- # Default: WARN and skip (exit 0) - can't determine provenance, so degrade gracefully (e.g. a -# shallow clone or a `mvn validate` build); the authoritative gate is copyright.yml (fetch-depth: 0). -out=$( (cd "$repoB" && COPYRIGHT_CHECK_FORK_POINT=0000000000000000000000000000000000000000 \ +# shallow clone or a `mvn validate` build); the authoritative gate is the `repo: hygiene` job in +# .github/workflows/repo-hygiene.yml (fetch-depth: 0 + strict mode). +# +# `env -u` IS LOAD-BEARING, and this case is worth nothing without it. It asserts the DEFAULT, so it +# must not inherit an ambient COPYRIGHT_CHECK_REQUIRE_FORK_POINT that redefines what the default is. +# That is not hypothetical: repo-hygiene.yml sets that variable at JOB level for the real gate, and +# the same job runs `bin/check-all.sh --with-tests`, which sweeps this self-test - so the variable +# reaches here and turned this assertion into "expected 0, got 2". Green on a laptop, red only in +# CI, which is the worst place to learn it. Clear it explicitly rather than trusting the caller's +# environment; the strict-mode case below sets it explicitly for the same reason. +out=$( (cd "$repoB" && env -u COPYRIGHT_CHECK_REQUIRE_FORK_POINT \ + COPYRIGHT_CHECK_FORK_POINT=0000000000000000000000000000000000000000 \ bash "$SCANNER") 2>&1 ) && rc=0 || rc=$? assert "missing fork point skips (exit 0) by default" 0 "$rc" assert_contains "missing fork point warns + explains the fix" "fetch-depth: 0" "$out" From 1c54ed32229761c5883240b41abaa188a819d2ea Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 13:39:35 +1200 Subject: [PATCH 08/14] test(core): assert the quarantine audit through the sweep that now runs it, not the workflow that named it QuarantinedAnnotationContractTest asserted that .github/workflows/maven.yml contained the literal strings `bin/check-quarantine-registry.sh` and `bin/check-quarantine-owners.sh`. This branch deleted maven.yml's `quarantine: audit` job, because both gates were already running on every pull request inside `repo: hygiene`'s `bin/check-all.sh --with-tests --strict` sweep, which DISCOVERS gates by globbing `bin/check-*.sh` rather than naming them. No workflow names either script on the per-PR path any more, so the test's grep-the-workflow mechanism could only fail - it cannot express "runs via a glob sweep". THE ASSERTION IS REPLACED, NOT DELETED, and the guarantee it protects is end-to-end as before. The per-PR audit now has two homes and both are asserted: (a) The discovering sweep. repo-hygiene.yml runs `bin/check-all.sh --with-tests --strict` and triggers on `pull_request`; check-all.sh discovers gates by the `bin/check-*.sh` glob; each gate EXISTS at a path that glob matches; and neither gate is named in check-all.sh's two exception lists, PR_SCOPED and NEEDS_ARGS, which are the only ways a globbed gate is skipped. That last link is what actually proves the glob still covers them - without it, "the glob covers them" is an assumption rather than a measurement. The lists are read through a helper that fails loudly if either stops being a plain double-quoted assignment, because a silently-empty read would turn every membership check into a pass. (b) quarantine-lane.yml, which still names both scripts explicitly as fail-fast steps ahead of the lane run. Those assertions are unchanged. The assertion that the lane RUN (`bin/quarantined-test.sh`) must NOT be in maven.yml is kept, and maven.yml is still read for it. The method is renamed from perPrWorkflowRunsTheAuditAndTheLaneWorkflowRunsTheTests to theHygieneSweepRunsTheAuditOnEveryPrAndTheLaneWorkflowRunsTheTests, and both the class javadoc and the method javadoc now state where the audit runs and why grepping a workflow proves nothing about it. A reader arriving at a failure can tell the topology from the test alone. CONTROL ARM. Adding `check-quarantine-registry.sh` to check-all.sh's PR_SCOPED list - the one change that would silently stop the per-PR sweep running the audit, with every workflow still green - fails the new case with the message naming the list and its contents. Restored, the class is 12/12 green. The previous assertion could not detect that state at all. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- .../QuarantinedAnnotationContractTest.java | 90 +++++++++++++++++-- 1 file changed, 81 insertions(+), 9 deletions(-) diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java index f0adf30aa..f3c4f1456 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java @@ -24,10 +24,17 @@ /** * Self-test of the quarantine lane's wiring - the {@link Quarantined} mechanism only works while a set * of otherwise-unrelated string sites stay in agreement (annotation meta-tag, pom group exclusion, the - * gating CI scripts, the lane runner, the workflow job, the release guard). Any one of them drifting in + * gating CI scripts, the lane runner, the lane workflow, the release guard). Any one of them drifting in * a big refactor breaks the lane SILENTLY - worst case, quarantined tests vanish from both lanes (no * red anywhere, coverage just gone). This test pins them all together so drift fails the gating unit * suite instead. + *

+ * WHERE THE PER-PR AUDIT RUNS: not in a job of its own, and not in maven.yml. It reaches every pull + * request through {@code repo: hygiene}'s discovering {@code bin/check-all.sh} sweep, and again as + * explicit fail-fast steps in quarantine-lane.yml. A grep for the script names in a workflow therefore + * proves nothing about the first of those - see + * {@link #theHygieneSweepRunsTheAuditOnEveryPrAndTheLaneWorkflowRunsTheTests()} for what is asserted + * instead, and why. */ class QuarantinedAnnotationContractTest { @@ -196,20 +203,85 @@ void quarantineLaneRunnerIncludesOnlyTheQuarantinedGroup() throws IOException { .that(lane).contains("-Dexcluded.groups="); } + /** + * The two registry gates, named as {@code bin/check-all.sh} names them - basenames, because that is + * the spelling its exception lists use. + */ + private static final List QUARANTINE_AUDIT_GATES = + Arrays.asList("check-quarantine-registry.sh", "check-quarantine-owners.sh"); + + /** + * The per-PR audit has TWO homes, and neither of them is maven.yml any more. + *

+ * It used to be maven.yml's {@code quarantine: audit} job, which named both scripts literally, so this + * test could grep the workflow for them. That job was folded away because the same scripts were already + * running in {@code repo: hygiene}'s {@code bin/check-all.sh --with-tests --strict} sweep, and the sweep + * DISCOVERS gates by globbing {@code bin/check-*.sh} rather than naming them - deliberately, so that a + * gate added to {@code bin/} cannot run nowhere. Nothing literally names the scripts on the per-PR path + * now, so grepping a workflow for them can only fail. + *

+ * What replaces the grep is the chain the glob actually depends on, asserted link by link: the sweep runs + * on {@code pull_request} with both flags; each gate EXISTS at a path the glob matches; and neither is + * named in {@code check-all.sh}'s two exception lists, {@code PR_SCOPED} and {@code NEEDS_ARGS}, which are + * the only ways a globbed gate is skipped. That last link is the one doing the work - without it "the + * glob covers them" is an assumption rather than a measurement. + *

+ * The second home is quarantine-lane.yml, which still names both scripts explicitly as fail-fast steps + * ahead of the lane run, and is asserted separately below. The lane RUN itself must stay out of maven.yml. + */ @Test - void perPrWorkflowRunsTheAuditAndTheLaneWorkflowRunsTheTests() throws IOException { - String maven = read(REPO_ROOT.resolve(".github/workflows/maven.yml")); - assertWithMessage("per-PR audit must enforce the registry") - .that(maven).contains("bin/check-quarantine-registry.sh"); - assertThat(maven).contains("bin/check-quarantine-owners.sh"); - assertWithMessage("the lane RUN must NOT be in maven.yml - it lives in its own workflow " + - "with its own trigger set") - .that(maven).doesNotContain("bin/quarantined-test.sh"); + void theHygieneSweepRunsTheAuditOnEveryPrAndTheLaneWorkflowRunsTheTests() throws IOException { + // (a) The per-PR audit, reached through the discovering sweep rather than by name. + String hygiene = read(REPO_ROOT.resolve(".github/workflows/repo-hygiene.yml")); + assertWithMessage("the per-PR audit reaches the quarantine gates only through check-all.sh's sweep, " + + "and only --with-tests --strict makes that sweep run the self-tests and refuse a CANNOT") + .that(hygiene).contains("bin/check-all.sh --with-tests --strict"); + assertWithMessage("the sweep must run on pull_request or the audit is not per-PR at all") + .that(hygiene).contains("pull_request:"); + + String checkAll = read(REPO_ROOT.resolve("bin/check-all.sh")); + assertWithMessage("check-all.sh must discover gates by glob - a hardcoded list is what the fold " + + "away from per-gate jobs relies on NOT existing") + .that(checkAll).contains("bin/check-*.sh"); + String prScoped = exceptionList(checkAll, "PR_SCOPED"); + String needsArgs = exceptionList(checkAll, "NEEDS_ARGS"); + for (String gate : QUARANTINE_AUDIT_GATES) { + assertWithMessage("bin/" + gate + " must exist where check-all.sh's bin/check-*.sh glob finds it") + .that(Files.exists(REPO_ROOT.resolve("bin/" + gate))).isTrue(); + assertWithMessage(gate + " is in check-all.sh's PR_SCOPED list, so the per-PR sweep SKIPS it and " + + "no workflow runs the quarantine audit on a pull request. Found: " + prScoped) + .that(groups(prScoped.replace(' ', ','))).doesNotContain(gate); + assertWithMessage(gate + " is in check-all.sh's NEEDS_ARGS list, so the sweep skips it and the " + + "per-PR quarantine audit does not run. Found: " + needsArgs) + .that(groups(needsArgs.replace(' ', ','))).doesNotContain(gate); + } + + // (b) The lane workflow, which does still name both scripts - see quarantineLaneRunnerIncludesOnly... String lane = read(REPO_ROOT.resolve(".github/workflows/quarantine-lane.yml")); assertThat(lane).contains("bin/quarantined-test.sh"); assertWithMessage("the lane fail-fasts on rule violations before spending a test run") .that(lane).contains("bin/check-quarantine-registry.sh"); assertThat(lane).contains("bin/check-quarantine-owners.sh"); + + String maven = read(REPO_ROOT.resolve(".github/workflows/maven.yml")); + assertWithMessage("the lane RUN must NOT be in maven.yml - it lives in its own workflow " + + "with its own trigger set") + .that(maven).doesNotContain("bin/quarantined-test.sh"); + } + + /** + * The space-separated value of one of {@code bin/check-all.sh}'s exception lists. A list that stopped + * being a plain double-quoted assignment fails here rather than silently reading as empty, which would + * turn every membership check above into a pass. + */ + private static String exceptionList(String checkAllBody, String name) { + java.util.regex.Matcher assignment = java.util.regex.Pattern + .compile("(?m)^" + java.util.regex.Pattern.quote(name) + "=\"([^\"]*)\"\\s*$") + .matcher(checkAllBody); + assertWithMessage("bin/check-all.sh must assign " + name + " as a plain double-quoted list - this test " + + "reads it to prove the quarantine gates are not excluded from the glob") + .that(assignment.find()).isTrue(); + return assignment.group(1); } /** From 1bfa32b218f34e56fb5c9a66587dbf6a93359a4c Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 13:41:01 +1200 Subject: [PATCH 09/14] docs(refactoring): register the six-times-duplicated Maven setup block, and why no scanner reports it Six jobs in maven.yml open with the same checkout, setup-java and cache-restore steps, and the cache key must agree in all six because the file's header forbids setup-java's own cache. Two of the six arrived with the job folds on this branch. There is no .github/actions/ directory, so nothing shares them. The entry carries the measurement rather than the assertion, because "the duplication scanners are green" reads as evidence and here it is not. PMD CPD never reads a workflow at all: its language detect is winner-take-all, Java wins, and YAML is not mappable by it. jscpd is language-agnostic and does read them - pointed at the workflow directory it reports real clones, including the one pair it still sees at the job's own default threshold, between the two review workflows. Pointed at maven.yml alone it reports nothing at any threshold down to 20 tokens, because each copy of the block has different comments interleaved between its steps: strip the comments and the six are identical, leave them in and the token streams differ. That is the capability half of the 2026-08-12 write-up on the scanners not looking where agents duplicate. Its scope half was fixed when the scan widened to the whole repo; this half survived that fix, and the entry says so where somebody checking a green report will find it. Recorded with the OSS Index audit steps, now duplicated across two workflows behind a "change both, or neither" note, because one composite-action decision settles both. Deliberately not done here: this PR already rewrites every workflow and owes ruleset edits at merge, and its whole risk is a check quietly ceasing to run. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- docs/refactoring.md | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/docs/refactoring.md b/docs/refactoring.md index 3266176c5..e741d1712 100644 --- a/docs/refactoring.md +++ b/docs/refactoring.md @@ -223,6 +223,46 @@ prevent. The `docs-views.mjs` split earned its place because docs is *several* f other command's single formatter still lives here, so moving `rank` alone makes it the odd one out. When the second one arrives, take `formatRank` with it and import the shared helpers one way. +### The JDK-and-Maven-cache setup block exists six times in `maven.yml`, and no detector can see it + +`prepare-deps`, `test`, `test-kafka-compat`, `scan`, `static` and `build` each open with the same +three steps: `actions/checkout`, `actions/setup-java` on Temurin 17, and an `actions/cache/restore` +keyed `setup-java-Linux-x64-maven-${{ hashFiles('**/pom.xml') }}` with the bare prefix as its +`restore-keys` fallback. Two of the six arrived with the 2026-09-07 job folds, which gave `scan: repo` +and `static: analysis` a build dependency they did not have before. `.github/actions/` does not exist, +so there is no composite action to point them at. + +The cost is drift: the cache key and its fallback must agree in all six places, and the file's own +header forbids `setup-java`'s built-in `cache: maven` because its immutable keys can freeze an +incomplete cache. Six hand-maintained copies of a rule that must not vary is the shape that produces +a silently wrong one. + +**Neither duplication engine will ever report this, and the reason is worth keeping.** `dups: clones` +runs two engines over the whole repo. PMD CPD's language auto-detect is winner-take-all, Java wins on +file count, and YAML is not CPD-mappable at all - so CPD never reads a workflow. jscpd *is* +language-agnostic and does read them: pointed at `.github/workflows` it finds real clones, including +`claude.yml` against `claude-code-review-dispatch.yml`, which is the single pair it reports at the +job's own default of 50 minimum tokens. Pointed at `maven.yml` alone it finds **nothing, at any +threshold down to 20 tokens**, because each copy of the block has *different explanatory comments +interleaved between its steps*: strip the comments and the six are identical, leave them in and the +token streams differ. The block is also only about 40 tokens, under the default 50. + +That is the capability half of +[`docs/solutions/workflow-issues/duplication-scanners-do-not-look-where-agents-duplicate-2026-08-12.md`](solutions/workflow-issues/duplication-scanners-do-not-look-where-agents-duplicate-2026-08-12.md), +which diagnosed both scope and capability. The scope half was fixed when the scan widened to the +whole repo; this is the half that survived it. + +**A second instance of the same fix**, recorded together because one composite-action decision +settles both: the five OSS Index audit steps now exist in `maven.yml` (the per-PR run) and in +`dependency-audit.yml` (the schedule and dispatch), with a "change both, or neither" note in each +header. That note is an admission that the mechanism is missing. + +**Deliberately not done in astubbs/parallel-consumer#457**, which introduced two of the copies. That +PR already rewrites every workflow and owes ten ruleset edits at merge; its whole risk is a check +quietly ceasing to run, and introducing a mechanism the repo has never used, in the files it is +already rewriting, widens exactly that blast radius. The duplication is stable and commented, so it +keeps. + ### The portable-mtime probe exists three times `hook_file_mtime` in `.claude/hooks/lib/hook-common.sh`, `_mtime` in From 6c310deafbded01dbd41c77a1ed2ec57fa3c2134 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 15:06:21 +1200 Subject: [PATCH 10/14] docs(inflight): the ruleset checklist names its lists, and stops counting them The note is the merge-time checklist for the ruleset edit the folds owe, and it was writing derived cardinalities all through its prose - "All ten old names", "An eleventh", "the four jobs", ordinal step positions, "Add two". Every one of them is a second copy of a list that sits a few lines away, and docs/inflight/AGENTS.md forbids exactly this ("Never write down what a command can answer" - and it says counts of any kind, not just git facts) because the copy is wrong the first time somebody edits the list and nothing goes red. Here the drift has an unusually sharp failure mode, which is why this was worth doing rather than filing: a reader who trusts "ten" over the list can leave a name required that no run will ever produce, and a required context nothing produces pends every PR in the repository - the spotbugs incident again. The count is the one thing in this document that can silently make its own instructions wrong. So the name lists are untouched and are now stated to be the authority, the ordinals are gone, and "how many" is answered by a command instead: gh api repos/astubbs/parallel-consumer/rules/branches/master \ --jq '.[] | select(.type == "required_status_checks") | .parameters.required_status_checks[].context' Run at the merge, that prints what is actually required, which is the thing the tally was standing in for. Verified against the live ruleset while writing this: it returns every name on the removal list. Two counts stay, because they cannot drift apart from what they count - each names its members in the same sentence ("three of the checklist's named self-test steps (a, b, c)", "all four self-tests ok" directly under the four script names). The rule is about a number that can outlive its list. The `` marker moved with the paragraph it excuses: the gate reads upward and stops at a blank line, so inserting a paragraph between the citation and its marker silently orphans both. bin/check-file-refs.sh caught that, which is the second time this run that a gate found something a read- through did not. Raised by codex in review on astubbs/parallel-consumer#457. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 71 +++++++++++++------- 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md index c723489ab..a0553b837 100644 --- a/docs/inflight/ci-fewer-jobs-ruleset-edits.md +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -3,28 +3,35 @@ -Three standalone jobs were deleted because `repo: hygiene` already ran every gate they ran, via +Standalone jobs were deleted because `repo: hygiene` already ran every gate they ran, via `bin/check-all.sh --with-tests`'s glob, on every PR: `Copyright header check` (the whole of `.github/workflows/copyright.yml`), and `.github/workflows/maven.yml`'s `quarantine: audit` and -`docs data: audit`. A fourth, `PR Checklist` (the whole of `.github/workflows/pr-checklist.yml`), +`docs data: audit`. `PR Checklist` (the whole of `.github/workflows/pr-checklist.yml`) was deleted because its steps could run as the tail of the same job - see "Why hygiene hosts the -checklist" below. Three more, `maven.yml`'s no-build scanners `dups: clones`, `dups: similarity` and -`deps: vulnerabilities`, became steps of one new job, `scan: repo` - see "The scanner fold" below. Two more, `maven.yml`'s -build-dependent static analysers `static: infer` and `static: spotbugs`, became steps of one new job, -`static: analysis` - see "The static-analysis fold" below. A tenth, `dependency-audit.yml`'s -`deps: whole-tree CVE scan`, became a fourth step of `scan: repo` - see "The CVE fold" below; that -workflow keeps its `schedule` and `workflow_dispatch` triggers and is not deleted. An eleventh, -`maven.yml`'s `Mutation Tests (PIT, PR-scoped)`, became the fifth and last step of `scan: repo` - -and is the one of these eleven folds that owes **no** ruleset edit at all; see "The PIT fold owes no +checklist" below. `maven.yml`'s no-build scanners `dups: clones`, `dups: similarity` and +`deps: vulnerabilities` became steps of one new job, `scan: repo` - see "The scanner fold" below. Its +build-dependent static analysers `static: infer` and `static: spotbugs` became steps of one new job, +`static: analysis` - see "The static-analysis fold" below. `dependency-audit.yml`'s +`deps: whole-tree CVE scan` became a further step of `scan: repo` - see "The CVE fold" below; that +workflow keeps its `schedule` and `workflow_dispatch` triggers and is not deleted. `maven.yml`'s +`Mutation Tests (PIT, PR-scoped)` became the last step of `scan: repo` - +and is the one fold here that owes **no** ruleset edit at all; see "The PIT fold owes no ruleset edit" below, which exists so a reader diffing the checks list does not go looking for the entry that is deliberately absent. -All ten old names are still **required status-check contexts in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it +Every name on the removal list in "The edit" below is still a **required status-check context in the master ruleset**, and the ruleset is repository settings, not tree state - no PR can change it ([`docs/ci.md`](../ci.md), "The required list is repository settings, not tree state"). +**The lists below are the authority; nothing here counts them.** A cardinality written into prose +drifts the moment a fold is added or dropped, and here the drift lands as an INCOMPLETE RULESET +EDIT - a context left required that nothing produces, which pends every PR in the repository +([`docs/inflight/AGENTS.md`](AGENTS.md), "Never write down what a command can answer"). Read the +lists; if you want the number, `gh` will tell you what the ruleset actually holds - see "The edit". + ## The edit -Remove these ten contexts from the master ruleset's `required_status_checks`: +Remove **every** context on this list from the master ruleset's `required_status_checks` - the list +is the checklist, and a name left behind is a context nothing will ever produce: - `Copyright header check` - `quarantine: audit` @@ -37,7 +44,7 @@ Remove these ten contexts from the master ruleset's `required_status_checks`: - `static: spotbugs` - `deps: whole-tree CVE scan` -Add two: +Add: - `scan: repo` - `static: analysis` @@ -46,11 +53,23 @@ Add two: `gh api repos/astubbs/parallel-consumer/rules/branches/master`; that command is the answer, not this sentence). `scan: repo` and `static: analysis` are new, so they are not. +**Diff the two lists above against the live ruleset rather than counting either.** What is required +right now, one context per line: + +```bash +gh api repos/astubbs/parallel-consumer/rules/branches/master \ + --jq '.[] | select(.type == "required_status_checks") + | .parameters.required_status_checks[].context' +``` + +The edit is done when that output carries none of the removal names and both of the additions. That +command is the check; a tally written here would only be a second, staler copy of it. + ## When: at the merge of the deleting PR, not before and not after A required context nothing produces leaves every PR **pending** - it never fails, it never passes. -- **Before the merge** the four jobs still run on every other open PR, so dropping the contexts +- **Before the merge** the deleted jobs still run on every other open PR, so dropping the contexts early only widens the window in which a broken header, a drifted registry or an unresolved checklist could merge on a PR opened before the fold. - **After the merge** no run produces them, so every PR in the repository pends until somebody edits @@ -58,7 +77,7 @@ A required context nothing produces leaves every PR **pending** - it never fails required"). So: edit the ruleset in the same sitting as the merge. Only one merge is exposed either way - the -deleting PR's own checks list shows the ten contexts as expected-but-missing until the ruleset +deleting PR's own checks list shows the removal list's contexts as expected-but-missing until the ruleset drops them, which is the intended tell that the edit is still owed, not a fault in that PR. **The add has the opposite ordering.** A new required context that no master run has produced @@ -69,10 +88,10 @@ the job definition is on master, so every PR opened afterwards produces the cont merge, then: 1. Merge the deleting PR. -2. In the same sitting, remove the ten old contexts. +2. In the same sitting, remove every context on the removal list above. 3. Add `scan: repo` and `static: analysis` only after the merge has landed and a PR run has produced each context - the first PR to rebase onto the merged master shows both in its checks list; that - is the evidence. Until then the three scanners and the two analysers gate nothing, which is a + is the evidence. Until then the folded scanners and analysers gate nothing, which is a window measured in one PR's CI run, and is preferable to every PR pending on a context nothing yet produces. @@ -112,7 +131,7 @@ fold had to keep: - `check-all.sh` used to exit 0 on a CANNOT (exit 2) as long as something else ran. The hygiene job now runs `--strict`, under which a CANNOT fails the sweep, and asserts PyYAML and shellcheck are present in named steps - so the folded gates cannot silently turn into skips on an image change. -- For the checklist fold: the three dropped self-test steps were confirmed against +- For the checklist fold: the dropped self-test steps were confirmed against `ls bin/test-*.sh` - each file exists and matches the `bin/test-*.sh` glob `check-all.sh --with-tests` iterates, so each already ran in this job. `bin/todo-index.sh --check` does not match `bin/check-*`, and cannot be added to the glob because run bare it regenerates the index @@ -178,16 +197,16 @@ re-read it whenever a job is renamed or folded. ## The PIT fold owes no ruleset edit - and that is the point of this section -`Mutation Tests (PIT, PR-scoped)` is the eleventh check the `ci-fewer-jobs` folds remove from a PR's -checks list, and the only one whose name is **not** on the removal list above. It was never a required +`Mutation Tests (PIT, PR-scoped)` is a check the `ci-fewer-jobs` folds remove from a PR's +checks list, and the only one removed whose name is **not** on the removal list above. It was never a required context: [`docs/ci.md`](../ci.md)'s "These are deliberately NOT required" table has always carried a row for it, because the job was `continue-on-error: true` and a required check reads the *conclusion* - which `continue-on-error` makes success even when the step fails. Requiring it would have gated nothing. So there is nothing to remove, and nothing to add. Written down because the absence is indistinguishable from an oversight. Somebody comparing a PR's -checks list before and after the merge sees eleven rows go and ten names accounted for here; without -this paragraph the eleventh reads as a missed edit, and the natural repair - adding +checks list before and after the merge sees one more row disappear than the removal list accounts +for; without this paragraph that row reads as a missed edit, and the natural repair - adding `Mutation Tests (PIT, PR-scoped)` to a ruleset that never had it - fails with a context nothing has ever produced. @@ -210,7 +229,7 @@ What the fold had to carry: including fork and Dependabot ones, so leaving them guarded would have handed the mutation lane a cold repository on exactly the PRs that get no CVE scan. Every step that talks to OSS Index is still guarded, and `server-id: ossindex` is inert when those steps skip. -- **`fetch-depth: 0` was already there** for the two duplication tools, which is what the mutation +- **`fetch-depth: 0` was already there** for the duplication tools, which is what the mutation lane needs to diff against the PR base; it must not be narrowed, because an unresolvable base ref makes `bin/ci-mutation-test.sh` fall back to the *full* glob. - **The self-test still runs first,** `continue-on-error` like the run step - it also runs @@ -231,7 +250,7 @@ PIT summary now hangs off `scan: repo`. `deps: whole-tree CVE scan` was the whole of `dependency-audit.yml`'s `ossindex` job, and that workflow's `pull_request` trigger was its only PR-time producer. The trigger is gone and the job is -now a fourth step of `scan: repo`; the workflow itself is **not deleted** - it keeps `schedule` and +now a step of `scan: repo`; the workflow itself is **not deleted** - it keeps `schedule` and `workflow_dispatch`, which is the half no PR can cover (an unchanged tree acquiring a new advisory). So the name `deps: whole-tree CVE scan` still exists in the tree, as both a job in that workflow and a step in this one, and is still on the removal list above: no PR run produces it any more, and a @@ -257,7 +276,7 @@ What the fold had to carry: - **`timeout-minutes: 25`** where the scanners held 10 and the audit job held 20. - **The exposure the standalone job avoided, and what contains it here.** That job deliberately held `contents: read` only, because it runs PR-authored build code in the same job as the OSS Index - token. `scan: repo` also holds `pull-requests: write`, for the three tools that post comments. + token. `scan: repo` also holds `pull-requests: write`, for the tools that post comments. What contains it is the guard itself: the CVE steps run only for a branch in *this* repository, pushed by somebody who already has write access, so the build code is not attacker-supplied the way a fork PR's would be. The OSS Index secrets stay in `env:` on the two steps that need them and @@ -269,6 +288,6 @@ What the fold had to carry: checks that the copies agree; both file headers say so. This note tracks only the owed edits. Once the live ruleset lists `scan: repo` and -`static: analysis` and none of the ten old contexts, nothing here is both true and unowned elsewhere +`static: analysis` and none of the removal list's contexts, nothing here is both true and unowned elsewhere - the reasoning is in [`docs/ci.md`](../ci.md), the `repo-hygiene.yml` and `dependency-audit.yml` headers, and the `scan: repo` and `static: analysis` jobs' own comments. From 48d3ebddc1ccf8a1b69b86476c13c444d0f0163c Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 15:06:45 +1200 Subject: [PATCH 11/14] ci(fewer-jobs): a prepare-deps failure must not skip the required batched checks The scanner fold handed `scan: repo` a `needs: prepare-deps` edge that none of the three jobs it replaces had. `dups: clones`, `dups: similarity` and `deps: vulnerabilities` never built anything and never touched the Maven cache; they reported through a failed cache lane. Only the CVE and PIT steps at the tail of the merged job want the cache, and the edge was added for them. With the implicit job-level `success()` still in force, that edge is a new way to wedge the repository. A failed or timed-out `prepare-deps` - the Azure west-US Maven Central timeout that job exists to fix, so not hypothetical - SKIPS the whole job. `scan: repo` is a required check, and a skipped required check does not go red: it stays PENDING, forever, on every PR. So a transient cache failure would have suppressed three scanners that need no cache at all and left the PR waiting on a context that would never arrive. MECHANISM CHOSEN, and why it is this one. The job's `if:` now reads `!cancelled() && github.event_name == 'pull_request'`. Naming a status function replaces the implicit `success()` gate on the `needs:` edge, so the job runs when prepare-deps fails and still reports a conclusion. `!cancelled()` rather than `always()` so a superseded push still cancels it instead of spending a runner on work nobody will read. NOTHING DEGRADES INTO A PASS, which is the constraint that ruled out the alternatives. Moving the dependency onto the Maven-dependent steps alone would have needed a per-step `needs`, which does not exist; dropping the edge would have cost every scan run a cold cache. Running anyway costs nothing false: the scanners need no Maven, the cache step is `actions/cache/restore` with a prefix `restore-keys` fallback so the CVE and PIT steps get an older or cold cache rather than none, and a Maven step that genuinely cannot resolve fails its step and reds the job. A cannot-measure reads as red, never as a tick - the defect class this repo keeps relearning. SAME DEFECT, SECOND SITE, fixed with it. `static: analysis` is the other required context this fold creates and it carries the same edge. There it is not a regression - `static: infer` and `static: spotbugs` were both already required and both already had it - but a required name about to be added to the ruleset can be wedged the same way, so it takes the same guard. Checked and dismissed: `test`, `test-kafka-compat` and `build` also need prepare-deps and are untouched by this PR, so their behaviour is unchanged and out of scope here. Verified: actionlint reports the same three findings as the base and no new ones, and the job-count harness still measures 18 PR jobs with 0 dropped checks, so the `if:` rewrite has not changed what the fold delivers. Raised by codex in review on astubbs/parallel-consumer#457. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- .github/workflows/maven.yml | 28 ++++++++++++++++++-- docs/ci.md | 6 ++++- docs/inflight/ci-fewer-jobs-ruleset-edits.md | 14 ++++++++++ 3 files changed, 45 insertions(+), 3 deletions(-) diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index cbf7a3b09..c703fea8f 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -618,8 +618,25 @@ jobs: # repository, i.e. one pushed by somebody who already has write access, so the build code they # execute is not attacker-supplied in the way a fork PR's would be. The OSS Index secrets stay on # the two steps that need them and are not visible to the pinned third-party actions above. + # A SKIPPED REQUIRED CHECK IS PENDING FOREVER, WHICH IS WHY THIS `if:` NAMES A STATUS FUNCTION. + # `!cancelled()` replaces the implicit job-level `success()`, so the `needs: prepare-deps` edge + # below can no longer SKIP this job when the cache lane fails. That mattered here and nowhere else + # in this fold: the three no-build scanners were three jobs with no dependency on prepare-deps at + # all, and they reported through a failed cache job; batching them behind the CVE and PIT steps' + # cache edge silently made a transient Maven Central timeout (docs/solutions/build-errors/ - the + # Azure west-US case prepare-deps exists for) able to wedge every PR on a required context that + # never resolves. Red is recoverable and visible; pending is neither. + # + # NOTHING IS DEGRADED INTO A PASS by running anyway, which is the constraint that decides the + # shape. The scanners need no Maven at all. The cache restore below is restore-only with a prefix + # fallback, so with prepare-deps dead the CVE and PIT steps get an older cache or a cold one - + # slower, not silently weaker - and a Maven step that genuinely cannot resolve still fails its + # step, which still fails this job. A CANNOT-measure reads as red, never as a tick. + # + # `!cancelled()` and not `always()`: a superseded push cancels this run, and it should stay + # cancelled rather than spend a runner finishing work nobody will read. scan: - if: github.event_name == 'pull_request' + if: ${{ !cancelled() && github.event_name == 'pull_request' }} name: "scan: repo" runs-on: ubuntu-latest # The scanners held 10, the CVE audit 20 and the mutation lane 30. Sized for the sum of the @@ -947,8 +964,15 @@ jobs: # is the check: `static: analysis` is the required-check name in the making, and the two names it # replaces are still in the master ruleset. docs/inflight/ci-fewer-jobs-ruleset-edits.md owns the # swap and its ordering. + # `!cancelled()` for the reason spelled out on `scan: repo` above: `static: analysis` is the other + # required context this fold creates, it also carries a `needs: prepare-deps` edge, and a required + # check that is SKIPPED pends forever instead of going red. Unlike `scan: repo` this is not a + # regression the fold introduced - `static: infer` and `static: spotbugs` were both already + # required and both already had the edge - but it is the same defect under a name that is about to + # be added to the ruleset, and the cache restore below is restore-only with a prefix fallback, so + # the analysers degrade to a cold cache rather than to a false pass. static: - if: github.event_name == 'pull_request' + if: ${{ !cancelled() && github.event_name == 'pull_request' }} name: "static: analysis" runs-on: ubuntu-latest # The two jobs held 15 and 10. The sum plus an Infer toolchain download on a cache miss needs diff --git a/docs/ci.md b/docs/ci.md index fc87f146b..bf9bd3ccd 100644 --- a/docs/ci.md +++ b/docs/ci.md @@ -79,7 +79,11 @@ document. This section is the detail behind it. `deps: vulnerabilities`, `deps: whole-tree CVE scan`, `Mutation Tests (PIT, PR-scoped)`), so a red step still reads the way the red check did. The PIT steps are the only ones in either job carrying `continue-on-error` - the lane was an advisory *job* before the fold, and `scan: repo` is required, - so the flag is what stops the fold promoting it to a gate. Push to + so the flag is what stops the fold promoting it to a gate. Both batched jobs guard their `if:` + with `!cancelled()` rather than leaning on the implicit `success()`, because each one `needs: + prepare-deps` and a **required check that is skipped waits forever** instead of going red - so a + transient cache failure would otherwise wedge every PR. The batched steps still run, fall back to + a prefix-key cache restore, and go red honestly if they genuinely cannot resolve. Push to master runs a single full `bin/ci-build.sh` on the default Kafka version to gate SNAPSHOT publishing. All jobs use explicit `cache/restore` with rotating keys from the `prepare-deps` job - never `setup-java cache: 'maven'`. diff --git a/docs/inflight/ci-fewer-jobs-ruleset-edits.md b/docs/inflight/ci-fewer-jobs-ruleset-edits.md index a0553b837..a99dcf565 100644 --- a/docs/inflight/ci-fewer-jobs-ruleset-edits.md +++ b/docs/inflight/ci-fewer-jobs-ruleset-edits.md @@ -153,6 +153,20 @@ signal lands first in the log. **The three old job names live on as the step nam reads in the log the way the red check used to, and the references across `docs/inflight/` to what `dups: clones` or `dups: similarity` found stay accurate - only the check name changed. +**The fold gave the scanners a `needs: prepare-deps` edge they never had, and that edge had to be +made non-skipping.** The CVE and PIT steps at the end of the job want the Maven cache, so the merged +job depends on the cache lane; the three scanners themselves depend on nothing. Left with the +implicit job-level `success()`, a failed or timed-out `prepare-deps` - the Azure west-US Maven +Central timeout that lane exists for - would SKIP `scan: repo` entirely, and a skipped **required** +check pends forever rather than going red, so a transient cache failure would wedge the PR on a +context nothing will emit ([`docs/ci.md`](../ci.md), "A skip does not satisfy a required check"). +The job's `if:` is therefore `!cancelled() && github.event_name == 'pull_request'`: the scanners +still report, the cache restore falls back to its prefix key, and a Maven step that genuinely cannot +resolve fails its step and reds the check. Nothing degrades into a tick. `static: analysis` carries +the same guard for the same reason - there the edge is inherited from the two jobs it merges rather +than introduced, but it is about to become a required context under a new name, so the same skip +would have the same effect. + Not folded into `repo: hygiene`: hygiene runs PR-authored scripts under a read-only token by design, and all three scanners need `pull-requests: write` to post. Keeping the write grant in a job that runs only pinned third-party actions is the same reviewer-isolation line `docs/ci.md` draws for From b94b5170a95aabeecc0bf1eef2da43fdcb355d04 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 15:07:05 +1200 Subject: [PATCH 12/14] test(ci): a regression case for --strict, the flag holding up the folded gates The dedicated per-gate CI jobs are gone from this branch - copyright, the quarantine audit, docs-data - and what replaced them is one `repo: hygiene` step running `bin/check-all.sh --with-tests --strict`. Those jobs installed what they needed explicitly, so a hosted image that stopped shipping PyYAML or ShellCheck turned them red. Folded into the sweep, the same image change turns the gate into a CANNOT, and the sweep's DEFAULT exits 0 on a CANNOT as long as something else ran. `--strict` is the single line of behaviour standing between that and a green `repo: hygiene` over gates that measured nothing. bin/test-check-all.sh never invoked it. Its exit-2 fixture asserted only the default's exit 0, so a regression accepting a CANNOT under strict would have been caught by no test in this repository, and the fold's own justification - "the sweep now takes --strict" - rested on an untested line. bin/AGENTS.md requires a checker fix to carry a case that goes red against the old code; this one had been recorded as a follow-up and not done. Four arms, and the pair of controls that make them mean something: - exit 2 FAILS under --strict, and says why a CANNOT on a runner is never a legitimate skip. - CONTROL, same fixture, one term changed: without --strict the identical CANNOT still exits 0. So the arm above measures the flag, not the fixture. - exit 3 stays nothing-in-scope under --strict. That distinction is what makes --strict safe to run in CI at all - "measured, nothing in scope" is a result where "could not measure" is not - and pinning it stops a later tightening quietly redefining one as the other. - a genuine exit-1 failure is still reported as a FAILURE and not relabelled a strict-CANNOT. Both exit 1, so only the message separates them. VERIFIED AGAINST THE PARENT IMPLEMENTATION, twice, because "it did not exist yet" is the weaker of the two controls: 1. Against origin/master's check-all.sh (pre---strict), the four new arms fail and every pre-existing arm passes: 20 passed, 4 failed. That proves the arms are exercising --strict and nothing else, but it fails for the trivial reason that the flag is an unknown argument. 2. The targeted one. Take the current script, keep --strict parsed and accepted, delete only the block that turns a CANNOT into exit 1. Exactly the two arms asserting that behaviour go red - exit 0 where 1 was wanted - and all 22 others stay green, the exit-3 and genuine-failure arms included. That is the regression the reviewer described, reproduced and caught. On this branch, unmodified: 24 passed, 0 failed. Raised by codex in review on astubbs/parallel-consumer#457. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- bin/test-check-all.sh | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/bin/test-check-all.sh b/bin/test-check-all.sh index dc5481197..4cc77d5a2 100755 --- a/bin/test-check-all.sh +++ b/bin/test-check-all.sh @@ -83,6 +83,46 @@ d="$(make_fixture nothing 3)"; out="$(run "$d")"; rc=$? assert "exit 3 is nothing-in-scope, also not a pass" 0 "1 nothing-in-scope" "$out" "$rc" rm -rf "$d" +echo +echo "=== --strict makes a CANNOT fail, which is the only thing holding up the folded gates ===" + +# THIS IS THE LOAD-BEARING ARM OF THE WHOLE FILE NOW. The dedicated per-gate CI jobs are gone - +# copyright, the quarantine registry, docs-data - and what replaced them is one `repo: hygiene` step +# running this sweep with --strict. Those jobs used to install what they needed explicitly, so a +# hosted image that stopped shipping PyYAML or ShellCheck showed up as a red job. Folded into the +# sweep, the same image change turns the gate into a CANNOT, and the sweep's DEFAULT exits 0 on a +# CANNOT as long as something else ran - a green `repo: hygiene` over gates that measured nothing. +# --strict is the single line of behaviour that stops that, and until this arm existed nothing here +# invoked it: the exit-2 fixture below asserted only the default's exit 0, so a regression that +# accepted a CANNOT under --strict would have been caught by no test in this repository. +d="$(make_fixture cannot 2)"; out="$(run "$d" --strict)"; rc=$? +assert "red: exit 2 FAILS the sweep under --strict" 1 "FAILED (strict - could not run): check-cannot.sh" "$out" "$rc" +assert "...and says why a CANNOT on a runner is never a legitimate skip" 1 \ + "a gate that could not run is a FAILURE here" "$out" "$rc" +rm -rf "$d" + +# CONTROL, same fixture, one term changed. Without --strict the identical CANNOT exits 0, so the arm +# above is measuring the flag rather than the fixture. (The exit-2 arm further up asserts the same +# thing from the other side; this repeats it here because a regression arm whose control lives +# thirty lines away is one edit from becoming a tautology.) +d="$(make_fixture cannot 2)"; out="$(run "$d")"; rc=$? +assert "green control: the same CANNOT does not fail the DEFAULT sweep" 0 "COULD NOT RUN: check-cannot.sh" "$out" "$rc" +rm -rf "$d" + +# EXIT 3 IS NOT SWEPT UP BY --strict, and that distinction is the reason --strict is safe to run in +# CI at all: "I measured and there was nothing in scope" is a result, "I could not measure" is not. +# Pinned so a future tightening of --strict cannot quietly redefine nothing-in-scope as a failure. +d="$(make_fixture nothing 3)"; out="$(run "$d" --strict)"; rc=$? +assert "green: exit 3 stays nothing-in-scope under --strict" 0 "1 nothing-in-scope" "$out" "$rc" +rm -rf "$d" + +# A FAILURE UNDER --strict MUST STILL REPORT AS A FAILURE, not be relabelled as a strict-CANNOT. +# The two exits are both 1, so only the message distinguishes them, and a reader who cannot tell +# "the gate said no" from "the gate never ran" has lost the distinction this script exists for. +d="$(make_fixture failing 1)"; out="$(run "$d" --strict)"; rc=$? +assert "red: --strict does not relabel a genuine failure" 1 "FAILED: check-failing.sh" "$out" "$rc" +rm -rf "$d" + echo echo "=== the exception list must not rot silently ===" From ab82a73ff4e0db247955ea36f4391c61357e3a08 Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 15:07:26 +1200 Subject: [PATCH 13/14] test(core): read the workflow trigger from the on: block, not from the file text `theHygieneSweepRunsTheAuditOnEveryPrAndTheLaneWorkflowRunsTheTests` asserted that the quarantine audit reaches every pull request by checking that repo-hygiene.yml contains the literal `pull_request:`. That file also carries the literal inside a comment - the roadmap gate's carrier line, `pull_request: astubbs#NNN` - so the assertion was satisfied by prose. Delete the real trigger and the test stayed green while the invariant it names, "the per-PR audit runs on every PR", was false. That matters more here than it looks. The whole point of the assertion is that the per-PR audit no longer has a job naming it: the fold left the discovering `check-all.sh` sweep as the only path, and this test is the chain that proves the sweep is reached. A link in that chain a comment can satisfy is not a link. The fix is a `workflowTriggers(String)` helper that walks the top-level `on:` block: it opens at a column-0 `on:` (or `"on":` - YAML 1.1 reads a bare `on` as a boolean, so some editors quote it), closes at the next column-0 key, skips comment and blank lines, and returns keys at the block's own indent, so `types:` nested under `pull_request:` is not mistaken for a trigger. The inline forms (`on: push`, `on: [push, pull_request]`) are handled too, so rewriting the block in flow style will not silently empty the result rather than failing. The assertion messages print the triggers actually found, which is the thing a reader needs when this goes red. CONTROL. Removed the real `pull_request:` trigger and its `types:` line from repo-hygiene.yml and ran the test: - the old assertion's evidence survived - `grep 'pull_request:'` still matched, on the comment line, so the previous form would have passed; - the new assertion failed, reporting `Declared triggers: [push, workflow_dispatch]`. Restored, and the class passes 12/12. SAME DEFECT, SECOND SITE, in the same file. `laneWorkflowDeclaresItsTriggers` asserts quarantine-lane.yml's three triggers the same way. It is pre-existing and happens to be correct today only because that file has no comment carrying one of those literals WITH its colon - correct by luck, not by construction, which is precisely the shape that went wrong one method up. It now reads through the same helper. Checked and dismissed: nothing else in the tree asserts a workflow trigger by substring (`grep -rn "pull_request\|workflow_dispatch\| schedule:" --include=*.java` returns only these two methods and unrelated javadoc). Raised by codex in review on astubbs/parallel-consumer#457. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- .../QuarantinedAnnotationContractTest.java | 93 +++++++++++++++++-- 1 file changed, 85 insertions(+), 8 deletions(-) diff --git a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java index f3c4f1456..78ef8d4ca 100644 --- a/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java +++ b/parallel-consumer-core/src/test/java/bz/stub/parallelconsumer/QuarantinedAnnotationContractTest.java @@ -15,6 +15,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -195,6 +196,71 @@ private static List groups(String commaSeparated) { return Arrays.asList(commaSeparated.split(",")); } + /** + * The trigger names declared in a workflow's top-level {@code on:} block - and nothing else in the file. + *

+ * A plain {@code contains("pull_request:")} over the whole file CANNOT assert a trigger, and + * repo-hygiene.yml is the live proof: it carries that exact literal inside a comment describing the + * roadmap gate's carrier line ({@code pull_request: astubbs#NNN}), so deleting the real trigger would + * leave the grep green. An assertion a comment can satisfy is prose-matching, not a contract. + *

+ * So this walks the block instead: it opens at a column-0 {@code on:} (or {@code "on":} - YAML 1.1 + * reads a bare {@code on} as a boolean, so some editors quote it), closes at the next column-0 key, + * skips comment and blank lines, and returns the keys at the block's own indent - which is why + * {@code types:} under {@code pull_request:} is not mistaken for a trigger. The inline forms + * ({@code on: push}, {@code on: [push, pull_request]}) are handled too, so rewriting the block in + * flow style does not silently empty the result. + */ + private static List workflowTriggers(String yaml) { + List triggers = new ArrayList<>(); + boolean inOnBlock = false; + int blockIndent = -1; + for (String raw : yaml.split("\n", -1)) { + String line = raw.replaceAll("\\s+$", ""); + String trimmed = line.trim(); + if (trimmed.isEmpty() || trimmed.startsWith("#")) { + continue; + } + java.util.regex.Matcher onKey = java.util.regex.Pattern + .compile("^[\"']?on[\"']?:\\s*(.*)$").matcher(line); + if (onKey.matches()) { + String inline = onKey.group(1).replaceAll("\\s+#.*$", "").trim(); + if (inline.isEmpty()) { + inOnBlock = true; + blockIndent = -1; + } else { + for (String t : inline.replaceAll("^\\[", "").replaceAll("]$", "").split(",")) { + if (!t.trim().isEmpty()) { + triggers.add(t.trim()); + } + } + inOnBlock = false; + } + continue; + } + if (!line.startsWith(" ")) { // any other column-0 key closes the block + inOnBlock = false; + continue; + } + if (!inOnBlock) { + continue; + } + java.util.regex.Matcher key = java.util.regex.Pattern + .compile("^(\\s+)([A-Za-z_][A-Za-z0-9_-]*):.*$").matcher(line); + if (!key.matches()) { + continue; + } + int indent = key.group(1).length(); + if (blockIndent == -1) { + blockIndent = indent; + } + if (indent == blockIndent) { + triggers.add(key.group(2)); + } + } + return triggers; + } + @Test void quarantineLaneRunnerIncludesOnlyTheQuarantinedGroup() throws IOException { String lane = read(REPO_ROOT.resolve("bin/quarantined-test.sh")); @@ -236,8 +302,11 @@ void theHygieneSweepRunsTheAuditOnEveryPrAndTheLaneWorkflowRunsTheTests() throws assertWithMessage("the per-PR audit reaches the quarantine gates only through check-all.sh's sweep, " + "and only --with-tests --strict makes that sweep run the self-tests and refuse a CANNOT") .that(hygiene).contains("bin/check-all.sh --with-tests --strict"); - assertWithMessage("the sweep must run on pull_request or the audit is not per-PR at all") - .that(hygiene).contains("pull_request:"); + assertWithMessage("the sweep must run on pull_request or the audit is not per-PR at all. Read from " + + "the parsed `on:` block, not the file text: this workflow also carries `pull_request:` in a " + + "comment, so a grep would stay green with the trigger deleted. Declared triggers: " + + workflowTriggers(hygiene)) + .that(workflowTriggers(hygiene)).contains("pull_request"); String checkAll = read(REPO_ROOT.resolve("bin/check-all.sh")); assertWithMessage("check-all.sh must discover gates by glob - a hardcoded list is what the fold " + @@ -287,16 +356,24 @@ private static String exceptionList(String checkAllBody, String name) { /** * The lane workflow must DECLARE the triggers it exists for - a real bug this test guards: the * dispatch trigger was once missing while docs claimed "run it manually", making that impossible. + *

+ * Read through {@link #workflowTriggers(String)} rather than by substring for the reason that method + * records: this file discusses all three trigger names in its own prose, so a text match would be + * satisfiable by a comment. It happens to have no comment carrying the literal WITH its colon today, + * which makes the substring form correct by luck rather than by construction - the same shape that + * did go wrong one method up. */ @Test void laneWorkflowDeclaresItsTriggers() throws IOException { String lane = read(REPO_ROOT.resolve(".github/workflows/quarantine-lane.yml")); - assertWithMessage("lane runs on every PR push (pre-merge attribution)") - .that(lane).contains("pull_request:"); - assertWithMessage("lane must run after every merge to master (canonical master-state record)") - .that(lane).contains("push:"); - assertWithMessage("manual lane runs need a declared workflow_dispatch trigger") - .that(lane).contains("workflow_dispatch:"); + List triggers = workflowTriggers(lane); + assertWithMessage("lane runs on every PR push (pre-merge attribution). Declared: " + triggers) + .that(triggers).contains("pull_request"); + assertWithMessage("lane must run after every merge to master (canonical master-state record). " + + "Declared: " + triggers) + .that(triggers).contains("push"); + assertWithMessage("manual lane runs need a declared workflow_dispatch trigger. Declared: " + triggers) + .that(triggers).contains("workflow_dispatch"); } @Test From 8bd41a61481ea042c62981e6bbd53c9e5f5976ef Mon Sep 17 00:00:00 2001 From: Antony Stubbs Date: Mon, 7 Sep 2026 15:39:17 +1200 Subject: [PATCH 14/14] docs(solutions): the CI job-batching run, including the hypotheses that were refuted The experiment record lived in gitignored scratch, so every hypothesis, measurement and conclusion from this work would have died with the machine that ran it. The squash message carries what changed; this carries why, what was tried and rejected, and the traps. The refuted ones are the point, and the cache-warming job is the worked example. It looked like a free job and the only lever that also cut wall clock. It is the recorded fix for a diagnosed Maven Central timeout, and an open branch depends on a step added to it. The failure worth writing down is not the wrong guess but the reason it survived review: the prior-art sweep searched the words the problem was framed in, and the owning document is filed under build errors. It was one grep away on the mechanism word. Also recorded: the target was set against the wrong CodeQL job count, so it was never reachable; the reviewer-failure explanation was refuted by a same-size comparison; and two things a job-count harness cannot see by construction, one of which let two failures reach CI because the local sweep was not the sweep CI runs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RRMqsJQrGcF6ZH8xyX8AU6 --- ...jobs-under-a-concurrency-cap-2026-09-07.md | 141 ++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 docs/solutions/workflow-issues/batching-per-pr-jobs-under-a-concurrency-cap-2026-09-07.md diff --git a/docs/solutions/workflow-issues/batching-per-pr-jobs-under-a-concurrency-cap-2026-09-07.md b/docs/solutions/workflow-issues/batching-per-pr-jobs-under-a-concurrency-cap-2026-09-07.md new file mode 100644 index 000000000..3f246cbb0 --- /dev/null +++ b/docs/solutions/workflow-issues/batching-per-pr-jobs-under-a-concurrency-cap-2026-09-07.md @@ -0,0 +1,141 @@ +--- +title: Batching per-PR jobs under a concurrency cap - what worked, what was refuted, and the traps +date: 2026-09-07 +category: workflow-issues +module: ci +problem_type: workflow_issue +component: development_workflow +severity: medium +root_cause: resource_contention +resolution_type: workflow_improvement +applies_when: + - Sharding a CI suite and finding the wall-clock win smaller than expected + - Proposing to merge, split, rename or delete a GitHub Actions job + - Deciding whether a required status check may be added or removed + - Reading a green duplication or coverage report as evidence about a change +symptoms: + - Big sharded suites sit queued while one-minute jobs run + - A PR push launches more jobs than the account's concurrent-job ceiling + - Job queue delay dominates job duration for small checks +tags: + - github-actions + - concurrency-cap + - required-status-checks + - job-batching + - measurement + - refuted-hypotheses +--- + +# Batching per-PR jobs under a concurrency cap + +## The problem, measured before it was theorised + +Sharding the heavy suites made each one faster and the crowding worse. On the free plan a +GitHub account gets **20 concurrent jobs**, shared across every repository and every open PR. +A single push here launched more than that on its own, so a second PR pushing at the same time +put roughly triple the ceiling into 20 slots. + +Measured over eight pushes before any change, from `gh api .../actions/runs//jobs`: + +- The **small no-build jobs queued longer by median than the big ones**, because the big ones + wait on the cache job and enter the queue later. Queue delay dominated their runtime. +- The critical path was the unit suite, and it sat behind the cache job's own queue wait. + +Reproduce the current shape rather than trusting a number written here: + +```bash +gh api repos/astubbs/parallel-consumer/commits//check-runs --jq '.check_runs[].name' +``` + +## What the metric had to be, and why not wall clock + +The lever is the **count of jobs a `pull_request` event starts**, which is a property of the +workflow YAML. Queue delay is GitHub's scheduler responding to that count *plus every other +repository on the account*, so timing a CI run per experiment measures other people's load. +The harness therefore parsed the workflow files, expanded matrices, and excluded `if: false` +and close-only triggers - no CI run in the loop at all. One real run confirms at the end. + +**A static harness must model discovery-by-glob or it lies.** The first measurement reported +four checks dropped, because the fingerprint could not see gates that `bin/check-all.sh` +finds by globbing `bin/check-*.sh`. Any job invoking that script runs the whole set minus its +declared exception lists. + +## The folds that worked + +Deleting three jobs outright, then batching in four rounds, took the per-push job count down by +about a third. The pattern that made each one cheap: + +- **Name each step after the job it replaces.** The log then reads as the checks list used to, + and prose elsewhere saying "`dups: clones` found X on PR N" stays literally true - only the + *check* name changed. This shrinks the citation sweep to references that say "job" or + "required check". +- **Guard every folded step with `if: ${{ !cancelled() }}`.** A red first step then never hides + the ones behind it, and the job conclusion aggregates them. No verdict step is needed; the + platform already does that job. + +## The pairing test, learned by getting it wrong + +A fold predicted at two jobs delivered one. **Trigger types and permission scopes are part of a +job's identity.** Before pairing two jobs, diff their `on.pull_request.types` and their +`permissions`. Any of these vetoes the fold or forces the union onto the host: + +- a write scope one side needs and the other must not have beside PR-authored code; +- a trigger only one side needs (`closed`, `edited`); +- a job-level `if:` that cannot express "skip seven of these ten steps" - that becomes a + job-level env var each step reads, and the guard's behaviour must be carried over verbatim. + +**A guard written for one purpose silently starves another.** The JDK and cache steps sat behind +a CVE-credentials condition, and the mutation lane runs on fork and Dependabot PRs where that +condition is false - so it would have run with no JDK. + +## The trap that costs the most: names are an API + +A required status check is matched by **job name**, and the ruleset is repository settings that +no PR can change or test. + +- **A skipped required check pends forever.** Giving a batched job a `needs:` edge means a + transient cache failure *skips* it, which wedges the PR permanently. Name a status function + (`if: ${{ !cancelled() && ... }}`) so the job runs and reports instead. +- **Removals and additions have opposite orderings.** Remove an orphaned context in the same + sitting as the merge; add a new one only once a PR has produced it. Backwards, every PR in + the repository pends on a check nothing emits. +- **A rename silently breaks name-matching consumers**, even when step names are preserved. + `bin/check-pr-analysis-surfaces.sh` filters check runs through a name regex; one alternative + in it had already been dead since an earlier rename, dropping a surface from its listing with + nothing to say so. Grep `bin/` and `.claude/hooks/` for the check name on every fold. + +## Refuted, and worth more than the successes + +- **Removing the cache-warming job.** It looked like a free job and the only lever that also cut + wall clock. It is the recorded fix for + [the Azure west-US Maven Central timeout](../build-errors/maven-central-timeout-azure-west-regions-2026-04-21.md), + where each uncached artifact hangs for four minutes, and a later PR added a step to it that an + open branch depends on. **The process failure is the finding**: the hypothesis survived two + rounds of review because the prior-art sweep searched "workflow", "runner queue", "job count" + and "concurrency" - none of which reach a document filed under build errors and titled for a + Maven timeout. The owning document was one grep away, on the mechanism word `prepare-deps`. + A real-CI control arm would also have been the *wrong* instrument: the incident is + region-dependent, so a green run measures the region you drew. +- **The target itself was wrong.** It was set as the cap minus three CodeQL analyze jobs. + Default setup runs **four**. Fitting under the cap was never reachable at the stated target, + and is not a batching problem at all: both the Python and JavaScript analyses have real source + here, so closing the gap is a coverage decision or a plan decision. +- **"The reviewer failed because the prompt was too large."** Refuted by comparison: a PR whose + body and comments were the same size reviewed successfully minutes earlier. + +## Two things a job-count harness structurally cannot see + +- **Dependency-chain latency.** It tracks the longest job, not how long a job waits for its + `needs:` edge. A fold that adds an edge looks free and is not. +- **Whether a local sweep is the same sweep CI runs.** `bin/check-all.sh` defaults to gates only; + CI runs `--with-tests`. Two failures reached CI because the pre-push check ran the shorter one. + The self-tests are where a fold's environment changes surface - a job-level env var set for a + gate also reaches that gate's self-test, whose fixtures may depend on its absence. + +## Also established + +- The duplication engines cannot see repeated workflow YAML. One never reads workflows at all; + the other does and finds clones elsewhere, but not within a file whose repeated block is + broken up by differing comments. Recorded with the measurement in `docs/refactoring.md`. +- A coverage flag can report a large drop with no code change, comparing against a base snapshot + that disagrees with the default branch. Merging the base in made it vanish.