Skip to content

Commit 991feb9

Browse files
Waleed Latifclaude
andcommitted
fix(search): stop asking the model to cite where the chat cannot render one
The `<source>` citation contract shipped with Sim Search in #7385: the tag, the inline chip, and the sources strip under a reply all arrived together. The tool that asks for it did not arrive gated. `manage_knowledge_base`'s query result appends the citation instruction unconditionally, and that tool runs in ordinary Build turns — so in a workspace without per-member access the model is still told to emit `<source>` tags and the chat still renders the chips and the sources popover. An unreleased surface, reachable today without touching Search at all. Ask for the citation only where per-member access is on. The gate belongs at the emission rather than at the renderer: a client that merely declined to render the tag would leave the raw `<source>{...}</source>` JSON sitting in the visible reply. Resolved alongside the search it accompanies, so the gate costs no round trip. Also from the review round: - `handleSubmit` re-applies the gate where the mode is consumed. `modeOverride` is passed straight in and never passes through `useMothershipMode`, so the hook alone was not the choke point the last commit claimed. - the `?q=` effect returns early instead of calling a setter the hook would drop, so the gate reads at the call site rather than at a distance. - `setMode` keeps returning its promise, so the `void` at its three call sites still means what it says. - comments: restore the subject the shortened one-liners had lost, drop a duplicated why, and stop the client hook from re-listing the server predicate's ingredients. - drop three hook tests the switcher's own tests already cover through the same adapter, and the container one of them leaked. Kept deliberately, against a reviewer's suggestion to delete them as now unreachable: the `memberAccessAvailable` guards on the member-connector queries. `useWorkspaceMemberConnectors` sets `placeholderData: keepPreviousData`, so a disabled observer can still surface previously cached rows — the render-time coercion to `EMPTY_MEMBER_CONNECTORS` is what stops rows from a flag-on render leaking into a flag-off one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014JmUGTitFeoQXVf9T3skEa
1 parent 9189a38 commit 991feb9

9 files changed

Lines changed: 81 additions & 50 deletions

File tree

