feat(miner): add Drain prefix tree skeleton (RFC 0001 §6.2 step 3) by jensholdgaard · Pull Request #17 · jensholdgaard/ourios

jensholdgaard · 2026-05-13T14:02:11Z

Summary

Lays out the parent-path data structure (Tree::descend_mut) that MinerCluster::ingest will walk before per-leaf simSeq selection. Shape is the Drain-paper canonical: root → length-N node → prefix-token nodes → leaf list. Skeleton-only PR; integration follows the same primitive-first / integration-second cadence as #15 (sim_seq math).

RFC anchor

RFC 0001 §6.2 step 3 — parent = tree.descend(len(L_masked), L_masked[0..d-1]). This PR ships the data structures + descend_mut. Step 4 (best-candidate selection) and step 5 (widening + audits) are follow-up PRs.

Hazards / invariants touched

None this PR. The tree exists but has no caller; no §3 invariant or §4 hazard changes observable behaviour. The next PR (cluster integration) will be the one that engages §3.1 (similarity threshold), H1 (no silent merges), and H5 (template versioning).

§5 scenarios

Flipped this PR: 0 (skeleton has no caller).
Unblocks: §6.2 step 4 best-candidate selection (next PR), §6.2 step 5 widening branch (PR after that), and most H1.*/H5.* hazards once widening lands.

Outer-loop cargo test --all-features count moves from 30 → 40 (10 new tree::tests::* cases). Ignored count unchanged at 23.

Notable design choices

OwnedToken is the storage counterpart to sim_seq::Token; tree leaves outlive the ingest call that created them, so they cannot hold borrowed slices. OwnedToken::as_borrowed() is the zero-copy bridge into sim_seq (covered by the round-trip test).
Recursive descend_mut rather than iterative — re-binding &mut PrefixNode inside a for loop runs into the stable borrow checker's well-known sub-borrow extension issue (Polonius would solve it; recursion is the safe-code idiom on stable). Recursion depth is bounded by prefix_depth (Drain default 2), so the stack is a non-issue.
prefix_depth semantics: the parameter is "number of prefix-token levels below the length node" (Drain-paper d - 2), not RFC §6.2's literal L_masked[0..d-1] notation. The two differ by one. Module docs flag the discrepancy and a follow-up RFC-clarification PR (same pattern as docs(rfc-0001): clarify template_id allocator scope (PR #13 driver) #14) will reconcile §6.2's wording. DEFAULT_PREFIX_DEPTH = 2 matches Drain3's default depth = 4.

Verification (per `CLAUDE.md` §6.6)

cargo fmt --all --check — clean
cargo clippy --all-targets --all-features -- -D warnings — clean
cargo test --all-features — 40 passed / 23 ignored / 0 failed
All 10 new tests follow strict AAA structure (per the established project convention)
N/A mdbook build — no docs touched

Test plan

tree shape (length grouping, prefix grouping, length partitioning)
descend_mut returns the same PrefixNode for repeat (length, prefix) shapes (pointer-identity check on the leaves vector + leaf written by call 1 visible to call 2)
short-line handling (line shorter than prefix_depth walks all tokens, no over-consumption)
prefix_depth = 0 collapses to length-bucket-only grouping
panic on empty input (precondition pin)
OwnedToken ↔ Token round-trip (Fixed + Wildcard)
full OwnedToken template feeds sim_seq via as_borrowed

🤖 Generated with Claude Code

Lays out the parent path that `MinerCluster::ingest` will walk before per-leaf `simSeq` selection. Shape is the Drain-paper canonical: root → length-N node → prefix-token nodes → leaf list. Skeleton-only PR — ships data structures + `Tree::descend_mut` only. Best-candidate selection via `simSeq`, the §6.2 step-5 widening branch, audit emission, and `MinerCluster` integration all land in follow-up PRs. Same primitive-first / integration- second cadence as PR #15 (`sim_seq` math). Notable design choices: - `OwnedToken` is the storage counterpart to `sim_seq::Token`; leaves outlive the ingest call that created them so they cannot hold borrowed slices. `OwnedToken::as_borrowed` is the zero-copy bridge into `sim_seq`. - Recursive `descend_mut` rather than iterative — re-binding `&mut PrefixNode` inside a `for` loop runs into the stable borrow checker's well-known sub-borrow-extension issue (Polonius would solve it; recursion is the safe-code idiom on stable). Recursion depth is bounded by `prefix_depth` (Drain default 2), so stack is a non-issue. - `prefix_depth` parameter is "number of prefix-token levels" (Drain-paper `d - 2`), not RFC §6.2's literal `d - 1`. The two differ by one; module docs flag the discrepancy and a follow-up RFC-clarification PR will reconcile §6.2's wording. - `DEFAULT_PREFIX_DEPTH = 2` matches Drain3's default `depth = 4`. §5 scenarios flipped: 0 (skeleton has no caller). §5 scenarios this PR unblocks: §6.2 step 4 best-candidate selection (next PR), the §6.2 step-5 widening branch (PR after). Verification: - `cargo fmt --all --check` — clean - `cargo clippy --all-targets --all-features -- -D warnings` — clean - `cargo test --all-features` — 40 passed / 23 ignored / 0 failed (10 new `tree::tests::*`, all AAA-structured per the project testing convention) - `mdbook build` — N/A (no docs touched)

Copilot

Pull request overview

Adds the initial Drain-style prefix tree data structures and a mutable descent helper (Tree::descend_mut) to support upcoming MinerCluster::ingest integration for RFC 0001 §6.2 step 3.

Changes:

Introduces crates/ourios-miner/src/tree.rs with Tree/LengthNode/PrefixNode/Leaf and OwnedToken, plus Tree::descend_mut.
Adds unit tests validating tree shape/partitioning and OwnedToken -> Token borrowing behavior.
Exposes the new module via pub mod tree; in ourios-miner’s lib.rs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
crates/ourios-miner/src/tree.rs	Adds Drain prefix tree skeleton (`descend_mut`), owned template token type, and tests.
crates/ourios-miner/src/lib.rs	Exports the new `tree` module.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Four substantive Copilot inline comments on the tree skeleton: - `descend_recursively` allocated a `String` on every level even for hot-path lookups where the child already exists. Switch to `contains_key`-then-`get_mut`, falling back to `insert` only on first-observation. `std::collections::HashMap`'s `Entry` API needs an owned key, so lookup-then-insert is the standard workaround on stable; the second `get_mut` is one extra hash, never an extra allocation. - Doc wording on `Tree::descend_mut` was inverted — short lines bottom out at a *shallower* prefix level, not deeper. Fixed. - Renamed `Leaf::id` → `Leaf::template_id` to match RFC 0001 §6.1 language and disambiguate from forthcoming `template_version`, slot-id, and alias-id additions. Doc clarifies the field's cluster-wide-unique allocator contract. - Softened `PrefixNode` "by construction" → "by convention". The field is `pub` so the integration PR can push into the node `descend_mut` returns; the type does not enforce intermediate- node emptiness, so the docs shouldn't claim it does. Verification: cargo fmt clean, clippy clean, 40 passed / 23 ignored / 0 failed (counts unchanged from the original PR).

jensholdgaard · 2026-05-13T14:19:31Z

Addressed all four Copilot inline comments in 8ade0ea:

