[Task 55] Phase 4b: args-buffer packing on perform side

[boldfield]

Summary

Plan B Task 55, Phase 4b: lifts the zero-arg-op restriction from Phase 3b/4a. Non-IO perform Effect.op(args...) sites now pack user args into a stack-allocated [u64; N] buffer that the runtime copies into the dispatched NextStep::Call's slots before invoking the arm fn.

Builds incrementally on PR #22 (Phases 2 → 4a). Each Phase 4 sub-restriction lifts on its own focused PR per the established Task 55 cadence.

What's in scope

• Args buffer packing in lower_perform_non_io_to_value (compiler/src/codegen.rs):
  • Stack-allocated [u64; N] slot via FunctionBuilder::create_sized_stack_slot, 8-byte aligned.
  • Per-arg widening: I8 (Bool/Byte/Unit) and I32 (Char) zero-extend via uextend; I64 (Int) and pointer_ty (String, user-type pointers) store directly.
  • Defensive compile-time assert! mirrors the runtime's args_len + 2 > MAX_INLINE_ARGS check.
• MAX_INLINE_ARGS consolidated into sigil_abi::effect (moved from sigil_runtime::handlers); the runtime pub use-re-exports it for backwards-compat. Closes the Task 56 MAX_INLINE_ARGS deviation entry.
• Walker (unsupported_handle_construct) drops two gates:
  • arm.params.is_empty() rejection — gone.
  • body_contains_non_io_perform_with_args traversal — gone, plus the now-dead helpers body_contains_non_io_perform_filtered / block_contains_non_io_perform_filtered removed.
• Three new e2e tests:
  • handle_with_int_arg_op_packs_args_buffer — single Int arg.
  • handle_with_three_int_args_packs_buffer — multi-arg packing.
  • handle_with_mixed_type_args_widens_correctly — widening: Int + Bool + String.

What's NOT in scope

• Arm-side arg reads — Phase 4c. Arm bodies remain Expr::IntLit only; the synthetic arm fn ignores args_ptr. The FFI plumbing now carries packed args end-to-end so Phase 4c only needs to wire arm-side reads.
• k-using arms (Phase 4d), multi-effect handles (Phase 4e), return arms (Phase 4f).

Documentation

• New [DEVIATION Task 55] Phase 4b — args-buffer packing on perform side; stack-slot allocation; arm side still IntLit-only entry in PLAN_B_DEVIATIONS.md covering the stack-slot rationale (vs. arena), per-arg widening discipline, and bound-check policy.
• Phase 3b restrictions entry's closure-point list updated: Zero-arg ops moves to closed.
• Walker-doesn't-follow-call-edges entry: op-arg gate marked lifted at HEAD.
• Task 56 MAX_INLINE_ARGS entry: closure point closed.
• PLAN_B_PROGRESS.md Task 55 entry extended with Phase 4b notes + commit reference.

Test plan

