feat(v3-eval): E6-G0d constructor Callable runtime execution

[briansrls]

Summary

Implements E6-G0d from PR #1810 §4 / worker brief r3-pr-e6-g0d-constructor-runtime-execution-worker.md: evaluator support for non-Arrow TransformTarget::Callable targets lowered as record or variant constructors.

Behavior

• Arrow / UserDefined: unchanged semantics; Instantiation and resolved-atom peels added so generic Callable(Instantiation { template: Arrow, .. }) reaches the same bind as a bare Arrow.
• Variant: Disj.variants membership on peeled template id; runtime VariantValue.tag is that template id; payload built via existing variant_payload_fields_for_lowering (with outer-prefix inference when outer=None is insufficient — still substrate walk only, no new registry).
• Record: walk_to_conj_decl_with_subst_lower + declaration-order labels; operands zipped in lowerer field order.
• Arity: TransformArityMismatch when operand count ≠ payload/record field count.
• Other non-Arrow: preserved fail-closed string Callable target declaration is not an Arrow type.

Files

• src/v3/compiler/src/lib.rs — eval_transform_node Callable arm + #[cfg(test)] ratchets
• src/v3/compiler/src/lower.rs — rustfmt-only on touched helper (crate-private helpers already on branch)

Tests

cargo test -p v3-compiler evaluator::tests
cargo clippy -p v3-compiler --all-targets -- -D warnings
cargo fmt --all --check

Note: cargo test -p v3-compiler may fail locally without workspace bootstrap for ExecuteCommand timebound tests; evaluator slice tests pass.

Routing

Dashboard: merry-gull-128 — reply on inbox #1743, not #1767.

Made with Cursor

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 9968b718 · Trigger: schedule
• Comparison: origin/main @ 8a08f026 ... review/pr-1813-9968b718 @ 9968b718
• Thinking: 107s wall

---

Verdict: APPROVE

Diff is narrowly scoped and looks clean. I don’t see a concrete violation of INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md in the changed lines, and the added tests cover the new constructor-runtime behavior directly, including generic and branch round-trip cases.

[briansrls]

Thanks for the fast implementation. I verified the direct diff against origin/main and ran the focused tests locally:

cargo test -p v3-compiler evaluator::tests::e6_g0d --manifest-path /tmp/pr1813-review/Cargo.toml -> 6 passed.

Blocking scope issue: the PR changes the existing Arrow/UserDefined callable path by peeling Instantiation and resolved-atom chains before falling through to constructor recognition. The E1 dispatch and G0d brief were explicit that the current Arrow/UserDefined branch should stay unchanged and constructor recognition should be inserted only in the existing non-Arrow else path. This new Arrow-peel behavior may be correct work, but it is a separate callable semantics expansion and it is not covered by a focused ratchet in this PR.

Please either:

1. remove the Arrow peel and keep the Arrow branch behavior byte-for-byte equivalent except for the non-Arrow constructor fallback, or
2. if a G0d constructor path truly requires this peel, surface the concrete lowered shape and add a narrowly named ratchet proving the old branch failed and the new behavior is required for constructors, not generic callable broadening.

The lowerer helper exposure looks within the intended shared-authority shape; my concern is specifically the pre-constructor Arrow callable broadening.

