feat: portal system, streaming, conversation & channel settings#506
feat: portal system, streaming, conversation & channel settings#506
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces per-conversation ConversationSettings and ResponseMode, portal-based conversations with SQLite persistence and CRUD APIs, a new Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jamiepine
changed the title
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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 (2)
src/agent/channel.rs (1)
839-847:⚠️ Potential issue | 🟡 Minor
/statusshould report resolved models.This still prints
routing.resolve(...), so once a conversation override is active the command shows the fallback, not the model actually used by the channel/branch. Resolve throughself.resolved_settingsfirst and only fall back to routing when no override exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 839 - 847, The /status output should prefer models from self.resolved_settings instead of always calling routing.resolve; change the channel/branch model resolution to first check for an override on self.resolved_settings (e.g. use any channel/branch model fields or a resolved model value stored on self.resolved_settings) and only call routing.resolve(ProcessType::Channel, None) or routing.resolve(ProcessType::Branch, None) as a fallback when no override exists, leaving the existing response_mode handling unchanged.interface/src/api/schema.d.ts (1)
1940-2007:⚠️ Potential issue | 🟠 MajorThe generated schema still omits the new settings API.
These additions only expose conversation metadata (
title,archived, IDs). I still don’t see any settings-bearing portal/webchat payload here, and this spec also lacks the new channel-settings routes, so the generated interface client cannot persist or rehydrate the per-conversation/per-channel settings this PR introduces. The fix needs to happen in the OpenAPI source, then this file should be regenerated.Also applies to: 2484-2487, 3536-3624, 8446-8562
🤖 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 generated TypeScript client is missing the new settings API and channel-settings routes; update the OpenAPI source to add the per-conversation/per-channel settings schemas and endpoints (the new settings-bearing request/response objects and routes similar to the existing "/webchat/send", "/webchat/history", "/webchat/conversations" and "/webchat/conversations/{session_id}" entries) so operations for persisting/rehydrating settings are declared (e.g., add operations akin to "create_webchat_conversation", "webchat_send" but for settings and channel-settings endpoints), then regenerate the API schema so the declarations (and operation names/types) appear in the generated interface file.
🟠 Major comments (27)
migrations/20260325130000_add_portal_conversation_settings.sql-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorIndex key order undermines the intended
settingsfilter optimization.At Line [7], indexing
(id, settings)won’t help queries that filter bysettingsunlessidis also constrained first;idis already covered by the PK anyway.Suggested migration fix
-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;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/20260325130000_add_portal_conversation_settings.sql` at line 7, The composite index currently uses (id, settings) which prevents queries filtering by settings from using the index because id (the PK) is leading; change the index so settings is the leading key — update the CREATE INDEX to index on settings alone (or use an appropriate GIN/JSONB operator class if settings is jsonb) and keep the WHERE settings IS NOT NULL predicate; reference the index name idx_portal_conversations_settings and the migration script so replace the column list (id, settings) with just settings (or USING gin (settings) if jsonb).src/agent/cortex.rs-3333-3334 (1)
3333-3334:⚠️ Potential issue | 🟠 MajorQueued task workers drop the originating worker settings.
Channel-spawned workers now carry worker-context settings, but this pickup path still hard-codes
WorkerMemoryMode::Noneand no model override. Delegated tasks can reach this flow, so once they leave the channel path they no longer honor the conversation's worker memory mode or per-process model override. Persist a worker-settings snapshot on the task and restore it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex.rs` around lines 3333 - 3334, The cortex worker construction currently hard-codes crate::conversation::settings::WorkerMemoryMode::None and a None model override, dropping originating worker settings for delegated tasks; update the task launch/path so a WorkerSettings snapshot (including memory_mode and model_override) is serialized onto the Task struct when created, and in the cortex worker initialization (the place where WorkerMemoryMode::None and None are set) detect and deserialize that snapshot from the incoming Task and restore its fields instead of using the hard-coded defaults; ensure you reference and map snapshot.memory_mode to the WorkerMemoryMode used here and snapshot.model_override to the model override parameter so delegated tasks honor the original worker context.packages/api-client/src/types.ts-1-1 (1)
1-1:⚠️ Potential issue | 🟠 MajorKeep the public types entrypoint inside the package boundary.
This
export *reaches intointerface/srcvia relative path, and TypeScript will emit this path as-is in the compiled output. Since there's no formalinterfacedependency inpackages/api-client/package.json, this breaks isolated builds and fails for any consumer outside the workspace (even if the package is published). Move or generate the contract types inside this package, or addinterfaceas an explicit dependency before exposing this as the public types entrypoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/src/types.ts` at line 1, The public types entrypoint currently re-exports via a relative path ("export * from '../../../interface/src/api/types'") which leaks a workspace-internal path into compiled output; fix by either relocating or generating the API contract types into this package and updating the export to point to a local module (e.g., create a local src/types.ts that re-exports the copied/generated declarations) or, if you intend to keep a shared module, add the interface package as an explicit dependency in package.json and change the export to import from the package name (e.g., "export * from 'interface/src/api/types'") so TypeScript emits a proper package import instead of a relative workspace path.interface/src/components/ChannelCard.tsx-97-113 (1)
97-113:⚠️ Potential issue | 🟠 MajorWait for the persisted channel settings snapshot before rendering an editable form.
The popover becomes editable as soon as
defaultsresolves, butsettingsstill starts as{}and is only hydrated later fromchannelSettingsData. On a cold open or refetch, a quick save can overwrite an existing per-channel override set with an empty/default snapshot.Also applies to: 188-199
🤖 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 - 113, The form becomes editable as soon as defaults resolves while settings state is still {} and later hydrated from channelSettingsData, risking overwriting persisted per-channel overrides; update the logic so the editable form and save actions wait for the persisted snapshot by (1) initializing/setting settings only after channelSettingsData?.settings is available (use the existing useEffect with channelSettingsData and setSettings to populate state), (2) prevent rendering or enabling the editable inputs/buttons until channelSettingsData is loaded (check channelSettingsData !== undefined and channelSettingsData.settings truthy alongside showSettings), and (3) ensure save handlers merge/update based on the populated settings rather than an empty object—apply the same guard where defaults and channelSettingsData are used later (the block around lines 188-199).interface/src/components/ConversationsSidebar.tsx-111-155 (1)
111-155:⚠️ Potential issue | 🟠 MajorUse keyboard-accessible controls for the conversation rows.
Both the active and archived rows are clickable
divs, so selecting a conversation is mouse-only. The inline action buttons are also hover-only, which leaves keyboard users tabbing into controls that are not visible.Also applies to: 169-192
🤖 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 - 155, The conversation rows are only clickable by mouse and their inline action buttons only appear on hover, which breaks keyboard accessibility; update the row container in ConversationsSidebar.tsx (the element using onSelectConversation(conv.id)) to be keyboard-focusable and activatable (either change the div to a semantic <button> or add tabIndex={0} and an onKeyDown handler that triggers onSelectConversation for Enter/Space), and update the hover-only visibility rules for the action buttons so they also appear when the row is focused (use group-focus-within or combined classes so the buttons become visible on keyboard focus as well); keep existing stopPropagation calls for handleRename, onArchiveConversation, and handleDelete so their behavior is preserved.src/config/load.rs-2335-2355 (1)
2335-2355:⚠️ Potential issue | 🟠 MajorKeep
[bindings.settings]partial when fields are omitted.Once a binding has a
settingstable, missingmemory,delegation, orresponse_modevalues are forced toFull/Standard/Active. A binding that only wants to overridemodelorsave_attachmentswill now reset the other knobs instead of inheriting the agent/channel defaults, and typos hit the same fallback paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` around lines 2335 - 2355, The current mapping forces defaults when a binding provides a settings table but omits or mistypes fields; change the conversions for s.memory, s.delegation, and s.response_mode to produce Option values instead of defaulting to MemoryMode::Full/DelegationMode::Standard/ResponseMode::Active. Concretely, map s.memory.as_deref() to Some(MemoryMode::Ambient)/Some(MemoryMode::Off) and map any other Some(_) to None (keep None for missing), do the same pattern for s.delegation -> Option<DelegationMode> and s.response_mode -> Option<ResponseMode>, and assign those Option<> fields into ConversationSettings so omitted or mistyped keys remain None and inherit existing defaults rather than being overwritten.src/config/load.rs-116-136 (1)
116-136:⚠️ Potential issue | 🟠 MajorPreserve explicit legacy overrides when translating
listen_only_mode.
parse_response_mode()currently treatslisten_only_mode = falsethe same as "unset". That means an old child config can no longer override an inherited quiet default back to active, because callers fall back to the parentresponse_mode. The same helper also silently accepts unknown strings by collapsing them toActive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/load.rs` around lines 116 - 136, parse_response_mode currently treats listen_only_mode = false as unset and maps unknown response_mode strings to Active, which prevents explicit child overrides and hides invalid values; update parse_response_mode so that when response_mode is provided you only return Some(ResponseMode::...) for recognized values and return None for unknown/invalid strings (so callers can inherit), and ensure that listen_only_mode = Some(true) maps to Some(ResponseMode::Quiet) (with the deprecation warn) while listen_only_mode = Some(false) explicitly returns Some(ResponseMode::Active) to allow children to override a parent quiet default; refer to the function parse_response_mode and the ResponseMode enum in crate::conversation::settings when making these changes.interface/src/components/WebChatPanel.tsx-188-195 (1)
188-195:⚠️ Potential issue | 🟠 MajorReset
settingsfrom the active conversation before rendering/saving.
settingsstarts as{}and is only mutated locally, so switching from conversation A to B leaves A's overrides in state. The popover then renders stale values for B, andsaveSettingsMutationwill persist those stale overrides under B'sactiveConversationId. The same problem also shows up ifagentIdchanges, because the state initializer only runs once.Also applies to: 197-204, 284-295, 323-333
🤖 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 - 195, Reset the local settings state whenever the active conversation or agent changes: add a useEffect that watches activeConversationId and agentId, reads the canonical settings for that conversation from liveStates (via useLiveContext / liveStates.conversations or the object holding conversations), and calls setSettings(conversation?.settings ?? {}) to override any stale local mutations; also ensure the same sync is applied where saveSettingsMutation is used so you only persist the currently synced settings for activeConversationId.interface/src/components/ConversationSettingsPanel.tsx-209-329 (1)
209-329:⚠️ Potential issue | 🟠 MajorExpose an "Inherit" option for the top-level settings.
This component already treats
currentSettingsas sparse overrides by falling back todefaults, but the core selects only ever write concrete values. After a user changes model/memory/delegation/response/worker context once, there is no UI path back to the binding/agent defaults. The per-process override controls below already solve this with an explicit inherit sentinel; the top-level fields need the same escape hatch.Also applies to: 420-484
🤖 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 209 - 329, The top-level selects in ConversationSettingsPanel (the Select blocks that read/write currentSettings.model, memory, delegation, response_mode and the worker-context selects referenced at lines ~420-484) need an explicit "Inherit" choice that clears the override instead of writing a concrete value; add a SelectItem (label "Inherit" or similar) to each Select and update the onValueChange handlers (for model, memory, delegation, response_mode and the worker context select) so when the inherit value is chosen you produce a new settings object with that key removed (or set to undefined) rather than assigning the inherit sentinel string — ensure the Select value/display logic still shows inherited state (use currentSettings.key ?? defaults.key to display but treat the inherit option as clearing the override).interface/src/components/WebChatPanel.tsx-316-320 (1)
316-320:⚠️ Potential issue | 🟠 MajorGive the settings trigger an accessible name.
This is an icon-only button, so screen readers currently just get an unlabeled control and the new settings UI is hard to discover non-visually.
Suggested 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 316 - 320, The settings trigger is an icon-only Button inside PopoverTrigger in WebChatPanel and needs an accessible name for screen readers; update the Button (the one with variant="ghost" size="icon" and the HugeiconsIcon/Settings02Icon) to include an aria-label (e.g., aria-label="Settings") or render visually-hidden text inside the Button (preferably using your i18n/localization string) so the control is discoverable; keep the existing icon and PopoverTrigger logic but add the accessible label attribute or hidden label text to the Button element.src/conversation/channel_settings.rs-34-42 (1)
34-42:⚠️ Potential issue | 🟠 MajorDon't hide corrupt
channel_settingsrows as "missing".
try_get(...).ok()andserde_json::from_str(...).ok()collapse read/parse failures intoNone. That makes a bad row look identical to "no override stored", so callers fall back to defaults and a later save can silently overwrite the corrupted payload instead of surfacing the problem.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 masks read/parse errors by using try_get(...).ok() and serde_json::from_str(...).ok(), making corrupted rows indistinguishable from “no override”; instead, change the logic in the closure that calls r.try_get::<String, _>("settings") and serde_json::from_str(&s) to propagate and return errors rather than converting them to None — e.g., handle the Result from try_get and, if Err, return Err up the function (or log and return an Err) and likewise return an Err when serde_json::from_str fails; keep the existing special-case behavior for s.is_empty() || s == "{}" returning None, but do not swallow any I/O or parse errors from try_get and from_str.src/api/channels.rs-997-1018 (1)
997-1018:⚠️ Potential issue | 🟠 Major404 unknown channels before touching
channel_settings.Both handlers only validate the agent.
GETwill return a default-looking payload for a typo/stalechannel_id, andPUTwill create an orphan settings row for any arbitrary ID. Please checkChannelStorefirst so these endpoints fail closed for channels that do not exist under that agent.Also applies to: 1034-1055
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/channels.rs` around lines 997 - 1018, The handler currently skips validating that the channel actually exists and therefore returns defaults or creates orphan settings; before touching ChannelSettingsStore in get_channel_settings (and the PUT handler at the other location), first instantiate and query the channel lookup (e.g., crate::conversation::ChannelStore::new(pool.clone()) and call its get/exists method with &query.agent_id and &channel_id), return StatusCode::NOT_FOUND if the channel lookup returns None, and only then proceed to use crate::conversation::ChannelSettingsStore::new(...) and ChannelSettingsStore::get(...) to read or write settings so settings are only accessed for existing channels.src/tools.rs-453-499 (2)
453-499:⚠️ Potential issue | 🟠 Major
DelegationMode::Directis still missing the execution toolset.This helper only adds memory recall/save. Shell, file read/write/edit/list, browser/web search, and
memory_deleteare still absent, so the new Direct mode cannot actually deliver the “hands-on / cortex-like” behavior the setting advertises.🤖 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; to implement full "Direct" execution toolset you should also register the execution and file/browser/memory-delete tools via handle.add_tool(...). Specifically, after the existing MemorySaveTool add Tool instances for MemoryDeleteTool::new(state.deps.memory_search.clone()), ShellExecTool::new(state.deps.shell.clone()) (or ExecTool/ShellTool matching your codebase), FileReadTool::new(state.deps.fs.clone()), FileWriteTool::new(state.deps.fs.clone()), FileEditTool::new(state.deps.fs.clone()), FileListTool::new(state.deps.fs.clone()), BrowserTool::new(state.deps.browser.clone()), and WebSearchTool::new(state.deps.web_search.clone()), awaiting each .await?; ensure you use the same dependency fields from state.deps and the same tool constructor names as in your codebase so Direct mode exposes shell, file (read/write/edit/list), browser/web search, and memory_delete functionality.
453-499:⚠️ Potential issue | 🟠 MajorDirect-mode tools need a removal path.
add_direct_mode_tools()mountsmemory_recall/memory_saveonto the same per-turn channel server, butremove_channel_tools()never unregisters them. A conversation that switches from Direct back to Standard will keep direct-only memory tools on the next turn, and re-adding them every Direct turn also risks colliding with existing registrations.docs/design-docs/conversation-settings.md-151-189 (1)
151-189:⚠️ Potential issue | 🟠 MajorCall out the worker/branch contract change explicitly.
WorkerHistoryMode::{Recent,Full}andWorkerMemoryMode::{Tools,Full}make workers consume channel history and branch-level memory access. If that is intentional, this draft should say it is changing the existing worker model and justify why a branch is not sufficient; otherwise future work will read like accidental architectural drift.Based on learnings: 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 - 189, The draft fails to explicitly state that WorkerHistoryMode::{Recent, Full} and WorkerMemoryMode::{Tools, Full} change the worker/branch contract by giving workers channel history and branch-level memory access; update the doc to call this out clearly: state that these options intentionally relax the current model, explain the rationale and trade-offs (why a branch would not suffice), or alternatively restrict/rename these modes (e.g., mark as experimental or move to a "branch-like worker" section) and keep the default behavior as WorkerHistoryMode::None and WorkerMemoryMode::None/Ambient; reference the enums WorkerHistoryMode and WorkerMemoryMode and the specific variants Recent, Full, and Tools when making this clarification.src/main.rs-1923-1938 (1)
1923-1938:⚠️ Potential issue | 🟠 MajorRehydrate platform settings in the idle-worker resume path.
This block always queries
PortalConversationStore, but platform overrides live in the channel-settings path. Once the resumed channel is inserted intoactive_channels, the first real inbound message never reaches the platform-resolution branch below, so a restarted Discord/Slack/etc. conversation keeps hard defaults instead of its persistedresponse_mode, model, and worker-context settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1923 - 1938, The resume path currently always loads settings from PortalConversationStore, which ignores persisted platform/channel overrides; change it to detect channel conversations (e.g., by checking active_channels for conversation_id or by attempting the channel store lookup first) and load from the channel-specific store when present: try ChannelConversationStore::new(...).get(&agent_id.to_string(), &conversation_id).await and, on Ok(Some(conv)), call ResolvedConversationSettings::resolve(conv.settings.as_ref(), None, None); otherwise fallback to PortalConversationStore::new(...).get(...) and finally default()—ensure you reference PortalConversationStore::new, ChannelConversationStore::new, ResolvedConversationSettings::resolve, active_channels and conversation_id in the updated logic.src/llm/model.rs-2604-2631 (1)
2604-2631:⚠️ Potential issue | 🟠 MajorDon’t coerce malformed raw Responses arguments to
{}.
parse_openai_tool_arguments()falls back to{}on parse failure, so a badresponse.output_item.donepayload can still dispatch the tool with empty/default arguments. The chat-completions path already treats malformed streamed arguments as an error; this raw Responses path should do the same and ideally reuseparse_streamed_tool_arguments().As per coding guidelines, "Don't silently discard errors. No
let _ =on Results. Handle them, log them, or propagate them."src/agent/worker.rs-259-261 (1)
259-261:⚠️ Potential issue | 🟠 MajorResumed workers lose their configured model and memory tools.
This recreates every resumed interactive worker with
WorkerMemoryMode::Noneand no model override, so a worker spawned under conversation settings can resume after restart with a different tool surface and a different model. Thread the resolved settings through the resume path, or persist them with the worker row.🤖 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, Resumed workers are being re-created with hardcoded defaults (Vec::new() for initial_history, WorkerMemoryMode::None, and None for model override) which discards the original conversation settings; instead thread the worker's resolved settings through the resume path (pass the saved/history-derived initial_history, the actual memory mode instead of WorkerMemoryMode::None, and the stored model override) or persist those settings on the worker row and read them when resuming so the resumed worker uses the same tools/memory/model as before.src/agent/channel_dispatch.rs-538-562 (1)
538-562:⚠️ Potential issue | 🟠 MajorPassing channel history into workers breaks the worker/branch contract.
RecentandFullclonestate.historyintoinitial_historyand then hand that toWorker::new*(), so workers stop being “fresh prompt + task” executions and start inheriting channel context. In this repo that boundary is intentional: if a task needs conversation history, it should be a branch instead of a worker.Based on learnings: 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: 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 538 - 562, The current code constructs initial_history from state.history for WorkerHistoryMode::Recent and ::Full and then passes it into Worker::new*(), which violates the worker/branch contract; change the logic in the initial_history construction so that WorkerHistoryMode::None, ::Summary, ::Recent and ::Full all result in an empty Vec (i.e. never clone or pass state.history into workers), remove any code paths that call Worker::new*() with channel history, and update the analogous block around the other occurrence (lines referenced in the review) to do the same so only branches (not workers) receive conversation history.src/llm/model.rs-1085-1105 (1)
1085-1105:⚠️ Potential issue | 🟠 MajorFinal SSE reconstruction is less robust than the live parser.
This path reparses
sse_textwithparse_openai_responses_sse_response(), but that helper only recognizes single-linedata:records. The loop above already acceptsdata:without a space and multiline payloads viaextract_sse_data_payload(), so a stream can emit chunks successfully and still fail at finalization with a spurious “missing response.completed event”. Reuse the same block parser here, or capture the completed response when you see that event instead of reparsing the raw text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 1085 - 1105, The finalization currently reparses sse_text with parse_openai_responses_sse_response (which only supports single-line `data: `) causing false failures; instead reuse the same block parser/output used by the streaming loop (or the payload produced by extract_sse_data_payload) so multiline and `data:`-without-space cases succeed. Replace the parse_openai_responses_sse_response(&sse_text, &provider_label) call in the saw_data_event path with the accumulated/completed payload captured by the loop (or call the same block parsing routine used while streaming), then pass that payload into parse_openai_responses_response and yield RawStreamingChoice::FinalResponse using that parsed body and usage (i.e., avoid reparsing raw sse_text and use the loop’s completed response buffer).src/agent/channel.rs-2211-2268 (1)
2211-2268:⚠️ Potential issue | 🟠 MajorBatched turns still ignore
MemoryMode::Off.This updates the single-message prompt builder only.
handle_message_batch()still goes throughbuild_system_prompt_with_coalesce(), which unconditionally injects the memory bulletin, working memory, and activity map. Any coalesced turn will therefore still expose memory context even when the conversation has memory disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 2211 - 2268, The batched-path still injects memory regardless of MemoryMode::Off; update build_system_prompt_with_coalesce (or handle_message_batch) to early-return or short-circuit memory rendering when self.resolved_settings.memory == MemoryMode::Off so it does not call render_working_memory or render_channel_activity_map or set memory_bulletin. Specifically, detect MemoryMode::Off and set working_memory = String::new(), channel_activity_map = String::new(), and memory_bulletin_text = None (or equivalent) before any awaits/formatting, and only call crate::memory::working::render_working_memory, render_channel_activity_map, or build the memory_bulletin when memory is Ambient/Enabled. Ensure the same memory-guard logic used in the single-message flow (the branch around render_working_memory/render_channel_activity_map/memory_bulletin_text) is applied to the coalesced/batched code path so coalesced turns do not expose memory when Off.src/hooks/spacebot.rs-474-480 (1)
474-480:⚠️ Potential issue | 🟠 MajorThe streaming loop overruns the configured turn cap.
Line 474 checks
>before incrementingcurrent_max_turns, so the loop still enters whencurrent_max_turns == max_turns + 1. Even if you want one final completion after the last tool call, this allows one extra model call past that budget.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/spacebot.rs` around lines 474 - 480, The check that prevents the streaming loop from exceeding the allowed turns uses if current_max_turns > max_turns + 1 which still permits an extra model call; change the logic in the streaming loop inside spacebot.rs so the guard triggers before that extra call (e.g., compare current_max_turns >= max_turns + 1 or increment current_max_turns before the check) and return the PromptError::MaxTurnsError with max_turns, chat_history and prompt when the limit is reached to prevent the loop from overrunning the configured cap.src/hooks/spacebot.rs-515-633 (1)
515-633:⚠️ Potential issue | 🟠 MajorDon't stream unvetted channel output.
These
TextDeltaevents are emitted before the turn goes through the later secret/tool-syntax checks. That means raw assistant text or partialreplycontent can be shown to subscribers and only get blocked afterwards. Please gate channel deltas behind the same validation/scrubbing path, or restrict streaming to already-vetted reply content.As per coding guidelines "Scan tool output and user input via
SpacebotHook.on_tool_result()for leak detection (API keys, tokens, PEM keys). Block exfiltration before outbound HTTP."Also applies to: 1097-1155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/spacebot.rs` around lines 515 - 633, The code currently calls <SpacebotHook as PromptHook<M>>::on_text_delta and on_tool_call_delta immediately when receiving StreamedAssistantContent::Text and ::ToolCallDelta, exposing unvetted assistant output; instead buffer incoming Text/TextDelta and ToolCallDelta events (use existing last_text_response, a temporary Vec for pending deltas, and the tool_calls/tool_results buffers) and defer invoking on_text_delta/on_tool_call_delta until after the corresponding tool call is completed and validated via on_tool_result (or after final vetting of the assembled reply); ensure on_tool_call still runs to decide Skip/Continue/Terminate, but do not emit per-delta hooks or channel deltas until after on_tool_result returns and leak-detection/scrubbing is applied; apply the same buffering/deferment logic to the other occurrence referenced (the 1097-1155 block) so no raw/partial reply is passed to the hook callbacks or subscribers before validation.src/agent/channel.rs-646-658 (1)
646-658:⚠️ Potential issue | 🟠 MajorLoad and merge existing settings before upserting to avoid clearing model, memory, and delegation overrides.
The
upsertimplementation replaces the entiresettingsJSON column on conflict (DO UPDATE SET settings = excluded.settings). By creating a freshConversationSettings { response_mode: mode, ..Default::default() }and upserting it, you overwrite any previously saved model overrides, memory mode, delegation mode, or other channel-specific settings with their default values. Load the current settings from the database, merge in the newresponse_mode, and upsert the merged result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 646 - 658, The current set_response_mode implementation creates a fresh ConversationSettings with only response_mode set and upserts it, which clobbers any existing model/memory/delegation overrides; instead load the existing settings from ChannelSettingsStore (e.g., call a getter like store.get(&self.deps.agent_id, self.id.as_ref()).await), merge the new response_mode into that ConversationSettings (preserve other fields), then call store.upsert(&self.deps.agent_id, self.id.as_ref(), &merged_settings). Keep self.resolved_settings.response_mode = mode as-is but ensure the upserted settings are the merged result rather than a default ConversationSettings.src/agent/channel.rs-2335-2379 (1)
2335-2379:⚠️ Potential issue | 🟠 MajorAdd mode-specific tool cleanup for direct delegation mode.
add_direct_mode_tools()registersMemoryRecallToolandMemorySaveToolin addition to standard channel tools. The cleanup in channel.rs (line ~2502) calls onlyremove_channel_tools(), which does not remove these memory tools. They will persist into the next turn or after switching back toDelegationMode::Standard.Either add
remove_direct_mode_tools()that removes memory tools in addition to channel tools, or addMemoryRecallTool::NAMEandMemorySaveTool::NAMEto the removal list inremove_channel_tools().Also applies to: 2501-2503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 2335 - 2379, The cleanup logic only calls remove_channel_tools() but add_direct_mode_tools() also registers MemoryRecallTool and MemorySaveTool, so those memory tools persist when switching modes; update the cleanup to remove them by either (A) creating and calling a new remove_direct_mode_tools() that removes channel tools plus MemoryRecallTool::NAME and MemorySaveTool::NAME, or (B) add MemoryRecallTool::NAME and MemorySaveTool::NAME to the removal list inside remove_channel_tools(); locate usages around add_direct_mode_tools, add_channel_tools and the existing call to remove_channel_tools and ensure the corresponding removal function is invoked when DelegationMode::Direct is used (or universally) to prevent memory tools leaking into subsequent turns.src/conversation/settings.rs-57-71 (1)
57-71: 🛠️ Refactor suggestion | 🟠 MajorDon't make worker history configurable.
Summary,Recent, andFullturn workers into history-bearing executions, which collapses the worker/branch split and invites downstream code to pass channel context into workers. If something needs parent-conversation history, it should be a branch instead.Based on learnings: "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: 128-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/settings.rs` around lines 57 - 71, The WorkerHistoryMode enum currently exposes history-bearing variants (Summary, Recent, Full) which allow workers to receive channel history; remove those variants and limit the enum to only the None variant (keeping #[default] and existing derives/serde attributes) so workers cannot be configured with history; update any referenced uses of WorkerHistoryMode (including the other occurrence mentioned) to assume only WorkerHistoryMode::None (or remove handling for Summary/Recent/Full) and adjust serialization/deserialization expectations accordingly.src/conversation/settings.rs-166-197 (1)
166-197:⚠️ Potential issue | 🟠 MajorUse patch semantics for override layers.
ConversationSettingsgivesmemory,delegation,response_mode, andworker_contextconcrete defaults, andresolve()then copies them unconditionally from any higher-precedence settings object. A partial override like[bindings.settings] response_mode = "quiet"will also reset inherited memory/delegation/worker-context values back tofull/standard/noneinstead of leaving them alone. These fields needOption<_>/ patch structs and a field-by-field merge; please add a regression test for a partial binding/channel override.Also applies to: 240-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/conversation/settings.rs` around lines 166 - 197, ConversationSettings currently uses concrete defaults for memory, delegation, response_mode, and worker_context and resolve() overwrites lower-precedence settings unconditionally; change those four fields in ConversationSettings to Option-wrapped types (or small patch structs if more granular control is needed) so they can represent "no override", update resolve() to merge field-by-field (apply a value only when the higher-precedence Option is Some, leaving the lower value intact), adjust ModelOverrides handling if necessary to preserve precedence semantics, and add a regression test that applies a partial override (e.g. only response_mode = "quiet" in a binding/channel) to verify memory/delegation/worker_context are not reset to defaults.
🟡 Minor comments (10)
src/messaging/target.rs-212-213 (1)
212-213:⚠️ Potential issue | 🟡 MinorMake portal normalization idempotent.
The comment says portal targets are full conversation IDs, but this branch returns the string unchanged. A caller that passes
portal:chat:mainthroughnormalize_target("portal", ...)and later formats aBroadcastTargetwill emitportal:portal:chat:main. Strip a repeatedportal:prefix here, or narrow the contract to only accept the suffix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/messaging/target.rs` around lines 212 - 213, The "portal" arm in normalize_target currently returns trimmed.to_string() which causes duplicated prefixes (e.g. "portal:portal:chat:main"); change the "portal" branch in normalize_target to strip any leading "portal:" prefix from trimmed (e.g., if trimmed starts_with "portal:" remove that leading segment, repeated if necessary) and return the cleaned suffix so normalization is idempotent when BroadcastTarget formatting later prepends "portal:".migrations/20260328000001_channel_settings.sql-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorLet schema drift fail loudly in this migration.
Using
IF NOT EXISTSin a versioned migration makes this non-deterministic: if a stale/manualchannel_settingstable already exists with the wrong columns or constraints, the migration will succeed and leave the app on the wrong schema. Prefer a plainCREATE TABLEhere, or explicitly reconcile the existing table.Suggested change
-CREATE TABLE IF NOT EXISTS channel_settings ( +CREATE TABLE channel_settings (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@migrations/20260328000001_channel_settings.sql` at line 3, The migration uses "CREATE TABLE IF NOT EXISTS channel_settings", which makes the versioned migration non-deterministic; change it to a plain "CREATE TABLE channel_settings" so schema drift fails loudly, or alternatively add explicit reconciliation logic to detect an existing channel_settings table and validate/alter its columns and constraints before proceeding; update the migration that defines the channel_settings table (the CREATE TABLE statement) accordingly and ensure the migration will error if the existing table is incompatible rather than silently succeeding.docs/design-docs/channel-settings-unification.md-37-53 (1)
37-53:⚠️ Potential issue | 🟡 MinorAdd languages to the fenced blocks to clear MD040.
markdownlintis already flagging these unlabeled fences. Tag them (text,rust,toml,sql, etc.) so the new doc stops generating warnings.Also applies to: 57-62, 168-186, 192-197, 218-226
🤖 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 37 - 53, The fenced code blocks in the document (e.g., the block starting with "Message arrives" and other blocks around the referenced regions) are unlabeled and trigger markdownlint rule MD040; update each triple-backtick fence in channel-settings-unification.md (including the blocks at the shown ranges and lines 57-62, 168-186, 192-197, 218-226) by adding an appropriate language tag (for example text, rust, toml, sql, etc.) after the opening ``` so the fences are explicit and MD040 warnings are resolved.docs/design-docs/channel-settings-unification.md-95-113 (1)
95-113:⚠️ Potential issue | 🟡 Minor
MentionOnlyhas two incompatible definitions here.The enum/table say unmentioned traffic is still recorded, while the later routing sections say those messages are dropped before a channel exists. Those imply different persistence, resource, and test behavior, so this draft needs one consistent contract.
Also applies to: 147-162, 168-186
🤖 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 95 - 113, The docs define MentionOnly inconsistently: the enum/table claim unmentioned messages are recorded in history (lightweight) but later routing text says unmentioned traffic is dropped before a channel exists; pick one consistent contract and make all references match. Update the enum comment for MentionOnly, the behavior comparison table rows for MentionOnly (and Quiet if needed), and the routing discussion that mentions "routing-level drop" / require_mention so they all state the same persistence and processing semantics (either "record in history but no memory persistence/LLM" or "dropped before channel creation"); also update any referenced tests or examples that depend on MentionOnly semantics to reflect the chosen behavior.interface/src/components/ConversationSettingsPanel.tsx-185-187 (1)
185-187:⚠️ Potential issue | 🟡 MinorUse the inherited response mode here.
When
currentSettings.response_modeis unset, the panel should mirror the server-provided default. Falling back to"active"makes Quiet/MentionOnly defaults render incorrectly until the user edits this field.Suggested fix
- const currentResponseMode = currentSettings.response_mode || "active"; + const currentResponseMode = + currentSettings.response_mode ?? defaults.response_mode;🤖 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 - 187, currentResponseMode currently falls back to the hardcoded "active" which overrides the server-provided default; change the assignment for currentResponseMode to use the inherited default (e.g., use currentSettings.response_mode ?? defaults.response_mode ?? "active") so the panel mirrors the server default when currentSettings.response_mode is unset; update the variable where it's declared (currentResponseMode) to use this nullish-coalescing fallback.docs/design-docs/conversation-settings.md-318-325 (1)
318-325:⚠️ Potential issue | 🟡 MinorUpdate the channel persistence/API section to match the implementation.
This draft still describes
ALTER TABLE channels ADD COLUMN settings TEXTandGET /api/channels/{id}for reads. The code in this PR persists per-channel overrides via a dedicated channel-settings store and exposes dedicatedGET/PUTsettings endpoints, so this doc currently points follow-up work at the wrong schema and route shape.Also applies to: 432-450
🤖 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 318 - 325, The docs currently instruct adding a settings TEXT column to the channels table and reading via GET /api/channels/{id}, but the code stores per-channel overrides in the channel-settings store and exposes dedicated settings endpoints; update the "channels" section to remove the ALTER TABLE suggestion and instead document that per-channel overrides live in the channel-settings store (falling back to agent defaults when absent) and replace references to GET /api/channels/{id} with the actual settings endpoints used by the implementation (the dedicated GET and PUT channel-settings endpoints for a given channel ID), updating any examples and prose accordingly (search for "channels table", "settings", and "GET /api/channels/{id}" to find and change all occurrences).docs/design-docs/conversation-settings.md-41-41 (1)
41-41:⚠️ Potential issue | 🟡 MinorAdd languages to the fenced blocks.
markdownlint is already flagging these examples. Tagging them (
text,rust,sql,toml,json,typescript) will clear the warnings and improve rendering in GitHub.Also applies to: 104-104, 254-254, 560-560, 769-769, 776-776, 785-785, 801-801, 807-807, 834-834, 852-852, 870-870, 903-903
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design-docs/conversation-settings.md` at line 41, Several fenced code blocks in docs/design-docs/conversation-settings.md are missing language tags (they currently end with bare ```), which triggers markdownlint — update each fenced block example to include the appropriate language identifier (e.g., ```text, ```rust, ```sql, ```toml, ```json, ```typescript) so GitHub renders them correctly; search for the bare triple-backtick examples referenced in the comment and replace the closing/opening backticks with the matching language tag for each code sample.docs/design-docs/conversation-settings.md-104-110 (1)
104-110:⚠️ Potential issue | 🟡 MinorDocument
response_modein the canonical settings schema.The central
ConversationSettingsdefinition leaves outResponseMode, and the later channel mockup falls back to a boolean “Mention Only”. That no longer matches the unified settings model in this PR, so the precedence, API, and migration sections are incomplete as written.Also applies to: 899-912
🤖 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 104 - 110, Add the missing ResponseMode to the canonical ConversationSettings schema by adding a response_mode: ResponseMode field to the ConversationSettings definition and replace the boolean “Mention Only” in the channel mockup with the proper ResponseMode enum option; update the Precedence, API, and Migration sections to describe how response_mode is resolved (defaults, overrides per-channel and per-conversation), how clients should send/receive the new response_mode field in APIs, and provide migration guidance for converting the old boolean “Mention Only” flag to the new ResponseMode values so the schema, channel examples, and docs are consistent (references: ConversationSettings, ResponseMode, and the previous “Mention Only” flag).interface/src/api/client.ts-2064-2069 (1)
2064-2069:⚠️ Potential issue | 🟡 MinorMissing error handling for
updateChannelSettings.Unlike
getChannelSettingswhich usesfetchJson,updateChannelSettingsreturns a rawResponsewithout checkingresponse.ok. This is inconsistent with similar PUT methods in this file (e.g.,updateAgentConfig) that throw on non-OK responses.Proposed fix
- updateChannelSettings: (channelId: string, agentId: string, settings: Types.ConversationSettings) => - fetch(`${getApiBase()}/channels/${encodeURIComponent(channelId)}/settings`, { + updateChannelSettings: async (channelId: string, agentId: string, settings: Types.ConversationSettings) => { + const response = await fetch(`${getApiBase()}/channels/${encodeURIComponent(channelId)}/settings`, { method: "PUT", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ agent_id: agentId, settings }), - }), + }); + if (!response.ok) { + throw new Error(`API error: ${response.status}`); + } + return response.json() as Promise<{ conversation_id: string; settings: Types.ConversationSettings }>; + },🤖 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 2064 - 2069, The updateChannelSettings function returns a raw fetch Response and lacks the error handling used elsewhere (e.g., getChannelSettings and updateAgentConfig); update it to use the same pattern — call fetchJson or await fetch then check response.ok and throw a descriptive error on non-OK responses — and return the parsed JSON or appropriate value like other API methods; reference updateChannelSettings (and mirror behavior from getChannelSettings/updateAgentConfig) so the method consistently throws on failures and returns the expected parsed result.src/conversation/portal.rs-239-291 (1)
239-291:⚠️ Potential issue | 🟡 MinorBackfill query doesn't filter by agent_id but loop assumes single agent.
Line 241 queries all
conversation_messageswithchannel_id LIKE 'portal:chat:%'without filtering byagent_id. However, the loop callsself.get(agent_id, &channel_id)which filters by the passedagent_id. This means:
- The query returns conversations for all agents
- The loop only processes those matching the current
agent_id- This is inefficient and could create conversations with mismatched
agent_idThe
channel_idformat isportal:chat:{agent_id}:{uuid}, so you could extract the agent from it or filter the query.Suggested fix to filter by agent_id pattern
async fn backfill_from_messages(&self, agent_id: &str) -> crate::error::Result<()> { + let pattern = format!("portal:chat:{}:%", agent_id); let rows = sqlx::query( - "SELECT DISTINCT channel_id FROM conversation_messages WHERE channel_id LIKE 'portal:chat:%'", + "SELECT DISTINCT channel_id FROM conversation_messages WHERE channel_id LIKE ?", ) + .bind(&pattern) .fetch_all(&self.pool)🤖 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 - 291, The backfill_from_messages function currently selects all channel_ids matching 'portal:chat:%' but then calls self.get(agent_id, &channel_id) assuming the channel belongs to the provided agent_id; change the query to restrict results to the current agent by using the agent_id in the SQL filter (e.g. channel_id LIKE 'portal:chat:{agent_id}:%') or extract the agent segment from channel_id and skip rows where it doesn't match agent_id before calling self.get; update the sqlx::query that selects DISTINCT channel_id and any subsequent binds to include agent_id so only that agent's conversations are processed, leaving the rest of the logic (title generation via generate_title, default_title, and INSERT into portal_conversations) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f0d0b97-8264-4310-9682-211f078ef4ec
⛔ Files ignored due to path filters (6)
desktop/src-tauri/Cargo.lockis excluded by!**/*.lock,!**/*.lockdesktop/src-tauri/Cargo.tomlis excluded by!**/*.tomldesktop/src-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**desktop/src-tauri/gen/schemas/macOS-schema.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**packages/api-client/package.jsonis excluded by!**/*.json
📒 Files selected for processing (56)
docs/design-docs/api-client-package-followup.mddocs/design-docs/channel-settings-unification.mddocs/design-docs/conversation-settings.mdinterface/src/api/client.tsinterface/src/api/schema.d.tsinterface/src/api/types.tsinterface/src/components/ChannelCard.tsxinterface/src/components/ConversationSettingsPanel.tsxinterface/src/components/ConversationsSidebar.tsxinterface/src/components/WebChatPanel.tsxinterface/src/hooks/usePortal.tsinterface/src/routes/Overlay.tsxjustfilemigrations/20260324000001_webchat_conversations.sqlmigrations/20260325120000_rename_webchat_to_portal.sqlmigrations/20260325130000_add_portal_conversation_settings.sqlmigrations/20260328000001_channel_settings.sqlpackages/api-client/src/client.tspackages/api-client/src/events.tspackages/api-client/src/index.tspackages/api-client/src/schema.d.tspackages/api-client/src/types.tssrc/agent/branch.rssrc/agent/channel.rssrc/agent/channel_dispatch.rssrc/agent/channel_history.rssrc/agent/compactor.rssrc/agent/cortex.rssrc/agent/worker.rssrc/api.rssrc/api/agents.rssrc/api/channels.rssrc/api/models.rssrc/api/portal.rssrc/api/server.rssrc/api/state.rssrc/api/webchat.rssrc/config/load.rssrc/config/runtime.rssrc/config/toml_schema.rssrc/config/types.rssrc/conversation.rssrc/conversation/channel_settings.rssrc/conversation/portal.rssrc/conversation/settings.rssrc/cron/scheduler.rssrc/hooks/spacebot.rssrc/llm/model.rssrc/main.rssrc/messaging.rssrc/messaging/portal.rssrc/messaging/target.rssrc/settings.rssrc/settings/store.rssrc/tools.rssrc/tools/spawn_worker.rs
💤 Files with no reviewable changes (2)
- src/settings/store.rs
- src/api/webchat.rs
- 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>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/agent/channel_history.rs (1)
433-436: Add a direct unit test forSettingsUpdatedfiltering.This new branch is correct, but it currently lacks an explicit match/non-match test for
ProcessEvent::SettingsUpdated, which makes future regressions easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_history.rs` around lines 433 - 436, Add a focused unit test for the SettingsUpdated branch of the filter: construct two ProcessEvent::SettingsUpdated events (one with channel_id equal to the test channel_id and one with a different channel_id) and assert that the filtering function in channel_history (the match arm handling ProcessEvent::SettingsUpdated) returns true for the matching event and false for the non-matching event; place the test alongside other channel_history tests and reuse the same helper/constructor used elsewhere to build ProcessEvent values so it exercises the exact branch (ProcessEvent::SettingsUpdated { channel_id: ..., .. }) you changed.interface/src/components/ChannelCard.tsx (2)
120-129: Query invalidation uses partial key; consider using full key for consistency.Line 126 invalidates
["channel-settings", channel.id]but the query (line 104) uses["channel-settings", channel.id, channel.agent_id]. React Query's default prefix matching should work, but using the full key improves clarity and avoids potential issues if multiple agents share channel IDs.♻️ Suggested fix
onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ["channel-settings", channel.id] }); + queryClient.invalidateQueries({ queryKey: ["channel-settings", channel.id, channel.agent_id] }); setShowSettings(false); },🤖 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 120 - 129, The query invalidation in saveSettingsMutation currently calls queryClient.invalidateQueries with the partial key ["channel-settings", channel.id]; update the invalidate call to use the full key used when fetching (["channel-settings", channel.id, channel.agent_id]) so it targets the exact query; locate saveSettingsMutation and replace the argument to queryClient.invalidateQueries accordingly (keeping onSuccess behavior and setShowSettings(false)).
198-211: Missing error state handling for defaults query.When
defaultsfails to load (network error, etc.), the UI shows "Loading..." indefinitely.WebChatPanelhandles this with an error message. Consider adding similar error handling:♻️ Suggested improvement
+ const { data: defaults, error: defaultsError } = useQuery<ConversationDefaultsResponse>({ queryKey: ["conversation-defaults", channel.agent_id], queryFn: () => api.getConversationDefaults(channel.agent_id), enabled: showSettings, }); ... <PopoverContent align="end" sideOffset={4} className="w-96 p-3" onClick={(e) => e.preventDefault()}> - {defaults ? ( + {defaultsError ? ( + <div className="py-4 text-center text-xs text-red-400"> + {defaultsError instanceof Error ? defaultsError.message : "Failed to load settings"} + </div> + ) : defaults ? ( <ConversationSettingsPanel ... /> ) : ( <div className="py-4 text-center text-xs text-ink-faint">Loading...</div> )} </PopoverContent>🤖 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 198 - 211, The PopoverContent currently only checks for a truthy defaults and otherwise shows "Loading...", which hides query errors; update the rendering logic around defaults (the variable from the defaults query) inside the PopoverContent so it also checks the query error state (e.g., defaultsError / defaultsQuery.isError) and renders an error message UI (similar to WebChatPanel) with optional retry, instead of the indefinite "Loading..."; ensure ConversationSettingsPanel is still used when defaults is present and wire any retry to re-fetch the same query and retain existing handlers like onSave (saveSettingsMutation) and onCancel (setShowSettings).interface/src/api/types.ts (2)
124-165: Manually defined types may drift from backend.
ConversationSettings,ModelOverrides, andConversationDefaultsResponseare defined locally rather than derived fromcomponents["schemas"]. If the backend adds or changes fields, these won't auto-update. Consider adding a// TODO: migrate to schema once backend types are generatedcomment, or verify these match the backend definitions.🤖 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 124 - 165, The manually defined types ConversationSettings, ModelOverrides, and ConversationDefaultsResponse risk drifting from backend schemas; add a clear TODO comment above each type (or above the block) stating "TODO: migrate to components[\"schemas\"]-generated types once backend types are available" and, if immediate migration is feasible, replace these manual definitions with imports or type aliases referencing the generated schema types (e.g., components["schemas"].ConversationSettings) so the types stay in sync with the backend.
172-182: Portal request types defined manually instead of using schema aliases.
CreatePortalConversationRequestandUpdatePortalConversationRequestare manually defined rather than aliasing schema types like the other Portal types. If the schema'sCreateWebChatConversationRequest/UpdateWebChatConversationRequestalready includesettings, consider using them. Otherwise, this is fine but should be migrated when the schema is updated.🤖 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 172 - 182, CreatePortalConversationRequest and UpdatePortalConversationRequest are manually declared duplicates; replace them by aliasing the corresponding schema types (e.g., CreateWebChatConversationRequest and UpdateWebChatConversationRequest) to avoid drift and keep parity with the schema. Update the type declarations so CreatePortalConversationRequest = CreateWebChatConversationRequest and UpdatePortalConversationRequest = UpdateWebChatConversationRequest (or import and re-export those schema types), and ensure ConversationSettings is preserved or removed only if already included in the aliased schema types; adjust imports accordingly.interface/src/components/WebChatPanel.tsx (1)
196-212: Inconsistent error handling patterns between queries.
listPortalConversationsmanually checksresponse.okwhilegetConversationDefaultsrelies on thefetchJsonhelper's internal error handling. Both work, but the inconsistency may cause confusion. Consider unifying—either usefetchJsonfor both or manual checks for both.🤖 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 196 - 212, Unify error handling between the two useQuery calls by making listPortalConversations use the same error behavior as getConversationDefaults (or vice versa); specifically, update the queryFn for the ["portal-conversations", agentId] useQuery (where listPortalConversations is called) to delegate to the shared fetch helper used by getConversationDefaults (or add the same manual response.ok check to getConversationDefaults) so both queryFns surface errors consistently to react-query; change the queryFn implementation around listPortalConversations (and ensure the returned value shape still provides .conversations) to match the pattern used by getConversationDefaults/getConversationDefaults helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/WebChatPanel.tsx`:
- Around line 284-295: The settings state isn't synced when activeConversationId
changes; add a useQuery that fetches the conversation (queryKey
["portal-conversation", activeConversationId], queryFn ->
api.getPortalConversation(agentId, activeConversationId), enabled:
!!activeConversationId) and a useEffect that watches conversation data and
activeConversationId and calls
setSettings(conversationData.conversation.settings) or setSettings({}) if none,
so saveSettingsMutation uses the current conversation's settings; mirror the
pattern used in ChannelCard to initialize/reset local settings.
---
Nitpick comments:
In `@interface/src/api/types.ts`:
- Around line 124-165: The manually defined types ConversationSettings,
ModelOverrides, and ConversationDefaultsResponse risk drifting from backend
schemas; add a clear TODO comment above each type (or above the block) stating
"TODO: migrate to components[\"schemas\"]-generated types once backend types are
available" and, if immediate migration is feasible, replace these manual
definitions with imports or type aliases referencing the generated schema types
(e.g., components["schemas"].ConversationSettings) so the types stay in sync
with the backend.
- Around line 172-182: CreatePortalConversationRequest and
UpdatePortalConversationRequest are manually declared duplicates; replace them
by aliasing the corresponding schema types (e.g.,
CreateWebChatConversationRequest and UpdateWebChatConversationRequest) to avoid
drift and keep parity with the schema. Update the type declarations so
CreatePortalConversationRequest = CreateWebChatConversationRequest and
UpdatePortalConversationRequest = UpdateWebChatConversationRequest (or import
and re-export those schema types), and ensure ConversationSettings is preserved
or removed only if already included in the aliased schema types; adjust imports
accordingly.
In `@interface/src/components/ChannelCard.tsx`:
- Around line 120-129: The query invalidation in saveSettingsMutation currently
calls queryClient.invalidateQueries with the partial key ["channel-settings",
channel.id]; update the invalidate call to use the full key used when fetching
(["channel-settings", channel.id, channel.agent_id]) so it targets the exact
query; locate saveSettingsMutation and replace the argument to
queryClient.invalidateQueries accordingly (keeping onSuccess behavior and
setShowSettings(false)).
- Around line 198-211: The PopoverContent currently only checks for a truthy
defaults and otherwise shows "Loading...", which hides query errors; update the
rendering logic around defaults (the variable from the defaults query) inside
the PopoverContent so it also checks the query error state (e.g., defaultsError
/ defaultsQuery.isError) and renders an error message UI (similar to
WebChatPanel) with optional retry, instead of the indefinite "Loading...";
ensure ConversationSettingsPanel is still used when defaults is present and wire
any retry to re-fetch the same query and retain existing handlers like onSave
(saveSettingsMutation) and onCancel (setShowSettings).
In `@interface/src/components/WebChatPanel.tsx`:
- Around line 196-212: Unify error handling between the two useQuery calls by
making listPortalConversations use the same error behavior as
getConversationDefaults (or vice versa); specifically, update the queryFn for
the ["portal-conversations", agentId] useQuery (where listPortalConversations is
called) to delegate to the shared fetch helper used by getConversationDefaults
(or add the same manual response.ok check to getConversationDefaults) so both
queryFns surface errors consistently to react-query; change the queryFn
implementation around listPortalConversations (and ensure the returned value
shape still provides .conversations) to match the pattern used by
getConversationDefaults/getConversationDefaults helper.
In `@src/agent/channel_history.rs`:
- Around line 433-436: Add a focused unit test for the SettingsUpdated branch of
the filter: construct two ProcessEvent::SettingsUpdated events (one with
channel_id equal to the test channel_id and one with a different channel_id) and
assert that the filtering function in channel_history (the match arm handling
ProcessEvent::SettingsUpdated) returns true for the matching event and false for
the non-matching event; place the test alongside other channel_history tests and
reuse the same helper/constructor used elsewhere to build ProcessEvent values so
it exercises the exact branch (ProcessEvent::SettingsUpdated { channel_id: ...,
.. }) you changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 603d5cde-ef1f-48d4-ab15-0cfe6adac32d
📒 Files selected for processing (9)
interface/src/api/types.tsinterface/src/components/ChannelCard.tsxinterface/src/components/WebChatPanel.tsxsrc/agent/channel.rssrc/agent/channel_history.rssrc/agent/cortex.rssrc/api/channels.rssrc/api/portal.rssrc/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/agent/cortex.rs
- src/api/channels.rs
- src/api/portal.rs
- src/agent/channel.rs
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>
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/channel.rs (1)
943-953:⚠️ Potential issue | 🟡 MinorExpose
/mention-onlyin the built-in help.The help text still uses the old "listen-only" wording for
/quietand doesn't list/mention-only, so the command surface shown to users is already out of date.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 943 - 953, Update the "/help" match arm's lines array so the displayed text matches current commands: change the "/quiet" entry from "listen-only mode" to "mention-only mode" and add a new entry for "/mention-only" (e.g. "- /mention-only: mention-only mode") in the same lines array; locate the block that builds the lines array inside the "/help" arm in channel.rs to make these string updates.
♻️ Duplicate comments (3)
src/agent/channel.rs (1)
688-700:⚠️ Potential issue | 🟠 MajorThis response-mode upsert still blows away the rest of the channel settings.
ConversationSettings { response_mode: mode, ..Default::default() }writes a fresh settings row, so/quiet//active//mention-onlywill erase any persisted model, memory, delegation, worker-context, or per-process overrides. When you fix that, keep the write off the command path so the ack doesn't wait on SQLite. As per coding guidelines, "Usetokio::spawnfor fire-and-forget database writes (conversation history saves, memory writes, worker log persistence) so the user gets their response immediately."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel.rs` around lines 688 - 700, The upsert in set_response_mode is overwriting all persisted settings by writing ConversationSettings { response_mode: mode, ..Default::default() }; instead, load the existing settings from ChannelSettingsStore (e.g., store.get(&self.deps.agent_id, self.id.as_ref()).await), clone/merge only the response_mode into that existing ConversationSettings, and then fire-and-forget the upsert using tokio::spawn so the command path doesn't await SQLite; update references in set_response_mode (resolved_settings, ChannelSettingsStore::upsert, ConversationSettings) to merge rather than replace, and ensure the spawned task calls upsert with the merged settings.src/config/load.rs (1)
2345-2377:⚠️ Potential issue | 🟠 Major
[bindings.settings]can't preserve inheritance in this form.
ConversationSettingsis seeded fromDefault::default(), so a binding that only setsmodelorsave_attachmentsstill materializes defaultmemory/delegation/response_modevalues. On top of that, unknown strings here silently coerce instead of followingparse_response_mode()'s warn-and-ignore behavior, so higher-level defaults never get a chance to apply.🤖 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 - 2377, The mapping for b.settings currently seeds a new ConversationSettings with Default::default(), which forces default enum values and prevents inheritance; change it to start from the existing higher-level/default ConversationSettings (rather than Default::default()) and only override fields when the binding explicitly provides them (i.e., only set model/save_attachments when s.model/s.save_attachments are Some), and for the enum string fields (s.memory, s.delegation, s.response_mode) use the existing parse_* helpers (e.g., parse_response_mode or equivalent) that return a Result/Option and only assign the parsed value on success so unknown strings are ignored (and logged) instead of coercing to a fallback enum; update the closure at b.settings.map and the handling of s.memory/s.delegation/s.response_mode accordingly.interface/src/components/WebChatPanel.tsx (1)
196-218:⚠️ Potential issue | 🟠 MajorLoad the selected conversation’s settings before using
settings.The new effect only clears local state on conversation changes. There is still no read path that hydrates
settingsfrom the active portal conversation, so existing overrides disappear from the header andsaveSettingsMutationcan overwrite them with{}when the user saves without reapplying every field.Also applies to: 290-301, 323-338
🤖 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 196 - 218, The effect currently only clears local settings on conversation change; add a read path that hydrates settings from the selected portal conversation so empty `{}` doesn't overwrite persisted values. Specifically, after conversations (conversationsData) finish loading and when activeConversationId changes, find the active conversation in conversations and call setSettings(activeConversation.settings || {}) (and setShowSettings(false) as needed), and ensure saveSettingsMutation uses the hydrated `settings` (or disable the save UI until settings are loaded) so saveSettingsMutation does not persist an empty object; update the useEffect that references setSettings/settings/activeConversationId and any save handler (saveSettingsMutation) to rely on this hydration logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/components/ChannelCard.tsx`:
- Around line 109-113: The effect that syncs settings from channelSettingsData
only runs when channelSettingsData changes, so reopen of the settings popover
can show an old edited draft; update the useEffect that currently references
channelSettingsData and setSettings so it also depends on the popover open flag
(e.g., isSettingsOpen / isPopoverOpen) and, when the popover opens, reset
settings to channelSettingsData?.settings via
setSettings(channelSettingsData.settings); ensure you guard for
channelSettingsData being undefined and perform this reset only on open (not
every render).
- Around line 125-128: The onSuccess handler currently only invalidates
["channel-settings", channel.id] and closes the popover, so the parent channel
data (which supplies channel.response_mode) isn't refreshed; update the
onSuccess in the mutation(s) (the callback that calls
queryClient.invalidateQueries and setShowSettings) to also invalidate the parent
channel query (e.g. call queryClient.invalidateQueries({ queryKey: ["channel",
channel.id] }) or queryClient.invalidateQueries({ queryKey: ["channels"] })
depending on which key is used to fetch the channel list) so the ChannelCard
badge reflects the saved response_mode; apply the same change to the other
onSuccess block around lines 162-171.
In `@src/agent/channel.rs`:
- Around line 647-679: The hot-reload currently collapses any get() failure to
None and calls ResolvedConversationSettings::resolve with no inherited defaults,
which can reset a live channel to hard defaults; update the logic in the channel
hot-reload to preserve the current effective settings on lookup errors or
missing partial records instead of replacing them: when
PortalConversationStore::get or ChannelSettingsStore::get returns Err or
Ok(None), log the error (or missing record) and reuse the existing resolved
settings (e.g. self.resolved_settings or self.state.model_overrides) as the
inherited/new_settings input to ResolvedConversationSettings::resolve so
transient DB failures or partial overrides do not silently clear live settings,
and only apply the newly resolved settings on success.
In `@src/api/channels.rs`:
- Around line 1092-1103: The send to channel_state.deps.event_tx currently
ignores failures; change the code that calls channel_states =
state.channel_states.read().await and the block that does
channel_state.deps.event_tx.send(...) so you handle the Result instead of using
"let _ =". Match on the send result and log an error (including the channel_id
and agent_id context and the error) when send returns Err, e.g. for the
ProcessEvent::SettingsUpdated send; do not silently discard the error so
hot-reload failures are visible at runtime.
In `@src/main.rs`:
- Around line 1933-1952: The current chain treats Err from portal_store.get and
channel_store.get as "no settings" and silently falls back to default; change
the logic to explicitly match the Result for each call, log any Err with context
(include agent_id_str and conversation_id) before falling back, then only call
ResolvedConversationSettings::resolve when Ok(Some(...)) or default when
Ok(None); update the branches around portal_store.get(...) and
channel_store.get(...) to use match or if let Err(e) = ... { error!(%e, agent_id
= %agent_id_str, convo = %conversation_id, "failed to read settings from
portal_store"); } (and similarly for channel_store) so database errors are not
discarded.
- Around line 2189-2224: On DB read errors for portal_conversations or
channel_settings, don't return ResolvedConversationSettings::default(); instead
call ResolvedConversationSettings::resolve(...) passing None for the DB settings
and binding_settings.as_ref() (same pattern used in the Ok(None) branches) so
binding defaults (response_mode/memory/model overrides) are preserved; update
the Err arms where store.get(...) and the portal read error currently return
ResolvedConversationSettings::default() to use
ResolvedConversationSettings::resolve(None, binding_settings.as_ref(), None).
---
Outside diff comments:
In `@src/agent/channel.rs`:
- Around line 943-953: Update the "/help" match arm's lines array so the
displayed text matches current commands: change the "/quiet" entry from
"listen-only mode" to "mention-only mode" and add a new entry for
"/mention-only" (e.g. "- /mention-only: mention-only mode") in the same lines
array; locate the block that builds the lines array inside the "/help" arm in
channel.rs to make these string updates.
---
Duplicate comments:
In `@interface/src/components/WebChatPanel.tsx`:
- Around line 196-218: The effect currently only clears local settings on
conversation change; add a read path that hydrates settings from the selected
portal conversation so empty `{}` doesn't overwrite persisted values.
Specifically, after conversations (conversationsData) finish loading and when
activeConversationId changes, find the active conversation in conversations and
call setSettings(activeConversation.settings || {}) (and setShowSettings(false)
as needed), and ensure saveSettingsMutation uses the hydrated `settings` (or
disable the save UI until settings are loaded) so saveSettingsMutation does not
persist an empty object; update the useEffect that references
setSettings/settings/activeConversationId and any save handler
(saveSettingsMutation) to rely on this hydration logic.
In `@src/agent/channel.rs`:
- Around line 688-700: The upsert in set_response_mode is overwriting all
persisted settings by writing ConversationSettings { response_mode: mode,
..Default::default() }; instead, load the existing settings from
ChannelSettingsStore (e.g., store.get(&self.deps.agent_id,
self.id.as_ref()).await), clone/merge only the response_mode into that existing
ConversationSettings, and then fire-and-forget the upsert using tokio::spawn so
the command path doesn't await SQLite; update references in set_response_mode
(resolved_settings, ChannelSettingsStore::upsert, ConversationSettings) to merge
rather than replace, and ensure the spawned task calls upsert with the merged
settings.
In `@src/config/load.rs`:
- Around line 2345-2377: The mapping for b.settings currently seeds a new
ConversationSettings with Default::default(), which forces default enum values
and prevents inheritance; change it to start from the existing
higher-level/default ConversationSettings (rather than Default::default()) and
only override fields when the binding explicitly provides them (i.e., only set
model/save_attachments when s.model/s.save_attachments are Some), and for the
enum string fields (s.memory, s.delegation, s.response_mode) use the existing
parse_* helpers (e.g., parse_response_mode or equivalent) that return a
Result/Option and only assign the parsed value on success so unknown strings are
ignored (and logged) instead of coercing to a fallback enum; update the closure
at b.settings.map and the handling of s.memory/s.delegation/s.response_mode
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e799d82-4416-40fe-b7ad-bddde0943e17
📒 Files selected for processing (6)
interface/src/components/ChannelCard.tsxinterface/src/components/WebChatPanel.tsxsrc/agent/channel.rssrc/api/channels.rssrc/config/load.rssrc/main.rs
- 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>
…pdate tests The `settings` field was removed from SlackConfig, TelegramConfig, and TelegramInstanceConfig but test code still referenced it. Also adds missing `model_overrides` and `worker_context_settings` fields to ChannelState initializers and missing args to create_worker_tool_server calls in context_dump tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…llapsible_if, collapsible_match, unwrap_or_default) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
ResponseModeenum (Active/Quiet/MentionOnly) replacing the fragmentedlisten_only_mode+require_mentionsystem. Unified underConversationSettingsfor both portal and platform channels.channel_settingsSQLite table. Platform channels (Discord, Slack, etc.) now load and persist settings alongside portal conversations.[bindings.settings]in TOML provides default settings for channels matched by a binding. Per-channel overrides always win.WorkerMemoryMode.listen_only_modeinfrastructure from Channel, RuntimeConfig, and SettingsStore. All response mode state flows throughConversationSettings/ResponseMode.Design docs included:
docs/design-docs/conversation-settings.mddocs/design-docs/channel-settings-unification.mdTest plan
/quiet,/active,/mention-onlycommands persist tochannel_settingstable and apply immediately[bindings.settings].response_mode = "quiet"in TOML → verify new channels default to quiet/activeon a quiet-by-default channel → verify per-channel override winslisten_only_mode = truein TOML still works with deprecation warningcargo checkandnpx tsc --noEmitpass clean🤖 Generated with Claude Code
Note
This PR consolidates conversation and channel settings into a unified system. The final commit removes legacy listen_only_mode infrastructure, completing the refactor. Settings now flow through a single
ResponseModeenum across all channel types (portal and platform), with per-channel persistence to SQLite and binding-level defaults via TOML. Workers receive conditional memory tools based on conversation memory mode. All response mode behavior (Active/Quiet/MentionOnly) is now centralized inConversationSettings, eliminating scattered state management.Written by Tembo for commit 15c9fe9. This will update automatically on new commits.