feat(evaluator): PR-E E2 EvalFrame lookup and Bind environment

[briansrls]

Summary

• Adds a focused evaluator module for PR-E E2 frame semantics over the existing EvalFrame / EvalStateStack carrier model.
• Implements innermost-frame-first lookup, top-frame-only binding updates, duplicate binding rejection, unbound-port diagnostics, and empty-stack diagnostics.
• Keeps the value payload generic so this slice does not introduce a new observable Value carrier or touch src/v3/std/runtime.dag.

Out of scope

• No runtime carrier changes.
• No List fallback.
• No cross-frame writes.
• No body evaluator, Transform, Branch, Loop, memoization, or new Value variants.

Validation

• cargo fmt --all --check
• RUSTC_WRAPPER= cargo test -p v3-compiler --lib evaluator::tests

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: d562f2a4 · Trigger: schedule
• Comparison: origin/main @ c1054988 ... review/pr-1374-d562f2a4 @ d562f2a4
• Thinking: 39s wall

---

APPROVE — small, narrowly scoped new module realizing the substrate EvalFrame/EvalStateStack carriers as a generic Rust helper. Fail-closed errors (DuplicateBinding, UnboundPort, EmptyStateStack), HashMap enforces the partial-function discipline, and the dissolution target is documented in the module header. Behavior-driven unit tests cover lookup shadowing, top-frame writes, duplicate rejection, unbound, and empty stack. No invariant or modeling-discipline violations observed in the diff.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: d562f2a4 · Trigger: schedule
• Thinking: 177s wall

---

Non-blocking — Strengths

• src/v3/compiler/src/evaluator.rs The implementation-local frame helper keeps bindings private, rejects duplicate local binds, and returns typed errors for unbound and empty-stack cases.

✅ Classified as code; no blocking concerns found.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: add20329 · Trigger: schedule
• Comparison: origin/main @ c1054988 ... review/pr-1374-add20329 @ add20329
• Thinking: 59s wall

---

Verdict: APPROVE

This looks clean. The added evaluator module is implementation-side, grounded in the existing EvalFrame / EvalStateStack substrate carriers, uses typed failures for duplicate, unbound, and empty-stack cases, and the tests cover the main lookup and binding behavior. I didn’t see any concrete violations of the pinned invariants, coding guidance, or testing discipline in the diff.

Tests not run, per review instructions.

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 8b0bae6a · Trigger: schedule
• Comparison: origin/main @ c1054988 ... review/pr-1374-8b0bae6a @ 8b0bae6a
• Thinking: 57s wall

---

Verdict: APPROVE — diff is small, narrowly scoped, and disciplined.

The new evaluator module is a clean Rust realization of the substrate EvalFrame/EvalStateStack shapes:

• Generic over V (lib.rs:65, :103) — avoids pre-committing to a Value carrier ahead of the body-evaluator slices, which respects layer separation.
• Fail-closed on duplicate bindings (lib.rs:84), unbound lookup (lib.rs:131), and empty stack (lib.rs:138) — errors are an enum, not strings.
• HashMap choice is justified inline (lib.rs:60-62) as the realization of the substrate Map<PortId, Value> finite partial-function discipline, with an explicit "do not replace with List<EvalBinding>" guard.
• Dissolution target is documented and named (lib.rs:46-48): shrink to generated/substrate-backed calls once .dag evaluator body owns frame mutation. Tracked-bridge criteria met.
• Tests are unit-level and behavior-named (innermost-first lookup, bind-top scope isolation, duplicate rejection, unbound, empty-stack) — aligned with TESTING.md.

Other hunks are housekeeping: test_runner.rs is a clippy format!→to_string cleanup; sg0_census_test.rs adds a documented ratchet entry for bin_shim_carrier_test.rs.

No invariant or modeling-discipline violations grounded in this diff.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: 394ae3f1 · Trigger: schedule
• Comparison: origin/main @ 1cbc02d2 ... review/pr-1374-394ae3f1 @ 394ae3f1
• Thinking: 89s wall

---

Findings

• src/v3/compiler/src/lib.rs:57 — New EvalFrameError has three variants but no Practice 4 coproduct checkpoint (🟢 / 🟡 / 🔴 plus ledger or named trigger as required). Violates docs/modeling-discipline.md §4 (“Coproduct dissolution”). Layer-wise this is host implementation, not a Dag-carried substrate sum, so treat as NON-BLOCKING: add a one-line classification (this enum is almost certainly 🟢 terminal: distinct fail-closed outcomes).

Verdict

APPROVE_WITH_COMMENTS — Eval frame/stack API is fail-closed (Result / typed errors), duplicates rejected, lookup walks inner→outer, module docs name a dissolution target, and unit tests match TESTING.md (minimal Dag, behavior-named tests). Census line removal matches absent bin_shim_carrier_test.rs. Only gap worth fixing before or right after merge is the enum classification comment above.

Exploratory (optional)

• pop_frame can drain the stack to empty; there is no test for “cannot pop root” vs “allowed.” Fine if later evaluator code always keeps a root frame by policy.

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 8af58efd · Trigger: schedule
• Comparison: origin/main @ 1cbc02d2 ... review/pr-1374-8af58efd @ 8af58efd
• Thinking: 28s wall

---

File removal is consistent with the census — fine.

