v0.0.127: Close @Nat subtraction soundness hole (#520)#554
Conversation
… (refs #520) Two follow-up issues filed during design discussion for #520: - #551 — `@Byte - @Byte` has the same underflow shape as `@Nat - @Nat`. Mechanical follow-up once the verifier helper and codegen guard from #520 are reusable. Joins #520 in row 1 of the bug-killing campaign as a paired cluster (mirroring #487+#348 and #346+#347+#490 elsewhere in the table). - #552 — `@Nat` invariant check fires only at function return position. Narrowing from `@Int` into a `@Nat`-typed let binding or function argument is not obligation-checked. #520 closes the subtraction subset of this hole; #552 is the architectural generalisation. Filed in the Verification depth table (alongside Tier 2 and effect handler lifting) and as a Limitation in KNOWN_ISSUES.md, deliberately scoped *not* to block #520 — ships separately after #520 + #551 so the obligation infrastructure is reusable. Limitation tracker stays consistent (33 in KNOWN_ISSUES.md). Co-Authored-By: Claude <noreply@anthropic.invalid>
Three spec edits, ahead of the implementation, naming what the verifier will check and what the codegen will emit: - spec/04-expressions.md §4.4 — short clause mirroring the existing divide-by-zero language: "Subtraction on unsigned types is undefined behaviour when it would underflow. The compiler SHOULD verify lhs >= rhs. If it cannot, it MUST insert a runtime check." - spec/11-compilation.md §11.2.1 — full treatment. Names the proof obligation, the Tier-3 fallback codegen (guarded sub trapping with WasmTrapError(kind="underflow")), the lift-back path via `requires`, and references #551 (@byte) and #552 (binding-site generalisation). - spec/11-compilation.md §11.3.3 — footnote on the operator table pointing back to §11.2.1, so readers of the canonical "what does each operator compile to" reference learn that @nat - @nat is conditionally guarded. Allowlist line shift in scripts/check_spec_examples.py (130 -> 132) is a mechanical consequence of the §4.4 insertion; no duplicate keys. The spec is now honest about what §11.2.1 has long claimed ("the non-negativity invariant of Nat is enforced by the contract system") — naming the one operation that breaks the promise and how the compiler will close the gap. Co-Authored-By: Claude <noreply@anthropic.invalid>
Closes the verifier half of #520; the codegen runtime guard for unverifiable sites lands in the next commit. vera/verifier.py - _walk_for_subtraction_obligations: walks the function body and emits an `lhs >= rhs` proof obligation at every BinaryExpr.SUB whose result is statically @Nat-typed. Mirrors _walk_for_calls structurally — descends through IfExpr (push/pop on smt._path_conditions for then/else), MatchExpr (per-arm pattern conditions), Block (let-bindings update slot_env), BinaryExpr, UnaryExpr, FnCall args. - _check_subtraction_obligation: refutation check via smt.check_valid; counts Tier-1 verified or Tier-3 fallback (timeout/untranslatable), and emits E502 with counterexample on violated. - _report_underflow: E502 diagnostic with rationale, fix suggestion (`requires(@Nat.0 >= @Nat.1)` or guarded if-branch), and spec ref to §4.4 + §11.2.1. - _is_nat_typed: conservative static-type helper. Returns True for SlotRef of type @nat, non-negative IntLits, arithmetic expressions where both operands are @nat (per vera/checker/expressions.py:264-267 Nat <: Int subtyping rule), IfExpr/MatchExpr where every branch is @nat, FnCall returning @nat. False elsewhere. - _has_nat_origin: stricter helper requiring at least one operand to derive from a definitely-@nat source (slot ref or function return) — distinguishes `@Nat.0 - 1` (real subtraction on flowed Nat value) from `0 - 1` (the common "I want -1 as a literal" idiom widely used in `Err(_) -> 0 - 1` and `throw(0 - 1)` positions). Pure-literal underflow into a @nat binding is Path B / #552 territory. - Wired into _verify_fn at step 5.5 (after body translation, before call-violation drainage). vera/errors.py - E502: "@nat subtraction underflow obligation not discharged" in the E5xx verification cluster, adjacent to E500 (postcondition verified false) and E501 (call-site precondition violation). tests/test_verifier.py - TestNatSubtractionObligation520 — 9 test cases covering: * requires-clause discharge * if-guard / path-condition discharge (canonical recursion shape) * trivial discharges (subtract 0, self-subtract) * unguarded subtract failure (counterexample required) * Int-Int and Nat-Int -> Int exclusions (no obligation) * partial-requires non-discharge * pure-literal exclusion documenting Path-A scope - Updated tier counts where the new obligations land: * test_recursive_call_decreases_verified: 3 -> 4 T1 (factorial's @Nat.0 - 1 discharges via path condition) * test_mixed_tiers: 3 -> 4 (sum_to-shape function) * test_overall_tier_counts: 219/26/245 -> 222/26/248 aggregate (factorial.vera +1, mutual_recursion.vera +2) * test_mutual_recursion_example_all_t1: 8 -> 10 T1 Doc and site-asset updates (auto-regenerated): - TESTING.md test count 3618 -> 3627; test_verifier.py row updated from 135/1896 to 144/2061 with #520 narrative entry - ROADMAP.md test count 3618 -> 3627 - docs/llms-full.txt and docs/sitemap.xml regenerated to match Verification: - 144 verifier tests pass; 3514 full pytest suite pass; mypy clean - All 81 conformance programs pass; all 33 examples pass - Zero corpus migration needed: every existing @nat-nat site is guarded by `if @Nat.0 == 0 then base else recursive(@Nat.0 - 1)` and Z3 discharges from the path condition Co-Authored-By: Claude <noreply@anthropic.invalid>
Closes the codegen half of #520; with the verifier obligation from b446cac, both static and runtime sides of the soundness hole are now closed. Programs that run `vera verify` get a Tier-1 counterexample diagnostic; programs that skip verification trap cleanly via the runtime guard rather than silently producing a negative @nat value. vera/wasm/operators.py - _is_nat_subtraction: detects sites the guard must fire on. Mirrors the verifier's `_is_nat_typed AND _has_nat_origin` exactly so both layers agree on what counts as a guarded site. - _is_static_nat_typed / _has_nat_origin_codegen: AST-level helpers (no SmtContext required) recursing through SlotRef, IntLit, BinaryExpr, IfExpr, Block, MatchExpr, FnCall. Static type follows the checker's Nat <: Int subtyping rule (vera/checker/expressions.py:264-267); provenance distinguishes real @Nat-flowed values from pure-literal idioms. - _emit_nat_sub_guard: emits the WAT guard sequence using two fresh i64 locals (`alloc_local("i64")` for lhs/rhs). Pattern: `local.set $rhs; local.tee $lhs; local.get $rhs; i64.lt_s; if; unreachable; end; local.get $lhs; local.get $rhs; i64.sub`. - Wired into _translate_binary's BinOp.SUB path: when the helper matches, emit the guarded sequence; otherwise the bare `i64.sub` as before. The guard traps via bare `unreachable`, classified by vera/codegen/api.py:_classify_trap as `kind="unreachable"`. A dedicated `kind="underflow"` requires new host-import scaffolding mirroring `vera.contract_fail` and is left as a follow-up enhancement. Users who want a precise diagnostic should run `vera verify` first; the guard's role is preventing silent corruption, not perfect runtime diagnostics. tests/test_codegen.py — TestNatSubtractionRuntimeGuard520 - 6 tests: underflow traps at runtime, safe subtraction returns correct result, structural WAT assertions for @nat-nat (guard present) and @int-int / pure-literal (guard absent), deep recursion (path-discharged site) runs without spurious traps. - Test fixtures use `@Nat.1 - @Nat.0` (first - second) rather than `@Nat.0 - @Nat.1` so call-site `unsafe(0, 1)` reads naturally despite De Bruijn ordering — `@T.0` is the most recent (last) binding. tests/conformance/ch04_nat_subtraction.vera (new — Section 4.4 / 11.2.1) - Demonstrates the canonical discharge patterns: * Explicit `requires(@Nat.0 >= @Nat.1)`. * `if`-guarded recursion (path-condition discharge — the canonical `if @Nat.0 == 0 then base else recursive(@Nat.0 - 1)` shape used throughout examples and conformance). * Trivial discharges (`@Nat.0 - 0`, `@Nat.0 - @Nat.0`). * `@Nat - @int -> @Int` exclusion documenting the type-rule promotion that opts out of the obligation. - 17 contracts verify at Tier 1; runs to produce 7. - Manifest entry at level=run with feature tags nat_subtraction / underflow_obligation / tier1_discharge / path_condition_discharge / trivial_discharge. - Conformance corpus is now 82 programs (was 81). Doc count synchronisation: - TESTING.md: 3,627 -> 3,638 tests; 81 -> 82 conformance (4 occurrences); test_codegen.py 990 -> 996 / 13,705 -> 13,933 lines; test_conformance.py 405 -> 410; run-level 71 -> 72. - ROADMAP.md, CLAUDE.md, SKILL.md, AGENTS.md, FAQ.md: 81 -> 82 conformance. CHANGELOG.md - [Unreleased] entry under ### Fixed naming the soundness hole, the two-sided fix (verifier obligation + codegen guard), the Path-A scope choice (pure-literal exclusion), and the #551 / #552 follow-up trackers. Includes the migration note that zero corpus updates were required because every existing @nat-nat site is path-discharged. Verification: - 3525 tests pass; mypy clean; all 82 conformance + 33 examples pass. The codegen guard agrees with the verifier on which sites count as @nat-nat subtraction; both layers exclude pure-literal idioms (Path B / #552 territory). Co-Authored-By: Claude <noreply@anthropic.invalid>
Cuts the v0.0.127 release covering #520 (the @nat subtraction soundness hole) and clears six stale references to bugs that shipped in earlier releases — #515 (closed v0.0.122), #516 Stages 2-3 (closed v0.0.124-125), and #517 (closed v0.0.126). The campaign tracker in ROADMAP.md now reflects the post-#520 state: eight bugs remain (was nine), with the priority table reordered so #475 takes row 1 and #551 (the @byte sibling of #520) takes row 2 as a now-mechanical follow-up. Version files (4-file sync via scripts/check_version_sync.py): - vera/__init__.py: 0.0.126 -> 0.0.127 - pyproject.toml: 0.0.126 -> 0.0.127 - README.md: project-status line + release / test counts - docs/index.html: header version badge CHANGELOG.md - Insert [0.0.127] - 2026-04-29 header above the existing Unreleased content (which becomes the v0.0.127 entry). - Add [0.0.127] link reference; bump [Unreleased] base to v0.0.127. HISTORY.md - One-line v0.0.127 row under the Stage 9 release table: "**`@Nat` subtraction soundness hole closed** ([#520]())." Single bolded outcome clause, single link, no em-dashes — per the hardened HISTORY-row discipline (detail belongs in CHANGELOG). KNOWN_ISSUES.md - Remove the #520 row from the Bugs table (closed by this PR). The #552 limitation row keeps its "generalisation of #520 (which fixes the subtraction-specific subset)" cross-reference so triagers can see why #552 exists as a separate ticket without re-reading the soundness story. Limitation count drops 33 -> 32. ROADMAP.md - "Where we are" intro: v0.0.126 -> v0.0.127. - Campaign tracker paragraph: append v0.0.127 closing #520, drop count from "nine remain" to "eight remain", remove #520 from the inline list and from the #520+#551 cluster note. - Priority table: row 1 was #520+#551 cluster (soundness), now becomes #475 (translator bugs); #551 moves to row 2 with a "now mechanical" note explaining that the verifier helper + codegen guard from #520 generalise directly. - "By the numbers" footer: 126 releases (as of v0.0.126) -> 127 releases (as of v0.0.127). SKILL.md (Known Bugs and Workarounds table) - Drop #515 row: $gc_collect bounds-checking shipped v0.0.122, no longer a current bug. - Drop #516 Stages 2-3 row: source backtrace shipped v0.0.124, per-kind Fix paragraphs shipped v0.0.125. Both stages closed. - Drop #517 "no tail-call optimization" row: v0.0.126 shipped TCO for non-allocating functions. Replace with #549 row documenting the residual ("TCO disabled for allocating functions") so agents know the remaining failure mode and the workaround (allocate outside the recursion or iterate via array_fold). - Update the diagnostic concluding paragraph: drop the #515 / #517 / #516-Stages-2-3 trap-shape pointers, add #549 for the allocating-tail-recursion case. Reframe "trap message tells you the what but not the where" as the now-shipped reality: every trap carries kind + Fix paragraph + source backtrace. Site-asset regeneration: - docs/SKILL.md, docs/index.md, docs/llms-full.txt, docs/llms.txt rebuilt automatically by the site-assets pre-commit hook to pick up the SKILL.md content drift. Verification: - All four pre-commit gates green: version sync (0.0.127 across 4 files), doc counts (3638 tests / 29 files / 82 conformance / 33 examples / 25 hooks / 7 CI jobs), limitation sync (32 KNOWN_ISSUES / 15 vera/README / 2 spec chapters), mypy. Co-Authored-By: Claude <noreply@anthropic.invalid>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
==========================================
- Coverage 91.06% 90.92% -0.14%
==========================================
Files 59 59
Lines 22178 22388 +210
Branches 259 259
==========================================
+ Hits 20196 20356 +160
- Misses 1975 2025 +50
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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:
📝 WalkthroughWalkthroughImplements fix for issue Changes
Sequence Diagram(s)sequenceDiagram
participant Verifier as Verifier
participant PathCond as PathConditions
participant SMT as SMT Solver (Z3)
participant Codegen as Codegen/WASM
Note over Verifier: Encounter `@Nat - `@Nat`` expression
Verifier->>PathCond: Read active path conditions (if/match, requires)
PathCond-->>Verifier: Return path constraints
Verifier->>SMT: check_valid(lhs >= rhs, caller_preconds + path_constraints)
alt Obligation discharged (Tier‑1)
SMT-->>Verifier: Valid
Verifier->>Verifier: Increment tier1_verified
Verifier->>Codegen: Emit direct i64.sub
else Obligation fails
SMT-->>Verifier: Counterexample
Verifier->>Verifier: Emit E502 diagnostic
Verifier->>Codegen: Emit guarded subtraction
Codegen->>Codegen: WAT: (if (i64.lt_s lhs rhs) (then unreachable)) i64.sub
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 77: The doc shows inconsistent example counts: update the project-layout
entry that currently reads "32 examples" to the correct count matching the
conformance line "`tests/conformance/` — 82 conformance programs" (i.e., change
"32" → "82") so both entries agree; locate the string "32 examples" (or the
"project-layout" bullet near it) in CLAUDE.md and edit the numeric count, then
re-run any doc checks to confirm the Markdown now matches the repo validation
scripts.
In `@HISTORY.md`:
- Line 264: The HISTORY.md update added the v0.0.127 release row but left the
summary footer roll-up unchanged; update the footer count that currently reads
"126 tagged releases" to "127 tagged releases" so the release total matches the
new v0.0.127 entry (verify the footer text near the file bottom and the new
release line "v0.0.127" are consistent).
In `@KNOWN_ISSUES.md`:
- Line 23: Update the KNOWN_ISSUES.md entry to avoid claiming a general "Tier-3
fallback" exists: change the wording to state that only the subtraction-specific
runtime guard implemented in vera/wasm/operators.py (the `#520` pattern) is
currently present, while broader let/argument `@Int-`>@Nat narrowing remains
unimplemented and tracked as future work in vera/verifier.py (issue `#552`);
ensure the text clearly ties the runtime protection to operators.py and not to a
project-wide Tier-3 fallback.
In `@ROADMAP.md`:
- Line 25: The ROADMAP wording overstates the fix by implying every `@Nat -
`@Nat`` subtraction is guarded; update the sentence to clarify the
verifier/runtime change only applies to subtractions where operands carry Nat
provenance (i.e., non-pure-literal cases), and that the verifier now emits a
Tier-1 `lhs >= rhs` proof obligation and the codegen emits a runtime guard only
for those provenance-bearing `@Nat - `@Nat`` sites (pure literals like `0 - 1`
without Nat provenance are exempt).
In `@SKILL.md`:
- Around line 2294-2298: Remove the misleading "recurse over an `@Int`
accumulator" workaround from the stack-exhaustion guidance and replace it with
the canonical remediation: recommend using array_fold / array_map (which compile
to WASM loops), splitting large recursion into bounded chunks of <5K frames, or
waiting for the GC-aware return_call fix (issue `#517` / `#549`); update the
sentence that currently reads "recurse over an `@Int` accumulator" to list only
those three remedies and ensure references to `array_fold`, `array_map`, and
`return_call` remain for clarity.
In `@spec/04-expressions.md`:
- Line 50: The spec currently requires underflow checks for both Nat and Byte
but the implementation in vera/verifier.py and vera/wasm/operators.py only
enforces Nat subtraction checks; update the spec text in spec/04-expressions.md
(the sentence about subtraction on unsigned types) to scope the MUST/SHOULD
requirement to Nat only (remove Byte) or alternatively note that Byte underflow
is not currently checked by the verifier, aligning the normative language with
the implementation; reference the Nat-specific behavior already present in
vera/verifier.py and vera/wasm/operators.py so readers know which type is
covered.
In `@spec/11-compilation.md`:
- Around line 46-55: Update the spec text for Nat-subtraction checks to match
implementation: change the statement that obligations are emitted at “every
`@Nat - `@Nat`` site” to explicitly note that the compiler omits obligations for
pure literal operands and only emits them when the result has Nat provenance
(i.e., non-literal or derived Nat slots), and replace the runtime trap
annotation `kind="underflow"` with `kind="unreachable"` to reflect the current
classification; keep the rest of the paragraph about guarded subtraction and the
note about `@Byte`/return checks intact.
In `@TESTING.md`:
- Line 9: The Tests summary row in TESTING.md is inconsistent: the table
currently reads "| **Tests** | 3,638 across 29 files (~34,500 lines of test
code; 3,613 passed, 14 skipped) |" but 3,613 + 14 = 3,627; update the table row
so the total and breakdown reconcile (either change the total from 3,638 to
3,627 or adjust the passed/skipped numbers to match 3,638). Edit the "|
**Tests** | ..." line accordingly to ensure the parenthetical breakdown sums to
the reported total.
In `@tests/test_codegen.py`:
- Around line 13902-13933: Add an explicit structural check that the generated
lowering for countdown still includes the runtime guard for the subtraction
rather than just relying on _run passing: after invoking the codegen for
test_recursion_with_path_guard_runs_clean (or by inspecting the lowered WAT for
the function named countdown produced by the same pipeline used by _run), assert
that the countdown body contains the guarded subtraction pattern (e.g., the
emitted lhs>=rhs or the specific guard instruction sequence your backend emits)
so the test fails if the guard is accidentally removed while the runtime
behavior remains 0.
In `@tests/test_verifier.py`:
- Around line 553-566: Update the class docstring for
TestNatSubtractionObligation520 to match the test behavior by adding the
provenance/literal exception: state that `@Nat - `@Nat`` sites generally require
proof of `lhs >= rhs` but exempt pure-literal (provenance) subtraction cases
(for example `0 - 1`) which the verifier treats specially; ensure the updated
text references the class name TestNatSubtractionObligation520 and the `lhs >=
rhs` obligation so readers can find the related tests and logic.
In `@vera/verifier.py`:
- Around line 871-1200: The walker and Nat classifiers currently only handle
ast.FnCall and thus miss module-qualified calls; update
_walk_for_subtraction_obligations to also descend into the module-qualified call
node (treat its args and return like ast.FnCall), and update _is_nat_typed and
_has_nat_origin to recognise module-qualified calls by resolving the callee with
the existing _lookup_module_function (or similar module-function resolver) and
then using _is_nat_type(fn.return_type) / _is_nat_type on the resolved function
return type (and _is_nat_origin logic for its provenance). Ensure you reference
and reuse the same resolution path used elsewhere (the _lookup_module_function
helper) so cross-module calls produce the same Nat typing/origin behaviour as
plain FnCall.
In `@vera/wasm/operators.py`:
- Around line 728-832: The helpers miss imported module function calls because
they only handle ast.FnCall; update _is_static_nat_typed and
_has_nat_origin_codegen (which feed _is_nat_subtraction) to also recognise and
treat imported/callable expressions that represent cross-module function calls
(e.g. qualified/imported calls where the AST node isn't ast.FnCall but has a
callee/qualified name) by invoking _infer_fncall_vera_type for those call-like
nodes (or by extracting the callee and calling the existing
_infer_fncall_vera_type) and returning True when that returns "Nat" so imported
`@Nat` functions are treated the same as ast.FnCall for both static-typing and
provenance checks.
🪄 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
Run ID: 0dd29930-8ea7-45f9-a958-5b1d1b1cd558
⛔ Files ignored due to path filters (7)
docs/SKILL.mdis excluded by!docs/**docs/index.htmlis excluded by!docs/**docs/index.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**docs/llms.txtis excluded by!docs/**docs/sitemap.xmlis excluded by!docs/**tests/conformance/ch04_nat_subtraction.verais excluded by!**/*.vera
📒 Files selected for processing (21)
AGENTS.mdCHANGELOG.mdCLAUDE.mdFAQ.mdHISTORY.mdKNOWN_ISSUES.mdREADME.mdROADMAP.mdSKILL.mdTESTING.mdpyproject.tomlscripts/check_spec_examples.pyspec/04-expressions.mdspec/11-compilation.mdtests/conformance/manifest.jsontests/test_codegen.pytests/test_verifier.pyvera/__init__.pyvera/errors.pyvera/verifier.pyvera/wasm/operators.py
Eleven Rabbit findings actioned plus one CI failure unblocked. Each finding was verified against the current code; small refinements where the underlying point was real but the suggested wording was imprecise. CI fix - uv.lock regenerated. v0.0.127 bump in pyproject.toml left the lockfile pinned at 0.0.126; lint job ran `uv lock --check` and failed. `uv lock` produces a one-line update (vera 0.0.126 -> 0.0.127); no transitive dependency churn. Documentation accuracy - CLAUDE.md project-layout: "32 example Vera programs" -> "33". (Rabbit suggested 32 -> 82, conflating examples with conformance; the actual stale figure was 32 vs the live 33.) - HISTORY.md "By the numbers" table: rightmost column updated from v0.0.126 (28 Apr) to v0.0.127 (29 Apr) with the new metrics (3,638 tests, 82 conformance, 33 examples). Total releases 126 -> 127; active development days 48 -> 49. - KNOWN_ISSUES.md #552 entry: rephrased to drop the misleading "Tier-3 fallback compiles correctly" claim. There is no general Tier-3 fallback — only #520's subtraction-specific guard in vera/wasm/operators.py. The new wording ties the runtime protection explicitly to the operators.py guard and notes that let / argument narrowing remains unimplemented at both static and runtime layers (issue #552). - ROADMAP.md campaign tracker: clarified that v0.0.127 only guards `@Nat - @Nat` sites with `@Nat` *provenance* (slot ref or function return), not pure-literal idioms like `0 - 1`. Mirrors the Path-A scope choice and points readers at #552 for the binding-site generalisation. - TESTING.md Tests row: pass/skip math reconciled. Total reads 3,638; previous breakdown was 3,613 + 14 = 3,627 (off by 11). Updated to 3,624 + 14 = 3,638. Spec / implementation alignment - spec/04-expressions.md §4.4 underflow clause: scoped the normative MUST/SHOULD to `Nat` only. Previous wording included `Byte`, but the implementation in vera/verifier.py and vera/wasm/operators.py only enforces `Nat`; `Byte` is tracked as #551 (now explicitly named in the spec text). - spec/11-compilation.md §11.2.1: two corrections. (a) Trap kind classification updated `kind="underflow"` -> `kind="unreachable"` to match the actual implementation (the bare `unreachable` instruction is classified as `kind="unreachable"` by vera/codegen/api.py:_classify_trap; a dedicated `underflow` kind requires new host-import scaffolding mirroring `vera.contract_fail` and is left as a future enhancement). (b) "At every such site" softened to "At every such site whose result is statically @nat AND at least one operand has @nat provenance" — pure-literal subtractions like `0 - 1` are intentionally exempt at Path-A scope, so the spec now matches what the verifier and codegen actually do. Verifier and codegen completeness - vera/verifier.py: `_walk_for_subtraction_obligations` now descends into `ast.ModuleCall` args (mirroring its `ast.FnCall` handling) so `@Nat - @Nat` sites inside `Foo.bar(arg)` are not silently skipped. `_is_nat_typed` and `_has_nat_origin` resolve `ast.ModuleCall` callees via the existing `_lookup_module_function` registry and check the resolved `FunctionInfo.return_type` — so a module-qualified call returning `@Nat` is treated identically to an unqualified `ast.FnCall` for both static-typing and provenance purposes. - vera/wasm/operators.py: `_is_static_nat_typed` and `_has_nat_origin_codegen` now handle `ast.ModuleCall` by synthesizing the equivalent `ast.FnCall(name=expr.name, args=expr.args)` and feeding it through the existing `_infer_fncall_vera_type`. Matches the desugar that vera/wasm/context.py:translate_expr already performs at line 315 (ModuleCall -> flat FnCall via flattened import names), so the codegen guard fires on the same set of sites the verifier obligates. Test coverage - tests/test_codegen.py `test_recursion_with_path_guard_runs_clean`: added a structural WAT assertion that the `@Nat.0 - 1` site inside `countdown` actually emits the `i64.lt_s` + `unreachable` guard. Pre-fix the test was coverage-blind: the path condition guarantees `@Nat.0 >= 1` in the recursive arm, so the runtime guard never fires whether or not it's emitted, and the behavioural assertion (`countdown(100) == 0`) would pass even if the codegen detector silently skipped path-discharged sites. - tests/test_verifier.py `TestNatSubtractionObligation520` class docstring: extended to name the provenance-bearing requirement and the pure-literal exemption explicitly, with a cross-reference to `test_pure_literal_subtraction_not_flagged` and the Path-B / #552 binding-site generalisation. SKILL.md "Known Bugs" #549 row workaround text: - Replaced the "@int accumulator" hint with the canonical remediation list: prefer `array_fold` / `array_map` (which compile to WASM loops with no per-iteration frame cost) for most cases; fall back to splitting the recursion into bounded chunks of <5K frames; or wait for the GC-aware `return_call` follow-up tracked by #549. The accumulator hint was misleading because most accumulator shapes carrying heap data are themselves allocating, so they don't actually escape the bug. Verification - 3,525 tests pass; mypy clean; doc-counts / limitations-sync / version-sync / spec-examples all green; all 82 conformance + 33 examples still pass. Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_codegen.py (1)
13712-13959:⚠️ Potential issue | 🟠 MajorAdd a ModuleCall-provenance regression for the Nat-underflow guard path.
This suite covers direct
@Natprovenance (slot refs/literals) well, but it does not exercise provenance flowing through a module-qualified call. A regression in theModuleCallpath could re-open the soundness hole while these tests remain green. Please add one case that sources a@Natoperand via a qualified call and assert both structural guard emission and runtime behaviour.As per coding guidelines:
tests/test_codegen.py: Add codegen/runtime tests intests/test_codegen.pyfor built-in functions — cover normal cases, edge cases (empty inputs, zero values), and composition with other built-ins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 13712 - 13959, Add a new test in the TestNatSubtractionRuntimeGuard520 class that exercises a ModuleCall provenance: create a small source that defines a module-scoped helper (e.g. fn m::nat_prov() -> `@Nat`) or a module-qualified call that returns a `@Nat` and then performs `@Nat - `@Nat`` where one operand comes from that qualified call; use the same helpers as the existing tests (_compile_ok, _run, execute) to assert runtime behaviour (trap on unsafe underflow and correct result on safe case) and a structural WAT assertion mirroring test_guard_emitted_in_wat_for_nat_sub / test_recursion_with_path_guard to check the guarded function body contains both "i64.lt_s" and "unreachable" (and for an Int-like control case assert absence), naming the new test clearly (e.g. test_modulecall_provenance_emits_guard_and_traps) and following the established patterns for slicing the WAT body and asserting presence/absence of the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TESTING.md`:
- Line 11: Update the prose totals in TESTING.md to match the new conformance
program count: replace the occurrence that still says "81" with "82" (the prose
phrase referencing the collection count, e.g., "a collection of N small, focused
programs") and update the pytest count that currently reads "405 tests" to "410
tests" (the sentence like "Via pytest (parametrized — N tests)") so all ungated
prose reflects 82 programs × 5 tests = 410 tests.
In `@vera/verifier.py`:
- Around line 871-1032: The walker _walk_for_subtraction_obligations misses
ast.ConstructorCall arguments (so subtractions like Just(`@Nat.0` - `@Nat.1`) aren't
recursed); add a branch analogous to the ast.FnCall / ast.ModuleCall cases:
detect isinstance(expr, ast.ConstructorCall) and for each arg call
self._walk_for_subtraction_obligations(decl, arg, smt, slot_env, assumptions) so
those argument expressions are inspected and _check_subtraction_obligation can
catch `@Nat-`@Nat sites.
---
Outside diff comments:
In `@tests/test_codegen.py`:
- Around line 13712-13959: Add a new test in the
TestNatSubtractionRuntimeGuard520 class that exercises a ModuleCall provenance:
create a small source that defines a module-scoped helper (e.g. fn m::nat_prov()
-> `@Nat`) or a module-qualified call that returns a `@Nat` and then performs `@Nat
- `@Nat`` where one operand comes from that qualified call; use the same helpers
as the existing tests (_compile_ok, _run, execute) to assert runtime behaviour
(trap on unsafe underflow and correct result on safe case) and a structural WAT
assertion mirroring test_guard_emitted_in_wat_for_nat_sub /
test_recursion_with_path_guard to check the guarded function body contains both
"i64.lt_s" and "unreachable" (and for an Int-like control case assert absence),
naming the new test clearly (e.g.
test_modulecall_provenance_emits_guard_and_traps) and following the established
patterns for slicing the WAT body and asserting presence/absence of the guard.
🪄 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
Run ID: e31dbd2c-bde6-476c-8943-f62fb097596c
⛔ Files ignored due to path filters (3)
docs/SKILL.mdis excluded by!docs/**docs/llms-full.txtis excluded by!docs/**uv.lockis excluded by!**/*.lock,!uv.lock
📒 Files selected for processing (12)
CLAUDE.mdHISTORY.mdKNOWN_ISSUES.mdROADMAP.mdSKILL.mdTESTING.mdspec/04-expressions.mdspec/11-compilation.mdtests/test_codegen.pytests/test_verifier.pyvera/verifier.pyvera/wasm/operators.py
Two more in-diff findings and one out-of-diff suggestion, all real. vera/verifier.py - _walk_for_subtraction_obligations now recurses into the args of ast.ConstructorCall and ast.QualifiedCall, in addition to the existing ast.FnCall and ast.ModuleCall branches. Previously `Some(@Nat.0 - @Nat.1)` would slip past the obligation walker because the constructor call's children weren't visited; now they are. ConstructorCall results are ADT-typed (not @nat), so _is_nat_typed / _has_nat_origin don't need branches for them. QualifiedCall is similar (e.g. `Map.get(...)`); the SMT layer doesn't translate QualifiedCall itself, so any obligation rooted ON one would already drop to Tier 3, but recursing here catches obligations rooted INSIDE the args. tests/test_codegen.py - New test_modulecall_provenance_emits_guard_and_traps in TestNatSubtractionRuntimeGuard520 (out-of-diff suggestion from Rabbit, real value-add). Exercises the round-1 ModuleCall fix end-to-end: compiles a function using `vera.math::abs(@Int.1) - vera.math::abs(@Int.0)` (a @nat - @nat site where both operands have @nat provenance via ast.ModuleCall, not ast.FnCall), asserts the codegen emits the i64.lt_s + unreachable guard in the WAT, asserts the runtime traps cleanly when called with arguments that underflow (`unsafe_modcall(0, 5)` → `abs(0) - abs(5)`), and asserts the safe case (`safe_modcall(5, 3)` → `abs(5) - abs(3) = 2`) runs cleanly through the guard. Pinning the De Bruijn ordering in comments since the test relies on `@Int.1` being the first parameter. TESTING.md - Updated two stale prose / code-block totals: "a collection of 81 small, focused programs" → "82" (matches the new conformance count from the v0.0.127 ch04_nat_subtraction.vera entry); "Via pytest (parametrized — 405 tests)" → "410 tests" (matches the 82 programs × 5 tests-per-program = 410 product). - Bumped test_codegen.py row counts (996 → 997, 13,958 → 14,039) and the overall Tests metric (3,638 → 3,639, passed 3,624 → 3,625) for the new test_modulecall_provenance test. ROADMAP.md - 3,638 → 3,639 in the "Where we are" intro paragraph and the By the Numbers footer (kept in sync with TESTING.md / live pytest count). Verification: - 16 #520 tests pass; mypy clean; doc-counts / limitations-sync / version-sync all green. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vera/verifier.py (1)
420-438:⚠️ Potential issue | 🟠 MajorDo not let the generic-function fast path bypass E502.
Line 420 returns before Line 525 ever runs, so
forall<T>functions with an otherwise ordinary@Nat -@nat:@Nat`` site never get the new verifier-side underflow check. An unrelated type parameter is enough to suppress the Tier-1 half of#520, which weakens the verifier-first guarantee to runtime trapping only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vera/verifier.py` around lines 420 - 438, The current fast path returns when decl.forall_vars is true, skipping the verifier-side underflow check that emits E502; change the logic so generic functions still run the underflow verification for each contract before returning (either by moving the E502-checking code to run prior to the early return or by invoking the same underflow-check routine that non-generic functions use). Specifically, in the block that handles decl.forall_vars and iterates decl.contracts (and uses self._is_trivial / self._warning), ensure you also call the verifier underflow check that can raise E502 for `@Nat-like` sites (or reuse the existing function that performs the E502 check) so that generic functions do not bypass E502 while still recording tier3_runtime where appropriate.
♻️ Duplicate comments (1)
tests/test_codegen.py (1)
13847-13854:⚠️ Potential issue | 🟡 MinorHarden the negative controls against comparator swaps.
Both tests only ban
i64.lt_s. If the guard regresses toi64.lt_u, or leaves only the trap marker behind, these assertions still pass while the body is still guarded. Assert absence of both the compare andunreachablehere.As per coding guidelines: `tests/**/*.py`: These are pytest test files. Review for correctness and coverage gaps.Suggested hardening
- assert "i64.lt_s" not in body, ( - f"Unexpected `i64.lt_s` in int_sub body — Int subtraction " + assert not re.search(r"i64\.lt_[su]", body), ( + f"Unexpected underflow-guard compare in int_sub body — Int subtraction " f"should not have an underflow guard. Body: {body!r}" ) + assert "unreachable" not in body, ( + f"Unexpected `unreachable` in int_sub body — Int subtraction " + f"should not trap. Body: {body!r}" + ) @@ - assert "i64.lt_s" not in body, ( - f"Pure-literal `0 - 1` should not get a guard at Path-A " + assert not re.search(r"i64\.lt_[su]", body), ( + f"Pure-literal `0 - 1` should not get an underflow-guard compare at Path-A " f"scope. Body: {body!r}" ) + assert "unreachable" not in body, ( + f"Pure-literal `0 - 1` should not emit a trap guard at Path-A " + f"scope. Body: {body!r}" + )Also applies to: 13893-13900
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 13847 - 13854, The test currently only asserts that "i64.lt_s" is not present in the generated body, which misses regressions that replace it with "i64.lt_u" or leave only a trap marker; update the assertions in the int_sub negative-control blocks (the ones checking for absence of underflow guard) to assert that none of "i64.lt_s", "i64.lt_u", nor the string "unreachable" appear in body (i.e., add checks like assert "i64.lt_u" not in body and assert "unreachable" not in body alongside the existing i64.lt_s check) and apply the same three-way absence checks to the other similar block around the later lines referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_codegen.py`:
- Around line 14014-14015: Update the stale comment to match the actual call
order used in main: it evaluates unsafe_modcall(0, 5), so change the explanation
to refer to unsafe_modcall(0, 5) and the resulting computation (abs(5) - abs(0)
= 5 - 0 = 5 / no underflow) instead of the reversed unsafe_modcall(5, 0); ensure
the comment near the test referencing unsafe_modcall and main reflects the
correct argument order.
---
Outside diff comments:
In `@vera/verifier.py`:
- Around line 420-438: The current fast path returns when decl.forall_vars is
true, skipping the verifier-side underflow check that emits E502; change the
logic so generic functions still run the underflow verification for each
contract before returning (either by moving the E502-checking code to run prior
to the early return or by invoking the same underflow-check routine that
non-generic functions use). Specifically, in the block that handles
decl.forall_vars and iterates decl.contracts (and uses self._is_trivial /
self._warning), ensure you also call the verifier underflow check that can raise
E502 for `@Nat-like` sites (or reuse the existing function that performs the E502
check) so that generic functions do not bypass E502 while still recording
tier3_runtime where appropriate.
---
Duplicate comments:
In `@tests/test_codegen.py`:
- Around line 13847-13854: The test currently only asserts that "i64.lt_s" is
not present in the generated body, which misses regressions that replace it with
"i64.lt_u" or leave only a trap marker; update the assertions in the int_sub
negative-control blocks (the ones checking for absence of underflow guard) to
assert that none of "i64.lt_s", "i64.lt_u", nor the string "unreachable" appear
in body (i.e., add checks like assert "i64.lt_u" not in body and assert
"unreachable" not in body alongside the existing i64.lt_s check) and apply the
same three-way absence checks to the other similar block around the later lines
referenced.
🪄 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
Run ID: 80960775-1d8f-474c-8513-4cdf02b3346b
📒 Files selected for processing (4)
ROADMAP.mdTESTING.mdtests/test_codegen.pyvera/verifier.py
✅ Actions performedComments resolved. Approval is disabled; enable |
…ertions Single-finding round. CodeRabbit pushed back on the round-2 ModuleCall reply with a concrete strengthening for the two negative control tests: ban both `i64.lt_s` AND `i64.lt_u` (signed and unsigned compare variants) AND assert `unreachable` is absent. The previous `"i64.lt_s" not in body` substring check would have silently passed if the codegen ever switched to unsigned comparison or a different compare-then-trap variant. tests/test_codegen.py - test_int_subtract_emits_no_guard: replaced `assert "i64.lt_s" not in body` with `assert not re.search(r"\bi64\.lt_[su]\b", body)` and added `assert "unreachable" not in body`. - test_pure_literal_subtract_emits_no_guard: same hardening. - Inline comments document why both bans matter together: the comparator could change (signed -> unsigned) without breaking the substring check, and the trap mechanism could change (unreachable -> a host-import call) without breaking the comparator check. Banning both keeps the test future-proof. - `re` is already imported at the top of the file (line 14), so this is drop-in. Verification: - All 7 TestNatSubtractionRuntimeGuard520 tests still pass; the positive cases (`test_underflow_traps_at_runtime`, `test_guard_emitted_in_wat_for_nat_sub`, `test_modulecall_provenance_emits_guard_and_traps`) all still see the expected `i64.lt_s` + `unreachable` guard sequence. Co-Authored-By: Claude <noreply@anthropic.invalid>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_codegen.py (1)
14031-14032:⚠️ Potential issue | 🟡 MinorFix the stale call-order comment.
The comment says
unsafe_modcall(5, 0), butmainactually callsunsafe_modcall(0, 5). In a De Bruijn-sensitive test, that mismatch is easy to cargo-cult.Minimal fix
- # Behavioural assertion: unsafe_modcall(5, 0) produces + # Behavioural assertion: unsafe_modcall(0, 5) produces🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 14031 - 14032, The comment incorrectly references unsafe_modcall(5, 0) while main actually calls unsafe_modcall(0, 5); update the behavioural assertion comment to reflect the real call order (unsafe_modcall(0, 5)) and correct the expected expression/result (abs(5) - abs(0) = 5 - 0 = 5, not an underflow/trap). Locate the call site in main and the comment near the test assertion and revise the text accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_codegen.py`:
- Around line 13712-14056: Add a new test that exercises the rhs-only-provenance
subtraction shape (literal left operand minus a `@Nat` slot) so the codegen helper
`_has_nat_origin_codegen` can't accidentally ignore the right operand; in the
TestNatSubtractionRuntimeGuard520 class add a case similar to `_GUARDED_SUB` /
`test_guard_emitted_in_wat_for_nat_sub` that uses a body like `0 - `@Nat.0`` (or a
wrapper fn that calls a private unsafe fn with that body), then assert the WAT
for that function contains both "i64.lt_s" and "unreachable" and that executing
the program with arguments that cause underflow (e.g. passing 1 where rhs is 1)
raises a trap using the same patterns/assertions as existing tests (reuse
function names such as `unsafe`, `unsafe_modcall` idiom and runtime checks with
pytest.raises and execute/_run).
---
Duplicate comments:
In `@tests/test_codegen.py`:
- Around line 14031-14032: The comment incorrectly references unsafe_modcall(5,
0) while main actually calls unsafe_modcall(0, 5); update the behavioural
assertion comment to reflect the real call order (unsafe_modcall(0, 5)) and
correct the expected expression/result (abs(5) - abs(0) = 5 - 0 = 5, not an
underflow/trap). Locate the call site in main and the comment near the test
assertion and revise the text accordingly.
🪄 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
Run ID: df4b7843-fb4d-41c5-844f-0d8590160a1c
📒 Files selected for processing (2)
TESTING.mdtests/test_codegen.py
…low-up #555 Three findings verified. One real fix (stale comment), one already-addressed duplicate (round 3 already handled it), and one real-but-out-of-scope soundness gap filed as #555 follow-up. tests/test_codegen.py - Stale Python-side comment in test_modulecall_provenance_emits_guard _and_traps fixed: round-2 updated the Vera-side comment in `main` to say `unsafe_modcall(0, 5)` (the corrected De Bruijn-aware call order) but missed the explanatory Python comment immediately above the `pytest.raises` block, which still said `unsafe_modcall(5, 0)`. Updated to match the actual call order and added an inline reminder that `@Int.1` is the first parameter (older / De Bruijn = 1) and `@Int.0` is the second (most-recent) so the test's De Bruijn-sensitive arg-vs-slot mapping reads cleanly. KNOWN_ISSUES.md - New Limitation entry tracking #555: generic (forall<T>) functions bypass the E502 underflow obligation at verify time because the verifier's early return for forall_vars skips _walk_for_subtraction_obligations along with all other static contract checks. Consistent with how E520-E525 all skip generics today; the codegen guard catches it post-monomorphization at runtime so the safety net is in place — only the static check is missing. #555 was filed via gh issue create; the PR's inline reply explains why the fix is non-trivial (SMT context setup needs concrete-typed params; type-variable params can't be declared as Z3 sorts) and why I'm scoping it out of #520 — it inherits the convention used by every other verifier check on generic functions, and the runtime guard at codegen time already provides safety. Round 4 ALSO had a "duplicate" finding asking us to harden test_int_subtract_emits_no_guard and test_pure_literal_subtract _emits_no_guard with `re.search(r"\bi64\.lt_[su]\b", body)` and `assert "unreachable" not in body`. Already addressed in round 3 commit 488deff; the inline reply points at that commit. Verification: - All 7 TestNatSubtractionRuntimeGuard520 tests pass. - Limitation tracking consistent (33 in KNOWN_ISSUES.md / 15 in vera/README.md / 2 across spec chapters). - mypy clean. Co-Authored-By: Claude <noreply@anthropic.invalid>
|
@coderabbitai Verified — this is a real gap, but I'm scoping it as out of #520 because the fix is non-trivial and the codegen guard already catches it at runtime. Filed as #555 for the follow-up. Two reasons for keeping the static skip in this PR:
Convention-wise this matches how E520 / E521 / E523 / E524 / E525 all bail out for generic functions today; #520's E502 inherits the same convention. #555 tracks extending E502 (and ideally the others) to generic-function bodies. |
|
@coderabbitai Already addressed in round-3 commit |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes.
The convention alignment with E520/E521/E523/E524/E525 (all bailing on generic functions today) is the right reference point. ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_codegen.py (1)
13712-14059:⚠️ Potential issue | 🟠 MajorAdd a rhs-only provenance regression test (
0 -@Nat.0``).The suite still misses the asymmetric rhs-only provenance path. If
_has_nat_origin_codegenever regresses to lhs-only checking, this class can still pass while shipping an unguarded underflow site.Proposed test addition
+ def test_rhs_only_nat_provenance_emits_guard_and_traps(self) -> None: + """`0 - `@Nat.0`` must still emit the underflow guard.""" + src = """ +private fn rhs_only(`@Nat` -> `@Nat`) + requires(true) ensures(true) effects(pure) +{ + 0 - `@Nat.0` +} + +public fn main(`@Unit` -> `@Nat`) + requires(true) ensures(true) effects(pure) +{ + rhs_only(1) +} +""" + result = _compile_ok(src) + wat = result.wat + fn_idx = wat.find("(func $rhs_only") + assert fn_idx >= 0 + body_end = wat.find("\n (func ", fn_idx + 1) + if body_end < 0: + body_end = len(wat) + body = wat[fn_idx:body_end] + assert "i64.lt_s" in body and "unreachable" in body + with pytest.raises((wasmtime.WasmtimeError, wasmtime.Trap, RuntimeError)): + execute(result, fn_name="main", args=[])As per coding guidelines: “DO flag: Missing edge cases for new compiler features.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_codegen.py` around lines 13712 - 14059, Add a new test in TestNatSubtractionRuntimeGuard520 that covers the asymmetric rhs-only provenance case (literal lhs, provenanced rhs) so regressions that check only the lhs don't slip; create a small src where the body computes `0 - `@Nat.0`` (eg. private fn unsafe_rhs_only(`@Nat` -> `@Nat`) { 0 - `@Nat.0` } and public main calling unsafe_rhs_only(1) to cause underflow), then assert structurally that the compiled WAT for the function (find "(func $unsafe_rhs_only") contains both "i64.lt_s" and "unreachable" and assert behaviourally that executing main traps (wrap call in pytest.raises like other tests) and add a safe case (e.g. call with 0) to verify non-underflow returns cleanly; place the test method alongside the existing tests in the same class so it covers the missing edge-case for _has_nat_origin_codegen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_codegen.py`:
- Around line 13712-14059: Add a new test in TestNatSubtractionRuntimeGuard520
that covers the asymmetric rhs-only provenance case (literal lhs, provenanced
rhs) so regressions that check only the lhs don't slip; create a small src where
the body computes `0 - `@Nat.0`` (eg. private fn unsafe_rhs_only(`@Nat` -> `@Nat`) { 0
- `@Nat.0` } and public main calling unsafe_rhs_only(1) to cause underflow), then
assert structurally that the compiled WAT for the function (find "(func
$unsafe_rhs_only") contains both "i64.lt_s" and "unreachable" and assert
behaviourally that executing main traps (wrap call in pytest.raises like other
tests) and add a safe case (e.g. call with 0) to verify non-underflow returns
cleanly; place the test method alongside the existing tests in the same class so
it covers the missing edge-case for _has_nat_origin_codegen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 04ab674c-b64a-49d9-bf9e-770acd2eceee
📒 Files selected for processing (3)
KNOWN_ISSUES.mdTESTING.mdtests/test_codegen.py
One real coverage gap; one duplicate finding already addressed in
round 4.
tests/test_codegen.py
- New test_rhs_only_provenance_emits_guard_and_traps in
TestNatSubtractionRuntimeGuard520. The previous tests pin three
provenance shapes:
* left-only: `@Nat.0 - 1` (test_recursion_with_path_guard)
* both: `@Nat.1 - @Nat.0` (test_underflow_traps_at_runtime)
* both via ModuleCall: `vera.math::abs(...) - vera.math::abs(...)`
(test_modulecall_provenance_emits_guard_and_traps)
But not RIGHT-only. Without that case, a future refactor that
accidentally ignored `expr.right` in `_has_nat_origin_codegen`
(or changed the `or` between the two helper calls to `and`)
would still pass every existing test while silently re-opening
the underflow hole for the right-only shape.
New test uses body `0 - @Nat.0` — non-negative IntLit on the
left, @nat slot on the right. Both operands are statically @nat
(per checker.py:62 — non-negative IntLit is @nat) but only the
slot has @nat *provenance*, so the `or` in
`_is_nat_subtraction` makes the subtraction-detector fire via
the rhs-only branch. Asserts:
* Structural: WAT body of `lit_minus_slot` contains both
`i64.lt_s` and `unreachable`.
* Behavioural (unsafe): `lit_minus_slot(1)` → `0 - 1` → trap.
* Behavioural (safe): `lit_minus_slot(0)` → `0 - 0 = 0`.
TESTING.md / ROADMAP.md
- Doc-counts bump for the new test:
* Tests headline: 3,639 → 3,640 across (3,626 passed, 14 skipped).
* test_codegen.py row: 997 → 998 / 14,059 → 14,131.
* ROADMAP.md by-the-numbers footer: 3,639 → 3,640.
The duplicate finding (stale comment about `unsafe_modcall(5, 0)`
on line 14031) was already corrected in round 4 commit d0ee770;
no further change needed. Inline reply on the PR points at d0ee770.
Verification:
- All 8 TestNatSubtractionRuntimeGuard520 tests pass; mypy clean;
doc-counts / limitations-sync / version-sync / spec-examples
all green.
Co-Authored-By: Claude <noreply@anthropic.invalid>
Closes #520. Spawns #551 (
@Bytesibling — now mechanical follow-up) and #552 (architectural generalisation:@Natinvariant check at every binding site, not just subtraction).The bug
The type system accepted
@Nat - @Nat : @Natbut the runtime emitted a plaini64.subwith no underflow check, so a negative i64 could end up in a@Natslot. Any Tier-1-verified contract relying onNat >= 0was logically undermined, and@Array<T>[@Nat.0]indexing with a bad@Natbecame a memory-safety issue. The refinement-type layer's central promise — "this value is non-negative" — didn't hold.Before / after
Before (v0.0.126):
After (v0.0.127),
vera verify:After (v0.0.127),
vera runskipping verification:Two-sided implementation
The fix has a static half (verifier) and a runtime half (codegen) that share their detection logic exactly so both layers agree on what counts as a guarded site:
Static — verifier proof obligation
vera/verifier.pywalks the function body via_walk_for_subtraction_obligations(mirrors_walk_for_callsstructurally; descends throughIfExpr/MatchExprwithsmt._path_conditionspush/pop,Blockwith let-binding env updates). At everyBinaryExpr.SUBwhose result is statically@NatAND at least one operand has@Natprovenance, it emits a Tier-1 obligationlhs >= rhsand discharges viasmt.check_validagainst current preconditions + path conditions. If Z3 can't discharge, emits E502 with counterexample, rationale, fix suggestion, and spec ref to §4.4 + §11.2.1.Runtime — codegen guard
vera/wasm/operators.pymirrors the verifier's detection (_is_static_nat_typed AND _has_nat_origin_codegenreproduce the verifier's analysis at AST level) and emits a guarded subtraction WAT sequence:Programs that ran
vera verifyfirst see the static error; programs that skip verification trap cleanly via the runtime guard rather than silently producing a negative@Nat. A dedicatedkind="underflow"trap label requires new host-import scaffolding mirroringvera.contract_failand is left as a follow-up enhancement; for now the trap is classified as the existingkind="unreachable".Path-A scope: pure-literal subtractions exempt
The corpus uses
0 - 1widely as the "I want -1" idiom (Err(_) -> 0 - 1,throw(0 - 1)) where the result is consumed at@Intpositions and the upcast is well-defined. Both verifier and codegen require at least one operand to have@Natprovenance (a slot ref or function returning@Nat), so pure-literal subtractions are intentionally not flagged. Catchinglet @Nat = 0 - 1requires the binding-site generalisation tracked as #552.Migration
Zero corpus migration needed. Every existing
@Nat - @Natsite inexamples/andtests/conformance/is guarded byif @Nat.0 == 0 then base else recursive(@Nat.0 - 1)and Z3 discharges the obligation from the path condition automatically. External programs that previously verified at Tier 1 may now require explicitrequires(lhs >= rhs)clauses; the diagnostic names the fix and shows the counterexample inputs.Spec
spec/04-expressions.md§4.4 — short clause mirroring the divide-by-zero language: subtraction on unsigned types is undefined behaviour when it would underflow; the compiler SHOULD verifylhs >= rhsand MUST insert a runtime check otherwise.spec/11-compilation.md§11.2.1 — full treatment with the Tier-1 proof obligation, Tier-3 fallback codegen, the lift-back path viarequires, and references to @Byte subtraction silent underflow (follow-up to #520) #551 (@byte) and Generalize @Nat invariant check to all binding sites (broader generalisation of #520) #552 (binding-site generalisation).spec/11-compilation.md§11.3.3 — footnote on the operator table pointing back to §11.2.1 so readers of the canonical "what does each operator compile to" reference learn that@Nat - @Natis conditionally guarded.Tests
New
TestNatSubtractionObligation520intests/test_verifier.py(9 tests):@Nat.0 - 0,@Nat.0 - @Nat.0)@Int - @Intand@Nat - @Int → @IntexclusionsNew
TestNatSubtractionRuntimeGuard520intests/test_codegen.py(6 tests):@Nat - @Nat, absent for@Int - @Intand pure-literal0 - 1)New
tests/conformance/ch04_nat_subtraction.verademonstrating canonical discharge patterns. Verifies at Tier 1 with 17 contracts; runs to produce 7. Conformance corpus 81 → 82.Tier-count assertions updated where the new obligations land:
test_recursive_call_decreases_verifiedandtest_mixed_tiers(3 → 4 T1),test_overall_tier_countsaggregate (219/26/245 → 222/26/248),test_mutual_recursion_example_all_t1(8 → 10 T1).New error code
E502 —
@Nat subtraction underflow obligation not discharged. Counterexample-bearing diagnostic with rationale, fix suggestion, and spec ref to §4.4 + §11.2.1.Documentation
[0.0.127].Test plan
pytest tests/ -q— 3,525 passed, 14 skipped (was 3,514 + 14)mypy vera/— cleanpython scripts/check_conformance.py— all 82 pass (was 81)python scripts/check_examples.py— all 33 passpython scripts/check_doc_counts.py— consistent (3,638 tests)python scripts/check_limitations_sync.py— consistent (32 KNOWN_ISSUES, 15 vera/README, 2 spec)python scripts/check_version_sync.py— version 0.0.127 consistent across 4 filesunsafe(0, 1)traps at runtime (was: silent -1)@Nat verifyof canonical recursion shapes still discharge from path conditionsCo-Authored-By: Claude noreply@anthropic.invalid
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests