diff --git a/cmd/mcpproxy/upstream_cmd.go b/cmd/mcpproxy/upstream_cmd.go index 092378e8c..5c475a73a 100644 --- a/cmd/mcpproxy/upstream_cmd.go +++ b/cmd/mcpproxy/upstream_cmd.go @@ -2113,14 +2113,45 @@ func runUpstreamImport(_ *cobra.Command, args []string) error { return outputError(output.NewStructuredError(output.ErrCodeOperationFailed, err.Error()), output.ErrCodeOperationFailed) } - // Output results based on format + // Apply once, before choosing a renderer. The parser's Imported slice is a + // plan; only successful writes may be reported as imported after execution. + planned := len(result.Imported) + applied := 0 + var applyErr error + if !upstreamImportDryRun && planned > 0 { + var successful []*configimport.ImportedServer + var failed []configimport.FailedServer + successful, failed, applyErr = applyImportedServers(result.Imported, globalConfig) + result.Imported = successful + result.Failed = append(result.Failed, failed...) + result.Summary.Imported = len(successful) + result.Summary.Failed = len(result.Failed) + applied = len(successful) + } + outcome := importOutcome{planned: planned, applied: applied, dryRun: upstreamImportDryRun} + + // Render the completed outcome even when some writes failed. Automation gets + // the per-server failures on stdout and a nonzero exit status. outputFormat := ResolveOutputFormat() - + var outputErr error if outputFormat == "json" || outputFormat == "yaml" { - return outputImportResultStructured(result, outputFormat) + outputErr = outputImportResultStructured(result, outcome) + } else { + outputErr = outputImportResultTable(result, outcome, upstreamImportNoQuarantine) } - - return outputImportResultTable(result, upstreamImportDryRun, upstreamImportNoQuarantine, globalConfig) + if outputErr != nil { + return outputErr + } + if applyErr != nil { + return applyErr + } + if result.Summary.Failed > 0 { + if outcome.dryRun { + return fmt.Errorf("validation failed for %d of %d source servers; no changes made", result.Summary.Failed, result.Summary.Total) + } + return fmt.Errorf("failed to import %d of %d servers", result.Summary.Failed, result.Summary.Total) + } + return nil } // importSelfListenAddrs returns the addresses the local mcpproxy answers on, @@ -2164,13 +2195,28 @@ func parseImportFormat(format string) configimport.ConfigFormat { } } -// outputImportResultStructured outputs the import result in JSON/YAML format -func outputImportResultStructured(result *configimport.ImportResult, format string) error { +type importOutcome struct { + planned int + applied int + dryRun bool +} + +func importOutputSummary(result *configimport.ImportResult, outcome importOutcome) map[string]int { + return map[string]int{ + "total": result.Summary.Total, "imported": result.Summary.Imported, + "planned": outcome.planned, "applied": outcome.applied, + "skipped": result.Summary.Skipped, "failed": result.Summary.Failed, + } +} + +// outputImportResultStructured outputs the import result in JSON/YAML format. +func outputImportResultStructured(result *configimport.ImportResult, outcome importOutcome) error { // Build output structure output := map[string]interface{}{ "format": result.Format, "format_name": result.FormatDisplayName, - "summary": result.Summary, + "summary": importOutputSummary(result, outcome), + "dry_run": outcome.dryRun, "imported": buildImportedServersOutput(result.Imported), "skipped": result.Skipped, "failed": result.Failed, @@ -2241,9 +2287,9 @@ func buildImportedServersOutput(imported []*configimport.ImportedServer) []map[s } // outputImportResultTable outputs the import result in table format -func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQuarantine bool, globalConfig *config.Config) error { +func outputImportResultTable(result *configimport.ImportResult, outcome importOutcome, noQuarantine bool) error { // Header - if dryRun { + if outcome.dryRun { fmt.Println("🔍 DRY RUN - No changes will be made") fmt.Println() } @@ -2254,14 +2300,15 @@ func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQ // Summary fmt.Printf("Summary:\n") fmt.Printf(" Total servers found: %d\n", result.Summary.Total) - fmt.Printf(" To import: %d\n", result.Summary.Imported) + fmt.Printf(" Planned: %d\n", outcome.planned) + fmt.Printf(" Applied: %d\n", outcome.applied) fmt.Printf(" Skipped: %d\n", result.Summary.Skipped) fmt.Printf(" Failed: %d\n", result.Summary.Failed) fmt.Println() // Show imported servers if len(result.Imported) > 0 { - if dryRun { + if outcome.dryRun { fmt.Println("Servers to import:") } else { fmt.Println("Imported servers:") @@ -2269,7 +2316,7 @@ func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQ for _, s := range result.Imported { statusIcon := "✅" - if dryRun { + if outcome.dryRun { statusIcon = "📋" } fmt.Printf(" %s %s (%s)\n", statusIcon, s.Server.Name, s.Server.Protocol) @@ -2325,12 +2372,7 @@ func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQ fmt.Println() } - // If not dry run and we have servers to import, actually add them - if !dryRun && len(result.Imported) > 0 { - err := applyImportedServers(result.Imported, globalConfig) - if err != nil { - return err - } + if outcome.applied > 0 { if noQuarantine { fmt.Println("✅ Servers imported without quarantine. They are ready to use.") } else { @@ -2342,7 +2384,7 @@ func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQ } // applyImportedServers adds the imported servers to the configuration -func applyImportedServers(imported []*configimport.ImportedServer, globalConfig *config.Config) error { +func applyImportedServers(imported []*configimport.ImportedServer, globalConfig *config.Config) ([]*configimport.ImportedServer, []configimport.FailedServer, error) { // Create context ctx := reqcontext.WithMetadata(context.Background(), reqcontext.SourceCLI) ctx, cancel := context.WithTimeout(ctx, 30*time.Second) @@ -2354,11 +2396,20 @@ func applyImportedServers(imported []*configimport.ImportedServer, globalConfig } // Direct config file mode - return applyImportedServersConfigMode(imported, globalConfig) + if err := applyImportedServersConfigMode(imported, globalConfig); err != nil { + failed := make([]configimport.FailedServer, 0, len(imported)) + for _, s := range imported { + failed = append(failed, configimport.FailedServer{Name: s.Server.Name, Error: "apply_failed", Details: err.Error()}) + } + return nil, failed, err + } + return imported, nil, nil } // applyImportedServersDaemonMode adds servers via the daemon -func applyImportedServersDaemonMode(ctx context.Context, client *cliclient.Client, imported []*configimport.ImportedServer) error { +func applyImportedServersDaemonMode(ctx context.Context, client *cliclient.Client, imported []*configimport.ImportedServer) ([]*configimport.ImportedServer, []configimport.FailedServer, error) { + var applied []*configimport.ImportedServer + var failed []configimport.FailedServer for _, s := range imported { req := &cliclient.AddServerRequest{ Name: s.Server.Name, @@ -2377,12 +2428,15 @@ func applyImportedServersDaemonMode(ctx context.Context, client *cliclient.Clien _, err := client.AddServer(ctx, req) if err != nil { - // Log error but continue with other servers - fmt.Fprintf(os.Stderr, " ❌ Failed to add '%s': %v\n", s.Server.Name, err) + failed = append(failed, configimport.FailedServer{Name: s.Server.Name, Error: "apply_failed", Details: err.Error()}) + continue } + applied = append(applied, s) } - - return nil + if len(failed) > 0 { + return applied, failed, fmt.Errorf("failed to import %d of %d servers", len(failed), len(imported)) + } + return applied, nil, nil } // runUpstreamInspect handles the 'upstream inspect' command (Spec 032) diff --git a/cmd/mcpproxy/upstream_import_effects_test.go b/cmd/mcpproxy/upstream_import_effects_test.go new file mode 100644 index 000000000..aee4eab64 --- /dev/null +++ b/cmd/mcpproxy/upstream_import_effects_test.go @@ -0,0 +1,321 @@ +package main + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/configimport" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +func importScratch(t *testing.T, source string) (string, string) { + t.Helper() + home := sandboxHome(t) + configPath := filepath.Join(home, "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{"listen":"127.0.0.1:19998","mcpServers":[]}`), 0600)) + sourcePath := filepath.Join(home, "source.json") + require.NoError(t, os.WriteFile(sourcePath, []byte(source), 0600)) + oldConfig, oldLocal := configFile, upstreamConfigPath + oldDry, oldFormat, oldServer, oldNoQuarantine := upstreamImportDryRun, upstreamImportFormat, upstreamImportServer, upstreamImportNoQuarantine + t.Cleanup(func() { + configFile, upstreamConfigPath = oldConfig, oldLocal + upstreamImportDryRun, upstreamImportFormat, upstreamImportServer, upstreamImportNoQuarantine = oldDry, oldFormat, oldServer, oldNoQuarantine + }) + configFile, upstreamConfigPath = configPath, "" + upstreamImportDryRun, upstreamImportFormat, upstreamImportServer, upstreamImportNoQuarantine = false, "", "", false + return configPath, sourcePath +} + +func captureImport(t *testing.T, sourcePath string) ([]byte, error) { + t.Helper() + old := os.Stdout + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stdout = w + defer func() { os.Stdout = old; _ = r.Close() }() + runErr := runUpstreamImport(nil, []string{sourcePath}) + require.NoError(t, w.Close()) + out, err := io.ReadAll(r) + require.NoError(t, err) + return out, runErr +} + +func TestUpstreamImportJSONAppliesBeforeReportingSuccess(t *testing.T) { + setOutputGlobals(t, "json", false) + configPath, sourcePath := importScratch(t, `{"mcpServers":{"new-one":{"command":"echo","args":["hello"]}}}`) + out, err := captureImport(t, sourcePath) + require.NoError(t, err) + var payload map[string]any + require.NoError(t, json.Unmarshal(out, &payload)) + cfg, err := config.LoadFromFile(configPath) + require.NoError(t, err) + require.Len(t, cfg.Servers, 1, "JSON success must persist its imported server") + require.Equal(t, "new-one", cfg.Servers[0].Name) + require.Equal(t, map[string]any{"total": float64(1), "imported": float64(1), "planned": float64(1), "applied": float64(1), "skipped": float64(0), "failed": float64(0)}, payload["summary"]) + require.Equal(t, false, payload["dry_run"]) +} + +func TestUpstreamImportTableAndYAMLApply(t *testing.T) { + for _, format := range []string{"table", "yaml"} { + t.Run(format, func(t *testing.T) { + setOutputGlobals(t, format, false) + configPath, sourcePath := importScratch(t, `{"mcpServers":{"new-one":{"command":"echo"}}}`) + out, err := captureImport(t, sourcePath) + require.NoError(t, err) + cfg, err := config.LoadFromFile(configPath) + require.NoError(t, err) + require.Len(t, cfg.Servers, 1) + if format == "yaml" { + var payload struct { + DryRun bool `yaml:"dry_run"` + Summary struct{ Planned, Applied, Failed int } `yaml:"summary"` + } + require.NoError(t, yaml.Unmarshal(out, &payload)) + require.False(t, payload.DryRun) + require.Equal(t, 1, payload.Summary.Planned) + require.Equal(t, 1, payload.Summary.Applied) + } else { + require.Contains(t, string(out), "Applied: 1") + } + }) + } +} + +func TestUpstreamImportDryRunAndDuplicateCounts(t *testing.T) { + setOutputGlobals(t, "json", false) + configPath, sourcePath := importScratch(t, `{"mcpServers":{"new-one":{"command":"echo"}}}`) + upstreamImportDryRun = true + out, err := captureImport(t, sourcePath) + require.NoError(t, err) + var payload struct { + DryRun bool `json:"dry_run"` + Summary struct{ Planned, Applied, Imported, Skipped int } `json:"summary"` + } + require.NoError(t, json.Unmarshal(out, &payload)) + require.True(t, payload.DryRun) + require.Equal(t, 1, payload.Summary.Planned) + require.Zero(t, payload.Summary.Applied) + // The legacy imported count continues to mean preview candidates in dry-run. + require.Equal(t, 1, payload.Summary.Imported) + cfg, err := config.LoadFromFile(configPath) + require.NoError(t, err) + require.Empty(t, cfg.Servers) + + upstreamImportDryRun = false + _, err = captureImport(t, sourcePath) + require.NoError(t, err) + out, err = captureImport(t, sourcePath) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(out, &payload)) + require.Zero(t, payload.Summary.Planned) + require.Zero(t, payload.Summary.Applied) + require.Equal(t, 1, payload.Summary.Skipped) + cfg, err = config.LoadFromFile(configPath) + require.NoError(t, err) + require.Len(t, cfg.Servers, 1) +} + +func TestUpstreamImportDaemonPartialFailureReportsAppliedAndFailed(t *testing.T) { + setOutputGlobals(t, "json", false) + _, sourcePath := importScratch(t, `{"mcpServers":{"good":{"command":"echo"},"bad":{"command":"echo"}}}`) + var added []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/status": + _, _ = w.Write([]byte(`{"success":true,"data":{"status":"running"}}`)) + case r.Method == http.MethodPost && r.URL.Path == "/api/v1/servers": + var req struct { + Name string `json:"name"` + } + _ = json.NewDecoder(r.Body).Decode(&req) + added = append(added, req.Name) + if req.Name == "bad" { + w.WriteHeader(http.StatusConflict) + return + } + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"success":true,"data":{"name":"good"}}`)) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + t.Setenv("MCPPROXY_TRAY_ENDPOINT", server.URL) + t.Setenv("MCPPROXY_API_KEY", "scratch-key") + upstreamImportDryRun = true + preview, err := captureImport(t, sourcePath) + require.NoError(t, err) + require.Empty(t, added, "dry-run may probe the daemon but must not add servers") + var previewPayload struct { + DryRun bool `json:"dry_run"` + Summary struct{ Planned, Applied int } `json:"summary"` + } + require.NoError(t, json.Unmarshal(preview, &previewPayload)) + require.True(t, previewPayload.DryRun) + require.Equal(t, 2, previewPayload.Summary.Planned) + require.Zero(t, previewPayload.Summary.Applied) + upstreamImportDryRun = false + out, err := captureImport(t, sourcePath) + require.Error(t, err) + require.Contains(t, err.Error(), "failed to import 1 of 2") + var payload struct { + Summary struct{ Planned, Applied, Imported, Failed int } `json:"summary"` + Imported []struct { + Name string `json:"name"` + } `json:"imported"` + Failed []struct{ Name, Error string } `json:"failed"` + } + require.NoError(t, json.Unmarshal(out, &payload)) + require.Equal(t, 2, payload.Summary.Planned) + require.Equal(t, 1, payload.Summary.Applied) + require.Equal(t, 1, payload.Summary.Imported) + require.Equal(t, 1, payload.Summary.Failed) + require.Equal(t, "good", payload.Imported[0].Name) + require.Equal(t, "bad", payload.Failed[0].Name) + require.ElementsMatch(t, []string{"good", "bad"}, added) + + setOutputGlobals(t, "table", false) + table, tableErr := captureImport(t, sourcePath) + require.Error(t, tableErr) + require.Contains(t, string(table), "Applied: 1") + require.Contains(t, string(table), "Failed: 1") + require.Contains(t, string(table), "Failed servers:") + require.Contains(t, string(table), "bad:") + require.Contains(t, string(table), "New servers are quarantined by default") +} + +func TestUpstreamImportParseFailureRetainsSuccessfulWrites(t *testing.T) { + setOutputGlobals(t, "json", false) + configPath, sourcePath := importScratch(t, `{"mcpServers":{"broken":{"url":"https://example.invalid/mcp"},"good":{"command":"echo"}}}`) + upstreamImportFormat = "claude-desktop" + out, err := captureImport(t, sourcePath) + require.Error(t, err) + var payload struct { + Summary struct{ Planned, Applied, Failed int } `json:"summary"` + Failed []struct{ Name, Error string } `json:"failed"` + } + require.NoError(t, json.Unmarshal(out, &payload)) + require.Equal(t, 1, payload.Summary.Planned) + require.Equal(t, 1, payload.Summary.Applied) + require.Equal(t, 1, payload.Summary.Failed) + require.Equal(t, "broken", payload.Failed[0].Name) + cfg, loadErr := config.LoadFromFile(configPath) + require.NoError(t, loadErr) + require.Len(t, cfg.Servers, 1) + require.Equal(t, "good", cfg.Servers[0].Name) +} + +func TestUpstreamImportDryRunInvalidSourceReportsValidationOnly(t *testing.T) { + setOutputGlobals(t, "json", false) + configPath, sourcePath := importScratch(t, `{"mcpServers":{"broken":{"url":"https://example.invalid/mcp"},"good":{"command":"echo"}}}`) + upstreamImportFormat = "claude-desktop" + upstreamImportDryRun = true + out, err := captureImport(t, sourcePath) + require.Error(t, err) + require.Contains(t, err.Error(), "validation failed for 1 of 2 source servers; no changes made") + var payload struct { + DryRun bool `json:"dry_run"` + Summary struct{ Planned, Applied, Failed int } `json:"summary"` + } + require.NoError(t, json.Unmarshal(out, &payload)) + require.True(t, payload.DryRun) + require.Equal(t, 1, payload.Summary.Planned) + require.Zero(t, payload.Summary.Applied) + require.Equal(t, 1, payload.Summary.Failed) + cfg, loadErr := config.LoadFromFile(configPath) + require.NoError(t, loadErr) + require.Empty(t, cfg.Servers) +} + +func TestUpstreamImportConfigSaveFailureRendersZeroApplied(t *testing.T) { + if runtime.GOOS == "windows" || os.Geteuid() == 0 { + t.Skip("directory permission bits are not enforced here") + } + setOutputGlobals(t, "json", false) + configPath, sourcePath := importScratch(t, `{"mcpServers":{"first":{"command":"echo"},"second":{"command":"echo"}}}`) + blockedDir := filepath.Join(filepath.Dir(configPath), "blocked") + require.NoError(t, os.Mkdir(blockedDir, 0700)) + blockedPath := filepath.Join(blockedDir, "config.json") + contents, err := os.ReadFile(configPath) + require.NoError(t, err) + require.NoError(t, os.WriteFile(blockedPath, contents, 0600)) + configFile = blockedPath + require.NoError(t, os.Chmod(blockedDir, 0500)) + t.Cleanup(func() { _ = os.Chmod(blockedDir, 0700) }) + out, runErr := captureImport(t, sourcePath) + require.Error(t, runErr) + require.Contains(t, runErr.Error(), "failed to save config") + var payload struct { + Summary struct{ Planned, Applied, Failed int } `json:"summary"` + Failed []struct{ Name, Error string } `json:"failed"` + } + require.NoError(t, json.Unmarshal(out, &payload)) + require.Equal(t, 2, payload.Summary.Planned) + require.Zero(t, payload.Summary.Applied) + require.Equal(t, 2, payload.Summary.Failed) + require.ElementsMatch(t, []string{"first", "second"}, []string{payload.Failed[0].Name, payload.Failed[1].Name}) + for _, item := range payload.Failed { + require.Equal(t, "apply_failed", item.Error) + } + stored, readErr := os.ReadFile(blockedPath) + require.NoError(t, readErr) + require.Equal(t, contents, stored) +} + +func TestApplyImportedServersConfigModeFailureReportsNoWrites(t *testing.T) { + home := sandboxHome(t) + parent := filepath.Join(home, "parent-file") + require.NoError(t, os.WriteFile(parent, []byte("sentinel"), 0600)) + oldConfig, oldLocal := configFile, upstreamConfigPath + t.Cleanup(func() { configFile, upstreamConfigPath = oldConfig, oldLocal }) + configFile, upstreamConfigPath = filepath.Join(parent, "config.json"), "" + items := []*configimport.ImportedServer{{Server: &config.ServerConfig{Name: "first", Command: "echo"}}, {Server: &config.ServerConfig{Name: "second", Command: "echo"}}} + cfg := &config.Config{DataDir: home} + applied, failed, err := applyImportedServers(items, cfg) + require.Error(t, err) + require.Empty(t, applied) + require.Len(t, failed, 2) + require.Equal(t, "first", failed[0].Name) + require.Equal(t, "second", failed[1].Name) + contents, readErr := os.ReadFile(parent) + require.NoError(t, readErr) + require.Equal(t, "sentinel", string(contents)) +} + +func TestApplyImportedServersDaemonModeContinuesAfterFailure(t *testing.T) { + var added []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req struct { + Name string `json:"name"` + } + _ = json.NewDecoder(r.Body).Decode(&req) + added = append(added, req.Name) + if req.Name == "bad" { + w.WriteHeader(http.StatusConflict) + return + } + _, _ = w.Write([]byte(`{"success":true,"data":{}}`)) + })) + defer server.Close() + client := cliclient.NewClientWithAPIKey(server.URL, "scratch-key", nil) + items := []*configimport.ImportedServer{{Server: &config.ServerConfig{Name: "bad"}}, {Server: &config.ServerConfig{Name: "good"}}} + applied, failed, err := applyImportedServersDaemonMode(context.Background(), client, items) + require.Error(t, err) + require.Equal(t, []string{"bad", "good"}, added) + require.Len(t, applied, 1) + require.Equal(t, "good", applied[0].Server.Name) + require.Len(t, failed, 1) + require.Equal(t, "bad", failed[0].Name) + require.True(t, strings.Contains(failed[0].Details, "already exists")) +} diff --git a/docs/cli/command-reference.md b/docs/cli/command-reference.md index a89fcbe46..edb6f1e96 100644 --- a/docs/cli/command-reference.md +++ b/docs/cli/command-reference.md @@ -311,6 +311,15 @@ mcpproxy upstream import [flags] | `--dry-run` | Preview import without making changes | `false` | | `--no-quarantine` | Don't quarantine imported servers (use with caution) | `false` | +Table, JSON, and YAML output apply the same import. Structured output includes +`dry_run` and `summary.planned`, `summary.applied`, `summary.skipped`, and +`summary.failed`. The existing `summary.imported` field remains available: +it counts applied servers during an import and preview candidates with +`--dry-run`. A partial failure is rendered with per-server details and returns +a nonzero exit status; successfully applied servers remain imported. An invalid +preview also returns nonzero and reports a validation failure without making +changes. + **Supported Formats:** | Source | Format Flag | Auto-detected |