δ

[briansrls]

Opened from session-dashboard for session smart-crane-443.

[briansrls]

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

[briansrls]

claude-review · director-mode · δ (#536)

✅ Ready to merge. Clean refactor of variant-payload handling across all three emitters, lifts the structural choice into substrate per the E-5 clean-emission-contract discipline.

What's new structurally

• std/clean_emission.dag gains VariantPayloadFieldAccessRule = AccessFromPayloadBinding | OverrideNamedFieldsAtBindingSite. Target specs pick their variant:

  • rust_clean_emission → OverrideNamedFieldsAtBindingSite (Rust can't hold anonymous struct-variant payloads as first-class values)
  • go_clean_emission → AccessFromPayloadBinding (type-switch witness is a whole carrier)
  • python_clean_emission → AccessFromPayloadBinding (match value is the whole carrier)

• compiler/src/variant_payload.rs (new shared module): VariantPayloadShape { Empty, PositionalSingle, NamedFields(Vec<String>) }, VariantPayloadBinding<T> { Direct(T), Fields(HashMap<String, T>) }, variant_payload_shape(dag, variant_id) classifier. Used identically by rust/go/python emitters.

• dag.rs adds VariantPayloadFieldAccessRuleVariants cache + accessor, mirroring PatternBindingRuleVariants. Bootstrap populates typed DeclarationIds; emitters read typed handles, no name-keyed lookups.

Substrate audit

Q	Result
Q1 Cardinality	✅ NamedFields(Vec<String>) only reachable when children.len() > 0 and not single positional — structurally ≥1 by construction; Vec here is compiler-internal, not substrate
Q2 Index/handle	✅ VariantPayloadFieldAccessRuleVariants { access_from_payload_binding, override_named_fields_at_binding_site } — typed Option<DeclarationId> handles, no name strings at lookup time
Q3 Duplicated fact	✅ Previous three-way field_overrides: HashMap<PortId, HashMap<String, T>> replaced with single payload_bindings: HashMap<PortId, VariantPayloadBinding<T>> — one authority for "is this a direct rendering or a per-field route?"
Q4 Coproduct compression	✅ AccessFromPayloadBinding vs OverrideNamedFieldsAtBindingSite — each variant encodes a distinct structural cause per target
Q5 Construction authority	✅ Typed variant cache populated at bootstrap end (same shape as PatternBindingRuleVariants / VerifierOutputPolicyVariants); three emitters read via the same typed accessor. This is the named_variant_id anti-pattern recovery landing again — consistent with the PR #521 / Stage 1c discipline.
Q6 Representation duality	✅ Same fact now has one canonical rule + one typed binding surface; previous ad-hoc field_overrides divergence is dissolved

Per-target fail-closed check

• Rust's parse_variant_payload_field_access_rule rejects AccessFromPayloadBinding with "Rust requires OverrideNamedFieldsAtBindingSite for named payloads"
• Go's equivalent rejects OverrideNamedFieldsAtBindingSite with "Go requires AccessFromPayloadBinding for native sum payloads"
• Python's equivalent rejects OverrideNamedFieldsAtBindingSite with "Python requires AccessFromPayloadBinding for native match carriers"

Each rejection names the constraint structurally. No silent accepts; no "unknown variant" panics. ✅ C-8.

Tests

Four per-target regression tests with structural assertions (not substring matches on rendered output):

• emit_rust_named_single_field_payload_routes_field_access_through_binding asserts Wrapped::Wrap { inner: payload } => (payload).x AND rejects (payload).inner
• emit_go_named_single_field_payload_uses_the_variant_value asserts case Wrap: return ((v).inner).x AND rejects payload := v.inner
• emit_python_named_single_field_payload_projects_from_match_value asserts __match.inner.x AND rejects double-projection
• emit_python_named_multi_field_payload_projects_from_match_value asserts __match.right AND rejects payload.right

Asserting the NEGATIVE (what must NOT appear) is particularly good — catches regressions where the renderer goes back to synthesizing intermediate bindings.

Spec-comment discipline

Each target's <target>_clean_emission block now carries a "LIVE" note naming the walker's current behavior:

• rust.dag: "routes named variant-payload field reads through match-site field bindings because Rust cannot hold anonymous struct-variant payloads as first-class values"
• go.dag: "treats the type-switch witness v as the whole payload carrier for named payloads, while positional single-field payloads bind directly to v._0"
• python.dag: "treats __match as the whole payload carrier for named payloads, while positional single-field payloads substitute __match._0"

Matches the convention from the Stage 1c pilot (PR #494/#496/#509).

Merge-ready

No blockers. Good example of substrate growth via the E-5 rule-class expansion pattern — authoring a new rule, picking per-target variants, consuming in all three emitters in the same PR (E-6 compliance). Stage 1c consumption list advances without a spec-authoring/consumption gap.

[briansrls]

ChatGPT Review

Principle audit.

Fail-closed. This looks good. The new rule is threaded as a required CleanEmissionContract field and all three emitters reject missing, malformed, or target-illegal values through explicit emitter errors instead of silently guessing. That is the right shape for the thesis’s “emission is mechanical translation” claim: the target fact is declared in spec and consumed fail-closed, not inferred ad hoc in the emitter.

Illegal states unrepresentable. Improved. The new VariantPayloadBinding split in src/v3/compiler/src/variant_payload.rs is cleaner than the old Rust-only field_overrides map and gives the emitters a typed distinction between “this payload port is a direct expression” and “this payload port can only answer per-field projections.” The remaining weak spot is the existing bootstrap-cache pattern in Dag (Option<DeclarationId> fields populated after bootstrap), which is still convention-backed rather than fully type-enforced, but this PR does not make that pattern worse; it follows the same shape as the already-existing clean-emission caches.

Facts flow forward. Strongly satisfied. The new fact is introduced once in src/v3/std/clean_emission.dag, instantiated in all three target specs, cached centrally in Dag, parsed into per-target contract bindings, and then actually consumed in renderer logic in Rust, Go, and Python. This is exactly the kind of end-to-end forward flow the discipline wants, and it avoids the old “same idea rediscovered three times in three emitters” failure mode.

Coproduct dissolution. I don’t see a problem here. The only new substrate coproduct is VariantPayloadFieldAccessRule in src/v3/std/clean_emission.dag, and it is explicitly classified as terminal. The new Rust enums in variant_payload.rs are implementation-local emitter helpers, so they do not trigger the substrate coproduct bar.

Single authority. Better than before. The new variant_payload.rs helper gives the emitters one shared authority for payload shape/binding behavior instead of keeping separate local shape logic in each backend, and the new Dag cache keeps the rule-label → DeclarationId bridge in one place. There is still duplicated contract-parsing code across the three emitters, but that is duplicated reading of one authority, not three competing sources of truth.

API-level enforcement. Mostly satisfied. The useful part is structural: target specs must now declare the rule, and invalid rule/target combinations are rejected at parse time. The less-strong part is still the bootstrap cache resolution by declaration name in dag.rs; that remains behavioral rather than type-level. I would not block on that here because this PR is extending an existing clean-emission bridge, not introducing a brand-new escape hatch.

Design question. Is variant_payload_field_access meant to be the final contract for payload bindings, or only the field-projection slice of a broader “what a bound payload port means” contract?

What’s at stake is that this PR very cleanly solves downstream payload.field... reads, especially the Rust named-single-field case, but it is intentionally narrower than “payload binding semantics” as a whole. If named payloads in .dag are supposed to be consumable as whole values, Rust still needs a declared story for that; if they are only guaranteed to support field projection, that boundary should be written down before more consumers accumulate around the narrower interpretation.

Path to convergence. I don’t see anything here that must change before merge.

As follow-up debt, I would make one thing explicit: either add a regression showing that direct whole-payload consumption for named Rust payloads fails closed with a targeted emitter error, or broaden the contract from “field access” to full payload-binding semantics if whole-payload uses are intended to be supported. Everything else in this PR looks like real convergence work: one new declared fact, one shared helper, three target consumers, and matching regressions.

Verdict. APPROVE.

This is a clean cross-layer fix that matches both the thesis and the active modeling discipline: the target-specific behavior is declared in spec, consumed mechanically, and tested in each backend rather than recovered through backend-local heuristics. LOOP HEALTH: converging — this round dissolves prior per-emitter ad hoc behavior into one declared contract plus shared payload helpers, and it adds real consumer coverage instead of shifting debt sideways.

---

View conversation

[briansrls]

codex · gpt-5.4 · cfe54d12

⚠️ Review (blocking: 2, non-blocking: 0+/0-)

BLOCKING (2)

Root Cause

• src/v3/compiler/src/emit_go.rs The new shared payload-binding abstraction models how downstream reads are answered but not whether the match site must emit a binding at all -> keep binding-site emission separate for Go or reject EmitBindingAlways until that path is implemented.
• src/v3/compiler/src/emit_rust.rs Clean-emission tests replace the whole structural contract record instead of preserving untouched fields, so adding any new required field invalidates focused fixtures -> patch the existing record in tests or add a helper that overwrites one contract field while carrying the rest forward.

⚠️ The new variant-payload rule is the right direction, but Go currently accepts a contract state it no longer implements and the Rust focused contract test needs to be repaired for the new required field.

[briansrls]

BLOCKING: Facts flow forward is broken here: the new path no longer emits any payload binding for the accepted EmitBindingAlways contract, so Go silently drops a supported clean-emission state and can still generate an unused v compile error.

[briansrls]

BLOCKING: This makes variant_payload_field_access mandatory without updating clean_emission_rejects_rust_invalid_pattern_binding_variants, so that focused fail-closed test now stops on a missing-field error before it reaches the intended invalid-pattern assertion.

[briansrls]

codex · gpt-5.4 · cfe54d12

⚠️ Review (blocking: 2, non-blocking: 0+/0-)

BLOCKING (2)

Root Cause

• src/v3/compiler/src/emit_go.rs The new shared payload-binding abstraction models how downstream reads are answered but not whether the match site must emit a binding at all -> keep binding-site emission separate for Go or reject EmitBindingAlways until that path is implemented.
• src/v3/compiler/src/emit_rust.rs Clean-emission tests replace the whole structural contract record instead of preserving untouched fields, so adding any new required field invalidates focused fixtures -> patch the existing record in tests or add a helper that overwrites one contract field while carrying the rest forward.

⚠️ The new variant-payload rule is the right direction, but Go currently accepts a contract state it no longer implements and the Rust focused contract test needs to be repaired for the new required field.