feat: introduce metabench, a unified benchmarking framework

[Martin Taillefer (geeknoid)]

No description provided.

[github-actions]

⚠️ Manual proc-macro SemVer review required

cargo semver-checks intentionally does not analyse proc-macro-only targets. 1 of 3 publishing crate(s) require manual review either for their own procedural macro contract or because they directly consume a manually reviewed breaking release. Ordinary libraries retain their automated result, and review advances to the next dependency edge only when the current release is breaking.

Crate	Baseline	Baseline commit	This PR	Minimum required	Status
metabench	new crate	—	0.1.0	0.1.0	✅ ok
metabench_macros	new crate	—	0.1.0	—	⚠️ manual proc-macro review required
metabench_macros_impl	new crate	—	0.1.0	0.1.0	✅ ok

⚠️ metabench_macros — manual proc-macro review detail

`metabench_macros` is a proc-macro-only crate. cargo-semver-checks intentionally skips proc-macro targets because they have no supported library API surface. Review exported macro names, accepted input syntax, diagnostics, and generated output manually; build and test results do not establish public API SemVer compatibility.

Proc-macro API compatibility must be reviewed manually; successful builds and tests do not establish SemVer compatibility. This check is informational and does not block the merge.

View the check run

[Copilot]

🟡 Changes recommended

There are a few repo-convention and operational issues (coverage-gate test-module annotations, very deep committed fixture paths, and a more robust workflow invocation) that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Introduces metabench, a new unified benchmarking framework intended to run the same workload across Criterion, Gungraun/Callgrind, Linux perf, and allocation tracking, and integrates it into the workspace + CI.

Changes:

• Adds new metabench runtime crate plus metabench_macros / metabench_macros_impl proc-macro crates, with benches and integration tests.
• Adds extensive fixture-based parsing tests for Criterion/Gungraun/perf/allocation artifacts and a CI smoke-check for benchmark registration output.
• Updates workspace dependencies/lockfile and wires metabench into mutation-testing grouping, spellcheck dictionary, and Miri exclusions.

File summaries

