docs: integrating VS Code and Copilot with a node - #294
Conversation
The other integration guides put the node's API token into the config file. This one does not: VS Code prompts for it and keeps it in its own secret store, so .vscode/mcp.json holds no secret and is committed here as the shortest working description of how to attach an agent to a node. Every example is captured from a live session rather than composed from the tool descriptions, which is how the page found its own errors. Two were mine: find_remote_tools does not return an empty list when peers fail, it returns entries carrying an error field beside the successful ones; and reloading the window does not start a server, VS Code waits to be asked, which is why an empty MCP log means never-started rather than started-and-failed. The third was not in the docs. The skill told agents remote tools are namespaced <service_name>.<tool_name>. They are mcp://everything/get-sum, built prefix + service + "/" + tool, so an agent following the skill would assemble names that cannot resolve. The stale doc comment on handleFindRemoteTools describing a "." filter that no longer exists went with it. The local development section runs the harness in --network none and reports what happened: fifteen tools granted, one tool call, twenty-three peers, one allowed external flow and one denial, and the Gemma system-role failure a reader will hit if they let the mesh choose the model.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces integration and documentation for connecting sam-node to VS Code and GitHub Copilot as an MCP server, including updating the remote tool namespacing format to ://<service_name>/<tool_name>. Feedback highlights an inconsistency in how ServiceName filtering is handled between the gossip fast path and the direct catalog fetch path in internal/node/mcp_handlers.go, as well as an outdated reference to the old service.tool format in SKILL.md that should be updated.
| // - Tools without a "." in their name (infra tools) are excluded. | ||
| // - If params.ServiceName is set, only tools whose name starts with | ||
| // "<service_name>." are returned. | ||
| // - If params.ServiceName is set, only tools from that service are returned. |
There was a problem hiding this comment.
There is an inconsistency in how ServiceName filtering is handled between the gossip fast path (gossipToolRows) and the direct catalog fetch path (fetchRemoteToolCatalogue):
-
In
gossipToolRows(line 405),serviceNameFilteris compared directly againstp.Service(e.g.,"everything"):if serviceNameFilter != "" && p.Service != serviceNameFilter {
This expects the filter to be the bare service name (without the
mcp://prefix). -
In
fetchRemoteToolCatalogue(line 576),connectServiceis prefixed withmcp://(becoming"mcp://everything"), and the filter check is:if serviceNameFilter == "" || connectService == serviceNameFilter || strings.HasPrefix(connectService, serviceNameFilter+".") {
This expects the filter to either include the
mcp://prefix or match the legacy.suffix (which is now obsolete). If a bare service name like"everything"is passed, it will fail to match here.
To fix this, we should normalize serviceNameFilter (e.g., strip the mcp:// prefix or ensure both paths compare the normalized/bare service name) and update the legacy . prefix checks to use the new / separator or remove them if no longer needed.
| For example `mcp://everything/get-sum`. Pass the name exactly as returned to | ||
| `describe_remote_tool` and `call_remote_tool`; do not reassemble it. |
There was a problem hiding this comment.
The documentation still contains a reference to the old service.tool namespacing format at line 167:
- `tool_name`: the discovered namespaced tool name, such as `service.tool`
Please update line 167 to use the new scheme://service_name/tool_name format (e.g., mcp://service/tool) to maintain consistency with the updated namespacing documentation.
find_remote_tools documents service_name as a bare name ("e.g.
'code-reviewer'"), but tool names carry the mcp:// prefix and the three
sites matching against the filter each compared a different thing:
gossipToolRows the provider's raw service, one branch of
fetchRemoteToolCatalogue the prefixed name, another the bare one, two of
them still testing a "." separator from the old namespacing.
The effect on the documented spelling was silence. service_name
"everything" returned [] against a mesh with twenty-eight matching tools,
while "mcp://everything" returned all of them.
Leniency and strictness are not the axis here; which wrong inputs must be
loud is. This tool only ever searches MCP services, so the scheme is
redundant and both spellings are accepted. A different scheme is not
redundant, it is wrong, and returning [] for it would repeat the same
bug: an empty array is already the honest answer for a service the mesh
does not have, so it cannot also mean the caller misspelled one. That
case is now an error naming both accepted forms.
Canonicalising once in the handler leaves the inner functions taking a
canonical filter, which is what their callers already meant.
Reported by review on google#294.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the SAM mesh MCP tool namespacing scheme from a dot-separated format (service.tool) to a URI-based format (scheme://service/tool). It introduces a normalizeServiceFilter helper to canonicalize service filters, updates discovery and catalog fetching logic accordingly, adds comprehensive integration documentation for VS Code and GitHub Copilot, and includes new unit tests. A review comment correctly identifies an inconsistency in fetchRemoteToolCatalogue where ToolName is set to the non-canonical targetService instead of connectService during a tool listing failure.
| } | ||
| } else { | ||
| if serviceNameFilter == "" || targetService == serviceNameFilter || strings.HasPrefix(targetService, serviceNameFilter+".") { | ||
| if serviceNameFilter == "" || connectService == serviceNameFilter { |
There was a problem hiding this comment.
In the else block (when ListTools fails), ToolName is set to targetService (e.g., "everything") instead of connectService (e.g., "mcp://everything"). This is inconsistent with the connection failure path above (line 607) which correctly uses connectService.
Using targetService here will return a non-namespaced service name in the error row, which violates the namespacing scheme and can break client expectations.
Consider updating ToolName to use connectService:
rows = append(rows, remoteToolRow{
PeerID: targetPeer.String(),
ToolName: connectService,
Error: fmt.Sprintf("failed to list tools: %v", err),
})
1 participant
The other integration guides put the node's API token into the config file. This one does not: VS Code prompts for it and keeps it in its own secret store, so .vscode/mcp.json holds no secret and is committed here as the shortest working description of how to attach an agent to a node.
Every example is captured from a live session rather than composed from the tool descriptions, which is how the page found its own errors. Two were mine: find_remote_tools does not return an empty list when peers fail, it returns entries carrying an error field beside the successful ones; and reloading the window does not start a server, VS Code waits to be asked, which is why an empty MCP log means never-started rather than started-and-failed.
The third was not in the docs. The skill told agents remote tools are namespaced <service_name>.<tool_name>. They are mcp://everything/get-sum, built prefix + service + "/" + tool, so an agent following the skill would assemble names that cannot resolve. The stale doc comment on handleFindRemoteTools describing a "." filter that no longer exists went with it.
The local development section runs the harness in --network none and reports what happened: fifteen tools granted, one tool call, twenty-three peers, one allowed external flow and one denial, and the Gemma system-role failure a reader will hit if they let the mesh choose the model.