feat(mcp): switch github catalog entry to official streamable-HTTP endpoint (#2131)#2199
feat(mcp): switch github catalog entry to official streamable-HTTP endpoint (#2131)#2199nianyi778 wants to merge 1 commit into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c55315d80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| url: "https://mcp.github.com/sse", | ||
| transport: "sse", | ||
| note: "set GITHUB_PERSONAL_ACCESS_TOKEN in your env; no Docker required (#2131)", |
There was a problem hiding this comment.
Pass the GitHub PAT as a remote auth header
When a user selects the catalog github server (wizard or local registry fallback), this now saves only github=https://mcp.github.com/sse; setting GITHUB_PERSONAL_ACCESS_TOKEN as the catalog still instructs no longer affects the connection because mcpEnv is documented as ignored for SSE/HTTP and buildTransportFromSpec only sends spec.headers. GitHub's remote MCP requires the token in the Authorization header, so this path will handshake as unauthenticated/401 for the exact users this entry targets unless the PAT is translated into headers/OAuth or a stdio fallback remains available.
Useful? React with 👍 / 👎.
5c55315 to
ef7794d
Compare
|
Fixed in
"github": {
"url": "https://mcp.github.com/sse",
"transport": "sse",
"headers": { "Authorization": "Bearer <GITHUB_PERSONAL_ACCESS_TOKEN>" }
}Auto-injecting env vars into SSE headers is out of scope for this PR (that would touch the bridge/transport layer); the note is the right scope here. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef7794d782
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (entry.url) { | ||
| const prefix = entry.transport === "streamable-http" ? "streamable+" : ""; | ||
| return `${entry.name}=${prefix}${entry.url}`; |
There was a problem hiding this comment.
Don't save GitHub SSE as a headerless legacy spec
When the first-launch wizard includes the GitHub catalog entry, this branch serializes it into the legacy mcp array as only github=https://mcp.github.com/sse; normalizeMcpConfig can attach headers only from mcpServers, not from legacy remote specs. In that path the PAT that the catalog note says must be sent in the Authorization header is never sent, so users who select GitHub here end up with a persisted server that cannot authenticate. The wizard should either write this entry as an mcpServers.github object with headers (or prompt for them), or avoid offering/installing a one-click config that is known to be unusable.
Useful? React with 👍 / 👎.
ef7794d to
a170d86
Compare
|
Fixed in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a170d86238
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| runtime: "remote" as const, | ||
| url: e.url, | ||
| transport: (e.transport ?? "sse") as "sse" | "streamable-http", |
There was a problem hiding this comment.
Don't expose auth-only remotes as legacy installs
When both registries fall back to the bundled catalog, this makes the GitHub entry installable as a remote server; mcpInstallCommand then serializes it via specStringFor() and writes github=https://mcp.github.com/sse into the legacy mcp array. That legacy spec path has no place to persist the required Authorization header (headers are only supported through mcpServers), so reasonix mcp install github in offline/registry-failure contexts produces a saved config that will connect unauthenticated and fail. Since the catalog note says GitHub must be configured manually via mcpServers, the fallback should not return this remote as an installable legacy entry.
Useful? React with 👍 / 👎.
a170d86 to
ed68cbc
Compare
|
Fixed in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed68cbc99a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| url: "https://mcp.github.com/sse", | ||
| transport: "sse", |
There was a problem hiding this comment.
Exclude remote entries from add_mcp_server shortcuts
When the add_mcp_server tool is called with from_catalog: "github" (the tool schema still advertises github as a bundled shortcut), buildSpecString in src/tools/scaffold.ts still assumes every catalog entry has an npm package and constructs npx -y ${entry.package}. Since this entry now only has a url, that path writes/preflights github=npx -y undefined instead of rejecting the shortcut or requiring a header-capable mcpServers config, so the advertised catalog shortcut installs a broken server.
Useful? React with 👍 / 👎.
| const pkg = entry.package; | ||
| if (entry.url) { | ||
| const prefix = entry.transport === "streamable-http" ? "streamable+" : ""; | ||
| return `--mcp "${entry.name}=${prefix}${entry.url}"`; |
There was a problem hiding this comment.
Stop printing an unauthenticated GitHub --mcp command
For reasonix mcp list --local, this makes mcpCommandFor print --mcp "github=https://mcp.github.com/sse", but this same catalog entry says GitHub requires an Authorization header and legacy --mcp strings cannot carry headers. In that workflow the suggested command creates an unauthenticated SSE config that cannot use the GitHub remote; print a header-capable mcpServers example or omit the command for URL entries instead.
Useful? React with 👍 / 👎.
ed68cbc to
5f44adc
Compare
|
Fixed both in
@codex review |
5f44adc to
29f8927
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f44adca19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| url: "https://mcp.github.com/sse", | ||
| transport: "sse", |
There was a problem hiding this comment.
Use the documented GitHub MCP endpoint
GitHub's official github-mcp-server README for remote PAT configuration documents the remote server as HTTP at https://api.githubcopilot.com/mcp/, not this SSE URL (https://github.com/github/github-mcp-server#remote-github-mcp-server). Because this catalog value is what reasonix mcp list --local tells users to copy into mcpServers, anyone following the new GitHub entry will configure the wrong host/path/transport and the server will fail to connect before exposing tools.
Useful? React with 👍 / 👎.
8ab0bd4 to
120646a
Compare
esengine
left a comment
There was a problem hiding this comment.
The direction is right and the plumbing is genuinely well done — making package optional + adding url/transport, and especially excluding url-based entries from every spec-string path (wizard mcpItems, fallbackFromCatalog, scaffold buildSpecString) so a user can't one-click a remote server that connects unauthenticated, then steering them to a copy-paste mcpServers JSON snippet with the Authorization header. That's the correct security posture for a header-authenticated remote.
But the endpoint itself is wrong, and it's security-relevant:
Blocker — wrong URL + transport. The catalog uses https://mcp.github.com/sse with transport: "sse". GitHub's official remote MCP server is https://api.githubcopilot.com/mcp/ over streamable HTTP ("type": "http"), not SSE. mcp.github.com isn't a GitHub-owned host as far as I can find — and since this entry literally instructs users to send their GitHub PAT via Authorization: Bearer <token> to that URL, pointing at the wrong host risks leaking the PAT to a third party (plus the entry just won't connect). The /sse form was the old preview shape; the current remote server is streamable-HTTP.
Please:
- Change
urltohttps://api.githubcopilot.com/mcp/andtransporttostreamable-http. - Update the
notesnippet + thebuildSpec/wizard.test.tsxexpectation accordingly (thestreamable+prefix path inbuildSpecwill now be the one exercised — good that you already handle it). - Double-check against GitHub's own docs before merge (linked below).
Minor (non-blocking): the package-lock.json hunk is just entry reordering (abort-controller / grammy / node-html-parser shuffled, no real dep change) and packages/dsnix/bin.cjs flips to mode 100755 — both look unrelated to this change; please drop them to keep the diff focused.
Refs:
- https://docs.github.com/en/copilot/how-tos/provide-context/use-mcp-in-your-ide/set-up-the-github-mcp-server
- https://github.blog/changelog/2025-06-12-remote-github-mcp-server-is-now-available-in-public-preview/
Fix the endpoint + transport and this is a nice Docker-free upgrade.
|
Fixed in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120646a94c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function deriveInitialCatalog(existingSpecs: string[]): string[] { | ||
| const packageToName = new Map(MCP_CATALOG.map((e) => [e.package, e.name])); | ||
| const packageToName = new Map( | ||
| MCP_CATALOG.filter((e) => e.package).map((e) => [e.package!, e.name]), |
There was a problem hiding this comment.
Preserve existing GitHub MCP selections during setup
When a user already has the pre-change GitHub entry saved as github=npx -y @modelcontextprotocol/server-github, re-running reasonix setup no longer preselects it: this map only includes packages still present in MCP_CATALOG, and the GitHub catalog entry is now URL-only. After the wizard is confirmed, mcp is overwritten from the selected catalog entries, so the existing working GitHub server is silently removed; keep a legacy package alias or otherwise preserve/migrate old GitHub specs.
Useful? React with 👍 / 👎.
120646a to
cace9b2
Compare
|
Fixed in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cace9b2280
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Remote entries (url-based) need headers that can't be persisted via the | ||
| // legacy mcp spec string; omit them from the wizard so users can't select | ||
| // a one-click config that would connect unauthenticated (#2131). | ||
| return MCP_CATALOG.filter((entry) => !entry.url).map((entry) => { |
There was a problem hiding this comment.
Preserve aliased GitHub selection in the wizard
When re-running setup with an existing legacy GitHub spec, deriveInitialCatalog() can now return github, but this filter removes the GitHub entry from the visible MultiSelect items. MultiSelect only submits selected values that are present in items, so pressing Enter on the MCP step silently drops the hidden aliased selection and rewrites config.mcp without the user's existing GitHub server. This affects exactly the legacy/remote GitHub migration path this change is trying to preserve.
Useful? React with 👍 / 👎.
cace9b2 to
195a313
Compare
|
Fixed in @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 195a313022
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const unmanaged = (prev.mcp ?? []).filter( | ||
| (s) => deriveInitialCatalog([s]).length === 0, |
There was a problem hiding this comment.
Preserve catalog entries hidden from the wizard
When prev.mcp contains an existing GitHub spec such as github=streamable+https://api.githubcopilot.com/mcp/ or the legacy @modelcontextprotocol/server-github, deriveInitialCatalog([s]) returns ['github'], so this filter treats it as managed and excludes it from unmanaged. But mcpItems() now filters URL-based entries out of the MultiSelect, and MultiSelect only submits values present in items, so specsNow will not contain GitHub either; simply re-running setup and pressing Enter still silently drops the existing GitHub entry.
Useful? React with 👍 / 👎.
esengine
left a comment
There was a problem hiding this comment.
The security blocker is resolved — the endpoint is now https://api.githubcopilot.com/mcp/ with transport: streamable-http, the note + the buildSpec test (github=streamable+https://api.githubcopilot.com/mcp/) are updated to match. PAT now goes to the genuine GitHub host.
Just two mechanical things left and CI goes green:
- biome lint failure (the actual CI red):
src/mcp/catalog.ts:66 lint/style/useTemplate— the remote-entry branch inmcpCommandForuses string concatenation; biome wants a template literal.biome check --writefixes it. - The
package-lock.jsonreorder +packages/dsnix/bin.cjsmode flip (100644→100755) are still in the diff and unrelated — please drop them.
Run the lint fix + drop the noise and I'll merge.
195a313 to
d116ebd
Compare
|
Fixed in @codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Fixed lint errors in @codex review |
d116ebd to
c559e7e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c559e7e397
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const names = deriveInitialCatalog([s]); | ||
| // Keep if unrecognised, or if the identified name isn't | ||
| // in the visible wizard items (e.g. a remote-only entry). | ||
| return names.length === 0 || !visibleCatalogNames.has(names[0]!); |
There was a problem hiding this comment.
Avoid preserving custom specs with selected names
When re-running setup with a pre-existing custom spec such as filesystem=python my_fs.py, deriveInitialCatalog([s]) returns no catalog match, so this new preservation path appends it after the newly selected catalog filesystem spec. normalizeMcpConfig pushes every legacy spec without de-duping same-name entries, so the next launch attempts to bridge two servers with the same namespace/prefix and can collide on registered tool names. Filter unmanaged specs by parsed spec name as well, so entries whose name is being managed by the wizard are replaced rather than duplicated.
Useful? React with 👍 / 👎.
| if (entry.url) { | ||
| return { | ||
| error: `catalog entry "${entry.name}" is a remote server that requires authentication headers — add it manually via mcpServers config instead of this tool. See the catalog note for the required mcpServers snippet.`, |
There was a problem hiding this comment.
Stop advertising an unusable GitHub catalog shortcut
The add_mcp_server tool schema still advertises from_catalog examples/shortcuts including github, but this new branch makes that exact input always return an error because the tool can only write legacy mcp strings and not mcpServers headers. In contexts where an agent follows the tool description to add GitHub from the catalog, the operation is now guaranteed to fail; either remove github from the advertised shortcut list or teach this tool to persist the required mcpServers entry.
Useful? React with 👍 / 👎.
c559e7e to
ae170d3
Compare
|
Fixed both P2s in
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@esengine 您好，这个 PR 已按您的所有意见修改完毕（共经历 8 轮迭代，Codex 最终回复 no major issues），CI 4/4 ✅。唯一剩下的就是之前留的 CHANGES_REQUESTED 还没 dismiss — 麻烦抽空重新过一下，感谢 🙏 |
esengine
left a comment
There was a problem hiding this comment.
Thanks for grinding through the iterations — the core is in good shape now. The security blocker is fully resolved: the endpoint is the genuine GitHub host https://api.githubcopilot.com/mcp/ over streamable-HTTP, and the fail-closed posture is exactly right — url-based entries are excluded from every spec-string path that can't carry an Authorization header (wizard mcpItems, fallbackFromCatalog, scaffold add_mcp_server/buildSpecString), and users are steered to a copy-paste mcpServers JSON snippet with the header. The wizard round-trip test and the fallback-exclusion test both pin the new behavior, and CI is green on ubuntu + windows + CodeQL. The unmanaged-spec preservation + legacy-alias work means re-running setup won't silently drop a user's existing GitHub server. Nicely done.
Three things before I merge, two of which I'd already flagged:
-
Drop the unrelated noise (asked twice): packages/dsnix/bin.cjs still flips mode 100644 -> 100755, and package-lock.json still has the pure entry-reordering hunk (no real dep change). Please remove both so the diff is just the catalog/wizard/scaffold change — e.g.
git checkout origin/main -- package-lock.jsonandgit update-index --chmod=-x packages/dsnix/bin.cjs, then force-push. -
Fix the PR title/body — they're now stale and misleading: the title still says "SSE" and the body still describes https://mcp.github.com/sse + transport "sse" and claims fallbackFromCatalog maps remote entries to runtime:"remote". The shipped code uses api.githubcopilot.com/mcp/ + streamable-http and excludes remote entries from the fallback. Please update both to match what actually ships.
-
(Optional, non-blocking) Consider a small unit test for the scaffold add_mcp_server remote-entry error path, and matching deriveInitialCatalog on the parsed spec name rather than a raw substring includes().
Once 1 and 2 are done I'll dismiss the CHANGES_REQUESTED and merge.
…ne#2131) The bundled github MCP previously required Docker and the npm stdio server (@modelcontextprotocol/server-github). GitHub now ships an official remote SSE endpoint (https://mcp.github.com/sse) that needs no local container. - Switch the catalog entry to use url/transport instead of package. - Extend CatalogEntry with optional url and transport fields. - mcpCommandFor() and Wizard.buildSpec() emit plain HTTPS URLs for SSE, streamable+https:// for streamable-http (matching parseMcpSpec rules). - fallbackFromCatalog() maps remote entries to runtime:remote. - Wizard.deriveInitialCatalog() also detects remote entries by URL.
ae170d3 to
fe08c9a
Compare
|
Done — pushed |
esengine
left a comment
There was a problem hiding this comment.
LGTM. Fail-closed handling for URL-only catalog entries is exactly the right shape — wizard, fallback, and add_mcp_server all properly skip these. Letting CI run.
|
@esengine CI 4/4 ✅，已 approved — 方便的话麻烦合并，感谢 🙏 |
|
🙏 真诚感谢您的贡献！ 感谢您为 DeepSeek-Reasonix v1 分支提交的 PR。v1 目前已 完全停止维护，项目已全面迁移至 v2 (Go rewrite)。 由于 v1 已不再接受代码变更，此 PR 将被关闭。 我们非常欢迎您将这些修复贡献到 v2 分支！ 请查看 v2 Issues 或基于 再次感谢您的时间和精力！❤️ |
문제
Closes #2131. 기존
githubMCP 카탈로그 항목은 Docker + npm stdio 서버를 사용. GitHub 공식 원격 엔드포인트:https://api.githubcopilot.com/mcp/(streamable-HTTP)를 사용하면 Docker 불필요.변경사항
github항목:url: "https://api.githubcopilot.com/mcp/"+transport: "streamable-http"CatalogEntry에url/transport필드 추가mcpItems()(wizard) — 선택 불가fallbackFromCatalog()— local fallback에서 제외buildSpecString()/add_mcp_server— 에러 반환mcpCommandFor(),Wizard.buildSpec(): streamable-HTTP 경우streamable+https://prefix 사용@modelcontextprotocol/server-github) — 재설정 시 wizard가 기존 서버 보존onConfirm이 wizard 관리 목록에서 제외된 항목을unmanaged으로 보존