ci: clang-format reports instead of failing (fixes #89); gate the three convention checkers

.clang-format

@@ -10,9 +10,11 @@
 #
 # NOTE: the tree predates this file, so a whole-tree `clang-format --dry-run`
 # is *not* clean today -- much of the code is hand-column-aligned in ways
-# clang-format cannot reproduce. CI therefore only checks the lines a PR
-# actually touches (see .github/workflows/lint.yaml); a one-shot reformat is
-# tracked separately so it never rides along with an unrelated change.
+# clang-format cannot reproduce. CI therefore only reports on the lines a PR
+# actually touches (the `lint (clang-format, changed lines)` job in
+# .github/workflows/ci.yml), and reports them for information only -- findings
+# never fail the build. A one-shot reformat is tracked separately so it never
+# rides along with an unrelated change.
 
 BasedOnStyle: LLVM
 Language: Cpp

.github/workflows/ci.yml

@@ -192,19 +192,81 @@ jobs:
  - name: Every `<<<` is inside the launch helper
  run: python3 tools/check-cuda-launches.py --check
 
+ # The three source conventions that had a checker but no gate.
+ #
+ # CLAUDE.md documents each of these as "enforced by tools/<x>.py --check",
+ # and until this job existed that was simply not true: nothing invoked any of
+ # them. All three passed on main, so the guarantee was fictional rather than
+ # broken -- which is the harder kind to notice, and the kind that quietly
+ # stops being true the first time somebody adds a header. Each convention was
+ # applied across 100+ files in one sweep (#145, #146, #91); re-doing that
+ # sweep because it rotted is much more expensive than running three scripts
+ # on every push.
+ #
+ # Unlike the clang-format job below, these DO fail the build, and the
+ # difference is deliberate. clang-format reports a style opinion about a tree
+ # that predates the style file. These encode decisions already taken and
+ # already applied everywhere, so a violation is a defect: an unprefixed macro
+ # in an installed header is a name taken from every downstream translation
+ # unit, and a stray `#ifndef` guard or a mis-delimited include is drift back
+ # towards the four-conventions-at-once state the consolidation removed.
+ #
+ # Kept off `apt` on purpose. All three are pure-stdlib Python that read files
+ # and exit, needing no toolchain at all, so this job installs nothing and
+ # uses the runner image's own interpreter -- the same reasoning as `test-hub`
+ # below, which survived the 2026-08-19 mirror stall precisely because it
+ # installs nothing while four jobs that do were killed at their
+ # timeout-minutes inside `apt-get`. A gate that runs in seconds should not
+ # have a package mirror on its critical path.
+ #
+ # Unconditional and not path-filtered, like the launch-site lint: these are
+ # properties of the whole tree, and the cost is three Python startups.
+ conventions:
+ name: lint (source conventions)
+ runs-on: ubuntu-latest
+ timeout-minutes: 10
+ steps:
+ - uses: actions/checkout@v5
+ # Each check is its own step so the red tick in the UI names the
+ # convention that broke, and each later one runs even after an earlier
+ # failure (`!cancelled()`) so a PR that trips two of them is told about
+ # both in one run rather than one per push. The job still fails if any
+ # step did.
+ - name: Public includes use <fastfields/...>, private ones quotes
+ run: python3 tools/normalise-include-delimiters.py --check
+ - name: Every header opens with `#pragma once`
+ if: ${{ !cancelled() }}
+ run: python3 tools/normalise-header-guards.py --check
+ - name: Every macro on the installed surface is FF_-prefixed
+ if: ${{ !cancelled() }}
+ run: python3 tools/rename-macros.py --check
+
+ # Purely informational, and that is a decision rather than a compromise:
+ # .clang-format was derived from existing code, but the tree predates it and
+ # is hand-column-aligned in many places, so a whole-tree check would flag
+ # essentially every file. Checking only the lines a PR touches makes new code
+ # conform from now on and leaves the one-shot reformat as its own reviewable
+ # change.
+ #
+ # So findings NEVER fail this job -- they are reported and the step exits 0.
+ # A gate that is red on essentially every PR is not a gate, it is training
+ # people to skim past the whole checks list, which costs more than unlinted
+ # whitespace does.
+ #
+ # Note there is no `continue-on-error: true` here any more. It was doing two
+ # things at once and only one of them was wanted: it stopped findings from
+ # blocking a merge (now handled properly, by not failing), and it also
+ # swallowed *infrastructure* failure -- a job whose apt install or checkout
+ # died would report the same not-quite-green as one that merely found
+ # unformatted lines. Splitting them is the point: findings are information
+ # and 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.
  clang-format:
  name: lint (clang-format, changed lines)
  # Needs a base ref to diff against; there is none on a push to main.
  if: github.event_name == 'pull_request'
  runs-on: ubuntu-latest
  timeout-minutes: 10
- # Reports findings for visibility without blocking merges: .clang-format
- # was derived from existing code, but the tree predates it and is
- # hand-column-aligned in many places, so a whole-tree check would flag
- # essentially every file. Checking only the lines a PR touches makes new
- # code conform from now on and leaves the one-shot reformat as its own
- # reviewable change.
- continue-on-error: true
  steps:
  - uses: actions/checkout@v5
  with:
@@ -216,27 +278,104 @@ jobs:
  # only -- clang-format-18 and git-clang-format-18 -- and NOT the
  # unversioned `git-clang-format`, so both are invoked by -18 name.
  run: sudo apt-get update && sudo apt-get install -y clang-format-18
- - name: Check the formatting of the lines this PR changes
+ - name: Report the formatting of the lines this PR changes
  env:
  BASE_REF: ${{ github.event.pull_request.base.ref }}
  run: |
  set -euo pipefail
  base=$(git merge-base "origin/${BASE_REF}" HEAD)
  echo "diffing against merge base ${base}"
+
+ # `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 everything below used to be
+ # unreachable in exactly the case it exists for -- every finding
+ # rendered as a bare exit code with no diff, no annotation and no
+ # hint (fastfields-lib#89). The clean path reached the report
+ # normally, which is why it went unnoticed for so long.
+ #
+ # So capture the status rather than dying on it. 0 = nothing to say,
+ # 1 = there is a diff, anything else = the tool itself failed (bad
+ # arguments, missing binary), which is worth telling apart from a
+ # finding instead of reporting an empty diff as "clean".
+ set +e
  out=$(git-clang-format-18 --binary clang-format-18 --diff \
  --extensions h,hpp,inl,cpp,cu,cuh "${base}")
+ status=$?
+ set -e
+
+ if [ "$status" -gt 1 ]; then
+ echo "$out"
+ echo "::warning::git-clang-format-18 exited ${status}; no formatting report was produced."
+ {
+ echo "### clang-format (informational)"
+ echo
+ echo "\`git-clang-format-18\` exited \`${status}\`, so no report could be produced."
+ } >> "$GITHUB_STEP_SUMMARY"
+ exit 0
+ fi
+
  case "$out" in
- "no modified files to format"|"clang-format did not modify any files")
- echo "clang-format: changed lines are clean"
- ;;
- *)
- echo "$out"
- echo "::error::clang-format would reformat the lines above."
- echo "Run: git clang-format $base"
- exit 1
+ ""|"no modified files to format"|"clang-format did not modify any files")
+ echo "clang-format: the lines this PR changes are clean"
+ echo "clang-format: the lines this PR changes are clean." \
+ >> "$GITHUB_STEP_SUMMARY"
+ exit 0
  ;;
  esac
 
