Skip to content

Allow hosted runtimes to register extra MCP prompts and resources - #213

Open
daltoniam wants to merge 1 commit into
mainfrom
feat/mcp-features
Open

Allow hosted runtimes to register extra MCP prompts and resources#213
daltoniam wants to merge 1 commit into
mainfrom
feat/mcp-features

Conversation

@daltoniam

Copy link
Copy Markdown
Owner

Summary

Hosted skill sharing needs a request-scoped way to expose org skills as standard MCP prompts and resources. This adds server.WithMCPFeatures so a hosted runtime can register those surfaces without changing Switchboard's search/execute tool model.

Test plan

  • go test ./server -run 'TestNew|TestWithMCPFeatures'

@acmacalister acmacalister left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice small extension point for hosted skill sharing — option shape matches the existing With* pattern. Two things worth tightening before this lands on the Crush/stateless path: prompt/resource listChanged defaults (same class of bug as #212), and the registration test currently doesn't exercise the MCP surface at all.

CI: build, test, lint, security, rust-sdk all green.

Comment thread server/server.go

s.registerTools()
for _, register := range s.features {
register(s.mcpServer)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once a register callback adds prompts/resources here, the go-sdk infers prompts/resources capabilities with listChanged: truestaticMCPCapabilities only pins tools:

// go-sdk Server.capabilities()
if s.prompts.len() > 0 {
    if caps.Prompts == nil {
        caps.Prompts = &PromptCapabilities{ListChanged: true}
    }
}

That reopens the #212 Crush path this option is aimed at: Crush always installs PromptListChangedHandler / ResourceListChangedHandler, and MCP SDK 1.7 opens subscriptions/listen whenever any of those handlers is set. On a request-scoped/stateless server that stream still has nowhere to live.

Worth extending staticMCPCapabilities the same way as tools:

func staticMCPCapabilities() *mcpsdk.ServerCapabilities {
        return &mcpsdk.ServerCapabilities{
                Logging:   &mcpsdk.LoggingCapabilities{},
                Tools:     &mcpsdk.ToolCapabilities{ListChanged: false},
                Prompts:   &mcpsdk.PromptCapabilities{ListChanged: false},
                Resources: &mcpsdk.ResourceCapabilities{ListChanged: false},
        }
}

(SDK still only advertises prompts/resources when something is actually registered; a non-nil field with listChanged:false just overrides the inferred default.)

Comment thread server/server_test.go
})
}))
require.NotNil(t, srv)
require.NotNil(t, srv.mcpServer)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only asserts the server constructed successfully — AddPrompt never has to succeed for the test to pass, and we don't check that clients can actually see or fetch the prompt.

TestWithExtraInstructions / TestStaticMCPCapabilities_DisableListChanged already wire an in-memory client session; same shape here would catch both registration and the listChanged pitfall:

clientTransport, serverTransport := mcpsdk.NewInMemoryTransports()
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

ss, err := srv.mcpServer.Connect(ctx, serverTransport, nil)
require.NoError(t, err)
defer ss.Close() //nolint:errcheck

client := mcpsdk.NewClient(&mcpsdk.Implementation{Name: "test", Version: "1.0"}, nil)
cs, err := client.Connect(ctx, clientTransport, nil)
require.NoError(t, err)
defer cs.Close() //nolint:errcheck

caps := cs.InitializeResult().Capabilities
require.NotNil(t, caps.Prompts)
assert.False(t, caps.Prompts.ListChanged)

prompts, err := cs.ListPrompts(ctx, nil)
require.NoError(t, err)
require.Len(t, prompts.Prompts, 1)
assert.Equal(t, "skill_review", prompts.Prompts[0].Name)

got, err := cs.GetPrompt(ctx, &mcpsdk.GetPromptParams{Name: "skill_review"})
require.NoError(t, err)
require.NotEmpty(t, got.Messages)

Hosted skill sharing needs a request-scoped way to expose org skills as
standard MCP prompts and resources without changing Switchboard's
search/execute tool surface.

@acmacalister acmacalister left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option shape looks good and matches the existing With* pattern. CI is green across build, test, lint, security, rust-sdk, and compose.

The two open threads from the earlier pass still block landing this on the Crush/stateless path:

  1. staticMCPCapabilities still only pins tools.listChanged=false. Once a feature callback registers prompts/resources, go-sdk v1.5.0 infers those caps with listChanged: true (see Server.capabilities()), which reopens the #212 subscriptions/listen failure mode.
  2. TestWithMCPFeaturesRegistersPrompt still only checks construction — it never asserts the prompt is listable/fetchable or that prompts.listChanged stays false. The in-memory client shape from TestStaticMCPCapabilities_DisableListChanged would cover both.

No new issues beyond those. Happy to approve once the threads are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants