feat(reference)!: publish deterministic C++ oracle fixtures#130
Conversation
- Add versioned schemas, canonical raw outputs, provenance manifests, and quantity-specific comparison rules for Rust validation. - Cover causal topology, every 2+1D move, nonzero Regge actions, persistence, bounded execution, and matched thread scaling. - Add clean-commit regeneration and offline validation workflows with a monotone, overflow-safe spherical population contract. - Correct stale LCOV inconsistency counts exposed by the updated utilities. BREAKING CHANGE: The spherical population estimator now uses a monotone, margin-adjusted schedule, changing generated point counts. The s3_bulk_action_alpha_minus_one API is now named s3_bulk_action_alpha_minus_one_imaginary_coefficient. Closes #94
WalkthroughThis PR adds versioned CDT++ reference fixtures, schemas, deterministic producers, raw outputs, manifests, offline validation, regeneration workflows, population-sizing and action API updates, build/CI changes, documentation, and formatted console-output policy checks. ChangesReference fixture package
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Developer
participant Justfile
participant Generator
participant FixtureBinary
participant Validator
participant ReferencePackage
Developer->>Justfile: run reference-regenerate
Justfile->>Generator: verify clean revision and invoke producers
Generator->>FixtureBinary: generate canonical JSON and raw outputs
FixtureBinary-->>Generator: return deterministic fixture data
Generator->>ReferencePackage: write artifacts and manifests
Justfile->>Validator: run reference-check
Validator->>ReferencePackage: validate schemas, topology, transitions, provenance, and hashes
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/Utilities.hpp`:
- Around line 1906-1916: Fix the clang-format alignment in struct
Generated_population_bounds by reformatting the member declarations and their
spacing to match the repository’s configured style, particularly
points_per_timeslice, input_vertices, and tetrahedra_upper_bound; do not change
the members or their types.
In `@pyproject.toml`:
- Line 51: Update the [tool.uv] no-build-package configuration to include
jsonschema alongside the existing development dependency pins, matching the
pinned "jsonschema==4.25.1" dependency and ensuring uv does not attempt a source
build.
In `@reference/manifests/v1/macos-arm64.json`:
- Around line 155-165: Update the manifest regeneration flow that produces
reference/manifests/v1/macos-arm64.json to refresh provenance fields from the
current verification run rather than copying stale template values. Ensure
verification.status and unchanged toolchain entries such as cmake and mpfr are
regenerated or explicitly replaced before publishing, while preserving the
manifest’s existing structure.
In `@scripts/generate_reference_fixtures.py`:
- Around line 286-304: Update the regeneration logic in the manifest-building
function and make_scaling_manifest so stale verification evidence is not
inherited from the committed template: remove the verification block and
toolchain keys such as toolchain.cmake and toolchain.mpfr that producers do not
report before writing refreshed fields. Ensure regenerated manifests require
verification to be deliberately re-recorded rather than retaining passed status,
base_revision, or base_run from the prior manifest.
- Around line 423-433: In regenerate, validate the argv[0] paths in
reference_commands and scaling_commands before calling make_reference_manifest,
make_scaling_manifest, validate_generated_json, or overwriting generated
artifacts. Reject any fixture, CDT, initialize, or benchmark binary path that is
not the default canonical producer path, and report the offending option/path
with an actionable error.
In `@scripts/tests/test_generate_reference_fixtures.py`:
- Around line 25-41: Add a Darwin/arm64 passing-path test for
validate_platform_identity that asserts no exception, alongside
test_platform_identity_must_match_manifest_names, to catch inverted guards. Also
add a record_commands test covering the length-mismatch branch in the generator,
asserting the expected behavior without introducing unrelated coverage.
In `@scripts/tests/test_validate_reference_fixtures.py`:
- Around line 103-119: Add a test asserting fnv1a64(b"") equals the published
vector cbf29ce484222325, anchoring the implementation’s offset basis and prime.
Also add a validate_scaling_records test covering its remaining validation path,
reusing the existing fixture and assertion conventions in the test module.
In `@scripts/validate_reference_fixtures.py`:
- Around line 82-176: Split validate_state at its existing validation boundaries
by extracting validate_edges, validate_facets, validate_cells, and
validate_incidence_closure helpers. Pass the existing state-specific data needed
by each helper, preserve all current checks and error messages, and keep
validate_state responsible for orchestration and classification/adjacency.
Remove the C901, PLR0912, and PLR0915 suppressions from validate_state without
changing behavior.
In `@tests/Metropolis_test.cpp`:
- Around line 530-545: Add a defensive assertion in the “A predeclared Pearson
statistic does not reject uniformity.” test that verifies sites.size() is
exactly 5 before applying critical_value, and keep the existing chi-square
calculation and threshold unchanged.
In `@tests/Reference_fixture.cpp`:
- Around line 380-407: Update the JSON construction in the fixture around the
constants and computed values so counts and parameters are streamed from n1,
n31, n22, alpha, k, and lambda instead of hard-coded literals. Preserve the
existing record IDs and value serialization, and ensure the alpha-minus-one and
alpha-one records emit their respective alpha values while sharing the declared
constants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1ae6f82c-d1ee-46cb-98e4-27c3285831e0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.gitignoreJustfileREADME.mdREFERENCES.mddocs/Doxyfiledocs/cgal-integration.mddocs/metropolis-hastings.mddocs/reproducibility.mdinclude/S3Action.hppinclude/Utilities.hpppyproject.tomlreference/README.mdreference/fixtures/v1/protocol.jsonreference/manifests/v1/macos-arm64.jsonreference/manifests/v1/scaling-macos-arm64.jsonreference/raw/v1/cpp-reference.jsonreference/raw/v1/end-to-end.txtreference/raw/v1/persistence-v1.offreference/raw/v1/persistence-v1.off.metareference/raw/v1/scaling-threads-1.txtreference/raw/v1/scaling-threads-2.txtreference/raw/v1/scaling-threads-4.txtreference/schema/fixture-v1.schema.jsonreference/schema/result-v1.schema.jsonreference/schema/run-manifest-v1.schema.jsonscripts/coverage.shscripts/generate_reference_fixtures.pyscripts/tests/test_generate_reference_fixtures.pyscripts/tests/test_validate_reference_fixtures.pyscripts/validate_reference_fixtures.pysrc/CMakeLists.txttests/CMakeLists.txttests/Metropolis_test.cpptests/Reference_fixture.cpptests/Runtime_config_test.cpptests/S3Action_test.cpptests/Utilities_test.cpp
- Rebuild manifests from canonical producers and current toolchain evidence. - Reject noncanonical binaries, malformed records, and stale verification state. - Standardize console and fixture output on fmt::print and enforce it with Semgrep. Closes #94
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/generate_reference_fixtures.py (1)
496-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate producer paths before spending the producer runs.
validate_producer_pathsonly needsdisplay_pathof the four binaries, but it runs afterproduce_raw_artifactshas already executed the fixture, bounded, persistence, and three benchmark producers (up to a 900 s timeout each). A noncanonical--benchmark-binarytherefore costs a full regeneration cycle before the error surfaces. Nothing is published yet, so this is purely about feedback latency.♻️ Proposed reordering in `regenerate`
fixture_command, bounded_command, persistence_command, benchmark_commands = producer_commands(*paths) + reference_commands = [ + [display_path(fixture_binary)], + [display_path(cdt_binary), *bounded_command[1:]], + [display_path(initialize_binary), *persistence_command[1:]], + ] + scaling_commands = [[display_path(benchmark_binary), *command[1:]] for command in benchmark_commands] + validate_producer_paths(reference_commands, scaling_commands) generated, fixture_json, scaling_records = produce_raw_artifacts(with the later duplicate construction at lines 506-512 removed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate_reference_fixtures.py` around lines 496 - 513, Move construction of reference_commands and scaling_commands, followed by validate_producer_paths, before the produce_raw_artifacts call in regenerate, using the existing binary and command variables. Remove the later duplicate command construction and validation while preserving the subsequent artifact generation and metadata flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/tests/test_generate_reference_fixtures.py`:
- Around line 80-103: Add a test for the divergent-version branch in
configured_cmake_version, using the existing cache_paths setup and mocking
run_command with a side_effect that returns two different CMake version strings.
Assert the function’s guard behavior for mismatched cache versions and verify
both cache commands are invoked with the expected timeout.
In `@tests/Reference_fixture.cpp`:
- Around line 41-42: Split output_precision into separate coordinate_precision
and value precision constants, keeping long double::max_digits10 for action
values and defining coordinate_precision for double coordinates. Update the
position formatting in the relevant output path to use coordinate_precision
while preserving output_precision for action values.
In `@tests/semgrep/formatted_output.cpp`:
- Around line 4-8: The formatted_output.cpp Semgrep fixture emits unintended
diagnostic-output policy findings alongside its intended assertions. Mark the
relevant std::cerr and std::clog diagnostic-output findings as expected in the
fixture, or exclude this non-doctest fixture from those runtime policy rules
while preserving the use-fmt-print-for-console-output assertions.
---
Outside diff comments:
In `@scripts/generate_reference_fixtures.py`:
- Around line 496-513: Move construction of reference_commands and
scaling_commands, followed by validate_producer_paths, before the
produce_raw_artifacts call in regenerate, using the existing binary and command
variables. Remove the later duplicate command construction and validation while
preserving the subsequent artifact generation and metadata flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76f18132-5c1c-4d02-b036-4b9a6f809133
📒 Files selected for processing (16)
include/Periodic_3_triangulations.hppinclude/Utilities.hpppyproject.tomlscripts/generate_reference_fixtures.pyscripts/tests/test_generate_reference_fixtures.pyscripts/tests/test_validate_reference_fixtures.pyscripts/validate_reference_fixtures.pysemgrep.yamlsrc/cdt-viewer.cppsrc/cdt.cppsrc/initialize.cpptests/CGAL_benchmark.cpptests/Metropolis_test.cpptests/Random_benchmark.cpptests/Reference_fixture.cpptests/semgrep/formatted_output.cpp
- Load Linux tests with the configured compiler's libstdc++. - Reduce sanitizer smoke work to avoid CI timeouts. - Validate canonical reference producers before execution. - Serialize fixture coordinates at double precision.
- Match producer-path assertions to repr-formatted Windows errors. - Discover Semgrep targets from one repository root to avoid RPC stalls.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test_generate_reference_fixtures.py (1)
89-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
pytest.raisesfor this exception assertion.Ruff PT027 flags
unittest.TestCase.assertRaises; this can failpython-checkeven though the assertions are behaviorally correct. Retain the message checks while switching topytest.raises(or verify that PT027 is intentionally disabled).Suggested change
- self.assertRaises(ValueError) as raised, + pytest.raises(ValueError) as raised,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_generate_reference_fixtures.py` around lines 89 - 100, Replace the unittest-style assertRaises context around generator.regenerate with pytest.raises(ValueError), preserving the existing exception message checks against raised.value or the captured exception. Update the test imports as needed, and do not disable PT027.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Justfile`:
- Around line 195-198: Bound the repository-root Semgrep invocation in the
Justfile recipe by adding explicit exclusions for ignored build, virtualenv,
coverage, and generated directories alongside tests/semgrep. Keep scanning the
repository root with --no-git-ignore while ensuring these paths cannot
contribute slow scans or false positives.
---
Outside diff comments:
In `@scripts/tests/test_generate_reference_fixtures.py`:
- Around line 89-100: Replace the unittest-style assertRaises context around
generator.regenerate with pytest.raises(ValueError), preserving the existing
exception message checks against raised.value or the captured exception. Update
the test imports as needed, and do not disable PT027.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: df30cf60-376f-44c3-8308-21eee188b6c4
📒 Files selected for processing (3)
Justfilescripts/tests/test_generate_reference_fixtures.pysemgrep.yaml
- Make Release the canonical optimized configuration and add a focused Debug CLI workflow. - Add pinned ccache support for local pkgx builds and Ubuntu CI jobs. - Enforce pinned, approved GitHub Actions with readable version annotations. - Bound repository Semgrep scans to relevant source directories.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Justfile (1)
45-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the full CTest gate to
just ci.Replacing
buildwithreference-generated-checkremoves the full build/CTest execution; that recipe only rebuilds the fixture producer and validates generated output. This contradicts the documented canonical CI contract.
Justfile#L45-L50: run the existing full build/CTest workflow in addition toreference-generated-check..github/CONTRIBUTING.md#L51-L65: retain this claim only after restoring the test gate; otherwise document the narrower command behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Justfile` around lines 45 - 50, The just ci recipe must restore the existing full build/CTest workflow while also running reference-generated-check; update Justfile lines 45-50 to invoke that build/test recipe in addition to the generated-reference validation. In .github/CONTRIBUTING.md lines 51-65, retain the documented canonical CI claim because it becomes accurate after the Justfile change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 175-176: Update the README description of the just build recipe to
refer to the platform-specific build script, covering scripts/build.sh on Unix
and scripts/build.bat on Windows instead of implying that only scripts/build.sh
is used.
In `@scripts/build.bat`:
- Line 11: Normalize PRESET to its canonical lowercase name before the cmake
--preset invocation, ensuring Debug, Reference, and Parallel inputs become
debug, reference, and parallel respectively. Update the validation and
preset-selection flow around the PRESET checks, including the debug branch shown
by the PRESET_READY jump, while preserving rejection of unsupported values.
In `@semgrep.yaml`:
- Line 102: Update the pattern-regex rule to recognize SHA-pinned uses entries
lacking a valid release/version comment, including trailing whitespace or
non-version comments such as “pinned”; preserve exclusions for the repository’s
accepted version-comment format. Add fixtures covering invalid comments and a
valid comment, and verify the rule matches only the former.
- Line 62: Update the pattern-regex rule to match docker:// action references
instead of excluding them, and enforce the same immutable-reference validation
by rejecting mutable tags or requiring a digest. Preserve the existing handling
for local actions and non-docker external references.
---
Outside diff comments:
In `@Justfile`:
- Around line 45-50: The just ci recipe must restore the existing full
build/CTest workflow while also running reference-generated-check; update
Justfile lines 45-50 to invoke that build/test recipe in addition to the
generated-reference validation. In .github/CONTRIBUTING.md lines 51-65, retain
the documented canonical CI claim because it becomes accurate after the Justfile
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2bcd853a-41f1-4b9a-9dba-339a88b8e48e
📒 Files selected for processing (20)
.github/CONTRIBUTING.md.github/workflows/ci.ymlCMakeLists.txtCMakePresets.jsonJustfileREADME.mdcmake/Cache.cmakecmake/StandardProjectSettings.cmakescripts/build.batscripts/build.shscripts/codeql-build.shscripts/coverage.shscripts/iwyu.shscripts/pkgx-build.shscripts/pkgx-env.shscripts/slurm.shsemgrep.yamlsrc/CMakeLists.txttests/semgrep/.github/actions/action_policy.ymltests/semgrep/.github/workflows/action_policy.yml
- Isolate ccache from compiler-toolchain dependency resolution. - Normalize Windows preset names before invoking CMake. - Require immutable container digests and readable action version comments. - Document the platform-specific build launchers.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@semgrep.yaml`:
- Around line 64-65: Update the external-action regex rules in semgrep.yaml at
the affected pinning, allowlist, and version-comment patterns to allow zero or
more additional path segments by changing the optional single-segment group to a
repeating group. Add a fixture covering a nested action path and verify all
three rules handle it correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea85c256-e61c-4449-a8d8-431ce331f147
📒 Files selected for processing (20)
.github/CONTRIBUTING.md.github/workflows/ci.ymlCMakeLists.txtCMakePresets.jsonJustfileREADME.mdcmake/Cache.cmakecmake/StandardProjectSettings.cmakescripts/build.batscripts/build.shscripts/codeql-build.shscripts/coverage.shscripts/iwyu.shscripts/pkgx-build.shscripts/pkgx-env.shscripts/slurm.shsemgrep.yamlsrc/CMakeLists.txttests/semgrep/.github/actions/action_policy.ymltests/semgrep/.github/workflows/action_policy.yml
Apply SHA pinning, allowlist, and version-comment checks to GitHub Action references with arbitrarily nested path segments.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@semgrep.yaml`:
- Around line 64-65: Update the `pattern-regex` rules on lines 64–65, 85, and
105 to match both standalone `uses:` keys and shorthand list entries prefixed
with `- uses:` while preserving their existing validation behavior. Add a
fixture covering a shorthand step such as `- uses: actions/checkout@v7` and
verify pinning, allowlisting, and version-comment enforcement apply to it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1035e04-b872-47b0-8c7d-59bdc58f3c48
📒 Files selected for processing (2)
semgrep.yamltests/semgrep/.github/workflows/action_policy.yml
Apply SHA pinning, allowlist, and version-comment checks to GitHub Action shorthand list entries, including container actions.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
semgrep.yaml (1)
428-430: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMatch chained stream insertions.
std::cout << $VALUE;only catches a single insertion, so chained output likestd::cout << "value: " << value << '\n';is missed. Use a chain-aware matcher, e.g.std::cout <<... << $VALUE;, and keeptests/semgrep/formatted_output.cppas regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@semgrep.yaml` around lines 428 - 430, Update the stream patterns in the semgrep rules to match chained insertions for std::cout, std::cerr, and std::clog, rather than only a single << operation. Preserve coverage for standalone insertions and add or update regression cases in formatted_output.cpp for chained output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@semgrep.yaml`:
- Around line 428-430: Update the stream patterns in the semgrep rules to match
chained insertions for std::cout, std::cerr, and std::clog, rather than only a
single << operation. Preserve coverage for standalone insertions and add or
update regression cases in formatted_output.cpp for chained output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 120f821d-ac38-499b-8d24-72f40db0e912
📒 Files selected for processing (2)
semgrep.yamltests/semgrep/.github/workflows/action_policy.yml
BREAKING CHANGE: The spherical population estimator now uses a monotone, margin-adjusted schedule, changing generated point counts. The s3_bulk_action_alpha_minus_one API is now named
s3_bulk_action_alpha_minus_one_imaginary_coefficient.
Closes #94
Summary by CodeRabbit