Repository navigation
docs(evaluator): E6-G0d constructor runtime execution brief - #1725
Conversation
|
Manager review: please patch before merge. The high-level gate shape is right, but the implementation section needs one authority fix before this is safe to hand to a worker:
Also please refresh the stale source anchors in the lowerer section. On current main I see the key anchors at roughly:
The rest of the brief is aligned with the dispatch fences: existing |
|
Review metadata
Verdict: APPROVE This is a docs-only worker brief, and the added guidance stays aligned with the rubric: it preserves Dag declarations as the constructor authority, keeps evaluator work to runtime interpretation, requires fail-closed shape/arity handling, and explicitly blocks mirror registries or new runtime carriers. No invariant, coding, or testing violations observed in the diff. Tests not run, per review instructions. |
|
Review metadata
1. Story of the diffThis PR adds a single worker brief for the next evaluator slice after E6-G0c. The brief identifies that the evaluator now executes 2. Invariant categories
Compliant — this is a docs-only brief, and it explicitly forbids parser/lowerer edits, widening
Compliant — single authority / facts-flow-forward are handled by keeping declaration connective shapes, variant membership, payload labels, record labels, generic substitution, and constructor identity in the Dag/lowerer authority, while the evaluator only reads those facts to build existing runtime values (
Compliant — the brief keeps the implementation surface small and localized to the existing
Compliant — no tests are required for this docs-only PR, but the brief requires the implementation PR to land focused evaluator ratchets in the existing
N/A — the diff does not reference or alter a locked thesis/design decision; it is a worker brief that explicitly avoids
Compliant — the brief is a planning scaffold, but it is bounded: status/non-goals identify what this PR is not, STOP conditions prevent authority drift, and the dissolution trigger is concrete: the next PR should be “implementation + ratchets,” with a constructor-returning body passing in the same PR that teaches the evaluator to construct the value ( 3. VerdictAPPROVE. The diff is a clean docs-only worker brief that names the current evaluator/lowerer seam, preserves substrate authority, keeps the future runtime change bounded to existing |
|
Patched in |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
7502df7f· Trigger:schedule - Thinking:
221s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsresolve_expected_variant_constructor can return an anonymous Instantiation for a generic variant while branch patterns resolve to the template variant id → specify that evaluator constructor execution must recover the template variant for membership/tag identity and carry the instantiation arguments for payload field walking.
| ## Responsibility Split | ||
|
|
||
| - **Substrate authority remains in the Dag declarations.** Declaration | ||
| connective shapes, variant membership, payload field labels, record field |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Patched the generic constructor blocker in |
|
Verified against current PR head |
|
Review metadata
Verdict: APPROVE — docs-only worker brief, scoped as an implementation/audit guide with clear non-goals and stop conditions. I don’t see concrete violations of the pinned modeling, coding, or testing discipline in the diff. |
|
Review metadata
1. Story of the diffThis PR adds a single worker brief for the next evaluator slice after E6-G0c: constructor runtime execution for lowered 2. Invariant categories
Compliant — this is docs-only and explicitly avoids substrate/runtime carrier expansion: “No new
Compliant — P2 / single authority is handled by making
N/A — no Rust implementation, helper, method, module, or API shape is added in this diff. The future implementation guidance does, however, point toward reuse/factoring instead of adding a registry mirror (
Compliant — no tests are required for this docs-only brief, and the brief requires the implementation PR to land focused evaluator ratchets in the existing
N/A — the diff does not alter a locked substrate/design decision. It references existing evaluator/lowerer authorities and constrains the future slice away from new substrate carrier design (
Compliant — the worker brief is a scaffold, but it is tracked: status is declared up front ( 3. VerdictAPPROVE. The PR is a scoped documentation brief that preserves single authority, keeps constructor execution inside existing runtime carriers, and names the executable ratchets and STOP conditions for the implementation slice. I do not see a diff-line-supported invariant violation. |
|
The variant authority fix is good, including the generic-instantiation preservation. One cleanup remains before I clear this: the lowerer source anchors are still stale on current Please update the brief's lowerer line references to the current anchors:
Current text still cites ranges like |
|
Patched the anchor cleanup in |
|
Review metadata
Verdict: APPROVE — this is a docs-only worker brief, and the added guidance is consistent with the pinned modeling, coding, and testing rubrics. I did not find a concrete invariant violation in the diff. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
9096cc87· Trigger:schedule - Thinking:
195s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/infer.rsresolve_branch_patterns resolves BranchPattern::ResolvedVariant from the parent Disj.variants template ids, while the brief tells E6-G0d to tag VariantValue with callee_decl for Instantiation targets → specify template-vs-instantiation tag equality explicitly and make the generic pack/unpack composition ratchet required.
| constructor `Callable` target and already-evaluated operands, the evaluator | ||
| may construct `Value::RecordValue` or `Value::VariantValue` using facts read | ||
| from `dag.declaration(target)`. It may enforce arity and shape fail-closed. | ||
| - **PB / Population A stays a consumer.** Pop A property migration should wait |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Verified against current PR head |
|
Patched the generic tag-normalization blocker in |
|
Verified against current PR head |
|
Review metadata
APPROVE — Docs-only addition of a worker brief. The brief is precise, cites concrete file:line anchors in the live evaluator/lowerer, explicitly preserves single substrate authority (Dag declarations), forbids Rust-side mirror registries / new |
|
Content cleared from my side. The requested fixes are present in the final file:
|
|
Review metadata
1. Story of the diffThis PR adds a single docs-only worker brief for the next evaluator slice, E6-G0d. The brief identifies the current runtime gap: the evaluator now executes Arrow/UserDefined 2. Invariant categories
Compliant — the diff is docs-only and explicitly avoids substrate/carrier edits: “No new
Compliant — single-authority and facts-flow-forward are handled by making
N/A — no Rust code, helpers, methods, error/result types, or module placement are introduced. The brief does constrain future code shape by requiring reuse/factoring of existing declaration-walking helpers and forbidding a Rust-side mirror registry (
Compliant — because this PR is a brief rather than an implementation, it does not need to add executable tests here; the future implementation is explicitly required to add focused evaluator tests in the existing
N/A — this diff does not alter a locked design surface. It explicitly keeps out parser/lowerer edits,
Compliant — the brief is bounded as a next-slice worker/audit artifact, and it names the concrete follow-on receipt: “implementation + ratchets,” with constructor-returning bodies that fail today and pass in the same implementation PR ( 3. VerdictAPPROVE. The PR is a clean docs-only brief that narrows the future evaluator work to existing runtime carriers and Dag declaration authority, with fail-closed behavior and focused ratchets spelled out. I did not find a diff-citable invariant violation. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
01753e55· Trigger:schedule - Thinking:
177s wall
BLOCKING (1)
Root Cause
docs/briefs/r3-pr-e6-g0d-constructor-runtime-execution-worker.mdConstructor normalization was tightened only for variant Instantiation targets → apply the same peel-template/retain-arguments rule to record constructor targets and require a generic record constructor ratchet.
| A payload that walks to `Conj` under the retained substitution arguments | ||
| becomes `RecordValue(fields)`; a nullary variant uses | ||
| `RecordValue(Vec::new())`. | ||
| - If no parent `Disj` membership is found, then a |
There was a problem hiding this comment.
BLOCKING: The record fallback only recognizes bare Conj targets, but record literals forward the expected declaration as the Callable target, so generic records can arrive as Instantiation and would lose the P2 facts-flow/substitution path.
|
Verified against current PR head |
|
Patched the generic record-constructor blocker in |
|
Verified against current PR head |
|
Review metadata
1. Story of the diffThis PR adds a docs-only worker brief for the next evaluator slice, scoped explicitly away from implementation, parser/lowerer edits, The load-bearing design choice is authority preservation: record fields, variant membership, generic substitution, and constructor identity remain facts read from DAG declarations/lowerer-produced callable targets, while the evaluator only interprets those facts into runtime values and fail-closes on arity or shape mismatches ( 2. Invariant categories
3. VerdictAPPROVE No blocking or non-blocking findings. The brief is tightly scoped, preserves constructor/runtime authority in existing DAG facts and existing evaluator carriers, and requires the right generic and runtime-tag ratchets for the follow-up implementation. |
Summary
docs/briefs/r3-pr-e6-g0d-constructor-runtime-execution-worker.md.TransformTarget::Callable(target).Value::RecordValue/Value::VariantValuecarriers, with STOP fences for substrate, lowerer, X1.b, Pop A, and fold-lens scope.Verification
git pushpre-push hook rancargo fmt --all --checksuccessfully.