File	Description
scripts/test-metabench-registration.sh	New CI script that asserts stable --list benchmark identities for metabench bench targets.
scripts/mutants.rs	Adds metabench crates to cargo-mutants test grouping.
README.md	Documents metabench as a primary crate in the repo.
crates/metabench/tests/report_api.rs	Integration tests for report wire format + IO behavior and invariants.
crates/metabench/tests/fixtures/artifacts/traversal/too_deep/d1/d2/d3/d4/d5/d6/d7/d8/d9/d10/d11/d12/d13/d14/d15/d16/d17/d18/d19/d20/d21/d22/d23/d24/d25/d26/d27/d28/d29/d30/d31/d32/d33/d34/d35/d36/d37/d38/d39/d40/d41/d42/d43/d44/d45/d46/d47/d48/d49/d50/d51/d52/d53/d54/d55/d56/d57/d58/d59/d60/d61/d62/d63/d64/d65/.keep	Depth-limit sentinel fixture for artifact traversal.
crates/metabench/tests/fixtures/artifacts/traversal/cycle/new/estimates.json	Traversal fixture: Criterion estimates file for cycle/symlink scenario.
crates/metabench/tests/fixtures/artifacts/traversal/cycle/new/benchmark.json	Traversal fixture: Criterion benchmark metadata for cycle/symlink scenario.
crates/metabench/tests/fixtures/artifacts/perf/unicode.jsonl	Perf fixture: unicode event name JSONL record.
crates/metabench/tests/fixtures/artifacts/perf/minimal.jsonl	Perf fixture: minimal valid perf JSONL record.
crates/metabench/tests/fixtures/artifacts/perf/malformed.jsonl	Perf fixture: malformed JSONL record.
crates/metabench/tests/fixtures/artifacts/perf/invalid_numeric.jsonl	Perf fixture: invalid numeric counter value.
crates/metabench/tests/fixtures/artifacts/perf/empty.jsonl	Perf fixture: empty file for “no records” error path.
crates/metabench/tests/fixtures/artifacts/perf/duplicate.jsonl	Perf fixture: duplicate event records.
crates/metabench/tests/fixtures/artifacts/gungraun/wrong_mapping/summary.json	Gungraun fixture: summary with out-of-range case mapping.
crates/metabench/tests/fixtures/artifacts/gungraun/minimal/summary.json	Gungraun fixture: minimal valid v6 summary.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_wrong_set/metabench-identities.json	Gungraun fixture: identity manifest with wrong set.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_order/metabench-identities.json	Gungraun fixture: identity manifest for ordering test.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_order/b/summary.json	Gungraun fixture: summary part of manifest ordering scenario.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_order/a/summary.json	Gungraun fixture: summary part of manifest ordering scenario.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_malformed/metabench-identities.json	Gungraun fixture: malformed identity manifest JSON.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_duplicate/metabench-identities.json	Gungraun fixture: duplicate identities in manifest.
crates/metabench/tests/fixtures/artifacts/gungraun/manifest_count/metabench-identities.json	Gungraun fixture: wrong manifest count vs selected benchmarks.
crates/metabench/tests/fixtures/artifacts/gungraun/malformed/summary.json	Gungraun fixture: malformed summary JSON.
crates/metabench/tests/fixtures/artifacts/gungraun/invalid_numeric/summary.json	Gungraun fixture: invalid numeric metric type.
crates/metabench/tests/fixtures/artifacts/gungraun/empty/.keep	Gungraun fixture: empty directory sentinel.
crates/metabench/tests/fixtures/artifacts/gungraun/duplicate/b/summary.json	Gungraun fixture: duplicate case index scenario (b).
crates/metabench/tests/fixtures/artifacts/gungraun/duplicate/a/summary.json	Gungraun fixture: duplicate case index scenario (a).
crates/metabench/tests/fixtures/artifacts/criterion/unicode/new/estimates.json	Criterion fixture: unicode identity estimates.
crates/metabench/tests/fixtures/artifacts/criterion/unicode/new/benchmark.json	Criterion fixture: unicode identity metadata.
crates/metabench/tests/fixtures/artifacts/criterion/missing_function/new/estimates.json	Criterion fixture: missing function id estimates.
crates/metabench/tests/fixtures/artifacts/criterion/missing_function/new/benchmark.json	Criterion fixture: missing function id metadata.
crates/metabench/tests/fixtures/artifacts/criterion/minimal/new/estimates.json	Criterion fixture: minimal valid estimates.
crates/metabench/tests/fixtures/artifacts/criterion/minimal/new/benchmark.json	Criterion fixture: minimal valid metadata.
crates/metabench/tests/fixtures/artifacts/criterion/malformed/new/estimates.json	Criterion fixture: malformed estimates.
crates/metabench/tests/fixtures/artifacts/criterion/malformed/new/benchmark.json	Criterion fixture: malformed metadata.
crates/metabench/tests/fixtures/artifacts/criterion/invalid_numeric/new/estimates.json	Criterion fixture: invalid numeric duration.
crates/metabench/tests/fixtures/artifacts/criterion/invalid_numeric/new/benchmark.json	Criterion fixture: metadata paired with invalid numeric estimates.
crates/metabench/tests/fixtures/artifacts/criterion/empty/.keep	Criterion fixture: empty directory sentinel.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/b/new/estimates.json	Criterion fixture: duplicate identity scenario (b) estimates.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/b/new/benchmark.json	Criterion fixture: duplicate identity scenario (b) metadata.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/a/new/estimates.json	Criterion fixture: duplicate identity scenario (a) estimates.
crates/metabench/tests/fixtures/artifacts/criterion/duplicate/a/new/benchmark.json	Criterion fixture: duplicate identity scenario (a) metadata.
crates/metabench/tests/fixtures/artifacts/allocations/minimal.json	Allocation fixture: minimal valid allocation metrics.
crates/metabench/tests/fixtures/artifacts/allocations/malformed.json	Allocation fixture: malformed JSON.
crates/metabench/tests/fixtures/artifacts/allocations/invalid_numeric.json	Allocation fixture: invalid numeric values.
crates/metabench/tests/fixtures/artifacts/allocations/empty.json	Allocation fixture: empty set.
crates/metabench/tests/fixtures/artifacts/allocations/duplicate.json	Allocation fixture: duplicate benchmark entries.
crates/metabench/src/mode.rs	Mode enum + parsing/display and unit tests.
crates/metabench/src/lib.rs	Public crate docs/exports + main! entry macro for benchmarks.
crates/metabench/src/group.rs	Runtime benchmark registry, filtering, and wildcard matching + tests.
crates/metabench/src/fixture.rs	Benchmark case/fixture traits + prepared output helper.
crates/metabench/src/error.rs	Public error model and formatting + tests.
crates/metabench/src/engines.rs	bitflags engine selection model and mapping to Mode.
crates/metabench/src/artifact.rs	Artifact parsing/writing for Criterion/Gungraun/perf/allocations + tests.
crates/metabench/src/arguments.rs	CLI argument parsing/routing + tests.
crates/metabench/README.md	Generated crate README for metabench.
crates/metabench/CHANGELOG.md	New changelog entry for crate introduction.
crates/metabench/Cargo.toml	New metabench crate manifest, benches, deps, and metadata.
crates/metabench/build.rs	Build script exporting target/profile metadata.
crates/metabench/benches/stateless_no_cases.rs	Example bench target: stateless/no-cases registration.
crates/metabench/benches/stateless_cases.rs	Example bench target: stateless/data-driven cases.
crates/metabench/benches/stateful_no_cases.rs	Example bench target: stateful/no-cases via SimpleFixture.
crates/metabench/benches/stateful_cases/main.rs	Example multi-file bench target entrypoint.
crates/metabench/benches/stateful_cases/hashmap.rs	Example stateful data-driven bench group + cases.
crates/metabench_macros/tests/ui/support.rs	UI-test support “mini runtime” surface for macro expansion.
crates/metabench_macros/tests/ui/pass/registration_selection.rs	Pass UI test: group naming + selection behavior.
crates/metabench_macros/tests/ui/pass/fixtures.rs	Pass UI test: fixtures/cases/stateful patterns.
crates/metabench_macros/tests/ui/fail/unsafe_method.stderr	Fail UI test output: unsafe method rejection.
crates/metabench_macros/tests/ui/fail/unsafe_method.rs	Fail UI test: unsafe method in benchmarks impl.
crates/metabench_macros/tests/ui/fail/unknown_argument.stderr	Fail UI test output: unknown attribute argument.
crates/metabench_macros/tests/ui/fail/unknown_argument.rs	Fail UI test: unsupported benchmarks attribute args.
crates/metabench_macros/tests/ui/fail/trait_impl.stderr	Fail UI test output: trait impl rejected.
crates/metabench_macros/tests/ui/fail/trait_impl.rs	Fail UI test: benchmarks applied to trait impl.
crates/metabench_macros/tests/ui/fail/non_path_group.stderr	Fail UI test output: non-path self type rejected.
crates/metabench_macros/tests/ui/fail/non_path_group.rs	Fail UI test: invalid group type.
crates/metabench_macros/tests/ui/fail/mutable_case.stderr	Fail UI test output: mutable case ref rejected.
crates/metabench_macros/tests/ui/fail/mutable_case.rs	Fail UI test: &mut Case parameter.
crates/metabench_macros/tests/ui/fail/multiple_arguments.stderr	Fail UI test output: multiple params rejected.
crates/metabench_macros/tests/ui/fail/multiple_arguments.rs	Fail UI test: multiple parameters.
crates/metabench_macros/tests/ui/fail/invalid_signature.stderr	Fail UI test output: async method rejected.
crates/metabench_macros/tests/ui/fail/invalid_signature.rs	Fail UI test: async method in benchmarks impl.
crates/metabench_macros/tests/ui/fail/invalid_receiver.stderr	Fail UI test output: by-value receiver rejected.
crates/metabench_macros/tests/ui/fail/invalid_receiver.rs	Fail UI test: self receiver.
crates/metabench_macros/tests/ui/fail/invalid_group_name.stderr	Fail UI test output: group name validation.
crates/metabench_macros/tests/ui/fail/invalid_group_name.rs	Fail UI test: group name contains /.
crates/metabench_macros/tests/ui/fail/invalid_benchmark_name.stderr	Fail UI test output: benchmark name validation.
crates/metabench_macros/tests/ui/fail/invalid_benchmark_name.rs	Fail UI test: benchmark name contains /.
crates/metabench_macros/tests/ui/fail/inline_method.stderr	Fail UI test output: inline attribute rejected.
crates/metabench_macros/tests/ui/fail/inline_method.rs	Fail UI test: method annotated #[inline].
crates/metabench_macros/tests/ui/fail/generic_method.stderr	Fail UI test output: generic method rejected.
crates/metabench_macros/tests/ui/fail/generic_method.rs	Fail UI test: generic method.
crates/metabench_macros/tests/ui/fail/generic_impl.stderr	Fail UI test output: generic impl rejected.
crates/metabench_macros/tests/ui/fail/generic_impl.rs	Fail UI test: generic impl block.
crates/metabench_macros/tests/ui/fail/empty_impl.stderr	Fail UI test output: empty impl rejected.
crates/metabench_macros/tests/ui/fail/empty_impl.rs	Fail UI test: empty impl.
crates/metabench_macros/tests/ui/fail/duplicate_names.stderr	Fail UI test output: duplicate benchmark names.
crates/metabench_macros/tests/ui/fail/duplicate_names.rs	Fail UI test: duplicate names via benchmark overrides.
crates/metabench_macros/tests/ui/fail/duplicate_benchmark_attribute.stderr	Fail UI test output: duplicate benchmark attr.
crates/metabench_macros/tests/ui/fail/duplicate_benchmark_attribute.rs	Fail UI test: duplicate #[benchmark] usage.
crates/metabench_macros/tests/ui/fail/duplicate_argument.stderr	Fail UI test output: duplicate attribute argument.
crates/metabench_macros/tests/ui/fail/duplicate_argument.rs	Fail UI test: repeated name = argument.
crates/metabench_macros/tests/ui/fail/const_method.stderr	Fail UI test output: const method rejected.
crates/metabench_macros/tests/ui/fail/const_method.rs	Fail UI test: const method.
crates/metabench_macros/tests/ui/fail/benchmarks_on_function.stderr	Fail UI test output: attribute placement error.
crates/metabench_macros/tests/ui/fail/benchmarks_on_function.rs	Fail UI test: benchmarks attribute on fn.
crates/metabench_macros/tests/ui/fail/benchmark_outside_impl.stderr	Fail UI test output: benchmark attribute placement error.
crates/metabench_macros/tests/ui/fail/benchmark_outside_impl.rs	Fail UI test: benchmark attribute outside benchmarks impl.
crates/metabench_macros/tests/ui/fail/associated_item.stderr	Fail UI test output: associated items rejected.
crates/metabench_macros/tests/ui/fail/associated_item.rs	Fail UI test: associated const in impl.
crates/metabench_macros/tests/ui.rs	trybuild harness for downstream macro UI tests.
crates/metabench_macros/src/lib.rs	Proc-macro entry points delegating to impl crate, with coverage exclusions.
crates/metabench_macros/README.md	Generated README for metabench_macros.
crates/metabench_macros/CHANGELOG.md	New changelog for metabench_macros.
crates/metabench_macros/Cargo.toml	Proc-macro crate manifest + coverage-gate override.
crates/metabench_macros_impl/src/lib.rs	Macro implementation: validates impls/methods and generates registrations.
crates/metabench_macros_impl/README.md	Generated README for metabench_macros_impl.
crates/metabench_macros_impl/CHANGELOG.md	New changelog for metabench_macros_impl.
crates/metabench_macros_impl/Cargo.toml	Macro-impl crate manifest and deps (syn2/quote/etc).
Cargo.toml	Workspace deps updated and new workspace members/deps added for metabench.
Cargo.lock	Lockfile updated for new/updated dependencies.
.spelling	Adds metabench-related vocabulary to spellcheck allowlist.
.github/workflows/main.yml	Adds metabench registration step; excludes metabench from Miri.

