rev

[navicore]

No description provided.

[claude]

Code Review — PR #404: Multi-Capture Order Fix

Overview

This PR fixes a correctness bug where closure captures with 2+ values arrived in the closure body in reversed caller order. The root cause was that patch_seq_push_closure stored captured values top-down into the env vector, while the typechecker stored types bottom-to-top — creating an index mismatch that would crash on any mixed-type (heterogeneous) multi-capture.

The fix is a single .reverse() call after the pop loop, making runtime env layout match the typechecker's convention and preserving the caller's visual stack order inside the closure body.

---

Code Quality

Strengths:

• Fix is minimal and precise — one line of runtime code, no codegen or typechecker changes needed.
• All call sites (strand.spawn, list.fold, list.map, etc.) get the fix simultaneously since they all route through patch_seq_push_closure.
• capture_analysis.rs doc comment is rewritten correctly and consistently with the new convention.
• Unit test in closures.rs is updated to assert the new env[0]=deepest, env[N-1]=shallowest layout.
• sss.seq simplification is correct — the compensating swap disappears and the body becomes the direct call.
• MIGRATION_5.5.md is clear and has a useful before/after example.

---

Issues

1. Module comment inconsistency (closures.rs, lines 22–23)

The module-level doc still lists Variant as a type "to be added in future PR":

//! - Variant (tagged unions)

But test-fold-capture-int-and-variant introduces a test that captures a list (Variant) and expects it to work correctly after this fix. If Variant now works via the generic patch_seq_env_get path, the comment should be updated to reflect that. This is the most visible documentation inconsistency in the PR.

2. Missing 3-capture ordering test

CAPTURE_ORDER_FIX.md (Checkpoint 2) describes a planned "3-capture fold body where each capture is a distinct int" test. Only 2-capture tests are included. A 3-capture test would provide stronger confidence that the ordering is correct for N > 2, and would directly exercise rot-based capture setups common in the codebase.

3. test-fold-capture-int-and-variant test body is hard to follow

The >aux/aux> double-dup pattern is correct but dense. The line:

aux> dup >aux   # ( acc elem list_cap int_cap )

achieves "peek into aux without consuming it," but this idiom isn't obvious. A brief inline comment like # peek: restore int_cap, keep a copy in aux for later would help future readers.

4. PR title and description

The commit message and PR title are both just "rev". Given that this is a documented breaking change (5.4 → 5.5), a more descriptive title and PR body would help with git log and release note generation. Suggest something like: fix(closures): preserve caller stack order for multi-value captures (breaking, 5.5).

5. CAPTURE_ORDER_FIX.md references internal memory files

Checkpoint 7 mentions memory/feedback_capture_order.md, which refers to Claude's internal memory system rather than any project artifact. This is harmless but will confuse contributors reading the design doc. Consider removing or replacing with something project-visible.

---

Test Coverage

• New test-fold-capture-two-ints: correctly position-encodes cap_below*10 + cap_top (= 12 per iter, 36 total). The encoding distinguishes correct order (36) from reversed (63) — well-designed.
• New test-fold-capture-int-and-variant: exercises the mixed-type path that would have crashed before the fix. Correct result (600 = 200 × 3 iters).
• Existing sss.seq / test-std-loops.seq / test-auto-capture.seq single-capture tests serve as regressions.
• Gap: no explicit 3+ capture test.

---

Summary

The fix is correct and well-reasoned. The main asks before merge:

1. Update the module-level comment in closures.rs that says Variant isn't supported yet as a capture type (since this PR proves it is).
2. Consider adding a 3-capture ordering test to match the design doc's checkpoint.
3. Improve the PR/commit title to reflect the breaking-change nature.

The code change itself (captures.reverse()) is approved — minimal, correct, and the updated tests confirm the expected behavior.

[claude]

Code Review — PR #404: Multi-capture closure ordering fix

Overview

This PR fixes a two-part bug in multi-value closure capture ordering:

