ci: make public baseline inputs declarative - #7958
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe public baseline now uses a declarative measurement configuration for toolchains, thresholds, workloads, warmups, and run counts. Benchmark drivers consume these settings, while fingerprint and artifact validation enforce the configured protocol. ChangesPublic baseline measurement protocol
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant run_public_baseline_sh
participant benchmark_drivers
participant public_baseline_assemble
run_public_baseline_sh->>benchmark_drivers: pass configured runs, warmups, workloads, and toolchains
benchmark_drivers->>public_baseline_assemble: provide recorded measurement metadata
public_baseline_assemble->>public_baseline_assemble: validate metadata against measurement configuration
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
proggeramlug
marked this pull request as ready for review
August 12, 2026 12:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@benchmarks/public_baseline.py`:
- Around line 238-279: Update the configuration validation flow around _load to
first require the JSON root to be a mapping before calling config.get. Use exact
integer validation that excludes booleans for schema_version,
consecutive_seconds, measured_runs, and warmup_runs, and exclude booleans from
the maximum_cpu_active_percent numeric check. Add tests covering malformed
non-object roots and boolean values for each affected field.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebc55b11-b98d-4b72-8631-459ec995e6e0
📒 Files selected for processing (7)
benchmarks/app-patterns/run.shbenchmarks/ci_public_baseline_check.pybenchmarks/public-baseline-config.jsonbenchmarks/public_baseline.pybenchmarks/run_public_baseline.shchangelog.d/7958-public-baseline-inputs.mdtests/test_public_baseline.py
Comment on lines
+238
to
+279
| config = _load(path) | ||
| if config.get("schema_version") != 1: | ||
| raise ArtifactError("public measurement config: unsupported schema") | ||
|
|
||
| toolchains = config.get("toolchains") | ||
| if not isinstance(toolchains, dict): | ||
| raise ArtifactError("public measurement config: missing toolchains") | ||
| for runtime in ("node", "bun"): | ||
| if not isinstance(toolchains.get(runtime), str) or not toolchains[runtime]: | ||
| raise ArtifactError( | ||
| f"public measurement config: invalid {runtime} toolchain pin" | ||
| ) | ||
|
|
||
| quiet = config.get("quiet_host") | ||
| if not isinstance(quiet, dict): | ||
| raise ArtifactError("public measurement config: missing quiet_host") | ||
| maximum = quiet.get("maximum_cpu_active_percent") | ||
| seconds = quiet.get("consecutive_seconds") | ||
| if not isinstance(maximum, (int, float)) or not 0 < maximum <= 100: | ||
| raise ArtifactError("public measurement config: invalid CPU-active maximum") | ||
| if not isinstance(seconds, int) or seconds < 1: | ||
| raise ArtifactError("public measurement config: invalid quiet-host duration") | ||
|
|
||
| components = config.get("components") | ||
| if not isinstance(components, dict): | ||
| raise ArtifactError("public measurement config: missing components") | ||
| for name in ("suite", "polyglot", "json_polyglot", "app_patterns", "honest_bench"): | ||
| component = components.get(name) | ||
| if not isinstance(component, dict): | ||
| raise ArtifactError(f"public measurement config: missing {name}") | ||
| measured = component.get("measured_runs") | ||
| if not isinstance(measured, int) or measured < 2: | ||
| raise ArtifactError( | ||
| f"public measurement config: {name}.measured_runs must be at least 2" | ||
| ) | ||
| for name in ("app_patterns", "honest_bench"): | ||
| warmup = components[name].get("warmup_runs") | ||
| if not isinstance(warmup, int) or warmup < 0: | ||
| raise ArtifactError( | ||
| f"public measurement config: {name}.warmup_runs must be non-negative" | ||
| ) | ||
| if components["honest_bench"].get("workloads") != [1, 3]: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-object and Boolean configuration values.
config.get(...) raises AttributeError when the JSON root is not an object. Also, True passes checks that use isinstance(value, int), and True == 1 accepts a Boolean schema_version.
Validate the root object first. Use exact integer checks for schema_version, durations, run counts, and warmups. Exclude Boolean values from the CPU percentage check. Add malformed-root and Boolean-value tests.
Proposed fix
def load_measurement_config(path: Path = MEASUREMENT_CONFIG) -> dict[str, Any]:
"""Load and validate the declarative inputs for public measurements."""
config = _load(path)
- if config.get("schema_version") != 1:
+ if not isinstance(config, dict) or type(config.get("schema_version")) is not int or config["schema_version"] != 1:
raise ArtifactError("public measurement config: unsupported schema")
@@
- if not isinstance(maximum, (int, float)) or not 0 < maximum <= 100:
+ if type(maximum) not in (int, float) or not 0 < maximum <= 100:
raise ArtifactError("public measurement config: invalid CPU-active maximum")
- if not isinstance(seconds, int) or seconds < 1:
+ if type(seconds) is not int or seconds < 1:
raise ArtifactError("public measurement config: invalid quiet-host duration")
@@
- if not isinstance(measured, int) or measured < 2:
+ if type(measured) is not int or measured < 2:
@@
- if not isinstance(warmup, int) or warmup < 0:
+ if type(warmup) is not int or warmup < 0:🤖 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 `@benchmarks/public_baseline.py` around lines 238 - 279, Update the
configuration validation flow around _load to first require the JSON root to be
a mapping before calling config.get. Use exact integer validation that excludes
booleans for schema_version, consecutive_seconds, measured_runs, and
warmup_runs, and exclude booleans from the maximum_cpu_active_percent numeric
check. Add tests covering malformed non-object roots and boolean values for each
affected field.
proggeramlug
pushed a commit
that referenced
this pull request
Aug 12, 2026
proggeramlug
added a commit
that referenced
this pull request
Aug 12, 2026
…file (#7967) * ci: put every Node the project chooses on 26, and assert it from one file .node-version (26.5.1) is the authoritative oracle, but the pin has leaked twice since #6367 made it single: CLAUDE.md's prose drifted off the file (#7599), and npm-launcher.yml was created by #6350 on the SAME DAY #6367 converted every existing workflow, keeping that day's ambient "22.23.1" literal by omission rather than by decision. - npm-launcher.yml (x2): "22.23.1" -> node-version-file: .node-version. It runs npm/perry/test/detect.test.cjs, which exercises the shipped launcher logic every installing user hits, so its Node is a behavioural input. Safe on the ubuntu-22.04 job: Node 26 needs glibc >= 2.28 and jammy has 2.35. - release-packages.yml: "20" -> "26". Node 20 reached EOL on 2026-04-30 and this is the repo's most privileged job (id-token: write, OIDC-publishes every platform package). - release-hono-server.yml: "24" -> "26". Both release workflows stay pinned to a bare MAJOR rather than node-version-file: they are publishing toolchains, and a gap-suite oracle bump must never be able to move the runtime that publishes releases. New scripts/check_node_version_consistency.py, wired as a lint step (a required context). It re-derives every restatement of a Node version from the file it quotes, and requires every literal node-version: in a workflow to be a registered exemption with a reason. Exemptions are asserted against the tree, so one that stops matching FAILS and must be updated or deleted. Reverting npm-launcher.yml to "22.23.1" reproduces the historical bug as two named failures. --self-test proves each rule can fail; both vacuity floors can fail too. Not changed, deliberately: test-compat/node-core/pinned-version.txt (v22.x runs Node's own corpus) and benchmarks/public-baseline-config.json (v22.23.1). The latter is in public_baseline.HARNESS_PATHS, so editing it alone reddens the required lint job -- measured, ci_public_baseline_check.py exits 2 with "benchmark harness changed". The pin and its ~2 h measurement are atomic by design (#7282/#7958) and the regeneration needs the quiet M1 mini, so it is registered as a self-clearing exemption carrying the runbook instead. Node 26 is faster than Node 22, so that regeneration is expected to reduce Perry's published advantage. Also: CLAUDE.md said "Two workflows are deliberately exempt" and then listed three, which is probably why the fourth pin read as accounted-for; and external-tools.json told readers to bump a NODE_PIN constant that does not exist in node_compat_matrix.mjs (it reads external-tools.json). Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 * docs: changelog fragment for #7967 Claude-Session: https://claude.ai/code/session_012B8z92S82sCfqCrVqrFgS2 --------- Co-authored-by: Ralph Küpper <ralph3@skelpo.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7282
What changed
The public benchmark protocol now lives in
benchmarks/public-baseline-config.json. It owns:run_public_baseline.shconsumes those values, and artifact assembly/freshness validation rejects component metadata, runtime versions, or quiet-host policy that disagrees with the config. The app-pattern runner now records the configured warmup/sample counts it actually passes to hyperfine.The fingerprint now covers the declarative config plus the honest-bench correctness oracle. Honest-bench kernels and fixture generators remain protected as source inputs. Large runner/checker files are plumbing, so logging, cleanup, error handling, and output-format changes no longer require a two-hour regeneration.
Reproduction and sabotage
Before this change, adding one comment to
benchmarks/json_polyglot/run.shchanged the harness digest from28117b86…tod9eec9be…and made the required checker exit 2.After the change:
513dba8f…unchanged and the checker exits 0.polyglot.measured_runsfrom 11 to 12 makes the checker exit 2.The existing artifact was green under the broader fingerprint before this change. An exact old-digest → new-digest migration accepts only this narrowing; any later input edit misses the pinned destination and hard-fails. No measurements or artifact values were changed.
Validation
python3 -m unittest tests.test_public_baseline— 11 passedpython3 benchmarks/ci_public_baseline_check.pybash -n benchmarks/run_public_baseline.sh benchmarks/app-patterns/run.shshellcheck -e SC2162 benchmarks/run_public_baseline.sh benchmarks/app-patterns/run.shpython3 -m py_compile ...python3 -m json.tool benchmarks/public-baseline-config.jsonbash scripts/check_file_size.shgit diff HEAD~1 --checkNo performance run is needed: this changes freshness bookkeeping and parameter ownership, while preserving every currently published parameter and measurement.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation