Skip to content

tests/parity/hf_snapshot.h has no guard against declaration-order breaks: the build catches them, but records-only sessions never run it #558

Description

@localai-bot

Split out from the now-closed #551 / #546. The header defect itself is already fixed on main by fafa16f0f (PR #556) — this issue is only the missing guard.

The recurring shape

af8170154 added Nemotron35LightningSnapshot() to tests/parity/hf_snapshot.h calling HfSnapshot eleven lines before its declaration. 14 TUs that include the header failed to compile. Because those suites are checkpoint-gated, ctest reported them as ***Not Run — which reads as missing checkpoints, not as a build break.

The commit that broke it was records-and-evidence work that never built C++ at all. A full build does catch this; the lane the breaking change ran in does not. That gap is what needs closing, not the ordering itself.

Proposed guard, already written and mutation-tested

A -fsyntax-only check on a TU including the header alone, as an additive method in the existing tests/scripts/test_check_snapshot_pins.py — a suite already wired to BOTH scripts/agent-preflight.sh (SUITES) and CI (.github/workflows/ci.yml:172, job agent-record), needing no CMake, build tree, GPU or checkpoint, costing ~0.25s.

Independent review mutation-tested it:

  • Restoring the broken ordering makes it the one failure of 20, quoting the compiler verbatim.
  • It generalizes: moving a different accessor (Qwen27NvfP4Snapshot) above HfSnapshot is also caught, naming that function.
  • Nothing else in the Python lane sees the defect — delete the guard and check-snapshot-pins.py exits 0 on a broken header. It is load-bearing.
  • It also catches a duplicate-definition hazard (see below), which no other check did.

Two known limitations to fix with it

  1. skipTest("no C++ compiler on PATH") exits 0, and agent-preflight.sh’s run() prints green ok while swallowing stdout — so the skip is invisible under preflight. Measured: env PATH=/nonexistent python3 …OK (skipped=1), exit 0. CI is sound today (ubuntu-latest ships g++, no continue-on-error) but nothing asserts it. Should fail rather than skip when CI is set.
  2. configured[:1] takes only the first token of $CXX, so a launcher-prefixed compiler (ccache g++, sccache clang++, distcc …) gives a spurious RED with a misleading message. Measured with CXX="env g++": AssertionError: 0 != 125 … /usr/bin/env: invalid option. Fail-closed, so not dangerous, but noisy.

Also worth knowing, not worth fixing: it hardcodes -std=c++20 rather than the CMake-configured standard (they agree today — CMakeLists.txt:35), carries no -Wall -Wextra -Werror, and does not prove include self-containment (deleting #include <string> still passes, since <filesystem> supplies it transitively). The docstring claims only "compiles on its own", which is what it tests.

A concrete near-miss this guard caught

Two independent repairs of the same defect — fafa16f0f on main (accessor moved before Qwen27NvfP4Snapshot) and a parallel row branch (moved after) — auto-merge with no conflict into a header defining Nemotron35LightningSnapshot() twice, re-breaking the same 14 TUs:

Auto-merging tests/parity/hf_snapshot.h
Automatic merge went well
$ grep -c "inline std::string Nemotron35LightningSnapshot" tests/parity/hf_snapshot.h
2
hf_snapshot.h:94:20: error: redefinition of "std::string parity::Nemotron35LightningSnapshot()"

Git cannot see that as a conflict — it is two independent moves of one block. The guard is what surfaces it.

Scope note

The guard closes this file. A header added tomorrow under tests/parity/ inherits nothing, so the honest generalization is a per-header self-containment sweep — related to #408 ("suites executed by nothing"). Recorded here rather than folded in silently.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions