Skip to content

feat(miner): reader render emits body+RetainedVerbatim for lossy rows (H7.3) - #163

Merged
jensholdgaard merged 5 commits into
mainfrom
feat/h7-3-reader-render
Jun 9, 2026
Merged

feat(miner): reader render emits body+RetainedVerbatim for lossy rows (H7.3)#163
jensholdgaard merged 5 commits into
mainfrom
feat/h7-3-reader-render

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jun 9, 2026

Copy link
Copy Markdown
Owner

What

Implements RFC 0001 scenario H7.3 against the merged §6.6 Reader render contract (RFC 0001-template-miner.md, amendment 2026-06-08).

The §6.6 amendment defines the per-row warning marker H7.3 references but that §6.6 previously left undefined. This PR adds it to ourios-miner and flips the H7.3 red-gate stub green.

The contract (§6.6 Reader render contract)

New API in crates/ourios-miner/src/reconstruct.rs, alongside reconstruct:

pub enum Reconstruction { Faithful, RetainedVerbatim }
pub fn render(record: &MinedRecord, template: &[OwnedToken]) -> (Vec<u8>, Reconstruction)
  • Reconstruction is the §6.6 warning marker: structured, out-of-band per-row metadata attached beside the rendered row. The body bytes are never mutated — a body-byte sentinel/prefix is explicitly rejected by §6.6 because it would break the verbatim guarantee operators rely on ([§3.3]).
  • Lossy / overflow split. For a String-body row with lossy_flag = true or any ParamType::Overflow param (§6.5), render returns the retained body verbatim with Reconstruction::RetainedVerbatim and does not invoke reconstruct (no template lookup, no token walk). Otherwise (clean String) it calls reconstruct and attaches Reconstruction::Faithful.
  • Body extraction reuses the same body_bytes_or_empty(record) helper the lossy path in reconstruct already uses.

Invariant / hazard coverage

Addresses §3.3 bit-identical body reconstruction / hazard 7: on a lossy row the reader returns the ingested bytes unchanged and reconstruct is never called, so a render-from-template can never silently corrupt "show me what was actually logged."

Scope (per the §6.6 amendment)

  • Structured / Absent bodies are out of scope of this amendment. render is scoped to the String path only and does not invent a Reconstruction classification for structured/absent rows; callers route only String rows through render, and structured/absent keep reconstruct's existing behaviour. Structured-body render/classification stays deferred (RFC0001.9 / canonical-JSON follow-up).
  • Clean-path read-time template registry deferred (RFC 0007). render takes the template slice exactly as reconstruct does today; the read-time (template_id, template_version) → tokens lookup is future work tracked with the querier's reader-materialisation story.
  • RFC 0001 stays specified; this implements one §6.6/§5 acceptance criterion, it does not advance the maturity stage.

Test (H7.3, load-bearing)