Review details

• Files reviewed: 122/126 changed files
• Comments generated: 8
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

It introduces a large new benchmarking subsystem (multiple crates + workspace/CI integration) that warrants focused human review of overall design, long-term maintenance costs, and operational behavior.

Review details

Suppressed comments (1)

crates/metabench/src/group.rs:357

• When selecting without an identity manifest, an out-of-range cases.nth(index) is reported as Error::UnknownBenchmark(index.to_string()), which produces a confusing message (treating an index as a benchmark name). Consider including context that this is a case index.

    let (group, benchmark) = cases.nth(index).ok_or_else(|| Error::UnknownBenchmark(index.to_string()))?;

• Files reviewed: 127/131 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The new public-facing rustdoc examples in metabench use ? without a fn main() -> Result<...> wrapper, so the documentation snippets should be adjusted to be self-contained.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces multiple new crates, a new macro-based registration surface, and CI/dependency graph changes that warrant final human validation.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 0 new
• Review effort level: Lite

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (a312e8b) to head (196e551).

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #732    +/-   ##
========================================
  Coverage   100.0%   100.0%            
========================================
  Files         583      584     +1     
  Lines       62930    63327   +397     
========================================
+ Hits        62930    63327   +397     

Flag	Coverage Δ
linux	100.0% <100.0%> (?)
linux-arm	100.0% <100.0%> (?)
windows	100.0% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

