Skip to content

docs(rfc-0001): define the §6.6 reader-render contract + lossy warning marker - #162

Merged
jensholdgaard merged 6 commits into
mainfrom
rfc0001-reader-render-contract
Jun 8, 2026
Merged

docs(rfc-0001): define the §6.6 reader-render contract + lossy warning marker#162
jensholdgaard merged 6 commits into
mainfrom
rfc0001-reader-render-contract

Conversation

@jensholdgaard

@jensholdgaard jensholdgaard commented Jun 8, 2026

Copy link
Copy Markdown
Owner

What & why

H7.3 (§5) says a lossy_flag = true row renders to the body column verbatim "with the §6.6 warning marker," and that reconstruct() is NOT called. But §6.6 defined no such marker — its reconstruct() just returns body defensively for lossy rows (no marker, and it is reconstruct). This DOC-ONLY amendment closes that gap (maintainer decision 2026-06-08).

The marker design

A new Reader render contract subsection in §6.6 defines the render result as (bytes, reconstruction). The marker is a structured, out-of-band per-row signal attached to the rendered row:

enum Reconstruction {
    Faithful,         // bytes == ingested line, reconstructed from template
    RetainedVerbatim, // bytes are the retained `body` column, not reconstructed
}

This Reconstruction signal is the "§6.6 warning marker." It lives beside the body bytes, never inside them — the consumer (DSL output / UI) renders RetainedVerbatim as a "reconstruction not faithful — original line retained verbatim" warning. A body-byte annotation (sentinel prefix / wrapper char) is explicitly rejected as the marker: it would break the §3.3 verbatim guarantee that "show me what was actually logged" returns the ingested bytes unchanged.

Lossy vs. clean path

  • Lossy path (H7.3): for lossy_flag = true (and the other non-reconstructable cases §6.6/§6.5/§6.1 already list — OVERFLOW params, body_kind = Structured) the reader returns body verbatim with Reconstruction::RetainedVerbatim and does NOT invoke reconstruct (no template lookup, no token walk). reconstruct()'s own lossy/overflow early returns stay as a defensive guard, not the primary mechanism.
  • Clean path: for a faithful row the reader invokes reconstruct(record) and attaches Reconstruction::Faithful.

Explicitly deferred

The clean path needs a read-time template registry ((template_id, template_version) → tokens). That lookup mechanism is a separate concern, out of scope of this amendment — today reconstruct() is exercised only where the template is in hand (write-side H7.1 / H7.4). It is deferred to the querier's reader-materialisation story (RFC 0007). This amendment pins the contract + the lossy path only; it does not invent a template-registry design.

Consistency

  • §3.3 (bit-identical for non-lossy) and the lossy_flag semantics (tokenizer failure / explicit rejection; the §6.3 low-confidence zone stays faithful-reconstructable, not flagged) are preserved.
  • H7.3 §5 wording + the §8 reader-behaviour test updated so "the §6.6 warning marker" resolves to the defined signal, kept testable.
  • RFC 0001 stays specified; §6.6 carries a dated amendment note. No on-disk schema or mining-algorithm change.

Verification

DOC-ONLY (no cargo). mdbook build succeeds; only the benign mdbook-mermaid version warning. §6.6 + H7.3 re-read for internal consistency.

🤖 Generated with Claude Code

Summary by CodeRabbit

Documentation

  • Updated template-miner reader specification with explicit contract for handling lossy data rows
  • Defined reconstruction signals to clarify data integrity marking behavior
  • Formalized reader behavior requirements for data reconstruction and recovery scenarios

…g marker

H7.3 referenced "the §6.6 warning marker" that §6.6 never defined. Add a
Reader render contract subsection defining the marker as a structured,
out-of-band per-row Reconstruction signal (Faithful | RetainedVerbatim) —
not a body-byte mutation — and pinning the lossy short-circuit (return body
verbatim, do not call reconstruct). Reject a body-byte annotation as the
marker (it would break the verbatim guarantee). The clean-path read-time
template registry is explicitly deferred to RFC 0007. Update H7.3 and the
§8 reader-behaviour test to resolve to the defined signal. RFC stays
specified; no schema or algorithm change.

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

Copy link
Copy Markdown
Owner Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 8, 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 25 minutes and 37 seconds. 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: 45c0fe9d-6be1-403e-9806-fac52a7ffebc

📥 Commits

Reviewing files that changed from the base of the PR and between 3e1d729 and c0082bd.

📒 Files selected for processing (1)
  • docs/rfcs/0001-template-miner.md
📝 Walkthrough

Walkthrough

RFC 0001's reader specification is updated to formalize lossy-row handling through an explicit out-of-band reconstruction signal. H7.3 now references a per-row Reconstruction enum; a new §6.6 "Reader render contract" section defines the enum and reader branching behavior; the testing scenario is rewritten to assert the new contract.

