Skip to content

PR-PreF: consolidate interval substrate parent - #1154

Merged
briansrls merged 8 commits into
mainfrom
session/eager-ram-167
Apr 29, 2026
Merged

briansrls merged 8 commits into
mainfrom
session/eager-ram-167

Conversation

@briansrls

@briansrls briansrls commented Apr 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add IntervalWidth, neutral PositiveIntervalWidth, and Interval<D> substrate declarations for PR-PreF bound consolidation.
  • Regenerate scalar/runtime mirrors and bootstrap snapshots so Dag::new() exposes the interval declarations.
  • Add CardinalityBound::interval() plus Rust/Go type realizations and focused inhabitance coverage.

Local verification

  • python3 scripts/regen_runtime_mirrors.py --check
  • cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verify
  • cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored --nocapture
  • cargo test -p v3-compiler --test integration m2_substrate_inhabitance_test -- --nocapture
  • cargo test -p v3-compiler --test integration handwritten_parse_snapshot_matches_manifest -- --nocapture

@briansrls
briansrls marked this pull request as ready for review April 29, 2026 01:49
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: 372b1f22 · Trigger: schedule
  • Comparison: origin/main @ 6a7f020b ... review/pr-1154-372b1f22 @ 372b1f22
  • Thinking: 38s wall

Findings

None. The meaningful diff adds Interval<D> in src/v3/std/substrate.dag with an explicit P1 / DAG-ancestor story, wires regen (scripts/regen_runtime_mirrors.py, dag_scalar_generated.rs), adds a single projection CardinalityBound::interval() in src/v3/compiler/src/dag.rs with an exhaustive match, and extends m2_substrate_inhabitance_test.rs with a structural “substrate vs runtime” check plus a small behavioral test. computation.dag’s SizeBound section documents the PreF split (concrete Interval vs deferred symbolic work), which matches the “tracked bridge” pattern rather than an unnamed scaffold. Nothing in this diff suggests duplicate authorities, fail-open paths, or test-discipline issues relative to the referenced docs.

Verdict

APPROVE — The change is narrowly scoped, matches the invariants’ “shared parent / retrofit instances” story, and the large bootstrap_generated*.rs churn is consistent with mirroring the new substrate. No concrete violations in the diff against INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: 372b1f22 · Trigger: manual
  • Comparison: main @ 6a7f020b ... session/eager-ram-167 @ 60348956
  • Conversation: View conversation

1. Story of the diff

This PR introduces Interval<D> as the shared substrate parent for bound-like concepts, positioning CardinalityBound, SizeBound, and loop cardinality bounds under a common interval model rather than continuing to grow sibling bound declarations. The substrate declaration lands in src/v3/std/substrate.dag:84-86, the runtime mirror generator is taught to emit Interval<D> in scripts/regen_runtime_mirrors.py:810-816, and the generated Rust scalar carrier appears in src/v3/compiler/src/dag_scalar_generated.rs:21-27.

The PR also adds a hand-written projection from the existing CardinalityBound runtime enum into the new parent (src/v3/compiler/src/dag.rs:468-479), adds Rust/Go target realizations for Interval (src/v3/spec/rust.dag:254-261, src/v3/spec/go.dag:188-195), and extends the substrate inhabitance tests to prove both the reflected sum shape and the cardinality projection behavior (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:987-996, 1388-1399). The large bootstrap-generated diffs look like mechanical regeneration after inserting the new substrate declaration.

2. Invariant categories

  1. LAYER MODEL (substrate vs implementation).

Finding — BLOCKING. src/v3/std/substrate.dag:85: = ExactInterval { lower: D, upper: D } introduces a substrate-level interval carrier whose product fields admit lower > upper. Because this is substrate, not implementation-only Rust, the illegal state becomes model-authoritative and is also exposed by the generated runtime carrier at src/v3/compiler/src/dag_scalar_generated.rs:22-24. The layer move is directionally right — a shared Interval<D> parent — but the current shape does not make the interval validity invariant unrepresentable.

  1. INVARIANTS.md + modeling-discipline.md.