There are correctness/maintainability issues in new code (notably a JSON assertion comparing Value to an integer and insufficiently-informative expect() messages) that should be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The report baseline logic can classify allocation regressions based on allocated_bytes shifts without exposing that shift in the public report model, which risks confusing downstream consumers.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The runner uses predictable temp-file/FIFO paths in a shared temp directory, which is vulnerable to interference on multi-user systems and should be hardened before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/src/runner.rs:705

• create_worker_token uses a predictable path under the global temp directory. On multi-user systems this can be interfered with (pre-creating the path, symlink tricks, etc.), causing spurious failures or writing/deleting an unintended file. Prefer a securely-created unique temp file (O_EXCL + random name) from tempfile and pass that path to the worker.

fn create_worker_token() -> Result<std::path::PathBuf, Error> {
    let sequence = WORKER_SEQUENCE.fetch_add(1, Ordering::Relaxed);
    let token = env::temp_dir().join(format!("metabench-worker-{}-{sequence}.token", std::process::id()));
    fs::write(&token, []).map_err(Error::CreateWorkerToken)?;
    Ok(token)
}

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The proc-macro implementation currently consumes any attribute ending in ::benchmark, which can conflict with unrelated attributes and should be narrowed to metabench’s intended forms.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Perf-mode measurements currently include black_box overhead because the Direct backend stops measurement after black-boxing the workload output.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/src/bencher.rs:277