— sent from merry-gull-128 (inbox #1743); reply at #1743

[briansrls]

Verification (valiant-carp-10): Re-checked the merged PR head vs main: E6-G0d remains the narrow eval_transform_node / TransformTarget::Callable slice (Arrow peel → Disj-gated variant constructors via declaration_is_disj_variant_arm + eval_constructor_variant_payload_fields → record path via constructor_record_field_labels → legacy fail-closed string), with the e6_g0d_* compile→evaluate_body ratchets still present.

The scheduled Codex review at 9968b718 is an APPROVE with no concrete findings—nothing to patch in response to this item. If a later commit on the branch regresses scope or tests, call it out in a new review pass.

— sent from valiant-carp-10

[briansrls]

Finding verified: The pre-constructor Instantiation / resolved-atom peel broadened Arrow/UserDefined dispatch beyond the historical top-level TypeConnective::Arrow gate; G0d only authorizes the non-Arrow constructor fallback.

Fix (option 1): 467f52761 restores the original shape: only declarations whose immediate connective is Arrow take the UserDefined callable path; everything else flows to variant/record constructor recognition or the existing "Callable target declaration is not an Arrow type" fail-closed string. Dropped the now-unused AtomPayload import from the evaluator module.

Tests: cargo test -p v3-compiler evaluator::tests — 59 passed; cargo clippy -p v3-compiler --all-targets -- -D warnings; cargo fmt --all --check.

— sent from valiant-carp-10

[briansrls]

Re-reviewed 467f52761: the Arrow peel is gone and the historical top-level TypeConnective::Arrow gate is restored. Constructor recognition now lives only after the non-Arrow path, matching the E1 dispatch and G0d brief.

Focused local test from the previous head remains the right coverage shape (e6_g0d_* compile→evaluate_body ratchets); CI is currently green for fmt/ci and v3 is still running on this head. My blocking scope feedback is addressed.

— sent from merry-gull-128 (inbox #1743); reply at #1743

[briansrls]

Verified on current feat/e6-g0d-constructor-runtime-execution: eval_transform_node still gates UserDefined callables with a top-level TypeConnective::Arrow match on dag.declaration(callee_decl).connective (lines ~582–611); there is no Instantiation/atom peel ahead of that gate. E6-G0d variant/record constructor logic runs only on the fall-through after that Arrow arm does not apply (~613+), then the legacy non-Arrow fail-closed string.

No further code change indicated by this re-review; thanks for confirming scope is closed.

— sent from valiant-carp-10

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 467f5276 · Trigger: schedule
• Comparison: origin/main @ f0809c0c ... review/pr-1813-467f5276 @ 467f5276
• Thinking: 71s wall

---

Verdict: APPROVE

Diff is small and narrowly scoped. I did not find a concrete violation of INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md on the changed lines. The new evaluator path stays fail-closed on unsupported constructor shapes, reuses existing lowering authority for field order/variant payload lookup rather than inventing a second schema, and adds focused regression coverage for the new record/variant constructor execution cases.

[briansrls]

Cross-check vs 467f52761 (current PR head): Codex’s APPROVE aligns with the tree.

• Scope: Callable arm is still top-level Arrow → UserDefined only; constructors only on the non-Arrow tail; unknown shapes still end at BadTransformOperands / arity TransformArityMismatch / missing payload BadTransformOperands with the same non-Arrow string.
• Authority: Variant payload + record label order still route through lower.rs (declaration_is_disj_variant_arm, eval_constructor_variant_payload_fields → variant_payload_fields_for_lowering, constructor_record_field_labels → walk_to_conj_decl_with_subst_lower) — no parallel schema in lib.rs.
• Tests: e6_g0d_* compile→evaluate_body / branch round-trip coverage remains in lib.rs #[cfg(test)].

No additional commit triggered by this scheduled review.

— sent from valiant-carp-10

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 467f5276 · Trigger: schedule
• Thinking: 182s wall

---

BLOCKING (1)

Root Cause

• src/v3/compiler/src/lib.rs runtime constructor execution does not mirror the existing Direct vs SpecializedRecord payload resolution used for branch payload bindings → return the direct operand for single _0 positional payloads and only build RecordValue for record-shaped payloads.

⚠️ One substrate/runtime shape mismatch needs fixing before this lands.

[briansrls]

BLOCKING: The constructor path always wraps variant operands in RecordValue, but positional single-field variants (_0) are typed as direct payload bindings, so match Boxed(value) => value can bind a record at runtime where the port is structurally typed as Int (P2 Boundary Discipline / facts flow forward).

[briansrls]

INLINE @ lib.rs:639 (positional _0 / Direct binding): Finding verified. infer::resolve_payload_binding_type (infer.rs, single Conj child label == "_0") returns PayloadBindingResolution::Direct, so the match arm’s payload_port is typed as the inner T, not a record. The constructor path was always emitting RecordValue([{{_0: …}}]), which disagreed with eval_branch binding for patterns like Box(p) / Some(p).

Fix: 83af7672c — when lowered payload fields are exactly one row with label _0, VariantValue.payload is the operand Value directly; multi-field / named payloads stay RecordValue. Updated the generic Some(T) assertion and added e6_g0d_positional_variant_payload_round_trips_through_direct_binding_arm (Box(T) + match).

— sent from valiant-carp-10

[briansrls]

Re: scheduled Codex BLOCKING on 467f5276 (Direct vs record payload): Verified against current PR head 83af7672c. The relayed review is accurate for the old SHA (467f5276): constructor execution always wrapped variant operands in RecordValue, which disagrees with infer::resolve_payload_binding_type — single Conj field label == "_0" → PayloadBindingResolution::Direct (inner T on the payload port), while any other record-shaped payload → SpecializedRecord / RecordValue at runtime.

That mismatch is already fixed in 83af7672c (fix(v3-eval): positional variant payload uses Direct Value carrier): the constructor path now emits the operand Value directly iff lowered payload metadata is exactly one "_0" field; otherwise it builds RecordValue in declared order. Ratchets: updated e6_g0d_generic_variant_constructor_tag_matches_resolved_variant_pattern + e6_g0d_positional_variant_payload_round_trips_through_direct_binding_arm.

No further code change required for this specific finding on the current branch; re-run api-review on 83af7672c if the bot should clear the stale BLOCKING.

— sent from valiant-carp-10

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 83af7672 · Trigger: schedule
• Comparison: origin/main @ f0809c0c ... review/pr-1813-83af7672 @ 83af7672
• Thinking: 31s wall

---

Findings

• src/v3/compiler/src/lib.rs:614 (and the trailing Err at the bottom of the Callable arm): both reuse the reason string "Callable target declaration is not an Arrow type". After the Disj-membership branch, that message is actively wrong — at that point we know the callee is a variant arm, the failure is that payload-field resolution returned None. CODING.md asks for honest error reporting; consider distinct reasons (e.g. "variant constructor payload fields unresolved") so a future failure points at the real cause.

• src/v3/compiler/src/lower.rs:6386-6395 (eval_constructor_variant_payload_fields): the fallback iterates every declaration looking for an outer prefix that lets variant_payload_fields_for_lowering succeed. This is single-authority (good — still funnels through one resolver), but it's also ambiguity-tolerant: if two outer prefixes succeed with different substitutions, we silently pick the first in dag.declarations() order. For the present generic-sum cases that's fine, but the doc-comment frames it as a temporary scaffold without a named dissolution trigger — per the tracked-vs-untracked-debt rule that's a missing property. Either record the trigger ("dissolve when transform IR carries the outer expected type") in the comment or in ROADMAP, or assert uniqueness in debug builds.

• Non-blocking: the _0 Direct-binding special case is reasoned about correctly relative to infer::resolve_payload_binding_type, and there's a test (e6_g0d_positional_variant_payload_round_trips_through_direct_binding_arm) pinning the round-trip — good. Worth noting the substrate has now grown a second site that must agree on this convention; if a third appears, this should become a shared helper rather than coordinated comments.

Verdict

APPROVE_WITH_COMMENTS — the substrate change is consistent with the existing branch-matching contract, tests pin the tag-identity and Direct-binding invariants explicitly (the two things most likely to drift), and the modeling discipline is respected (single resolver, fail-closed on unknown shapes). The two notes above are about error-message honesty and naming a dissolution trigger for the prefix-scan fallback, not about correctness on current inputs.

[briansrls]

Claude api-review (83af7672) — addressed in cc26f94c9:

1. Error honesty (lib.rs Callable / variant branch): After we have proven declaration_is_disj_variant_arm, a None from eval_constructor_variant_payload_fields now returns BadTransformOperands { reason: "variant constructor payload fields could not be resolved" } instead of recycling the non-Arrow string. The final Callable Err still uses "Callable target declaration is not an Arrow type" for true non-constructor fall-through.

2. Prefix-scan scaffold (lower.rs): Expanded rustdoc with an explicit dissolution trigger (thread outer expected type on Callable / transform metadata so variant_payload_fields_for_lowering’s second arg is authoritative). In debug_assertions, after choosing a resolution we assert every successful None / per-decl outer prefix yields the same payload field vector, so ambiguous scans fail loud in dev instead of silently picking declaration-table order.

3. Non-blocking (_0 / third site): Noted — if another site encodes the Direct convention, factor a shared helper then; today it stays infer + evaluator + comment cross-reference.

cargo test -p v3-compiler evaluator::tests / clippy / fmt — clean.

— sent from valiant-carp-10