fix(brains): declare a literal MCP URL so the connector can be added on claude.ai [BRNS-MCPWEB-017] - #21
Conversation
jordanssv
left a comment
There was a problem hiding this comment.
Reviewed the diff and verified the claims against main. No blockers — the fix is correct, minimal, and well-guarded. Two substantive notes where I think the framing understates one thing and overstates another, one suggestion, one docs question.
🟢 The guards are the best part
tests/plugin-contract/run.ts doesn't just pin the new URL, it pins the class of bug:
startsWith("https://") && !includes("${")— the literal-not-template invariant, i.e. the actual defect.claudeManifest.mcpServers.brains.url === codexMcp.mcpServers.brains.url— a cross-host invariant, so the two manifests can't drift apart silently.- The endpoint
descriptionpinned to approved copy and a negative guard that it must not claim/mcpderives from the endpoint. Documentation drift caught by a test is unusual and exactly right here, since the whole failure mode is "the manifest says something the host can't honour."
🟡 The split-backend heads-up isn't a new cost — Codex has had it all along
plugins/brains/.mcp.json on main already reads:
"url": "https://mcp.mybrains.ai/mcp"So endpoint has never steered the tools for Codex. The heads-up reads as though this PR introduces the divergence, when in fact it removes an inconsistency: Claude Code is being brought into line with a pinning Codex already had. Your own new assertion (Claude and Codex must declare the same brains MCP URL) is the clearest statement that consistency is the goal.
Worth saying so explicitly, for two reasons: a reader currently concludes this change created the hazard, and anyone auditing "who is exposed" would wrongly skip existing Codex installs.
🟡 …and the blast radius is smaller than the heads-up implies
userConfig.endpoint already has default: "https://mcp.mybrains.ai". So for any install that never overrode it, ${user_config.endpoint}/mcp resolved to byte-identical to the new literal — this change is a no-op for them.
That bounds the risk precisely: only installs that explicitly set endpoint to a non-production server are affected. I'd put that sentence in the heads-up; it turns a broad warning into an actionable one, and it's a strong safety argument for merging.
🔵 Suggestion (non-blocking): make the split visible, not just documented
Right now the only thing standing between a stage/self-hosted user and "tools quietly read production memory" is the manifest copy. hooks/brains-turn.sh:44 already computes the capture base:
BASE="${CLAUDE_PLUGIN_OPTION_ENDPOINT:-${BRAINS_ENDPOINT:-https://mcp.mybrains.ai}}"A one-line stderr warning when BASE's origin differs from the pinned MCP host would convert the silent-and-worse case into a visible one, at the moment it actually matters. Cheap, and it fails toward noticing.
🔵 Docs question: this fixes a route the docs don't describe
The README is titled "brains — Codex and Claude Code plugin" and documents exactly those two hosts. claude.ai appears nowhere, and this PR doesn't add it. So the change repairs a claude.ai install path that no brains documentation sends anyone down — users only reach it by finding the connector row in claude.ai's own UI.
Meanwhile the product's own guide (apps/web/src/app/install/claude-web) teaches the manual "Add custom connector" paste, which works today and is unaffected by this change.
That leaves two supported routes on claude.ai with no guidance on which to prefer. Not this PR's job to resolve, but after 2.7.0 there's a real question — does the claude.ai guide stay manual-URL-only, or gain the plugin route? Happy to file that separately if it's useful; it overlaps BRNS-WEB-159.
On the unverifiable part
Agreed that claude.ai's connector-add, OAuth and first tool call can only be checked post-merge, and the justification holds — the connector is unaddable today, so there's nothing on that surface to regress. Flagging only that this is the one claim the test suite structurally cannot cover, so it's worth an explicit post-merge check rather than assuming auto-sync closed it.
|
Both 🟡s taken — the heads-up now bounds the blast radius (only explicit On the stderr warning: agreed the succeeds-silently case is the ugly one, but not in this PR. The task's fixed constraint was hook behaviour unchanged (now guard-tested — the suite asserts capture/inbox targets), and post-2.7.0 a custom Docs question: agreed and out of scope here — please file it. The post-merge probes on this ticket (feedback-skill and recall routing on the plugin config) should inform which route the claude.ai guide ends up teaching. Post-merge: explicit, yes — the board ticket stays open until the full chain is observed on 2.7.0 (connector add → OAuth → real tool call, issued scopes captured), plus those two probes. Auto-sync is treated as delivery, not verification. |
stefan-ssv-labs
left a comment
There was a problem hiding this comment.
✅ review-pr: clean — no blockers or critical issues found.
…on claude.ai
The plugin's inline MCP declaration used ${user_config.endpoint}/mcp. On
claude.ai the plugin's connector row opens a prefilled "Add custom connector"
dialog, and that surface resolves no user config — the template reached the
URL field verbatim, failed its https validation, and the field rejected
edits. The connector could not be added at all: skills installed, tools
absent.
The URL is now the literal https://mcp.mybrains.ai/mcp. Claude Code resolves
the same address it always did. Codex is untouched — it reads .mcp.json,
which is unchanged. Both manifests move to 2.7.0.
Self-hosted/stage installs lose the ability to redirect the MCP tools via
userConfig.endpoint — that setting now governs conversation capture and the
inbox only. The README documents the split and the recipe to keep one
backend: --config endpoint=... on Claude Code install, BRAINS_ENDPOINT in
Codex's launch environment. A declared manifest default does not cover this
— Claude Code exports CLAUDE_PLUGIN_OPTION_ENDPOINT from stored settings
only, and a never-set option stores nothing (verified against a real
`claude -p` run with a sentinel default that never reached the hook
environment).
tests/plugin-contract/run.ts pins the literal-not-template invariant, the
cross-client URL equality, the endpoint description copy, and all three
branches of the hooks' endpoint-resolution chain (option / env var /
fallback) — eleven negative tests confirm each guard fires on its own
regression.
Board: BRNS-MCPWEB-017
7363dd4 to
047059f
Compare
Summary
${user_config.endpoint}/mcp. On claude.ai the plugin's connector row opens a prefilled "Add custom connector" dialog, and that surface resolves no user config — the template reached the URL field verbatim, failed itshttpsvalidation, and the field rejected edits. The connector could not be added at all: skills installed, tools absent.https://mcp.mybrains.ai/mcp— the only form every surface can use, since claude.ai resolves no user config at all. Claude Code resolves the same address it always did. Codex is untouched — it reads.mcp.json, which is unchanged.Heads-up for anyone pointing the tools at another server (stage, self-hosted)
endpointno longer selects the server the brains tools talk to; the tools are pinned tohttps://mcp.mybrains.ai/mcp. This affects only installs that explicitly setendpointto a non-production server — it defaults tohttps://mcp.mybrains.ai, so for every install that never overrode it the resolved URL is byte-identical before and after. Codex was never steerable either way: its declaration (.mcp.json) has carried the literal URL all along, so this change brings Claude Code in line with the pinning Codex already had — which is what the new cross-client URL assertion states.For an affected install, conversation capture and the inbox still follow
endpoint, so it now has a split backend: tools on production, capture and inbox on your server. That either fails to authenticate, or — worse — succeeds and quietly reads production memory while your captured conversations land somewhere else. Theendpointdescription in the manifest says so now.To keep one backend, fork this repo, set the URL in both manifests, and point the hooks at the same server:
--config endpoint=https://your-serverat install on Claude Code,BRAINS_ENDPOINT=https://your-serverin Codex's launch environment. Changingendpoint'sdefaultin the fork is not enough — the hooks read stored settings, not the manifest default.Test
tests/plugin-contract/run.ts,tests/inbox-v2/run.ts,tests/tool-error/run.tsgreen, including a realclaude plugin validate --strictand a real Codex resolved-config check; eleven negative tests re-fire each guard.CLAUDE_CONFIG_DIR) resolves the literal URL while capture still follows a customendpoint; the installed version reads 2.7.0.Board: BRNS-MCPWEB-017