D

[briansrls]

Opened from session-dashboard for session zesty-carp-794.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 87e7404124

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Keep ResolvedIdentifier compatibility until callers migrate

Renaming AtomPayload::ResolvedIdentifier to ResolvedByStructure/ResolvedByName here breaks the build immediately because the rest of the compiler still constructs and matches ResolvedIdentifier (for example lower.rs:538 and infer.rs:168 in this same commit), so cargo check -p v3-compiler fails with dozens of E0599 errors. This change needs to be landed with call-site migrations (or a temporary compatibility variant) in the same commit to keep the branch buildable.

Useful? React with 👍 / 👎.

[briansrls]

claude-review · director-mode · D (#544) — AtomPayload resolution provenance split

⚠️ Q4 dissolution shape is right, but E-6 violation: no first consumer lands with the split.

What changed

AtomPayload::ResolvedIdentifier(DeclarationId) splits into two variants:

• ResolvedByStructure(DeclarationId) — lowering-time structural edges (parameter refinement, narrowed refinement, refined-generic substitution)
• ResolvedByName(DeclarationId) — name-keyed fallback via resolve_pending_identifiers

AtomPayload::resolved_id() -> Option<DeclarationId> helper treats both variants identically for consumers that don't need the distinction.

Four call sites updated:

• lower.rs::lower_parameter_refinement → ResolvedByStructure
• lower.rs::build_narrowed_refinement → ResolvedByStructure
• lower.rs::run_identifier_sweep → ResolvedByName
• infer.rs::materialize_substituted_refined_decl → ResolvedByStructure

Substrate audit (Q4 coproduct dissolution)

Per the substrate-principle-audit memo, Q4 splits a compressed variant when N distinct causes need downstream distinction:

Check	Result
Are the causes structurally distinct?	✅ yes — one is lowering-side typed wiring, the other is post-sweep name resolution
Is there a downstream consumer that pattern-matches on the distinction?	❌ no — resolved_id() helper collapses them
Can downstream code today detect the provenance?	✅ yes (via explicit match on variant), but nothing does

Q4 mechanically landed right, but E-6 (no substrate change without a same-PR consumer) is violated: the split lands without anything that actually CARES about the distinction. The resolved_id() helper explicitly tells consumers "you can ignore the split" — which is the opposite of what a legitimate Q4 recovery should do.

The likely motivating consumer (not in this PR)

α (#533) §8 Spec reading protocol says "The walker MUST NOT call dag.declaration_by_name(…) at emission time" — i.e., emission should only see structurally-resolved references, never name-fallback ones. This split provides the substrate surface needed to implement that discipline as a structural ratchet: post-sweep, any ResolvedByName atom reachable from emission is a bug.

If that's the motivating use case, it's a fine Q4 split — but the first consumer needs to land either in this PR or as a tightly-scheduled follow-up:

• Option A (preferred): add the audit in this PR. A tests/emit_sees_only_structural_resolutions.rs that walks a representative emitted DAG and asserts zero ResolvedByName atoms reachable. Fails today if any exist; the fix then forces the structural-walk discipline to actually land.
• Option B: document the named dissolution trigger in the AtomPayload rustdoc ("first consumer: post-#XXX emission audit"), add a ROADMAP follow-up with a yellow-flag threshold, and note why the resolved_id() helper is transitional until the audit lands (not a permanent collapse).

Additional concerns

1. Compatibility of resolved_id(): the helper treats the two variants identically. If the motivating consumer is "emission should never see ResolvedByName," the helper itself becomes the escape hatch — consumers use resolved_id() to avoid the distinction. That undermines the split. Either narrow the helper to resolved_by_structure_id() + resolved_id_any_provenance(), OR drop the helper and force every consumer to pattern-match explicitly.

2. No design-doc pointer: the PR description is "Opened from session-dashboard for session zesty-carp-794." — no DB number, no design rationale, no reference to α α #533 §8 or feedback memos. For a substrate change splitting a core variant, the design context should be in a design doc or ROADMAP follow-up entry. Post-merge audits will struggle to reconstruct why this split exists.

3. Rustdoc on the old ResolvedIdentifier mentioned the split discipline ("Structurally distinct from the unresolved form — no Option field hiding a phase coproduct"). That framing applied to the unresolved-vs-resolved split. The new rustdoc extends the rationale ("unresolved stubs, structural references, and name-fallback references are on separate variants") — correct framing, but not tied to a consumer.

Merge path

Either (Option A) add a first consumer in this PR OR (Option B) name the dissolution trigger explicitly with a follow-up ROADMAP entry. Without one of those, this is a preemptive split that adds coproduct surface with no enforcement.

The underlying substrate decision is right — emission-visible provenance is the kind of thing α §8 wants to lock structurally. Just needs to close the loop between substrate and consumer before landing.

[briansrls]

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

[briansrls]

claude-review · director-mode · D (#544) — rebase to unblock

Main now has commit 59d510847 fixing the Layer 2 ratchet false-positive on cost_dag_compiles_cleanly. Rebase onto main to pick it up.

The AtomPayload provenance-split E-6 concern from my prior review still stands separately — the split needs a first consumer before merging.

[briansrls]

codex · gpt-5.4 · 68bd42db

✅ Review (blocking: 0, non-blocking: 0+/0-)

ROADMAP — Verified

• DB-17 reference-resolution provenance: ResolvedByName is now reflected in substrate.dag, exposed through lens_structural_resolution, and covered by consumer/test updates, so the scheduled-deletion row matches the shipped surface.

✅ No blocking issues; the provenance split is modeled as a first-class substrate fact and the touched lowering, inference, emitter, reflection, and test paths all carry it forward consistently.

[briansrls]

claude-review · director-mode · D (#544) — merge-ready

E-6 concern from my prior review fully addressed. The first consumer landed structurally:

New consumer: lenses/structural_resolution.dag::name_keyed_references

fn check_name_keyed_payload(decl: Declaration, payload: AtomPayload) -> List<NameKeyedReference> =
  match payload {
    ...
    ResolvedByStructure(id) => empty()
    ResolvedByName(id) => cons({declaration: decl.id, resolved_to: id, span: decl.span}, empty())
    ...
  }

This is exactly the E-6 consumer the split needed: the lens explicitly pattern-matches on the distinction — ResolvedByName atoms are collected as a reportable surface, ResolvedByStructure atoms are not. Per α §8's discipline ("The walker MUST NOT call declaration_by_name at emission time"), this lens is now the structural audit that will eventually enforce it.

Scope grew appropriately

Original PR was 4 files; final is 17 files:

• First consumer: lenses/structural_resolution.dag (+49) with name_keyed_references + NameKeyedReference carrier
• Regenerated: lens_structural_resolution_generated.rs (+66) — you ran the regen binary per generated-file discipline. Good.
• Three emitters updated: emit_rust.rs, emit_go.rs, emit_python.rs each +~8-14 lines consuming the variants
• Pipeline updates: infer.rs +68/-34, lower.rs +71/-32 — the new variants threaded through cleanly
• Tests: 5 test files updated to reflect the new carrier shape

166 occurrences of ResolvedByStructure / ResolvedByName across the diff — deep, honest wiring.

Inline limitation declared honestly

"The DB-17 user-range policy boundary is not yet a first-class reflected fact, so callers that specifically want 'user only' must currently filter these results by their own source-range policy."

Good inline receipt — names the limitation, doesn't silently paper over it. If/when the DB-17 user-range fact lands, the lens gets a natural filter.

CI green

All three checks passing post-rebase (picked up 59d510847 from main).

Merge when ready. Strong iteration from E-6 concern to complete consumer landing.

[briansrls]

claude-review — LGTM modulo two clarification asks.

What's right

• Correct DB-17 substrate addition. New ResolvedByName(DeclarationId) variant on AtomPayload in src/v3/std/substrate.dag — minimal, structurally sound, walks via the lens at src/v3/lenses/structural_resolution.dag. Matches DB-17's narrowed scope (user-range AtomPayload fallback only).
• ROADMAP rows correctly distinguish the two debt classes. User-range ResolvedByName (DB-17 covers; dissolves at M2 module scoping) vs compiler-internal declaration_by_name call sites (DB-17 explicitly does NOT cover; dissolves at self-hosting or per-site substrate amendment). This is exactly the discipline feedback_dissolve_bridges + project_node_to_std are after — not conflating helper-name with debt-class.
• Lens consumer wired. lens_structural_resolution::name_keyed_references walks declaration atoms for ResolvedByName; the substrate fact has at least one structural reader, not just lowering-side emission.
• Defense-in-depth doc on structural_resolution.dag explains the historical R13 fix anchor without preserving the review history (live-state invariant respected).

Asks before merge

1. Why all 3 emit_*.rs files? 17 files is sizable. If emit_rust.rs, emit_go.rs, emit_python.rs changes are mechanical-from-variant-addition (each emitter learning to render the new AtomPayload::ResolvedByName(DeclarationId) variant), that's expected and clean. If any emitter encodes domain logic for ResolvedByName rendering (e.g., rendering the resolution chain as a comment), call that out — it would be a place where the new variant leaks into target representation rather than staying as substrate fact.
2. Confirm fail-closed on a failed resolution. A reference that cannot resolve to any declaration must surface as a Diagnostic per C-8, not a silent absence. If lane_d test fixtures cover this path, name them in the PR body. If not, add one.

Sequencing risk

• Both D #544 and B #545 (B) touch std/substrate.dag + dag.rs. Not structural conflict (D adds ResolvedByName variant; B adds BoolPortRef + reshapes WorkflowEffect carriers); first-to-land forces the other to rebase. No discipline call needed — order doesn't matter materially.

[briansrls]

Principle audit.

Fail-closed. This looks good. The changed lowering path still fails through the existing diagnostic sweep rather than introducing a silent success path: unresolved identifier stubs are rewritten to a resolved form inside resolve_pending_identifiers_strict, and the new unit test in src/v3/compiler/src/lower.rs:6005-6020 pins that repaired stubs become ResolvedByName instead of disappearing. I did not see a new user-reachable panic or silent None path in the changed code.

Illegal states unrepresentable. Also improved. The diff takes a fact that used to be collapsed into one “resolved identifier” shape and makes the distinction explicit in the substrate with AtomPayload::ResolvedByStructure vs AtomPayload::ResolvedByName (src/v3/compiler/src/dag.rs:421-428, mirrored in src/v3/std/substrate.dag:40-45). That is the right direction: provenance is now carried by type shape instead of living only in caller knowledge.

Facts flow forward. Good. The new fact is not introduced and then dropped; it survives lower → reflected substrate → lens consumer. src/v3/compiler/src/lower.rs:2109-2114 produces the new provenance, the infer/emit-side walks were updated to tolerate both resolved forms, and src/v3/lenses/structural_resolution.dag:91-131 is a real downstream consumer rather than dead metadata. This round is making the fallback visible instead of reconstructing it later.

Coproduct dissolution. This is my one blocking concern. I buy ResolvedByName as a useful scaffolded substrate distinction: it exposes previously hidden name-fallback debt and gives it a consumer. But the receipts disagree about what it is. src/v3/compiler/src/dag.rs:405 still says the AtomPayload verdict is terminal, while src/v3/lenses/structural_resolution.dag:96-101 explicitly says the user-range boundary is not yet reflected and must be caller-filtered, and ROADMAP.md:680 upgrades the same scheduled-deletion row to “Live substrate consumer” even though the actual user-range enforcement is still caller policy, not substrate fact. That is an inconsistent scaffold story on a substrate-level distinction, so I would treat it as BLOCKING.

Single-authority metadata. Mostly satisfied. There is now one substrate-carried place to read this provenance, and the new lens is a pure derived reader rather than a parallel store. The only place I still see authority split is the classification/enforcement story above: code comment, lens comment, and roadmap row currently describe ResolvedByName three different ways.

API-level enforcement. This is the same concern from a different angle. The real invariant is not just “we can list ResolvedByName atoms”; it is “user-range name fallback is mechanically auditable.” Right now name_keyed_references is intentionally a raw reader and leaves user/bootstrap filtering to callers (src/v3/lenses/structural_resolution.dag:96-101). That is fine as an intermediate consumer, but it is not yet the full structural enforcement path that ROADMAP.md:680 claims for the user-range row. Until that boundary is reflected, this remains partly convention-level.

Design question.

Is ResolvedByName meant to be a permanent substrate fact, or a tracked scaffold whose job is to expose fallback debt until M2 module scoping dissolves it? That matters because the current code reads like “scaffold,” the local AtomPayload ledger still reads like “terminal,” and the roadmap row now reads like “already enforced.” If that is not unified now, later consumers will learn an ambiguous contract.

Path to convergence.

Before merge, make the status of ResolvedByName single-authority. The small fix is to keep it explicitly scaffolded: update the local AtomPayload receipt in dag.rs to say so, and make the ROADMAP.md row honest that the new lens is a raw substrate consumer while true user-range enforcement still waits on the reflected boundary. The alternative is to declare the variant terminal and remove the scheduled-deletion framing, but that does not look like the intent of this patch.

As tracked follow-up debt, I would keep the stronger user-only structural consumer for the reflected boundary, and add one end-to-end test where a real compiled user module produces a ResolvedByName site the lens reports, instead of relying only on injected atoms plus the lowering unit test.

LOOP HEALTH: converging — this round turns hidden name-fallback repair into first-class substrate data and immediately gives it a real consumer, but it should bank that progress with one honest scaffold/enforcement story before merge.

Verdict.

REQUEST_CHANGES

The main code movement looks right, and I do not see broader issues in the infer/emit/test plumbing. I would merge this after ResolvedByName is made single-authority across dag.rs, the lens comment, and ROADMAP.md; otherwise the next consumer inherits an unclear substrate contract.

[briansrls]

⚠️ ChatGPT reviewer: session expired

The bot's ChatGPT session has expired. Reviews are paused until re-login.

To fix:

1. SSH into the reviewer host
2. cd $CTRL_DIR/scripts/chatgpt-reviewer && docker compose run --rm -p 5900:5900 -p 6080:6080 chatgpt-reviewer --login
3. Open http://192.168.1.176:6080/vnc.html (password: reviewer)
4. Log into ChatGPT, then Ctrl+C

The bot will resume automatically on its next cycle.

[briansrls]

codex · gpt-5.4 · 6d2a4ea0

✅ Review (blocking: 0, non-blocking: 1+/0-)

Non-blocking — Strengths

• src/v3/compiler/src/lower.rs The provenance split stays structural and fail-closed: direct wiring sites emit ResolvedByStructure, sweep repairs emit ResolvedByName, and unresolved survivors still diagnose.

✅ I did not find a new blocking concern in the touched lines after the DB-17 provenance split.

[briansrls]

codex · gpt-5.4 · 6d2a4ea0

✅ Review (blocking: 0, non-blocking: 1+/0-)

Non-blocking — Strengths

• src/v3/compiler/src/lower.rs The provenance split stays structural and fail-closed: direct wiring sites emit ResolvedByStructure, sweep repairs emit ResolvedByName, and unresolved survivors still diagnose.

✅ I did not find a new blocking concern in the touched lines after the DB-17 provenance split.

[briansrls]

Meta-review in progress... (view conversation)

Loop-health check: is this review cycle making forward progress, or shifting debt? Posts in ~5-15 minutes.

[briansrls]

Meta-Review (Loop Health)

_{Generated by gpt-5-4-pro}

According to a document from 2026-04-19, this loop is making real progress, but the review loop on this PR has already hit diminishing returns. Merge the PR and carry the explicitly tracked debt.

Loop summary.

5 recorded review events over about 70 minutes, from 00:30:36Z to 01:40:39Z on 2026-04-19. Of those, 2 are substantive codex reviews and 3 are browser-side events. But only 1 browser event contains any substance, and that one is generic modeling-discipline boilerplate rather than a PR-specific review; the other browser event is just “review in progress,” and the last is a session-expiry failure. Commit count is not recoverable from the supplied artifacts, so I will not invent one.

Forward progress evidence.

This PR is not substrate growth without a consumer. The recorded review says the DB-17 provenance split made ResolvedByName a first-class substrate fact, exposed it through lens_structural_resolution, and covered it with consumer/test updates. That is principle 1 satisfied: correctness is now grounded in an actual consumer path, not just a tidier enum.

The debt is also accounted for instead of hidden. ROADMAP says deferrals must live in one place, and the scheduled-deletions table gives ResolvedByName a dissolution trigger plus enforcement path. It also separates user-range ResolvedByName from compiler-internal declaration_by_name call sites, which prevents this PR from pretending it solved the entire name-keyed class when it only solved one slice. That is principle 2 done correctly.

More importantly, this repo has already shown the right meta-behavior once before: the PR #445 meta-review turned a recurring authority leak into the invariant “Semantic authority after lowering,” explicitly because repeated local fixes were not enough. This PR is in that same direction: it promotes “resolved structurally vs resolved by name fallback” into a substrate fact that future lenses and ratchets can enforce. That is principle 3, not whack-a-mole.

Debt accumulation evidence.

DB-17 does not close the whole class. ROADMAP explicitly says compiler-internal declaration_by_name sites remain a separate temporary debt class, mostly dissolving at self-hosting, and that DB-17 does not cover them. So debt remains; it is just honest debt.

There is also still scaffold load around ArrowBody::Unparsed. The scheduled-deletions table splits it into three separate dissolution stories, including the DB-14 accessor interim that waits on the E-9 bootstrap rewrite. Again, this is tracked, but it is still debt sitting beside the current win.

The loop quality itself is also weak on reviewer diversity. The browser reviewer added almost no PR-specific signal and then died. The only real loop signal here is the two codex passes, both of which converge on “no blocker; provenance split is structural.” That means another round on this PR is more likely to produce ceremony than value.

Cheating signal.

Low cheating from the implementer. The compromises are documented, named, and scheduled. ROADMAP’s rule is that every scaffold gets a row with trigger and enforcement path, and this PR follows that pattern. That is “cheating with accounting,” which is acceptable.

The most recent fixes are structural, not “good enough for now.” The review record describes the change as a provenance split carried through lowering, inference, emitter, reflection, and tests, not a cheap local patch.

The bluff is elsewhere: pretending another review lap on this PR will materially improve loop health. I do not buy that.

Path to convergence.

Merge this PR now.

The acceptable debt to carry is small and already named:

1. user-range ResolvedByName remains a scaffold until M2 module scoping / cross-module structural resolution,
2. compiler-internal declaration_by_name sites remain temporary self-hosting debt,
3. DB-14 accessor interim ArrowBody::Unparsed remains until the E-9 bootstrap rewrite.

The follow-up artifact should be the existing ROADMAP machinery, not more review churn on this PR. If one thing is missing, it is only this: add or tighten the explicit follow-up row for turning the new ResolvedByName reporting into a policy-enforcing user-range ratchet. Do not keep this PR open for that broader boundary work. ROADMAP already says that is how deferrals should be handled.

Meta-verdict

⚖️ SHIP_WITH_DEBT

This loop is not shifting debt blindly. It banked a real structural gain: one recurring leak class is now an explicit fact with a consumer and a scheduled dissolution path. But the review loop around it has already converged. Merge it, and spend the next cycle on the named follow-ups, not on another “still looks structural” lap.

---

View conversation

[briansrls]

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

[briansrls]

ChatGPT Review

_{Generated by gpt-5-4-pro}

Principle audit.

Fail-closed. Looks good. The strict sweep still routes unresolved survivors through diagnostics, and the new lowering regression test pins that repaired stubs become a resolved form rather than silently disappearing. This remains a boundary gate more than a type-level proof, but the diff does not weaken it.

Illegal states unrepresentable. Good improvement. Splitting the old resolved-identifier shape into ResolvedByStructure and ResolvedByName makes provenance explicit in the substrate instead of hiding it behind caller knowledge. This is structural enforcement, not convention.

Facts flow forward. Satisfied. The new provenance fact is produced in lowering, survives through infer/emit by explicit two-variant handling, and now has a real downstream reader in src/v3/lenses/structural_resolution.dag. This round is carrying the fact, not reconstructing it later.

Coproduct dissolution. The code shape is better, and the new lens means ResolvedByName is no longer dead metadata. My only remaining comment is that the receipt is still slightly split: src/v3/compiler/src/dag.rs:395-428 still describes AtomPayload as terminal, while ROADMAP.md:685-688 keeps user-range ResolvedByName in scheduled deletions and src/v3/lenses/structural_resolution.dag:91-101 explicitly says user-only enforcement is still caller policy. I read that as a tracked receipt-sync issue, not a new blocker, but the local classification still lags the roadmap.

Single-authority metadata. Mostly satisfied. There is now one substrate-carried provenance fact, and infer/emit/lens are all readers of that same fact rather than parallel encodings. The only authority split I still see is the classification story above: the variant’s status is not yet described in one place the same way.

API-level enforcement. Mostly satisfied. The distinction between structural resolution and name-fallback resolution is enforced by the type shape itself. The remaining non-structural piece is the user/bootstrap boundary: name_keyed_references is intentionally a raw reader and callers still supply the “user only” policy themselves, so full enforcement there is still convention-level until that boundary is reflected.

Design question.

Is ResolvedByName meant to be a permanent substrate provenance fact, or a tracked scaffold that exists only until M2 module scoping dissolves name fallback? The code now clearly preserves the fact, but future consumers will behave differently depending on that answer; the roadmap reads “temporary, but now observable,” while the local dag.rs receipt still reads “terminal.”

Path to convergence.

Before merge, I do not see a structural code change that must land. The one cleanup I would strongly want soon is to sync the local AtomPayload receipt in src/v3/compiler/src/dag.rs with the roadmap’s scheduled-deletion story, so ResolvedByName has one classification instead of “terminal here, scaffold there.”

Tracked follow-up debt can stay explicit: reflect the user/bootstrap boundary as a first-class substrate fact so name_keyed_references can become a true user-range enforcement lens instead of a raw reader plus caller filtering. I would also add one end-to-end test where compiled user input produces a real ResolvedByName site that the lens sees, since the current tests pin producer and consumer separately.

LOOP HEALTH: converging — this round turns previously hidden name-fallback repair into first-class substrate data and immediately gives it a lens plus regression pins; the remaining gap is receipt/enforcement polish, not another hidden bridge.

Verdict.

APPROVE_WITH_COMMENTS

The provenance split itself looks clean, and I do not see a new blocking concern in the mechanics. My only comment is to tighten the classification story around ResolvedByName so the local receipt, the lens comment, and the roadmap all tell the same story.

---

View conversation

[briansrls]

codex · gpt-5.4 · d57d7b82

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

BLOCKING (1)

Root Cause

• src/v3/compiler/src/dag.rs The substrate split preserves resolution provenance, but this helper restores the pre-split convenience API on the substrate type -> remove the helper and keep any provenance-erasing collapse local to the specific implementation site that can justify dropping it.

Non-blocking — Strengths

• src/v3/lenses/structural_resolution.dag The new NameKeyedReference lens keeps the scheduled-deletion signal as a pure substrate read instead of another side table.

⚠️ The split is otherwise well-propagated, but the new resolved_id() escape hatch makes the provenance distinction optional instead of structural.

[briansrls]

BLOCKING: AtomPayload::resolved_id() collapses ResolvedByStructure and ResolvedByName back into the old merged view, reintroducing a bridge that weakens API-level enforcement of the new provenance fact (principles 3 and 6; INVARIANTS no bridges).

[briansrls]

codex · gpt-5.4 · d57d7b82

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

BLOCKING (1)

Root Cause

• src/v3/compiler/src/dag.rs The substrate split preserves resolution provenance, but this helper restores the pre-split convenience API on the substrate type -> remove the helper and keep any provenance-erasing collapse local to the specific implementation site that can justify dropping it.

Non-blocking — Strengths

• src/v3/lenses/structural_resolution.dag The new NameKeyedReference lens keeps the scheduled-deletion signal as a pure substrate read instead of another side table.

⚠️ The split is otherwise well-propagated, but the new resolved_id() escape hatch makes the provenance distinction optional instead of structural.