fix(gc): shadow-root pair-typed let bindings — host-import pairs were swept by the next alloc - #846
Merged
Merged
Conversation
… swept by the next alloc — v0.0.194 The plain-let pair branch (String / Array<T> -> (ptr, len) locals) in translate_block never pushed its pointer local onto the GC shadow stack — the last unrooted sibling of the #705 scalar-i32 let fix and the #707 let-destruct pair fix. Vera-side pair producers masked the gap by shadow-pushing their own freshly-allocated dst at the alloc site, but a host-import pair (IO.args -> Array<String>, IO.read_line -> String) is rooted only host-side during construction, so the first Vera-side allocation after the let collected the block while the locals still pointed at it: printing an IO.args element after a nat_to_string call yielded free-list bytes (2::++2@ instead of 2:aa+bb). Found stress-testing #237 under VERA_EAGER_GC=1; no WASI code involved. Pair lets are now rooted unconditionally (static / null pointers fail the conservative scan's heap range check, so the push is harmless). RED-first regression tests pin the args and read_line reproducers and confirm the IO.read_file / IO.get_env ADT paths were already rooted. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
📝 WalkthroughWalkthroughAdds explicit GC shadow-stack rooting for pair-typed ChangesGC rooting fix and regression tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…nd test class name The vera/ change is a comment-prefix edit only (adds the #846 anchor to the rooting comment); the CHANGELOG bullet itself gains the PR link rather than a new bullet. Skip-changelog: backfilling #846 into the existing v0.0.194 bullet; vera/ diff is comment-only Co-Authored-By: Claude <noreply@anthropic.invalid>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #846 +/- ##
=======================================
Coverage 91.90% 91.90%
=======================================
Files 93 93
Lines 27836 27838 +2
Branches 332 332
=======================================
+ Hits 25583 25585 +2
Misses 2245 2245
Partials 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@HISTORY.md`:
- Line 430: The HISTORY.md version row for v0.0.194 contains two links, but it
must follow the one-sentence/one-link rule. Edit the row to keep the PR
reference in the GC rooting note and remove the extra stress-test issue link
from the mention of the stress-testing run, preserving the rest of the sentence.
Locate the entry by the v0.0.194 row and ensure only a single markdown link
remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1c88773d-96bf-4b58-801d-4a50c1b416b8
📒 Files selected for processing (5)
CHANGELOG.mdHISTORY.mdTESTING.mdtests/test_codegen_gc_rooting.pyvera/wasm/context.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
…in ROADMAP Issue #847 (filed independently from the #237 WASI-branch work) reports the same bug this PR fixes; the CHANGELOG bullet, HISTORY row, and test class docstring now anchor on it, and the PR closes it on merge. The issue's host-side mechanism hypothesis is corrected on the issue with a pre-fix differential: element reads with no intervening Vera-side alloc are clean under eager GC, so the host construction (_ShadowGuard, #706-era) is sound and the hole is the wasm-side pair let. Also adds the ROADMAP Tier 3 row for #848 (the test_async_await_keyboard_interrupt_live_request ordering race that cancelled the fail-fast matrix on this PR's first CI run). Skip-changelog: docs-only — issue anchors + roadmap tracking rows; no compiler change Co-Authored-By: Claude <noreply@anthropic.invalid>
A flaky test is a defect, not future feature work: the entry belongs in the KNOWN_ISSUES.md Bugs table (one-to-one with open bug-labelled issues; #848 is now labelled) rather than the ROADMAP Tier 3 table. Skip-changelog: docs-only — relocates the #848 tracking row between doc files Co-Authored-By: Claude <noreply@anthropic.invalid>
…ot v0.0.194 Maintainer call: this bug fix does not need to cut its own release. Reverts the version bump across vera/__init__.py, pyproject.toml, uv.lock, README.md, docs/index.html, and the regenerated site assets; moves the CHANGELOG bullet from the [0.0.194] section into [Unreleased] (section and link ref removed, [Unreleased] compare link restored to v0.0.193...HEAD); and drops the HISTORY.md Stage 16 row, which records cut releases only — the row gets written when a future release actually ships this. The live doc counts (5,620 tests, gc_rooting file rows) stay: they reflect the code, not the release. Co-Authored-By: Claude <noreply@anthropic.invalid>
aallan
changed the title
aallan
pushed a commit
that referenced
this pull request
Jul 2, 2026
…the sprint Carrying the row in this PR would only conflict with the #846 rebase that removes it (maintainer direction on the #849/#846 coordination). Issue #847 remains open and tracked; #846 closes it on merge. Skip-changelog: KNOWN_ISSUES-only edit; #847 tracking is on the issue and PR #846 Co-Authored-By: Claude <noreply@anthropic.invalid>
Merged
4 tasks
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 218: The project-status text and the project-structure summary are
inconsistent about the number of examples; update the count in the README so it
matches the current total used by the status line, or adjust both places if 35
is the intended value. Check the “Vera is in active development” sentence and
the nearby project-structure block that mentions “example Vera programs” to keep
the example count consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3146197b-c766-47ab-8e68-f64a3ae680ab
📒 Files selected for processing (5)
CHANGELOG.mdKNOWN_ISSUES.mdREADME.mdTESTING.mdtests/test_codegen_gc_rooting.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
aallan/vera-bench(manual)
… fail-fast: false for the matrix test_async_await_keyboard_interrupt_live_request printed its progress marker between async(...) and await, racing the worker thread's request — whose arrival gates the interrupt — against the guest reaching the print. A lost race yields the correct exit 130 with empty stdout, failing the "before await" assertion: twice on macos-26 runners across PR #846's matrices, each time cancelling the other 11 fail-fast jobs. The print now precedes the async(...), so program order supplies a real happens-before (buffer write -> request issuance -> arrival -> interrupt); the stdout assertion strengthens from `in` to `==`. Prompt issuance at the async point stays pinned by the #841 request-ordering and operand-stack tests, so the reorder loses nothing. The mocked sibling (Future.result patched to interrupt) was already deterministic and is untouched. The test matrix also sets fail-fast: false so a single red combo reports alone instead of cancelling eleven healthy jobs — job names are unchanged, so the branch-protection required-checks list is unaffected. Validated: 21 consecutive local runs of the reordered test; full test_runtime_traps.py green. Co-Authored-By: Claude <noreply@anthropic.invalid>
…d ROADMAP The true count is 36 (checked by scripts/check_examples.py and stated by the README status line); the structure-block comment and the ROADMAP status line had drifted. Surfaced by the PR #846 review. Skip-changelog: docs-only count correction Co-Authored-By: Claude <noreply@anthropic.invalid>
aallan
pushed a commit
that referenced
this pull request
Jul 2, 2026
…-p2-target Conflict resolution: the #846/#848 [Unreleased] Fixed bullets ride the v0.0.194 section this PR cuts (the queued-entries convention); doc counts recomputed against the merged tree (5,768 tests); main's updated TESTING rows for test_codegen_gc_rooting / test_runtime_traps kept. #847 is closed by #846, so no KNOWN_ISSUES row returns. Co-Authored-By: Claude <noreply@anthropic.invalid>
aallan
added a commit
that referenced
this pull request
Jul 2, 2026
…arget wasi-p2 — v0.0.194 (#849) * feat(wasi): experimental WASI Preview 2 target — vera compile/run --target wasi-p2 — v0.0.194 Emit a binary WebAssembly component whose vera.* IO + Random imports are implemented over WASI 0.2 interfaces (dispatch-table shim topology, GC-exempt cabi_realloc arena, adapter core module elem-planted before any lift), runnable by stock wasmtime with no flags and no Vera bindings. vera run --target wasi-p2 executes it under the built-in add_wasip2 host with the core-path ExecuteResult contract, including trap-kind classification recovered from the backtrace text and the WASI stderr channel. Unsupported host families are a diagnostic naming the family, never a silent fallback. Dual-target conformance differential: 71 of 88 run-level programs byte-identical across targets (rest family-gated, main-less, or wall-clock; loud skips). New spec chapter 13 documents the architecture and the inherent WASI 0.2 divergences (exit codes degrade to 0/1, no structured trap frames). Also: doc-shadowing scanner skips .claude/; IO.args GC-rooting bug found in passing filed as #847 with a KNOWN_ISSUES row. Refs #237 Co-Authored-By: Claude <noreply@anthropic.invalid> * fix(wasi): CRLF read_line parity + cross-clock slack in the time test — windows-latest CI The core host reads stdin through Python's universal-newlines text layer, so read_line never returns a trailing \r on any platform; the adapter now strips a trailing \r alongside the \n (RED-first via an explicit CRLF-bytes stdin test that failed on every platform). A lone \r separator stays content — documented divergence, spec section 13.6. The LF stdin fixture now writes binary so its bytes are deterministic cross-platform. The time-bracket test gains 100 ms of cross-clock slack (wall-clock vs time.time() quantization overshot by 1 ms on windows-latest) — still catches unit errors, the test's real target. Co-Authored-By: Claude <noreply@anthropic.invalid> * docs(known-issues): drop the #847 row — PR #846 fixes it right after the sprint Carrying the row in this PR would only conflict with the #846 rebase that removes it (maintainer direction on the #849/#846 coordination). Issue #847 remains open and tracked; #846 closes it on merge. Skip-changelog: KNOWN_ISSUES-only edit; #847 tracking is on the issue and PR #846 Co-Authored-By: Claude <noreply@anthropic.invalid> * fix(wasi): CodeRabbit round 1 — preopen descriptor cache, marker-scan narrowing, doc sync - Cache the first preopen descriptor in a $preopen_fd global (sentinel -2): get-directories returns a fresh OWNED descriptor list per call, so the previous per-file-op fetch leaked one handle into the instance's resource table on every IO.read_file/write_file. Same process-lifetime pattern as the cached std stream handles; pinned structurally (single l_get_dirs call site) and behaviorally (60 recursive reads through one instance). - Narrow the reserved-identifier scan to non-data WAT lines: a program PRINTING "$wasi_tbl" is not a collision (regression pair pins both directions: literal accepted, real fn-name collision still rejected). - random_int equal-bounds test (low == high == 4 -> exactly 4) pins upper-bound inclusivity deterministically. - Import WasmTrapError from vera.codegen.api (the CLI's canonical import site; same class object). - Doc sync: README project-structure block (14 chapters / 36 examples), TESTING.md conformance prose (104), stale allowlist note in check_skill_examples.py re-anchored + entries reordered. Co-Authored-By: Claude <noreply@anthropic.invalid> * fix(wasi): CodeRabbit round 2 — identifier-boundary marker scan, stdin tail pins - The reserved-marker scan now requires an identifier boundary after each exact marker ($wasi_tblish is a different identifier, not a collision; $wasi_sig_ stays the deliberate prefix family) — pinned by an acceptance test that runs a fn named wasi_tblish end to end, mutation-validated RED with the boundary disabled. - Two stdin tail pins, each verified against the core host's universal-newlines behavior first: \r-at-EOF IS stripped (Python's text layer treats it as a line break — gating the strip on a \n terminator would diverge from the core path), and a lone \r separator stays content per the documented spec §13.6 divergence. Co-Authored-By: Claude <noreply@anthropic.invalid> * fix(test): #848 round 2 — gate the live-interrupt on the main thread being parked in await Server arrival is produced by the executor worker thread and says nothing about the main thread's position; interrupt_main() only sets a pending SIGINT flag, and firing it early let the flag materialize outside execute()'s protected region on slow runners (xdist worker crash on two macOS jobs, exit_code=None on ubuntu — 3/12 matrix jobs at PR #849's merge head). The interrupter now polls sys._current_frames() until the main thread's stack shows host_async_await -> Future.result before firing (bounded deadline so the test can never hang); the sub-second local runtimes prove the gate matches rather than falling through to the deadline. Analysis on the reopened #848. Co-Authored-By: Claude <noreply@anthropic.invalid> --------- Co-authored-by: T <t@e.invalid> Co-authored-by: Claude <noreply@anthropic.invalid>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a pre-existing GC use-after-free in the core compiler, found while stress-testing #237 under
VERA_EAGER_GC=1(no WASI code involved; reachable under ordinary collection pressure).The plain-
letpair-type branch (String /Array<T>→ (ptr, len) locals) intranslate_block(vera/wasm/context.py) never pushed its pointer local onto the GC shadow stack — the last unrooted sibling of the #705 scalar-i32 let fix and the #707 let-destruct pair fix (whose review comment invera/wasm/data.pyhad already named this gap class).Why it stayed hidden: every Vera-side pair producer (array literal, string builtin) shadow-pushes its own freshly-allocated
dstat the alloc site, and that push survives to the function epilogue — accidentally rooting the let. A host-import pair (IO.args→Array<String>,IO.read_line→ String) is rooted only host-side during construction (_ShadowGuardinvera/runtime/heap.py, popped on return), so after thelocal.setpair the block is invisible to the conservative scan. The first Vera-side alloc after theletcollected it, and the free list overwrote the payload's first words:printed
2::++2@instead of2:aa+bb(andstring_joinover the swept backing chased overwritten element pointers).Fix: root pair lets unconditionally, mirroring the #705/#707 siblings. Static and null pointers fail the conservative scan's heap range + alignment check, so the unconditional push is harmless for non-heap values.
Test-first evidence
The new
TestHostImportPairLetRootingclass intests/test_codegen_gc_rooting.pywent RED first, for the predicted reason — the read_line reproducer failed with'5:5\x00\x00\x00o'(free-list next-pointer overwriting the swept payload's first word), and bothIO.argsreproducers failed with reclaimed bytes. All three flipped green with the one-site fix.Two further tests confirm the neighbouring host-import ADT paths (
IO.read_file→Result<String, String>,IO.get_env→Option<String>) were already rooted by the #705-era scalar-let and match-arm fixes — green before and after, included to pin the paths the #237 stress testing had already exercised.Validation
mypy vera/clean; all 104 conformance programs; all 36 examples (check + verify)No release: the fix rides the CHANGELOG's
[Unreleased]section (maintainer call — the version stays 0.0.193 and the HISTORY row will be written when a future release ships this).Also fixes #848 (the flaky live-interrupt test)
The #841 live-interrupt test flaked twice on this PR's matrices (macos-26/3.11, then /3.12): it printed its progress marker between
async(...)andawait, racing the worker thread's request — whose arrival gates the interrupt — against the guest reaching the print. A lost race yields the correct exit 130 with empty stdout. The print now precedes theasync(...), so program order supplies the happens-before (buffer write → request issuance → arrival → interrupt) and the stdout assertion strengthens frominto==. Prompt issuance at theasyncpoint stays pinned by the #841 request-ordering and operand-stack tests; the mocked sibling was already deterministic and is untouched. The test matrix also setsfail-fast: false, so a single red combo reports alone instead of cancelling eleven healthy jobs (job names unchanged — no branch-protection impact). Validated with 21 consecutive local runs.Closes #847
Closes #848
🤖 Generated with Claude Code
Summary by CodeRabbit
letbindings hold(ptr, len)data, improving safety for strings, arrays, and other paired results.