+ # There is a diff. Print it in full in the log, then surface it
+ # where a reviewer sees it without opening the job at all: an
+ # annotation on the run, and the step summary on the PR's checks
+ # tab. Being informational is only useful if the information is
+ # actually reachable.
+ echo "$out"
+
+ files=$(printf '%s\n' "$out" \
+ | sed -n 's|^diff --git a/\(.*\) b/.*|\1|p' | sort -u)
+ nfiles=$(printf '%s\n' "$files" | sed '/^$/d' | wc -l | tr -d ' ')
+
+ echo "::notice::clang-format would reformat lines this PR touches in ${nfiles} file(s). Informational only -- this job does not fail. To apply: git clang-format ${base}"
+
+ # GITHUB_STEP_SUMMARY is capped at 1 MiB and is dropped wholesale if
+ # exceeded, so cap the embedded diff; the job log always has it in
+ # full.
+ max=400
+ total=$(printf '%s\n' "$out" | wc -l | tr -d ' ')
+ shown=$(printf '%s\n' "$out" | head -n "$max")
+
+ {
+ echo "### clang-format (informational)"
+ echo
+ echo "clang-format would reformat lines this PR touches in **${nfiles} file(s)**."
+ echo "This does **not** fail CI: the tree predates \`.clang-format\` and is"
+ echo "hand-column-aligned in many places, so only the lines a PR changes are"
+ echo "reported, and only as information."
+ echo
+ echo "Apply just these hunks with:"
+ echo
+ echo '```sh'
+ echo "git clang-format ${base}"
+ echo '```'
+ echo
+ printf '%s\n' "$files" | sed '/^$/d' | sed 's/^/- `/; s/$/`/'
+ echo
+ echo "<details><summary>Proposed diff</summary>"
+ echo
+ echo '```diff'
+ printf '%s\n' "$shown"
+ if [ "$total" -gt "$max" ]; then
+ echo "... truncated at ${max} of ${total} lines; the full diff is in the job log."
+ fi
+ echo '```'
+ echo
+ echo "</details>"
+ } >> "$GITHUB_STEP_SUMMARY"
+
+ # Informational by design: findings never fail this job.
+ exit 0
+
  # ==========================================================================
  # THE gate: the CPU correctness suite.
  # ==========================================================================

