Codecov Report

❌ Patch coverage is 94.06780% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.90%. Comparing base (8f9e15b) to head (a167d22).

@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
+ Coverage   90.88%   90.90%   +0.02%     
==========================================
  Files          60       60              
  Lines       23529    23606      +77     
  Branches      259      259              
==========================================
+ Hits        21384    21459      +75     
- Misses       2138     2140       +2     
  Partials        7        7              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant Host
  participant ShadowGuard as _ShadowGuard
  participant Serde as SerdeWriter
  participant WASM as WASM_memory
  participant GC as gc_collect

  Host->>ShadowGuard: __enter__ (snapshot gc_sp)
  Host->>Serde: write(..., guard, input)
  Serde->>WASM: alloc(field_size)
  WASM-->>Serde: field_ptr
  Serde->>ShadowGuard: push(field_ptr)
  Serde->>WASM: alloc(body_size)
  WASM->>GC: gc_collect (if memory full)
  GC->>WASM: scan shadow_stack (sees field_ptr)
  GC-->>WASM: mark field_ptr live
  WASM-->>Serde: body_ptr
  Serde->>Serde: recurse with same guard
  Host->>ShadowGuard: __exit__ (restore gc_sp)

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

@coderabbitai also applied the outside-diff TESTING.md finding in 4b8c127: line 193's pytest-parametrized shell-block comment is now # Via pytest (parametrized — 440 tests) to match the conformance suite's new 88 programs × 5 stages total.

@aallan: Confirmed — 88 × 5 = 440 checks out. The comment is now consistent with the rest of the conformance-count updates in this PR. Thanks for the follow-through.

pr-review-toolkit: code-reviewer agent findings

Agent reviewed: CLAUDE.md compliance, general code quality, bug detection, release-prep correctness. Ran pytest tests/test_codegen.py::TestHostWalkerGCRooting692 tests/test_codegen.py::TestWorklistOverflow348 (7/7 pass), scripts/check_conformance.py (88/88 pass), grepped for stale 87 references (none found).

Critical / Important

None blocking. The fix correctly identifies and closes the bug class.

Suggestions

Pre-existing similar bug class outside #692 scope. _alloc_array_of_strings at vera/codegen/api.py:1214-1230 has the identical shape — backing_ptr held in a Python local across count sub-allocations of str_ptr via _alloc_string. Called from at least 5 sites (host_args, host_md_extract_code_blocks, Regex.findall, Map.keys, Map.values, Set elements path). Not regressed by this PR (predates it) and not in the bug report, but it's the same shape and would benefit from the same _ShadowGuard treatment.

• Verdict: ⏭️ Skipped with reason. Out of scope for html_parse and json_parse trap with Out-of-bounds memory access on inputs that pressure GC during host-side tree walk #692 — file as a follow-up issue rather than expand this PR. The agent specifically rated this 78/100 ("would file a follow-up issue rather than block") and I agree.

Comment slightly misleading at host_html_query (vera/codegen/api.py:3203-3206): "WASM codegen at the call site shadow-pushes it via the standard mechanism" overstates the protection. _translate_html_query in vera/wasm/calls_markup.py:93-110 emits just call $vera.html_query with no explicit shadow push; the consuming let/match is what shadow-pushes the result.

• Verdict: ✅ Applied in 4e43eaa. Comment softened to: "the guard exits before the function returns; the WASM-side caller's local-store push is what roots the result thereafter. Safe in practice because no allocation happens between return and local-store."

Strengths flagged by the agent

• Round-2 fix is structurally sound. Redundant guard.push(arr_ptr) correctly removed from markdown call sites after helpers that root internally.
• JString reorder in json_serde.py:103-116 allocates the string first and roots it before the JString wrapper — eliminates the secondary bug shape.
• WAT exports use inline export syntax — minimal surface change.
• Module-docstring discipline — all three serde modules explain the guard contract upfront.
• Release prep correctness: CHANGELOG [0.0.158] mirrors [0.0.157] shape; link references chained correctly; version sync clean across 6 files; HISTORY.md row consistent with the v0.0.156/v0.0.157 longer-form precedent.

