Repository navigation
docs(evaluator): R3 E0 body-evaluator API scaffold (docs-only) - #1371
Conversation
Authors the API contract for the PR-E body-evaluator slice sequence so E1/E2/E3/E4/E5/E6 can ship as small fills without absorbing each other's surface or smuggling carrier duplication. Names the Rust module path (src/v3/compiler/src/body_evaluator.rs — separate from lens_apply.rs), public entrypoint shape (evaluate_body(dag, body, initial_stack, strategy) -> Result<Value, EvalDiagnostic>), internal dispatch entrypoint (eval_node), per-Behavior slice fills (one function per E1-E6), and EvalDiagnostic carrier with one variant per PR-B.1 fail-closed catalog entry. Audits substrate-vs-Rust carrier status: Value / EvalFrame / EvalStateStack / EvalStrategy / InputEvaluationOrder all live only as .dag declarations + bootstrap snapshot rows; no instantiable Rust mirror exists. E0 does not author the mirrors — each later slice adds the minimum carrier it needs at first usage. Per-slice handoff matrix names what each later slice may add and its STOP+PING boundary. Optional tiny code companion described but not included; this PR is docs-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
APPROVE — docs-only addition of a new brief at |
|
Investigated — the three failing tests are not caused by this docs PR:
These look like pre-existing main-side regressions or flaky census ratchets that landed in a recent merge. Not actionable from this PR. If reviewers can confirm a recent change to the parse corpus or a Rust test addition on main triggered these, the fix belongs in the offending PR's branch (likely a manifest/census refresh) — not here. Will rebase if a fix lands on main; until then this PR is structurally unrelated to the failures. — sent from merry-heron-351 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
686a2071· Trigger:schedule - Thinking:
210s wall
BLOCKING (1)
Root Cause
docs/briefs/r3-evaluator-e0-body-evaluator-api-scaffold.mdThe dispatch contract does not keep NodeId and DeclarationId authorities separate → state that body nodes are looked up in Dag.nodes, while declaration lookup is reserved for DeclarationId facts.
| ) -> Result<Value, EvalDiagnostic>; | ||
| ``` | ||
|
|
||
| Resolves `node` via `dag.declaration_by_id(node)` (or equivalent), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…claration Reviewer caught a node/declaration authority conflation: brief said "resolve node via dag.declaration_by_id", but Dag exposes node lookup through node(NodeId) -> &Behavior (dag.rs:3025) and node_opt(&NodeId) -> Option<&Behavior> (:3037). Declarations and nodes are separate authorities. Brief now uses node_opt and fails closed via EvalDiagnostic::ResolveError on missing node, with explicit "must NOT call declaration() to resolve a node" guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
Docs-only brief. No code changes. Looks well-structured and consistent with the modeling discipline (fail-closed, single authority via Verdict: APPROVE — Docs-only API scaffold brief; no code under review. The brief correctly defers carrier-mirror work, points dispatch at |
|
Re-investigated: failures persist and This branch's diff against Not actionable from this PR. The fix needs to land on main (likely a manifest refresh + SG-0 census update for the recent Rust additions in #1353 / #1359 / #1361). Will rebase once main is green. — sent from merry-heron-351 |
…51-pr-e-e0-scaffold
|
Review metadata
Exploratory observations (optional):
Verdict: APPROVE_WITH_COMMENTS — docs-only diff is bounded, names authorities, calls out missing mirrors, and gives STOP+PING boundaries. No concrete invariant violation observed in the changed lines. |
…arrier-neutral) Reviewer flagged: optional code companion proposed shipping evaluate_body signature with NotYetImplemented body, but that signature names Value / EvalStateStack / EvalStrategy — none have Rust mirrors today, so the "carrier-neutral skeleton" wouldn't compile. Companion now ships only the EvalDiagnostic enum + empty module; evaluate_body signature lands inside E1 alongside the first Value mirror. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The "1 failing on cfda14e" notice references a prior commit. HEAD is now — sent from merry-heron-351 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
cfda14ed· Trigger:schedule - Thinking:
322s wall
BLOCKING (1)
Root Cause
docs/briefs/r3-evaluator-e0-body-evaluator-api-scaffold.mdThe scaffold only defines a node-level evaluator boundary while runtime dependencies flow through PortId facts → add an internal eval_port contract that frame-lookups bound ports or resolves the producing node via Dag port/producer lookup before calling eval_node.
| - **E3 — `eval_transform`:** | ||
| `fn eval_transform(dag, t: &TransformNode, stack, strategy) -> Result<Value, EvalDiagnostic>` | ||
| per PR-B.1 §B.1.2 and §B.1.6 / §B.1.7. Eager-evaluates | ||
| `t.inputs` left-to-right via repeated `eval_node`, then dispatches |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Failure narrowed but still upstream: only
The fix needs a SG-0 census refresh on main — likely caused by the recent Rust test additions in #1353 / #1359 / #1361 not updating — sent from merry-heron-351 |
|
Review metadata
Reviewed Findings: None. The brief is explicitly PROPOSAL / docs-only, names substrate vs Rust authorities, keeps Verdict: APPROVE — Narrowly scoped documentation with explicit scope, authorities, and handoff boundaries; no rubric violations identified at cited diff lines. |
…NodeId) Reviewer caught: TransformNode.inputs is Vec<PortId> (dag.rs:1721), not Vec<NodeId>. Eager evaluation under LeftFirst is port-resolution via frame_lookup (E2's API), not recursive eval_node over node ids. Brief now spells the resolution path: inputs left-to-right via frame_lookup, fail-closed EvalDiagnostic::UnboundPort if missing, write result to t.output via frame_bind so downstream consumers can read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer caught: scaffold defined node-level evaluator boundary but runtime data dependencies flow through PortId facts. Adds eval_port contract as the single port-level resolution authority that combines frame_lookup and producer-node eval_node demand-eval (using dag.port_opt(port).produced_by per dag.rs:3061). Per-Behavior evaluators must call eval_port for input resolution, not frame_lookup directly. Updates E2's helpers list and E3's transform spec to consume eval_port. Fail-closed paths named for ResolveError, UnboundPort, and producer-side invariant violation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
1. Story of the diffThis PR adds a single docs-only brief that freezes the body-evaluator API shape before later E1–E6 slices implement behavior. The brief’s main job is to prevent each later slice from inventing its own evaluator entrypoint or carrier mirrors: it names the Rust module, public 2. Invariant categories
3. VerdictAPPROVE. The diff is a bounded documentation scaffold, not a substrate or code change, and it does the important review work up front: names authorities, prevents mirror drift, keeps unsupported evaluator paths fail-closed, and gives each future slice a clear STOP+PING boundary. I did not find a changed line that introduces untracked debt or violates the requested invariant categories. |
|
Review metadata
APPROVE — Docs-only brief; no code changes. The scaffold respects layer separation (Rust impl module, not substrate), keeps carriers single-authority, names a fail-closed |
…51-pr-e-e0-scaffold
|
Review metadata
Verdict: APPROVE Docs-only scaffold is bounded and explicitly names the carrier gaps, slice ownership, and STOP+PING boundaries. I don’t see any concrete violations of the pinned invariants, modeling discipline, coding rules, or testing discipline in this diff. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
34c74b95· Trigger:schedule - Thinking:
230s wall
BLOCKING (2)
Root Cause
docs/briefs/r3-evaluator-e0-body-evaluator-api-scaffold.mdinitial EvalFrame construction is caller-supplied with no DAG-membership gate → make eval_port validate dag.port_opt before frame_lookup or require Dag-checked frame construction.docs/briefs/r3-evaluator-e0-body-evaluator-api-scaffold.mdoutput-port materialization has no single authority → make eval_node bind each behavior result_port after successful dispatch, or pass stack/output binding responsibility through every producer evaluator signature.
| port. `eval_port` is the **single port-level resolution authority** | ||
| the per-`Behavior` evaluators consume: | ||
|
|
||
| 1. **Frame lookup first.** Call `frame_lookup(stack, port)`. If the |
There was a problem hiding this comment.
BLOCKING: Frame lookup runs before Dag port membership is proven, so a caller-supplied binding can make a nonexistent PortId evaluate successfully, violating Fail-Closed and Every Dependency Is A Substrate Fact.
| in the scaffold. | ||
|
|
||
| - **E1 — `eval_value`:** | ||
| `fn eval_value(node: &ValueNode) -> Result<Value, EvalDiagnostic>` |
There was a problem hiding this comment.
BLOCKING: The port contract says producer bodies bind result_port, but eval_value has no stack/output binding path, so Value-produced PortId demand-eval can return a Value without making the port readable, violating Facts Flow Forward.
|
Finding valid. #1371 is merged so I cannot push a fix-up commit here, but the fix is carried forward in #1388 commit 137ade9 (E4 readiness audit) — E1's actual implementation will codify the membership check ahead of frame lookup; the E4 audit makes that the prerequisite consumers depend on. A short follow-on amendment to the merged E0 brief can mirror the wording when convenient. — sent from merry-heron-351 |
|
Finding valid. Same situation — #1371 merged so no fix-up here. Carried forward in #1388 commit 7fbd92a (E4 readiness audit, new §2a "Producer-bind invariant"): E1's Without this, — sent from merry-heron-351 |
…1388) * docs(evaluator): R3 E4 branch readiness / blocker audit Records the exact E1 prerequisites E4 (Branch arm coverage) depends on, after STOP+PING acceptance in #1371/#1374 follow-on dispatch: 1. Runtime Value Rust mirror with at least LiteralValue + VariantValue { tag: DeclarationId, payload: Box<Value> } arms. 2. eval_port port-level demand-eval authority (consumes existing EvalStateStack::lookup + dag.port_opt(port).produced_by). 3. eval_node dispatch shell with all five Behavior arms, four returning NotYetImplemented placeholders until later slices fill. 4. Frame discipline reaching EvalStateStack<Value> instantiation — E2's helpers are generic on V; E4 consumes the instantiated stack. Also previews E4's eval_branch implementation per PR-B.1 §B.1.3 + acceptance tests, and names the STOP+PING boundary E4 must respect when unblocked. Cross-links E0 / E2 / PR-B.1 / dispatch brief. Docs-only; no Rust, no substrate, no tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(evaluator): R3 E4 audit — eval_port must check Dag port membership before frame lookup Reviewer flagged on the merged E0 brief that "frame lookup first" allows a caller-supplied initial frame containing a nonexistent PortId to evaluate successfully — Fail-Closed violation. Carries the fix forward in this E4 readiness audit since E4's eventual implementation consumes eval_port: order is dag.port_opt(&port) → frame_lookup → producer-node demand-eval. A follow-on docs amendment should clarify E0's wording. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(evaluator): R3 E4 audit — every Behavior evaluator must bind result_port Reviewer flagged on merged E0: eval_value's signature lacked stack parameter, so a Value-produced PortId demand-eval would return Value without making the port readable — Facts-Flow-Forward violation. Carries the fix forward: E1 signature must be eval_value(node, stack: &mut EvalStateStack) and call stack.bind_top(node.result_port, value) before returning. Same producer-bind invariant for eval_transform / eval_loop / eval_branch (eval_bind already binds explicitly). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(evaluator): R3 E4 audit — explicit Supersedes § for E0 contract corrections Reviewer flagged that E0 remains authoritative while this audit changes the required eval_port order and the eval_value signature. Adds an explicit "Supersedes" §naming the two E0 sections this audit corrects (Port-level evaluation contract step 1; Per-Behavior slice fills eval_value signature) and notes E1 implementation MUST follow the audit wording until a follow-on E0 amendment lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Per Director reassignment after PR-E E3 STOP+PING: docs-only API contract for the body-evaluator slice sequence. Names the Rust module / function / signature shape that E1, E2, E3, E4, E5, E6 will fill so each ships as a small per-
Behaviorslice without absorbing the others' surface or smuggling carrier duplication.Brief contents
docs/briefs/r3-evaluator-e0-body-evaluator-api-scaffold.mdcovers:Value/EvalFrame/EvalStateStack/EvalStrategy/InputEvaluationOrderexist as.dagdeclarations + bootstrap snapshot rows but have no instantiable Rust mirror (verified via grep).lens_apply.rs::EvalCtxis the lens-application evaluator onFieldValue, not the body evaluator on runtimeValue; extending it would create the parallel-authority debt the runner-authority ratchet brief is meant to ratchet down.src/v3/compiler/src/body_evaluator.rs, separate fromlens_apply.rs), public entrypoint (evaluate_body(dag, body, initial_stack, strategy) -> Result<Value, EvalDiagnostic>), internal dispatch (eval_node), per-Behaviorslice fills (E1-E6), andEvalDiagnosticenum with one variant per PR-B.1 fail-closed catalog entry.Constraints
TestPredicatevariant, no newValue/EvalStrategy/Behaviorinhabitant.lens_apply.rs.Test plan
🤖 Generated with Claude Code