feat: scoped accounts#161
Conversation
|
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:
WalkthroughThis PR introduces an experimental ChangesScoped accounts and row-based balances/metadata refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Script
participant Interpreter
participant CachedBalances
participant FundsQueue
participant Postings
Script->>Interpreter: scoped(`@account`, "reserve")
Interpreter->>Interpreter: checkScopeName(scope)
Interpreter-->>Script: AccountAddress{Name, Scope}
Script->>Interpreter: send from scoped account
Interpreter->>CachedBalances: fetchBalance(AccountAddress)
CachedBalances-->>Interpreter: amount
Interpreter->>FundsQueue: pushSender(Sender{Account, Amount})
FundsQueue-->>Interpreter: pulled senders
Interpreter->>Postings: newPosting(SourceScope, DestinationScope)
Postings-->>Script: ExecutionResult with scoped postings
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
3613cf9 to
bcba9a9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specs_format/index.go (1)
87-107:⚠️ Potential issue | 🟡 MinorCheck function has an implicit precondition: specs must be pre-validated, but this is not documented.
While the production code path in
runRawSpec(runner.go:160) does guardCheckwithvalidateSpecs, theCheckfunction is exported and lacks documentation of this precondition. Tests intentionally callCheckdirectly without validation—which is fine for unit testing—but any external caller or future code path that directly invokesCheckcould bypass the guard.Consider either:
- Adding a docstring to
Checkdocumenting the precondition thatvalidateSpecsmust be called first- Making
Checkinternal (unexported) to enforce the contract at the type level- Moving validation into
Checkitself to ensure it always holds🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specs_format/index.go` around lines 87 - 107, The exported Check function has an implicit precondition that the specs parameter must be pre-validated by calling validateSpecs first, but this requirement is not documented. Add a docstring to the Check function that clearly documents this precondition, explaining that callers must ensure specs have been validated before invoking Check. This will make the contract explicit and prevent external callers or future code paths from accidentally bypassing the validation guard.
♻️ Duplicate comments (2)
internal/analysis/check.go (1)
127-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the scoped feature flag for
scoped()version gating.Line 130 still references
flags.ExperimentalGetAmountFunctionFeatureFlagforFnVarOriginScoped. This makes static analysis require the wrong flag forscoped()and can diverge from interpreter behavior. Switch it toflags.ExperimentalScopedFunction.Proposed fix
FnVarOriginScoped: VarOriginFnCallResolution{ Params: []string{TypeAccount, TypeString}, Return: TypeAccount, Docs: "returns the scoped version of that account. Empty string means no scope. Overwrites the previous scope", VersionConstraints: []VersionClause{ { Version: parser.NewVersionInterpreter(0, 0, 25), - FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, + FeatureFlag: flags.ExperimentalScopedFunction, }, }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/analysis/check.go` around lines 127 - 131, The VersionClause for FnVarOriginScoped (the scoped() function) in the check.go file is using the incorrect feature flag. Replace flags.ExperimentalGetAmountFunctionFeatureFlag with flags.ExperimentalScopedFunction in the FeatureFlag field of the VersionClause that has Version 0.0.25. This ensures that static analysis uses the correct scoped feature flag for version gating and aligns with the interpreter behavior.internal/interpreter/interpreter.go (1)
184-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep canonical account validation free of the DSL
@sigil.Line 184 now accepts an optional leading
@. This allows raw variable values like@srcto validate as account addresses and propagate into postings/store-facing data, which breaks canonical account contract boundaries.Suggested fix
-var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") +var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` at line 184, The accountNameRegex variable at line 184 includes an optional leading @ character (^@?) which allows DSL-specific variable values like `@src` to be validated as canonical account addresses, violating the account contract boundary. Remove the @? optional matching from the beginning of the regex pattern so it only validates canonical account names without the DSL @ sigil prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/balances.go`:
- Around line 83-88: The CompareBalances function can incorrectly return true
for non-equal multisets when duplicate (account, asset, color) keys exist,
because CompareBalancesIncluding only resolves the first match of each key. Add
duplicate key validation in the CompareBalances function to check both input
Balances for duplicate keys and return false immediately if any duplicates are
found, ensuring that only well-formed (non-duplicate) Balances can pass the
equality check.
In `@internal/interpreter/evaluate_expr.go`:
- Around line 154-158: The code unconditionally calls colorExpr.GetRange() when
constructing the InvalidColor error return at line 157, but colorExpr can be nil
according to the evaluateOptExprAs contract. Add a nil check for colorExpr
before dereferencing it; if colorExpr is nil, either omit the Range field from
the InvalidColor struct or provide a zero/default Range value instead of calling
GetRange().
In `@internal/interpreter/function_exprs.go`:
- Around line 181-185: Remove the invalid pointer dereferences in the scoped()
function. The parameters scope and acc are value types (string type aliases),
not pointers, so they should not be dereferenced with the * operator. Remove the
* prefix from all occurrences: the *scope in the validateScope call, the *scope
in the InvalidScope struct literal, and both *acc and *scope in the appendScope
call.
In `@internal/interpreter/internal_balances.go`:
- Around line 71-82: In the Set method of InternalBalances, the code currently
stores caller-owned *big.Int pointers directly (at both the assignment on line
74 and the append on line 81), which allows external mutation of cached state
and permits nil pointers to enter the runtime balance state. Copy the amount
value instead of storing the pointer directly: create a new big.Int and use the
Set method to copy the value (e.g., new(big.Int).Set(amount)) at both locations
where Amount is assigned, and ensure nil amounts are handled appropriately by
normalizing them to zero or a safe default big.Int value.
In `@internal/interpreter/store.go`:
- Around line 47-52: Add nil checks before dereferencing row.Amount in the
BalanceRow initialization within the store materialization code. When
constructing BalanceRow objects and assigning Amount using
new(big.Int).Set(row.Amount), first check if row.Amount is nil to prevent panics
during query execution. Apply this same nil-checking pattern to all locations
where row.Amount is dereferenced through the Set method call, ensuring the code
gracefully handles null amount values.
In `@internal/mcp_impl/handlers.go`:
- Around line 69-72: The error handling for BindArguments is inconsistent with
other error handling in this handler. Currently, when BindArguments fails, the
code returns (nil, err) which propagates it as a server-level error. Refactor
this to wrap the error in mcp.NewToolResultError(...) and return
(mcp.NewToolResultError(...), nil) instead, matching the pattern used for other
errors in the same handler such as RequireString failures and interpreter
errors. This ensures all tool-level errors are handled consistently as
structured tool result errors rather than server-level errors.
---
Outside diff comments:
In `@internal/specs_format/index.go`:
- Around line 87-107: The exported Check function has an implicit precondition
that the specs parameter must be pre-validated by calling validateSpecs first,
but this requirement is not documented. Add a docstring to the Check function
that clearly documents this precondition, explaining that callers must ensure
specs have been validated before invoking Check. This will make the contract
explicit and prevent external callers or future code paths from accidentally
bypassing the validation guard.
---
Duplicate comments:
In `@internal/analysis/check.go`:
- Around line 127-131: The VersionClause for FnVarOriginScoped (the scoped()
function) in the check.go file is using the incorrect feature flag. Replace
flags.ExperimentalGetAmountFunctionFeatureFlag with
flags.ExperimentalScopedFunction in the FeatureFlag field of the VersionClause
that has Version 0.0.25. This ensures that static analysis uses the correct
scoped feature flag for version gating and aligns with the interpreter behavior.
In `@internal/interpreter/interpreter.go`:
- Line 184: The accountNameRegex variable at line 184 includes an optional
leading @ character (^@?) which allows DSL-specific variable values like `@src` to
be validated as canonical account addresses, violating the account contract
boundary. Remove the @? optional matching from the beginning of the regex
pattern so it only validates canonical account names without the DSL @ sigil
prefix.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce101ddb-95c1-4b5f-a5ba-b512764f07fe
⛔ Files ignored due to path filters (140)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/mixed-source-dest-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/send-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/allocate-dont-take-too-much.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/allocation.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/ask-balance-twice.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance-not-found.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance-simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/bigint-literal.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/capped-when-less-than-needed.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/capped-when-more-than-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/cascading-sources.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft-on-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft-when-double-spending.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/dynamic-allocation.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/empty-postings.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/account-interpolation/account-interp.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance-when-missing-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restriction-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send-overdrat.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-with-asset-precision.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/empty-color.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/no-solution.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/update-swap-account-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-amount-function/get-amount-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-asset-function/get-asset-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/expr-in-var-origin.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/midscript-balance-after-decrease.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/midscript-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-all-failing.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-source-send-first-branch.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-singleton.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/update-balances-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-use-case-remove-debt.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-positive.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/reach-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/feature-flag-syntax.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/inoder-destination.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/insufficient-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/kept-in-send-all-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/kept-with-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/many-kept-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/many-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/max-with-unbounded-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/metadata.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-monetary.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-prefix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/negative-max-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/negative-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/nested-remaining-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/ovedrafts-playground-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-in-send-all-when-noop.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-balance-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-ovedraft-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/remaining-kept-in-send-all-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/remaining-none-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__multi-postings.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-a-different-asset.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-all-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-causes-failure.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-more-than-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__with-asset-var.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__with-monetary-var.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-destinatio-allot-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-destinatio-allot.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-many-max-in-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-multi.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-variable.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-when-negative-with-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-when-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-in-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-in-src.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-when-no-amount.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-when-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-allotment-invalid-amt.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-overlapping.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances3.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/unbounded-overdraft-when-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/update-balances.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/use-balance-twice.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/use-different-assets-with-same-source-account.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-asset.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__1.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__3.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__4.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__5.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables-json.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/world-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snapinternal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snapspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (36)
internal/analysis/check.gointernal/cmd/run.gointernal/cmd/test_init.gointernal/cmd/test_init_test.gointernal/flags/flags.gointernal/interpreter/accounts_metadata.gointernal/interpreter/append_scope.gointernal/interpreter/append_scope_test.gointernal/interpreter/args_parser.gointernal/interpreter/args_parser_test.gointernal/interpreter/asset_scaling.gointernal/interpreter/balances.gointernal/interpreter/balances_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/evaluate_expr.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/infix_overloads.gointernal/interpreter/internal_balances.gointernal/interpreter/internal_balances_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_error.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gointernal/interpreter/testdata/script-tests/experimental/scoped-function/simple.numinternal/interpreter/value.gointernal/mcp_impl/handlers.gointernal/parser/ast.gointernal/specs_format/index.gointernal/specs_format/parse_test.gointernal/specs_format/run_test.gointernal/specs_format/runner.gointernal/specs_format/runner_test.gointernal/utils/utils.gonumscript.gonumscript_test.go
💤 Files with no reviewable changes (2)
- internal/utils/utils.go
- internal/parser/ast.go
| isValidColor := colorRe.Match([]byte(string(color))) | ||
| if !isValidColor { | ||
| return nil, InvalidColor{ | ||
| return "", InvalidColor{ | ||
| Range: colorExpr.GetRange(), | ||
| Color: *color, | ||
| Color: string(color), |
There was a problem hiding this comment.
Guard nil colorExpr before using GetRange().
Line 157 dereferences colorExpr even though evaluateOptExprAs permits colorExpr == nil. If empty color is rejected, this can panic at runtime.
Proposed fix
func (s *programState) evaluateColor(colorExpr parser.ValueExpr) (String, InterpreterError) {
+ if colorExpr == nil {
+ return "", nil
+ }
+
color, err := evaluateOptExprAs(s, colorExpr, expectString)
if err != nil {
return "", err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isValidColor := colorRe.Match([]byte(string(color))) | |
| if !isValidColor { | |
| return nil, InvalidColor{ | |
| return "", InvalidColor{ | |
| Range: colorExpr.GetRange(), | |
| Color: *color, | |
| Color: string(color), | |
| func (s *programState) evaluateColor(colorExpr parser.ValueExpr) (String, InterpreterError) { | |
| if colorExpr == nil { | |
| return "", nil | |
| } | |
| color, err := evaluateOptExprAs(s, colorExpr, expectString) | |
| if err != nil { | |
| return "", err | |
| } | |
| // ... rest of function continues |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interpreter/evaluate_expr.go` around lines 154 - 158, The code
unconditionally calls colorExpr.GetRange() when constructing the InvalidColor
error return at line 157, but colorExpr can be nil according to the
evaluateOptExprAs contract. Add a nil check for colorExpr before dereferencing
it; if colorExpr is nil, either omit the Range field from the InvalidColor
struct or provide a zero/default Range value instead of calling GetRange().
There was a problem hiding this comment.
that's not a bug, that is never nil
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "evaluateColor" --type=go -C3Length of output: 4192
🏁 Script executed:
#!/bin/bash
grep -rn "evaluateColor" --include="*.go" .Length of output: 500
🏁 Script executed:
#!/bin/bash
sed -n '500,545p;670,695p' internal/interpreter/interpreter.go
grep -n "colorExpr" internal/interpreter/*.go
grep -rn "Color \+parser.ValueExpr\|Color parser.ValueExpr" --include="*.go" .
grep -n "Color " internal/parser/ast.goLength of output: 3207
Confirmed — both call sites in internal/interpreter/interpreter.go pass colorExpr straight through from takeAllFromAccount/tryTakingFromAccount, and both explicitly check if colorExpr != nil right before calling evaluateColor, but that check only gates the feature-flag validation; they still call evaluateColor(colorExpr) unconditionally afterward, including when colorExpr is nil. In that nil case, evaluateOptExprAs short-circuits and returns ("", nil) without invoking expect, so GetRange() is never reached because the err != nil branch isn't taken and the color validation with colorExpr.GetRange() only executes when isValidColor is false — which can't happen when color == "" matches colorRe (assuming the empty string is a valid match). So while colorExpr can indeed be nil at runtime, it never reaches the GetRange() line unless the empty string fails the color regex.
You're right that this isn't a live panic given the current regex/empty-string behavior — withdrawing the concern.
(‑, ‑)ゞ
🧬 Code Graph Analysis Results
internal/interpreter/evaluate_expr.go
L12-L111 (handles all parser.ValueExpr variants; default/unreachable paths call utils.NonExhaustiveMatchPanic and then return nil, nil):
func (st *programState) evaluateExpr(expr parser.ValueExpr) (Value, InterpreterError) {
switch expr := expr.(type) {
case *parser.AssetLiteral:
return Asset(expr.Asset), nil
case *parser.AccountInterpLiteral:
var parts []string
for _, part := range expr.Parts {
switch part := part.(type) {
case parser.AccountTextPart:
parts = append(parts, part.Name)
case *parser.Variable:
err := st.checkFeatureFlag(flags.ExperimentalAccountInterpolationFlag)
if err != nil {
return nil, err
}
value, err := st.evaluateExpr(part)
if err != nil {
return nil, err
}
strValue, err := castToString(value, expr.Range)
if err != nil {
return nil, err
}
parts = append(parts, strValue)
}
}
name := strings.Join(parts, "")
return NewAccountAddress(name)
case *parser.StringLiteral:
return String(expr.String), nil
case *parser.PercentageLiteral:
return Portion(*expr.ToRatio()), nil
case *parser.NumberLiteral:
return MonetaryInt(*expr.Number), nil
case *parser.MonetaryLiteral:
asset, err := evaluateExprAs(st, expr.Asset, expectAsset)
if err != nil {
return nil, err
}
amount, err := evaluateExprAs(st, expr.Amount, expectNumber)
if err != nil {
return nil, err
}
return Monetary{Asset: asset, Amount: amount}, nil
case *parser.Variable:
value, ok := st.ParsedVars[expr.Name]
if !ok {
return nil, UnboundVariableErr{
Name: expr.Name,
Range: expr.Range,
}
}
return value, nil
case *parser.BinaryInfix:
switch expr.Operator {
case parser.InfixOperatorPlus:
return st.plusOp(expr.Left, expr.Right)
case parser.InfixOperatorMinus:
return st.subOp(expr.Left, expr.Right)
case parser.InfixOperatorDiv:
return st.divOp(expr.Range, expr.Left, expr.Right)
default:
utils.NonExhaustiveMatchPanic[any](expr.Operator)
return nil, nil
}
case *parser.Prefix:
switch expr.Operator {
case parser.PrefixOperatorMinus:
return st.unaryNegOp(expr.Expr)
default:
utils.NonExhaustiveMatchPanic[any](expr.Operator)
return nil, nil
}
case *parser.FnCall:
if !st.varOriginPosition {
err := st.checkFeatureFlag(flags.ExperimentalMidScriptFunctionCall)
if err != nil {
return nil, err
}
}
return st.handleFnCall(nil, *expr)
default:
utils.NonExhaustiveMatchPanic[any](expr)
return nil, nil
}
}L113-L119 (evaluateOptExprAs returns zero value t with nil error when expr == nil):
func evaluateOptExprAs[T any](st *programState, expr parser.ValueExpr, expect func(Value, parser.Range) (T, InterpreterError)) (T, InterpreterError) {
var t T
if expr == nil {
return t, nil
}
return evaluateExprAs(st, expr, expect)
}L121-L134 (evaluateExprAs does not nil-check expr; it calls expr.GetRange()):
func evaluateExprAs[T any](st *programState, expr parser.ValueExpr, expect func(Value, parser.Range) (T, InterpreterError)) (T, InterpreterError) {
var default_ T
value, err := st.evaluateExpr(expr)
if err != nil {
return default_, err
}
res, err := expect(value, expr.GetRange())
if err != nil {
return default_, err
}
return res, nil
}internal/parser/ast.go
L8-L11 (ValueExpr requires Ranged; evaluateExprAs calls expr.GetRange()):
ValueExpr interface {
Ranged
valueExpr()
}internal/utils/utils.go
L8-L10 (NonExhaustiveMatchPanic panics and formats the unexpected value):
func NonExhaustiveMatchPanic[T any](value any) T {
panic(fmt.Sprintf("Non exhaustive match (got '%#v')", value))
}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specs_format/index.go (1)
274-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
validateSpecsis defined but never called.The
validateSpecsfunction validates that balance lists don't contain duplicate entries, butCheck()never invokes it. Duplicate balance entries will silently use the last value instead of being rejected as the docstring intends.Add
if err := validateSpecs(specs); err != nil { return SpecsResult{}, ... }at the start ofCheck().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specs_format/index.go` around lines 274 - 289, The validateSpecs function is defined but never invoked, allowing duplicate balance entries to silently use the last value instead of being rejected. Add a call to validateSpecs at the beginning of the Check() function to validate the specs Balances before any processing occurs; if validateSpecs returns an error, return a SpecsResult with the error rather than continuing.
♻️ Duplicate comments (2)
internal/interpreter/value.go (1)
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScoped account strings passed as variable values are not parsed into separate Name/Scope.
When a caller passes an account string containing a scope suffix (e.g.,
"src/x"as a variable value),NewAccountAddressstores it entirely in theNamefield without extracting the scope. This meansAccountAddress{Name: "src/x", Scope: ""}is created instead of{Name: "src", Scope: "x"}.Balance queries and postings will then use the wrong keys, looking for
account="src/x", scope=""rather thanaccount="src", scope="x".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value.go` around lines 44 - 49, The NewAccountAddress function does not parse scoped account strings that contain a scope suffix. When a string like "src/x" is passed as the src parameter, the entire string is stored in the Name field without extracting the scope portion. Modify NewAccountAddress to parse the src string, extract any scope suffix (typically separated by a delimiter), and populate both the Name and Scope fields of the returned AccountAddress struct accordingly. The validation with checkAccountName should be applied appropriately to handle both scoped and unscoped account strings.internal/interpreter/interpreter.go (1)
199-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccount regex rejects digit-containing scopes that
validateScopeaccepts.The regex pattern
(?:/[a-z_]+)?only allows lowercase letters and underscores in the scope suffix, butvalidateScopeaccepts digits (e.g.,"x1"). A script usingscoped(@src, "x1")will pass scope validation during interpretation but then failcheckPostingInvariantswhen the posting is created.Align the regex with
validateScopeby changing[a-z_]+to[a-z0-9_]+.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` around lines 199 - 206, The account name regex pattern in accountNameRegex uses a character class [a-z_]+ for the scope suffix that does not match the validation logic in validateScope, which accepts digits. Update the regex pattern to include digits by changing the scope suffix pattern from (?:/[a-z_]+)? to (?:/[a-z0-9_]+)? so that the regex accepts digit-containing scopes like "x1" that validateScope permits, preventing validation failures downstream.
🧹 Nitpick comments (3)
internal/interpreter/value_test.go (1)
43-51: ⚡ Quick winConsider adding test coverage for scoped address JSON marshaling.
The test only covers an
AccountAddresswithout a scope. Adding a test case forAccountAddress{Name: "abc", Scope: "xyz"}marshaling to"abc/xyz"would ensure the scope formatting logic is verified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value_test.go` around lines 43 - 51, The TestMarshalAddress function currently only tests marshaling of AccountAddress with a Name field but no Scope. Add an additional test case within TestMarshalAddress to verify that AccountAddress with both Name and Scope fields (e.g., Name: "abc", Scope: "xyz") marshals correctly to the format "abc/xyz". This ensures the scope formatting logic is properly covered by the test suite.internal/interpreter/interpreter.go (1)
581-586: 💤 Low valueDirect map access on
CachedBalancesassumes account was pre-queried.The code accesses
s.CachedBalances[account]directly and returns an error if the key is missing. This relies on the balance query batching having populated that key. If the batching logic ever changes or misses a path, this will surface as an opaqueInvalidUnboundedAddressInScalingAddresserror rather than indicating the actual cache miss.This is a potential fragility but not an immediate bug given current batching coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` around lines 581 - 586, The direct map access on s.CachedBalances[account] at the point where the ok check fails assumes the balance query batching has already populated that key, but provides no explicit verification or clear indication when a cache miss occurs. Instead of returning the generic InvalidUnboundedAddressInScalingAddress error, add logging or improve the error message to explicitly indicate that the account's balance was not found in the cache, making it clear that the issue is a cache miss rather than an unbounded address problem. This will help surface any gaps in the balance query batching logic more explicitly.internal/interpreter/value.go (1)
275-287: 💤 Low valueSilent fallback to scale=0 on parse failure may hide malformed assets.
When the scale suffix cannot be parsed as an integer,
GetBaseAndScalesilently returnsscale=0. This could mask malformed asset strings that look valid but have non-numeric scale suffixes (e.g.,"USD/abc").Consider whether callers need to distinguish between "no scale specified" and "invalid scale format".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value.go` around lines 275 - 287, The GetBaseAndScale function on the Asset type silently masks malformed asset strings by returning scale=0 when the scale suffix fails to parse as an integer, making it impossible for callers to distinguish between a missing scale and an invalid scale format. Modify the GetBaseAndScale function signature to return a third error value (changing from (string, int64) to (string, int64, error)), and when the strconv.ParseInt call fails, return the error instead of silently returning 0. This allows callers to detect and handle genuinely malformed assets like "USD/abc" rather than treating them the same as valid assets without a scale suffix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/specs_format/index.go`:
- Around line 274-289: The validateSpecs function is defined but never invoked,
allowing duplicate balance entries to silently use the last value instead of
being rejected. Add a call to validateSpecs at the beginning of the Check()
function to validate the specs Balances before any processing occurs; if
validateSpecs returns an error, return a SpecsResult with the error rather than
continuing.
---
Duplicate comments:
In `@internal/interpreter/interpreter.go`:
- Around line 199-206: The account name regex pattern in accountNameRegex uses a
character class [a-z_]+ for the scope suffix that does not match the validation
logic in validateScope, which accepts digits. Update the regex pattern to
include digits by changing the scope suffix pattern from (?:/[a-z_]+)? to
(?:/[a-z0-9_]+)? so that the regex accepts digit-containing scopes like "x1"
that validateScope permits, preventing validation failures downstream.
In `@internal/interpreter/value.go`:
- Around line 44-49: The NewAccountAddress function does not parse scoped
account strings that contain a scope suffix. When a string like "src/x" is
passed as the src parameter, the entire string is stored in the Name field
without extracting the scope portion. Modify NewAccountAddress to parse the src
string, extract any scope suffix (typically separated by a delimiter), and
populate both the Name and Scope fields of the returned AccountAddress struct
accordingly. The validation with checkAccountName should be applied
appropriately to handle both scoped and unscoped account strings.
---
Nitpick comments:
In `@internal/interpreter/interpreter.go`:
- Around line 581-586: The direct map access on s.CachedBalances[account] at the
point where the ok check fails assumes the balance query batching has already
populated that key, but provides no explicit verification or clear indication
when a cache miss occurs. Instead of returning the generic
InvalidUnboundedAddressInScalingAddress error, add logging or improve the error
message to explicitly indicate that the account's balance was not found in the
cache, making it clear that the issue is a cache miss rather than an unbounded
address problem. This will help surface any gaps in the balance query batching
logic more explicitly.
In `@internal/interpreter/value_test.go`:
- Around line 43-51: The TestMarshalAddress function currently only tests
marshaling of AccountAddress with a Name field but no Scope. Add an additional
test case within TestMarshalAddress to verify that AccountAddress with both Name
and Scope fields (e.g., Name: "abc", Scope: "xyz") marshals correctly to the
format "abc/xyz". This ensures the scope formatting logic is properly covered by
the test suite.
In `@internal/interpreter/value.go`:
- Around line 275-287: The GetBaseAndScale function on the Asset type silently
masks malformed asset strings by returning scale=0 when the scale suffix fails
to parse as an integer, making it impossible for callers to distinguish between
a missing scale and an invalid scale format. Modify the GetBaseAndScale function
signature to return a third error value (changing from (string, int64) to
(string, int64, error)), and when the strconv.ParseInt call fails, return the
error instead of silently returning 0. This allows callers to detect and handle
genuinely malformed assets like "USD/abc" rather than treating them the same as
valid assets without a scale suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b25e2ab-7af6-44c2-882c-bbc9c4753249
📒 Files selected for processing (18)
internal/interpreter/append_scope.gointernal/interpreter/append_scope_test.gointernal/interpreter/args_parser_test.gointernal/interpreter/balances.gointernal/interpreter/batch_balances_query.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/funds_queue.gointernal/interpreter/funds_queue_test.gointernal/interpreter/get_involved_accounts.gointernal/interpreter/internal_balances.gointernal/interpreter/internal_balances_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gointernal/interpreter/value.gointernal/interpreter/value_test.gointernal/specs_format/index.go
💤 Files with no reviewable changes (1)
- internal/interpreter/append_scope.go
✅ Files skipped from review due to trivial changes (1)
- internal/interpreter/internal_balances_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/interpreter/balances.go
- internal/interpreter/function_exprs.go
- internal/interpreter/args_parser_test.go
- internal/interpreter/interpreter_test.go
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 70.04% 70.74% +0.70%
==========================================
Files 55 57 +2
Lines 5024 5258 +234
==========================================
+ Hits 3519 3720 +201
- Misses 1313 1324 +11
- Partials 192 214 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@numscript.go`:
- Line 63: The AccountMetadataRow alias on line 63 points to
interpreter.AccountMetadataRow which is not defined or exported, causing a build
failure. Either locate the correct exported type name from the interpreter
package and update the alias to reference that existing type, or ensure that
AccountMetadataRow is properly exported from the internal/interpreter package so
it can be aliased here.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14d3feb5-8866-4448-9cf0-e92ffcf4d5b8
⛔ Files ignored due to path filters (6)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/metadata.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/override-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/set-account-meta.num.specs.jsonis excluded by!**/*.jsonspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
internal/cmd/test_init.gointernal/interpreter/function_exprs.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gonumscript.gonumscript_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/interpreter/store.go
- internal/interpreter/interpreter_test.go
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/interpreter/accounts_metadata.go (1)
22-30: ⚡ Quick winInclude
Scope/Colorin metadata pretty-print output.Line 27 currently drops scoped/color dimensions, so distinct metadata rows can print identically and become hard to debug.
Suggested refactor
func (m AccountsMetadata) PrettyPrint() string { - header := []string{"Account", "Name", "Value"} + header := []string{"Account", "Name", "Value"} + showColor := false + showScope := false + for _, row := range m { + showColor = showColor || row.Color != "" + showScope = showScope || row.Scope != "" + } + if showColor { + header = append(header, "Color") + } + if showScope { + header = append(header, "Scope") + } var rows [][]string for _, row := range m { - rows = append(rows, []string{row.Account, row.Key, row.Value}) + out := []string{row.Account, row.Key, row.Value} + if showColor { + out = append(out, row.Color) + } + if showScope { + out = append(out, row.Scope) + } + rows = append(rows, out) } return utils.CsvPretty(header, rows, true) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/accounts_metadata.go` around lines 22 - 30, The PrettyPrint method in AccountsMetadata is missing the Scope and Color dimensions in its output, which causes distinct metadata rows to print identically. Add "Scope" and "Color" to the header slice alongside "Account", "Name", and "Value", and then modify the loop that builds rows to include the Scope and Color fields from each row object (row.Scope and row.Color) when appending to the rows slice, ensuring all dimensions are captured in the formatted output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/accounts_metadata.go`:
- Around line 35-44: The CompareAccountsMetadata function uses slices.Contains
to check if elements from slice a exist in slice b, but this approach only
verifies presence and not count, allowing unequal datasets with duplicate values
to incorrectly return true. Fix this by implementing proper multiset comparison
that accounts for element counts: either sort both AccountsMetadata slices and
use slices.Equal for direct comparison, or use a map-based approach to count
occurrences of each element in both slices and verify the counts match. Ensure
the fix correctly handles the case where slices have equal length but different
element frequencies.
---
Nitpick comments:
In `@internal/interpreter/accounts_metadata.go`:
- Around line 22-30: The PrettyPrint method in AccountsMetadata is missing the
Scope and Color dimensions in its output, which causes distinct metadata rows to
print identically. Add "Scope" and "Color" to the header slice alongside
"Account", "Name", and "Value", and then modify the loop that builds rows to
include the Scope and Color fields from each row object (row.Scope and
row.Color) when appending to the rows slice, ensuring all dimensions are
captured in the formatted output.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af3f1cfb-a4a2-4cfc-853a-d9e730c2b869
📒 Files selected for processing (6)
internal/interpreter/accounts_metadata.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/internal_accounts_metadata.gointernal/interpreter/interpreter.gointernal/specs_format/index.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/interpreter/function_exprs.go
- internal/specs_format/index.go
- internal/interpreter/interpreter.go
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
d353ea3 to
62c70e1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
62c70e1 to
c07d256
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The runtime changes are mostly coherent, but the new slice-based metadata comparison can produce false-positive spec assertions when expected metadata contains duplicates.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The runtime scoped-account path is mostly covered, but adding the builtin without updating the public involved-account analyzer leaves scoped scripts failing in that API.
|
@corerabbitai review |
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
f8c7fae to
4eb0731
Compare
🛑 Changes requested — automated reviewThe PR introduces scoped accounts to Numscript but has several significant correctness issues that must be addressed before merging. Two items that were previously flagged as blockers (undefined |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 10 new inline findings.
Summary: #161 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #161 (comment)
|
@coderabbitai review |
✅ Action performedReview finished.
|
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 7 new inline findings.
Summary: #161 (comment)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/interpreter/balances.go (1)
100-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake subset checks multiplicity-aware.
CompareBalancesIncludingstill reuses the first matching row for every duplicate inb1. Sinceinternal/specs_format/index.gouses this helper forexpect.endBalances.include, an assertion like[x, x]currently passes against[x]. Count matches the same way asCompareBalances, or reject duplicate rows up front.Suggested fix
func CompareBalancesIncluding(b1 Balances, b2 Balances) bool { - for _, entry := range b1 { - amount2, ok := findRow(b2, entry.Account, entry.Asset, entry.Color, entry.Scope) - if !ok || !amountsEqual(entry.Amount, amount2) { - return false - } - } - return true + type rowKey struct{ account, asset, color, scope, amount string } + mk := func(r BalanceRow) rowKey { + amount := "0" + if r.Amount != nil { + amount = r.Amount.String() + } + return rowKey{r.Account, r.Asset, r.Color, r.Scope, amount} + } + + counts := make(map[rowKey]int, len(b2)) + for _, r := range b2 { + counts[mk(r)]++ + } + for _, r := range b1 { + k := mk(r) + counts[k]-- + if counts[k] < 0 { + return false + } + } + return true }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/balances.go` around lines 100 - 108, Make CompareBalancesIncluding multiplicity-aware: it currently reuses the same matching row from b2 for duplicate entries in b1, so [x, x] can incorrectly match [x]. Update the logic in CompareBalancesIncluding to track matched rows (or otherwise count occurrences the same way CompareBalances does), or explicitly reject duplicate rows before comparing; keep the behavior consistent with how internal/specs_format/index.go uses this helper for expect.endBalances.include.
♻️ Duplicate comments (1)
internal/interpreter/accounts_metadata.go (1)
35-44: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValue column can be dropped when all values are empty strings.
CsvPrettyOmitEmptyColstreats every column uniformly, but onlyScopeis meant to be optional here per the comment on Line 36. If all rows have an emptyValue, the column disappears entirely, making explicit empty-value metadata indistinguishable from the previous always-shown value output. Flagged previously as unresolved.♻️ Possible fix direction
-func CsvPrettyOmitEmptyCols(header []string, rows [][]string, sortRows bool) string +func CsvPrettyOmitEmptyCols(header []string, rows [][]string, sortRows bool, optionalCols ...int) stringThen only consider columns in
optionalColsfor omission, always keeping the rest (e.g.Value).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/accounts_metadata.go` around lines 35 - 44, AccountsMetadata.PrettyPrint is letting the Value column be omitted because CsvPrettyOmitEmptyCols currently drops any all-empty column, but only Scope should be optional. Update the pretty-print path in AccountsMetadata.PrettyPrint (and/or the CsvPrettyOmitEmptyCols handling it uses) so only columns explicitly marked optional can be removed. Keep Value always included even when every row has an empty string, while preserving the existing Scope-dropping behavior.
🧹 Nitpick comments (3)
internal/specs_format/compare_movements_test.go (1)
10-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd scope-sensitive cases here.
This only proves multiplicity and amount handling.
compareMovementsnow keys onSourceScopeandDestinationScopetoo, so a regression that collapses scoped and unscoped movements together would still pass this test.Suggested addition
func TestCompareMovementsMultiplicity(t *testing.T) { x := Movement{Source: "world", Destination: "a", Asset: "USD", Amount: big.NewInt(1)} y := Movement{Source: "world", Destination: "b", Asset: "USD", Amount: big.NewInt(1)} @@ // differing amount on the same key is not equal z := Movement{Source: "world", Destination: "a", Asset: "USD", Amount: big.NewInt(2)} require.False(t, compareMovements(Movements{x}, Movements{z})) + + // scope is part of the identity + scopedSource := Movement{Source: "world", SourceScope: "s1", Destination: "a", Asset: "USD", Amount: big.NewInt(1)} + scopedDestination := Movement{Source: "world", Destination: "a", DestinationScope: "s1", Asset: "USD", Amount: big.NewInt(1)} + require.False(t, compareMovements(Movements{x}, Movements{scopedSource})) + require.False(t, compareMovements(Movements{x}, Movements{scopedDestination})) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specs_format/compare_movements_test.go` around lines 10 - 25, Update TestCompareMovementsMultiplicity to cover SourceScope and DestinationScope in compareMovements so scoped and unscoped movements are not treated as equal. Add cases using the existing Movement and compareMovements symbols that assert identical Source/Destination/Asset/Amount but different scope values do not compare equal, and that matching scope values still do. Keep the current multiplicity and amount assertions, but extend them with scope-sensitive pairs to prevent regressions where scope is ignored.internal/interpreter/interpreter.go (1)
36-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCopy the amount inside
newPosting.
newPostingstores the caller-owned*big.Intdirectly; thepushReceiverpath passes queue-ownedsender.Amount. Sincebig.Intis mutable, copy it here so emitted postings cannot accidentally alias later queue/balance operations.Proposed fix
func newPosting(source AccountAddress, destination AccountAddress, amount *big.Int, asset string, color string) Posting { + amountCopy := new(big.Int).Set(amount) return Posting{ Source: source.Name, SourceScope: source.Scope, Destination: destination.Name, DestinationScope: destination.Scope, - Amount: amount, + Amount: amountCopy, Asset: asset, Color: color, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` around lines 36 - 49, newPosting is reusing the caller-owned *big.Int directly, which can alias mutable queue/balance state. Update newPosting in interpreter.go to make an internal copy of the amount before assigning it to Posting.Amount, using the existing newPosting helper and its callers like pushReceiver as the fix point. Ensure the returned Posting owns its own amount value so later mutations to sender.Amount cannot affect already-emitted postings.internal/interpreter/value_test.go (1)
91-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider comparing parsed values with
require.Equal, not justString().Comparing only via
v.String()won't catch a bug that swaps struct fields (e.g.Name/Scope) yet happens to produce the same formatted string, or a parse result of the wrong concrete type with a matchingString()output.♻️ Suggested strengthening
parsed, err := interpreter.ParseTaggedValue(j) require.Nil(t, err) - // compare on the canonical source form - require.Equal(t, v.String(), parsed.String()) + require.Equal(t, v, parsed)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value_test.go` around lines 91 - 99, Strengthen the ParseTaggedValue round-trip test in value_test by comparing the parsed value directly instead of only relying on v.String(). Update the assertion in the loop around interpreter.ParseTaggedValue and the value types being marshaled so the test verifies the actual concrete value/state (including struct fields like Name and Scope), which will catch swapped fields or wrong types even when String() matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/value.go`:
- Around line 124-129: The tagged JSON account decode branch in ParseTaggedValue
currently returns an AccountAddress from taggedAccount without validation, so it
can admit invalid name/scope values. Update the valueTypeAccount case in
internal/interpreter/value.go to validate the decoded taggedAccount fields using
the same account-name and scope checks used by the other account-entry paths
before constructing the AccountAddress, and return an error on invalid input so
malformed specs/API payloads are rejected early.
- Around line 44-48: Normalize account inputs in NewAccountAddress before
persisting them: the constructor currently stores the raw src value in
AccountAddress.Name, which can preserve a leading @ and break downstream
rendering/querying. Update NewAccountAddress to either strip a single leading @
from accepted inputs before returning AccountAddress{Name: ...}, or reject
`@-prefixed` values consistently so Name always contains the bare identifier.
In `@internal/specs_format/index.go`:
- Around line 21-35: Update ExpectedTxMeta.UnmarshalJSON so it preserves null as
a nil/empty value instead of always allocating a map, matching the behavior of
the other expect.* fields. Also make it accept legacy scalar metadata entries by
handling non-object values before calling interpreter.ParseTaggedValue, so both
tagged objects and simple string/scalar forms continue to unmarshal
successfully. Then ensure the downstream tx-metadata assertion path that
consumes ExpectedTxMeta does not treat a null input as a failure.
In `@numscript.go`:
- Around line 62-68: The removal of the exported ParseResult.GetInvolvedAccounts
API is a breaking change, so restore backward compatibility or provide a clear
migration path. Update numscript.go to keep the public ParseResult method and
the accounts package symbols, even if they delegate to the new
interpreter-backed implementation or return a limited result for scoped
accounts. If you cannot keep the API, add a compatibility shim or ensure the
change is explicitly called out in the release notes/changelog with the
replacement symbol names.
---
Outside diff comments:
In `@internal/interpreter/balances.go`:
- Around line 100-108: Make CompareBalancesIncluding multiplicity-aware: it
currently reuses the same matching row from b2 for duplicate entries in b1, so
[x, x] can incorrectly match [x]. Update the logic in CompareBalancesIncluding
to track matched rows (or otherwise count occurrences the same way
CompareBalances does), or explicitly reject duplicate rows before comparing;
keep the behavior consistent with how internal/specs_format/index.go uses this
helper for expect.endBalances.include.
---
Duplicate comments:
In `@internal/interpreter/accounts_metadata.go`:
- Around line 35-44: AccountsMetadata.PrettyPrint is letting the Value column be
omitted because CsvPrettyOmitEmptyCols currently drops any all-empty column, but
only Scope should be optional. Update the pretty-print path in
AccountsMetadata.PrettyPrint (and/or the CsvPrettyOmitEmptyCols handling it
uses) so only columns explicitly marked optional can be removed. Keep Value
always included even when every row has an empty string, while preserving the
existing Scope-dropping behavior.
---
Nitpick comments:
In `@internal/interpreter/interpreter.go`:
- Around line 36-49: newPosting is reusing the caller-owned *big.Int directly,
which can alias mutable queue/balance state. Update newPosting in interpreter.go
to make an internal copy of the amount before assigning it to Posting.Amount,
using the existing newPosting helper and its callers like pushReceiver as the
fix point. Ensure the returned Posting owns its own amount value so later
mutations to sender.Amount cannot affect already-emitted postings.
In `@internal/interpreter/value_test.go`:
- Around line 91-99: Strengthen the ParseTaggedValue round-trip test in
value_test by comparing the parsed value directly instead of only relying on
v.String(). Update the assertion in the loop around interpreter.ParseTaggedValue
and the value types being marshaled so the test verifies the actual concrete
value/state (including struct fields like Name and Scope), which will catch
swapped fields or wrong types even when String() matches.
In `@internal/specs_format/compare_movements_test.go`:
- Around line 10-25: Update TestCompareMovementsMultiplicity to cover
SourceScope and DestinationScope in compareMovements so scoped and unscoped
movements are not treated as equal. Add cases using the existing Movement and
compareMovements symbols that assert identical Source/Destination/Asset/Amount
but different scope values do not compare equal, and that matching scope values
still do. Keep the current multiplicity and amount assertions, but extend them
with scope-sensitive pairs to prevent regressions where scope is ignored.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2380aed5-5081-4908-a258-85f7a743da3f
⛔ Files ignored due to path filters (30)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/__snapshots__/accounts_metadata_test.snapis excluded by!**/*.snap,!**/*.snapinternal/interpreter/__snapshots__/pretty_print_meta_test.snapis excluded by!**/*.snap,!**/*.snapinternal/interpreter/__snapshots__/pretty_print_postings_test.snapis excluded by!**/*.snap,!**/*.snapinternal/interpreter/testdata/numscript-cookbook/experimental/meta-calc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/add-numbers.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/account-interpolation/account-interp.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-amount-function/get-amount-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-asset-function/get-asset-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/capped.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/color-and-scope.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/read-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/save.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/set-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/metadata.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/override-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/set-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/set-tx-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/sub-monetaries.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/sub-numbers.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables-json.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables.num.specs.jsonis excluded by!**/*.jsoninternal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snapinternal/utils/__snapshots__/pretty_csv_test.snapis excluded by!**/*.snap,!**/*.snapspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (48)
internal/analysis/check.gointernal/cmd/run.gointernal/cmd/test_init.gointernal/cmd/test_init_test.gointernal/flags/flags.gointernal/interpreter/accounts_metadata.gointernal/interpreter/accounts_metadata_test.gointernal/interpreter/append_scope.gointernal/interpreter/append_scope_test.gointernal/interpreter/args_parser_test.gointernal/interpreter/balances.gointernal/interpreter/balances_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/evaluate_expr.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/funds_queue.gointernal/interpreter/funds_queue_test.gointernal/interpreter/internal_accounts_metadata.gointernal/interpreter/internal_balances.gointernal/interpreter/internal_balances_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_error.gointernal/interpreter/interpreter_test.gointernal/interpreter/pretty_print_meta_test.gointernal/interpreter/pretty_print_postings_test.gointernal/interpreter/set_accounts_metadata.gointernal/interpreter/store.gointernal/interpreter/testdata/script-tests/experimental/scoped-function/allotment.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/balance.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/capped.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/color-and-scope.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/overdraft.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/read-account-meta.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/save.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/set-account-meta.numinternal/interpreter/testdata/script-tests/experimental/scoped-function/simple.numinternal/interpreter/value.gointernal/interpreter/value_test.gointernal/mcp_impl/handlers.gointernal/mcp_impl/handlers_test.gointernal/specs_format/compare_movements_test.gointernal/specs_format/index.gointernal/specs_format/runner_test.gointernal/utils/pretty_csv.gointernal/utils/pretty_csv_test.gonumscript.gonumscript_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/interpreter/testdata/script-tests/experimental/scoped-function/capped.num
- internal/interpreter/testdata/script-tests/experimental/scoped-function/set-account-meta.num
- internal/interpreter/testdata/script-tests/experimental/scoped-function/save.num
- internal/interpreter/args_parser_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/interpreter/internal_balances_test.go
- internal/cmd/test_init.go
- internal/interpreter/funds_queue.go
- internal/interpreter/funds_queue_test.go
- internal/flags/flags.go
- internal/interpreter/function_statements.go
- internal/interpreter/append_scope.go
- numscript_test.go
- internal/interpreter/function_exprs.go
- internal/interpreter/testdata/script-tests/experimental/scoped-function/simple.num
- internal/interpreter/store.go
- internal/interpreter/interpreter_test.go
| func (m *ExpectedTxMeta) UnmarshalJSON(data []byte) error { | ||
| var raw map[string]json.RawMessage | ||
| if err := json.Unmarshal(data, &raw); err != nil { | ||
| return err | ||
| } | ||
| out := ExpectedTxMeta{} | ||
| for k, v := range raw { | ||
| value, err := interpreter.ParseTaggedValue(v) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| out[k] = value | ||
| } | ||
| *m = out | ||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve null and scalar tx-metadata compatibility.
Line 21 now always materializes a non-nil map, so "expect.txMetadata": null starts triggering the assertion at Line 200 instead of behaving like the other expect.* fields. It also rejects existing scalar forms such as "k": "v", because ParseTaggedValue only accepts tagged objects. Both are user-visible spec-format regressions.
Suggested fix
+import "bytes"
+
func (m *ExpectedTxMeta) UnmarshalJSON(data []byte) error {
+ if bytes.Equal(bytes.TrimSpace(data), []byte("null")) {
+ *m = nil
+ return nil
+ }
+
var raw map[string]json.RawMessage
if err := json.Unmarshal(data, &raw); err != nil {
return err
}
- out := ExpectedTxMeta{}
+ out := make(ExpectedTxMeta, len(raw))
for k, v := range raw {
value, err := interpreter.ParseTaggedValue(v)
if err != nil {
- return err
+ var s string
+ if err2 := json.Unmarshal(v, &s); err2 == nil {
+ out[k] = interpreter.String(s)
+ continue
+ }
+ return err
}
out[k] = value
}
*m = out
return nil
}Also applies to: 200-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/specs_format/index.go` around lines 21 - 35, Update
ExpectedTxMeta.UnmarshalJSON so it preserves null as a nil/empty value instead
of always allocating a map, matching the behavior of the other expect.* fields.
Also make it accept legacy scalar metadata entries by handling non-object values
before calling interpreter.ParseTaggedValue, so both tagged objects and simple
string/scalar forms continue to unmarshal successfully. Then ensure the
downstream tx-metadata assertion path that consumes ExpectedTxMeta does not
treat a null input as a failure.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #161 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #161 (comment)
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/interpreter/accounts_metadata_test.go (1)
1-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing coverage for the actual
AccountsMetadatamultiset comparison.This test thoroughly covers multiplicity semantics for
CompareSetAccountsMetadata/SetAccountsMetadata, but this file (the row-basedAccountsMetadatamodel layer) has no equivalent test for the plainAccountsMetadatacomparison function. Per review feedback on this PR, that comparison "uses a subset-style check instead of a multiset comparison, which can hide failures when duplicate rows exist." A parallel test here (mirroring this one, but againstAccountsMetadata/AccountMetadataRow) would have caught that regression.Want me to draft a
TestCompareAccountsMetadatatest with the same multiplicity cases as here?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/accounts_metadata_test.go` around lines 1 - 46, The row-based AccountsMetadata comparison is missing the same multiplicity coverage already exercised for CompareSetAccountsMetadata. Add a parallel TestCompareAccountsMetadata in the interpreter test suite that uses AccountMetadataRow and AccountsMetadata to verify order-insensitive equality plus duplicate-sensitive multiset behavior, including cases like [x, x] vs [x, y] and identical duplicates, so the plain comparison can’t pass with subset-style logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/interpreter/accounts_metadata_test.go`:
- Around line 1-46: The row-based AccountsMetadata comparison is missing the
same multiplicity coverage already exercised for CompareSetAccountsMetadata. Add
a parallel TestCompareAccountsMetadata in the interpreter test suite that uses
AccountMetadataRow and AccountsMetadata to verify order-insensitive equality
plus duplicate-sensitive multiset behavior, including cases like [x, x] vs [x,
y] and identical duplicates, so the plain comparison can’t pass with
subset-style logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26bb24af-4775-4985-9da6-e625fcba4f38
⛔ Files ignored due to path filters (12)
internal/interpreter/testdata/numscript-cookbook/experimental/meta-calc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/add-numbers.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/account-interpolation/account-interp.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-amount-function/get-amount-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-asset-function/get-asset-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/set-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/set-tx-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/sub-monetaries.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/sub-numbers.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables-json.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables.num.specs.jsonis excluded by!**/*.jsonspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
internal/interpreter/accounts_metadata_test.gointernal/interpreter/function_exprs.gointernal/interpreter/interpreter.gointernal/interpreter/value.gointernal/interpreter/value_test.gointernal/specs_format/index.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/interpreter/function_exprs.go
- internal/interpreter/value_test.go
- internal/interpreter/interpreter.go
- internal/interpreter/value.go
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 11 new inline findings.
Summary: #161 (comment)
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 12 new inline findings.
Summary: #161 (comment)
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalScopedFunction, |
There was a problem hiding this comment.
🟠 [major] Wrong feature flag gates scoped() in static analysis
reported by NumaryBot, codex
scoped() is registered in the static analyzer under experimental-get-amount-function instead of experimental-scoped-function. Scripts that enable the documented scoped flag still receive a missing-flag diagnostic from numscript check/LSP, while scripts enabling only the get-amount flag pass static analysis but fail at runtime because the interpreter checks ExperimentalScopedFunction. This analysis/runtime mismatch makes the feature unreliable for any user relying on static checking.
Suggestion: Change the feature-flag constraint for the scoped builtin in the checker to reference experimental-scoped-function / ExperimentalScopedFunction, matching what the interpreter enforces at runtime.
| Params: []string{TypeAccount, TypeString}, | ||
| Return: TypeAccount, | ||
| Docs: "returns the scoped version of that account. Empty string means no scope. Overwrites the previous scope", | ||
| VersionConstraints: []VersionClause{ |
There was a problem hiding this comment.
🟠 [major] Mid-script function call flag not required for scoped() expression usage
When scoped() is used in mid-script expression positions (e.g., source = scoped(@src, "s") or balance(scoped(@src, "s"), USD)), the runtime's evaluateExpr requires experimental-mid-script-function-call in addition to experimental-scoped-function. The analyzer only checks for the scoped flag, so scripts accepted by numscript check can still fail at runtime unless both flags are enabled. The requirement should be made consistent between the checker and interpreter.
Suggestion: Either require both flags in the checker registration, or ensure the runtime does not require the mid-script flag for scoped() expressions, making the two consistent.
| return AccountAddress{}, InvalidAccountName{Name: src} | ||
| } | ||
| return AccountAddress(src), nil | ||
| return AccountAddress{Name: src}, nil |
There was a problem hiding this comment.
🟠 [major] Slash-scoped account strings accepted but not split into AccountAddress.Scope
When an account value arrives as a raw string in the form src/x (the format emitted by AccountAddress.String()), the updated regex accepts it but NewAccountAddress stores the entire string in Name with an empty Scope. Balance and metadata lookups therefore query account="src/x", scope="" instead of account="src", scope="x", so they cannot match scoped rows and postings won't populate sourceScope/destinationScope.
Suggestion: In NewAccountAddress, split the accepted string on / to populate Name and Scope separately when a slash is present.
| Account string | ||
| Asset string | ||
| Color string | ||
| Scope string |
There was a problem hiding this comment.
🟠 [major] TestInitStore does not propagate Scope when seeding generated balances
TestInitStore.GetBalances keys and appends generated balances only by account/asset/color, omitting Scope. When test-init funds a scoped source (e.g., scoped(@src, "x")), the generated spec records an unscoped balance. Replaying the spec then queries the scoped balance as zero and produces incorrect expected balances or assertion failures.
Suggestion: Include Scope in the key used for deduplication and populate BalanceRow{Scope: b.Scope} when appending generated balance rows to the spec store.
| } | ||
|
|
||
| return utils.CsvPretty(header, rows, true) | ||
| return utils.CsvPrettyOmitEmptyCols(header, rows, true) |
There was a problem hiding this comment.
🟠 [major] CompareAccountsMetadata uses subset check instead of multiset comparison
slices.Contains only checks presence, not count. Two AccountsMetadata slices that differ only in duplicate rows can incorrectly compare as equal (e.g., expected [A, A] vs. actual [A, B] when lengths match), masking spec assertion failures where expected rows appear more times than actual rows.
Suggestion: Use a frequency-count map: increment counts for each row in slice a, decrement for each row in b, and return false if any count goes below zero.
| return AccountAddress{Name: v.Name, Scope: v.Scope}, nil | ||
|
|
||
| case valueTypeAsset: | ||
| var v taggedAsset |
There was a problem hiding this comment.
🟡 [minor] Asset values not validated when decoding tagged JSON
Tagged JSON account values are validated before becoming in-memory values, but tagged asset values are accepted directly without validation. Payloads like {"type":"asset","name":"bad asset"} can create impossible Asset values that all normal DSL and variable-entry paths would reject, meaning malformed metadata expectations are only caught much later or not at all at the boundary.
Suggestion: Apply the same asset-name validation used elsewhere (e.g., checkAssetName) to the asset and monetary branches of ParseTaggedValue before returning the value.
| return CsvPretty(header, rows, sortRows) | ||
| } | ||
|
|
||
| keep := make([]bool, len(header)) |
There was a problem hiding this comment.
🟡 [minor] CsvPrettyOmitEmptyCols renders empty header for empty result sets
When rows is empty, every keep flag remains false, so the function renders a table with an empty header (|) for empty postings, balances, or metadata. This regresses prior behavior that showed expected column headers even for empty results.
Suggestion: Special-case an empty rows slice to keep all non-optional headers, or treat the header row itself as a signal to preserve column visibility.
| Asset string `json:"asset"` | ||
| Amount *big.Int `json:"amount"` | ||
| Color string `json:"color,omitempty"` | ||
| Scope string `json:"scope,omitempty"` |
There was a problem hiding this comment.
🟡 [minor] Scope not shown in balance pretty-print output
Balances.PrettyPrint renders only account/asset/color/amount columns, so two rows that differ only by scope appear as indistinguishable entries in the pretty output, making debugging difficult.
Suggestion: Add a Scope column to the pretty-printer when any row in the result set has a non-empty scope.
| "required": ["type", "name"], | ||
| "properties": { | ||
| "type": { "const": "asset" }, | ||
| "name": { "type": "string", "pattern": "^([A-Z]+(/[0-9]+)?)$" } |
There was a problem hiding this comment.
🟡 [minor] Asset pattern in spec schema is narrower than the runtime's checkAssetName
The new Value schema for tagged metadata assets only allows letters plus an optional scale, but the interpreter's checkAssetName also accepts digits and an optional _SUFFIX (e.g., USD_TEST/2). Schema validation therefore rejects spec files containing runtime-valid assets.
Suggestion: Align the asset regex in the schema with the pattern actually enforced by checkAssetName in the interpreter.
| if err != nil { | ||
| return nil, err | ||
| return mcp.NewToolResultError(err.Error()), nil | ||
| } |
There was a problem hiding this comment.
🟡 [minor] Inconsistent error handling for BindArguments failure
All other errors in this handler are returned as mcp.NewToolResultError(...) with a nil transport error. A BindArguments failure returns (nil, err), propagating as a server-level error with potentially different client-visible behavior.
Suggestion: Change the BindArguments error path to return mcp.NewToolResultError(err.Error()), nil to be consistent with other error returns in the handler.
2 participants
This PR implements a scoped() function which "tags" accounts as scoped. We now carry around a {name: string, scope: string} struct to represent accounts. No scope is the same as empty scope (empty string). Semantics are same as before, we just represent account with this tuple instead. As a consequence, metadata is keyed by scope as well. This also involves the contract with the numscript consumer
In order to avoid using some "special" syntax to encode that (e.g.
acc/scope) we also needed to add aaccountScopeanddestinationScopefield in a bunch of place where json repr is involved: inputs format, specs format, postings.The reason why we aren't using special syntax for the scope had been discussed in previous threads; the TL;DR is:
#but we want to be able to reserve that for other stuff in the future@acc # "scope"(scopes must be repr'd as a string because they can be variable, and we don't want to introduce a new ad-hoc datatype just for this, for now)