fix(mcp): defer impl construction so Start's writeEnabled controls readonly#3730
fix(mcp): defer impl construction so Start's writeEnabled controls readonly#3730SarthakB11 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SarthakB11 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
|
Welcome @SarthakB11! It looks like this is your first PR to knative/func 🎉 |
|
Hi @SarthakB11. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
…adonly mcp.New eagerly constructed the underlying *mcp.Server with Instructions: instructions(s.readonly). At that point s.readonly is whatever WithReadonly set it to, defaulting to false. Start then reassigns s.readonly = !writeEnabled, but the Instructions string is already baked into the impl by then. cmd/mcp.go invokes mcp.New(mcp.WithPrefix(cmdPrefix)) without WithReadonly, then client.StartMCPServer(cmd.Context(), writeEnabled). writeEnabled defaults to false; FUNC_ENABLE_MCP_WRITE=1 flips it. The default invocation therefore runs in readonly mode but ships the non-readonly system prompt to the connected agent. instructions_warning.md is silently dropped and the agent assumes it can deploy and delete. Move the mcp.NewServer call and the AddTool / AddResource registration out of New and into a buildImpl method invoked from Start after s.readonly is resolved. New keeps its public surface and Options behavior unchanged. The existing TestInstructions test still passes because newTestPairWithReadonly threads WithReadonly into New, which already produced the correct baked Instructions; the test was unaware of the default flow. Add TestInstructions_StartArgDrivesReadonly which mirrors cmd/mcp.go: mcp.New is called without WithReadonly, then Start receives a concrete writeEnabled value. Verifies the readonly warning appears or is absent based on the Start argument, not on what New saw at construction time. Signed-off-by: SarthakB11 <sarthak.bhardwaj21b@iiitg.ac.in>
075aafe to
ae019ee
Compare
|
This seems to be superseded by #3707 which addresses the same issue with a simpler approach. |
|
Thanks for the contribution, but closing this one as a duplicate |
Problem
mcp.Neweagerly constructs the underlying*mcp.ServerwithInstructions: instructions(s.readonly). At that points.readonlyis whatever
WithReadonlyset it to (defaultfalse).Startlaterreassigns
s.readonly = !writeEnabled, but theInstructionsstringis already baked into the impl.
cmd/mcp.gocallsmcp.New(mcp.WithPrefix(cmdPrefix))with noWithReadonly, thenclient.StartMCPServer(cmd.Context(), writeEnabled).writeEnableddefaults tofalseand only flips whenFUNC_ENABLE_MCP_WRITE=1is set. The default
func mcpinvocation therefore runs in readonly modebut ships the non-readonly system prompt.
instructions_warning.mdissilently dropped and the agent assumes it can deploy and delete.
TestInstructionsdid not catch this becausenewTestPairWithReadonlythreads
WithReadonlyintoNew, which already produces the correctbaked
Instructionsfor that call path.Fix
Move the
mcp.NewServercall and theAddTool/AddResourceregistration out of
Newinto abuildImplmethod.Startinvokesit after
s.readonly = !writeEnabled, so the resolved value is theone baked into
Instructions.New's public surface and Optionsbehavior are unchanged.
Tests
TestInstructionsStartArgDrivesReadonlymirrors thecmd/mcp.goflow:mcp.Newis called withoutWithReadonly, thenStartreceives aconcrete
writeEnabled. The two subtests (readonly/write)assert that the readonly warning appears or is absent based on the
Startargument, not on whatNewsaw at construction time.make checkandmake testclean./kind bug