Verdict

Ship after dropping the redundant pushes (already done in 4b8c127) and softening the host_html_query comment (done in 4e43eaa). Pre-existing _alloc_array_of_strings follow-up tracked outside this PR.

pr-review-toolkit: pr-test-analyzer agent findings

Agent reviewed: behavioural test coverage on the new GC-rooting fix, conformance manifest wiring, test sizing rationale.

Important gaps

1. JObject branch of write_json has no large-input test (rating 7). vera/wasm/json_serde.py:155-180 now pushes three distinct intermediate pointers per dict iteration plus map_dict is a Python-local-only int dict — the exact conservative-scan-blind-spot pattern that motivated the whole PR. None of the four original tests exercise this branch.

• Verdict: ✅ Applied in 4e43eaa. Added test_json_parse_500_key_object — 500-key flat JObject like {"k":0, "k":0, ..., "last":0}. Exercises the val_ptr-pushed-per-iteration pattern + wrapper_ptr push + body alloc.

2. host_html_query is touched but untested at scale (rating 6). vera/codegen/api.py:3196-3216 adds a _ShadowGuard around the arr_ptr + per-match write_html loop. Existing tests use trivial 1-3 element inputs.

• Verdict: ✅ Applied in 4e43eaa. Added test_html_query_30_matches — 30 <p>x</p> siblings under <root>, then html_query("p") to exercise the re-walk path. Bounded at 30 rather than 500: each match's recursive write_html accumulates pushes within a single guard window, and empirically 100 overflows the 4096-entry stack when combined with the prior html_parse's retained pushes. 30 still triggers GC during the walk.

3. _ShadowGuard is not unit-tested directly (rating 5). The four added tests cover the integration path but never the contract: __exit__ resetting $gc_sp on success, __exit__ doing so on exception, the no-push case, overflow detection.

• Verdict: ⏭️ Skipped with reason. _ShadowGuard is a closure inside compile_to_wasm, not module-level — direct unit testing requires extracting it. The behavioural contract is implicitly verified by 6 in-process tests + 4 conformance tests all passing under VERA_EAGER_GC=1 (forces GC on every alloc, exercising the rooting path maximally). Filing as a follow-up if a regression surfaces.

Test quality issues

• test_worklist_size_quadrupled_in_wat couples to exact WAT string formatting. Acceptable as-is — it's a structural sentinel — but flag for awareness.

• Documentation of dropped patterns: the "unclosed tags 100 → 50 → dropped due to recursion limit" design history isn't preserved.

  • Verdict: ⏭️ The conformance test header at lines 23-28 explains the 500-bound choice, and the CHANGELOG at line 23 mentions the recursion limit constraint. Adequate without a separate breadcrumb.

Strengths flagged

• Test sizes target page-growth rather than absolute memory pressure — making accidental-pass on larger-initial-memory machines unlikely.
• Each new test has a clear docstring naming the specific intermediate pointers it exercises — DAMP.
• Class docstring cross-references the WAT-side cousins (TestArrayMapGCRooting570, TestFoldAccumulator515, TestClosureReturnShadowAsymmetry593).
• Structural WAT assertion correctly pins the new exported $gc_stack_limit shape.
• Manifest entry correctly placed alongside related ch09 entries.

Verdict

Two important test gaps closed in 4e43eaa (host_html_query + JObject large-input). _ShadowGuard unit-test gap deferred — implicit coverage is strong; explicit unit tests require closure-extraction work outside this PR's scope.

pr-review-toolkit: comment-analyzer agent findings

Agent reviewed: comment accuracy vs implementation, cross-reference correctness, claim-vs-code drift.

Improvement opportunities

