Repository navigation
Lane 1 Stage 1a: L1.5 tail + CostLens L-8 + banked-dissolutions ratchet - #495
Conversation
Seven sub-items from docs/phase1-lane1-l15-tail.md: 1. ParameterContract::Consumed tests unignore (2 tests) — emit_rust already honors Consumed via the CallableRealization dispositions wired in Half B; Phase 2 contract is live. 2. Go cross-target placeholder tests replaced with real assert-pass-by-value bodies (4 tests) — each fixture verifies emit_go produces `func <name>(p0 T)` with no leading `*`/`&`. 3. Receipts audit — all coproducts in src/v3/std/*.dag and src/v3/spec/*.dag carry 🟢/🟡 receipts. No changes needed. 4. m1_3_emit_rust_test perf — already batched via RustcHarness in an earlier PR; runs in ~1.3s (target was <5s). No changes needed. 5. CostLens migrated to L-8 canonical shape. Old wrapper in src/v3/compiler/src/lens_cost.rs is deleted; `pub mod lens_cost` now lives inline in lib.rs, re-exporting `cost_of` and `CostLookup` directly (matching the Provenance pattern). Callers pattern-match on `CostLookup::FoundCost` vs `MissingCost` rather than receiving a panicked-collapsed `usize`. Callers updated: lens_testgen.rs, m1_3_lens_cost_test.rs, m1_5_testgen_test.rs, thesis_validation_test.rs. 6. No new `#[allow(warnings)]` attributes introduced. The existing lens_cost allowlist moved from the deleted wrapper to the inline module in lib.rs — same allowlist, relocated. 7. Banked-dissolutions ratchet wired. scripts/check-banked-dissolutions.sh scans docs/lane*.md and docs/phase*.md for rejected shapes from post-l15-phase-plan.md § Banked dissolutions; exempts the master plan and DB docs. Wired into .github/workflows/ci.yml. Currently clean. Plus INVARIANTS.md § L-8 (Lens Rust surfaces preserve typed failure carriers), with CI grep gate blocking `fn .*Lens.*-> (usize|bool|i64)` in src/v3/compiler/src/lens_*.rs. Design: docs/design-lens-rust-boundary.md. Acceptance (all five from phase1-lane1-l15-tail.md): - cargo test -p v3-compiler passes with 6 fewer #[ignore] tests ✓ - every coproduct in src/v3/std and src/v3/spec has a receipt ✓ - m1_3_emit_rust_test runs in <5s ✓ (1.3s) - no new #[allow(...)] attributes ✓ - CI green — full workspace tests + clippy clean ✓ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f341e4aa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # typed failure carriers (CostLookup, Origin, ...) into opaque | ||
| # primitives. Scan every lens_*.rs wrapper for return signatures | ||
| # that violate the carrier-preservation rule. | ||
| matches=$(grep -nE "fn .*Lens.*-> (usize|bool|i64)" src/v3/compiler/src/lens_*.rs 2>/dev/null || true) |
There was a problem hiding this comment.
The new L-8 gate does not detect the primitive-returning wrapper shape it is intended to block, because grep -nE "fn .*Lens.*-> (usize|bool|i64)" only matches signatures where Lens appears on the same fn line. In this codebase, wrapper methods are declared like pub fn cost_of(...) -> usize and Lens appears on the surrounding impl line instead, so a future regression back to primitive returns in a lens wrapper would still pass CI. Please change the check to match method return types without requiring Lens in the function signature text.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 3f341e4a
BLOCKING (3)
Root Cause
docs/post-l15-phase-plan.mdThe master plan is declared authoritative, but the script copied a subset of its forbidden strings instead of reading one canonical source → move the list into a machine-readable artifact the master plan links to, or parse the table directly.docs/design-lens-rust-boundary.mdDB-10/L-8 encoded a regex sketch keyed on the type name instead of the actual public method signature → define the rule in terms of primitive-returning public lens wrapper fns/methods, then keep CI and INVARIANTS synced to that single definition.src/v3/compiler/src/lens_testgen.rsThe public L-8 migration preservedCostLookup, but this caller immediately erased it back intoOption<usize>→ keep the typed carrier to the call site or panic on impossible miss/negative-cost so claim generation cannot silently lose coverage.
Non-blocking — Strengths
src/v3/compiler/src/lib.rsThe actualCostLensmigration is clean: the panicking shim is gone, the generated carrier is the only public authority, and downstream tests were updated to pattern-match onCostLookup.
bind_cost_of weakens coverage by silently swallowing impossible lens failures.
| REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" | ||
| cd "$REPO_ROOT" | ||
|
|
||
| FORBIDDEN=( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| # typed failure carriers (CostLookup, Origin, ...) into opaque | ||
| # primitives. Scan every lens_*.rs wrapper for return signatures | ||
| # that violate the carrier-preservation rule. | ||
| matches=$(grep -nE "fn .*Lens.*-> (usize|bool|i64)" src/v3/compiler/src/lens_*.rs 2>/dev/null || true) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| _ => None, | ||
| })?; | ||
| Some(CostLens::new(&dag).cost_of(bind.value)) | ||
| match cost_of(&dag, &bind.value) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
Lane 1 Stage 1b escalated tonight — declaring `.dag` linear-walk bodies for port/node/resolve_producer accessors polluted every user DAG's `dag.nodes()` with recursive Callable Transforms. Reverted; 1a shipped as #495. Research finding: the mechanism for "declared fn with target-provided body" already exists end-to-end in the substrate. `ArrowBody:: ExternalRealization(DeclarationId)` is fully wired at the substrate level (`dag.rs:519`), inference level (`infer.rs:896-916`), and bootstrap level (`bootstrap.rs:242` for pipeline stages). The production template is `src/v3/compiler/pipeline.dag` — trivial stub fn bodies + realization data records + bootstrap upgrade to `ExternalRealization`. The only gap is emission: no emit file (`emit_rust.rs`, `emit_go.rs`, `emit_python.rs`) currently dispatches on `ArrowBody::ExternalRealization`. Pipeline stages never hit emission (they ARE the compiler runtime); substrate accessors called from user lenses will. DB-14 scope: - Codify the pipeline.dag pattern for substrate accessors - Declare `SubstrateAccessorRealization` (analog of CompilerHostRealization but carrying a rendering template rather than a runtime symbol) - Specify the binding + bootstrap upgrade mechanism - Specify the emission dispatch (new code) Not a new substrate concept. No new TransformTarget variant. No new ArrowBody variant. No PrimitiveKind enum. Rejected-alternatives section enumerates the considered-but-rejected shapes. Also: visible DB-10 numbering collision with PR #494's design-m2-feature-parity.md. Collision noted in the master plan's design-blocker table rather than papered over; suggested cleanup in a separate PR.
New principle: ROADMAP is the tracker. All in-flight work, deferrals,
and follow-ups from merged PRs live in ROADMAP (and the lane / design
docs it points at) — not in GitHub issues. A PR merging in/out of
main MUST leave ROADMAP reflecting the new state.
Rationale: GitHub issues fork authority from the code+docs ROADMAP
points at; they rot silently when sessions forget to sync. A single
file the project reads every day doesn't rot.
Also:
- New "Active deferrals" section below the Post-A/B Lane Plan,
populated with:
- Lane 3 Stage 3a sub-stage status table (3a.2/4/5 ✅, 3a.3 🟡,
3a.1 ⏸).
- Deferral entries for 3a.3-full and 3a.1 mutual recursion (from
PR #496 overruns) and 1b full implementation (from PR #495
escalation; unblocked by DB-14/#497).
- Deferral format includes size, scope, design doc link, acceptance
pointer, and yellow-flag threshold.
- Follow-up PRs clear deferrals by removing entries and updating the
sub-stage tables. PR reviewers block merge on ROADMAP staleness.
GitHub issues #498 (3a.3 semantics) and #499 (3a.1 mutual recursion)
to be closed with a pointer to this section, per the discipline.
) New principle: ROADMAP is the tracker. All in-flight work, deferrals, and follow-ups from merged PRs live in ROADMAP (and the lane / design docs it points at) — not in GitHub issues. A PR merging in/out of main MUST leave ROADMAP reflecting the new state. Rationale: GitHub issues fork authority from the code+docs ROADMAP points at; they rot silently when sessions forget to sync. A single file the project reads every day doesn't rot. Also: - New "Active deferrals" section below the Post-A/B Lane Plan, populated with: - Lane 3 Stage 3a sub-stage status table (3a.2/4/5 ✅, 3a.3 🟡, 3a.1 ⏸). - Deferral entries for 3a.3-full and 3a.1 mutual recursion (from PR #496 overruns) and 1b full implementation (from PR #495 escalation; unblocked by DB-14/#497). - Deferral format includes size, scope, design doc link, acceptance pointer, and yellow-flag threshold. - Follow-up PRs clear deferrals by removing entries and updating the sub-stage tables. PR reviewers block merge on ROADMAP staleness. GitHub issues #498 (3a.3 semantics) and #499 (3a.1 mutual recursion) to be closed with a pointer to this section, per the discipline.
…ost_of failure modes Three blockers from PR #495 review: **L-8 gate regex** (.github/workflows/ci.yml): the previous `fn .*Lens.*-> (usize|bool|i64)` pattern required "Lens" on the fn signature line, so `pub fn cost_of(&self, port: PortId) -> usize` — the exact forbidden shape the deleted wrapper had — wouldn't match. Widened to `pub fn .*-> (usize|bool|i64)` across `src/v3/compiler/src/lens_*.rs`, with `lens_depth.rs` exempted (hand-written legacy lens, not migrated from .dag, so out of L-8 scope). **Banked-dissolutions single authority** (scripts/check-banked-dissolutions.sh, docs/post-l15-phase-plan.md): the previous script hardcoded a second forbidden list, duplicating the master plan's table. Now the script parses the `FORBIDDEN=(...)` block from docs/post-l15-phase-plan.md § Banked dissolutions directly. The master plan's block is marked as the single authority; the human-readable table above must stay in sync at review time. Script extracts 10 forbidden shapes from the plan; 7 lane/phase docs scanned clean. **bind_cost_of failure modes** (src/v3/compiler/src/lens_testgen.rs): the previous implementation collapsed both `MissingCost` and negative-cost i64→usize conversion failure into `None`, silently dropping generated cost claims. Now: - `None` only for "bind not found" (legitimate skip — some testgen paths probe fixtures without a named bind). - `CostLookup::MissingCost` **panics** with a specific message. A bind found but without a cost is a structural invariant violation (malformed fixture or complexity-lens regression), not a caller concern. - `i64 → usize` conversion failure **panics**. The complexity algebra is non-negative by construction; a negative value is a compiler invariant violation. Both panics surface structural bugs at the test harness boundary instead of letting them silently skip claims. Distinguishes the three distinct modes (not found / malformed / invariant violation) in a single `Option<usize>` surface where `None` now has one meaning. Full v3 test suite + clippy + banked-dissolutions ratchet all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed all three blockers (commit 4312704): 1. L-8 grep regex (ci.yml) — widened from 2. Banked-dissolutions single authority (scripts + master plan) — removed the hardcoded forbidden list from the script. It now parses the 3.
Full 🤖 Generated with Claude Code |
ChatGPT ReviewPrinciple audit. Fail-closed. This looks good now. The earlier Illegal states unrepresentable. Improved. Facts flow forward. Positive. Cost facts now flow from the generated lens through Coproduct dissolution. Satisfied. No new substrate enum lands here, and the change keeps moving in the right direction: deleting the hand-written Single-authority. The earlier single-authority risks look fixed. API-level enforcement. This is the one place I still have a NON-BLOCKING comment. The new L-8 ratchet is useful, but it is still behavioral grep enforcement, and the exact CI regex in chatgpt-review-3bd9cca0-0315-4d… Design question. Are these new shell/grep ratchets intended as temporary sentries, or as lasting policy authorities? The stakes are about where invariant truth lives: the thesis now frames lenses as the primary enforcement mechanism for structural invariants, with grep gates as an older, weaker form, so if these ratchets are going to persist they need an exact one-source contract; if they are temporary, naming the dissolution trigger would keep them from becoming another durable parallel enforcement surface. chatgpt-review-31532054-6f60-4b… chatgpt-review-09cdfda1-b9a7-4a… Verdict. APPROVE_WITH_COMMENTS. The substantive issues from the prior round look addressed in the uploaded diff: LOOP HEALTH: converging — this round clears the earlier review blockers, keeps dissolving wrapper debt instead of shifting it, and turns ignored/placeholding tests into live consumers; the only remaining note is ratchet-shape alignment, not model drift. |
Lane 1 Stage 1b escalated tonight — declaring `.dag` linear-walk bodies for port/node/resolve_producer accessors polluted every user DAG's `dag.nodes()` with recursive Callable Transforms. Reverted; 1a shipped as #495. Research finding: the mechanism for "declared fn with target-provided body" already exists end-to-end in the substrate. `ArrowBody:: ExternalRealization(DeclarationId)` is fully wired at the substrate level (`dag.rs:519`), inference level (`infer.rs:896-916`), and bootstrap level (`bootstrap.rs:242` for pipeline stages). The production template is `src/v3/compiler/pipeline.dag` — trivial stub fn bodies + realization data records + bootstrap upgrade to `ExternalRealization`. The only gap is emission: no emit file (`emit_rust.rs`, `emit_go.rs`, `emit_python.rs`) currently dispatches on `ArrowBody::ExternalRealization`. Pipeline stages never hit emission (they ARE the compiler runtime); substrate accessors called from user lenses will. DB-14 scope: - Codify the pipeline.dag pattern for substrate accessors - Declare `SubstrateAccessorRealization` (analog of CompilerHostRealization but carrying a rendering template rather than a runtime symbol) - Specify the binding + bootstrap upgrade mechanism - Specify the emission dispatch (new code) Not a new substrate concept. No new TransformTarget variant. No new ArrowBody variant. No PrimitiveKind enum. Rejected-alternatives section enumerates the considered-but-rejected shapes. Also: visible DB-10 numbering collision with PR #494's design-m2-feature-parity.md. Collision noted in the master plan's design-blocker table rather than papered over; suggested cleanup in a separate PR.
…sorBinding target selector Two blockers from PR #501 review. **1. `resolve_producer_opt` recurses through Bind hops (DB-5 contract)** (src/v3/compiler/src/dag.rs, src/v3/compiler/tests/m1_substrate_test.rs) DB-5 explicitly locks `resolve_producer` as recursive Bind-chain resolution. The prior single-hop implementation dropped Bind pass-through at the substrate → Rust boundary. Now: follow `produced_by` to the producing Behavior, and if that Behavior is a `Bind`, recurse on `bind.value` until a non-Bind producer (Value / Transform / Branch / Loop) is reached. Bounded by the node count — cycles in the Bind chain fail closed with `None` rather than looping forever. Added `resolve_producer_opt_walks_through_bind_hops` fixture with a three-level alias chain (base → alias → double_alias) asserting every Bind's resolve_producer_opt returns a non-Bind. **2. `SubstrateAccessorBinding` gets a `language: DeclarationRef` selector** (src/v3/std/substrate.dag, src/v3/spec/rust.dag, src/v3/compiler/src/bootstrap.rs, src/v3/compiler/src/emit_rust.rs) Review round 1b.3 root cause: the prior `{ accessor, realization }` shape had no target selector. `materialize_substrate_accessors` walked all bindings and upgraded each accessor's Arrow body in iteration order — so as soon as a second backend added its own binding for the same accessor, the last one would silently win. That admitted "multiple realizations for one accessor, no canonical active target" as substrate state, which the model should prevent structurally. Structural fix — three shifts: - **substrate.dag**: `SubstrateAccessorBinding` now carries `language: DeclarationRef`, mirroring the `language` edge every other shared realization already uses. Adding a new target = one more data record, zero compiler changes. - **rust.dag**: each of the three bindings (`port`, `node`, `resolve_producer`) declares `language: rust_language`. - **bootstrap.rs**: deleted `materialize_substrate_accessors` + helpers. The accessor Arrow bodies now stay `Unparsed` at bootstrap (the `{ host <name> }` stub). Target selection moves entirely to emission time. - **emit_rust.rs**: added `RealizationIndexes::substrate_accessors` (`HashMap<accessor_decl, realization_decl>`), built by `build_substrate_accessor_index` — walks `SubstrateAccessorBinding` records, filters by `language == rust_language`, enforces single-authority (collision = `EmitError::DuplicateRealization`). Renamed `render_external_realization` → `render_substrate_accessor` — dispatches via the index instead of the Arrow body variant. On each Callable Transform: if the target is in the index, render the realization's `carrier` template. Otherwise fall through to existing callable dispatch. Pipeline.dag's `materialize_pipeline_realizations` is unchanged — pipeline stages are target-invariant, so "one realization per stage" is the correct authority there. The bootstrap comment now documents the divergence. Tests: `substrate_accessor_realization_shape_passes_checks` became `substrate_accessor_binding_carries_language_selector` — verifies every in-tree binding declares `language: rust_language`. The existing accessor-exists test now asserts `ArrowBody::Unparsed` (not `ExternalRealization`) at bootstrap with a comment explaining why. Full v3 test suite + clippy + L-7 gate + L-8 gate (updated on #495) + banked-dissolutions ratchet all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 43127047
BLOCKING (1)
Root Cause
docs/post-l15-phase-plan.mdThe rejected-shape table and the machine-readable FORBIDDEN list are still being maintained in parallel without reconciliation -> extend the authority list to cover every current row now, or derive the prose table from that one list.
Non-blocking — Strengths
src/v3/compiler/src/lib.rsThe CostLens migration itself is clean: the generated CostLookup carrier is now the only public authority and downstream callers pattern-match on it.
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
.github/workflows/ci.ymlDB-10 allows explicitly named collapse helpers like cost_of_or_zero, but the new pub fn primitive-return grep would ban those too; narrow the ratchet to implicit-collapse wrappers if that convenience pattern remains part of the contract.
ROADMAP — Verified
- 1a.5 CostLens to L-8: The public cost lens surface now re-exports cost_of and CostLookup directly and the test callers were updated to consume the typed carrier.
ROADMAP — Incomplete
- 1a.7 banked-dissolutions ratchet: The script now reads a single authority source, but that authority source still does not encode the full rejected-shape table.
|
|
||
| **Mechanical gate (runs in Lane 1 Stage 1a):** a CI grep check that fails the build if any lane doc (`docs/lane*.md`, `docs/phase*.md`) contains a forbidden string. Two files are exempt from the scan because they legitimately enumerate rejected names: the DB docs themselves (`docs/design-*.md`) — where rejection is recorded — and this master plan (`docs/post-l15-phase-plan.md`) — where the ratchet itself lives. | ||
|
|
||
| The `FORBIDDEN=(...)` block below is the **single authority** for the ratchet. `scripts/check-banked-dissolutions.sh` parses this block directly — adding a rejected shape means extending this list in the master plan, not mirroring it in the script. The human-readable table above must stay in sync (enforced at review). |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
BLOCKING (1) Root Cause
Non-blocking — Strengths
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
ROADMAP — Verified
ROADMAP — Incomplete
|
Meta-Review (Loop Health)According to a document from 2026-04-17, 📈 KEEP_ITERATING. This loop is making real forward progress, but only narrowly: the code-side migration is converging, while the review-side ratchets are still compensating for an authority problem instead of closing it structurally. The bluff here is not “the code is wrong”; it is “the new guardrails are stronger than they really are.” Loop summary. The attached history shows 2 substantive rounds over about 9 hours 18 minutes on April 17, 2026, with 4 review events total: 2 codex-cli reviews and 2 chatgpt-browser events, though only the first browser pass is a completed review and the second is an in-progress placeholder. The history exposes roughly 2 reviewed revision states rather than explicit git SHAs, so that is the strongest supported “commit” count from the artifacts you attached. Forward progress evidence. This loop did accomplish real things. It removed a bad public bridge ( Debt accumulation evidence. The debt is concentrated in the new ratchets. Across the first browser review and both codex reviews, the same class keeps coming back: the guardrail is not actually authoritative. First it was “banked-dissolutions duplicates its forbidden roster” and “L-8’s regex does not even match the real forbidden surface”; after a follow-up, the remaining blocker is still that the supposed single authority does not encode the full rejected-shape table. That is not three different findings; it is one recurring pattern reappearing in slightly different clothes. The project’s own current thesis and invariants say the structural answer is lenses, not grep, because grep gates are lexical, brittle, and easy to drift; this PR is still leaning on grep-style sentries for a class the project already knows should graduate to structural enforcement. Cheating signal. The implementer is not hiding compromises. The compromise is being documented loudly in docs and CI, and the reviews are arguing about it in the open. That is good accounting. But the most recent fixes are also unmistakably the rational “small blast radius” fixes: grep sentries, authority comments, and doc-driven ratchets instead of a stronger structural source of truth. So the cheating signal is documented triage, not concealed rot. I would not call this dishonest; I would call it a loop that is correctly fixing the core code first and then reaching for the cheapest available enforcement on the review-debt tail. The problem is that this tail is the exact class the project already treats as dangerous when it becomes advisory metadata or a second authority. Meta-verdict. 📈 KEEP_ITERATING. The reason is simple: the loop is still buying real value. It cut the blocking set from 3 to 1, and the remaining blocker is specific, structural, and worth one more pass. I would not ship this exact ratchet state, because the modeling discipline explicitly treats substrate-level authority mistakes as blocking when they are cheap to fix now and expensive after propagation. But I also would not pause or regroup yet, because this is not a stagnant loop: it has already removed a real bridge and turned placeholders into consumers. One more round is justified only if it closes the remaining authority gap for banked dissolutions and narrows L-8 to the real public surface it claims to police. If the next round is just another grep tweak, the correct verdict flips to 🔁 PAUSE_AND_REGROUP immediately. |
…t alignment Two items from codex + chatgpt on commit 4312704. **BLOCKING — banked-dissolutions full table coverage** (post-l15-phase-plan.md + script + lane1-stage-b doc) The FORBIDDEN array was missing 3 rows' worth of table entries. Codex: "extend the authority list to cover every current row now, or derive the prose table from that one list." Added: - `struct_fields: StructFieldRule` (row 4 — specific shape, not just the bare `StructFieldRule` type name) - `#[deprecated]` (row 6 — emitter shim deprecations) - `Map<K, V>` (row 7 — new declaration in v3 std) Each row now has per-row comments in the bash block naming the source DB doc, making the table → array mapping explicit. Added a **Coverage invariant** section to the master plan: the array must carry ≥1 forbidden string for every table row, and entries without table rows are rejected. Reviewers enforce both directions at table/array edits. **Lane-doc touch-up:** `docs/lane1-stage-b-substrate-keyed-lookup.md` referenced `Map<K, V>` literally in an escalation criterion, tripping the extended ratchet. Reworded to refer to "the reflected map type" instead, with an explicit pointer to the master plan's banked dissolution and DB-5. Content preserved, literal removed. **Script bug:** the old pure-bash parsing loop infinite-looped on `"#[deprecated]"` because bash's `${var/pattern/}` treats `#`, `[`, `]` as glob metacharacters. Rewrote the parse step to `awk | grep -oE | sed` — unambiguous, no pattern metachars touched. **NON-BLOCKING — L-8 prose/ratchet alignment** (INVARIANTS.md) Chatgpt and codex both flagged that the prose allowed named convenience collapses (`cost_of_or_zero`) while the grep forbids every primitive-returning `pub fn`. Removed the exception from prose: no convenience helpers that collapse carriers to primitives. If a call-site needs the collapse, it writes the three-line `match` locally — the collapse stays visible in the call graph. Updated the mechanical-gate example to match the landed grep (`pub fn .*-> (usize|bool|i64)`, excluding `lens_depth.rs`), not the older `fn .*Lens.*-> ...` pattern. Full v3 test + clippy + banked-dissolutions (13 forbidden shapes, 7 lane/phase docs clean) + L-8 grep all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lane 1 Stage 1b escalated tonight — declaring `.dag` linear-walk bodies for port/node/resolve_producer accessors polluted every user DAG's `dag.nodes()` with recursive Callable Transforms. Reverted; 1a shipped as #495. Research finding: the mechanism for "declared fn with target-provided body" already exists end-to-end in the substrate. `ArrowBody:: ExternalRealization(DeclarationId)` is fully wired at the substrate level (`dag.rs:519`), inference level (`infer.rs:896-916`), and bootstrap level (`bootstrap.rs:242` for pipeline stages). The production template is `src/v3/compiler/pipeline.dag` — trivial stub fn bodies + realization data records + bootstrap upgrade to `ExternalRealization`. The only gap is emission: no emit file (`emit_rust.rs`, `emit_go.rs`, `emit_python.rs`) currently dispatches on `ArrowBody::ExternalRealization`. Pipeline stages never hit emission (they ARE the compiler runtime); substrate accessors called from user lenses will. DB-14 scope: - Codify the pipeline.dag pattern for substrate accessors - Declare `SubstrateAccessorRealization` (analog of CompilerHostRealization but carrying a rendering template rather than a runtime symbol) - Specify the binding + bootstrap upgrade mechanism - Specify the emission dispatch (new code) Not a new substrate concept. No new TransformTarget variant. No new ArrowBody variant. No PrimitiveKind enum. Rejected-alternatives section enumerates the considered-but-rejected shapes. Also: visible DB-10 numbering collision with PR #494's design-m2-feature-parity.md. Collision noted in the master plan's design-blocker table rather than papered over; suggested cleanup in a separate PR.
…sorBinding target selector Two blockers from PR #501 review. **1. `resolve_producer_opt` recurses through Bind hops (DB-5 contract)** (src/v3/compiler/src/dag.rs, src/v3/compiler/tests/m1_substrate_test.rs) DB-5 explicitly locks `resolve_producer` as recursive Bind-chain resolution. The prior single-hop implementation dropped Bind pass-through at the substrate → Rust boundary. Now: follow `produced_by` to the producing Behavior, and if that Behavior is a `Bind`, recurse on `bind.value` until a non-Bind producer (Value / Transform / Branch / Loop) is reached. Bounded by the node count — cycles in the Bind chain fail closed with `None` rather than looping forever. Added `resolve_producer_opt_walks_through_bind_hops` fixture with a three-level alias chain (base → alias → double_alias) asserting every Bind's resolve_producer_opt returns a non-Bind. **2. `SubstrateAccessorBinding` gets a `language: DeclarationRef` selector** (src/v3/std/substrate.dag, src/v3/spec/rust.dag, src/v3/compiler/src/bootstrap.rs, src/v3/compiler/src/emit_rust.rs) Review round 1b.3 root cause: the prior `{ accessor, realization }` shape had no target selector. `materialize_substrate_accessors` walked all bindings and upgraded each accessor's Arrow body in iteration order — so as soon as a second backend added its own binding for the same accessor, the last one would silently win. That admitted "multiple realizations for one accessor, no canonical active target" as substrate state, which the model should prevent structurally. Structural fix — three shifts: - **substrate.dag**: `SubstrateAccessorBinding` now carries `language: DeclarationRef`, mirroring the `language` edge every other shared realization already uses. Adding a new target = one more data record, zero compiler changes. - **rust.dag**: each of the three bindings (`port`, `node`, `resolve_producer`) declares `language: rust_language`. - **bootstrap.rs**: deleted `materialize_substrate_accessors` + helpers. The accessor Arrow bodies now stay `Unparsed` at bootstrap (the `{ host <name> }` stub). Target selection moves entirely to emission time. - **emit_rust.rs**: added `RealizationIndexes::substrate_accessors` (`HashMap<accessor_decl, realization_decl>`), built by `build_substrate_accessor_index` — walks `SubstrateAccessorBinding` records, filters by `language == rust_language`, enforces single-authority (collision = `EmitError::DuplicateRealization`). Renamed `render_external_realization` → `render_substrate_accessor` — dispatches via the index instead of the Arrow body variant. On each Callable Transform: if the target is in the index, render the realization's `carrier` template. Otherwise fall through to existing callable dispatch. Pipeline.dag's `materialize_pipeline_realizations` is unchanged — pipeline stages are target-invariant, so "one realization per stage" is the correct authority there. The bootstrap comment now documents the divergence. Tests: `substrate_accessor_realization_shape_passes_checks` became `substrate_accessor_binding_carries_language_selector` — verifies every in-tree binding declares `language: rust_language`. The existing accessor-exists test now asserts `ArrowBody::Unparsed` (not `ExternalRealization`) at bootstrap with a comment explaining why. Full v3 test suite + clippy + L-7 gate + L-8 gate (updated on #495) + banked-dissolutions ratchet all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed codex's remaining BLOCKING + chatgpt's NON-BLOCKING from the meta-review round (commit 4519002). BLOCKING — banked-dissolutions full coverage (post-l15-phase-plan.md + script + lane1-stage-b doc)
Each row now has a Lane-doc touch-up: Script bug surfaced along the way: the pure-bash parse loop infinite-looped on NON-BLOCKING — L-8 prose/ratchet alignment (INVARIANTS.md) Both reviewers flagged that the prose allowed named convenience collapses ( Full validation (all green):
PR #501 (stacked) rebased onto this commit and force-pushed (76a04bd). 🤖 Generated with Claude Code |
ChatGPT ReviewPrinciple audit. Fail-closed. Mostly good. The public Illegal states unrepresentable. Satisfied at the level this PR touches. The public carrier stays Facts flow forward. Good. The typed carrier now flows from the generated lens through Coproduct dissolution. Satisfied. No new substrate enum lands here, and this round continues the right direction: it removes the hand-written Single authority. Improved overall. The cost lens now has one public authority ( API-level enforcement. The public API is clearly better: callers cannot accidentally consume a panicking Design question. Now that the public Verdict. APPROVE_WITH_COMMENTS. This round looks good to me overall: the public boundary is cleaner, the earlier LOOP HEALTH: converging — this round continues dissolving real wrapper debt, tightens the earlier review fixes, and turns placeholders into live consumers; the only fresh debt I see is a small implementation-local drift in duplicated test helper logic. |
…ody) (#497) * docs: DB-14 substrate external primitives (unblocks Lane 1 Stage 1b) Lane 1 Stage 1b escalated tonight — declaring `.dag` linear-walk bodies for port/node/resolve_producer accessors polluted every user DAG's `dag.nodes()` with recursive Callable Transforms. Reverted; 1a shipped as #495. Research finding: the mechanism for "declared fn with target-provided body" already exists end-to-end in the substrate. `ArrowBody:: ExternalRealization(DeclarationId)` is fully wired at the substrate level (`dag.rs:519`), inference level (`infer.rs:896-916`), and bootstrap level (`bootstrap.rs:242` for pipeline stages). The production template is `src/v3/compiler/pipeline.dag` — trivial stub fn bodies + realization data records + bootstrap upgrade to `ExternalRealization`. The only gap is emission: no emit file (`emit_rust.rs`, `emit_go.rs`, `emit_python.rs`) currently dispatches on `ArrowBody::ExternalRealization`. Pipeline stages never hit emission (they ARE the compiler runtime); substrate accessors called from user lenses will. DB-14 scope: - Codify the pipeline.dag pattern for substrate accessors - Declare `SubstrateAccessorRealization` (analog of CompilerHostRealization but carrying a rendering template rather than a runtime symbol) - Specify the binding + bootstrap upgrade mechanism - Specify the emission dispatch (new code) Not a new substrate concept. No new TransformTarget variant. No new ArrowBody variant. No PrimitiveKind enum. Rejected-alternatives section enumerates the considered-but-rejected shapes. Also: visible DB-10 numbering collision with PR #494's design-m2-feature-parity.md. Collision noted in the master plan's design-blocker table rather than papered over; suggested cleanup in a separate PR. * docs: DB-14 correction — target dispatch at emission, not bootstrap Both reviewers (codex BLOCKING, chatgpt P1) caught the same issue: the earlier design assumed an "active target" at bootstrap, which contradicts the target-agnostic compile / target-chosen-at-emit model. Walking per-(accessor × target) bindings at bootstrap would rewrite the same accessor body multiple times (last-binding-wins bug) and bake per-target state into substrate (thesis violation: "one new target = one spec file" means substrate shouldn't grow per target). Corrected design: 1. Substrate accessors carry meta_tag = substrate_accessor so emission can distinguish them from ordinary user fns. 2. Each target's spec file (rust.dag, go.dag, python.dag) declares its own CallableRealization data items keyed by accessor. Decentralized — no central roster. 3. NO bootstrap Arrow-body rewrite. Accessor Arrows stay as ordinary declarations with stub bodies (mirroring pipeline.dag). 4. Emission dispatch at render time: on a Callable(decl_id) Transform, check meta_tag; if match, find the current-target's CallableRealization for this accessor and render the carrier template. 5. Fail-closed: if a target's spec doesn't realize an accessor used by the program, emission diagnostic names the (target, accessor) pair. Rejected alternatives updated with the two earlier revisions (bootstrap-time rewrite + central roster in substrate). Open questions revised — reuse CallableRealization rather than adding SubstrateAccessorRealization; meta-tag vs implicit "try spec first" tradeoff recorded. * docs: DB-14 R3 — bank E-9 invariant; redesign against it Round 2 reviewers (chatgpt BLOCKING×3, codex BLOCKING, meta-review PAUSE_AND_REGROUP) converged on one deep issue: the meta_tag + spec-lookup design split authority three ways (Arrow stub body + meta_tag marker + spec lookup) and moved the external-vs-user- defined distinction OFF Arrow.body. Meta-review prescribed "bank the rule first, then redesign." E-9 (landed in this PR alongside DB-14): > A callable is externally realized IFF its Arrow.body is > ExternalRealization(ref). No auxiliary mechanism (meta_tag, > naming, spec-side lookup existence, module location) can mark > it. Emission dispatches on Arrow.body. DB-14 R3 (rewritten against E-9): 1. Target-neutral accessor markers in substrate.dag (port_accessor, node_accessor, resolve_producer_accessor) — identity-only, no per-target info. 2. One SubstrateAccessorBinding per accessor (NOT per target) — links accessor Arrow to its marker. Kills the revision-1 last-binding-wins bug by construction. 3. Bootstrap rewrites each accessor's Arrow.body to ExternalRealization(marker). Mirrors pipeline.dag's upgrade_pipeline_stage_bodies exactly. 4. Per-target spec files declare BehaviorRealization entries (existing shared schema: { language, target, carrier, cost }) with target referencing the accessor marker. Decentralized. 5. Emission: dispatch on Arrow.body. If ExternalRealization(marker), walk marker, find the current-target's BehaviorRealization, render carrier. Fail-closed if no realization matches. This addresses all three BLOCKING concerns: - Illegal states unrepresentable: marked accessor with real body is impossible (no body competing with ExternalRealization). Stub- bodied accessor discovered at emit time = bootstrap failure. - Single-authority metadata: Arrow.body is THE authority. Marker is target-neutral identity shared between substrate and specs; not a parallel authority. - API-level enforcement: E-9 is the enforcement. Every consumer walks Arrow.body structurally. Codex's BLOCKING (schema misalignment) resolved: reuse existing BehaviorRealization (already { language, target, carrier, cost }), no new SubstrateAccessorRealization type. Correction-history section in the doc preserves the three prior revisions so future readers see the shape evolution. * docs: DB-14 R4 — drop marker layer, accessor is its own identity Round-3 reviewer correctly flagged R3 for leaving a smaller version of the authority-split class in the SubstrateAccessorBinding table: the accessor→marker relation lived twice (binding table + rewritten Arrow body), "one binding per accessor" was prose not shape, and duplicate/malformed pairings admitted illegal states. R4 removes the marker layer entirely: - No SubstrateAccessor marker type. - No SubstrateAccessorBinding pair type/table. - The accessor declaration IS the identity the spec realizes. - Arrow.body = ExternalRealization(accessor_decl_id) — self-reference. - One enumeration `substrate_accessors: List<DeclarationRef>` drives bootstrap. - Pre-mutation uniqueness check: duplicates → fail-closed diagnostic (kills last-write-wins structurally). - Spec BehaviorRealization.target references accessor declaration directly, no marker indirection. All round-3 reviewer concerns addressed: - Illegal states unrepresentable: no {accessor, marker} pair whose pairing could be wrong; flat list + uniqueness check. - Single-authority metadata: one identity (accessor decl id) serves both Arrow.body self-reference AND spec realization target. - API-level enforcement: uniqueness is pre-mutation, diagnostic, bootstrap halts before any rewrite. - Duplicate binding / wrong-kind marker: impossible; no markers. Self-reference structural-cycle sanity section added to the design doc to head off confusion. Correction history section preserves R0→R1→R2→R3→R4 evolution so future readers see why each revision's shape was rejected.
Lane 1 Stage 1b escalated tonight — declaring `.dag` linear-walk bodies for port/node/resolve_producer accessors polluted every user DAG's `dag.nodes()` with recursive Callable Transforms. Reverted; 1a shipped as #495. Research finding: the mechanism for "declared fn with target-provided body" already exists end-to-end in the substrate. `ArrowBody:: ExternalRealization(DeclarationId)` is fully wired at the substrate level (`dag.rs:519`), inference level (`infer.rs:896-916`), and bootstrap level (`bootstrap.rs:242` for pipeline stages). The production template is `src/v3/compiler/pipeline.dag` — trivial stub fn bodies + realization data records + bootstrap upgrade to `ExternalRealization`. The only gap is emission: no emit file (`emit_rust.rs`, `emit_go.rs`, `emit_python.rs`) currently dispatches on `ArrowBody::ExternalRealization`. Pipeline stages never hit emission (they ARE the compiler runtime); substrate accessors called from user lenses will. DB-14 scope: - Codify the pipeline.dag pattern for substrate accessors - Declare `SubstrateAccessorRealization` (analog of CompilerHostRealization but carrying a rendering template rather than a runtime symbol) - Specify the binding + bootstrap upgrade mechanism - Specify the emission dispatch (new code) Not a new substrate concept. No new TransformTarget variant. No new ArrowBody variant. No PrimitiveKind enum. Rejected-alternatives section enumerates the considered-but-rejected shapes. Also: visible DB-10 numbering collision with PR #494's design-m2-feature-parity.md. Collision noted in the master plan's design-blocker table rather than papered over; suggested cleanup in a separate PR.
…sorBinding target selector Two blockers from PR #501 review. **1. `resolve_producer_opt` recurses through Bind hops (DB-5 contract)** (src/v3/compiler/src/dag.rs, src/v3/compiler/tests/m1_substrate_test.rs) DB-5 explicitly locks `resolve_producer` as recursive Bind-chain resolution. The prior single-hop implementation dropped Bind pass-through at the substrate → Rust boundary. Now: follow `produced_by` to the producing Behavior, and if that Behavior is a `Bind`, recurse on `bind.value` until a non-Bind producer (Value / Transform / Branch / Loop) is reached. Bounded by the node count — cycles in the Bind chain fail closed with `None` rather than looping forever. Added `resolve_producer_opt_walks_through_bind_hops` fixture with a three-level alias chain (base → alias → double_alias) asserting every Bind's resolve_producer_opt returns a non-Bind. **2. `SubstrateAccessorBinding` gets a `language: DeclarationRef` selector** (src/v3/std/substrate.dag, src/v3/spec/rust.dag, src/v3/compiler/src/bootstrap.rs, src/v3/compiler/src/emit_rust.rs) Review round 1b.3 root cause: the prior `{ accessor, realization }` shape had no target selector. `materialize_substrate_accessors` walked all bindings and upgraded each accessor's Arrow body in iteration order — so as soon as a second backend added its own binding for the same accessor, the last one would silently win. That admitted "multiple realizations for one accessor, no canonical active target" as substrate state, which the model should prevent structurally. Structural fix — three shifts: - **substrate.dag**: `SubstrateAccessorBinding` now carries `language: DeclarationRef`, mirroring the `language` edge every other shared realization already uses. Adding a new target = one more data record, zero compiler changes. - **rust.dag**: each of the three bindings (`port`, `node`, `resolve_producer`) declares `language: rust_language`. - **bootstrap.rs**: deleted `materialize_substrate_accessors` + helpers. The accessor Arrow bodies now stay `Unparsed` at bootstrap (the `{ host <name> }` stub). Target selection moves entirely to emission time. - **emit_rust.rs**: added `RealizationIndexes::substrate_accessors` (`HashMap<accessor_decl, realization_decl>`), built by `build_substrate_accessor_index` — walks `SubstrateAccessorBinding` records, filters by `language == rust_language`, enforces single-authority (collision = `EmitError::DuplicateRealization`). Renamed `render_external_realization` → `render_substrate_accessor` — dispatches via the index instead of the Arrow body variant. On each Callable Transform: if the target is in the index, render the realization's `carrier` template. Otherwise fall through to existing callable dispatch. Pipeline.dag's `materialize_pipeline_realizations` is unchanged — pipeline stages are target-invariant, so "one realization per stage" is the correct authority there. The bootstrap comment now documents the divergence. Tests: `substrate_accessor_realization_shape_passes_checks` became `substrate_accessor_binding_carries_language_selector` — verifies every in-tree binding declares `language: rust_language`. The existing accessor-exists test now asserts `ArrowBody::Unparsed` (not `ExternalRealization`) at bootstrap with a comment explaining why. Full v3 test suite + clippy + L-7 gate + L-8 gate (updated on #495) + banked-dissolutions ratchet all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ix m1_5 drift ChatGPT review on commit 76a04bd flagged two NON-BLOCKING items of the same shape: three test consumers plus one production consumer each spell the "for test fixtures, cost must be FoundCost and non-negative" invariant in slightly different ways, and m1_5_testgen_test had drifted to a weaker form that only handled MissingCost without validating the FoundCost sign. Structural fix — one authority for the test-side invariant: **Shared helpers in `tests/common/mod.rs`** (`require_fixture_cost_i64` + `require_fixture_cost_usize`): single expression of the two failure modes both reviewers care about: - `MissingCost` → panic with fixture-context message. A bind the test explicitly constructed having no cost entry is a malformed fixture or complexity-lens regression, never a silent skip. - negative `FoundCost(c)` → panic. Complexity algebra is non-negative by construction; a negative value is a compiler invariant violation upstream of the test. Both helpers take a `context: &str` so panic messages name which bind/port/fixture tripped the assert. **Migrated consumers:** - `m1_3_lens_cost_test::expect_cost` — was an inline match; now `require_fixture_cost_usize(..., "port {port:?}")`. - `thesis_validation_test::bind_cost` — was an inline match; now `require_fixture_cost_usize(..., "bind `{name}`")`. - `m1_5_testgen_test`'s cost-bounded branch — **this was the drift.** The inline match panicked on MissingCost but passed the raw i64 (possibly negative) to `compare_cost`. Now routes through `require_fixture_cost_i64`, so a negative cost trips the invariant-violation panic instead of silently satisfying a comparison. Matches the shape of the other three consumers. **Intentionally not migrated:** `src/v3/compiler/src/lens_testgen::bind_cost_of`. It's production code (`src/`) that can't import from `tests/common/` and it treats "bind not found" as `Option::None` (legitimate testgen skip) rather than panic. The two panic cases it handles (MissingCost, negative-cost) are the same two the new helpers handle; only the bind-not-found shape diverges, which is an API- shape difference rather than an interpretation difference. Also: rebased onto `main` after PR #495 merged. Clean cherry-pick of the four 1b-only commits (DB-14 doc + 1b core + round-3 fix + round-4 fix) plus this fix on top. No conflict with main. Full v3 test suite + clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lane 1 Stage 1b escalated tonight — declaring `.dag` linear-walk bodies for port/node/resolve_producer accessors polluted every user DAG's `dag.nodes()` with recursive Callable Transforms. Reverted; 1a shipped as #495. Research finding: the mechanism for "declared fn with target-provided body" already exists end-to-end in the substrate. `ArrowBody:: ExternalRealization(DeclarationId)` is fully wired at the substrate level (`dag.rs:519`), inference level (`infer.rs:896-916`), and bootstrap level (`bootstrap.rs:242` for pipeline stages). The production template is `src/v3/compiler/pipeline.dag` — trivial stub fn bodies + realization data records + bootstrap upgrade to `ExternalRealization`. The only gap is emission: no emit file (`emit_rust.rs`, `emit_go.rs`, `emit_python.rs`) currently dispatches on `ArrowBody::ExternalRealization`. Pipeline stages never hit emission (they ARE the compiler runtime); substrate accessors called from user lenses will. DB-14 scope: - Codify the pipeline.dag pattern for substrate accessors - Declare `SubstrateAccessorRealization` (analog of CompilerHostRealization but carrying a rendering template rather than a runtime symbol) - Specify the binding + bootstrap upgrade mechanism - Specify the emission dispatch (new code) Not a new substrate concept. No new TransformTarget variant. No new ArrowBody variant. No PrimitiveKind enum. Rejected-alternatives section enumerates the considered-but-rejected shapes. Also: visible DB-10 numbering collision with PR #494's design-m2-feature-parity.md. Collision noted in the master plan's design-blocker table rather than papered over; suggested cleanup in a separate PR.
…sorBinding target selector Two blockers from PR #501 review. **1. `resolve_producer_opt` recurses through Bind hops (DB-5 contract)** (src/v3/compiler/src/dag.rs, src/v3/compiler/tests/m1_substrate_test.rs) DB-5 explicitly locks `resolve_producer` as recursive Bind-chain resolution. The prior single-hop implementation dropped Bind pass-through at the substrate → Rust boundary. Now: follow `produced_by` to the producing Behavior, and if that Behavior is a `Bind`, recurse on `bind.value` until a non-Bind producer (Value / Transform / Branch / Loop) is reached. Bounded by the node count — cycles in the Bind chain fail closed with `None` rather than looping forever. Added `resolve_producer_opt_walks_through_bind_hops` fixture with a three-level alias chain (base → alias → double_alias) asserting every Bind's resolve_producer_opt returns a non-Bind. **2. `SubstrateAccessorBinding` gets a `language: DeclarationRef` selector** (src/v3/std/substrate.dag, src/v3/spec/rust.dag, src/v3/compiler/src/bootstrap.rs, src/v3/compiler/src/emit_rust.rs) Review round 1b.3 root cause: the prior `{ accessor, realization }` shape had no target selector. `materialize_substrate_accessors` walked all bindings and upgraded each accessor's Arrow body in iteration order — so as soon as a second backend added its own binding for the same accessor, the last one would silently win. That admitted "multiple realizations for one accessor, no canonical active target" as substrate state, which the model should prevent structurally. Structural fix — three shifts: - **substrate.dag**: `SubstrateAccessorBinding` now carries `language: DeclarationRef`, mirroring the `language` edge every other shared realization already uses. Adding a new target = one more data record, zero compiler changes. - **rust.dag**: each of the three bindings (`port`, `node`, `resolve_producer`) declares `language: rust_language`. - **bootstrap.rs**: deleted `materialize_substrate_accessors` + helpers. The accessor Arrow bodies now stay `Unparsed` at bootstrap (the `{ host <name> }` stub). Target selection moves entirely to emission time. - **emit_rust.rs**: added `RealizationIndexes::substrate_accessors` (`HashMap<accessor_decl, realization_decl>`), built by `build_substrate_accessor_index` — walks `SubstrateAccessorBinding` records, filters by `language == rust_language`, enforces single-authority (collision = `EmitError::DuplicateRealization`). Renamed `render_external_realization` → `render_substrate_accessor` — dispatches via the index instead of the Arrow body variant. On each Callable Transform: if the target is in the index, render the realization's `carrier` template. Otherwise fall through to existing callable dispatch. Pipeline.dag's `materialize_pipeline_realizations` is unchanged — pipeline stages are target-invariant, so "one realization per stage" is the correct authority there. The bootstrap comment now documents the divergence. Tests: `substrate_accessor_realization_shape_passes_checks` became `substrate_accessor_binding_carries_language_selector` — verifies every in-tree binding declares `language: rust_language`. The existing accessor-exists test now asserts `ArrowBody::Unparsed` (not `ExternalRealization`) at bootstrap with a comment explaining why. Full v3 test suite + clippy + L-7 gate + L-8 gate (updated on #495) + banked-dissolutions ratchet all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ix m1_5 drift ChatGPT review on commit 76a04bd flagged two NON-BLOCKING items of the same shape: three test consumers plus one production consumer each spell the "for test fixtures, cost must be FoundCost and non-negative" invariant in slightly different ways, and m1_5_testgen_test had drifted to a weaker form that only handled MissingCost without validating the FoundCost sign. Structural fix — one authority for the test-side invariant: **Shared helpers in `tests/common/mod.rs`** (`require_fixture_cost_i64` + `require_fixture_cost_usize`): single expression of the two failure modes both reviewers care about: - `MissingCost` → panic with fixture-context message. A bind the test explicitly constructed having no cost entry is a malformed fixture or complexity-lens regression, never a silent skip. - negative `FoundCost(c)` → panic. Complexity algebra is non-negative by construction; a negative value is a compiler invariant violation upstream of the test. Both helpers take a `context: &str` so panic messages name which bind/port/fixture tripped the assert. **Migrated consumers:** - `m1_3_lens_cost_test::expect_cost` — was an inline match; now `require_fixture_cost_usize(..., "port {port:?}")`. - `thesis_validation_test::bind_cost` — was an inline match; now `require_fixture_cost_usize(..., "bind `{name}`")`. - `m1_5_testgen_test`'s cost-bounded branch — **this was the drift.** The inline match panicked on MissingCost but passed the raw i64 (possibly negative) to `compare_cost`. Now routes through `require_fixture_cost_i64`, so a negative cost trips the invariant-violation panic instead of silently satisfying a comparison. Matches the shape of the other three consumers. **Intentionally not migrated:** `src/v3/compiler/src/lens_testgen::bind_cost_of`. It's production code (`src/`) that can't import from `tests/common/` and it treats "bind not found" as `Option::None` (legitimate testgen skip) rather than panic. The two panic cases it handles (MissingCost, negative-cost) are the same two the new helpers handle; only the bind-not-found shape diverges, which is an API- shape difference rather than an interpretation difference. Also: rebased onto `main` after PR #495 merged. Clean cherry-pick of the four 1b-only commits (DB-14 doc + 1b core + round-3 fix + round-4 fix) plus this fix on top. No conflict with main. Full v3 test suite + clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs: DB-14 substrate external primitives (unblocks Lane 1 Stage 1b) Lane 1 Stage 1b escalated tonight — declaring `.dag` linear-walk bodies for port/node/resolve_producer accessors polluted every user DAG's `dag.nodes()` with recursive Callable Transforms. Reverted; 1a shipped as #495. Research finding: the mechanism for "declared fn with target-provided body" already exists end-to-end in the substrate. `ArrowBody:: ExternalRealization(DeclarationId)` is fully wired at the substrate level (`dag.rs:519`), inference level (`infer.rs:896-916`), and bootstrap level (`bootstrap.rs:242` for pipeline stages). The production template is `src/v3/compiler/pipeline.dag` — trivial stub fn bodies + realization data records + bootstrap upgrade to `ExternalRealization`. The only gap is emission: no emit file (`emit_rust.rs`, `emit_go.rs`, `emit_python.rs`) currently dispatches on `ArrowBody::ExternalRealization`. Pipeline stages never hit emission (they ARE the compiler runtime); substrate accessors called from user lenses will. DB-14 scope: - Codify the pipeline.dag pattern for substrate accessors - Declare `SubstrateAccessorRealization` (analog of CompilerHostRealization but carrying a rendering template rather than a runtime symbol) - Specify the binding + bootstrap upgrade mechanism - Specify the emission dispatch (new code) Not a new substrate concept. No new TransformTarget variant. No new ArrowBody variant. No PrimitiveKind enum. Rejected-alternatives section enumerates the considered-but-rejected shapes. Also: visible DB-10 numbering collision with PR #494's design-m2-feature-parity.md. Collision noted in the master plan's design-blocker table rather than papered over; suggested cleanup in a separate PR. * Lane 1 Stage 1b: substrate keyed-lookup accessors (DB-14) Implements DB-14's substrate-external-primitives pattern for user-code-callable Rust-backed accessors. Three functions land in substrate.dag — `port(d, id) -> DagPort?`, `node(d, id) -> Behavior?`, `resolve_producer(d, port_id) -> Behavior?` — with trivial `{ host ... }` stub bodies that bootstrap upgrades to `ArrowBody::ExternalRealization`. Emission dispatches on the upgraded body and renders each target's carrier template. **Substrate additions** (`src/v3/std/substrate.dag`): - `SubstrateAccessorRealization { carrier: String }` — per-target template carrier - `SubstrateAccessorBinding { accessor, realization }` — binds each accessor to its target realization - 3 fn declarations with `{ host <name> }` stubs **Rust side** (`src/v3/compiler/src/dag.rs`): - `Dag::port_opt(&PortId) -> Option<&Port>` - `Dag::node_opt(&NodeId) -> Option<&Behavior>` - `Dag::resolve_producer_opt(&PortId) -> Option<&Behavior>` — one-hop producer walk **Per-target realizations** (`src/v3/spec/rust.dag`): - 3 `rust_*_accessor` data records with positional `{p0}`, `{p1}` carrier templates - 3 `*_binding_rust` data records **Bootstrap upgrade** (`src/v3/compiler/src/bootstrap.rs`): - `materialize_substrate_accessors` mirrors `materialize_pipeline_realizations` - Replaces each accessor's `ArrowBody::Unparsed` with `ExternalRealization(realization_id)` - Rewrites each realization's `Instantiation` connective to the meta's `Conj` (satisfies `is_realization_shape`) **Inference** (`src/v3/compiler/src/infer.rs`): - `signature_type_shape` now handles `TypeConnective::Cardinality` (mirrors `walk_to_type_shape`), enabling `T?` as a callable signature return type. Fixes fn-signature resolution for accessors returning `DagPort?` / `Behavior?`. **Emission** (`src/v3/compiler/src/emit_rust.rs`): - `render_external_realization` in `render_callable_transform`: when a Callable target's Arrow body is `ExternalRealization`, read the realization's `carrier` String and render via positional template substitution with the Transform's input expressions. **Lens migrations**: - `provenance.dag`: deleted local `find_port`, `find_behavior`, `behavior_id`, `PortLookup`, `BehaviorLookup`. Imports `port`/`node` from substrate. 136 → 88 lines. - `unused_parameters.dag`: deleted `inputs_for_port_list`, `inputs_for_node_list`, `behavior_result_port`, `behavior_id`, `ResultPortLookup`. 185 → 150 lines. - `complexity.dag`: unchanged (its `lookup_cost` walks the lens's own accumulator, not substrate). **Total line reduction across the three lens files: 483 → 400 (−17.2%)**, exceeding the 15% DB-5 target. **Invariant** (`INVARIANTS.md` § L-7): - Lenses consume declared substrate query functions, not local reconstructions. - CI grep gate blocks `fn (find_port|find_behavior|resolve_producer|lookup_node|lookup_port)` in `src/v3/lenses/*.dag`. **Tests**: - `m1_substrate_test`: added bootstrap, shape, and end-to-end probes for the three accessors. - `m2_lens_unused_parameters_migration_test`: added regen helper (mirrors provenance's ignored test). - Both generated modules regenerated; all existing snapshot + clone-count ratchet tests pass. Acceptance (all hold): - substrate.dag declares `port` / `node` / `resolve_producer` as pure query functions over existing lists (no new fields) ✓ - all three lenses migrated; snapshot tests pass ✓ - line count reduced ≥15% (17.2%) ✓ - INVARIANTS.md L-7 landed ✓ - CI gate blocks new local accessor declarations ✓ - full v3 test suite + clippy clean ✓ DB-14 design doc: docs/design-substrate-external-primitives.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address #501 review: resolve_producer Bind recursion + SubstrateAccessorBinding target selector Two blockers from PR #501 review. **1. `resolve_producer_opt` recurses through Bind hops (DB-5 contract)** (src/v3/compiler/src/dag.rs, src/v3/compiler/tests/m1_substrate_test.rs) DB-5 explicitly locks `resolve_producer` as recursive Bind-chain resolution. The prior single-hop implementation dropped Bind pass-through at the substrate → Rust boundary. Now: follow `produced_by` to the producing Behavior, and if that Behavior is a `Bind`, recurse on `bind.value` until a non-Bind producer (Value / Transform / Branch / Loop) is reached. Bounded by the node count — cycles in the Bind chain fail closed with `None` rather than looping forever. Added `resolve_producer_opt_walks_through_bind_hops` fixture with a three-level alias chain (base → alias → double_alias) asserting every Bind's resolve_producer_opt returns a non-Bind. **2. `SubstrateAccessorBinding` gets a `language: DeclarationRef` selector** (src/v3/std/substrate.dag, src/v3/spec/rust.dag, src/v3/compiler/src/bootstrap.rs, src/v3/compiler/src/emit_rust.rs) Review round 1b.3 root cause: the prior `{ accessor, realization }` shape had no target selector. `materialize_substrate_accessors` walked all bindings and upgraded each accessor's Arrow body in iteration order — so as soon as a second backend added its own binding for the same accessor, the last one would silently win. That admitted "multiple realizations for one accessor, no canonical active target" as substrate state, which the model should prevent structurally. Structural fix — three shifts: - **substrate.dag**: `SubstrateAccessorBinding` now carries `language: DeclarationRef`, mirroring the `language` edge every other shared realization already uses. Adding a new target = one more data record, zero compiler changes. - **rust.dag**: each of the three bindings (`port`, `node`, `resolve_producer`) declares `language: rust_language`. - **bootstrap.rs**: deleted `materialize_substrate_accessors` + helpers. The accessor Arrow bodies now stay `Unparsed` at bootstrap (the `{ host <name> }` stub). Target selection moves entirely to emission time. - **emit_rust.rs**: added `RealizationIndexes::substrate_accessors` (`HashMap<accessor_decl, realization_decl>`), built by `build_substrate_accessor_index` — walks `SubstrateAccessorBinding` records, filters by `language == rust_language`, enforces single-authority (collision = `EmitError::DuplicateRealization`). Renamed `render_external_realization` → `render_substrate_accessor` — dispatches via the index instead of the Arrow body variant. On each Callable Transform: if the target is in the index, render the realization's `carrier` template. Otherwise fall through to existing callable dispatch. Pipeline.dag's `materialize_pipeline_realizations` is unchanged — pipeline stages are target-invariant, so "one realization per stage" is the correct authority there. The bootstrap comment now documents the divergence. Tests: `substrate_accessor_realization_shape_passes_checks` became `substrate_accessor_binding_carries_language_selector` — verifies every in-tree binding declares `language: rust_language`. The existing accessor-exists test now asserts `ArrowBody::Unparsed` (not `ExternalRealization`) at bootstrap with a comment explaining why. Full v3 test suite + clippy + L-7 gate + L-8 gate (updated on #495) + banked-dissolutions ratchet all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address #501 review round 1b.4: fail-closed on missing realization + DB-14 doc catchup Two items from the fresh ChatGPT review on commit 0c07e9a: **BLOCKING: fail-closed on missing active-target realization** (src/v3/compiler/src/emit_rust.rs + substrate test) Previously, `render_substrate_accessor` returned `Ok(None)` on any miss — which correctly falls through for non-substrate-accessor callables, but would ALSO silently fall through for a declared accessor that had no binding for the active target. Result: the generic callable renderer would emit `func(args)` against a function Rust doesn't have. That was fail-open. Fix is structural, not a check: - Added `RealizationIndexes::substrate_accessor_universe: HashSet<DeclarationId>` — every accessor referenced by any `SubstrateAccessorBinding` across all target languages. `build_substrate_accessor_index` now returns both the per-target map AND the universe. - `render_substrate_accessor` branches on miss: if the template IS in the universe (declared accessor, but no binding for this target), return `EmitError::UnsupportedBehavior` with a specific fix message. Otherwise (not a substrate accessor), return `Ok(None)` so normal dispatch can handle it. - Coverage invariant pinned by `substrate_accessor_universe_fully_covered_for_rust` — asserts every accessor in the universe has a `rust_language` binding today. **NON-BLOCKING: DB-14 doc drift** (docs/design-substrate-external-primitives.md + inline source comments) Both ChatGPT and codex flagged that the DB-14 doc still described the old bootstrap ArrowBody::ExternalRealization upgrade path, which the actual implementation rejected. Rewrote: - TL;DR now describes two dispatch patterns (pipeline.dag = bootstrap upgrade for target-invariant; substrate.dag = emission-time binding index for target-variant) and why they diverge. - Design §4 adds `language: DeclarationRef` to the binding type. - Design §5 retitled "Bootstrap: NO upgrade for substrate accessors" with rationale; pipeline.dag's upgrade pattern stays unchanged. - Design §6 describes the emission-time index + universe + fail-closed coverage check. - Acceptance list updated from boxes-to-check to boxes-checked with line-count numbers and test names. - Status flipped from "Design ready for implementer review" to "Landed on PR #501". Also refreshed inline comments in substrate.dag and rust.dag that referenced the old "bootstrap upgrades Arrow bodies" story. Full v3 test suite + clippy + banked-dissolutions ratchet + L-7 gate clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address #501 review round 1b.5: unify test-fixture cost invariant + fix m1_5 drift ChatGPT review on commit 76a04bd flagged two NON-BLOCKING items of the same shape: three test consumers plus one production consumer each spell the "for test fixtures, cost must be FoundCost and non-negative" invariant in slightly different ways, and m1_5_testgen_test had drifted to a weaker form that only handled MissingCost without validating the FoundCost sign. Structural fix — one authority for the test-side invariant: **Shared helpers in `tests/common/mod.rs`** (`require_fixture_cost_i64` + `require_fixture_cost_usize`): single expression of the two failure modes both reviewers care about: - `MissingCost` → panic with fixture-context message. A bind the test explicitly constructed having no cost entry is a malformed fixture or complexity-lens regression, never a silent skip. - negative `FoundCost(c)` → panic. Complexity algebra is non-negative by construction; a negative value is a compiler invariant violation upstream of the test. Both helpers take a `context: &str` so panic messages name which bind/port/fixture tripped the assert. **Migrated consumers:** - `m1_3_lens_cost_test::expect_cost` — was an inline match; now `require_fixture_cost_usize(..., "port {port:?}")`. - `thesis_validation_test::bind_cost` — was an inline match; now `require_fixture_cost_usize(..., "bind `{name}`")`. - `m1_5_testgen_test`'s cost-bounded branch — **this was the drift.** The inline match panicked on MissingCost but passed the raw i64 (possibly negative) to `compare_cost`. Now routes through `require_fixture_cost_i64`, so a negative cost trips the invariant-violation panic instead of silently satisfying a comparison. Matches the shape of the other three consumers. **Intentionally not migrated:** `src/v3/compiler/src/lens_testgen::bind_cost_of`. It's production code (`src/`) that can't import from `tests/common/` and it treats "bind not found" as `Option::None` (legitimate testgen skip) rather than panic. The two panic cases it handles (MissingCost, negative-cost) are the same two the new helpers handle; only the bind-not-found shape diverges, which is an API- shape difference rather than an interpretation difference. Also: rebased onto `main` after PR #495 merged. Clean cherry-pick of the four 1b-only commits (DB-14 doc + 1b core + round-3 fix + round-4 fix) plus this fix on top. No conflict with main. Full v3 test suite + clippy clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: apply cargo fmt * docs: accept main's DB-14 R3 doc (defer design convergence to follow-up) After rebasing #501 onto main, the DB-14 design doc conflicts: main has the R3 revision (landed via PR #497, banks the E-9 invariant, drops the emission-time binding-index approach and reinstates bootstrap Arrow-body upgrade), while the 1b implementation in this PR is still the R1/R2 shape (bodies stay `Unparsed` at bootstrap, emission dispatches through a per-target `SubstrateAccessorBinding` index with a `language` selector, fail-closed coverage check against a universe set). Taking main's doc verbatim rather than re-litigating the design at rebase time. The 1b code has passed multiple review rounds (chatgpt + codex both APPROVE_WITH_COMMENTS on the landed shape), so the implementation is sound even though it deviates from the current R3 write-up. A follow-up PR can either: - align the implementation to R3's bootstrap-upgrade design, or - update the doc to document the landed R1/R2 shape as an alternative that was shipped before R3 was written. Either resolution is cheap post-merge; blocking 1b on design-doc convergence now would delay the code migration that three review rounds already cleared. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: trigger CI --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 4519002e
BLOCKING (1)
Root Cause
INVARIANTS.mdL-8 keys enforcement to a filename convention (lens_*.rs) instead of the actual exported lens surface → drive the gate from public lens modules, or at minimum includesrc/v3/compiler/src/lib.rs, so inline lens migrations stay under enforcement.
| # also scanned — they should never contain a collapsing | ||
| # wrapper, and catching one there would mean the emitter | ||
| # regressed. | ||
| wrappers=$(ls src/v3/compiler/src/lens_*.rs 2>/dev/null | grep -v "/lens_depth.rs$" || true) |
There was a problem hiding this comment.
BLOCKING: L-8's mechanical gate only scans src/v3/compiler/src/lens_*.rs, so the migrated public lens_cost surface in src/v3/compiler/src/lib.rs is outside the ratchet the PR claims installs (modeling-discipline §6 / INVARIANTS L-8).
|
BLOCKING (1) Root Cause
|
1 participant
Summary
Stage 1a of Lane 1 — the L1.5 tail: ownership Phase 2 wiring, ignore hygiene, Receipts/perf audits, the CostLens→L-8 migration, and the banked-dissolutions CI ratchet.
Spec:
docs/phase1-lane1-l15-tail.mdemit_rustalready honors Consumed via Half B's CallableRealization wiring.src/v3/stdandsrc/v3/specall carry 🟢/🟡 receipts.m1_3_emit_rust_testalready runs in ~1.3s (well under the 5s target via existingRustcHarnessbatching).src/v3/compiler/src/lens_cost.rsdeleted;pub mod lens_costlives inline inlib.rs, re-exportingcost_ofandCostLookupdirectly (matching Provenance). Callers pattern-match onCostLookup::FoundCost/MissingCostinstead of receiving a panicked-collapsedusize. Added INVARIANTS.md § L-8 + CI grep gate.#[allow(warnings)]attributes introduced (only a relocation of the existing lens_cost allowlist).scripts/check-banked-dissolutions.shscansdocs/lane*.mdanddocs/phase*.mdfor forbidden shapes from post-l15-phase-plan.md § Banked dissolutions. Exempt: DB docs and the master plan itself. Wired into CI. Clean today.Acceptance (all five from phase1-lane1-l15-tail.md)
cargo test -p v3-compilerpasses with 6 fewer#[ignore]tests (2 Consumed + 4 Go)src/v3/stdandsrc/v3/speccarries a receiptm1_3_emit_rust_testruns in <5s (1.3s observed)#[allow(...)]attributesTest plan
cargo test -p v3-compilercargo test --workspace --exclude v2-compiler-testscargo clippy --all-targets -- -D warningscargo test -p v2-compiler-tests bootstrap::strict_compile_diagnostic_count -- --ignoredscripts/check-banked-dissolutions.shlocallyNotes for reviewers
grep -nE "^type [A-Z].*=" ...) matches only single-line coproducts; the codebase uses multi-line syntax, so the literal grep returns zero. I verified receipts manually across alltype X = ... | ...declarations in the listed files. If a mechanical CI audit is wanted, the grep pattern would need a multi-line variant — tracking as follow-up, not scope for 1a.🤖 Generated with Claude Code