1. Runtime mismatch: patch_seq_push_closure popped values top-down and stored them as-is, so env[0] held the shallowest (top) capture. Codegen then pushed env[0..N-1] in order, leaving the body stack reversed from the caller.
2. Latent type/value index mismatch: The typechecker already produced captures bottom-to-top (captures[0] = deepest type), while the runtime stored them top-down. For homogeneous multi-captures this was harmless; for mixed types (Int + Variant) it would have caused env_get_int to be called on a Variant — a silent crash waiting to happen.

Fix: One line — captures.reverse() in patch_seq_push_closure after the pop loop — makes the runtime match the typechecker convention. The codegen and typechecker are untouched.

---

Strengths

• Minimal blast radius: One line of runtime change fixes both bugs simultaneously. Codegen and typechecker need no changes.
• Excellent documentation: MIGRATION_5.5.md and docs/design/CAPTURE_ORDER_FIX.md clearly explain the "why," the two-bug nature of the problem, and migration steps. The design doc's analysis of all three layers (runtime, codegen, typechecker) is particularly valuable.
• Strong test suite: Three new integration tests specifically designed to catch ordering regressions:
  • Two-capture (2-int): validates basic ordering
  • Three-capture (3-int): position-encodes as cap_a*100 + cap_b*10 + cap_c, so any permutation other than the correct one produces a different 3-digit number — not just a full reversal
  • Heterogeneous (List + Int): directly exercises the latent crash scenario
• sss.seq simplification: Removing the compensating swap is a good real-world validation that the fix works end-to-end.
• Updated test assertions in closures.rs have clear comments explaining why env[0] is now the deepest capture.

---

Issues and Suggestions

Minor: test-fold-capture-int-and-variant body is hard to follow

>aux                                # stash int_cap
aux> dup >aux                       # peek: restore int_cap, keep copy in aux
list.get drop                       # list_cap[int_cap] = 200

The >aux / aux> dup >aux sequence is non-obvious. The intent seems to be: use int_cap as an index for list.get while also "preserving" it via aux. However, since closure captures are re-pushed from env on every fold iteration (they're not consumed from the env), the aux preservation at the end (aux> drop) appears to be dead code — the captured values come fresh on the next iteration regardless.

More importantly, the drop after list.get is suspicious. Does list.get leave an extra value on the stack? If it returns a Maybe/optional, that would need unwrapping, not dropping. Please add a comment explaining what list.get returns and why drop is needed, or simplify if the aux dance is truly unnecessary.

Minor: No explicit sss.seq end-to-end test

The design doc lists "Shamir rewrite: split-bytes ... output matches SUCCESS — secret recovered!" as checkpoint 5. The sss.seq change looks correct, but there's no test file or CI step that runs the example and checks its output. If the project has a way to run integration tests against example projects, covering this would strengthen the PR.

Nit: PR title

rev is not searchable in git history or GitHub. Consider something like fix: reverse capture env order to match typechecker (breaking, 5.4→5.5).

---

Correctness Verification

The core arithmetic in the new tests checks out:

• test-fold-capture-two-ints: cap_below*10 + cap_top = 1*10 + 2 = 12 × 3 iterations = 36 ✓
• test-fold-capture-three-ints: cap_a*100 + cap_b*10 + cap_c = 100 + 20 + 3 = 123 × 4 iterations = 492 ✓
• test-fold-capture-int-and-variant: list[1] = 200 × 3 iterations = 600 ✓ (assuming list.get is 0-indexed and the drop is intentional)

The captures.reverse() placement — after the pop loop, before closure construction — is correct. Single-capture closures are unaffected as noted (reversing a 1-element Vec is a no-op).

---

Summary

This is a clean, well-justified breaking fix. The documentation quality is high and the test coverage directly targets the failure modes described. The two issues above are minor — primarily a readability concern in one test body and the absence of an sss.seq smoke test. The core change is correct.

Verdict: Approve with the suggestion to clarify list.get drop in the heterogeneous test.