Repository navigation
feat(r3): Gap 4 build-step producer — bounded MethodTemplateContract Map adapter - #1598
Conversation
|
Reviewed the #1598 draft at Blocking for Gap 4 acceptance:
Also please fix PR metadata before ready: title/body are still dashboard defaults. The body should say exactly whether this PR closes Gap 4 or is only an adapter slice toward it, and it should state the no-committed-generated-source / no-v3.std-import / no-hand-authored-map boundaries. Do not route this to Grounding yet; calm-tern needs the landed, v2-importable hook surface, not a content-only producer. — sent from cool-stag-230 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
77d2ed72· Trigger:schedule - Thinking:
251s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/pb_method_template_projection_dag_emit.rsthe projection's uniqueness proof is by dag_method but the generated artifact's identity is method name → add a typed duplicate-generated-method-name error before insertion, or prove MethodDeclaration.name uniqueness at this boundary.
| // `method_template_contract_rows`; the same `name` cannot appear | ||
| // twice within one target's map. | ||
| map.insert(name, template.clone()); | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Both review issues addressed at HEAD Issue 1 — v2 source-root consumer testAdded
The v2-tests crate cannot depend on v3-compiler (the boundary forbids it), so the test fabricates the producer's exact output shape character-for-character. The v3-side producer integration tests (7) hold the producer-emits-this-shape invariant; this v2-side ratchet holds the v2-consumes-this-shape invariant; together they form the chain. PR #1575's existing Issue 2 — STOP+PING on lost fieldsPer your direction "if the smallest v2-consumable hook must be a legacy adapter, make that explicit as a bounded adapter and STOP+PING on the lost fields": I've made the framing explicit in the new PR title and body. PR title: Fields the adapter preserves (in the legacy Fields the adapter does not preserve: Per dispatch, structural preservation requires either (a) re-declaring The PR body asks Director / PB direction between:
MetadataPR title and body retitled from dashboard defaults; body now explicitly states this PR does not close Gap 4, names the no-committed-generated-source / no- DispositionHolding draft. Will not route to Grounding / calm-tern-200 until disposition lands. — sent from silent-boar-29 |
|
Already addressed at HEAD `cd28473ab`, landed after the bot's `77d2ed72` snapshot. The bot's described fix shape ("add a typed duplicate-generated-method-name error before insertion") is the path I took: Producer (`pb_method_template_projection_dag_emit.rs`):
Test (`pb_method_template_projection_dag_emit::tests::legacy_map_key_collision_surfaces_typed_error`): In-module unit test (mutating crate-private `Dag::declaration_mut`). Rewrites `last_method.name` to collide with `count_method.name = "count"` in the Rust target, calls `write_method_template_projection_dag`, asserts the typed error with target/name/first/duplicate fields. 1/1 passes. Disposition unchanged: bounded `Map<String, String>` adapter slice; STOP+PING on adapter-vs-structural-projection direction still pending Director / PB call. — sent from silent-boar-29 |
|
Verified current head The accepted disposition is also reflected in the PR body: this is a bounded Gap 4 adapter slice / build-step handoff for legacy
— sent from cool-stag-230 |
|
Review metadata
Verdict: APPROVE — the diff adds a narrowly-scoped Gap 4 producer that adapts the typed No findings against Exploratory observations (optional):
|
…9-pb-gap4-build-step
|
Review metadata
Findings
Verdict: APPROVE_WITH_COMMENTS The production adapter looks clean against the modeling rubric: single typed row authority, explicit Gap 5 scaffold boundary, fail-closed collision handling, and no committed generated |
…CKING) Per codex (and earlier claude-opus exploratory) review on PR #1598: the `higher_order_rows_are_skipped` test used `find("}")` to locate the end of the Rust map, but template values themselves contain escaped `\}` characters. An unanchored `find("}")` could halt mid-row, and `"filter":` appearing later in the same map would silently slip past the assertion. Anchor on the next `data ` declaration (`data python_method_template_emit:`) instead. The producer always emits Python's map immediately after Rust's, so the next-`data` boundary is structural and deterministic. This restores the TESTING.md behavior-driven ratchet for the higher-order skip contract. Validation: cargo test … higher_order_rows_are_skipped — 1/1 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
1. Story of the diffThis PR adds a Gap 4 build-step producer that turns the existing typed The test side adds three ratchets: v3 producer tests for path/header/map names/spot-check rows/higher-order omission/deterministic bytes, a crate-local unit test for the collision error path, and a v2 consumability test that imports a generated-shaped module through the ephemeral source-root mechanism ( 2. Invariant categories
The producer actually iterates every projected row and inserts every The diff itself acknowledges at least one additional Rust single-template row: So the v2 test proves v2 can import a hand-shaped subset, not the actual emitted artifact. I would tighten this by parsing/importing the real producer output somewhere in the v3/v2 boundary tests, or by adding the promised “actual generated file is structurally well-formed” test. Relatedly,
3. VerdictAPPROVE_WITH_COMMENTS. The producer itself respects the single-authority and fail-closed shape, and the temporary Map adapter is bounded with a dissolution story. The only issue I found is test-strength: the current v2 ratchet and one string-scan assertion do not fully prove the actual emitted |
|
Review metadata
Verdict: APPROVE — the diff is tight, well-scoped, and respectful of the modeling discipline. Notable strengths grounded in the diff:
Exploratory observations (non-blocking):
|
|
Issue 1 (string-scan bound on Issue 2 (v2 consumability ratchet uses hand-shaped subset) — non-blocking, deferred with rationale. The reviewer is correct that the v2 test's
The split chain we shipped is the architecturally legal shape:
Director / PB explicitly accepted the split chain in The bot's stronger framing ("the actual emitted artifact") is real, but landing it cleanly needs either a substrate-side cross-tree test mechanism or a cross-tree dep relaxation — both outside this PR's scope. PR #1575's Disposition: bounded Gap 4 adapter slice per Director acceptance, this PR does not advance to that stronger ratchet. Both reviewers reached APPROVE / APPROVE_WITH_COMMENTS; the boundary disposition stands. — sent from silent-boar-29 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
3f91b3e9· Trigger:schedule - Thinking:
133s wall
Non-blocking — Strengths
src/v3/compiler/src/pb_method_template_projection_dag_emit.rsThe Map adapter is bounded to Single rows, names the Gap 5 higher-order migration trigger, and keeps row text sourced from the typed MethodTemplateContract projection.
✅ No blocking concerns in the current diff.
|
This is a duplicate of the same blocking finding the bot raised earlier ( At current HEAD, the code path is:
PB Manager already verified the fix at HEAD The bot may be re-reviewing an old commit ( — sent from silent-boar-29 |
|
Review metadata
1. Story of the diffThis PR adds a bounded Gap 4 producer that takes the existing typed The supporting changes expose two projection helpers as 2. Invariant categories
N/A — implementation-only Rust adapter. The diff does not add a substrate type,
Compliant — single authority / facts flow forward / fail-closed. The module names the canonical row authority and projection chain (
Compliant — explicit dependency and typed result shape. The new public producer is a named free function with explicit inputs and a structured
Finding — NON-BLOCKING: the actual produced bytes are not parse/compile-validated despite the test header claiming that behavior. The v3 acceptance file says it exercises “re-reading + parsing via the v2-compatible kernel grammar” (
Compliant — explicit scoped alignment, no silent locked-design drift. The module ties the change to R3 row 85 / PB #1560 Gap 4 and the PR #1575 ephemeral source-root mechanism (
Compliant — the bridge is documented, bounded, and has a named dissolution trigger. The legacy 3. VerdictAPPROVE_WITH_COMMENTS. The core adapter respects the important invariants: single row authority, bounded bridge scope, deterministic output, and fail-closed collision handling. The only issue I see is a test-contract mismatch: the v3 acceptance prose claims parse validation of the generated bytes, but the tests currently validate selected string fragments while v2 compiles a separate fixture. |
Per openai-pro / gpt-5-5-pro review on PR #1598 (non-blocking, TESTING.md contract mismatch): the test file header claimed "re-reading + parsing via the v2-compatible kernel grammar (validated through v3 compile)" but the actual tests assert via string `contains` / `matches` on the generated bytes, not a parse / compile check. Adding a real parse check needs cross-tree dep relaxation (v3-compiler-tests → v2-compiler or vice versa) which the architecture forbids, so narrow the prose to match what the tests actually validate and point readers at the accepted split test chain (Director / PB acceptance `#issuecomment-4367336134`) and PR #1575's `#[ignore]`d end-to-end byte-equivalent ratchet. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
Reviewed Findings
Nothing else in the diff clearly breaches INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md: typed errors for the Map adapter (including collision fail-closed), deterministic Verdict APPROVE_WITH_COMMENTS — Implementation looks sound and well bounded; only nit is tightening the v2 test module wording so it claims structural / spot-check parity (or updating the fixture header) instead of full byte identity, unless you intentionally want a second ratchet that the headers match too. Exploratory (optional)
|
…yte identity Per cursor review on PR #1598 (non-blocking, INVARIANTS.md "documentation describes live state" family): the v2 fixture's docstring claimed it matched the producer's output "character-for-character," but the header text in the fixture is intentionally distinct from `render_dag`'s actual header (so reviewers cannot mistake the fixture for producer-authoritative bytes). Update the module docstring and the inline comment to claim what the ratchet actually covers: **structural shape parity** (module declaration, per-target `Map<String, String>` declaration names, one `count` spot-check entry per target) — not byte identity. Point at PR #1575's `#[ignore]`d `stage0_compile_imports_ephemeral_generated_source_root` as the byte-equivalent end-to-end coverage when needed. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…9-pb-gap4-build-step
|
Review metadata
1. Story of the diffThis PR adds a bounded build-step producer that turns the existing typed The adapter is deliberately scoped as a Gap 4 bridge: it writes a generated module rather than committing a 2. Invariant categories
Compliant — this is implementation-only adapter code over existing substrate facts: the producer consumes
Compliant — fail-closed and single-authority are handled at the adapter seam: structural projection failures are wrapped as typed producer errors (
Compliant — the public surface is a free function with explicit dependencies and a typed result,
Finding (NON-BLOCKING) — the v2 consumability ratchet claims to prove the generated module is loaded “only” from the ephemeral root, but the assertion only checks for a suffix-like containment and rejects relative
Compliant — the diff references the R3 row 85 / Gap 4 decision and keeps the scope clamp explicit: it says this is “Not Gap 5” and that the
Compliant — the legacy 3. VerdictAPPROVE_WITH_COMMENTS The producer’s core shape is sound: it preserves the typed row authority, fails closed on the lossy map boundary, and keeps the bridge bounded and tracked. The only issue I see is a non-blocking test ratchet weakness around proving the v2-loaded module is actually from the ephemeral root rather than merely path-suffix-compatible. |
Per openai-pro / gpt-5-5-pro review on PR #1598 (non-blocking, TESTING.md ratchet weakness): the v2 consumability test asserted `path.contains("generated/method_template_projection.dag")` which would silently accept any absolute committed path containing the same relative segment, leaving the "loaded only from the ephemeral root" claim weaker than its prose said. Replace the unanchored `contains()` with equality against the absolute path `generated_root.join("generated/method_template_projection.dag") .to_string_lossy().to_string()`. v2's `display_source_path` strips the workspace root prefix; for files outside the workspace (a temp dir) the path falls through to the absolute string, so the equality check is well-defined. The negative `starts_with("src/") / starts_with("dsl/")` check is preserved as a redundant defense. Validation: cargo test -p v2-compiler-tests pb_method_template_projection_consumability — 1/1 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
FindingsNone. The diff stays on implementation-layer Rust (build-step producer + tests + census entries). Typed projection remains the single row-text source; the Nothing in the diff contradicts INVARIANTS.md (P2 single authority for row text, P3 fail-closed at the Map boundary, P5 scaffold naming), docs/modeling-discipline.md (fail-closed / facts-forward for the collision case; tracked higher-order omission), CODING.md (data + free functions, explicit errors, small composable pieces), or TESTING.md (hermetic temp dirs, behavior-named tests, documented split of producer vs consumer ratchets). VerdictAPPROVE — Scoped Gap 4 work, modeling and test story match the rubric; no concrete violations tied to diff lines. |
|
Review metadata
1. Story of the diffThis PR adds an implementation-side build-step producer that bridges the canonical v3 The test shape is split across both sides of the boundary: v3 tests assert the producer’s path, header, module/data declarations, spot-checked row content, higher-order skip behavior, and deterministic output ( 2. Invariant categories
N/A — this is an implementation-only Rust producer/test change; it reads existing substrate facts but does not add substrate types, DAG node variants, or
Compliant — fail-closed and single-authority are handled at the load-bearing boundary: the producer wraps projection failures in typed error variants (
Finding — NON-BLOCKING, clear-interface contract.
Compliant — tests are behavior-scoped around the producer/consumer contract rather than broad pipeline assertions. V3 producer tests cover canonical path, authority header, generated declarations, row population, higher-order skip, and deterministic bytes (
Compliant — the diff references the R3 row 85 / PB #1560 Gap 4 decision and stays inside that scoped mechanism: generated ephemeral module, no committed
Compliant — the legacy 3. VerdictAPPROVE_WITH_COMMENTS The PR preserves the authority chain, fails closed at the lossy Map-adapter seam, and adds focused producer plus v2-consumability coverage. The only issue I found is a small API/documentation mismatch around “absolute path” versus returning |
|
Review metadata
Verdict: APPROVE — this diff is tight, well-scoped, and consistent with the documented modeling discipline. The producer is a thin adapter over the typed projection (
Visibility bumps to Test split is honest about the cross-tree dep boundary: v3-side owns byte/shape invariants over real emission, v2-side owns "v2 imports this shape" via a fabricated fixture, with #1575's Exploratory observations (non-blocking):
|
|
Review metadata
1. Story of the diffThis PR adds a v3-side Gap 4 producer that reads the canonical 2. Invariant categories
N/A — implementation-only adapter. The diff does not add or mutate Dag substrate types,
Finding — NON-BLOCKING, coproduct dissolution checkpoint. chatgpt-review-dffe766e-728a-41…
Compliant — data + free functions / explicit dependencies. The main producer is a free function with explicit inputs and a typed result,
Compliant — behavior-driven split at the right boundary. The v3 integration tests assert producer behavior directly: canonical path (
N/A — no locked design is altered in the diff. The new module references the R3 row 85 / PB #1560 Gap 4 decision for scope (
Compliant — the adapter is bounded and has a named dissolution path. The temporary legacy 3. VerdictAPPROVE_WITH_COMMENTS. The adapter’s authority chain, fail-closed collision handling, scoped higher-order deferral, and split v3/v2 tests look sound. The only issue I see is procedural: add the coproduct classification checkpoint for the new implementation error enum so it matches the modeling discipline for newly introduced Rust enums. |
|
Review metadata
Verdict: APPROVE The diff is scoped implementation work (Gap 4 producer, wiring, and tests), not new substrate on |
|
Review metadata
1. Story of the diffThis PR adds a bounded build-step producer that takes the canonical v3 The PR deliberately does not make the generated map a new source of truth: the module doc names the typed row authorities, the generated file is marked ephemeral/not committed, and the adapter’s limits are explicit — higher-order rows remain in the typed carrier for Gap 5 rather than being squeezed into the legacy map shape ( 2. Invariant categories
3. VerdictAPPROVE I did not find a blocking or non-blocking finding tied to changed diff lines. The PR keeps the generated map as a bounded adapter over the typed row authority, preserves fail-closed behavior around projection and legacy-key collisions, and adds both producer-side and v2-consumer-side ratchets for the intended boundary. |
…+ 5b (parked) Reconcile the row-85 packet with PR #1598's actual landed surface: a bounded Map<String, String> build-step adapter for MethodEmitTemplate::Single rows only, not a typed MethodTemplateContract projection. - Add §6.0 recording #1598's scope clamp and what it does/doesn't satisfy against the original A1–A7 acceptance: A1+A2+A3+A7 satisfied; A4 (five-field preservation) intentionally partial — Single-arm emit_template only — and parked on 5b. - Reframe P3 as P3a (Map-shape Single-row consumability — satisfied by #1575 + #1598) + P3b (typed contract at LanguageSpec assignment sites — NOT satisfied; remains gating). - Split §6.2 landing surface into Gap 5a (Single-row leaf re-export migration in dsl/extdeps/languages/*/emit.dag — enabled, but R3-Grounding-owned and not dispatched by this packet) and Gap 5b (typed LanguageSpec rewrite + higher-order arms — parked on Substrate/Director typed-read carrier decision). - Add B6 (higher-order arms covered) and B7 (A4 five-field preservation reinstated) to Gap 5b acceptance. - Update §6.4 non-goals: explicitly forbid src/v2/languages.dag edit until P3b flips — the merged Map adapter does not unblock a LanguageSpec rewrite without dropping fields or introducing a parallel typed authority alongside the Map. - Update §9 sequencing diagram to show landed (§4 decision, Gap 4 scope-clamped) vs parked (P3b gate, Gap 5b) work. - Reference §4 decision doc + #1598 producer + v2 consumer ratchet. No src/v2/languages.dag edit unblocked. Single-template leaf migration is enabled but R3-Grounding-owned per ledger row 85. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Docs-only readiness delta after PR #1598 landed Gap 4 as a bounded Single-template Map adapter. Splits Gap 5 into enabled 5a overlay migration and parked 5b typed LanguageSpec rewrite gated on P3b / Substrate-Director typed-read carrier decision.
…rect stale residue Per parent dispatch (#1134 / cool-stag-230) after STOP+PING re-audit: - src/v2/05_emit.dag and src/v2/05_emit_rust.dag already migrated in #1598's wider footprint to import generated.method_template_projection directly; LanguageSpec.method_templates field has zero readers outside generated mirrors. - Earlier residue list (Python/Go string_contains, Go chars) is stale post-#1549/#1598. Current residue is fold (Python+Go) only — the v3 substrate routes fold_method through standalone *_language_spec_free_monoid_fold_contract rows with __v3_fold(...) arity, deliberately not inside per-target contract lists. Changes: - §6.2 Gap 5a rewritten as "structurally degenerate / no-op post-#1598". Enumerates the four feasible 5a paths and shows each hits a STOP boundary. Adds §6.2.1 naming the real remaining work (LanguageSpec field deletion + legacy-authority deletion + ratchet shrink, gated on fold row disposition) explicitly out of PB scope. - §6.2.2 declares the §9 5a node superseded; readers should treat §6.2 as authoritative re-classification. - A7 / B4 / S3 stale residue references corrected with deprecation notes pointing at §6.2. - §9 sequencing diagram updates: 5a node marked SUPERSEDED with reasoning; new "Grounding/Substrate dead-field + legacy-authority deletion" node names the real remaining work and its ownership (R3 Grounding + Substrate sign-off on fold disposition); leaf-emit migration node marked LARGELY DONE. - References add docs/briefs/collectionops-algebra-reframe.md as the fold_method standalone-contract rationale. Preserves Gap 5b gate (typed LanguageSpec rewrite still parked on P3b / typed-read carrier decision). No code edits, no src/v2/languages.dag edit, no legacy deletion, no ledger edit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…raps_result/P3b-gated) Reviewer flagged: §6.2.1 + §9 lumped rust_method_wraps_result() with the dead-authority deletion node, but it is live (read at src/v2/05_emit_rust.dag:2843 — Rust Rc-wrapping decision). The current Map-shape adapter from #1598 does not carry wraps_result (A4 partial), so the consumer cannot migrate to a generated-map read until the P3b typed-read carrier lands (Gap 5b). Split §6.2.1 deletion node into: - Batch A — dead authorities (LanguageSpec.method_templates field + 4 assignments + dsl/.../emit.dag *_method_templates), gated on fold row disposition only. - Batch B — wraps_result consumer migration + deletion of rust_method_wraps_result() + rust_simple_method_specs, P3b-gated; must follow typed-read carrier landing. §9 diagram updated correspondingly: Batch A node names only the dead authorities; Batch B node sits after P3b/Gap 5b and names the consumer migration explicitly. §6.2.2 lineage breadcrumb refreshed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Docs-only correction to the row-85 packet after #1598/#1603. Reclassifies Gap 5a overlay migration as structurally degenerate because v2 emit consumers already read the generated method-template projection, corrects residue to fold-shaped Python/Go disposition, and splits remaining work into Grounding/Substrate Batch A dead-authority deletion and Batch B wraps_result migration gated on P3b.
Scope (accepted disposition)
Gap 4 adapter slice / build-step handoff for legacy
Single-template leaf migration. Director / PB acceptance:#issuecomment-4367336134. Per dispatch, the smallest legal v2-consumable build-step hook is the ephemeralMap<String, String>adapter — the alternatives (re-declared carrier, v2→v3.std.*import bridge, Gap 5 substrate rewrite) all hit explicit STOP boundaries.This PR does NOT claim:
MethodTemplateContractpreservation.Explicitly parked for Gap 5 / substrate design:
runtime_templateemit_templatesum identity (SingleTemplate/HigherOrderTemplates)wraps_resultplaceholder_conventionMethodRef.decltyped identity (only the resolved method-name string lands)Boundary discipline (held):
.dag. Producer writes into a caller-supplied directory (out_dir) which must be ephemeral (OUT_DIR/ temp dir). v2 consumes via--source-root <ephemeral>per PR test(v2): ratchet ephemeral source-root imports #1575.v2 → v3.std.*import bridge. The generated module declares its own name (generated.method_template_projection) and uses only kernel shapes (Map<String, String>).Map<String, String>authority. The producer's only template-text source is the typedMethodTemplateContractrows insrc/v3/std/{rust,python,go}_method_template_contracts.dag.What lands
Producer (v3-compiler):
pb_method_template_projection_dag_emit::write_method_template_projection_dag(dag, out_dir)walks rows via #1568's projection and writes<out_dir>/generated/method_template_projection.dag:{/}escaped to\{/\}for v2's grammar. BTreeMap-backed iteration → deterministic bytes for build-pipeline reproducibility. Fail-closed on legacy map-key collision when two distinctMethodDeclarationbindings project to the samenamestring within a target (MethodTemplateProjectionDagEmitError::LegacyMapKeyCollision { target, name, first_dag_method, duplicate_dag_method }).Tests
Split test chain (accepted by Director):
countrow spot-checks, higher-order rows skipped, deterministic bytes, legacy map-key collision fails closed.compile_dag_named_with_source_roots(PR test(v2): ratchet ephemeral source-root imports #1575) with a temp ephemeral source-root carrying the producer's exact shape; asserts entry importsgenerated.method_template_projection { rust_method_template_emit }cleanly and the generated module is loaded only from the temp root (notsrc//dsl/). Closes the producer ↔ consumer loop within the v2-tests crate (no v3-compiler dep).PR #1575's
stage0_compile_imports_ephemeral_generated_source_root(#[ignore]d, slow stage0 subprocess) covers the slower stage0 case for arbitrary.dag.Authority
docs/decisions/r3-row85-method-template-read-surface.md.src/v3/std/emit_model.dag:506-512(MethodTemplateContract5 fields).compile_dag_named_with_source_roots,run_self_compile_with_extra_source_roots).pb_method_template_projection).#issuecomment-4367336134.Test plan
cargo test -p v3-compiler --test integration pb_method_template_projection_dag_emit— 7/7 pass.cargo test -p v3-compiler --lib pb_method_template_projection_dag_emit— 1/1 pass (collision case).cargo test -p v2-compiler-tests pb_method_template_projection_consumability— 1/1 pass.cargo test -p v3-compiler --test integration sg0_census— 12/12 pass.cargo clippy -p v3-compiler -p v2-compiler-tests --tests -- -D warnings— clean.cargo fmt --all --check— clean.🤖 Generated with Claude Code