feat(desktop): present MCP catalog as connectors - #59692
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the connector presentation work. One compatibility issue needs resolution before this can be safely salvaged.
Problems
apps/desktop/src/app/skills/mcp-tab.tsx:1417dereferencesentry.setup_steps.length(with equivalent dereferences at lines 1429 and 1441), whileapps/desktop/src/types/hermes.ts:1013-1019makes the new metadata required. Currentmaindoes not return those fields fromGET /api/mcp/catalog(hermes_cli/web_server.py:11263-11293). A new Desktop paired with an older runtime therefore receivesundefinedarrays and crashes while rendering the catalog.
Suggested changes
- Normalize absent connector metadata to empty strings/arrays at the Desktop API boundary, or make the fields optional and guard every consumer.
- Add a render-level regression test using the legacy catalog response shape without these fields.
This is an automated hermes-sweeper review.
| <span>{setupSummary}</span> | ||
| </div> | ||
| )} | ||
| {entry.setup_steps.length > 0 && ( |
There was a problem hiding this comment.
Desktop can run against an older Hermes runtime, whose /api/mcp/catalog response lacks this new field. Normalize connector metadata to empty arrays at the API boundary (or guard optional fields) before dereferencing .length; the same applies to capabilities and danger_notes below.
joelbrilliant
left a comment
There was a problem hiding this comment.
Thanks for this — the connector presentation is a clean, well-scoped layer, and the server-side parsing is nicely defensive (optional ui: block, CatalogError on malformed lists, defaults that keep manifest_version: 1 manifests valid).
I checked it out, rebased it onto current main, and ran the stated plan.
Test evidence (from a worktree at the PR head):
python -m pytest tests/hermes_cli/test_mcp_catalog.py tests/hermes_cli/test_dashboard_admin_endpoints.py::TestMcpEndpoints::test_catalog_lists_entries -q→ 38 passedvitest run src/lib/mcp-catalog.test.ts→ 10 passed
One blocker before merge — undefined deref under version skew.
The seven new fields are typed non-optional (types/hermes.ts:1013-1019) and read unguarded in the renderer: entry.setup_steps.length (mcp-tab.tsx:1417), entry.capabilities.length (:1429), entry.danger_notes.length / danger_notes[0] (:1441-1442), and inside connectorSetupSummary (mcp-catalog.ts, the stepCount line). Current main's GET /api/mcp/catalog does not return these fields, and Desktop updates independently of the runtime it connects to. A newer Desktop against an older runtime therefore gets undefined and crashes rendering the catalog.
Suggested fix: normalise the fields to []/'' at the Desktop API boundary (hermes.ts), or make them optional and guard each read with ?? []. Please add a render/unit test using a catalog entry with these fields absent — the current tests only cover the fully-populated shape, so the crash path isn't exercised.
Smaller notes:
- "Connect" vs "Install" is currently a label-only ternary —
onClickcalls the sameinstall(entry)in both cases (:1471). Fine if the OAuth-chaining follow-up lands in the same release; otherwise the "Connect" label over-promises. - Renaming the tab from "MCP" to "Connectors" drops the "MCP" search term; consider "MCP Connectors" to keep it discoverable.
- Heads-up for whoever builds the connect flow: main already has
completeMcpDesktopOAuthinapps/desktop/src/lib/mcp-dashboard-oauth.ts(used atmcp-tab.tsx:583for server re-auth). The catalog install path can chain into that rather than adding new OAuth code.
Rebase: not fast-forwardable, but the only real conflict is one adjacent-import line in mcp-tab.tsx (this PR's @/lib/mcp-catalog import vs main's new @/lib/mcp-dashboard-oauth import). Keep both. Everything else auto-merges.
Merge recommendation: back this as the core presentation PR over the duplicate #59872. Fix the deref, add the legacy-shape test, rebase, and it's good to go. The 262-manifest bulk import in #59872 is better reviewed as a separate data PR so it doesn't gate this layer.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two open PRs address the connector presentation work: #59692 adds the focused Desktop/API metadata layer and three curated manifest annotations, while #59872 duplicates that core and additionally bundles SSE support, navigation changes, and a large connector-manifest import. Both diffs retain the reported version-skew crash because the Desktop requires and dereferences metadata fields that older runtimes do not return.
Related pull requests
- #59692
related— (+355/-12) — no Verify verdict recorded; keep open with a salvage path: the focused diff adds connector cards, optional manifest UI metadata, API exposure, and tests, butentry.setup_steps.length,entry.capabilities.length, andentry.danger_notes.lengthremain unsafe for legacy catalog responses. This follows the keep_open review on #59692: normalize missing metadata or guard it with defaults and add the requested render-level legacy-response regression test. - #59872
duplicate— (+6968/-23) — no Verify verdict recorded; author action: split out the distinct bulk manifest/SSE work: the connector-presentation core repeats #59692, while this diff separately adds SSE transport support, navigation changes, and the large connector import. The keep_open review on #59872 supports salvage but explicitly requires the same version-skew fix and separate review of the manifest import.
Duplicates
#59872 duplicates #59692 for the connector-presentation core, including the Desktop catalog rendering, UI metadata model and parsing, API fields, and principal helper logic; #59872's SSE support, navigation changes, and bulk manifest import are additional rather than duplicate work.
Suggested consolidation
Keep #59692 open with a salvage path: make all new catalog metadata backward-compatible at the Desktop boundary and add a render-level regression test using the legacy response shape. For #59872, author action: split the SSE support and bulk manifest import into separately reviewable work, then close #59872 as duplicate of #59692 for the shared connector-presentation core; this preserves both visible keep_open reviews without carrying the duplicated implementation twice.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
subgraph Dup59692 ["PRs duplicating each other"]
P59692["PR #59692 (open)"]
P59872["PR #59872 (open)"]
end
class P59692 open
class P59872 open
class P59692 target
click P59692 "https://github.com/NousResearch/hermes-agent/pull/59692"
click P59872 "https://github.com/NousResearch/hermes-agent/pull/59872"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 379 kB of PR diffs, 2 kB of issue/PR text, 6 kB of discussion (4 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
uimetadata to MCP catalog manifests and expose it through/api/mcp/catalogTest Plan
uv run python -m pytest tests/hermes_cli/test_mcp_catalog.py tests/hermes_cli/test_dashboard_admin_endpoints.py::TestMcpEndpoints::test_catalog_lists_entries -qnpm run test:ui -- src/lib/mcp-catalog.test.tsnpm run typechecknpx eslint src/lib/mcp-catalog.ts src/lib/mcp-catalog.test.ts src/app/skills/mcp-tab.tsx src/types/hermes.ts src/i18n/en.ts src/i18n/types.ts src/i18n/zh.tsnpm run build