fix(import): reject servers that fail config validation before persisting (RC4-IMPORT-001)

internal/config/config.go

@@ -2335,6 +2335,25 @@ func (c *Config) ValidateDetailed() []ValidationError {
 	return errors
 }
 
+// ValidateServerForLoad runs the per-server checks config.Load applies at boot
+// (the stdio command-required check, url-required for HTTP protocols, protocol
+// and name validity, ...) against a single server and returns only the errors
+// attributable to it. Write surfaces that persist a server built outside the
+// config loader (e.g. imports) use it so they can never save an entry the next
+// startup would refuse to load.
+func ValidateServerForLoad(server *ServerConfig) []ValidationError {
+	cfg := DefaultConfig()
+	cfg.Servers = []*ServerConfig{server}
+	const prefix = "mcpServers[0]"
+	var out []ValidationError
+	for _, e := range cfg.validateDetailedCore() {
+		if strings.HasPrefix(e.Field, prefix) {
+			out = append(out, e)
+		}
+	}
+	return out
+}
+
 // oauthRedirectURIErrors reports every per-server `oauth.redirect_uri` that the
 // loopback callback listener cannot honor.
 //

internal/configimport/import.go

@@ -2,7 +2,10 @@ package configimport
 
 import (
 	"fmt"
+	"strings"
 	"time"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
 )
 
 // Import parses configuration content and imports servers.
@@ -146,6 +149,22 @@ func Import(content []byte, opts *ImportOptions) (*ImportResult, error) {
 		// Map to ServerConfig
 		serverConfig, skipped, warnings := MapToServerConfig(parsed, opts.Now)
 
+		// Reject entries the next config.Load would refuse (e.g. a URL-only
+		// entry forced through a stdio-only format yields a commandless stdio
+		// server). Persisting one bricks the next startup with exit code 4.
+		if verrs := config.ValidateServerForLoad(serverConfig); len(verrs) > 0 {
+			msgs := make([]string, 0, len(verrs))
+			for _, ve := range verrs {
+				msgs = append(msgs, ve.Message)
+			}
+			result.Failed = append(result.Failed, FailedServer{
+				Name:    originalName,
+				Error:   "invalid_server",
+				Details: strings.Join(msgs, "; "),
+			})
+			continue
+		}
+
 		// Override quarantine if SkipQuarantine is set
 		if opts.SkipQuarantine {
 			serverConfig.Quarantined = false

internal/configimport/import_invalid_server_test.go

@@ -0,0 +1,34 @@
+package configimport
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// RC4-IMPORT-001: a URL-only entry forced through the claude-desktop parser
+// maps to a commandless stdio server. config.Load rejects that on the next
+// boot (exit 4), so Import must report it as failed and never offer it for
+// persistence, while valid entries in the same payload still import.
+func TestImport_RejectsServerThatFailsConfigValidation(t *testing.T) {
+	content := []byte(`{"mcpServers": {
+		"broken": {"url": "https://example.com/mcp"},
+		"good": {"command": "npx", "args": ["-y", "some-server"]}
+	}}`)
+
+	for _, skipQuarantine := range []bool{false, true} {
+		result, err := Import(content, &ImportOptions{FormatHint: FormatClaudeDesktop, SkipQuarantine: skipQuarantine})
+		require.NoError(t, err)
+
+		require.Len(t, result.Imported, 1, "only the valid server may be importable")
+		assert.Equal(t, "good", result.Imported[0].Server.Name)
+
+		require.Len(t, result.Failed, 1)
+		assert.Equal(t, "broken", result.Failed[0].Name)
+		assert.Equal(t, "invalid_server", result.Failed[0].Error)
+		assert.Contains(t, result.Failed[0].Details, "command is required")
+		assert.Equal(t, 1, result.Summary.Failed)
+		assert.Equal(t, 1, result.Summary.Imported)
+	}
+}

internal/configimport/import_test.go

@@ -54,8 +54,14 @@ func TestImport(t *testing.T) {
 		if result.Format != FormatCodex {
 			t.Errorf("Format = %v, want %v", result.Format, FormatCodex)
 		}
-		if result.Summary.Imported != 5 {
-			t.Errorf("Summary.Imported = %d, want 5", result.Summary.Imported)
+		// The fixture's `filesystem` entry has cwd=/home/user, which fails the
+		// same working_dir-exists check config.Load applies when that directory
+		// is absent on the machine running the test, so it may be Failed.
+		if got := result.Summary.Imported + result.Summary.Failed; got != 5 {
+			t.Errorf("Summary.Imported+Failed = %d, want 5", got)
+		}
+		if result.Summary.Imported < 4 {
+			t.Errorf("Summary.Imported = %d, want at least 4", result.Summary.Imported)
 		}
 	})
 

internal/httpapi/import.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
 	"github.com/smart-mcp-proxy/mcpproxy-go/internal/configimport"
 	"github.com/smart-mcp-proxy/mcpproxy-go/internal/oauth"
 )
@@ -507,6 +508,28 @@ func (s *Server) runImport(r *http.Request, content []byte, formatHint string, s
 				result.Imported[i].Server.Name = newName
 			}
 		}
