feat(evaluator): PR-A.2 EvalFrame / EvalStateStack carriers

[briansrls]

Summary

Lands PR-A.2 of the R2 Evaluator runtime-value model: declares EvalFrame and EvalStateStack as evaluator-internal state carriers in src/v3/std/runtime.dag, alongside the Value / NamedField observable surface PR-A.1 (#1243) put in place.

• EvalFrame { bindings: Map<PortId, Value> } — a finite partial function from PortId to runtime Value. Duplicate binding for one PortId is unrepresentable at the carrier boundary. Map<K, V> = PartialFunction<K, V> is imported from std.types (dsl/std/types.dag:213); no parallel keyed-collection authority.
• EvalStateStack { frames: List<EvalFrame> } — locked by the merged PR-A.0 brief and PR-A.2 audit.

Per docs/design-pb-runtime-interpreter.md §3.3 and docs/briefs/r2-pr-a-runtime-value-model.md §Substrate Targets.

Out of scope

• No new observable Value variants. No ClosureValue (forbidden P1 substrate change).
• No EvalThunk / EvalStrategy / EvalMemoKey — PR-A.3.
• No body evaluator — PR-B.
• No new Map / Stack / unique-binding authority. The Stack<EvalFrame> orientation is a separate design-amendment discussion.

Files

• src/v3/std/runtime.dag — EvalFrame + EvalStateStack declarations.
• src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs — focused integration test pr_a_2_eval_frame_and_state_stack_carriers_match_pb_runtime_section_3_3 pinning:
  • EvalFrame.bindings is Map<PortId, Value> (template Map/PartialFunction, two args).
  • EvalStateStack.frames is List<EvalFrame>.
  • Value coproduct does not contain ClosureValue, EvalFrame, or EvalStateStack.
• src/v3/compiler/src/bootstrap_generated*.rs — regenerated via cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap.

Test plan

• cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap — fresh bootstrap matches committed snapshots.
• cargo test -p v3-compiler --test integration pr_a_2_eval_frame — new test passes.

Dependencies

• Builds on PR-A.1 (feat(evaluator): PR-A.1 runtime Value coproduct #1243) for Value / NamedField.
• Audit/tracking: docs(evaluator): PR-A.2 dependency audit (blocked on PR-A.1 + Map carrier) #1222 (merged dependency audit).

🤖 Generated with Claude Code

[briansrls]

This is the right conceptual PR-A.2 carrier shape: EvalFrame { bindings: Map<PortId, Value> } and EvalStateStack { frames: List<EvalFrame> }, with explicit comments rejecting List<EvalBinding>, UniqueBindings, ClosureValue, and PR-A.3 carriers.

Blocking before review/ready:

1. Map is referenced but not imported in src/v3/std/runtime.dag. Per the merged audit, use the existing std.types / dsl/std/types.dag:213 authority; do not create a new carrier. Add the correct import and verify it compiles.
2. This PR only edits runtime.dag. PR-A.2 needs the same bootstrap/test treatment PR-A.1 needed: regenerate checked-in bootstrap files so generated_full_bootstrap_dag() includes EvalFrame / EvalStateStack, and add focused tests that pin:
   • EvalFrame.bindings is an instantiation of Map<PortId, Value>
   • EvalStateStack.frames is List<EvalFrame>
   • Value remains unchanged and has no ClosureValue
3. Update the existing runtime-value fixture/manager brief only if needed to record PR-A.2 status; keep wording honest while the PR is open.
4. Replace dashboard placeholder title/body and move out of draft only after focused tests pass locally.

I ran cargo fmt --all --check locally in your worktree and it passed. The focused integration command is still compiling, but the missing import / missing bootstrap-test coverage is already enough to address before review.

— sent from snappy-moth-795 (inbox #1131); reply at #1131

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: f7c63f25 · Trigger: schedule
• Thinking: 244s wall

---

BLOCKING (1)

Root Cause

• docs/briefs/r2-pr-a-runtime-value-model.md The PR-A.2 substrate target copied the stack concept into a List-shaped field despite dsl/std/stack.dag already being the LIFO authority → import/use Stack or record an explicit locked exception for why this state stack is not the std stack primitive.

Non-blocking — Strengths

• src/v3/std/runtime.dag EvalFrame uses Map<PortId, Value>, matching the locked partial-function shape and making duplicate bindings for one PortId unrepresentable.

⚠️ One substrate-shape issue should be fixed before this lands because downstream evaluator consumers will otherwise couple to the wrong carrier.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: e00bad96 · Trigger: schedule
• Thinking: 251s wall

---

✅ No blocking concerns in the current diff.

[briansrls]

Blocking before this can be marked ready:

1. EvalStateStack.frames must stay on the locked/audited PR-A.2 shape: List<EvalFrame>, not Stack<EvalFrame>. Stack<T> may be a reasonable future carrier, but this PR cannot silently amend the PR-A.0/PR-A.2 authority. The merged brief and audit both name EvalStateStack { frames: List<EvalFrame> } (docs/briefs/r2-pr-a-runtime-value-model.md and docs/briefs/r2-pr-a-2-eval-frame-dependency-audit.md). Please restore List<EvalFrame>, remove the std.stack { Stack } import, and update the focused test expectation accordingly. If you want Stack<EvalFrame>, route that as a design-amendment PR first because it affects PR-A.3 EvalStateKey / memoization and body-evaluator push/pop semantics.

2. The PR is still in draft with placeholder title/body. Once the carrier shape is restored and local verification passes, update the title/body to the actual PR-A.2 implementation scope and mark ready.

The Map<PortId, Value> direction is correct. Keep std.types { Map }; that resolves the unique-binding requirement without adding a parallel carrier.

— sent from snappy-moth-795 (inbox #1131); reply at #1131

[briansrls]

Current head e1a15ba0 still has the same ready blocker: EvalStateStack.frames is Stack<EvalFrame> and the test asserts Stack, while the merged PR-A.0 brief + PR-A.2 audit still lock List<EvalFrame>.

Please do not mark this ready yet. The acceptable fix for this implementation PR is:

• runtime.dag: remove import std.stack { Stack }
• runtime.dag: restore frames: List<EvalFrame>
• focused test: expect List<EvalFrame>
• PR metadata: replace placeholder title/body with PR-A.2 implementation scope

If you want to pursue the automated-review suggestion that Stack<EvalFrame> is the better substrate carrier, that needs a separate design-amendment PR first. It should update docs/briefs/r2-pr-a-runtime-value-model.md and docs/briefs/r2-pr-a-2-eval-frame-dependency-audit.md, and it should call out the PR-A.3 EvalStateKey / memoization and PR-B body-evaluator push/pop consequences. That is a valid discussion, but not a silent change inside this implementation PR.

— sent from snappy-moth-795 (inbox #1131); reply at #1131

[briansrls]

Reverted to List<EvalFrame> per the locked PR-A.0/PR-A.2 authority. Removed std.stack { Stack } import; test expectation back to List. The earlier Stack<EvalFrame> swap was a response to a separate blocking review (codex on f7c63f2) citing M9; agreed on routing — Stack-vs-List orientation is a design amendment that affects PR-A.3 memoization keying and belongs in its own brief-amendment PR, not here.

Title/body update + ready flip will follow once regen_bootstrap finishes and the focused integration test passes locally.

— sent from merry-heron-351

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 1adae656 · Trigger: schedule
• Thinking: 154s wall

---

Non-blocking — Strengths

• src/v3/std/runtime.dag EvalFrame uses Map<PortId, Value>, preserving single binding authority and making duplicate bindings unrepresentable at the carrier boundary.
• src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs The added ratchet keeps EvalFrame/EvalStateStack out of observable runtime Value variants, matching the evaluator-state boundary.

✅ No blocking concerns in the current diff.

[briansrls]

Good fix on the List<EvalFrame> revert. One remaining ready blocker remains visible from your worktree/PR state:

• PR feat(evaluator): PR-A.2 EvalFrame / EvalStateStack carriers #1255 still only lists runtime.dag + m2_substrate_inhabitance_test.rs, but your worktree has uncommitted generated files:
  • src/v3/compiler/src/bootstrap_generated.rs
  • src/v3/compiler/src/bootstrap_generated_without_parse_surface.rs

Please commit and push those generated bootstrap updates, then update the PR title/body from the dashboard placeholders and mark ready. I also started the focused test locally in your worktree, but it sat on Cargo’s artifact-directory lock for ~90s, so I killed that local attempt rather than leave a process running. Please include your local focused test result when you push.

— sent from snappy-moth-795 (inbox #1131); reply at #1131

[briansrls]

All four blockers from the parent review on e1a15ba0 are addressed at HEAD 7696752f0:

• ✅ runtime.dag: import std.stack { Stack } removed.
• ✅ runtime.dag: EvalStateStack.frames restored to List<EvalFrame>. Comment now explicitly cites the PR-A.0 brief + audit as the authority and notes that an orientation amendment (Stack vs List) belongs in a separate brief-amendment PR with PR-A.3 / body-evaluator analysis.
• ✅ Focused test asserts List<EvalFrame>, runs locally, passes.
• ✅ Title/body replaced with the PR-A.2 implementation scope; PR is ready.

Bootstrap regenerated (regen_bootstrap) so generated_full_bootstrap_dag() includes the new carriers. Test result: pr_a_2_eval_frame_and_state_stack_carriers_match_pb_runtime_section_3_3 ... ok.

— sent from merry-heron-351

[briansrls]

All addressed at HEAD 7696752f0:

• ✅ Bootstrap files committed: 7696752f0 WIP: merry-heron-351 includes src/v3/compiler/src/bootstrap_generated.rs and src/v3/compiler/src/bootstrap_generated_without_parse_surface.rs (verified via git log --name-only 7696752f0 -1). Branch up to date with origin/session/merry-heron-351-pr-a-2-impl; worktree clean.
• ✅ Title updated: "feat(evaluator): PR-A.2 EvalFrame / EvalStateStack carriers". Body replaced with PR-A.2 implementation scope (verified via gh pr view 1255 --json title,isDraft → isDraft: false).
• ✅ PR is out of draft.

Local focused test result (re-ran after the bootstrap update completed):

running 1 test
test m2_substrate_inhabitance_test::pr_a_2_eval_frame_and_state_stack_carriers_match_pb_runtime_section_3_3 ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 672 filtered out; finished in 0.21s

Codex auto-review on 1adae656 already returned ✅ "No blocking concerns" (Map<PortId, Value> and the EvalFrame/EvalStateStack-not-Value-variant ratchet both called out as strengths).

— sent from merry-heron-351

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: 7696752f · Trigger: schedule
• Comparison: origin/main @ 0e5c2b78 ... review/pr-1255-7696752f @ 7696752f
• Thinking: 104s wall

---

Findings

• src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2189 — TESTING.md (“one claim per test”): the new pr_a_2_eval_frame_and_state_stack_carriers_match_pb_runtime_section_3_3 test folds several independent checks (frame module + bindings instantiation + stack frames + two negative assertions on Value variants) into a single #[test]. Acceptable as integration coverage, but it is a mild departure from the doc’s one-claim discipline for new tests; splitting would make failures easier to localize.

Verdict

APPROVE_WITH_COMMENTS — Substrate change matches the locked shapes (Map<PortId, Value>, List<EvalFrame>), stays out of the Value sum, documents GREEN/terminal with brief/design pointers, and the integration test encodes the §3.3-style structural receipts (authority module, Map/PartialFunction, keys/values, no smuggling into Value). No invariant/modeling-faithfulness issue in the diff beyond the optional testing-style nit above.

(Nothing else in the diff calls for changes: bootstrap_generated*.rs churn is mechanical; runtime.dag additions align with P2 single-authority + illegal duplicate-bindings framing.)

[briansrls]

Thanks for the approve. Leaving the pr_a_2_eval_frame_and_state_stack_carriers_match_pb_runtime_section_3_3 test as a single #[test] intentionally: the assertions encode one composite claim — "PR-A.2 lands EvalFrame/EvalStateStack in the runtime authority module with the locked carrier shapes and without smuggling them into Value" — and they share the same generated_full_bootstrap_dag() setup. The mutually-reinforcing structure also matches the existing runtime_value_carrier_matches_pb_runtime_shape_and_marker_boundary test from PR-A.1, which folds the analogous Value-coproduct receipts into a single #[test]. Splitting now would diverge from the immediate sibling without an offsetting clarity gain. Happy to revisit if PR-A.3 carrier work makes any individual receipt drift independently.

— sent from merry-heron-351

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: a949be2b · Trigger: schedule
• Comparison: origin/main @ 0e5c2b78 ... review/pr-1255-a949be2b @ a949be2b
• Thinking: 38s wall

---

Verdict: APPROVE — Clean, narrowly scoped substrate addition.

Two new types in src/v3/std/runtime.dag (EvalFrame, EvalStateStack) with locked-shape comments citing the briefs, plus a focused inhabitance test that pins both the carrier shapes and the fail-closed property that Value does not gain ClosureValue/EvalFrame/EvalStateStack variants. The rest of the diff is regenerated bootstrap_generated*.rs.

Notes worth flagging:

• bindings: Map<PortId, Value> makes the "no duplicate binding for one PortId" property unrepresentable at the carrier — exactly the modeling-discipline move (runtime.dag:65).
• The test at m2_substrate_inhabitance_test.rs:2237-2255 directly enforces fail-closed Value shape, which is the right place for the invariant.
• Both new types carry the three properties for tracked-debt comments (documented, bounded to PR-A.2 scope, named successor PR-A.3 trigger for revisit). Good.

No blocking concerns.

[briansrls]

CI failure is mechanical and PR-local: v3 fails only in parse_stage4_prep::handwritten_parse_snapshot_matches_manifest because the handwritten parse snapshot manifest still has the old src/v3/std/runtime.dag row.

Current CI diff shows expected/generated row:

src/v3/std/runtime.dag	9	10990	e4dc8f7383ff7660

but the checked-in manifest still has:

src/v3/std/runtime.dag	6	7993	0e703b02069526f6

Please refresh and commit the parse snapshot manifest, likely via the test-named command from the manifest header:

cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored

Then rerun cargo test -p v3-compiler --test integration parse_stage4_prep::handwritten_parse_snapshot_matches_manifest or the broader v3 check if convenient, and push. No substrate-shape issue remains from my side.

— sent from snappy-moth-795 (inbox #1131); reply at #1131

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: a949be2b · Trigger: schedule
• Thinking: 199s wall

---

Non-blocking — Strengths

• src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs The carrier ratchet accepts both Map and its lowered PartialFunction, checking the algebraic authority rather than alias spelling.

✅ No blocking concerns in the current PR diff.

[briansrls]

Already pushed in ce173f1 — ran cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored, committed the single-line diff (runtime.dag row updated to 9 10990 e4dc8f7383ff7660), and pushed. Branch HEAD is now ce173f15f. No substrate-shape change in this commit.

— sent from merry-heron-351

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: ce173f15 · Trigger: schedule
• Comparison: origin/main @ 196f403f ... review/pr-1255-ce173f15 @ ce173f15
• Thinking: 43s wall

---

Verdict: APPROVE — Diff is narrowly scoped and disciplined. Two new carriers in src/v3/std/runtime.dag:60,75 use Map<PortId, Value> to make duplicate-binding unrepresentable, and the test at m2_substrate_inhabitance_test.rs:2190+ actively guards against ClosureValue/EvalFrame/EvalStateStack leaking into the Value Disj — this is exactly the fail-closed / illegal-states-unrepresentable posture INVARIANTS asks for. Bootstrap regen and manifest hash bump are consistent with the source change. No findings.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: c325a3c3 · Trigger: schedule
• Comparison: origin/main @ 95c36f8f ... review/pr-1255-c325a3c3 @ c325a3c3
• Thinking: 1119s wall

---

Verdict: APPROVE

The diff is narrowly scoped: runtime.dag adds EvalFrame and EvalStateStack as substrate carriers with explicit terminal classification, grounded in existing Map/PartialFunction and List authorities, plus a focused shape regression test. I found no concrete violations of the pinned invariants, modeling discipline, coding, or testing docs.

Verification note: I attempted the focused Cargo test, but the cold build was still compiling after several minutes, so I stopped it before completion.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: be7a6694 · Trigger: schedule
• Comparison: origin/main @ 95c36f8f ... review/pr-1255-be7a6694 @ be7a6694
• Thinking: 564s wall

---

Verdict: APPROVE

No findings. The diff adds the PR-A.2 evaluator-state carriers in the runtime authority module, keeps them out of observable Value, annotates the new carrier shapes, and adds a focused regression test for the substrate shape.

Verified with:

cargo test -p v3-compiler --test integration pr_a_2_eval_frame_and_state_stack_carriers_match_pb_runtime_section_3_3

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-5-thinking
• Commit: be7a6694 · Trigger: manual
• Comparison: main @ 95c36f8f ... session/merry-heron-351-pr-a-2-impl @ be7a6694
• Conversation: View conversation

---

1. Story of the diff

This PR adds evaluator-state carriers to the PB runtime substrate without making them observable runtime Values. runtime.dag imports Map and PortId, then declares EvalFrame as bindings: Map<PortId, Value> and EvalStateStack as frames: List<EvalFrame> (src/v3/std/runtime.dag:15, src/v3/std/runtime.dag:17, src/v3/std/runtime.dag:60-61, src/v3/std/runtime.dag:73-74). The comments are load-bearing: they explicitly reject a duplicate-admitting List<EvalBinding> and reject ClosureValue / frame-stack variants as observable Value inhabitants (src/v3/std/runtime.dag:51-59, src/v3/std/runtime.dag:64-72). The generated bootstrap files and parse manifest are refreshed to reflect the new declarations, and the integration test ratchets the carrier shapes plus the “not a Value variant” boundary (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2191-2265).

2. Invariant categories

1. LAYER MODEL (substrate vs implementation).

Compliant — this is substrate/modeling work, and the new state carriers are placed in the runtime authority module rather than in ad hoc evaluator implementation state; the test enforces that both EvalFrame and EvalStateStack resolve from src/v3/std/runtime.dag (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2197-2200, src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2238-2240).

1. INVARIANTS.md + modeling-discipline.md.

Compliant — illegal states are made unrepresentable at the carrier boundary: EvalFrame.bindings is a Map<PortId, Value>, so duplicate bindings for one port are not modeled as a list of entries (src/v3/std/runtime.dag:55-61). Facts also flow forward through generated bootstrap and are ratcheted by checking key/value type arguments as PortId and runtime Value (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2216-2229).

1. CODING.md.

Compliant — the change uses data carriers and structural declarations rather than methods/builders/hidden evaluator objects: type EvalFrame { bindings: ... } and type EvalStateStack { frames: ... } are plain model data (src/v3/std/runtime.dag:60-61, src/v3/std/runtime.dag:73-74). The Rust addition is a focused integration test over generated declarations, not new production-side object behavior (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2191-2265).

1. TESTING.md.

Compliant — the PR adds a behavior-driven ratchet for the new substrate contract: location in the runtime authority module, Map/PartialFunction<PortId, Value> shape for frame bindings, List<EvalFrame> stack shape, and explicit absence from observable Value variants (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2197-2263). This is the right level because the claim is about generated substrate inhabitance rather than an evaluator algorithm.

1. LOCKED DESIGN DECISIONS.

Compliant — the diff explicitly names the locked carrier shape and forbids silent replacement with List<EvalBinding> (src/v3/std/runtime.dag:55-59). It also bounds the next design lane by saying PR-A.3 owns EvalThunk / EvalStrategy / EvalMemoKey and may revisit orientation only when memoization keying lands (src/v3/std/runtime.dag:68-72).

1. TRACKED vs UNTRACKED DEBT.

Compliant — I do not see new untracked scaffolding. The only forward-looking note is bounded to PR-A.3 and names the trigger area, memoization keying, rather than leaving an open-ended TODO (src/v3/std/runtime.dag:68-72). The carrier declarations themselves are marked terminal at PR-A.2 scope (src/v3/std/runtime.dag:51, src/v3/std/runtime.dag:64).

3. Verdict

APPROVE. The PR lands the evaluator-state carriers as substrate data with the right single-authority placement, avoids making evaluator state observable as Value, and adds a focused regression test for the exact carrier contract. I found no blocking or non-blocking findings against the diff.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: be7a6694 · Trigger: schedule
• Thinking: 190s wall

---

✅ No blocking concerns in the current PR diff.