sg-4b

[briansrls]

Opened from session-dashboard for session zesty-seal-178.

[briansrls]

Latest diff read: this is not the SG-4b-1-fix lane we briefed.

The briefed lane was narrowly scoped:

• remove the post-#609 parallel declaration lookup from the infer helper path
• delete DeclarationLookup / find_declaration from infer_helpers.dag
• route those consumers through the canonical declaration accessor instead

But this PR is touching a different surface entirely:

• variant_payload.dag
• variant_payload_generated.rs
• std/substrate.dag
• spec/rust.dag
• dag.rs
• bootstrap snapshots

So my current read is:

• the direction may be reasonable as a general substrate-accessor cleanup
• but it is not the requested SG-4b-1-fix on the live post-#609 infer-helper debt
• it also widens the scope substantially by adding a new substrate accessor realization and bootstrap/spec churn

Concretely, the post-merge issue from #609 still appears unaddressed here:

• I do not see infer_helpers.dag / infer_helpers_generated.rs in the changed files
• so the DeclarationLookup / find_declaration authority path that merged in #609 is still live unless there is a second PR handling it

Direction:

1. if the goal is truly SG-4b-1-fix, retarget this lane to the infer helper path itself
2. if the goal is a broader substrate accessor cleanup, reframe/retitle it honestly as that lane instead of SG-4b-1-fix
3. avoid counting this as closure of the #609 follow-up unless the actual infer-helper lookup path is removed

So from my side this is a scope mismatch blocker, not yet a converged follow-up to #609.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: 3009736f · Trigger: schedule
• Thinking: 438s wall

---

Non-blocking — Strengths

• src/v3/lenses/variant_payload.dag Replacing the private find_declaration scan with declaration_by_id matches L-7 and removes a local fork of declaration lookup authority.

✅ The new accessor, Rust realization, and regenerated artifacts are consistent with the existing dense DeclarationId substrate invariant, and I did not find a blocking thesis or invariant violation in the changed lines.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: 309ca607 · Trigger: schedule
• Thinking: 319s wall

---

Non-blocking — Strengths

• src/v3/compiler/tests/integration/m1_substrate_test.rs The updated binding-count ratchet keeps declaration_by_id on the same fail-closed DB-14 substrate-accessor path as the existing Rust bindings.

✅ I did not find a new thesis, invariant, or modeling-discipline violation in the changed lines.

[briansrls]

Flagging a brief-vs-comment mismatch before retargeting.

The brief at docs/briefs/sg-4b-1-fix-declaration-lookup-cleanup.md (now on main as of the rescope, commit 510b8d1dc) explicitly scopes this lane to variant_payload.dag, not infer_helpers.dag:

Context (brief): "Per a later cleanup on main, infer_helpers.dag / infer_helpers_generated.rs no longer carry it (only TemplateArgumentLookup remains there). But the pattern persists on main in src/v3/lenses/variant_payload.dag"

Read first: "src/v3/lenses/infer_helpers.dag (sanity-check only — confirm it no longer has DeclarationLookup; the pattern migrated/was cleaned up here)"

Non-goals: "Not touching infer_helpers.dag or infer_helpers_generated.rs (those were cleaned up independently; confirm via sanity grep but do not modify)"

And the substrate-accessor extension is not scope-creep — it's Step 1 of the rescoped two-step lane, added by 510b8d1dc ("rescope SG-4b-1-fix as two-step (substrate + migration)") in direct response to reviewer feedback that substrate.dag had no declaration_by_id accessor, so consumers couldn't drop find_declaration without walking d.declarations inline instead.

Sanity-grep confirms infer_helpers.dag / infer_helpers_generated.rs already have no DeclarationLookup / find_declaration on main — the #609 follow-up debt is in variant_payload.dag, which this PR removes (0 hits after merge).

Happy to retarget if the brief itself is wrong, but from the current text the PR matches the rescoped lane exactly. Could you confirm whether the comment is based on the pre-rescope brief (pre-510b8d1dc)?

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 312s wall

---

Non-blocking — Strengths

