From b847df70fce47656c9d02cf796070cc28c6e29d3 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Tue, 29 Sep 2026 14:59:01 +0300 Subject: [PATCH 1/3] fix(catalog): emit added_server_name from CLI search/show and retire legacy AddServerModal --- ROADMAP.md | 2 +- cmd/mcpproxy/catalog_cmd.go | 59 +- cmd/mcpproxy/catalog_cmd_test.go | 73 +- frontend/src/components/AddServerModal.vue | 1256 ----------------- frontend/src/components/AuthErrorModal.vue | 4 +- .../add-server-duplicate-endpoint.spec.ts | 273 ---- frontend/tests/unit/add-server-manual.spec.ts | 48 + ...-server-modal-protocol-and-handoff.spec.ts | 213 --- .../tests/unit/add-server-trust-mode.spec.ts | 212 --- frontend/tests/unit/modal-a11y.spec.ts | 126 +- .../onboarding-wizard-client-paths.spec.ts | 1 - .../onboarding-wizard-dismiss-race.spec.ts | 2 - ...rding-wizard-import-conflict-toast.spec.ts | 1 - .../onboarding-wizard-import-footer.spec.ts | 5 - .../onboarding-wizard-import-rows.spec.ts | 1 - .../onboarding-wizard-inline-review.spec.ts | 1 - .../onboarding-wizard-servers-step.spec.ts | 3 - ...nboarding-wizard-skip-reason-label.spec.ts | 1 - ...nboarding-wizard-usable-completion.spec.ts | 1 - .../onboarding-wizard-verify-hints.spec.ts | 1 - .../unit/servers-first-run-empty.spec.ts | 5 - .../unit/telemetry-banner-wizard.spec.ts | 2 - frontend/tests/unit/z-index-scale.spec.ts | 1 - specs/109-ux-navigation-consistency/tasks.md | 2 +- 24 files changed, 158 insertions(+), 2135 deletions(-) delete mode 100644 frontend/src/components/AddServerModal.vue delete mode 100644 frontend/tests/unit/add-server-duplicate-endpoint.spec.ts delete mode 100644 frontend/tests/unit/add-server-modal-protocol-and-handoff.spec.ts delete mode 100644 frontend/tests/unit/add-server-trust-mode.spec.ts diff --git a/ROADMAP.md b/ROADMAP.md index 677f89a60..70f661135 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -1036,5 +1036,5 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) | | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) | | [108-profiles-v3](./specs/108-profiles-v3/) | `in-flight` | 23/153 (15%) | -| [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `in-flight` | 35/180 (19%) | +| [109-ux-navigation-consistency](./specs/109-ux-navigation-consistency/) | `in-flight` | 36/180 (20%) | | [110-catalog-popularity](./specs/110-catalog-popularity/) | `in-flight` | 19/23 (83%) | diff --git a/cmd/mcpproxy/catalog_cmd.go b/cmd/mcpproxy/catalog_cmd.go index 4d84d11db..8ccc8e4cc 100644 --- a/cmd/mcpproxy/catalog_cmd.go +++ b/cmd/mcpproxy/catalog_cmd.go @@ -155,8 +155,7 @@ func newCatalogShowCmd() *cobra.Command { } hit := registries.BuildCatalogHit(reg, *entry) - result := registries.ToCatalogResult(hit, catalogAddedFromConfig(cfg)(hit)) - return renderCatalogShow(formatter, result) + return renderCatalogShow(formatter, catalogResultForConfig(cfg, hit)) }, } return cmd @@ -273,33 +272,43 @@ func catalogSearchInProcess(ctx context.Context, cfg *config.Config, q, source, // httpapi.handleCatalogSearch). hits, sections, unavailable := registries.SearchAll(ctx, q, tag, limit, registries.SearchOptions{Source: source}) - added := catalogAddedFromConfig(cfg) resp := &cliclient.CatalogSearchResponse{Query: q, Unavailable: unavailable} if resp.Unavailable == nil { resp.Unavailable = []registries.SourceError{} } for _, h := range hits { - resp.Results = append(resp.Results, registries.ToCatalogResult(h, added(h))) + resp.Results = append(resp.Results, catalogResultForConfig(cfg, h)) } if sections != nil { resp.Sections = &cliclient.CatalogSections{} for _, h := range sections.Official { - resp.Sections.Official = append(resp.Sections.Official, registries.ToCatalogResult(h, added(h))) + resp.Sections.Official = append(resp.Sections.Official, catalogResultForConfig(cfg, h)) } for _, h := range sections.Popular { - resp.Sections.Popular = append(resp.Sections.Popular, registries.ToCatalogResult(h, added(h))) + resp.Sections.Popular = append(resp.Sections.Popular, catalogResultForConfig(cfg, h)) } } return resp, nil } -// catalogAddedFromConfig returns a predicate reporting whether a catalog hit -// matches an already-configured server (contracts/rest-api.md#catalog "added"): -// a registry-sourced server also needs a matching source, a manual add -// matches on install target alone. -func catalogAddedFromConfig(cfg *config.Config) func(registries.CatalogHit) bool { - byRegistryAndTarget := make(map[string]bool) - byTargetOnly := make(map[string]bool) +// catalogResultForConfig builds a CatalogResult for hit, joining it against the +// loaded config the way REST does: Added, plus AddedServerName when exactly one +// configured server matches. +func catalogResultForConfig(cfg *config.Config, hit registries.CatalogHit) registries.CatalogResult { + isAdded, name := catalogAddedFromConfig(cfg)(hit) + result := registries.ToCatalogResult(hit, isAdded) + result.AddedServerName = name + return result +} + +// catalogAddedFromConfig returns a resolver reporting whether a catalog hit +// matches an already-configured server (contracts/rest-api.md#catalog "added") +// and, only for a unique match, that server's name (mirrors +// httpapi.catalogAddedResolver): a registry-sourced server also needs a +// matching source, a manual add matches on install target alone. +func catalogAddedFromConfig(cfg *config.Config) func(registries.CatalogHit) (bool, string) { + byRegistryAndTarget := make(map[string][]string) + byTargetOnly := make(map[string][]string) if cfg != nil { for _, s := range cfg.Servers { if s == nil { @@ -308,21 +317,33 @@ func catalogAddedFromConfig(cfg *config.Config) func(registries.CatalogHit) bool target := catalogInstallTargetForConfigServer(s) if s.SourceRegistryID == "" { // Manual add: matches any source by install target alone. - byTargetOnly[target] = true + byTargetOnly[target] = append(byTargetOnly[target], s.Name) } else { // Registry-sourced: must also match its own source, so it // never falsely matches a different source's identical // install target (contracts/rest-api.md#catalog "added"). - byRegistryAndTarget[s.SourceRegistryID+"\x00"+target] = true + key := s.SourceRegistryID + "\x00" + target + byRegistryAndTarget[key] = append(byRegistryAndTarget[key], s.Name) } } } - return func(h registries.CatalogHit) bool { + return func(h registries.CatalogHit) (bool, string) { target := registries.CatalogInstallTarget(registries.ToCatalogResult(h, false).Install) - if byRegistryAndTarget[h.Source+"\x00"+target] { - return true + names := append([]string(nil), byRegistryAndTarget[h.Source+"\x00"+target]...) + names = append(names, byTargetOnly[target]...) + if len(names) == 0 { + return false, "" + } + // De-duplicate defensively so a malformed legacy configuration with a + // repeated name is not reported as ambiguous. + unique := make(map[string]struct{}, len(names)) + for _, name := range names { + unique[name] = struct{}{} + } + if len(unique) == 1 { + return true, names[0] } - return byTargetOnly[target] + return true, "" } } diff --git a/cmd/mcpproxy/catalog_cmd_test.go b/cmd/mcpproxy/catalog_cmd_test.go index 50c439b04..37290a409 100644 --- a/cmd/mcpproxy/catalog_cmd_test.go +++ b/cmd/mcpproxy/catalog_cmd_test.go @@ -3,6 +3,7 @@ package main import ( "bytes" "context" + "encoding/json" "io" "net/http" "net/http/httptest" @@ -38,34 +39,73 @@ func TestParseCatalogRef_RejectsMissingSlash(t *testing.T) { // TestCatalogAddedFromConfig pins the join rule (contracts/rest-api.md#catalog // "added"): a registry-sourced configured server needs source AND install -// target; a manual add matches on install target alone. +// target; a manual add matches on install target alone. The resolver also +// reports the server name, but only for a unique match (REST parity). func TestCatalogAddedFromConfig(t *testing.T) { cfg := &config.Config{ Servers: []*config.ServerConfig{ {Name: "manual", URL: "https://manual.example.com/mcp"}, {Name: "from-official", Command: "npx", Args: []string{"server-x"}, SourceRegistryID: "official"}, + {Name: "dup-a", Command: "npx", Args: []string{"server-dup"}}, + {Name: "dup-b", Command: "npx", Args: []string{"server-dup"}}, }, } added := catalogAddedFromConfig(cfg) manualHit := registries.CatalogHit{Source: "smithery", Entry: registries.ServerEntry{ID: "m", URL: "https://manual.example.com/mcp"}} - if !added(manualHit) { - t.Error("expected a manual add to match by install target regardless of source") + if ok, name := added(manualHit); !ok || name != "manual" { + t.Errorf("expected a manual add to match by install target regardless of source, got (%v, %q)", ok, name) } officialHit := registries.CatalogHit{Source: "official", Entry: registries.ServerEntry{ID: "x", InstallCmd: "npx server-x"}} - if !added(officialHit) { - t.Error("expected the registry-sourced server to match its own source + target") + if ok, name := added(officialHit); !ok || name != "from-official" { + t.Errorf("expected the registry-sourced server to match its own source + target, got (%v, %q)", ok, name) } wrongSourceHit := registries.CatalogHit{Source: "smithery", Entry: registries.ServerEntry{ID: "x", InstallCmd: "npx server-x"}} - if added(wrongSourceHit) { - t.Error("a registry-sourced configured server must not match a different source with the same target") + if ok, name := added(wrongSourceHit); ok || name != "" { + t.Errorf("a registry-sourced configured server must not match a different source with the same target, got (%v, %q)", ok, name) } noMatchHit := registries.CatalogHit{Source: "official", Entry: registries.ServerEntry{ID: "y", InstallCmd: "npx server-y"}} - if added(noMatchHit) { - t.Error("expected no match for an unrelated entry") + if ok, name := added(noMatchHit); ok || name != "" { + t.Errorf("expected no match for an unrelated entry, got (%v, %q)", ok, name) + } + + ambiguousHit := registries.CatalogHit{Source: "official", Entry: registries.ServerEntry{ID: "d", InstallCmd: "npx server-dup"}} + if ok, name := added(ambiguousHit); !ok || name != "" { + t.Errorf("expected added=true with no name when two servers match, got (%v, %q)", ok, name) + } +} + +// TestCatalogSearchInProcess_AddedServerName pins CLI/REST parity: the offline +// search path emits added_server_name for a uniquely matched configured server. +func TestCatalogSearchInProcess_AddedServerName(t *testing.T) { + withCatalogCLIFixture(t, `[{"id":"gh","name":"GitHub Tool","description":"desc","url":"https://x.example.com/mcp"}]`) + cfg := &config.Config{Servers: []*config.ServerConfig{{Name: "my-gh", URL: "https://x.example.com/mcp"}}} + + resp, err := catalogSearchInProcess(context.Background(), cfg, "GitHub", "", "", 10) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(resp.Results) != 1 { + t.Fatalf("expected 1 result, got %+v", resp.Results) + } + if !resp.Results[0].Added || resp.Results[0].AddedServerName != "my-gh" { + t.Errorf("expected added=true added_server_name=my-gh, got %+v", resp.Results[0]) + } + + empty, err := catalogSearchInProcess(context.Background(), cfg, "", "", "", 10) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if empty.Sections == nil { + t.Fatal("expected sections for an empty query") + } + for _, r := range append(append([]registries.CatalogResult{}, empty.Sections.Official...), empty.Sections.Popular...) { + if r.ID == "gh" && r.AddedServerName != "my-gh" { + t.Errorf("section entry missing added_server_name: %+v", r) + } } } @@ -207,6 +247,21 @@ func TestCatalogShow(t *testing.T) { } } +// TestCatalogResultForConfig pins that 'catalog show' builds its result with +// the added_server_name join, like REST. +func TestCatalogResultForConfig(t *testing.T) { + cfg := &config.Config{Servers: []*config.ServerConfig{{Name: "my-gh", URL: "https://x.example.com/mcp"}}} + hit := registries.CatalogHit{Source: "official", Entry: registries.ServerEntry{ID: "gh", URL: "https://x.example.com/mcp"}} + result := catalogResultForConfig(cfg, hit) + if !result.Added || result.AddedServerName != "my-gh" { + t.Errorf("expected added=true added_server_name=my-gh, got %+v", result) + } + data, err := json.Marshal(result) + if err != nil || !strings.Contains(string(data), `"added_server_name":"my-gh"`) { + t.Errorf("expected added_server_name in JSON, got %s (err %v)", data, err) + } +} + // TestPrintCatalogDeprecationNotice pins FR-066: 'registry search'/'registry // add' print a deprecation note pointing at the 'catalog' equivalent. func TestPrintCatalogDeprecationNotice(t *testing.T) { diff --git a/frontend/src/components/AddServerModal.vue b/frontend/src/components/AddServerModal.vue deleted file mode 100644 index 4b26ec8e1..000000000 --- a/frontend/src/components/AddServerModal.vue +++ /dev/null @@ -1,1256 +0,0 @@ - - - diff --git a/frontend/src/components/AuthErrorModal.vue b/frontend/src/components/AuthErrorModal.vue index cc581d45d..0eb80fe95 100644 --- a/frontend/src/components/AuthErrorModal.vue +++ b/frontend/src/components/AuthErrorModal.vue @@ -108,7 +108,7 @@