fix(numscript): preload unbounded-overdraft sources via static involved-accounts discovery by aalloiteau · Pull Request #1504 · formancehq/ledger

aalloiteau · 2026-07-06T13:48:11Z

Summary

Fixes #1500 — send overdraft(...) failing at execution with STORAGE_OPERATION_FAILED / read of undeclared key on v3.

Root cause

Numscript dependency discovery (DiscoverNumscriptDependencies in emulate.go) determines which account balances to preload/lock by emulating the script against fake infinite balances. The discovery store returns a large positive balance for every account, so:

overdraft(@credit, USD/2) sees a positive balance → folds to USD/2 0
a zero-amount send emits no posting
the source @repay (an allowing unbounded overdraft source, which the interpreter never balance-queries) appears in neither queriedVolumes nor the postings

At execution the overdraft is non-zero, the posting @repay → @credit needs @repay's volume, but it was never preloaded → read of undeclared key.

Emulation is fundamentally unable to cover this: balance() needs a positive fake balance to allow sends, overdraft() needs a negative one to be non-zero — no single value satisfies both. This is exactly the static-analysis gap the existing code comment anticipates.

Fix

Union numscript's static GetInvolvedAccounts analysis into sourceVolumes. It walks the AST and reports every account each send touches — including allowing unbounded overdraft sources — regardless of the resolved amount. Two small resolvers fold the involved-account expressions to concrete (account, asset) strings (e.g. GetAsset{GetOverdraft{@credit, USD/2}} → "USD/2").

Accounts already classified as destinations are skipped, so the source/destination partition (and existing exact-length assertions) is preserved.
The union is a safe superset: over-preloading only locks an extra row; under-preloading was the bug.
Best-effort: if GetInvolvedAccounts errors on an experimental construct it doesn't model yet (e.g. asset scaling), we keep the emulation-only result rather than regressing scripts that work today.

Tests

