Skip to content

Follow-ups from Spec 109-j catalog review (#1383) #1398

Description

@Dumbris

Follow-up checklist from the verified review ledger for #1383 (Spec 109-j). These remaining findings were deferred during that PR's review; this issue does not replace the post-merge live acceptance run.

  • F-G (cmd/mcpproxy/upstream_add_secret.go:61-66): applySecretFlags' documented 'never leaves the first flag's secret orphaned' rollback guarantee silently no-ops when the keyring backend is still marked unavailable at rollback time (e.g. the same wedged backend that just failed a later Store call) -- resolver.Delete refuses when provider.IsAvailable() is false, and the error is discarded (_ = resolver.Delete(...)).
  • F-H (cmd/mcpproxy/upstream_add_secret.go:68-95): --secret-env/--secret-header accept an empty field name (no name=="" validation, unlike the sibling parseRegistryEnv), silently storing a secret under the generic ref <server>-env/<server>-header and persisting a bogus empty-keyed env/header entry in the new server's config.
  • F-I (frontend/src/components/CatalogSearch.vue:290,319; native/macos/MCPProxy/MCPProxy/Views/CatalogView.swift:127-128,164-165): 'Added ✓ · Open' is a no-op (macOS) or silently re-emits 'add' causing a failed duplicate-add attempt (Web) for a catalog result that was already added in an EARLIER session (backend added:true, but no local addedNames[key] from this page visit) -- because the catalog DTO deliberately never carries the configured server's name (FR-007 caller-visibility rule), so neither surface can navigate to it. The two surfaces also diverge in what happens on click, which is itself an FR-063 consistency gap.
  • F-J (native/macos/MCPProxy/MCPProxy/API/CatalogModels.swift:42-44 (used at CatalogView.swift:102,118)): CatalogResult conforms to Identifiable via the bare catalog id, which SearchAll only de-dupes by (source, id) -- not globally unique -- so SwiftUI's ForEach can receive duplicate ids when two enabled sources list the same server, triggering undefined list-identity behavior (dropped/misrendered cards).
  • F-K (internal/registries/catalog.go:152; internal/registries/search.go:538-562): The new GET /catalog/search tag parameter (FR-060) is forwarded into the pre-existing filterServers, which accepts a tag argument but never actually filters on it -- the parameter is a silent no-op on every catalog source.
  • F-L (internal/configimport/detector.go:58): The Paste URL-detection branch accepts a line with trailing junk after a valid http(s):// URL (url.Parse tolerates spaces in the path/host-adjacent text), so a URL with accidental trailing text is detected as FormatURL and stored verbatim as an unusable server URL rather than being rejected or treated as a command.
  • F-M (internal/shellwords/shellwords.go:17-49): Split's doc comment promises POSIX-shell tokenization but performs no backslash-escape processing at all (backslash is handled as a plain default-case rune, no escape state unlike the quote state), so a pasted command using shell-style escaped spaces splits into different argv than the shell it was copied from.
  • F-N (frontend/src/components/CatalogSearch.vue:202-204; frontend/src/components/PasteServer.vue:127-128; frontend/src/components/SecretToggle.vue:69-79): Parents default a secret-like field to 'secret' mode without consulting current keyring availability, and SecretToggle's correcting watch only fires on a CHANGE of keyringAvailable (not immediate), so when the availability probe has already resolved to false before the field is created, the field sits in secret mode and Add fails with 'keyring unavailable' until the user manually flips it -- contradicting SecretToggle's own stated invariant that a field 'can never enter secret mode' when the keyring is unavailable.
  • F-O (internal/httpapi/catalog.go:136-151): getVisibleServersForCatalog silently swallows a ListServers error from the management service and proceeds with an empty server list (no else branch on the error), so a transient storage/runtime failure makes every catalog entry read added:false with no error signal to the caller, unlike handleGetServers which 500s on the same error.
  • F-P (native/macos/MCPProxy/MCPProxyTests/CatalogTests.swift:113-128): FR-065's stated three-way pin ('Go, vitest AND Swift all decode the shared fixture internal/secret/testdata/ref_names.json') is not actually implemented for Swift: CatalogTests hand-copies an inline snapshot of the fixture instead of loading the real file, so a future case added to ref_names.json (e.g. a Unicode case where Swift's full Unicode case-folding diverges from Go's simple ToLower) would silently never run on Swift and a real algorithm divergence could ship undetected. Today's hand-copy matches all 6 current fixture cases, so nothing is currently broken -- only the promised pin doesn't exist.
  • QA-config-flag: tracked separately in upstream add/remove/etc. ignore -c/--config and load the default config #1396; candidate fix is fix(catalog,cli): MCP secret_like parity + upstream add --config flag #1397. Close this checklist item only after final-head review and live QA.
    Re-evaluate severity when reproducing each item. Normal-flow failures found during post-merge QA must be fixed before declaring catalog acceptance complete.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    kind/bugSomething isn't workingpriority/lowNice to have; address when bandwidth allowstriage/acceptedTriaged and accepted for the backlog

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions