GeezerVison

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: handleCallTool returns CallTool errors to coderd as Detail: 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: redactErrorURLs is the fourth error-URL redactor in the repository (others in coderd/x/chatd/mcpclient, chatdebug, chaterror), each with different rules.
• agent/x/agentmcp/plugindata.go:20: plugin name validation lives in agent/agentcontext; if the next PR's caller is there, PluginDataDir may belong next to it.

🤖 This review was automatically generated with Coder Agents.

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 as 169.254.169.254.
• agent/x/agentmcp/mcphttpclient.go:45: on a cross-origin redirect, net/http sends Referer with the full configured URL, so a ?api_key= query reaches the redirect target even though headers are withheld.
• agent/x/agentmcp/config.go:82: ParseConfig accepts header names that differ only in case, so which value is sent depends on map iteration order.
• agent/x/agentmcp/mcphttpclient.go:94: mcpHTTPClient detects tests with flag.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.

Docs LGTM

Docs LGTM 👍

Edit: I forgot I already reviewed this lol