Verdict: APPROVE — diff is small and narrowly scoped. The new evaluator module is implementation-layer (generic V, file-scoped Rust helpers mirroring the substrate EvalFrame/EvalStateStack shapes), with fail-closed errors for empty stack / duplicate bind / unbound port and behavior-driven unit tests for each. Module doc names a concrete dissolution trigger (when .dag evaluator body owns frame mutation directly), satisfying the tracked-bridge bar. Census ratchet update matches the deleted test file.

Exploratory observation (non-blocking): frames_outer_to_inner exposes &[EvalFrame<V>] but EvalFrame::lookup_local is the only way to peek inside, which is fine; if later slices need iteration order guarantees, the name already documents it.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 394ae3f1 · Trigger: schedule
• Thinking: 156s wall

---

✅ Classified as code; no blocking concerns found.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: caf4ffc1 · Trigger: schedule
• Comparison: origin/main @ 8f051650 ... review/pr-1374-caf4ffc1 @ caf4ffc1
• Thinking: 52s wall

---

APPROVE — diff is narrow and consistent with the referenced discipline. It uses the existing EvalFrame / EvalStateStack substrate shape, preserves Map<PortId, Value> semantics with a HashMap, rejects duplicate local binds, and keeps lookup/bind failures typed rather than silent.

I did not run builds or tests per the review instructions.

[briansrls]

Review metadata

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

---

1. Story of the diff

This PR adds a narrow Rust helper surface for E2 evaluator binding scope in src/v3/compiler/src/lib.rs:37-232. The new evaluator module models one frame as a private HashMap<PortId, V> and a stack as Vec<EvalFrame<V>>, with lookup walking from inner to outer scope and binding limited to the current top frame. The important contract is fail-closed frame access: duplicate current-frame bindings, unbound lookups, and empty-stack operations return typed EvalFrameError variants rather than fabricating values. The module docs explicitly bound this as a host helper over existing evaluator-frame substrate carriers, not a new runtime value authority, and name the dissolution target once .dag evaluator body mutation owns the behavior directly at src/v3/compiler/src/lib.rs:40-50.

2. Invariant categories

1. LAYER MODEL — Compliant. This is implementation-side Rust only: the diff adds pub mod evaluator in lib.rs and imports crate::dag::PortId, but does not mutate dag.rs, add substrate variants, or add Dag-resident fields. The docs keep the layer boundary explicit by saying the module is the Rust realization of existing EvalFrame / EvalStateStack carriers at src/v3/compiler/src/lib.rs:40-42, and the generic payload avoids introducing a parallel observable Value carrier at src/v3/compiler/src/lib.rs:43-46.
2. INVARIANTS.md + modeling-discipline.md — Compliant. Fail-closed is handled structurally: duplicate binding returns EvalFrameError::DuplicateBinding at src/v3/compiler/src/lib.rs:93-95, empty-stack pop/bind returns EvalFrameError::EmptyStateStack at src/v3/compiler/src/lib.rs:126-127 and src/v3/compiler/src/lib.rs:139-142, and full-stack lookup returns EvalFrameError::UnboundPort at src/v3/compiler/src/lib.rs:130-135. Single authority / illegal-state pressure is also respected inside a frame: the backing map is private at src/v3/compiler/src/lib.rs:71-72, and construction goes through bind via from_bindings at src/v3/compiler/src/lib.rs:82-89, so duplicate-admitting binding lists are not exposed as the steady-state shape.
3. CODING.md — Compliant. The public interfaces use structured carriers instead of primitive sentinels: from_bindings returns Result<Self, EvalFrameError> at src/v3/compiler/src/lib.rs:82-84, pop_frame returns Result<EvalFrame<V>, EvalFrameError> at src/v3/compiler/src/lib.rs:126-127, and lookup returns Result<&V, EvalFrameError> at src/v3/compiler/src/lib.rs:130-135. The methods are small and operate on private representation invariants, so this does not read as object-style hidden behavior despite being method-shaped.
4. TESTING.md — Compliant. The diff adds focused unit tests for the frame helper behavior rather than relying on full-pipeline compilation: inner-before-outer lookup at src/v3/compiler/src/lib.rs:167-176, top-frame-only binding at src/v3/compiler/src/lib.rs:179-196, duplicate rejection at src/v3/compiler/src/lib.rs:199-208, unbound lookup at src/v3/compiler/src/lib.rs:211-219, and empty stack rejection at src/v3/compiler/src/lib.rs:222-229. The ports helper constructs only enough Dag state to mint PortIds at src/v3/compiler/src/lib.rs:160-164, which keeps the tests unit-scoped.
5. LOCKED DESIGN DECISIONS — N/A. The diff does not alter reflection completeness, Arrow.body external realization, test runner host-process behavior, or any other locked design surface visible in the provided reference set.
6. TRACKED vs UNTRACKED DEBT — Compliant. The host helper is explicitly bounded and tracked: the module says it is a “narrow Rust realization” at src/v3/compiler/src/lib.rs:40, avoids owning a new runtime value carrier at src/v3/compiler/src/lib.rs:43-46, and names the dissolution trigger as the point where the .dag evaluator body implementation owns frame mutation directly at src/v3/compiler/src/lib.rs:48-50. I did not see new TODOs, unbounded scaffolds, or temporary alternate authorities in the diff.

3. Verdict

APPROVE. The change is a bounded implementation helper with typed failure outcomes, private frame storage, unit coverage for the load-bearing behaviors, and an explicit dissolution target. I do not see a blocking substrate, modeling, testing, or locked-design issue in the diff.