Changes

Reader render contract specification for template miner RFC

Layer / File(s) Summary
H7.3 scenario reference to reconstruction signal
docs/rfcs/0001-template-miner.md
H7.3 specification updated to require Reconstruction::RetainedVerbatim signal for lossy rows and to avoid calling reconstruct() for those rows, replacing prior "warning marker" language.
Reader render contract specification and amendment
docs/rfcs/0001-template-miner.md
Amendment note added to clarify prior H7.3 reference, followed by new §6.6 "Reader render contract" section defining Reconstruction enum, rejecting in-band body mutation, and specifying reader short-circuit logic for lossy/overflow/structured rows (return body verbatim with RetainedVerbatim) and clean path (call reconstruct(record) with Faithful).
Reader behaviour test scenario alignment
docs/rfcs/0001-template-miner.md
Testing strategy updated to assert the new contract: verbatim body and Reconstruction::RetainedVerbatim for lossy rows with no reconstruct() call, and Reconstruction::Faithful for faithful rows.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

A rabbit hops through pages old and wise,
Refining signals, banishing the guise—
From warnings whispered deep in bytes so long,
To faithful, verdant reconstructs, bright and strong. ✨🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description provides detailed context (What & why, design details, deferred items, consistency), but lacks the structured checklist sections required by the template. Add the required Checklist section from the template and explicitly address each item (cargo fmt/clippy, tests, docs/CHANGELOG, RFC link status) to match the repository's description standard.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: defining the reader-render contract and the lossy warning marker in RFC 0001 §6.6.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 rfc0001-reader-render-contract

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 8, 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

This PR amends RFC 0001 (§6.6) to explicitly define the reader-side render contract and to clarify what the previously-referenced “§6.6 warning marker” is for lossy rows, without changing schema or mining behavior.

Changes:

  • Updates Scenario H7.3 to specify byte-for-byte body rendering plus an out-of-band reconstruction signal (Reconstruction::RetainedVerbatim) and to reiterate that reconstruct() is not called.
  • Adds a dated amendment note and a new “Reader render contract” subsection to §6.6 defining (bytes, reconstruction) and rejecting in-band body markers.
  • Updates the §8 “Reader behaviour test” description to align with the new render contract and marker definition.

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

Comment thread docs/rfcs/0001-template-miner.md
Comment thread docs/rfcs/0001-template-miner.md Outdated

@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 `@docs/rfcs/0001-template-miner.md`:
- Around line 1503-1508: The fenced code block showing the Reconstruction enum
lacks a language identifier; update the Markdown fence to include a language tag
(e.g., "rust") so the enum declaration for Reconstruction (variants Faithful and
RetainedVerbatim) is syntax-highlighted and rendered correctly.
🪄 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: eeac2991-faaf-4c1a-b2db-fda2d3f41a92

📥 Commits

Reviewing files that changed from the base of the PR and between aa4b903 and 3e1d729.

📒 Files selected for processing (1)
  • docs/rfcs/0001-template-miner.md

Comment thread docs/rfcs/0001-template-miner.md Outdated
jensholdgaard and others added 2 commits June 9, 2026 00:53
…r structured

A body_kind=Structured row renders its canonical body, but its
Faithful-vs-not classification turns on the deferred OTLP-canonical-JSON
decision (RFC0001.9). Scope the §6.6 Reconstruction marker + lossy/clean
split to the implemented String path; structured-body rendering lands
with RFC0001.9.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e marker wording

Addresses review: language-tag the enum code fence; reword the
RetainedVerbatim consumer guidance to "rendered from the retained body
bytes, not reconstructed" (accurate for the lossy + overflow String
paths).

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 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread docs/rfcs/0001-template-miner.md Outdated
Avoid contradicting §6.1 (the canonical-encoding rule is specified) and
the RFC0001.9 scenario: the §6.6 reader-render amendment scopes the
Reconstruction marker to String bodies; structured-body render
classification is out of scope here, specified with RFC0001.9.

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 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread docs/rfcs/0001-template-miner.md Outdated
…ignal

RFC0001.9 covers structured-body rendering (the short-circuit) but does
not define this amendment new Reconstruction marker. Reword: structured
render is exercised by RFC0001.9; mapping a structured render to a
Reconstruction signal is left open here, settled when structured render
is wired.

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 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread docs/rfcs/0001-template-miner.md Outdated
The (bytes, reconstruction) intro read as a universal contract; scope it
to body_kind=String up front so structured rows (out of scope here)
aren't implied to carry a Reconstruction value.

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 1 out of 1 changed files in this pull request and generated no new comments.

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