• cargo check --workspace clean
• cargo fmt --all -- --check clean
• cargo clippy -p sigil-runtime --all-targets -- -D warnings clean
• cargo clippy -p sigil-compiler --all-targets -- -D warnings clean
• cargo test -p sigil-runtime --lib passes (re-export of MAX_INLINE_ARGS doesn't break runtime tests)
• scripts/check-no-interior-pointers.sh clean
• Pod-verify green
• CI green on x86_64-unknown-linux-gnu (ubuntu-24.04) — full e2e suite incl. the 3 new tests
• CI green on aarch64-apple-darwin (macos-14) — same
• scripts/reproducibility.sh byte-stable (no test runs the new Phase 4b shape via examples — examples/handle_*.sigil only land in Stage 6's Task 59 set)
• scripts/smoke.sh unaffected (Phase 4b touches no example file)

[boldfield]

Review of PR #23 (Plan B Task 55 — Phase 4b: args-buffer packing on perform side)

Small, focused PR that lifts exactly the zero-arg restriction. The stack-slot strategy is sound (lifetime trivially outlives sigil_perform since the runtime copies args into the dispatched NextStep::Call slots before returning), the per-arg widening discipline matches the runtime's u64-stride read in runtime/src/handlers.rs::sigil_perform, the MAX_INLINE_ARGS consolidation into sigil_abi::effect correctly removes a cross-crate drift hazard, and the walker simplification (~115 LOC of dead body_contains_non_io_perform_filtered helpers gone) is a nice cleanup.

CI green on both targets. A couple of items worth addressing before merge.

Substantive

1. Test coverage gap: the new e2e tests verify only "program doesn't crash", not "args arrive correctly".

All three new tests (handle_with_int_arg_op_packs_args_buffer, handle_with_three_int_args_packs_buffer, handle_with_mixed_type_args_widens_correctly) declare arm bodies as IntLit (per the still-in-force Phase 4b restriction) and the arms ignore args_ptr. The arm body returns its literal regardless of what's in the args buffer. The tests catch:

• Cranelift verifier failures (e.g., stack_store I8 mismatch) at compile time
• Stack-slot overflow / misalignment faults at runtime
• sigil_perform's args_len + 2 > MAX_INLINE_ARGS overflow trip

They do not catch:

• Wrong widening direction (uextend vs sextend — Bool/Char are unsigned-by-convention so uextend is correct, but a future signed Byte type would silently miscompile)
• Off-by-one slot offsets (the runtime would copy garbage into the NextStep::Call slots; arm doesn't read so no crash)
• Wrong endianness on the slot store (same — invisible to arm)
• Off-by-one args_len (would read garbage past the slot, or skip a real arg — both invisible)

The test docstring claims to "pin the buffer-packing path doesn't off-by-one or misalign across arg count > 1", but with no arm-side reads there's no observable signal for off-by-one. The tests effectively validate the FFI plumbing's overflow boundary, not the args' contents.

This is acknowledged in the test docstrings ("Phase 4c will read the bound names"), but Phase 4c is a separate PR. Until then, a misaligned-store or wrong-widening regression that doesn't trip Cranelift's verifier would land green. Two cheap mitigations:

• (a) Defer real validation to Phase 4c — explicitly note in the PR's test plan that args-content correctness is unvalidated until then. Acknowledged limitation, not a fix, but makes the gap visible to future bisectors.
• (b) Add a runtime debug helper (e.g., a #[cfg(test)] sigil_perform variant or a thread-local args-trace ring buffer the test harness can inspect) so Phase 4b's args content is observable without arm-side reads. More invasive but closes the gap now.

I'd take (a) at minimum; (b) only if the team wants real Phase 4b coverage rather than waiting on Phase 4c.

2. Doc/code drift: deviations entry says debug_assert, code is assert!.

PLAN_B_DEVIATIONS.md (Task 56 MAX_INLINE_ARGS entry, updated this PR) says:

a defensive debug_assert at the perform-site catches any operation that exceeds MAX_INLINE_ARGS - 2 user args before the runtime's matching check fires.

The actual code at compiler/src/codegen.rs::lower_perform_non_io_to_value (assert!, not debug_assert!) panics the compiler in release builds when an operation exceeds the cap. The PR description correctly says "compile-time assert!"; the deviations entry has it wrong.

Pick one. Recommendation: change the code to debug_assert! and let the runtime's sigil_perform overflow check be the source of truth (already there at runtime/src/handlers.rs:588). The compiler-side check is defense-in-depth; firing it as a hard panic gives users a worse error message than the runtime's named guard.

Minor

3. The else widening branch silently miscompiles future floats in release builds.

lower_perform_non_io_to_value widening:

if arg_ty == types::I64 { arg_v }
else if arg_ty.is_int() && arg_ty.bits() < 64 { uextend(I64, arg_v) }
else {
    debug_assert_eq!(arg_ty, self.pointer_ty, ...);
    arg_v
}

debug_assert_eq! is compiled out in release. If Sigil ever gains an F32/F64 surface type, the float would fall through the else branch and store its bit-pattern as if it were a pointer-sized value — silently wrong for F32 (upper 32 bits garbage), bit-identical-but-typed-wrong for F64. The neighboring comment notes the future-32-bit-target case but not the future-floats case.

Two options: change debug_assert_eq! to assert! so a future-floats regression panics in release too, OR add an explicit else if arg_ty == types::F32 || arg_ty == types::F64 arm with a panic!("Phase 4b doesn't support float perform args; closure point: ...") and a TODO. Either is fine.

4. (p.args.len() * 8) as u32 could overflow in principle, but the (p.args.len() as u32).saturating_add(2) <= MAX_INLINE_ARGS check above already bounds args.len() at 30, so this is dead defensiveness. Skip.

---

Item #1 is the only one I'd block on if I were merging. #2 is a 30-second fix. #3 is informational; today Sigil has no floats so the wrong-widening branch is unreachable.

[boldfield]

Review at dea0106

Focused, well-scoped Phase 4b PR. Args-buffer packing is implemented cleanly: stack-allocated [u64; N] slot, per-arg uextend for narrower types, defensive compile-time bound check mirroring the runtime-side check, MAX_INLINE_ARGS unified into sigil_abi::effect. Walker simplification drops ~120 lines of dead helpers. Mechanical state clean (4 lanes CI green, 421 lib tests pass locally, runtime 64+1, pod-verify OK).

Three issues to address before merge per your standing directive on this branch.

Per-task

Task	Status
55 Phase 4b — args-buffer packing on perform side; arm side still IntLit-only	ship after fixes

Mechanical (verified locally)

• cargo test -p sigil-compiler --lib: 421 passed
• cargo test -p sigil-runtime --lib: 64 passed, 1 ignored
• bash scripts/pod-verify.sh: OK
• MAX_INLINE_ARGS unified at abi/src/effect.rs:48; runtime/src/handlers.rs correctly uses pub use sigil_abi::effect::MAX_INLINE_ARGS for backwards-compat. Two new abi unit tests (next_step_tags_are_distinct_and_pinned, max_inline_args_pinned_at_32) pin the constants.
• runtime/src/handlers.rs test for the 1 + MAX_INLINE_ARGS + 1 overflow path (mentioned in the runtime entry's "auditing both sides" note) — not regressed; runtime bound check still in place.
• All 4 CI lanes SUCCESS.

Must-fix before merge

[MF1] Args-value verification gap. The three new e2e tests confirm the FFI plumbing compiles + runs without crashing, but none verifies arg values are correctly transmitted from perform to runtime. Specifically:

• handle_with_int_arg_op_packs_args_buffer passes 99 and ignores it; arm returns 0.
• handle_with_three_int_args_packs_buffer passes (10, 20, 30) and ignores them; arm returns 7.
• handle_with_mixed_type_args_widens_correctly passes (42, true, "hi") and ignores them; arm returns 11.

The test comment for the third even acknowledges this directly: "Without this test, the widen branch sits dead until Phase 4c ships an arm body that reads the bound name." The widen branch IS exercised by the codegen path, but the test only proves "no Cranelift verifier failure + no runtime crash" — it does NOT prove the widened value matches the source value. A signed-vs-zero-extension mistake (e.g., on Char which is I32 — currently uextend, correct because chars are unsigned codepoints, but the test doesn't pin this) would silently produce wrong values that surface only when Phase 4c lands arm-side arg reads, by which point the bug has been latent across an inter-PR boundary.

Add ONE of:

1. Runtime-side #[cfg(test)] verification path — a test that calls sigil_perform directly with known args, reads them back from the dispatched NextStep::Call's args slots, and asserts equality. Bypasses the codegen path but verifies the Phase 4b runtime contract independently.
2. Codegen-side direct-FFI test — a small Rust test that lowers a synthetic Expr::Perform with known arg values, then reads back the stack slot (or hooks sigil_perform to capture the buffer pointer + dump contents).
3. Defer arg-verification e2e tests as a Phase 4c must-fix precondition in the Phase 4b deviation entry — explicit pre-registered acceptance criterion that Phase 4c's PR description must include arg-verification tests covering Int/Bool/String/Char widening.

Option (3) is the cheapest. (1) is the most defense-in-depth. Pick one. Don't ship the widening logic with literally zero value-correctness coverage.

[MF2] Stack-slot lifetime is implicitly tied to the synchronous call pattern; not explicitly documented. lower_perform_non_io_to_value allocates the args buffer on the caller's stack via create_sized_stack_slot (codegen.rs:1957). This works today because:

• sigil_perform is invoked synchronously from native code,
• sigil_run_loop is invoked synchronously after,
• Both return before the caller fn returns,
• The runtime copies args into the dispatched NextStep::Call's arena slots before returning,

so the stack slot is valid throughout the runtime call chain. But Phase 4d (per the PB4 deviation entry: "Native callers drive sigil_run_loop synchronously; tail-call discipline lifts in Phase 4d") converts perform sites to return NextStep::Call to the caller's trampoline rather than synchronously calling run_loop from native code. At that point, the args buffer must outlive the perform site's return — the stack-slot strategy becomes wrong (the buffer dies when the cps_fn returns, but the runtime needs to read it during the trampoline's next dispatch).

The null_k_closure / null_k_fn PHASE-4-RESTRICTION markers correctly call out the k-usage gate but don't mention this stack-slot implication. Either:

1. Add a // PHASE-4-RESTRICTION (Plan B Task 55, Phase 4d): stack-slot lifetime relies on synchronous sigil_perform → sigil_run_loop call pattern; Phase 4d migrates args buffer to arena allocation comment on the create_sized_stack_slot line (codegen.rs:1957), so the dependency is grep-findable.
2. Add a paragraph to the new Phase 4b deviation entry under "Closure point" naming Phase 4d as the trigger for migrating args from stack to arena.

Either works. Prefer (1) — markers next to the affected code are easier to find than deviation-entry cross-references.

[MF3] PR #22's "single-branch" deviation entry is now stale. PR #22's foundation-phase deviation entry (PLAN_B_DEVIATIONS.md first Task 55 entry) says: "Task 55 lands across multiple commits on a single branch (plan-b-task-55) rather than as one monolithic commit ... PR opens only when the full Task 55 path actually compiles and runs at least one real handler example end-to-end."

PR #22 merged at 2f56e87. PR #23 (this PR) is on a SEPARATE branch (plan-b-task-55-phase-4b) with its own PR. The cadence pivoted from "single PR for Task 55, foundation through full handler" to "one PR per Phase 4 sub-task, all on separate branches against main." This is arguably the better cadence (smaller reviewable PRs), but the existing deviation entry's claim is now factually wrong.

Update the entry's closure point or add a follow-up entry explaining the pivot. The deviation history needs to track what actually happened, not what was originally planned — otherwise future readers (or future agents resuming work) will be confused about the cadence.

Deferred (genuine future-task work)

• Arm-side arg reads (Phase 4c) — the matching consumer of the buffer this PR packs.
• k-using arms (Phase 4d) — also flips the synchronous → async perform calling convention, addresses MF2's underlying concern.
• Multi-effect handles (Phase 4e), return arms (Phase 4f).

Spec adherence

The new [DEVIATION Task 55] Phase 4b entry follows the established 4-section structure and includes a closure-point list naming Phase 4c/4d/4e/4f for each remaining restriction. Quality matches prior Task 55 entries. Prior closure points correctly closed:

• Phase 3b restrictions: Zero-arg ops → closed at HEAD ✓
• Walker entry: op-arg gate → lifted at HEAD ✓
• Task 56 MAX_INLINE_ARGS entry: closed at HEAD ✓

Regressions

None.

---

Once MF1, MF2, MF3 land, ready to merge.

[boldfield]

Review-fixup at 7ce8871

All 5 items addressed in one fixup commit. Per-item summary:

MF1 — args-content verification gap

Per your option (3) — cheapest of the three offered. Phase 4c's PR is now a pre-registered acceptance precondition to ship 4 named arg-content readback tests:

1. Int arg readback — pins source value reaches arm.
2. Bool / Char arg readback — exercises uextend widening.
3. String arg readback — exercises pointer-store path.
4. Multi-arg readback in declared order — (a, b, c, k) => b returning the middle of (10, 20, 30) pins offset arithmetic matches runtime's args_ptr.add(i) u64-stride read.

Recorded as the trailing "Acceptance precondition for Phase 4c — args-content verification (PR #23 review MF1)" section in the new Phase 4b deviation entry. A bisecting agent investigating wrong-args-value bugs after Phase 4c lands gets a pointer back to Phase 4b ([HEAD]) as the suspect.

Did NOT take option (1) (runtime #[cfg(test)] verification path) because Phase 4c lands the same coverage end-to-end through the real path; adding a parallel #[cfg(test)] shim would duplicate work and create a separate API surface to maintain. Did NOT take option (2) (codegen-side direct-FFI test reading back the stack slot) because it's invasive — the Lowerer doesn't currently have a test surface for synthetic Expr::Perform input.

MF2 — stack-slot lifetime tied to synchronous call pattern

Per your preferred option (1) — marker next to affected code. New // PHASE-4-RESTRICTION (Plan B Task 55, Phase 4d): comment on the create_sized_stack_slot line in lower_perform_non_io_to_value. Notes the Phase 4d migration to arena allocation when perform sites convert to returning NextStep::Call to the caller's trampoline; cross-references the existing Native callers drive sigil_run_loop synchronously deviation entry. The new Phase 4b deviation entry's Closure point also calls this out, but the inline marker is the grep-findable one you wanted.

MF3 — PR #22 single-branch deviation entry is now stale

Added a "Cadence pivot (added 2026-04-26 in Phase 4b)" addendum to the foundation-phase deviation entry. Original "single-PR convention" text preserved unchanged for historical accuracy; addendum records:

• The pivot's actual scope: foundation → Phase 4a landed via PR [Task 55] CPS handler codegen: foundation through Phase 4a (multi-arm dispatch) #22; Phases 4b–4f each land as their own focused PR against main on per-phase branches (plan-b-task-55-phase-4b, plan-b-task-55-phase-4c, etc.).
• Reasoning matrix: reviewability (PR [Task 55] CPS handler codegen: foundation through Phase 4a (multi-arm dispatch) #22 was already 2,373/-272 with two review rounds — bundling 4b–4f would compound), independent CI cycles, rollback granularity, review checkpoint cadence (Plan B's "every change reviewed by a human" honored more naturally per-phase).
• Forward guidance for resuming agents: expect the per-phase-PR cadence going forward.

The Closure point text updated to reflect "1/5 of the post-Phase-4a Phase 4 PRs in flight" (Phase 4b at PR #23).

Review #2 — doc/code drift assert! vs debug_assert!

Code change: lower_perform_non_io_to_value's bound check is now debug_assert! (was assert!). Per your reasoning — the runtime's sigil_perform overflow check at runtime/src/handlers.rs:588 aborts with a named effect_id / op_id message that's strictly better than the codegen-side "internal compiler error" panic; the compiler-side check exists to catch the bug pre-link in dev builds before the runtime guard fires. v1's effect arities (0–2 user args) sit so far below the 30-arg cap that this delta has zero observable surface today. Phase 4b deviation entry's "Bound check" paragraph rewritten to match the actual code shape.

Review #3 — widening fallthrough silently miscompiles future floats in release

Code change: debug_assert_eq! → assert! in the widening fallthrough's pointer-type assertion. A future F32/F64 surface type or 32-bit-target port that smuggles a non-pointer-width value through the args-buffer path now panics in both dev and release builds rather than silently storing the bit-pattern as if it were a pointer-sized value. Comment expanded to explain the trade-off ("cheap insurance until v2 either adds floats with an explicit branch or this assertion fires and forces the question"). New "Float / 32-bit-target safety net" paragraph in the Phase 4b deviation entry documents the policy.

Did NOT take the alternative (explicit F32/F64 panic arm) because v1 has no float surface type — adding a dedicated branch with a panic that names floats would be premature, and the assertion's named error message ("Phase 4b only supports I64 (Int), I32 (Char), I8 (Bool/Byte/Unit), and pointer_ty (String / user-type pointers); floats and 32-bit-target pointer types need a dedicated branch") tells a future agent exactly what to add when the assertion fires.

---

Pod-verify clean. Pushing now; CI re-running.

[boldfield]

Re-review of PR #23 — fixup commit 7ce88718

All three review items addressed cleanly. The author also self-identified one additional concern (MF2) and one stale-doc item (MF3) and folded both into this fixup. CI green on both targets.

Items closed from prior review:

• #1 Args-content verification gap — took option (a): defer to Phase 4c with a pre-registered acceptance criterion in the Phase 4b deviation entry. Phase 4c's PR is now bound to ship four named tests before merging: Int readback, Bool/Char readback (exercises uextend), String readback (exercises pointer-store), multi-arg-in-declared-order readback (pins offset arithmetic against the runtime's args_ptr.add(i) u64-stride). Strong contract — Phase 4c's reviewer can refuse merge if any are missing. Bonus: explicit pointer in the deviation entry telling a future bisecting agent to suspect Phase 4b for any wrong-args-value bug surfacing in Phase 4c.
• #2 debug_assert vs assert! drift — code changed to debug_assert!, deviation text updated to match. Comment now correctly identifies the runtime's named overflow check as source of truth and the compiler-side debug_assert! as dev-build defense.
• #3 Future-floats fallthrough — debug_assert_eq! → assert_eq! so future F32/F64 or 32-bit-target ports panic in both dev and release. Comment expanded to explain the trade-off; deviation entry has a "Float / 32-bit-target safety net" paragraph.

Bonus self-identified items:

• MF2 (Phase 4d arena migration) — PHASE-4-RESTRICTION (Plan B Task 55, Phase 4d): marker added directly on the create_sized_stack_slot line. Notes that the stack-slot lifetime is sound only under the synchronous lower_perform_non_io_to_value → sigil_perform → sigil_run_loop chain; Phase 4d's trampoline conversion will require migrating to sigil_arena_alloc. Cross-references the existing "Native callers drive sigil_run_loop synchronously" deviation entry. Caught a real Phase 4d dependency I missed.
• MF3 (PR #22 deviation entry stale) — PR #22's "single-PR convention" claim turned out wrong once Phase 4b landed as its own PR. Cadence-pivot addendum recording per-phase-PR cadence with reasoning (reviewability, independent CI cycles, rollback granularity, review-checkpoint cadence). Original text preserved unchanged for historical accuracy. Honest correction.

Nothing else to flag. Ready to merge once any other reviewers sign off.

[boldfield]

Re-review at 7ce8871 — merge

All 3 must-fix items + 2 nits from the no-context reviewer addressed cleanly. CI green on all 4 lanes; 421 compiler lib tests pass locally.

Verification

Item	Verification
MF1 — args-content verification gap	✅ Substantive Phase 4c precondition at PLAN_B_DEVIATIONS.md:108-115. Names 4 specific required tests (Int readback, Bool/Char readback exercising uextend, String readback exercising pointer-store, multi-arg in declared order). Includes explicit "bisecting agent hint" pointing back to Phase 4b as the suspect commit if a wrong-args bug surfaces post-Phase-4c
MF2 — stack-slot lifetime tied to synchronous call pattern	✅ Multi-paragraph comment immediately above create_sized_stack_slot at compiler/src/codegen.rs:1980 explaining the synchronous-call dependency and naming Phase 4d as the trigger for migrating to sigil_arena_alloc. Cross-references both the PB4 "Native callers drive sigil_run_loop synchronously" entry and the Phase 4b entry. Goes beyond what I asked for (I suggested a single-line marker; they wrote 14 lines of context)
MF3 — PR #22 single-branch deviation entry stale	✅ Substantive "Cadence pivot (added 2026-04-26 in Phase 4b)" addendum at PLAN_B_DEVIATIONS.md:35-42. Four named reasons (reviewability, independent CI cycles, rollback granularity, review checkpoint cadence). Original "single-PR convention" claim preserved unchanged for historical accuracy. Sets explicit expectation for future Task 55 phases (plan-b-task-55-phase-{4c,4d,4e,4f} against main)
Review #2 (no-context reviewer) — assert! → debug_assert!	✅ compiler/src/codegen.rs:1907. Reasoning sound — runtime overflow check has the better diagnostic with named effect_id/op_id; compiler-side guard exists to catch in dev builds pre-link
Review #3 (no-context reviewer) — debug_assert_eq! → assert! for widening fallthrough	✅ compiler/src/codegen.rs:2015. Inverts the dev/release tradeoff for SAFETY — future F32/F64 surface types or 32-bit-target ports will panic in both dev AND release builds rather than silently storing the bit-pattern. Defensible because this code runs at compile time, not at FFI runtime, so panic semantics are fine

Mechanical

• cargo test -p sigil-compiler --lib: 421 passed, 0 failed
• All 4 CI lanes SUCCESS on 7ce8871
• Pod-verify clean (per fix-up commit message)
• No new dependencies, no clippy warnings

Process note

The assert! ↔ debug_assert! flips in opposite directions (Review #2 toward less strict, Review #3 toward more strict) demonstrate the agent thinking about each site's failure mode independently rather than mechanically applying a discipline. The MAX_INLINE_ARGS bound is a redundant check (runtime catches it with a better message); the widening fallthrough is a safety check (silent miscompile if missed). Different concerns, different choices. Good judgment.

Ready to merge.