fix(knowledge-tools): scope kb_* tools to composer-selected bases too#16743
Conversation
The composer's per-turn "/" knowledge base picker was silently dropped before it reached main: `ai.stream_open` had no field for it, and the kb_search/kb_list/kb_read/kb_manage tools' exposure gate only checked the assistant's static `knowledgeBaseIds`. When an assistant had no bound knowledge base, selecting one via the composer had no effect — the model never even saw the kb_* tools as available. Thread the composer selection through AiStreamOpenRequest -> PersistentChatContextProvider -> buildAgentParams, union it with the assistant's bound bases, and gate/execute the four kb_* tools against that effective scope instead of the assistant's bound ids alone. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
There was a problem hiding this comment.
This review was translated automatically.
This change passes the composer's per-turn knowledge base selection through to ai.stream_open, PersistentChatContextProvider, buildAgentParams, and four kb_* tools, and updates the related tool unit tests. The direction is correct, but there are two issues that must be fixed first: effective knowledgeBaseIds is not being included in tool selection, causing KB tools to not actually be exposed; additionally, the main side directly trusting requestIds passed from the renderer will bypass the assistant's static KB binding boundary.
Original Content
这次改动把 composer 的 per-turn knowledge base 选择一路传到 ai.stream_open、PersistentChatContextProvider、buildAgentParams 和四个 kb_* tools，并更新了相关 tool 单测。方向是对的，但目前有两个必须先修的问题：effective knowledgeBaseIds 没有进入 tool selection，导致 KB tools 实际不会暴露；同时 main 侧直接信任 renderer 传入的 requestIds 会绕过 assistant 的静态 KB 绑定边界。
| applyHttpTrace(sdkConfig, request.chatId, model) | ||
| const fileAttachments = collectFileAttachments(request.messages) | ||
| const hasFileAttachments = fileAttachments.length > 0 | ||
| const knowledgeBaseIds = resolveKnowledgeBaseIds(assistant, request.knowledgeBaseIds) |
There was a problem hiding this comment.
This review comment was translated automatically.
[Blocker][A1] The knowledgeBaseIds calculated here is not passed into resolveTools() / registry.selectActive(), but the applies for the four kb_* tools have already been changed to check scope.knowledgeBaseIds. The actual path is: composer passes in request.knowledgeBaseIds, here we get a non-empty scope, but line 80 still calls resolveTools(request, assistant, model, hasFileAttachments); resolveTools() line 190's selectActive({ assistant, mcpToolIds, hasFileAttachments, hasAnyKnowledgeBase }) doesn't include knowledgeBaseIds, so KnowledgeSearchTool etc.'s applies all 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 effective knowledgeBaseIds into resolveTools, and pass it to registry.selectActive({ ..., knowledgeBaseIds }); also add a buildAgentParams level test asserting that both assistant-bound and request-selected scenarios will select kb_* 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 工具也会被隐藏。最小修复：把 effective knowledgeBaseIds 传进 resolveTools，并传给 registry.selectActive({ ..., knowledgeBaseIds })；同时加一个 buildAgentParams 级别测试，断言 assistant-bound 和 request-selected 两种场景都会选中 kb_* 工具。
| * not a replacement for it. | ||
| */ | ||
| export function resolveKnowledgeBaseIds(assistant: Assistant | undefined, requestIds: string[] | undefined): string[] { | ||
| return Array.from(new Set([...(assistant?.knowledgeBaseIds ?? []), ...(requestIds ?? [])])) |
There was a problem hiding this comment.
This review comment was translated automatically.
[Blocker][A4] Directly union-ing the requestIds from the renderer into the assistant's KB scope turns the assistant's static binding boundary into one that relies solely on UI constraints. Failure path: an assistant for a certain topic is only bound to kb-public, but the renderer/IPC request can send knowledgeBaseIds: ["kb-private"]; this function would return kb-public + kb-private, and after fixing the tool selection above, the model could access unbound private knowledge bases via kb_search/kb_read (kb_manage would also fall into the approvable write tool scope). The PR description mentions that the composer UI limits the selection range when the assistant has configured KBs, but the main-side IPC contract cannot trust this UI filtering. Minimum fix: when assistant.knowledgeBaseIds is non-empty, filter requestIds to that collection (or directly return the assistant scope, keeping the union from shrinking the default range); only allow request ids to expand the scope in scenarios where the assistant has no KB binding / no assistant selected-only case, and add test coverage for out-of-scope request ids being dropped.
Original Content
[Blocker][A4] 这里直接把 renderer 传来的 requestIds union 进 assistant 的 KB scope，会把 assistant 静态绑定边界变成仅靠 UI 约束。失败路径：某个 topic 的 assistant 只绑定了 kb-public，但 renderer/IPC 请求可以发送 knowledgeBaseIds: ["kb-private"]；这个函数会返回 kb-public + kb-private，修复上面的 tool selection 后模型就能通过 kb_search/kb_read 访问未绑定的私有知识库（kb_manage 还会进入可审批的写工具范围）。PR 说明里提到 composer UI 会在 assistant 已配置 KB 时限制选择范围，但 main 侧 IPC 契约不能信任这个 UI 过滤。最小修复：当 assistant.knowledgeBaseIds 非空时，把 requestIds 过滤到该集合（或直接返回 assistant scope，保持 union 不缩小默认范围）；只有 assistant 未绑定 KB / 无 assistant 的 selected-only 场景才允许 request ids 扩大 scope，并加测试覆盖 out-of-scope request id 被丢弃。
resolveTools() computed the effective knowledgeBaseIds but never passed it into registry.selectActive(), so all kb_* tools' applies predicates always saw an empty scope — a regression breaking even assistant-bound knowledge bases. Separately, resolveKnowledgeBaseIds() unioned the assistant's static binding with the renderer-supplied request ids, letting an IPC request expand a tool's reach beyond what the assistant was configured for. Thread knowledgeBaseIds through resolveTools() into selectActive(), and make the assistant's static binding authoritative (not unioned) whenever it is non-empty, falling back to the request-supplied ids only when the assistant has no binding. Update the six sibling doc comments this PR introduced that still described the old union semantics. Addresses review feedback on PR #16743. Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
|
Thanks for the review — both Blockers were real, confirmed against current HEAD, and now fixed in ede850d. Fixed
Both fixes are pinned by new tests in
|
There was a problem hiding this comment.
This review was translated automatically.
The current implementation direction has corrected the previous two blockers; there is only one contract comment remaining that is inconsistent with the new trust-boundary semantics, and it is recommended to update it accordingly.
Original Content
当前实现方向已经修正了前面两个 blocker；这里只剩一处契约注释与新 trust-boundary 语义不一致，建议同步更新。
| topicId: string | ||
| /** UniqueModelIds selected by the composer model selector — Main dispatches one execution per model. */ | ||
| mentionedModelIds?: UniqueModelId[] | ||
| /** Knowledge bases selected via the composer `/` picker for this turn — unioned with the assistant's own bound bases. */ |
There was a problem hiding this comment.
This review comment was translated automatically.
[C6] The contract comment here still describes the old union semantics, but the current resolveKnowledgeBaseIds() has been changed so that when an assistant has static binding, the assistant scope takes precedence, and only when there is no static binding does it use the composer's current selection. I suggest changing "unioned with the assistant bound bases" here to reflect the current rules; similar outdated descriptions also exist in the AiBaseRequest.knowledgeBaseIds comment in src/main/ai/types/requests.ts, and in the file header of src/main/ai/tools/knowledgeLookup.ts where it says "AI-SDK path uses assistant knowledgeBaseIds". Otherwise, future callers may mistakenly think that KB ids passed from the renderer can extend the assistant scope.
Original Content
[C6] 这里的契约注释还在描述旧的 union 语义，但当前 resolveKnowledgeBaseIds() 已经改成 assistant 有静态绑定时以 assistant scope 为准，只有无静态绑定时才使用 composer 本轮选择。建议把这里的 “unioned with the assistant bound bases” 改成当前规则；同类旧描述也还在 src/main/ai/types/requests.ts 的 AiBaseRequest.knowledgeBaseIds 注释，以及 src/main/ai/tools/knowledgeLookup.ts 文件头里 “AI-SDK path uses assistant knowledgeBaseIds” 的说明。否则后续调用方会误以为 renderer 传入的 KB id 可以扩展 assistant scope。
…horitative-scope rule Three more contract comments (AiStreamOpenRequest.knowledgeBaseIds, AiBaseRequest.knowledgeBaseIds, and the knowledgeLookup.ts file header) still described the old union semantics that the prior fix removed — misleading a future reader into thinking a renderer-supplied id can expand an assistant's bound scope. Reworded all three to match resolveKnowledgeBaseIds's actual either/or rule, using consistent wording across all three sites. Addresses review feedback on PR #16743. Signed-off-by: eeee0717 <chentao020717Work@outlook.com>
|
Thanks — fixed in adbc8b6. Fixed
A Typecheck is clean; this is a doc-only change so no new tests were needed. |
eeee0717
added
the
ready-to-merge
There was a problem hiding this comment.
This review was translated automatically.
LGTM
Scope: Integrate request-level knowledgeBaseIds into AI stream transport, persistent chat context, agent params, and kb_* tool registry, and use the resolved knowledge base scope uniformly during tool selection/execution; add context and registry tests.
Impact: When knowledge bases are passed manually, assistants without a bound static knowledge base will expose/execute knowledge base tools according to the request scope; assistants with a bound static knowledge base continue to use the static binding. The previous two blockers have been handled in the current head, and no new blockers/warnings were found.
Original Content
LGTM
范围：把请求级 knowledgeBaseIds 接入 AI stream transport、persistent chat context、agent params 和 kb_* tool registry，并在工具选择/执行时统一使用解析后的知识库 scope；补充上下文和 registry 测试。
影响：手动传入知识库时，未绑定静态知识库的 assistant 会按请求 scope 暴露/执行知识库工具；已绑定静态知识库的 assistant 继续以静态绑定为准。之前两个 blocker 已在当前 head 处理，未发现新的 blocker/warning。
5 participants
What this PR does
Before this PR: when the active assistant had no knowledge base statically bound, selecting one via the composer's
/picker had no effect — theai.stream_openrequest had no field to carry that per-turn selection, and thekb_search/kb_list/kb_read/kb_managetools' exposure gate only checked the assistant's staticknowledgeBaseIds. The model never even saw the tools as available, so it could not answer questions about the selected knowledge base.After this PR: the composer's per-turn selection is threaded through
AiStreamOpenRequest→PersistentChatContextProvider→buildAgentParams, unioned with the assistant's statically bound bases, and used to gate/scope the fourkb_*tools. Selecting a knowledge base via/now makes it searchable for that turn regardless of the assistant's static binding.Fixes #
Why we need it and why it was done in this way
The following tradeoffs were made: the per-turn selection is unioned with the assistant's bound bases rather than overriding them (unlike the existing
mcpToolIds"caller wins" pattern). The composer UI already restricts per-turn picks to a subset of the assistant's bound bases whenever any are configured, so union and override are equivalent in every reachable state today — union is simply safer against a future caller that passes an empty array.The following alternatives were considered: mirroring the
mcpToolIdscaller-wins override; rejected because it could unintentionally shrink the assistant's default scope.Links to places where the discussion took place: N/A
Breaking changes
None.
Special notes for your reviewer
RequestContext.knowledgeBaseIds/ToolApplyScope.knowledgeBaseIdsare optional (default[]) rather than required, so unrelated tests that construct minimal scope objects (registry/meta-tool tests) didn't need touching.Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
mainfor active development,v1for v1 maintenance fixes/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note