1. host_html_query comment overstates protection (vera/codegen/api.py:3203-3206): "WASM codegen at the call site shadow-pushes it via the standard mechanism" — but _translate_html_query in calls_markup.py:93-110 emits just call $vera.html_query with no automatic shadow push of the (arr_ptr, count) return pair. The receiving local push is what roots it.

• Verdict: ✅ Applied in 4e43eaa. Same finding as code-reviewer's v0.0.5: Type checker #2 from a different angle. Comment now: "the guard exits before the function returns; the WASM-side caller's local-store push is what roots the result thereafter. Safe in practice because no allocation happens between return and local-store, but the guard's protection does NOT extend past the function boundary."

2. host_md_parse "Mirrors the narrow-catch pattern in host_html_parse and host_json_parse" (vera/codegen/api.py:1797-1798): this implies structural identity, but host_html_parse (pre-fix) placed the _ShadowGuard block INSIDE its try while host_md_parse and host_json_parse had it OUTSIDE.

• Verdict: ✅ Applied in 4e43eaa. Combined with silent-failure-hunter's same finding: fixed host_html_parse to put shadow-stack outside the try (structural match restored), and updated the comment to "Matches the parse-only-in-try structure of host_html_parse and host_json_parse (both narrow their catch around only the parse call, with _ShadowGuard usage outside)" — now structurally accurate.

3. _write_inline_array docstring missing rooting contract (vera/wasm/markdown.py:316-336): one-line docstring doesn't tell a developer reading just the function that backing remains on the shadow stack after return.

• Verdict: ✅ Applied in 4e43eaa. Added rooting-contract docstrings to all four markdown array helpers (_write_inline_array, _write_block_array, _write_array_of_block_arrays, _write_table_data) so future maintainers see the convention from the function signature.

4. assembly.py:367-368 "$gc_sp is (mut)" phrasing elides WASM-side mutator: "the host advances it to push, resets it to pop" could be read as the host being the only mutator, but $gc_collect and function prologues/epilogues also mutate it.

• Verdict: ⏭️ Skipped with reason. The agent rated this low ("could infer") and the comment block is already substantial. The WAT-side gc_shadow_push helper documented at vera/wasm/helpers.py:113 makes the WAT-side mutation discoverable. Adding another sentence would tip into over-commenting.

Positive findings

• _ShadowGuard.push() comment explaining why struct.pack_into doesn't work (LP_c_ubyte lacks buffer protocol) and why data_ptr must be re-acquired on every push — exactly the "non-obvious why" comments should capture.
• markdown.py module docstring's "allocate fields first, root them, allocate the body last" convention is the architectural invariant future maintainers need.
• "No second push needed here" comments in markdown branches are well-targeted; without them a reviewer might "fix" the omission.
• HISTORY.md row matches v0.0.156/v0.0.157 precedent — denser than older Stage 12 rows but not drifted into mini-CHANGELOG shape.
• #694 entry in KNOWN_ISSUES.md — file:line refs to test_browser.py:160 and :1736 both verified accurate (both contain timeout=30); diagnostic concrete and actionable.

Verdict

Three of four findings applied in 4e43eaa; one skipped as low-value over-commenting. Comments now accurately describe the code on every flagged site.

pr-review-toolkit: silent-failure-hunter agent findings

Agent reviewed: silent failures, exception-scope correctness, masking, resource leaks.

Critical

