From 970e73071a042d08c643003e2e0b0863395e1005 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 14:06:44 -0700 Subject: [PATCH 1/2] fix(desktop): a workflow captured from a chat with no primary has no primary The create-workflow modal computes the captured default in two places: from the chat's own selection read, and from the generated workflow's `knowledge_bases` block. Both fell back to `visible[0]` when the chat named no primary, so the saved workflow's `default` was the first visible base. `apply_knowledge_selection` maps `default: Some(id)` to `PrimaryUpdate::Set(id)`, so every chat the workflow started got a primary, the target of KB-less writes, that the chat it was captured from never had. The daemon's rule is the opposite. `plan_knowledge_selection` never infers the primary from `visible`, and the doc on `plan_workflow_knowledge_selection` says why: a promoted primary turns "I did not say where to write" into a commit into someone's base. With no primary, a KB-less write fails and names the candidates. Both paths now go through `primaryAmong`: the named primary if it is among the bases the workflow will see, and otherwise `null`. The daemon's block for a chat with no primary omits `default` entirely (`skip_serializing_if`), and that reads as `null` too. A primary outside `visible` becomes `null`, not unioned in the way the daemon unions an author's `default`. The daemon does that because somebody wrote the workflow and meant it. A captured primary outside its own set is an inconsistent read. The block is one locked snapshot and never is one, but the modal's own read is two requests (the base list and the selection), and a base created or deleted between their answers leaves the selection naming a base the list lacks. Unioning could save a deleted base as the default, which `set_visible_kbs` refuses, so every chat the workflow starts would fail with a 400. After a failed list it would save the primary as the only visible base, hiding every other one. Tests: the read path with no generated block; the generated block with `default: null` and with `default` absent (the daemon's own wire shape); and a primary outside the visible bases, once from the read and once from the block. All five fail before this change: each saved the first visible base. --- .../CreateWorkflowFromSessionModal.tsx | 51 +++-- .../CreateWorkflowFromSessionModal.test.tsx | 184 +++++++++++++++--- 2 files changed, 196 insertions(+), 39 deletions(-) diff --git a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx index d7b4361a7..c11c4f4a2 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) { diff --git a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx index d06994d93..2ffc1f05d 100644 --- a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx +++ b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx @@ -3,7 +3,7 @@ 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 +85,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 +649,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 +724,143 @@ 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; + + 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'] }); + }); + }); + }); }); From 7c76088ed58fb386713e2cd47253d2c1ae1a8c17 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 14:07:54 -0700 Subject: [PATCH 2/2] fix(desktop): only the Default control names a workflow's primary knowledge base The previous commit stops the create-workflow modal from inventing a primary when it captures a chat's selection. Editing the selection afterwards still invented one, in two places: - The picker (`WorkflowResourcePicker`). Switching a base on made it the default whenever none was set, and switching the default off handed the role to `next[0]`. - The modal's `onKnowledgeBaseIdsChange`. It promoted `ids[0]` on any change while no default was set, including switching off an unrelated base. So a user who captured a chat with no primary and then removed one base got the first remaining base as the write target of every chat the workflow starts. These are user gestures, not captures, which is the case for keeping them. They go anyway. The gesture is "this workflow may search this base", and the promotion adds "and KB-less writes go here", which the user did not ask for. It is the promotion the daemon removed in 3a6e6888 ("never promote a sole visible base to the primary"): "exactly one candidate" was treated as consent, which it is not, and the author who wants that base as the write target has a field to say so. In the picker, that field is the Default control, one click away on every selected row. - Switching a base on changes the selection only. - Switching the default off leaves no default, rather than passing it on. - The modal's handler keeps only the membership rule: a named default stays while its base is selected and becomes `null` when it is not. - The Default control is now a toggle (`aria-pressed`), so pressing the current default clears it. Without that, "these bases, and no default", the state a chat with no primary now captures, could not be restored once any base had been made the default, short of switching it off and on again. Each control is named for its row ("Default KB: lab-notes") so the pressed state says which base it belongs to. Tests, in the modal: switching a base on, switching another off, and switching the primary off each save no primary. All three fail on the previous commit. Switching a base on still fails with only the handler fixed (the picker names the base) or only the picker fixed (the handler names `ids[0]`). In the picker: switching on leaves the default unset, switching the default off clears it, and the Default control names a base and clears it when pressed again. All three fail before this change. A fourth, switching another base off leaves the default alone, passes either way as a guard against clearing it on every toggle. --- .../CreateWorkflowFromSessionModal.tsx | 10 +- .../CreateWorkflowFromSessionModal.test.tsx | 79 +++++++++++- .../shared/WorkflowResourcePicker.tsx | 23 ++-- .../__tests__/WorkflowResourcePicker.test.tsx | 122 ++++++++++++++++++ 4 files changed, 219 insertions(+), 15 deletions(-) create mode 100644 ui/desktop/src/components/workflows/shared/__tests__/WorkflowResourcePicker.test.tsx diff --git a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx index c11c4f4a2..45c5f9b57 100644 --- a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx +++ b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx @@ -509,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 2ffc1f05d..4a8f963a3 100644 --- a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx +++ b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx @@ -1,4 +1,14 @@ -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'; @@ -764,6 +774,22 @@ describe('CreateWorkflowFromSessionModal', () => { 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({ @@ -862,5 +888,56 @@ describe('CreateWorkflowFromSessionModal', () => { 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.