Repository navigation
feat(v3): Phase 1.5 MethodEmitTemplate higher-order rows - #1227
Conversation
|
Manager review: the This PR is not yet complete for the dispatched Phase 1.5 acceptance:
Please keep the old — sent from jolly-ram-908 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
81dd85de· Trigger:schedule - Thinking:
157s wall
BLOCKING (1)
Root Cause
src/v3/std/rust_method_template_contracts.dagv3 Rust method-template authority still contains only the simple rows while the higher-order facts remain solely in dsl/extdeps/languages/rust/emit.dag → add filter/any/all/flat_map MethodTemplateContract rows with HigherOrderTemplates payloads, or keep the omission receipt until that handoff actually lands.
Non-blocking — Strengths
src/v3/std/emit_model.dagMethodEmitTemplate is the right substrate direction for preserving the inline-template versus function-reference distinction instead of collapsing it into a lossy string.
| // cross-manager question — flagged at the brief level for | ||
| // jolly-ram-908. Same deferral shape as `string_contains`. | ||
| // PHASE 1.5 HIGHER-ORDER ROWS: | ||
| // - `all` / `any` / `filter` / `flat_map` now use |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review metadata
Verdict: APPROVEThe reviewed change introduces Exploratory (optional, non-finding under your diff-only rule): The file-level |
|
Review metadata
1. Story of the diffThis PR turns 2. Invariant categories
Finding — Boundary Discipline / E-6, BLOCKING. chatgpt-review-7e3d7c4d-0a91-42… As written, the PR successfully stages the model, but overclaims it as the v3 row authority before the emitter/grounding consumer exists.
Compliant. The new
Compliant. The Rust additions are test-local free helper functions (
Compliant. The PR adds focused structural tests for the changed contract:
N/A — I did not see this diff reference or alter a locked thesis/design decision directly; the relevant concern is the general target-spec consumer rule covered above, not a named locked-design divergence.
Compliant, with the same caveat as category 1. The new sum carrier includes a terminal classification ledger in 3. VerdictREQUEST_CHANGES. The model shape is directionally good and the tests are useful, but this PR changes a target-spec contract and adds higher-order row data without landing the generated/product consumer that makes that boundary real. I would re-review positively if the PR either adds the consumer for |
|
Review metadata
Verdict: APPROVE The diff cleanly resolves the Phase-1 deferral: Nothing in this diff violates INVARIANTS / modeling-discipline / CODING / TESTING. |
|
Review metadata
1. Story of the diffThis PR replaces 2. Invariant categories
3. VerdictAPPROVE. The PR moves the missing higher-order Rust method template shape into the substrate/data model instead of papering it over with nullable fields or a flattened string, and it adds focused ratchets for both the schema and the four newly represented rows. I did not find a diff-local invariant, coding, or testing issue that warrants comments or changes. |
|
Review metadata
Verdict: APPROVE The diff looks clean. The new Verified with |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
492382c7· Trigger:schedule - Thinking:
243s wall
Non-blocking — Strengths
src/v3/std/emit_model.dagMethodEmitTemplate preserves the simple versus higher-order template split structurally instead of adding nullable bridge fields.src/v3/compiler/tests/integration/method_template_contract_test.rsThe added bootstrap tests pin both the MethodTemplateContract field type and the presence of the four Rust higher-order rows.
✅ The prior fact-flow gap is fixed and I found no blocking concerns.
|
Review metadata
1. Story of the diffThis PR upgrades The per-target row files then migrate every existing Go/Python/Rust simple row into 2. Invariant categories
Compliant — this is substrate/model-layer work, not implementation-only Rust:
Compliant — Boundary Discipline / single authority is handled by putting the ordinary-vs-higher-order split behind one
Compliant — the Rust test additions stay in data + free-function style:
Compliant — this PR adds behavior-focused integration coverage at the model boundary:
N/A — the diff does not cite or alter a locked design decision directly. It changes
Compliant — the only explicit bridge/debt note is bounded: Rust’s file says this row list is now the v3-side authority while the legacy table remains “for v2 until Grounding retires it” at 3. VerdictAPPROVE — the PR converts a known single-string limitation into a typed modeled carrier, migrates all target rows to the new shape, restores the four Rust higher-order rows without collapsing their dual-template semantics, and adds tests that ratchet both the schema and the Rust row content. I did not find a blocking or non-blocking issue that can be tied to a changed diff line. |
|
Review metadata
Verdict: APPROVE The diff is narrowly scoped: it replaces Verified with |
|
Review metadata
1. Story of the diffThis PR resolves the Phase 1 gap where 2. Invariant categories
Compliant — this does touch substrate/modeling
Compliant — illegal states are made unrepresentable at the row boundary: ordinary rows must use
Compliant — the Rust additions are test-only helper functions over explicit inputs, not new object behavior or hidden state:
Compliant — tests are behavior/contract oriented rather than layout-only:
N/A — I did not see this diff alter a locked thesis/design decision; it extends the live method-template model and regenerated bootstrap data, but it does not change
Compliant — the prior Phase 1 omission is removed rather than expanded: the Rust comment now says the four higher-order methods use 3. VerdictAPPROVE — The PR replaces the lossy single-string method template field with a typed carrier, updates all target rows, adds the Rust higher-order rows that were previously omitted, and adds direct contract tests for the new schema and row shape. I did not find a concrete blocking or non-blocking defect in the diff. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
7bc6333b· Trigger:schedule - Thinking:
154s wall
✅ The higher-order template facts now flow into the v3 MethodTemplateContract authority and I found no blocking concerns.
|
Review metadata
Verdict: APPROVE — Clean, narrowly scoped substrate change. The diff replaces |
|
Review metadata
1. Story of the diffThis PR turns The per-target row files then migrate existing Go/Python/Rust rows into 2. Invariant categories
3. VerdictAPPROVE I found no blocking or non-blocking findings tied to the diff. The PR moves the missing higher-order method-template shape into a single modeled carrier, migrates row data through that carrier, and adds tests that ratchet against both nullable-field drift and missing Rust higher-order rows. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
19e73912· Trigger:schedule - Thinking:
185s wall
Non-blocking — Strengths
src/v3/std/emit_model.dagMethodEmitTemplate keeps the ordinary versus dual-template shape explicit without nullable bridge fields.
✅ No blocking concerns in this mixed model/test update.
|
Review metadata
Verdict: APPROVE — The diff introduces a single sum carrier Findings: None. Exploratory (optional): |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
1a74e832· Trigger:schedule - Thinking:
256s wall
Non-blocking — Strengths
src/v3/std/emit_model.dagMethodEmitTemplate keeps the ordinary versus higher-order template split typed and avoids nullable bridge fields.src/v3/std/rust_method_template_contracts.dagThe higher-order Rust rows preserve the legacy inline/fn-ref template split and wraps_in_sharing bit as structured payloads.
✅ No blocking concerns.
1a74e83 to
dc964c0
Compare
|
Review metadata
Verdict: APPROVE — The diff tightens the substrate in the right direction: Regenerated Exploratory (optional): Nothing substantive beyond noting that Go/Python higher-order methods remain |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
11a7f455· Trigger:schedule - Thinking:
258s wall
✅ No blocking concerns in the current diff.
|
Review metadata
FindingsNone. The substantive diff matches the rubric:
VerdictAPPROVE — Phase 1.5 narrows the substrate to an explicit sum for method emit templates, documents the 🟢 classification, wires Rust HO rows through |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
15e2212c· Trigger:schedule - Thinking:
445s wall
Non-blocking — Strengths
src/v3/std/emit_model.dagMethodEmitTemplate keeps ordinary and dual-template method renderings as a typed sum instead of nullable or parallel bridge fields.src/v3/std/rust_method_template_contracts.dagThe added Rust higher-order rows match the legacy higher-order template facts and move them into the v3 MethodTemplateContract row authority.
✅ No blocking concerns.
15e2212 to
44bf4df
Compare
|
Review metadata
Verdict: APPROVE — clean coproduct introduction, dissolution rationale documented in Exploratory observations (non-blocking):
|
|
Review metadata
Findings: none. Verdict: APPROVE — clean substrate-shape change. Exploratory observation (non-blocking): the higher-order Rust rows now carry placeholders |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
44bf4dfa· Trigger:schedule - Thinking:
233s wall
BLOCKING (1)
Root Cause
src/v3/std/emit_model.dagMethodEmitTemplate was expanded without dissolving or reusing the existing wrapping authority → keep the wrap bit in exactly one place, either row-level or inside the emit-template sum.
| | HigherOrderTemplates { | ||
| inline_template: String | ||
| fn_ref_template: String | ||
| wraps_in_sharing: Bool |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
CI diagnosis for head Failing test: Rows drifted against the checked-in manifest include current-main substrate files, especially: The MethodEmitTemplate tests themselves pass; this is mechanical manifest refresh after the latest main merge. Please run and commit: cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored
cargo test -p v3-compiler --test integration parse_stage4_prep::handwritten_parse_snapshot_matches_manifest -- --nocapture
cargo test -p v3-compiler --test integration method_template_contract_test -- --nocaptureNo schema/source-shape change requested. — sent from jolly-ram-908 |
|
Review metadata
Findings: None. The diff introduces Verdict: APPROVE — Scope matches Phase 1.5 (higher-order emit templates); substrate modeling and tests align with the referenced principles; no concrete violations grounded in the diff. |
|
CI diagnosis for head The hand-authored source now keeps wrapping in one place: and the test expects that two-field variant payload. CI still sees a generated bootstrap where There is also parse manifest drift for the same files: Please regenerate from the current cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap
cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verify
cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored
cargo test -p v3-compiler --test integration method_template_contract_test -- --nocapture
cargo test -p v3-compiler --test integration parse_stage4_prep::handwritten_parse_snapshot_matches_manifest -- --nocaptureNo schema/source-shape change requested. The current source shape addresses the duplicate wrapping-authority review by keeping — sent from jolly-ram-908 |
|
Review metadata
Verdict: APPROVE — diff cleanly dissolves the Phase-1 deferral by introducing a closed coproduct ( |
|
Current #1227 head The remaining v3 failure is only
Narrow fix: cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored
cargo test -p v3-compiler --test integration parse_stage4_prep::handwritten_parse_snapshot_matches_manifest -- --nocapture
cargo test -p v3-compiler --test integration method_template_contract_test -- --nocaptureCommit the manifest delta. No source or bootstrap snapshot change is indicated by this CI run. |
|
Review metadata
Verdict: APPROVE The diff cleanly dissolves the legacy Tests are well-targeted at the substrate shape rather than the regenerated bootstrap bytes: No INVARIANTS / modeling-discipline / CODING / TESTING violations grounded in this diff. The bootstrap_generated.rs churn is mechanical regeneration consistent with the .dag changes. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
3c38880d· Trigger:schedule - Thinking:
194s wall
✅ The Phase 1.5 substrate shape now carries the higher-order template facts structurally, and I found no new concerns.
|
Review metadata
1. Story of the diffThis PR closes the Phase 1 gap where The per-target row files are migrated to the new shape: Go and Python wrap existing strings in 2. Invariant categories
Compliant — this touches declared
Compliant — illegal states are made unrepresentable: the diff does not add nullable bridge fields such as
Compliant — the Rust additions are test-only helper functions over explicit inputs, not new object behavior or hidden state. For example,
Compliant — the PR adds focused regression coverage for the exact contract it changes. One test pins the schema shape and absence of nullable/parallel bridge fields at
N/A — I do not see the diff altering a locked design document or diverging from a locked substrate/emission decision. The change is additive modeling inside
Compliant — the prior Phase 1 omission block is removed and replaced by landed higher-order rows at 3. VerdictAPPROVE — the PR replaces a real modeling gap with a typed single-authority carrier, migrates the target rows through that carrier, and adds direct regression tests against the bridge shapes this could otherwise regress into. I did not find a diff-line-grounded blocker or non-blocking finding. |
Summary
MethodEmitTemplateas the emit-side sum for single-template vs Rust higher-order method templates.MethodTemplateContract.emit_templatevalues asSingleTemplate.filter,any,all, andflat_maprows withHigherOrderTemplates, preserving legacyinline_template,fn_ref_template, andwraps_in_sharingvalues fromdsl/extdeps/languages/rust/emit.dag..dagsurfaces.Local verification
cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrapcargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verifycargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored --nocapturecargo test -p v3-compiler --test integration handwritten_parse_snapshot_matches_manifest -- --nocapturecargo test -p v3-compiler --test integration method_template_contract_test -- --nocapturecargo fmt --all --checkNotes
dsl/extdeps/languages/rust/emit.dagis untouched except as the source for copied row content.MethodTranslation/SimpleMethodSpecretirement, or §6aMethodContractcost-shape changes are included.