-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(knowledge-tools): scope kb_* tools to composer-selected bases too #16743
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
b7b091c
ab1ca2a
ede850d
adbc8b6
5f4bb0a
e3951c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,7 @@ export async function buildAgentParams(input: BuildAgentParamsInput): Promise<Bu | |
| applyHttpTrace(sdkConfig, request.chatId, model) | ||
| const fileAttachments = collectFileAttachments(request.messages) | ||
| const hasFileAttachments = fileAttachments.length > 0 | ||
| const knowledgeBaseIds = resolveKnowledgeBaseIds(assistant, request.knowledgeBaseIds) | ||
| const { tools, deferredEntries, mcpToolIds } = canModelConsumeTools(model) | ||
| ? await resolveTools(request, assistant, model, hasFileAttachments) | ||
| : { tools: undefined, deferredEntries: [] as ToolEntry[], mcpToolIds: new Set<string>() } | ||
|
|
@@ -89,7 +90,8 @@ export async function buildAgentParams(input: BuildAgentParamsInput): Promise<Bu | |
| topicId: request.chatId, | ||
| assistant, | ||
| abortSignal: signal, | ||
| fileAttachments | ||
| fileAttachments, | ||
| knowledgeBaseIds | ||
| } | ||
|
|
||
| const scope: RequestScope = { | ||
|
|
@@ -105,7 +107,8 @@ export async function buildAgentParams(input: BuildAgentParamsInput): Promise<Bu | |
| aiSdkProviderId, | ||
| requestContext, | ||
| mcpToolIds, | ||
| hasFileAttachments | ||
| hasFileAttachments, | ||
| knowledgeBaseIds | ||
| } | ||
|
|
||
| const features = extraFeatures?.length ? [...INTERNAL_FEATURES, ...extraFeatures] : INTERNAL_FEATURES | ||
|
|
@@ -218,6 +221,16 @@ function resolveHasAnyKnowledgeBase(): boolean { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Effective knowledge base scope for this request: the assistant's own bound bases, unioned with | ||
| * whatever the composer's `/` picker selected for this turn. Union (not caller-wins, unlike | ||
| * `mcpToolIds`) because a per-turn selection is additive on top of the assistant's default scope, | ||
| * not a replacement for it. | ||
| */ | ||
| export function resolveKnowledgeBaseIds(assistant: Assistant | undefined, requestIds: string[] | undefined): string[] { | ||
| return Array.from(new Set([...(assistant?.knowledgeBaseIds ?? []), ...(requestIds ?? [])])) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[Blocker][A4] Directly union-ing the Original Content[Blocker][A4] 这里直接把 renderer 传来的 |
||
| } | ||
|
|
||
| /** | ||
| * Assemble `AgentOptions`: capability-driven providerOptions overlaid with | ||
| * the user's customParameters (split into AI-SDK standard params vs | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Blocker][A1] The
knowledgeBaseIdscalculated here is not passed intoresolveTools()/registry.selectActive(), but theappliesfor the fourkb_*tools have already been changed to checkscope.knowledgeBaseIds. The actual path is: composer passes inrequest.knowledgeBaseIds, here we get a non-empty scope, but line 80 still callsresolveTools(request, assistant, model, hasFileAttachments);resolveTools()line 190'sselectActive({ assistant, mcpToolIds, hasFileAttachments, hasAnyKnowledgeBase })doesn't includeknowledgeBaseIds, soKnowledgeSearchTooletc.'sappliesall return false. The result is that not only the composer-selected KB that this PR is supposed to fix won't expose tools, but the originally assistant statically-bound KB tools will also be hidden. Minimal fix: pass the effectiveknowledgeBaseIdsintoresolveTools, and pass it toregistry.selectActive({ ..., knowledgeBaseIds }); also add abuildAgentParamslevel test asserting that both assistant-bound and request-selected scenarios will selectkb_*tools.Original Content
[Blocker][A1] 这里算出的
knowledgeBaseIds没有进入resolveTools()/registry.selectActive(),但四个kb_*tool 的applies已经改成检查scope.knowledgeBaseIds。实际路径是:composer 传入request.knowledgeBaseIds,这里得到非空 scope,但第 80 行仍调用resolveTools(request, assistant, model, hasFileAttachments);resolveTools()第 190 行的selectActive({ assistant, mcpToolIds, hasFileAttachments, hasAnyKnowledgeBase })没带knowledgeBaseIds,所以KnowledgeSearchTool等的applies都返回 false。结果不仅本 PR 要修的 composer-selected KB 不会暴露工具,原本 assistant 静态绑定的 KB 工具也会被隐藏。最小修复:把 effectiveknowledgeBaseIds传进resolveTools,并传给registry.selectActive({ ..., knowledgeBaseIds });同时加一个buildAgentParams级别测试,断言 assistant-bound 和 request-selected 两种场景都会选中kb_*工具。