chore: address code review — tests, redaction, trust boundary docs

- Add rate-limit middleware tests (6 cases: under/over limit, window reset,
  per-user isolation, per-limiter isolation, error payload shape)
- Document messages[] trust boundary in aiSummarize schema — caller can
  claim any assistant role, acceptable for single-shot, requires
  server-side state when a follow-up UI is built
- Extend secret redaction to JSON-shape ("key":"value") and HTTP-header
  shape (X-Api-Key: value) + tests
- Add supportsTraceContext gate tests — pattern subject and event
  subjects without a traceId must never enable the span fetch
- Dedupe coerce-attribute helpers into formatHelpers.ts; used by both
  subject formatters and traceContext
- Simplify TraceAttributeValue to `unknown` (the other union members were
  absorbed anyway)
- Comment why abort-reset uses queueMicrotask instead of setTimeout(0)
  (jest.useFakeTimers would otherwise freeze the reset)

packages/api/src/middleware/__tests__/rateLimit.test.ts

@@ -0,0 +1,111 @@
+import type { NextFunction, Request, Response } from 'express';
+
+jest.mock('@/utils/logger', () => ({
+  __esModule: true,
+  default: { info: jest.fn(), warn: jest.fn(), error: jest.fn() },
+}));
+
+// Dynamic user id per test so we can simulate multiple callers
+let mockUserId = 'user-a';
+jest.mock('@/middleware/auth', () => ({
+  getNonNullUserWithTeam: () => ({
+    teamId: 't',
+    userId: mockUserId,
+    email: 'e',
+  }),
+}));
+
+import { createRateLimiter } from '../rateLimit';
+
+function makeReq(): Request {
+  return {} as Request;
+}
+function makeRes(): Response {
+  return {} as Response;
+}
+
+// Invoke the middleware and capture what it passes to next()
+function invoke(mw: ReturnType<typeof createRateLimiter>): {
+  error?: unknown;
+  passed: boolean;
+} {
+  let captured: unknown;
+  let passed = false;
+  const next: NextFunction = err => {
+    if (err) captured = err;
+    else passed = true;
+  };
+  mw(makeReq(), makeRes(), next);
+  return { error: captured, passed };
+}
+
+describe('createRateLimiter', () => {
+  beforeEach(() => {
+    mockUserId = 'user-a';
+  });
+
+  it('allows requests under the limit', () => {
+    const mw = createRateLimiter({ windowMs: 60_000, max: 3, name: 'test' });
+    expect(invoke(mw).passed).toBe(true);
+    expect(invoke(mw).passed).toBe(true);
+    expect(invoke(mw).passed).toBe(true);
+  });
+
+  it('rejects the (max+1)th request with a 429', () => {
+    const mw = createRateLimiter({ windowMs: 60_000, max: 2, name: 'test' });
+    invoke(mw);
+    invoke(mw);
+    const { passed, error } = invoke(mw);
+    expect(passed).toBe(false);
+    expect(error).toBeDefined();
+    // Api429Error has statusCode 429. The project's BaseError pattern puts
+    // the detail message in `.name` and a generic description in `.message`.
+    expect((error as { statusCode?: number }).statusCode).toBe(429);
+    expect((error as Error).name).toContain('Rate limit');
+  });
+
+  it('resets after the window elapses', () => {
+    jest.useFakeTimers();
+    jest.setSystemTime(new Date('2026-01-01T00:00:00Z'));
+    const mw = createRateLimiter({ windowMs: 1000, max: 1, name: 'test' });
+    expect(invoke(mw).passed).toBe(true);
+    expect(invoke(mw).passed).toBe(false); // over
+    jest.setSystemTime(new Date('2026-01-01T00:00:01.500Z')); // past window
+    expect(invoke(mw).passed).toBe(true);
+    jest.useRealTimers();
+  });
+
+  it('tracks users independently', () => {
+    const mw = createRateLimiter({ windowMs: 60_000, max: 1, name: 'test' });
+    mockUserId = 'user-a';
+    expect(invoke(mw).passed).toBe(true);
+    expect(invoke(mw).passed).toBe(false); // a is over
+
+    mockUserId = 'user-b';
+    expect(invoke(mw).passed).toBe(true); // b is fresh
+  });
+
+  it('returns independent buckets per limiter instance', () => {
+    const mwA = createRateLimiter({ windowMs: 60_000, max: 1, name: 'A' });
+    const mwB = createRateLimiter({ windowMs: 60_000, max: 1, name: 'B' });
+    expect(invoke(mwA).passed).toBe(true);
+    expect(invoke(mwA).passed).toBe(false);
+    // different limiter — still fresh
+    expect(invoke(mwB).passed).toBe(true);
+  });
+
+  it('includes the limiter name and retry seconds in the error message', () => {
+    jest.useFakeTimers();
+    jest.setSystemTime(new Date('2026-01-01T00:00:00Z'));
+    const mw = createRateLimiter({
+      windowMs: 30_000,
+      max: 1,
+      name: 'summarize',
+    });
+    invoke(mw);
+    const { error } = invoke(mw);
+    expect((error as Error).name).toContain('summarize');
+    expect((error as Error).name).toMatch(/Try again in \d+s/);
+    jest.useRealTimers();
+  });
+});

