diff --git a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx index d7b4361a7..45c5f9b57 100644 --- a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx +++ b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx @@ -26,6 +26,35 @@ interface CreateWorkflowFromSessionModalProps { const KNOWLEDGE_SELECTION_UNREAD = "Could not load this chat's knowledge bases, so none were selected automatically."; +/** + * The primary a captured selection keeps: the one it names, if that base is + * among the ones the workflow will see, and otherwise none. + * + * ⚠ **Never the first visible base.** A saved `default` becomes the primary of + * every chat the workflow starts (`apply_knowledge_selection`), and the primary + * is where KB-less writes go. The daemon never infers it from `visible` + * (`plan_knowledge_selection` in `crates/biorouter/src/workflow/runtime.rs`), + * so a chat with no primary gives a workflow with no primary. Falling back to + * `visible[0]` gave each of those chats a write target the chat it was captured + * from never had. + * + * A primary outside `visible` is dropped, where the daemon would union it in. + * The daemon unions a `default` because somebody wrote that workflow and meant + * it; a captured primary outside its own set is an inconsistent read instead. + * The generated block is one locked snapshot and never is one, but this + * modal's own read is two requests, and a base created or deleted between their + * answers leaves the selection naming a base the list lacks. Unioning it could + * save a deleted base as the default, which `set_visible_kbs` refuses, so every + * chat the workflow starts would fail; after a failed list, it would save the + * primary as the only visible base and hide every other one. + */ +function primaryAmong( + primary: string | null | undefined, + visible: readonly string[] +): string | null { + return primary && visible.includes(primary) ? primary : null; +} + export default function CreateWorkflowFromSessionModal({ isOpen, onClose, @@ -143,10 +172,8 @@ export default function CreateWorkflowFromSessionModal({ const visible = bases .filter((base) => !selection.hiddenKbIds.has(base.id)) .map((base) => base.id); - const primary = selection.primaryKbId; - const defaultId = primary && visible.includes(primary) ? primary : (visible[0] ?? null); setWorkflowKnowledgeBaseIds(visible); - setDefaultKnowledgeBaseId(defaultId); + setDefaultKnowledgeBaseId(primaryAmong(selection.primaryKbId, visible)); }); // The daemon's catalog, so a skill bundled inside an installed extension @@ -233,21 +260,11 @@ export default function CreateWorkflowFromSessionModal({ // modal's own read said, the selection is known now. setKnowledgeSelectionUnread(false); const visible = workflow.knowledge_bases.visible ?? []; - generatedResourcesRef.current.knowledgeBases = { - default: - workflow.knowledge_bases.default && - visible.includes(workflow.knowledge_bases.default) - ? workflow.knowledge_bases.default - : (visible[0] ?? null), - visible, - }; + // The daemon sends no `default` at all for a chat with no primary. + const defaultId = primaryAmong(workflow.knowledge_bases.default, visible); + generatedResourcesRef.current.knowledgeBases = { default: defaultId, visible }; setWorkflowKnowledgeBaseIds(visible); - setDefaultKnowledgeBaseId( - workflow.knowledge_bases.default && - visible.includes(workflow.knowledge_bases.default) - ? workflow.knowledge_bases.default - : (visible[0] ?? null) - ); + setDefaultKnowledgeBaseId(defaultId); } if (workflow.skills && workflow.skills.length > 0) { @@ -492,11 +509,11 @@ export default function CreateWorkflowFromSessionModal({ onKnowledgeBaseIdsChange={(ids) => { resourceEditsRef.current.knowledgeBases = true; setWorkflowKnowledgeBaseIds(ids); - if (defaultKnowledgeBaseId && !ids.includes(defaultKnowledgeBaseId)) { - setDefaultKnowledgeBaseId(ids[0] ?? null); - } else if (!defaultKnowledgeBaseId && ids.length > 0) { - setDefaultKnowledgeBaseId(ids[0]); - } + // Switching bases on or off never names a primary: only the + // picker's Default control does. A primary already named stays + // while its base is selected, and goes when it is not, rather + // than passing to whichever base is left. + setDefaultKnowledgeBaseId((current) => primaryAmong(current, ids)); }} defaultKnowledgeBaseId={defaultKnowledgeBaseId} onDefaultKnowledgeBaseIdChange={(id) => { diff --git a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx index d06994d93..4a8f963a3 100644 --- a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx +++ b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx @@ -1,9 +1,19 @@ -import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from 'vitest'; +import { + describe, + it, + expect, + vi, + beforeAll, + afterAll, + beforeEach, + afterEach, + type MockInstance, +} from 'vitest'; import { act, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import CreateWorkflowFromSessionModal from '../CreateWorkflowFromSessionModal'; import { createWorkflow, getActive, listBases, skillCatalogHandler } from '../../../api/sdk.gen'; -import type { CreateWorkflowResponse } from '../../../api/types.gen'; +import type { CreateWorkflowResponse, WorkflowKnowledgeBases } from '../../../api/types.gen'; import { saveWorkflow } from '../../../workflow/workflow_management'; import { reachGatedGetActive, USER_ACTION_KEY } from '../../../test/reachGate'; @@ -85,6 +95,28 @@ const mockListBases = vi.mocked(listBases); const mockSkillCatalog = vi.mocked(skillCatalogHandler); const mockSaveWorkflow = vi.mocked(saveWorkflow); +/** Cross a macrotask boundary, so every `.then` already queued has run. */ +async function settleReads() { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); +} + +/** Wait for the form, press "Create workflow", and return what was saved. */ +async function saveTheWorkflow(user: ReturnType) { + await waitFor( + () => { + expect(screen.getByTestId('create-workflow-button')).toBeEnabled(); + }, + { timeout: 2000 } + ); + await user.click(screen.getByTestId('create-workflow-button')); + await waitFor(() => { + expect(mockSaveWorkflow).toHaveBeenCalled(); + }); + return mockSaveWorkflow.mock.calls[0]?.[0]; +} + describe('CreateWorkflowFromSessionModal', () => { const defaultProps = { isOpen: true, @@ -627,27 +659,6 @@ describe('CreateWorkflowFromSessionModal', () => { warn.mockRestore(); }); - /** Cross a macrotask boundary, so every `.then` already queued has run. */ - async function settleReads() { - await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 0)); - }); - } - - async function saveTheWorkflow(user: ReturnType) { - await waitFor( - () => { - expect(screen.getByTestId('create-workflow-button')).toBeEnabled(); - }, - { timeout: 2000 } - ); - await user.click(screen.getByTestId('create-workflow-button')); - await waitFor(() => { - expect(mockSaveWorkflow).toHaveBeenCalled(); - }); - return mockSaveWorkflow.mock.calls[0]?.[0]; - } - it('captures no knowledge bases rather than every base', async () => { const user = userEvent.setup(); render(); @@ -723,4 +734,210 @@ describe('CreateWorkflowFromSessionModal', () => { }); }); }); + + /** + * A chat with no primary knowledge base gives a workflow with no primary. + * + * The modal filled the gap with the first visible base, on the read path and + * the generation path alike, and `apply_knowledge_selection` + * (`crates/biorouter/src/workflow/runtime.rs`) turns a saved `default` into + * `PrimaryUpdate::Set`. So every chat the workflow started got a write target + * the chat it was captured from never had. The daemon's rule is the opposite + * (`plan_knowledge_selection`): the primary comes only from `default`, and is + * never inferred from `visible`. + */ + describe("the workflow's primary knowledge base", () => { + const kb = (id: string) => ({ id, name: id, color: '#cf6d47', created_at: '' }); + /** What the daemon answers for this chat: two bases, and neither is primary. */ + const NO_PRIMARY = { + kb_ids: ['lab-notes', 'soul'], + primary_kb: null, + active_kb: null, + hidden_kbs: ['grant-drafts'], + }; + const generation = (knowledgeBases?: WorkflowKnowledgeBases) => ({ + data: { + workflow: { + title: 'Analyzed Workflow Title', + description: 'Analyzed description', + instructions: 'Analyzed instructions', + ...(knowledgeBases ? { knowledge_bases: knowledgeBases } : {}), + }, + error: undefined, + }, + error: undefined, + request: new globalThis.Request('http://localhost/test'), + response: new globalThis.Response(), + }); + /** Leave the generation's block as the only statement of the chat's selection. */ + const failTheSelectionRead = () => + mockGetActive.mockResolvedValue({ data: undefined, error: 'Failed to fetch' } as never); + let warn: MockInstance; + + beforeAll(() => { + // The picker is a Radix popover, and floating-ui measures it. + vi.stubGlobal( + 'ResizeObserver', + class { + observe() {} + unobserve() {} + disconnect() {} + } + ); + }); + + afterAll(() => { + vi.unstubAllGlobals(); + }); + + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + mockListBases.mockResolvedValue({ + data: [kb('soul'), kb('lab-notes'), kb('grant-drafts')], + error: undefined, + } as never); + mockGetActive.mockResolvedValue({ data: NO_PRIMARY, error: undefined } as never); + // No block of its own, so the chat's selection as the modal read it is the + // only place the saved workflow can take its bases from. + mockCreateWorkflow.mockResolvedValue(generation()); + }); + + afterEach(() => { + warn.mockRestore(); + mockListBases.mockResolvedValue({ data: [], error: undefined } as never); + mockGetActive.mockResolvedValue({ + data: { active_kb: null, hidden_kbs: [] }, + error: undefined, + } as never); + }); + + it('saves no primary when the chat has none', async () => { + const user = userEvent.setup(); + render(); + + const saved = await saveTheWorkflow(user); + + expect(mockGetActive).toHaveBeenCalledWith( + expect.objectContaining({ query: { session_id: defaultProps.sessionId } }) + ); + expect(saved?.knowledge_bases).toEqual({ default: null, visible: ['soul', 'lab-notes'] }); + }); + + // The daemon never sends `default: null` itself: the field is + // `skip_serializing_if = "Option::is_none"`, so its own block for a chat + // with no primary has no `default` at all. The two mean the same thing. + it.each<[string, WorkflowKnowledgeBases]>([ + ['null', { default: null, visible: ['lab-notes', 'soul'] }], + ['absent', { visible: ['lab-notes', 'soul'] }], + ])('saves no primary when the generated block has none (default %s)', async (_, block) => { + const user = userEvent.setup(); + failTheSelectionRead(); + mockCreateWorkflow.mockResolvedValue(generation(block)); + render(); + + const saved = await saveTheWorkflow(user); + + expect(saved?.knowledge_bases).toEqual({ default: null, visible: ['lab-notes', 'soul'] }); + }); + + /** + * A primary that is not among the bases the workflow will see is dropped, + * not unioned into them the way `plan_knowledge_selection` unions a + * `default` missing from `visible`. The daemon does that for a workflow + * somebody wrote, whose author plainly meant that `default`. Nobody wrote + * this one: a captured primary outside its own set is an inconsistent read. + */ + describe('a primary outside the visible bases', () => { + // The modal reads the base list and the selection as two requests, which + // can answer in either order, so a base created or deleted between them + // leaves the selection naming a primary the list does not hold. Unioning it + // in could save a base that no longer exists as the default, and + // `set_visible_kbs` refuses a primary outside the set, so every chat the + // workflow starts would then fail. + it("is not saved from the chat's selection", async () => { + const user = userEvent.setup(); + mockGetActive.mockResolvedValue({ + data: { + kb_ids: ['lab-notes', 'new-notes', 'soul'], + primary_kb: 'new-notes', + active_kb: 'new-notes', + hidden_kbs: ['grant-drafts'], + }, + error: undefined, + } as never); + render(); + + const saved = await saveTheWorkflow(user); + + expect(saved?.knowledge_bases).toEqual({ default: null, visible: ['soul', 'lab-notes'] }); + }); + + // The daemon's block is one locked snapshot whose primary is always a + // member of its set (`selection_unlocked`), so this guards against a block + // that breaks that; today's daemon never sends one. + it('is not saved from the generated block', async () => { + const user = userEvent.setup(); + failTheSelectionRead(); + mockCreateWorkflow.mockResolvedValue( + generation({ default: 'grant-drafts', visible: ['lab-notes', 'soul'] }) + ); + render(); + + const saved = await saveTheWorkflow(user); + + expect(saved?.knowledge_bases).toEqual({ default: null, visible: ['lab-notes', 'soul'] }); + }); + }); + + /** + * Only the picker's Default control names a primary. Switching a base on + * says the workflow may search it, not that KB-less writes go there, and + * switching one off must not hand the role to whichever base is left. + */ + describe('when the user edits the knowledge bases', () => { + async function openThePicker(user: ReturnType) { + await user.click(await screen.findByText('2 KBs selected')); + } + + it('switching a base on does not make it the primary', async () => { + const user = userEvent.setup(); + render(); + + await openThePicker(user); + await user.click(screen.getByRole('switch', { name: 'Toggle grant-drafts' })); + + const saved = await saveTheWorkflow(user); + expect(saved?.knowledge_bases).toEqual({ + default: null, + visible: ['soul', 'lab-notes', 'grant-drafts'], + }); + }); + + it('switching a base off does not make another the primary', async () => { + const user = userEvent.setup(); + render(); + + await openThePicker(user); + await user.click(screen.getByRole('switch', { name: 'Toggle lab-notes' })); + + const saved = await saveTheWorkflow(user); + expect(saved?.knowledge_bases).toEqual({ default: null, visible: ['soul'] }); + }); + + it('switching the primary off leaves no primary', async () => { + const user = userEvent.setup(); + mockGetActive.mockResolvedValue({ + data: { ...NO_PRIMARY, primary_kb: 'lab-notes', active_kb: 'lab-notes' }, + error: undefined, + } as never); + render(); + + await openThePicker(user); + await user.click(screen.getByRole('switch', { name: 'Toggle lab-notes' })); + + const saved = await saveTheWorkflow(user); + expect(saved?.knowledge_bases).toEqual({ default: null, visible: ['soul'] }); + }); + }); + }); }); diff --git a/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx b/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx index 8b9ab5b87..a6e609aca 100644 --- a/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx +++ b/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx @@ -67,21 +67,22 @@ export function WorkflowResourcePicker({ }); }, [items, query, selected]); + // Switching an item on or off changes the selection, never the default: only + // the Default control names one. For knowledge bases the default becomes the + // primary of every chat the workflow starts, which is where KB-less writes + // go, and the daemon never infers that pointer (`plan_knowledge_selection`). + // So a base switched on is not made the default, and switching the default + // off leaves none rather than passing the role to the first base left. const toggleSelected = (id: string) => { if (selected.has(id)) { - const next = selectedIds.filter((selectedId) => selectedId !== id); - onSelectedIdsChange(next); + onSelectedIdsChange(selectedIds.filter((selectedId) => selectedId !== id)); if (defaultId === id) { - onDefaultIdChange?.(next[0] ?? null); + onDefaultIdChange?.(null); } return; } - const next = [...selectedIds, id]; - onSelectedIdsChange(next); - if (!defaultId) { - onDefaultIdChange?.(id); - } + onSelectedIdsChange([...selectedIds, id]); }; return ( @@ -161,8 +162,12 @@ export function WorkflowResourcePicker({ )} {onDefaultIdChange && isSelected && ( + // A toggle, so "selected, and no default" stays reachable + // once a default has been named.