feat(bench): §9 benchmarks.md results appender (PR-J3) - #55
Merged
Conversation
Implements the `--update-benchmarks-md` path — the last
unwritten piece of the `report` module. A bench run with the
flag now folds its results into `docs/benchmarks.md` §9.
Lands:
- `src/report.rs` — `update_status_section(md, results) ->
String`, a pure transform. It maintains one block per
`(git_sha, hardware_kind)` pair inside a marker-delimited,
bench-managed region under a `### Results` sub-heading:
- **RFC0006.4**: re-running on the same `(sha, hw)` rewrites
that block in place — no duplicate sub-headings.
- **RFC0006.6**: a partial `--gates` run replaces only the
gates it measured (`a1`/`c1`/`c2` that are `Some`),
leaving previously recorded rows for the other gates
intact.
Prose outside the `BENCH-RESULTS` markers is never touched;
the region is regenerated wholesale from parsed-then-merged
blocks, so the parser only ever reads its own output.
- `src/main.rs` — `--update-benchmarks-md` now reads
`docs/benchmarks.md`, calls `update_status_section`, and
writes it back (was a "not implemented" warning). Thin file
I/O around the pure transform.
- `src/lib.rs` — re-exports `update_status_section`.
Tests (colocated in report.rs, pure `&str` transforms per the
§6 "temp markdown file" strategy):
- first run creates the Results block + preserves prose;
- rerun on same (sha, hw) rewrites in place, no dup heading
(RFC0006.4);
- C1-only rerun preserves the prior A1 row byte-for-byte
(RFC0006.6);
- missing `## 9.` anchor is a `Report` error.
Verified end-to-end: `--update-benchmarks-md` on the seed
corpus writes a valid §9 table, `mdbook build` renders it, and
the run leaves the committed `docs/benchmarks.md` untouched in
this PR (results blocks are committed deliberately, not by the
bench during CI).
Verification (CLAUDE.md §6.6):
- cargo fmt --all --check — clean.
- cargo clippy --all-targets --all-features -- -D warnings —
clean.
- cargo test --all-features — 267 passed (was 263; +4 §9
appender tests).
- mdbook build — clean.
Remaining for RFC 0006 `red → green`: only the RFC0006.7
reproducibility test (still `#[ignore]`'d). The §9 appender +
all three gates + the CLI are now done.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements end-to-end markdown benchmark reporting: adds ChangesMarkdown Benchmark Results Updater
Sequence DiagramsequenceDiagram
participant ResultsFile
participant update_status_section
participant parse_region
participant render_region
participant splice_region
participant MarkdownFile
ResultsFile->>update_status_section: provide ResultsFile
update_status_section->>parse_region: parse existing managed region (if any)
update_status_section->>update_status_section: merge gate rows for (git_sha, hardware_kind)
update_status_section->>render_region: render deterministic managed region
render_region->>splice_region: produce spliced markdown fragment
splice_region->>MarkdownFile: replace or insert managed region in docs/benchmarks.md
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
crates/ourios-bench/src/main.rs (1)
10-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale “not implemented yet” docs for
--update-benchmarks-md.Line 10 and Line 64 still claim this path is unimplemented, but Line 154 now executes it. This makes module docs and CLI help misleading.
Also applies to: 63-65
🤖 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 `@crates/ourios-bench/src/main.rs` around lines 10 - 13, The module docs and CLI help still state that the `--update-benchmarks-md` path is “not implemented yet” even though the flag is executed at runtime; update the comments at the top of main.rs (around the module doc comment at lines ~10 and the help text near line ~64) to remove “not implemented yet” and instead describe the actual behavior performed when `--update-benchmarks-md` is passed (i.e., that the markdown appender runs and JSON results are written) so the CLI help and module documentation match the code path executed later (the `--update-benchmarks-md` handling around line ~154).crates/ourios-bench/src/lib.rs (1)
11-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRefresh crate docs that still say markdown appender is pending.
Line 11 and Line 91 state this path is not implemented, but it is now wired in the binary and publicly re-exported.
Also applies to: 89-91
🤖 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 `@crates/ourios-bench/src/lib.rs` around lines 11 - 13, Update the crate-level documentation text that still says the markdown appender is pending: remove or reword the sentences referencing the unimplemented `--update-benchmarks-md` path and the pending markdown appender, and instead document that the appender is now wired in the binary and publicly available; update the mention near `write_results_json` (and any adjacent comment blocks around the `--update-benchmarks-md` flag) to reflect the implemented behavior and re-export so users know the feature is present.
🧹 Nitpick comments (1)
crates/ourios-bench/src/report.rs (1)
337-362: ⚡ Quick winConsider detecting malformed marker state to avoid silent corruption.
If
REGION_BEGINexists butREGION_ENDwas manually deleted, the current logic falls through to the first-run path and appends a new region—creating duplicate BEGIN markers and corrupting the file further.Detecting this edge case upfront would fail fast with a clear error instead:
♻️ Suggested validation
fn splice_region(md: &str, region: &str) -> Result<String, BenchError> { + let has_begin = md.contains(REGION_BEGIN); + let has_end = md.contains(REGION_END); + + if has_begin != has_end { + return Err(BenchError::Report { + detail: format!( + "docs/benchmarks.md has mismatched BENCH-RESULTS markers (begin={has_begin}, end={has_end}); \ + fix manually or regenerate the file" + ), + }); + } + if let (Some(begin), Some(end_rel)) = ( md.find(REGION_BEGIN), md.find(REGION_END).map(|e| e + REGION_END.len()), ) {🤖 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 `@crates/ourios-bench/src/report.rs` around lines 337 - 362, In splice_region, add validation for mismatched region markers: if md.find(REGION_BEGIN).is_some() but md.find(REGION_END).is None (or vice versa), return a BenchError::Report with a clear message about malformed BEGIN/END markers instead of falling through to the "first run" path; update the error text to mention the exact marker names (REGION_BEGIN/REGION_END) and that the file may be corrupted so the caller can fail fast—make this check at the top of splice_region before the existing begin/end replacement logic that uses REGION_BEGIN and REGION_END.
🤖 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 `@crates/ourios-bench/src/lib.rs`:
- Around line 11-13: Update the crate-level documentation text that still says
the markdown appender is pending: remove or reword the sentences referencing the
unimplemented `--update-benchmarks-md` path and the pending markdown appender,
and instead document that the appender is now wired in the binary and publicly
available; update the mention near `write_results_json` (and any adjacent
comment blocks around the `--update-benchmarks-md` flag) to reflect the
implemented behavior and re-export so users know the feature is present.
In `@crates/ourios-bench/src/main.rs`:
- Around line 10-13: The module docs and CLI help still state that the
`--update-benchmarks-md` path is “not implemented yet” even though the flag is
executed at runtime; update the comments at the top of main.rs (around the
module doc comment at lines ~10 and the help text near line ~64) to remove “not
implemented yet” and instead describe the actual behavior performed when
`--update-benchmarks-md` is passed (i.e., that the markdown appender runs and
JSON results are written) so the CLI help and module documentation match the
code path executed later (the `--update-benchmarks-md` handling around line
~154).
---
Nitpick comments:
In `@crates/ourios-bench/src/report.rs`:
- Around line 337-362: In splice_region, add validation for mismatched region
markers: if md.find(REGION_BEGIN).is_some() but md.find(REGION_END).is None (or
vice versa), return a BenchError::Report with a clear message about malformed
BEGIN/END markers instead of falling through to the "first run" path; update the
error text to mention the exact marker names (REGION_BEGIN/REGION_END) and that
the file may be corrupted so the caller can fail fast—make this check at the top
of splice_region before the existing begin/end replacement logic that uses
REGION_BEGIN and REGION_END.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8f3e4f9-bcb3-4549-b49b-5adf0b1299f9
📒 Files selected for processing (3)
crates/ourios-bench/src/lib.rscrates/ourios-bench/src/main.rscrates/ourios-bench/src/report.rs
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the missing --update-benchmarks-md workflow in ourios-bench by adding a pure markdown transform that merges a bench run’s results into docs/benchmarks.md §9 within a marker-managed region, and wiring the CLI flag to read/transform/write the doc.
Changes:
- Added
update_status_section(&str, &ResultsFile) -> Result<String, BenchError>to parse/merge/render the §9 managed results region, including tests for first-run insertion, in-place rewrite, partial-gate merge, and missing-anchor error. - Implemented the
--update-benchmarks-mdcode path in the binary to updatedocs/benchmarks.mdon demand. - Re-exported
update_status_sectionfrom the crate API.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/ourios-bench/src/report.rs | Adds the pure §9 markdown region parser/merger/renderer and associated unit tests. |
| crates/ourios-bench/src/main.rs | Wires --update-benchmarks-md to read/transform/write docs/benchmarks.md. |
| crates/ourios-bench/src/lib.rs | Re-exports update_status_section for use by the binary (and others). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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
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.
Summary
Implements the
--update-benchmarks-mdpath — the last unwritten piece of thereportmodule. A bench run with the flag folds its results intodocs/benchmarks.md§9.Lands
src/report.rs—update_status_section(md, results) -> String, a pure transform. It maintains one block per(git_sha, hardware_kind)pair inside a marker-delimited, bench-managed region under a### Resultssub-heading:(sha, hw)rewrites that block in place — no duplicate sub-headings.--gatesrun replaces only the gates it measured (a1/c1/c2that areSome), leaving previously recorded rows for the other gates intact.BENCH-RESULTSmarkers is never touched; the region is regenerated wholesale from parsed-then-merged blocks, so the parser only ever reads its own output.src/main.rs—--update-benchmarks-mdreadsdocs/benchmarks.md, calls the transform, writes it back (was a "not implemented" warning).src/lib.rs— re-exportsupdate_status_section.Tests (pure
&strtransforms, per the §6 "temp markdown file" strategy)(sha, hw)rewrites in place, no dup heading (RFC0006.4);## 9.anchor is aReporterror.Verified end-to-end
--update-benchmarks-mdon the seed corpus writes a valid §9 table:mdbook buildrenders it, and the run leaves the committeddocs/benchmarks.mduntouched in this PR (results blocks are committed deliberately by a maintainer, not by the bench during CI).Test plan
cargo fmt --all --check— clean.cargo clippy --all-targets --all-features -- -D warnings— clean.cargo test --all-features— 267 passed (+4 §9 appender tests).mdbook build— clean.--update-benchmarks-mdround (above).Maturity stage
Remaining for RFC 0006
red → green: only the RFC0006.7 reproducibility test (still#[ignore]'d). All three gates + CLI + JSON + §9 appender are done.🤖 Generated with Claude Code
Summary by CodeRabbit