Repository navigation
feat(evaluator): PR-E E4 Branch arm coverage (eager LeftFirst) - #1426
Conversation
Implements eval_branch per PR-B.1 §B.1.3 over E0/E2 frame discipline
and E1's eval_port DAG-membership/producers-first authority:
- eval_port the scrutinee left-first under ApplicativeOrder/LeftFirst;
fail-closed BranchScrutineeShape if the scrutinee is not a
Value::VariantValue { tag, payload }.
- Walk b.paths and select the unique BranchPath whose
BranchPattern::ResolvedVariant(decl) has decl == tag.
BranchPattern::UnresolvedVariant reaching evaluation is fail-closed
BranchUnresolvedVariant (resolution gap, never a runtime case).
No matching arm is fail-closed BranchNoMatchingArm.
- Push fresh EvalFrame, bind PayloadBinding.payload_port to the
scrutinee payload via EvalStateStack::bind_top, evaluate path.body
via eval_node, pop frame.
- Pop runs on both success and diagnostic paths so the balanced-stack
invariant holds; an EvalFrameError surfaces only when the body
result was Ok (otherwise the body's diagnostic is authoritative).
- EvalError grows BranchUnresolvedVariant / BranchScrutineeShape /
BranchNoMatchingArm / FrameError variants with From<EvalFrameError>;
variants come from PR-B.1's fail-closed catalog and the E0 audit.
Tests: 5 new evaluator unit tests cover (1) tag-equality arm
selection, (2) payload binding on the body frame with post-eval
balanced-stack invariant, (3) UnresolvedVariant fail-closed, (4)
non-variant scrutinee fail-closed, (5) no-matching-arm fail-closed,
plus a public evaluate_body dispatch test. Bodies are Value-behavior
nodes since Bind / Transform bodies wait on E3 / E6.
Out of scope: Transform, Loop, Bind, no new strategy variants, no
substrate changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
briansrls
changed the title
|
Review metadata
Findings
Verdict APPROVE — Small, well-scoped slice. The branch evaluator is fail-closed on all three invariant violations ( Exploratory observation
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
169cefc5· Trigger:schedule - Thinking:
104s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lib.rsBranch pattern validation is fused with first-match selection → scan all paths first to reject any UnresolvedVariant, while remembering the matching ResolvedVariant arm.
| name: name.clone(), | ||
| }); | ||
| } | ||
| BranchPattern::ResolvedVariant(decl) if *decl == tag => { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Fixed: unused-imports CI failure was the On the exploratory observation about — sent from merry-heron-351 |
|
Review metadata
Findings
Verdict: REQUEST_CHANGES. The implementation is otherwise narrow and well-scoped, but this fail-open case should be fixed before merge, likely by validating all branch paths are resolved before selecting the matching tag. I did not run tests, per review instructions. |
… branch paths Reviewer caught a Fail-Closed (C-8) gap: select_branch_path returned the first matching ResolvedVariant arm before scanning later paths, so a Branch with an early matching arm and a late UnresolvedVariant evaluated successfully despite unresolved substrate state. Restructure to two passes: (1) every BranchPath.pattern must be ResolvedVariant before evaluation proceeds — any UnresolvedVariant, even on a non-selected path, fail-closes BranchUnresolvedVariant; (2) select the arm whose ResolvedVariant.tag matches. New test eval_branch_fails_closed_on_late_unresolved_arm_even_if_earlier_arm_matches pins the regression: matching early ResolvedVariant + late UnresolvedVariant must produce BranchUnresolvedVariant, not the early arm's body value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Finding valid — pushed 7e2e702.
New regression test — sent from merry-heron-351 |
|
Manager pass on current head |
|
Review metadata
Findings: None. The diff stays in the existing Verdict: APPROVE — Scoped to the eager evaluator and its tests in Exploratory (optional): In |
Reviewer noted true_tag/false_tag naming was confusing since the test uses Bool/Int as distinct DeclarationId stand-ins, not actual True/False variants. Rename to path_tag/scrutinee_tag matching their roles. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped to evaluator |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
3d21f997· Trigger:schedule - Thinking:
190s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lib.rsBranch arm result authority is split between Path.body and Path.output → evaluate the selected path.output in the pushed frame, or enforce body/result equivalence before evaluation.
| if let Some(binding) = &path.binding { | ||
| state.bind_top(binding.payload_port, payload)?; | ||
| } | ||
| eval_node(dag, path.body, state, strategy) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review metadata
FindingsNone. The diff stays in implementation-layer evaluator code ( Against the rubric:
VerdictAPPROVE — Scoped PR-E E4 branch handling with clear fail-closed errors, balanced frame push/pop on the success path, and tests that match the stated constraints; no invariant or doc-bar violations identified in the diff. |
…'s local result Reviewer caught Facts-Flow-Forward gap: eval_branch returned the body node's eval_node result, but the substrate names BranchPath.output (PortId) as the arm's authoritative result-port edge. An arm whose output is a payload-bound or shared port (the arm "returns its payload") would drop that fact when the evaluator returned the body's local value. Fix: evaluate body via eval_node for its frame-binding side effects, then read path.output via eval_port for the authoritative arm value. Body diagnostic still propagates through the `?`. New regression test eval_branch_returns_value_at_path_output_not_body_value constructs body that returns a sentinel literal but path.output is the payload port; pre-fix code returned the sentinel, post-fix returns the payload value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
Verdict: APPROVE The diff is narrowly scoped to evaluator branch-arm handling, adds typed fail-closed outcomes for unresolved patterns, non-variant scrutinees, and no matching arm, and covers the new behavior with focused unit tests. I did not see a concrete violation of the pinned invariants, modeling discipline, coding guide, or testing guide in the changed lines. Per instruction, I did not run builds or tests. |
|
Review metadata
Reviewed Verdict: APPROVE — The diff adds eager branch evaluation with typed |
|
Review metadata
Reviewed Findings: None. The diff aligns with fail-closed behavior (typed Verdict: APPROVE — Narrowly scoped E4 branch coverage with clear fail-closed semantics and regression tests; no concrete violations of |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
18e23543· Trigger:schedule - Thinking:
383s wall
BLOCKING (2)
Root Cause
src/v3/compiler/src/lib.rsruntime Bool inhabitation is split between LiteralBits::Bool and VariantValue tag identities → materialize Disj variants with DeclarationId tags, or make eval_branch map Bool literals to the resolved True/False arm ids from the branch input type.src/v3/compiler/src/lower.rsproducerless arm outputs are encoded by setting Path.body to the Branch node itself → either stop using branch.id as an arm-body sentinel or make eval_branch treat BranchPath.output as authoritative without mandatory body evaluation.
| let scrutinee = eval_port(dag, branch.input, state, strategy)?; | ||
| let (tag, payload) = match scrutinee { | ||
| Value::VariantValue { tag, payload } => (tag, payload), | ||
| _ => return Err(EvalError::BranchScrutineeShape { node: branch.id }), |
There was a problem hiding this comment.
BLOCKING: BranchScrutineeShape rejects LiteralBits::Bool values from Value/Transform, so well-typed if branches lowered as Branch on Bool cannot evaluate even though Bool is the Disj scrutinee (Facts Flow Forward / semantic authority after lowering).
| // payload directly) would drop that fact if the evaluator | ||
| // returned the body's local result instead. Any body | ||
| // diagnostic propagates through the `?`. | ||
| let _ = eval_node(dag, path.body, state, strategy)?; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review metadata
Verdict: APPROVE — clean, narrowly scoped E4 slice. The diff implements Branch evaluation per PR-B.1 §B.1.3 with the right modeling discipline:
Exploratory observations (non-asks):
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
7507ce7b· Trigger:schedule - Thinking:
175s wall
|
Review metadata
VerdictAPPROVE — The diff only touches Findings: None. No concrete violation with a Exploratory observations (optional): The |
|
Review metadata
APPROVE — Clean, narrowly-scoped slice. The diff adds Branch arm evaluation to the eager LeftFirst evaluator with typed fail-closed variants, two-pass |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
9368424c· Trigger:schedule - Thinking:
277s wall
|
Review metadata
Verdict: APPROVE The diff is narrowly scoped to eager |
|
Review metadata
Findings
Verdict: REQUEST_CHANGES. The Branch path machinery is otherwise typed and well-covered, but this misses the main Bool-branch path created by lowering and supported by |
|
Verified the current-head Bool discriminator finding on |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
7006df9a· Trigger:schedule - Thinking:
214s wall
|
Review metadata
APPROVE — diff is narrowly scoped to PR-E E4: it adds Branch evaluation, four typed fail-closed Modeling discipline checks out:
Tests follow Exploratory observations (non-blocking)
|
|
Review metadata
Verdict: APPROVE — clean slice. The Branch evaluator follows PR-B.1 §B.1.3, fail-closed coverage is thorough (unresolved pre-pass, scrutinee shape, no-matching-arm), frame discipline is balanced on both success and error paths, and the test set is honest (variant tags grounded in real Exploratory observation (non-blocking): the |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
732eeb66· Trigger:schedule - Thinking:
175s wall
1 participant
Summary
Implements
eval_branchper PR-B.1 §B.1.3 over E0/E2 frame discipline and E1'seval_portDAG-membership/producers-first authority. Smallest Branch-only slice; scoped to non-Bool / user-Disj Branch execution (type Sign = Plus | Minus-style tagged unions). Boolifevaluation is deferred — see "Deferred substrate prerequisite" below.What lands
eval_branch(dag, branch, state, strategy) -> Result<Value, EvalError>insrc/v3/compiler/src/lib.rs(evaluatormodule):eval_port(LeftFirst).Value::VariantValue { tag, payload }shape check; non-variant scrutinee → fail-closedBranchScrutineeShape.UnresolvedVariant(Fail-Closed C-8); pass 2 selects matchingResolvedVariant(decl) == tag; no match →BranchNoMatchingArm.EvalFrame, bindPayloadBinding.payload_portviabind_top, evaluate body viaeval_nodefor side effects, read authoritative arm value atpath.outputviaeval_port, pop frame.path.body == branch.id(the lower.rs sentinel for arms whose result port has no producer node, seelower.rs:6133-6134, 6265), skip body evaluation and readpath.outputdirectly. Avoids re-enteringeval_branchon the same node.EvalErrorextensions:BranchUnresolvedVariant,BranchScrutineeShape,BranchNoMatchingArm,FrameError(EvalFrameError)withFrom<EvalFrameError>. Variants match PR-B.1's fail-closed catalog and the E0 readiness audit (docs(evaluator): R3 E4 branch readiness / blocker audit (docs-only) #1388).eval_nodedispatch:Behavior::Brancharm wired toeval_branch; other behaviors continue fail-closedUnsupportedBehaviorper E1.Deferred substrate prerequisite — Bool
ifevaluationBool currently has two un-bridged runtime representations:
Value::LiteralValue(LiteralBits::Bool(_))— what E1'seval_valuereturns for a Bool ValueNode (per PR-A.1 runtime model).BranchPattern::ResolvedVariant(<True | False decl>)— whatlower.rs:6131-6165produces forif-then-elsepatterns (resolved against Bool'sDisjchildren perinfer.rs:984-995).There is no canonical reification bridge between the two in the substrate today. Bool
ifevaluation is therefore intentionally NOT in this PR's scope —eval_branchcorrectly fail-closesBranchScrutineeShapeon aLiteralValue(LiteralBits::Bool)scrutinee, surfacing the gap rather than masking it with a localLiteralBits::Bool → True/False DeclarationIdmapping. Substrate (issue #1130) owns the canonical shared Bool literal reification bridge and ratchets; E4 will pick up Boolifafter that lands. Per parent disposition: "Do not add local Bool literal→True/False declaration mapping in E4."Tests (8 new, all passing locally)
eval_branch_selects_resolved_variant_by_tageval_branch_binds_payload_in_fresh_frame_for_body(frame discipline; body-reads-payload waits on E3/E6)eval_branch_returns_value_at_path_output_not_body_value(path.output is authoritative)eval_branch_handles_producerless_arm_sentinel(path.body == branch.id sentinel from lower.rs)eval_branch_fails_closed_on_unresolved_varianteval_branch_fails_closed_on_late_unresolved_arm_even_if_earlier_arm_matches(full pre-pass)eval_branch_fails_closed_on_non_variant_scrutinee(covers Bool literal case as fail-closed today)eval_branch_fails_closed_on_no_matching_armevaluate_body_dispatches_branch_through_eval_nodeOut of scope
ifevaluation — gated on Substrate session/jolly-ram-908 · jolly-ram-908 #1130 reification bridge.Transform/Loop/Bindbodies — wait on E3 / E5 / E6.EvalStrategy/InputEvaluationOrderinhabitants.BranchPatternvariants, newValuevariants.EvalThunk, lazy strategy, memoization.Cross-references
r3-evaluator-dispatch.md§E4.🤖 Generated with Claude Code