feat: MCP phase 1 read-only operations#579
Draft
kfelternv wants to merge 1 commit into
Draft
Conversation
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kfelternv
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
cli/mcp/schema_test.go (1)
84-123: ⚡ Quick winAdd a regression test for
{in,name}override + uniqueRequiredentries.Current tests are strong, but they do not yet assert operation-level override behavior against path-item parameters with the same key, or enforce uniqueness in
schema.Required.Suggested test addition
+func TestBuildInputSchema_OperationParamOverridesPathItemParam(t *testing.T) { + item := appcli.PathItem{ + Parameters: []appcli.Parameter{ + {Name: "status", In: "query", Required: true, Description: "from item", Schema: &appcli.Schema{Type: "string"}}, + }, + } + op := &appcli.Operation{ + OperationID: "get-override-case", + Parameters: []appcli.Parameter{ + {Name: "status", In: "query", Required: false, Description: "from op", Schema: &appcli.Schema{Type: "string"}}, + }, + } + + schema := buildInputSchema(item, op) + require.Equal(t, "from op", schema.Properties["status"].Description) + require.NotContains(t, schema.Required, "status") + require.Len(t, schema.Required, len(uniqueStrings(schema.Required))) +} + +func uniqueStrings(in []string) map[string]struct{} { + out := make(map[string]struct{}, len(in)) + for _, s := range in { + out[s] = struct{}{} + } + return out +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/mcp/schema_test.go` around lines 84 - 123, Add a regression test that verifies operation-level parameters override path-item parameters when they share the same (In, Name) key and that schema.Required entries are deduplicated: construct an appcli.PathItem with a parameter (e.g., In: "path", Name: "id", Description: "path desc") and an appcli.Operation that also declares a parameter with the same In/Name but a different Description and that includes the parameter in its Required list (and ensure the path-item also marks it required), call buildInputSchema(pathItem, operation), then assert the resulting schema.Properties["id"].Description equals the operation-level description (ensuring override) and assert the schema.Required slice contains "id" only once (use require.Contains/require.Equal/require.Len as appropriate). Reference buildInputSchema, appcli.PathItem, appcli.Operation, schema.Required, and schema.Properties to locate where to add the test.cli/mcp/config.go (1)
128-128: 💤 Low valuePrefer idiomatic slice declaration.
Use
var missing []stringrather than[]string{}when initializing an empty slice that will be appended to. The former is the idiomatic Go pattern and avoids an unnecessary allocation when no elements are appended.♻️ Suggested change
func (c resolvedConfig) requireNonEmpty() error { - missing := []string{} + var missing []string if c.Org == "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/mcp/config.go` at line 128, Replace the non-idiomatic empty-slice literal initialization for the variable named "missing" with the idiomatic zero-value declaration; locate the declaration of missing in the config.go snippet and change the initialization from []string{} to using the zero value (var missing []string) so it avoids an unnecessary allocation and follows Go conventions.cli/mcp/transport_test.go (1)
286-292: ⚡ Quick winAssert per-call success in the refresh loop.
The loop currently closes the body without validating result status/payload, so the test can pass on counter effects even when a tool call fails. Add
StatusOKandisError=falsechecks per iteration.Suggested test hardening
for i := 0; i < 2; i++ { resp := mcpPost(t, ts.URL, "", jsonrpcRequest(i+1, "tools/call", map[string]any{ "name": "nico_get_foo", "arguments": map[string]any{"fooId": "foo-" + itoa(i)}, })) - _ = resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + _ = resp.Body.Close() + result := decodeToolCallResult(t, body) + require.False(t, result.IsError, "tool call should succeed: %s", body) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/mcp/transport_test.go` around lines 286 - 292, The test loop sends two tool calls using mcpPost with jsonrpcRequest but currently closes resp.Body without verifying success; update the loop that calls mcpPost (in the for i := 0; i < 2; i++) to assert the HTTP response is StatusOK and the JSON-RPC payload indicates isError == false for each iteration (use the same response parsing used elsewhere in this test), then close resp.Body after those assertions so each call is validated individually; reference mcpPost, jsonrpcRequest, and the per-response StatusOK/isError checks when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/mcp/cmd.go`:
- Around line 146-147: LoadConfig's returned error is being ignored; change the
call to capture the error (e.g., cfg, err := appcli.LoadConfig()) and handle it
instead of discarding it — validate err and bail out (return or log/exit) with a
clear message so malformed configs don't continue, then only call
appcli.ApplyEnvOverrides(cfg) when err is nil.
- Around line 122-124: The code registers sigCh with signal.Notify but never
unregisters it; add a call to signal.Stop(sigCh) to unregister the channel when
the server/goroutine exits (either via a defer immediately after
signal.Notify(sigCh, ...) or in the existing shutdown/cleanup path) so that
repeated in-process runs don't accumulate stale signal registrations; reference
the sigCh variable and the existing signal.Notify usage and place
signal.Stop(sigCh) in the same scope where the server shutdown is handled.
- Around line 103-107: Validate the user-supplied path before registering it
with ServeMux to avoid panics: in the function creating the mux (where variables
path, shutdownTimeout, mux and call mux.Handle(path, NewHandler(server)) live),
check that path is non-empty and starts with '/' (and optionally reject patterns
containing spaces or invalid characters), and if invalid return an error or exit
with a clear message instead of calling mux.Handle; only call mux.Handle(path,
NewHandler(server)) when the validation passes.
In `@cli/mcp/schema.go`:
- Around line 50-66: The code builds allParams by appending item.Parameters then
op.Parameters which can leave path-level params duplicated when an
operation-level param overrides the same (in,name), causing incorrect required
entries; change the logic to deduplicate by the tuple (p.In, p.Name) preferring
operation-level values: build a map keyed by fmt.Sprintf("%s|%s", p.In, p.Name)
(or equivalent) and first insert path-item parameters then overwrite with
operation parameters (or insert op params last to override), then iterate the
deduplicated slice/map to populate props and required using paramToJSONSchema
and the existing checks (p.In, p.Name, p.Required), ensuring required only
reflects the final parameter set.
In `@cli/mcp/server.go`:
- Around line 161-184: The splitArgs function currently drops unsupported shapes
silently (coerceToString returning ""), which can hide bad requests; change
splitArgs to return an error (e.g., func splitArgs(...) (pathParams, queryParams
map[string]string, err error)) and when coerceToString(raw) yields an empty
string due to an unsupported type (arrays/maps/objects) return a descriptive
error mentioning the parameter name and type instead of skipping it; add a TODO
in the function noting that full OpenAPI style/explode serialization is
intentionally deferred and should be implemented later (reference splitArgs,
coerceToString, and appcli.Parameter), and apply the same fail-fast behavior to
the other analogous block mentioned (lines ~186-213).
In `@go.mod`:
- Line 56: Update the dependency version for module
github.com/modelcontextprotocol/go-sdk from v0.8.0 to v1.4.1 in go.mod and
ensure the repository's Go toolchain constraint (go directive in go.mod) is set
to at least 1.25 to satisfy the new module's requirement; run go mod tidy
afterwards to refresh go.sum and resolve transitive changes.
---
Nitpick comments:
In `@cli/mcp/config.go`:
- Line 128: Replace the non-idiomatic empty-slice literal initialization for the
variable named "missing" with the idiomatic zero-value declaration; locate the
declaration of missing in the config.go snippet and change the initialization
from []string{} to using the zero value (var missing []string) so it avoids an
unnecessary allocation and follows Go conventions.
In `@cli/mcp/schema_test.go`:
- Around line 84-123: Add a regression test that verifies operation-level
parameters override path-item parameters when they share the same (In, Name) key
and that schema.Required entries are deduplicated: construct an appcli.PathItem
with a parameter (e.g., In: "path", Name: "id", Description: "path desc") and an
appcli.Operation that also declares a parameter with the same In/Name but a
different Description and that includes the parameter in its Required list (and
ensure the path-item also marks it required), call buildInputSchema(pathItem,
operation), then assert the resulting schema.Properties["id"].Description equals
the operation-level description (ensuring override) and assert the
schema.Required slice contains "id" only once (use
require.Contains/require.Equal/require.Len as appropriate). Reference
buildInputSchema, appcli.PathItem, appcli.Operation, schema.Required, and
schema.Properties to locate where to add the test.
In `@cli/mcp/transport_test.go`:
- Around line 286-292: The test loop sends two tool calls using mcpPost with
jsonrpcRequest but currently closes resp.Body without verifying success; update
the loop that calls mcpPost (in the for i := 0; i < 2; i++) to assert the HTTP
response is StatusOK and the JSON-RPC payload indicates isError == false for
each iteration (use the same response parsing used elsewhere in this test), then
close resp.Body after those assertions so each call is validated individually;
reference mcpPost, jsonrpcRequest, and the per-response StatusOK/isError checks
when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5948a2fe-d9b7-48d9-94e8-70bbf4b3cd77
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
AGENTS.mdcli/README.mdcli/cmd/cli/main.gocli/mcp/cmd.gocli/mcp/config.gocli/mcp/config_test.gocli/mcp/schema.gocli/mcp/schema_test.gocli/mcp/server.gocli/mcp/server_test.gocli/mcp/transport_test.gogo.mod
Comment on lines
+103
to
+107
| path := c.String("path") | ||
| shutdownTimeout := c.Duration("shutdown-timeout") | ||
|
|
||
| mux := http.NewServeMux() | ||
| mux.Handle(path, NewHandler(server)) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls
echo "== Files =="
git ls-files 'cli/mcp/cmd.go' || true
echo
echo "== cmd.go (context around requested lines) =="
# show around 70-180 to capture the snippet and adjacent code
sed -n '1,220p' cli/mcp/cmd.go | cat -n
echo
echo "== Search for path flag + ServeMux.Handle =="
rg -n --hidden --no-ignore -S 'String\("path"\)|--path|ServeMux\.Handle|mux\.Handle\(' cli/mcp/cmd.go
echo
echo "== Search for signal.Notify and Stop =="
rg -n --hidden --no-ignore -S 'signal\.Notify|signal\.Stop|NotifyContext' cli/mcp/cmd.go
echo
echo "== Search for LoadConfig usage and error handling =="
rg -n --hidden --no-ignore -S 'LoadConfig\(' cli/mcp/cmd.goRepository: NVIDIA/infra-controller-rest
Length of output: 7741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate LoadConfig implementation =="
fd -t f 'LoadConfig' -a cli -a . || true
rg -n --hidden --no-ignore -S 'func\s+LoadConfig\s*\(' -g'*.go' .
echo
echo "== Show LoadConfig definition =="
# Print the exact definition lines around the match
rg -n --hidden --no-ignore -S 'func\s+LoadConfig\s*\(' -g'*.go' . | head -n 20
# Extract file path from first match
first=$(rg -n --hidden --no-ignore -S 'func\s+LoadConfig\s*\(' -g'*.go' . | head -n 1 | cut -d: -f1)
echo "First match file: $first"
if [[ -n "$first" ]]; then
sed -n '1,220p' "$first" | cat -n
fi
echo
echo "== Check net/http ServeMux.Handle panic/validation docs (local via grep if go has src) =="
# Try to find local Go source if available in environment
goenv=$(go env GOROOT 2>/dev/null || true)
if [[ -n "${goenv}" && -d "${goenv}/src/net/http" ]]; then
rg -n 'func\s+\(.*ServeMux\)\s+Handle\(' "${goenv}/src/net/http" -S || true
file=$(rg -n 'func\s+\(.*ServeMux\)\s+Handle\(' "${goenv}/src/net/http" -S | head -n 1 | cut -d: -f1)
echo "Go source file: $file"
if [[ -n "${file}" ]]; then
sed -n '1,260p' "$file" | rg -n 'ServeMux|Handle\(|panic|pattern|/\' -n | head -n 120 || true
# Also dump the specific Handle method block if we can
sed -n '1,260p' "$file" | cat -n | sed -n '/func (.*ServeMux).*Handle/,+120p' || true
fi
else
echo "Go source not available (GOROOT missing)."
fiRepository: NVIDIA/infra-controller-rest
Length of output: 6440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Show cli/pkg/config.go LoadConfig + ApplyEnvOverrides =="
sed -n '1,220p' cli/pkg/config.go | cat -n
echo
echo "== Extract relevant portion of net/http ServeMux.Handle (Go stdlib) =="
# Print around Handle implementation and validatePattern it calls.
# Use sed to avoid regex pitfalls; just locate by line numbers via rg.
server_go="/usr/local/go/src/net/http/server.go"
handle_line=$(rg -n 'func \(mux \*ServeMux\) Handle\(' "$server_go" | head -n1 | cut -d: -f1)
echo "ServeMux.Handle line: $handle_line"
start=$((handle_line-40))
end=$((handle_line+90))
sed -n "${start},${end}p" "$server_go" | cat -n
echo
echo "== Extract validatePattern helper =="
validate_line=$(rg -n 'validatePattern\(' "$server_go" | head -n1 | cut -d: -f1)
echo "validatePattern line: $validate_line"
vstart=$((validate_line-40))
vend=$((validate_line+140))
sed -n "${vstart},${vend}p" "$server_go" | cat -nRepository: NVIDIA/infra-controller-rest
Length of output: 13148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate ApplyEnvOverrides =="
rg -n --hidden --no-ignore -S 'func\s+ApplyEnvOverrides\s*\(' cli/pkg | head -n 20
file=$(rg -n --hidden --no-ignore -S 'func\s+ApplyEnvOverrides\s*\(' cli/pkg | head -n 1 | cut -d: -f1)
echo "File: $file"
sed -n '1,220p' "$file" | cat -n
echo
echo "== Check any callers expecting non-nil cfg =="
rg -n --hidden --no-ignore -S 'ApplyEnvOverrides\(' . | head -n 50Repository: NVIDIA/infra-controller-rest
Length of output: 10016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
server_go="/usr/local/go/src/net/http/server.go"
echo "== parsePattern implementation =="
pstart=$(rg -n 'func parsePattern\(' "$server_go" | head -n1 | cut -d: -f1)
echo "parsePattern line: $pstart"
start=$((pstart-60))
end=$((pstart+200))
sed -n "${start},${end}p" "$server_go" | cat -n | head -n 260
echo
echo "== net/http ServeMux registerErr invalid pattern conditions (for reference) =="
# already printed but reprint narrower around registerErr
hstart=$(rg -n 'func \(mux \*ServeMux\) registerErr\(' "$server_go" | head -n1 | cut -d: -f1)
echo "registerErr line: $hstart"
start=$((hstart-10))
end=$((hstart+80))
sed -n "${start},${end}p" "$server_go" | cat -n
echo
echo "== Check runServe for signal.Stop usage =="
rg -n --hidden --no-ignore -S 'signal\.Stop\(' cli/mcp/cmd.go cli/mcp -S || true
echo
echo "== Show runServe tail to ensure no cleanup =="
sed -n '80,170p' cli/mcp/cmd.go | cat -nRepository: NVIDIA/infra-controller-rest
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
server_go="/usr/local/go/src/net/http/server.go"
echo "== parsePattern implementation =="
pstart=$(rg -n 'func parsePattern\(' "$server_go" | head -n1 | cut -d: -f1)
echo "parsePattern line: $pstart"
start=$((pstart-60))
end=$((pstart+200))
sed -n "${start},${end}p" "$server_go" | cat -n | head -n 260
echo
echo "== net/http ServeMux registerErr invalid pattern conditions (for reference) =="
hstart=$(rg -n 'func \(mux \*ServeMux\) registerErr\(' "$server_go" | head -n1 | cut -d: -f1)
echo "registerErr line: $hstart"
start=$((hstart-10))
end=$((hstart+80))
sed -n "${start},${end}p" "$server_go" | cat -n
echo
echo "== Check runServe for signal.Stop usage =="
rg -n --hidden --no-ignore -S 'signal\.Stop\(' cli/mcp/cmd.go cli/mcp -S || true
echo
echo "== Show runServe tail to ensure no cleanup =="
sed -n '80,170p' cli/mcp/cmd.go | cat -nRepository: NVIDIA/infra-controller-rest
Length of output: 106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
server_root="/usr/local/go/src/net/http"
echo "== Locate parsePattern across net/http =="
rg -n 'func parsePattern\(' "$server_root" | head -n 20
echo
echo "== Print the parsePattern function body (first match) =="
pfile=$(rg -n 'func parsePattern\(' "$server_root" | head -n 1 | cut -d: -f1)
pline=$(rg -n 'func parsePattern\(' "$server_root" | head -n 1 | cut -d: -f2)
echo "parsePattern: $pfile:$pline"
start=$((pline-40))
end=$((pline+240))
sed -n "${start},${end}p" "$pfile" | cat -n | head -n 260
echo
echo "== Check runServe for signal.Stop usage (full file) =="
rg -n --hidden --no-ignore -S 'signal\.Stop\(' cli/mcp/cmd.go cli/mcp -S || true
echo
echo "== Show end of runServe in cli/mcp/cmd.go =="
# runServe ends around line ~140 in earlier output; show that region
sed -n '90,170p' cli/mcp/cmd.go | cat -nRepository: NVIDIA/infra-controller-rest
Length of output: 12868
Validate --path before registering the handler.
path comes directly from the flag and is passed to net/http’s ServeMux.Handle, which panics when the pattern is invalid. Enforce basic validity (e.g., non-empty and starting with /) before registering.
Proposed fix
path := c.String("path")
+ if path == "" || path[0] != '/' {
+ return fmt.Errorf("invalid --path %q: must be non-empty and start with '/'", path)
+ }
shutdownTimeout := c.Duration("shutdown-timeout")
mux := http.NewServeMux()
mux.Handle(path, NewHandler(server))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| path := c.String("path") | |
| shutdownTimeout := c.Duration("shutdown-timeout") | |
| mux := http.NewServeMux() | |
| mux.Handle(path, NewHandler(server)) | |
| path := c.String("path") | |
| if path == "" || path[0] != '/' { | |
| return fmt.Errorf("invalid --path %q: must be non-empty and start with '/'", path) | |
| } | |
| shutdownTimeout := c.Duration("shutdown-timeout") | |
| mux := http.NewServeMux() | |
| mux.Handle(path, NewHandler(server)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/mcp/cmd.go` around lines 103 - 107, Validate the user-supplied path
before registering it with ServeMux to avoid panics: in the function creating
the mux (where variables path, shutdownTimeout, mux and call mux.Handle(path,
NewHandler(server)) live), check that path is non-empty and starts with '/' (and
optionally reject patterns containing spaces or invalid characters), and if
invalid return an error or exit with a clear message instead of calling
mux.Handle; only call mux.Handle(path, NewHandler(server)) when the validation
passes.
Comment on lines
+122
to
+124
| sigCh := make(chan os.Signal, 1) | ||
| signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and display the relevant section in cli/mcp/cmd.go
FILE="cli/mcp/cmd.go"
if [ ! -f "$FILE" ]; then
echo "Missing file: $FILE"
exit 1
fi
echo "== Context around requested lines =="
nl -ba "$FILE" | sed -n '90,170p'Repository: NVIDIA/infra-controller-rest
Length of output: 171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for other signal handlers / cleanup patterns across the codebase
rg -n --hidden --no-ignore-vcs "signal\.Notify\(|signal\.Stop\(|signal\.Reset\(" cli -S || trueRepository: NVIDIA/infra-controller-rest
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for any tests that might repeatedly invoke this code path
rg -n --hidden --no-ignore-vcs "mcp.*cmd|cli/mcp|New.*MCP|cmd\.go" . -S --glob '*_test.go' || trueRepository: NVIDIA/infra-controller-rest
Length of output: 54
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="cli/mcp/cmd.go"
# Show around the target region (avoid nl)
sed -n '100,150p' "$FILE" | cat -nRepository: NVIDIA/infra-controller-rest
Length of output: 1846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all signal.Notify / signal.Stop usage in this repo
rg -n --hidden --no-ignore-vcs "signal\.Notify\(|signal\.Stop\(" . -S || trueRepository: NVIDIA/infra-controller-rest
Length of output: 671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the whole function / block likely containing the snippet
# (heuristic: print from the nearest preceding 'func' to ~30 lines after the snippet region)
FILE="cli/mcp/cmd.go"
start=$(awk 'NR>=1 && /func / {last=$1} NR>=80 && NR<=150 { } END {print last}' < "$FILE")
echo "Heuristic start NR: $start"
sed -n "${start},${start+120}p" "$FILE" | cat -nRepository: NVIDIA/infra-controller-rest
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check whether the repo uses signal.Stop anywhere
rg -n --hidden --no-ignore-vcs "signal\.Stop\(" . || true
# 2) Inspect a few other call sites around their signal.Notify to learn prevailing patterns
for f in \
"site-agent/cmd/site-agent/main.go" \
"site-agent/pkg/components/managers/context.go" \
"powershelf-manager/cmd/serve.go" \
"nvswitch-manager/cmd/serve.go" \
"flow/cmd/serve.go" \
"cert-manager/pkg/core/context.go"; do
echo "---- $f ----"
# Print ~60 lines around the first signal.Notify occurrence in each file
ln=$(rg -n "signal\.Notify\(" "$f" | head -n1 | cut -d: -f1 || true)
if [ -n "${ln:-}" ]; then
start=$((ln-20))
end=$((ln+25))
sed -n "${start},${end}p" "$f" | cat -n
else
echo "No signal.Notify found in $f (unexpected)."
fi
done
# 3) Inspect a bit more around cli/mcp/cmd.go signal.Notify block
echo "---- cli/mcp/cmd.go ----"
ln=$(rg -n "signal\.Notify\(sigCh" cli/mcp/cmd.go | head -n1 | cut -d: -f1 || true)
if [ -n "${ln:-}" ]; then
start=$((ln-30))
end=$((ln+40))
sed -n "${start},${end}p" cli/mcp/cmd.go | cat -n
fiRepository: NVIDIA/infra-controller-rest
Length of output: 9983
Stop signal notifications on exit to avoid lingering signal registrations.
signal.Notify(sigCh, ...) registers sigCh with the runtime, but this code never calls signal.Stop(sigCh), so repeated in-process invocations (e.g., tests) can accumulate stale registrations even after the server goroutine exits.
Proposed fix
sigCh := make(chan os.Signal, 1)
signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)
+ defer signal.Stop(sigCh)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sigCh := make(chan os.Signal, 1) | |
| signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) | |
| sigCh := make(chan os.Signal, 1) | |
| signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM) | |
| defer signal.Stop(sigCh) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/mcp/cmd.go` around lines 122 - 124, The code registers sigCh with
signal.Notify but never unregisters it; add a call to signal.Stop(sigCh) to
unregister the channel when the server/goroutine exits (either via a defer
immediately after signal.Notify(sigCh, ...) or in the existing shutdown/cleanup
path) so that repeated in-process runs don't accumulate stale signal
registrations; reference the sigCh variable and the existing signal.Notify usage
and place signal.Stop(sigCh) in the same scope where the server shutdown is
handled.
Comment on lines
+146
to
+147
| cfg, _ := appcli.LoadConfig() | ||
| appcli.ApplyEnvOverrides(cfg) |
Contributor
There was a problem hiding this comment.
Handle config-loading errors instead of discarding them.
Ignoring LoadConfig errors can silently mask malformed config and start the server with unintended defaults.
Proposed fix
- cfg, _ := appcli.LoadConfig()
+ cfg, err := appcli.LoadConfig()
+ if err != nil {
+ return Options{}, fmt.Errorf("loading config: %w", err)
+ }
appcli.ApplyEnvOverrides(cfg)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/mcp/cmd.go` around lines 146 - 147, LoadConfig's returned error is being
ignored; change the call to capture the error (e.g., cfg, err :=
appcli.LoadConfig()) and handle it instead of discarding it — validate err and
bail out (return or log/exit) with a clear message so malformed configs don't
continue, then only call appcli.ApplyEnvOverrides(cfg) when err is nil.
Comment on lines
+50
to
+66
| allParams := append([]appcli.Parameter{}, item.Parameters...) | ||
| allParams = append(allParams, op.Parameters...) | ||
| for _, p := range allParams { | ||
| if p.Name == "org" { | ||
| // Resolved from the config layer (org override or server | ||
| // default). The OpenAPI {org} segment is filled in by | ||
| // appcli.Client.Do. | ||
| continue | ||
| } | ||
| if p.In != "path" && p.In != "query" { | ||
| continue | ||
| } | ||
| props[p.Name] = paramToJSONSchema(p) | ||
| if p.In == "path" || p.Required { | ||
| required = append(required, p.Name) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Handle OpenAPI parameter override semantics before building required.
Line 50 and Line 64 currently append parameters directly, which can leave duplicate or stale entries in Required when an operation-level parameter overrides a path-item-level parameter with the same {in,name}. That can produce an invalid or misleading schema contract.
Proposed fix
func buildInputSchema(item appcli.PathItem, op *appcli.Operation) *jsonschema.Schema {
props := map[string]*jsonschema.Schema{}
- var required []string
-
- allParams := append([]appcli.Parameter{}, item.Parameters...)
- allParams = append(allParams, op.Parameters...)
- for _, p := range allParams {
+ requiredSet := map[string]struct{}{}
+
+ type paramKey struct {
+ in string
+ name string
+ }
+ merged := map[paramKey]appcli.Parameter{}
+ for _, p := range item.Parameters {
+ merged[paramKey{in: p.In, name: p.Name}] = p
+ }
+ for _, p := range op.Parameters {
+ // operation-level overrides path-item-level by {in,name}
+ merged[paramKey{in: p.In, name: p.Name}] = p
+ }
+ for _, p := range merged {
if p.Name == "org" {
// Resolved from the config layer (org override or server
// default). The OpenAPI {org} segment is filled in by
// appcli.Client.Do.
continue
}
if p.In != "path" && p.In != "query" {
continue
}
props[p.Name] = paramToJSONSchema(p)
if p.In == "path" || p.Required {
- required = append(required, p.Name)
+ requiredSet[p.Name] = struct{}{}
}
}
+ required := make([]string, 0, len(requiredSet))
+ for name := range requiredSet {
+ required = append(required, name)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/mcp/schema.go` around lines 50 - 66, The code builds allParams by
appending item.Parameters then op.Parameters which can leave path-level params
duplicated when an operation-level param overrides the same (in,name), causing
incorrect required entries; change the logic to deduplicate by the tuple (p.In,
p.Name) preferring operation-level values: build a map keyed by
fmt.Sprintf("%s|%s", p.In, p.Name) (or equivalent) and first insert path-item
parameters then overwrite with operation parameters (or insert op params last to
override), then iterate the deduplicated slice/map to populate props and
required using paramToJSONSchema and the existing checks (p.In, p.Name,
p.Required), ensuring required only reflects the final parameter set.
Comment on lines
+161
to
+184
| func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryParams map[string]string) { | ||
| pathParams = map[string]string{} | ||
| queryParams = map[string]string{} | ||
| for _, p := range params { | ||
| if p.Name == "org" { | ||
| continue | ||
| } | ||
| raw, ok := in[p.Name] | ||
| if !ok { | ||
| continue | ||
| } | ||
| s := coerceToString(raw) | ||
| if s == "" { | ||
| continue | ||
| } | ||
| switch p.In { | ||
| case "path": | ||
| pathParams[p.Name] = s | ||
| case "query": | ||
| queryParams[p.Name] = s | ||
| } | ||
| } | ||
| return pathParams, queryParams | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast on unsupported argument shapes instead of silently dropping them.
Provided values like arrays/maps are currently converted to "" and then omitted, which can generate incorrect requests without surfacing any error. Return an explicit tool error when a supplied argument cannot be serialized, and add a TODO for full OpenAPI style/explode support if deferred.
Suggested direction
-func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryParams map[string]string) {
+func splitArgs(in map[string]any, params []appcli.Parameter) (pathParams, queryParams map[string]string, err error) {
pathParams = map[string]string{}
queryParams = map[string]string{}
for _, p := range params {
...
- s := coerceToString(raw)
- if s == "" {
- continue
- }
+ s, ok := coerceToString(raw)
+ if !ok {
+ return nil, nil, fmt.Errorf("unsupported argument type for %q: %T", p.Name, raw)
+ }
switch p.In {
...
}
}
- return pathParams, queryParams
+ return pathParams, queryParams, nil
}
-func coerceToString(v any) string {
+func coerceToString(v any) (string, bool) {
switch t := v.(type) {
case nil:
- return ""
+ return "", true
case string:
- return t
+ return t, true
...
default:
- return ""
+ return "", false
}
}As per coding guidelines: Add TODO comments for features or nuances not important to implement right away and Document when you have intentionally omitted code that the reader might otherwise expect to be present.
Also applies to: 186-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli/mcp/server.go` around lines 161 - 184, The splitArgs function currently
drops unsupported shapes silently (coerceToString returning ""), which can hide
bad requests; change splitArgs to return an error (e.g., func splitArgs(...)
(pathParams, queryParams map[string]string, err error)) and when
coerceToString(raw) yields an empty string due to an unsupported type
(arrays/maps/objects) return a descriptive error mentioning the parameter name
and type instead of skipping it; add a TODO in the function noting that full
OpenAPI style/explode serialization is intentionally deferred and should be
implemented later (reference splitArgs, coerceToString, and appcli.Parameter),
and apply the same fail-fast behavior to the other analogous block mentioned
(lines ~186-213).
| github.com/lib/pq v1.10.9 | ||
| github.com/metal-stack/v v1.0.3 | ||
| github.com/mitchellh/mapstructure v1.5.0 | ||
| github.com/modelcontextprotocol/go-sdk v0.8.0 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify current vulnerability records for the exact module/version via OSV.
curl -sS https://api.osv.dev/v1/query \
-H 'Content-Type: application/json' \
-d '{"package":{"ecosystem":"Go","name":"github.com/modelcontextprotocol/go-sdk"},"version":"0.8.0"}' \
| jq .
# Check which newer versions are available (to pick a patched target).
curl -sS https://proxy.golang.org/github.com/modelcontextprotocol/go-sdk/@v/list | tail -n 20Repository: NVIDIA/infra-controller-rest
Length of output: 19411
Upgrade github.com/modelcontextprotocol/go-sdk from v0.8.0 to v1.4.1 in go.mod (line 56) before merge.
OSV reports multiple HIGH advisories affecting versions ≤ 1.4.0 (so v0.8.0 is impacted), including:
- GHSA-89xv-2j6f-qhc8 — fixed in v1.4.1
- GHSA-q382-vc8q-7jhj — fixed in v1.4.1
- GHSA-wvj2-96wp-fq3f — fixed in v1.3.1
- GHSA-xw59-hvm2-8pj6 — fixed in v1.4.0
v1.4.1 also requires Go 1.25+, so ensure the repo’s Go toolchain constraint supports the upgrade.
🧰 Tools
🪛 OSV Scanner (2.3.8)
[HIGH] 56-56: github.com/modelcontextprotocol/go-sdk 0.8.0: MCP Go SDK Vulnerable to Improper Handling of Case Sensitivity in github.com/modelcontextprotocol/go-sdk
(GO-2026-4569)
[HIGH] 56-56: github.com/modelcontextprotocol/go-sdk 0.8.0: Improper handling of null Unicode character when parsing JSON in github.com/modelcontextprotocol/go-sdk
(GO-2026-4770)
[HIGH] 56-56: github.com/modelcontextprotocol/go-sdk 0.8.0: Cross-Site Tool Execution for HTTP Servers without Authorizatrion in github.com/modelcontextprotocol/go-sdk
(GO-2026-4773)
[HIGH] 56-56: github.com/modelcontextprotocol/go-sdk 0.8.0: Cross-Site Tool Execution for HTTP Servers without Authorizatrion in github.com/modelcontextprotocol/go-sdk
[HIGH] 56-56: github.com/modelcontextprotocol/go-sdk 0.8.0: Improper handling of null Unicode character when parsing JSON in github.com/modelcontextprotocol/go-sdk
[HIGH] 56-56: github.com/modelcontextprotocol/go-sdk 0.8.0: MCP Go SDK Vulnerable to Improper Handling of Case Sensitivity
[HIGH] 56-56: github.com/modelcontextprotocol/go-sdk 0.8.0: DNS Rebinding Protection Disabled by Default in Model Context Protocol Go SDK for Servers Running on Localhost
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 56, Update the dependency version for module
github.com/modelcontextprotocol/go-sdk from v0.8.0 to v1.4.1 in go.mod and
ensure the repository's Go toolchain constraint (go directive in go.mod) is set
to at least 1.25 to satisfy the new module's requirement; run go mod tidy
afterwards to refresh go.sum and resolve transitive changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.