• src/v3/lenses/variant_payload.dag This replaces the lens-local declaration-table walk with a declared substrate accessor, which matches INVARIANTS.md L-7 and removes the parallel lookup path.

✅ I did not find a thesis, invariant, or modeling-discipline violation in the changed lines.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 360s wall

---

Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)

• src/v3/lenses/variant_payload.dag The new comment says emit.rs / emit/rust_target.rs / emit/python_target.rs already fail closed on Missing, but the Python emitter still has helper paths that return Ok(None) or a direct __match fallback there, so either narrow the comment to the consumers that really error today or ratchet those paths in a follow-up C-8 cleanup.

✅ The single-authority cleanup itself is coherent: the new declaration accessor, Rust binding, regenerated outputs, and binding-count ratchet line up cleanly.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 264s wall

---

Non-blocking — Strengths

• src/v3/spec/rust.dag The new declaration_by_id binding lands inside the existing SubstrateAccessorBinding path, so the accessor is shared through the same per-target realization mechanism instead of becoming a one-off compiler escape hatch.
• src/v3/compiler/tests/integration/m1_substrate_test.rs The updated binding-universe ratchet makes missing Rust realization coverage obvious the next time a substrate accessor is added.

✅ No blocking thesis, invariant, or modeling-discipline issues found in the changed lines.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 290s wall

---

BLOCKING (1)

Root Cause

• src/v3/lenses/variant_payload.dag VariantPayloadShapeLookup still conflates "DeclarationId lookup failed" with "declaration exists but is not a payload product" → give the absent-declaration case its own diagnosed path, or diagnose immediately at the lookup boundary before returning a normal payload-shape result.

⚠️ The single-authority accessor is the right direction, but the new None path is still being normalized into a non-error result, so the cleanup is not fail-closed yet.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 433s wall

---

BLOCKING (1)

Root Cause

• src/v3/compiler/src/emit/python_target.rs Variant-payload consumers were never fully ratcheted to fail closed after the earlier lookup cleanup, so this lane now relies on a caller contract that does not yet hold -> either land the missing emitter-side diagnostics now or narrow the new comments/contracts to say C-8 is still incomplete here.

⚠️ The authority cleanup is directionally right, but the new caller-side fail-closed claim should not land until it matches the actual emitter behavior.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 256s wall

---

⚠️ Earlier C-8 fail-closed blockers on declaration_by_id miss handling remain unresolved in the current diff, so this branch is not ready to land.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 466s wall

---

Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)

• src/v3/lenses/variant_payload.dag The new comment overstates the fail-closed story: emit/rust_target.rs and emit/python_target.rs still have helper paths that turn VariantPayloadShapeLookup::Missing into Ok(None)/__match rather than a typed failure, so either narrow the claim here or finish that Lane 1e emitter cleanup.

✅ No blocking thesis, invariant, or modeling-discipline issue stood out in the changed lines.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: 53663a0a · Trigger: schedule
• Thinking: 338s wall

---

Non-blocking — Strengths

• src/v3/lenses/variant_payload.dag switching this lens to declaration_by_id removes the duplicated declaration scan and moves the authority to the reflected substrate where it belongs.
• src/v3/spec/rust.dag the Rust accessor binding keeps the lookup on the declared substrate boundary instead of leaking Dag.declarations storage shape into lens code.

⚠️ the earlier C-8 fail-closed blocker around VariantPayloadShapeLookup::Missing is still unresolved in this revision.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: ba343431 · Trigger: schedule
• Thinking: 487s wall

---

BLOCKING (1)

Root Cause

• src/v3/lenses/variant_payload.dag declaration_by_id adds a new integrity-failure case but VariantPayloadShapeLookup still erases it into Missing, and existing emit helpers already use Missing as ordinary control flow -> add a distinct corruption carrier or diagnose at the lens boundary before returning Missing.

⚠️ The single-authority cleanup is good, but the new declaration_by_id path still normalizes missing declaration ids into the ordinary Missing carrier instead of preserving a fail-closed integrity error.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: 53663a0a · Trigger: schedule
• Thinking: 392s wall

---

Non-blocking — Strengths

• src/v3/lenses/variant_payload.dag Switching the lens from a private declaration scan to declaration_by_id restores single-authority lookup for declaration facts instead of reconstructing them downstream.

✅ This looks clean: the real change is an authority cleanup plus the matching substrate/Rust accessor plumbing, and I did not find a new blocking concern in the added lines.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: 53663a0a · Trigger: schedule
• Thinking: 280s wall

---

⚠️ Earlier blocking C-8 comments on the None => Missing collapse are still unresolved; I did not find a new issue beyond those.

[briansrls]

Review metadata

• Provider / model: codex / gpt-5.4
• Commit: 759c8c78 · Trigger: schedule
• Thinking: 294s wall

---

Non-blocking — Strengths

• src/v3/lenses/variant_payload.dag Routing the lens through declaration_by_id removes the private declaration-table scan and restores single-authority lookup.
• src/v3/spec/rust.dag The new Rust binding lands on the existing SubstrateAccessorBinding mechanism, so the accessor stays on the declared substrate boundary instead of becoming a compiler-only escape hatch.

✅ The prior C-8 blocker is fixed in the current diff, and I did not find a new thesis-, invariant-, or modeling-discipline concern in the added lines.

[briansrls]

Stale — the blocking C-8 finding (lines 44/47/51/54) was addressed in commit 759c8c787:

• VariantPayloadShapeLookup now splits DeclarationMissing (substrate-integrity violation) from NotPayloadProduct (ordinary non-Conj connective). Absent declarations no longer collapse into the non-product case.
• All 6 emit call sites fail closed on DeclarationMissing with a typed diagnostic, including the two helpers (rust_target::payload_binding_for_path, python_target::emit_variant_constructor_call, python_target::default_variant_payload_binding) that previously returned Ok(None) / __match.

Recent review relays appear to be evaluating the pre-split commit (53663a0a or earlier). New codex pass against current HEAD should see both findings resolved.

[briansrls]

claude-review (director) — ✅ converged as SG-4b-1-fix (rescoped two-step)

Followed the rescoped brief correctly. Breakdown:

Change	LOC
substrate.dag (declaration_by_id accessor)	~8
dag.rs (host realization)	~8
variant_payload.dag (delete DeclarationLookup + find_declaration; route through new accessor)	~37
variant_payload_generated.rs	~11
emit consumers (rust/python targets)	~24
bootstrap_generated*.rs regen	~1950

Core hand-authored delta: ~80 LOC, matches S-M brief sizing. Rest is mechanical regen fallout from adding the substrate accessor (expected).

CI 4/4 green. Both brief steps satisfied:

• ✅ Step 1 (substrate extension): fn declaration_by_id(d, id) -> Declaration? added with host binding
• ✅ Step 2 (variant_payload migration): DeclarationLookup + find_declaration deleted; consumers route through reflected accessor

Ship it. After merge: ROADMAP "SG-4b-1 DeclarationLookup parallel authority" debt row graduates to DONE. Reflected substrate gains id-keyed declaration lookup parity with port/node accessors — a real thesis-level move toward lens-consumers-as-substrate-consumers.

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-4-pro
• Commit: ba343431 · Trigger: manual
• Conversation: View conversation

---

Verdict: APPROVE_WITH_COMMENTS — In the visible portion of pr-617.diff, this looks like bootstrap-generated ID churn rather than a substantive modeling/API/testing change, and I don’t see a concrete violation of the referenced invariants or coding/testing rules. The only caveat is that the attached diff itself appears truncated at the end, so this review is necessarily limited to the portion provided.

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-4-pro
• Commit: 53663a0a · Trigger: manual
• Conversation: View conversation

---

Verdict: APPROVE_WITH_COMMENTS — In the visible portion of src/v3/compiler/src/bootstrap_generated.rs, this reads as generated DeclarationId/PortId churn rather than a substantive modeling/API/test change, and I don’t see a concrete invariant violation to flag from the lines shown. The uploaded diff artifact itself ends with ... (truncated), so this review is necessarily limited to the visible prefix rather than the full PR.