r2il: fix lossy v1 sink artifacts, add facts/residuals reader - #102
Merged
Conversation
The v1 sink shipped in PR #101 wrote a lossy artifact: the residuals TSV carried only reason.as_str() plus the three facet coordinates, dropping ResidualReason's per-variant typed payload, ResidualFact::at_prefix, and ResidualFact::provenance entirely. The facts TSV likewise omitted FactProvenance.value. No reader existed for either, so this was never caught by round-tripping — v1 was write-only. Widen both schemas additively (append columns, never reorder, per this crate's own STAGED-CODEGEN-GUIDE.md §4 convention) and bump both version headers: - FACTS_VERSION 1->2: add value_id column. - RESIDUALS_VERSION 1->2: widen from 4 columns to 14, carrying the typed reason payload (two opcode/u32/usize slots), the FacetPrefix variant, and the full FactProvenance tuple. Add read_facts/read_residuals plus the shared codecs both facts and residuals need (provenance_cols/parse_provenance_cols, facet_cols/ parse_facet_cols, prefix_cols/parse_prefix_cols, reason_payload_cols/ parse_reason, concern_from_str, fact_kind_from_str, facet_from_raw). Add OpTag::parse as the exact inverse of as_str (one arm per variant, None on any unrecognized string) so the residual reason payload can round-trip an OpTag through the TSV. Named `parse` rather than `from_str` to avoid colliding with clippy's should_implement_trait lint against std::str::FromStr's canonical shape. Both new round-trip tests are disable-run verified: mutating the write or parse path drives them red, restoring the fix brings them back green.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_c20360c9-72bc-4fbb-a1c0-a3613c1aa113) |
AdaWorldAPI
marked this pull request as ready for review
August 18, 2026 12:03
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Continues PR2 (
.claude/plans/r2il-behavioral-ir-v1.md) after PR #101 shippedthe
RefinedTruthSinktrait +OfflineSink+ the §12 corpus-profile finding.While building the reader for
sink.rs's TSV artifacts, found that the v1writer (merged in #101) was write-only and lossy:
reason.as_str()plus the three facetcoordinates — dropping
ResidualReason's per-variant typed payloadentirely, plus
ResidualFact::at_prefixandResidualFact::provenance.FactProvenance.value(thevalue_idcolumn).No reader ever existed to catch this — v1 shipped without a round-trip test.
What changed
FACTS_VERSION1→2 (addedvalue_idcolumn) andRESIDUALS_VERSION1→2 (widened 4→14 columns: typed reason payload, theFacetPrefixvariant, and the fullFactProvenancetuple) — additive perSTAGED-CODEGEN-GUIDE.md§4 (append columns, never reorder, version theheader).
read_facts/read_residualsplus the shared codecs both facts andresiduals need (
provenance_cols/parse_provenance_cols,facet_cols/parse_facet_cols,prefix_cols/parse_prefix_cols,reason_payload_cols/parse_reason,concern_from_str,fact_kind_from_str,facet_from_raw).OpTag::parse— the exact inverse ofas_str()(one arm pervariant,
Noneon anything unrecognized), needed so a residual reason'sopcode payload can round-trip through the TSV. Named
parserather thanfrom_strto avoid clippy'sshould_implement_traitlint againststd::str::FromStr's canonical shape.parse path, confirmed red, restored, confirmed green):
read_facts_round_trips_every_field_including_value_id,read_facts_refuses_an_unknown_opcode_rather_than_guessing,read_residuals_round_trips_a_two_opcode_payload_reason,read_residuals_round_trips_a_payload_free_reason_with_no_facet.Not in this slice
Per PR2's own incremental scoping (see #101): DTO/codebook wiring beyond
what
facet.rsalready has, the round-trip reconstruction oracle (R2IL →routes → semantic-equivalent R2IL — SPO is explicitly NOT this oracle per
the plan), and the optional SPO projection of semantic facts (calls/objects
only).
Test plan
cargo test -p ruff_r2il— 40 lib + 12 integration tests, all greencargo clippy -p ruff_r2il --all-targets -- -D warnings— cleancargo fmt --check— cleanred → restore → green)
uv run --only-group dev prek run --files <changed files>— clean🤖 Generated with Claude Code
Generated by Claude Code