Desktop: a workflow never invents a primary knowledge base (capture, picker)

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) => {

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<typeof userEvent.setup>) {
+  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<typeof userEvent.setup>) {
-        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(<CreateWorkflowFromSessionModal {...defaultProps} sessionId={PRIVATE_CHAT} />);
@@ -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(<CreateWorkflowFromSessionModal {...defaultProps} />);
+
+      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(<CreateWorkflowFromSessionModal {...defaultProps} />);
+
+      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(<CreateWorkflowFromSessionModal {...defaultProps} />);
+
+        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(<CreateWorkflowFromSessionModal {...defaultProps} />);
+
+        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<typeof userEvent.setup>) {
+        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(<CreateWorkflowFromSessionModal {...defaultProps} />);
+
+        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(<CreateWorkflowFromSessionModal {...defaultProps} />);
+
+        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(<CreateWorkflowFromSessionModal {...defaultProps} />);
+
+        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'] });
+      });
+    });
+  });
 });