feat(bench): redefine C2 gate as per-service (#444) - #451
Conversation
C2 is specified over "a corpus from a single stable service", so grading a multi-service OTLP capture with one whole-corpus ratio is a category error: it conflates a noisy infra broker (high-cardinality offset/path tokens) with clean application services, failing the gate even when every application service converges perfectly (v8 §9.12). Redefine the gate to fold over the per-service decomposition (#445): a corpus passes iff every service with >= 1M lines has ratio >= 0.5, fails if any such service is below 0.5, abstains when none reach 1M. A single-service corpus — including the plain-text <unknown> bucket — collapses to the whole-corpus verdict, so historical text-corpus rows are unchanged; only multi-service OTLP corpora differ. The whole-corpus ratio is retained as a diagnostic. Under this gate the v8 capture flips FAIL -> PASS: cart is the sole service clearing the 1M-line floor and it converges at ratio 1.000; kafka's fragmentation (14,608 templates on 137k lines) is a bounded diagnostic, not a gate failure. Token-level polishing of hostile infra logs is an OTel Collector concern (transform/redaction processor upstream), not the miner's — consistent with "format parsing is the Collector's job". RFC 0006 §3.4.3 pass condition amended (maintainer-approved 2026-07-10); docs/benchmarks.md C2 grain + §9.12 v8 record updated. Cargo.lock synced (main's lock still listed async-stream for ourios-ingester after the TLS work dropped it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughC2 convergence gating now evaluates services independently at the 1M-line threshold, folds their verdicts, and abstains when no service qualifies. Whole-corpus convergence remains diagnostic, with corresponding tests, report handling, output, and documentation updates. ChangesC2 gate semantics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BenchmarkRun
participant C2Accumulator
participant ServiceResults
participant Report
BenchmarkRun->>C2Accumulator: finalize observations
C2Accumulator->>ServiceResults: compute per-service convergence
ServiceResults-->>C2Accumulator: return gated service verdicts
C2Accumulator-->>BenchmarkRun: return gate verdict and diagnostic ratio
BenchmarkRun->>Report: render C2 measurements
Report-->>BenchmarkRun: validate nullable measurement state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
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 (2)
crates/ourios-bench/src/c2.rs (2)
252-266: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve the historical single-service verdict.
gate_pass(&by_service)always uses the per-service ratio. For a single service, that ratio is sampled at exactly its millionth line, while the historical whole-corpus diagnostic uses the nearest global curve sample; on corpora larger than 1M lines these can differ and change legacy results. Special-case a single bucket to derivepassfrom the whole-corpus diagnostic, or make both sampling rules identical, and add a regression test.Also applies to: 275-286
🤖 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/c2.rs` around lines 252 - 266, The single-service verdict must preserve the historical whole-corpus sampling behavior instead of relying on the per-service millionth-line ratio. Update the verdict logic around the per-service result construction and gate_pass so a single bucket derives pass from the whole-corpus diagnostic (or use identical sampling rules), while retaining per-service ratios for multiple services. Add a regression test covering a corpus larger than one million lines where the sampling points differ.
252-258: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle 1M+ services with zero created templates explicitly.
s.lines >= ONE_MILLION && s.created > 0drops a qualifying service fromgate_pass, so the corpus can returnSome(true)while that service is never evaluated. Make the zero-template case fail or abstain by design, and add a test forcreated == 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 `@crates/ourios-bench/src/c2.rs` around lines 252 - 258, The 1M-line evaluation currently excludes services with zero created templates from gate evaluation. Update the logic around the at_1m/ratio/pass calculation to explicitly handle s.lines >= ONE_MILLION with s.created == 0 by returning a deliberate failure or abstention consistent with gate_pass semantics, and add a test covering created == 0 to verify the corpus cannot incorrectly pass.
🧹 Nitpick comments (1)
crates/ourios-bench/src/c2.rs (1)
430-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename this test and add a real failing-gate case.
non_converging_curve_fails_the_gatehas no service data and now assertspass == None, so it tests only the diagnostic ratio—not a gate failure. Add arecord-based test with a ≥1M-line service whose per-service ratio is below 0.5. As per coding guidelines, non-trivial behavior requires colocated unit tests.🤖 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/c2.rs` around lines 430 - 457, The test non_converging_curve_fails_the_gate only validates the convergence ratio because observe-based data leaves pass as None. Rename it to reflect diagnostic-only coverage, then add a colocated unit test using C2Accumulator::record that processes at least 1M lines for a service with a per-service convergence ratio below 0.5 and asserts the gate fails.Source: Coding guidelines
🤖 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/benchmarks.md`:
- Around line 1131-1133: Rephrase the benchmark conclusion to state that cart,
the sole service meeting the 1M-line gate, passes, while the smaller non-gating
services abstain; remove the claim that every application service converges
essentially perfectly.
---
Outside diff comments:
In `@crates/ourios-bench/src/c2.rs`:
- Around line 252-266: The single-service verdict must preserve the historical
whole-corpus sampling behavior instead of relying on the per-service
millionth-line ratio. Update the verdict logic around the per-service result
construction and gate_pass so a single bucket derives pass from the whole-corpus
diagnostic (or use identical sampling rules), while retaining per-service ratios
for multiple services. Add a regression test covering a corpus larger than one
million lines where the sampling points differ.
- Around line 252-258: The 1M-line evaluation currently excludes services with
zero created templates from gate evaluation. Update the logic around the
at_1m/ratio/pass calculation to explicitly handle s.lines >= ONE_MILLION with
s.created == 0 by returning a deliberate failure or abstention consistent with
gate_pass semantics, and add a test covering created == 0 to verify the corpus
cannot incorrectly pass.
---
Nitpick comments:
In `@crates/ourios-bench/src/c2.rs`:
- Around line 430-457: The test non_converging_curve_fails_the_gate only
validates the convergence ratio because observe-based data leaves pass as None.
Rename it to reflect diagnostic-only coverage, then add a colocated unit test
using C2Accumulator::record that processes at least 1M lines for a service with
a per-service convergence ratio below 0.5 and asserts the gate fails.
🪄 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: c6b707f5-d3a4-42e8-9be8-892281fc2d55
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/ourios-bench/src/c2.rscrates/ourios-bench/src/lib.rscrates/ourios-bench/src/main.rsdocs/benchmarks.mddocs/rfcs/0006-bench-harness.md
There was a problem hiding this comment.
Pull request overview
Redefines the C2 (template-count convergence) thesis gate in ourios-bench from a whole-corpus metric to a per-service (service.name) metric, aligning the bench harness and docs with RFC 0006’s “single stable service” scope and the #444 decision.
Changes:
- Updates C2 gate computation to fold over per-service convergence results while keeping whole-corpus convergence metrics as diagnostics.
- Adjusts CLI summary output and result-schema documentation to reflect the per-service gate semantics (PASS/FAIL/ABSTAIN rules).
- Updates benchmark documentation (including the otel-demo v8 record) and syncs
Cargo.lock.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/0006-bench-harness.md | Amends RFC 0006 C2 gate definition to be evaluated per service.name on multi-service corpora. |
| docs/benchmarks.md | Updates C2 gate “grain” definition and flips v8 C2 record to PASS under per-service evaluation; retains whole-corpus as diagnostic. |
| crates/ourios-bench/src/main.rs | Updates human-readable summary output to label whole-corpus ratio as diagnostic and clarify ABSTAIN semantics. |
| crates/ourios-bench/src/lib.rs | Updates C2Result and related docs to define pass as the per-service fold while preserving whole-corpus diagnostic fields. |
| crates/ourios-bench/src/c2.rs | Implements per-service gate fold (gate_pass) and reclassifies whole-corpus convergence as diagnostic; adds unit tests for the fold behavior. |
| Cargo.lock | Removes stale async-stream dependency entry for ourios-ingester. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
#444) Review follow-ups on #451: - Copilot: the single-service gate is measured at the *exact* millionth service line (created_at_1m), not the sampled whole-corpus point, so the collapse is verdict-equivalence for historical converged corpora (ratio far from the 0.5 boundary), not bit-identity with the sampled whole-corpus diagnostic. Corrected the over-claim in gate_pass's doc, RFC 0006 §3.4.3, and benchmarks.md §C2 — no behaviour change; the exact-1M measurement is strictly more precise than the diagnostic, so special-casing len==1 to the diagnostic would regress it. - CodeRabbit: §9.12 no longer says "every application service converges essentially perfectly" — cart (the sole gated service) passes; the smaller services abstain below the 1M-line floor and are not graded, though their observed counts sit at the same near-flat convergence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
…444) Copilot review on #451: - Correctness: gate_pass inferred the gated set via filter_map(|s| s.pass), assuming pass=Some iff a service reached 1M lines — but the per-service builder left pass=None when a >=1M service minted zero templates (every line NO_TEMPLATE), so gate_pass silently dropped it and `None` meant both "below 1M" and "degenerate >=1M". Fix: the builder now gates on lines>=1M unconditionally; a zero-template >=1M service passes trivially (flat count is the strongest convergence — C2's falsifier is *linear* growth) with an undefined ratio. `None` from gate_pass now unambiguously means "no service reached 1M lines". main.rs prints the new (ratio=None, pass=Some(true)) case. Regression test added. - Docs: the module-level "Pass" / ratio definitions and the attribute() comment still described the pre-#444 whole-corpus gate; updated to the per-service gate (whole-corpus ratio is diagnostic-only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
Copilot review on #451: count_at_1m can be 0 (with SS > 0) when the first template is minted only after the 1M-line mark, so the ratio's defined range includes 0. Corrected the module-level c2.rs doc and RFC 0006 §3.4.3 (a >=1M service that mints *zero* templates overall stays undefined/None, distinct from ratio 0.0). Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
…e_pass (#444) Copilot review on #451 (round 4): - Correctness (report.rs contract): an all-NO_TEMPLATE >=1M corpus (SS=0) produced the mixed diagnostic pair (template_count_at_1m_lines=Some(0), convergence_ratio=None), which report.rs rejects as an inconsistent C2 result — report generation would error. This was latent pre-#444; the new zero-template test made the state reachable. Fix: finalize now gates the whole-corpus count on template_count_at_end > 0 too, so the pair is always both-Some or both-None. report.rs renders the SS=0 >=1M case as "n/a (SS 0 — no templates mined)", distinct from the sub-1M "corpus < 1 M lines" abstention (uses the existing corpus_at_least_1m flag). Two regression tests: the accumulator asserts the consistent (None, None) pair; report.rs asserts the render doesn't error. - Style: gate_pass no longer allocates a temporary Vec<bool> — a single pass that short-circuits on the first failing gated service. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
…ding (#444) Copilot review on #451 (round 5): - report.rs: the (None,None) && corpus_at_least_1m arm rendered "no templates mined" without checking SS==0, so a corrupt ResultsFile with corpus_at_least_1m=true and SS>0 but no measurement would render a self-contradictory row instead of erroring (the surrounding contract's intent). Tightened: the arm now requires template_count_at_end==0; any >=1M/SS>0 (None,None) falls to the inconsistency error (message broadened to state the pair is set iff >=1M with SS>0). Regression test added. - Docs: reworded the ratio-0 condition from the causal "first template minted after 1M" to the observable "no template minted as of the sample nearest the 1M mark" — count_1m is the nearest sample, not the exact millionth line, so a sample landing after 1M can already include post-1M templates. c2.rs module doc + RFC 0006 §3.4.3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
Copilot review on #451 (round 6) — propagate the SS==0 (zero-template) semantics through the RFC spec text so it matches the implementation: - Convergence ratio is defined only when SS > 0; undefined (null, paired with a null count_at_1m) for a >=1M corpus that mints no templates. - Per-service pass rule now states the zero-template trivial-PASS exception, and that each service's count_at_1m is its *exact* millionth line (not the whole-corpus nearest-sample diagnostic). - "Reported as" paragraph: the two whole-corpus fields are null for corpora <1M *or* SS==0, and form a matched pair (both null or both set) that report.rs enforces. Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
…ICES note (#444) Copilot review on #451 (round 7): - report.rs: the C2 status-table Measurement column rendered an unlabeled "ratio …" that reads like the gate metric, but it's the whole-corpus diagnostic now — on a multi-service corpus it can be sub-0.5 with a PASS Verdict (per-service). Labeled "whole-corpus ratio … — diagnostic; verdict is per-service", matching the CLI summary's phrasing. - c2.rs: gate_pass doc still referenced "the filter_map below" after the round-4 rewrite to a for-loop; updated to describe the fold. - RFC 0006 §3.4.3: documented the MAX_SERVICES=1024 cardinality cap and the <other> fold (c2.services_truncated) — beyond the cap the gate is no longer strictly per-service; the truncation is surfaced, never silent. Doc/label-only; no behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
Copilot review on #451 (round 8): the C2Result doc said every >=1M service must pass "its own ratio >= 0.5" without the zero-template exception the code and RFC 0006 §3.4.3 now carry. Added it — a >=1M service that mints zero templates (SS=0, undefined 0/0 ratio) passes trivially. Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
… too (#444) Copilot review on #451 (round 9): the CLI printed the per-service breakdown only when by_service.len() > 1, so a single-service (or plain-text <unknown>) corpus showed only the whole-corpus *diagnostic* ratio, hiding the per-service exact-1M measurement that the verdict is actually based on. Now printed whenever any service bucket exists. Extracted the per-service status formatting into a testable per_service_status() helper with a unit test covering the ratio pass/fail, SS=0 trivial-pass, and abstain states. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
…444) Copilot review on #451 (round 10) — two stale descriptions after the round-9 change (breakdown now prints for single-service corpora too): - c2.rs: stable_curve_ratio_is_one's header comment said "ratio 1.0 → pass", but the test drives observe (no service.name) and asserts the per-service gate abstains (pass=None). Reworded to "whole-corpus diagnostic ratio 1.0", matching the body comment. - benchmarks.md §9.12: said the CLI prints the breakdown only for corpora with "more than one bucket"; it now prints whenever any service bucket exists. Updated (and relabeled the decomposition the gate, not just a diagnostic). Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
|
@coderabbitai review |
✅ Action performedReview finished.
|
* docs(bench): reconcile §9.12 with the resolved #444 decision The C2 gate landed per-service in #451 (option 3), but §9.12 still framed #444 as an open three-way fork ("tokenizer masking vs. length-aware thresholding vs. accept-and-scope — an RFC-level pillar #2 decision") and called it "a decision, not a performance one". Update both spots to the resolution: option 3 shipped (per-service gate, RFC 0006 §3.4.3), masking parked as a future strategic RFC (a Collector transform/redaction processor can polish infra tokens upstream), length-thresholding rejected; #444 accepted the query-capability tradeoff on hostile infra logs. Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): clarify §9.12 re-score date and the template-probe recovery CodeRabbit review on #452: - The §9.12 heading is the run date (2026-07-09); the #444 resolution is 2026-07-10. Flag the C2 re-score date in the heading so the two aren't read as contradictory. - "no single template query recovers it" understated the measurement — a single template_id probe recovers that id's slice (11,523 rows), just not the full dominant event. Reworded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): short §9.12 heading + note; unslash transform/redaction Copilot review on #452: - The re-score annotation made the §9.12 heading long/noisy (ToC + anchor slug). Reverted to the short heading and moved the note to an italic sentence right below (CodeRabbit's "explicitly state" option). - "transform`/`redaction` processor" read as one processor and broke oddly across the slash line-break; now "transform or redaction". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): clarify template_id 1 is a run-specific id, not canonical Copilot review on #452: "template_id == 1" read as if 1 were a stable canonical template. Reworded to "cart's dominant template (id 1 in this run — a run-specific identifier, not a canonical one)". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): drop positional "below" in the §9.12 gate reference Copilot review on #452: "the per-service gate below" is a positional reference that misleads if the paragraph moves (the gate is introduced earlier in the section). Reference it directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): make the #444 antecedent explicit in §9.12 Copilot review on #452: "#444 resolved it" could read as resolving the red-herring failure-flag confound rather than the fragmentation decision. Reword to "#444 settled how to handle the fragmentation". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 * docs(bench): use "length-aware thresholding" consistently in §9.12 Copilot review on #452: the same #444 option was named "length-aware thresholding" then "length-thresholding". Use the full term both times. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
Redefine the C2 (template-count convergence) thesis gate from whole-corpus to per-service, closing the
#444decision (option 3, maintainer-approved 2026-07-10).C2 is specified over "a corpus from a single stable service" (RFC 0006 §3.4.3 / benchmarks.md §C2). Grading a multi-service OTLP capture with one whole-corpus ratio is a category error: it conflates a noisy infra broker emitting high-cardinality offset/path tokens (Kafka: 14,608 templates on 137k lines) with clean application services, so the whole-corpus number fails even when every application service converges perfectly. The v8 capture (§9.12) surfaced this.
Change
c2.rs):passnow folds over the per-service decomposition (feat(bench): per-service C2 decomposition + v8 B2 pricing (#444) #445 gave us the diagnostic). A corpus passes iff every service with ≥ 1 M lines has ratio ≥ 0.5, fails if any such service is below 0.5, abstains (null) when no service reaches 1 M. Newgate_pass()is unit-tested directly (theobserve-based curves have noservice.name, so they exercise the whole-corpus diagnostic, not the gate).<unknown>bucket — collapses to the whole-corpus verdict, so every historical text-corpus §9 row is unchanged. Only multi-service OTLP corpora differ.convergence_ratio/template_count_at_1m_linesstay in the result and print labelled as diagnostics;by_serviceis now the gate basis.async-streamforourios-ingesterafter the TLS work dropped it from itsCargo.toml).v8 flip
Under the per-service gate the v8 capture passes:
cart(2.76 M lines) is the sole service clearing the 1 M-line floor and converges at ratio 1.000 with two templates. Kafka's fragmentation is now a bounded diagnostic, not a gate failure — and per §9.12's B2 pricing it costs template-exact query precision, not pruning or latency.Hazard note (§2 pillar #2 — miner correctness)
This is a measurement-honesty change, not a miner change: no similarity threshold, tokenizer, or merge behaviour moves, so §3.1 (no silent merges) and §3.3 (bit-identical reconstruction) are untouched. Token-level polishing of hostile infra logs is deliberately left to the OTel Collector (a
transform/redactionprocessor upstream), consistent with "format parsing is the Collector's job" — aggressive high-entropy masking in the miner is parked as a future strategic RFC with no commitment.Gate
cargo fmt --all --check✅cargo clippy --workspace --all-targets --all-features -- -D warnings✅cargo test -p ourios-bench --all-features✅ (all pass, incl. 7 c2 unit tests)mdbook build✅🤖 Generated with Claude Code
https://claude.ai/code/session_01X467Druw4cQeEPVp52DTG3
Summary by CodeRabbit
New Features
service.namefor services with at least 1M lines.Bug Fixes
Documentation
Tests