feat: add dry-run mode for execute (Phase 3) #140
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| package server | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
|
|
||
| mcp "github.com/daltoniam/switchboard" | ||
| mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp" | ||
| ) | ||
|
|
||
| func (s *Server) handleDryRun(ctx context.Context, toolName mcp.ToolName, args map[string]any) (*mcpsdk.CallToolResult, error) { | ||
| integration, toolDef, err := s.findTool(toolName) | ||
| if err != nil { | ||
| return errorResult(err.Error()), nil | ||
| } | ||
|
|
||
| if err := validateArgs(toolDef, args); err != nil { | ||
|
daltoniam marked this conversation as resolved.
|
||
| return errorResult(fmt.Sprintf("dry-run validation failed: %s", err)), nil | ||
| } | ||
|
|
||
| cb := s.getBreaker(integration.Name()) | ||
| if !cb.allow() { | ||
| cb.recordSuccess() | ||
|
Collaborator
Author
There was a problem hiding this comment. This This was flagged in the first review round but looks like it was lost in the squash. The fix is to remove this call and only keep the one at line 29: if !cb.allow() {
return errorResult(fmt.Sprintf(
"dry-run: integration %q temporarily unavailable (circuit breaker open)",
integration.Name(),
)), nil
}
cb.recordSuccess() |
||
| return errorResult(fmt.Sprintf( | ||
| "dry-run: integration %q temporarily unavailable (circuit breaker open)", | ||
| integration.Name(), | ||
| )), nil | ||
| } | ||
| cb.recordSuccess() | ||
|
|
||
| if !integration.Healthy(ctx) { | ||
| return errorResult(fmt.Sprintf( | ||
| "dry-run: integration %q is unhealthy — call would likely fail", | ||
| integration.Name(), | ||
| )), nil | ||
| } | ||
|
|
||
| if dri, ok := integration.(mcp.DryRunIntegration); ok { | ||
| if result, handled := dri.DryRun(ctx, toolName, args); handled { | ||
| return &mcpsdk.CallToolResult{ | ||
| Content: []mcpsdk.Content{ | ||
| &mcpsdk.TextContent{Text: result.Data}, | ||
| }, | ||
| IsError: result.IsError, | ||
| }, nil | ||
| } | ||
| } | ||
|
|
||
| result, err := mcp.JSONResult(map[string]any{ | ||
| "dry_run": true, | ||
| "tool": toolName, | ||
| "integration": integration.Name(), | ||
| "validated_args": args, | ||
| "status": "ok", | ||
| "note": "Simulated dry-run: arguments are valid, integration is healthy. This tool does not support native dry-run preview.", | ||
| }) | ||
| if err != nil { | ||
| return errorResult(err.Error()), nil | ||
| } | ||
| return &mcpsdk.CallToolResult{ | ||
| Content: []mcpsdk.Content{ | ||
| &mcpsdk.TextContent{Text: result.Data}, | ||
| }, | ||
| }, nil | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,234 @@ | ||
| package server | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "testing" | ||
|
|
||
| mcp "github.com/daltoniam/switchboard" | ||
| mcpsdk "github.com/modelcontextprotocol/go-sdk/mcp" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| type mockDryRunIntegration struct { | ||
| mockIntegration | ||
| dryRunFn func(ctx context.Context, toolName mcp.ToolName, args map[string]any) (*mcp.ToolResult, bool) | ||
| } | ||
|
|
||
| func (m *mockDryRunIntegration) DryRun(ctx context.Context, toolName mcp.ToolName, args map[string]any) (*mcp.ToolResult, bool) { | ||
| if m.dryRunFn != nil { | ||
| return m.dryRunFn(ctx, toolName, args) | ||
| } | ||
| return nil, false | ||
| } | ||
|
|
||
| func dryRunRequest(toolName string, args map[string]any) *mcpsdk.CallToolRequest { | ||
| data, _ := json.Marshal(map[string]any{ | ||
| "tool_name": toolName, | ||
| "arguments": args, | ||
| "dry_run": true, | ||
| }) | ||
| return &mcpsdk.CallToolRequest{ | ||
| Params: &mcpsdk.CallToolParamsRaw{ | ||
| Name: "execute", | ||
| Arguments: json.RawMessage(data), | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func TestDryRun_SimulatedFallback(t *testing.T) { | ||
| mi := &mockIntegration{ | ||
| name: "github", | ||
| healthy: true, | ||
| tools: []mcp.ToolDefinition{ | ||
| { | ||
| Name: "github_create_issue", | ||
| Description: "Create issue", | ||
| Parameters: map[string]string{"owner": "Owner", "repo": "Repo", "title": "Title"}, | ||
| Required: []string{"owner", "repo", "title"}, | ||
| }, | ||
| }, | ||
| } | ||
| s := setupTestServer(mi) | ||
| ctx := context.Background() | ||
|
|
||
| result, err := s.handleExecute(ctx, dryRunRequest("github_create_issue", map[string]any{ | ||
| "owner": "daltoniam", | ||
| "repo": "switchboard", | ||
| "title": "Test issue", | ||
| })) | ||
| require.NoError(t, err) | ||
| require.False(t, result.IsError) | ||
|
|
||
| var resp map[string]any | ||
| require.NoError(t, json.Unmarshal([]byte(result.Content[0].(*mcpsdk.TextContent).Text), &resp)) | ||
| assert.Equal(t, true, resp["dry_run"]) | ||
| assert.Equal(t, "github_create_issue", resp["tool"]) | ||
| assert.Equal(t, "github", resp["integration"]) | ||
| assert.Equal(t, "ok", resp["status"]) | ||
| args := resp["validated_args"].(map[string]any) | ||
| assert.Equal(t, "daltoniam", args["owner"]) | ||
| } | ||
|
|
||
| func TestDryRun_ValidationFails(t *testing.T) { | ||
| mi := &mockIntegration{ | ||
| name: "github", | ||
| healthy: true, | ||
| tools: []mcp.ToolDefinition{ | ||
| { | ||
| Name: "github_create_issue", | ||
| Parameters: map[string]string{"owner": "Owner", "repo": "Repo", "title": "Title"}, | ||
| Required: []string{"owner", "repo", "title"}, | ||
| }, | ||
| }, | ||
| } | ||
| s := setupTestServer(mi) | ||
| ctx := context.Background() | ||
|
|
||
| result, err := s.handleExecute(ctx, dryRunRequest("github_create_issue", map[string]any{ | ||
| "owner": "daltoniam", | ||
| })) | ||
| require.NoError(t, err) | ||
| assert.True(t, result.IsError) | ||
| assert.Contains(t, result.Content[0].(*mcpsdk.TextContent).Text, "dry-run validation failed") | ||
| assert.Contains(t, result.Content[0].(*mcpsdk.TextContent).Text, "repo") | ||
| } | ||
|
|
||
| func TestDryRun_NativeIntegration(t *testing.T) { | ||
| mi := &mockDryRunIntegration{ | ||
| mockIntegration: mockIntegration{ | ||
| name: "aws", | ||
| healthy: true, | ||
| tools: []mcp.ToolDefinition{ | ||
| { | ||
| Name: "aws_lambda_invoke", | ||
| Parameters: map[string]string{"function_name": "Function"}, | ||
| Required: []string{"function_name"}, | ||
| }, | ||
| }, | ||
| }, | ||
| dryRunFn: func(_ context.Context, _ mcp.ToolName, args map[string]any) (*mcp.ToolResult, bool) { | ||
| return &mcp.ToolResult{ | ||
| Data: `{"dry_run":true,"native":true,"would_invoke":"` + args["function_name"].(string) + `"}`, | ||
|
Collaborator
Author
There was a problem hiding this comment. Unsafe type assertion here — if fn, ok := args["function_name"].(string)
require.True(t, ok, "expected function_name to be a string")
return &mcp.ToolResult{
Data: `{"dry_run":true,"native":true,"would_invoke":"` + fn + `"}`,
}, true |
||
| }, true | ||
| }, | ||
| } | ||
| s := setupTestServerWithIntegration(mi) | ||
| ctx := context.Background() | ||
|
|
||
| result, err := s.handleExecute(ctx, dryRunRequest("aws_lambda_invoke", map[string]any{ | ||
| "function_name": "my-func", | ||
| })) | ||
| require.NoError(t, err) | ||
| require.False(t, result.IsError) | ||
|
|
||
| var resp map[string]any | ||
| require.NoError(t, json.Unmarshal([]byte(result.Content[0].(*mcpsdk.TextContent).Text), &resp)) | ||
| assert.Equal(t, true, resp["native"]) | ||
| assert.Equal(t, "my-func", resp["would_invoke"]) | ||
| } | ||
|
|
||
| func TestDryRun_NativeDeclines_FallsBackToSimulated(t *testing.T) { | ||
| mi := &mockDryRunIntegration{ | ||
| mockIntegration: mockIntegration{ | ||
| name: "aws", | ||
| healthy: true, | ||
| tools: []mcp.ToolDefinition{ | ||
| { | ||
| Name: "aws_s3_list", | ||
| Parameters: map[string]string{"bucket": "Bucket"}, | ||
| Required: []string{"bucket"}, | ||
| }, | ||
| }, | ||
| }, | ||
| dryRunFn: func(_ context.Context, _ mcp.ToolName, _ map[string]any) (*mcp.ToolResult, bool) { | ||
| return nil, false | ||
| }, | ||
| } | ||
| s := setupTestServerWithIntegration(mi) | ||
| ctx := context.Background() | ||
|
|
||
| result, err := s.handleExecute(ctx, dryRunRequest("aws_s3_list", map[string]any{ | ||
| "bucket": "my-bucket", | ||
| })) | ||
| require.NoError(t, err) | ||
| require.False(t, result.IsError) | ||
|
|
||
| var resp map[string]any | ||
| require.NoError(t, json.Unmarshal([]byte(result.Content[0].(*mcpsdk.TextContent).Text), &resp)) | ||
| assert.Equal(t, true, resp["dry_run"]) | ||
| assert.Equal(t, "ok", resp["status"]) | ||
| } | ||
|
|
||
| func TestDryRun_UnhealthyIntegration(t *testing.T) { | ||
| mi := &mockIntegration{ | ||
| name: "github", | ||
| healthy: false, | ||
| tools: []mcp.ToolDefinition{ | ||
| {Name: "github_create_issue", Parameters: map[string]string{"title": "Title"}, Required: []string{"title"}}, | ||
| }, | ||
| } | ||
| s := setupTestServer(mi) | ||
| ctx := context.Background() | ||
|
|
||
| result, err := s.handleExecute(ctx, dryRunRequest("github_create_issue", map[string]any{ | ||
| "title": "test", | ||
| })) | ||
| require.NoError(t, err) | ||
| assert.True(t, result.IsError) | ||
| assert.Contains(t, result.Content[0].(*mcpsdk.TextContent).Text, "unhealthy") | ||
| } | ||
|
|
||
| func TestDryRun_ToolNotFound(t *testing.T) { | ||
| s := setupTestServer(&mockIntegration{name: "test", healthy: true}) | ||
| ctx := context.Background() | ||
|
|
||
| result, err := s.handleExecute(ctx, dryRunRequest("nonexistent_tool", nil)) | ||
| require.NoError(t, err) | ||
| assert.True(t, result.IsError) | ||
| assert.Contains(t, result.Content[0].(*mcpsdk.TextContent).Text, "not found") | ||
| } | ||
|
|
||
| func TestDryRun_DoesNotExecute(t *testing.T) { | ||
| executed := false | ||
| mi := &mockIntegration{ | ||
| name: "github", | ||
| healthy: true, | ||
| tools: []mcp.ToolDefinition{ | ||
| {Name: "github_create_issue", Parameters: map[string]string{"title": "Title"}, Required: []string{"title"}}, | ||
| }, | ||
| execFn: func(_ context.Context, _ mcp.ToolName, _ map[string]any) (*mcp.ToolResult, error) { | ||
| executed = true | ||
| return &mcp.ToolResult{Data: `{"id":1}`}, nil | ||
| }, | ||
| } | ||
| s := setupTestServer(mi) | ||
| ctx := context.Background() | ||
|
|
||
| _, err := s.handleExecute(ctx, dryRunRequest("github_create_issue", map[string]any{ | ||
| "title": "test", | ||
| })) | ||
| require.NoError(t, err) | ||
| assert.False(t, executed, "dry-run should not call Execute") | ||
| } | ||
|
|
||
| func TestDryRun_NoPinning(t *testing.T) { | ||
| mi := &mockIntegration{ | ||
| name: "github", | ||
| healthy: true, | ||
| tools: []mcp.ToolDefinition{ | ||
| {Name: "github_create_issue", Parameters: map[string]string{"title": "Title"}, Required: []string{"title"}}, | ||
| }, | ||
| } | ||
| s := setupTestServer(mi) | ||
| ctx := context.Background() | ||
|
|
||
| _, err := s.handleExecute(ctx, dryRunRequest("github_create_issue", map[string]any{ | ||
| "title": "test", | ||
| })) | ||
| require.NoError(t, err) | ||
|
|
||
| sess := s.sessionStore.GetOrCreate("default") | ||
| assert.Equal(t, 0, sess.PinnedCount(), "dry-run should not pin results") | ||
| } | ||
|
daltoniam marked this conversation as resolved.
|
||
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MarkdownIntegration.RenderMarkdownhas a method-level doc explaining its return contract;DryRundoesn't. Worth adding a note that(nil, false)is the expected "decline" signal (not an error), so adapter authors don't reach for(*ToolResult{IsError: true}, true)when they just want to fall back: