From f4da3c5ffd111563be9a99d6f7e4af85a16f4541 Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Fri, 24 Apr 2026 15:17:59 -0600 Subject: [PATCH 1/2] =?UTF-8?q?feat(mcp):=20insights=5Fview=20MCP=20App=20?= =?UTF-8?q?=E2=80=94=20time-series=20explorer=20(KLA-402)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the Directory Insights time-series dashboard as an interactive MCP App. Parameters mirror `jc insights query` (service, event_type, last, start, end, user); the rendered app shows a stacked bar chart by time bucket with a top-users ranking alongside. Server side (apps_insights.go): - insightsViewArgs: typed inputs so the JSON schema auto-derives; LLMs see the same shape they already know from the CLI - fetchInsightsViewData: CountEvents for true total, QueryEvents capped at 10k for the sample, aggregate into bins keyed by bucket start (bucket size auto-chosen from window: 5m/1h/6h/1d), tally top 10 users, keep 20-event preview - resolveInsightsWindow: parses last/start/end, defaults to 24h, validates start < end; uses local duration parser anchored on our nowFunc so tests stay deterministic (api.ParseTimeRange uses its own wall-clock) - bucketSizeFor: 5m / 1h / 6h / 1d depending on window length addToolWithMetaTyped[In any]: generic variant of addToolWithMeta so apps that take parameters (insights_view now, user_view / device_view next) get an auto-derived JSON input schema from the typed arg struct. Still wires rate limiting, audit logging, and Meta with _meta.ui.resourceUri. Client side (apps_html/insights.html): - Filter form (service, last, event_type, user) with Apply + Refresh buttons - Proactive fetch on load, re-fetches on filter submit via jcApp.callTool - Per-bucket stacked bar chart with event-type color legend; top-users list - Warning banner when the window is large enough that we sampled vs fetched all events - Uses the shared jcApp scaffolding from common.js; no inline postMessage re-implementation. Purely content + rendering. Tests (apps_insights_test.go): - TestBucketSizeFor — auto-picked bucket size - TestResolveInsightsWindow — defaults, Last override, start/end ordering - TestFetchInsightsViewData_Aggregates — fixture of 5 events, verifies total, event type set, top-user ranking, bin count, sum of bin counts - TestFetchInsightsViewData_EventTypeFilter — filter narrows correctly - TestInsightsView_HasUIMetadata — tool exposes _meta.ui.resourceUri - TestInsightsResource_ServesHTMLWithInjection — ui:// resource has common.js injected and the marker stripped Stacks on juergen/mcp-apps-refactor (PR #18) since it depends on addToolWithMetaTyped and the renderAppHTML helper introduced there. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/mcp/apps.go | 52 +++- internal/mcp/apps_html/insights.html | 431 +++++++++++++++++++++++++++ internal/mcp/apps_insights.go | 381 +++++++++++++++++++++++ internal/mcp/apps_insights_test.go | 290 ++++++++++++++++++ internal/mcp/tools_test.go | 5 +- 5 files changed, 1156 insertions(+), 3 deletions(-) create mode 100644 internal/mcp/apps_html/insights.html create mode 100644 internal/mcp/apps_insights.go create mode 100644 internal/mcp/apps_insights_test.go diff --git a/internal/mcp/apps.go b/internal/mcp/apps.go index eedc4f1..fec7b89 100644 --- a/internal/mcp/apps.go +++ b/internal/mcp/apps.go @@ -79,6 +79,51 @@ func renderAppHTML(raw string) string { return strings.Replace(raw, appCommonMarker, "", 1) } +// addToolWithMetaTyped wraps mcp.AddTool with rate limiting, audit logging, +// tool filtering, and Meta support — the typed-input variant of +// addToolWithMeta for MCP Apps that accept parameters (service filters, +// time ranges, target IDs, etc.). The In type parameter is used by the SDK +// to auto-derive the tool's JSON input schema. +func addToolWithMetaTyped[In any](s *Server, name, description string, meta mcp.Meta, handler func(ctx context.Context, req *mcp.CallToolRequest, args In) (*mcp.CallToolResult, any, error)) { + if !s.toolFilter.isAllowed(name) { + return + } + + tool := &mcp.Tool{ + Name: name, + Description: description, + Meta: meta, + } + + wrapped := func(ctx context.Context, req *mcp.CallToolRequest, args In) (*mcp.CallToolResult, any, error) { + if err := s.limiter.allow(); err != nil { + s.auditLog.log(name, req.Params.Arguments, false, err.Error()) + return errorResult(err.Error()), nil, nil + } + + result, out, err := handler(ctx, req, args) + + if err != nil { + s.auditLog.log(name, req.Params.Arguments, false, err.Error()) + } else if result != nil && result.IsError { + errMsg := "tool error" + if len(result.Content) > 0 { + if tc, ok := result.Content[0].(*mcp.TextContent); ok { + errMsg = tc.Text + } + } + s.auditLog.log(name, req.Params.Arguments, false, errMsg) + } else { + s.auditLog.log(name, req.Params.Arguments, true, "") + } + + return result, out, err + } + + mcp.AddTool(s.mcpServer, tool, wrapped) + s.toolNames = append(s.toolNames, name) +} + // addToolWithMeta wraps mcp.AddTool with rate limiting, audit logging, and // tool filtering — same as addTool but also sets Meta on the tool definition. // This is used for MCP App tools that need _meta.ui.resourceUri. @@ -124,12 +169,17 @@ func (s *Server) addToolWithMeta(name, description string, meta mcp.Meta, handle // --- MCP App registration entry points --- -// registerAppTools registers the tool half of every MCP App in appSpecs. +// registerAppTools registers the tool half of every MCP App in appSpecs, +// plus any apps with typed inputs that live outside the no-args slice. // Each tool carries _meta.ui.resourceUri pointing at its matching ui:// resource. func (s *Server) registerAppTools() { for _, spec := range appSpecs { s.registerAppTool(spec) } + // Apps that accept parameters are registered individually because they + // need typed handler generics. Each call registers both tool and resource + // (the resource half is no different from a no-args app). + s.registerInsightsView() } // registerAppResources registers the ui:// resource half of every MCP App. diff --git a/internal/mcp/apps_html/insights.html b/internal/mcp/apps_html/insights.html new file mode 100644 index 0000000..d305df2 --- /dev/null +++ b/internal/mcp/apps_html/insights.html @@ -0,0 +1,431 @@ + + + + + +JumpCloud Insights Explorer + + + +
+
+

Directory Insights

+
+
+ +
+ +
+ + + + + +
+ +
+
+
Loading…
+ + + + + + diff --git a/internal/mcp/apps_insights.go b/internal/mcp/apps_insights.go new file mode 100644 index 0000000..2e664d8 --- /dev/null +++ b/internal/mcp/apps_insights.go @@ -0,0 +1,381 @@ +package mcp + +import ( + "context" + _ "embed" + "encoding/json" + "fmt" + "sort" + "strings" + "time" + + "github.com/klaassen-consulting/jc/internal/api" + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +//go:embed apps_html/insights.html +var insightsHTML string + +const ( + insightsResourceURI = "ui://jc/insights" + + // Upper bound on events fetched per invocation. Directory Insights windows + // can hold millions of events; we sample with this cap to keep the payload + // sendable and the aggregation responsive. The tool's Total field always + // reflects the true count so clients can detect truncation. + insightsMaxFetchEvents = 10000 + + // Max number of events returned in the preview array (for drill-down). + insightsPreviewLimit = 20 + + // Max number of users returned in the top-users ranking. + insightsTopUsers = 10 +) + +// insightsViewArgs is the tool input. Mirrors the shape of `jc insights query` +// flags so LLMs familiar with the CLI can invoke the tool naturally. +type insightsViewArgs struct { + // Service to query. Accepts "all", "sso", "ldap", "radius", "directory", + // or comma-separated combinations. Defaults to "all". + Service string `json:"service,omitempty" jsonschema:"Service to query (sso, ldap, radius, directory, all). Accepts comma-separated values."` + // EventType filters by a specific event type (e.g. sso_auth_failed). + EventType string `json:"event_type,omitempty" jsonschema:"Filter by event type (e.g. sso_auth_failed, password_change)."` + // Last is a time-range shortcut: 1h / 24h / 7d / 30d / 1m. Ignored when + // Start or End is set. Defaults to 24h. + Last string `json:"last,omitempty" jsonschema:"Time range shortcut: 1h, 24h, 7d, 30d, 1m. Defaults to 24h. Ignored if start/end provided."` + // Start is an RFC3339 timestamp or YYYY-MM-DD date. + Start string `json:"start,omitempty" jsonschema:"Start time (RFC3339 or YYYY-MM-DD)."` + // End is an RFC3339 timestamp or YYYY-MM-DD date. Defaults to now. + End string `json:"end,omitempty" jsonschema:"End time (RFC3339 or YYYY-MM-DD). Defaults to now."` + // User filters events whose initiated_by.username matches. + User string `json:"user,omitempty" jsonschema:"Filter by initiated_by.username."` +} + +// insightsViewData is the JSON payload the tool returns to the app iframe. +type insightsViewData struct { + Service string `json:"service"` + EventType string `json:"event_type,omitempty"` + User string `json:"user,omitempty"` + Start string `json:"start"` + End string `json:"end"` + BucketSize string `json:"bucket_size"` + Total int `json:"total"` // true count from CountEvents + Sampled int `json:"sampled"` // number of events actually aggregated + EventTypes []string `json:"event_types"` + Bins []insightsBin `json:"bins"` + TopUsers []insightsUserCnt `json:"top_users"` + Preview []json.RawMessage `json:"preview"` + Warnings []string `json:"warnings,omitempty"` +} + +// insightsBin is one time bucket with a per-event-type count. +type insightsBin struct { + // Bucket is the RFC3339 start timestamp of the bucket. + Bucket string `json:"bucket"` + // Counts maps event type → count within this bucket. + Counts map[string]int `json:"counts"` +} + +// insightsUserCnt is one entry in the top-users ranking. +type insightsUserCnt struct { + Username string `json:"username"` + Count int `json:"count"` +} + +// registerInsightsView wires the insights_view MCP App: typed tool + ui:// +// resource. Lives outside appSpecs because it takes input args. +func (s *Server) registerInsightsView() { + meta := mcp.Meta{ + "ui": map[string]any{"resourceUri": insightsResourceURI}, + "ui/resourceUri": insightsResourceURI, + } + addToolWithMetaTyped(s, "insights_view", + "Directory Insights event explorer: stacked time-series chart by event type with top-users ranking and event preview. "+ + "Parameters mirror `jc insights query` (service, event_type, last, start, end, user). "+ + "Renders as an interactive dashboard in MCP App-capable hosts.", + meta, + func(ctx context.Context, req *mcp.CallToolRequest, args insightsViewArgs) (*mcp.CallToolResult, any, error) { + data, err := fetchInsightsViewData(ctx, args) + if err != nil { + return errorResult(fmt.Sprintf("fetching insights: %v", err)), nil, nil + } + res, err := jsonResult(data) + if err != nil { + return errorResult(err.Error()), nil, nil + } + return res, nil, nil + }, + ) + + rendered := renderAppHTML(insightsHTML) + s.mcpServer.AddResource( + &mcp.Resource{ + URI: insightsResourceURI, + Name: "Insights Explorer", + Description: "Interactive Directory Insights time-series and top-users view", + MIMEType: mcpAppMIMEType, + }, + func(ctx context.Context, req *mcp.ReadResourceRequest) (*mcp.ReadResourceResult, error) { + return &mcp.ReadResourceResult{ + Contents: []*mcp.ResourceContents{{ + URI: insightsResourceURI, + MIMEType: mcpAppMIMEType, + Text: rendered, + }}, + }, nil + }, + ) +} + +// resolveInsightsWindow turns args into a concrete [start, end] pair in UTC, +// applying defaults (service=all, last=24h if nothing else set). +func resolveInsightsWindow(args insightsViewArgs, now time.Time) (start, end time.Time, err error) { + // End first: default to now if unspecified. + if args.End != "" { + end, err = parseInsightsTime(args.End) + if err != nil { + return time.Time{}, time.Time{}, fmt.Errorf("parsing end: %w", err) + } + } else { + end = now + } + + switch { + case args.Start != "": + start, err = parseInsightsTime(args.Start) + if err != nil { + return time.Time{}, time.Time{}, fmt.Errorf("parsing start: %w", err) + } + case args.Last != "": + // Parse the duration shortcut locally so the start anchors to our + // end time (which honors nowFunc in tests). api.ParseTimeRange + // uses its own clock, which causes drift in unit tests. + dur, err := parseLastDuration(args.Last) + if err != nil { + return time.Time{}, time.Time{}, fmt.Errorf("parsing last: %w", err) + } + start = end.Add(-dur) + default: + // Default window: 24h back from end. + start = end.Add(-24 * time.Hour) + } + + if !start.Before(end) { + return time.Time{}, time.Time{}, fmt.Errorf("start (%s) must be before end (%s)", start.Format(time.RFC3339), end.Format(time.RFC3339)) + } + return start.UTC(), end.UTC(), nil +} + +// parseInsightsTime accepts RFC3339, YYYY-MM-DD, or duration shorthand. +func parseInsightsTime(s string) (time.Time, error) { + return api.ParseTimeRange(s) +} + +// parseLastDuration turns a "Xh" / "Xd" / "Xm" shortcut into a time.Duration. +// Mirrors api.ParseTimeRange's shortcut syntax but returns the duration itself +// so callers can anchor it against their own clock rather than time.Now(). +func parseLastDuration(s string) (time.Duration, error) { + s = strings.TrimSpace(s) + s = strings.TrimPrefix(s, "last ") + s = strings.TrimSpace(s) + if len(s) < 2 { + return 0, fmt.Errorf("invalid duration shortcut %q", s) + } + unit := s[len(s)-1] + var n int + if _, err := fmt.Sscanf(s[:len(s)-1], "%d", &n); err != nil { + return 0, fmt.Errorf("invalid duration shortcut %q: %w", s, err) + } + switch unit { + case 'h': + return time.Duration(n) * time.Hour, nil + case 'd': + return time.Duration(n) * 24 * time.Hour, nil + case 'm': // months (calendar) — approximate as 30d for the purpose of the shortcut window. + return time.Duration(n) * 30 * 24 * time.Hour, nil + } + return 0, fmt.Errorf("invalid duration unit %q in %q", string(unit), s) +} + +// bucketSizeFor picks a sensible time bucket for the given window. Bigger +// windows use bigger buckets to keep the bin count manageable. +func bucketSizeFor(window time.Duration) time.Duration { + switch { + case window <= 6*time.Hour: + return 5 * time.Minute + case window <= 48*time.Hour: + return time.Hour + case window <= 7*24*time.Hour: + return 6 * time.Hour + default: + return 24 * time.Hour + } +} + +// formatBucketSize returns a human-readable representation of a bucket size. +func formatBucketSize(d time.Duration) string { + switch { + case d >= 24*time.Hour: + return fmt.Sprintf("%dd", int(d/(24*time.Hour))) + case d >= time.Hour: + return fmt.Sprintf("%dh", int(d/time.Hour)) + default: + return fmt.Sprintf("%dm", int(d/time.Minute)) + } +} + +// fetchInsightsViewData runs the Directory Insights queries, aggregates events +// into time buckets, and returns a payload ready for the UI to render. +func fetchInsightsViewData(ctx context.Context, args insightsViewArgs) (*insightsViewData, error) { + now := nowFunc().UTC() + + start, end, err := resolveInsightsWindow(args, now) + if err != nil { + return nil, err + } + + service := args.Service + if service == "" { + service = "all" + } + + query := api.InsightsQuery{ + Service: service, + StartTime: start.Format(time.RFC3339), + EndTime: end.Format(time.RFC3339), + } + filter := map[string]any{} + if args.EventType != "" { + filter["event_type"] = args.EventType + } + if args.User != "" { + filter["initiated_by.username"] = args.User + } + if len(filter) > 0 { + query.SearchTermFilter = filter + } + + client, err := newInsightsClientFunc() + if err != nil { + return nil, fmt.Errorf("insights client: %w", err) + } + + total, err := client.CountEvents(ctx, query) + if err != nil { + return nil, fmt.Errorf("counting events: %w", err) + } + + // Fetch events (bounded). If total > cap, we still aggregate what we can + // and add a warning so the UI can flag partial data. + fetchLimit := total + if fetchLimit > insightsMaxFetchEvents { + fetchLimit = insightsMaxFetchEvents + } + + var events []json.RawMessage + if fetchLimit > 0 { + res, err := client.QueryEvents(ctx, query, api.InsightsQueryOptions{ + Limit: fetchLimit, + Sort: "-timestamp", + }) + if err != nil { + return nil, fmt.Errorf("querying events: %w", err) + } + events = res.Data + } + + window := end.Sub(start) + bucket := bucketSizeFor(window) + + data := &insightsViewData{ + Service: service, + EventType: args.EventType, + User: args.User, + Start: start.Format(time.RFC3339), + End: end.Format(time.RFC3339), + BucketSize: formatBucketSize(bucket), + Total: total, + Sampled: len(events), + Bins: []insightsBin{}, + TopUsers: []insightsUserCnt{}, + Preview: []json.RawMessage{}, + } + + if total > len(events) { + data.Warnings = append(data.Warnings, + fmt.Sprintf("Window contains %d events; aggregated the most recent %d (chart reflects the sample).", total, len(events))) + } + + // Pre-seed bins for every bucket in the window so the chart shows empty + // slots where no events occurred. + binIndex := map[string]*insightsBin{} + for t := start.Truncate(bucket); !t.After(end); t = t.Add(bucket) { + key := t.UTC().Format(time.RFC3339) + b := insightsBin{Bucket: key, Counts: map[string]int{}} + data.Bins = append(data.Bins, b) + binIndex[key] = &data.Bins[len(data.Bins)-1] + } + + // Tally events into bins, track event types and top users, collect preview. + eventTypeSet := map[string]struct{}{} + userCounts := map[string]int{} + + for i, raw := range events { + var evt struct { + Timestamp string `json:"timestamp"` + EventType string `json:"event_type"` + InitiatedBy struct { + Username string `json:"username"` + } `json:"initiated_by"` + } + if err := json.Unmarshal(raw, &evt); err != nil { + continue + } + + evtTime, err := time.Parse(time.RFC3339, evt.Timestamp) + if err != nil { + continue + } + + if evt.EventType == "" { + evt.EventType = "unknown" + } + eventTypeSet[evt.EventType] = struct{}{} + + key := evtTime.UTC().Truncate(bucket).Format(time.RFC3339) + if b, ok := binIndex[key]; ok { + b.Counts[evt.EventType]++ + } + + if evt.InitiatedBy.Username != "" { + userCounts[evt.InitiatedBy.Username]++ + } + + if i < insightsPreviewLimit { + data.Preview = append(data.Preview, raw) + } + } + + // Sort event types for stable rendering (legend order). + data.EventTypes = make([]string, 0, len(eventTypeSet)) + for k := range eventTypeSet { + data.EventTypes = append(data.EventTypes, k) + } + sort.Strings(data.EventTypes) + + // Top-N users by count (desc), tiebreak on username asc for stability. + for u, c := range userCounts { + data.TopUsers = append(data.TopUsers, insightsUserCnt{Username: u, Count: c}) + } + sort.Slice(data.TopUsers, func(i, j int) bool { + if data.TopUsers[i].Count != data.TopUsers[j].Count { + return data.TopUsers[i].Count > data.TopUsers[j].Count + } + return strings.Compare(data.TopUsers[i].Username, data.TopUsers[j].Username) < 0 + }) + if len(data.TopUsers) > insightsTopUsers { + data.TopUsers = data.TopUsers[:insightsTopUsers] + } + + return data, nil +} diff --git a/internal/mcp/apps_insights_test.go b/internal/mcp/apps_insights_test.go new file mode 100644 index 0000000..36aea79 --- /dev/null +++ b/internal/mcp/apps_insights_test.go @@ -0,0 +1,290 @@ +package mcp + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// startInsightsServer serves deterministic fixtures for /events and +// /events/count endpoints consumed by fetchInsightsViewData. +func startInsightsServer(t *testing.T, events []map[string]any) *httptest.Server { + t.Helper() + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/insights/directory/v1/events": + // Basic body parse to honor the search_term_filter for tests that + // exercise event_type/user filtering. + var body map[string]any + _ = json.NewDecoder(r.Body).Decode(&body) + filter, _ := body["search_term_filter"].(map[string]any) + + filtered := events + if len(filter) > 0 { + filtered = nil + for _, e := range events { + match := true + for k, v := range filter { + switch k { + case "event_type": + if e["event_type"] != v { + match = false + } + case "initiated_by.username": + ib, _ := e["initiated_by"].(map[string]any) + if ib == nil || ib["username"] != v { + match = false + } + } + } + if match { + filtered = append(filtered, e) + } + } + } + _ = json.NewEncoder(w).Encode(filtered) + case "/insights/directory/v1/events/count": + var body map[string]any + _ = json.NewDecoder(r.Body).Decode(&body) + filter, _ := body["search_term_filter"].(map[string]any) + filtered := events + if len(filter) > 0 { + filtered = nil + for _, e := range events { + match := true + for k, v := range filter { + switch k { + case "event_type": + if e["event_type"] != v { + match = false + } + case "initiated_by.username": + ib, _ := e["initiated_by"].(map[string]any) + if ib == nil || ib["username"] != v { + match = false + } + } + } + if match { + filtered = append(filtered, e) + } + } + } + _ = json.NewEncoder(w).Encode(map[string]int{"count": len(filtered)}) + default: + w.WriteHeader(404) + } + })) +} + +func TestBucketSizeFor(t *testing.T) { + cases := []struct { + window time.Duration + want time.Duration + }{ + {1 * time.Hour, 5 * time.Minute}, + {6 * time.Hour, 5 * time.Minute}, + {12 * time.Hour, time.Hour}, + {48 * time.Hour, time.Hour}, + {7 * 24 * time.Hour, 6 * time.Hour}, + {8 * 24 * time.Hour, 24 * time.Hour}, + {30 * 24 * time.Hour, 24 * time.Hour}, + } + for _, c := range cases { + if got := bucketSizeFor(c.window); got != c.want { + t.Errorf("bucketSizeFor(%v) = %v, want %v", c.window, got, c.want) + } + } +} + +func TestResolveInsightsWindow_DefaultsTo24h(t *testing.T) { + now := time.Date(2026, 4, 24, 18, 0, 0, 0, time.UTC) + start, end, err := resolveInsightsWindow(insightsViewArgs{}, now) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if !end.Equal(now) { + t.Errorf("end = %v, want %v", end, now) + } + if end.Sub(start) != 24*time.Hour { + t.Errorf("window = %v, want 24h", end.Sub(start)) + } +} + +func TestResolveInsightsWindow_LastOverridesDefault(t *testing.T) { + now := time.Date(2026, 4, 24, 18, 0, 0, 0, time.UTC) + start, end, err := resolveInsightsWindow(insightsViewArgs{Last: "7d"}, now) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if end.Sub(start) < 6*24*time.Hour || end.Sub(start) > 8*24*time.Hour { + t.Errorf("window = %v, want ~7d", end.Sub(start)) + } +} + +func TestResolveInsightsWindow_StartBeforeEndRequired(t *testing.T) { + now := time.Date(2026, 4, 24, 18, 0, 0, 0, time.UTC) + _, _, err := resolveInsightsWindow(insightsViewArgs{ + Start: "2026-04-25T00:00:00Z", + End: "2026-04-24T00:00:00Z", + }, now) + if err == nil { + t.Error("expected error when start >= end") + } +} + +func TestFetchInsightsViewData_Aggregates(t *testing.T) { + setupToolTest(t) + + // Fixed "now" so bucket boundaries are deterministic. + origNow := nowFunc + nowFunc = func() time.Time { return time.Date(2026, 4, 24, 12, 0, 0, 0, time.UTC) } + t.Cleanup(func() { nowFunc = origNow }) + + // Five events across a 24h window: three sso_auth_failed, two password_change. + // Two users: alice (3 events), bob (2 events). + events := []map[string]any{ + {"timestamp": "2026-04-24T11:30:00Z", "event_type": "sso_auth_failed", "initiated_by": map[string]any{"username": "alice"}}, + {"timestamp": "2026-04-24T11:00:00Z", "event_type": "sso_auth_failed", "initiated_by": map[string]any{"username": "alice"}}, + {"timestamp": "2026-04-24T06:15:00Z", "event_type": "sso_auth_failed", "initiated_by": map[string]any{"username": "bob"}}, + {"timestamp": "2026-04-24T03:00:00Z", "event_type": "password_change", "initiated_by": map[string]any{"username": "alice"}}, + {"timestamp": "2026-04-23T20:30:00Z", "event_type": "password_change", "initiated_by": map[string]any{"username": "bob"}}, + } + ts := startInsightsServer(t, events) + t.Cleanup(ts.Close) + overrideInsightsClientForTest(t, ts.URL) + + data, err := fetchInsightsViewData(context.Background(), insightsViewArgs{Last: "24h"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + + if data.Total != 5 { + t.Errorf("Total = %d, want 5", data.Total) + } + if data.Sampled != 5 { + t.Errorf("Sampled = %d, want 5", data.Sampled) + } + if len(data.EventTypes) != 2 { + t.Errorf("EventTypes len = %d, want 2 (%v)", len(data.EventTypes), data.EventTypes) + } + if data.TopUsers[0].Username != "alice" || data.TopUsers[0].Count != 3 { + t.Errorf("top user = %+v, want alice/3", data.TopUsers[0]) + } + if data.TopUsers[1].Username != "bob" || data.TopUsers[1].Count != 2 { + t.Errorf("2nd user = %+v, want bob/2", data.TopUsers[1]) + } + + // Bucket size for 24h window is 1h, so we expect 25 bins (inclusive). + if len(data.Bins) < 24 || len(data.Bins) > 26 { + t.Errorf("bins len = %d, want ~25 for 24h window", len(data.Bins)) + } + + // Sum the per-bucket counts and confirm they equal total events that + // fell inside the window. (Events outside the window would not bin; all + // 5 are within 24h of noon on the 24th so all should bin.) + sum := 0 + for _, b := range data.Bins { + for _, c := range b.Counts { + sum += c + } + } + if sum != 5 { + t.Errorf("sum of bin counts = %d, want 5", sum) + } +} + +func TestFetchInsightsViewData_EventTypeFilter(t *testing.T) { + setupToolTest(t) + origNow := nowFunc + nowFunc = func() time.Time { return time.Date(2026, 4, 24, 12, 0, 0, 0, time.UTC) } + t.Cleanup(func() { nowFunc = origNow }) + + events := []map[string]any{ + {"timestamp": "2026-04-24T11:30:00Z", "event_type": "sso_auth_failed", "initiated_by": map[string]any{"username": "alice"}}, + {"timestamp": "2026-04-24T10:00:00Z", "event_type": "password_change", "initiated_by": map[string]any{"username": "bob"}}, + } + ts := startInsightsServer(t, events) + t.Cleanup(ts.Close) + overrideInsightsClientForTest(t, ts.URL) + + data, err := fetchInsightsViewData(context.Background(), insightsViewArgs{ + Last: "24h", + EventType: "sso_auth_failed", + }) + if err != nil { + t.Fatalf("fetch: %v", err) + } + if data.Total != 1 { + t.Errorf("filtered total = %d, want 1", data.Total) + } + if len(data.EventTypes) != 1 || data.EventTypes[0] != "sso_auth_failed" { + t.Errorf("event types = %v, want only sso_auth_failed", data.EventTypes) + } +} + +func TestInsightsView_HasUIMetadata(t *testing.T) { + setupToolTest(t) + cs := connectToolTestServer(t, Options{}) + + ctx := context.Background() + result, err := cs.ListTools(ctx, nil) + if err != nil { + t.Fatalf("ListTools: %v", err) + } + + var found *mcp.Tool + for _, tool := range result.Tools { + if tool.Name == "insights_view" { + found = tool + break + } + } + if found == nil { + t.Fatal("insights_view tool not found in ListTools result") + } + + meta := map[string]any(found.Meta) + ui, ok := meta["ui"].(map[string]any) + if !ok { + t.Fatalf("expected _meta.ui to be a map, got %T", meta["ui"]) + } + if uri, _ := ui["resourceUri"].(string); uri != insightsResourceURI { + t.Errorf("resourceUri = %q, want %q", uri, insightsResourceURI) + } +} + +func TestInsightsResource_ServesHTMLWithInjection(t *testing.T) { + setupToolTest(t) + cs := connectToolTestServer(t, Options{}) + + result, err := cs.ReadResource(context.Background(), &mcp.ReadResourceParams{ + URI: insightsResourceURI, + }) + if err != nil { + t.Fatalf("ReadResource: %v", err) + } + if len(result.Contents) == 0 { + t.Fatal("empty resource contents") + } + c := result.Contents[0] + if c.MIMEType != mcpAppMIMEType { + t.Errorf("MIME = %q, want %q", c.MIMEType, mcpAppMIMEType) + } + if !strings.Contains(c.Text, "window.jcApp") { + t.Error("served HTML missing common.js injection (window.jcApp)") + } + if strings.Contains(c.Text, appCommonMarker) { + t.Error("served HTML still contains the injection marker") + } + if !strings.Contains(c.Text, "Directory Insights") { + t.Error("served HTML missing page title") + } +} diff --git a/internal/mcp/tools_test.go b/internal/mcp/tools_test.go index 00d5152..d6f4ef7 100644 --- a/internal/mcp/tools_test.go +++ b/internal/mcp/tools_test.go @@ -375,6 +375,7 @@ func TestMCP_ListTools_AllRegistered(t *testing.T) { "recipe_run", "plan", "explain", // MCP Apps "dashboard_view", + "insights_view", } toolNames := make(map[string]bool) @@ -389,8 +390,8 @@ func TestMCP_ListTools_AllRegistered(t *testing.T) { } // Verify exact count — update when adding/removing tools. - if len(result.Tools) != 195 { - t.Errorf("expected 195 tools, got %d", len(result.Tools)) + if len(result.Tools) != 196 { + t.Errorf("expected 196 tools, got %d", len(result.Tools)) } } From daf3c880e19a0bafc8ff3bd151e143cd8bc088ed Mon Sep 17 00:00:00 2001 From: Juergen Klaassen Date: Fri, 24 Apr 2026 15:49:03 -0600 Subject: [PATCH 2/2] fix(mcp): address Bugbot findings on PR #19 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Medium: insights.html onToolResult race The initialized flag was set to true BEFORE the try-catch in onToolResult. If parseToolResult threw or returned null, the flag was already set and the setTimeout fallback (which uses the more robust load() path) never fired — the UI locked on the loading spinner with no recovery. Now the flag only flips after a successful render. Low: consolidate addToolWithMeta to delegate addToolWithMeta and addToolWithMetaTyped had ~25 lines of identical rate-limiting + audit-logging + tool-filtering wrapper. Any future fix would need to land in both places. Collapsed to a 1-line shim that calls the typed variant with struct{}, so the wrapper exists once. Call sites unchanged (s.addToolWithMeta still works). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/mcp/apps.go | 44 +++------------------------- internal/mcp/apps_html/insights.html | 11 +++++-- 2 files changed, 13 insertions(+), 42 deletions(-) diff --git a/internal/mcp/apps.go b/internal/mcp/apps.go index fec7b89..7f82f3f 100644 --- a/internal/mcp/apps.go +++ b/internal/mcp/apps.go @@ -124,47 +124,11 @@ func addToolWithMetaTyped[In any](s *Server, name, description string, meta mcp. s.toolNames = append(s.toolNames, name) } -// addToolWithMeta wraps mcp.AddTool with rate limiting, audit logging, and -// tool filtering — same as addTool but also sets Meta on the tool definition. -// This is used for MCP App tools that need _meta.ui.resourceUri. +// addToolWithMeta is the no-args convenience wrapper for addToolWithMetaTyped. +// Kept as a method on *Server for call-site ergonomics (s.addToolWithMeta(...)) +// since no-args App tools are the common case. func (s *Server) addToolWithMeta(name, description string, meta mcp.Meta, handler func(ctx context.Context, req *mcp.CallToolRequest, args struct{}) (*mcp.CallToolResult, any, error)) { - if !s.toolFilter.isAllowed(name) { - return - } - - tool := &mcp.Tool{ - Name: name, - Description: description, - Meta: meta, - } - - wrappedHandler := func(ctx context.Context, req *mcp.CallToolRequest, args struct{}) (*mcp.CallToolResult, any, error) { - if err := s.limiter.allow(); err != nil { - s.auditLog.log(name, req.Params.Arguments, false, err.Error()) - return errorResult(err.Error()), nil, nil - } - - result, out, err := handler(ctx, req, args) - - if err != nil { - s.auditLog.log(name, req.Params.Arguments, false, err.Error()) - } else if result != nil && result.IsError { - errMsg := "tool error" - if len(result.Content) > 0 { - if tc, ok := result.Content[0].(*mcp.TextContent); ok { - errMsg = tc.Text - } - } - s.auditLog.log(name, req.Params.Arguments, false, errMsg) - } else { - s.auditLog.log(name, req.Params.Arguments, true, "") - } - - return result, out, err - } - - mcp.AddTool(s.mcpServer, tool, wrappedHandler) - s.toolNames = append(s.toolNames, name) + addToolWithMetaTyped[struct{}](s, name, description, meta, handler) } // --- MCP App registration entry points --- diff --git a/internal/mcp/apps_html/insights.html b/internal/mcp/apps_html/insights.html index d305df2..eee3a3a 100644 --- a/internal/mcp/apps_html/insights.html +++ b/internal/mcp/apps_html/insights.html @@ -404,10 +404,17 @@

Top users

var initialized = false; jcApp.onToolResult(function(params) { if (initialized) return; - initialized = true; + // Only mark initialized after a successful render; a thrown error or + // null payload leaves initialized=false so the setTimeout fallback + // below takes over and issues a real callTool with proper error + // handling. Previously setting the flag up-front could leave the UI + // stuck on the loading spinner. try { var data = jcApp.parseToolResult(params); - if (data) renderAll(data); + if (data) { + initialized = true; + renderAll(data); + } } catch (e) { /* proactive fetch below will retry */ } });