refactor: migrate UI primitives to @spaceui packages#513
refactor: migrate UI primitives to @spaceui packages#513
Conversation
…overrides, worker memory tools - Load ConversationSettings from PortalConversationStore at channel creation for portal conversations instead of hardcoding defaults - Add ModelOverrides struct for per-process model selection (channel, branch, worker, compactor) with blanket fallback - Thread model overrides through ChannelState → Branch, Worker, Compactor - Wire worker memory tools (recall/save/delete) based on WorkerMemoryMode - Add memory persistence guard (Ambient/Off conversations skip persistence) - Pull available models dynamically from models.dev catalog filtered by configured providers - Resolve default model from agent routing config - Wire frontend Apply Settings to PUT API endpoint - Refactor ConversationSettingsPanel as reusable popover component with per-process model overrides in advanced section - Clean up ConversationsSidebar: match app background, full-width new conversation button, compact item layout Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tence, binding defaults
Phase 1-3 of channel settings unification (design doc included).
- Add ResponseMode enum (Active/Quiet/MentionOnly) to ConversationSettings,
replacing the fragmented listen_only_mode/require_mention system
- Add save_attachments field to ConversationSettings
- Channel suppression logic now checks response_mode instead of listen_only_mode
- /quiet, /active, /mention-only commands set response_mode and persist to DB
- New channel_settings SQLite table for per-channel settings on platform channels
- ChannelSettingsStore with get/upsert methods
- GET/PUT /channels/{channel_id}/settings API endpoints
- Platform channels load settings from channel_settings table at creation
- Gear icon + settings popover on ChannelCard using ConversationSettingsPanel
- [bindings.settings] TOML support — binding-level defaults for matched channels
- resolve_agent_for_message returns binding settings alongside agent_id
- Settings resolution: per-channel DB > binding defaults > agent defaults > system
- response_mode field on ChannelConfig with listen_only_mode backwards compat
- Design doc: docs/design-docs/channel-settings-unification.md
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 4 cleanup of channel settings unification: - Remove listen_only_mode and listen_only_session_override fields from Channel - Remove sync_listen_only_mode_from_runtime() and set_listen_only_mode() - Add is_suppressed() helper that checks resolved_settings.response_mode - Remove channel_listen_only_explicit from RuntimeConfig - Simplify RuntimeConfig::set_settings() — no longer manages listen_only state - Remove channel_listen_only_mode* methods and constants from SettingsStore - Rename listen_only_mode param to is_suppressed in standalone functions - All response mode state now flows through ConversationSettings/ResponseMode Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add ProcessEvent::SettingsUpdated — emitted by API when settings change - Channel handle_event re-loads settings from DB on SettingsUpdated - update_channel_settings and update_portal_conversation emit the event via channel_state.deps.event_tx for immediate hot-reload - Channel reload_settings() re-reads from ChannelSettingsStore or PortalConversationStore and updates resolved_settings + shared state - ChannelCard shows response mode badge (Quiet=amber, Mention Only=red) only when not Active (default) - list_channels API includes response_mode and model from running channel state - Portal header shows current model name and response mode badge Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
stopPropagation on the trigger button was preventing Radix's PopoverTrigger from receiving the click event. Manually toggle showSettings state in the onClick handler while still preventing the parent Link navigation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add max-h-[80vh] + overflow-y-auto so the popover scrolls instead of
going off-screen. Add collisionPadding={16} so Radix repositions the
popover to stay within the viewport.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- /status now shows resolved model overrides instead of routing defaults - Reset settings state when switching portal conversations - ChannelCard popover waits for channel settings to load before rendering - parse_response_mode: listen_only_mode=false maps to Active (not ignored), unknown response_mode strings return None instead of defaulting to Active - Binding settings only override enum fields when explicitly set in TOML, omitted fields inherit from agent/system defaults - Idle worker resume path loads from ChannelSettingsStore for platform channels (not just PortalConversationStore) - Batched turn path respects MemoryMode::Off — skips memory bulletin, working memory, and channel activity map rendering - Channel settings API validates channel exists before get/put Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ChannelCard: reset settings from DB on popover reopen, invalidate ["channels"] query on save so badge refreshes - WebChatPanel: hydrate settings from conversationsData on switch, fix declaration ordering - channel.rs reload_settings: preserve existing settings on DB errors instead of resetting to defaults - channel.rs set_response_mode: load existing settings and merge response_mode instead of overwriting all fields, use tokio::spawn for non-blocking DB write - channel.rs /help: update descriptions for /quiet, add /mention-only - channels.rs: log event_tx.send errors instead of discarding - config/load.rs: log unknown enum values in binding settings - main.rs: log DB errors in idle worker resume path, preserve binding defaults on DB errors in channel creation path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit migrates the interface to use @spaceui/primitives and @spaceui/tokens: ## Migrated Primitives (16 components) - Badge, Banner, Button, Card - Checkbox, Dialog, FilterButton, Input - Loader, NumberStepper, Popover - RadioGroup, Select, Toggle, ToggleGroup, Tooltip ## Token Migration - Removed interface/src/ui/style/colors.scss - Colors now sourced from @spaceui/tokens - Updated tailwind.config.ts to use SpaceUI tokens - Updated useTheme.ts for proper token integration ## Cleanup - Removed dead code and unused color definitions - Updated Settings.tsx with migrated components All components now consume design tokens from @spaceui/tokens package.
WalkthroughThis PR introduces a comprehensive architectural refactor centered on conversation settings and portal conversation management. It renames the webchat messaging adapter to portal, migrates frontend UI components to SpaceUI primitives, adds per-conversation and per-channel settings persistence, and restructures backend agent/worker/channel logic to support configurable response modes, memory handling, and model overrides. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| "@radix-ui/react-switch": "^1.2.6", | ||
| "@radix-ui/react-tabs": "^1.1.13", | ||
| "@radix-ui/react-tooltip": "^1.2.8", | ||
| "@spaceui/primitives": "file:../../spaceui/packages/primitives", |
There was a problem hiding this comment.
These deps use file:../../spaceui/..., which points outside this repo (and there’s no spaceui/ directory checked in). That makes bun install in a clean checkout/CI non-reproducible. If these are meant to be the published @spaceui packages, consider switching to semver versions (also worth double-checking Tailwind peer dependency expectations vs interface’s Tailwind v3).
| ); | ||
| } | ||
| ({className, variant, padding, ...props}, ref) => { | ||
| void ref; |
There was a problem hiding this comment.
forwardRef currently drops the ref (void ref;). If callers ever pass refs (focus mgmt, measurements, etc.), they’ll silently break. Either remove forwardRef or forward through to the SpaceUI component.
| void ref; | |
| const paddingClass = padding === "none" ? "p-0" : undefined; | |
| return ( | |
| <SpaceUICardCompat | |
| ref={ref as any} | |
| className={cx(cardStyles({variant}), paddingClass, className)} | |
| {...props} | |
| /> | |
| ); |
| ALTER TABLE portal_conversations ADD COLUMN settings TEXT; | ||
|
|
||
| -- Create index for efficient filtering by settings (if we query by specific settings later) | ||
| CREATE INDEX IF NOT EXISTS idx_portal_conversations_settings ON portal_conversations(id, settings) WHERE settings IS NOT NULL; |
There was a problem hiding this comment.
Since id is the PK, including it in this index doesn’t help filtering by settings and adds write overhead. If you keep this index, it probably wants to be on settings alone (or an expression index once query patterns are known).
| CREATE INDEX IF NOT EXISTS idx_portal_conversations_settings ON portal_conversations(id, settings) WHERE settings IS NOT NULL; | |
| CREATE INDEX IF NOT EXISTS idx_portal_conversations_settings ON portal_conversations(settings) WHERE settings IS NOT NULL; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/config/types.rs (1)
1574-1579:⚠️ Potential issue | 🟡 MinorReject portal messages that lack an
agent_id.Right now a
portalmessage withagent_id = Nonefalls through to the generic routing logic, so the first matching portal binding can win and the turn gets routed to the wrong agent instead of being rejected as malformed.Suggested fix
- // For portal messages, match based on agent_id in the message - if message.source == "portal" - && let Some(message_agent_id) = &message.agent_id - { - return message_agent_id.as_ref() == self.agent_id; - } + if message.source == "portal" { + return message.agent_id.as_deref() == Some(self.agent_id.as_str()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/types.rs` around lines 1574 - 1579, When the message.source is "portal" you must explicitly reject messages with message.agent_id == None instead of letting them fall through; inside the portal-routing branch (the block referencing message.source, message.agent_id, and self.agent_id) change the logic so that if message.agent_id is Some(id) you compare id.as_ref() == self.agent_id and return that result, and if message.agent_id is None you immediately return false to mark the message as malformed.src/messaging/portal.rs (1)
178-181:⚠️ Potential issue | 🟡 MinorUse a single connection for this in-memory SQLite test.
sqlite::memory:is scoped per connection. With a pooledSqlitePool, the setup statements and the laterConversationLoggerread can hit different connections, which makes this test intermittently fail with missing-table data.Suggested fix
- let pool = SqlitePool::connect("sqlite::memory:") - .await - .expect("in-memory sqlite should connect"); + let pool = sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .expect("in-memory sqlite should connect");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/portal.rs` around lines 178 - 181, The test currently creates a pooled connection with SqlitePool::connect which can hit different in-memory connections; change the setup to create and reuse a single connection instead: replace SqlitePool::connect(...) with a single sqlx::SqliteConnection (e.g. SqliteConnection::connect("sqlite::memory:").await) and run the schema/setup statements on that connection, then make the test/ConversationLogger use that same connection (or adapt ConversationLogger to accept/reuse the SqliteConnection) so both setup and reads operate on the exact same in-memory DB.interface/src/api/schema.d.ts (1)
1940-2007:⚠️ Potential issue | 🟠 MajorRegenerate the OpenAPI schema from the Rust route source.
The generated contract in this file still exposes
/webchat/*endpoints, but the Rust backend defines only/portal/*and/conversation-defaultsroutes. The SDK inpackages/api-client/src/client.tscorrectly targets/portal/*routes that match the backend implementation. The published contract and published client now disagree on the canonical API surface.Verify by checking the Rust source (
src/api/portal.rs): all webchat endpoints are registered as/portal/*routes, not/webchat/*. Regenerateinterface/src/api/schema.d.tsfrom the OpenAPI spec to align with the actual backend.Also applies to: 8446-8685
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/schema.d.ts` around lines 1940 - 2007, The OpenAPI TypeScript contract still contains /webchat/* endpoints (e.g., "/webchat/conversations", "/webchat/conversations/{session_id}", "/webchat/history", "/webchat/send") but the Rust backend exposes these routes under /portal/* (see portal route registrations in src/api/portal.rs); regenerate the OpenAPI schema and re-run the TypeScript generator to replace all /webchat/* entries with the correct /portal/* (and ensure /conversation-defaults is present) so the generated interface (interface/src/api/schema.d.ts) matches the actual backend routes and the SDK in packages/api-client/src/client.ts. Ensure you run the same OpenAPI generation step used in the repo (openapi export from Rust router or build script) and commit the updated schema.d.ts.
🟠 Major comments (37)
interface/src/ui/NumberStepper.tsx-39-40 (1)
39-40:⚠️ Potential issue | 🟠 MajorConnect the visible label and help text to the stepper input programmatically.
After migration, the
labelanddescriptionare visual siblings without programmatic connection to the stepper. This breaks:
- Clicking the label won't focus the input (label has no
htmlFor)- Screen readers won't associate them with the control (no
aria-labelledby/aria-describedby)Thread
idthrough toSpaceUINumberStepperCompatand addaria-labelledby/aria-describedby, or use the primitive's built-in labeling API if available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/ui/NumberStepper.tsx` around lines 39 - 40, The label and description are not programmatically linked to the input; update NumberStepper to accept/forward an id (or generate one) to SpaceUINumberStepperCompat, set the label element's htmlFor to that id, and add aria-labelledby and aria-describedby on the stepper input pointing to the label and description ids (e.g., `${id}-label`, `${id}-desc`); alternatively use the primitive labeling API on SpaceUINumberStepperCompat if it exposes props for label/description linkage—ensure the same id tokens are used consistently so clicking the label focuses the input and screen readers associate the text.interface/src/ui/NumberStepper.tsx-5-5 (1)
5-5:⚠️ Potential issue | 🟠 MajorAddress type erasure and inaccessible label structure.
The
unknown as React.ComponentType<any>cast (line 5) disables prop validation at the integration boundary. More immediately, the label and description (lines 39-40) are rendered separately from the primitive withouthtmlFororaria-labelledbylinking, breaking the semantic association for assistive technology.Additionally, the compact variant styling (line 50) uses descendant selectors (
[&>div>span],[&>div>button]) that couple to the internal DOM structure ofSpaceUINumberStepper, creating fragility if the primitive's markup changes.At minimum:
- Add
idto the primitive wrapper and link the label viahtmlForand description viaaria-describedby.- Replace the
anycast with an explicit compatibility type to catch prop mismatches early.Suggested changes
-const SpaceUINumberStepperCompat = SpaceUINumberStepper as unknown as React.ComponentType<any>; +type SpaceUINumberStepperCompatProps = { + value: number; + onChange: (value: number) => void; + min?: number; + max?: number; + step?: number; + allowFloat?: boolean; + showProgress?: boolean; + className?: string; +}; + +const SpaceUINumberStepperCompat = + SpaceUINumberStepper as unknown as React.ComponentType<SpaceUINumberStepperCompatProps>;For accessibility, assign an
idto the stepper element and reference it from the label and description:<div ref={ref} className="flex flex-col gap-1.5"> - {label && <label className="text-sm font-medium text-ink">{label}</label>} - {description && <p className="text-tiny text-ink-faint">{description}</p>} + {label && <label htmlFor="stepper-input" className="text-sm font-medium text-ink">{label}</label>} + {description && <p id="stepper-description" className="text-tiny text-ink-faint">{description}</p>} <div className={cx("flex items-center gap-2.5", (label || description) && "mt-1")}> <SpaceUINumberStepperCompat + id="stepper-input" + aria-describedby={description ? "stepper-description" : undefined} ... />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/ui/NumberStepper.tsx` at line 5, Replace the unsafe "unknown as React.ComponentType<any>" cast on SpaceUINumberStepper by declaring an explicit compatibility prop type (e.g., SpaceUINumberStepperProps) that matches the stepper's public props and use React.ComponentType<SpaceUINumberStepperProps> for SpaceUINumberStepperCompat; add a stable id (generated from props.id or with useId) and pass it into the primitive wrapper (SpaceUINumberStepper / the element rendered by NumberStepper) and set the label's htmlFor to that id and the description's id referenced by aria-describedby on the primitive; finally remove fragile descendant selectors in the compact variant and instead set a data-compact or className on the wrapper and target its immediate children (e.g., wrapper[data-compact] > span, wrapper[data-compact] > button) so styling does not depend on internal DOM shape.interface/src/ui/Badge.tsx-37-55 (1)
37-55:⚠️ Potential issue | 🟠 MajorForward the
refandsizethrough to the underlying component.Line 39 explicitly discards the forwarded
refwithvoid ref;, breaking theReact.forwardRefcontract for any caller that needs to measure or focus the badge. Line 54 hardcodessize="sm"toSpaceUIBadgeCompateven though callers pass bothsize="sm"andsize="md"(confirmed usage in TaskBoard.tsx).The wrapper applies the size to CSS classes correctly (line 55), but the underlying component never receives the actual size value, so any internal size-dependent behavior in
@spaceui/primitiveswill always usesminstead of the caller's intended size. Either forward bothrefandsizethrough, or remove them from the public API instead of silently ignoring them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/ui/Badge.tsx` around lines 37 - 55, The Badge component is discarding the forwarded ref and hardcoding size to "sm"; update the React.forwardRef wrapper (Badge) to stop ignoring ref (remove the `void ref;`) and pass the received ref to SpaceUIBadgeCompat, and forward the incoming `size` prop (from BadgeProps) instead of the hardcoded `size="sm"`, keeping className using badgeStyles({variant, size})—i.e., propagate both ref and size into SpaceUIBadgeCompat so consumers and internal size-dependent behavior work correctly.interface/src/ui/Popover.tsx-1-8 (1)
1-8:⚠️ Potential issue | 🟠 MajorThis removes part of the existing
Popovermodule surface.The re-export-only rewrite drops
PopoverHeader,PopoverFooter, and the localPopoverContentPropstype. Any existing import frominterface/src/ui/Popoverthat relies on those shims becomes a compile-time break, which doesn't fit the PR's stated non-breaking migration goal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/ui/Popover.tsx` around lines 1 - 8, The new re-export-only Popover.tsx removed existing symbols causing a breaking change; restore the previous module surface by re-introducing exports for PopoverHeader and PopoverFooter and the local PopoverContentProps type (or re-export them from "@spaceui/primitives" if the primitives package exposes compatible symbols) alongside the existing PopoverRoot/PopoverTrigger/etc. Ensure the file still exports PopoverHeader, PopoverFooter, PopoverContentProps and the current Popover alias (PopoverRoot as Popover) so existing imports continue to compile.interface/src/ui/Card.tsx-20-38 (1)
20-38:⚠️ Potential issue | 🟠 Major
padding="sm"andpadding="lg"are ignored here.
cardStylesdefines the padding variants, but the render path only callscardStyles({variant})and special-cases"none". Anything other than"none"now falls back to the default"md"padding.💡 Minimal fix
export const Card = React.forwardRef<HTMLDivElement, CardProps>( ({className, variant, padding, ...props}, ref) => { void ref; - const paddingClass = padding === "none" ? "p-0" : undefined; - return ( <SpaceUICardCompat - className={cx(cardStyles({variant}), paddingClass, className)} + className={cx(cardStyles({variant, padding}), className)} {...props} /> ); }, );Also applies to: 47-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/ui/Card.tsx` around lines 20 - 38, The padding variants in cardStyles are never applied because the render calls only pass variant (and special-case "none")—update the call site(s) that invoke cardStyles (e.g., inside the Card component render) to forward the padding prop as well: call cardStyles({ variant, padding }) and preserve the existing special-case logic for "none" if present; ensure any other invocations (lines noted around 47-55) are updated similarly so "sm" and "lg" take effect.interface/src/ui/Button.tsx-6-27 (1)
6-27:⚠️ Potential issue | 🟠 Major
buttonStylesis now a no-op export.All size and variant branches return
"", while the actual styling lives in the private maps below. Any caller still composing classes withbuttonStyles({variant, size})will silently lose styling after this refactor.Also applies to: 39-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/ui/Button.tsx` around lines 6 - 27, buttonStyles was turned into a no-op (all variant/size values are empty strings), so callers using buttonStyles({ variant, size }) silently lose styling; revert buttonStyles to compute classes by delegating to the private maps used below (or reintroduce the original class strings) instead of returning empty strings — update the buttonStyles cva variants to reference the existing private style maps (or call a helper that merges those maps based on variant and size) so button components receive the correct composed class names (refer to buttonStyles and the private maps declared below in this file).interface/src/ui/Select.tsx-1-17 (1)
1-17:⚠️ Potential issue | 🟠 MajorImport the React type locally instead of relying on an ambient namespace.
export type {ComponentPropsWithoutRef} from "react"only re-exports the type for consumers; it does not putReactin scope in this file.React.ComponentPropsWithoutReftherefore depends on an undefined namespace and fails under strict TypeScript mode. All other UI components in this directory (e.g., Button.tsx, Checkbox.tsx) explicitly import React.Minimal fix
+import type {ComponentPropsWithoutRef} from "react"; export type {ComponentPropsWithoutRef} from "react"; -export type SelectProps = React.ComponentPropsWithoutRef< +export type SelectProps = ComponentPropsWithoutRef< typeof import("@spaceui/primitives").SelectRoot >;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/ui/Select.tsx` around lines 1 - 17, The file uses React.ComponentPropsWithoutRef in SelectProps but never imports React into scope; add a local type import from React (e.g., import type { ComponentPropsWithoutRef } from "react") and then change SelectProps to use ComponentPropsWithoutRef directly (SelectProps = ComponentPropsWithoutRef<typeof import("@spaceui/primitives").SelectRoot>); keep or adjust the existing export type re-export as needed so the local type import resolves under strict TypeScript.interface/src/components/ChannelCard.tsx-97-107 (1)
97-107:⚠️ Potential issue | 🟠 MajorAdd error state handling to the popover.
The two useQuery hooks (lines 97-107) only destructure
data, notisErrororerror. When either query fails,defaultsorchannelSettingsDataremain undefined, and the conditional at line 200 shows theLoading...placeholder indefinitely. The popover should check query error states and render an error message instead of a perpetual loading state so users can recover from failed API calls.This also applies to lines 200-211.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ChannelCard.tsx` around lines 97 - 107, The popover currently only reads `data` from the two useQuery calls (`defaults` and `channelSettingsData`) and treats undefined as "Loading..." which never recovers on failure; update the queries to also destructure `isError`, `error`, and `isFetching` (e.g. const { data: defaults, isError: defaultsError, error: defaultsErrorObj, isFetching: defaultsFetching } = useQuery(...)) and change the popover rendering logic (the conditional around `showSettings` / the Loading... branch) to check those flags and render a user-facing error state when `isError` is true (show a concise error message and a retry button that calls the query's `refetch` or toggles `enabled`) instead of the perpetual Loading placeholder; ensure both `defaults` and `channelSettingsData` queries are handled this way so either failure shows the error UI and allows retry.src/conversation/channel_settings.rs-34-42 (1)
34-42:⚠️ Potential issue | 🟠 MajorPropagate malformed settings rows instead of treating them as absent.
Both
try_get(...).ok()andserde_json::from_str(...).ok()turn read/decode failures intoNone. That hides corruption and makes the next save path behave as if the channel never had persisted settings.Suggested fix
- Ok(row.and_then(|r| { - r.try_get::<String, _>("settings").ok().and_then(|s| { - if s.is_empty() || s == "{}" { - None - } else { - serde_json::from_str(&s).ok() - } - }) - })) + let Some(row) = row else { + return Ok(None); + }; + + let settings_json: String = row + .try_get("settings") + .map_err(|error| anyhow::anyhow!(error))?; + + if settings_json.is_empty() || settings_json == "{}" { + return Ok(None); + } + + let settings = serde_json::from_str::<ConversationSettings>(&settings_json) + .map_err(|error| anyhow::anyhow!(error))?; + + Ok(Some(settings))As per coding guidelines, "Don't silently discard errors. No
let _ =on Results. Handle them, log them, or propagate them."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/channel_settings.rs` around lines 34 - 42, The current closure hides read/parse failures by using try_get(...).ok() and serde_json::from_str(...).ok(), turning errors into None; change it to propagate errors instead: remove the .ok() calls and handle the Result from r.try_get::<String, _>("settings") and from serde_json::from_str(&s) (or use .and_then(|s| if s.is_empty() || s == "{}" { Ok(None) } else { serde_json::from_str(&s).map(Some) })) so that parse/read failures return Err (or map to your function's error type) rather than silently returning None; update the surrounding Ok(row.and_then(...)) to return/propagate the error from try_get or from_str accordingly (use map_err/transpose or match) so malformed settings rows surface instead of being treated as absent.interface/src/components/ChannelCard.tsx-94-95 (1)
94-95:⚠️ Potential issue | 🟠 MajorInitialize editable settings only once per popover session.
The effect at lines 109-113 re-applies server settings whenever the query updates, so any refetch while the popover is open will wipe unsaved edits. Use a flag to sync settings only on the initial open, not on subsequent query refetches.
Suggested fix
const [showSettings, setShowSettings] = useState(false); const [settings, setSettings] = useState<ConversationSettings>({}); +const [didInitSettings, setDidInitSettings] = useState(false); useEffect(() => { - if (showSettings) { - setSettings(channelSettingsData?.settings ?? {}); - } -}, [channelSettingsData, showSettings]); + if (!showSettings) { + setDidInitSettings(false); + return; + } + + if (!didInitSettings && channelSettingsData) { + setSettings(channelSettingsData.settings ?? {}); + setDidInitSettings(true); + } +}, [channelSettingsData, didInitSettings, showSettings]);Also, the conditional at line 201 treats query failures the same as loading, leaving the panel stuck on "Loading...". Check for error states and display appropriate feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ChannelCard.tsx` around lines 94 - 95, The popover's editable settings (settings, setSettings, ConversationSettings) are being re-synced every time the server query updates, wiping unsaved edits; change the effect that currently copies server values into settings so it only runs when the popover is opened (use showSettings/setShowSettings) or when the popover transitions from closed→open (use a flag or check previous showSettings) to initialize settings once per popover session. Additionally, update the render logic that currently treats query failures like loading — inspect the query result/error (e.g., conversationQuery / whatever query variable you use) and render an error state/message when query.error is present instead of showing "Loading...".docs/design-docs/channel-settings-unification.md-98-112 (1)
98-112:⚠️ Potential issue | 🟠 Major
MentionOnlyis specified two different ways.The enum/table say unmatched messages are still recorded in history, but the later routing explanation and Decisions
#1sayrequire_mentiondrops them before a channel/history exists. Please resolve that contradiction before implementation, or the backend, UI, and migration plan will target different behaviors.Also applies to: 153-162, 263-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/channel-settings-unification.md` around lines 98 - 112, The documentation currently contradicts on MentionOnly behavior: update the spec so MentionOnly (enum value MentionOnly) has a single clear behavior and make require_mention consistent with it; decide whether unmatched messages are recorded in history (lightweight) or dropped before channel/history creation, then update all references (the enum description, the behavior table, the routing explanation, and Decisions `#1`) to state that single behavior and use the exact symbols MentionOnly and require_mention when describing logic so backend, UI, and migration plans implement the same flow.interface/src/components/ConversationSettingsPanel.tsx-185-188 (1)
185-188:⚠️ Potential issue | 🟠 MajorUse the inherited response-mode default here.
Unlike memory and delegation,
currentResponseModefalls back to hard-coded"active"instead ofdefaults.response_mode, so inherited quiet / mention-only conversations render as Active and show the wrong description.Suggested fix
- const currentResponseMode = currentSettings.response_mode || "active"; + const currentResponseMode = + currentSettings.response_mode ?? defaults.response_mode ?? "active";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ConversationSettingsPanel.tsx` around lines 185 - 188, currentResponseMode is falling back to a hard-coded "active" instead of using the inherited default; change its fallback from the literal "active" to defaults.response_mode so it respects inherited response-mode values (update the expression that sets currentResponseMode which reads currentSettings.response_mode || "active" to use defaults.response_mode instead).src/agent/channel_dispatch.rs-541-546 (1)
541-546:⚠️ Potential issue | 🟠 MajorDon't silently treat
summaryasnone.This path warns and hands the worker an empty history, so selecting
WorkerHistoryMode::Summaryquietly degrades context with no user-visible failure. Either block this mode until summarization exists or provide a deterministic fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_dispatch.rs` around lines 541 - 546, The match arm handling WorkerHistoryMode::Summary in channel_dispatch.rs currently warns and returns an empty Vec silently; change it to either (a) return an explicit error/result so callers fail fast (e.g., return Err(UnsupportedHistoryMode::Summary) or a WorkerError) or (b) implement a deterministic fallback with an explicit, non-silent record (e.g., fall back to WorkerHistoryMode::None or Full but emit a clear error-level log and metric) instead of quietly handing an empty history; update the match arm for WorkerHistoryMode::Summary accordingly (and the surrounding function signature/return type if needed) so the behavior is explicit and discoverable.src/agent/channel_dispatch.rs-514-535 (1)
514-535:⚠️ Potential issue | 🟠 MajorKeep worker launches context-free.
This now pulls channel knowledge/activity/history into worker prompts and passes cloned conversation history into
Worker::new*(). That breaks the worker/branch split and makes worker behavior depend on channel context instead of a fresh prompt + task.Based on learnings: Don't give workers channel context. Workers get a fresh prompt and a task. If a worker needs conversation context, that's a branch, not a worker; pass history as external state via
.with_history(&mut history)on each agent prompt call, not as part of agent state.Also applies to: 548-562, 569-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_dispatch.rs` around lines 514 - 535, The code injects channel knowledge/history into the worker's system prompt (see render_working_memory, system_prompt and the worker creation via Worker::new*), which makes workers context-dependent; remove the block that appends knowledge_synthesis and working_memory to system_prompt so Worker instances are created with a fresh prompt and no channel history. Instead, surface any needed conversation history to the caller/branch and pass it into agent requests using .with_history(&mut history) when invoking the agent prompt; ensure Worker::new* calls and any cloned conversation history are not used to seed worker state (also apply the same removal for the other similar blocks around render_working_memory in this file).src/main.rs-1923-1972 (1)
1923-1972:⚠️ Potential issue | 🟠 MajorInherited conversation defaults disappear on these paths.
Channel creation applies binding defaults only in memory, and every
ResolvedConversationSettings::resolve(...)call here usesNonefor the final defaults layer. That means channels without an explicit settings row fall straight back to system defaults, and resumed channels cannot recover the binding-derived baseline they were originally created with.Also applies to: 2186-2254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1923 - 1972, The resolve calls for conversation settings (in the resolved_settings block and the similar block at 2186-2254) always pass None for the "binding defaults" layer, so resumed conversations lose the channel-binding-derived defaults; update the match arms that call spacebot::conversation::settings::ResolvedConversationSettings::resolve (both when portal_store.get returns Some(conv) and when channel_store.get returns Some(settings), and the fallback paths) to supply the appropriate binding-defaults value instead of None — obtain those defaults from the channel binding info available for the conversation (e.g., derive them from ChannelSettingsStore or the channel metadata used at creation) and pass that object into ResolvedConversationSettings::resolve so resumed channels recover their original binding baseline.src/config/load.rs-1571-1585 (1)
1571-1585:⚠️ Potential issue | 🟠 MajorSynchronize the legacy bool with the resolved enum.
These builders can now produce contradictory
ChannelConfigvalues whenresponse_modeis set withoutlisten_only_mode. For example,[defaults.channel] response_mode = "quiet"still leaveslisten_only_modeat its inherited/default value, and a per-agentresponse_mode = "active"can still inheritlisten_only_mode = truefrom defaults. Any remaining legacy reader of the bool will observe the wrong behavior.Also applies to: 1783-1798
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` around lines 1571 - 1585, Compute response_mode first (as you already do via parse_response_mode) and then ensure the legacy bool listen_only_mode is derived from that resolved enum rather than left inconsistent: after response_mode is set, determine listen_only_mode by taking channel_config.listen_only_mode.unwrap_or(base_defaults.channel.listen_only_mode) and then override/normalize it to match response_mode (e.g., set true for response modes that imply listen-only and false for active modes, or use a helper like response_mode.is_listen_only()). Apply the same change to the other builder block that constructs ChannelConfig (the similar code at the second occurrence) so the enum and legacy bool are always synchronized.src/config/load.rs-2345-2387 (1)
2345-2387:⚠️ Potential issue | 🟠 MajorPartial
[bindings.settings]tables no longer inherit.
b.settings.map(...)always returnsSome(ConversationSettings { ..Default::default() })once the table exists. That means a binding that only setsmodel, or only contains an unrecognized enum value, now pinsmemory,delegation, andresponse_modetoConversationSettings::default()instead of leaving them inherited. BecauseBinding.settingsstops beingNone, downstream merge logic can’t distinguish “unset” from “explicitly defaulted here”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` around lines 2345 - 2387, The current b.settings.map(...) always returns Some(ConversationSettings::default()) whenever the TOML table exists, losing the ability to distinguish "unset" fields; modify the closure so it tracks whether any field was actually provided/recognized (e.g. introduce a boolean like any_set), only set cs fields when the corresponding s.field.is_some() or an enum string is recognized (set any_set = true), and return Some(cs) only if any_set is true, otherwise return None—update the mapping around ConversationSettings, the memory/delegation/response_mode match arms, and the save_attachments/model assignments accordingly.interface/src/components/WebChatPanel.tsx-188-189 (1)
188-189:⚠️ Potential issue | 🟠 MajorReset the selected conversation when
agentIdchanges.
useState(getPortalSessionId(agentId))only runs on the first mount. If this component is reused across/agents/:agentIdtransitions,usePortal()will keep reading and writing the previous agent’s conversation until the user manually switches it.[suggested_follow_up]
Possible fix
const [activeConversationId, setActiveConversationId] = useState<string>(getPortalSessionId(agentId)); + +useEffect(() => { + setActiveConversationId(getPortalSessionId(agentId)); +}, [agentId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/WebChatPanel.tsx` around lines 188 - 189, The initial state activeConversationId is seeded with getPortalSessionId(agentId) only on first mount, so when agentId changes the selected conversation must be reset; add an effect that watches agentId and calls setActiveConversationId(getPortalSessionId(agentId)) (and optionally clears related error/isSending state if needed) so usePortal(agentId, activeConversationId) always operates on the current agent’s conversation. Ensure the effect references agentId, setActiveConversationId and getPortalSessionId(agentId) so the state updates on agentId transitions.interface/src/components/WebChatPanel.tsx-343-347 (1)
343-347:⚠️ Potential issue | 🟠 MajorLabel the icon-only settings trigger.
Without an accessible name, assistive tech only announces a generic “button”, which makes the conversation settings entrypoint hard to discover.
Possible fix
-<Button variant="ghost" size="icon" className="h-7 w-7"> +<Button + variant="ghost" + size="icon" + className="h-7 w-7" + aria-label="Conversation settings" +>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/WebChatPanel.tsx` around lines 343 - 347, The settings Popover trigger is an icon-only Button (within Popover/PopoverTrigger) that lacks an accessible name; add an accessible label to the Button so screen readers announce it (e.g., add aria-label="Conversation settings" or aria-labelledby) on the Button used with HugeiconsIcon/Settings02Icon and keep existing showSettings and setShowSettings behavior intact; ensure the label is localized if your app uses i18n.src/llm/model.rs-1002-1009 (1)
1002-1009:⚠️ Potential issue | 🟠 MajorUse the same SSE framing rules in the final parse path.
The live loop extracts payloads with
extract_sse_data_payload(), so it accepts bothdata:anddata:lines and handles multilinedataframes. The finalparse_openai_responses_sse_response()pass doesn’t—it line-scans fordata:only. A valid stream can therefore yield deltas successfully and then fail during finalization.Also applies to: 1085-1095
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 1002 - 1009, The finalization path uses a stricter line-scan for "data: " which can reject valid SSE frames; update parse_openai_responses_sse_response (and the other final-parse location that mirrors this logic) to reuse the same framing logic as extract_sse_data_payload: iterate over SSE blocks, call extract_sse_data_payload to accept both "data:" and "data: " and multiline data frames, concatenate multiple data payloads with newlines exactly like the live loop does, ignore non-data lines, and then feed the combined trimmed payload into the existing JSON/response parsing so the final pass accepts the same streams the live loop accepted.src/llm/model.rs-983-1105 (1)
983-1105:⚠️ Potential issue | 🟠 MajorResponses streaming can finish with an empty choice even when the final body is complete.
Unlike the chat-completions path, this branch never flushes
pending_tool_calls, and bothmessagedone handlers emit onlyMessageId. Sincecollect_streaming_completion_response()rebuildschoicefrom streamed events—not fromFinalResponse.body—any provider/proxy that batches content or tool calls intooutput_item.done/response.completedwill drop that assistant turn entirely.Also applies to: 2459-2464, 2633-2636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 983 - 1105, Streaming can end with an empty choice because pending_tool_calls are never flushed and message-done handlers only emit MessageId; update the EOF handling (after the block_buffer flush and before yielding RawStreamingChoice::FinalResponse in collect_streaming_completion_response) to flush pending_tool_calls into the same event stream (call the same logic used elsewhere: process_openai_responses_stream_raw_event/process_openai_responses_stream_event) so any pending tool outputs produce full Message/Content events, and ensure the message-done path emits full Message events (not just MessageId) so rebuilding choice from streamed events includes the final assistant content; apply the same fix in the other affected branches noted (the other EOF handling around the referenced regions).interface/src/api/client.ts-2308-2311 (1)
2308-2311:⚠️ Potential issue | 🟠 MajorDon't rename the audio path to a permanent 501.
portalSendAudio()is now the public portal audio helper, but it ignoressessionIdand always returns a synthetic 501. Once callers switch to this method, voice uploads fail every time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 2308 - 2311, The portalSendAudio implementation must not always return 501 and must actually use the sessionId and blob to upload audio; replace the stub in portalSendAudio(agentId, _sessionId, _blob) so it forwards the provided sessionId and blob to the real upload path (or existing internal helper) instead of logging and returning a synthetic 501 — locate the portalSendAudio function and modify it to call the real upload routine (or perform a fetch to the portal audio endpoint) passing agentId, sessionId and blob and return that Response so callers receive the actual upload result.interface/src/components/ConversationsSidebar.tsx-111-149 (1)
111-149:⚠️ Potential issue | 🟠 MajorUse semantic controls for row selection and actions.
The selectable rows are mouse-only
<div>s, and the icon-only action buttons depend ontitleinstead of an accessible name. Keyboard and screen-reader users won't be able to reliably discover or operate these controls.Also applies to: 169-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/components/ConversationsSidebar.tsx` around lines 111 - 149, The conversation rows are non-semantic divs with click handlers and the icon-only actions rely solely on title attributes, breaking keyboard and screen-reader accessibility; change the row container (where activeConversationId is compared and onSelectConversation(conv.id) is called) to a semantic <button type="button"> (or an element with role="button" plus keyboard handlers) so it can be focused and activated with Enter/Space, ensure it exposes state (e.g., aria-pressed or aria-current when activeConversationId === conv.id), and add descriptive aria-label attributes to the icon action handlers (the buttons that call handleRename(conv), onArchiveConversation(conv.id, true), and handleDelete(conv)) so screen readers can announce their purpose; also keep the existing e.stopPropagation() behavior to prevent row activation when action buttons are used.src/agent/worker.rs-172-174 (1)
172-174: 🛠️ Refactor suggestion | 🟠 MajorKeep fresh workers task-scoped.
Passing parent conversation history into
new()/new_interactive()collapses the worker/branch boundary and makes worker behavior depend on channel transcript size. If a task needs conversation context, route it through a branch or summarize it into the task instead of cloning raw history into the worker.As per coding guidelines, "Don't give workers channel context. Workers get a fresh prompt and a task. If a worker needs conversation context, that is a branch, not a worker."
Also applies to: 207-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/worker.rs` around lines 172 - 174, The constructor/factory signatures new() and new_interactive() should not accept or clone parent channel history (the initial_history: Vec<rig::message::Message> parameter) because workers must be task-scoped and start with a fresh prompt; remove the initial_history parameter from Worker creation (and any handling of it alongside WorkerMemoryMode and model_override), update all callers to stop passing channel transcripts and instead pass required context via a branch or a summarized task payload, and ensure any remaining uses of initial_history (also referenced near the other constructor overloads) are eliminated so worker behavior no longer depends on channel transcript size.docs/design-docs/conversation-settings.md-151-188 (1)
151-188: 🛠️ Refactor suggestion | 🟠 MajorDon't standardize channel-history injection for workers.
WorkerHistoryMode::{Summary, Recent, Full}turns workers into branch-like agents with inherited channel context. That makes workers more expensive and undermines the branch/worker separation the runtime already relies on; if conversation context is required, the design should route through a branch instead of cloning history into worker state.As per coding guidelines, "Don't give workers channel context. Workers get a fresh prompt and a task. If a worker needs conversation context, that is a branch, not a worker."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/conversation-settings.md` around lines 151 - 188, The docs introduce worker channel-history injection via WorkerHistoryMode::{Summary, Recent, Full}, which violates our guideline that workers receive only a fresh prompt+task; remove or disable those history modes and document that WorkerContextMode.WorkerHistoryMode must be None, and ensure runtime code that constructs worker contexts (e.g., any spawn_worker/create_worker_context logic) does not inject channel history into workers; keep WorkerMemoryMode clarifications if desired but explicitly state workers get no channel history and route any conversation-context needs to a branch.src/agent/worker.rs-259-261 (1)
259-261:⚠️ Potential issue | 🟠 MajorResume with the original model and memory settings.
The worker now builds its tool server and model from
worker_memory_modeandmodel_override, butresume_interactive()resets both to defaults. A worker resumed after restart can silently lose memory tools and switch models mid-session even though its transcript is restored.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/worker.rs` around lines 259 - 261, resume_interactive() is resetting WorkerMemoryMode and model_override to defaults causing resumed workers to lose memory tools and switch models; update the resume path to restore the original settings by reading and reusing the saved worker_memory_mode and model_override (e.g., use the prior_history's associated worker_memory_mode and model_override or the stored fields) when reconstructing the worker state instead of passing WorkerMemoryMode::None and None; ensure the code paths that build the tool server and model (references: resume_interactive(), worker_memory_mode, model_override, initial_history) use these restored values so tools and model overrides are preserved after restart.src/tools.rs-453-499 (1)
453-499:⚠️ Potential issue | 🟠 MajorFinish direct-mode registration and cleanup.
add_direct_mode_tools()currently only addsmemory_recall/memory_saveand then returns, so Direct mode still lacksmemory_deleteand the execution tools it claims to expose. Because those extra tools are added on the shared per-turn handle without symmetric removal inremove_channel_tools(), switching a conversation back to Standard on the next turn will still leave memory tools available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.rs` around lines 453 - 499, add_direct_mode_tools currently only registers MemoryRecallTool and MemorySaveTool but must also register MemoryDeleteTool and the execution/shell/file tools it promises (e.g., add_tool(MemoryDeleteTool::new(state.deps.memory_search.clone())) and the Shell/File/Execution tools using the same per-turn handle and appropriate deps), and ensure symmetry by unregistering those same tool instances in remove_channel_tools() (or switch to a per-turn disposable registration API) so returning to Standard mode doesn’t leave direct-mode tools available; update add_direct_mode_tools to add the missing tools (referencing MemoryDeleteTool, the Shell/File/Execution tool types you intend to use) and update remove_channel_tools to remove them by the same identifiers or implement matching cleanup logic.src/hooks/spacebot.rs-1117-1127 (1)
1117-1127:⚠️ Potential issue | 🟠 MajorNon-
replytool deltas leak entries inreply_tool_delta_state.The map entry is created before the
replycheck, but the cleanup paths only removereplycalls. Any streamed delta for another tool leaves a permanentinternal_call_identry on this long-lived hook.Also applies to: 1172-1176, 1252-1256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/spacebot.rs` around lines 1117 - 1127, The code currently creates a map entry with guard.entry(internal_call_id.to_string()).or_insert_with(ReplyToolDeltaState::default) before checking whether tool_name is "reply", which causes non-reply tool deltas to leave permanent entries; change the logic to avoid inserting unless the tool is actually "reply": check tool_name first (e.g., if let Some(tool_name) = tool_name { if tool_name != "reply" { return HookAction::Continue } } else { return HookAction::Continue }), and only then insert or get a mutable reference to the map entry (use guard.get_mut or entry(...) after the reply check) so entries are only created for "reply" and are cleaned up on reply completion; apply the same pattern wherever reply-tool handling uses ReplyToolDeltaState (the other similar spots around the code).src/api/portal.rs-209-215 (1)
209-215:⚠️ Potential issue | 🟠 MajorClamp
limitbefore calling the stores.
portal_history()only doesmin(200), solimit <= 0still flows through, andlist_portal_conversations()does not enforce the documented max of 500 at all. Depending on store/SQLite semantics, negative or oversized values can turn into unbounded scans and very large responses.Also applies to: 249-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/portal.rs` around lines 209 - 215, portal_history currently only applies min(200) allowing zero/negative or >500 limits to reach the stores; clamp the incoming query.limit to a safe range (e.g., 1..=500) before passing it to any store calls. Specifically, normalize/validate query.limit up front (or compute a local let limit = query.limit.clamp(1, 500) / ensure positive and capped) and then use that limit when calling logger.load_recent(&channel_id, limit) and when calling list_portal_conversations(&state.store, &user.id, limit) so stores never receive 0, negative, or oversized values.src/agent/channel.rs-706-727 (1)
706-727:⚠️ Potential issue | 🟠 MajorA settings read error here can wipe the rest of the channel config.
When
store.get(..)fails, this falls back toConversationSettings::default()and still writes the row back with the newresponse_mode. A transient read failure would silently reset delegation, memory, worker context, and model overrides to defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 706 - 727, The code currently treats a read error from ChannelSettingsStore::get as if there were no existing settings and proceeds to upsert ConversationSettings::default with the new response_mode, risking wiping other fields; change the error branch in the tokio::spawn block so that when store.get(&agent_id, &channel_id) returns Err(error) you log the warning (as you already do) and then return/exit the async task without calling store.upsert, ensuring you only upsert when get returns Ok(Some) or Ok(None) and avoid overwriting existing config on transient read failures; update references around ChannelSettingsStore::new, store.get, ConversationSettings::default, settings.response_mode, and store.upsert accordingly.src/hooks/spacebot.rs-460-466 (1)
460-466:⚠️ Potential issue | 🟠 MajorStreaming state is being reused across iterations.
did_call_toolandlast_text_responselive outside the loop, so a tool-using iteration can affect the next one. A following empty or reasoning-only completion can then spin until max turns or return stale text from the previous iteration instead of the current result.Also applies to: 515-537, 652-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/spacebot.rs` around lines 460 - 466, The variables did_call_tool and last_text_response (and any per-iteration streaming state like current_max_turns, prompt_message, and chat_history clones) are declared outside the loop and thus get reused across iterations; move their initialization into the loop body so each iteration starts fresh (e.g., create prompt_message via Message::from(prompt), clone history into chat_history, and set did_call_tool = false, last_text_response = String::new(), and current_max_turns = 0 inside the loop that processes completions) and do the same fix for the other affected blocks around the regions noted (lines ~515-537 and ~652-695) to avoid leaking state between iterations.src/agent/channel.rs-522-529 (1)
522-529:⚠️ Potential issue | 🟠 MajorHot-reloading never reaches the compactor.
Compactor::new(...)snapshots the compactor override once, butreload_settings()only updatesself.resolved_settingsand shared channel state. After a settings update, compaction jobs still run on the old model until the channel is recreated, even though the channel logs that settings were hot-reloaded.Also applies to: 641-695
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 522 - 529, Compactor::new currently snapshots the model override from resolved_settings, so compaction keeps using the old model after hot-reload; change this so Compactor reads the override dynamically or is updated during reload_settings: either (a) make Compactor hold an Arc/Clone of the shared ResolvedSettings (or a small wrapper that exposes resolve_model) instead of a one-time String, or (b) add a Compactor::update_model_override(&self, Option<String>) and call it from the channel's reload_settings() after updating self.resolved_settings; update usages of Compactor::new and any compactor field to enable dynamic model changes without recreating the channel.src/conversation/settings.rs-178-196 (1)
178-196:⚠️ Potential issue | 🟠 Major
ConversationSettingsbreaks precedence for sparse (partial) payloads.Non-Option fields (
memory,delegation,response_mode,worker_context) use#[serde(default)], so a sparse frontend request like{ response_mode: Quiet }deserializes to a full struct with system defaults for missing fields.ResolvedConversationSettings::resolve()then unconditionally assigns these fields, overwriting inherited channel/agent values instead of only applying the provided overrides.For example, a partial update to change only
response_modewould resetmemory,delegation, andworker_contextto system defaults, breaking the documented precedence order (conversation > channel > agent > system).The
modelandsave_attachmentsfields avoid this because they areOption<T>and have conditional checks in resolve. Non-Option fields need the same treatment: either make themOption<T>or change resolve to skip unconditional assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/settings.rs` around lines 178 - 196, ConversationSettings currently uses concrete types for memory, delegation, response_mode, and worker_context so sparse payloads overwrite inherited values; change these fields to Option<MemoryMode>, Option<DelegationMode>, Option<ResponseMode>, and Option<WorkerContextMode> (matching model and save_attachments) and update their serde attrs (keep #[serde(default, skip_serializing_if = "Option::is_none")]) so missing keys deserialize to None; then update ResolvedConversationSettings::resolve() to only assign/override when the corresponding ConversationSettings field is Some(...) (leave existing fallback logic to channel/agent/system values).interface/src/api/types.ts-139-142 (1)
139-142:⚠️ Potential issue | 🟠 Major
worker_context.historyis typed too narrowly for the backend payload.The backend defines
WorkerHistoryMode::Recent(u32), a payloaded enum variant that carries a count. When serialized, this becomes something like{"recent": 20}, not the bare string"recent"that the frontend type currently allows. BothConversationSettingsandConversationDefaultsResponsewill fail to round-trip this value.The frontend type should be a discriminated union or object variant rather than a simple string literal to match the backend contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/types.ts` around lines 139 - 142, The worker_context.history field is currently a simple string union but the backend uses a payloaded enum (WorkerHistoryMode::Recent(u32)) which serializes like {"recent": 20}; update the frontend types (e.g., the worker_context.history in ConversationSettings and ConversationDefaultsResponse) to a discriminated/object variant that models payloaded variants (e.g., an object form for recent that includes the count) so the serialized shape matches the backend contract.src/conversation/portal.rs-58-68 (1)
58-68:⚠️ Potential issue | 🟠 MajorPropagate settings serialization failures instead of storing an empty string.
These
unwrap_or_default()calls turn a serde error into"". That value is later read back asNone, so a brokenConversationSettingspayload gets silently dropped instead of failing the request.As per coding guidelines "Don't silently discard errors. No `let _ =` on Results. Handle them, log them, or propagate them."🛠️ Proposed fix
+ let settings_json = settings + .as_ref() + .map(serde_json::to_string) + .transpose() + .map_err(|error| anyhow::anyhow!(error))?; + sqlx::query( "INSERT INTO portal_conversations (id, agent_id, title, title_source, settings) VALUES (?, ?, ?, ?, ?)", ) .bind(&id) .bind(agent_id) .bind(&title) .bind(title_source) - .bind(settings.as_ref().map(|s| serde_json::to_string(s).unwrap_or_default())) + .bind(settings_json.as_deref()) .execute(&self.pool) .await .map_err(|error| anyhow::anyhow!(error))?;- let settings_json = settings - .as_ref() - .map(|s| serde_json::to_string(s).unwrap_or_default()); + let settings_json = settings + .as_ref() + .map(serde_json::to_string) + .transpose() + .map_err(|error| anyhow::anyhow!(error))?;Also applies to: 160-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/portal.rs` around lines 58 - 68, The insert uses settings.as_ref().map(|s| serde_json::to_string(s).unwrap_or_default()) which silences serialization errors; instead, serialize settings before building the SQL and propagate any serde_json::to_string error (e.g., let settings_json: Option<String> = settings.as_ref().map(|s| serde_json::to_string(s)).transpose().map_err(|e| anyhow::anyhow!(e))? ), then bind settings_json to the query (replace the current unwrap_or_default binding) and do the same fix for the other occurrence around the code that previously used unwrap_or_default (lines noted in the review); this ensures serde errors are returned rather than stored as an empty string.packages/api-client/src/client.ts-187-195 (1)
187-195:⚠️ Potential issue | 🟠 MajorInclude
offsetinlistMemories()requests.The generated contract for
GET /agents/memoriesmarksoffsetas required. Right now every call omits it, so consumers get an invalid request unless the handler happens to inject a server-side default.🛠️ Proposed fix
- listMemories(agentId: string, limit = 12, sort = "recent") { + listMemories(agentId: string, limit = 12, offset = 0, sort = "recent") { const params = new URLSearchParams({ agent_id: agentId, limit: String(limit), + offset: String(offset), sort, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/src/client.ts` around lines 187 - 195, The listMemories function omits the required offset parameter causing invalid requests; update listMemories(agentId: string, limit = 12, sort = "recent", offset = 0) to accept an offset (default 0) and include offset in the URLSearchParams passed to request (i.e., add offset: String(offset)) so the generated contract for GET /agents/memories is satisfied; update any callers/tests accordingly if they relied on the old signature.packages/api-client/src/client.ts-91-101 (1)
91-101:⚠️ Potential issue | 🟠 MajorClamp
channelMessages()to the documented max.The current default of
200exceeds the generated contract forGET /channels/messages(max: 100). That makes the zero-argument path invalid as soon as the server validates instead of silently clamping.🛠️ Proposed fix
- channelMessages(channelId: string, limit = 200, before?: string) { + channelMessages(channelId: string, limit = 100, before?: string) { const params = new URLSearchParams({ channel_id: channelId, - limit: String(limit), + limit: String(Math.min(limit, 100)), });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/src/client.ts` around lines 91 - 101, channelMessages() uses a default limit of 200 which violates the API contract (max 100); change the implementation in channelMessages(channelId: string, limit = 200, before?: string) to clamp the provided limit to the documented maximum (e.g., const clamped = Math.min(limit, 100) and use clamped when building URLSearchParams) and optionally set the default parameter to 100 so the zero-argument call is valid; update any references inside channelMessages (params creation and param names) to use the clamped value.
| prior_history: if initial_history.is_empty() { | ||
| None | ||
| } else { | ||
| Some(initial_history) | ||
| }, |
There was a problem hiding this comment.
Don't treat seeded history as a resumed worker.
run() uses self.prior_history.is_some() as the resume sentinel. After this change, any fresh worker with non-empty initial_history lands here as Some(...), so it skips the initial task, goes down the resumed/idle path, and never executes the spawned work.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agent/worker.rs` around lines 145 - 149, The bug is that prior_history is
being set from initial_history, making run() treat seeded workers as resumed;
change the design so prior_history remains the resume sentinel only and do not
populate it from initial_history. Instead add a separate field (e.g.,
seeded_history or initial_seed) on the same struct to hold initial_history, set
prior_history to None for fresh workers, and update run() to check prior_history
for resume logic and use the new seeded_history/initial_seed only to enqueue or
execute the initial task path. Ensure references to prior_history and run() are
updated and any constructors/creators set the new field rather than
prior_history.
| pub async fn delete(&self, agent_id: &str, session_id: &str) -> crate::error::Result<bool> { | ||
| let mut tx = self | ||
| .pool | ||
| .begin() | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| sqlx::query("DELETE FROM conversation_messages WHERE channel_id = ?") | ||
| .bind(session_id) | ||
| .execute(&mut *tx) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| let result = sqlx::query("DELETE FROM portal_conversations WHERE agent_id = ? AND id = ?") | ||
| .bind(agent_id) | ||
| .bind(session_id) | ||
| .execute(&mut *tx) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| tx.commit().await.map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| Ok(result.rows_affected() > 0) |
There was a problem hiding this comment.
Don’t delete message history before proving session ownership.
Lines 197-201 delete conversation_messages by channel_id before Lines 203-208 check agent_id. If a caller passes another agent’s session_id, this method wipes that history and then returns false because the portal row itself was untouched.
🛠️ Proposed fix
let mut tx = self
.pool
.begin()
.await
.map_err(|error| anyhow::anyhow!(error))?;
- sqlx::query("DELETE FROM conversation_messages WHERE channel_id = ?")
- .bind(session_id)
- .execute(&mut *tx)
- .await
- .map_err(|error| anyhow::anyhow!(error))?;
+ let owned = sqlx::query_scalar::<_, i64>(
+ "SELECT 1 FROM portal_conversations WHERE agent_id = ? AND id = ?",
+ )
+ .bind(agent_id)
+ .bind(session_id)
+ .fetch_optional(&mut *tx)
+ .await
+ .map_err(|error| anyhow::anyhow!(error))?
+ .is_some();
+
+ if !owned {
+ tx.rollback().await.map_err(|error| anyhow::anyhow!(error))?;
+ return Ok(false);
+ }
+
+ sqlx::query("DELETE FROM conversation_messages WHERE channel_id = ?")
+ .bind(session_id)
+ .execute(&mut *tx)
+ .await
+ .map_err(|error| anyhow::anyhow!(error))?;📝 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.
| pub async fn delete(&self, agent_id: &str, session_id: &str) -> crate::error::Result<bool> { | |
| let mut tx = self | |
| .pool | |
| .begin() | |
| .await | |
| .map_err(|error| anyhow::anyhow!(error))?; | |
| sqlx::query("DELETE FROM conversation_messages WHERE channel_id = ?") | |
| .bind(session_id) | |
| .execute(&mut *tx) | |
| .await | |
| .map_err(|error| anyhow::anyhow!(error))?; | |
| let result = sqlx::query("DELETE FROM portal_conversations WHERE agent_id = ? AND id = ?") | |
| .bind(agent_id) | |
| .bind(session_id) | |
| .execute(&mut *tx) | |
| .await | |
| .map_err(|error| anyhow::anyhow!(error))?; | |
| tx.commit().await.map_err(|error| anyhow::anyhow!(error))?; | |
| Ok(result.rows_affected() > 0) | |
| pub async fn delete(&self, agent_id: &str, session_id: &str) -> crate::error::Result<bool> { | |
| let mut tx = self | |
| .pool | |
| .begin() | |
| .await | |
| .map_err(|error| anyhow::anyhow!(error))?; | |
| let owned = sqlx::query_scalar::<_, i64>( | |
| "SELECT 1 FROM portal_conversations WHERE agent_id = ? AND id = ?", | |
| ) | |
| .bind(agent_id) | |
| .bind(session_id) | |
| .fetch_optional(&mut *tx) | |
| .await | |
| .map_err(|error| anyhow::anyhow!(error))? | |
| .is_some(); | |
| if !owned { | |
| tx.rollback().await.map_err(|error| anyhow::anyhow!(error))?; | |
| return Ok(false); | |
| } | |
| sqlx::query("DELETE FROM conversation_messages WHERE channel_id = ?") | |
| .bind(session_id) | |
| .execute(&mut *tx) | |
| .await | |
| .map_err(|error| anyhow::anyhow!(error))?; | |
| let result = sqlx::query("DELETE FROM portal_conversations WHERE agent_id = ? AND id = ?") | |
| .bind(agent_id) | |
| .bind(session_id) | |
| .execute(&mut *tx) | |
| .await | |
| .map_err(|error| anyhow::anyhow!(error))?; | |
| tx.commit().await.map_err(|error| anyhow::anyhow!(error))?; | |
| Ok(result.rows_affected() > 0) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/conversation/portal.rs` around lines 190 - 212, In delete (pub async fn
delete) you currently delete conversation_messages by channel_id before
verifying the portal_conversations row belongs to the provided agent_id; change
the flow to first verify ownership (e.g., select or attempt DELETE on
portal_conversations WHERE agent_id = ? AND id = ? and check rows_affected or
SELECT agent_id FROM portal_conversations WHERE id = ?) within the same
transaction, and only if that check succeeds proceed to delete
conversation_messages for that session_id and then commit; ensure the
transaction is rolled back or not committed when ownership is not confirmed so
no messages are removed erroneously.
| async fn backfill_from_messages(&self, agent_id: &str) -> crate::error::Result<()> { | ||
| let rows = sqlx::query( | ||
| "SELECT DISTINCT channel_id FROM conversation_messages WHERE channel_id LIKE 'portal:chat:%'", | ||
| ) | ||
| .fetch_all(&self.pool) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; | ||
|
|
||
| for row in rows { | ||
| let channel_id: String = row.try_get("channel_id").unwrap_or_default(); | ||
| if channel_id.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| let existing = self.get(agent_id, &channel_id).await?; | ||
| if existing.is_some() { | ||
| continue; | ||
| } | ||
|
|
||
| let title = sqlx::query( | ||
| "SELECT content FROM conversation_messages \ | ||
| WHERE channel_id = ? AND role = 'user' \ | ||
| ORDER BY created_at ASC LIMIT 1", | ||
| ) | ||
| .bind(&channel_id) | ||
| .fetch_optional(&self.pool) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))? | ||
| .and_then(|title_row| title_row.try_get::<String, _>("content").ok()) | ||
| .map(|content| generate_title(&content)) | ||
| .unwrap_or_else(default_title); | ||
|
|
||
| let title_source = if title == default_title() { | ||
| "system" | ||
| } else { | ||
| "user" | ||
| }; | ||
|
|
||
| sqlx::query( | ||
| "INSERT INTO portal_conversations (id, agent_id, title, title_source, settings) VALUES (?, ?, ?, ?, NULL) \ | ||
| ON CONFLICT(id) DO NOTHING", | ||
| ) | ||
| .bind(&channel_id) | ||
| .bind(agent_id) | ||
| .bind(&title) | ||
| .bind(title_source) | ||
| .execute(&self.pool) | ||
| .await | ||
| .map_err(|error| anyhow::anyhow!(error))?; |
There was a problem hiding this comment.
Scope the backfill query to the current agent.
Line 241 currently scans every portal:chat:* channel in the instance, and Lines 277-284 then insert any missing row with the current agent_id. Listing agent A will therefore backfill agent B’s sessions into A, which corrupts metadata and leaks cross-agent conversation titles/previews.
🛠️ Proposed fix
- let rows = sqlx::query(
- "SELECT DISTINCT channel_id FROM conversation_messages WHERE channel_id LIKE 'portal:chat:%'",
- )
+ let rows = sqlx::query(
+ "SELECT DISTINCT channel_id FROM conversation_messages WHERE channel_id LIKE ?",
+ )
+ .bind(format!("portal:chat:{agent_id}:%"))
.fetch_all(&self.pool)
.await
.map_err(|error| anyhow::anyhow!(error))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/conversation/portal.rs` around lines 239 - 287, The backfill currently
scans all "portal:chat:%" channels and then inserts them using the current
agent_id, which can backfill other agents' conversations; in
backfill_from_messages modify the initial SELECT to only return channels
belonging to the current agent by joining against portal_conversations and
adding a WHERE p.agent_id = ? (or otherwise filtering by agent_id) so only
channels already associated with this agent are considered, bind agent_id to
that query, and keep the subsequent get/INSERT logic (references:
backfill_from_messages, the "SELECT DISTINCT channel_id FROM
conversation_messages ..." query and the "INSERT INTO portal_conversations ..."
query) so no cross-agent channel IDs are inserted for the wrong agent.
| async fn on_tool_call_delta( | ||
| &self, | ||
| _tool_call_id: &str, | ||
| internal_call_id: &str, | ||
| tool_name: Option<&str>, | ||
| tool_call_delta: &str, | ||
| ) -> HookAction { | ||
| if self.process_type != ProcessType::Channel { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| let Some(channel_id) = self.channel_id.clone() else { | ||
| return HookAction::Continue; | ||
| }; | ||
|
|
||
| let mut guard = match self.reply_tool_delta_state.lock() { | ||
| Ok(guard) => guard, | ||
| Err(_) => return HookAction::Continue, | ||
| }; | ||
|
|
||
| let state = guard | ||
| .entry(internal_call_id.to_string()) | ||
| .or_insert_with(ReplyToolDeltaState::default); | ||
|
|
||
| if let Some(tool_name) = tool_name { | ||
| state.tool_name = Some(tool_name.to_string()); | ||
| } | ||
|
|
||
| if state.tool_name.as_deref() != Some("reply") { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| state.raw_args.push_str(tool_call_delta); | ||
| let Some(content) = Self::extract_partial_reply_content(&state.raw_args) else { | ||
| return HookAction::Continue; | ||
| }; | ||
|
|
||
| if !content.starts_with(&state.emitted_content) { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| let delta = &content[state.emitted_content.len()..]; | ||
| if delta.is_empty() { | ||
| return HookAction::Continue; | ||
| } | ||
|
|
||
| state.emitted_content = content.clone(); | ||
| self.event_tx | ||
| .send(ProcessEvent::TextDelta { | ||
| agent_id: self.agent_id.clone(), | ||
| process_id: self.process_id.clone(), | ||
| channel_id: Some(channel_id), | ||
| text_delta: delta.to_string(), | ||
| aggregated_text: content, | ||
| }) | ||
| .ok(); | ||
|
|
||
| HookAction::Continue |
There was a problem hiding this comment.
Reply deltas can exfiltrate secrets before the reply guard runs.
on_tool_call_delta() pushes partial reply.content to TextDelta immediately, but the secret scan only happens later in on_tool_call() / on_tool_result(). If the model starts streaming a token or PEM into the reply body, SSE clients see it before the final reply is blocked. Buffer or scrub these deltas with the same leak guard before emitting them.
Based on learnings: Scan tool output and user input via SpacebotHook.on_tool_result() for leak detection (API keys, tokens, PEM keys). Block exfiltration before outbound HTTP.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/spacebot.rs` around lines 1097 - 1154, on_tool_call_delta currently
emits partial reply content immediately (via self.event_tx ->
ProcessEvent::TextDelta) which can leak secrets before the leak-guard in
on_tool_result runs; modify on_tool_call_delta / ReplyToolDeltaState so deltas
are buffered and/or scrubbed using the same leak-detection logic used in
on_tool_result before sending: collect partial content with
ReplyToolDeltaState.raw_args/emitted_content, run the existing leak scan
function used in on_tool_result (or call the same helper) against the newly
computed delta/content, and only send ProcessEvent::TextDelta when the scan
passes (or send a redacted delta if the scan flags secrets); ensure
extract_partial_reply_content, reply_tool_delta_state locking, and event_tx send
logic are updated to use the guarded/buffered content path to prevent emitting
unscanned tokens.
1 participant
Summary
This PR migrates the interface UI primitives from local implementations to the official @spaceui/primitives and @spaceui/tokens packages.
Migrated Primitives (16 components)
The following components have been migrated to use @spaceui/primitives:
Token Migration
Build Verification
Breaking Changes
None - this is a pure refactor. All component APIs remain compatible.
Cleanup
Dependencies
Related
Based on work in #506