Finding — BLOCKING. src/v3/std/substrate.dag:84: type Interval<D> adds a new two-variant coproduct, but the nearby checkpoint comment at src/v3/std/substrate.dag:79-83 does not classify it as terminal/scaffold/dissolvable, nor does it provide a ledger/trigger for that coproduct shape. Separately, the lower/upper pair at src/v3/std/substrate.dag:85 violates the illegal-states-unrepresentable practice for a bound interval unless the model carries an order/well-formedness witness or represents the upper side as a non-negative offset / checked bounded range.

  1. CODING.md.

Compliant. src/v3/compiler/src/dag.rs:469 adds pub fn interval(self) -> Interval<u32> as a pure projection from a typed carrier to a structured typed carrier; it has no hidden dependencies, no fallback default, and every enum arm is handled explicitly at src/v3/compiler/src/dag.rs:470-476.

  1. TESTING.md.

Compliant. The diff adds behavior-facing coverage at the right seam: src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:988-996 checks that substrate reflection exposes the new Interval variants, and src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:1389-1399 checks the public CardinalityBound::interval() contract across all existing cardinality variants. The missing validity check for lower <= upper is a modeling-shape problem, not a test omission; the current type makes the bad state constructible rather than testably rejected.

  1. LOCKED DESIGN DECISIONS.

N/A — the diff references the P1 substrate-target rationale at src/v3/std/substrate.dag:81, but I do not see a change to, or divergence from, a locked thesis/design decision in the diff.

  1. TRACKED vs UNTRACKED DEBT.

Compliant for the explicit bridge; blocked by the substrate findings above. The staged PR-PreF note in src/v3/std/computation.dag:28-29 documents the temporary split, bounds it to “constant explicit counts” versus “symbolic size parameters,” and names the dissolution trigger as first-class symbolic size refs. That bridge is tracked enough for review; the untracked part is the new Interval<D> coproduct/well-formedness shape called out in categories 1–2.

3. Verdict

REQUEST_CHANGES

The PR is making the right architectural move by introducing a shared bound parent and proving the runtime projection path, but the new substrate carrier currently admits invalid intervals and lacks the required coproduct classification. Because those are substrate modeling issues, they should be fixed before this shape becomes authoritative.

@briansrls
briansrls force-pushed the session/eager-ram-167 branch from 6034895 to 28119ae Compare April 29, 2026 02:03
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: 28119ae6 · Trigger: schedule
  • Comparison: origin/main @ 041ed678 ... review/pr-1154-28119ae6 @ 28119ae6
  • Thinking: 72s wall

Reviewed git diff origin/main...HEAD (substantive paths plus git diff --stat). The bulk of the line count is regenerated bootstrap_generated*.rs from the substrate/spec changes; the authored intent is narrow.

Findings: None that rise to a doc-backed violation with review teeth. The new Interval<D> parent in substrate.dag, regen wiring in regen_runtime_mirrors.py, the CardinalityBound::interval() projection in dag.rs, spec realizations, manifest refresh, and integration tests are consistent with INVARIANTS P1’s “shared parent / inhabitance” story and do not introduce fail-open paths or parallel authorities. computation.dag adds an Interval import used only in comments today; if the toolchain ever enforces used-imports on .dag, that would be a follow-up hygiene tweak, not something evidenced as broken here.

Verdict: APPROVE — Diff is scoped to introducing the shared Interval<D> substrate + runtime mirror, a single projection helper, and regression coverage; nothing in the reviewed hunks clearly breaches INVARIANTS, modeling-discipline, CODING, or TESTING beyond normal regen churn.

Exploratory (optional): substrate.dag commentary references projecting through Interval<Int> while dag.rs exposes interval() -> Interval<u32>, matching the existing CardinalityBound::Exact(u32) mirror. Worth a future one-line clarification so readers do not assume substrate Int is literally the Rust type parameter (not blocking).

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review metadata

  • Provider / model: codex / unknown
  • Commit: 60348956 · Trigger: schedule
  • Thinking: 319s wall

