Skip to content

Commit e382bb2

Browse files
authored
fix(cli): apply imports before rendering structured output (#1729)
## Description Fix UX audit F1: `upstream import -o json` and `-o yaml` previously reported imported servers without applying them. All output formats now execute the same import before rendering, expose planned/applied counts and dry-run state, and return nonzero with per-server details for partial failures. Invalid previews report validation failure and explicitly make no changes. ## Testing - Regression reproduced before the fix. - `go test ./cmd/mcpproxy ./internal/configimport -count=1` and focused import suite with `-race` pass. - Rebuilt current head and verified JSON/YAML/table effects in both a scratch daemon and saved config; dry-run and invalid preview made no writes; duplicate replay skipped; partial failure returned nonzero and retained the successful write. - Independent fresh-context Claude CLI (`claude-fable-5-1`) review: **VERDICT: PASS** at dc8c9b7, after correcting dry-run wording and Windows/root test portability. - Exact-head `qa-gate`: success. CI must pass before merge. Example partial outcome: `planned=1 applied=1 failed=1`, exit 1. Invalid dry-run: `planned=1 applied=0 failed=1`, exit 1, `validation failed ... no changes made`.
1 parent 14cb288 commit e382bb2

3 files changed

Lines changed: 410 additions & 26 deletions

File tree

‎cmd/mcpproxy/upstream_cmd.go‎

Lines changed: 80 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -2113,14 +2113,45 @@ func runUpstreamImport(_ *cobra.Command, args []string) error {
21132113
return outputError(output.NewStructuredError(output.ErrCodeOperationFailed, err.Error()), output.ErrCodeOperationFailed)
21142114
}
21152115

2116-
// Output results based on format
2116+
// Apply once, before choosing a renderer. The parser's Imported slice is a
2117+
// plan; only successful writes may be reported as imported after execution.
2118+
planned := len(result.Imported)
2119+
applied := 0
2120+
var applyErr error
2121+
if !upstreamImportDryRun && planned > 0 {
2122+
var successful []*configimport.ImportedServer
2123+
var failed []configimport.FailedServer
2124+
successful, failed, applyErr = applyImportedServers(result.Imported, globalConfig)
2125+
result.Imported = successful
2126+
result.Failed = append(result.Failed, failed...)
2127+
result.Summary.Imported = len(successful)
2128+
result.Summary.Failed = len(result.Failed)
2129+
applied = len(successful)
2130+
}
2131+
outcome := importOutcome{planned: planned, applied: applied, dryRun: upstreamImportDryRun}
2132+
2133+
// Render the completed outcome even when some writes failed. Automation gets
2134+
// the per-server failures on stdout and a nonzero exit status.
21172135
outputFormat := ResolveOutputFormat()
2118-
2136+
var outputErr error
21192137
if outputFormat == "json" || outputFormat == "yaml" {
2120-
return outputImportResultStructured(result, outputFormat)
2138+
outputErr = outputImportResultStructured(result, outcome)
2139+
} else {
2140+
outputErr = outputImportResultTable(result, outcome, upstreamImportNoQuarantine)
21212141
}
2122-
2123-
return outputImportResultTable(result, upstreamImportDryRun, upstreamImportNoQuarantine, globalConfig)
2142+
if outputErr != nil {
2143+
return outputErr
2144+
}
2145+
if applyErr != nil {
2146+
return applyErr
2147+
}
2148+
if result.Summary.Failed > 0 {
2149+
if outcome.dryRun {
2150+
return fmt.Errorf("validation failed for %d of %d source servers; no changes made", result.Summary.Failed, result.Summary.Total)
2151+
}
2152+
return fmt.Errorf("failed to import %d of %d servers", result.Summary.Failed, result.Summary.Total)
2153+
}
2154+
return nil
21242155
}
21252156

21262157
// importSelfListenAddrs returns the addresses the local mcpproxy answers on,
@@ -2164,13 +2195,28 @@ func parseImportFormat(format string) configimport.ConfigFormat {
21642195
}
21652196
}
21662197