packages/api/src/routers/api/__tests__/aiSummarize.test.ts

@@ -151,6 +151,30 @@ describe('redactSecrets', () => {
       'error: database timeout after 30s',
     );
   });
+
+  it('redacts JSON-shape secrets (quoted key/value)', () => {
+    const input = '{"password":"s3cret","user":"alice"}';
+    const out = redactSecrets(input);
+    expect(out).not.toContain('s3cret');
+    expect(out).toContain('"password":"[REDACTED]"');
+    expect(out).toContain('"user":"alice"');
+  });
+
+  it('redacts JSON-shape with whitespace', () => {
+    const input = '{ "api_key" : "abc123" }';
+    const out = redactSecrets(input);
+    expect(out).not.toContain('abc123');
+    expect(out).toContain('[REDACTED]');
+  });
+
+  it('redacts HTTP-style secret headers', () => {
+    expect(redactSecrets('X-Api-Key: abc123')).toContain(
+      'X-Api-Key: [REDACTED]',
+    );
+    expect(redactSecrets('X-Auth-Token: xyz')).toContain(
+      'X-Auth-Token: [REDACTED]',
+    );
+  });
 });
 
 // ---------------------------------------------------------------------------

packages/api/src/routers/api/aiSummarize.ts

@@ -32,6 +32,15 @@ export const summarizeBodySchema = z.object({
   tone: z.enum(TONE_VALUES).optional(),
   // Optional conversation history for future follow-up-question flows.
   // Each message is bounded; history is bounded; prevents unbounded prompt growth.
+  //
+  // SECURITY / TRUST BOUNDARY: messages are client-supplied, which means a
+  // caller can claim the assistant previously said anything. That is fine for
+  // the current single-shot summarize flow (no downstream consumer trusts
+  // these as "authentic model output"). When a real follow-up UI is built,
+  // move conversation state to the server (keyed by a server-issued
+  // conversationId) instead of round-tripping messages through the client, or
+  // restrict `role` to 'user' only and let the server interleave its own
+  // prior assistant replies from storage.
   messages: z
     .array(
       z.object({
@@ -119,17 +128,41 @@ export function wrapContent(content: string): string {
 
 // ---------------------------------------------------------------------------
 // Secret redaction — scrubs obvious credentials from context before sending.
-// Catches common patterns: `password=...`, `token=...`, `api_key=...`,
-// `Authorization: Bearer ...`, JWT-shaped strings.
+// Best-effort allowlist; not a guarantee. Recipes matched today:
+// - key=value pairs:              password=secret, api_key=abc
+// - JSON key/value pairs:         "password": "secret"
+// - HTTP Authorization values:    Bearer ..., Basic ...
+// - HTTP-style secret headers:    X-Api-Key: abc, X-Auth-Token: xyz
+// - JWT-shaped strings:           eyJ...header.payload.signature
+//
+// Missing recipes (extend when spotted): URL-encoded values, basic-auth URLs
+// (user:pass@host), SSH private key blocks. If redaction fires in production,
+// count it so we can tell if users are routinely sending secrets.
 // ---------------------------------------------------------------------------
 
+const SECRET_KEY_TOKENS =
+  'password|passwd|pwd|secret|token|api[_-]?key|apikey|access[_-]?key|private[_-]?key|client[_-]?secret|authorization|auth';
+
 const REDACTION_PATTERNS: { re: RegExp; replace: string }[] = [
-  // key=value pairs with secret-ish keys
+  // key=value pairs
   {
-    re: /\b(password|passwd|pwd|secret|token|api[_-]?key|apikey|access[_-]?key|private[_-]?key|client[_-]?secret|authorization|auth)=([^\s,;&"'`]+)/gi,
+    re: new RegExp(`\\b(${SECRET_KEY_TOKENS})=([^\\s,;&"'\`]+)`, 'gi'),
     replace: '$1=[REDACTED]',
   },
-  // HTTP Authorization headers
+  // JSON-shape: "key": "value" or "key":"value" (quoted value)
+  {
+    re: new RegExp(`("(?:${SECRET_KEY_TOKENS})"\\s*:\\s*)"[^"]*"`, 'gi'),
+    replace: '$1"[REDACTED]"',
+  },
+  // HTTP-header shape: X-Api-Key: value  (colon-separated on one line)
+  {
+    re: new RegExp(
+      `\\b(x[-_]?(?:api[-_]?key|auth[-_]?token|access[-_]?token)|api[-_]?key)\\s*:\\s*([^\\s,;]+)`,
+      'gi',
+    ),
+    replace: '$1: [REDACTED]',
+  },
+  // Authorization headers
   {
     re: /Bearer\s+[A-Za-z0-9._~+/=-]+/gi,
     replace: 'Bearer [REDACTED]',

packages/app/src/components/__tests__/AISummarize.test.tsx

@@ -38,8 +38,13 @@ jest.mock('@/source', () => ({
   useSource: () => ({ data: undefined, isLoading: false }),
 }));
 
+const mockUseEventsAroundFocus = jest.fn((_args: unknown) => ({
+  rows: [] as unknown[],
+  meta: [] as unknown[],
+  isFetching: false,
+}));
 jest.mock('../DBTraceWaterfallChart', () => ({
-  useEventsAroundFocus: () => ({ rows: [], meta: [], isFetching: false }),
+  useEventsAroundFocus: (args: unknown) => mockUseEventsAroundFocus(args),
 }));
 
 let mockEasterEggVisible = true;
@@ -470,6 +475,54 @@ describe('AISummarizePatternButton', () => {
 // Direct import of the panel for isolated tests
 import AISummaryPanelComponent from '../aiSummarize/AISummaryPanel';
 
+// ---------------------------------------------------------------------------
+// supportsTraceContext gate — a subject without trace context support must
+// never enable the trace-span fetch, even when trace props are passed.
+// ---------------------------------------------------------------------------
+
+describe('trace context gate', () => {
+  beforeEach(() => {
+    mockUseEventsAroundFocus.mockClear();
+    mockMeData = { aiAssistantEnabled: true };
+    mockEasterEggVisible = false;
+  });
+
+  it('pattern subject never enables trace fetch', () => {
+    const pattern: Pattern = {
+      id: 'p1',
+      pattern: 'GET /api/<*>',
+      count: 10,
+      samples: [],
+    };
+    renderWithMantine(
+      <AISummarizePatternButton
+        pattern={pattern}
+        serviceNameExpression="ServiceName"
+      />,
+    );
+    expect(mockUseEventsAroundFocus).toHaveBeenCalled();
+    // Every call must have enabled=false for a pattern subject, regardless
+    // of other props — the subject doesn't support trace context.
+    for (const call of mockUseEventsAroundFocus.mock.calls) {
+      const args = call[0] as { enabled: boolean };
+      expect(args.enabled).toBe(false);
+    }
+  });
+
+  it('event subject without traceId does not enable trace fetch', () => {
+    renderWithMantine(
+      <AISummarizeButton
+        rowData={{ __hdx_body: 'test' }}
+        severityText="info"
+      />,
+    );
+    for (const call of mockUseEventsAroundFocus.mock.calls) {
+      const args = call[0] as { enabled: boolean };
+      expect(args.enabled).toBe(false);
+    }
+  });
+});
+
 describe('AISummaryPanel', () => {
   const AISummaryPanel = AISummaryPanelComponent;
 

packages/app/src/components/aiSummarize/eventSubject.ts

@@ -1,4 +1,5 @@
 // Subject: single log or trace event
+import { attrToString } from './formatHelpers';
 import { SummarySubject } from './subjects';
 
 export interface EventSubjectInput {
@@ -6,16 +7,9 @@ export interface EventSubjectInput {
   severityText?: string;
 }
 
-function coerceAttrValue(v: unknown): string {
-  if (v == null) return '';
-  if (typeof v === 'string') return v;
-  if (typeof v === 'number' || typeof v === 'boolean') return String(v);
-  try {
-    return JSON.stringify(v);
-  } catch {
-    return String(v);
-  }
-}
+// Effectively unlimited for event attributes — 500 is far above typical
+// attribute lengths but caps pathological cases.
+const coerceAttrValue = (v: unknown) => attrToString(v, 500);
 
 export function formatEventContent(
   { rowData, severityText }: EventSubjectInput,

packages/app/src/components/aiSummarize/formatHelpers.ts

@@ -0,0 +1,24 @@
+// Shared string-coercion helpers used by the subject formatters and the
+// trace context builder. Each of these was previously duplicated with
+// slight variations — keeping them here keeps the LLM-prompt output
+// consistent across subjects.
+
+/**
+ * Turn any attribute value into a short string suitable for the LLM prompt.
+ * Objects are JSON-stringified (with a try/catch for circular refs).
+ * Truncates at `maxLen` with an ellipsis.
+ */
+export function attrToString(v: unknown, maxLen = 100): string {
+  if (v == null) return '';
+  let s: string;
+  if (typeof v === 'string') s = v;
+  else if (typeof v === 'number' || typeof v === 'boolean') s = String(v);
+  else {
+    try {
+      s = JSON.stringify(v);
+    } catch {
+      s = String(v);
+    }
+  }
+  return s.length > maxLen ? s.slice(0, maxLen - 3) + '...' : s;
+}

packages/app/src/components/aiSummarize/patternSubject.ts

@@ -5,6 +5,7 @@ import {
   SEVERITY_TEXT_COLUMN_ALIAS,
 } from '@/hooks/usePatterns';
 
+import { attrToString } from './formatHelpers';
 import { SummarySubject } from './subjects';
 
 export interface PatternSubjectInput {
@@ -19,15 +20,9 @@ const SKIP_KEYS = new Set([
   'SortKey',
 ]);
 
-function coerceAttr(v: unknown): string {
-  if (typeof v === 'string') return v;
-  if (typeof v === 'number' || typeof v === 'boolean') return String(v);
-  try {
-    return JSON.stringify(v);
-  } catch {
-    return String(v);
-  }
-}
+// Per-attribute cap; 200 chars is enough for URLs, status codes, short
+// messages — anything longer is likely redacted body content.
+const coerceAttr = (v: unknown) => attrToString(v, 200);
 
 export function formatPatternContent({
   pattern,

packages/app/src/components/aiSummarize/traceContext.ts

@@ -2,18 +2,10 @@
 // Includes: summary stats, span groups with duration stats, and error spans.
 
 import { isErrorEvent } from './classifiers';
+import { attrToString } from './formatHelpers';
 
-// SpanAttributes come from ClickHouse Map columns — values can be string,
-// number, boolean, or nested objects depending on the source. Normalize
-// before using.
-export type TraceAttributeValue =
-  | string
-  | number
-  | boolean
-  | null
-  | undefined
-  | unknown;
-
+// SpanAttributes come from ClickHouse Map columns — values can be any type
+// depending on the source schema.
 export interface TraceSpan {
   Body?: string;
   ServiceName?: string;
@@ -22,7 +14,7 @@ export interface TraceSpan {
   SeverityText?: string;
   SpanId?: string;
   ParentSpanId?: string;
-  SpanAttributes?: Record<string, TraceAttributeValue>;
+  SpanAttributes?: Record<string, unknown>;
 }
 
 interface SpanGroup {
@@ -44,22 +36,6 @@ function fmtMs(ms: number): string {
   return `${(ms / 1000).toFixed(2)}s`;
 }
 
-// Coerce any attribute value to a short string for the LLM prompt.
-function attrToString(v: TraceAttributeValue, maxLen = 100): string {
-  if (v == null) return '';
-  let s: string;
-  if (typeof v === 'string') s = v;
-  else if (typeof v === 'number' || typeof v === 'boolean') s = String(v);
-  else {
-    try {
-      s = JSON.stringify(v);
-    } catch {
-      s = String(v);
-    }
-  }
-  return s.length > maxLen ? s.slice(0, maxLen - 3) + '...' : s;
-}
-
 function isSpanError(span: TraceSpan): boolean {
   return isErrorEvent({
     severity: span.SeverityText,

packages/app/src/components/aiSummarize/useAISummarizeState.ts

@@ -113,9 +113,13 @@ export function useAISummarizeState<TInput>({
       clearTimeout(timerRef.current);
       timerRef.current = null;
     }
-    // release abort flag synchronously on next microtask — in-flight
-    // callbacks queued before this point see abortRef=true and bail out;
-    // any mutate() called *after* this runs works normally.
+    // Release abort flag on the next microtask — in-flight callbacks queued
+    // before this point see abortRef=true and bail out; any mutate() called
+    // *after* this runs works normally.
+    //
+    // Uses queueMicrotask (not setTimeout(0)) deliberately: tests that enable
+    // jest.useFakeTimers() would freeze a setTimeout-based reset and block
+    // all subsequent onSuccess callbacks. Microtasks are not faked by Jest.
     queueMicrotask(() => {
       abortRef.current = false;
     });