fix(bench): tighten OTLP-loader consistency surface (PR-K2.1) - #59
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDefines recursive corpus loading that consumes ChangesBench Harness Corpus Specification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
Actionable comments posted: 0 |
Contributor
There was a problem hiding this comment.
Pull request overview
Updates RFC 0006 (bench harness) documentation to be internally consistent with the already-landed OTLP/JSON corpus loader behavior from PR #58, specifically around corpus byte accounting and the --corpus flag surface.
Changes:
- Broadens §3.4.1
bytes(raw_corpus)to include*.txt,*.jsonl, and*.jsoninputs and updates thefindexample accordingly. - Broadens §3.7
--corpusdocumentation to describe multi-format recursive loading and skipping unknown extensions. - Adds a note about OTLP/JSON envelope overhead affecting A1 comparability across encodings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Follow-up to PR #58. Copilot and CodeRabbit between them flagged that the OTLP/JSON loader landed with a surface that wasn't internally consistent — and one of the issues was a real correctness bug. Bundles the fixes here as a single squash-merge so the seven review threads across PR #58 + PR #59 close together. **Fixes (G — the real bug):** §3.4.1's A1 math invariant — both `bytes(raw_corpus)` and `bytes(zstd_corpus)` must process the same input. PR #58 broadened `bytes(raw_corpus)` to OTLP/JSON via the loader change but left `zstd_level_19_bytes` in `a1.rs` filtering on `*.txt` only. An OTLP-only corpus would therefore have `bytes(raw_corpus) > 0` and `bytes(zstd_corpus) = 0`, producing `zstd_ratio = 0` and an undefined A1 delta. The zstd reference codec now consumes the same extension set (`.txt | .jsonl | .json`) — kept in lockstep with `corpus::walk`'s dispatch via a comment on both sides. RFC §3.4.1 amended to spell out the invariant. **Tightens (consistency):** - `corpus.rs` `CorpusLoad` field docs + the `no non-empty corpus lines` error message now name the actual supported extensions. - The extension dispatch drops the per-entry `to_ascii_lowercase` allocation and uses `eq_ignore_ascii_case` on `&str` (allocation- free, hot loader path). `match`-with-guards per the project's feedback memory. - `Body::from_any_value` (already the single-sourced fork in `ourios-core::otlp`) replaces the local duplicate `any_value_to_body` in the loader. Drift risk → 0. - `main.rs` clap help text on `--corpus` broadened so `ourios-bench --help` matches RFC §3.7. - `ourios-core/Cargo.toml` comment aligned to the actual `serde_json::from_str` call (was `from_slice`). - RFC §3.4.1 reworded so `corpus.directory` no longer claims to disambiguate mixed-format corpora — mixed dirs produce an aggregate number; cleanly comparable runs need one encoding per dir. Per-format byte breakdown noted as a future enhancement. **Test (+1):** `a1::tests::zstd_consumes_every_corpus_extension` — mixed-extension corpus produces > 0 bytes, OTLP-only corpus does too (guards the failure mode the pre-fix `txt`-only filter introduced). Closes the Copilot threads on PR #58 (corpus.rs:38 / :459 / :275 + Cargo.toml:19) and PR #59 (RFC :278 / :289 / :640). 5 files changed (§5.2 phase limit). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jensholdgaard
force-pushed
the
docs/rfc-0006-broaden-raw-corpus
branch
from
May 29, 2026 06:29
b7eb0e1 to
c0d9792
Compare
jensholdgaard
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/rfcs/0006-bench-harness.md`:
- Around line 282-284: Replace the case-sensitive find predicates (-name) with
case-insensitive ones (-iname) in the example find command that filters
extensions (the tokens '-name' used for '*.txt', '*.jsonl', '*.json') so the
manual size-summing command uses case-insensitive matching (i.e., use '-iname'
for those predicates) to match the runtime loader dispatch behavior.
- Around line 295-297: RFC0006.1's acceptance criteria are inconsistent with the
broadened corpus scope: update the Scenario RFC0006.1 wording so it no longer
restricts counting to "txt-only" but instead counts across the same extension
set covered by bytes(zstd_corpus)/bytes(raw_corpus); specifically, modify the
Scenario RFC0006.1 description (references: "Scenario RFC0006.1",
"bytes(zstd_corpus)", "bytes(raw_corpus)") to state that byte/count comparisons
and validation operate over the full extension set implied by bytes(zstd_corpus)
rather than only .txt files, and adjust any test/validation language that
mentions txt-only counting to reflect the unified extension set and math
invariant.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8ac2494-ce80-4c84-93ee-3eb23daecdfe
📒 Files selected for processing (5)
crates/ourios-bench/src/a1.rscrates/ourios-bench/src/corpus.rscrates/ourios-bench/src/main.rscrates/ourios-core/Cargo.tomldocs/rfcs/0006-bench-harness.md
✅ Files skipped from review due to trivial changes (2)
- crates/ourios-bench/src/main.rs
- crates/ourios-core/Cargo.toml
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
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
Follow-up to PR #58 addressing two Copilot threads on that PR + three on this one, plus two CodeRabbit threads on this PR's first revision. They all cluster as one coherent issue: the OTLP/JSON loader landed without fully consistent surfaces, and one of the gaps is a real correctness bug.
Fixes (G — the real bug)
§3.4.1's A1 math invariant requires both
bytes(raw_corpus)andbytes(zstd_corpus)to process the same input. PR #58 broadenedbytes(raw_corpus)to OTLP/JSON via the loader change but leftzstd_level_19_bytesina1.rsfiltering on*.txtonly — so an OTLP-only corpus would havebytes(raw_corpus) > 0andbytes(zstd_corpus) = 0, producingzstd_ratio = 0and an undefined A1 delta.zstd_level_19_bytesnow consumes the same extension set ascorpus::walk(.txt | .jsonl | .json), with a code comment on both sides noting they must stay in lockstep.Tightens
corpus.rs—CorpusLoadfield docs +load()'s doc + theno non-empty corpus lineserror message broadened to name the actual supported extensions. Extension dispatch dropped the per-entryto_ascii_lowercaseallocation in favour ofeq_ignore_ascii_caseon the borrowed&str(match-with-guards per project convention). Localany_value_to_bodydeleted; the loader now callsourios_core::otlp::Body::from_any_valuedirectly (single-sourced fork — drift risk → 0).main.rs— clap help text on--corpusbroadened soourios-bench --helpmatches RFC §3.7.Cargo.toml— thewith-serdecomment now readsserde_json::from_str(matches the actual call; previousfrom_slicewas wrong).corpus.directoryclaim reworded (a directory path can't disambiguate a mixed-format corpus; cleanly comparable runs need one encoding per dir; per-format byte breakdown noted as a future enhancement).bytes(zstd_corpus)definition spelled out to cover the same extension set with the math-invariant rationale. §5 Scenario RFC0006.1 broadened to match.findexample uses-iname(case-insensitive, matching loader behavior).Test plan
cargo fmt --all --checkcargo clippy --all-targets --all-features -- -D warningscargo test --all-features— 285 passed / 19 ignored (+1 vs main: the newa1::tests::zstd_consumes_every_corpus_extension)mdbook buildThe new test guards against the failure mode the pre-fix
txt-only filter introduced: a mixed-extension corpus produces > 0 zstd bytes, an OTLP-only corpus does too, and an unrelated.mdfile in the same dir leaves the byte count unchanged (the silent assertion the first revision missed).Closes
Body::from_any_valuededup), corpus.rs:275 (allocation-free dispatch), Cargo.toml:19 (from_stralignment).-inamefor case-insensitivefind, §5 RFC0006.1 acceptance criteria broadening..md-skip assertion in the new test.5 files changed (§5.2 phase limit).
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
--corpusCLI help text to describe recursive directory loading and supported file extensions (*.txt, *.jsonl, *.json)Improvements