Uh oh!
There was an error while loading. Please reload this page.
ci: clang-format reports instead of failing (fixes #89); gate the three convention checkers - #156
Conversation
Two problems, one job. The reporting bug (#89). The step runs under `set -euo pipefail` and captures the diff with `out=$(git-clang-format-18 ... --diff "$base")`. But `git-clang-format --diff` exits 1 BY CONTRACT whenever it would reformat something -- that is how it says "here is a diff", not an error. Under `set -e` a failing command substitution in an assignment aborts the shell, so the entire `case` that prints `$out` was unreachable in exactly the situation it exists for. Every finding rendered as a bare exit code: no diff, no annotation, no "Run: git clang-format ..." hint. The clean path reached the report normally, which is why this survived so long. The status is now captured instead of being fatal, and an exit above 1 (a real tool failure) is told apart from a finding rather than being reported as an empty "clean". The policy. The job was `continue-on-error: true`, so it did not block -- but it still put a red mark on essentially every PR, which trains people to skim past the whole checks list. Findings are now reported and the step exits 0, always. When there is a diff it prints it in full in the log, emits a `::notice::` annotation, and writes a step-summary block with the file list and the diff (capped at 400 lines, since GITHUB_STEP_SUMMARY is dropped wholesale past 1 MiB). Being informational is only useful if the information is reachable. `continue-on-error` is dropped, because it was doing two jobs and only one was wanted. It stopped findings from blocking a merge -- now handled properly, by not failing -- and it also swallowed infrastructure failure, so a job whose apt install or checkout died looked the same as one that merely found unformatted lines. Splitting them is the point: findings are always green, while a lint that cannot run at all is a real defect and is allowed to go red, so it cannot rot unnoticed the way the reporting path just did. Verified against a deliberately mis-formatted file, which is precisely what nobody did before: the findings path prints the diff, annotates and exits 0; the clean path and the tool-failure path both exit 0 too. Also corrects .clang-format's pointer to `.github/workflows/lint.yaml`, which has not existed since the consolidation. Closes#89
CLAUDE.md documents each of `tools/normalise-include-delimiters.py`, `tools/normalise-header-guards.py` and `tools/rename-macros.py` as enforcing its convention via `--check`. No workflow invoked any of them, so that guarantee was fictional. All three pass on `main` today, which makes this latent rather than broken -- and latent is the harder kind to notice, because nothing goes red until long after the convention has already rotted. Each of these conventions was applied across 100+ files in a single sweep (#145, #146, #91). Re-doing such a sweep because it drifted back costs far more than running three scripts on every push, and the drift they guard against -- an unprefixed macro in an installed header taking a name from every downstream translation unit, a stray `#ifndef` guard, a mis-delimited include -- is invisible to every compile and test gate in this repo. A new `lint (source conventions)` job rather than extra steps on `lint (cuda launch sites)`. They are the same shape (fast, stdlib-Python, whole-tree textual invariants), but #154 built that job's name and its comment around one specific rule, and a red tick reading "cuda launch sites" for a header-guard violation would misdirect. Keeping them separate also avoids renaming a check that landed two commits ago. Each checker is its own step, so the failing tick names the convention, and the later steps carry `if: !cancelled()` so a change tripping two of them hears about both in one run instead of one per push. These DO fail the build, unlike the clang-format job. That one reports a style opinion about a tree predating the style file; these encode decisions already taken and already applied everywhere, so a violation is a defect. The job installs nothing -- no apt, not even setup-python -- and uses the runner image's interpreter. All three are pure stdlib and read files. Four jobs were killed at their timeout-minutes inside `apt-get` during the 2026-08-19 mirror stall while `test-hub` rode it out precisely because it installs nothing; a gate that runs in seconds should not have a package mirror on its critical path. Verified each checker still fires by injecting a violation of each into `include/fastfields/api/distance.h` in turn -- a quoted public include, a whole-file `#ifndef` guard, and an unprefixed `#define` -- and confirming exit 1 with a message naming the file and line, the same way #154 proved its lint fires with a rogue launch.
balbasty
commented
Aug 20, 2026
Both behaviours verified against real Actions, not just locallyI put up a throwaway PR (#157, now closed, branch deleted) based on this branch rather than
Green while reporting findings is the whole point, and is exactly the case that was red-and-silent under #89.
Two things that log also settles:
Locally I also exercised the two paths a probe PR cannot easily produce, running the step body extracted verbatim from the committed YAML: a tool failure (binary exiting 3 → The gate is untouchedThis PR touches
13 suites, 59,886 checks, 0 failures — the expected number exactly. All five Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Two CI-hygiene changes, one commit each. Neither touches any C++ source, so the
59,886 / 13 / 0 gate cannot move — confirmed anyway, see Validation.
1.
lint (clang-format, changed lines)is informational — and can finally reportPer the owner on #145: "clang-format should be run for information alone, not
make the CI fail (we know we are not linted)."
It was red and silent, which is the worst combination. Two separate
problems, fixed together because the second is why the first mattered:
The reporting bug (#89). The step runs under
set -euo pipefailand doesout=$(git-clang-format-18 ... --diff "$base"). Butgit-clang-format --diffexits 1 by contract whenever it would reformat — that is how it says "here
is a diff", not an error. Under
set -ea failing command substitution in anassignment aborts the shell, so the whole
casethat prints$outwasunreachable in exactly the situation it exists for. Every finding rendered as
a bare exit code: no diff, no annotation, no
Run: git clang-format …hint.The clean path reached the report normally, which is why it survived. The exit
status is now captured rather than fatal.
The policy.
continue-on-error: truemeant it did not block, but it stillput a red ✗ on essentially every PR, which trains everyone to skim past the
whole checks list. Findings now always exit 0. When there is a diff the job
prints it in full in the log, emits a
::notice::annotation, and writes a$GITHUB_STEP_SUMMARYblock with the file list and the diff.Three details worth flagging for review:
continue-on-erroris dropped, deliberately. It was doing two jobs andonly one was wanted: it stopped findings from blocking a merge (now handled
properly, by not failing) and it swallowed infrastructure failure, so a
job whose apt install or checkout died looked the same as one that merely
found unformatted lines. Splitting them is the point — findings are always
green, while a lint that cannot run at all is a real defect and is allowed to
go red, so it cannot rot unnoticed the way the reporting path just did.
binary) and is now told apart from a finding, with a
::warning::, instead ofbeing reported as an empty "clean". Still exits 0.
GITHUB_STEP_SUMMARYis droppedwholesale past 1 MiB, and a truncated summary that renders beats an oversized
one that vanishes. The job log always has it in full.
Also corrects
.clang-format's pointer to.github/workflows/lint.yaml, gonesince the consolidation.
2. The three convention checkers now actually run
CLAUDE.mddocumentstools/normalise-include-delimiters.py,tools/normalise-header-guards.pyandtools/rename-macros.pyas enforcingtheir conventions via
--check. No workflow invoked any of them. All threepass on
maintoday, so this was latent rather than broken — the harder kindto notice, because nothing goes red until long after the convention has rotted.
Each was applied across 100+ files in one sweep (#145, #146, #91), and the
drift they guard against (an unprefixed macro in an installed header taking a
name from every downstream TU, a stray
#ifndef, a mis-delimited include) isinvisible to every compile and test gate here.
These genuinely fail the build, unlike clang-format. That one reports a
style opinion about a tree predating the style file; these encode decisions
already taken and already applied everywhere, so a violation is a defect.
A new
lint (source conventions)job rather than extra steps onlint (cuda launch sites)— the one judgement call here, happy to be overruled. They arethe same shape (fast, stdlib-Python, whole-tree textual invariants), but #154
built that job's name and its comment block around one specific rule, and a red
tick reading "cuda launch sites" for a header-guard violation would misdirect.
It also avoids renaming a check that landed two commits ago. Each checker is
its own step so the failing tick names the convention, and the later steps carry
if: !cancelled()so a change tripping two of them hears about both in one runrather than one per push.
The job installs nothing — no
apt, not evensetup-python; all three arepure stdlib and just read files, so it uses the runner image's interpreter.
Four jobs were killed at their
timeout-minutesinsideapt-getduring the2026-08-19 mirror stall while
test-hubrode it out precisely because itinstalls nothing.
Validation
The clang-format reporting path, against a deliberately mis-formatted file —
which is precisely what nobody did before, and why #89 existed. Run with the
step body extracted verbatim from the committed YAML:
::notice::+ summary::warning::Each convention checker still fires, the same way #154 proved its lint does
with a rogue launch. A violation of each was injected into
include/fastfields/api/distance.hin turn:#include "fastfields/core/dlpack.h"normalise-include-delimiters#ifndef FF_API_DISTANCE_Hnormalise-header-guards#define SNEAKY_UNPREFIXED_MACRO 1rename-macrosAll three pass on this branch,
codespellis clean, the workflow parses, andevery
run:step in the file is valid bash.Gate:
tools/test-baseline.sh --legs default,lib --check tools/test-baseline.expected— unchanged. The diff contains zero C++source files (
.github/workflows/ci.yml,CLAUDE.md,.clang-formatonly), soit is structurally impossible for it to move.
Closes#89.
Generated by Claude Code