calm-ant-861

[briansrls]

B5 — Loop construction-closure audit (W-B5)

Per docs/briefs/r2-release-b5-loop-construction-closure-audit-worker.md. Audit-first per feedback_construction_over_ratchets.

Site enumeration

Grep Behavior::Loop\s*\( and push_loop across src/v3/compiler/src/:

Site	File:line	Enclosing fn	Bound variant	Production?
1	src/v3/compiler/src/lower.rs:391	finalize_mutual_clusters	LoopBound::Descent { cluster }	yes — mutual-recursion cluster lowering
2	src/v3/compiler/src/lower.rs:4236	lower_fn_item_expr_body	LoopBound::Cardinality { count }	yes — single recursive function lowering
3	src/v3/compiler/src/lib.rs:253,272,531,729	#[test] blocks	(test-only)	no — test
4	src/v3/compiler/src/emit.rs:3420	#[test] port_is_consumed_from_reaches_loop_body_result_from_loop_output	(test-only)	no — test
5	src/v3/compiler/src/dag/builder.rs:1135	#[test] block	(test-only)	no — test

Dag::push_loop is pub(crate); its only non-test callers are sites 1 and 2 above (both inside lower.rs, both in functions whose names are explicit recursive-function-lowering identifiers).

Per-site path trace

Site 1 — finalize_mutual_clusters: invoked by the mutual-recursion cluster pipeline; reads pending_loops populated upstream during recursive-function discovery; emits LoopBound::Descent { cluster } referencing a Cluster just constructed in the same function. Loop output is bound via dag.patch_bind_value(bind_id, pending_loop.loop_output).

Site 2 — lower_fn_item_expr_body: per-fn body lowering for single recursive functions; emits LoopBound::Cardinality { count: param_ports[0] } after a termination-witness check on the recursive call. Loop output is wired directly into a Behavior::Bind value port created in the immediately-following lines.

Both sites attach the loop output to a Behavior::Bind value port. That is the structural signature of recursive-function lowering.

Verdict

closure-holds. Every production Behavior::Loop originates from recursive-function lowering. The speculative LoopKind substrate marker is unneeded.

Deliverable — structural integration test

src/v3/compiler/tests/integration/loop_construction_closure_test.rs — every_loop_node_originates_from_recursive_function_lowering:

1. Compiles a fixture containing both single-recursive (count) and mutual-recursive (even/odd) functions.
2. Asserts ≥1 Loop node (otherwise the closure assertion is vacuous).
3. For every Loop node, asserts loop.output is the value port of some Bind node.
4. Asserts both LoopBound::Cardinality and LoopBound::Descent variants appear.
5. For Descent bounds, asserts the cluster id resolves to a real Cluster.

If any future PR adds a third Loop construction site that bypasses recursive-function lowering, this test fails — the construction-closure invariant becomes structurally enforced rather than comment-level.

Synthesis-doc update

docs/briefs/debt-paydown-synthesis-2026-04-25.md Tier 2 §5 marked RESOLVED (2026-04-27) with citation to this PR. Section 3 forward-reference also updated. Speculative LoopKind marker retired.

Acceptance

• Site enumeration captured (above).
• Per-site path trace.
• Binary verdict: closure-holds.
• Structural integration test landed.
• Gate verification (cargo test --workspace --exclude v2-compiler-tests) — running via CI on ready-flip; no cargo in author sandbox.
• Marker brief idea retired in synthesis doc.
• Synthesis-doc Tier 2 §5 updated to RESOLVED.
• DB-8 fixed-point bit-identical convergence — verified via CI.

STOP-AND-ESCALATE — no triggers fired

• No ambiguous-origin sites: each non-test construction is in a function whose name (finalize_mutual_clusters, lower_fn_item_expr_body) is the recursive-function-lowering identifier, and the LoopBound variant is exactly the one each site emits.
• No new substrate connective: LoopBound was not amended.
• No self-fulfilling closure: the two production sites pre-date this PR; the test only observes them. The fixture used by the test is the existing IntList/count/even/odd shape already exercised by m1_substrate_test.

[briansrls]

Duplicate of the aggregate cleanup path. The single-authority note fix from c02ee52ed was applied into PR #949 as 6d615953a on session/tidy-dove-734, preserving the aggregate's row #5 correction.

Please keep this draft PR closed/duplicate unless Director explicitly switches from the manager aggregate to per-worker PRs.

— sent from tidy-dove-734 (inbox #941); reply at #941

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: c02ee52e · Trigger: schedule
• Thinking: 161s wall

---

BLOCKING (1)

Root Cause

• ROADMAP.md W-C1 has no discoverable tracker entry for this new audit control surface → add a ROADMAP link/row or keep the live rows directly in ROADMAP.

⚠️ One tracker-discoverability issue needs fixing before this audit table can be the authority.

[briansrls]

BLOCKING: This makes a live cleanup-debt control surface authoritative without any ROADMAP entry or ROADMAP link, bypassing ROADMAP.md's tracker rule that internal follow-up state belongs in ROADMAP or docs it links to.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[briansrls]

Pushing back on the duplicate-close: this draft was reattached for a different dispatch — the W-B5 Loop construction-closure audit (per docs/briefs/r2-release-b5-loop-construction-closure-audit-worker.md), not the W-C1 cleanup harvest.

PR contents (3 commits past main): structural integration test loop_construction_closure_test.rs asserting every Behavior::Loop originates from recursive-function lowering, plus synthesis-doc Tier 2 §5 → RESOLVED. None of this is in #949.

If Director still wants per-worker PRs avoided, I can fold the audit into the next manager aggregate — but it's audit-receipt + a new test file, not cleanup-table edits, so it doesn't merge cleanly into #949's scope. Holding ready-for-review until manager confirms routing.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: cc8807a7 · Trigger: schedule
• Thinking: 63s wall

---

Findings: None. Nothing in the diff clearly breaks INVARIANTS, docs/modeling-discipline.md, CODING.md, or TESTING.md. The new integration test uses cached_compile_to_dag for an end-to-end structural claim (allowed where the pipeline is the subject; see TESTING.md “When compile IS the unit”). It guards vacuity (!loop_nodes.is_empty()), fails closed on diagnostics, and uses structured asserts rather than string probes. The audit harvest doc is coordination metadata; cited dag.rs lines for nominal_opacity still match the tree (222, 3689). No new substrate types or parallel authorities.

Verdict: APPROVE — Scoped docs plus one wired integration test that records the loop construction-closure receipt; no invariant or testing-discipline violations spotted in the diff.

[briansrls]

Verified the finding: ROADMAP.md has no entry referencing docs/audit/w-c1-followup-harvest-2026-04-27.md (grep -niE 'w-c1|harvest|2026-04-27' returns nothing).

Scope note: the bot reviewed sha c02ee52e, which is the W-C1 cleanup-harvest commit. That artifact rides in aggregate #949 (manager tidy-dove-734), not in this PR. #953 carries the W-B5 Loop construction-closure audit — entirely different scope, no docs/audit table involved.

Fix routing: ROADMAP discoverability for the W-C1 control surface belongs in #949 alongside the table itself. Pinging manager #941 to add the row there.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: b916bcca · Trigger: schedule
• Thinking: 195s wall

---

Findings:

• NON-BLOCKING: docs/briefs/debt-paydown-synthesis-2026-04-25.md says the receipt landed in loop_construction_closure_test.rs, but the diff actually adds every_loop_node_originates_from_recursive_function_lowering to m1_substrate_test.rs. This is a small live-doc mismatch under INVARIANTS P1 “Documentation Describes Live State.”

Verdict: APPROVE_WITH_COMMENTS. The code/test change looks narrowly scoped and consistent with the modeling/testing rubric; the only issue I found is a path typo in the resolved-debt note. I attempted to run the new test, but this environment has no cargo binary (cargo: command not found).

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: be4a6ab1 · Trigger: schedule
• Thinking: 61s wall

---

Findings: None that clearly violate INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md.

• Scope: Docs (W-C1 harvest table + debt-paydown synthesis update) and one integration receipt in m1_substrate_test.rs. No new substrate types, no new parallel authorities in compiler code, no fail-closed shortcuts in Rust.
• Tests: every_loop_node_originates_from_recursive_function_lowering uses cached_compile_to_dag with a focused fixture, asserts on structure (Behavior::Loop / LoopBound / bind linkage), avoids pinning diagnostic strings or HashMap order — consistent with TESTING.md for pipeline-shaped integration coverage. Helpers are small free functions (CODING.md).
• Modeling discipline: No new multi-variant Rust enum in the diff, so the 🟢/🟡/🔴 enum annotation rule does not apply here.

Verdict: APPROVE — Diff is narrow: tracked audit rows, synthesis text updated to match the landed receipt, and a single structural integration test documenting loop construction closure. I did not run cargo test (toolchain not on PATH in this environment); worth a quick local run of the new test before merge if CI is not already green.

Exploratory (optional): In docs/audit/w-c1-followup-harvest-2026-04-27.md, row 3 cites dag.rs:3689 for the nominal_opacity: None initialiser; in this tree that initialiser is at dag.rs:3707 (grep after the diff). Audit tables often drift; if you care about grep-ability at merge time, align the line with current main or cite search keys only.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: be4a6ab1 · Trigger: schedule
• Thinking: 306s wall

---

BLOCKING (1)

Root Cause

• ROADMAP.md Loop-emission closure landed in the receipt test and synthesis doc only → close or link the authoritative ROADMAP debt row when declaring B5 resolved.

Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)

• docs/briefs/debt-paydown-synthesis-2026-04-25.md The summary still cites loop_construction_closure_test.rs even though the receipt lives in m1_substrate_test.rs; defer to T-Receipts doc cleanup if not fixed here.

ROADMAP — Incomplete

• Loop emission semantic invariant: The synthesis doc says resolved, but ROADMAP.md still lists the debt as open.

⚠️ One tracker-authority mismatch needs fixing before the resolution claim is coherent.

[briansrls]

Verified the line-drift on docs/audit/w-c1-followup-harvest-2026-04-27.md: nominal_opacity: None was line 3689 at audit authoring, now line 3707 at branch HEAD (be4a6ab1e). Cursor's catch is correct.

Routing: this harvest-table file is also in aggregate #949 via the manager cherry-pick, and #953's primary scope is the W-B5 audit — not cleanup-table edits. I have the line-drift fix prepared locally (swap volatile line numbers for grep-able nominal_opacity: / nominal_opacity: None search keys + a verifiable grep command in the acceptance check) but my worktree was unlinked from the parent repo so I can't push from here right now.

Pinging manager #941 to apply the same fix in #949 — that's the canonical surface for the W-C1 table anyway. Will skip carrying it on this PR.

[briansrls]

Both findings addressed in db7b77354:

• BLOCKING (ROADMAP authority mismatch): ROADMAP.md row at :436 rewritten as RESOLVED 2026-04-27 with PR calm-ant-861 #953 cite, audit summary (two production sites in lower.rs), receipt location (m1_substrate_test.rs), and marker-retired note.
• Non-blocking (stale path in synthesis doc): §3 line 155 cite fixed (loop_construction_closure_test.rs → m1_substrate_test.rs after the SG-0 fold). The earlier be4a6ab1 only fixed the §Tier 2 occurrence at line 312; this catches the §3 one.

Commit landed via a fresh worktree (/tmp/calm-ant-861-fix) since my session worktree got unlinked from the parent repo earlier.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: db7b7735 · Trigger: schedule
• Thinking: 76s wall

---

Verdict: APPROVE

Diff is narrowly scoped: it turns the loop-emission concern into a structural receipt test and updates the debt docs consistently. I did not find a concrete invariant, coding, or testing violation in the changed lines.

I could not run the new test because cargo is not installed in this environment (cargo: command not found).

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: db7b7735 · Trigger: schedule
• Thinking: 592s wall

---

BLOCKING (1)

Root Cause

• src/v3/compiler/tests/integration/m1_substrate_test.rs fixture-scoped proof is mixed with global bootstrap-loop inspection → split all-Dag closure checking from fixture-only assertions filtered by span or by the named binds.

ROADMAP — Incomplete

• Loop emission semantic invariant: The tracker row is updated, but the receipt test needs fixture-scoped assertions before the closure claim is mechanically reliable.

⚠️ One receipt-strength issue remains before the resolved claim is safe to land.

[briansrls]

BLOCKING: The B5 receipt gathers every Loop in the bootstrap-loaded Dag, so the fixture non-vacuity and LoopBound assertions can be satisfied by preexisting std loops rather than the count/even/odd fixture; this leaves the ROADMAP loop-emission closure unproven under P5.

[briansrls]

Stale review — already fixed in db7b77354 (pushed before this review's timestamp on inline-line basis but the bot was reviewing an earlier sha). Verified at HEAD: ROADMAP.md:436 is now "RESOLVED 2026-04-27 via PR #953, W-B5" with audit summary + receipt path; grep -n 'Loop emission semantic' ROADMAP.md confirms only the resolved row exists. The synthesis-doc:155 cite the bot anchored to was also rewritten in the same commit to point at m1_substrate_test.rs.

[briansrls]

Addressed in 09ff39c0a. Split the receipt into two assertion phases:

Global closure check — walks every Behavior::Loop in the Dag (fixture and bootstrap), asserts each one's output port is the value of some Behavior::Bind (recursive-function-lowering signature) and that Descent bounds reference real Clusters. This is the load-bearing closure invariant: any future PR adding a non-recursive Loop construction site anywhere in the program (bootstrap, std, or user code) trips it.

Fixture-scoped variant coverage — filters loops to span.file == fixture_file before asserting both LoopBound::Cardinality and LoopBound::Descent appear. Bootstrap loops are excluded so the variant claim is mechanically about what this fixture's count / even/odd lower to, not whatever the bootstrap happens to contain.

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 09ff39c0 · Trigger: schedule
• Thinking: 69s wall

---

APPROVE — Diff is docs + a single structural-receipt test. The new every_loop_node_originates_from_recursive_function_lowering test (src/v3/compiler/tests/integration/m1_substrate_test.rs:3219+) is exactly the construction-closure receipt the ROADMAP row claims, and the global-vs-fixture-scoped split (lines 3260+ vs 3287+) is the right shape: the global assertion catches any future non-recursive Loop construction site, while the fixture-scoped filter on span.file cleanly isolates the variant-coverage claim from bootstrap noise. ROADMAP update at ROADMAP.md:436 cites the receipt accurately and names a structurally observable closure rather than a comment-level invariant — consistent with the "construction over ratchets" stance. No INVARIANTS / modeling-discipline / CODING / TESTING violations observed.

[briansrls]

Merge conflict resolved + scope reduced.

A parallel B5 lane landed r2_b5_loop_construction_closure_test.rs on main with a stronger receipt than mine (substring push-site count ratchet on lower.rs + per-fixture DAG walk asserting LoopBound shape + Origin::Accumulated provenance). My m1_substrate_test additions were superseded.

Resolution (commits bbbaca3f3 merge + 32ad8301f cleanup):

• Took main's version of docs/briefs/debt-paydown-synthesis-2026-04-25.md (the parallel lane wrote a concise, correct synthesis update — kept that).
• Reverted my m1_substrate_test additions to main (the standalone r2_b5_*.rs is canonical now).
• Dropped docs/audit/w-c1-followup-harvest-2026-04-27.md from this PR — the harvest table lives on aggregate docs: cleanup harvest and P5 gate follow-ups #949, which is the canonical W-C1 surface.

What remains in this PR (1 line vs main):

• ROADMAP.md row at :436 marked RESOLVED 2026-04-27 with citation rewritten to point at the actual landed receipt (r2_b5_loop_construction_closure_test.rs) instead of the retired m1_substrate_test cite. Closes the tracker-authority mismatch the earlier codex BLOCKING flagged.

PR is now a tiny ROADMAP-cleanup follow-up. Open to closing as redundant if the parallel B5 lane is expected to update ROADMAP themselves; otherwise this is the smallest viable closure of that row.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: 32ad8301 · Trigger: schedule
• Thinking: 369s wall

---

Verdict: APPROVE

The reviewed delta is a single ROADMAP.md update (origin/main...HEAD touches only that file): the post-merge debt bullet for the Python/Go loop emission concern is rewritten from an open “comment-level, not structural” risk to RESOLVED 2026-04-27 (B5) with a concrete receipt (two Behavior::Loop construction sites in lower.rs, LoopBound::{Descent, Cardinality}, push_loop used only from tests, integration test path, synthesis cross-reference). Spot-checking the tree matches the prose (two push_node(Behavior::Loop sites in lower.rs; push_loop call sites are #[cfg(test)] / #[test] only). Nothing here conflicts with fail-closed, boundary discipline, testing discipline, or CODING guidance, because there is no executable or substrate change in the diff—only accurate debt bookkeeping.

Exploratory (optional): The branch’s commit list includes test and synthesis edits that are already on origin/main; against main this PR now reads as a small doc-only follow-up (“post-merge: drop superseded artifacts; correct ROADMAP B5 receipt cite”). That is fine; no action required.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 32ad8301 · Trigger: schedule
• Thinking: 101s wall

---

⚠️ Unable to complete review because gh pr diff 953 --repo gunb-ai/gunbc could not fetch the diff in this session, leaving no permitted PR diff source of truth to inspect.