Added overdraft send discovers the unbounded-overdraft source (ledger#1500). Verified it fails without the fix (@repay missing) and passes with it.
All existing TestDiscoverNumscriptDependencies subtests still pass (no regression).

⚠️ Blocked on numscript release — do not merge as-is

GetInvolvedAccounts + the github.com/formancehq/numscript/accounts package are merged on numscript main but not yet in a tagged release (ledger pins v0.0.24). This PR temporarily pins the numscript main pseudo-version (v0.0.25-0.20260608064954-1fcd4893eafe) so it compiles and CI can run.

Before merge:

Tag a numscript release (e.g. v0.0.25) containing GetInvolvedAccounts.
go get github.com/formancehq/numscript@v0.0.25 && go mod tidy and re-pin to the clean tag.

Notes

Target branch is release/v3.0: this code path (emulate.go / admission preload) is v3-only; main is the v2.x line and has no equivalent, so there is nothing to land there or backport from.

coderabbitai · 2026-07-06T13:48:21Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 89916099-b943-43d1-81a1-9fad196314f8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/numscript-overdraft-dependency-discovery

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

codecov · 2026-07-06T13:57:35Z

Codecov Report

❌ Patch coverage is 55.22388% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.39%. Comparing base (1b7e332) to head (0174c98).
⚠️ Report is 2 commits behind head on release/v3.0.

Files with missing lines	Patch %	Lines
internal/domain/processing/numscript/emulate.go	56.89%	22 Missing and 3 partials ⚠️
internal/domain/processing/numscript/errors.go	44.44%	3 Missing and 2 partials ⚠️

❌ Your patch check has failed because the patch coverage (55.22%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@               Coverage Diff                @@
##           release/v3.0    #1504      +/-   ##
================================================
+ Coverage         74.35%   74.39%   +0.03%     
================================================
  Files               396      396              
  Lines             40087    40156      +69     
================================================
+ Hits              29808    29873      +65     
+ Misses             7661     7658       -3     
- Partials           2618     2625       +7

Flag	Coverage Δ
e2e	`74.39% <55.22%> (+0.03%)`	⬆️
scenario	`74.39% <55.22%> (+0.03%)`	⬆️
unit	`74.39% <55.22%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

NumaryBot · 2026-07-07T14:37:17Z

✅ Approve — automated review

The PR implements static involved-account discovery to preload unbounded-overdraft sources during numscript emulation. The one significant concern that was raised—arithmetic-wrapped overdraft expressions not being handled in GetInvolvedAccounts—was identified in prior review threads and has since been resolved. The sole new reviewer found no additional correctness issues. Patch coverage (~55%) remains below the project average (~74%), which was already noted in prior discussion but does not constitute a blocking issue. No new blockers or major findings remain.

No findings.

@acc

…ed-accounts discovery Dependency discovery emulates scripts against fake infinite (positive) balances. overdraft(@acc, A) folds to zero against a positive balance, so `send overdraft(...)` becomes a zero-amount send that emits no posting. The source account then appears in neither the balance queries nor the postings and is missed by discovery, causing a "read of undeclared key" STORAGE_OPERATION_FAILED at execution time. Union numscript's static GetInvolvedAccounts analysis into sourceVolumes. It reports every account each send touches -- including `allowing unbounded overdraft` sources -- regardless of the resolved amount. Accounts already classified as destinations are skipped to keep the source/destination partition intact; the union is a safe superset. Requires numscript >= the release exposing GetInvolvedAccounts (temporarily pinned to the main pseudo-version until v0.0.25 is tagged). Fixes #1500

@repay

…ad (ledger#1500) Add two e2e specs that exercise the fix end-to-end through the gRPC apply path. Both were verified to fail without the emulate.go change and pass with it: 1. Mid-script `send overdraft(@credit, USD/2) (source = @repay allowing unbounded overdraft, ...)` — the exact reproduction from the issue. Before the fix this failed at apply with STORAGE_OPERATION_FAILED "read of undeclared key" because @repay was missed by dependency discovery. 2. Vars-bound `monetary $due = overdraft(@credit_vars, USD/2)` — the variant the issue explicitly calls out ("Binding overdraft() through a vars block instead of a mid-script call fails identically"). Both assert the repay posting materialises for the overdrawn amount and that end-balances match (credit flat, repay = -overdrawn).

NumaryBot

NumaryBot posted 1 new inline finding.

Summary: #1504 (comment)

… bump The workload submodule has its own go.mod with a replace directive pointing at the root, so it transitively picks up the numscript pseudo-version bump. tests/antithesis/run_model_test.sh cross-builds the workload driver from that submodule and was failing the Tests-Model gate on `go: updates to go.mod needed` even though the root go.mod was already tidied. Run `go mod tidy` in the submodule to pick up the same sentry-go and x/exp bumps that landed in the root.

NumaryBot

NumaryBot posted 1 new inline finding.

Summary: #1504 (comment)

…ed-accounts Extend resolveInvolvedAsset to walk monetary-arithmetic node types. Same class of bug as ledger#1500: when a send amount wraps overdraft() inside arithmetic (e.g. `[USD/2 0] + $due` where `$due = overdraft(@credit, USD/2)`), the amount collapses to zero under emulation and no posting is emitted, so the `allowing unbounded overdraft` source is missed by the postings path. The involved-accounts entry uses AssetExpr = GetAsset{Add{...}}, and until this change the resolver fell through on Add/Sub/Div/SubPrefix and the source was skipped, resurfacing "read of undeclared key" at apply. Arithmetic on monetaries is asset-preserving in numscript — operands must share an asset — so recursing into either operand is safe. Add a unit test that reproduces the gap; verified it fails without the resolver changes.

NumaryBot

✅ Approve — automated review

The PR correctly implements static involved-account discovery to preload unbounded-overdraft sources. The previously raised major issue regarding arithmetic-wrapped overdraft expressions (Add/Sub/SubPrefix nodes) was flagged in prior automated review threads, but the latest reviewer confirms the current diff already addresses those cases. No new blockers or major issues have been identified. Patch coverage (~64%) remains below the project average (~74%), which is worth improving but does not warrant blocking the merge.

Findings outside the diff

🟡 [minor] Patch coverage below project average — internal/domain/processing/numscript/emulate.go

The new code has ~64% patch coverage against a project baseline of ~74%. Arithmetic-wrapping paths and other edge cases in GetInvolvedAccounts appear to lack test coverage, which could allow regressions to go undetected.

Suggestion: Add tests covering overdraft expressions embedded inside compound monetary arithmetic nodes (Add, Sub, SubPrefix) to bring patch coverage in line with the project average.

Summary: #1504 (comment)

Azorlogh

Safe as a stopgap — the union only ever over-preloads, and discovery is admission-only (never touches the FSM). Three notes:

Wrap GetInvolvedAccounts in a recover. Every other numscript call here goes through SafeRun because the interpreter can panic (numscript still has open panic fixes, e.g. numscript#153). Degrade a panic to the same best-effort skip as the error branch.
FYI (no action needed): GetInvolvedAccounts was removed from numscript main in numscript#166 and never tagged, so the "tag v0.0.25 + re-pin" plan can't work. The intended replacement is numscript#135 (ResolveDependencies, returns concrete values), which would let this whole hack be deleted rather than re-pinned.
Nit: e2e step-1 comment says "balance on @main allows the send" — @main is the destination; source is @credit.

@Azorlogh

…mment nit Two review notes from @Azorlogh on ledger#1504: 1. Wrap GetInvolvedAccounts in a recover. Every other numscript call here goes through SafeRun because the interpreter can still panic on open issues (e.g. formancehq/numscript#153). Add safeGetInvolvedAccounts mirroring SafeRun so a panic degrades to the same best-effort skip as the error branch — admission never rejects a script emulation already accepted. 2. Fix wording in the e2e step-1 comment: @main is the destination and @credit is the unbounded-overdraft source, not the other way around. Clarify why emulation lets this transaction through without exercising the discovery gap.

Azorlogh · 2026-07-08T09:57:47Z

LGTM, but dirty check is failing

golangci-lint --fix (nlreturn) inserts a blank line before the return statements in resolveInvolvedAccount / resolveInvolvedAsset. Without them the Dirty CI check fails on the diff left by `just pre-commit`.

gfyrag marked this pull request as ready for review July 7, 2026 14:35

gfyrag requested review from Azorlogh, gfyrag and paul-nicolas as code owners July 7, 2026 14:35

aalloiteau and others added 2 commits July 7, 2026 16:59

gfyrag force-pushed the fix/numscript-overdraft-dependency-discovery branch from f97297f to 89b7bf1 Compare July 7, 2026 15:00

NumaryBot reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/domain/processing/numscript/emulate.go

NumaryBot reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/domain/processing/numscript/emulate.go

NumaryBot reviewed Jul 7, 2026

View reviewed changes

Azorlogh reviewed Jul 7, 2026

View reviewed changes

style(numscript): blank line before returns to satisfy nlreturn

0174c98

golangci-lint --fix (nlreturn) inserts a blank line before the return statements in resolveInvolvedAccount / resolveInvolvedAsset. Without them the Dirty CI check fails on the diff left by `just pre-commit`.

Azorlogh approved these changes Jul 8, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(numscript): preload unbounded-overdraft sources via static involved-accounts discovery#1504

fix(numscript): preload unbounded-overdraft sources via static involved-accounts discovery#1504
aalloiteau wants to merge 6 commits into
release/v3.0from
fix/numscript-overdraft-dependency-discovery

aalloiteau commented Jul 6, 2026

Uh oh!

coderabbitai Bot commented Jul 6, 2026 •

edited

Loading

Review skipped

Uh oh!

codecov Bot commented Jul 6, 2026 •

edited

Loading

Uh oh!

NumaryBot commented Jul 7, 2026 •

edited

Loading

Uh oh!

NumaryBot left a comment

Uh oh!

Uh oh!

NumaryBot left a comment

Uh oh!

Uh oh!

NumaryBot left a comment

Uh oh!

Azorlogh left a comment •

edited

Loading

Uh oh!

Azorlogh commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aalloiteau commented Jul 6, 2026

Summary

Root cause

Fix

Tests

⚠️ Blocked on numscript release — do not merge as-is

Notes

Uh oh!

coderabbitai Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

codecov Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

NumaryBot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Approve — automated review

Uh oh!

NumaryBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NumaryBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NumaryBot left a comment

Choose a reason for hiding this comment

✅ Approve — automated review

Findings outside the diff

Uh oh!

Azorlogh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Azorlogh commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jul 6, 2026 •

edited

Loading

codecov Bot commented Jul 6, 2026 •

edited

Loading

NumaryBot commented Jul 7, 2026 •

edited

Loading

Azorlogh left a comment •

edited

Loading