+		// configimport validated the pre-rename name; re-check the renamed
+		// servers with the same boot-path validation so a rename cannot
+		// persist an entry the next startup refuses (RC4-IMPORT-001).
+		kept := result.Imported[:0]
+		for _, imported := range result.Imported {
+			if errs := config.ValidateServerForLoad(imported.Server); len(errs) > 0 {
+				msgs := make([]string, 0, len(errs))
+				for _, e := range errs {
+					msgs = append(msgs, e.Error())
+				}
+				result.Failed = append(result.Failed, configimport.FailedServer{
+					Name:    imported.Server.Name,
+					Error:   "invalid_server",
+					Details: strings.Join(msgs, "; "),
+				})
+				continue
+			}
+			kept = append(kept, imported)
+		}
+		result.Imported = kept
+		result.Summary.Imported = len(result.Imported)
+		result.Summary.Failed = len(result.Failed)
 	}
 
 	// Apply the caller's field overrides (Paste tab env/header edits)

internal/httpapi/import_invalid_server_test.go

@@ -0,0 +1,65 @@
+package httpapi
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+)
+
+type recordingImportController struct {
+	mockImportController
+	mu    sync.Mutex
+	added []string
+}
+
+func (m *recordingImportController) AddServer(_ context.Context, s *config.ServerConfig) error {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	m.added = append(m.added, s.Name)
+	return nil
+}
+
+// RC4-IMPORT-001: a URL-only entry imported with a forced claude-desktop
+// format and skip_quarantine=true used to be persisted as a commandless stdio
+// server, which made the next core startup exit with code 4.
+func TestImportServersJSON_RejectsInvalidServerNotPersisted(t *testing.T) {
+	mock := &recordingImportController{mockImportController: mockImportController{apiKey: "test-key"}}
+	server := NewServer(mock, zap.NewNop().Sugar(), nil)
+
+	body, _ := json.Marshal(ImportRequest{
+		Format: "claude-desktop",
+		Content: `{"mcpServers": {
+			"broken": {"url": "https://example.com/mcp"},
+			"good": {"command": "npx", "args": ["-y", "some-server"]}
+		}}`,
+	})
+	req := httptest.NewRequest("POST", "/api/v1/servers/import/json?skip_quarantine=true", bytes.NewReader(body))
+	req.Header.Set("Content-Type", "application/json")
+	req.Header.Set("X-API-Key", "test-key")
+	rr := httptest.NewRecorder()
+	server.router.ServeHTTP(rr, req)
+
+	require.Equal(t, http.StatusOK, rr.Code, rr.Body.String())
+	var wrapped wrappedImportResponse
+	require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &wrapped))
+
+	require.Len(t, wrapped.Data.Imported, 1)
+	assert.Equal(t, "good", wrapped.Data.Imported[0].Name)
+	require.Len(t, wrapped.Data.Failed, 1)
+	assert.Equal(t, "broken", wrapped.Data.Failed[0].Name)
+	assert.Contains(t, wrapped.Data.Failed[0].Details, "command is required")
+
+	mock.mu.Lock()
+	defer mock.mu.Unlock()
+	assert.Equal(t, []string{"good"}, mock.added, "the invalid server must never reach AddServer")
+}

internal/httpapi/import_test.go

@@ -739,3 +739,25 @@ func TestImportServersJSON_EmptyMcpServersStill400(t *testing.T) {
 		t.Errorf("Expected status 400, got %d: %s", rr.Code, rr.Body.String())
 	}
 }
+
+// RC4-IMPORT-001 follow-up: configimport validated the pre-rename name, so a
+// rename to a name the boot path refuses (':' is the qualified-name
+// separator) must fail the entry instead of persisting it.
+func TestRunImport_RenameToInvalidNameIsRejected(t *testing.T) {
+	const content = `{"mcpServers": {"good": {"command": "good-mcp"}}}`
+	mock := &mockImportController{apiKey: "test-key"}
+	server := NewServer(mock, zap.NewNop().Sugar(), nil)
+	req := httptest.NewRequest("POST", "/api/v1/servers/import/path", http.NoBody)
+	req.Header.Set("X-API-Key", "test-key")
+
+	resp, err := server.runImport(req, []byte(content), "claude-desktop", nil, true, map[string]string{"good": "bad:name"}, nil, false)
+	if err != nil {
+		t.Fatalf("runImport returned error: %v", err)
+	}
+	if len(resp.Imported) != 0 {
+		t.Fatalf("a rename to an invalid name must not be imported, got %+v", resp.Imported)
+	}
+	if len(resp.Failed) != 1 || resp.Failed[0].Error != "invalid_server" {
+		t.Fatalf("expected one invalid_server failure, got %+v", resp.Failed)
+	}
+}