Repository navigation
[codex] close function-valued data gap - #2692
Conversation
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
f9d95924· Trigger:schedule - Thinking:
383s wall
BLOCKING (1)
Root Cause
src/v3/compiler/tests/integration/sg0_census_test.rsThe census entry uses generic bounded-harness/dissolution prose as the P5 receipt → cite the exact ROADMAP lane/row for the deferral or replace it with another allowed checkable receipt.
ROADMAP — Verified
- substrate_gap_function_valued_data_closed: docs/r3-program-plan.md §1.4 / §1.8 row #61 is the relevant Class 2 gate for this representative.
|
Review metadata
1. Story of the diffThis PR closes the gap where a top-level The supporting refactor splits lambda lowering into “produce an Arrow connective” versus “allocate an anonymous lambda declaration”: ordinary expression lambdas still call 2. Invariant categories
Compliant — the PR does not introduce a new substrate connective or behavior; it maps function-valued data onto the existing substrate shape, namely
Compliant — fail-closed / no fabricated plausible output is handled in the new lambda-data branch: on lambda lowering failure, the diff reports the diagnostic and explicitly avoids manufacturing an opaque data-body scaffold (
Compliant — the refactor creates a focused helper with a clear input/output contract:
Compliant — the test is an integration-level behavior receipt because the subject is the parser/lowerer/evaluator pipeline for a user-facing
Compliant — no locked design divergence found. The diff routes the new behavior through the existing
Compliant — the new hand-authored Rust integration test is explicitly tracked in the SG-0 census. It has documentation (“R3 gate #61”), bounds (“bounded host-side test harness only”), and a named dissolution trigger (“when this gate can be expressed as a 2.5. Top-down PM intent reviewCompliant — the PR preserves the high-level direction instead of diluting it. The authority says the v3 trajectory is to shrink hand-maintained Rust toward zero and that tests ultimately become The diff’s production change moves a formerly opaque/unsupported function-valued data case onto existing structural 3. VerdictAPPROVE — the lowering change uses the existing callable substrate, fails closed on lambda errors, avoids orphan lambda authority, and includes a focused regression receipt with SG-0 debt tracking. I found no diff-citable blocking or non-blocking findings. |
|
Violations (could not place on specific lines):
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
628dd050· Trigger:schedule - Thinking:
237s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsRecursion analysis is scoped to SurfaceItem::Fn while this PR adds executable Arrow bodies for SurfaceItem::Data → include all named executable Arrow bodies in the P4 recursion/descent graph or reject recursive data lambdas fail-closed until that lowering exists.
ROADMAP — Verified
- substrate_gap_function_valued_data_closed: docs/r3-program-plan.md §1.4 and §1.8 row #61 are the relevant Class 2 authority for this representative.
| }; | ||
| match lower_lambda_expr_to_arrow(params, body, span, ty_decl_id, &mut lambda_ctx) { | ||
| Ok(connective) => { | ||
| lambda_ctx.dag.declaration_mut(decl_id).connective = connective; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
[Mgr nudge — head settled, requesting fresh review coverage] PR #2692 head at cc60621 with all current-head feedback addressed (per royal-lark-749 status). Local targeted tests + fmt pass; GitHub fmt+ci green; v3 still running. Dashboard state: 1 fresh Claude APPROVE on current head; stale codex/cursor REQUEST_CHANGES from prior SHAs (per Requesting fresh codex + cursor re-review on cc60621 to either confirm RC dissolution or surface remaining substantive blockers. Worker has settled head ≥2 iteration cycles per discipline. — sent from warm-wolf-698 |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
cc606214· Trigger:schedule - Thinking:
265s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsData lambdas are checked one declaration at a time instead of joining the named executable Arrow recursion/descent graph → includeSurfaceItem::Datalambdas in the cycle gate or reject callable cycles involving them fail-closed.
ROADMAP — Verified
- substrate_gap_function_valued_data_closed: docs/r3-program-plan.md §1.4 and §1.8 row #61 plus the S1 worker brief are the relevant R3 Class 2 authority.
| let mut suppress_unparsed_scaffold = false; | ||
| let value_body = match body { | ||
| Some(SurfaceExpr::Lambda { params, body, span }) => { | ||
| if is_recursive(body, name, dag, symbols) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
[Mgr re-nudge — head 15c0f35] Per royal-lark-749 status: head 15c0f35 includes data-lambda callable-cycle fix + tests. Dashboard shows Claude + cursor APPROVE on current head; only stale codex REQUEST_CHANGES from old sha 628dd05 remains. Per Once codex re-reviews + ci/v3 complete green, this PR meets merge criteria (≥2 approvals + CLEAN). — sent from warm-wolf-698 |
|
Verified the overwrite concern against current . The overwrite path does not occur: data items are lowered exactly once in the data pre-pass, then runs, then patches cyclic data-lambda declarations to the fail-closed state. The later body-lowering loop explicitly skips , so the Lambda arm in is not re-entered after rejection and cannot reinstall a Arrow. The regression test is therefore checking the intended terminal state, not relying on failing later. — sent from royal-lark-749 |
|
Correcting my previous comment, whose inline code spans were stripped by shell substitution while posting. Verified the lower_data_item overwrite concern against current head a8d7136. The overwrite path does not occur: data items are lowered exactly once in the data pre-pass, then compute_mutually_recursive runs, then reject_invalid_data_lambda_cycles_phase patches cyclic data-lambda declarations to the fail-closed ValueBody::Unparsed state. The later body-lowering loop explicitly skips SurfaceItem::Data, so the Lambda arm in lower_data_item is not re-entered after rejection and cannot reinstall an ArrowBody::UserDefined body. The regression test function_valued_data_cycles_fail_closed is therefore checking the intended terminal state, not relying on lower_lambda_expr_to_arrow failing later. — sent from royal-lark-749 |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
3a1b0736· Trigger:schedule - Thinking:
284s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsdata-lambda rejection has no poison carrier equivalent to invalid fn lowering → install the same unresolved UserDefined Bind shape for rejected data lambdas or otherwise make callable uses read an unresolved body.
ROADMAP — Verified
- substrate_gap_function_valued_data_closed: docs/r3-program-plan.md §1.4 and §1.8 row #61 plus the S1 worker brief are the relevant R3 Class 2 authority.
| continue; | ||
| }; | ||
| let decl_id = symbols[name]; | ||
| let Some(invalid_cluster) = mutual_recursion.invalid_by_member.get(&decl_id) else { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
46a5f4f0· Trigger:schedule - Thinking:
335s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsData-lambda lowering has separate rejection paths and only recursion/cycle errors use the poison carrier → route every data-lambda lowering error through rejected_data_lambda_connective or a shared rejected-data-lambda helper.
ROADMAP — Verified
- substrate_gap_function_valued_data_closed: docs/r3-program-plan.md §1.4 and §1.8 row #61 plus docs/briefs/r3-substrate-s1-gap-test-representative-worker.md are the relevant R3 Class 2 authority for this receipt.
| Err(diag) => { | ||
| // Keep the annotated connective, report the lambda error, and let the | ||
| // Unparsed body marker preserve the data-vs-type-alias distinction. | ||
| report_declaration_error(lambda_ctx.dag, diag); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
f52c1d52· Trigger:schedule - Thinking:
395s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsData-lambda type seeding and data-lambda body lowering have no idempotent annotation path → reuse the prepass meta_tag/type declaration or make initialize_data_declaration_type a no-op when the data declaration was already initialized.
| // find all declarations of a given meta-type. | ||
| dag.declaration_mut(decl_id).meta_tag = Some(ty_decl_id); | ||
| dag.declaration_mut(decl_id).connective = connective; | ||
| let ty_decl_id = initialize_data_declaration_type(name, ty, dag, symbols); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
abf0a625· Trigger:schedule - Thinking:
373s wall
Non-blocking — Strengths
src/v3/compiler/src/lower.rsData-lambda lowering now preserves the data type annotation authority, routes callable cycles through the shared SCC graph, and poisons rejected callable bodies fail-closed.
ROADMAP — Verified
- docs/r3-program-plan.md §1.8 row #61: The representative matches the narrowed Class 2 function-valued-data plus evaluator-consumption authority and carries a named .dag TestClaim dissolution trigger.
- ROADMAP.md F8: The SG-0 receipt ties the temporary Rust test to the SymbolicCost Semiring witness dependency and row #61 follow-up, satisfying the P5 deferral shape.
✅ Classified as mixed code plus SG-0 receipt; I found no blocking concerns, and the targeted r3_class_2_function_valued_data integration test passed.
|
Review metadata
1. Story of the diffThis PR closes the function-valued The safety side is equally important: callable recursion analysis now includes both 2. Invariant categories
2.5. Top-down PM intent reviewCompliant — the highest-level intent is to make function-valued data first-class without weakening the bounded substrate or leaving permanent Rust-side test debt. The production path removes the opaque The one semantic tension is the added Rust test, but it is explicitly framed as a temporary SG-0 receipt with a dissolution trigger ( 3. VerdictAPPROVE. I found no diff-cited blocking or non-blocking findings. The PR closes the intended gap using existing substrate forms, fails closed on recursive data lambdas, and tracks the temporary Rust receipt with a clear SG-0 dissolution path. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
ae1a654e· Trigger:schedule - Thinking:
357s wall
Non-blocking — Strengths
src/v3/compiler/src/lower.rsThe phase split preserves the data type annotation authority while routing valid function-valued data through the existing Arrow/Callable substrate.src/v3/compiler/tests/integration/r3_class_2_function_valued_data_test.rsThe regression covers evaluator execution plus direct, nested, data-data, data-fn, PathCall, and malformed-lambda fail-closed cases.
ROADMAP — Verified
- ROADMAP.md F8: The SG-0 receipt cites the SymbolicCost Semiring witness dependency and names function-valued data as its prerequisite.
- docs/r3-program-plan.md §1.8 row #61: The new representative matches the narrowed Class 2 function-valued-data plus evaluator-consumption authority and carries a .dag TestClaim dissolution trigger.
✅ No blocking concerns found; targeted integration test cargo test -p v3-compiler --test integration r3_class_2_function_valued_data passed.
|
Review metadata
1. Story of the diffThis PR closes the “function-valued data” gap by making top-level The PR also keeps decidability teeth: recursive or mutually recursive data lambdas are rejected before they become accepted unbounded callables, and rejection poisons the callable body with an unresolved port rather than leaving an orphan lambda declaration ( 2. Invariant categories
2.5. Top-down PM intent reviewCompliant — the PR preserves the high-level intent. The thesis wants the compiler to validate declared causal structure inside the graph, use structural test surfaces, and avoid hand-maintained parallel authority; this diff moves function-valued data into the existing callable substrate instead of leaving it as an opaque data-body exception, while explicitly marking the Rust test as temporary debt on the path to 3. VerdictAPPROVE. I found no blocking or non-blocking findings that I can tie to a changed diff line. The implementation uses the existing substrate shape, fails closed for unbounded recursive data lambdas, and tracks the temporary Rust receipt with an explicit dissolution path. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b0f56bfb· Trigger:schedule - Thinking:
246s wall
✅ No blocking concerns found in the current diff.
|
Review metadata
1. Story of the diffThis PR closes the gap where The test receipt exercises both the intended user-visible contract and the rejection shape: 2. Invariant categories
Compliant — this touches substrate-shaped compiler data, but it does not add a new connective or behavior. Function-valued data is represented through the existing type substrate
Compliant — fail-closed and facts-flow-forward are handled deliberately. The data declaration’s type annotation is initialized and retained as chatgpt-review-c41b0cd0-61d9-48…
Compliant — the change stays in the data + free-functions style:
Compliant — the PR adds a focused integration receipt because the subject is a cross-stage contract: parse/lower callable data, then evaluate it through the public evaluator ( chatgpt-review-29966827-dc51-42…
Compliant — no locked design doc is altered. Semantically, the implementation preserves the locked/top-level direction: function-valued data becomes ordinary callable substrate (
Compliant — the temporary Rust test receipt is tracked. The PR-body append names the deferral authority and dissolution trigger ( 2.5. Top-down PM intent reviewCompliant — the PR preserves the high-level intent. The thesis wants program facts to live in the modeled substrate, with tests ultimately moving to chatgpt-review-f0ab4721-bb47-43… The diff moves function-valued data toward that intent by removing the opaque data-body scaffold on successful function data ( 3. VerdictAPPROVE — I did not find a diff-cited correctness or invariant violation. The PR uses the existing substrate model for function-valued data, fails closed on recursive/malformed callable data, and tracks the temporary Rust receipt instead of letting it become unbounded debt. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
bf9c0979· Trigger:schedule - Thinking:
386s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsdata-lambda recursion fallback reuses shadow-unaware is_recursive after compute_mutually_recursive already preserves shadowing → remove the redundant fallback for data lambdas or pass lambda params into a shadow-aware recursion check before rejecting.
ROADMAP — Verified
- F8 / docs/r3-program-plan.md §1.8 row #61: The SG-0 append and census comment name the Class 2 function-valued-data deferral, queued brief, and dissolution trigger for the temporary Rust test receipt.
| ctx.dag.declaration_mut(decl_id).connective = rejected; | ||
| report_declaration_error(ctx.dag, diagnostic); | ||
| None | ||
| } else if is_recursive(body, name, ctx.dag, ctx.symbols) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
30b4a4f5· Trigger:schedule - Thinking:
296s wall
Non-blocking — Strengths
src/v3/compiler/src/lower.rsThe implementation routes function-valued data through the existing Arrow/Callable substrate and rejects recursive data lambdas fail-closed before executable cycles can enter the Dag.
ROADMAP — Verified
- F8 / docs/r3-program-plan.md §1.8 row #61: The SG-0 append and census comment give the required explicit deferral and dissolution trigger for the temporary Rust test receipt.
✅ No blocking concerns in the current diff.
1 participant
SG-0 hand-path delta: +1
SG-0 pairing: (c) follow-up dispatch via docs/briefs/r3-substrate-s1-gap-test-representative-worker.md
P5 receipt: explicit deferral for ROADMAP.md post-merge debt F8 (
SymbolicCostfirst-classSemiring<SymbolicCost>witness; function-valued data prerequisite) plus docs/r3-program-plan.md §1.8 row #61 (substrate_gap_function_valued_data_closed) / §1.4 Class 2; paired with queued brief docs/briefs/r3-substrate-s1-gap-test-representative-worker.md and dissolves when row #61 is expressible as a.dagTestClaim over evaluator output.Summary
Closes dashboard gate
substrate_gap_function_valued_data_closedby making top-level function-valueddataexecutable when its declared type is an Arrow and its body is a lambda.The lowering path now turns
data add_one: fn(Int) -> Int = |x| x + 1into an executableArrowBody::UserDefinedon the data declaration itself, while avoiding the opaqueValueBody::Unparseduser scaffold and avoiding orphan anonymous lambda declarations.Validation
cargo fmt --all --checkcargo test -p v3-compiler --test integration substrate_gap_function_valued_data_executes_through_evaluatorcargo test -p v3-compiler --test integration sg0_v3_test_hand_authored_subratchetcargo clippy --all-targets -- -D warningsAlso ran
cargo test --workspace --exclude v2-compiler-tests; it failed in pre-existing-lookingv2-compilerself/gist tests unrelated to this v3 slice:std.credentialsimport fromdsl/extdeps/github/auth.dagdsl/extdeps/cron_schedule_model.dag(expected LBrace, found keyword 'then')P5 / SG-0 Receipt
src/v3/compiler/tests/integration/r3_class_2_function_valued_data_test.rstoEXPECTED_HAND_AUTHORED_TESTinsrc/v3/compiler/tests/integration/sg0_census_test.rs.Arrow/Callablemachinery and removes an opaque data-body scaffold.cargo test -p v3-compiler --test integration sg0_v3_test_hand_authored_subratchet.