From 4651beb4c8430d05e2fa00ed7194cfa485d27a85 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Wed, 21 Oct 2026 19:33:09 +0100 Subject: [PATCH] Preserve boolean MCP fleet filters through HTTP projection The canonical fleet tool advertises hasFindings as boolean, but GET/DELETE projection silently dropped decoded bool values. Encode true and false without broadening unsupported argument types or changing string, path, private metadata, auth or write-body behaviour. Exercise actual MCP stdio, manifest discovery and HTTP transport against the production fleet handler with safe resource/finding fixtures. Keep false and omitted unfiltered, compose dimensions, and retain text/structured output parity. Reported-by: X1pheR (Pulse PR #3097) Change-source: pulse-maintainer --- cmd/pulse-mcp/main_test.go | 189 ++++++++++++++++++ .../v6/internal/subsystems/ai-runtime.md | 18 ++ .../v6/internal/subsystems/api-contracts.md | 18 ++ internal/agentcapabilities/http_test.go | 75 +++---- internal/agentcapabilities/projection.go | 11 +- internal/agentcapabilities/projection_test.go | 57 ++++++ 6 files changed, 324 insertions(+), 44 deletions(-) diff --git a/cmd/pulse-mcp/main_test.go b/cmd/pulse-mcp/main_test.go index c874568a21..859bf58aa7 100644 --- a/cmd/pulse-mcp/main_test.go +++ b/cmd/pulse-mcp/main_test.go @@ -8,11 +8,18 @@ import ( "net/http" "net/http/httptest" "os" + "reflect" + "sort" "strings" "sync" "testing" + "time" "github.com/rcourtman/pulse-go-rewrite/internal/agentcapabilities" + "github.com/rcourtman/pulse-go-rewrite/internal/api" + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + unified "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" ) type httpDoerFunc func(*http.Request) (*http.Response, error) @@ -1491,3 +1498,185 @@ func TestServer_ToolsCallTopLevelArgsExcludesPathPlaceholders(t *testing.T) { t.Errorf("non-placeholder fields must reach the body; got %v", sent) } } + +type mcpFleetSnapshot struct{ resources []unified.Resource } + +func (s mcpFleetSnapshot) ReadSnapshot() models.StateSnapshot { + return models.StateSnapshot{LastUpdate: time.Now()} +} +func (s mcpFleetSnapshot) UnifiedResourceSnapshot() ([]unified.Resource, time.Time) { + return s.resources, time.Now() +} + +type mcpFleetFindings map[string][]api.AgentResourceFindingSnapshot + +func (f mcpFleetFindings) ActiveFindingsForResource(id string) []api.AgentResourceFindingSnapshot { + return f[id] +} +func (f mcpFleetFindings) ActiveFindingCount() int { + count := 0 + for _, findings := range f { + count += len(findings) + } + return count +} + +// Exercise the real stdio decoder, published schema, HTTP projection, fleet +// handler and MCP stdout together. Only collected resources/findings are fixtures; +// the backend must not reimplement the production filter in a test double. +func TestServer_FleetBooleanFiltersReachRealHandler(t *testing.T) { + all := []string{"container:info", "container:warning", "vm:critical", "vm:healthy"} + cases := []struct { + name, args, query string + ids []string + }{ + {"true", `{"hasFindings":true}`, "hasFindings=true", []string{"container:info", "container:warning", "vm:critical"}}, + {"false_is_unfiltered", `{"hasFindings":false}`, "hasFindings=false", all}, + {"omitted_is_unfiltered", `{}`, "", all}, + {"true_and_technology", `{"hasFindings":true,"technology":"qemu"}`, "hasFindings=true&technology=qemu", []string{"vm:critical"}}, + {"false_and_technology", `{"hasFindings":false,"technology":"qemu"}`, "hasFindings=false&technology=qemu", []string{"vm:critical", "vm:healthy"}}, + {"true_and_severity", `{"hasFindings":true,"severity":"warning"}`, "hasFindings=true&severity=warning", []string{"container:warning"}}, + {"empty_intersection", `{"hasFindings":true,"technology":"qemu","severity":"warning"}`, "hasFindings=true&severity=warning&technology=qemu", []string{}}, + // Retain old string transport compatibility, without recommending an + // out-of-schema client workaround for the advertised boolean. + {"legacy_string", `{"hasFindings":"true"}`, "hasFindings=true", []string{"container:info", "container:warning", "vm:critical"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resources := api.NewResourceHandlers(&config.Config{DataPath: t.TempDir()}) + resources.SetStateProvider(mcpFleetSnapshot{resources: []unified.Resource{ + {ID: "vm:healthy", Type: "vm", Name: "healthy", Technology: "qemu"}, + {ID: "vm:critical", Type: "vm", Name: "critical", Technology: "qemu"}, + {ID: "container:warning", Type: "container", Name: "warning", Technology: "docker"}, + {ID: "container:info", Type: "system-container", Name: "info", Technology: "lxc"}, + }}) + handler := api.NewAgentContextHandler(resources) + handler.SetFindingsProvider(mcpFleetFindings{ + "vm:critical": {{ID: "fixture-critical", Severity: "critical"}}, + "container:warning": {{ID: "fixture-warning", Severity: "warning"}}, + "container:info": {{ID: "fixture-info", Severity: "info"}}, + }) + manifest := agentcapabilities.CanonicalManifest() + calls := make(chan string, 2) + backend := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == agentcapabilities.AgentCapabilitiesPath { + if r.Header.Get(agentcapabilities.AgentAPITokenHeader) != "" { + t.Error("discovery unexpectedly sent a token") + } + _ = json.NewEncoder(w).Encode(manifest) + return + } + if r.URL.Path != agentcapabilities.FleetContextCapabilityPath || r.Method != http.MethodGet { + t.Errorf("unexpected fleet request %s %s", r.Method, r.URL.Path) + http.Error(w, "unexpected request", http.StatusNotFound) + return + } + calls <- r.URL.RawQuery + if r.Header.Get(agentcapabilities.AgentAPITokenHeader) != "fixture-token" || r.Header.Get(agentcapabilities.AgentSurfaceHeader) != agentcapabilities.AgentSurfacePulseMCP { + t.Error("MCP token or surface header was lost") + http.Error(w, "unauthorized", http.StatusUnauthorized) + return + } + body, err := io.ReadAll(r.Body) + if err != nil || len(body) != 0 { + t.Errorf("GET unexpectedly carried a body: %q, %v", body, err) + } + handler.HandleFleetContext(w, r) + })) + defer backend.Close() + fetched, err := agentcapabilities.FetchManifest(context.Background(), backend.Client(), backend.URL) + if err != nil { + t.Fatal(err) + } + server := &mcpServer{baseURL: backend.URL, token: "fixture-token", manifest: fetched, http: backend.Client()} + input := `{"jsonrpc":"2.0","id":1,"method":"tools/list"}` + "\n" + + `{"jsonrpc":"2.0","id":2,"method":"tools/call","params":{"name":"get_fleet_context","arguments":` + tc.args + `}}` + "\n" + var stdout bytes.Buffer + server.serve(strings.NewReader(input), &stdout) + decoder := json.NewDecoder(&stdout) + var list struct { + ID int + Result struct { + Tools []agentcapabilities.ProjectedTool + } + Error any + } + if err := decoder.Decode(&list); err != nil { + t.Fatal(err) + } + if list.ID != 1 || list.Error != nil { + t.Fatalf("tools/list: %+v", list) + } + found := false + for _, tool := range list.Result.Tools { + if tool.Name == agentcapabilities.FleetContextCapabilityName { + var schema struct { + Properties map[string]struct{ Type string } + } + if err := json.Unmarshal(tool.InputSchema, &schema); err != nil { + t.Fatal(err) + } + if schema.Properties["hasFindings"].Type != "boolean" { + t.Fatalf("hasFindings is not advertised as boolean: %s", tool.InputSchema) + } + found = true + } + } + if !found { + t.Fatal("canonical MCP tool list omitted fleet context") + } + var reply struct { + ID int + Result agentcapabilities.MCPToolResult + Error any + } + if err := decoder.Decode(&reply); err != nil { + t.Fatal(err) + } + if reply.ID != 2 || reply.Error != nil || reply.Result.IsError || len(reply.Result.Content) != 1 { + t.Fatalf("tools/call: %+v", reply) + } + var fleet api.AgentFleetContext + if err := json.Unmarshal([]byte(reply.Result.Content[0].Text), &fleet); err != nil { + t.Fatal(err) + } + if fleet.Resources == nil || fleet.GeneratedAt.IsZero() { + t.Fatalf("lost fleet envelope: %+v", fleet) + } + ids := make([]string, 0, len(fleet.Resources)) + for _, resource := range fleet.Resources { + ids = append(ids, resource.CanonicalID) + } + sort.Strings(ids) + if !reflect.DeepEqual(ids, tc.ids) { + t.Errorf("MCP resource ids = %v, want %v", ids, tc.ids) + } + var structured api.AgentFleetContext + encoded, err := json.Marshal(reply.Result.StructuredContent) + if err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(encoded, &structured); err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(structured, fleet) { + t.Errorf("MCP structured output differs from text: %+v vs %+v", structured, fleet) + } + select { + case query := <-calls: + if query != tc.query { + t.Errorf("encoded query = %q, want %q", query, tc.query) + } + default: + t.Error("MCP call never reached the fleet handler") + } + if len(calls) != 0 { + t.Error("MCP issued more than one fleet request") + } + var extra any + if err := decoder.Decode(&extra); err != io.EOF { + t.Errorf("unexpected extra stdout: %v (%v)", extra, err) + } + }) + } +} diff --git a/docs/release-control/v6/internal/subsystems/ai-runtime.md b/docs/release-control/v6/internal/subsystems/ai-runtime.md index c04f51f834..fe21905570 100644 --- a/docs/release-control/v6/internal/subsystems/ai-runtime.md +++ b/docs/release-control/v6/internal/subsystems/ai-runtime.md @@ -25,6 +25,24 @@ that same result. Successful reads retain their content and execution provenance ## Purpose +### Typed MCP fleet filters survive HTTP projection + +GET/DELETE capability projection preserves public string and boolean query +arguments, including explicit `false`, without stringifying numbers, null, +arrays or objects. Path parameters and internal approval metadata remain out +of the query; write-body and authentication contracts are unchanged. +`get_fleet_context` still advertises `hasFindings` as boolean: true narrows to +resources with active findings; false/omitted applies no finding-presence +filter. String dimensions intersect with it, and legacy string transport stays +compatible. No endpoint or API-handler filter semantics change. + +`TestProjectCapabilityCallGETDELETEPreservesTypedFilters` pins conversion and +shielding; the HTTP request test pins encoding and token preservation. +`TestServer_FleetBooleanFiltersReachRealHandler` joins actual MCP stdio/schema, +HTTP transport, production fleet handler and text/structured output over safe +resource/finding fixtures. It covers true/false/omission, composed filters and +an empty intersection; this is not released-binary or customer-estate proof. + ### Uncertain remediation retains a model-led follow-up An inconclusive verification remains unresolved and may be investigated again diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 5be5084834..c67c7da013 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -21,6 +21,24 @@ ## Purpose +### Typed MCP fleet filters survive HTTP projection + +GET/DELETE capability projection preserves public string and boolean query +arguments, including explicit `false`, without stringifying numbers, null, +arrays or objects. Path parameters and internal approval metadata remain out +of the query; write-body and authentication contracts are unchanged. +`get_fleet_context` still advertises `hasFindings` as boolean: true narrows to +resources with active findings; false/omitted applies no finding-presence +filter. String dimensions intersect with it, and legacy string transport stays +compatible. No endpoint or API-handler filter semantics change. + +`TestProjectCapabilityCallGETDELETEPreservesTypedFilters` pins conversion and +shielding; the HTTP request test pins encoding and token preservation. +`TestServer_FleetBooleanFiltersReachRealHandler` joins actual MCP stdio/schema, +HTTP transport, production fleet handler and text/structured output over safe +resource/finding fixtures. It covers true/false/omission, composed filters and +an empty intersection; this is not released-binary or customer-estate proof. + ### Single-alert mutation response admission The client requires the existing acknowledge/unacknowledge/snooze/unsnooze diff --git a/internal/agentcapabilities/http_test.go b/internal/agentcapabilities/http_test.go index fdcb6cd9a9..a16cb2ab86 100644 --- a/internal/agentcapabilities/http_test.go +++ b/internal/agentcapabilities/http_test.go @@ -103,48 +103,39 @@ func TestBuildCapabilityHTTPRequestProjectsPathBodyAndAuth(t *testing.T) { } func TestBuildCapabilityHTTPRequestForwardsGETFiltersAsQuery(t *testing.T) { - // A GET capability with non-path arguments must forward them as URL query - // parameters (not a body, not dropped) so filter-style tools like - // get_fleet_context can receive agent-supplied filters through the MCP - // tools/call path. - cap := Capability{ - Name: FleetContextCapabilityName, - Method: http.MethodGet, - Path: FleetContextCapabilityPath, - } - req, projected, err := BuildCapabilityHTTPRequest(context.Background(), "http://pulse.local/", "token", cap, map[string]any{ - "hasFindings": "true", - "technology": "docker", - }) - if err != nil { - t.Fatalf("BuildCapabilityHTTPRequest: %v", err) - } - if req.Method != http.MethodGet { - t.Fatalf("method = %q, want GET", req.Method) - } - if projected.HasBody { - t.Fatalf("GET projection must not produce a body; projected = %+v", projected) - } - if projected.Query == nil { - t.Fatal("projected.Query must be populated for a GET with filter args") - } - if got := projected.Query.Get("hasFindings"); got != "true" { - t.Errorf("query hasFindings = %q, want true", got) - } - if got := projected.Query.Get("technology"); got != "docker" { - t.Errorf("query technology = %q, want docker", got) - } - // The query must land on the request URL encoded. - if req.URL.RawQuery == "" { - t.Fatalf("RawQuery empty; url = %q", req.URL.String()) - } - gotFindings := req.URL.Query().Get("hasFindings") - gotTech := req.URL.Query().Get("technology") - if gotFindings != "true" || gotTech != "docker" { - t.Errorf("url query = hasFindings=%q technology=%q", gotFindings, gotTech) - } - if req.Body != nil { - t.Fatalf("GET request must not carry a body; got %+v", req.Body) + cap := Capability{Name: FleetContextCapabilityName, Method: http.MethodGet, Path: FleetContextCapabilityPath} + for _, tc := range []struct { + name string + value any + want string + }{ + {"boolean_true", true, "true"}, + {"boolean_false", false, "false"}, + {"legacy_string", "true", "true"}, + } { + t.Run(tc.name, func(t *testing.T) { + req, projected, err := BuildCapabilityHTTPRequest(context.Background(), "http://pulse.local/", "fixture-token", cap, map[string]any{ + "hasFindings": tc.value, + "technology": "qemu & lxc", + ApprovalArgumentKey: "private-approval", + }) + if err != nil { + t.Fatal(err) + } + if req.Method != http.MethodGet || req.Body != nil || projected.HasBody { + t.Fatalf("GET must retain its method and no body: %s %+v", req.Method, projected) + } + wantQuery := "hasFindings=" + tc.want + "&technology=qemu+%26+lxc" + if req.URL.RawQuery != wantQuery || projected.Query.Encode() != wantQuery { + t.Errorf("encoded query = %q, projection = %q, want %q", req.URL.RawQuery, projected.Query.Encode(), wantQuery) + } + if req.URL.Query().Get("technology") != "qemu & lxc" { + t.Error("query encoding changed the string filter") + } + if req.Header.Get(AgentAPITokenHeader) != "fixture-token" { + t.Error("filter projection lost authentication header") + } + }) } } diff --git a/internal/agentcapabilities/projection.go b/internal/agentcapabilities/projection.go index 9bea18187d..96e6839610 100644 --- a/internal/agentcapabilities/projection.go +++ b/internal/agentcapabilities/projection.go @@ -6,6 +6,7 @@ import ( "net/http" "net/url" "regexp" + "strconv" "strings" ) @@ -546,8 +547,14 @@ func ProjectCapabilityCall(cap Capability, args map[string]any) (ProjectedCall, if pathParams[k] { continue } - if s, ok := v.(string); ok { - query.Set(k, s) + switch value := v.(type) { + case string: + query.Set(k, value) + case bool: + // Manifest booleans (including explicit false) must survive + // JSON-RPC decoding just like string filters. Do not stringify + // arbitrary values such as objects, arrays or null. + query.Set(k, strconv.FormatBool(value)) } } if len(query) > 0 { diff --git a/internal/agentcapabilities/projection_test.go b/internal/agentcapabilities/projection_test.go index 2f618b66f4..c56cd5c73f 100644 --- a/internal/agentcapabilities/projection_test.go +++ b/internal/agentcapabilities/projection_test.go @@ -809,3 +809,60 @@ func TestProjectCapabilityCallReadCapabilityHasNoBody(t *testing.T) { t.Fatalf("GET capability must not project body, got hasBody=%v body=%s", got.HasBody, got.Body) } } + +// GET/DELETE filters share the manifest projection. Keep boolean zero values, +// existing strings and the path/private-argument boundary without broadening +// unsupported values into arbitrary query text. +func TestProjectCapabilityCallGETDELETEPreservesTypedFilters(t *testing.T) { + cases := []struct { + name string + filters map[string]any + want string + }{ + {"true", map[string]any{"hasFindings": true}, "hasFindings=true"}, + {"false", map[string]any{"hasFindings": false}, "hasFindings=false"}, + {"legacy_string", map[string]any{"hasFindings": "true"}, "hasFindings=true"}, + {"mixed", map[string]any{"hasFindings": true, "technology": "qemu & lxc"}, "hasFindings=true&technology=qemu+%26+lxc"}, + {"empty_string", map[string]any{"technology": ""}, "technology="}, + {"unsupported", map[string]any{"null": nil, "number": float64(1), "integer": 1, "array": []any{true}, "object": map[string]any{"x": true}}, ""}, + {"private_boolean", map[string]any{ApprovalArgumentKey: true}, ""}, + {"omitted", nil, ""}, + } + for _, method := range []string{http.MethodGet, http.MethodDelete} { + for _, tc := range cases { + t.Run(method+"/"+tc.name, func(t *testing.T) { + args := map[string]any{ResourceIDArgumentName: "vm:101/console", ApprovalArgumentKey: "private-approval"} + for key, value := range tc.filters { + args[key] = value + } + before, err := json.Marshal(args) + if err != nil { + t.Fatal(err) + } + got, err := ProjectCapabilityCall(Capability{Method: method, Path: ResourceContextCapabilityPath}, args) + if err != nil { + t.Fatal(err) + } + if got.Path != "/api/agent/resource-context/vm%3A101%2Fconsole" { + t.Fatalf("path = %q", got.Path) + } + if got.HasBody || len(got.Body) != 0 { + t.Fatalf("%s projected a body: %+v", method, got) + } + if got.Query.Encode() != tc.want { + t.Errorf("query = %q, want %q", got.Query.Encode(), tc.want) + } + if tc.want == "" && got.Query != nil { + t.Errorf("empty projection must retain nil query: %+v", got.Query) + } + after, err := json.Marshal(args) + if err != nil { + t.Fatal(err) + } + if string(after) != string(before) { + t.Errorf("projection mutated caller arguments: %s -> %s", before, after) + } + }) + } + } +}