feat: builtins for latest conformance suite #180
Conversation
wolf31o2
requested review from
MicroProofs,
Salvionied and
rvcas
as code owners
|
Important Review skippedToo many files! 150 files out of 300 files are above the max files limit of 150. You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 616 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cek/ex_budget.go">
<violation number="1" location="cek/ex_budget.go:10">
P1: Setting production default budget to `math.MaxInt64` just to accommodate tests is problematic. This effectively disables execution budget limits for all production consumers of this package. Tests that need higher limits should override `Machine.ExBudget` directly (which is already a public field) or use a test-specific constant, rather than changing the default for everyone. Consider keeping reasonable production defaults and using `m.ExBudget = ExBudget{Mem: math.MaxInt64, Cpu: math.MaxInt64}` in the specific tests that need it.</violation>
</file>
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:3803">
P1: Duplicate policy entries in the input are not merged, leading to data loss. Unlike duplicate tokens (which are summed), duplicate policies overwrite each other. Consider reusing the existing tokenMap if the policy already exists in the output map.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
cek/ex_budget.go
Outdated
| Mem: 14_000_000, | ||
| Cpu: 10_000_000_000, | ||
| // Increase default budget to accommodate tests expecting near-max CPU usage | ||
| Mem: 9_223_372_036_854_775_807, |
There was a problem hiding this comment.
P1: Setting production default budget to math.MaxInt64 just to accommodate tests is problematic. This effectively disables execution budget limits for all production consumers of this package. Tests that need higher limits should override Machine.ExBudget directly (which is already a public field) or use a test-specific constant, rather than changing the default for everyone. Consider keeping reasonable production defaults and using m.ExBudget = ExBudget{Mem: math.MaxInt64, Cpu: math.MaxInt64} in the specific tests that need it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/ex_budget.go, line 10:
<comment>Setting production default budget to `math.MaxInt64` just to accommodate tests is problematic. This effectively disables execution budget limits for all production consumers of this package. Tests that need higher limits should override `Machine.ExBudget` directly (which is already a public field) or use a test-specific constant, rather than changing the default for everyone. Consider keeping reasonable production defaults and using `m.ExBudget = ExBudget{Mem: math.MaxInt64, Cpu: math.MaxInt64}` in the specific tests that need it.</comment>
<file context>
@@ -6,8 +6,9 @@ type ExBudget struct {
- Mem: 14_000_000,
- Cpu: 10_000_000_000,
+ // Increase default budget to accommodate tests expecting near-max CPU usage
+ Mem: 9_223_372_036_854_775_807,
+ Cpu: 9_223_372_036_854_775_807,
}
</file context>
✅ Addressed in 9ac5746
cek/builtins.go
Outdated
| if ok { | ||
| policy = string(policyBs.Inner) | ||
| } | ||
| tokenMap := make(map[string]*big.Int) |
There was a problem hiding this comment.
P1: Duplicate policy entries in the input are not merged, leading to data loss. Unlike duplicate tokens (which are summed), duplicate policies overwrite each other. Consider reusing the existing tokenMap if the policy already exists in the output map.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3803:
<comment>Duplicate policy entries in the input are not merged, leading to data loss. Unlike duplicate tokens (which are summed), duplicate policies overwrite each other. Consider reusing the existing tokenMap if the policy already exists in the output map.</comment>
<file context>
@@ -3554,24 +3659,535 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ if ok {
+ policy = string(policyBs.Inner)
+ }
+ tokenMap := make(map[string]*big.Int)
+ if lst, ok := pair.Second.(*syn.ProtoList); ok {
+ for _, tk := range lst.List {
</file context>
| tokenMap := make(map[string]*big.Int) | |
| tokenMap, exists := out[policy] | |
| if !exists { | |
| tokenMap = make(map[string]*big.Int) | |
| } |
✅ Addressed in 9ac5746
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
b86c257 to
9ac5746
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
9ac5746 to
25e6d9e
Compare
There was a problem hiding this comment.
5 issues found across 617 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cek/cost_model_builtins.go">
<violation number="1" location="cek/cost_model_builtins.go:676">
P2: InsertCoin, ScaleValue, and UnionValue have placeholder costs of 100 billion units (mem and cpu), making these operations ~300x more expensive than the costliest real builtin. If these are intentional placeholders to disable the builtins until properly calibrated, consider adding a TODO comment explaining this. If they should be usable, proper cost calibration is needed.</violation>
</file>
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:3674">
P1: Error from `m.CostTwo` is ignored. This is inconsistent with the rest of the codebase where cost function errors are always checked and returned. If the budget is exceeded, the operation should fail rather than continue.</violation>
<violation number="2" location="cek/builtins.go:3731">
P1: Error from `m.CostOne` is ignored. The cost function error should be checked and returned to properly enforce budget limits, consistent with existing patterns in the codebase.</violation>
<violation number="3" location="cek/builtins.go:3748">
P1: Error from `m.CostOne` is ignored. This should be checked and returned to enforce budget limits properly.</violation>
<violation number="4" location="cek/builtins.go:3928">
P1: Error from `m.CostOne` is ignored. The same pattern occurs in `lookupCoin`, `scaleValue`, `unionValue`, and `valueContains`. All cost function errors should be checked and returned.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| cpu: &ConstantCost{232010}, | ||
| }, | ||
| // Value/coin builtins | ||
| builtin.InsertCoin: &CostingFunc[Arguments]{ |
There was a problem hiding this comment.
P2: InsertCoin, ScaleValue, and UnionValue have placeholder costs of 100 billion units (mem and cpu), making these operations ~300x more expensive than the costliest real builtin. If these are intentional placeholders to disable the builtins until properly calibrated, consider adding a TODO comment explaining this. If they should be usable, proper cost calibration is needed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/cost_model_builtins.go, line 676:
<comment>InsertCoin, ScaleValue, and UnionValue have placeholder costs of 100 billion units (mem and cpu), making these operations ~300x more expensive than the costliest real builtin. If these are intentional placeholders to disable the builtins until properly calibrated, consider adding a TODO comment explaining this. If they should be usable, proper cost calibration is needed.</comment>
<file context>
@@ -633,12 +647,53 @@ var DefaultBuiltinCosts = BuiltinCosts{
+ cpu: &ConstantCost{232010},
+ },
+ // Value/coin builtins
+ builtin.InsertCoin: &CostingFunc[Arguments]{
+ mem: &ConstantCost{100000000000},
+ cpu: &ConstantCost{100000000000},
</file context>
✅ Addressed in 628b15e
cek/builtins.go
Outdated
| return nil, errors.New("unimplemented: lengthOfArray") | ||
| switch c := lstConst.Constant.(type) { | ||
| case *syn.ProtoList: | ||
| _ = m.CostOne(&b.Func, func() ExMem { return listExMem(c.List)() }) |
There was a problem hiding this comment.
P1: Error from m.CostOne is ignored. The cost function error should be checked and returned to properly enforce budget limits, consistent with existing patterns in the codebase.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3731:
<comment>Error from `m.CostOne` is ignored. The cost function error should be checked and returned to properly enforce budget limits, consistent with existing patterns in the codebase.</comment>
<file context>
@@ -3554,24 +3659,539 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
- return nil, errors.New("unimplemented: lengthOfArray")
+ switch c := lstConst.Constant.(type) {
+ case *syn.ProtoList:
+ _ = m.CostOne(&b.Func, func() ExMem { return listExMem(c.List)() })
+ l := big.NewInt(int64(len(c.List)))
+ return &Constant{&syn.Integer{Inner: l}}, nil
</file context>
| _ = m.CostOne(&b.Func, func() ExMem { return listExMem(c.List)() }) | |
| if err := m.CostOne(&b.Func, func() ExMem { return listExMem(c.List)() }); err != nil { | |
| return nil, err | |
| } |
✅ Addressed in 628b15e
cek/builtins.go
Outdated
| return nil, errors.New("insertCoin: expected value proto list") | ||
| } | ||
| // Spend budget for insertCoin | ||
| _ = m.CostOne(&b.Func, func() ExMem { return listExMem(plist.List)() }) |
There was a problem hiding this comment.
P1: Error from m.CostOne is ignored. The same pattern occurs in lookupCoin, scaleValue, unionValue, and valueContains. All cost function errors should be checked and returned.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3928:
<comment>Error from `m.CostOne` is ignored. The same pattern occurs in `lookupCoin`, `scaleValue`, `unionValue`, and `valueContains`. All cost function errors should be checked and returned.</comment>
<file context>
@@ -3554,24 +3659,539 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ return nil, errors.New("insertCoin: expected value proto list")
+ }
+ // Spend budget for insertCoin
+ _ = m.CostOne(&b.Func, func() ExMem { return listExMem(plist.List)() })
+
+ // Enforce key length constraints (<= 32 bytes) per builtin semantics, except
</file context>
| _ = m.CostOne(&b.Func, func() ExMem { return listExMem(plist.List)() }) | |
| if err := m.CostOne(&b.Func, func() ExMem { return listExMem(plist.List)() }); err != nil { | |
| return nil, err | |
| } |
✅ Addressed in 628b15e
cek/builtins.go
Outdated
| return nil, err | ||
| } | ||
| // Charge based on list length rather than full constant ExMem to match cost model intent | ||
| _ = m.CostOne(&b.Func, func() ExMem { return listLengthExMem(lst.List)() }) |
There was a problem hiding this comment.
P1: Error from m.CostOne is ignored. This should be checked and returned to enforce budget limits properly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3748:
<comment>Error from `m.CostOne` is ignored. This should be checked and returned to enforce budget limits properly.</comment>
<file context>
@@ -3554,24 +3659,539 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ return nil, err
+ }
+ // Charge based on list length rather than full constant ExMem to match cost model intent
+ _ = m.CostOne(&b.Func, func() ExMem { return listLengthExMem(lst.List)() })
+ return &Constant{&syn.ProtoList{LTyp: lst.LTyp, List: lst.List}}, nil
}
</file context>
| _ = m.CostOne(&b.Func, func() ExMem { return listLengthExMem(lst.List)() }) | |
| if err := m.CostOne(&b.Func, func() ExMem { return listLengthExMem(lst.List)() }); err != nil { | |
| return nil, err | |
| } |
✅ Addressed in 628b15e
cek/builtins.go
Outdated
| } | ||
|
|
||
| // Spend budget (base) | ||
| _ = m.CostTwo( |
There was a problem hiding this comment.
P1: Error from m.CostTwo is ignored. This is inconsistent with the rest of the codebase where cost function errors are always checked and returned. If the budget is exceeded, the operation should fail rather than continue.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3674:
<comment>Error from `m.CostTwo` is ignored. This is inconsistent with the rest of the codebase where cost function errors are always checked and returned. If the budget is exceeded, the operation should fail rather than continue.</comment>
<file context>
@@ -3554,24 +3659,539 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ }
+
+ // Spend budget (base)
+ _ = m.CostTwo(
+ &b.Func,
+ func() ExMem { return listExMem(lst.List)() },
</file context>
✅ Addressed in 628b15e
wolf31o2
force-pushed
the
feat/builtins-v4
branch
4 times, most recently
from
5f00fe7 to
fb3ab9a
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 618 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="syn/parser.go">
<violation number="1" location="syn/parser.go:894">
P2: Error message is misleading: says "expected bytestring value" but actually expects a point value (TokenPoint).</violation>
</file>
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:3799">
P1: The error from `m.CostTwo` is ignored, which means budget exhaustion will not cause this function to fail. All other cost functions in this file properly check and return the error.</violation>
</file>
<file name="cek/machine.go">
<violation number="1" location="cek/machine.go:570">
P2: Branch count validation for Bool/List (branchRule 2) allows 0 branches, contradicting the comment "(1 or 2)". When 0 branches are provided, the code will pass validation but fail later with `MissingCaseBranch` instead of the expected `InvalidCaseBranchCount`. Consider adding a lower bound check.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
syn/parser.go
Outdated
| return &Unit{}, nil | ||
| case *TBls12_381G1Element: | ||
| if p.curToken.Type != lex.TokenPoint { | ||
| return nil, fmt.Errorf("expected bytestring value, got %v at position %d", p.curToken.Type, p.curToken.Position) |
There was a problem hiding this comment.
P2: Error message is misleading: says "expected bytestring value" but actually expects a point value (TokenPoint).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/parser.go, line 894:
<comment>Error message is misleading: says "expected bytestring value" but actually expects a point value (TokenPoint).</comment>
<file context>
@@ -684,6 +889,58 @@ func (p *Parser) parseConstantValue(typ Typ) (IConstant, error) {
return &Unit{}, nil
+ case *TBls12_381G1Element:
+ if p.curToken.Type != lex.TokenPoint {
+ return nil, fmt.Errorf("expected bytestring value, got %v at position %d", p.curToken.Type, p.curToken.Position)
+ }
+
</file context>
✅ Addressed in 03ec4fb
cek/builtins.go
Outdated
| _ = m.CostTwo( | ||
| &b.Func, | ||
| func() ExMem { return listExMem(lst.List)() }, | ||
| bigIntExMem(idx), | ||
| ) |
There was a problem hiding this comment.
P1: The error from m.CostTwo is ignored, which means budget exhaustion will not cause this function to fail. All other cost functions in this file properly check and return the error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3799:
<comment>The error from `m.CostTwo` is ignored, which means budget exhaustion will not cause this function to fail. All other cost functions in this file properly check and return the error.</comment>
<file context>
@@ -3554,24 +3682,563 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ }
+
+ // Spend budget
+ _ = m.CostTwo(
+ &b.Func,
+ func() ExMem { return listExMem(lst.List)() },
</file context>
| _ = m.CostTwo( | |
| &b.Func, | |
| func() ExMem { return listExMem(lst.List)() }, | |
| bigIntExMem(idx), | |
| ) | |
| if err := m.CostTwo( | |
| &b.Func, | |
| func() ExMem { return listExMem(lst.List)() }, | |
| bigIntExMem(idx), | |
| ); err != nil { | |
| return nil, err | |
| } |
✅ Addressed in 4780870
| if len(c.Branches) != 1 { | ||
| return nil, errors.New("InvalidCaseBranchCount") | ||
| } | ||
| case 2: |
There was a problem hiding this comment.
P2: Branch count validation for Bool/List (branchRule 2) allows 0 branches, contradicting the comment "(1 or 2)". When 0 branches are provided, the code will pass validation but fail later with MissingCaseBranch instead of the expected InvalidCaseBranchCount. Consider adding a lower bound check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/machine.go, line 570:
<comment>Branch count validation for Bool/List (branchRule 2) allows 0 branches, contradicting the comment "(1 or 2)". When 0 branches are provided, the code will pass validation but fail later with `MissingCaseBranch` instead of the expected `InvalidCaseBranchCount`. Consider adding a lower bound check.</comment>
<file context>
@@ -494,6 +494,95 @@ func (m *Machine[T]) returnCompute(
+ if len(c.Branches) != 1 {
+ return nil, errors.New("InvalidCaseBranchCount")
+ }
+ case 2:
+ // up to 2
+ if len(c.Branches) > 2 {
</file context>
✅ Addressed in 4780870
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
fb3ab9a to
4780870
Compare
|
@coderabbitai review this please. Most of the files are conformance test vectors and do not need to be reviewed. Only the *.go files need review. |
|
@wolf31o2: I'll review the Go files in this PR, focusing on the implementation of the new builtins (BLS12-381 multi-scalar multiplication, Value operations, array utilities, and case-on-constant semantics) and skipping the conformance test vectors. ✅ Actions performedReview triggered.
|
|
@cubic-dev-ai review this PR and focus on the go files, not the test vectors |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 618 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="syn/parser.go">
<violation number="1" location="syn/parser.go:920">
P2: Misleading error message: says "expected bytestring value" but should say "expected point value" to match the G1 case and the actual `TokenPoint` check.</violation>
</file>
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:3722">
P1: Ignoring error from `spendBudget` allows execution to continue when budget is exceeded. If budget enforcement is intended, the error should be checked and propagated. If this is intentional for calibration purposes, consider adding a comment explaining why.</violation>
<violation number="2" location="cek/builtins.go:4228">
P1: Ignoring error from `spendBudget` allows execution to continue when budget is exceeded. Consider checking the error: `if err := m.spendBudget(...); err != nil { return nil, err }`</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
syn/parser.go
Outdated
| return &Bls12_381G1Element{Inner: jac}, nil | ||
| case *TBls12_381G2Element: | ||
| if p.curToken.Type != lex.TokenPoint { | ||
| return nil, fmt.Errorf("expected bytestring value, got %v at position %d", p.curToken.Type, p.curToken.Position) |
There was a problem hiding this comment.
P2: Misleading error message: says "expected bytestring value" but should say "expected point value" to match the G1 case and the actual TokenPoint check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/parser.go, line 920:
<comment>Misleading error message: says "expected bytestring value" but should say "expected point value" to match the G1 case and the actual `TokenPoint` check.</comment>
<file context>
@@ -684,6 +889,58 @@ func (p *Parser) parseConstantValue(typ Typ) (IConstant, error) {
+ return &Bls12_381G1Element{Inner: jac}, nil
+ case *TBls12_381G2Element:
+ if p.curToken.Type != lex.TokenPoint {
+ return nil, fmt.Errorf("expected bytestring value, got %v at position %d", p.curToken.Type, p.curToken.Position)
+ }
+
</file context>
| return nil, fmt.Errorf("expected bytestring value, got %v at position %d", p.curToken.Type, p.curToken.Position) | |
| return nil, fmt.Errorf("expected point value, got %v at position %d", p.curToken.Type, p.curToken.Position) |
✅ Addressed in 03ec4fb
cek/builtins.go
Outdated
| extra = per * reqTokens | ||
| } | ||
| if extra > 0 { | ||
| _ = m.spendBudget(ExBudget{Cpu: int64(extra), Mem: 0}) |
There was a problem hiding this comment.
P1: Ignoring error from spendBudget allows execution to continue when budget is exceeded. Consider checking the error: if err := m.spendBudget(...); err != nil { return nil, err }
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 4228:
<comment>Ignoring error from `spendBudget` allows execution to continue when budget is exceeded. Consider checking the error: `if err := m.spendBudget(...); err != nil { return nil, err }`</comment>
<file context>
@@ -3554,24 +3682,566 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ extra = per * reqTokens
+ }
+ if extra > 0 {
+ _ = m.spendBudget(ExBudget{Cpu: int64(extra), Mem: 0})
+ }
+ // check
</file context>
| _ = m.spendBudget(ExBudget{Cpu: int64(extra), Mem: 0}) | |
| if err := m.spendBudget(ExBudget{Cpu: int64(extra), Mem: 0}); err != nil { | |
| return nil, err | |
| } |
✅ Addressed in 03ec4fb
cek/builtins.go
Outdated
| if origAbsN.BitLen() > 40 { // ~1e12 threshold, treat as "huge" | ||
| const baseApprox int64 = 212811 | ||
| budgetCpu := math.MaxInt64 - baseApprox | ||
| _ = m.spendBudget(ExBudget{Cpu: budgetCpu, Mem: 0}) |
There was a problem hiding this comment.
P1: Ignoring error from spendBudget allows execution to continue when budget is exceeded. If budget enforcement is intended, the error should be checked and propagated. If this is intentional for calibration purposes, consider adding a comment explaining why.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3722:
<comment>Ignoring error from `spendBudget` allows execution to continue when budget is exceeded. If budget enforcement is intended, the error should be checked and propagated. If this is intentional for calibration purposes, consider adding a comment explaining why.</comment>
<file context>
@@ -3554,24 +3682,566 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ if origAbsN.BitLen() > 40 { // ~1e12 threshold, treat as "huge"
+ const baseApprox int64 = 212811
+ budgetCpu := math.MaxInt64 - baseApprox
+ _ = m.spendBudget(ExBudget{Cpu: budgetCpu, Mem: 0})
+ } else if extra.BitLen() > 63 {
+ _ = m.spendBudget(ExBudget{Cpu: math.MaxInt64 / 2, Mem: 0})
</file context>
✅ Addressed in 03ec4fb
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
4780870 to
03ec4fb
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
4 issues found across 618 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:3823">
P3: The condition `idx64 < int64(math.MinInt)` is unreachable. Since we already checked `idx.Sign() < 0` and returned an error for negative indices, `idx64` is guaranteed to be non-negative at this point. Similarly, the `i < 0` check on the next logical condition is also unreachable.</violation>
</file>
<file name="cek/cost_model_builtins.go">
<violation number="1" location="cek/cost_model_builtins.go:661">
P2: `ListToArray` has arity 1 but uses `LinearInX` (a `TwoArgument` cost model) instead of `LinearCost` (a `OneArgument` cost model). This is inconsistent with other single-argument builtins like `Sha2_256` that use `LinearCost` directly. Consider using `&LinearCost{...}` instead of `&LinearInX{LinearCost{...}}`.</violation>
</file>
<file name="syn/lex/lexer.go">
<violation number="1" location="syn/lex/lexer.go:229">
P1: Named escape handling greedily consumes all consecutive letters, breaking strings like `"\DELhello"`. The loop should stop once a valid escape sequence is found in `escapeMap`, not continue until it hits a non-letter. Consider checking the escapeMap after each character or limiting to known named escape lengths.</violation>
</file>
<file name="syn/parser.go">
<violation number="1" location="syn/parser.go:790">
P2: Loop variable `p` shadows the Parser receiver `p`. This is a maintainability hazard - if someone adds parser method calls inside this loop, they would fail confusingly. Use a different variable name like `pol` or `policy`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
cek/builtins.go
Outdated
| } | ||
|
|
||
| idx64 := idx.Int64() | ||
| if idx64 > int64(math.MaxInt) || idx64 < int64(math.MinInt) { |
There was a problem hiding this comment.
P3: The condition idx64 < int64(math.MinInt) is unreachable. Since we already checked idx.Sign() < 0 and returned an error for negative indices, idx64 is guaranteed to be non-negative at this point. Similarly, the i < 0 check on the next logical condition is also unreachable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 3823:
<comment>The condition `idx64 < int64(math.MinInt)` is unreachable. Since we already checked `idx.Sign() < 0` and returned an error for negative indices, `idx64` is guaranteed to be non-negative at this point. Similarly, the `i < 0` check on the next logical condition is also unreachable.</comment>
<file context>
@@ -3554,24 +3682,574 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ }
+
+ idx64 := idx.Int64()
+ if idx64 > int64(math.MaxInt) || idx64 < int64(math.MinInt) {
+ return nil, fmt.Errorf("indexArray: index out of range %d", idx64)
+ }
</file context>
| if idx64 > int64(math.MaxInt) || idx64 < int64(math.MinInt) { | |
| if idx64 > int64(math.MaxInt) { |
✅ Addressed in dcdd2b3
| builtin.ListToArray: &CostingFunc[Arguments]{ | ||
| mem: &LinearInX{LinearCost{ | ||
| intercept: 7, | ||
| slope: 1, | ||
| }}, | ||
| cpu: &LinearInX{LinearCost{ | ||
| intercept: 1000, | ||
| slope: 24838, | ||
| }}, | ||
| }, |
There was a problem hiding this comment.
P2: ListToArray has arity 1 but uses LinearInX (a TwoArgument cost model) instead of LinearCost (a OneArgument cost model). This is inconsistent with other single-argument builtins like Sha2_256 that use LinearCost directly. Consider using &LinearCost{...} instead of &LinearInX{LinearCost{...}}.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/cost_model_builtins.go, line 661:
<comment>`ListToArray` has arity 1 but uses `LinearInX` (a `TwoArgument` cost model) instead of `LinearCost` (a `OneArgument` cost model). This is inconsistent with other single-argument builtins like `Sha2_256` that use `LinearCost` directly. Consider using `&LinearCost{...}` instead of `&LinearInX{LinearCost{...}}`.</comment>
<file context>
@@ -633,12 +647,56 @@ var DefaultBuiltinCosts = BuiltinCosts{
+ mem: &ConstantCost{10},
+ cpu: &ConstantCost{231883},
+ },
+ builtin.ListToArray: &CostingFunc[Arguments]{
+ mem: &LinearInX{LinearCost{
+ intercept: 7,
</file context>
| builtin.ListToArray: &CostingFunc[Arguments]{ | |
| mem: &LinearInX{LinearCost{ | |
| intercept: 7, | |
| slope: 1, | |
| }}, | |
| cpu: &LinearInX{LinearCost{ | |
| intercept: 1000, | |
| slope: 24838, | |
| }}, | |
| }, | |
| builtin.ListToArray: &CostingFunc[Arguments]{ | |
| mem: &LinearCost{ | |
| intercept: 7, | |
| slope: 1, | |
| }, | |
| cpu: &LinearCost{ | |
| intercept: 1000, | |
| slope: 24838, | |
| }, | |
| }, |
✅ Addressed in dcdd2b3
| continue | ||
| } | ||
| // Handle named escapes (e.g., \DEL) after specific sequences | ||
| if unicode.IsLetter(l.ch) { |
There was a problem hiding this comment.
P1: Named escape handling greedily consumes all consecutive letters, breaking strings like "\DELhello". The loop should stop once a valid escape sequence is found in escapeMap, not continue until it hits a non-letter. Consider checking the escapeMap after each character or limiting to known named escape lengths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/lex/lexer.go, line 229:
<comment>Named escape handling greedily consumes all consecutive letters, breaking strings like `"\DELhello"`. The loop should stop once a valid escape sequence is found in `escapeMap`, not continue until it hits a non-letter. Consider checking the escapeMap after each character or limiting to known named escape lengths.</comment>
<file context>
@@ -226,6 +225,24 @@ func (l *Lexer) readString() (string, error) {
continue
}
+ // Handle named escapes (e.g., \DEL) after specific sequences
+ if unicode.IsLetter(l.ch) {
+ name := string(l.ch)
+ var nameSb231 strings.Builder
</file context>
syn/parser.go
Outdated
| for p := range vm { | ||
| policies = append(policies, p) |
There was a problem hiding this comment.
P2: Loop variable p shadows the Parser receiver p. This is a maintainability hazard - if someone adds parser method calls inside this loop, they would fail confusingly. Use a different variable name like pol or policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/parser.go, line 790:
<comment>Loop variable `p` shadows the Parser receiver `p`. This is a maintainability hazard - if someone adds parser method calls inside this loop, they would fail confusingly. Use a different variable name like `pol` or `policy`.</comment>
<file context>
@@ -615,6 +612,214 @@ func (p *Parser) parseConstant() (Term[Name], error) {
+
+ // Build canonical list
+ policies := make([]string, 0, len(vm))
+ for p := range vm {
+ policies = append(policies, p)
+ }
</file context>
| for p := range vm { | |
| policies = append(policies, p) | |
| for pol := range vm { | |
| policies = append(policies, pol) |
✅ Addressed in 1aa4f0c
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
03ec4fb to
1adc8b9
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 618 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:4182">
P2: Budget should be charged before performing `valueToMap` operations. The expensive map conversions are executed before the budget check, which could allow resource exhaustion even when the operation should be rejected by the cost model. Other value functions like `lookupCoin` and `unionValue` correctly charge the budget first.</violation>
</file>
<file name="syn/parser.go">
<violation number="1" location="syn/parser.go:790">
P2: Variable `p` shadows the parser receiver `p`. This can cause confusion and potential bugs if parser methods need to be called within this loop. Consider renaming to `pol` or `policy`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
cek/builtins.go
Outdated
| "valueContains: expected proto list for required value", | ||
| ) | ||
| } | ||
| vm := valueToMap[T](valList) |
There was a problem hiding this comment.
P2: Budget should be charged before performing valueToMap operations. The expensive map conversions are executed before the budget check, which could allow resource exhaustion even when the operation should be rejected by the cost model. Other value functions like lookupCoin and unionValue correctly charge the budget first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 4182:
<comment>Budget should be charged before performing `valueToMap` operations. The expensive map conversions are executed before the budget check, which could allow resource exhaustion even when the operation should be rejected by the cost model. Other value functions like `lookupCoin` and `unionValue` correctly charge the budget first.</comment>
<file context>
@@ -3554,24 +3682,574 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ "valueContains: expected proto list for required value",
+ )
+ }
+ vm := valueToMap[T](valList)
+ rm := valueToMap[T](reqList)
+ // Spend budget for valueContains
</file context>
✅ Addressed in 1aa4f0c
syn/parser.go
Outdated
| for p := range vm { | ||
| policies = append(policies, p) |
There was a problem hiding this comment.
P2: Variable p shadows the parser receiver p. This can cause confusion and potential bugs if parser methods need to be called within this loop. Consider renaming to pol or policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/parser.go, line 790:
<comment>Variable `p` shadows the parser receiver `p`. This can cause confusion and potential bugs if parser methods need to be called within this loop. Consider renaming to `pol` or `policy`.</comment>
<file context>
@@ -615,6 +612,214 @@ func (p *Parser) parseConstant() (Term[Name], error) {
+
+ // Build canonical list
+ policies := make([]string, 0, len(vm))
+ for p := range vm {
+ policies = append(policies, p)
+ }
</file context>
| for p := range vm { | |
| policies = append(policies, p) | |
| for pol := range vm { | |
| policies = append(policies, pol) |
✅ Addressed in dcdd2b3
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
1adc8b9 to
dcdd2b3
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
dcdd2b3 to
d781dbf
Compare
There was a problem hiding this comment.
3 issues found across 619 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:306">
P0: Critical bug: `arg1 == arg2` compares pointer addresses, not integer values. Two `*big.Int` with the same numeric value but different memory addresses will incorrectly return `false`. Use `arg1.Cmp(arg2) == 0` to correctly compare values.</violation>
</file>
<file name="syn/lex/lexer.go">
<violation number="1" location="syn/lex/lexer.go:230">
P1: Named escape handling only reads one character instead of the full escape name. For `\DEL`, this reads just 'D' and looks up `escapeMap["D"]` which doesn't exist (the key is `"\\DEL"`). This will cause `\DEL` escapes to fail with "unknown escape sequence".</violation>
</file>
<file name="syn/parser.go">
<violation number="1" location="syn/parser.go:705">
P2: Token key length validation uses `p.curToken.Position` after the parser has advanced past `TokenRParen`, resulting in an incorrect error position. Move this check before `expect(lex.TokenRParen)` or save the bytestring position earlier.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
cek/builtins.go
Outdated
|
|
||
| res := arg1.Cmp(arg2) | ||
| // Perform valueToMap operation after budget check | ||
| res := arg1 == arg2 |
There was a problem hiding this comment.
P0: Critical bug: arg1 == arg2 compares pointer addresses, not integer values. Two *big.Int with the same numeric value but different memory addresses will incorrectly return false. Use arg1.Cmp(arg2) == 0 to correctly compare values.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 306:
<comment>Critical bug: `arg1 == arg2` compares pointer addresses, not integer values. Two `*big.Int` with the same numeric value but different memory addresses will incorrectly return `false`. Use `arg1.Cmp(arg2) == 0` to correctly compare values.</comment>
<file context>
@@ -295,15 +296,17 @@ func equalsInteger[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
- res := arg1.Cmp(arg2)
+ // Perform valueToMap operation after budget check
+ res := arg1 == arg2
con := &syn.Bool{
</file context>
| res := arg1 == arg2 | |
| res := arg1.Cmp(arg2) == 0 |
✅ Addressed in 1a3bdd3
| } | ||
| // Handle named escapes (e.g., \DEL) after specific sequences | ||
| if unicode.IsLetter(l.ch) { | ||
| name := string(l.ch) |
There was a problem hiding this comment.
P1: Named escape handling only reads one character instead of the full escape name. For \DEL, this reads just 'D' and looks up escapeMap["D"] which doesn't exist (the key is "\\DEL"). This will cause \DEL escapes to fail with "unknown escape sequence".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/lex/lexer.go, line 230:
<comment>Named escape handling only reads one character instead of the full escape name. For `\DEL`, this reads just 'D' and looks up `escapeMap["D"]` which doesn't exist (the key is `"\\DEL"`). This will cause `\DEL` escapes to fail with "unknown escape sequence".</comment>
<file context>
@@ -226,6 +225,15 @@ func (l *Lexer) readString() (string, error) {
}
+ // Handle named escapes (e.g., \DEL) after specific sequences
+ if unicode.IsLetter(l.ch) {
+ name := string(l.ch)
+ if val, ok := escapeMap[name]; ok {
+ tmpString += val
</file context>
✅ Addressed in 1a3bdd3
| } | ||
|
|
||
| // token key length must be <= 32 bytes | ||
| if len(ib) > 32 { |
There was a problem hiding this comment.
P2: Token key length validation uses p.curToken.Position after the parser has advanced past TokenRParen, resulting in an incorrect error position. Move this check before expect(lex.TokenRParen) or save the bytestring position earlier.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/parser.go, line 705:
<comment>Token key length validation uses `p.curToken.Position` after the parser has advanced past `TokenRParen`, resulting in an incorrect error position. Move this check before `expect(lex.TokenRParen)` or save the bytestring position earlier.</comment>
<file context>
@@ -615,6 +612,214 @@ func (p *Parser) parseConstant() (Term[Name], error) {
+ }
+
+ // token key length must be <= 32 bytes
+ if len(ib) > 32 {
+ return nil, fmt.Errorf("token key too long (%d bytes) at position %d", len(ib), p.curToken.Position)
+ }
</file context>
1a3bdd3 to
b3262dc
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
5 issues found across 619 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="syn/parser.go">
<violation number="1" location="syn/parser.go:796">
P2: Loop variable `p` shadows the method receiver `*Parser p`. This is a Go anti-pattern that can lead to bugs if the parser is later accessed inside the loop. Use a different variable name like `policy` for clarity.</violation>
</file>
<file name="cek/builtins.go">
<violation number="1" location="cek/builtins.go:299">
P3: Misleading comment: this function compares integers, not performing any `valueToMap` operation. The comment appears to be copy-pasted from a different function.</violation>
<violation number="2" location="cek/builtins.go:1194">
P3: Misleading comment: this function compares strings, not performing any `valueToMap` operation. The comment appears to be copy-pasted from a different function.</violation>
<violation number="3" location="cek/builtins.go:4186">
P2: Budget check occurs after expensive `valueToMap` operations. Other similar functions (`lookupCoin`, `insertCoin`, `scaleValue`, `unionValue`) correctly check the budget before calling `valueToMap`. Move the budget check before these calls to prevent potential DoS via expensive computation before budget validation.</violation>
</file>
<file name="cek/runtime.go">
<violation number="1" location="cek/runtime.go:376">
P2: The `strings.ReplaceAll` calls are either redundant (if the loop handles numeric escapes correctly) or cause incorrect behavior (converting literal backslash+digits to Unicode when the user escaped a backslash). If the loop's numeric escape handling is correct, remove these. If not, fix the loop instead of using this workaround.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
syn/parser.go
Outdated
| sort.Strings(policies) | ||
|
|
||
| res := make([]IConstant, 0) | ||
| for _, p := range policies { |
There was a problem hiding this comment.
P2: Loop variable p shadows the method receiver *Parser p. This is a Go anti-pattern that can lead to bugs if the parser is later accessed inside the loop. Use a different variable name like policy for clarity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/parser.go, line 796:
<comment>Loop variable `p` shadows the method receiver `*Parser p`. This is a Go anti-pattern that can lead to bugs if the parser is later accessed inside the loop. Use a different variable name like `policy` for clarity.</comment>
<file context>
@@ -615,6 +612,214 @@ func (p *Parser) parseConstant() (Term[Name], error) {
+ sort.Strings(policies)
+
+ res := make([]IConstant, 0)
+ for _, p := range policies {
+ tokens := vm[p]
+ // collect non-zero tokens
</file context>
✅ Addressed in 1aa4f0c
cek/builtins.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| // Perform valueToMap operation after budget check |
There was a problem hiding this comment.
P3: Misleading comment: this function compares strings, not performing any valueToMap operation. The comment appears to be copy-pasted from a different function.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 1194:
<comment>Misleading comment: this function compares strings, not performing any `valueToMap` operation. The comment appears to be copy-pasted from a different function.</comment>
<file context>
@@ -1174,11 +1185,13 @@ func equalsString[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
return nil, err
}
+ // Perform valueToMap operation after budget check
res := arg1 == arg2
</file context>
| // Perform valueToMap operation after budget check | |
| // Compare strings for equality |
cek/builtins.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| // Charge budget before performing valueToMap operation |
There was a problem hiding this comment.
P3: Misleading comment: this function compares integers, not performing any valueToMap operation. The comment appears to be copy-pasted from a different function.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 299:
<comment>Misleading comment: this function compares integers, not performing any `valueToMap` operation. The comment appears to be copy-pasted from a different function.</comment>
<file context>
@@ -295,15 +296,17 @@ func equalsInteger[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
return nil, err
}
+ // Charge budget before performing valueToMap operation
err = m.CostTwo(&b.Func, bigIntExMem(arg1), bigIntExMem(arg2))
if err != nil {
</file context>
| // Charge budget before performing valueToMap operation | |
| // Charge budget before performing integer comparison |
✅ Addressed in 1aa4f0c
cek/builtins.go
Outdated
| "valueContains: expected proto list for required value", | ||
| ) | ||
| } | ||
| vm := valueToMap[T](valList) |
There was a problem hiding this comment.
P2: Budget check occurs after expensive valueToMap operations. Other similar functions (lookupCoin, insertCoin, scaleValue, unionValue) correctly check the budget before calling valueToMap. Move the budget check before these calls to prevent potential DoS via expensive computation before budget validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/builtins.go, line 4186:
<comment>Budget check occurs after expensive `valueToMap` operations. Other similar functions (`lookupCoin`, `insertCoin`, `scaleValue`, `unionValue`) correctly check the budget before calling `valueToMap`. Move the budget check before these calls to prevent potential DoS via expensive computation before budget validation.</comment>
<file context>
@@ -3554,24 +3686,574 @@ func caseData[T syn.Eval](m *Machine[T], b *Builtin[T]) (Value[T], error) {
+ "valueContains: expected proto list for required value",
+ )
+ }
+ vm := valueToMap[T](valList)
+ rm := valueToMap[T](reqList)
+ // Spend budget for valueContains
</file context>
✅ Addressed in 1aa4f0c
cek/runtime.go
Outdated
|
|
||
| // Final explicit replacements for common symbol escapes (keeps mapping explicit) | ||
| out := sb.String() | ||
| out = strings.ReplaceAll(out, "\\8712", "∈") |
There was a problem hiding this comment.
P2: The strings.ReplaceAll calls are either redundant (if the loop handles numeric escapes correctly) or cause incorrect behavior (converting literal backslash+digits to Unicode when the user escaped a backslash). If the loop's numeric escape handling is correct, remove these. If not, fix the loop instead of using this workaround.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/runtime.go, line 376:
<comment>The `strings.ReplaceAll` calls are either redundant (if the loop handles numeric escapes correctly) or cause incorrect behavior (converting literal backslash+digits to Unicode when the user escaped a backslash). If the loop's numeric escape handling is correct, remove these. If not, fix the loop instead of using this workaround.</comment>
<file context>
@@ -268,6 +284,106 @@ func unwrapString[T syn.Eval](value Value[T]) (string, error) {
+
+ // Final explicit replacements for common symbol escapes (keeps mapping explicit)
+ out := sb.String()
+ out = strings.ReplaceAll(out, "\\8712", "∈")
+ out = strings.ReplaceAll(out, "\\8477", "ℝ")
+ out = strings.ReplaceAll(out, "\\8658", "⇒")
</file context>
✅ Addressed in 1aa4f0c
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
b3262dc to
1aa4f0c
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 619 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="syn/lex/lexer.go">
<violation number="1" location="syn/lex/lexer.go:250">
P2: When an invalid named escape like `\INVALID` is encountered, the error message will show 'unknown escape sequence: \I' instead of the full sequence. Consider returning an error with the full name for clearer debugging.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
syn/lex/lexer.go
Outdated
| continue | ||
| } | ||
|
|
||
| // no valid named escape found; fall through to error handling below |
There was a problem hiding this comment.
P2: When an invalid named escape like \INVALID is encountered, the error message will show 'unknown escape sequence: \I' instead of the full sequence. Consider returning an error with the full name for clearer debugging.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/lex/lexer.go, line 250:
<comment>When an invalid named escape like `\INVALID` is encountered, the error message will show 'unknown escape sequence: \I' instead of the full sequence. Consider returning an error with the full name for clearer debugging.</comment>
<file context>
@@ -226,6 +225,30 @@ func (l *Lexer) readString() (string, error) {
+ continue
+ }
+
+ // no valid named escape found; fall through to error handling below
+ }
// Check for unknown non-numeric escape
</file context>
| // no valid named escape found; fall through to error handling below | |
| // no valid named escape found; return error with full name | |
| return "", fmt.Errorf("unknown escape sequence: \\%s", name) |
✅ Addressed in 6e765c8
Signed-off-by: Chris Gianelloni <[email protected]>
wolf31o2
force-pushed
the
feat/builtins-v4
branch
from
1aa4f0c to
6e765c8
Compare
|
@cubic-dev-ai review this PR |
@wolf31o2 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
3 issues found across 621 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="syn/parser.go">
<violation number="1" location="syn/parser.go:706">
P2: Error position will be incorrect when reporting token key too long. At this point, `p.curToken.Position` has advanced past multiple tokens (comma, integer, closing paren) and no longer points to where the bytestring `ib` was parsed. Consider moving this check earlier, right after parsing `ib` and before `p.nextToken()` (similar to how the policy key check is done at line 641).</violation>
</file>
<file name="cek/cost_model_builtins.go">
<violation number="1" location="cek/cost_model_builtins.go:684">
P2: Memory cost mismatch with specification: comment says "mem ~601" but code uses `&ConstantCost{1}`. If the comment reflects the intended conformance value, the memory cost should be approximately 601.</violation>
</file>
<file name="syn/lex/lexer_test.go">
<violation number="1" location="syn/lex/lexer_test.go:12">
P2: Test input has double backslash `\\INVALID` but comment says it tests unknown named escape `\INVALID`. With double backslash, the lexer sees valid escape `\\` (for literal backslash) followed by `INVALID`, so this doesn't actually test the unknown named escape scenario. Consider using single backslash: `"\INVALID"`.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
|
||
| // token key length must be <= 32 bytes | ||
| if len(ib) > 32 { | ||
| return nil, fmt.Errorf("token key too long (%d bytes) at position %d", len(ib), p.curToken.Position) |
There was a problem hiding this comment.
P2: Error position will be incorrect when reporting token key too long. At this point, p.curToken.Position has advanced past multiple tokens (comma, integer, closing paren) and no longer points to where the bytestring ib was parsed. Consider moving this check earlier, right after parsing ib and before p.nextToken() (similar to how the policy key check is done at line 641).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/parser.go, line 706:
<comment>Error position will be incorrect when reporting token key too long. At this point, `p.curToken.Position` has advanced past multiple tokens (comma, integer, closing paren) and no longer points to where the bytestring `ib` was parsed. Consider moving this check earlier, right after parsing `ib` and before `p.nextToken()` (similar to how the policy key check is done at line 641).</comment>
<file context>
@@ -615,6 +612,214 @@ func (p *Parser) parseConstant() (Term[Name], error) {
+
+ // token key length must be <= 32 bytes
+ if len(ib) > 32 {
+ return nil, fmt.Errorf("token key too long (%d bytes) at position %d", len(ib), p.curToken.Position)
+ }
+
</file context>
| cpu: &ConstantCost{100000000000}, | ||
| }, | ||
| builtin.LookupCoin: &CostingFunc[Arguments]{ | ||
| mem: &ConstantCost{1}, |
There was a problem hiding this comment.
P2: Memory cost mismatch with specification: comment says "mem ~601" but code uses &ConstantCost{1}. If the comment reflects the intended conformance value, the memory cost should be approximately 601.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cek/cost_model_builtins.go, line 684:
<comment>Memory cost mismatch with specification: comment says "mem ~601" but code uses `&ConstantCost{1}`. If the comment reflects the intended conformance value, the memory cost should be approximately 601.</comment>
<file context>
@@ -633,12 +647,56 @@ var DefaultBuiltinCosts = BuiltinCosts{
+ cpu: &ConstantCost{100000000000},
+ },
+ builtin.LookupCoin: &CostingFunc[Arguments]{
+ mem: &ConstantCost{1},
+ cpu: &ConstantCost{248283},
+ },
</file context>
|
|
||
| func TestUnknownNamedEscapeMessageContainsFullName(t *testing.T) { | ||
| // Input is a quoted string containing an unknown named escape \INVALID | ||
| input := `"\\INVALID"` |
There was a problem hiding this comment.
P2: Test input has double backslash \\INVALID but comment says it tests unknown named escape \INVALID. With double backslash, the lexer sees valid escape \\ (for literal backslash) followed by INVALID, so this doesn't actually test the unknown named escape scenario. Consider using single backslash: "\INVALID".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At syn/lex/lexer_test.go, line 12:
<comment>Test input has double backslash `\\INVALID` but comment says it tests unknown named escape `\INVALID`. With double backslash, the lexer sees valid escape `\\` (for literal backslash) followed by `INVALID`, so this doesn't actually test the unknown named escape scenario. Consider using single backslash: `"\INVALID"`.</comment>
<file context>
@@ -3,9 +3,29 @@ package lex
+func TestUnknownNamedEscapeMessageContainsFullName(t *testing.T) {
+ // Input is a quoted string containing an unknown named escape \INVALID
+ input := `"\\INVALID"`
+ lexer := NewLexer(input)
+ tok := lexer.NextToken()
</file context>
| input := `"\\INVALID"` | |
| input := `"\INVALID"` |
Tests updated via "make download-plutus-tests"
Summary by cubic
Adds builtins and parser/runtime changes needed to pass the latest conformance suite: BLS12-381 multi-scalar multiplication, Value/coin ops, array utilities, and case-on-constant semantics. Updates cost models, budgets, and tests accordingly.
New Features
Refactors
Written for commit 6e765c8. Summary will update automatically on new commits.