CLAUDE.md

@@ -110,11 +110,21 @@ the CPU path is the tested source of truth and CUDA is **compile+link only**.
 
 ## CI
 
-`.github/workflows/ci.yml`, path-filtered. `codespell` and `lint (cuda launch
-sites)` always; `test-cpu` (a 3-leg `BOUNDFLAGS`/`SPLINEFLAGS` matrix + an
-`INDEXFLAGS` leg + a g++ leg), `sanitize` (ASan+UBSan) and `tsan` on
-kernels/cpu/hub changes; `test-hub` on hub changes; `build-cuda` (two legs, one
-per `FF_INDEX32` position) and `compile-probe-cuda` on kernels/cuda changes.
+`.github/workflows/ci.yml`, path-filtered. `codespell`, `lint (cuda launch
+sites)` and `lint (source conventions)` always; `test-cpu` (a 3-leg
+`BOUNDFLAGS`/`SPLINEFLAGS` matrix + an `INDEXFLAGS` leg + a g++ leg),
+`sanitize` (ASan+UBSan) and `tsan` on kernels/cpu/hub changes; `test-hub` on
+hub changes; `build-cuda` (two legs, one per `FF_INDEX32` position) and
+`compile-probe-cuda` on kernels/cuda changes.
+
+**`lint (clang-format, changed lines)` is informational and cannot fail on
+findings.** It reports what clang-format would change in the lines a PR
+touches, as a `::notice::` plus a step-summary diff, and exits 0 either way —
+the tree predates `.clang-format`, so a blocking check would be red on
+essentially every PR. The three checks in `lint (source conventions)` are the
+opposite and *do* fail: they enforce conventions already applied tree-wide
+(`normalise-include-delimiters`, `normalise-header-guards`, `rename-macros`),
+so a violation there is a defect rather than a style opinion.
 
 **The `tsan` leg is the only one that runs anything in parallel.** With the
 shipping `GRAIN_SIZE` (32768) every workload in `tests/lib-cpu/` is below the
@@ -226,6 +236,8 @@ pushpull's fully-static order×bound compile is nightly
  library, so those *are* `<cstdint>` there) and the non-nvcc `__device__` /
  `__host__` fallbacks. Prefer an `inline` function to a macro where one will
  do — a function in `ff::` is collision-safe without any prefix.
+ `tools/rename-macros.py --check` enforces this, and runs in CI on every push,
+ in the `lint (source conventions)` job.
 - **`<fastfields/…>` for the public interface, `"…"` for private headers.**
  `include/fastfields/` is what gets installed and what `fastfields-dlpack`
  puts on its include path, so it is spelled with angle brackets like any other
@@ -235,7 +247,8 @@ pushpull's fully-static order×bound compile is nightly
  configuration and there is no `-iquote` anywhere — so this is about saying
  which category a dependency is in, not about lookup.
  `tools/normalise-include-delimiters.py --check` enforces it, and also checks
- the converse: every quoted include must resolve beside its includer.
+ the converse: every quoted include must resolve beside its includer. It runs
+ in CI on every push, in the `lint (source conventions)` job.
 - **`#pragma once` on line 1 of every header — no `#ifndef` include guards.**
  Line 1 with no exception, licence and provenance comments included; they keep
  their text and sit one line lower. **`include/fastfields/core/dlpack.h` is
@@ -254,7 +267,8 @@ pushpull's fully-static order×bound compile is nightly
  the `FF_*_MAX_NBATCH` / `FF_AUTOCAST_PINNED_HOST` build knobs all stay.
  Enforced by `tools/normalise-header-guards.py --check`, which also applies
  the convention and audits that no guard macro is tested from another file —
- the one way deleting a `#define` could change what compiles.
+ the one way deleting a `#define` could change what compiles. It runs in CI on
+ every push, in the `lint (source conventions)` job.
 - `include/fastfields/core/dlpack.h` is vendored upstream code: do not edit it,
  and it is skipped by `codespell` (see `.codespellrc`). It is the only
  verbatim third-party file in the tree.