1. host_html_parse shadow-stack work is INSIDE its narrow except (vera/codegen/api.py:3129-3155): the with-block sits between try: and except (ValueError, TypeError, AttributeError). Silently repackages ValueError from _alloc_map_wrapper → _wrap_handle (the #578 _validate_wrap_handle range check), AttributeError from a malformed node dict, TypeError from _write_i32/alloc boundary mismatches. Comment at 3146-3154 promises "we deliberately do NOT catch invariant violations" but the code structure contradicts it.

• Verdict: ✅ Applied in 4e43eaa. Same finding caught by comment-analyzer from a different angle. Restructured host_html_parse to put parse-only-in-try, shadow-stack work + write_html + Result.Ok alloc OUTSIDE the narrow except. All three host walkers (html, json, md) now structurally match. This was the most material finding of the review.

Important

2. host_md_parse except Exception is broader than necessary — parse_markdown has zero raise statements anywhere in the module today.

• Verdict: ⏭️ Skipped with reason. Keep broad as defensive: parse_markdown could grow raise statements in future, and a future bug there should still surface as Result.Err. Narrowing now creates a regression risk for marginal cleanup benefit.

3. _ShadowGuard.push() raises bare RuntimeError that becomes "python exception" trap — different diagnostic shape from the WAT-side unreachable overflow.

• Verdict: ⏭️ Skipped with reason. The message starts with #692: and includes sp= / limit= values, which is enough debugging context. A dedicated HostShadowStackOverflow subclass would be marginal diagnostic-quality improvement for non-zero implementation cost.

4. _ShadowGuard.__init__ raises bare KeyError if globals not exported.

• Verdict: ✅ Applied in 4e43eaa. Wrapped both caller["gc_sp"] and caller["gc_stack_limit"] lookups in a try/except that re-raises as RuntimeError("#692: host walker requires the module to export $gc_spand$gc_stack_limit ...") so hand-crafted-.wat scenarios get an actionable diagnostic.

5. _ShadowGuard.__exit__'s if self._initial_sp is not None is defensive but unreachable — should be assert.

• Verdict: ✅ Applied in 4e43eaa. Tightened to assert self._initial_sp is not None. The only way the None case is reachable is misuse outside a with block; the assert pins the invariant.

6. Markdown helpers push backing AFTER alloc — exception between them leaks the block (vera/wasm/markdown.py:329-330, 352-353, 377-378, 402-403, 414-415).

• Verdict: ✅ Applied in 4e43eaa. Added explicit comment block documenting the alloc-then-push allocation order is load-bearing — a future "fix" must NOT try to push before alloc (impossible without a ptr) or wrap the push in try/except (would break the trap-on-overflow invariant). Confirmed correct semantics: exception propagates, walk abandoned, next GC reclaims the leaked block.

Important (data integrity)

7. JSON JObject loop accumulates val_ptrs in map_dict after pushes are unwound on exception.

• Verdict: ✅ Applied in 4e43eaa. Added an exception-safety note to the JObject branch documenting: map_dict partial state is safe to discard on mid-loop raise (function exits via the raise BEFORE map_alloc is called; the val_ptrs are guaranteed-invalid after guard exit and a future "rescue" would trigger use-after-free).

Suggestions

8. __exit__'s set_value could mask original body exception (Python's normal exception-chain semantics; minor diagnostic quality).

• Verdict: ⏭️ Skipped with reason. Theoretical — set_value on a known-valid Global doesn't fail in practice. Wrapping in another try/except adds complexity for a path that doesn't fire.

9. _ShadowGuard._caller could become invalid if leaked — guard always used inside a single host function; no path to leak.

• Verdict: ⏭️ Skipped with reason. No reachable leak path; theoretical concern.

Positive findings

• host_json_parse is the gold-standard pattern — narrow catch around _json.loads only, _ShadowGuard block outside. Used as the reference for restructuring host_html_parse.
• if s_ptr != 0 empty-string guards correctly avoid pushing the GC's "not a heap object" sentinel.
• Comments throughout the diff explain why each push happens before each alloc with concrete trap signatures.
• fields-first-then-body convention in markdown is exactly the right invariant — body pointer never has to be rooted.
• gc_stack_limit is correctly immutable in WAT (no (mut i32)) — host can read but not corrupt.

Verdict