BLOCKING (2)

Root Cause

  • src/v3/std/substrate.dag Interval landed as a shared substrate parent before its ordered-bound invariant and dissolution receipt were encoded → add the order/refinement witness and the 🟢/🟡/🔴 receipt, or keep existing carriers until that evidence exists.

⚠️ The shared Interval substrate shape needs modeling receipts and invariant enforcement before it lands.

Comment thread src/v3/std/substrate.dag
// domains; this parent is the substrate target named by INVARIANTS.md §P1.
// Existing carriers retrofit as instances instead of growing new sibling
// bound declarations.
type Interval<D>

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verified against current head 783808f1: Interval<D> now has the required coproduct classification directly above the declaration. Current src/v3/std/substrate.dag:93-96 reads // 🟢 TERMINAL. Interval is the durable shared bound parent... No dissolution trigger..., followed by type Interval<D> at line 97. This was addressed after the reviewed stale shape; no further code change is needed for this checkpoint.

— sent from eager-ram-167

Comment thread src/v3/std/substrate.dag Outdated
// Existing carriers retrofit as instances instead of growing new sibling
// bound declarations.
type Interval<D>
= ExactInterval { lower: D, upper: D }

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Verified against current head 6551dd625: this was fixed in ab87d39e1 and regenerated in bca173c77 / 6551dd625. ExactInterval no longer exists; Interval<D> now uses BoundedInterval { lower, upper, order: IntervalOrderWitness }, and the comment above it states the endpoint pair is not complete without the witness. CardinalityBound::interval() now emits EqualEndpoints for exact bounds and OrderedEndpoints for AtMostOne, with regression coverage in m2_substrate_inhabitance_test.

— sent from eager-ram-167

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: bca173c7 · Trigger: schedule
  • Comparison: origin/main @ 041ed678 ... review/pr-1154-bca173c7 @ bca173c7
  • Thinking: 46s wall

Findings

None that tie to a concrete invariant violation in the diff. The substrate change matches INVARIANTS.md P1’s DAG-ancestor procedure (Interval<D> as named parent; docs/modeling-discipline.md Practice 4’s 🟢 ledger-style rationale on substrate.dag). CardinalityBound::interval() in dag.rs is total over the enum (no silent gaps). Tests in m2_substrate_inhabitance_test.rs ratchet carrier shapes and pin the projection — consistent with TESTING.md’s behavior/fixture discipline for this suite.

Verdict

APPROVE — Scoped substrate addition (Interval / IntervalOrderWitness), regen/mirrors, Rust projection helper, target realizations, and inhabitance tests read coherent and aligned with the documented modeling intent; nothing in the reviewed hunks argues for blocking.

Exploratory observations (optional)

  • src/v3/std/computation.dag:21 imports Interval while the only mention in this file is in the new comment block (SizeBound does not yet reference Interval). If your DAG toolchain ever enforces unused imports, this may need tightening in a follow-up; otherwise it’s harmless staging for PR-PreF.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: bca173c7 · Trigger: manual
  • Comparison: main @ 041ed678 ... session/eager-ram-167 @ 477b7692
  • Conversation: View conversation

1. Story of the diff

This PR introduces Interval<D> as the shared substrate parent for bound-like concepts, with IntervalOrderWitness intended to carry endpoint-order evidence. The substrate declaration lands in src/v3/std/substrate.dag, the runtime mirror generator is taught to emit the new Rust carriers, and the regenerated bootstrap fixtures pick up the new declarations. The PR also adds Rust/Go target realizations for the new substrate types, updates the parse corpus manifest, and adds a CardinalityBound::interval() projection plus integration coverage that checks both the reflected substrate variants and the cardinality-to-interval mapping.

2. Invariant categories

  1. LAYER MODEL — Finding