Hot-path allocation (line 169 → ~190). descend_recursively now uses contains_key(&str) + get_mut(&str), falling back to insert only on first-observation of a prefix shape. Steady state hits zero String allocations per descend. Std's Entry API requires an owned key, so lookup-then-insert is the standard stable workaround; the second get_mut is one extra hash, never an extra allocation.
Inverted doc wording (line 132). Short lines bottom out shallower, not deeper — fixed.
Leaf::id → Leaf::template_id (line 81). Matches RFC 0001 §6.1 language; disambiguates from the template_version, slot-id, and alias-id fields that the integration PR will add. Field doc clarifies the cluster-wide-unique allocator contract.
PrefixNode "by construction" → "by convention" (line 93). The field is pub, the type doesn't enforce empty leaves on intermediates; doc now reflects that and notes the upcoming integration PR is the legitimate caller of pub leaves.

Verification: cargo fmt --all --check clean, cargo clippy --all-targets --all-features -- -D warnings clean, cargo test --all-features 40 passed / 23 ignored / 0 failed (counts unchanged).

Polish suggestions from the second-reviewer team verdict (speculative Clone/PartialEq derives on tree nodes, a descend_mut_default() convenience, pub use tree::{...} re-exports) were declined per CLAUDE.md §6.4 — no current test or caller needs them, and the integration PR will inform whether they're actually wanted.

Four substantive Copilot inline comments on the tree skeleton: - `descend_recursively` allocated a `String` on every level even for hot-path lookups where the child already exists. Switch to `contains_key`-then-`get_mut`, falling back to `insert` only on first-observation. `std::collections::HashMap`'s `Entry` API needs an owned key, so lookup-then-insert is the standard workaround on stable; the second `get_mut` is one extra hash, never an extra allocation. - Doc wording on `Tree::descend_mut` was inverted — short lines bottom out at a *shallower* prefix level, not deeper. Fixed. - Renamed `Leaf::id` → `Leaf::template_id` to match RFC 0001 §6.1 language and disambiguate from forthcoming `template_version`, slot-id, and alias-id additions. Doc clarifies the field's cluster-wide-unique allocator contract. - Softened `PrefixNode` "by construction" → "by convention". The field is `pub` so the integration PR can push into the node `descend_mut` returns; the type does not enforce intermediate- node emptiness, so the docs shouldn't claim it does. Verification: cargo fmt clean, clippy clean, 40 passed / 23 ignored / 0 failed (counts unchanged from the original PR).

Wires `MinerCluster::ingest` to the prefix tree skeleton from PR #17. `MinerCluster::TenantState`'s `HashMap<Vec<String>, u64>` placeholder is gone; per-tenant templates now live in a `Tree`, walked via the read-only `Tree::descend` for the existence-check phase of an ingest and `Tree::descend_mut` for the allocate-and-insert path. Attach decision: **exact-match only** — a candidate line attaches to a leaf when `sim_seq(line, leaf.template) == 1.0`; anything else creates a new leaf in the same `(length, prefix)` bucket. The §6.2 step-5 widening branch and its §3.1 audit-event invariant are deferred to the next PR; widening + audits land together because §3.1 forbids merges without an audit event, so they cannot be split. Hazard #1 invariant story: this PR does no merges. Every line either matches an existing template exactly or creates a fresh leaf. §3.1 "no silent template merges" is preserved vacuously. The new locking test `ingest_creates_separate_leaves_for_near_match_under_same_parent` pins this no-widening behaviour explicitly so the next PR's review surfaces the contract change rather than silently editing it away (CLAUDE.md §6.2 "Tests are specifications"). Tree-module additions (alongside the existing `descend_mut`): - `Tree::descend(&self, masked, depth) -> Option<&PrefixNode>` — read-only counterpart for the lookup phase of an ingest. - `Tree::leaf_count(&self) -> usize` — backs cluster `template_count`. - `Tree::collect_leaves(&self) -> Vec<&Leaf>` — backs cluster `templates_for`. Module docs on both `tree.rs` and `cluster.rs` updated to reflect the new state (tree no longer described as "skeleton with no caller"; cluster no longer described as "exact-match HashMap placeholder"). `docs/roadmap.md` §3 refreshed: 6/29 §5 scenarios green; the miner now has tree + descend + cluster integration; the C2 blocker is reduced to "widen + best-candidate selection," the tree itself is done. §5 scenarios flipped: 0 (no new acceptance criteria pass; this PR is foundational wiring). Outer-loop test count: 40 → 48 passed (8 new — 7 tree, 1 cluster locking test); 23 ignored unchanged. Verification: - `cargo fmt --all --check` — clean - `cargo clippy --all-targets --all-features -- -D warnings` — clean - `cargo test --all-features` — 48 passed / 23 ignored / 0 failed - `mdbook build` — clean (`docs/roadmap.md` touched)