One critical asymmetry fixed (host_html_parse restructure — the headline finding). Five important findings applied; three skipped with documented reasons. All four host walkers now structurally consistent and the stated invariants match the code.

@coderabbitai outside-diff finding (vera/wasm/json_serde.py JObject children rooting) verified — real architectural bug, but pre-dates #692 and applies to the broader Map<K, T_heap> contract, not just JObject.

Confirmed by inspecting git show $(git merge-base main HEAD):vera/wasm/json_serde.py: the pre-#692 JObject branch had the same map_dict[str(k)] = val_ptr pattern that stores heap pointers as Python ints in _map_store. The #692 fix improved DURING-walk safety by pushing val_ptrs to the shadow stack via guard, but does NOT address POST-walk reachability — after the guard exits (resetting $gc_sp), val_ptrs are once again only reachable from _map_store[handle], which the conservative GC scan can't see (handle is bit-31 tagged per #578, so it's structurally not a heap pointer).

The bug affects all Map<K, T_heap> not just JObject:

• 🐛 Vulnerable: Map<K, Json> (JObject), Map<K, HtmlNode>, Map<K, Md*>, any user-defined Map<K, T> where T is heap-allocated.
• ✅ Safe: Map<K, V_inline> (Int/Bool/Float — values stored as Python primitives), Map<K, String> (strings re-encoded into WASM memory at access time, which is why HtmlElement.attrs doesn't have this bug).

Filed as #695 with a reproducer sketch, three candidate fix options (host-side WASM container per entry / extend GC tracing to walk _map_store / serialise values into WASM at insertion), and the affected-Map-types breakdown. Tracked in KNOWN_ISSUES.md per the PR-generated-new-issues-must-be-tracked-in-the-same-PR rule (commit 9b170bd).

Deferring rather than expanding #693 because: (a) the bug is pre-existing, (b) the fix is a substantive architectural change touching the Map contract or the GC tracer, (c) #693 is already at 27 files / +710 lines / 5 commit rounds and adding a Map-contract change muddles the review surface. Closing #692's during-walk-safety in this PR, leaving #695 as a clean follow-up.

@aallan: Analysis is correct and the triage is clean.

The critical asymmetry you've identified is precise: _ShadowGuard closes the during-walk window (val_ptrs on shadow stack while write_json recurses), but the post-walk window is a structurally different problem — once __exit__ resets $gc_sp, the only remaining reference to each val_ptr is _map_store[handle] where the handle carries the bit-31 ownership tag from #578, which is exactly what prevents the conservative GC from treating it as a heap pointer. That's not a gap in #693's fix; it's a separate invariant the Map contract needs to satisfy.

The affected/safe breakdown is also sound — Map<K, String> escapes because strings are re-materialised from WASM memory at access time rather than stored as raw heap pointers, whereas any Map<K, T_heap> where T is a heap-resident structured value has the same exposure.

Deferral to #695 is the right call. The three candidate options span a meaningful design space (per-entry host container, GC tracer extension, eagerly serialise into WASM at insertion), and picking among them deserves a clean PR without the noise of an already-large diff. Good that the KNOWN_ISSUES.md entry is in this commit so the gap is visible to anyone reviewing #693 in the future.

Learnt from: aallan
URL: https://github.com/aallan/vera/pull/693

Timestamp: 2026-05-22T11:25:05.071Z
Learning: In vera/wasm/json_serde.py and the broader Map<K, T_heap> contract: after a _ShadowGuard exits and resets $gc_sp, val_ptrs stored as Python ints in _map_store[handle] are no longer reachable by the conservative GC. The handle carries a bit-31 ownership tag (per `#578`) which prevents the GC from treating it as a heap pointer. This post-walk reachability gap affects Map<K, Json>, Map<K, HtmlNode>, Map<K, Md*>, and any Map<K, T> where T is heap-allocated. Map<K, V_inline> (Int/Bool/Float) and Map<K, String> are safe. Tracked as issue `#695`.