• Same issue in the setup-based Direct backend: black_box(&output) happens before control.stop(), so perf counters (and any other MeasurementControl) include black-box overhead in the measured span. Consider stopping measurement immediately after the workload returns, then black-boxing and running cleanup outside the measured region (i.e., call control.stop() before black_box(&output) and cleanup(output)).

        Backend::Direct { iterations, control } => {
            for _ in 0..*iterations {
                let input = black_box(setup());
                if let Err(error) = control.start() {
                    bencher.failure = Some(error);
                    return;
                }
                let output = match catch_unwind(AssertUnwindSafe(|| invoke_with_input(&mut routine, input))) {
                    Ok(output) => output,
                    Err(payload) => {
                        let _ = catch_unwind(AssertUnwindSafe(|| control.stop()));
                        resume_unwind(payload);
                    }
                };
                black_box(&output);
                let stop = catch_unwind(AssertUnwindSafe(|| control.stop()));
                cleanup(output);

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces a large new benchmarking framework (runtime + macros + CI/dependency updates), which warrants final human review for API/maintenance and ecosystem impact.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces multiple new crates (including proc macros) plus workspace/CI/dependency changes that warrant final human review for cross-workspace impact.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

crates/metabench_macros_impl/src/lib.rs currently deviates from the repo’s established coverage configuration pattern for test modules, and dependency additions introduce a duplicated nix version that should be evaluated/aligned.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench_macros_impl/src/lib.rs:315

• Repository convention is to exclude unit-test modules from coverage measurement by placing #[cfg_attr(coverage_nightly, coverage(off))] immediately above #[cfg(test)] mod tests (for example crates/internity_macros_impl/src/lib.rs:56-58). Add the same attribute here so test-only lines don't count toward coverage reporting.

• Files reviewed: 127/131 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A small but concrete maintainability issue remains in crates/metabench/src/arguments.rs where --allocations-arg is still implied as supported despite being intentionally removed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Allocation-mode measurement currently includes black_box(&output) inside the allocation span in Bencher, which can skew the tracked allocation window and should be reordered.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/src/bencher.rs:258

• In allocation-tracking mode for setup-based benchmarks, black_box(&output) runs before the measurement span is dropped, so black-boxing is included in the measured allocation window. Drop the span immediately after the workload returns, then black-box / cleanup outside measurement to keep boundaries consistent across backends.

                let input = black_box(setup());
                let span = operation.measure_process().iterations(1);
                let output = invoke_with_input(&mut routine, input);
                black_box(&output);
                drop(span);
                cleanup(output);

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The metabench crate’s public __private re-exports should be reflected in its cargo_check_external_types allowlist to keep metadata consistent with the exposed API surface.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 127/131 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

A few newly added tests/docs include clear correctness and maintainability issues (non-existent Duration constructors and session-specific documentation in repo code) that should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/src/arguments.rs:628

• std::time::Duration has no from_mins/from_hours constructors. Use from_secs here so the test expresses the same durations without relying on non-existent APIs.

        for (value, expected) in [
            ("1ms", Duration::from_millis(1)),
            ("2s", Duration::from_secs(2)),
            ("3m", Duration::from_mins(3)),
            ("4h", Duration::from_hours(4)),
        ] {

• Files reviewed: 79/83 changed files
• Comments generated: 4
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

It includes PR/session-specific internal process commentary in a committed test doc comment (and minor trailing-whitespace), which should be cleaned up before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/metabench/tests/spawned_benchmark.rs:13

• The module docs include PR/session-specific process notes ("write_unverified", "WSL-crash policy", "this session forbids") that aren't relevant to the repository and will become stale/noisy over time. Please remove this internal workflow commentary and keep the doc comment focused on what the test validates.

• Files reviewed: 80/84 changed files
• Comments generated: 1
• Review effort level: Lite

[martintmk]

[AI AGENT]: Review of the new metabench crate, public-API first.

What I verified. I read CI on this exact head (35f3d96) rather than re-running the suite, and reproduced the two concrete failures locally in a clean worktree: cargo test -p metabench --doc gives test result: FAILED. 18 passed; 7 failed (every crate-level example), and cargo test -p metabench --lib exact_identity_and_mode_filters_report_whether_work_remains reproduces the CI assertion failure. I also probed the builder surface from a consumer's position with a throwaway integration test, and verified my replacement assertions for the broken test pass. All probes have been reverted.

Public surface reviewed. The crate root exports Suite, SuiteBuilder, Case, case, Engines, Throughput, ThroughputSpec and the main! macro, plus the #[doc(hidden)] __private module. The typestate builder chain (GroupBuilder, BenchmarkBuilder and ~20 lifecycle marker types) is pub inside the private builder module and reachable through return types but not nameable. Two findings land here: serde is an unapproved external type on the re-exported Throughput, and the unnameable builder types make suites unfactorable. I also reviewed dependencies and features, the report/artifact comparison logic, the Miri and coverage gate changes, and the test fixtures.

Areas with no finding. classify/percentage_shift in report.rs handle the zero-baseline and threshold boundaries correctly and are covered by targeted tests; the Gungraun identity-manifest staleness paths in group.rs are guarded and have fixtures for the count, order and wrong-set cases; the unsafe env mutation in runner.rs is genuinely the sole CARGO_TARGET_DIR writer in the suite, as its SAFETY comment claims. No telemetry or resilience surface is touched, so those gates did not apply.

What I could not assess. I did not execute the Criterion, Gungraun, perf or allocation backends end to end — they need Linux and Callgrind — so the measurement-boundary changes made in earlier review rounds are reasoned about from the source only, not measured. I also did not evaluate the report's numerical output against real engine artifacts.

Verdict: changes requested. The three blocking items are the seven broken doctests, the failing retain_mode test that leaves mode filtering untested, and serde in the public API. CI on this head is red across pr-test, testing, static-analysis, spell-check and external-type-exposure; the findings below cover the ones that point at a design or correctness problem rather than restating tooling output. Separately, this PR adds ~12k lines and a new published crate with an empty description — a short summary of the engine model and the reporting contract would help future readers considerably.

[martintmk]

[AI AGENT]: Every crate-level usage example fails to compile, so the entire onboarding documentation for this crate is broken on docs.rs and in cargo test --doc. main! expands fn __metabench_build_plan() plus mod __metabench_gungraun { ... super::__metabench_build_plan ... }. Rustdoc sees no fn main item in the snippet (the macro call is not expanded during that check), wraps the whole thing in fn main() { ... }, and __metabench_build_plan becomes a function-local item that super:: can no longer resolve.

Verified: cargo test -p metabench --doc -> test result: FAILED. 18 passed; 7 failed, failing lib.rs lines 29, 47, 69, 96, 118, 141 and main (line 282), each with error[E0425]: cannot find value `__metabench_build_plan` in module `super` . This matches testing (ubuntu-latest) on this head. Suggested fix: main! is documented as a module-scope entry-point macro, so these snippets cannot be rustdoc-compiled as written — render them as ```text (or split them so the compiled part is the init function and only metabench::main!(init); is un-compiled). Compile coverage for the macro is already provided by the four benches/*.rs targets, which expand it at real module scope.

[martintmk]

[AI AGENT]: This claim does not survive contact with the compiler, and it locks in an API that consumers cannot factor. GroupBuilder and BenchmarkBuilder live in the private builder module and are not re-exported, so a consumer cannot write a helper function that takes a group — every benchmark file must inline all registration into one closure passed to Suite::build. That is a real constraint for the multi-group suites this crate exists to serve.

Verified: an integration test containing fn register(group: metabench::GroupBuilder) fails with error[E0425]: cannot find type `GroupBuilder` in crate `metabench` . The repo's own committed trybuild fixtures also falsify "ordinary callers never name them": tests/ui/group_setup_called_twice.stderr blesses the diagnostic no method named `setup` found for struct `metabench::builder::GroupBuilder<metabench::builder::NoSuiteState, metabench::builder::GroupState<u8>>` . Typestate builders name their types at consumers on every misuse, and here every such path is unwritable and absent from rustdoc. Suggested fix: re-export GroupBuilder, BenchmarkBuilder and the typestate markers from the crate root (purely additive), and correct this paragraph. If they should stay unnameable, that is a decision worth stating explicitly, but it should not be justified by a claim the fixtures contradict.

[martintmk]

[AI AGENT]: This assertion is wrong, and it is the only test of retain_mode, so mode filtering currently has no working coverage. retain_mode (builder.rs L2427) retains benchmarks whose engine set contains the mode; it does not narrow engines to that mode. The buffers/allocate cases in ordered_cases() declare no engines and therefore keep Engines::DEFAULT, so exact equality with Engines::CRITERION cannot hold.

Verified: cargo test -p metabench --lib exact_identity_and_mode_filters_report_whether_work_remains -> assertion failed: mode.benchmarks().all(|(_, benchmark)| benchmark.engines() == Engines::CRITERION) at tests.rs:280, reproducing pr-test/testing on this head. Note that simply relaxing this to .contains(Engines::CRITERION) would be a tautology — it restates retain_mode's own predicate. The assertions below instead discriminate real behavior: Callgrind drops the CRITERION-only benchmark, and Perf (selected by nothing) empties the plan. Verified passing with test ... ok.

Suggested change

	assert!(mode.benchmarks().all(|(_, benchmark)| benchmark.engines() == Engines::CRITERION));
	assert_eq!(mode.benchmark_count(), 4);
	let mut callgrind = ordered_cases();
	assert!(callgrind.contains_mode(Mode::Callgrind));
	callgrind.retain_mode(Mode::Callgrind);
	assert_eq!(identities(&callgrind), ["buffers/allocate/small", "buffers/allocate/medium", "buffers/allocate/large"]);
	let mut perf = ordered_cases();
	assert!(!perf.contains_mode(Mode::Perf));
	perf.retain_mode(Mode::Perf);
	assert!(perf.is_empty());

[martintmk]

[AI AGENT]: These derives put serde in metabench's public API and fail the external-type-exposure gate. Throughput is a re-exported crate-root type, so its Serialize/Deserialize impls make serde's major version part of metabench's semver: a serde 2.0 would become a metabench breaking change, and consumers inherit the bound forever.

Verified: external-type-exposure on this head reports error: Unapproved external type `serde_core::de::Deserialize` referenced in public API --> crates/metabench/src/throughput.rs:9:30 and the same for serde_core::ser::Serialize. Note this is distinct from the __private re-export thread — Throughput is not doc(hidden), which is why the tool does fail here. Suggested fix: the only reason Throughput is serialized is the internal artifact side-channel (artifact::write_throughput/read_throughput), so prefer a private DTO in artifact.rs and drop the derives, keeping the public type serde-free. If you'd rather keep the derives, crates/tick/Cargo.toml is the repo precedent — add "serde_core::de::*" and "serde_core::ser::*" to allowed_external_types — but that makes an internal on-disk format a permanent public bound.

[martintmk]

[AI AGENT]: non-blocking: combined with crates/metabench/**/* in codecov.yml, this leaves a new 12k-line crate with no coverage enforcement at all — neither the repo gate nor Codecov. I understand the intent from the comment above (engine execution really does happen in harness-free bench targets), and I see this replaced the crate-wide coverage(off) from an earlier thread. But the exclusion is far wider than the justification: arguments.rs (700 lines), artifact.rs (813), report.rs (1755) and builder.rs (2854) are pure parsing, filtering and rendering logic with committed fixtures and existing unit tests.

A visible consequence is that Codecov's "All modified and coverable lines are covered by tests" on this PR is now vacuous — none of metabench is coverable. Suggested fix: keep the crate in both gates and set a real min-lines-percent from the logic modules' current measured coverage, excluding only the modules that genuinely require an external engine (bencher.rs, runner.rs) rather than the whole crate.

[martintmk]

[AI AGENT]: non-blocking: the stated justification is not accurate, which matters because it is the record for why the exclusion is safe. Nothing in metabench forbids unsafe code — there is no #![forbid(unsafe_code)] in the crate and rust.unsafe_code is not denied in [workspace.lints] — and crates/metabench/src/runner.rs contains three unsafe blocks (L1350, L1358, L1360) calling env::set_var/env::remove_var.

Those are in a #[cfg(test)] module, so miri test --lib --tests is exactly the run that would have covered them, and env-var mutation in a threaded libtest binary is precisely the class of hazard Miri flags. The second half of the reason (integration tests orchestrate subprocesses and the filesystem) is sound on its own. Suggested fix: drop the "forbids unsafe code" clause and justify the exclusion on the subprocess/filesystem orchestration alone, noting that the crate's only unsafe is test-local env mutation whose safety argument rests on being the sole CARGO_TARGET_DIR writer in the suite.

[martintmk]

[AI AGENT]: non-blocking: syn2 is added to the workspace dependency table but nothing references it — git grep syn2 matches only this line. Earlier review threads on this PR discuss crates/metabench_macros and crates/metabench_macros_impl, which are not in the current diff, so this looks like leftover config from that removed proc-macro crate. It also plausibly explains the bug[unresolved-workspace-dependency] reported by static-analysis on this head.

Suggested fix: remove the entry.

Suggested change

syn2 = { package = "syn", version = "2.0.111", default-features = false }

[martintmk]

[AI AGENT]: non-blocking: this rustdoc comment points readers at docs/TODO.md, which does not exist in this repository — git log origin/main -- docs/TODO.md returns nothing, so it was never committed. crates/metabench/src/builder.rs:2752 carries the same dangling reference. Pointing a reader at a private planning artifact to explain a design decision leaves the decision unexplained for every future maintainer.

This sentence is also one of the four spell-check failures on this head (P1 at group.rs:39; the others are iterable at builder.rs:977, bencher at builder.rs:2601 and Gungraun's at group.rs:32, none of which are in the .spelling additions). Suggested fix: inline the actual reason the plan-build cache was not pursued and drop both docs/TODO.md references.

[martintmk]

[AI AGENT]: non-blocking: time is used for exactly two lines — OffsetDateTime::now_utc().format(&Rfc3339) at report.rs:365-366 — to produce the report timestamp. jiff is already a workspace dependency (Cargo.toml:141) and is this repo's preferred date/time crate over the legacy chrono/time pair; jiff::Timestamp::now() renders RFC 3339 via Display with no format-description machinery.

Suggested fix: swap to jiff and drop the time dependency, which also removes a crate from metabench's graph rather than adding one.

[martintmk]

[AI AGENT]: Design note, no change requested: this public From impl makes criterion's semver metabench's. criterion::Throughput is an external type, so a criterion 0.9 that touches this enum forces a metabench breaking release, and consumers can rely on the conversion. cargo-check-external-types does not flag impl Trait for ExternalType, so the allowed_external_types list at Cargo.toml:24 does not protect this the way it protects the serde derives above. Its only production use is runner.rs:881, so a pub(crate) fn to_criterion(self) would give the same behavior with none of the coupling.

Worth deciding alongside that: neither Throughput nor ThroughputSpec is #[non_exhaustive]. Adding a unit (bits, operations) or a new derivation strategy later would be breaking, and ThroughputSpec's variants publicly expose Box<dyn Fn(&Input) -> Throughput + Send + Sync>, committing to that exact closure shape. Since this is 0.1.0 and unreleased, both are cheap to settle now and expensive afterwards.

[Copilot]

🟡 Changes recommended

Artifact path selection and Criterion estimate validation have correctness issues that can produce misleading or nonsensical reports in real-world runs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 69/73 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Artifact isolation/validation and worker-token hardening issues should be addressed to prevent stale/misleading reports and strengthen internal marker handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

crates/metabench/src/artifact.rs:50

• Artifact output directories are derived from pre-existing CRITERION_HOME/GUNGRAUN_HOME and default to shared paths under the report root (e.g., target/metabench//criterion). This makes it easy to accidentally mix stale artifacts from earlier runs (or from a user’s environment overrides) into a new combined report. Prefer per-run, metabench-owned directories so each invocation parses only the artifacts it just produced.

        let run = root.join("runs").join(format!("{timestamp}-{}", std::process::id()));
        let criterion = std::env::var_os("CRITERION_HOME").map_or_else(|| root.join("criterion"), PathBuf::from);
        let gungraun = std::env::var_os("GUNGRAUN_HOME").map_or_else(|| root.join("gungraun"), PathBuf::from);
        fs::create_dir_all(&run).map_err(|source| Error::ArtifactIo { path: run.clone(), source })?;

crates/metabench/src/artifact.rs:314

• Criterion estimates are only validated for finiteness; negative medians/interval bounds will currently be accepted. The fixtures include an "invalid_numeric" case with a negative point_estimate, and for time-based measurements a negative estimate is nonsensical—treat these as invalid artifact data.

fn validate_finite(path: &Path, value: f64) -> Result<(), Error> {
    if value.is_finite() {
        Ok(())
    } else {
        Err(Error::ArtifactFormat {

crates/metabench/tests/spawned_benchmark.rs:26

• This spawns cargo bench from within the test without pinning the package, which can become ambiguous or run the wrong target when invoked from a workspace root. Add -p metabench so the test always targets this crate’s benches.
  crates/metabench/tests/spawned_benchmark.rs:37
• This spawns cargo bench from within the test without pinning the package, which can become ambiguous or run the wrong target when invoked from a workspace root. Add -p metabench so the test always targets this crate’s benches.

• Files reviewed: 68/72 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Workspace dependency entries were added that appear unused in-tree (e.g., syn2, nix, bitflags), which should be removed or justified to avoid unnecessary maintenance/audit surface.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Cargo.toml:169

• nix was added as a workspace dependency, but nothing in the repo currently depends on it directly (no use nix::... and no workspace = true dependency entries). As a result, the lockfile only carries nix 0.27.1 from command-group, so the workspace nix = 0.31.3 entry is effectively unused and the nearby comment about “duplicate” versions becomes misleading.

Unless you’re about to add a direct nix usage, remove this workspace dependency entry.

nix = { version = "0.31.3", default-features = false }

• Files reviewed: 82/86 changed files
• Comments generated: 2
• Review effort level: Lite