fix: address 6 more CodeRabbit findings from third review round

Aureliolo · Aureliolo · commit 824ae2d8fc7b · 2026-04-05T15:37:33.000+02:00
- agent_engine: type PersonalityTrimPayload identifier fields (agent_id,
  agent_name, task_id) as NotBlankStr per CLAUDE.md identifier convention.
  Pydantic validators don't fire inside a TypedDict at runtime, but the
  alias documents intent and keeps the notifier contract consistent with
  the rest of the codebase.

- web/CLAUDE.md: resolve the Tooltip inconsistency -- the 'Creating New
  Components' and 'Base UI Adoption Decisions' sections no longer mention
  Tooltip as preferred Base UI, since the adoption table does not list it
  as adopted. Contributors are directed to reach for existing primitives
  first and add a row to the table if a real Tooltip requirement appears.

- stores/agents: null-guard WsEvent.payload before casting to
  Record&lt;string, unknown&gt;. The TypeScript 'as' cast is compile-time only
  -- a malformed broker sending null would throw on the next property
  access. The guard drops the event and logs the drop.

- stores/agents: sanitize attacker-controlled values in log.warn calls --
  'agent' (URL route param) in fetchAgentDetail, 'status' in the
  unknown-status branch of the agent.status_changed handler, and
  'event_type' in the new payload-null guard. Matches the
  'sanitizeForLog attacker-controlled fields' rule in web/CLAUDE.md.

- dialog.tsx: reuse the shared Button component inside DialogCloseButton
  via Base UI's render prop, instead of duplicating the icon-button
  styles. Keeps tokens, focus states, and hover semantics centralized.

- dialog.tsx: DialogDescription was text-muted (already fixed in prior
  commit, retained here).

- useGlobalNotifications.test: resolve the 'agents' binding by channel
  name via bindings.find(b =&gt; b.channel === 'agents') instead of by
  index, so adding unrelated subscriptions upstream cannot silently
  break the suite.
diff --git a/src/synthorg/engine/agent_engine.py b/src/synthorg/engine/agent_engine.py
@@ -13,6 +13,7 @@
 from synthorg.budget.currency import DEFAULT_CURRENCY
 from synthorg.budget.errors import BudgetExhaustedError, QuotaExhaustedError
 from synthorg.budget.quota import DegradationAction