BLOCKING — substrate illegal state remains representable. The diff touches substrate directly: src/v3/std/substrate.dag:94 adds type Interval<D>, and src/v3/std/substrate.dag:95 defines BoundedInterval { lower: D, upper: D, order: IntervalOrderWitness }. The adjacent comment says the endpoint pair is not complete without a witness and that constructors/reconciliation should reject inverted endpoints before the bound becomes authoritative (src/v3/std/substrate.dag:85-89), but the landed substrate type still allows contradictory inhabitants: lower > upper with OrderedEndpoints, or unequal endpoints with EqualEndpoints from src/v3/std/substrate.dag:91-92. Because this is a substrate shape, the illegal-state concern should be closed at the authoritative type/constructor boundary in this PR, not deferred behind a comment.

  1. INVARIANTS.md + modeling-discipline.md — Finding

Illegal states unrepresentable / API-level enforcement. The witness is modeled as a free nullary coproduct (src/v3/std/substrate.dag:90-92) and then stored beside unconstrained endpoint coordinates (src/v3/std/substrate.dag:95). That makes the ordering fact convention-level rather than structurally enforced: consumers must trust that the chosen witness matches the endpoints. This weakens the intended P2/P3 discipline; the model should either make invalid endpoint/witness combinations unconstructable, or expose only a fail-closed constructor/reconciliation API that produces Interval<D> after validation.

  1. CODING.md — Compliant

The implementation projection is total and typed: CardinalityBound::interval matches every variant explicitly and has no default/fabricating arm (src/v3/compiler/src/dag.rs:470-481). The return shape is a structured carrier, Interval<u32>, rather than a primitive sentinel (src/v3/compiler/src/dag.rs:469).

  1. TESTING.md — Compliant

The diff adds behavior-level regression coverage for both the substrate mirror and the projection. substrate_coproducts_match_runtime_carriers now asserts the reflected Interval and IntervalOrderWitness variant surfaces (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:988-1008), and cardinality_bound_projects_to_interval_parent pins Exact, AtMostOne, and Unbounded projection behavior (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:1400-1418). The tests are focused and hermetic for the behavior they claim.

  1. LOCKED DESIGN DECISIONS — N/A

The diff references INVARIANTS.md §P1 in the new substrate comment (src/v3/std/substrate.dag:81-82), but it does not modify a thesis/design file or explicitly alter a locked design decision.

  1. TRACKED vs UNTRACKED DEBT — Compliant with the blocker above separated

The staged SizeBound note is documented, bounded, and gives a dissolution trigger: constant explicit counts project through Interval<Int>, while symbolic size parameters remain SizeBound until their refs become first-class (src/v3/std/computation.dag:28-29). That is a tracked bridge shape. The blocker is not the existence of the staged bridge; it is that the newly authoritative Interval substrate shape still permits invalid endpoint/witness combinations.

3. Verdict

REQUEST_CHANGES

The PR’s direction is aligned with the substrate-parent story, and the generator/tests are wired cleanly, but Interval<D> is a new substrate authority and currently relies on a witness tag that can disagree with the raw endpoints. I would close that modeling hole before landing, either by making construction fail-closed in the same PR or by reshaping the interval carrier so invalid endpoint/order combinations are not representable.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 477b7692 · Trigger: schedule
  • Comparison: origin/main @ 041ed678 ... review/pr-1154-477b7692 @ 477b7692
  • Thinking: 28s wall

Verdict: APPROVE

Clean PR. Adds Interval<D> as the shared bound parent in src/v3/std/substrate.dag:90-98 with a proof-carrying IntervalOrderWitness (illegal-states-unrepresentable for inverted endpoints — good modeling discipline), classifies it 🟢 TERMINAL with rationale, retrofits CardinalityBound as a projection via CardinalityBound::interval() in src/v3/compiler/src/dag.rs:468-483, and covers it with a focused unit test in m2_substrate_inhabitance_test.rs:1400-1419. Runtime carriers and Rust/Go realizations are regenerated/declared consistently; Python omission matches existing CardinalityBound omission. The computation.dag:27-29 note on SizeBound projection is a properly tracked bridge (documented, bounded to symbolic-ref carriers, named trigger: "until their refs become first-class").

