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..cb76b4523 --- /dev/null +++ b/ui/desktop/src/components/MentionPopover.privateChat.test.tsx @@ -0,0 +1,186 @@ +import { render, screen } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } 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 }, + }) + ); + }); + + /** + * 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 b49b56b7f..92e4579d6 100644 --- a/ui/desktop/src/components/MentionPopover.tsx +++ b/ui/desktop/src/components/MentionPopover.tsx @@ -10,8 +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 { 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'; @@ -99,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. * @@ -543,16 +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 }), - getActive({ - query: sessionId ? { session_id: sessionId } : undefined, - 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). @@ -588,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/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..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. */ @@ -186,6 +167,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 +438,13 @@ 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. 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, }); if (cancelled || generation !== selectionGenerationRef.current) return; @@ -462,12 +452,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/knowledge/knowledgeSelection.ts b/ui/desktop/src/components/knowledge/knowledgeSelection.ts new file mode 100644 index 000000000..299f3ff8e --- /dev/null +++ b/ui/desktop/src/components/knowledge/knowledgeSelection.ts @@ -0,0 +1,75 @@ +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 } + | 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; +} + +/** 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; + } +} 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 diff --git a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx index b17c85653..d7b4361a7 100644 --- a/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx +++ b/ui/desktop/src/components/workflows/CreateWorkflowFromSessionModal.tsx @@ -5,7 +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 { 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'; @@ -22,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, @@ -36,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<{ @@ -107,17 +114,13 @@ export default function CreateWorkflowFromSessionModal({ Promise.all([ listBases({ throwOnError: false }), - getActive({ query: { session_id: sessionId }, 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, @@ -125,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); }); @@ -209,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: @@ -270,6 +293,7 @@ export default function CreateWorkflowFromSessionModal({ setKnowledgeBaseItems([]); setWorkflowKnowledgeBaseIds([]); setDefaultKnowledgeBaseId(null); + setKnowledgeSelectionUnread(false); setSkillItems([]); setWorkflowSkillIds([]); generatedResourcesRef.current = {}; @@ -479,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 2a4cfa209..d06994d93 100644 --- a/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx +++ b/ui/desktop/src/components/workflows/__tests__/CreateWorkflowFromSessionModal.test.tsx @@ -1,10 +1,20 @@ -import { describe, it, expect, vi, beforeEach } 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, 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'; + +/** 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(), @@ -70,6 +80,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 +495,232 @@ 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 }, + }) + ); + }); + + /** + * 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}} ); } 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) }); + }; +}