Repository navigation
feat: accept streamable-http MCP type, add plugin data dir helper - #29324
jscottmiller wants to merge 1 commit into
Conversation
73f758b to
6ae7d81
Compare
6ae7d81 to
cdc3d6b
Compare
cdc3d6b to
2961c77
Compare
08b2b01 to
ce83c77
Compare
ce83c77 to
747265b
Compare
747265b to
0b3c2c8
Compare
0b3c2c8 to
58e12e8
Compare
d48e7f8 to
2b25f78
Compare
2b25f78 to
7f4fba0
Compare
7f4fba0 to
8102b7c
Compare
8102b7c to
0812a94
Compare
0812a94 to
46d7a7f
Compare
46d7a7f to
b767701
Compare
b767701 to
b330dc2
Compare
b330dc2 to
6a761a4
Compare
6a761a4 to
2eee941
Compare
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
/coder-agents-review |
2 similar comments
|
/coder-agents-review |
|
/coder-agents-review |
There was a problem hiding this comment.
Re-review of ef2b518, which is now rebased onto the earlier Agent Plugins PRs. All 20 round 2 findings are fixed. Round 3 findings: 2 P2, 4 P3, 7 P4, 2 Nit.
The CI failures on this head come from a cancelled run. go build, go vet, gofmt, the package tests, and markdownlint pass locally.
Each round has found a URL form the redactor misses: round 1 raw errors, round 2 %v-flattened and user:***@ URLs, and round 3 quote characters, scheme-relative and flattened relative Location values, and parse-error fragments. Building the catalog text from parts the agent controls, such as the operation, the configured origin, and an error class, with the full error kept in the agent log, would remove this whole class instead of adding another pattern each round.
The author's reply on the retained-client finding (manager.go:809) says a follow-up ticket is coming; no ticket link has been posted yet.
Out of scope (needs a ticket or explicit acceptance by a human):
agent/x/agentmcp/api.go:54:handleCallToolreturnsCallToolerrors to coderd asDetail: err.Error()without redaction, so an HTTP error during a tool call sends the configured URL path and query to coderd.
Notes:
agent/x/agentmcp/mcphttpclient.go:144:redactErrorURLsis the fourth error-URL redactor in the repository (others incoderd/x/chatd/mcpclient,chatdebug,chaterror), each with different rules.agent/x/agentmcp/plugindata.go:20: plugin name validation lives inagent/agentcontext; if the next PR's caller is there,PluginDataDirmay belong next to it.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Re-review of 4e056e9. The PR is narrower now: configured headers are withheld from off-origin requests, redirects are followed as before, and connect errors no longer go into the catalog. All 15 round 3 findings are closed, by a fix or because the code was removed. Round 4 findings: 1 P2, 1 P3, 8 P4, 4 Nit, plus one reopened naming finding.
Withholding headers matches Agent Plugins spec 7.2.1, and removing the redactor ends the URL-leak findings from rounds 1 to 3. The removed work (refusing off-origin redirects, reporting connect errors in the catalog, and showing the hidden error after a failed reconnect) is described as tracked in a sub-issue of CODAGT-1101, but no link has been posted. This needs a human decision: link the ticket, or accept dropping that work.
Out of scope (needs a ticket or explicit acceptance by a human):
agent/x/agentmcp/mcphttpclient.go:57: the HTTP and SSE clients follow up to 10 redirects from a remote MCP server to any origin, including loopback and link-local addresses such as169.254.169.254.agent/x/agentmcp/mcphttpclient.go:45: on a cross-origin redirect, net/http sendsRefererwith the full configured URL, so a?api_key=query reaches the redirect target even though headers are withheld.agent/x/agentmcp/config.go:82:ParseConfigaccepts header names that differ only in case, so which value is sent depends on map iteration order.agent/x/agentmcp/mcphttpclient.go:94:mcpHTTPClientdetects tests withflag.Lookup("test.v");testing.Testing()answers that directly.
Notes:
- The commit subject and PR title are 81 characters, over the 72-character limit.
agent/x/agentmcp/manager.go:805
P2 [CRF-84] The stricter policy removed in this round (refusing off-origin redirects, catalog connect errors) and the hidden error state after a failed reconnect are described as "tracked separately" and "a follow-up sub-issue of CODAGT-1101", but no reply or description links that sub-issue. (Mafu-san, Leorio)
Without a link, a reviewer cannot confirm the work exists, so it is dropped rather than deferred. Link the sub-issue in the PR description and in the replies, or state that the work is dropped.
🤖
🤖 This review was automatically generated with Coder Agents.
The streamable-http transport name from the Agent Plugins mcp.json schema is normalized to http when a config is parsed, and the extending-agents docs list the accepted type names. PluginDataDir derives the per-plugin data directory under the user's home from a validated plugin name and the plugin's cleaned install directory, for the plugin mcp.json loader added next.
tl;dr: Accept the Agent Plugins
streamable-httpMCP type name and add thePluginDataDirhelper that the pluginmcp.jsonloader uses.inferTransportmapstype: "streamable-http", the Agent Plugins spelling, to the existinghttptransport, so.mcp.jsonfiles can use either name. The extending-agents docs list the accepted type names.PluginDataDir(home, name, installDir)returns<home>/.coder/plugin-data/<name>-<hash>, where the hash covers the cleaned install directory so two installs of the same plugin get separate directories. It rejects names that are not valid plugin names and does not create the directory.Header handling for
.mcp.jsonservers is unchanged. The same-origin header rule from the Agent Plugins specification applies only to plugin servers and lands with the plugin loader in the next PR. A stricter origin policy is tracked in CODAGT-1383.PR map
scott/agent-plugins/4-chatd..2289fe7a33agent/x/agentmcp/plugindata.go(+25/-0, new):PluginDataDirvalidates the plugin name and returns<home>/.coder/plugin-data/<name>-<hash>, with the hash taken over the cleaned install directory. It does not create the directory.agent/x/agentmcp/config.go(+4/-0):inferTransportmapsstreamable-httptohttp.This PR was generated by Coder Agents on behalf of @jscottmiller.