+from synthorg.core.types import NotBlankStr  # noqa: TC001
 from synthorg.engine._security_factory import (
     make_security_interceptor,
     registry_with_approval_tool,
@@ -158,14 +159,19 @@ class PersonalityTrimPayload(TypedDict):
     """Structured payload forwarded to :data:`PersonalityTrimNotifier` callbacks.
 
     All keys are always present when the engine invokes the notifier from
-    :meth:`AgentEngine._prepare_context`.  ``trim_tier`` is one of ``1``, ``2``,
-    or ``3`` (enforced upstream by
+    :meth:`AgentEngine._prepare_context`.  Identifier fields (``agent_id``,
+    ``agent_name``, ``task_id``) are typed as :data:`NotBlankStr` to match
+    the project-wide identifier convention documented in CLAUDE.md -- the
+    Pydantic constraint is not enforced inside a ``TypedDict`` at runtime,
+    but the alias communicates the contract to readers and keeps this
+    surface consistent with the rest of the codebase.  ``trim_tier`` is
+    one of ``1``, ``2``, or ``3`` (enforced upstream by
     :class:`synthorg.engine.prompt_helpers.PersonalityTrimInfo`).
     """
 
-    agent_id: str
-    agent_name: str
-    task_id: str
+    agent_id: NotBlankStr
+    agent_name: NotBlankStr
+    task_id: NotBlankStr
     before_tokens: int
     after_tokens: int
     max_tokens: int
diff --git a/web/CLAUDE.md b/web/CLAUDE.md
@@ -111,7 +111,7 @@ When a new shared component is needed (not covered by the inventory above):
 3. Export props as a TypeScript interface
 4. Use design tokens exclusively -- no hardcoded colors, fonts, or spacing
 5. Import `cn` from `@/lib/utils` for conditional class merging
-6. **For primitives backed by Base UI** (Dialog, Popover, Menu, Tabs, Tooltip, etc. -- see the Adoption Decisions table below for the canonical list; `Select`, `Toast`, `Drawer`, `Meter`, `Combobox` are intentionally **not** adopted):
+6. **For primitives backed by Base UI** (Dialog, AlertDialog, Popover, Menu, Tabs -- see the Adoption Decisions table below for the canonical list; `Select`, `Toast`, `Drawer`, `Meter`, `Combobox`, `Tooltip` are intentionally **not** adopted):
    - Import from the specific subpath: `import { Dialog } from '@base-ui/react/dialog'`
    - Use the component's `render` prop for polymorphism: `<Dialog.Trigger render={<Button>Open</Button>} />`. Never spread props manually.
    - For Dialog/AlertDialog/Popover: compose with `Portal` + `Backdrop` + `Popup`. Popover and Menu additionally require a `Positioner` wrapper that owns `side` / `align` / `sideOffset`.
@@ -155,7 +155,7 @@ The dashboard uses Base UI as its primitive layer via shadcn/ui. Base UI ships s
 | `Select` | **Not adopted** | `SelectField` is a native `<select>` -- we intentionally keep the native mobile picker for iOS/Android UX. Replacing with a custom dropdown would lose that. |
 | `Combobox`, `Autocomplete` | **Not adopted (for now)** | No current typeahead call sites in the dashboard that would benefit. Re-evaluate when filterable selects become a feature requirement. |
 
-When adding new dashboard primitives, prefer Base UI components for accessibility (Dialog, Menu, Popover, Tabs, Tooltip, etc.) and keep the existing custom components (`SelectField`, `Drawer`, `Toast`, `ProgressGauge`, animations) where they are -- see the Adoption Decisions table above for the canonical rationale.
+When adding new dashboard primitives, prefer Base UI components for accessibility (Dialog, AlertDialog, Popover, Tabs, Menu) and keep the existing custom components (`SelectField`, `Drawer`, `Toast`, `ProgressGauge`, animations) where they are -- see the Adoption Decisions table above for the canonical rationale.  Tooltip is not yet adopted; reach for an existing primitive first and add a row to the table above if a real Tooltip requirement appears.
 
 ## Post-Training Reference (TypeScript 6 & Storybook 10)
 
diff --git a/web/src/__tests__/hooks/useGlobalNotifications.test.ts b/web/src/__tests__/hooks/useGlobalNotifications.test.ts
@@ -41,8 +41,12 @@ describe('useGlobalNotifications', () => {
     const { bindings } = options as {
       bindings: Array<{ channel: string; handler: (event: WsEvent) => void }>
     }
+    // Resolve by channel name rather than index so adding unrelated
+    // subscriptions upstream cannot silently break this test.
+    const agentsBinding = bindings.find((b) => b.channel === 'agents')
+    expect(agentsBinding).toBeDefined()
 
-    bindings[0]!.handler({
+    agentsBinding!.handler({
       event_type: 'agent.status_changed',
       channel: 'agents',
       timestamp: '2026-04-05T10:00:00Z',
@@ -59,8 +63,12 @@ describe('useGlobalNotifications', () => {
     const { bindings } = options as {
       bindings: Array<{ channel: string; handler: (event: WsEvent) => void }>
     }
+    // Resolve by channel name rather than index so adding unrelated
+    // subscriptions upstream cannot silently break this test.
+    const agentsBinding = bindings.find((b) => b.channel === 'agents')
+    expect(agentsBinding).toBeDefined()
 
-    bindings[0]!.handler({
+    agentsBinding!.handler({
       event_type: 'personality.trimmed',
       channel: 'agents',
       timestamp: '2026-04-05T10:00:00Z',
diff --git a/web/src/components/ui/dialog.tsx b/web/src/components/ui/dialog.tsx
@@ -1,6 +1,7 @@
 import { Dialog as BaseDialog } from '@base-ui/react/dialog'
 import { X } from 'lucide-react'
 import { cn } from '@/lib/utils'
+import { Button } from './button'
 
 export interface DialogProps {
   open: boolean
@@ -90,15 +91,16 @@ export interface DialogCloseButtonProps {
 export function DialogCloseButton({ className }: DialogCloseButtonProps) {
   return (
     <BaseDialog.Close
-      aria-label="Close"
-      className={cn(
-        'rounded-md p-1 text-muted-foreground transition-colors',
-        'hover:bg-card-hover hover:text-foreground',
-        'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent',
-        className,
-      )}
-    >
-      <X className="size-4" />
-    </BaseDialog.Close>
+      render={
+        <Button
+          variant="ghost"
+          size="icon"
+          aria-label="Close"
+          className={className}
+        >
+          <X className="size-4" />
+        </Button>
+      }
+    />
   )
 }
diff --git a/web/src/stores/agents.ts b/web/src/stores/agents.ts
@@ -3,6 +3,7 @@ import { listAgents, getAgent, getAgentPerformance, getAgentActivity, getAgentHi
 import { listTasks } from '@/api/endpoints/tasks'
 import { useToastStore } from '@/stores/toast'
 import { getErrorMessage } from '@/utils/errors'
+import { sanitizeForLog } from '@/utils/logging'
 import { createLogger } from '@/lib/logger'
 import type {
   AgentActivityEvent,
@@ -161,7 +162,9 @@ export const useAgentsStore = create<AgentsState>()((set, get) => ({
       })
     } catch (err) {
       if (_detailRequestName !== name) return
-      log.warn('Failed to load agent detail', { agent: name }, err)
+      // `name` originates from a URL segment / router param and is therefore
+      // attacker-controlled; sanitize before embedding in the structured log.
+      log.warn('Failed to load agent detail', { agent: sanitizeForLog(name) }, err)
       set({ detailLoading: false, detailError: getErrorMessage(err) })
     }
   },
@@ -226,6 +229,16 @@ export const useAgentsStore = create<AgentsState>()((set, get) => ({
   },
 
   updateFromWsEvent: (event) => {
+    // Runtime null-guard: `event.payload` is typed as `Record<string, unknown>`
+    // on the wire, but a malformed broker could still send `null` or a
+    // non-object.  The `as` cast below would not catch that, so we filter
+    // here once and treat every invalid envelope as a dropped event.
+    if (typeof event.payload !== 'object' || event.payload === null) {
+      log.warn('WS event dropped: payload is not an object', {
+        event_type: sanitizeForLog(event.event_type),
+      })
+      return
+    }
     if (event.event_type === 'agent.status_changed') {
       const payload = event.payload as Record<string, unknown>
       const agentId = payload.agent_id
@@ -238,8 +251,10 @@ export const useAgentsStore = create<AgentsState>()((set, get) => ({
         return
       }
       if (!VALID_RUNTIME_STATUSES.has(status)) {
+        // `status` arrives from an untrusted WebSocket payload, so sanitize
+        // before embedding in the structured log.
         log.warn('agent.status_changed received unknown status', {
-          status,
+          status: sanitizeForLog(status),
           knownStatuses: [...VALID_RUNTIME_STATUSES],
         })
         return
