feat: MCP first MVP #97
Conversation
WalkthroughAdds an MCP server exposing evaluate and check tools for Numscript, a hidden Cobra subcommand to run it, registers the subcommand with the root command, and adds a helper to convert diagnostic severities to plain strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI (cobra root)
participant MCP as mcpCmd
participant Server as MCP Server
participant Tools as Tools (evaluate/check)
participant Engine as Numscript Engine
participant Anal as Analyzer
User->>CLI: run app mcp
CLI->>MCP: Execute mcpCmd
MCP->>Server: RunServer()
Server-->>User: serve over stdio (MCP)
rect rgb(230,245,255)
note right of User: evaluate flow
User->>Server: tool:evaluate {script, balances, vars}
Server->>Tools: invoke evaluate handler
Tools->>Tools: parse balances & vars
Tools->>Engine: interpreter.RunProgram(...)
alt success
Engine-->>Tools: result
Tools-->>Server: ToolResultJSON
Server-->>User: success response
else error
Engine-->>Tools: error
Tools-->>Server: ToolResultError
Server-->>User: error response
end
end
rect rgb(240,255,240)
note right of User: check flow
User->>Server: tool:check {script}
Server->>Tools: invoke check handler
Tools->>Anal: parse + analyze(script)
Anal-->>Tools: diagnostics [kind,severity,span]
Tools-->>Server: ToolResultJSON {errors:[...]}
Server-->>User: response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
internal/mcp_impl/handlers.go
Outdated
| server.WithInstructions(` | ||
| You're a numscript expert AI assistant. Numscript is a DSL that allows modelling finantial transaction in an easy and declarative way. Numscript scripts alwasy terminate. | ||
| `), | ||
| // TODO add prompt |
There was a problem hiding this comment.
we can add a more complete prompt in future releases
| interpreter.StaticStore{ | ||
| Balances: balances, | ||
| }, | ||
| map[string]struct{}{}, |
There was a problem hiding this comment.
we'll add feature flags in future releases
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #97 +/- ##
==========================================
- Coverage 70.56% 68.18% -2.39%
==========================================
Files 43 45 +2
Lines 4142 4290 +148
==========================================
+ Hits 2923 2925 +2
- Misses 1061 1208 +147
+ Partials 158 157 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/cmd/mcp.go (1)
8-22: Set Silence flags on the command itself (optional).Avoids duplicate error/usage output without toggling inside RunE.
var mcpCmd = &cobra.Command{ Use: "mcp", Short: "Run the mcp server", Hidden: true, + SilenceErrors: true, + SilenceUsage: true, RunE: func(cmd *cobra.Command, args []string) error { err := mcp_impl.RunServer() if err != nil { - cmd.SilenceErrors = true - cmd.SilenceUsage = true return err } return nil }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
internal/analysis/diagnostic_kind.go(1 hunks)internal/cmd/mcp.go(1 hunks)internal/cmd/root.go(1 hunks)internal/mcp_impl/handlers.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/analysis/diagnostic_kind.go (1)
internal/utils/utils.go (1)
NonExhaustiveMatchPanic(8-10)
internal/mcp_impl/handlers.go (3)
internal/interpreter/interpreter.go (1)
RunProgram(228-285)internal/analysis/check.go (1)
CheckSource(296-304)internal/analysis/diagnostic_kind.go (2)
SeverityToString(39-52)Severity(12-12)
internal/cmd/mcp.go (1)
internal/mcp_impl/handlers.go (1)
RunServer(175-196)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (3)
internal/analysis/diagnostic_kind.go (1)
39-52: Helper looks correct and consistent with existing mapping.internal/cmd/root.go (1)
27-27: Wiring the hidden mcp subcommand looks good.internal/mcp_impl/handlers.go (1)
12-14: Confirmed valid mcp-go version pin
go.mod is pinned to v0.41.1, which matches an existing release.
| for asset, amountRaw := range assets { | ||
| amount, ok := amountRaw.(float64) | ||
| if !ok { | ||
| return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Expected float for amount: %v", amountRaw)) | ||
| } | ||
|
|
||
| n, _ := big.NewFloat(amount).Int(new(big.Int)) | ||
| iBalances[account][asset] = n | ||
| } |
There was a problem hiding this comment.
Amounts parsing risks precision loss and silently truncates fractions. Enforce integer semantics and accept safe types.
- Converting via big.NewFloat(amount).Int ignores fractional parts and float64 precision limits (money correctness risk).
- Validate integer-ness, reject NaN/Inf, and optionally accept string integers.
Apply:
- for asset, amountRaw := range assets {
- amount, ok := amountRaw.(float64)
- if !ok {
- return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Expected float for amount: %v", amountRaw))
- }
-
- n, _ := big.NewFloat(amount).Int(new(big.Int))
- iBalances[account][asset] = n
- }
+ for asset, amountRaw := range assets {
+ var n *big.Int
+ switch v := amountRaw.(type) {
+ case float64:
+ if math.IsNaN(v) || math.IsInf(v, 0) {
+ return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Invalid number for amount at %s/%s: %v", account, asset, v))
+ }
+ if v != math.Trunc(v) {
+ return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Amount must be an integer at %s/%s, got: %v", account, asset, v))
+ }
+ if v > float64(math.MaxInt64) || v < float64(math.MinInt64) {
+ return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Amount out of 64-bit range at %s/%s: %v", account, asset, v))
+ }
+ n = big.NewInt(int64(v))
+ case string:
+ n = new(big.Int)
+ if _, ok := n.SetString(v, 10); !ok {
+ return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Invalid integer string for amount at %s/%s: %q", account, asset, v))
+ }
+ default:
+ return interpreter.Balances{}, mcp.NewToolResultError(fmt.Sprintf("Expected integer (number or string) for amount at %s/%s, got: %T", account, asset, amountRaw))
+ }
+ iBalances[account][asset] = n
+ }Additionally, add import:
@@
-import (
+import (
"context"
"fmt"
+ "math"
"math/big"
"strings"Committable suggestion skipped: line range outside the PR's diff.
internal/mcp_impl/handlers.go
Outdated
| } | ||
|
|
||
| out, iErr := interpreter.RunProgram( | ||
| context.Background(), |
There was a problem hiding this comment.
You might want to use the proper context or a derived one ?
There was a problem hiding this comment.
This one should be ok: we are only doing in-memory operations, and fetching data in a go struct
(We need context because the implementor of the interface might be sql queries )
There was a problem hiding this comment.
David is right.
Always use the correct context.
There is no good reason to not use it.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/mcp_impl/handlers.go (2)
33-41: Amount parsing still risks precision loss and silent truncation.This issue was previously flagged but remains unresolved. Converting
float64tobig.Intviabig.NewFloatsilently discards fractional parts and is vulnerable to floating-point precision limits. For financial amounts, this poses data integrity risks.The previous review provided a comprehensive fix that validates integer-ness, rejects NaN/Inf, and optionally accepts string integers. Please refer to the detailed solution in the earlier review comment.
104-111: Critical: Parse error is created but not returned.When parse errors are detected, line 110 creates a
ToolResultErrorbut doesn't return it. Execution continues to line 113 with an invalidparsed.Value, which will cause incorrect behavior or runtime errors.This issue was previously flagged and marked as addressed, but the fix was not applied.
Apply this diff:
parsed := parser.Parse(script) if len(parsed.Errors) != 0 { out := make([]string, len(parsed.Errors)) for index, err := range parsed.Errors { out[index] = err.Msg } - mcp.NewToolResultError(strings.Join(out, ", ")) + return mcp.NewToolResultError(strings.Join(out, ", ")), nil }
🧹 Nitpick comments (3)
internal/cmd/mcp.go (1)
12-21: Simplify error handling logic.The conditional check is redundant. The silence flags can be set unconditionally before calling RunServer, and you can directly return its result.
Apply this diff:
RunE: func(cmd *cobra.Command, args []string) error { - err := mcp_impl.RunServer() - if err != nil { - cmd.SilenceErrors = true - cmd.SilenceUsage = true - return err - } - - return nil + cmd.SilenceErrors = true + cmd.SilenceUsage = true + return mcp_impl.RunServer() },internal/mcp_impl/handlers.go (2)
159-166: Consider renaming "errors" variable to "diagnostics" for clarity.The variable is named
errorsbut contains diagnostics of all severity levels (errors, warnings, info, hints). The name is misleading.Apply this diff:
- var errors []any + var diagnostics []any for _, d := range checkResult.Diagnostics { - errors = append(errors, map[string]any{ + diagnostics = append(diagnostics, map[string]any{ "kind": d.Kind.Message(), "severity": analysis.SeverityToString(d.Kind.Severity()), "span": d.Range, }) } return mcp.NewToolResultJSON(map[string]any{ - "errors": errors, + "diagnostics": diagnostics, })
188-194: Simplify return statement.The explicit nil return at line 193 is redundant since
ServeStdioalready returnsnilon success.Apply this diff:
// Start the server - if err := server.ServeStdio(s); err != nil { - return err - } - - return nil + return server.ServeStdio(s) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (4)
internal/analysis/diagnostic_kind.go(1 hunks)internal/cmd/mcp.go(1 hunks)internal/cmd/root.go(1 hunks)internal/mcp_impl/handlers.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/analysis/diagnostic_kind.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/mcp_impl/handlers.go (4)
internal/interpreter/interpreter.go (1)
RunProgram(228-285)internal/analysis/check.go (1)
CheckSource(296-304)internal/analysis/diagnostic_kind.go (2)
SeverityToString(39-52)Severity(12-12)internal/parser/range.go (1)
Range(13-16)
internal/cmd/mcp.go (1)
internal/mcp_impl/handlers.go (1)
RunServer(174-194)
🔇 Additional comments (2)
internal/cmd/root.go (1)
27-27: LGTM!The MCP command registration follows the existing pattern and is positioned appropriately between the LSP and check commands.
internal/mcp_impl/handlers.go (1)
123-135: LGTM!The interpreter execution properly uses the provided context and handles errors correctly. The error guard at lines 132-134 ensures the dereferencing at line 135 is safe.
Just an initial working MPV of an MCP integration for numscript. There are endless improvement that can be made, by improving current prompts and adding many other tools, but for now that's 80% of the value we can get out of it, with not so much complexity