feat: func mcp #3155
feat: func mcp #3155
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
approved
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
4 times, most recently
from
b75b584 to
59f42c0
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3155 +/- ##
==========================================
+ Coverage 59.37% 65.72% +6.34%
==========================================
Files 134 145 +11
Lines 13500 14104 +604
==========================================
+ Hits 8016 9270 +1254
+ Misses 4540 3847 -693
- Partials 944 987 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
3 times, most recently
from
7330b01 to
be3efaf
Compare
| /pkg/functions/testdata/default_home/go | ||
| /pkg/functions/testdata/default_home/.cache | ||
| /pkg/functions/testdata/migrations/*/.gitignore | ||
| /pkg/creds/auth.json |
|
I'd group I'd than do something like https://github.com/containers/kubernetes-mcp-server/blob/main/pkg/toolsets/core/toolset.go#L23 for registering those. |
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
from
be3efaf to
961dccc
Compare
apply rearchitecture to remaining tools
lkingland
force-pushed
the
push-krlrkrmlkzqt
branch
from
961dccc to
ea28447
Compare
There was a problem hiding this comment.
Hi @lkingland! Great job on the MCP integration 😸 👍
Most of my comments so far are cleanups, refactorings and a few typos. Some are opinions, so you decide if you want to go that route 😸 . Many can probably be done in a follow up.
I finished reviewing up to tools_build_test.go. Not much left. Will submit another after lunch.
There was a problem hiding this comment.
If this is the actual API (for agents) I would use composition to structure, so that future fine tuning and modifications will be easier and not always to the same string.
No need to do now. Can be done in a follow up. We can create an issue for that and give it more thought. It's a good-first-issue.
Just a draft:
type AgentAPI struct {}
func (AgentAPI) funcDisabledHeader() string {
return "⚠️ CRITICAL: FUNCTION TOOLS ARE DISABLED\n";
}
func (AgentAPI) funcEnabledHeader() string {
return "✅ Function tools are enabled!\n";
}
func (a AgentAPI) terminology() string {
return `TERMINOLOGY: Always capitalize "Function" when referring to a deployable Function (the service).
Use lowercase "function" only for programming concepts (functions in code).`
}
func (a AgentAPI) funcEnabledInstructions() string {
return fmt.Sprintf(`%s
%s
IMPORTANT INSTRUCTIONS FOR YOU (the AI assistant):
When the user requests to create, deploy, build, or modify Functions, you MUST:
1. STOP immediately - do NOT attempt to use function tools
2. STOP immediately - do NOT offer to run 'func' commands directly
3. STOP immediately - do NOT try to work around this limitation
Instead, you MUST tell the user:
"The Function tools are currently disabled. To enable them:
1. Close/exit this application completely
2. Set FUNC_ENABLE_MCP=true in your MCP client configuration
3. Restart the application
For setup instructions, see:
https://github.com/knative/func/blob/main/docs/mcp-integration/integration.md"
DO NOT attempt any workarounds. The user must restart the client with the environment variable set.`
`, a.funcDisabledHeader(), a.terminology());
}
//...There was a problem hiding this comment.
Totally agree. Not going to do it now just because a giant, annoying string is easy to iterate on. But if this ends up being "correct-ish", then yes, let's make it composable and/or use string templates 👍🏻
| // Returns an error with setup instructions if not enabled. | ||
| func checkMCPEnabled() error { | ||
| if os.Getenv("FUNC_ENABLE_MCP") != "true" { | ||
| return fmt.Errorf(`CANNOT EXECUTE: Function tools are disabled. |
There was a problem hiding this comment.
This error could also go into the AgentAPI stuct proposed above.
| return nil, err | ||
| } | ||
|
|
||
| return &mcp.ReadResourceResult{ |
There was a problem hiding this comment.
This boilerplate code repeats and could be abstracted into a function.
| uri string | ||
| } | ||
|
|
||
| func (r cmdHelpResource) desc() *mcp.Resource { |
There was a problem hiding this comment.
Untested => uses title() and description() internally which contains logic.
| } | ||
| } | ||
|
|
||
| func (r templatesResource) handle(ctx context.Context, request *mcp.ReadResourceRequest, _ string, _ Executor) (*mcp.ReadResourceResult, error) { |
There was a problem hiding this comment.
Untested. See comment about fetchTemplates().
| return mcp.NewToolResultError(fmt.Sprintf("func deploy failed: %s", out)), nil | ||
| // errorResult creates an error CallToolResult with the given message. | ||
| // Used by tools to return errors in MCP protocol format. | ||
| func errorResult(msg string) *mcp.CallToolResult { |
There was a problem hiding this comment.
Nice! This is what I meant in my messages above too 😸 👍 ...abstractions help to keep the code clean.
| DestructiveHint: ptr(false), | ||
| IdempotentHint: true, // Building the same source code multiple times produces the same container image. | ||
| }, | ||
| InputSchema: map[string]any{ |
There was a problem hiding this comment.
InputSchema creation boilerplate is duplicated and could be abstracted into helper functions or a InputSchemaBuilder struct.
Same everywhere else in tools_*.go
The kubernetes-mcp-server toolset registry pattern is good for that use case (dozens of tools across multiple independent domains like Kubernetes, Helm, and Config), but is a bit overly complex here.
For our single-domain, static toolset of only 8 tools, explicit registration in New() is simple, without the indirection of init functions, global registries, etc. Lower cognitive load. I'd say we follow that pattern if our complexity grows 👍 |
| cmdParts := parseCommand(cmdPrefix) | ||
| cmdParts = append(cmdParts, args...) | ||
|
|
||
| output, err := executor.Execute(ctx, ".", cmdParts[0], cmdParts[1:]...) | ||
| if err != nil { | ||
| return errorResult(fmt.Sprintf("func build failed: %s\nOutput: %s", err, string(output))), nil | ||
| } | ||
|
|
||
| // Build structured output | ||
| result := BuildOutput{ | ||
| Message: string(output), | ||
| } | ||
|
|
||
| return jsonResult(result), nil |
There was a problem hiding this comment.
This part is nearly the same in all handlers, could be a helper parseAndExecute(...).
There was a problem hiding this comment.
Similar comment applies to this test code: setup and execution code could be in test helpers, so that only what differs between the tests stays in them.
| } | ||
|
|
||
| // Assert executor invoked | ||
| if !executor.ExecuteInvoked { |
There was a problem hiding this comment.
This couples the test(s) to an implementation detail of the handler. I would not check that the executer was invoked but if the result the handler returns has everything I expect. Wdys?
| defer cancel() | ||
|
|
||
| executor := mock.NewExecutor() | ||
| executor.ExecuteFn = func(ctx context.Context, dir string, name string, args ...string) ([]byte, error) { |
There was a problem hiding this comment.
This mocked function repeats in all the other tests which qualifies it for a test helper 😸.
| defer cancel() | ||
|
|
||
| executor := mock.NewExecutor() | ||
| executor.ExecuteFn = func(ctx context.Context, dir string, name string, args ...string) ([]byte, error) { |
There was a problem hiding this comment.
This mocked function repeats (with small differences) in all the other tests which qualifies it for a test helper 😸.
There was a problem hiding this comment.
It looks like all those tool tests are very similar. One could factor out a bunch of test helpers (I left a few comments) and/or one could use table driven tests for the three test types Test*, Test*_Options, Test*_BinaryFailure and remove a lot of duplication.
|
|
||
| import "os" | ||
|
|
||
| func instructions() string { |
There was a problem hiding this comment.
Would extract the strings into functions so this one is a bit better readable.
func instructions() string {
if os.Getenv("FUNC_ENABLE_MCP") != "true" {
return disabledInstructions()
}
return enabledInstructions()
}
const terminology = `TERMINOLOGY: Always capitalize "Function" when referring to a deployable Function (the service).
Use lowercase "function" only for programming concepts (functions in code).`
func disabledInstruction() string {
return fmt.Sprintf(`⚠️ CRITICAL: FUNCTION TOOLS ARE DISABLED
%s
IMPORTANT INSTRUCTIONS FOR YOU (the AI assistant):
...`, terminology)
}
func enabledInstructions() string {
return fmt.Sprintf(`✅ Function tools are enabled!
%s
...`, terminology)
}
Co-authored-by: Stanislav Jakuschevskij <[email protected]>
Co-authored-by: Stanislav Jakuschevskij <[email protected]>
Co-authored-by: Stanislav Jakuschevskij <[email protected]>
Co-authored-by: Stanislav Jakuschevskij <[email protected]>
There was a problem hiding this comment.
pls remove or include in .gitignore as per #3158
same with absoluteLinkWindows
knative-prow
bot
added
the
do-not-merge/work-in-progress
Converting back to draft, so we can make it slightly more... slightly more mature. |
4 participants
Updates the POC Functions MCP server to be a slightly more mature experimental MCP server.
Enable with FUNC_ENABLE_MCP and start (by configuring one's client LLM) via
func mcp start.