2167-
// outputImportResultStructured outputs the import result in JSON/YAML format
2168-
func outputImportResultStructured(result *configimport.ImportResult, format string) error {
2198+
type importOutcome struct {
2199+
planned int
2200+
applied int
2201+
dryRun bool
2202+
}
2203+
2204+
func importOutputSummary(result *configimport.ImportResult, outcome importOutcome) map[string]int {
2205+
return map[string]int{
2206+
"total": result.Summary.Total, "imported": result.Summary.Imported,
2207+
"planned": outcome.planned, "applied": outcome.applied,
2208+
"skipped": result.Summary.Skipped, "failed": result.Summary.Failed,
2209+
}
2210+
}
2211+
2212+
// outputImportResultStructured outputs the import result in JSON/YAML format.
2213+
func outputImportResultStructured(result *configimport.ImportResult, outcome importOutcome) error {
21692214
// Build output structure
21702215
output := map[string]interface{}{
21712216
"format": result.Format,
21722217
"format_name": result.FormatDisplayName,
2173-
"summary": result.Summary,
2218+
"summary": importOutputSummary(result, outcome),
2219+
"dry_run": outcome.dryRun,
21742220
"imported": buildImportedServersOutput(result.Imported),
21752221
"skipped": result.Skipped,
21762222
"failed": result.Failed,
@@ -2241,9 +2287,9 @@ func buildImportedServersOutput(imported []*configimport.ImportedServer) []map[s
22412287
}
22422288

22432289
// outputImportResultTable outputs the import result in table format
2244-
func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQuarantine bool, globalConfig *config.Config) error {
2290+
func outputImportResultTable(result *configimport.ImportResult, outcome importOutcome, noQuarantine bool) error {
22452291
// Header
2246-
if dryRun {
2292+
if outcome.dryRun {
22472293
fmt.Println("🔍 DRY RUN - No changes will be made")
22482294
fmt.Println()
22492295
}
@@ -2254,22 +2300,23 @@ func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQ
22542300
// Summary
22552301
fmt.Printf("Summary:\n")
22562302
fmt.Printf(" Total servers found: %d\n", result.Summary.Total)
2257-
fmt.Printf(" To import: %d\n", result.Summary.Imported)
2303+
fmt.Printf(" Planned: %d\n", outcome.planned)
2304+
fmt.Printf(" Applied: %d\n", outcome.applied)
22582305
fmt.Printf(" Skipped: %d\n", result.Summary.Skipped)
22592306
fmt.Printf(" Failed: %d\n", result.Summary.Failed)
22602307
fmt.Println()
22612308

22622309
// Show imported servers
22632310
if len(result.Imported) > 0 {
2264-
if dryRun {
2311+
if outcome.dryRun {
22652312
fmt.Println("Servers to import:")
22662313
} else {
22672314
fmt.Println("Imported servers:")
22682315
}
22692316

22702317
for _, s := range result.Imported {
22712318
statusIcon := "✅"
2272-
if dryRun {
2319+
if outcome.dryRun {
22732320
statusIcon = "📋"
22742321
}
22752322
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
23252372
fmt.Println()
23262373
}
23272374

2328-
// If not dry run and we have servers to import, actually add them
2329-
if !dryRun && len(result.Imported) > 0 {
2330-
err := applyImportedServers(result.Imported, globalConfig)
2331-
if err != nil {
2332-
return err
2333-
}
2375+
if outcome.applied > 0 {
23342376
if noQuarantine {
23352377
fmt.Println("✅ Servers imported without quarantine. They are ready to use.")
23362378
} else {
@@ -2342,7 +2384,7 @@ func outputImportResultTable(result *configimport.ImportResult, dryRun bool, noQ
23422384
}
23432385

23442386
// applyImportedServers adds the imported servers to the configuration
2345-
func applyImportedServers(imported []*configimport.ImportedServer, globalConfig *config.Config) error {
2387+
func applyImportedServers(imported []*configimport.ImportedServer, globalConfig *config.Config) ([]*configimport.ImportedServer, []configimport.FailedServer, error) {
23462388
// Create context
23472389
ctx := reqcontext.WithMetadata(context.Background(), reqcontext.SourceCLI)
23482390
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
@@ -2354,11 +2396,20 @@ func applyImportedServers(imported []*configimport.ImportedServer, globalConfig
23542396
}
23552397

23562398
// Direct config file mode
2357-
return applyImportedServersConfigMode(imported, globalConfig)
2399+
if err := applyImportedServersConfigMode(imported, globalConfig); err != nil {
2400+
failed := make([]configimport.FailedServer, 0, len(imported))
2401+
for _, s := range imported {
2402+
failed = append(failed, configimport.FailedServer{Name: s.Server.Name, Error: "apply_failed", Details: err.Error()})
2403+
}
2404+
return nil, failed, err
2405+
}
2406+
return imported, nil, nil
23582407
}
23592408

23602409
// applyImportedServersDaemonMode adds servers via the daemon
2361-
func applyImportedServersDaemonMode(ctx context.Context, client *cliclient.Client, imported []*configimport.ImportedServer) error {
2410+
func applyImportedServersDaemonMode(ctx context.Context, client *cliclient.Client, imported []*configimport.ImportedServer) ([]*configimport.ImportedServer, []configimport.FailedServer, error) {
2411+
var applied []*configimport.ImportedServer
2412+
var failed []configimport.FailedServer
23622413
for _, s := range imported {
23632414
req := &cliclient.AddServerRequest{
23642415
Name: s.Server.Name,
@@ -2377,12 +2428,15 @@ func applyImportedServersDaemonMode(ctx context.Context, client *cliclient.Clien
23772428

23782429
_, err := client.AddServer(ctx, req)
23792430
if err != nil {
2380-
// Log error but continue with other servers
2381-
fmt.Fprintf(os.Stderr, " ❌ Failed to add '%s': %v\n", s.Server.Name, err)
2431+
failed = append(failed, configimport.FailedServer{Name: s.Server.Name, Error: "apply_failed", Details: err.Error()})
2432+
continue
23822433
}
2434+
applied = append(applied, s)
23832435
}
2384-
2385-
return nil
2436+
if len(failed) > 0 {
2437+
return applied, failed, fmt.Errorf("failed to import %d of %d servers", len(failed), len(imported))
2438+
}
2439+
return applied, nil, nil
23862440
}
23872441

23882442
// runUpstreamInspect handles the 'upstream inspect' command (Spec 032)

0 commit comments

Comments
 (0)