‎apps/sim/app/workspace/[workspaceId]/home/components/knowledge-search-results/knowledge-search-results.tsx‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,8 @@ export function KnowledgeSearchResults({
186186
error,
187187
} = useWorkspaceKnowledgeSearch(workspaceId, knowledgeBaseIds, query)
188188
/**
189-
* Judged by the workspace, as the server judges it: with per-member access
190-
* off, member-scoped documents are hidden, so no source is indexing anything
191-
* the viewer will see, and the list is not worth asking for.
189+
* With per-member access off, member-scoped documents are hidden, so the
190+
* indexing list is not worth asking for.
192191
*/
193192
const memberAccessAvailable = useMemberAccessAvailable()
194193
const { data: memberConnectorRows } = useWorkspaceMemberConnectors(workspaceId, {

‎apps/sim/app/workspace/[workspaceId]/home/components/search-sources/search-sources.tsx‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,7 @@ interface SearchSourcesProps {
140140
*/
141141
export function SearchSources({ workspaceId }: SearchSourcesProps) {
142142
const { integrationAvailability } = usePermissionConfig()
143-
/**
144-
* Judged by the workspace, as the server judges it: with per-member access
145-
* off, a connect is refused, so the chips say so instead of offering one.
146-
*/
143+
/** With per-member access off, a connect is refused, so the chips say so instead. */
147144
const memberAccessAvailable = useMemberAccessAvailable()
148145
const { data: workspacePermissions } = useWorkspacePermissionsQuery(workspaceId)
149146
/** The first connect of a source turns it on for the workspace, which takes an admin. */

‎apps/sim/app/workspace/[workspaceId]/home/home.tsx‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -190,11 +190,14 @@ export function Home({ chatId, userName, userId }: HomeProps) {
190190
/**
191191
* A link that carries a query but no mode opens in Search with the query in
192192
* the box; the composer follows the live query the same way (below), so the
193-
* box and the results never show two different queries.
193+
* box and the results never show two different queries. Where per-member
194+
* access is off there is no Search to open into, so the query stays a plain
195+
* Build draft rather than a mode write `useMothershipMode` would drop.
194196
*/
195197
useEffect(() => {
198+
if (!memberAccessAvailable) return
196199
if (searchQuery.trim() && composerMode === 'build') void setComposerMode('search')
197-
}, [searchQuery, composerMode, setComposerMode])
200+
}, [memberAccessAvailable, searchQuery, composerMode, setComposerMode])
198201
const hasCheckedLandingStorageRef = useRef(false)
199202
const initialViewInputRef = useRef<HTMLDivElement>(null)
200203
const initialViewUserInputRef = useRef<UserInputHandle>(null)
@@ -492,8 +495,13 @@ export function Home({ chatId, userName, userId }: HomeProps) {
492495
* Search lists documents, not a turn of the agent, and only a query can
493496
* be searched: attachments alone have nothing to search for. Assistant
494497
* makes the query a turn of the agent grounded in the sources.
498+
*
499+
* The override skips `useMothershipMode`, so the gate is applied again
500+
* where the mode is consumed: both modes answer from the workspace's
501+
* indexed sources, and neither is offered where those do not exist.
495502
*/
496-
const mode = modeOverride ?? composerMode
503+
const requestedMode = modeOverride ?? composerMode
504+
const mode = requestedMode !== 'build' && !memberAccessAvailable ? 'build' : requestedMode
497505
const answering = mode === 'assistant'
498506
if (mode === 'search') {
499507
/** A search sends nothing, so an edit in progress is released rather than left waiting. */
@@ -536,6 +544,7 @@ export function Home({ chatId, userName, userId }: HomeProps) {
536544
workspaceId,
537545
chatId,
538546
composerMode,
547+
memberAccessAvailable,
539548
editingQueuedId,
540549
cancelQueueEdit,
541550
prepareResourceViewForAgentTurn,

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-mothership-mode.test.tsx‎

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -69,16 +69,12 @@ afterEach(() => {
6969
vi.useRealTimers()
7070
})
7171

72+
/**
73+
* The mode's ordinary read/write behavior is covered through the UI in
74+
* `mode-switcher.test.tsx`; one write stands here as the control the
75+
* per-member-access cases are read against.
76+
*/
7277
describe('useMothershipMode', () => {
73-
it('defaults to Build and reads the mode the URL names', () => {
74-
mount()
75-
expect(mode()).toBe('build')
76-
77-
act(() => root?.unmount())
78-
mount('?mode=search')
79-
expect(mode()).toBe('search')
80-
})
81-
8278
it('writes the chosen mode to the URL', async () => {
8379
mount()
8480
await setMode('search')
@@ -87,13 +83,6 @@ describe('useMothershipMode', () => {
8783
expect(mockUrlUpdate.mock.lastCall?.[0].searchParams.get('mode')).toBe('search')
8884
})
8985

90-
it('drops the query and its filters on every mode but Search', async () => {
91-
mount('?mode=search&q=budget&source=upload&updated=7d')
92-
await setMode('assistant')
93-
94-
expect(mockUrlUpdate.mock.lastCall?.[0].searchParams.toString()).toBe('mode=assistant')
95-
})
96-
9786
describe('without per-member access', () => {
9887
beforeEach(() => {
9988
mockMemberAccessAvailable.mockReturnValue(false)

‎apps/sim/app/workspace/[workspaceId]/home/hooks/use-mothership-mode.ts‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,19 +16,17 @@ import { useMemberAccessAvailable } from '@/hooks/use-member-access'
1616
* separate Search and Assistant routes do. Build is the clean URL.
1717
*
1818
* Search and Assistant both answer from the workspace's indexed sources, so
19-
* both exist only where per-member access is on. This is the one place that
20-
* decides it: with the feature off the mode reads Build whatever the URL says
21-
* — a stale link cannot strand the composer in a mode whose switcher is not
22-
* rendered to leave it — and a write to either mode is dropped rather than
23-
* putting a mode in the URL that the next read would contradict.
19+
* both exist only where per-member access is on. With the feature off the mode
20+
* reads Build whatever the URL says, and a write to either is dropped rather
21+
* than leaving a mode in the URL that the next read would contradict.
2422
*/
2523
export function useMothershipMode() {
2624
const memberAccessAvailable = useMemberAccessAvailable()
2725
const [{ mode }, setParams] = useQueryStates(composerModeParsers, resourceUrlKeys)
2826
const setMode = useCallback(
29-
(next: MothershipMode) => {
27+
async (next: MothershipMode) => {
3028
if (next !== 'build' && !memberAccessAvailable) return
31-
void setParams(
29+
await setParams(
3230
{
3331
mode: next,
3432
...(next === 'search' ? {} : { q: null, ...CLEARED_SEARCH_FILTERS }),

‎apps/sim/app/workspace/[workspaceId]/search/search.tsx‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,8 @@ export function Search() {
124124
const workspaceId = (params?.workspaceId as string) || ''
125125
const { integrationAvailability } = usePermissionConfig()
126126
/**
127-
* Judged by the workspace, as the server judges it: with per-member access
128-
* off, every connect is refused, so the rows say so instead of offering
129-
* one and the memberships are not fetched.
127+
* With per-member access off, every connect is refused, so the rows say so
128+
* and the memberships are not fetched.
130129
*/
131130
const memberAccessAvailable = useMemberAccessAvailable()
132131
const { data: workspacePermissions } = useWorkspacePermissionsQuery(workspaceId)

‎apps/sim/hooks/use-member-access.ts‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,11 @@
33
import { useOptionalWorkspaceHostContext } from '@/app/workspace/[workspaceId]/providers/workspace-host-provider'
44

55
/**
6-
* Whether per-member knowledge access is on for the routed workspace, as the
7-
* server judged it: the `knowledge-member-access` flag and Credential Groups,
8-
* resolved once into the workspace host context.
6+
* Whether per-member knowledge access is on for the routed workspace, as
7+
* `isKnowledgeMemberAccessAvailable` judged it, resolved once into the
8+
* workspace host context.
99
*
10-
* The single client-side reading of that judgement, so every Sim Search
11-
* surface — the tab, the page, the composer's Search mode, the source rows,
12-
* and the connector modals — appears and behaves together, and none can drift
10+
* The single client-side reading of that judgement, so no surface can drift
1311
* into offering a feature the server refuses. Outside a workspace route there
1412
* is no workspace to judge, so it reads false.
1513
*/

‎apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.test.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ const {
2525
mockCreateKnowledgeConnector,
2626
mockCreateKnowledgeTag,
2727
mockListKnowledgeTags,
28+
mockIsKnowledgeMemberAccessAvailable,
2829
knowledgeOperations,
2930
} = vi.hoisted(() => {
3031
const defineOperation = (id: string, minimumRole: 'read' | 'write') =>
@@ -58,6 +59,7 @@ const {
5859
mockCreateKnowledgeConnector: vi.fn(),
5960
mockCreateKnowledgeTag: vi.fn(),
6061
mockListKnowledgeTags: vi.fn(),
62+
mockIsKnowledgeMemberAccessAvailable: vi.fn(),
6163
knowledgeOperations: {
6264
addWorkspaceFiles: defineOperation('knowledge.documents.add_workspace_files', 'write'),
6365
bulkDelete: defineOperation('knowledge.bulk_delete', 'write'),
@@ -98,6 +100,9 @@ vi.mock('@/lib/core/telemetry', () => ({
98100
}))
99101
vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: mockCaptureServerEvent }))
100102
vi.mock('@/lib/knowledge/application/operations', () => ({ knowledgeOperations }))
103+
vi.mock('@/lib/knowledge/access/availability', () => ({
104+
isKnowledgeMemberAccessAvailable: mockIsKnowledgeMemberAccessAvailable,
105+
}))
101106
vi.mock('@/lib/knowledge/application/add-workspace-files', () => ({
102107
addWorkspaceFilesToKnowledgeBase: {
103108
operation: knowledgeOperations.addWorkspaceFiles,
@@ -260,6 +265,7 @@ describe('manage_knowledge_base trusted application delegation', () => {
260265
],
261266
failed: [],
262267
})
268+
mockIsKnowledgeMemberAccessAvailable.mockResolvedValue(true)
263269
mockSearchKnowledge.mockResolvedValue({
264270
results: [],
265271
query: 'query',
@@ -407,6 +413,30 @@ describe('manage_knowledge_base trusted application delegation', () => {
407413
expect(mockReadKnowledgeBase).not.toHaveBeenCalled()
408414
})
409415

416+
it('asks for citations where per-member access is on', async () => {
417+
const result = await knowledgeBaseServerTool.execute(
418+
{ operation: 'query', args: { knowledgeBaseId: KNOWLEDGE_BASE.id, query: 'query' } },
419+
{ ...CONTEXT, resolvedSecretTraceRegistry: new ResolvedSecretTraceRegistry() }
420+
)
421+
422+
expect(result.message).toContain('<source>')
423+
expect(mockIsKnowledgeMemberAccessAvailable).toHaveBeenCalledWith({
424+
workspaceId: 'workspace-paid',
425+
})
426+
})
427+
428+
it('asks for no citation where the workspace cannot render one', async () => {
429+
mockIsKnowledgeMemberAccessAvailable.mockResolvedValue(false)
430+
431+
const result = await knowledgeBaseServerTool.execute(
432+
{ operation: 'query', args: { knowledgeBaseId: KNOWLEDGE_BASE.id, query: 'query' } },
433+
{ ...CONTEXT, resolvedSecretTraceRegistry: new ResolvedSecretTraceRegistry() }
434+
)
435+
436+
expect(result).toMatchObject({ success: true })
437+
expect(result.message).toBe('Found 0 result(s) for query "query".')
438+
})
439+
410440
it('returns a safe model result for search infrastructure failures', async () => {
411441
mockSearchKnowledge.mockRejectedValueOnce(new Error('database unavailable'))
412442

‎apps/sim/lib/copilot/tools/server/knowledge/knowledge-base.ts‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
import { asOrchestrationError } from '@/lib/core/orchestration/types'
2121
import { PlatformEvents } from '@/lib/core/telemetry'
2222
import { getEffectiveDecryptedEnv } from '@/lib/environment/utils'
23+
import { isKnowledgeMemberAccessAvailable } from '@/lib/knowledge/access/availability'
2324
import { addWorkspaceFilesToKnowledgeBase } from '@/lib/knowledge/application/add-workspace-files'
2425
import { KnowledgeUsageLimitExceededError } from '@/lib/knowledge/application/billing'
2526
import {
@@ -64,6 +65,12 @@ const DEFAULT_QUERY_TOP_K = 5
6465
* How the model cites a knowledge result in its reply. The `<source>` tag is
6566
* what the chat renders as a link back to the document, so a result without
6667
* a source URL is quoted by name instead.
68+
*
69+
* Asked for only where per-member access is on. The chip and the sources strip
70+
* that render the tag arrived with Sim Search, so a workspace without the
71+
* feature must not be told to emit one: the gate belongs here, at the emission,
72+
* because a client that merely declined to render the tag would leave the raw
73+
* `<source>{...}</source>` JSON sitting in the visible reply.
6774
*/
6875
const KNOWLEDGE_CITATION_INSTRUCTION =
6976
'Cite each result you use inline, right after the sentence it supports, as <source>{"url":"<sourceUrl>","title":"<documentName>","siteName":"<knowledgeBaseName>","connectorType":"<connectorType>","snippet":"<the sentence or two of content you relied on>","updatedAt":"<sourceModifiedAt>","author":"<author>"}</source> with every value JSON-escaped; leave out any optional field whose value is null or unknown, and omit the tag for a result whose sourceUrl is null and name the document instead.'
@@ -436,13 +443,16 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
436443
'Failed to query knowledge base: Knowledge result secret provenance is unavailable',
437444
}
438445
}
439-
const searchResult = await executeCopilotKnowledgeUseCase(context, searchKnowledge, {
440-
workspaceId,
441-
knowledgeBaseIds: [args.knowledgeBaseId],
442-
query: modelQuery,
443-
topK,
444-
resultSecretRegistry: context.resolvedSecretTraceRegistry,
445-
})
446+
const [searchResult, citable] = await Promise.all([
447+
executeCopilotKnowledgeUseCase(context, searchKnowledge, {
448+
workspaceId,
449+
knowledgeBaseIds: [args.knowledgeBaseId],
450+
query: modelQuery,
451+
topK,
452+
resultSecretRegistry: context.resolvedSecretTraceRegistry,
453+
}),
454+
isKnowledgeMemberAccessAvailable({ workspaceId }),
455+
])
446456
const results = searchResult.results
447457
const knowledgeBase = searchResult.knowledgeBases[0]
448458
if (!knowledgeBase)
@@ -455,9 +465,11 @@ export const knowledgeBaseServerTool: BaseServerTool<KnowledgeBaseArgs, Knowledg
455465
userId: context.userId,
456466
})
457467

468+
const foundMessage = `Found ${results.length} result(s) for query "${truncate(args.query, 50)}".`
469+
458470
return {
459471
success: true,
460-
message: `Found ${results.length} result(s) for query "${truncate(args.query, 50)}". ${KNOWLEDGE_CITATION_INSTRUCTION}`,
472+
message: citable ? `${foundMessage} ${KNOWLEDGE_CITATION_INSTRUCTION}` : foundMessage,
461473
data: {
462474
knowledgeBaseId: args.knowledgeBaseId,
463475
knowledgeBaseName: knowledgeBase.name,

0 commit comments

Comments
 (0)