Repository navigation
fix(v3): B2 lower_fn_body Arrow re-derive to fail-closed diagnostic - #817
Conversation
|
Director check-in — direction is right, three open items before flipping ready: Deliverable A (defensive fallback → diagnostic): ✅ landed. The replacement at `lower.rs:3585-3611` correctly emits a diagnostic on the non-`Arrow` arm and early-returns. Good shape. Open items per brief acceptance:
On scope: the brief's STOP-AND-ESCALATE list flagged "audit reveals a legitimate runtime path that produces non-`Arrow` connectives" as a stop condition — your audit (item 1) is what answers that. Don't skip it. Flip to ready once the body documents items 1 + 2 + 3. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
e151b511· Trigger:schedule - Thinking:
166s wall
Non-blocking — Strengths
src/v3/compiler/src/lower.rsThe fallback removal is consistent with FAIL-CLOSED and single authority: a non-Arrow seeded function declaration now emits a diagnostic instead of re-deriving signature facts.
✅ No blocking concerns; this is a clean fail-closed tightening of the lowering invariant.
|
Addressed the Director check-in items:
Verification note: I attempted to mark the PR ready through the GitHub connector, but the connector mutation failed on an internal GraphQL field selection ( |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
a53cb4d8· Trigger:schedule - Thinking:
146s wall
✅ No blocking concerns; the code change is focused and does not introduce new substrate shapes, duplicate authority, or untracked debt.
|
Flipping ready. Apologies for the delay — worker addressed all three Director feedback items in the PR body (seed-phase audit ✅, regression test ✅, ResolveError taxonomy note ✅) but I was checking the diff rather than the rendered description. The PR is complete-and-well-documented; the worker explicitly asked for CI to cover Rust verification post-ready-flip. Sending now. |
|
Review metadata
Verdict: APPROVE — Converts a silent re-derive fallback into a fail-closed diagnostic at Exploratory observation (non-blocking): |
|
Review metadata
Verdict: APPROVE The diff removes a fabricated re-derive fallback and replaces it with a typed diagnostic path, which aligns with Fail-Closed and Single Authority. The added regression test is focused on that behavior. Verified with |
|
Review metadata
1. Story of the diffThis PR removes the defensive re-derivation path in 2. Invariant categories
Compliant — this touches a substrate consumer, not substrate shape:
Compliant — P3 / fail-closed is handled directly: the changed branch reports chatgpt-review-af662107-55e7-49…
Compliant — the change stays in the existing free-function lowering flow, uses explicit inputs already in scope, and routes failure through the existing diagnostic carrier rather than adding hidden state, methods, or a side-channel; the relevant path is
Compliant — the PR adds a focused regression test for the exact behavior change:
N/A — the diff does not alter thesis/design documents, substrate schema, target realization rules, or any explicitly locked design surface.
N/A — the diff introduces no new TODOs, scaffolds, compatibility bridges, or temporary representations; it deletes a local fallback and replaces it with fail-closed behavior. 3. VerdictAPPROVE. The PR is narrowly aligned with the stated fix: it removes a downstream re-derivation fallback, preserves the seeded Arrow as the single authority, and adds a regression test for the fail-closed diagnostic path. |
|
Review metadata
Verdict: APPROVE — Diff converts a silent re-derive fallback into a fail-closed |
|
Review metadata
Verdict: APPROVE Diff is small and aligned with P3 fail-closed and P2 single-authority: the fallback Arrow re-derivation is removed, non-Arrow seeded declarations now emit a typed Verified with |
PM review — APPROVE_WITH_COMMENTSImplementation is sound: defensive fallback at One ask before merging — the brief's Acceptance bullet 2 explicitly requires: "Seed-phase audit recorded in PR description: every Add one paragraph to the PR description stating which case applies:
Otherwise this is a clean approve. The audit narrative is the discipline |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
3a635986· Trigger:schedule - Thinking:
172s wall
Non-blocking — Strengths
src/v3/compiler/src/lower.rsThe non-Arrow function-declaration path now fails closed through a Dag diagnostic instead of fabricating a second signature, matching fail-closed and single-authority discipline.
✅ No blocking concerns; this is a focused implementation-only lowering fix with no new substrate shape, silent fact drop, or untracked scaffold.
|
Verified against the current PR description. This ask is already addressed in the Current body records case (a) Audit found no gap and cites the audited paths:
No code or PR-body change needed for this relayed comment. |
|
Already in the PR body — see "## Seed-Phase Audit" section (case (a): no gap found, structural-assertion-only). The audit traces the relevant paths:
Conclusion in body: "every reachable SurfaceItem::Fn body-lowering path should already have an Arrow connective" — converts dead hedge → fail-closed contract. Diagnostic-taxonomy note also recorded (ResolveError reuse intentional pending future taxonomy lane). Closing as fixed. |
|
Review metadata
Verdict: APPROVE This looks clean. The diff removes the Arrow re-derive fallback and replaces it with a fail-closed diagnostic, preserving the seeded signature as the single authority. The added regression test covers that path and passes with |
|
Review metadata
Verdict: APPROVE — replaces a silent defensive re-derive with a fail-closed diagnostic, exactly the direction INVARIANTS pushes. The new test is hermetic, sets up the violation by skipping Minor exploratory note (not a finding): the |
|
Review metadata
Verdict: APPROVE This looks clean. The diff removes a fabricated Arrow re-derive fallback and replaces it with a fail-closed diagnostic at the invariant breach, which aligns with P3 and the single-authority lowering comment already in place. Verified with |
|
Review metadata
1. Story of the diffThis PR removes a defensive fallback in 2. Invariant categories
Compliant — this is implementation-only lowering code, not a substrate type change. The key decision is to read the seeded
Compliant — fail-closed / single-authority is handled correctly: the old re-derivation path is removed, and the non-Arrow state now emits a diagnostic and returns (
Compliant — the change keeps the logic inside the existing lowering free-function flow, uses explicit inputs already available to
Finding (NON-BLOCKING) —
This pins the test to diagnostic wording rather than a structured diagnostic carrier. The production side creates the same pressure by encoding the lowering-invariant failure inside
N/A — the diff does not reference or alter a locked thesis/design decision; it changes only a defensive branch in Rust lowering and its regression test.
N/A — the diff does not add TODOs, scaffolds, temporary bridges, or new staged representations. It removes a fallback bridge by deleting the Arrow re-derivation path. 3. VerdictAPPROVE_WITH_COMMENTS The substantive behavior moves in the right direction: seeded function |
|
Verified the non-blocking diagnostic-shape comment against the current PR body and code. No code change needed in this PR: the concern is already tracked in the Diagnostic Taxonomy Note. Current state:
So I’m treating the review note as valid but already addressed/documented, with no further change for B2. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
932d41aa· Trigger:schedule - Thinking:
147s wall
✅ No blocking concerns; the diff replaces the only changed fallback with a diagnostic-only fail-closed path and adds focused coverage for the non-Arrow seeded-function invariant.
|
Review metadata
Loop summaryN = 13 review events across roughly 7 time clusters, from 2026-04-25 20:25:50Z to 2026-04-26 00:29:50Z — about 4 hours 4 minutes elapsed. K = 8 codex reviews. L = 2 browser/openai-pro reviews. There are also 3 Claude/API reviews. M commits is not recoverable from the attached review dump because no commit SHAs are included; from the current attached diff, the visible patch state is a single-file chatgpt-review-b9813acc-5526-46… chatgpt-review-b9813acc-5526-46… Forward progress evidenceThis loop made real forward progress on the original debt. The PR deletes a downstream Arrow re-derive fallback in chatgpt-review-b9813acc-5526-46… It also adds a real consumer: The loop also dissolved a scaffold-like fallback rather than adding one. Multiple reviews explicitly say the change introduces no new substrate shape, duplicate authority, or untracked scaffold, and deletes the local fallback bridge. chatgpt-review-b9813acc-5526-46… No new invariant needed to be added to chatgpt-review-b8f05eb3-6951-45… Debt accumulation evidenceThere is one small debt item: the new test identifies the diagnostic by probing free-form text with That debt maps to existing discipline, not a new category. The attached testing guidance also says tests should not pin diagnostic message text; they should match structured diagnostic shape where available. chatgpt-review-80c39707-5659-43… The other repeated observation — the unconditional Cheating signalMostly clean. The implementer is not hiding the main compromise: the main fix is structural for the original problem. It removes the fabricated fallback and makes the seed-phase Arrow the authority. There is, however, a small “good enough for now” signal around diagnostics: using The most recent reviews are not discovering new implementation problems; they are mostly repeating “approve, no blocking concerns,” with one non-blocking diagnostic-shape comment. Continuing this review loop is now producing diminishing returns. chatgpt-review-b9813acc-5526-46… chatgpt-review-b9813acc-5526-46… Path to convergenceDo not keep iterating on this PR just to perfect the diagnostic carrier. The smallest action that would justify KEEP_ITERATING would be: add a structured diagnostic variant or structured field for lowering invariant violations, then update the test to match that carrier instead of probing message text. That would fully pay the residual DB-1/C-5 debt. But that is not necessary for this PR to converge. The acceptable debt to carry is narrow:
The follow-up should dissolve by either adding Meta-verdict⚖️ SHIP_WITH_DEBT — diminishing returns, accept tracked debt, merge and address in follow-up PRs |
…1208) * WIP: loyal-ram-816 * docs(r2): reconcile B-wave Tier 0+2 status in Release Manager brief Mark Tier 0 PRs #820/#817/#821 and Tier 2 B5/B6/B7 as landed at HEAD with synthesis and code pointers. Close the B5 worker brief with a CLOSED banner, checked acceptance boxes, and a closure receipt stanza. Refresh autonomous dispatch wording for closed B-wave scope. Made-with: Cursor * docs(r2): pin Tier 0 merge dates to 2026-04-26 (UTC) in Release Manager brief Clarify program-scope row, standing deliverable #7, and Working state: GitHub merge metadata for #820/#817/#821; distinguish brief reconciliation date from Tier 0 landings; keep B5 Tier-2 closure on 2026-04-27 per synthesis. Made-with: Cursor
Summary
Implements
docs/briefs/b2-lower-fn-body-arrow-rederive-worker.md.Arrowfunction-body lowering fallback insrc/v3/compiler/src/lower.rswith a fail-closedDiagnostic::ResolveError.lower_fn_item_expr_bodywith a non-Arrowconnective and asserts the diagnostic is emitted.Seed-Phase Audit
Audit outcome: no seed-phase gap found. This PR is structural-assertion-only.
Verified paths:
lower_bodies_phasecallsseed_function_signatures_phase(dag, &module.items, symbols, is_first)before the type/data/refinement pre-passes and before the final body-lowering loop.seed_function_signatures_phaseiterates the samemodule.itemsplus the sameis_firstfilter as body lowering.seed_function_signatures_phaseseeds both function-shaped top-level items:SurfaceItem::FnwithArrowBody::PendingandSurfaceItem::FnExternalBodywithArrowBody::Unparsed(body_span).lower_fn_item_expr_bodyis only called from theSurfaceItem::Fnarm inlower_item; duplicateFnitems skipped byis_firstare also skipped by seeding/body lowering.SurfaceItem::FnExternalBodyis seeded but intentionally no-op at body-lowering time, per the existing DB-11 single-construction-authority comment.Conclusion: every reachable
SurfaceItem::Fnbody-lowering path should already have anArrowconnective. A non-Arrowconnective at this site indicates seed phase was skipped or the declaration was clobbered, so the new diagnostic converts the previous dead hedge into a fail-closed contract.Diagnostic Taxonomy Note
The diagnostic currently reuses
Diagnostic::ResolveError. That is intentional for this PR becauseCompilerDiagnosticKindinsrc/v3/std/diagnostics.dagis a closed enum (TokenizerError | ParseError | TypeMismatch | UnitMismatch | ArityMismatch | ResolveError) and has no dedicated lowering-invariant variant.ResolveErroris the closest existing shape; adding a new kind should be handled by a future diagnostic-taxonomy lane with the required coproduct/substrate updates.Verification
git diff --checkpasses.cargo/rustc/rustfmtare unavailable on this worker PATH, so I could not runcargo fmt --all --check,cargo test --workspace --exclude v2-compiler-tests,cargo clippy --all-targets -- -D warnings, or DB-8 fixed-point locally. PR CI should cover the Rust verification once the PR is ready for review.