No findings against the diff.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 0774ea5a · Trigger: schedule
  • Comparison: origin/main @ 2cdfd4fa ... review/pr-1154-0774ea5a @ 0774ea5a
  • Thinking: 30s wall

Verdict: APPROVE — Adds an Interval<D> / IntervalWidth substrate parent in std/substrate.dag plus a projection from CardinalityBound (dag.rs:468-481), with mirror regen, language realizations in rust.dag/go.dag, and inhabitance tests. Modeling discipline holds: BoundedInterval is lower + nonnegative width, making inverted intervals unrepresentable (illegal-states-unrepresentable, INVARIANTS P1). CardinalityBound remains the single authority; Interval is a derived projection, not a parallel source. The projection cases (Exact→[v,v], AtMostOne→[0,1], Unbounded→Unbounded) are correct and covered by cardinality_bound_projects_to_interval_parent. No diff line violates the rubric.

Exploratory observation (non-blocking): The doc says SizeBound and the cardinality arm of LoopBound "retrofit as instances" of Interval, but only CardinalityBound actually projects in this PR. Worth tracking that follow-up so the parent doesn't sit half-attached — but the comment in computation.dag:27-28 already names the deferred case (symbolic size params), so this is an accepted bridge rather than a missing one.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: 337e9502 · Trigger: schedule
  • Comparison: origin/main @ d3ea75a2 ... review/pr-1154-337e9502 @ 337e9502
  • Thinking: 268s wall

Findings: None. The diff adds substrate-level Interval<D> / IntervalWidth with explicit modeling-discipline-style classification (substrate.dag lines 80–99), wires generated mirrors (regen_runtime_mirrors.py, dag_scalar_generated.rs), adds a single projection CardinalityBound::interval() (dag.rs lines 468–482), target realizations in go.dag / rust.dag, and regression coverage in m2_substrate_inhabitance_test.rs. That matches P1’s “shared parent / retrofit as instances” story from INVARIANTS.md / docs/modeling-discipline.md rather than introducing parallel bound types. Nothing here suggests fail-open behavior, duplicate authorities for the same fact, or scaffold debt missing a trigger—the substrate comments document durable terminal shapes.

Verdict: APPROVE — Scoped substrate mirror + projection + tests; no concrete rubric violations spotted in the reviewed hunks.

Exploratory observation (optional): IntervalWidth::PositiveWidth reuses PositiveDescentAmount from std.termination (substrate.dag lines 89–91). That avoids a second Peano-style positive carrier but ties generic “interval width” vocabulary to the termination module; worth watching if future non-termination intervals need a clearer semantic boundary—conversation only, not a blocker.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 783808f1 · Trigger: schedule
  • Comparison: origin/main @ ecc2ccc3 ... review/pr-1154-783808f1 @ 783808f1
  • Thinking: 25s wall

APPROVE — Introduces Interval<D> / IntervalWidth as the shared bound parent named in the PR thesis, plus a CardinalityBound::interval() projection. Substrate authority lives in src/v3/std/substrate.dag (single source); Rust mirrors are regenerated from spec via dag_scalar_generated.rs; Go/Rust realizations and the inhabitance test are updated symmetrically. Modeling discipline is respected: width is encoded as lower + nonnegative width so inverted intervals are unrepresentable (illegal-states-unrepresentable), and PositiveDescentAmount is reused from std.termination rather than cloned (DFS / no parallel representation). The terminal-vs-bridge status of both new types is documented in the .dag file. No findings.

Exploratory observation (non-blocking): reusing PositiveDescentAmount — a termination-flavored name — as the carrier of an interval's width works but reads oddly at the substrate layer (PositiveWidth(OneStep) for AtMostOne is correct yet a bit cryptic). If a more neutral PositiveCount/PositiveAmount parent ever emerges, this is a natural follow-up site. Not a blocker; no diff line to change today.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review metadata

  • Provider / model: codex / unknown
  • Commit: 0774ea5a · Trigger: schedule
  • Thinking: 491s wall

