From 8954de1c4617f6ef9650ae283131f6fac5b6679b Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 10:46:20 -0700 Subject: [PATCH 1/6] fix(desktop): prove the person on every read of a chat's knowledge selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /knowledge/active` naming a chat is on the reach gate's list (`session_reach.rs`), exactly like the POST beside it. The POST in `syncSelection` has always sent `userActionHeaders()`; the reads did not — the KnowledgeProvider hydrate and its recovery re-read, the `/` palette's knowledge-base rows, and the create-workflow modal. So the desktop's own daemon answered each of them as a public caller and refused every private chat, whatever model was bound. Measured on 2026-09-11 in a sandboxed dev instance on versa_azure (GPT-5.5): one turn ratcheted a new chat private, and its selection was set to primary `lab-notes` with `grant-drafts` hidden. - `GET /knowledge/active?session_id=…` without the proof: 403, "That chat is private, or there is no chat with that id. This request was made on a public model …" — for a chat bound to a private model. With `X-User-Action`: 200 and the chat's selection. - The console printed "Knowledge selection not hydrated: That chat is private, or there is no chat with that id." and the chip showed all three bases on: the renderer's cache, not the daemon's selection. - One click on another base in that stale chip wrote the stale set back; the daemon then had `grant-drafts` visible to the chat again. - The `/` palette offered `kb:Grant drafts` as "Knowledge base in this chat" and named no primary. With the proof on the reads, the same chat hydrates to the daemon's selection, the chip shows Grant drafts off, the same click leaves it hidden, and the palette offers Lab notes (as the primary) and Soul only. This reaches nothing the renderer could not already read: the selection is a strict subset of the transcript `getSession` reads with the same proof. And `userActionHeaders()` is the one place the surface is decided, so on a browser surface these reads state the host's model once SD-9's change to that helper lands (#229), with no further change here. The KnowledgeContext comment that called the refusal "a correct outcome" for "a private chat opened while a public model is bound" is corrected: the read was refused for every private chat, and what followed was not correct. Fail-before: all five new tests — the hydrate, the recovery re-read, the toggle that re-exposed a hidden base, the palette and the workflow capture — fail with the production change reverted. `src/test/reachGate.ts` is the one model of the gate they share. --- .../MentionPopover.privateChat.test.tsx | 129 ++++++++++++++++++ ui/desktop/src/components/MentionPopover.tsx | 16 ++- .../knowledge/KnowledgeContext.test.tsx | 90 +++++++++++- .../components/knowledge/KnowledgeContext.tsx | 29 +++- .../CreateWorkflowFromSessionModal.tsx | 8 +- .../CreateWorkflowFromSessionModal.test.tsx | 94 ++++++++++++- ui/desktop/src/test/reachGate.ts | 55 ++++++++ 7 files changed, 407 insertions(+), 14 deletions(-) create mode 100644 ui/desktop/src/components/MentionPopover.privateChat.test.tsx create mode 100644 ui/desktop/src/test/reachGate.ts diff --git a/ui/desktop/src/components/MentionPopover.privateChat.test.tsx b/ui/desktop/src/components/MentionPopover.privateChat.test.tsx new file mode 100644 index 000000000..a079dec8e --- /dev/null +++ b/ui/desktop/src/components/MentionPopover.privateChat.test.tsx @@ -0,0 +1,129 @@ +import { render, screen } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import MentionPopover from './MentionPopover'; +import { reachGatedGetActive, USER_ACTION_KEY } from '../test/reachGate'; + +const mocks = vi.hoisted(() => ({ + getActive: vi.fn(), + listBases: vi.fn(), + getSlashCommands: vi.fn(), + getSessionExtensions: vi.fn(), + // ONE array, as the real context's state is: a fresh `[]` per render + // recreates `loadReferenceItems` every render, and the palette reloads itself + // in a loop that detaches every row it has just drawn. + extensionsList: [] as never[], +})); + +vi.mock('../api', () => ({ + getActive: mocks.getActive, + listBases: mocks.listBases, + getSlashCommands: mocks.getSlashCommands, + getSessionExtensions: mocks.getSessionExtensions, +})); + +vi.mock('./ConfigContext', () => ({ + useConfig: () => ({ extensionsList: mocks.extensionsList }), +})); + +vi.mock('./skills/useSkillCatalog', () => ({ + fetchSkillCatalog: vi.fn(async () => ({ generation: 0, roots: [], skills: [], bundles: [] })), + pickerBundles: () => [], + standaloneSkills: () => [], +})); + +const PRIVATE_CHAT = 'chat-private'; + +function base(id: string, name: string) { + return { id, name, color: '#cf6d47', created_at: '', schema_version: 1, tier: 'public' }; +} + +function renderPalette() { + return render( + {}} + onSelect={() => {}} + onClose={() => {}} + /> + ); +} + +/** + * Issue #56 Task 58: the `/` palette's knowledge-base rows come from the chat's + * selection, and `GET /knowledge/active` naming a PRIVATE chat is on the + * daemon's reach gate. Measured in the running desktop app on 2026-09-11: the + * read went out with no proof, was refused, and the palette fell back to "no + * base is hidden, none is primary" — offering a base the chat had hidden as + * "Knowledge base in this chat", and naming no primary at all. + */ +describe('the / palette in a private chat', () => { + let savedElectron: unknown; + let savedScrollIntoView: PropertyDescriptor | undefined; + + beforeEach(() => { + vi.clearAllMocks(); + savedElectron = (window as { electron?: unknown }).electron; + Object.assign(window, { + electron: { getUserActionKey: vi.fn(async () => USER_ACTION_KEY) }, + }); + // jsdom has no `scrollIntoView`, and the palette scrolls its selected row + // into view on every render — an effect that throws unmounts the palette. + savedScrollIntoView = Object.getOwnPropertyDescriptor(Element.prototype, 'scrollIntoView'); + Object.defineProperty(Element.prototype, 'scrollIntoView', { + configurable: true, + writable: true, + value: vi.fn(), + }); + mocks.getSlashCommands.mockResolvedValue({ data: { commands: [] } }); + mocks.getSessionExtensions.mockResolvedValue({ data: { extensions: [] } }); + mocks.listBases.mockResolvedValue({ + data: [ + base('soul', 'Soul'), + base('lab-notes', 'Lab notes'), + base('grant-drafts', 'Grant drafts'), + ], + }); + mocks.getActive.mockImplementation( + reachGatedGetActive([PRIVATE_CHAT], (sessionId) => + sessionId + ? { + kb_ids: ['lab-notes', 'soul'], + primary_kb: 'lab-notes', + active_kb: 'lab-notes', + hidden_kbs: ['grant-drafts'], + } + : { kb_ids: ['grant-drafts', 'lab-notes', 'soul'], primary_kb: 'soul', hidden_kbs: [] } + ) + ); + }); + + afterEach(() => { + Object.assign(window, { electron: savedElectron }); + if (savedScrollIntoView) { + Object.defineProperty(Element.prototype, 'scrollIntoView', savedScrollIntoView); + } else { + delete (Element.prototype as { scrollIntoView?: unknown }).scrollIntoView; + } + }); + + it("offers this chat's knowledge bases, not every base, and names its primary", async () => { + renderPalette(); + + expect(await screen.findByText('kb:Lab notes')).toBeInTheDocument(); + expect(screen.getByText('Primary knowledge base · lab-notes')).toBeInTheDocument(); + expect(screen.getByText('kb:Soul')).toBeInTheDocument(); + expect(screen.queryByText('kb:Grant drafts')).not.toBeInTheDocument(); + expect(mocks.getActive).toHaveBeenCalledWith( + expect.objectContaining({ + query: { session_id: PRIVATE_CHAT }, + headers: { 'X-User-Action': USER_ACTION_KEY }, + }) + ); + }); +}); diff --git a/ui/desktop/src/components/MentionPopover.tsx b/ui/desktop/src/components/MentionPopover.tsx index b49b56b7f..21ecb6ffe 100644 --- a/ui/desktop/src/components/MentionPopover.tsx +++ b/ui/desktop/src/components/MentionPopover.tsx @@ -12,6 +12,7 @@ import { ItemIcon } from './ItemIcon'; import BuiltInBadge from './ui/BuiltInBadge'; import { CommandType, getActive, getSessionExtensions, getSlashCommands, listBases } from '../api'; import type { CatalogView } from '../api'; +import { userActionHeaders } from '../utils/userAction'; import { getInitialWorkingDir } from '../utils/workingDir'; import { IMAGE_EXTENSIONS } from '../utils/imageFormats'; import { labelledRefTag, refTag, type RefKind } from '../utils/resourceRefs'; @@ -549,10 +550,17 @@ const MentionPopover = forwardRef< ? getSlashCommands({ throwOnError: true }) : Promise.resolve({ data: { commands: [] } }), listBases({ throwOnError: false }), - getActive({ - query: sessionId ? { session_id: sessionId } : undefined, - throwOnError: false, - }), + // Issue #56 Task 58: a GET naming a PRIVATE chat needs the user's + // proof, as `setActive` sends it. Refused, the rows below fell back + // to "nothing hidden, nothing primary" and offered every base as + // one this chat has. + userActionHeaders().then((headers) => + getActive({ + query: sessionId ? { session_id: sessionId } : undefined, + headers, + throwOnError: false, + }) + ), // The daemon's catalog, not a renderer scan: a skill bundled inside // an installed extension was loadable by the model and absent from // this list, so `@skill:word` completed to nothing (#113). diff --git a/ui/desktop/src/components/knowledge/KnowledgeContext.test.tsx b/ui/desktop/src/components/knowledge/KnowledgeContext.test.tsx index 7221f8e20..760d9fadd 100644 --- a/ui/desktop/src/components/knowledge/KnowledgeContext.test.tsx +++ b/ui/desktop/src/components/knowledge/KnowledgeContext.test.tsx @@ -1,7 +1,8 @@ import { act, render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { KnowledgeProvider, useKnowledge } from './KnowledgeContext'; +import { reachGatedGetActive, USER_ACTION_KEY } from '../../test/reachGate'; /** A promise the test resolves by hand, so "after the response settled" is a fact, not a race. */ function deferred() { @@ -583,4 +584,91 @@ describe('KnowledgeContext', () => { expect(mocks.setActive).not.toHaveBeenCalled(); }); }); + + // Issue #56 Task 58. `GET /knowledge/active` naming a PRIVATE chat is on the + // reach gate's list, and the desktop app gets through it the way `setActive` + // already does: with the user's proof. The reads carried none, so the daemon + // refused every private chat's selection — whatever model was bound — and the + // chip fell back to whatever this renderer had cached, which the daemon may + // have moved past (the agent's `kb_set_active`, the CLI, another window). + describe('a private chat', () => { + let savedElectron: unknown; + + beforeEach(() => { + savedElectron = (window as { electron?: unknown }).electron; + Object.assign(window, { + electron: { getUserActionKey: vi.fn(async () => USER_ACTION_KEY) }, + }); + mocks.getActive.mockImplementation( + reachGatedGetActive(['chat-1'], (sessionId) => + sessionId ? daemon.session : daemon.machine + ) + ); + }); + + afterEach(() => { + Object.assign(window, { electron: savedElectron }); + }); + + it('hydrates its selection from the daemon, not from what this renderer cached', async () => { + localStorage.setItem('knowledge_active_kb:chat-1', 'beta'); + localStorage.setItem('knowledge_hidden_kbs:chat-1', '[]'); + + renderProvider(); + + await waitFor(() => expect(screen.getByTestId('primary').textContent).toBe('alpha')); + expect(screen.getByTestId('hidden').textContent).toBe('beta'); + expect(mocks.getActive).toHaveBeenCalledWith( + expect.objectContaining({ + query: { session_id: 'chat-1' }, + headers: { 'X-User-Action': USER_ACTION_KEY }, + }) + ); + }); + + // Seeded to match the daemon, so the first read cannot be what puts the + // selection back: only the recovery read can. + it('re-reads its selection with the proof when a write does not land', async () => { + localStorage.setItem('knowledge_active_kb:chat-1', 'alpha'); + localStorage.setItem('knowledge_hidden_kbs:chat-1', '["beta"]'); + const pending = deferred(); + renderProvider(); + await waitFor(() => expect(mocks.getActive).toHaveBeenCalledTimes(1)); + await settle(() => {}); + + mocks.setActive.mockReturnValue(pending.promise); + await userEvent.click(screen.getByRole('button', { name: 'make beta primary' })); + expect(screen.getByTestId('primary').textContent).toBe('beta'); + + await settle(() => pending.reject(new Error('network down'))); + + await waitFor(() => expect(mocks.getActive).toHaveBeenCalledTimes(2)); + expect(mocks.getActive.mock.calls[1]?.[0]).toMatchObject({ + query: { session_id: 'chat-1' }, + headers: { 'X-User-Action': USER_ACTION_KEY }, + }); + await waitFor(() => expect(screen.getByTestId('primary').textContent).toBe('alpha')); + expect(screen.getByTestId('hidden').textContent).toBe('beta'); + }); + + // What the refused read cost, measured in the running app on 2026-09-11: + // the chip showed a hidden base as switched on, and one click on another + // base wrote that stale set back — the hidden base was in the chat again. + // A set-only edit is only as right as the set it starts from. + it('toggles from the set the daemon holds, so a toggle cannot re-expose a hidden base', async () => { + localStorage.setItem('knowledge_hidden_kbs:chat-1', '[]'); + renderProvider(); + await waitFor(() => expect(mocks.getActive).toHaveBeenCalled()); + // Let the hydrate's answer land, whichever way the daemon answered it. + await act(async () => { + await Promise.allSettled(mocks.getActive.mock.results.map((result) => result.value)); + }); + + await userEvent.click(screen.getByRole('button', { name: 'toggle alpha' })); + + await waitFor(() => expect(mocks.setActive).toHaveBeenCalled()); + const calls = mocks.setActive.mock.calls; + expect(calls[calls.length - 1]?.[0]?.body.hidden_kbs).toEqual(['alpha', 'beta']); + }); + }); }); diff --git a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx index 34b513075..03333859f 100644 --- a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx +++ b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx @@ -186,6 +186,8 @@ export function KnowledgeProvider({ try { const res = await getActive({ query: sessionId ? { session_id: sessionId } : undefined, + // The same proof the hydrate below sends, for the same reason. + headers: await userActionHeaders(), throwOnError: true, }); if (generation !== selectionGenerationRef.current) return; @@ -455,6 +457,12 @@ export function KnowledgeProvider({ try { const res = await getActive({ query: sessionId ? { session_id: sessionId } : undefined, + // Issue #56 Task 58: a GET naming a PRIVATE chat is on the reach + // gate's list exactly as the POST in `syncSelection` is, and the + // desktop gets through it the same way — by proving the person. This + // selection is a strict subset of what `getSession` already reads + // with that proof. + headers: await userActionHeaders(), throwOnError: true, }); if (cancelled || generation !== selectionGenerationRef.current) return; @@ -462,12 +470,21 @@ export function KnowledgeProvider({ applyHidden(readHidden(res.data) ?? []); } catch (err) { if (cancelled) return; - // ⚠ One quiet line, deliberately. A private chat opened while a - // public model is bound refuses this read with a ~900-character - // paragraph addressed to an AI agent, and printing it here made a - // correct outcome — the chat opened and rendered in full — look like a - // crash. The person's answer is the composer's pinned-model note; see - // `selectionWarning.ts` for the full reasoning. + // ⚠ One quiet line, deliberately: the gate's refusal is ~900 + // characters addressed to an AI agent, and its first sentence is all a + // person reading the console needs (`selectionWarning.ts`). + // + // ⚠ Until 2026-09-11 this comment called that refusal "a correct + // outcome" for "a private chat opened while a public model is bound". + // Neither half held. This read carried no proof, so the daemon refused + // it for EVERY private chat, whatever model was bound — measured with a + // chat on its own private model — and what followed was not correct: + // the chip showed this renderer's cache instead of the daemon's + // selection, and the next toggle wrote that stale set back over it. A + // refusal still lands here for a caller that genuinely has neither the + // proof nor a private model (a daemon started without a user-action + // key, a browser tab not running a private model), and only there is + // it the right answer. console.warn('Knowledge selection not hydrated:', briefSelectionFailure(err)); setPrimaryKbIdState(local); setHiddenKbIdsState(localHidden); diff --git a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx index b17c85653..b79582b70 100644 --- a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx +++ b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx @@ -6,6 +6,7 @@ import { Button } from '../ui/button'; import { WorkflowFormFields } from './shared/WorkflowFormFields'; import { WorkflowFormData } from './shared/workflowFormSchema'; import { createWorkflow, getActive, getSessionExtensions, listBases } from '../../api/sdk.gen'; +import { userActionHeaders } from '../../utils/userAction'; import { WorkflowParameter } from './shared/workflowFormSchema'; import { toastError } from '../../toasts'; import { saveWorkflow } from '../../workflow/workflow_management'; @@ -107,7 +108,12 @@ export default function CreateWorkflowFromSessionModal({ Promise.all([ listBases({ throwOnError: false }), - getActive({ query: { session_id: sessionId }, throwOnError: false }), + // Issue #56 Task 58: this modal opens from the chat it names, and a GET + // naming a PRIVATE chat needs the user's proof. Refused, the capture + // took every base, with whichever came first as its default. + userActionHeaders().then((headers) => + getActive({ query: { session_id: sessionId }, headers, throwOnError: false }) + ), ]).then(([basesRes, activeRes]) => { if (cancelled) return; const bases: Manifest[] = basesRes.data ?? []; diff --git a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx index 2a4cfa209..aa23765f1 100644 --- a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx +++ b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx @@ -1,10 +1,11 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import CreateWorkflowFromSessionModal from '../CreateWorkflowFromSessionModal'; -import { createWorkflow, skillCatalogHandler } from '../../../api/sdk.gen'; +import { createWorkflow, getActive, listBases, skillCatalogHandler } from '../../../api/sdk.gen'; import type { CreateWorkflowResponse } from '../../../api/types.gen'; import { saveWorkflow } from '../../../workflow/workflow_management'; +import { reachGatedGetActive, USER_ACTION_KEY } from '../../../test/reachGate'; vi.mock('../../../api/sdk.gen', () => ({ createWorkflow: vi.fn(), @@ -70,6 +71,8 @@ vi.mock('../../../workflow/workflow_management', () => ({ })); const mockCreateWorkflow = vi.mocked(createWorkflow); +const mockGetActive = vi.mocked(getActive); +const mockListBases = vi.mocked(listBases); const mockSkillCatalog = vi.mocked(skillCatalogHandler); const mockSaveWorkflow = vi.mocked(saveWorkflow); @@ -483,4 +486,91 @@ describe('CreateWorkflowFromSessionModal', () => { expect(createButton).toBeDisabled(); }); }); + + /** + * Issue #56 Task 58: the modal reads the chat's knowledge-base selection with + * `GET /knowledge/active`, and naming a PRIVATE chat there is on the daemon's + * reach gate. The read carried no proof, was refused, and the modal fell back + * to "nothing is hidden, nothing is primary" — so a workflow captured from a + * private chat took EVERY base, with whichever came first as its default. + */ + describe('from a private chat', () => { + const PRIVATE_CHAT = 'private-session-id'; + const kb = (id: string) => ({ id, name: id, color: '#cf6d47', created_at: '' }); + + beforeEach(() => { + Object.assign(window, { + electron: { + listSkillDirs: vi.fn().mockResolvedValue([]), + getUserActionKey: vi.fn(async () => USER_ACTION_KEY), + }, + }); + mockListBases.mockResolvedValue({ + data: [kb('soul'), kb('lab-notes'), kb('grant-drafts')], + error: undefined, + } as never); + mockGetActive.mockImplementation( + reachGatedGetActive([PRIVATE_CHAT], () => ({ + kb_ids: ['lab-notes', 'soul'], + primary_kb: 'lab-notes', + active_kb: 'lab-notes', + hidden_kbs: ['grant-drafts'], + })) as never + ); + // A generation with no knowledge-base block of its own, so the chat's + // selection is the only place the saved workflow can take its bases from. + mockCreateWorkflow.mockResolvedValue({ + data: { + workflow: { + title: 'Analyzed Workflow Title', + description: 'Analyzed description', + instructions: 'Analyzed instructions', + }, + error: undefined, + }, + error: undefined, + request: new globalThis.Request('http://localhost/test'), + response: new globalThis.Response(), + }); + }); + + afterEach(() => { + // `clearAllMocks` keeps implementations, so put back what the rest of the + // file expects rather than leak this chat's gate into it. + mockListBases.mockResolvedValue({ data: [], error: undefined } as never); + mockGetActive.mockResolvedValue({ + data: { active_kb: null, hidden_kbs: [] }, + error: undefined, + } as never); + }); + + it("captures the chat's knowledge bases and its primary, not every base", async () => { + const user = userEvent.setup(); + render(); + + await waitFor( + () => { + expect(screen.getByTestId('create-workflow-button')).toBeEnabled(); + }, + { timeout: 2000 } + ); + await user.click(screen.getByTestId('create-workflow-button')); + await waitFor(() => { + expect(mockSaveWorkflow).toHaveBeenCalled(); + }); + + expect(mockSaveWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ + knowledge_bases: { default: 'lab-notes', visible: ['soul', 'lab-notes'] }, + }), + null + ); + expect(mockGetActive).toHaveBeenCalledWith( + expect.objectContaining({ + query: { session_id: PRIVATE_CHAT }, + headers: { 'X-User-Action': USER_ACTION_KEY }, + }) + ); + }); + }); }); diff --git a/ui/desktop/src/test/reachGate.ts b/ui/desktop/src/test/reachGate.ts new file mode 100644 index 000000000..c4f7a74c4 --- /dev/null +++ b/ui/desktop/src/test/reachGate.ts @@ -0,0 +1,55 @@ +/** + * The daemon's session reach gate, as a renderer test meets it on + * `GET /knowledge/active` — one model of it, so the tests that need it cannot + * drift into three different ideas of what the daemon does. + * + * The real gate is `refuse_unless_reachable` in + * `crates/biorouter-server/src/routes/session_reach.rs`: a request naming a + * PRIVATE chat is answered only for a caller whose stated capability covers it + * or that carries the user's proof. The desktop app never states a capability + * — it proves the person, with the `X-User-Action` header `userActionHeaders()` + * builds from the preload bridge — so on the desktop the proof is the only way + * through, and that is all this models. A chat that is not private, and a + * request naming no chat at all, pass untouched: the gate is inert there. + * + * A refusal arrives the way the generated client delivers this route's + * `text/plain` 403 (`api/client/client.gen.ts`): THROWN as a bare string under + * `throwOnError: true`, and returned as `{ error }` with no `data` otherwise. + */ + +/** What the stubbed preload bridge hands `userActionHeaders()`. */ +export const USER_ACTION_KEY = 'renderer-test-user-action-key'; + +/** + * The first sentence of `SESSION_OUT_OF_REACH` in `session_reach.rs`. The rest + * of that paragraph is addressed to a model and never matters to a renderer + * assertion; this sentence is the one the console line keeps. + */ +export const SESSION_OUT_OF_REACH = 'That chat is private, or there is no chat with that id.'; + +type GetActiveOptions = { + query?: { session_id?: string }; + headers?: Record; + throwOnError?: boolean; +}; + +/** + * A `getActive` implementation that refuses a private chat to a request without + * the user's proof, and otherwise answers with `answer(sessionId)`. + */ +export function reachGatedGetActive( + privateSessionIds: readonly string[], + answer: (sessionId: string | undefined) => unknown +) { + return (options?: GetActiveOptions) => { + const sessionId = options?.query?.session_id; + const namesPrivateChat = sessionId !== undefined && privateSessionIds.includes(sessionId); + const proven = options?.headers?.['X-User-Action'] === USER_ACTION_KEY; + if (namesPrivateChat && !proven) { + return options?.throwOnError + ? Promise.reject(SESSION_OUT_OF_REACH) + : Promise.resolve({ data: undefined, error: SESSION_OUT_OF_REACH }); + } + return Promise.resolve({ data: answer(sessionId) }); + }; +} From 4befa3bc50c1527d8841f208447e5fef24935a2e Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 10:46:26 -0700 Subject: [PATCH 2/6] docs(desktop): the knowledge-selection refusal was never about the bound model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `selectionWarning.ts` and its test carried the premise the previous commit corrected in KnowledgeContext: that the desktop app was refused `GET /knowledge/active` for a private chat "while a public model is bound", as "a normal, correct outcome". It was refused for every private chat, because its read carried no proof. What the module does is still right — the refusal is prose addressed to a model, and a console's reader is a person — so only the reasoning changes. The pointer to the composer's pinned-model note goes with it: that note answers why a chosen model is not in effect, which this refusal never was about. --- .../knowledge/selectionWarning.test.ts | 13 ++++--- .../components/knowledge/selectionWarning.ts | 34 ++++++++++--------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/ui/desktop/src/components/knowledge/selectionWarning.test.ts b/ui/desktop/src/components/knowledge/selectionWarning.test.ts index 8c33a0422..ebf1b96d5 100644 --- a/ui/desktop/src/components/knowledge/selectionWarning.test.ts +++ b/ui/desktop/src/components/knowledge/selectionWarning.test.ts @@ -2,11 +2,14 @@ import { describe, expect, it } from 'vitest'; import { briefSelectionFailure } from './selectionWarning'; /** - * F14. Opening a private chat while a public model is bound refuses - * `GET /knowledge/active` with a paragraph addressed to an AI agent. The - * refusal is correct and stays exactly as it is; what was wrong is that it was - * printed in full in the devtools console of a desktop app, where the chat had - * opened and rendered correctly and the only reader is a person. + * F14. A request naming a private chat on `GET /knowledge/active` with neither + * the user's proof nor a private model is refused with a paragraph addressed to + * an AI agent. The refusal stays exactly as it is; what was wrong is that it was + * printed in full in a devtools console, whose only reader is a person. + * + * (F14 took the desktop app's refusal for a correct one, on a chat bound to a + * public model. It was not: the desktop's read carried no proof, and was refused + * for every private chat — see `KnowledgeContext.tsx`.) */ const AGENT_REFUSAL = 'That chat is private, or there is no chat with that id. This request was made on a public ' + diff --git a/ui/desktop/src/components/knowledge/selectionWarning.ts b/ui/desktop/src/components/knowledge/selectionWarning.ts index cfe619d52..f5a056c8a 100644 --- a/ui/desktop/src/components/knowledge/selectionWarning.ts +++ b/ui/desktop/src/components/knowledge/selectionWarning.ts @@ -10,23 +10,25 @@ const MAX_REASON_CHARS = 160; * A failed knowledge-selection read, reduced to one line for the console. * * ⚠ **The paragraph this trims is deliberate, correct, and written for somebody - * else.** Opening a private chat while a public model is bound refuses - * `GET /knowledge/active` with ~900 characters addressed to an AI AGENT — "Do - * not retry as you are… If this task genuinely needs that chat, stop and ask - * the user to open it for you." That text is privacy-critical, pinned by - * repo-grep tests in `crates/biorouter-server/src/routes/session_reach.rs`, and - * nothing here changes it or should. + * else.** A request naming a private chat on `GET /knowledge/active` with + * neither the user's proof nor a private model behind it is refused with ~900 + * characters addressed to an AI AGENT — "Do not retry as you are… If this task + * genuinely needs that chat, stop and ask the user to open it for you." That + * text is privacy-critical, pinned by repo-grep tests in + * `crates/biorouter-server/src/routes/session_reach.rs`, and nothing here + * changes it or should. In a console its only reader is a person, who has no + * task, is not an agent and cannot open the chat "for" anybody, so the console + * keeps a one-line record: the read did not settle, and which one. * - * What was wrong is where it landed: in the devtools console of a **desktop** - * app, where the chat had opened correctly and rendered in full, and where the - * only reader is a person who has no task, is not an agent, and cannot open the - * chat "for" anybody. A normal, correct outcome printed as a wall of red-flag - * prose reads as a crash. - * - * So the console keeps a one-line record — the read did not settle, and which - * one — and the user-facing answer lives where a person will actually see it: - * the composer's own note about the model this chat is pinned to - * (`privacy/PinnedModelNote`). + * ⚠ **Until 2026-09-11 this said the refusal was "a normal, correct outcome" + * for a private chat opened in the desktop app while a public model was bound. + * It was neither.** The desktop's reads carried no proof, so the daemon refused + * every private chat whatever model was bound (measured with a chat on its own + * private model), and the renderer went on to show — and write back — the + * selection it had cached. The reads carry `userActionHeaders()` now, as the + * write always did (`KnowledgeContext.tsx`). The refusal still reaches a + * console from a caller that genuinely has neither: a daemon started without a + * user-action key, or a browser tab not running a private model. * * Trimming to the first sentence rather than to a fixed prefix is what keeps * this useful for the failures that are NOT the refusal: "Failed to fetch" and From 15b8a901643e65fc076bfb3dcf2213747ab904d2 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 10:51:08 -0700 Subject: [PATCH 3/6] docs(desktop): say what the proof already reaches, not that the selection is part of it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hydrate's comment called a chat's knowledge-base selection "a strict subset of what `getSession` already reads". It is not part of that response at all — the selection lives beside the session store, not in the transcript. The point the sentence was making is about reach: the same proof already reads the whole transcript, which discloses far more than which bases the chat uses. --- ui/desktop/src/components/knowledge/KnowledgeContext.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx index 03333859f..3a615dcbb 100644 --- a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx +++ b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx @@ -459,9 +459,10 @@ export function KnowledgeProvider({ query: sessionId ? { session_id: sessionId } : undefined, // Issue #56 Task 58: a GET naming a PRIVATE chat is on the reach // gate's list exactly as the POST in `syncSelection` is, and the - // desktop gets through it the same way — by proving the person. This - // selection is a strict subset of what `getSession` already reads - // with that proof. + // desktop gets through it the same way — by proving the person. It + // reaches nothing new: the same proof already reads the chat's whole + // transcript through `getSession`, which says far more than which + // knowledge bases the chat uses. headers: await userActionHeaders(), throwOnError: true, }); From e2a2738435fbb43867cf516ec72dc11ccf0893de Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:38:26 -0700 Subject: [PATCH 4/6] refactor(desktop): read a knowledge-selection answer in one place `readPrimary` and `readHidden` move out of `KnowledgeContext` into a module of their own, `knowledge/knowledgeSelection.ts`, unchanged. The `/` palette and the create-workflow modal each re-spelled the same two reads inline (`primary_kb ?? active_kb`, `hidden_kbs ?? []`), and the next commits give them a shared reader, which needs these two in a place all three callers can import. No behavior change. --- .../components/knowledge/KnowledgeContext.tsx | 21 +------------------ .../knowledge/knowledgeSelection.ts | 19 +++++++++++++++++ 2 files changed, 20 insertions(+), 20 deletions(-) create mode 100644 ui/desktop/src/components/knowledge/knowledgeSelection.ts diff --git a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx index 3a615dcbb..2201920b2 100644 --- a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx +++ b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx @@ -9,6 +9,7 @@ import { useState, } from 'react'; import { listBases, getActive, setActive } from '../../api'; +import { readHidden, readPrimary } from './knowledgeSelection'; import { briefSelectionFailure } from './selectionWarning'; import { userActionHeaders } from '../../utils/userAction'; /** @@ -52,26 +53,6 @@ type PrimaryUpdate = | { kind: 'inherit' } | { kind: 'set'; id: string }; -/** The shape both selection endpoints answer with — GET /active and POST /active. */ -type SelectionPayload = - | { primary_kb?: string | null; active_kb?: string | null; hidden_kbs?: string[] | null } - | undefined; - -/** `active_kb` is the deprecated mirror, read so a fresh renderer keeps working - * against a daemon that predates `primary_kb`. */ -function readPrimary(data: SelectionPayload): string | null { - return data?.primary_kb ?? data?.active_kb ?? null; -} - -/** `null` means "this answer did not state a set" (a daemon that predates the - * field) — distinct from an empty set, and the caller must leave what it has - * rather than erase the session's whole working set. */ -function readHidden(data: SelectionPayload): string[] | null { - return Array.isArray(data?.hidden_kbs) - ? data.hidden_kbs.filter((id): id is string => typeof id === 'string') - : null; -} - interface KnowledgeContextType { bases: KbListEntry[]; /** The session's knowledge bases — the one axis. Searchable, readable, usable. */ diff --git a/ui/desktop/src/components/knowledge/knowledgeSelection.ts b/ui/desktop/src/components/knowledge/knowledgeSelection.ts new file mode 100644 index 000000000..7a8c2717f --- /dev/null +++ b/ui/desktop/src/components/knowledge/knowledgeSelection.ts @@ -0,0 +1,19 @@ +/** The shape both selection endpoints answer with — GET /active and POST /active. */ +type SelectionPayload = + | { primary_kb?: string | null; active_kb?: string | null; hidden_kbs?: string[] | null } + | undefined; + +/** `active_kb` is the deprecated mirror, read so a fresh renderer keeps working + * against a daemon that predates `primary_kb`. */ +export function readPrimary(data: SelectionPayload): string | null { + return data?.primary_kb ?? data?.active_kb ?? null; +} + +/** `null` means "this answer did not state a set" (a daemon that predates the + * field) — distinct from an empty set, and the caller must leave what it has + * rather than erase the session's whole working set. */ +export function readHidden(data: SelectionPayload): string[] | null { + return Array.isArray(data?.hidden_kbs) + ? data.hidden_kbs.filter((id): id is string => typeof id === 'string') + : null; +} From 83d64fafc751522e2a7981c7f454fca101e81dc5 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:41:24 -0700 Subject: [PATCH 5/6] fix(desktop): the / palette makes no claim from a failed selection read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The palette's knowledge-base rows come from `GET /knowledge/active`, read with `throwOnError: false`, and a failed read left `data` undefined. The rows then read `hidden_kbs ?? []` and `primary_kb ?? active_kb ?? null`, so a failure was drawn as "nothing is hidden, nothing is primary": every base, the chat's hidden ones included, labelled "Knowledge base in this chat", and no primary. Now that the read carries the user's proof (#235), a failure is a genuine error: a surface that cannot prove the person, a dropped connection, an older daemon. None of those is a statement about the chat. `KnowledgeContext` already states the rule for `listBases`, and it holds here: a failed request is not an empty answer. `readKnowledgeSelection` is the one reader for a surface that shows or saves a chat's selection. It sends the proof, and resolves to `null` on any failure, rejection included, with one brief console line. It never rejects, which fixes a second failure: the palette loads its commands, skills and extensions in the same `Promise.all`, and a selection read that threw emptied all of them. With no selection, the palette still offers every base, because a reference names its base by id and an explicit id reaches a base whatever the chat's selection (`kb_id_or_primary` in the knowledge server). What goes is the claim. Each row reads "Knowledge base · ", with neither "in this chat" nor "Primary knowledge base". Tests: the refused read (the reach gate model with no proof), a transport failure, and a rejection. All three fail before this change; the rejection case failed with an empty palette. --- .../MentionPopover.privateChat.test.tsx | 59 ++++++++++++++++++- ui/desktop/src/components/MentionPopover.tsx | 42 +++++++------ .../knowledge/knowledgeSelection.ts | 56 ++++++++++++++++++ 3 files changed, 137 insertions(+), 20 deletions(-) diff --git a/ui/desktop/src/components/MentionPopover.privateChat.test.tsx b/ui/desktop/src/components/MentionPopover.privateChat.test.tsx index a079dec8e..cb76b4523 100644 --- a/ui/desktop/src/components/MentionPopover.privateChat.test.tsx +++ b/ui/desktop/src/components/MentionPopover.privateChat.test.tsx @@ -1,5 +1,5 @@ import { render, screen } from '@testing-library/react'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest'; import MentionPopover from './MentionPopover'; import { reachGatedGetActive, USER_ACTION_KEY } from '../test/reachGate'; @@ -126,4 +126,61 @@ describe('the / palette in a private chat', () => { }) ); }); + + /** + * With the proof attached, a read that still fails is a genuine error: a + * surface that cannot prove the person, a dropped connection, an older + * daemon. None of those said "no base is hidden, none is primary", and the + * palette used to render exactly that — every base as "Knowledge base in + * this chat", Grant drafts included, and no primary. + * + * It keeps offering every base, because a reference names its base by id and + * an explicit id reaches a base whatever the chat's selection + * (`kb_id_or_primary` in the knowledge server). What it drops is the claim. + */ + describe('when its selection cannot be read', () => { + let warn: MockInstance; + + beforeEach(() => { + // The failure is reported, not swallowed; this keeps it out of the run's + // output and lets the test say so. + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + warn.mockRestore(); + }); + + it.each<[string, () => void]>([ + [ + 'the daemon refuses it', + // A preload with no bridge: `userActionHeaders()` sends no proof, and + // the gate answers the way it answers any caller that has none. + () => Object.assign(window, { electron: {} }), + ], + [ + 'the request fails in transit', + () => mocks.getActive.mockResolvedValue({ error: new TypeError('Failed to fetch') }), + ], + [ + 'the request throws', + // Before, this took the whole palette with it: `Promise.all` rejected + // and not one row, command or skill, was left to pick. + () => mocks.getActive.mockRejectedValue(new SyntaxError('Unexpected end of JSON input')), + ], + ])('offers every base and claims none of them for the chat when %s', async (_, fail) => { + fail(); + renderPalette(); + + expect(await screen.findByText('kb:Grant drafts')).toBeInTheDocument(); + expect(screen.getByText('kb:Lab notes')).toBeInTheDocument(); + expect(screen.getByText('kb:Soul')).toBeInTheDocument(); + for (const id of ['grant-drafts', 'lab-notes', 'soul']) { + expect(screen.getByText(`Knowledge base · ${id}`)).toBeInTheDocument(); + } + expect(screen.queryAllByText(/Primary knowledge base/)).toHaveLength(0); + expect(screen.queryAllByText(/in this chat/)).toHaveLength(0); + expect(warn).toHaveBeenCalledWith('Knowledge selection not read:', expect.any(String)); + }); + }); }); diff --git a/ui/desktop/src/components/MentionPopover.tsx b/ui/desktop/src/components/MentionPopover.tsx index 21ecb6ffe..92e4579d6 100644 --- a/ui/desktop/src/components/MentionPopover.tsx +++ b/ui/desktop/src/components/MentionPopover.tsx @@ -10,9 +10,9 @@ import { import { createPortal } from 'react-dom'; import { ItemIcon } from './ItemIcon'; import BuiltInBadge from './ui/BuiltInBadge'; -import { CommandType, getActive, getSessionExtensions, getSlashCommands, listBases } from '../api'; +import { CommandType, getSessionExtensions, getSlashCommands, listBases } from '../api'; import type { CatalogView } from '../api'; -import { userActionHeaders } from '../utils/userAction'; +import { readKnowledgeSelection, type KnowledgeSelection } from './knowledge/knowledgeSelection'; import { getInitialWorkingDir } from '../utils/workingDir'; import { IMAGE_EXTENSIONS } from '../utils/imageFormats'; import { labelledRefTag, refTag, type RefKind } from '../utils/resourceRefs'; @@ -100,6 +100,20 @@ const REFERENCE_KIND: Partial> = { Extension: 'extension', }; +/** + * What a knowledge-base row may say about the chat, given what the daemon said. + * + * With no selection the read failed, and a failure answers neither question the + * other two labels claim to — is this base in the chat, is it the primary — so + * the row names only what it is. It is still offered: a reference names its + * base by id, and an explicit id reaches a base whatever the chat's selection + * (`kb_id_or_primary` in the knowledge server). + */ +const knowledgeBaseRole = (selection: KnowledgeSelection | null, kbId: string) => { + if (!selection) return 'Knowledge base'; + return selection.primaryKbId === kbId ? 'Primary knowledge base' : 'Knowledge base in this chat'; +}; + /** * The resource `item` names, or `null` if it is not a resource reference. * @@ -544,23 +558,16 @@ const MentionPopover = forwardRef< const loadReferenceItems = useCallback( async (includeCommands: boolean) => { - const [commandsResponse, basesResponse, activeResponse, skillsResult, sessionExtensions] = + const [commandsResponse, basesResponse, selection, skillsResult, sessionExtensions] = await Promise.all([ includeCommands ? getSlashCommands({ throwOnError: true }) : Promise.resolve({ data: { commands: [] } }), listBases({ throwOnError: false }), - // Issue #56 Task 58: a GET naming a PRIVATE chat needs the user's - // proof, as `setActive` sends it. Refused, the rows below fell back - // to "nothing hidden, nothing primary" and offered every base as - // one this chat has. - userActionHeaders().then((headers) => - getActive({ - query: sessionId ? { session_id: sessionId } : undefined, - headers, - throwOnError: false, - }) - ), + // Issue #56 Task 58: sent with the user's proof, which a GET naming + // a PRIVATE chat needs. `null` when the read failed anyway, and + // `knowledgeBaseRole` then claims nothing about the chat. + readKnowledgeSelection(sessionId), // The daemon's catalog, not a renderer scan: a skill bundled inside // an installed extension was loadable by the model and absent from // this list, so `@skill:word` completed to nothing (#113). @@ -596,14 +603,11 @@ const MentionPopover = forwardRef< } } - const hiddenKbIds = new Set(activeResponse.data?.hidden_kbs ?? []); - const primaryKbId = - activeResponse.data?.primary_kb ?? activeResponse.data?.active_kb ?? null; for (const base of basesResponse.data ?? []) { - if (hiddenKbIds.has(base.id)) continue; + if (selection?.hiddenKbIds.has(base.id)) continue; commandItems.push({ name: `kb:${base.name}`, - extra: `${primaryKbId === base.id ? 'Primary knowledge base' : 'Knowledge base in this chat'} · ${base.id}`, + extra: `${knowledgeBaseRole(selection, base.id)} · ${base.id}`, itemType: 'KnowledgeBase', relativePath: base.id, }); diff --git a/ui/desktop/src/components/knowledge/knowledgeSelection.ts b/ui/desktop/src/components/knowledge/knowledgeSelection.ts index 7a8c2717f..299f3ff8e 100644 --- a/ui/desktop/src/components/knowledge/knowledgeSelection.ts +++ b/ui/desktop/src/components/knowledge/knowledgeSelection.ts @@ -1,3 +1,7 @@ +import { getActive } from '../../api'; +import { userActionHeaders } from '../../utils/userAction'; +import { briefSelectionFailure } from './selectionWarning'; + /** The shape both selection endpoints answer with — GET /active and POST /active. */ type SelectionPayload = | { primary_kb?: string | null; active_kb?: string | null; hidden_kbs?: string[] | null } @@ -17,3 +21,55 @@ export function readHidden(data: SelectionPayload): string[] | null { ? data.hidden_kbs.filter((id): id is string => typeof id === 'string') : null; } + +/** A chat's knowledge-base selection, as the daemon answered it. */ +export interface KnowledgeSelection { + primaryKbId: string | null; + hiddenKbIds: ReadonlySet; +} + +/** + * Read the selection of the chat `sessionId` names, or the machine-wide one + * when it names none, for a surface that shows the selection or saves it. + * + * Resolves to `null` when the read failed, and never rejects. + * + * ⚠ **`null` is not "nothing hidden, nothing primary".** Both callers used to + * read a failed request as exactly that, because `data?.hidden_kbs ?? []` + * cannot tell the two apart: the `/` palette offered every base as one "in this + * chat", and the create-workflow modal saved every base into a workflow that + * outlives the chat. A failure is a genuine error since the read carries the + * proof — a surface that cannot prove the person, a dropped connection, an + * older daemon — and none of those is a statement about the chat. The same + * rule `KnowledgeContext` states for `listBases`: a failed request is not an + * empty answer. So the caller gets nothing to mistake for one. + * + * Never rejecting is the other half. The palette reads this beside its + * commands, skills and extensions in one `Promise.all`, and a read that threw + * took every one of them with it. + */ +export async function readKnowledgeSelection( + sessionId: string | null | undefined +): Promise { + try { + const res = await getActive({ + query: sessionId ? { session_id: sessionId } : undefined, + // Issue #56 Task 58: a GET naming a PRIVATE chat is on the reach gate's + // list, and the desktop gets through it by proving the person — as the + // hydrate in `KnowledgeContext` does, and as `setActive` always has. + headers: await userActionHeaders(), + throwOnError: false, + }); + if (!res.data) { + console.warn('Knowledge selection not read:', briefSelectionFailure(res.error)); + return null; + } + return { + primaryKbId: readPrimary(res.data), + hiddenKbIds: new Set(readHidden(res.data) ?? []), + }; + } catch (err) { + console.warn('Knowledge selection not read:', briefSelectionFailure(err)); + return null; + } +} From 4ada6de6f93ceb86fd29354d548a9011840bf2b4 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 11:41:24 -0700 Subject: [PATCH 6/6] fix(desktop): workflow capture takes no bases from a failed selection read The create-workflow modal reads the chat's selection beside the base list, and on a failed read (`data` undefined) it computed `visible` as every base and the default as `visible[0]`. `handleCreateWorkflow` prefers that state over the generated workflow's own `knowledge_bases` block whenever it is non-empty, so the workflow saved every base. Unlike the palette's rows, this claim is written to disk and outlives the chat. It lost in two orders: - The generation brings no block, which happens when it fails and the user fills the form in: every base is saved. - The generation's block lands first and the failed read after it, inside the ~500 ms before the form shows: every base overwrote the daemon's own block. After that window the effect has been torn down, so a late answer is dropped. A failed read, via `readKnowledgeSelection`, now captures nothing. The base list still fills the picker. The daemon's block is what the save falls back to, and `knowledge_bases_for_session` produces one whenever a base exists. Without a block, the workflow says nothing about knowledge bases, which the runtime treats as "leave the selection alone". So that an empty picker does not read as "this chat uses no knowledge bases", the picker says why: "Could not load this chat's knowledge bases, so none were selected automatically." The notice is a `Note` with `role="status"`, because the settings visual vocabulary makes every in-place notice one. It clears when the generation brings the daemon's block, because the selection is known then. Tests: no bases captured without a block, the notice, and the late failed read that overwrote the block. All three fail before this change. A fourth covers the usual order (the read fails first, the block arrives later). It passes before the change as a guard, and fails if the notice is not cleared. --- .../CreateWorkflowFromSessionModal.tsx | 53 ++++-- .../CreateWorkflowFromSessionModal.test.tsx | 154 +++++++++++++++++- .../workflows/shared/WorkflowFormFields.tsx | 4 + .../shared/WorkflowResourcePicker.tsx | 5 + 4 files changed, 198 insertions(+), 18 deletions(-) diff --git a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx index b79582b70..d7b4361a7 100644 --- a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx +++ b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx @@ -5,8 +5,8 @@ import { Save, Play, Loader2 } from '../icons/app-icons'; import { Button } from '../ui/button'; import { WorkflowFormFields } from './shared/WorkflowFormFields'; import { WorkflowFormData } from './shared/workflowFormSchema'; -import { createWorkflow, getActive, getSessionExtensions, listBases } from '../../api/sdk.gen'; -import { userActionHeaders } from '../../utils/userAction'; +import { createWorkflow, getSessionExtensions, listBases } from '../../api/sdk.gen'; +import { readKnowledgeSelection } from '../knowledge/knowledgeSelection'; import { WorkflowParameter } from './shared/workflowFormSchema'; import { toastError } from '../../toasts'; import { saveWorkflow } from '../../workflow/workflow_management'; @@ -23,6 +23,9 @@ interface CreateWorkflowFromSessionModalProps { onWorkflowCreated?: (workflow: Workflow) => void; } +const KNOWLEDGE_SELECTION_UNREAD = + "Could not load this chat's knowledge bases, so none were selected automatically."; + export default function CreateWorkflowFromSessionModal({ isOpen, onClose, @@ -37,6 +40,9 @@ export default function CreateWorkflowFromSessionModal({ const [knowledgeBaseItems, setKnowledgeBaseItems] = useState([]); const [workflowKnowledgeBaseIds, setWorkflowKnowledgeBaseIds] = useState([]); const [defaultKnowledgeBaseId, setDefaultKnowledgeBaseId] = useState(null); + // The chat's selection could not be read, and the generation has not brought + // the daemon's own copy of it: nothing was captured, and the picker says so. + const [knowledgeSelectionUnread, setKnowledgeSelectionUnread] = useState(false); const [skillItems, setSkillItems] = useState([]); const [workflowSkillIds, setWorkflowSkillIds] = useState([]); const generatedResourcesRef = useRef<{ @@ -108,22 +114,13 @@ export default function CreateWorkflowFromSessionModal({ Promise.all([ listBases({ throwOnError: false }), - // Issue #56 Task 58: this modal opens from the chat it names, and a GET - // naming a PRIVATE chat needs the user's proof. Refused, the capture - // took every base, with whichever came first as its default. - userActionHeaders().then((headers) => - getActive({ query: { session_id: sessionId }, headers, throwOnError: false }) - ), - ]).then(([basesRes, activeRes]) => { + // Issue #56 Task 58: this modal opens from the chat it names, and the + // read carries the user's proof, which a GET naming a PRIVATE chat + // needs. `null` when the read failed anyway. + readKnowledgeSelection(sessionId), + ]).then(([basesRes, selection]) => { if (cancelled) return; const bases: Manifest[] = basesRes.data ?? []; - const hidden = new Set(activeRes.data?.hidden_kbs ?? []); - const visible = bases.filter((base) => !hidden.has(base.id)).map((base) => base.id); - // The captured default is the session's primary; `active_kb` is the - // deprecated mirror, read so a fresh renderer survives an older daemon. - const primary = activeRes.data?.primary_kb ?? activeRes.data?.active_kb ?? null; - const defaultId = primary && visible.includes(primary) ? primary : (visible[0] ?? null); - setKnowledgeBaseItems( bases.map((base) => ({ id: base.id, @@ -131,6 +128,23 @@ export default function CreateWorkflowFromSessionModal({ description: base.id, })) ); + + if (!selection) { + // A failed read is not "nothing is hidden, nothing is primary", and + // capturing it as that saved every base, with whichever came first as + // the default, into a workflow that outlives this chat. Capture + // nothing instead. The generation carries the daemon's own block + // whenever a base exists, and the save path falls back to it; with + // none, the picker says why nothing is selected. A block that already + // arrived needs no such note, and must not be overwritten. + if (!generatedResourcesRef.current.knowledgeBases) setKnowledgeSelectionUnread(true); + return; + } + 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); }); @@ -215,6 +229,9 @@ export default function CreateWorkflowFromSessionModal({ } if (workflow.knowledge_bases) { + // The daemon read the chat's selection itself, so whatever this + // modal's own read said, the selection is known now. + setKnowledgeSelectionUnread(false); const visible = workflow.knowledge_bases.visible ?? []; generatedResourcesRef.current.knowledgeBases = { default: @@ -276,6 +293,7 @@ export default function CreateWorkflowFromSessionModal({ setKnowledgeBaseItems([]); setWorkflowKnowledgeBaseIds([]); setDefaultKnowledgeBaseId(null); + setKnowledgeSelectionUnread(false); setSkillItems([]); setWorkflowSkillIds([]); generatedResourcesRef.current = {}; @@ -485,6 +503,9 @@ export default function CreateWorkflowFromSessionModal({ resourceEditsRef.current.knowledgeBases = true; setDefaultKnowledgeBaseId(id); }} + knowledgeBaseNotice={ + knowledgeSelectionUnread ? KNOWLEDGE_SELECTION_UNREAD : undefined + } skillItems={skillItems} selectedSkillIds={workflowSkillIds} onSkillIdsChange={(ids) => { diff --git a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx index aa23765f1..d06994d93 100644 --- a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx +++ b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx @@ -1,5 +1,5 @@ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { render, screen, waitFor } from '@testing-library/react'; +import { describe, it, expect, vi, 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'; @@ -7,6 +7,15 @@ import type { CreateWorkflowResponse } from '../../../api/types.gen'; import { saveWorkflow } from '../../../workflow/workflow_management'; import { reachGatedGetActive, USER_ACTION_KEY } from '../../../test/reachGate'; +/** A promise the test settles by hand, so "which answer landed first" is a fact, not a race. */ +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + vi.mock('../../../api/sdk.gen', () => ({ createWorkflow: vi.fn(), // The modal and its child WorkflowFormFields now load extensions, knowledge @@ -572,5 +581,146 @@ describe('CreateWorkflowFromSessionModal', () => { }) ); }); + + /** + * With the proof attached, a read that still fails is a genuine error — a + * surface that cannot prove the person, a dropped connection, an older + * daemon — and none of those said "nothing is hidden, nothing is primary". + * The modal saved it as exactly that: every base, with whichever came first + * as the default, written into a workflow that outlives the chat. + * + * A failed read now captures nothing. The daemon's own block, which the + * generation carries whenever a base exists, is what the workflow keeps; + * without one the workflow has nothing to say about knowledge bases, and + * the picker says why nothing is selected. + */ + describe('when its selection cannot be read', () => { + const SELECTION_UNREAD = + "Could not load this chat's knowledge bases, so none were selected automatically."; + const withGeneratedKnowledgeBases = { + data: { + workflow: { + title: 'Analyzed Workflow Title', + description: 'Analyzed description', + instructions: 'Analyzed instructions', + // The daemon reads the chat's selection itself, past no gate. + knowledge_bases: { default: 'lab-notes', visible: ['lab-notes', 'soul'] }, + }, + error: undefined, + }, + error: undefined, + request: new globalThis.Request('http://localhost/test'), + response: new globalThis.Response(), + }; + let warn: MockInstance; + + beforeEach(() => { + // A preload with no bridge: `userActionHeaders()` sends no proof, and + // the gate answers the way it answers any caller that has none. + Object.assign(window, { + electron: { listSkillDirs: vi.fn().mockResolvedValue([]) }, + }); + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + 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(); + + const saved = await saveTheWorkflow(user); + + expect(saved?.knowledge_bases).toBeUndefined(); + expect(warn).toHaveBeenCalledWith('Knowledge selection not read:', expect.any(String)); + }); + + it('says why no knowledge base is selected', async () => { + const user = userEvent.setup(); + render(); + + // Nothing was captured, so nothing opens the advanced options for us. + await user.click(await screen.findByText('Advanced options')); + + const notice = screen.getByText(SELECTION_UNREAD); + expect(notice.closest('[role="status"]')).not.toBeNull(); + expect(screen.getByText('No KBs selected')).toBeInTheDocument(); + }); + + // The race the old code lost: a read answering after the generation wrote + // every base over the daemon's own block. Only inside the analysis window, + // though — once the form is up, the effect has been torn down and a late + // answer is dropped — so the read is released while the spinner still runs. + it("keeps the generation's knowledge bases when the failed read answers after it", async () => { + const user = userEvent.setup(); + const bases = deferred(); + mockListBases.mockReturnValue(bases.promise as never); + mockCreateWorkflow.mockResolvedValue(withGeneratedKnowledgeBases); + render(); + + await settleReads(); + expect(screen.getByTestId('analyzing-state')).toBeInTheDocument(); + bases.resolve({ data: [kb('soul'), kb('lab-notes'), kb('grant-drafts')] }); + await settleReads(); + + expect(await screen.findByTestId('form-state')).toBeInTheDocument(); + expect(screen.getByText('2 KBs selected')).toBeInTheDocument(); + expect(screen.queryByText(SELECTION_UNREAD)).not.toBeInTheDocument(); + const saved = await saveTheWorkflow(user); + expect(saved?.knowledge_bases).toEqual({ + default: 'lab-notes', + visible: ['lab-notes', 'soul'], + }); + expect(warn).toHaveBeenCalledWith('Knowledge selection not read:', expect.any(String)); + }); + + // The usual order in the app: the read fails at once, and the generation, + // which waits on a model, brings the daemon's block afterwards. That block + // IS the chat's selection, so the picker must not go on calling it unread. + it('stops calling the selection unread once the generation brings it', async () => { + const user = userEvent.setup(); + const generation = deferred(); + mockCreateWorkflow.mockReturnValue(generation.promise as never); + render(); + + await waitFor(() => expect(mockGetActive).toHaveBeenCalled()); + await settleReads(); + await act(async () => { + generation.resolve(withGeneratedKnowledgeBases); + }); + + // The generation's block opens the advanced options on its own. + expect(await screen.findByText('2 KBs selected')).toBeInTheDocument(); + expect(screen.queryByText(SELECTION_UNREAD)).not.toBeInTheDocument(); + const saved = await saveTheWorkflow(user); + expect(saved?.knowledge_bases).toEqual({ + default: 'lab-notes', + visible: ['lab-notes', 'soul'], + }); + }); + }); }); }); diff --git a/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx b/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx index 0d55c9cf2..6be9eaea7 100644 --- a/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx +++ b/ui/desktop/src/components/workflows/shared/WorkflowFormFields.tsx @@ -30,6 +30,8 @@ interface WorkflowFormFieldsProps { onKnowledgeBaseIdsChange?: (ids: string[]) => void; defaultKnowledgeBaseId?: string | null; onDefaultKnowledgeBaseIdChange?: (id: string | null) => void; + /** Shown beside the knowledge-base picker, e.g. why nothing in it is selected. */ + knowledgeBaseNotice?: string; skillItems?: WorkflowResourceItem[]; selectedSkillIds?: string[]; onSkillIdsChange?: (ids: string[]) => void; @@ -72,6 +74,7 @@ export function WorkflowFormFields({ onKnowledgeBaseIdsChange, defaultKnowledgeBaseId, onDefaultKnowledgeBaseIdChange, + knowledgeBaseNotice, skillItems = [], selectedSkillIds = [], onSkillIdsChange, @@ -686,6 +689,7 @@ export function WorkflowFormFields({ onSelectedIdsChange={onKnowledgeBaseIdsChange} defaultId={defaultKnowledgeBaseId} onDefaultIdChange={onDefaultKnowledgeBaseIdChange} + notice={knowledgeBaseNotice} emptyText="No knowledge bases found" searchPlaceholder="Search knowledge bases..." noun="KB" diff --git a/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx b/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx index d181e7929..8b9ab5b87 100644 --- a/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx +++ b/ui/desktop/src/components/workflows/shared/WorkflowResourcePicker.tsx @@ -1,5 +1,6 @@ import { useMemo, useState } from 'react'; import { Check, ChevronDown, Search } from '../../icons/app-icons'; +import { Note } from '../../ui/note'; import { Popover, PopoverContent, PopoverTrigger } from '../../ui/popover'; import { Switch } from '../../ui/switch'; import { cn } from '../../../utils'; @@ -19,6 +20,8 @@ interface WorkflowResourcePickerProps { onSelectedIdsChange: (ids: string[]) => void; defaultId?: string | null; onDefaultIdChange?: (id: string | null) => void; + /** A standing condition the selection is subject to, shown under the label. */ + notice?: string; emptyText: string; searchPlaceholder: string; noun: string; @@ -38,6 +41,7 @@ export function WorkflowResourcePicker({ onSelectedIdsChange, defaultId, onDefaultIdChange, + notice, emptyText, searchPlaceholder, noun, @@ -182,6 +186,7 @@ export function WorkflowResourcePicker({ + {notice && {notice}} ); }