feat: scoped accounts#161
Open
ascandone wants to merge 16 commits into
Open
Conversation
Contributor
|
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:
WalkthroughThe PR migrates the interpreter balance representation from a map to a row-based ChangesBalances model, typed value semantics, and scoped builtin
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Analyzer
participant Interpreter
participant CachedBalances
participant Store
Parser->>Analyzer: scoped(`@src`, "x")
Analyzer->>Analyzer: checkFnCall validates FnVarOriginScoped registered
Interpreter->>Interpreter: evaluate scoped(account, scope)
Interpreter->>Interpreter: expectAccount validates `@src`
Interpreter->>Interpreter: expectString extracts "x"
Interpreter->>Interpreter: validateScope checks ^[a-z0-9_]*$
Interpreter-->>Parser: AccountAddress{Name: "src", Scope: "x"}
Parser->>Interpreter: send via scoped source
Interpreter->>CachedBalances: fetchBalance(src/x, asset, color)
CachedBalances-->>Interpreter: *big.Int balance
Interpreter->>Store: query balances for src/x
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
An error occurred while trying to automatically change base from
feat/color-as-first-class-citizen
to
main
June 15, 2026 13:13
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}
Contributor
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
Comment on lines
+154
to
+158
| isValidColor := colorRe.Match([]byte(string(color))) | ||
| if !isValidColor { | ||
| return nil, InvalidColor{ | ||
| return "", InvalidColor{ | ||
| Range: colorExpr.GetRange(), | ||
| Color: *color, | ||
| Color: string(color), |
Contributor
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.
Suggested change
| 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().
Comment on lines
+69
to
72
| err = request.BindArguments(&args) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent error handling for BindArguments failure.
Other errors in this handler (e.g., RequireString at line 53, parse errors at line 62, interpreter errors at line 84) return (mcp.NewToolResultError(...), nil), treating them as tool-level errors. Returning (nil, err) here propagates the error as a server-level error, which may cause different client behavior (e.g., no structured error message).
Proposed fix for consistent error handling
err = request.BindArguments(&args)
if err != nil {
- return nil, err
+ return mcp.NewToolResultError(err.Error()), nil
}📝 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.
Suggested change
| err = request.BindArguments(&args) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = request.BindArguments(&args) | |
| if err != nil { | |
| return mcp.NewToolResultError(err.Error()), nil | |
| } |
🤖 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/mcp_impl/handlers.go` around lines 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.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
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 67.81% 69.66% +1.84%
==========================================
Files 49 58 +9
Lines 5223 5531 +308
==========================================
+ Hits 3542 3853 +311
+ Misses 1472 1460 -12
- Partials 209 218 +9 ☔ 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.
Contributor
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.
Contributor
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.
Model accounts as a struct carrying an optional scope instead of encoding
it into the address string. Scope flows structured through balances,
postings, the funds queue, balance/metadata queries and the store.
Account metadata is now row-based externally (AccountsMetadata as a list of
AccountMetadataRow, mirroring Balances) and indexed by a {account, scope,
key} map internally (InternalAccountsMetadata) for fast lookups.
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
reviewed
Jun 25, 2026
NumaryBot
left a comment
NumaryBot
left a comment
Contributor
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
reviewed
Jun 25, 2026
NumaryBot
left a comment
NumaryBot
left a comment
Contributor
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.
Contributor
Author
|
@corerabbitai review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.