Non-blocking — Strengths

  • src/v3/std/substrate.dag The interval parent is documented as a durable shared bound substrate and the CardinalityBound projection is covered by a focused test.

✅ No blocking concerns in this mixed substrate/runtime mirror change.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 948660a2 · Trigger: schedule
  • Comparison: origin/main @ cc5e5c39 ... review/pr-1154-948660a2 @ 948660a2
  • Thinking: 992s wall

Verdict: APPROVE

The diff is narrowly scoped: it adds the Interval substrate parent, mirrors it into generated Rust, adds target realizations, and tests the CardinalityBound projection. I don’t see a concrete violation of the pinned invariants, modeling discipline, coding guide, or test discipline on the changed lines.

Verification note: I attempted cargo test -p v3-compiler --test integration m2_substrate_inhabitance; the cold worktree spent several minutes compiling dependencies/local test crates without producing test results, so I stopped it rather than leave it running.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: ddc31fd7 · Trigger: schedule
  • Comparison: origin/main @ 8fae08eb ... review/pr-1154-ddc31fd7 @ ddc31fd7
  • Thinking: 368s wall

Findings: None. The substantive diff introduces Interval<D> / IntervalWidth on the substrate with explicit terminal classification and rationale (lower + nonnegative width so inverted intervals are unrepresentable), wires them through regen (scripts/regen_runtime_mirrors.py), adds a single projection CardinalityBound::interval() plus parity/regression coverage in m2_substrate_inhabitance_test.rs, and extends Rust/Go TypeRealization rows with the expected bootstrap churn. Nothing here contradicts fail-closed behavior, single-authority substrate modeling, or the testing doc’s expectations for this layer (integration tests that pin substrate ↔ runtime carrier shape and a focused projection test).

Verdict: APPROVE — Scoped substrate + mirror + test updates; modeling notes match INVARIANTS P1 / modeling-discipline (DAG parent, terminal coproduct classification). No diff-grounded violations of the pinned rubric.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review metadata

  • Provider / model: codex / unknown
  • Commit: ddc31fd7 · Trigger: schedule
  • Thinking: 176s wall

Non-blocking — Strengths

  • src/v3/std/substrate.dag The interval substrate is grounded as a shared parent for existing bound carriers and keeps the CardinalityBound projection as an instance rather than a sibling representation.
  • src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs The new runtime mirror shapes and CardinalityBound-to-Interval projection are covered by focused inhabitance tests.

✅ No blocking concerns in this mixed substrate/runtime mirror update.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: 3c9e7ebe · Trigger: schedule
  • Comparison: origin/main @ 651163b6 ... review/pr-1154-3c9e7ebe @ 3c9e7ebe
  • Thinking: 81s wall

Findings (if any):
None. The hand-authored changes implement the P1 “shared bound parent / retrofit as instances” story from the rubric: Interval<D> and IntervalWidth are declared in substrate.dag with explicit terminal classification, CardinalityBound is projected in dag.rs without a second authority, and m2_substrate_inhabitance_test.rs extends coproduct/coverage and adds a focused projection test. Regen (regen_runtime_mirrors.py, dag_scalar_generated.rs, and the large bootstrap_generated* updates) is consistent with that substrate addition. I did not see a concrete diff line that violates P2/P3, docs/modeling-discipline.md (enums are annotated), CODING.md, or TESTING.md for new work in this change.

Verdict: APPROVE — The diff is coherent and narrowly focused: a substrate-level interval parent, Rust mirrors + projections, spec realizations, and tests/ratchets. No policy violations grounded in the diff.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: 0fd716bd · Trigger: schedule
  • Comparison: origin/main @ e77ada9a ... review/pr-1154-0fd716bd @ 0fd716bd
  • Thinking: 54s wall

