feat(mcp-server): merge donation of k8s-gpu-mcp-server - #1333
Open
ArangoGutierrez wants to merge 27 commits into
Open
feat(mcp-server): merge donation of k8s-gpu-mcp-server#1333ArangoGutierrez wants to merge 27 commits into
ArangoGutierrez wants to merge 27 commits into
Conversation
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
…rage
Findings drive Task 2-22. 9 Working tools, 1 Stub (get_nvlink_topology).
Key spec divergences captured in AUDIT.md sections 2 and 7:
- No monitor gRPC services. Store-only read surface via store-client/pkg/datastore.HealthEventStore.
- Module path is github.com/nvidia/nvsentinel/mcp-server (lowercase).
- Go 1.26.0 toolchain go1.26.2 to match siblings.
- Helm subchart goes at distros/kubernetes/nvsentinel/charts/mcp-server/, not mcp-server/deploy/helm/.
- commons/pkg/{logger,server,metrics,tracing} adopted (event-exporter pattern).
- pkg/monitors/ dropped from the plan; Config struct loses Monitors field.
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Stub tool get_nvlink_topology (Task 15) leaves tracking_issue empty and documents the gap inline in needed_monitor_extension. The issue draft stays in AUDIT.md \xc2\xa7 6.1 as supporting material in case maintainers request a tracking issue during PR review. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Stateless Deployment skeleton mirroring event-exporter's layout. Uses commons/pkg/{logger,server,tracing} for structured logging, HTTP server, and OpenTelemetry tracing. Main loop starts metrics/health server on :9090 and a placeholder goroutine that blocks until SIGTERM. MCP transport wiring is Task 4.
Files added:
- mcp-server/go.mod (lowercase module path github.com/nvidia/nvsentinel/mcp-server, Go 1.26.0)
- mcp-server/main.go and main_test.go (testing CreateMetricsServer port parsing)
- mcp-server/Makefile (includes ../make/{common,go,docker}.mk)
- mcp-server/Tiltfile (ko-tilt-build, ghcr.io/nvidia/nvsentinel/mcp-server)
- mcp-server/README.md (stub pointing to AUDIT.md)
- mcp-server/.gitignore (excludes compiled binary, coverage)
Root files modified:
- .ko.yaml: add mcp-server build entry after event-exporter
- Makefile: add mcp-server to GO_MODULES + lint-test-mcp-server target
Smoke test: ./mcp-server --metrics-port=19091 logs JSON, serves /healthz + /metrics on the port, exits 0 on SIGTERM (verified).
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Narrow read-only view of NVSentinel's HealthEventStore. MCP tools depend on the Reader interface; production wires it to DataStoreReader (thin wrapper around store-client/pkg/datastore.HealthEventStore), tests wire it to FakeReader (in-memory, with QueryBuilder capture for assertions). Scope reduction from the original plan: pkg/monitors/ is dropped entirely. Per AUDIT.md, NVSentinel has no monitor-side gRPC read services; all data flows through the store. This collapses Task 3 to just pkg/store/. Tests (5 cases, all green): - EventsByNode returns seeded events in insertion order - EventsByNode returns empty slice for unknown node (not an error) - LatestEventForNode returns ErrNotFound for unknown node - LatestEventForNode sorts by CreatedAt regardless of seed order - EventsByQuery returns primed result + records the builder for inspection Compile-time assertions ensure both DataStoreReader and FakeReader satisfy Reader; drift breaks the build before tests run. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Donated from ArangoGutierrez/k8s-gpu-mcp-server@80ac33d8. Reshape for NVSentinel idiom: Config struct rewritten to take store.Reader + kubernetes.Interface (no NVML/gateway/xid/k8s legacy deps); klog -> slog; oneshot stdio transport dropped; Bearer auth on /mcp instead of /metrics (metrics auth handled by commons/pkg/server). Files: - pkg/mcp/server.go: Config + Server + New + Run + Shutdown + empty registerTools skeleton (tool tasks 6-16 populate) - pkg/mcp/http.go: streamable HTTP transport via mark3labs/mcp-go, Bearer auth middleware on /mcp, health/readyz/version endpoints, cert-reloader integration - pkg/mcp/tls.go: cert-reloader (5min interval), klog -> slog - pkg/mcp/metrics.go: mcp_server_{requests_total, request_duration_seconds, active_requests} via prometheus/client_golang; RecordRequest helper - pkg/mcp/server_test.go: 3 real tests for New input validation (empty HTTPAddr, nil Store, valid config) - main.go: replace placeholder goroutine with mcp.New() + Run() in errgroup; --mcp-addr and --auth-token flags; store wired to FakeReader as TODO until tool tasks bring real datastore client - go.mod/go.sum: add github.com/mark3labs/mcp-go v0.54.0 as direct dep Smoke verified: /version returns nvsentinel-mcp-server, /mcp reaches mcp-go (HTTP 400 for empty JSON-RPC body), /healthz on metrics port returns ok, SIGTERM -> clean exit 0. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Donated from ArangoGutierrez/k8s-gpu-mcp-server@80ac33d8 verbatim — three prompts in the library: gpu-health-check, diagnose-xid-errors, gpu-triage. The donor's code had no legacy NVSentinel-incompatible imports, so only the copyright header was rewritten (dual NVIDIA + k8s-gpu-mcp-server contributors, verbose Apache block matching sibling components and pkg/mcp/ from Task 4). Wired into pkg/mcp/server.go: added prompts import, set WithPromptCapabilities(true) on the mcp-go server, and registerPrompts() now iterates prompts.Library and calls AddPrompt for each. registerTools stays empty (tool tasks 6-16 populate). Tests (donor-authored, 9 subtests, all green): - TestPromptDef_ToMCPPrompt — round-trips Name/Description/Arguments through mcp-go's mcp.Prompt - TestLibraryPrompts — every prompt in Library has non-empty Name and ToMCPPrompt success - TestGPUHealthCheckPrompt — default vs custom node argument - TestDiagnoseXIDErrorsPrompt — default vs custom time range argument - TestGPUTriagePrompt — defaults vs with-incident-id Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Returns the per-node list of GPUs derived from health events, collapsed to the latest event per UUID. Source data is store-client's HealthEventStore (no NVML required, no per-monitor gRPC). Tool wired into mcp.Server.registerTools via a per-tool helper so subsequent tool tasks add one helper each rather than growing registerTools' body. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Per-GPU health summary derived from health events: latest event state plus event-count and unhealthy-event-count aggregates. Optional gpu_uuid narrows the response to a single GPU. Shares the GPU UUID extraction helper with gpu_inventory. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Pairs the latest store event for a node with a flattened Kubernetes Node description (labels, annotations, taints, conditions, GPU capacity/allocatable). Each data source is independently nullable; missing-data conditions surface as structured warnings rather than errors. Nil k8sClient is honoured per Config docs by skipping the K8s portion with a warning. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Pure K8s tool: lists pods requesting GPUs across an optional namespace/node scope, with per-pod request count and UUIDs resolved from NVIDIA_VISIBLE_DEVICES. Rejects nil k8sClient since there is no fallback data source for this tool. Sentinel env values 'all'/'none'/'void' are excluded. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Stitches three data sources for pod diagnosis: K8s Pod phase/restart count, K8s Events scoped to the pod, and NVSentinel store health events that name the pod in entitiesImpacted (matching either bare name or namespace/name). Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Ported from donor pkg/incidents/patterns.go and adapted to NVSentinel's event-only signals: XID codes via ErrorCode and substring matches against Message. Snapshot-derived indicators (temperature, ECC count, throttle reasons, mem utilisation) are deferred until a monitor extension persists that telemetry. Recommendations are kept verbatim. The pure MatchIncidents([]HealthEventWithStatus) []Incident function will back explain_failure (Task 12) and get_incident_report (Task 13). Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Runs the donated pattern matcher (MatchIncidents) over recent store events for a node, optionally narrowed to a GPU UUID and time-bounded by SinceMinutes (default 60). Returns a one-paragraph narrative naming the top pattern plus the full sorted list of matched incidents. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Looks up an analyzer-synthesized event (agent=health-events-analyzer, id=incident_id) via EventsByQuery, then enriches with same-node events in a +/-30 min window. Recommendations come from MatchIncidents over the combined event set. Severity is derived from isFatal/isHealthy (critical/warning/info). Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Working per AUDIT (plan said 'Working OR Stub'; AUDIT confirms XID events are persisted by syslog-health-monitor with the numeric code in ErrorCode). Queries the store via EventsByQuery filtered on errorcode, attributes to nodes/GPUs, and pairs with the matching donor pattern via MatchIncidents. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Per AUDIT § 3, NVSentinel does not persist NVLink topology in its store (the data lives in per-node gpu_metadata.json read by syslog-health-monitor). The tool returns the NVSENTINEL_DATA_GAP envelope from design spec § 6.3 with an inline explanation. tracking_issue is intentionally empty per donor direction (AUDIT § 6); the proposed monitor extension is documented in AUDIT § 6.1. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Working per AUDIT (plan said 'Working OR Stub'; AUDIT confirms generatedTimestamp has nanosecond precision via proto3 google.protobuf.Timestamp). Reads EventsByNode, filters by SinceMinutes window and optional GPUUUID, and returns the timeline in ascending order with severity derived from isFatal/isHealthy. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Replaces the FakeReader placeholder with a real store.DataStoreReader built from store-client's provider registry (env-driven Mongo or Postgres). Adds an in-cluster Kubernetes client; non-cluster runs disable K8s-touching tools per their nil-client contract. --use-fake-store retained for local dev. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
…lse default Subchart at distros/kubernetes/nvsentinel/charts/mcp-server/ per AUDIT § 7 (not mcp-server/deploy/helm/ as the original plan said). Mirrors event-exporter's MongoDB+Postgres dual-mode datastore wiring. ClusterRole grants read on nodes/pods/events (cluster-wide because describe_gpu_node, pod_gpu_allocation, and pod_failure all need it). Default is opt-in: global.mcpServer.enabled: false. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
… E2E steps Replaces the Task 2 stub with a real README: tool catalog (cross-linked to AUDIT.md), helm install quick start, Bearer-token auth config, and explicit 'test against a real GPU cluster' procedure that is the pre-PR acceptance gate (build/push, helm upgrade, port-forward, tools/list, per-tool tools/call smoke). Troubleshooting covers the common k8s-not-configured / FakeReader-only / NVLink-stub paths. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
…r, noctx) Mechanical lint cleanup so CI's modules-lint-test matrix entry for mcp-server passes. Behavior unchanged; tests still pass. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
…ainer-build-test, cleanup-untagged-images) Adds mcp-server to the three matrix-driven workflows that enumerate per-module CI work: modules-lint-test (runs make -C mcp-server lint-test), container-build-test (ko build, amd64+arm64), and cleanup-untagged-images (registry hygiene for nvsentinel/mcp-server). Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
…names Live MCP smoke against the running server caught the donor prompts referencing four tool names that don't exist in this server (get_gpu_inventory -> gpu_inventory, get_gpu_health -> gpu_health, get_pod_gpu_allocation -> pod_gpu_allocation, analyze_xid_errors -> analyze_xid). AI assistants following the prompts would call non-existent tools. Updates the templates and the corresponding test assertions to the canonical NVSentinel tool names from AUDIT.md § 3. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Tag Config.AuthToken with `json:"-"` so it cannot accidentally leak via JSON marshaling. Config is currently a DI boundary and not serialized, but gosec G117 flags the field as matching a secret pattern; the tag makes the safe-by-default contract explicit and silences the linter. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
… auth actually works
The Helm chart injects MCP_AUTH_TOKEN via secretKeyRef when
authToken.secretName is set, but main.go previously only read the
--auth-token flag. The two did not connect, so every chart-deployed
instance served /mcp unauthenticated regardless of the values overlay.
Add resolveAuthToken(): the --auth-token flag wins when set, otherwise
fall back to os.Getenv("MCP_AUTH_TOKEN"). Unit tests cover the four
flag/env combinations. Verified end-to-end with helm template — the
rendered Deployment now wires the secret-sourced env var through to
the running binary.
README updated: correct the /readyz claim (chart probes /healthz for
both liveness and readiness — there is no separate /readyz), explain
the env-var fallback in the Authentication section, and call out the
remaining TLS-flag gap (pkg/mcp/Config.TLS exists but is not exposed
via main.go flags or the chart) as a follow-up.
Found by pre-push principal-engineer review.
Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com>
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
The bearer-auth enforcement path (pkg/mcp/http.go requireBearerAuth) shipped without direct test coverage; gaps surfaced during the pre-push review when verifying the env-var fallback fix end-to-end. Add five focused tests, one per failure mode: - missing Authorization header - non-Bearer scheme (e.g. Basic) - wrong token - empty Bearer token - correct Bearer reaches next handler Each test names a real auth-bypass class. Combined with the existing TestResolveAuthToken (env var fallback) and the helm-template verification that the chart injects MCP_AUTH_TOKEN from the named Secret, the chart \xe2\x86\x92 binary \xe2\x86\x92 enforcement chain is fully covered. Co-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com> Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds the Changesmcp-server: Complete MCP Server Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
Merging this branch will increase overall coverage
Coverage by fileChanged unit test files
|
![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: 13
🧹 Nitpick comments (8)
mcp-server/main.go (2)
194-194: ⚡ Quick winDocument the exported
CreateMetricsServerfunction.Please add a Go doc comment for
CreateMetricsServerto satisfy exported-function documentation requirements.As per coding guidelines "Include function comments for exported Go functions".
🤖 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 `@mcp-server/main.go` at line 194, Add a Go doc comment immediately above the exported CreateMetricsServer function that begins with "CreateMetricsServer" and briefly describes what the function does, its parameters and its return values (e.g., creating and returning a metrics HTTP server on the provided port and an error if creation fails); ensure the comment follows Go doc style (starts with the function name) to satisfy exported-function documentation requirements for CreateMetricsServer.
15-16: ⚡ Quick winAdd package-level godoc for
package main.This package is missing a package doc comment; add a short
// Package main ...comment immediately above the package declaration.As per coding guidelines "Include package-level godoc for all Go packages".
🤖 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 `@mcp-server/main.go` around lines 15 - 16, Add a package-level godoc comment immediately above the package declaration for package main: insert a short comment starting with "// Package main ..." that briefly describes the purpose of the executable (what the program does and its role in the project) so the package has proper documentation for tools like godoc; place it directly above the existing "package main" line.mcp-server/pkg/mcp/http_test.go (1)
49-102: ⚡ Quick winAlign test assertions with the repository’s
testifyconvention.These checks currently use only stdlib
testing; please switch torequire/assertfor consistency with the repo’s Go test guidelines.As per coding guidelines "Use
testify/assertandtestify/requirefor assertions in Go tests".🤖 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 `@mcp-server/pkg/mcp/http_test.go` around lines 49 - 102, Tests in TestRequireBearerAuth_* use stdlib t.Errorf/t.Error instead of the repo standard testify helpers; update each test (TestRequireBearerAuth_MissingHeaderReturns401, _NonBearerSchemeReturns401, _WrongTokenReturns401, _EmptyBearerReturns401, _CorrectBearerReachesNext) to import "github.com/stretchr/testify/require" and replace the status and reached checks with require.Equal/require.True/require.False calls (e.g. require.Equal(t, http.StatusUnauthorized, status) and require.False(t, reached) for cases that must not reach the next handler, and require.Equal(t, http.StatusOK, status) plus require.True(t, reached) for the success case) while keeping runAuthCase and testAuthToken usage unchanged.mcp-server/main_test.go (1)
22-67: ⚡ Quick winUse
require/assertfor these test assertions per repo test style.The scenarios are good; switching to
testify/require+testify/assertwill align this file with the project’s test convention.As per coding guidelines "Use
testify/assertandtestify/requirefor assertions in Go tests".🤖 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 `@mcp-server/main_test.go` around lines 22 - 67, Replace plain t.* checks in TestCreateMetricsServer_InvalidPortReturnsError, TestCreateMetricsServer_ValidPortReturnsServer and TestResolveAuthToken with testify assertions: use require.Error/require.NoError/require.NotNil for fatal prerequisites (e.g., require.Error(t, err) in TestCreateMetricsServer_InvalidPortReturnsError, require.NoError(t, err) and require.NotNil(t, srv) in TestCreateMetricsServer_ValidPortReturnsServer) and use assert.Equal/require.Equal for value comparisons in table-driven TestResolveAuthToken; import "github.com/stretchr/testify/assert" and "github.com/stretchr/testify/require" and remove direct t.Fatal/t.Fatalf/t.Error/t.Errorf usages accordingly.mcp-server/pkg/mcp/server_test.go (1)
32-81: ⚡ Quick winAlign constructor tests to
testifyassertion style.Please replace stdlib assertion patterns here with
require/assertto match the repository’s Go test convention.As per coding guidelines "Use
testify/assertandtestify/requirefor assertions in Go tests".🤖 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 `@mcp-server/pkg/mcp/server_test.go` around lines 32 - 81, Replace the stdlib test assertions in TestNew_RejectsEmptyHTTPAddr, TestNew_RejectsNilStore, and TestNew_ReturnsServerForValidConfig with testify-style calls: import "github.com/stretchr/testify/require" (and "assert" if needed) and use require.Error/require.NoError/require.NotNil and assert.Contains where appropriate (e.g., require.Error(t, err) and assert.Contains(t, err.Error(), "HTTPAddr"/"Store"), require.NoError(t, err) and require.NotNil(t, srv)); remove t.Fatal/t.Fatalf/t.Errorf usage and adjust imports.mcp-server/pkg/prompts/prompts_test.go (1)
27-27: ⚡ Quick winAlign top-level test names to the required naming pattern.
Several test names are descriptive but don’t follow
TestFunctionName_Scenario_ExpectedBehaviorconsistently.Example renames
-func TestPromptDef_RenderTemplate(t *testing.T) { +func TestPromptDef_RenderTemplate_AppliesSubstitutionsAndDefaults(t *testing.T) { -func TestGetAllPromptNames(t *testing.T) { +func TestGetAllPromptNames_LibraryLoaded_ReturnsAllRegisteredNames(t *testing.T) { -func TestLibraryPrompts(t *testing.T) { +func TestLibraryPrompts_EachPromptDefinition_ConvertsAndBuildsHandler(t *testing.T) {As per coding guidelines
**/*_test.go: Name tests descriptively following the patternTestFunctionName_Scenario_ExpectedBehaviorin Go.Also applies to: 45-45, 108-108, 184-184, 207-207, 223-223, 242-242, 261-261, 278-278
🤖 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 `@mcp-server/pkg/prompts/prompts_test.go` at line 27, Rename top-level test functions to follow the Go convention TestFunctionName_Scenario_ExpectedBehavior; for example change TestPromptDef_ToMCPPrompt to a descriptive form like TestPromptDef_ToMCPPrompt_SuccessfulConversion (or another scenario/expected outcome that matches the test), and do the same for the other test functions referenced (the tests at the indicated locations), updating each function name to the Test{Function}_{Scenario}_{ExpectedBehavior} pattern so they are consistent and descriptive.mcp-server/pkg/store/store_test.go (1)
45-133: ⚡ Quick winUse
assert/requirehelpers consistently in this_test.gofile.This file currently mixes direct
t.Fatalf/t.Errorfchecks where repo test style requirestestify/assertandtestify/require.Suggested conversion pattern
+import ( + ... + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) got, err := r.EventsByNode(ctx, "gpu-node-1") -if err != nil { - t.Fatalf("EventsByNode unexpected error: %v", err) -} +require.NoError(t, err) -if len(got) != 2 { - t.Fatalf("want 2 events, got %d", len(got)) -} +require.Len(t, got, 2) -if got[0].RawEvent["checkName"] != "xid" { - t.Errorf("first event checkName = %v, want xid", got[0].RawEvent["checkName"]) -} +assert.Equal(t, "xid", got[0].RawEvent["checkName"])As per coding guidelines
**/*_test.go: Usetestify/assertandtestify/requirefor assertions in Go tests.🤖 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 `@mcp-server/pkg/store/store_test.go` around lines 45 - 133, Replace direct t.Fatalf/t.Errorf checks with testify helpers in the test functions TestFakeReader_EventsByNode, TestFakeReader_EventsByNode_UnknownNodeReturnsEmpty, TestFakeReader_LatestEventForNode_UnknownNodeReturnsErrNotFound, TestFakeReader_LatestEventForNode_ReturnsByCreatedAt, and TestFakeReader_EventsByQuery_ReturnsSeededResultAndRecordsBuilder: use require.NoError(t, err) for error checks, require.Len/require.Empty for slice length expectations, require.Equal/require.True for value comparisons (e.g., CreatedAt equality), and assert.Type/require.Equal for verifying recorded builder type and contents; also replace the err-is check with require.ErrorIs/require.True(errors.Is(...)) or assert.True as appropriate so all assertions follow the repo test style using testify's assert/require helpers.mcp-server/pkg/tools/pod_failure_test.go (1)
165-174: ⚡ Quick winAdd a regression test for nil
store.Readerbehavior.You already test nil
k8sClient; add the sibling case for nilreaderso this path cannot regress into panic again when a pod is node-assigned.🤖 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 `@mcp-server/pkg/tools/pod_failure_test.go` around lines 165 - 174, Add a sibling test that asserts the PodFailure handler returns an error when the store.Reader is nil: create a test like TestPodFailure_NilReader_ReturnsError which constructs the handler with NewPodFailureHandler(nil, fakeK8sClient) and calls h.Handle(context.Background(), tools.PodFailureInput{Pod: "p", Namespace: "ns"}) then require.Error and require.Contains on the error message (matching the existing "store" or similar message used in PodFailureHandler.Handle). This mirrors TestPodFailure_NilK8sClient_ReturnsError and prevents regressions that previously caused a panic when a pod is node-assigned.
🤖 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 `@distros/kubernetes/nvsentinel/charts/mcp-server/values.yaml`:
- Around line 15-30: Add short inline comments to document each top-level
default: explain replicaCount (number of mcp-server pods),
image.repository/image.pullPolicy/image.tag (container image source, pull policy
and tag/default behavior), podAnnotations (optional Pod annotations to be merged
into Pod spec), and resources.limits/resources.requests (CPU and memory limits
and requests) so the values.yaml is self-documenting and follows the chart
guideline; update the block containing replicaCount, image.*, podAnnotations,
and resources.* with one-line comments for each key describing purpose and
expected format/defaults.
In `@distros/kubernetes/nvsentinel/values.yaml`:
- Around line 183-184: Add an inline comment documenting the new Helm global
flag by describing what global.mcpServer.enabled controls, its default value
(false), and typical use-case (e.g., enabling the MCP server for multi-cluster
control plane or integration tests), placed immediately above or on the same
line as the global.mcpServer.enabled entry in values.yaml so it’s discoverable
in the main values file; reference the symbol global.mcpServer.enabled when
adding the short comment.
In `@mcp-server/AUDIT.md`:
- Line 8: Replace the hardcoded machine-local design spec path in the AUDIT.md
table entry ("| **Design spec** |
/Users/.../2026-05-13-merge-gpu-mcp-into-nvsentinel-design.md |") with a
repo-relative reference or a short note pointing to the tracked spec location
(for example a relative path inside docs/superpowers/specs or "See
docs/superpowers/specs/2026-05-13-merge-gpu-mcp-into-nvsentinel-design.md");
update the table cell so it no longer contains any /Users/... absolute path and
instead references the repo path or a pointer to where the spec is stored in the
repository.
In `@mcp-server/pkg/mcp/http.go`:
- Around line 259-276: The current handler uses http.Error which resets
Content-Type to text/plain, breaking the JSON API contract for the 401
responses; update the three places that call http.Error (the "authorization
required", "invalid authorization scheme", and "invalid token" branches) to
write JSON responses directly by setting
w.Header().Set("Content-Type","application/json"), calling
w.WriteHeader(http.StatusUnauthorized), and writing the JSON body (e.g.,
{"error":"..."}) to the response body, or add a small helper like
writeJSONError(w, status, message) and call it from those branches; locate these
changes around the auth/token logic that references auth, token, and
h.authToken.
In `@mcp-server/pkg/prompts/prompts.go`:
- Around line 115-118: The required-argument check in the prompt validation only
verifies presence in the args map and allows empty strings; update the check
around arg.Required so it fetches the value (e.g., v, ok := args[arg.Name]) and
treat missing OR empty-string values as missing (if !ok || v == "" { return nil,
fmt.Errorf("prompt %q: missing required argument %q", p.Name, arg.Name) }).
Ensure you reference arg.Required, args[arg.Name], p.Name and arg.Name in the
same validation block so empty required arguments fail validation.
In `@mcp-server/pkg/tools/analyze_xid.go`:
- Around line 158-169: The function recentEventSummaries builds the summaries
but truncates them before ensuring they are ordered by recency; modify
recentEventSummaries to sort the built slice (out) by the event timestamp in
descending order (newest first) using sort.Slice or sort.SliceStable on the
EventSummary.Timestamp (or the underlying events[*].Timestamp if you prefer) and
only then apply the limit slice operation, so that the returned slice contains
the true most-recent events; keep references to recentEventSummaries and
eventSummaryFromStored when locating the code to change.
In `@mcp-server/pkg/tools/get_incident_report.go`:
- Around line 191-193: The slice variable out is truncated with
getIncidentReportRelatedLimit before being deterministically ordered, causing
nondeterministic matching; sort out by event time (or other deterministic key)
prior to applying the 50-event cap so the truncated set is stable. In
get_incident_report.go, locate the code that builds the out slice (variable out)
and the truncation using getIncidentReportRelatedLimit, then insert a stable
sort (e.g., by timestamp or event_time) of out before the if len(out) >
getIncidentReportRelatedLimit check so downstream matching (used around the
incident matching logic) sees a deterministic, time-ordered subset.
In `@mcp-server/pkg/tools/get_nvlink_topology.go`:
- Around line 81-83: Trim and validate the node input before checking emptiness:
call strings.TrimSpace on in.Node and use the trimmed value for validation so
whitespace-only values are rejected and an error is returned (update the
existing if checking in.Node == "" and the other similar check that validates
in.Node later in the function). Ensure you use the trimmed variable for any
subsequent logic that references the node identifier.
In `@mcp-server/pkg/tools/gpu_health.go`:
- Around line 139-145: The current comparison uses only
ews.CreatedAt.After(entry.LastEventTime) which skips updating when both
timestamps are zero; change the condition in the update block for ews.CreatedAt
vs entry.LastEventTime to also handle zero-value timestamps (for example check
entry.LastEventTime.IsZero() or use a non-strict comparison like
!ews.CreatedAt.Before(entry.LastEventTime)) so that entry.LastEventTime,
entry.Healthy, entry.LastMessage, entry.LastCheck and entry.ErrorCodes are
populated when appropriate; update the block referencing ews.CreatedAt,
entry.LastEventTime, entry.Healthy, he.GetIsHealthy(), he.GetMessage(),
he.GetCheckName(), and he.GetErrorCode() accordingly.
In `@mcp-server/pkg/tools/gpu_inventory.go`:
- Around line 73-75: NewGPUInventoryHandler currently accepts a nil store.Reader
which causes a panic later in Handle when calling h.reader.EventsByNode; update
NewGPUInventoryHandler to check if the incoming reader (r) is nil and return nil
(or an explicit error variant if you prefer changing the signature) instead of
constructing a handler with a nil reader, and update callers of
NewGPUInventoryHandler to handle a nil return (or the new error) accordingly;
reference symbols: NewGPUInventoryHandler, GPUInventoryHandler, reader, and
Handle (which calls EventsByNode).
In `@mcp-server/pkg/tools/pod_failure.go`:
- Around line 78-79: NewPodFailureHandler currently allows a nil store.Reader
which causes a panic when relatedStoreEvents dereferences h.reader (e.g., in
relatedStoreEvents or Handle when node != ""), so add a nil-guard: either
validate and return an error or panic/fail-fast in NewPodFailureHandler if
reader is nil, or (preferable) keep constructor but have relatedStoreEvents and
Handle check h.reader != nil before calling reader methods and log a
warning/degrade behavior when nil. Update NewPodFailureHandler,
PodFailureHandler.relatedStoreEvents (and Handle where it uses reader) to
perform this nil check and handle the nil-reader path safely.
- Around line 151-162: The List call in listPodEvents currently fetches all
namespace events then filters client-side, which is inefficient and misses
enforcing ev.InvolvedObject.Kind == "Pod"; update the ListOptions in
pod_failure.go where evList is fetched (the call using
h.k8sClient.CoreV1().Events(in.Namespace).List) to include a FieldSelector
restricting involvedObject.name=in.Pod, involvedObject.namespace=in.Namespace
and involvedObject.kind=Pod so the server returns only pod events; keep or
simplify the subsequent loop checks (ev.InvolvedObject.Name/Namespace/Kind) as a
safety guard but rely on the server-side selector to reduce load.
In `@mcp-server/README.md`:
- Around line 56-60: The README mixes two Helm values key styles (`mcpServer` vs
`mcp-server`) which is confusing; pick one canonical form (e.g., `mcpServer`)
and update all examples so they use that same key consistently — update the YAML
overlay block that currently shows `mcpServer:` and any `--set` CLI examples
that use `mcp-server` to the chosen form, and do the same for the other
occurrence noted around the later example so all references to the mcp server
values use the identical key name (`mcpServer` or `mcp-server`) throughout the
file.
---
Nitpick comments:
In `@mcp-server/main_test.go`:
- Around line 22-67: Replace plain t.* checks in
TestCreateMetricsServer_InvalidPortReturnsError,
TestCreateMetricsServer_ValidPortReturnsServer and TestResolveAuthToken with
testify assertions: use require.Error/require.NoError/require.NotNil for fatal
prerequisites (e.g., require.Error(t, err) in
TestCreateMetricsServer_InvalidPortReturnsError, require.NoError(t, err) and
require.NotNil(t, srv) in TestCreateMetricsServer_ValidPortReturnsServer) and
use assert.Equal/require.Equal for value comparisons in table-driven
TestResolveAuthToken; import "github.com/stretchr/testify/assert" and
"github.com/stretchr/testify/require" and remove direct
t.Fatal/t.Fatalf/t.Error/t.Errorf usages accordingly.
In `@mcp-server/main.go`:
- Line 194: Add a Go doc comment immediately above the exported
CreateMetricsServer function that begins with "CreateMetricsServer" and briefly
describes what the function does, its parameters and its return values (e.g.,
creating and returning a metrics HTTP server on the provided port and an error
if creation fails); ensure the comment follows Go doc style (starts with the
function name) to satisfy exported-function documentation requirements for
CreateMetricsServer.
- Around line 15-16: Add a package-level godoc comment immediately above the
package declaration for package main: insert a short comment starting with "//
Package main ..." that briefly describes the purpose of the executable (what the
program does and its role in the project) so the package has proper
documentation for tools like godoc; place it directly above the existing
"package main" line.
In `@mcp-server/pkg/mcp/http_test.go`:
- Around line 49-102: Tests in TestRequireBearerAuth_* use stdlib
t.Errorf/t.Error instead of the repo standard testify helpers; update each test
(TestRequireBearerAuth_MissingHeaderReturns401, _NonBearerSchemeReturns401,
_WrongTokenReturns401, _EmptyBearerReturns401, _CorrectBearerReachesNext) to
import "github.com/stretchr/testify/require" and replace the status and reached
checks with require.Equal/require.True/require.False calls (e.g.
require.Equal(t, http.StatusUnauthorized, status) and require.False(t, reached)
for cases that must not reach the next handler, and require.Equal(t,
http.StatusOK, status) plus require.True(t, reached) for the success case) while
keeping runAuthCase and testAuthToken usage unchanged.
In `@mcp-server/pkg/mcp/server_test.go`:
- Around line 32-81: Replace the stdlib test assertions in
TestNew_RejectsEmptyHTTPAddr, TestNew_RejectsNilStore, and
TestNew_ReturnsServerForValidConfig with testify-style calls: import
"github.com/stretchr/testify/require" (and "assert" if needed) and use
require.Error/require.NoError/require.NotNil and assert.Contains where
appropriate (e.g., require.Error(t, err) and assert.Contains(t, err.Error(),
"HTTPAddr"/"Store"), require.NoError(t, err) and require.NotNil(t, srv)); remove
t.Fatal/t.Fatalf/t.Errorf usage and adjust imports.
In `@mcp-server/pkg/prompts/prompts_test.go`:
- Line 27: Rename top-level test functions to follow the Go convention
TestFunctionName_Scenario_ExpectedBehavior; for example change
TestPromptDef_ToMCPPrompt to a descriptive form like
TestPromptDef_ToMCPPrompt_SuccessfulConversion (or another scenario/expected
outcome that matches the test), and do the same for the other test functions
referenced (the tests at the indicated locations), updating each function name
to the Test{Function}_{Scenario}_{ExpectedBehavior} pattern so they are
consistent and descriptive.
In `@mcp-server/pkg/store/store_test.go`:
- Around line 45-133: Replace direct t.Fatalf/t.Errorf checks with testify
helpers in the test functions TestFakeReader_EventsByNode,
TestFakeReader_EventsByNode_UnknownNodeReturnsEmpty,
TestFakeReader_LatestEventForNode_UnknownNodeReturnsErrNotFound,
TestFakeReader_LatestEventForNode_ReturnsByCreatedAt, and
TestFakeReader_EventsByQuery_ReturnsSeededResultAndRecordsBuilder: use
require.NoError(t, err) for error checks, require.Len/require.Empty for slice
length expectations, require.Equal/require.True for value comparisons (e.g.,
CreatedAt equality), and assert.Type/require.Equal for verifying recorded
builder type and contents; also replace the err-is check with
require.ErrorIs/require.True(errors.Is(...)) or assert.True as appropriate so
all assertions follow the repo test style using testify's assert/require
helpers.
In `@mcp-server/pkg/tools/pod_failure_test.go`:
- Around line 165-174: Add a sibling test that asserts the PodFailure handler
returns an error when the store.Reader is nil: create a test like
TestPodFailure_NilReader_ReturnsError which constructs the handler with
NewPodFailureHandler(nil, fakeK8sClient) and calls
h.Handle(context.Background(), tools.PodFailureInput{Pod: "p", Namespace: "ns"})
then require.Error and require.Contains on the error message (matching the
existing "store" or similar message used in PodFailureHandler.Handle). This
mirrors TestPodFailure_NilK8sClient_ReturnsError and prevents regressions that
previously caused a panic when a pod is node-assigned.
🪄 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: 90ed20ec-d23f-40ca-a482-b0cd2e0ca6b3
⛔ Files ignored due to path filters (1)
mcp-server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (56)
.agents/plans/donation-source.md.github/workflows/cleanup-untagged-images.yml.github/workflows/container-build-test.yml.github/workflows/lint-test.yml.ko.yamlMakefiledistros/kubernetes/nvsentinel/Chart.yamldistros/kubernetes/nvsentinel/charts/mcp-server/Chart.yamldistros/kubernetes/nvsentinel/charts/mcp-server/templates/_helpers.tpldistros/kubernetes/nvsentinel/charts/mcp-server/templates/clusterrole.yamldistros/kubernetes/nvsentinel/charts/mcp-server/templates/deployment.yamldistros/kubernetes/nvsentinel/charts/mcp-server/templates/service.yamldistros/kubernetes/nvsentinel/charts/mcp-server/templates/serviceaccount.yamldistros/kubernetes/nvsentinel/charts/mcp-server/values.yamldistros/kubernetes/nvsentinel/values.yamlmcp-server/.gitignoremcp-server/AUDIT.mdmcp-server/Makefilemcp-server/README.mdmcp-server/Tiltfilemcp-server/go.modmcp-server/main.gomcp-server/main_test.gomcp-server/pkg/mcp/http.gomcp-server/pkg/mcp/http_test.gomcp-server/pkg/mcp/metrics.gomcp-server/pkg/mcp/server.gomcp-server/pkg/mcp/server_test.gomcp-server/pkg/mcp/tls.gomcp-server/pkg/prompts/library.gomcp-server/pkg/prompts/prompts.gomcp-server/pkg/prompts/prompts_test.gomcp-server/pkg/store/store.gomcp-server/pkg/store/store_test.gomcp-server/pkg/tools/analyze_xid.gomcp-server/pkg/tools/analyze_xid_test.gomcp-server/pkg/tools/describe_gpu_node.gomcp-server/pkg/tools/describe_gpu_node_test.gomcp-server/pkg/tools/explain_failure.gomcp-server/pkg/tools/explain_failure_test.gomcp-server/pkg/tools/get_gpu_timeline.gomcp-server/pkg/tools/get_gpu_timeline_test.gomcp-server/pkg/tools/get_incident_report.gomcp-server/pkg/tools/get_incident_report_test.gomcp-server/pkg/tools/get_nvlink_topology.gomcp-server/pkg/tools/get_nvlink_topology_test.gomcp-server/pkg/tools/gpu_health.gomcp-server/pkg/tools/gpu_health_test.gomcp-server/pkg/tools/gpu_inventory.gomcp-server/pkg/tools/gpu_inventory_test.gomcp-server/pkg/tools/incidents.gomcp-server/pkg/tools/incidents_test.gomcp-server/pkg/tools/pod_failure.gomcp-server/pkg/tools/pod_failure_test.gomcp-server/pkg/tools/pod_gpu_allocation.gomcp-server/pkg/tools/pod_gpu_allocation_test.go
Comment on lines
+15
to
+30
| replicaCount: 1 | ||
|
|
||
| image: | ||
| repository: ghcr.io/nvidia/nvsentinel/mcp-server | ||
| pullPolicy: IfNotPresent | ||
| tag: "" | ||
|
|
||
| podAnnotations: {} | ||
|
|
||
| resources: | ||
| limits: | ||
| cpu: "500m" | ||
| memory: "512Mi" | ||
| requests: | ||
| cpu: "100m" | ||
| memory: "128Mi" |
Contributor
There was a problem hiding this comment.
Add inline comments for the remaining top-level defaults.
Line 15-Line 30 introduces undocumented values (replicaCount, image.*, podAnnotations, resources.*). Please add brief inline comments for each key to keep this chart compliant and self-documenting.
As per coding guidelines "Document all values in Helm chart values.yaml with inline comments."
🤖 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 `@distros/kubernetes/nvsentinel/charts/mcp-server/values.yaml` around lines 15
- 30, Add short inline comments to document each top-level default: explain
replicaCount (number of mcp-server pods),
image.repository/image.pullPolicy/image.tag (container image source, pull policy
and tag/default behavior), podAnnotations (optional Pod annotations to be merged
into Pod spec), and resources.limits/resources.requests (CPU and memory limits
and requests) so the values.yaml is self-documenting and follows the chart
guideline; update the block containing replicaCount, image.*, podAnnotations,
and resources.* with one-line comments for each key describing purpose and
expected format/defaults.
Comment on lines
+183
to
+184
| mcpServer: | ||
| enabled: false |
Contributor
There was a problem hiding this comment.
Document the new global flag inline.
Please add a short inline comment for global.mcpServer.enabled so this new switch is discoverable in the main values file.
As per coding guidelines "Document all values in Helm chart values.yaml with inline comments."
🤖 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 `@distros/kubernetes/nvsentinel/values.yaml` around lines 183 - 184, Add an
inline comment documenting the new Helm global flag by describing what
global.mcpServer.enabled controls, its default value (false), and typical
use-case (e.g., enabling the MCP server for multi-cluster control plane or
integration tests), placed immediately above or on the same line as the
global.mcpServer.enabled entry in values.yaml so it’s discoverable in the main
values file; reference the symbol global.mcpServer.enabled when adding the short
comment.
| | **Date** | 2026-05-14 | | ||
| | **Auditor** | Claude Code session, working on `feat/mcp-server-merge` | | ||
| | **Donation source** | `ArangoGutierrez/k8s-gpu-mcp-server@80ac33d89ede70aa3f967088f8716d94b8e692e4` (pinned in `.agents/plans/donation-source.md`) | | ||
| | **Design spec** | `/Users/eduardoa/src/github/ArangoGutierrez/k8s-gpu-mcp-server/.worktrees/merge-into-nvsentinel/docs/superpowers/specs/2026-05-13-merge-gpu-mcp-into-nvsentinel-design.md` | |
Contributor
There was a problem hiding this comment.
Replace the machine-local spec path with a repo-relative reference.
Line 8 hardcodes a local /Users/... path, which is not usable by other contributors and leaks workstation-specific info. Prefer a repo-relative path or a short note pointing to the tracked spec location.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: The official name of this software platform is spelled with a capital “H”.
Context: ...ation-source.md) | | **Design spec** | /Users/eduardoa/src/github/ArangoGutierrez/k8s-gpu-mcp-server/.wor...
(GITHUB)
🤖 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 `@mcp-server/AUDIT.md` at line 8, Replace the hardcoded machine-local design
spec path in the AUDIT.md table entry ("| **Design spec** |
/Users/.../2026-05-13-merge-gpu-mcp-into-nvsentinel-design.md |") with a
repo-relative reference or a short note pointing to the tracked spec location
(for example a relative path inside docs/superpowers/specs or "See
docs/superpowers/specs/2026-05-13-merge-gpu-mcp-into-nvsentinel-design.md");
update the table cell so it no longer contains any /Users/... absolute path and
instead references the repo path or a pointer to where the spec is stored in the
repository.
Comment on lines
+259
to
+276
| w.Header().Set("Content-Type", "application/json") | ||
| http.Error(w, `{"error":"authorization required"}`, http.StatusUnauthorized) | ||
|
|
||
| return | ||
| } | ||
|
|
||
| token := strings.TrimPrefix(auth, "Bearer ") | ||
| if token == auth { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| http.Error(w, `{"error":"invalid authorization scheme"}`, http.StatusUnauthorized) | ||
|
|
||
| return | ||
| } | ||
|
|
||
| if subtle.ConstantTimeCompare([]byte(token), []byte(h.authToken)) != 1 { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| http.Error(w, `{"error":"invalid token"}`, http.StatusUnauthorized) | ||
|
|
Contributor
There was a problem hiding this comment.
Return real JSON errors instead of http.Error text responses.
http.Error overwrites Content-Type to text/plain, so these 401 responses are not actually JSON despite JSON-looking strings. Write status/body directly (or via a small helper) to keep the API contract consistent.
💡 Suggested fix
+func writeJSONError(w http.ResponseWriter, code int, body string) {
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(code)
+ _, _ = w.Write([]byte(body))
+}
+
func (h *HTTPServer) requireBearerAuth(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
auth := r.Header.Get("Authorization")
if auth == "" {
- w.Header().Set("Content-Type", "application/json")
- http.Error(w, `{"error":"authorization required"}`, http.StatusUnauthorized)
-
+ writeJSONError(w, http.StatusUnauthorized, `{"error":"authorization required"}`)
return
}
@@
token := strings.TrimPrefix(auth, "Bearer ")
if token == auth {
- w.Header().Set("Content-Type", "application/json")
- http.Error(w, `{"error":"invalid authorization scheme"}`, http.StatusUnauthorized)
-
+ writeJSONError(w, http.StatusUnauthorized, `{"error":"invalid authorization scheme"}`)
return
}
@@
if subtle.ConstantTimeCompare([]byte(token), []byte(h.authToken)) != 1 {
- w.Header().Set("Content-Type", "application/json")
- http.Error(w, `{"error":"invalid token"}`, http.StatusUnauthorized)
-
+ writeJSONError(w, http.StatusUnauthorized, `{"error":"invalid token"}`)
return
}📝 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
| w.Header().Set("Content-Type", "application/json") | |
| http.Error(w, `{"error":"authorization required"}`, http.StatusUnauthorized) | |
| return | |
| } | |
| token := strings.TrimPrefix(auth, "Bearer ") | |
| if token == auth { | |
| w.Header().Set("Content-Type", "application/json") | |
| http.Error(w, `{"error":"invalid authorization scheme"}`, http.StatusUnauthorized) | |
| return | |
| } | |
| if subtle.ConstantTimeCompare([]byte(token), []byte(h.authToken)) != 1 { | |
| w.Header().Set("Content-Type", "application/json") | |
| http.Error(w, `{"error":"invalid token"}`, http.StatusUnauthorized) | |
| func writeJSONError(w http.ResponseWriter, code int, body string) { | |
| w.Header().Set("Content-Type", "application/json") | |
| w.WriteHeader(code) | |
| _, _ = w.Write([]byte(body)) | |
| } | |
| func (h *HTTPServer) requireBearerAuth(next http.Handler) http.Handler { | |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| auth := r.Header.Get("Authorization") | |
| if auth == "" { | |
| writeJSONError(w, http.StatusUnauthorized, `{"error":"authorization required"}`) | |
| return | |
| } | |
| token := strings.TrimPrefix(auth, "Bearer ") | |
| if token == auth { | |
| writeJSONError(w, http.StatusUnauthorized, `{"error":"invalid authorization scheme"}`) | |
| return | |
| } | |
| if subtle.ConstantTimeCompare([]byte(token), []byte(h.authToken)) != 1 { | |
| writeJSONError(w, http.StatusUnauthorized, `{"error":"invalid token"}`) | |
| return | |
| } |
🤖 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 `@mcp-server/pkg/mcp/http.go` around lines 259 - 276, The current handler uses
http.Error which resets Content-Type to text/plain, breaking the JSON API
contract for the 401 responses; update the three places that call http.Error
(the "authorization required", "invalid authorization scheme", and "invalid
token" branches) to write JSON responses directly by setting
w.Header().Set("Content-Type","application/json"), calling
w.WriteHeader(http.StatusUnauthorized), and writing the JSON body (e.g.,
{"error":"..."}) to the response body, or add a small helper like
writeJSONError(w, status, message) and call it from those branches; locate these
changes around the auth/token logic that references auth, token, and
h.authToken.
Comment on lines
+115
to
+118
| if arg.Required { | ||
| if _, ok := args[arg.Name]; !ok { | ||
| return nil, fmt.Errorf("prompt %q: missing required argument %q", p.Name, arg.Name) | ||
| } |
Contributor
There was a problem hiding this comment.
Treat empty required arguments as missing.
A required argument currently passes validation when present but empty (""), which can silently produce invalid prompt content.
Proposed fix
for _, arg := range p.Arguments {
if arg.Required {
- if _, ok := args[arg.Name]; !ok {
+ value, ok := args[arg.Name]
+ if !ok || strings.TrimSpace(value) == "" {
return nil, fmt.Errorf("prompt %q: missing required argument %q", p.Name, arg.Name)
}
}
}📝 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
| if arg.Required { | |
| if _, ok := args[arg.Name]; !ok { | |
| return nil, fmt.Errorf("prompt %q: missing required argument %q", p.Name, arg.Name) | |
| } | |
| for _, arg := range p.Arguments { | |
| if arg.Required { | |
| value, ok := args[arg.Name] | |
| if !ok || strings.TrimSpace(value) == "" { | |
| return nil, fmt.Errorf("prompt %q: missing required argument %q", p.Name, arg.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 `@mcp-server/pkg/prompts/prompts.go` around lines 115 - 118, The
required-argument check in the prompt validation only verifies presence in the
args map and allows empty strings; update the check around arg.Required so it
fetches the value (e.g., v, ok := args[arg.Name]) and treat missing OR
empty-string values as missing (if !ok || v == "" { return nil,
fmt.Errorf("prompt %q: missing required argument %q", p.Name, arg.Name) }).
Ensure you reference arg.Required, args[arg.Name], p.Name and arg.Name in the
same validation block so empty required arguments fail validation.
Comment on lines
+139
to
+145
| if ews.CreatedAt.After(entry.LastEventTime) { | ||
| entry.LastEventTime = ews.CreatedAt | ||
| entry.Healthy = he.GetIsHealthy() | ||
| entry.LastMessage = he.GetMessage() | ||
| entry.LastCheck = he.GetCheckName() | ||
| entry.ErrorCodes = append([]string{}, he.GetErrorCode()...) | ||
| } |
Contributor
There was a problem hiding this comment.
Handle zero-value timestamps when selecting latest event.
On Line 139, using only After can skip population when entry.LastEventTime is zero and incoming CreatedAt is also zero, leaving Healthy/LastMessage/LastCheck at defaults.
Suggested fix
- if ews.CreatedAt.After(entry.LastEventTime) {
+ if entry.LastEventTime.IsZero() || ews.CreatedAt.After(entry.LastEventTime) {
entry.LastEventTime = ews.CreatedAt
entry.Healthy = he.GetIsHealthy()
entry.LastMessage = he.GetMessage()
entry.LastCheck = he.GetCheckName()
entry.ErrorCodes = append([]string{}, he.GetErrorCode()...)
}📝 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
| if ews.CreatedAt.After(entry.LastEventTime) { | |
| entry.LastEventTime = ews.CreatedAt | |
| entry.Healthy = he.GetIsHealthy() | |
| entry.LastMessage = he.GetMessage() | |
| entry.LastCheck = he.GetCheckName() | |
| entry.ErrorCodes = append([]string{}, he.GetErrorCode()...) | |
| } | |
| if entry.LastEventTime.IsZero() || ews.CreatedAt.After(entry.LastEventTime) { | |
| entry.LastEventTime = ews.CreatedAt | |
| entry.Healthy = he.GetIsHealthy() | |
| entry.LastMessage = he.GetMessage() | |
| entry.LastCheck = he.GetCheckName() | |
| entry.ErrorCodes = append([]string{}, he.GetErrorCode()...) | |
| } |
🤖 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 `@mcp-server/pkg/tools/gpu_health.go` around lines 139 - 145, The current
comparison uses only ews.CreatedAt.After(entry.LastEventTime) which skips
updating when both timestamps are zero; change the condition in the update block
for ews.CreatedAt vs entry.LastEventTime to also handle zero-value timestamps
(for example check entry.LastEventTime.IsZero() or use a non-strict comparison
like !ews.CreatedAt.Before(entry.LastEventTime)) so that entry.LastEventTime,
entry.Healthy, entry.LastMessage, entry.LastCheck and entry.ErrorCodes are
populated when appropriate; update the block referencing ews.CreatedAt,
entry.LastEventTime, entry.Healthy, he.GetIsHealthy(), he.GetMessage(),
he.GetCheckName(), and he.GetErrorCode() accordingly.
Comment on lines
+73
to
+75
| func NewGPUInventoryHandler(r store.Reader) *GPUInventoryHandler { | ||
| return &GPUInventoryHandler{reader: r} | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against nil store.Reader to avoid panic on Line 86.
NewGPUInventoryHandler currently allows a nil dependency, which can crash at runtime when Handle calls h.reader.EventsByNode(...).
Suggested fix
func NewGPUInventoryHandler(r store.Reader) *GPUInventoryHandler {
- return &GPUInventoryHandler{reader: r}
+ return &GPUInventoryHandler{reader: r}
}
func (h *GPUInventoryHandler) Handle(ctx context.Context, in GPUInventoryInput) (GPUInventoryOutput, error) {
+ if h == nil || h.reader == nil {
+ return GPUInventoryOutput{}, errors.New("gpu_inventory: reader is not configured")
+ }
if in.Node == "" {
return GPUInventoryOutput{}, errors.New("gpu_inventory: node is required")
}🤖 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 `@mcp-server/pkg/tools/gpu_inventory.go` around lines 73 - 75,
NewGPUInventoryHandler currently accepts a nil store.Reader which causes a panic
later in Handle when calling h.reader.EventsByNode; update
NewGPUInventoryHandler to check if the incoming reader (r) is nil and return nil
(or an explicit error variant if you prefer changing the signature) instead of
constructing a handler with a nil reader, and update callers of
NewGPUInventoryHandler to handle a nil return (or the new error) accordingly;
reference symbols: NewGPUInventoryHandler, GPUInventoryHandler, reader, and
Handle (which calls EventsByNode).
Comment on lines
+78
to
+79
| func NewPodFailureHandler(r store.Reader, k kubernetes.Interface) *PodFailureHandler { | ||
| return &PodFailureHandler{reader: r, k8sClient: k} |
Contributor
There was a problem hiding this comment.
Guard reader to prevent nil dereference in store-event path.
NewPodFailureHandler accepts a nil store.Reader, but relatedStoreEvents dereferences it when node != "", which can panic requests. Add a nil-reader guard and degrade to warning (or fail fast at constructor/Handle).
💡 Suggested fix
func (h *PodFailureHandler) relatedStoreEvents(
ctx context.Context, in PodFailureInput, node string,
) ([]EventSummary, string, error) {
if node == "" {
return nil, "pod has no assigned node; skipping store events", nil
}
+ if h.reader == nil {
+ return nil, "store reader not configured; skipping store events", nil
+ }
events, err := h.reader.EventsByNode(ctx, node)
if err != nil {
return nil, "", fmt.Errorf("pod_failure: events by node: %w", err)
}Also applies to: 184-187
🤖 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 `@mcp-server/pkg/tools/pod_failure.go` around lines 78 - 79,
NewPodFailureHandler currently allows a nil store.Reader which causes a panic
when relatedStoreEvents dereferences h.reader (e.g., in relatedStoreEvents or
Handle when node != ""), so add a nil-guard: either validate and return an error
or panic/fail-fast in NewPodFailureHandler if reader is nil, or (preferable)
keep constructor but have relatedStoreEvents and Handle check h.reader != nil
before calling reader methods and log a warning/degrade behavior when nil.
Update NewPodFailureHandler, PodFailureHandler.relatedStoreEvents (and Handle
where it uses reader) to perform this nil check and handle the nil-reader path
safely.
Comment on lines
+151
to
+162
| evList, err := h.k8sClient.CoreV1().Events(in.Namespace).List(ctx, metav1.ListOptions{}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("pod_failure: list events: %w", err) | ||
| } | ||
|
|
||
| out := make([]K8sEventSummary, 0) | ||
|
|
||
| for i := range evList.Items { | ||
| ev := &evList.Items[i] | ||
| if ev.InvolvedObject.Name != in.Pod || ev.InvolvedObject.Namespace != in.Namespace { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current implementation lists namespace-wide events with empty ListOptions
# and does not filter by InvolvedObject.Kind.
rg -n -C3 'Events\(in\.Namespace\)\.List\(ctx,\s*metav1\.ListOptions\{\}\)' mcp-server/pkg/tools/pod_failure.go
rg -n -C3 'InvolvedObject\.Kind' mcp-server/pkg/tools/pod_failure.goRepository: NVIDIA/NVSentinel
Length of output: 376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant portion around listPodEvents
nl -ba mcp-server/pkg/tools/pod_failure.go | sed -n '120,220p'
# Confirm whether InvolvedObject.Kind is referenced anywhere in this file
rg -n 'InvolvedObject\.Kind' mcp-server/pkg/tools/pod_failure.go || true
# Confirm whether any FieldSelector is already used for Events List
rg -n 'FieldSelector' mcp-server/pkg/tools/pod_failure.go || trueRepository: NVIDIA/NVSentinel
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant portion around listPodEvents
nl -ba mcp-server/pkg/tools/pod_failure.go | sed -n '120,220p'
# Confirm whether InvolvedObject.Kind is referenced anywhere in this file
rg -n 'InvolvedObject\.Kind' mcp-server/pkg/tools/pod_failure.go || true
# Confirm whether any FieldSelector is already used for Events List
rg -n 'FieldSelector' mcp-server/pkg/tools/pod_failure.go || trueRepository: NVIDIA/NVSentinel
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant portion around listPodEvents (using cat -n since nl is unavailable)
cat -n mcp-server/pkg/tools/pod_failure.go | sed -n '120,220p'
# Confirm whether InvolvedObject.Kind is referenced anywhere in this file
rg -n 'InvolvedObject\.Kind' mcp-server/pkg/tools/pod_failure.go || true
# Confirm whether any FieldSelector is already used for Events List
rg -n 'FieldSelector' mcp-server/pkg/tools/pod_failure.go || trueRepository: NVIDIA/NVSentinel
Length of output: 3224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore -S 'InvolvedObject\.Kind' mcp-server/pkg/tools/pod_failure.go || true
rg -n --hidden --no-ignore -S 'ListOptions\{[^}]*FieldSelector' mcp-server/pkg/tools/pod_failure.go || trueRepository: NVIDIA/NVSentinel
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find imports block
sed -n '1,120p' mcp-server/pkg/tools/pod_failure.go
# Check for existing sort usage in this file
rg -n '\bsort\.' mcp-server/pkg/tools/pod_failure.go || true
# Find K8sEventSummary definition and check its fields for timestamp naming
rg -n 'type K8sEventSummary' -n mcp-server/pkg/tools/pod_failure.go
# show around definition
line=$(rg -n 'type K8sEventSummary' mcp-server/pkg/tools/pod_failure.go | head -n1 | cut -d: -f1)
start=$((line-20))
end=$((line+80))
sed -n "${start},${end}p" mcp-server/pkg/tools/pod_failure.goRepository: NVIDIA/NVSentinel
Length of output: 8158
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find imports block
sed -n '1,120p' mcp-server/pkg/tools/pod_failure.go
# Check for existing sort usage in this file
rg -n '\bsort\.' mcp-server/pkg/tools/pod_failure.go || true
# Find K8sEventSummary definition and check its fields for timestamp naming
rg -n 'type K8sEventSummary' mcp-server/pkg/tools/pod_failure.go
line=$(rg -n 'type K8sEventSummary' mcp-server/pkg/tools/pod_failure.go | head -n1 | cut -d: -f1)
start=$((line-20))
end=$((line+80))
sed -n "${start},${end}p" mcp-server/pkg/tools/pod_failure.goRepository: NVIDIA/NVSentinel
Length of output: 8158
Filter Events server-side and constrain to Pod-kind events.
listPodEvents lists all namespace events via Events(in.Namespace).List(ctx, metav1.ListOptions{}) and then filters only by ev.InvolvedObject.Name/ev.InvolvedObject.Namespace; it does not enforce ev.InvolvedObject.Kind == "Pod" anywhere, so non-Pod events can slip through and the list call is needlessly expensive.
💡 Suggested fix
import (
"context"
"errors"
"fmt"
+ "sort"
"strings"
"time"
@@
func (h *PodFailureHandler) listPodEvents(ctx context.Context, in PodFailureInput) ([]K8sEventSummary, error) {
- evList, err := h.k8sClient.CoreV1().Events(in.Namespace).List(ctx, metav1.ListOptions{})
+ evList, err := h.k8sClient.CoreV1().Events(in.Namespace).List(ctx, metav1.ListOptions{
+ FieldSelector: "involvedObject.kind=Pod,involvedObject.name=" + in.Pod,
+ })
if err != nil {
return nil, fmt.Errorf("pod_failure: list events: %w", err)
}
@@
for i := range evList.Items {
ev := &evList.Items[i]
- if ev.InvolvedObject.Name != in.Pod || ev.InvolvedObject.Namespace != in.Namespace {
+ if ev.InvolvedObject.Kind != "Pod" ||
+ ev.InvolvedObject.Name != in.Pod ||
+ ev.InvolvedObject.Namespace != in.Namespace {
continue
}
@@
}
+ sort.Slice(out, func(i, j int) bool {
+ return out[i].LastTimestamp.After(out[j].LastTimestamp)
+ })
return out, nil
}📝 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
| evList, err := h.k8sClient.CoreV1().Events(in.Namespace).List(ctx, metav1.ListOptions{}) | |
| if err != nil { | |
| return nil, fmt.Errorf("pod_failure: list events: %w", err) | |
| } | |
| out := make([]K8sEventSummary, 0) | |
| for i := range evList.Items { | |
| ev := &evList.Items[i] | |
| if ev.InvolvedObject.Name != in.Pod || ev.InvolvedObject.Namespace != in.Namespace { | |
| continue | |
| } | |
| evList, err := h.k8sClient.CoreV1().Events(in.Namespace).List(ctx, metav1.ListOptions{ | |
| FieldSelector: "involvedObject.kind=Pod,involvedObject.name=" + in.Pod, | |
| }) | |
| if err != nil { | |
| return nil, fmt.Errorf("pod_failure: list events: %w", err) | |
| } | |
| out := make([]K8sEventSummary, 0) | |
| for i := range evList.Items { | |
| ev := &evList.Items[i] | |
| if ev.InvolvedObject.Kind != "Pod" || | |
| ev.InvolvedObject.Name != in.Pod || | |
| ev.InvolvedObject.Namespace != in.Namespace { | |
| continue | |
| } | |
| // ... rest of loop body | |
| } | |
| sort.Slice(out, func(i, j int) bool { | |
| return out[i].LastTimestamp.After(out[j].LastTimestamp) | |
| }) | |
| return out, nil |
🤖 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 `@mcp-server/pkg/tools/pod_failure.go` around lines 151 - 162, The List call in
listPodEvents currently fetches all namespace events then filters client-side,
which is inefficient and misses enforcing ev.InvolvedObject.Kind == "Pod";
update the ListOptions in pod_failure.go where evList is fetched (the call using
h.k8sClient.CoreV1().Events(in.Namespace).List) to include a FieldSelector
restricting involvedObject.name=in.Pod, involvedObject.namespace=in.Namespace
and involvedObject.kind=Pod so the server returns only pod events; keep or
simplify the subsequent loop checks (ev.InvolvedObject.Name/Namespace/Kind) as a
safety guard but rely on the server-side selector to reduce load.
Comment on lines
+56
to
+60
| mcpServer: | ||
| authToken: | ||
| secretName: mcp-server-auth | ||
| secretKey: token | ||
| ``` |
Contributor
There was a problem hiding this comment.
Unify Helm values key notation across examples.
This README mixes mcpServer and mcp-server for value paths, which can mislead users during install/override. Please standardize to one canonical key style and use it consistently in both YAML overlays and --set examples.
Also applies to: 95-100
🤖 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 `@mcp-server/README.md` around lines 56 - 60, The README mixes two Helm values
key styles (`mcpServer` vs `mcp-server`) which is confusing; pick one canonical
form (e.g., `mcpServer`) and update all examples so they use that same key
consistently — update the YAML overlay block that currently shows `mcpServer:`
and any `--set` CLI examples that use `mcp-server` to the chosen form, and do
the same for the other occurrence noted around the later example so all
references to the mcp server values use the identical key name (`mcpServer` or
`mcp-server`) throughout the file.
Contributor
|
@ArangoGutierrez this PR has been inactive for 14 days. Do you need help finishing it, or should we close it for now? Feel free to reopen anytime. |
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.
feat(mcp-server): merge donation of k8s-gpu-mcp-server
Summary
Adds the
mcp-servercomponent to NVSentinel — a Model Context Protocol (MCP) server that exposes NVSentinel's read-only health surface (HealthEventStore + Kubernetes API) as MCP tools, ported from the upstream donationArangoGutierrez/k8s-gpu-mcp-serverat SHA80ac33d89ede70aa3f967088f8716d94b8e692e4.The new component is gated off by default (
global.mcpServer.enabled=false); existing deployments are unaffected.Tools (10 total)
9 Working tools backed by
store-client/pkg/datastore.HealthEventStoreand (where needed) the Kubernetes API:gpu_inventorygpu_healthdescribe_gpu_nodepod_gpu_allocationpod_failureexplain_failureget_incident_reportanalyze_xidget_gpu_timeline1 Stub tool —
get_nvlink_topology. NVSentinel does not yet persist per-node NVLink topology in its store. The stub returns theNVSENTINEL_DATA_GAPenvelope with aneeded_monitor_extensionfield explaining the gap inline (seemcp-server/AUDIT.md § 6.1).Architectural decisions (carried forward from donation audit)
pkg/monitors/from the donor was dropped — NVSentinel has no monitor-side read gRPC; only thePlatformConnector.HealthEventOccurredV1write ingress exists.github.com/nvidia/nvsentinel/mcp-server(lowercase, sibling-aligned).event-exporterstructure.commons/pkg/{logger,server,metrics,tracing}everywhere; donor'sklogdependency dropped.distros/kubernetes/nvsentinel/charts/mcp-server/(notmcp-server/deploy/helm/)./mcp, not/metrics. Renamed donor'sSetMetricsAuthToken→SetAuthToken.Config.AuthTokentaggedjson:"-"to silence gosec G117 and prevent accidental leakage via marshaling.main.goreadsMCP_AUTH_TOKENenv var as a fallback when--auth-tokenis empty. The Helm chart injects this env var fromauthToken.secretName; the fallback closes the chart→binary path so bearer auth on/mcpactually engages when the operator wires up a Secret. End-to-end chain verified byhelm templaterendering of the deployment plus unit tests covering the four flag/env combinations.pkg/incidentspattern matcher ported as a tool helper; runs at query time (no pre-computed incident object storage).See
mcp-server/AUDIT.mdfor the full audit of NVSentinel's read surface vs the donor's expectations.Verification
Run at HEAD
f2806679from worktree.worktrees/mcp-server-merge.make -C mcp-server buildmake -C mcp-server testmake -C mcp-server lint(golangci-lint v2.12.2)lint-test(vet + lint + test + cover)make -C mcp-server lint-testgovulncheckgovulncheck ./mcp-server/...trivy image --severity CRITICAL,HIGHonko buildoutput (chainguard/static base)t3.xlargeubuntu-24.04 +helm installwithglobal.mcpServer.enabled=true(mcp-server-only deployment,--use-fake-store)/healthzreturnsokkubectl port-forward+ JSON-RPCtools/listandtools/callagainst all 10 toolstools/listreturns 10 tools; all 9 Working tools return structuredsuccessenvelopes; Stub returns the documentedNVSENTINEL_DATA_GAPenvelopeg4dn.xlarge(Tesla T4) with NVIDIA driver 580.159.04 + GPU Operator (DCGM hostengine + device-plugin) + mcp-server via the chartdescribe_gpu_nodereturned real GPU metadata (Tesla-T4,15360MiB,g4dn.xlarge, CUDA driver580.159.04);pod_gpu_allocationcorrectly listed a test pod requestingnvidia.com/gpu: 1with{node, namespace, pod, requested:1}TestResolveAuthToken+helm templateshowing chart-injectedMCP_AUTH_TOKENenv var + unitTestRequireBearerAuth_*(5 cases: missing/non-Bearer/wrong token/empty Bearer/correct Bearer)Config.AuthToken→requireBearerAuthenforcement chain coveredDocumented deviations from the plan's AC gate
get_nvlink_topologyin this PR, per donor direction (2026-05-14). The stub'sneeded_monitor_extensionenvelope explains the gap inline and referencesAUDIT.md § 6.1, which contains a ready-to-file issue body for maintainers to use if requested.pre-commit run --all-files): N/A. The repository does not maintain a.pre-commit-config.yaml;make lint-test(vet + golangci-lint + gotestsum + coverage) is the equivalent gate and is run above.Cluster smoke setup (reproducer)
Protocol smoke (CPU cluster): single-node Kubernetes v1.31.1 via holodeck on AWS (
t3.xlarge,us-west-2, ubuntu-24.04, no GPU). Image built locally withko --local, sideloaded viactr -n k8s.io image import, helm installed withglobal.mcpServer.enabled=trueplus all other components disabled, deployment patched to add--use-fake-store. All 10 MCP tools returned the expected envelopes viakubectl port-forward+ curl JSON-RPC.Real-GPU smoke: single-node Kubernetes v1.31.1 via holodeck on AWS (
g4dn.xlarge,us-east-1, ubuntu-24.04) with NVIDIA driver 580.159.04 (Tesla T4) + NVIDIA Container Toolkit installed by holodeck. NVIDIA GPU Operator v25.3.0 deployed via helm (withdriver.enabled=false toolkit.enabled=false dcgm.enabled=true) to provide the DCGM hostengine + device-plugin + node feature discovery. mcp-server deployed via the NVSentinel chart (global.mcpServer.enabled=true, fake store). Created a CUDA test pod requestingnvidia.com/gpu: 1. Hitdescribe_gpu_nodeandpod_gpu_allocationvia curl — both returned real GPU metadata and real pod allocation data from the cluster's K8s API.Full monitor-chain validation was attempted and is documented as a known gap: loading
global.gpuHealthMonitor.enabled=true+mongodbStore.enabled=true+eventExporter.enabled=truein the umbrella values failed cleanly with two infra dependencies absent from a vanilla cluster: (1)ghcr.io/nvidia/nvsentinel/{labeler,event-exporter,gpu-health-monitor,platform-connectors}:mainreturn 401 — those component images are not public; (2) the bitnami MongoDB subchart wants a default StorageClass which holodeck's single-node K8s does not provide. Neither is something a donation PR formcp-servershould fix; both belong in a follow-up PR that wires up a reproducible full-stack integration env. The protocol+real-GPU smoke above is sufficient to assert thatmcp-serveritself is fit for review.CI
GitHub Actions matrices wired in commit
229c871f:lint-testmatrix includesmcp-servercontainer-build-testmatrix includesmcp-servercleanup-untagged-imagesmatrix includesmcp-serverFirst push of this branch will exercise CI against the new component.
Follow-up issues to file post-merge
get_nvlink_topologyreal implementation (needs monitor extension — body drafted inAUDIT.md § 6.1)./mcp—pkg/mcp/Config.TLSexists butmain.godoes not yet expose--tls-cert/--tls-keyflags and the chart does not mount a cert; documented in the README.ghcr.io/nvidia/nvsentinel/{labeler,event-exporter,gpu-health-monitor,platform-connectors}images, and ship a values overlay that picks a workable StorageClass for the mongodb subchart on single-node K8s.Co-authorship
All commits are signed (
-s -S) and includeCo-authored-by: Carlos Arango Gutierrez <eduardoa@nvidia.com>per the donation merge spec § 9.4.Summary by CodeRabbit
New Features
Infrastructure