refactor(sncloud): use session-bound organization and context#94
refactor(sncloud): use session-bound organization and context#94
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors StreamNative Cloud (SNCloud) MCP tools/prompts to rely more on session-bound organization/context, and introduces reusable tool/prompt constructors with accompanying tests and doc updates.
Changes:
- Replaced inline SNCloud tool/prompt definitions with reusable constructor functions and exported handlers.
- Updated SNCloud resource tools to resolve organization preferring the active SNCloud session (with fallbacks).
- Added docs updates and new unit tests covering constructors and organization resolution behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/mcp/streamnative_resources_tools.go | Refactors SNCloud resource apply/delete tools and resolves organization from session/context/options. |
| pkg/mcp/streamnative_resources_log_tools.go | Refactors SNCloud logs tool into reusable constructor + exported handler. |
| pkg/mcp/streamnative_cloud_primitives_test.go | Adds tests for new constructors and session-preferred organization resolution. |
| pkg/mcp/sncontext_tools.go | Updates available-clusters tool to use the renamed SNCloud cluster listing prompt handler. |
| pkg/mcp/prompts.go | Refactors SNCloud prompts into reusable constructors and renames prompt handlers. |
| docs/tools/streamnative_cloud.md | Updates tool docs language to reflect session-bound context. |
Comments suppressed due to low confidence (1)
pkg/mcp/streamnative_resources_log_tools.go:125
HandleSNCloudLogsreturns a non-nil Go error when the SNCloud session is missing. For MCP tool handlers in this repo, missing sessions are typically surfaced asmcp.NewToolResultError(...)with a nil error so clients get a structured tool error instead of an internal handler failure.
// Get log client from session
session := context2.GetSNCloudSession(ctx)
if session == nil {
return nil, fmt.Errorf("failed to get StreamNative Cloud session")
}
| promptResponse, err := HandleListPulsarClusters(ctx, mcp.GetPromptRequest{}) | ||
| promptResponse, err := HandleListSNCloudClusters(ctx, mcp.GetPromptRequest{}) | ||
| if err != nil || promptResponse == nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("Failed to list pulsar clusters: %v", err)), nil |
There was a problem hiding this comment.
In handleAvailableContexts, the error message still says "pulsar clusters" even though this tool now calls HandleListSNCloudClusters and the surrounding docs/tool naming is StreamNative Cloud clusters. Please update the error message to match the SNCloud terminology to avoid confusing users and log triage.
| return mcp.NewToolResultError(fmt.Sprintf("Failed to list pulsar clusters: %v", err)), nil | |
| return mcp.NewToolResultError(fmt.Sprintf("Failed to list StreamNative Cloud clusters: %v", err)), nil |
| // NewSNCloudLogsTool creates the reusable StreamNative Cloud logs tool definition. | ||
| func NewSNCloudLogsTool() mcp.Tool { | ||
| return mcp.NewTool("sncloud_logs", | ||
| mcp.WithDescription("Display the logs of resources in StreamNative Cloud, including pulsar functions, pulsar source connectors, pulsar sink connectors, and kafka connect connectors logs running along with PulsarInstance and PulsarCluster."+ |
There was a problem hiding this comment.
The sncloud_logs tool description concatenates two string literals without a separating space, resulting in "PulsarCluster.This tool..." in the rendered description. Add a space at the boundary so the sentence reads correctly in clients.
| mcp.WithDescription("Display the logs of resources in StreamNative Cloud, including pulsar functions, pulsar source connectors, pulsar sink connectors, and kafka connect connectors logs running along with PulsarInstance and PulsarCluster."+ | |
| mcp.WithDescription("Display the logs of resources in StreamNative Cloud, including pulsar functions, pulsar source connectors, pulsar sink connectors, and kafka connect connectors logs running along with PulsarInstance and PulsarCluster. "+ |
| var requestedPaths []string | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| requestedPaths = append(requestedPaths, r.URL.Path) | ||
|
|
There was a problem hiding this comment.
requestedPaths is appended to from the HTTP handler goroutine while the test goroutine later reads it. With t.Parallel() and go test -race, this will be reported as a data race. Protect the slice with a mutex or send paths through a channel and collect them in the test goroutine.
| switch r.Method { | ||
| case http.MethodGet: | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(`{"metadata":{"name":"pi-test","resourceVersion":"1"}}`)) | ||
| case http.MethodPut: | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(`{"metadata":{"name":"pi-test","resourceVersion":"2"}}`)) | ||
| default: | ||
| t.Fatalf("unexpected method %s", r.Method) | ||
| } |
There was a problem hiding this comment.
The HTTP handler calls t.Fatalf from a different goroutine (the server's handler goroutine). testing.T methods are not goroutine-safe and this can lead to racy / flaky failures. Instead, record the unexpected condition (e.g., via an error channel/atomic) and assert in the main test goroutine, or respond with an HTTP error and fail after the request completes.
| package mcp | ||
|
|
||
| import ( |
There was a problem hiding this comment.
This new Go file is missing the standard Apache 2.0 license header used throughout the repo (e.g., other files under pkg/mcp/** and pkg/mcp/builders/**). Adding the header will keep make license-check/CI happy and stay consistent with existing files.
No description provided.