Findings: None. The hand-authored diff adds Interval<D> and IntervalWidth in substrate.dag with TERMINAL (ledger-style) notes that match INVARIANTS.md P1’s DAG-ancestor / “shared parent” story; wires PositiveDescentAmount for width; adds a single projection CardinalityBound::interval() -> Interval<u32> in dag.rs (one derived view of the existing carrier, not a second authority); extends regen and go/rust TypeRealization entries; and extends m2_substrate_inhabitance with structural tests for DAG/runtime alignment and the projection. Nothing here contradicts fail-closed (exhaustive match), boundary discipline (substrate is still single authority; regen is the consumer path), or TESTING (behavior- and interface-oriented assertions). The computation.dag addition is a scoped design note on an already YELLOW-classified type with a named dissolution direction, not a new untracked scaffold.

Verdict: APPROVE — The change is focused, models the shared parent as substrate facts plus a small Rust projection, includes terminal coproduct documentation and tests; no principled issues grounded in this diff.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: 53dab518 · Trigger: schedule
  • Comparison: origin/main @ 8a41cbcb ... review/pr-1154-53dab518 @ 53dab518
  • Thinking: 108s wall

Findings: None. The diff adds substrate parents and projections with explicit terminal classification, documents the P1 “shared parent” rationale, keeps bounded intervals as lower + nonnegative width (illegal inverted ranges not representable), wires regen/spec mirrors, and extends m2_substrate_inhabitance_test for DAG parity plus cardinality_bound_projects_to_interval_parent. Nothing here contradicts fail-closed discipline, boundary/single-authority intent, or TESTING/CODING in a way that needs a blocking cite.

Verdict: APPROVE — Scoped modeling step: new Interval / IntervalWidth in substrate.dag, mechanical mirror updates, and a small CardinalityBound::interval() projection with tests. No invariant rubric violations tied to specific diff lines.

Exploratory (optional): Substrate prose refers to projecting through Interval<Int> while the Rust helper returns Interval<u32> (dag.rs); if that is not already the established Int→mirror mapping story everywhere, a one-line alignment between comment and carrier could reduce reader drift (not raised as a finding without a rubric breach).

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review metadata

  • Provider / model: codex / unknown
  • Commit: 0fd716bd · Trigger: schedule
  • Thinking: 257s wall

✅ No blocking concerns in this mixed substrate/runtime mirror update.

@briansrls
briansrls force-pushed the session/eager-ram-167 branch from 53dab51 to 9b62f6f Compare April 29, 2026 06:00
@briansrls briansrls changed the title eager-ram-167 PR-PreF: consolidate interval substrate parent Apr 29, 2026
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 9b62f6f8 · Trigger: schedule
  • Comparison: origin/main @ 3f1f1008 ... review/pr-1154-9b62f6f8 @ 9b62f6f8
  • Thinking: 27s wall

Verdict: APPROVE — small, well-scoped substrate consolidation. Adds Interval<D>/IntervalWidth as the shared bound parent in std/substrate.dag, mirrors them in the Rust runtime + go/rust realizations, and gives CardinalityBound an interval() projection with direct test coverage. No invariant violations spotted: substrate authority stays in the .dag, the runtime mirror is generated, and BoundedInterval { lower, width } makes inverted intervals unrepresentable as the comment claims. The "no dissolution trigger" is explicitly called out as a terminal parent rather than a bridge, which is consistent with the modeling-discipline framing.

Exploratory observation (non-blocking): AtMostOne → BoundedInterval { lower: 0, width: OneStep } encodes a width of 1 to cover {0,1}. Worth confirming downstream consumers read width as inclusive count vs. open length so OneStep doesn't get reinterpreted as {0} only — but nothing in this diff misuses it.

@briansrls
briansrls merged commit 909a69e into main Apr 29, 2026
4 checks passed

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review metadata

  • Provider / model: codex / unknown
  • Commit: 53dab518 · Trigger: schedule
  • Thinking: 289s wall

Non-blocking — Strengths

  • src/v3/std/substrate.dag The shared interval parent is mirrored through the generated Rust carriers and covered by runtime-shape and projection tests.

✅ No blocking concerns in this mixed substrate/runtime mirror update.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: 477b7692 · Trigger: manual
  • Conversation: View conversation

1. Story of the diff

This PR introduces Interval<D> as a new substrate-level shared parent for bound-like carriers, with IntervalOrderWitness intended to distinguish equal endpoints from ordered endpoints. The substrate declaration lands in src/v3/std/substrate.dag, the Rust scalar mirror is generated in dag_scalar_generated.rs, the regen script is taught to emit those mirrors, and target specs for Rust/Go get TypeRealization rows. The only direct runtime projection added is CardinalityBound::interval() in dag.rs, with tests checking that the substrate coproducts reflect into the runtime carriers and that CardinalityBound maps into the new interval parent.

2. Invariant categories

  1. LAYER MODEL (substrate vs implementation).

Finding — BLOCKING. This is substrate, not implementation-only: src/v3/std/substrate.dag:98-100 adds type Interval<D> = BoundedInterval { lower: D, upper: D, order: IntervalOrderWitness } | Unbounded. The comment says the bounded interval is “proof-carrying” and that constructors/reconciliation will reject inverted endpoints before the bound becomes authoritative (src/v3/std/substrate.dag:85-89), but the landed substrate shape itself still admits lower > upper paired with OrderedEndpoints. Since IntervalOrderWitness is only EqualEndpoints | OrderedEndpoints (src/v3/std/substrate.dag:90-92) and carries no relationship to the endpoints, the illegal state is still representable at the substrate boundary.

  1. INVARIANTS.md + modeling-discipline.md.

Finding — BLOCKING. This violates illegal-states-unrepresentable / fail-closed modeling discipline: the authoritative carrier line src/v3/std/substrate.dag:99 allows arbitrary { lower, upper, order }, while the enforcement mechanism is deferred to “typed constructors / reconciliation” at src/v3/std/substrate.dag:88. The safe CardinalityBound::interval() projection at src/v3/compiler/src/dag.rs:468-484 does not close the substrate surface; it only proves one producer constructs valid intervals. Other .dag authors or future generated values can still fabricate a witness for an invalid endpoint pair.

  1. CODING.md.

Compliant. The Rust implementation work is small and explicit: CardinalityBound::interval at src/v3/compiler/src/dag.rs:468-484 is an exhaustive pure mapping from the existing carrier into the new mirror, and the regen hook at scripts/regen_runtime_mirrors.py:810-822 keeps generated scalar mirrors single-source rather than hand-authoring parallel Rust enums.

  1. TESTING.md.

Compliant for the behavior that actually landed, but not sufficient to rescue the substrate issue above. The diff adds reflection-shape coverage for both new coproducts at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:988-1008, a focused projection test for CardinalityBound::interval() at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:1400-1418, and includes both new substrate types in the reflected-realization list at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:1432-1433. What is missing is not just an assertion; the model currently has no fail-closed construction/reconciliation path to test for inverted endpoints.

  1. LOCKED DESIGN DECISIONS.

N/A — no locked thesis/design decision is altered in the diff. The only explicit design-rule reference I see is the substrate comment pointing to INVARIANTS.md §P1 at src/v3/std/substrate.dag:81-82.

  1. TRACKED vs UNTRACKED DEBT.

Finding — BLOCKING as part of the same substrate issue. The diff names a future enforcement seam — “typed constructors / reconciliation” at src/v3/std/substrate.dag:88 — but then classifies Interval as terminal and says “No dissolution trigger” at src/v3/std/substrate.dag:96. If the checked construction path is intentionally deferred, this is a scaffold and needs documentation, bounds, and a named dissolution trigger; if it is not deferred, the substrate type needs to make the endpoint-order invariant authoritative in this PR.

3. Verdict

REQUEST_CHANGES. The generated mirrors, target realizations, and CardinalityBound projection are coherent, but the new substrate carrier is landing as authoritative while its core endpoint-order invariant remains convention-level. Because this is substrate and admits an illegal state directly in the modeled type, I would block until the interval construction/enforcement story is landed or the interim shape is explicitly tracked as a bounded scaffold with a dissolution trigger.

@briansrls
briansrls deleted the session/eager-ram-167 branch June 1, 2026 18:42
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.

1 participant