fix(mcp): omit empty subcommand in defaultExecutor argument list#3667
fix(mcp): omit empty subcommand in defaultExecutor argument list#3667Ankitsinghsisodya wants to merge 1 commit intoknative:mainfrom
Conversation
- Implemented TestBuildArgs to ensure buildArgs constructs command arguments correctly, omitting empty subcommands. - Added TestResource_Help to confirm that help resource handlers invoke the executor with the correct arguments, preventing empty strings in command execution. - Updated mcp.go to utilize the new buildArgs function for command argument construction, enhancing clarity and correctness in command execution.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya 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 |
knative-prow
Bot
added
size/L
|
Hi @Ankitsinghsisodya. 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 Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain 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. |
There was a problem hiding this comment.
Pull request overview
Fixes MCP help resource execution against real func binaries by preventing empty command parts from being injected into the executed argument list, and adds targeted regression tests.
Changes:
- Refactors command argument construction into
buildArgs(...)and omits the subcommand when it’s empty. - Removes the spurious
""argument from the root help resource registration. - Adds tests covering
buildArgsbehavior and verifying help resource handlers don’t pass empty command parts to the executor.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/mcp/mcp.go | Removes empty root help cmd segment and uses new buildArgs helper to construct exec args without empty subcommand injection. |
| pkg/mcp/mcp_test.go | Adds table-driven tests validating buildArgs behavior across empty subcommand and multi-word prefixes. |
| pkg/mcp/resources_test.go | Adds end-to-end-ish resource-handler tests to ensure help URIs map to correct executor args and never include empty strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (e defaultExecutor) Execute(ctx context.Context, subcommand string, args ...string) ([]byte, error) { | ||
| // Parse prefix: "func" or "kn func" -> ["func"] or ["kn", "func"] | ||
| cmdParts := strings.Fields(e.s.prefix) | ||
| cmdParts = append(cmdParts, subcommand) | ||
| cmdParts = append(cmdParts, args...) | ||
|
|
||
| cmdParts := buildArgs(e.s.prefix, subcommand, args) | ||
| cmd := exec.CommandContext(ctx, cmdParts[0], cmdParts[1:]...) | ||
| // cmd.Dir not set - inherits process working directory which is the current working directory |
| // TestResource_Help verifies that every help resource handler invokes the | ||
| // executor with an empty subcommand and the correct command arguments ending | ||
| // with "--help". This is the regression test for the bug where the empty | ||
| // subcommand was unconditionally appended, injecting a spurious "" argument | ||
| // into every help command. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3667 +/- ##
==========================================
+ Coverage 56.02% 56.73% +0.70%
==========================================
Files 181 181
Lines 20732 20734 +2
==========================================
+ Hits 11615 11763 +148
+ Misses 7905 7759 -146
Partials 1212 1212
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:
|
Problem
Every help resource handler calls
executor.Execute(ctx, "", cmd..., "--help"), passing an empty string as thesubcommandargument.defaultExecutor.Executeunconditionally appended that empty string to the command parts, producing invocations like:Cobra receives
""as the first positional argument, fails to match it against any known subcommand, and returns an error. Every help resource was broken when used against a realfuncbinary.A second instance of the same class of bug existed on the root help resource registration, which passed
""as acmdvariadic argument:This injected a second empty argument even with the executor fix in place.
The test suite did not catch either defect because every resource test uses a mock executor via
WithExecutor, sodefaultExecutoris never exercised.Fix
buildArgs(prefix, subcommand string, args []string) []stringfromdefaultExecutor.Execute. The helper omits the subcommand when it is empty.""argument from the root help resource registration.Tests
TestBuildArgs— five table-driven cases directly testingbuildArgs, including the critical empty-subcommand case and multi-word prefix handling.TestResource_Help— eleven table-driven cases covering every registered help URI end-to-end through the resource handler → mock executor path. Asserts thatsubcommandis always""and no element inargsis ever an empty string.