crates/ourios-miner/tests/hazards.rs::h7_3_reader_emits_body_verbatim_when_lossy_flag_is_true — stub (#[ignore] + todo!()) replaced with a real AAA test:

  • Arrange: drives the §6.2 tokenizer-failure path (embedded NUL) so the miner emits a String-body record with lossy_flag = true and body = the original bytes.
  • Act: calls render(rec, &wrong_template) where wrong_template = [Fixed("WRONG")] — a template that, if walked, would yield "WRONG", never the body.
  • Assert: returned bytes == raw (NUL included) and marker == RetainedVerbatim. Because the wrong template would produce different bytes, body-equality proves reconstruct was not called — the lossy short-circuit fired first. This is H7.3's "reconstruct is NOT called on a lossy row."

Three unit tests added next to render cover the lossy, overflow, and clean (Faithful) cases.

Verification (run locally from the worktree)

  • cargo fmt --all --check — clean
  • cargo clippy --all-targets --all-features -- -D warnings — clean
  • cargo test --all-features — whole workspace green; H7.3 now passes and is no longer #[ignore]d

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added reconstruction tracking that indicates whether data was faithfully reconstructed or retained verbatim from the original
    • Enhanced handling of string data with lossy or overflow conditions
  • Tests

    • Added test coverage for reconstruction scenarios including lossy and overflow data handling

… (H7.3)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jensholdgaard
jensholdgaard requested a review from Copilot June 9, 2026 06:55
@jensholdgaard

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jensholdgaard, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 1 second. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 68786729-37eb-42d3-b74a-addd8885d3e3

📥 Commits

Reviewing files that changed from the base of the PR and between 5ab2fae and 3bf1110.

📒 Files selected for processing (2)
  • crates/ourios-miner/src/reconstruct.rs
  • crates/ourios-miner/tests/hazards.rs
📝 Walkthrough

Walkthrough

A new public Reconstruction enum and render() function enable callers to distinguish between lossy/overflow string rows (emitting RetainedVerbatim with original bytes) and clean rows (emitting Faithful with reconstructed bytes). Unit and integration tests validate the branching logic and end-to-end behavior.

Changes

Reconstruction Signal Exposure

Layer / File(s) Summary
Reconstruction contract and render implementation
crates/ourios-miner/src/reconstruct.rs
Public Reconstruction enum with Faithful and RetainedVerbatim variants; public render(record, template) function selects between returning retained body bytes (when lossy_flag is true or any ParamType::Overflow is present) with RetainedVerbatim marker, or calling reconstruct() and returning the result with Faithful marker.
Unit test coverage for render behavior
crates/ourios-miner/src/reconstruct.rs
Three unit tests verify render() returns retained bytes with RetainedVerbatim for lossy string rows, overflow string rows, and reconstructed bytes with Faithful for clean string rows.
Integration test for lossy rendering
crates/ourios-miner/tests/hazards.rs
Previously ignored test stub h7_3_reader_emits_body_verbatim_when_lossy_flag_is_true now active; ingests a lossy record with NUL-containing body, calls render() with an intentionally wrong template, and asserts returned bytes equal the original retained body and reconstruction marker is RetainedVerbatim.

Possibly related PRs

  • jensholdgaard/ourios#162: Introduces the out-of-band per-row reconstruction signal and lossy RetainedVerbatim behavior for bypassing reconstruct() on error-state rows.

Poem

🐰 A signal now marks every row's fate—
Lossy or true, it's your choice to slate!
Verbatim retained when the data won't bend,
Faithful reconstructed when math's the best friend. ✨

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main feature: a new render function that emits body+RetainedVerbatim for lossy rows, addressing RFC scenario H7.3.
Description check ✅ Passed The description is comprehensive and addresses all required template sections: detailed what/why, related RFC references, and verification steps. Checklist is present but incomplete (items unchecked rather than marked complete/incomplete appropriately).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/h7-3-reader-render

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai

coderabbitai Bot commented Jun 9, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements RFC 0001 hazard scenario H7.3 by adding a read-time render() API that returns rendered row bytes plus an out-of-band reconstruction marker, and by turning the H7.3 hazard test from a red-gate stub into a real passing test.

Changes:

  • Added Reconstruction marker enum and render(record, template) -> (Vec<u8>, Reconstruction) to short-circuit lossy/overflow rows to retained body bytes.
  • Added unit tests for render() covering lossy, overflow, and clean/faithful cases.
  • Replaced the ignored H7.3 hazard test stub with an end-to-end test that proves render() does not walk the template when lossy_flag=true.

Reviewed changes

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

File Description
crates/ourios-miner/src/reconstruct.rs Introduces Reconstruction and render() plus unit tests enforcing the §6.6 render contract.
crates/ourios-miner/tests/hazards.rs Makes H7.3 a real (non-ignored) hazard test validating verbatim body rendering for lossy rows.

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@crates/ourios-miner/src/reconstruct.rs`:
- Around line 199-212: render currently decides RetainedVerbatim only from
lossy_flag/Overflow but then calls reconstruct which can itself fall back to the
original body; that fallback must also be reported as RetainedVerbatim. Change
render to compute let out = reconstruct(record, template) and if out ==
body_bytes_or_empty(record) return (out, Reconstruction::RetainedVerbatim) else
return (out, Reconstruction::Faithful); update the same logic in the other
identical block referenced (the later 414-474 area) so any reconstruct-produced
body fallback is classified as RetainedVerbatim rather than Faithful.
🪄 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: a14a0b7c-e77f-45c0-a572-9afc2a4778b1

📥 Commits

Reviewing files that changed from the base of the PR and between 202f088 and 5ab2fae.

📒 Files selected for processing (2)
  • crates/ourios-miner/src/reconstruct.rs
  • crates/ourios-miner/tests/hazards.rs

Comment thread crates/ourios-miner/src/reconstruct.rs
…truction

render keyed only off lossy_flag/Overflow, so a clean row whose template
shape mismatched (corrupt row / wrong template lookup) got Faithful even
though reconstruct fell back to the body — breaking the §6.6 contract.
render now checks template_shape_matches_record: Faithful only when the
template is actually walked, else body verbatim + RetainedVerbatim. Add
#[non_exhaustive] to Reconstruction (forward-extensible) + a String-only
debug_assert + a shape-mismatch test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread crates/ourios-miner/tests/hazards.rs Outdated
Comment thread crates/ourios-miner/src/reconstruct.rs Outdated
Comment thread crates/ourios-miner/src/reconstruct.rs Outdated
Comment thread crates/ourios-miner/src/reconstruct.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread crates/ourios-miner/tests/hazards.rs Outdated
…+ docs

render now uses assert! (not debug_assert!) for its String-only public
precondition — a release-build non-String call would otherwise silently
mislabel as RetainedVerbatim (matches sim_seq.rs; # Panics documented).
The H7.3 test keeps a deliberately-wrong template but its comment no
longer claims byte-equality proves reconstruct was uncalled (reconstruct
also returns body for lossy) — it asserts the observable contract and
notes the no-call guarantee is structural. Faithful doc = rebuilt from
template (equality only with the correct template). Shape-mismatch test
sets valid separators to exercise the wildcard/param-count case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread crates/ourios-miner/src/reconstruct.rs
The faithful guard already establishes reconstruct clean-path
preconditions (not lossy, no overflow, shape matches), so call the inner
template walk directly instead of reconstruct — avoiding a redundant
overflow scan + a second shape-check template walk.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread crates/ourios-miner/src/reconstruct.rs Outdated
Comment thread crates/ourios-miner/src/reconstruct.rs
RetainedVerbatim is not always the ingested line: a shape-mismatch
fallback with no retained body yields empty bytes. Reword the enum +
render docs to "rendered from the retained body (when present), ingested
verbatim for lossy/overflow" and "bytes a reader should display
(reconstructed when faithful, else retained body)".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@jensholdgaard
jensholdgaard merged commit 523adaf into main Jun 9, 2026
11 checks passed
jensholdgaard added a commit that referenced this pull request Jun 11, 2026
…ld placeholder

The Debug-rendering interim-placeholder claim and the deferred-to-
storage-layer claim were both stale (false post-#163/#166/#174);
reviewer-flagged, so fixed here rather than the planned follow-up.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants