Repository navigation
ModelCore PrimitiveFactBundle.spec_facts opaque-Node fill — replace untyped catch-all with structured fact bundle using declared primitive_fact_axis_{width,signedness,range,encoding} symbols (T-33 #3474 codex RC discharge as follow-up debt-PR on main) - #3500
Conversation
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
c9c49ec7· Trigger:schedule - Thinking:
366s wall
BLOCKING (2)
Root Cause
src/v4/std/model_core.dagModelCore introduced axis bindings before the axis vocabulary covered every T-4 primitive fact or separated range from overflow disposition → extend the axes/fact carriers and bind each source-spec fact under its own correct key.
ROADMAP — Incomplete
- T-4 fact-bundle contract: The PR moves toward structured fact bundles, but the new axis fill does not yet satisfy T-4's required source-spec facts.
| rust_primitive_fact_bundle(facts_node: rust_integer_facts_node(facts: facts)) | ||
| PrimitiveFactBundle { | ||
| substrate_carrier: rust_integer_facts_node(facts: facts), | ||
| spec_facts: [ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| fact: rust_int_kind_node(kind: facts.signedness) | ||
| ), | ||
| rust_primitive_axis_binding( | ||
| axis: primitive_fact_axis_range, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e-spec facts Adds three declared axis Symbols and binds them in the Rust extdep producer: - primitive_fact_axis_surface_spelling — was omitted (codex BLOCKING #1) - primitive_fact_axis_overflow_disposition — was misnamed as range (codex BLOCKING #2) - primitive_fact_axis_std_projection — closes T-4 source-spec coverage Integer bundles bind {surface_spelling, width, signedness, overflow_disposition, std_projection}. Float bundles bind {surface_spelling, width, encoding, std_projection}. Non-integer bundles bind {surface_spelling, encoding, std_projection}. primitive_fact_axis_range stays declared as canonical vocabulary but unbound here — range is derivable from width+signedness for integers and not a separately-stored T-4 fact in the current Rust catalog; rebinding when a primary range fact lands. Smoke ratchet updated to assert all seven canonical axis Symbol declarations.
|
Discharged the codex schedule review (3 BLOCKING) at (1) (2) (3) Axes incomplete for T-4 source-spec facts — addressed. Declared third new axis
Ratchet |
|
Updated PR body to match landed bindings — explicit per-kind axis sets (Integer/Float/Non-integer) and the intentional The exploratory "duplicate-or-missing axis keys" observation noted; tracked-not-acted as out of scope for this debt-PR — if axis completeness becomes load-bearing, a stricter carrier (e.g. fixed-record-per-kind or set-of-axes invariant) is a separate modeling pass. — sent from crisp-raven-275 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
638bc872· Trigger:schedule - Thinking:
338s wall
BLOCKING (3)
Root Cause
src/v4/extdeps/languages/rust.dagRustIntegerPrimitiveFacts has no representation or encoding source field → add the Rust integer representation carrier and axis binding before exposing the bundle as the ModelCore boundary.src/v4/std/model_core.dagPrimitiveFactBundle.spec_facts mixes source-spec facts and std-projection evidence → keep spec_facts source-only and model std coincidence through a separate grounded relation or existing coincidence consumer.src/v3/compiler/tests/integration/v4_std_model_core_dag_smoke_test.rsThe ModelCore ratchet is still being extended as hand-maintained Rust → add the required checkable P5 receipt or move this assertion into the structural/generated test lane.
| rust_primitive_fact_bundle(facts_node: rust_integer_facts_node(facts: facts)) | ||
| PrimitiveFactBundle { | ||
| substrate_carrier: rust_integer_facts_node(facts: facts), | ||
| spec_facts: [ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| fact: rust_overflow_node(overflow: facts.overflow_release) | ||
| ), | ||
| rust_primitive_axis_binding( | ||
| axis: primitive_fact_axis_std_projection, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
BLOCKING: The added hand-written src/v3 Rust test expands the SG-0 test surface without the required P5 PR-body or planning-artifact receipt in the supplied review surface, violating INVARIANTS.md P5 for new Rust tests.
|
Discharge of codex schedule re-review on (1) Inline (2) Inline (3) Inline (4) Inline Per-primitive axis-keyed spec_facts now:
|
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
0302768b· Trigger:schedule - Thinking:
315s wall
BLOCKING (2)
Root Cause
src/v4/extdeps/languages/rust.dagRustIntegerPrimitiveFacts has no range carrier/source fields → add declared min/max range facts to the record/node and bind them through primitive_fact_axis_range.src/v4/extdeps/languages/rust.dagRustIntegerPrimitiveFacts collapses overflow to overflow_release: OverflowAction → model the Rust Reference overflow cases as a source-spec disposition carrier and bind that carrier in spec_facts.
| rust_primitive_fact_bundle(facts_node: rust_integer_facts_node(facts: facts)) | ||
| PrimitiveFactBundle { | ||
| substrate_carrier: rust_integer_facts_node(facts: facts), | ||
| spec_facts: [ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| fact: rust_inhabitant_atom(id: rust_repr_integer_twos_complement) | ||
| ), | ||
| rust_primitive_axis_binding( | ||
| axis: primitive_fact_axis_overflow_disposition, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Discharge of codex re-review on (1) Inline (2) Inline Net integer spec_facts now: 7/7 |
|
Review metadata
1. Story of the diffThis PR turns 2. Invariant categories
Finding — Modeling Faithfulness / facts flow forward. The PR is substrate-touching: Either bind the range axis for integer primitives now, or remove/retarget the range axis from this canonical spec-fact contract and provide the structural derivation that makes it intentionally absent from
Finding — Practice 8 fact-bundle modeling / P1 Modeling Faithfulness. Same blocking issue as above: The PR improves the shape substantially by replacing the opaque
Compliant. The Rust test helper added at
Finding — regression test does not cover the load-bearing fact flow. The added test verifies that
N/A — the diff does not edit a locked design document or reverse a locked decision; it implements the live T-33/Practice-8 direction rather than changing the design authority.
Finding — untracked staged axis. 2.5. Top-down PM intent reviewFinding. The PM-level intent behind this work is to replace opaque/hollow primitive modeling with fact-bundle modeling so target primitive facts become trustworthy homomorphism inputs; the modeling discipline frames fact modeling as producing the homomorphism’s inputs and requires external primitive facts such as width, signedness, representation, range, and encoding to be represented structurally. chatgpt-review-c9d060e0-6a8a-42… chatgpt-review-c9d060e0-6a8a-42… The PR advances that intent by replacing 3. VerdictREQUEST_CHANGES The PR is directionally right and removes the worst opaque-Node shape, but the newly canonical fact-bundle surface drops the declared integer |
openai-pro RC: `primitive_fact_axis_overflow_disposition` added in this PR is unbound without a tracked dissolution trigger. Both `range` (declared in T-33 #3474) and the new `overflow_disposition` are awaiting rust.dag substrate evolution before they can faithfully bind. Add 🟡 needs-more-work scaffold gates per P5 staged-surface discipline: - range: dissolve-on per-primitive range source carrier (range_min/range_max on RustIntegerPrimitiveFacts) - overflow_disposition: dissolve-on mode-aware carrier (OverflowDisposition<IRCarrier> or equivalent debug/release + overflow_checks flag)
|
Discharge of openai-pro REQUEST_CHANGES on (#2) Untracked staged axis
( (#1) (#3) Test does not ratchet that integer producer binds Route #1 is consistent with prior cursor APPROVE_WITH_COMMENTS and parent-tier scope direction ("don't absorb rust.dag substrate evolution into this PR; the pre-ratified-rework-must-re-confirm-shape memory point applies"). Routing the higher-weight openai-pro reviewer call to parent for explicit re-confirm. — sent from crisp-raven-275 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
68c9f42d· Trigger:schedule - Thinking:
267s wall
BLOCKING (1)
Root Cause
src/v4/std/model_core.dagPrimitive fact axes are modeled as association-list entries instead of a unique-key carrier → make spec_facts a Map/unique axis query or another shape that makes duplicate axis facts unrepresentable, then update rust.dag and the ratchet.
| type PrimitiveFactBundle { | ||
| substrate_carrier: Node | ||
| spec_facts: Node | ||
| spec_facts: List<PrimitiveFactAxisBinding> |
There was a problem hiding this comment.
BLOCKING: PrimitiveFactBundle.spec_facts is an ordered List of axis bindings, so the substrate admits duplicate or conflicting facts for the same axis and leaves the keyed-bundle invariant to convention, violating INVARIANTS.md P2 / modeling-discipline Practice 6.
|
Codex schedule review on Premise valid; held line on scope. Identical-shape finding to cursor's earlier APPROVE_WITH_COMMENTS exploratory observation on This is the 4th distinct substrate-evolution scope-expansion finding on this debt-PR (after range source carriers, mode-aware overflow carrier, and now Map-vs-List carrier shape). All three are premise-valid substrate gaps but outside the operator-ratified brief ("fill the opaque Currently parked awaiting operator re-confirm-shape ruling routed via parent jolly-eagle-492 → PM smart-lynx-142 on the prior absorb-vs-followup question (range binding). This new uniqueness finding folds into the same routing; if operator rules "absorb substrate-evolution" it covers Map-shape + range-binding + mode-aware overflow together as one coherent rework, otherwise all three stay as tracked debt for separate dispatch. No fix commit on this round per parking posture; will revisit on operator ruling. — sent from crisp-raven-275 |
…IEEE 754 spelling codex/codex-default BLOCKING (3 of 3) on #3533 HEAD f96a95b: float/complex encoding axis used `ieee754_binary64`, which is a CPython implementation tightening of the Python Language Reference's actual wording. Per Reference §3.2: "These represent machine-level double precision floating point numbers." and for complex: "as a pair of machine-level double precision floating point numbers." Rename the encoding symbols + struct fields + node field labels to `machine_representation` / `machine_double_precision` / `pair_machine_double_precision` to track reference wording exactly. The tighter IEEE 754 binary64 commitment is a CPython/ABI fact for a separate (future) implementation carrier. Findings #1 (model_core.dag not landed) and #2 (std_projection conflates source spec with std-coincidence) addressed by reply on PR: #1 is factually wrong (model_core.dag landed in PR #3474 on main, rust.dag already consumes it via #3500); #2 is the operator-ratified rust.dag pattern from the brief — if it's a cross-cutting design defect it belongs in a lane-wide RFC against rust.dag, not a python-slice-only unilateral divergence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…3533) * T-4 python.dag wave-1 — fact-bundle + ModelCore (rust.dag pattern) Authors the Python 3.14 Reference fact-bundles for int / float / complex / bool / None / NotImplemented / Ellipsis grounding into ModelCore, mirroring the rust.dag wave-1 shape (`rust_model_core_wave1`). int honestly omits the width / signedness / overflow axes — the Python reference defines int as unbounded arbitrary-precision with no overflow event, so we bind only surface_spelling + encoding rather than fabricate a width tag (per fact-bundle reseed discipline). bool binds the int-subtype encoding. Singletons bind the sole-inhabitant encoding. Lex / grammar / MVP-1 / LanguageModel scaffolding deliberately deferred — that work lands behind T-11 rust-first specialization, not python wave-1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * T-4 python.dag — drop unused imports + tighten bool bundle comment cursor/composer-2.5 review on #3533: `Set` and `primitive_fact_axis_signedness` were copy-ahead from the rust slice and never referenced; the bool bundle comment incorrectly said it "binds signedness" when it only binds surface_spelling + encoding. Drop both unused imports; rewrite the bool comment to explain why signedness is NOT bound (bool is `int`-subtype, signedness lives on the parent int fact-bundle, not re-asserted inside bool). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * T-4 python.dag — discharge codex E-6 finding: tag consumer-less ModelCore as E-6(b) staging codex/codex-default REQUEST_CHANGES on #3533 (premise valid): the diff added `python_model_core_wave1()` but no in-module consumer binds it (no PythonLanguageModel carrier yet — that lives behind T-11 rust-first specialization). Under INVARIANTS §P2 / E-6, a target-spec decl without a same-PR consumer is admissible only as the bounded E-6(b) exception with a named scaffold marker + dissolution trigger. Tighten the header status claim ("LanguageModel wave-1" → "primitive fact-bundles + a wave-1 ModelCore staging carrier", explicitly noting the E-6(b) shape) and add the inline 🟡 E-6(b) marker on `python_model_core_wave1` with dissolve-on = PythonLanguageModel binding `core:` to it. No model-logic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * T-4 python.dag — codex finding #3: track Reference §3.2 wording, not IEEE 754 spelling codex/codex-default BLOCKING (3 of 3) on #3533 HEAD f96a95b: float/complex encoding axis used `ieee754_binary64`, which is a CPython implementation tightening of the Python Language Reference's actual wording. Per Reference §3.2: "These represent machine-level double precision floating point numbers." and for complex: "as a pair of machine-level double precision floating point numbers." Rename the encoding symbols + struct fields + node field labels to `machine_representation` / `machine_double_precision` / `pair_machine_double_precision` to track reference wording exactly. The tighter IEEE 754 binary64 commitment is a CPython/ABI fact for a separate (future) implementation carrier. Findings #1 (model_core.dag not landed) and #2 (std_projection conflates source spec with std-coincidence) addressed by reply on PR: #1 is factually wrong (model_core.dag landed in PR #3474 on main, rust.dag already consumes it via #3500); #2 is the operator-ratified rust.dag pattern from the brief — if it's a cross-cutting design defect it belongs in a lane-wide RFC against rust.dag, not a python-slice-only unilateral divergence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…isBinding> → Map<Symbol, Node> uniqueness constraint — discharge E3-deferred follow-up from #3500 (codex BLOCKING #4: List admits duplicate axis bindings; std/collection already declares Map) (#3549) * WIP: ModelCore substrate-evolution (3/4): spec_facts: List<PrimitiveFactAxisB * WIP: ModelCore substrate-evolution (3/4): spec_facts: List<PrimitiveFactAxisB * WIP: ModelCore substrate-evolution (3/4): spec_facts: List<PrimitiveFactAxisB * Fix v4 primitive fact map helper syntax
…oproduct evolution + bind primitive_fact_axis_overflow_disposition — discharge E3-deferred follow-up from #3500 (OverflowAction needs debug/release mode-dependence per Rust Reference) (#3553) * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * Revert cross-lane fold rename from overflow PR * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * Refresh v4 language owns ledgers after map repair * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr * WIP: rust.dag substrate-evolution (2/4): overflow_disposition mode-aware copr
Summary
T-33 (#3474) codex RC follow-up debt-PR: replaces the opaque catch-all
PrimitiveFactBundle.spec_facts: Nodewith a structured axis-keyed bundle that consumes the declaredprimitive_fact_axis_*symbols, giving primitive facts a typed authority instead of a single unstructured Node payload.src/v4/std/model_core.dag: addPrimitiveFactAxisBinding { axis: Symbol, fact: Node }; changePrimitiveFactBundle.spec_factsfromNodetoList<PrimitiveFactAxisBinding>. Declared axis vocabulary expanded to seven canonical Symbols:width,signedness,range,encoding,surface_spelling,overflow_disposition,std_projection.src/v4/extdeps/languages/rust.dag: per-kind producers emit axis-keyed bindings covering T-4 source-spec facts —{surface_spelling, width, signedness, overflow_disposition, std_projection}{surface_spelling, width, encoding, std_projection}{surface_spelling, encoding, std_projection}primitive_fact_axis_rangeis declared as canonical vocabulary but intentionally unbound in the Rust producer: range is derivable fromwidth + signednessfor integers and is not a separately-stored T-4 fact in the current catalog. It will be bound when a primary range fact lands.v4_std_model_core_dag_smoke_test.rs: addPrimitiveFactAxisBindingto the carrier inventory; new ratchetv4_std_model_core_primitive_fact_bundle_uses_axis_keyed_spec_factsasserts the bundle/binding shape and that all seven canonical axis Symbols remain declared.Test plan
cargo test -p v3-compiler v4_std_model_core— 7/7 pass (including new axis-keyed-spec-facts ratchet).