…tch (#19) * feat(miner): route MinerCluster through Drain tree + sim_seq exact-match Wires `MinerCluster::ingest` to the prefix tree skeleton from PR #17. `MinerCluster::TenantState`'s `HashMap<Vec<String>, u64>` placeholder is gone; per-tenant templates now live in a `Tree`, walked via the read-only `Tree::descend` for the existence-check phase of an ingest and `Tree::descend_mut` for the allocate-and-insert path. Attach decision: **exact-match only** — a candidate line attaches to a leaf when `sim_seq(line, leaf.template) == 1.0`; anything else creates a new leaf in the same `(length, prefix)` bucket. The §6.2 step-5 widening branch and its §3.1 audit-event invariant are deferred to the next PR; widening + audits land together because §3.1 forbids merges without an audit event, so they cannot be split. Hazard #1 invariant story: this PR does no merges. Every line either matches an existing template exactly or creates a fresh leaf. §3.1 "no silent template merges" is preserved vacuously. The new locking test `ingest_creates_separate_leaves_for_near_match_under_same_parent` pins this no-widening behaviour explicitly so the next PR's review surfaces the contract change rather than silently editing it away (CLAUDE.md §6.2 "Tests are specifications"). Tree-module additions (alongside the existing `descend_mut`): - `Tree::descend(&self, masked, depth) -> Option<&PrefixNode>` — read-only counterpart for the lookup phase of an ingest. - `Tree::leaf_count(&self) -> usize` — backs cluster `template_count`. - `Tree::collect_leaves(&self) -> Vec<&Leaf>` — backs cluster `templates_for`. Module docs on both `tree.rs` and `cluster.rs` updated to reflect the new state (tree no longer described as "skeleton with no caller"; cluster no longer described as "exact-match HashMap placeholder"). `docs/roadmap.md` §3 refreshed: 6/29 §5 scenarios green; the miner now has tree + descend + cluster integration; the C2 blocker is reduced to "widen + best-candidate selection," the tree itself is done. §5 scenarios flipped: 0 (no new acceptance criteria pass; this PR is foundational wiring). Outer-loop test count: 40 → 48 passed (8 new — 7 tree, 1 cluster locking test); 23 ignored unchanged. Verification: - `cargo fmt --all --check` — clean - `cargo clippy --all-targets --all-features -- -D warnings` — clean - `cargo test --all-features` — 48 passed / 23 ignored / 0 failed - `mdbook build` — clean (`docs/roadmap.md` touched) * refactor(miner): flatten ingest lookup into Option/Iterator chain The Phase-1 read-only lookup in `MinerCluster::ingest` was nested three deep (`if let Some(state)` → `if let Some(parent)` → `for leaf in &parent.leaves`). Replace with an `Option::and_then` + `Iterator::find` + `Option::map` chain so the data flow reads top-to-bottom and the conditional return sits at the same level as the rest of the function. Behaviour-preserving and zero-cost: `and_then`, `find`, and `map` are all `#[inline]`-able std methods, and rustc inlines them with the closures in release builds — the chain compiles to the same control flow as the original imperative form (match on Option discriminant → match on Option discriminant → loop with break on first match → field access). No allocation, no dyn dispatch, no extra Option construction that survives. Verification: cargo fmt clean, clippy clean, 48 passed / 23 ignored / 0 failed (counts unchanged). * fix(miner): address PR #19 review — empty input + O(1) template_count Three substantive Copilot inline comments on the cluster/tree integration: 1. **Empty-input panic (cluster.rs:121,122 + tree.rs:181).** `tokenize` documents and tests inputs with no tokens (`""`, whitespace-only); `mask` preserves length, so a public `MinerCluster::ingest` call with such a line tripped `Tree::descend`/`descend_mut`'s `N ≥ 1` precondition and panicked. `ingest` now short-circuits on empty masked tokens and returns the new `NO_TEMPLATE` sentinel (`0`) — the value `MinerCluster::new` already reserved for "no template allocated" per its existing `next_template_id: 1` comment. Placeholder for the `parse_failures_total` metric path that lands with the §6.3 confidence-zone branching PR. 2. **`template_count` O(1) regression (cluster.rs:172).** The pre-tree `HashMap::len()` was O(1); the post-tree `Tree::leaf_count()` walks the whole tree on every call. Add a `template_count: usize` cache to `TenantState`, incremented after each leaf push in Phase 2. Reading from the cache restores O(1). `Tree::leaf_count` stays for tree-level introspection (still used by `tree::tests`); the cluster simply doesn't use it on the read path. 3. **Misleading panic message (tree.rs:181).** "tokenize+mask guarantee N ≥ 1" was factually wrong — tokenize explicitly supports zero-token inputs. Trimmed to the precondition statement alone (`should_panic` test expectations still match — they assert the substring "must be non-empty"). Two new tests: - `ingest_returns_no_template_sentinel_for_empty_input` — pins the `""` and `" \t\n"` paths against `NO_TEMPLATE` and confirms `template_count` stays 0. - `template_count_grows_with_each_distinct_template` — pins the cache invariant: 3 ingests, 2 distinct shapes, count == 2. Verification: cargo fmt clean, clippy clean, 50 passed (was 48; +2 new cluster tests) / 23 ignored / 0 failed.

…aintainer-gated fold-in (#494) * docs(bench): rfc 0031 §9 comparative entry draft (runs #8–#17) §9.13 compiles the RFC 0031 comparative program's honest-metric era (runs #8–#17 on corpus/otel-demo-v8 vs digest-pinned Loki 3.5.3): L1 and L3 provisional must-win passes on both channels, L2 parity-plus storage-side with named levers, the time-window losses published, the Loki flag deviations and nondeterminism recorded, and the §7 freeze inputs listed as open maintainer decisions. Fold-in is maintainer-gated; this is the draft. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(bench): rfc 0031 §9.13 — auditability round: full digest, run #16 row, scoped determinism Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — every quoted ratio carries its raw loki bytes Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — l2 ledger carries its raw loki bytes too Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — full reproduction rows, streaks audit from the entry alone Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — l2 reproduction rows back the quoted band Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — run #13's salvaged pairs are counted; say so Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — deviation flags spelled exactly as passed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — bytes floor labeled as analog of the latency gate; .11 citation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — bloom provenance cites impl + amendment; full run table Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — run #18 latency channel: rfc0031.7 passes as written Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 §9.13 — heading spans #18; pass claim scoped to counted runs Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

…run #21) (#528) The §7 deferral's named condition is met: RFC 0033's v2 compressed template map merged (#522) and comparative run #21 measured every pair warm at 187,904 B acquisition vs the 513,862 B fold (§9.15). M_L2 freezes per channel, derived from the run record: - processed (primary): M_L2 = 10 — measured 32.5–39.3× across §9.13 runs #10–#17, 37.3–45.1× on the post-artifact total. - storage-side: a 1.1× floor, integer-exact as ourios × 11 ≤ loki_storage × 10 (m_l2_storage_floor_tenths = 11). The post-artifact honest total (0 + 2,035,267 + 187,904 = 2,223,171 B) computes to 1.20–1.51× against the recorded Loki storage band; 1.1 sits below the weakest point with margin for Loki's documented chunk-boundary wobble. A parity-plus floor, not a 10× claim — the write-side lever stays recorded, not chased. Harness: frozen_gate_failures() gates the L2 pair on both channels in the existing salvage ordering; scenario RFC0031.3 un-stubs green in the .2/.4 style (boundary math + record-derived evidence, including the pre-artifact total correctly failing the floor). The RFC 0033 §5.6 corpus acquisition gate (warm ≤ fold/2 when a warm pair exists) also asserts in the dispatch run — fold from a cold pair when one exists, one off-timed-path refold in the all-warm steady state, loudly non-evaluable on refold failure. bytes_must_win_tenths carries the sub-integer floor with the lgates honesty guards (zero ⇒ Invalid, overflow fails closed on the correct side), unit-tested at the boundaries. M_L4 and F_L7 deferrals are untouched. Merge is gated on a fresh dispatch (run #23) from this branch proving the new assertions pass. Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Run #17 validated the completeness-margin design and immediately refined it: the poll-completion check passed cleanly (1153/1197, 96.3%), but the equivalence check then hard-failed on a single cell landing 1 row OVER Ourios's count (114 vs 113 for one bucket/value) while the aggregate total stayed a solid under-count — consistent with the same step-grid boundary imprecision already characterized (a record landing in an adjacent bucket), not fabrication. The original compare_aggregations_within_margin checked "Loki > Ourios" per cell, which was too strict for that kind of noise. Refined to check for phantom cells (a (bucket, group_key) Loki reports that Ourios's own answer doesn't contain at all) and Loki's TOTAL exceeding Ourios's total instead — this still catches the failure mode that would actually indicate a bug (wrong regex or wrong bucket math would produce cells Ourios never produced at all, or push the total over) while tolerating single-cell boundary noise on keys both systems agree exist. Added a test for the exact run #17 shape (a single cell over, total still under, must pass) alongside the existing phantom-cell and net-overcount tests (renamed from "overcount" now that the check is total-level). RFC 0031 §7's L4 entry updated with the refinement and its rationale. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, ourios-bench lib tests (33 passed, +1 new) + local rfc0031_comparative integration tests (40 passed), mdbook build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

PR #536's code review (14 fresh findings across Copilot + CodeRabbit's post-run-18 passes) surfaced one substantive correctness gap and several real documentation/robustness issues in the completeness- margin work. All verified against current code before fixing. Substantive fix — cross-key redistribution gap (CodeRabbit, Major): compare_aggregations_within_margin's grand-total-only check (from the run #17 fix) let Loki over-count one group_key while under-counting another by the same amount and still read as 100% complete: Ourios {A: 100, B: 100} vs Loki {A: 190, B: 10} sums to a "complete" 200/200 while hiding A being fabricated to compensate for B being nearly lost. Refined to aggregate ourios/loki BY group_key first (summing each key across every bucket it appears in), then apply the phantom/overcount/margin checks per-key. This still tolerates run #17's exact shape (a single bucket's +1 doesn't change a key's own total across its buckets) while rejecting the redistribution a pure grand-total check missed. Added regression tests for both shapes. Also populates real per-key examples in mismatch reports (Copilot: the old design returned examples: Vec::new() on both mismatch paths, despite the function accepting examples_cap and RFC0031.1 calling for example keys on a failed comparison). Documentation/robustness fixes (all verified against current code, none required a runtime-behavior change beyond the fix above): - Two stale comments still asserted Loki's same-(timestamp, body) ingester dedup as the shortfall's mechanism, contradicting the nearby docs that say this was directly disproven and the true mechanism is uncharacterized (Copilot, 6 threads pointing at 2 real sites: the frequency_shape_rejection rejection message and one test comment — the other 4 threads were already-accurate historical narrative, verified and left alone). - Missing `//` justification comment on one #[allow(cast_precision_loss)] (CodeRabbit). - L4 picker silently continues when no viable candidate exists (l4_spec.is_none()) — only an eprintln, no run failure, despite L4 being a must-win class (CodeRabbit). Now pushes into `failures`. - PR description inaccurately described L4's equivalence assertion (exact compare_aggregations, ordered after the frozen gates) — rewritten to match actual behavior (margin-based, before the frozen gates, matching the run #11 salvage design already documented inline). - RFC 0031 §7's L4 entry claimed the picker "prefers the lowest- frequency viable candidate" — the actual algorithm is first-fit in ascending (template_id, param) order, not an exhaustive ranking (CodeRabbit, tagged Heavy Lift). Reworded to describe actual behavior and the deliberate scope decision (a real ranking pass would cost a query per candidate against a corpus with tens of thousands of templates; not built given first-fit has now found a validated candidate three real dispatches running). - Double-backtick delimiters for a LogQL code span containing literal backticks (CodeRabbit, MD038). Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, ourios-bench lib tests (34 passed, +2 new) + local rfc0031_comparative integration tests (40 passed), mdbook build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* feat(bench): rfc 0031 l4 — wire into the live dispatch loop Live-wires PairClass::L4 into rfc0031_indicative_comparative_run, the #[ignore]d container-based dispatch test. The previous slice proved the L4 machinery (ourios_aggregate_answer, parse_loki_matrix, pick_frequency_pair, compare_aggregations) only at the fixture level, against a hand-built Loki matrix response — this slice makes it real against a running Loki container and the actual corpus. L4 is picked and measured as its own step, kept OUT of the `Picks`/`specs: Vec<PairSpec>` pipeline the L1/L2/L3/L6 classes share: an aggregation's (bucket, group) -> count map is not a LineKey multiset, and forcing it through OuriosAnswer/compare_lines would misrepresent the state rather than model it (the same "make invalid states unrepresentable" reasoning the miner/parquet layers already follow). Concretely: pick_frequency_pair runs post-store-build like pick_template_pair; its PairSpec is built with the exact dsl/logql shape the fixture-level test already pinned; loki_query_matrix issues a real query_range metric call with `step` pinned to the bucket width so evaluation instants land on parse_loki_matrix's documented bucket-alignment convention (t = bucket_start + width); loki_measure_frequency_pair polls it to completeness the same way loki_measure_pair does for line-returning pairs. Both share the same Loki container and corpus replay as the existing pairs. Equivalence-required-but-bytes-unasserted: RFC0031.1 (result-set equivalence) is never optional, so run_l4_pair asserts compare_aggregations(...).is_equal() unconditionally — an L4 mismatch fails the run exactly like every other class's equivalence check. Only the bytes RATIO stays unasserted (M_L4 is still §7-DEFERRED, no frozen margin to gate against yet): print_l4_report reuses print_pair_bytes_gates, which already prints L4's ratio with no verdict. L4 is measured, equivalence-checked, and reported LAST — after the L1-L3/L6 evidence has printed and their frozen gates have already asserted — so an L4-only failure cannot destroy that evidence (the same run #11 salvage lesson the rest of the harness follows). A missing candidate is reported loudly at pick time, never silently skipped. Purely additive: class_pair_specs, build_pair_specs, frozen_gate_failures, print_pair_bytes_gates, print_indicative_report, PairSpec, and PairClass are unchanged — no frozen-gate behavior for L1/L2/L3/L6 is touched. Verification: cargo fmt --all --check, cargo clippy --all-targets --all-features -- -D warnings (workspace), cargo nextest run -p ourios-bench (165 passed, 7 skipped) and cargo test -p ourios-bench --all-features all green, including the untouched fixture-level rfc0031_5_l4_frequency_aggregation_bytes. The corpus-scale dispatch test itself needs Docker + OURIOS_COMPARATIVE_CORPUS, neither available in this sandbox — its first live proof is the comparative-bench dispatch workflow, same as every other slice in this harness's history. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — backtick-delimit the loki regexp argument The dispatch's first-ever run failed: capture_regex's own Go RE2 escapes (\s+, \S+) were embedded inside a double-quoted LogQL string literal, which tried to interpret those backslashes as its own escape sequences (\s is not a valid one) and Loki rejected the query with "invalid char escape" before the pattern reached the regex engine. Fixed by switching to a backtick-delimited (LogQL/Go raw string) regexp argument, which passes the pattern through literally. Extracted the duplicated PairSpec-construction block (present independently in the fixture test and the live-wiring loop) into one shared l4_pair_spec helper, closing the drift risk and centralizing the fix. Added a backtick guard: a capture_regex containing a backtick (regex_escape does not escape backticks) would prematurely close the raw string, so the candidate is now rejected loudly instead of emitting a malformed query. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — measure L4 before, not after, the L1-L6 failure asserts The second dispatch run failed on the pre-existing, documented L3 Loki-side flake (0 of 9 rows before timeout) — but the run never even attempted L4: the failures.is_empty() assert for L1-L6's own salvaged measurement failures sat textually BEFORE the L4 measurement/report code, so any earlier pair's failure aborted the test before L4 was ever reached. This inverted the design intent (an L4-only failure should not destroy L1-L6 evidence, not the other way around). Moved L4's measurement to run immediately after the report prints, before the gate/failures assertions. run_l4_pair now pushes a Loki-side measurement failure (flake) into the same failures vec the other classes salvage into, instead of panicking immediately — so a flaky L4 measurement no longer aborts before the L1-L6 evidence is captured, symmetric with the fix for the reverse direction. A genuine L4 equivalence MISMATCH still hard-panics immediately, unchanged: RFC0031.1 equivalence is never optional, matching L1-L6's own compare_lines assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — cap the picker's row-count ceiling at 100K The third dispatch got past the control-flow fix and genuinely measured L4 — but the picked candidate (a service's dominant, near-catch-all template) summed to ~971K matching rows, and Loki returned only 811,775 of them before the 300s poll deadline (the same budget every other class's loki_measure_pair uses). L4_MIN_ROWS was a floor with no ceiling, so the picker had no reason to prefer a smaller, still-meaningful candidate. Added L4_MAX_ROWS=100_000 (comfortable margin at the observed ~2.7K rows/s Loki throughput) to frequency_shape_rejection, so the picker moves on to a candidate the poll can actually finish measuring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — disable loki's query-range results cache Run #4's L4 pair plateaued at 11,053/11,523 rows across every 10s poll instead of climbing to completeness. The picker's row ceiling (run #3's fix) had already ruled out "too large to finish in time" — the count never moved at all, which points at a cache serving the same stale answer on every retry rather than a slow ingest. Loki's bundled local-config.yaml enables the embedded results cache for query_range's metric/matrix path (L4's loki_query_matrix), keyed by the query+start+end+step tuple that loki_measure_frequency_pair repolls unchanged. The first (still-incomplete) response gets cached and echoed back on every subsequent poll. Plain log queries (loki_query_range, used by L1-L3/L6) aren't extent-cached the same way, so they self-heal across polls untouched by this. -query-range.cache-results=false trades Loki's own query latency for correctness of the harness's completeness poll — in Loki's favour, same as the other operator-tuning flags already on this container. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — correct the results-cache disable flag name Run #5 never got past container startup: `-query-range.cache-results` doesn't exist ("flag provided but not defined"), so Loki's /ready check timed out on a container that failed to start at all. Checked the pinned v3.5.3 source directly instead of guessing again: queryrangebase.Config.CacheResults is registered under the `querier.` flag prefix in roundtrip.go, not `query-range.`. Correct flag is -querier.cache-results=false. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — widen the loki poll deadline to 900s Run #6 (with the corrected -querier.cache-results=false flag from the prior commit) proved the results-cache theory wrong: L1-L3/L6 all measured cleanly, but L4 still plateaued — 10752/11523 rows (93.3%), even slightly worse than run #4's 95.9% pre-fix, and the shortfall varies run to run rather than repeating a fixed cached answer. That points at genuine, variable completion time rather than a bug: L4's LogQL runs a `| regexp` capture over every candidate line before grouping and counting, a real per-line cost the other classes' plain stream/count queries never pay. Widened loki_measure_frequency_pair's deadline from 300s to 900s — well inside the CI job's unset (360 min default) timeout given the whole run has taken ~95-100 min so far. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — raise loki's max-entries-limit, revert deadline theory Runs #4/#6/#7 all converged L4 to ~93-96% of expected rows, independent of poll deadline (300s vs 900s made no measurable difference) — ruling out both a results-cache echo (already disabled in #fe5915a) and a "just needs more time" theory (the deadline widening from the prior commit). A stable, time-independent shortfall points at something being permanently excluded, not merely delayed. Pulled the frozen otel-demo-v8 corpus locally and checked every log line matching the L4 pair's needle ("Wrote producer snapshot at offset") against its capture regex directly: all 11,525 matches parse cleanly. The regex/content isn't the problem — some matching lines are never being scanned at all. That points at Loki's default -validation.max-entries-limit (5000): count_over_time with a |regexp stage has to scan every raw kafka log line in a query-frontend split before the line filter narrows it down, and kafka's per-split volume exceeds 5000 lines often enough to silently truncate the scan before every matching line is reached. Raised the limit well past the corpus's noisiest single template's volume (~971K rows). Reverted the 900s deadline back to 300s (matching loki_measure_pair) — the widened deadline never addressed the actual bottleneck, and keeping it would misattribute the fix in a way that'd mislead the next reader. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — widen poll deadline now the entries cap is gone Run #8 (max-entries-limit raised) moved L4 from a hard ~93% plateau to 97.1% (11192/11523) — real progress, and unlike runs #4/#6/#7 the remaining gap now plausibly behaves like genuine ingest settle time rather than a fixed ceiling, since the artificial cap that made the prior 300s vs 900s test inconclusive is gone. Widened the deadline to 600s to test that directly before assuming a third factor is at play. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — revert unhelpful deadline widening, add diagnostics Run #9 (600s) measured 96.5% (11123/11523), statistically the same as run #8's 97.1% at 300s — deadline widening does nothing here, so the remaining shortfall after the entries-limit fix is a second stable cap, not settle time. Reverted the deadline back to 300s to match loki_measure_pair rather than keep an unjustified change. Wired the existing dump_loki_diagnostics helper (already used by loki_measure_pair on a deadline miss) into loki_measure_frequency_pair too — it's built around spec.logql + stats parsing, which is query-shape-agnostic, so it works unmodified for the matrix path. If L4 still falls short, the next run's failure carries the raw Loki stats (chunk-fetch counts, any warnings) instead of another guess. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — epoch-align the loki query window to bucket boundaries Run #10's diagnostics (wired in the prior commit) confirmed the L4 query itself is well-formed and Loki answers it successfully — no error, no chunk-fetch shortfall visible in the sampled response. That, combined with runs #6-#10 all converging to a stable ~93-97% regardless of poll deadline (300s/600s/900s all statistically indistinguishable), rules out both a timing race and a malformed query. The real mismatch: Loki's query_range evaluates a step-grid starting exactly at `start` (start, start+step, ..., end), but Ourios's own bucket(width) semantics are epoch-aligned (floor(ts/width)*width) — `min_effective_time_unix_nano` (the corpus's raw earliest timestamp) has no reason to already be a bucket-boundary multiple. Unless (end - start) is an exact multiple of the bucket width, the step-grid leaves a fractional sliver at the tail of the range with no evaluated window covering it at all — real, ingested, settled data that's simply never queried, independent of poll duration. That's exactly the shape every run has shown. Snap `start` down and `end` up to the nearest bucket-width boundary in l4_pair_spec — costs nothing (no data exists outside [min, max] to inflate the count) and guarantees the step-grid fully covers the range. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, local (non-container) rfc0031_comparative unit tests — 39 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * diag(bench): rfc 0031 l4 — add an ingest-vs-query-side split probe Run #11 (bucket-aligned query window) measured 96.6% (11133/11523) — narrower than the pre-fix ~93% plateau, but still in the same stable band as runs #6-#10, all independent of poll deadline, entries-limit, and now bucket alignment. Six straight dispatches without closing the gap means continuing to guess at query-side LogQL/config knobs isn't warranted anymore. Added a decisive probe: on a deadline miss, loki_measure_frequency_pair now also runs a PLAIN line-filter count (no count_over_time, no regexp) for the same needle + window via the new loki_query_range_uncapped (limit sized to expected_rows, unlike the shared loki_query_range's fixed 5000 cap — which is below this pair's 11523 expected rows and would itself lie about the count). If that plain count also falls short by the same margin, the shortfall is ingest-side (Loki never stored those lines) and no further query tuning will fix it; if it's ~complete, the loss is specific to the aggregation path. Diagnostic-only change — no behavior change to the measurement itself, just evidence gathering on the existing failure path. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, local (non-container) rfc0031_comparative unit tests — 39 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — reject high-frequency candidates prone to loki dedup Run #12's decisive diagnostic confirmed the L4 shortfall is ingest-side: a plain unaggregated line-filter count for the same needle+window came back just as short (11160/11523) as every aggregation-path attempt. Loki's ingester silently drops a log entry that collides with another on (timestamp, body) within the same stream — a drop invisible to the OTLP push response's partial_success (push_otlp already asserts that field is clean on every push in every run so far). No query-side fix was ever going to close this gap; the picker was choosing a candidate Loki structurally can't ingest identically. kafka's template_id=16 ("Wrote producer snapshot") fires roughly every 15s. A local exploration against the real frozen corpus (offline, no Loki container — pick_frequency_pair only touches Ourios's own pipeline) found candidates at much lower frequency clear of the same floors: template_id=60 ("Periodic task") at ~144s average cadence, ~10x the failing candidate's margin. Added L4_MIN_AVG_INTERVAL_SECONDS (100s) to frequency_shape_rejection as a durable picker rule, not a one-off override — this protects any future re-run of the picker against landing on another collision-prone high-frequency template, not just this specific dispatch. Updated two pre-existing tests (pick_frequency_pair_finds_a_moderate_ cardinality_group, rfc0031_5_l4_frequency_aggregation_bytes) whose synthetic sub-3s timelines — convenient for test speed, not meant to model real timing risk — tripped the new floor; scaled their timestamps 1000x (preserving cardinality/row-count/needle assertions unchanged) so they represent a realistic, non-collision-prone example. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, local (non-container) rfc0031_comparative unit tests — 40 passed (39 prior + 1 new: frequency_shape_rejection_enforces_the_average_ interval_floor). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * diag(bench): rfc 0031 l4 — dump loki's own container logs on a deadline miss Run #13's lower-frequency candidate (template_id=60, ~144s average cadence) still fell short (1144/1197), and a corpus-side check ruled out the leading theory entirely: every one of the 1197 matching records has a UNIQUE timestamp AND a unique body (verified via jq against the frozen otel-demo-v8 corpus locally) — zero exact (timestamp, body) collisions possible. Loki's documented dedup rule cannot be the mechanism here, which means it likely wasn't the full story for the prior candidate either, even though lowering the frequency floor did measurably help (17.5% loss -> 4.4% loss). Also checked push_corpus_to_loki/push_otlp end to end for a harness- side drop: the batching loop appends every non-empty corpus line's resource_logs to `pending` before any flush, with a final flush after the read loop — no line is skippable, and push_otlp's retry resends the identical Bytes payload, so no bug found there either. Everything checkable from the client side (query responses, corpus content, our own push code) is now ruled out or confirmed clean. The next place to look is Loki itself: on a deadline miss, loki_measure_frequency_pair now also dumps the Loki container's own stderr, filtered to warn/error/drop/reject/rate-limit/stream-limit lines — the ingester logs these for exactly the mechanisms still on the table (rate limiting, out-of-order rejection, stream-limit drops), none of which are visible in a query response or push_otlp's already- clean partial_success check. Diagnostic-only — no change to measurement behavior. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, local (non-container) rfc0031_comparative unit tests — 40 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * diag(bench): rfc 0031 l4 — scrape loki's discarded-samples metrics Run #14's level=warn/level=error stderr scan came back with zero matches in 6618 total lines — whatever is causing the L4 shortfall (still 1147/1197 with the lower-frequency candidate), Loki doesn't consider it log-worthy. That rules out rate limiting, out-of-order rejection, and stream-limit drops as commonly logged at WARN. (The first attempt at the stderr filter was a naive "contains warn" substring match, which drowned in false positives from query text like `severity_text="WARN"` appearing inside level=info lines — fixed to match on the `level=` field precisely.) Loki's distributor increments loki_discarded_samples_total/ loki_discarded_bytes_total (labeled by reason) even for discards that don't warrant a log line — its own dedicated counter for exactly this question. Added dump_loki_discard_metrics, scraping /metrics on a deadline miss (extracted as its own function, alongside dump_loki_diagnostics, to keep loki_measure_frequency_pair under clippy's line-count lint). Diagnostic-only — no change to measurement behavior. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, local (non-container) rfc0031_comparative unit tests — 40 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * feat(bench): rfc 0031 l4 — documented completeness margin (§7, 2026-07-17) Sixteen dispatches (runs #1-#16) exhausted every mechanism checkable from the harness's side without ever reaching exact L4 completeness. Runs #13-#16, specifically, ruled out: query-shape artifacts (a plain line-filter count matched the aggregation-path shortfall exactly), Loki's documented same-(timestamp,body) dedup (zero exact collisions found via direct corpus analysis), interleaving between a genuine mid-corpus kafka restart's two service-instance periods (cleanly sequential), a harness-side push/batching bug (read end to end, none found), anything Loki logs at WARN/ERROR (zero matches bar one harmless startup transient), and Loki's own discarded-samples Prometheus accounting (zero discards of any kind, any reason). This matches an open, unresolved upstream Loki issue (grafana/loki#10658 and related): wide-time-range queries silently missing a small, consistent percentage of lines, with no error, no discard signal, and no maintainer-identified root cause. It's a documented, external, currently-unfixable characteristic of the comparison partner, not an Ourios or harness defect. Adds L4_COMPLETENESS_MARGIN = 0.90 (real headroom over the observed 3.9-4.4% loss band) and compare_aggregations_within_margin — narrowly scoped: it still hard-fails, at any margin, on Loki reporting MORE than Ourios for any cell or a cell absent from Ourios's own answer, the two signals that would actually indicate a correctness bug. Only aggregate under-counting up to the margin is tolerated. compare_aggregations (exact) is untouched and still gates the RFC0031.5 fixture-level test's synthetic Loki answer. Wires the margin into both loki_measure_frequency_pair's poll-complete threshold (accept short-of-exact within margin instead of always retrying to a hard timeout) and run_l4_pair's equivalence assertion. RFC 0031 amended: RFC0031.1's L4 clause now states the margin explicitly, and §7's L4-query-shape entry (previously open) is closed with the full evidence trail and the margin decision. M_L4 (bytes-read) stays deferred — this unblocks a measurement, it doesn't freeze that margin. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, ourios-bench lib tests (32 passed, comparative module) + local rfc0031_comparative integration tests (40 passed), mdbook build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — margin check is total-level, not per-cell Run #17 validated the completeness-margin design and immediately refined it: the poll-completion check passed cleanly (1153/1197, 96.3%), but the equivalence check then hard-failed on a single cell landing 1 row OVER Ourios's count (114 vs 113 for one bucket/value) while the aggregate total stayed a solid under-count — consistent with the same step-grid boundary imprecision already characterized (a record landing in an adjacent bucket), not fabrication. The original compare_aggregations_within_margin checked "Loki > Ourios" per cell, which was too strict for that kind of noise. Refined to check for phantom cells (a (bucket, group_key) Loki reports that Ourios's own answer doesn't contain at all) and Loki's TOTAL exceeding Ourios's total instead — this still catches the failure mode that would actually indicate a bug (wrong regex or wrong bucket math would produce cells Ourios never produced at all, or push the total over) while tolerating single-cell boundary noise on keys both systems agree exist. Added a test for the exact run #17 shape (a single cell over, total still under, must pass) alongside the existing phantom-cell and net-overcount tests (renamed from "overcount" now that the check is total-level). RFC 0031 §7's L4 entry updated with the refinement and its rationale. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, ourios-bench lib tests (33 passed, +1 new) + local rfc0031_comparative integration tests (40 passed), mdbook build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — per-group_key margin, address PR #536 review PR #536's code review (14 fresh findings across Copilot + CodeRabbit's post-run-18 passes) surfaced one substantive correctness gap and several real documentation/robustness issues in the completeness- margin work. All verified against current code before fixing. Substantive fix — cross-key redistribution gap (CodeRabbit, Major): compare_aggregations_within_margin's grand-total-only check (from the run #17 fix) let Loki over-count one group_key while under-counting another by the same amount and still read as 100% complete: Ourios {A: 100, B: 100} vs Loki {A: 190, B: 10} sums to a "complete" 200/200 while hiding A being fabricated to compensate for B being nearly lost. Refined to aggregate ourios/loki BY group_key first (summing each key across every bucket it appears in), then apply the phantom/overcount/margin checks per-key. This still tolerates run #17's exact shape (a single bucket's +1 doesn't change a key's own total across its buckets) while rejecting the redistribution a pure grand-total check missed. Added regression tests for both shapes. Also populates real per-key examples in mismatch reports (Copilot: the old design returned examples: Vec::new() on both mismatch paths, despite the function accepting examples_cap and RFC0031.1 calling for example keys on a failed comparison). Documentation/robustness fixes (all verified against current code, none required a runtime-behavior change beyond the fix above): - Two stale comments still asserted Loki's same-(timestamp, body) ingester dedup as the shortfall's mechanism, contradicting the nearby docs that say this was directly disproven and the true mechanism is uncharacterized (Copilot, 6 threads pointing at 2 real sites: the frequency_shape_rejection rejection message and one test comment — the other 4 threads were already-accurate historical narrative, verified and left alone). - Missing `//` justification comment on one #[allow(cast_precision_loss)] (CodeRabbit). - L4 picker silently continues when no viable candidate exists (l4_spec.is_none()) — only an eprintln, no run failure, despite L4 being a must-win class (CodeRabbit). Now pushes into `failures`. - PR description inaccurately described L4's equivalence assertion (exact compare_aggregations, ordered after the frozen gates) — rewritten to match actual behavior (margin-based, before the frozen gates, matching the run #11 salvage design already documented inline). - RFC 0031 §7's L4 entry claimed the picker "prefers the lowest- frequency viable candidate" — the actual algorithm is first-fit in ascending (template_id, param) order, not an exhaustive ranking (CodeRabbit, tagged Heavy Lift). Reworded to describe actual behavior and the deliberate scope decision (a real ranking pass would cost a query per candidate against a corpus with tens of thousands of templates; not built given first-fit has now found a validated candidate three real dispatches running). - Double-backtick delimiters for a LogQL code span containing literal backticks (CodeRabbit, MD038). Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, ourios-bench lib tests (34 passed, +2 new) + local rfc0031_comparative integration tests (40 passed), mdbook build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — absolute row tolerance, not pure percentage Run #19 (the verification dispatch for the round-1 review fixes) found a real edge case in the per-group_key percentage margin: a group_key with exactly 1 total Ourios row, where Loki captured 0 (0%). A pure ratio has no meaningful middle ground at n=1 — it's binary, 0% or 100% — yet losing one isolated occurrence is fully consistent with the already-characterized ~4-8% aggregate loss rate this whole margin exists to tolerate. Converted the per-key check from a ratio (loki/ourios >= margin) to an absolute row tolerance floored at 1: ceil(ourios_key_total * (1 - margin)).max(1). This tolerates a cardinality-1 key losing its only row while still catching a real shortfall on a large key (100 rows, tolerance 10, losing 20 still rejects) — the phantom-cell and per-key-overcount hard-fail checks are unaffected. Two new regression tests cover both ends. Verified: cargo fmt --all --check, workspace cargo clippy -D warnings, ourios-bench lib tests (36 passed, +2 new) + local rfc0031_comparative integration tests (40 passed), mdbook build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — margin comparator precision, review triage compare_aggregations_within_margin's tolerance formula (ceil(o*(1-margin)) .max(1)) was itself miscalibrated for small-but-not-1 totals, per two independent Copilot review threads (o=2 at 90%: tolerance=1 permits 50% completeness, not 90%). Replace the subtract-then-round row tolerance with a direct, epsilon-guarded comparison — loki_total >= ourios_total * margin — which also sidesteps a second bug the naive floor() fix introduced: 1.0 - 0.9 isn't exactly 0.1 in f64, so floor(40.0 * (1.0 - 0.9)) truncated to 3 instead of 4, tightening the tolerance at exact 90%-boundary cases (caught by the existing margin_comparison_tolerates_undercount_within_margin test's svcB case). Extract phantom_cells and aggregate_by_group_key helpers to bring the function back under clippy's line limit, and add a # Panics section for the margin-validation assert. Fix several accumulated PR #536 review findings: run_l4_pair's doc comment claimed L4 runs after the L1-L3/L6 frozen gates assert (it actually runs before, printing first); three "ingest-vs-query" overclaims (the plain line-filter probe still calls query_range, so it can rule out "specific to the metric-aggregation path" but not prove ingest-side loss); L4_COMPLETENESS_MARGIN's own doc comment still described the superseded total-level design. Add a clarifying comment on l4_pair_spec's step-grid reasoning (the first evaluated instant decodes to an empty phantom bucket, not a lost real one) and fix the RFC's LogQL code span, which kept the backslash escaping needed for single backticks even after switching to a double-backtick delimiter that makes it unnecessary. Reconcile RFC0031.5's must-win predicate with M_L4 staying deferred — add a note that the predicate is the target contract, not currently gated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — panic-safe diagnostic probe, more review triage loki_query_range_uncapped used expect()/assert!() internally, but it runs on the L4 deadline-miss diagnostic path inside the same runtime.block_on that gathers L1-L3/L6's evidence — a panic there (a real Loki error response, a malformed body) would unwind the whole async block and lose all of it, defeating the print-before-assert salvage design (Copilot). Converted to return Result<u64, String> instead of panicking, matching the already-panic-free sibling diagnostics (dump_loki_diagnostics et al.). Also: fix a test comment that said "one row under the ceiling" for a fixture that actually lands exactly at the ceiling; fix an unreachable! message's imprecise invariant claim (the real gating condition is l4_spec.is_some() implies l4_loki.is_some(), not "iff frequency is Some"); document loki_query_matrix's whole-second/bucket-alignment precondition and verify it against l4_pair_spec, its only caller; reorder loki_measure_frequency_pair's deadline-miss diagnostics to run only when the completeness margin is actually missed, not on every deadline-miss regardless of outcome; fix two fixture comments claiming "~300s average spacing" that don't match their own timestamps (actually ~580s) and a comment attributing the L4 shortfall to ingest-side dedup after that theory was directly disproven elsewhere in the same file. Verified the remaining ~40 accumulated review threads (mostly a recurring "shared 300s deadline" doc/code mismatch and the run_l4_pair ordering claim, duplicated across many review rounds) against current code: all already correct, superseded by earlier commits in this investigation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — round-4 review triage on the margin comparator Validate margin at function entry rather than after the phantom/overcount checks, so an invalid margin always panics per the documented contract instead of potentially returning a data-shaped mismatch first (CodeRabbit). Fix the doc comment paragraph still describing the superseded floor-based tolerance (Copilot). Reword the L4-skip diagnostic and failure message to name both reasons l4_spec can be None (picker bounds vs a backtick in the capture regex) and to make clear the skip fails the dispatch rather than reading as benign (Copilot, two sites). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * fix(bench): rfc 0031 l4 — margin=1.0 strictness + panic-safe matrix poll Two Copilot findings on the previous commit, both verified genuine: The cardinality-1 exemption applied at any margin, so a caller passing margin = 1.0 (exact completeness) would still accept Loki returning 0 of 1 for an n=1 key — the exemption now only applies to a genuinely fractional margin, with a regression test covering both directions at 1.0. Bit-identical behavior at the harness's 0.90. loki_query_matrix still used expect/assert internally, so a transient transport error, 5xx, or torn body during the L4 poll — which runs LAST in the same async block holding every other pair's already-collected measurement — would panic and unwind all of it. Converted to Result<L4Measured, String>; the poll loop now retries an Err until its deadline exactly like an incomplete answer, then surfaces it as the pair's failure. Extracted the below-margin shortfall diagnostics into dump_l4_shortfall_diagnostics to stay under clippy's function-length limit. Neither change alters the measured semantics run #23 is currently confirming (the comparator formula is untouched; at margin 0.90 the exemption gating is unchanged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(bench): rfc 0031 l4 — document why the phantom check is cell-level Copilot's latest pass proposed weakening phantom detection from (bucket, group_key) cells to bare group_keys so a boundary-exact record shifting into an empty adjacent bucket can't read as phantom. Declined: a systematic bucket-decode error (every cell shifted one width — the run #11 bug class) leaves every per-key total intact, so the cell-level check is the only guard that catches it, while the false positive it risks requires a nanosecond-exact bucket-boundary timestamp that no real dispatch has ever produced. Documented the trade-off on phantom_cells instead of changing behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

… (§9.29) (#594) * feat(bench): rfc 0036 real-corpus window-materialization before/after (§9.29) §9.26 established that ourios-bench's `build_comparative_store` writes one ingest file per partition, so `compact_partition` no-ops and RFC 0036's compaction-time sort never runs there — the before/after win could not be measured on the frozen comparative harness. §9.27 measured it on a *synthetic* hour (1.43×). This lands the **real otel-demo-v8 corpus** analogue. Adds an opt-in compacted store builder, `build_comparative_store_compacted[_with_threshold]`, that round-robins each partition's rows across two interleaved ingest files then compacts every partition, so the consolidated file is §3.1-clustered by (service.name, time), rotates row groups, and declares `sorting_columns`. The default `build_comparative_store` path — the frozen RFC 0031 dispatch's store — is a separate function and stays byte-for-byte untouched (frozen gates are NOT re-based; a1/c1/c2/reproducibility/rfc0031_comparative all still pass). The measurement (`tests/rfc0036_realcorpus.rs`, `#[ignore]`d, skips with a clear message when the gitignored capture is absent so CI/other machines never fail) builds the store two ways from a v8 subset and runs one L6-shape window query against each, reading materialization bytes (footer survivor-chunk sum, the RFC 0036 §9 metric — not the count-scan `stats.bytes_read`) plus `row_groups_scanned`. Recorded §9.29 (indicative, local, real-corpus subset, Ourios-only, no Loki): 120,000 LogsData batches, service `ad` (the §9.13 run #17 low-volume case), one busy hour. before 3,806,306 B (1/1 groups, whole file) → after 731,521 B (1/7 groups) = 5.20× materialization-bytes win, identical 10,110-row answer. Confirms §9.28 finding 3 on real data: the shipped 32 MiB threshold compacts a real hour to a single group (no prune — the test skips); a finer threshold is what rotates the hour into service-clustered groups (2 MiB → 7 groups 5.20×, 4 MiB → 2 groups 1.32×). Authoritative full-v8 + Loki arm stays deferred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * test(bench): guard compacted-builder debug_assert on files_before + honest doc (review) - The debug_assert assumed every partition holds 2 ingest files, but a partition that saw a single record has one file (round-robin can't split it) and legitimately no-ops — guard on outcome.files_before < 2 so it no longer spuriously panics, while still catching a >=2-file no-commit. - window_service_bytes doc: note it deliberately keys on effective_time_unix_nano (what the comparative querier prunes on), not time_unix_nano like the querier test's namesake. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * test(bench): assert both services survive + are §3.1-sorted, not just row count (review) The compacted-builder test claimed "preserves the row multiset" but only checked the total count — a drop/dup/swap across services would pass. Add a footer service.name min/max assertion (min=svc-a, max=svc-b) proving both services survive and land in §3.1 order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * test(bench): enumerate live files via the manifest, not physical *.parquet (review) committed_parquet_files walked the data tree and counted every *.parquet, but the manifest is the authoritative live set (RFC 0005 §3.9) — a partition can hold orphaned superseded inputs (gc_failures) or a lost-CAS output alongside the live file. Both the compacted-builder unit test's file-count assertion and the §9.29 busiest-partition pick now read manifest.files when a partition dir has a manifest.json (falling back to physical enumeration for pre-compaction leaves), so they match the storage contract rather than a local-only coincidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y * docs(benchmarks): keep §9.29 range(...) inline code on one line (review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

jensholdgaard requested a review from Copilot May 13, 2026 14:05

Copilot started reviewing on behalf of jensholdgaard May 13, 2026 14:06 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

Comment thread crates/ourios-miner/src/tree.rs

Comment thread crates/ourios-miner/src/tree.rs Outdated

Comment thread crates/ourios-miner/src/tree.rs Outdated

Comment thread crates/ourios-miner/src/tree.rs

jensholdgaard force-pushed the feat/miner-prefix-tree-skeleton branch from 8ade0ea to 3b34fdb Compare May 13, 2026 14:29

jensholdgaard merged commit 0c16e33 into main May 13, 2026
7 checks passed

jensholdgaard deleted the feat/miner-prefix-tree-skeleton branch May 13, 2026 20:26

jensholdgaard mentioned this pull request May 13, 2026

feat(miner): route MinerCluster through Drain tree + sim_seq exact-match #19

Merged

17 tasks

jensholdgaard mentioned this pull request Jul 14, 2026

feat(bench): RFC 0031 §7 — M_L2 unfrozen and asserting per RFC 0033 (run #21) #528

Merged

jensholdgaard mentioned this pull request Jul 17, 2026

feat(bench): RFC 0031 L4 — wire into the live dispatch loop #536

Merged

This was referenced Jul 21, 2026

docs(rfc): RFC 0036 — write-side layout: compacted clustering + row-group sizing #584

Merged

feat(bench): rfc 0036 real-corpus window-materialization before/after (§9.29) #594

Merged

jensholdgaard mentioned this pull request Jul 22, 2026

feat(parquet): rfc0036 adaptive compacted row-group threshold (target-K) #595

Merged

Navigation Menu

feat(miner): add Drain prefix tree skeleton (RFC 0001 §6.2 step 3) - #17

feat(miner): add Drain prefix tree skeleton (RFC 0001 §6.2 step 3)#17
jensholdgaard merged 2 commits into
mainfrom
feat/miner-prefix-tree-skeleton

jensholdgaard commented May 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jensholdgaard commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jensholdgaard commented May 13, 2026

Summary

RFC anchor

Hazards / invariants touched

§5 scenarios

Notable design choices

Verification (per CLAUDE.md §6.6)

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jensholdgaard commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Verification (per `CLAUDE.md` §6.6)