From ff2620dfcfbb62d9b13513f02a1b578092ff9a20 Mon Sep 17 00:00:00 2001 From: matevip Date: Tue, 26 May 2026 14:31:07 +0800 Subject: [PATCH] =?UTF-8?q?fix(wiki):=20harden=20kbName/kbId=20routing=20?= =?UTF-8?q?=E2=80=94=20ambiguous=20fail-closed,=20kbId=20param,=20prompt?= =?UTF-8?q?=20cleanup=20(#224)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../mate/wiki/service/WikiContextService.java | 35 +- .../service/WikiKnowledgeBaseService.java | 38 ++- .../java/vip/mate/wiki/tool/WikiTool.java | 318 ++++++++++++------ .../wiki/service/WikiContextServiceTest.java | 82 +++++ .../service/WikiKnowledgeBaseServiceTest.java | 58 ++++ .../wiki/tool/WikiToolKbNameRoutingTest.java | 166 +++++++-- .../wiki/tool/WikiToolSpringBindingTest.java | 163 +++++++++ 7 files changed, 714 insertions(+), 146 deletions(-) create mode 100644 mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolSpringBindingTest.java diff --git a/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiContextService.java b/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiContextService.java index e2969a18..a46d2a83 100644 --- a/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiContextService.java +++ b/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiContextService.java @@ -138,25 +138,29 @@ public class WikiContextService { int totalChars = 0; int maxChars = properties.getMaxContextChars(); - // Every page is rendered under its KB heading so the agent can see - // which KB each slug lives in. This becomes load-bearing when more - // than one KB is visible: wiki tools accept an optional `kbName` - // argument, and the LLM is expected to copy the heading text into - // that argument when reaching for a slug outside the primary KB. - // Without the heading the agent would call wiki_read_page(slug=...) - // without kbName and silently miss the slug — that's the surface of - // issue #224 mapped into the prompt-side context. + // Each KB renders as a HEADING-ONLY block (### ) followed by a + // metadata line and its page list. The heading deliberately contains + // ONLY the KB name — no em-dash, no parenthesised page count, no + // description — so the LLM can safely copy the entire post-### text + // verbatim into the `kbName` tool argument. The previous form + // "### — (N pages)" let the LLM paste the + // whole row into kbName and break the exact-match lookup. boolean multipleKbs = kbs.size() > 1; for (WikiKnowledgeBaseEntity kb : kbs) { List pages = pageService.listSummaries(kb.getId()); if (pages.isEmpty()) continue; - sb.append("### ").append(kb.getName()); + // Heading: pure KB name. This is what `kbName` expects verbatim. + sb.append("### ").append(kb.getName()).append("\n"); + // Metadata line: page count first (easy to scan), then optional + // description. Lives on its own line so it can't be confused for + // part of the name. + sb.append(pages.size()).append(" pages"); if (kb.getDescription() != null && !kb.getDescription().isBlank()) { sb.append(" — ").append(kb.getDescription()); } - sb.append(" (").append(pages.size()).append(" pages)\n\n"); + sb.append("\n\n"); boolean compact = pages.size() > 20; @@ -183,11 +187,12 @@ public class WikiContextService { sb.append("Use wiki_read_page(slug) for details. Use wiki_search_pages(query) to search.\n"); if (multipleKbs) { - sb.append("Multiple knowledge bases visible — every wiki tool takes an optional ") - .append("`kbName` argument (the heading text above). Pass it when the slug ") - .append("you want lives outside the agent's primary KB; otherwise the tool ") - .append("falls back to the primary and may return 'page not found'. Call ") - .append("wiki_list_kbs first if unsure which KB to target.\n"); + sb.append("Multiple knowledge bases visible — every wiki tool takes an ") + .append("optional `kbName` argument. Set it to the EXACT text after ") + .append("`### ` on the heading line (do NOT include the page count or ") + .append("description). When two KBs share a name, call wiki_list_kbs ") + .append("and pass `kbId` instead. Omit both and the tool falls back to ") + .append("the agent's primary KB, which may return 'page not found'.\n"); } sb.append(""); diff --git a/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiKnowledgeBaseService.java b/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiKnowledgeBaseService.java index e82866fb..08448118 100644 --- a/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiKnowledgeBaseService.java +++ b/mateclaw-server/src/main/java/vip/mate/wiki/service/WikiKnowledgeBaseService.java @@ -146,16 +146,46 @@ public class WikiKnowledgeBaseService { *

* Match is exact and case-sensitive — the LLM is expected to copy the * name verbatim from {@code wiki_list_kbs} output. Returns {@code null} - * when no visible KB has that name, so callers can surface a "use - * wiki_list_kbs to discover names" hint instead of silently falling back - * to the primary KB (which would mask a bad pick). + * when zero OR more than one KB matches the name; callers wanting to + * distinguish the two cases (so the LLM can be told to disambiguate by + * id) should call {@link #findAllByName} instead. The single-match + * convenience contract here keeps the legacy call sites simple. */ public WikiKnowledgeBaseEntity findByName(Long agentId, String kbName) { + List matches = findAllByName(agentId, kbName); + return matches.size() == 1 ? matches.get(0) : null; + } + + /** + * All KBs visible to {@code agentId} whose name matches {@code kbName} + * exactly. Returns an empty list when the name is blank or no KB matches; + * returns >1 entries when the workspace has duplicate KB names (no DB + * unique constraint protects against this), in which case the caller + * MUST disambiguate (typically by surfacing a kbId-based picker to the + * LLM) rather than silently picking the first one. + */ + public List findAllByName(Long agentId, String kbName) { if (kbName == null || kbName.isBlank()) { - return null; + return List.of(); } + List out = new java.util.ArrayList<>(); for (WikiKnowledgeBaseEntity kb : listByAgentId(agentId)) { if (kbName.equals(kb.getName())) { + out.add(kb); + } + } + return out; + } + + /** + * Resolve a KB by id, but ONLY when it's in the agent's visibility set — + * a deliberate fail-closed gate so an LLM cannot pivot to an arbitrary KB + * by guessing or scraping an id from someone else's workspace. + */ + public WikiKnowledgeBaseEntity findVisibleById(Long agentId, Long kbId) { + if (kbId == null) return null; + for (WikiKnowledgeBaseEntity kb : listByAgentId(agentId)) { + if (kbId.equals(kb.getId())) { return kb; } } diff --git a/mateclaw-server/src/main/java/vip/mate/wiki/tool/WikiTool.java b/mateclaw-server/src/main/java/vip/mate/wiki/tool/WikiTool.java index 4b1d5dbc..f9518cc6 100644 --- a/mateclaw-server/src/main/java/vip/mate/wiki/tool/WikiTool.java +++ b/mateclaw-server/src/main/java/vip/mate/wiki/tool/WikiTool.java @@ -31,7 +31,14 @@ import java.util.stream.Collectors; /** * Wiki knowledge base tools for agent conversations. *

- * All tools auto-resolve kbId from agentId; LLM never needs to pass it directly. + * Every tool resolves its target KB through a small precedence ladder: + * an explicit {@code kbId} from {@code wiki_list_kbs} wins outright, then + * an explicit {@code kbName} (with fail-closed "ambiguous"/"not visible" + * errors when needed), and only when both are absent does the tool fall + * back to the agent's primary KB. The {@code kbId} surface is public so + * the LLM can disambiguate duplicate-named KBs — see + * {@link #wiki_list_kbs} and {@code WikiKnowledgeBaseService.findVisibleById} + * for the visibility gate. * * @author MateClaw Team */ @@ -90,19 +97,21 @@ public class WikiTool { bound to the agent and shared workspace-level KBs. Every other wiki tool (read / list / search / semantic search / …) - accepts an OPTIONAL `kbName` parameter. When omitted, the tool falls - back to the agent's "primary" KB (the agent-bound KB if any, else - the most recently updated shared KB), which is fine when the agent - only reaches one KB. When the agent reaches more than one and the - data you need lives in a non-primary KB, call wiki_list_kbs first - and pass the chosen `name` as `kbName` so the tool actually queries - the right KB instead of silently hitting the primary one. + accepts two OPTIONAL routing arguments. Use them in this order: + 1. `kbName` — readable name from the output below. Easiest. + 2. `kbId` — numeric id from the output below. Use this when + two KBs share the same name and `kbName` returns + an "Ambiguous kbName" error. + Omit both and the tool falls back to the agent's primary KB, + which is fine when the agent only reaches one KB. Output fields per KB: - - name — copy verbatim into other tools' `kbName` param + - kbId — string-encoded numeric id (use as `kbId` param) + - name — copy verbatim into `kbName` param - description — operator-supplied summary - pageCount — number of pages currently in the KB - - isPrimary — true for the KB used when `kbName` is omitted + - isPrimary — true for the KB used when both routing + arguments are omitted - boundToAgent — true if the KB is explicitly bound to this agent """) public String wiki_list_kbs( @@ -113,7 +122,14 @@ public class WikiTool { JSONArray arr = new JSONArray(); for (WikiKnowledgeBaseEntity kb : kbs) { + // kbId is rendered as a STRING per the workspace-wide Snowflake- + // precision rule: a 19-digit id round-tripped through a JSON + // number loses its last 2-3 digits whenever it touches a JS + // runtime. The LLM hands the value back to us through a Java + // Long @ToolParam, which is precision-safe, so the lossy hop + // is purely defensive. arr.add(JSONUtil.createObj() + .set("kbId", String.valueOf(kb.getId())) .set("name", kb.getName()) .set("description", kb.getDescription()) .set("pageCount", kb.getPageCount() == null ? 0 : kb.getPageCount()) @@ -140,16 +156,16 @@ public class WikiTool { @ToolParam(description = "Page slug") String slug, @ToolParam(description = "Max characters to return (null = full page)", required = false) Integer maxChars, @ToolParam(description = "Section heading to extract (null = all sections)", required = false) String sectionHeading, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (slug == null || slug.isBlank()) { return error("slug is required"); } - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) { - return noKbError(kbName); - } + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); WikiPageEntity page = pageService.getBySlug(kbId, slug); if (page == null) { @@ -186,12 +202,12 @@ public class WikiTool { public String wiki_list_pages( @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Title keyword filter (optional)", required = false) String query, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) { - return noKbError(kbName); - } + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); List pages; if (query != null && !query.isBlank()) { @@ -244,16 +260,16 @@ public class WikiTool { @ToolParam(description = "Search query") String query, @ToolParam(description = "Mode: keyword|semantic|hybrid (default: hybrid)", required = false) String mode, @ToolParam(description = "Max results (default 5, max 20)", required = false) Integer topK, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (query == null || query.isBlank()) { return error("query is required"); } - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) { - return noKbError(kbName); - } + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); int k = (topK != null && topK > 0) ? Math.min(topK, 20) : 5; List results = hybridRetriever.search(kbId, query, mode, k); @@ -294,16 +310,16 @@ public class WikiTool { @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Natural language query") String query, @ToolParam(description = "Max results (default 5)", required = false) Integer topK, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (query == null || query.isBlank()) { return error("query is required"); } - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) { - return noKbError(kbName); - } + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); int k = (topK != null && topK > 0) ? Math.min(topK, 20) : 5; List hits = hybridRetriever.searchChunks(kbId, query, k); @@ -358,16 +374,16 @@ public class WikiTool { public String wiki_trace_source( @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Page slug") String slug, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (slug == null || slug.isBlank()) { return error("slug is required"); } - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) { - return noKbError(kbName); - } + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); WikiPageEntity page = pageService.getBySlug(kbId, slug); if (page == null) { @@ -389,7 +405,8 @@ public class WikiTool { @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Page title") String title, @ToolParam(description = "Page content (Markdown)") String content, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (title == null || title.isBlank()) { return error("title is required"); @@ -398,10 +415,9 @@ public class WikiTool { return error("content is required"); } - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) { - return noKbError(kbName); - } + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); String slug = title.toLowerCase() .replaceAll("[^a-z0-9\\u4e00-\\u9fff]+", "-") @@ -443,13 +459,15 @@ public class WikiTool { @ToolParam(description = "Topic to compile a page about (natural language)") String topic, @ToolParam(description = "Optional explicit slug for the page", required = false) String slug, @ToolParam(description = "Max evidence chunks (default 8, max 20)", required = false) Integer maxEvidenceChunks, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (topic == null || topic.isBlank()) { return error("topic is required"); } - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (compileService == null) return error("Compile service not available"); try { @@ -491,11 +509,13 @@ public class WikiTool { @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Comma-separated slugs (max 10)") String slugs, @ToolParam(description = "Max chars returned per page (default 2000, max 8000)", required = false) Integer maxCharsPerPage, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (slugs == null || slugs.isBlank()) return error("slugs is required"); - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); int cap = (maxCharsPerPage == null || maxCharsPerPage <= 0) ? 2000 : Math.min(8000, maxCharsPerPage); List slugList = Arrays.stream(slugs.split(",")) @@ -537,8 +557,9 @@ public class WikiTool { public String wiki_archive_page( @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Page slug to archive") String slug, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { - return setArchivedTool(agentId, slug, true, "archived", kbName); + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { + return setArchivedTool(agentId, slug, true, "archived", kbName, kbId); } @Tool(description = """ @@ -548,14 +569,16 @@ public class WikiTool { public String wiki_unarchive_page( @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Page slug to unarchive") String slug, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { - return setArchivedTool(agentId, slug, false, "unarchived", kbName); + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { + return setArchivedTool(agentId, slug, false, "unarchived", kbName, kbId); } - private String setArchivedTool(Long agentId, String slug, boolean archive, String verb, String kbName) { + private String setArchivedTool(Long agentId, String slug, boolean archive, String verb, String kbName, Long kbId) { if (slug == null || slug.isBlank()) return error("slug is required"); - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); boolean changed; try { changed = pageService.setArchived(kbId, slug, archive); @@ -576,16 +599,16 @@ public class WikiTool { public String wiki_delete_page( @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Page slug to delete") String slug, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (slug == null || slug.isBlank()) { return error("slug is required"); } - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) { - return noKbError(kbName); - } + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); WikiPageEntity page = pageService.getBySlug(kbId, slug); if (page == null) { @@ -624,10 +647,12 @@ public class WikiTool { @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Page slug") String slug, @ToolParam(description = "Max results (default 5, max 10)", required = false) Integer topK, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (relationService == null) return error("Relation service not available"); int k = (topK != null && topK > 0) ? Math.min(topK, 10) : 5; @@ -656,10 +681,12 @@ public class WikiTool { @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "First page slug") String slugA, @ToolParam(description = "Second page slug") String slugB, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (relationService == null) return error("Relation service not available"); RelationExplanation ex = relationService.explain(kbId, slugA, slugB); @@ -681,10 +708,12 @@ public class WikiTool { public String wiki_enrich_page( @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Page slug") String slug, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (jobService == null || eventPublisher == null) return error("Job service not available"); WikiPageEntity page = pageService.getBySlug(kbId, slug); @@ -712,9 +741,11 @@ public class WikiTool { """) public String wiki_list_transformations( @ToolParam(description = "Agent ID") Long agentId, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (transformationService == null) return error("Transformations not available"); WikiKnowledgeBaseEntity kb = kbService.getById(kbId); @@ -742,11 +773,13 @@ public class WikiTool { @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Transformation name (from wiki_list_transformations)") String name, @ToolParam(description = "Raw material ID to run the transformation against") Long rawId, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (name == null || name.isBlank()) return error("name is required"); if (rawId == null) return error("rawId is required"); - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (transformationService == null || transformationExecutor == null) { return error("Transformations not available"); } @@ -788,11 +821,13 @@ public class WikiTool { @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Transformation name (from wiki_list_transformations)") String name, @ToolParam(description = "Source wiki page slug to run the transformation against") String slug, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (name == null || name.isBlank()) return error("name is required"); if (slug == null || slug.isBlank()) return error("slug is required"); - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (transformationService == null || transformationExecutor == null) { return error("Transformations not available"); } @@ -838,10 +873,12 @@ public class WikiTool { public String wiki_aggregate_transformation( @ToolParam(description = "Agent ID") Long agentId, @ToolParam(description = "Transformation name (from wiki_list_transformations)") String name, - @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB.", required = false) String kbName) { + @ToolParam(description = "Target knowledge base name (from wiki_list_kbs). Omit to use the agent's primary KB; switch to `kbId` when two KBs share the name.", required = false) String kbName, + @ToolParam(description = "Numeric KB id from wiki_list_kbs. Use when `kbName` returns an ambiguous-name error.", required = false) Long kbId) { if (name == null || name.isBlank()) return error("name is required"); - Long kbId = resolveKbId(agentId, kbName); - if (kbId == null) return noKbError(kbName); + KbResolution kbRes = resolveKb(agentId, kbName, kbId); + if (kbRes.hasError()) return kbRes.errorJson(); + kbId = kbRes.kbId(); if (transformationService == null || transformationAggregator == null) { return error("Transformations not available"); } @@ -880,37 +917,124 @@ public class WikiTool { // ==================== Helpers ==================== private Long resolveKbId(Long agentId) { - return resolveKbId(agentId, null); + return resolveKbId(agentId, null, null); } /** - * Resolve the KB a tool call should operate on, honouring an optional - * caller-supplied {@code kbName}. + * Outcome of resolving a KB for a tool call. Exactly one of + * {@code kbId} / {@code errorJson} is non-null: *

    - *
  • {@code kbName} blank → existing single-primary fallback - * ({@link WikiKnowledgeBaseService#resolvePrimaryKb}). Preserves - * the legacy single-KB UX for agents with only one accessible KB.
    • - *
  • {@code kbName} provided → exact-name lookup restricted to the - * agent's visible KBs. Returns {@code null} when the name does - * not match a visible KB so the caller can emit a "use - * wiki_list_kbs" hint instead of silently routing to the primary - * (which is the surface of issue #224 — LLM picks a non-primary - * KB but the tool still hits the primary one).
    • + *
  • {@code kbId} present → caller proceeds with that KB.
    • + *
  • {@code errorJson} present → caller returns it as-is so the LLM + * sees an unambiguous error pointing at the next action + * (call {@code wiki_list_kbs} / pick a different name / pass + * {@code kbId}).
    • *
*/ - private Long resolveKbId(Long agentId, String kbName) { - if (kbName == null || kbName.isBlank()) { - WikiKnowledgeBaseEntity primary = kbService.resolvePrimaryKb(agentId); - return primary == null ? null : primary.getId(); + private record KbResolution(Long kbId, String errorJson) { + static KbResolution ok(Long id) { return new KbResolution(id, null); } + static KbResolution err(String json) { return new KbResolution(null, json); } + boolean hasError() { return errorJson != null; } + } + + /** + * Single helper every wiki tool uses. Caller passes the agent id and at + * most one of {@code kbId} / {@code kbName}; the helper decides which + * KB the operation runs against and emits a uniform error when no + * unambiguous target can be picked. + * + *

Resolution rules (in order): + *

    + *
  1. {@code kbId} non-null → resolve only via + * {@link WikiKnowledgeBaseService#findVisibleById}. Out-of-visibility + * ids fail closed — no silent fallback to the primary or to a + * same-name shared KB.
    2. + *
  2. {@code kbName} non-blank → look up every visible KB with that + * name. Single match → use it. Zero match → fail closed pointing + * at {@code wiki_list_kbs}. Multiple matches → fail closed with + * the list of candidate {@code kbId}s, telling the LLM to retry + * with {@code kbId}.
    3. + *
  3. Both blank → fall back to + * {@link WikiKnowledgeBaseService#resolvePrimaryKb} so single-KB + * agents keep their old zero-config behaviour.
    4. + *
+ */ + private KbResolution resolveKb(Long agentId, String kbName, Long kbId) { + // Treat kbId<=0 as "not supplied". Spring AI's @Tool JSON-schema + // generator doesn't carry the "optional, may be absent" semantic + // through to the LLM in the way Java would expect a nullable Long, + // so the model frequently fills unused numeric optionals with 0 + // ("openai-chatgpt" was observed doing this on every wiki call). + // Real Snowflake ids are always 19-digit positive longs, so + // {0, negative} can be safely treated as the empty case. + if (kbId != null && kbId > 0L) { + WikiKnowledgeBaseEntity byId = kbService.findVisibleById(agentId, kbId); + if (byId == null) { + return KbResolution.err(error( + "Knowledge base id=" + kbId + " not visible to this agent. " + + "Use wiki_list_kbs to see available KBs.")); + } + return KbResolution.ok(byId.getId()); } - WikiKnowledgeBaseEntity match = kbService.findByName(agentId, kbName); - return match == null ? null : match.getId(); + if (kbName != null && !kbName.isBlank()) { + List matches = kbService.findAllByName(agentId, kbName); + if (matches.isEmpty()) { + return KbResolution.err(error( + "Knowledge base '" + kbName + "' not visible to this agent. " + + "Use wiki_list_kbs to see available KBs.")); + } + if (matches.size() > 1) { + // Duplicate names exist (no DB unique constraint). The LLM + // cannot disambiguate from kbName alone — surface every + // candidate's id (as String per the Snowflake-precision + // contract) and demand a kbId retry. + JSONArray candidates = new JSONArray(); + for (WikiKnowledgeBaseEntity kb : matches) { + candidates.add(JSONUtil.createObj() + .set("kbId", String.valueOf(kb.getId())) + .set("name", kb.getName()) + .set("description", kb.getDescription()) + .set("boundToAgent", kb.getAgentId() != null)); + } + JSONObject obj = JSONUtil.createObj() + .set("error", "Ambiguous kbName '" + kbName + "' — " + + matches.size() + " visible KBs share this name. " + + "Retry with `kbId` from the candidates list below.") + .set("candidates", candidates); + return KbResolution.err(obj.toString()); + } + return KbResolution.ok(matches.get(0).getId()); + } + WikiKnowledgeBaseEntity primary = kbService.resolvePrimaryKb(agentId); + if (primary == null) { + return KbResolution.err(error("No wiki knowledge base found for this agent")); + } + return KbResolution.ok(primary.getId()); + } + + /** + * Legacy 2-arg routing kept for the tool methods that haven't been + * widened to accept {@code kbId} yet. Always returns null when the + * resolution would have surfaced an error — callers turn that into a + * {@link #noKbError(String)} message. + */ + private Long resolveKbId(Long agentId, String kbName) { + return resolveKbId(agentId, kbName, null); + } + + private Long resolveKbId(Long agentId, String kbName, Long kbId) { + KbResolution res = resolveKb(agentId, kbName, kbId); + return res.hasError() ? null : res.kbId(); } /** * Standardised "couldn't resolve KB" error. When the caller passed a * non-blank {@code kbName} that didn't match, the message points them at * {@code wiki_list_kbs} so the LLM has a clear next step. + * + *

NOTE: ambiguous-kbName errors are emitted directly by + * {@link #resolveKb} so the LLM also gets the candidate list, not just + * a flat string. This helper handles the simpler "not visible" case. */ private String noKbError(String kbName) { if (kbName != null && !kbName.isBlank()) { diff --git a/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiContextServiceTest.java b/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiContextServiceTest.java index 547d15ad..635ac044 100644 --- a/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiContextServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiContextServiceTest.java @@ -6,6 +6,7 @@ import org.junit.jupiter.api.Test; import vip.mate.wiki.WikiProperties; import vip.mate.wiki.dto.PageSearchResult; import vip.mate.wiki.model.WikiKnowledgeBaseEntity; +import vip.mate.wiki.model.WikiPageEntity; import java.util.List; @@ -127,4 +128,85 @@ class WikiContextServiceTest { return PageSearchResult.of(slug, slug, snippet, snippet, List.of("keyword"), null, score); } + + // ==================== buildWikiContext heading + hint format ==================== + // + // These tests lock in the unambiguous heading layout: heading text after + // `### ` MUST equal the KB name verbatim and nothing more, so the LLM + // can safely copy it into the `kbName` tool argument. The previous form + // "### {name} — {description} ({N} pages)" let the LLM paste the entire + // row and break findByName's exact-match lookup. The multi-KB hint must + // also call out kbId as the disambiguator for duplicate names. + + private static WikiKnowledgeBaseEntity kbWithName(long id, String name, String description, Long agentId) { + WikiKnowledgeBaseEntity kb = new WikiKnowledgeBaseEntity(); + kb.setId(id); + kb.setName(name); + kb.setDescription(description); + kb.setAgentId(agentId); + kb.setPageCount(0); + return kb; + } + + private static WikiPageEntity simplePage(String slug, String title, String summary) { + WikiPageEntity p = new WikiPageEntity(); + p.setSlug(slug); + p.setTitle(title); + p.setSummary(summary); + p.setPageType("user"); + return p; + } + + @Test + @DisplayName("buildWikiContext heading is JUST the KB name — no description, no page count") + void buildWikiContextHeadingIsBareName() { + WikiKnowledgeBaseEntity kb = kbWithName(100L, "QA-Bug-Test KB", + "A KB created via UI E2E test to surface wiki bugs", null); + when(kbService.listByAgentId(1L)).thenReturn(List.of(kb)); + when(pageService.listSummaries(100L)).thenReturn(List.of( + simplePage("mateclaw", "MateClaw", "Entry page"))); + + String out = service.buildWikiContext(1L); + + // Heading line is exact — pasting this into kbName must work without trim/strip. + assertThat(out).contains("### QA-Bug-Test KB\n"); + // Description and page count live on the next line, not in the heading. + assertThat(out).doesNotContain("### QA-Bug-Test KB —"); + assertThat(out).doesNotContain("### QA-Bug-Test KB ("); + assertThat(out).contains("1 pages — A KB created via UI E2E test"); + } + + @Test + @DisplayName("buildWikiContext multi-KB hint mentions kbName + kbId + wiki_list_kbs") + void buildWikiContextMultiKbHint() { + when(kbService.listByAgentId(1L)).thenReturn(List.of( + kbWithName(100L, "Alpha", null, null), + kbWithName(200L, "Beta", null, null))); + when(pageService.listSummaries(100L)).thenReturn(List.of(simplePage("a", "A", null))); + when(pageService.listSummaries(200L)).thenReturn(List.of(simplePage("b", "B", null))); + + String out = service.buildWikiContext(1L); + + // Hint must point the LLM at the right argument and at the + // disambiguator for duplicate names. + assertThat(out) + .contains("kbName") + .contains("kbId") + .contains("wiki_list_kbs") + .contains("EXACT text after `### `"); + } + + @Test + @DisplayName("buildWikiContext single-KB output omits the multi-KB hint") + void buildWikiContextSingleKbSkipsHint() { + WikiKnowledgeBaseEntity kb = kbWithName(100L, "Solo", null, null); + when(kbService.listByAgentId(1L)).thenReturn(List.of(kb)); + when(pageService.listSummaries(100L)).thenReturn(List.of(simplePage("a", "A", null))); + + String out = service.buildWikiContext(1L); + + // The "multiple knowledge bases" hint is wasted prompt budget when + // there's only one KB; it must stay off. + assertThat(out).doesNotContain("Multiple knowledge bases visible"); + } } diff --git a/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiKnowledgeBaseServiceTest.java b/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiKnowledgeBaseServiceTest.java index 7713f89e..a965e820 100644 --- a/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiKnowledgeBaseServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/wiki/service/WikiKnowledgeBaseServiceTest.java @@ -122,4 +122,62 @@ class WikiKnowledgeBaseServiceTest { assertThat(service.findByName(7L, "")).isNull(); assertThat(service.findByName(7L, " ")).isNull(); } + + // ==================== findByName ambiguity + findAllByName + findVisibleById ==================== + // + // mate_wiki_knowledge_base has no unique constraint on name (one DB row + // per workspace + (name nullable + duplicates allowed) by design), so + // a non-blank kbName can match more than one visible KB. The single- + // result findByName must not silently pick "the first one" in that + // case — callers route through findAllByName + an ambiguous-error + // surface so the LLM is forced to disambiguate by kbId. + + @Test + @DisplayName("findByName returns null when more than one visible KB shares the name") + void findByNameAmbiguousReturnsNull() { + when(kbMapper.selectList(any())).thenReturn(List.of( + kb(100L, 7L, "Docs"), + kb(900L, null, "Docs"))); + + assertThat(service.findByName(7L, "Docs")) + .as("ambiguous matches collapse to null — caller must use findAllByName") + .isNull(); + } + + @Test + @DisplayName("findAllByName returns every visible KB sharing the name") + void findAllByNameReturnsAllMatches() { + when(kbMapper.selectList(any())).thenReturn(List.of( + kb(100L, 7L, "Docs"), + kb(900L, null, "Docs"), + kb(800L, null, "Other"))); + + List hits = service.findAllByName(7L, "Docs"); + assertThat(hits).hasSize(2); + assertThat(hits).extracting(WikiKnowledgeBaseEntity::getId).containsExactly(100L, 900L); + } + + @Test + @DisplayName("findAllByName returns empty for blank kbName") + void findAllByNameBlankReturnsEmpty() { + assertThat(service.findAllByName(7L, null)).isEmpty(); + assertThat(service.findAllByName(7L, " ")).isEmpty(); + } + + @Test + @DisplayName("findVisibleById returns the KB only when it is in the agent's visibility set") + void findVisibleByIdGate() { + when(kbMapper.selectList(any())).thenReturn(List.of( + kb(100L, 7L, "Bound KB"), + kb(900L, null, "Shared KB"))); + + // Visible: returned. + assertThat(service.findVisibleById(7L, 100L)).isNotNull(); + assertThat(service.findVisibleById(7L, 900L)).isNotNull(); + + // Not in visibility set: deliberate fail-closed gate so an LLM + // can't pivot to an arbitrary KB by guessing an id. + assertThat(service.findVisibleById(7L, 99999L)).isNull(); + assertThat(service.findVisibleById(7L, null)).isNull(); + } } diff --git a/mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolKbNameRoutingTest.java b/mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolKbNameRoutingTest.java index d09884a3..df3a435f 100644 --- a/mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolKbNameRoutingTest.java +++ b/mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolKbNameRoutingTest.java @@ -21,27 +21,31 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; /** - * Routing-level coverage for {@link WikiTool#resolveKbId(Long, String)} - * (exercised through the public {@code wiki_list_pages} and - * {@code wiki_list_kbs} surfaces). + * Routing-level coverage for the {@code kbName} / {@code kbId} resolver + * shared by every wiki tool (exercised through {@code wiki_list_pages} and + * {@code wiki_list_kbs}). * *

The behaviour these tests pin in place is the fix for the upstream * "single-KB collapse" bug: when an agent reaches more than one knowledge * base, every wiki tool used to silently operate on whichever KB the * primary-fallback picked, with no way for the LLM to target a different - * one. The {@code kbName} parameter and {@code wiki_list_kbs} discovery - * tool together close that gap. These tests verify: + * one. The fix shipped in three layers: *

    - *
  1. blank {@code kbName} → routes to the primary KB (legacy behaviour - * preserved for single-KB agents);
    2. - *
  2. named {@code kbName} that matches a visible KB → routes to that - * specific KB, not the primary;
    3. - *
  3. named {@code kbName} that doesn't match → fail-closed error that - * names the bad pick and points the agent at - * {@code wiki_list_kbs} so the next attempt can pick a valid name;
    4. - *
  4. {@code wiki_list_kbs} surfaces every visible KB along with the - * {@code isPrimary} / {@code boundToAgent} flags the LLM needs to - * decide which to target.
    5. + *
  5. blank {@code kbName} + blank {@code kbId} → still routes to the + * primary KB so the single-KB UX stays zero-config;
    6. + *
  6. named {@code kbName} that matches exactly one visible KB → routes + * to that KB, not the primary;
    7. + *
  7. named {@code kbName} that doesn't match any visible KB → fail-closed + * error naming the bad pick and pointing at {@code wiki_list_kbs};
    8. + *
  8. named {@code kbName} that matches MORE than one visible KB (the + * schema has no unique constraint on KB name) → fail-closed error + * listing every candidate's {@code kbId} so the LLM can retry via + * {@code kbId} instead;
    9. + *
  9. {@code kbId} provided → uses {@link WikiKnowledgeBaseService#findVisibleById} + * (visibility gate enforced; out-of-set ids fail closed);
    10. + *
  10. {@code wiki_list_kbs} surfaces every visible KB with {@code kbId} + * rendered as a String (workspace-wide Snowflake-precision rule), + * plus {@code isPrimary} / {@code boundToAgent} flags.
    11. *
*/ class WikiToolKbNameRoutingTest { @@ -49,6 +53,8 @@ class WikiToolKbNameRoutingTest { private static final Long AGENT = 7L; private static final long PRIMARY_KB = 100L; private static final long OTHER_KB = 200L; + private static final long DUP_BOUND_KB = 300L; + private static final long DUP_SHARED_KB = 400L; private final WikiPageService pageService = mock(WikiPageService.class); private final WikiKnowledgeBaseService kbService = mock(WikiKnowledgeBaseService.class); @@ -83,17 +89,21 @@ class WikiToolKbNameRoutingTest { page("primary-only-slug", "Primary KB Page"))); when(pageService.listSummaries(eq(OTHER_KB))).thenReturn(List.of( page("other-only-slug", "Other KB Page"))); + when(pageService.listSummaries(eq(DUP_BOUND_KB))).thenReturn(List.of( + page("dup-bound-slug", "Bound Docs Page"))); + when(pageService.listSummaries(eq(DUP_SHARED_KB))).thenReturn(List.of( + page("dup-shared-slug", "Shared Docs Page"))); } - // ==================== wiki_list_pages routing ==================== + // ==================== wiki_list_pages routing — happy paths ==================== @Test - @DisplayName("wiki_list_pages with blank kbName routes to the primary KB") - void blankKbNameRoutesToPrimary() { + @DisplayName("blank kbName + blank kbId routes to the primary KB") + void blankRoutesToPrimary() { wirePages(); when(kbService.resolvePrimaryKb(AGENT)).thenReturn(kb(PRIMARY_KB, "Primary", AGENT)); - String json = tool.wiki_list_pages(AGENT, null, null); + String json = tool.wiki_list_pages(AGENT, null, null, null); JSONObject obj = JSONUtil.parseObj(json); JSONArray pages = obj.getJSONArray("pages"); @@ -102,15 +112,15 @@ class WikiToolKbNameRoutingTest { } @Test - @DisplayName("wiki_list_pages with a known kbName routes to that KB instead of the primary") + @DisplayName("known kbName routes to that KB instead of the primary") void namedKbNameRoutesToNamedKb() { wirePages(); // Primary fallback still wired so the test would fail loudly if the // tool silently used it despite a non-blank kbName. when(kbService.resolvePrimaryKb(AGENT)).thenReturn(kb(PRIMARY_KB, "Primary", AGENT)); - when(kbService.findByName(AGENT, "Other")).thenReturn(kb(OTHER_KB, "Other", null)); + when(kbService.findAllByName(AGENT, "Other")).thenReturn(List.of(kb(OTHER_KB, "Other", null))); - String json = tool.wiki_list_pages(AGENT, null, "Other"); + String json = tool.wiki_list_pages(AGENT, null, "Other", null); JSONObject obj = JSONUtil.parseObj(json); JSONArray pages = obj.getJSONArray("pages"); @@ -119,28 +129,120 @@ class WikiToolKbNameRoutingTest { } @Test - @DisplayName("wiki_list_pages with an unknown kbName fails closed and names the bad pick") + @DisplayName("kbId routes through the visibility gate to that KB") + void kbIdRoutesViaVisibilityGate() { + wirePages(); + when(kbService.resolvePrimaryKb(AGENT)).thenReturn(kb(PRIMARY_KB, "Primary", AGENT)); + when(kbService.findVisibleById(AGENT, OTHER_KB)).thenReturn(kb(OTHER_KB, "Other", null)); + + String json = tool.wiki_list_pages(AGENT, null, null, OTHER_KB); + JSONObject obj = JSONUtil.parseObj(json); + + assertThat(obj.getJSONArray("pages").getJSONObject(0).getStr("slug")) + .isEqualTo("other-only-slug"); + } + + @Test + @DisplayName("kbId wins when both kbName and kbId are supplied") + void kbIdWinsOverKbName() { + wirePages(); + when(kbService.findVisibleById(AGENT, OTHER_KB)).thenReturn(kb(OTHER_KB, "Other", null)); + // Deliberately do NOT stub findAllByName — if the tool consulted + // kbName at all (or fell back to primary), the call would NPE. + + String json = tool.wiki_list_pages(AGENT, null, "anything", OTHER_KB); + JSONObject obj = JSONUtil.parseObj(json); + assertThat(obj.getJSONArray("pages").getJSONObject(0).getStr("slug")) + .isEqualTo("other-only-slug"); + } + + // ==================== fail-closed paths ==================== + + @Test + @DisplayName("unknown kbName fails closed and names the bad pick") void unknownKbNameFailsClosed() { - when(kbService.findByName(AGENT, "Bogus")).thenReturn(null); + when(kbService.findAllByName(AGENT, "Bogus")).thenReturn(List.of()); // Primary still mockable; the routing must NOT silently fall through. when(kbService.resolvePrimaryKb(AGENT)).thenReturn(kb(PRIMARY_KB, "Primary", AGENT)); - String json = tool.wiki_list_pages(AGENT, null, "Bogus"); + String json = tool.wiki_list_pages(AGENT, null, "Bogus", null); JSONObject obj = JSONUtil.parseObj(json); - String err = obj.getStr("error"); - assertThat(err) + assertThat(obj.getStr("error")) .as("error message should name the bad pick and point at wiki_list_kbs") .contains("Bogus") .contains("wiki_list_kbs"); } @Test - @DisplayName("wiki_list_pages with no resolvable KB at all emits the no-KB error (legacy path)") + @DisplayName("ambiguous kbName fails closed and surfaces every candidate kbId") + void ambiguousKbNameFailsClosed() { + when(kbService.findAllByName(AGENT, "Docs")).thenReturn(List.of( + kb(DUP_BOUND_KB, "Docs", AGENT), + kb(DUP_SHARED_KB, "Docs", null))); + when(kbService.resolvePrimaryKb(AGENT)).thenReturn(kb(PRIMARY_KB, "Primary", AGENT)); + + String json = tool.wiki_list_pages(AGENT, null, "Docs", null); + JSONObject obj = JSONUtil.parseObj(json); + + // Error must be ambiguity-flavoured so the LLM knows to retry with kbId. + assertThat(obj.getStr("error")) + .contains("Ambiguous") + .contains("Docs") + .contains("kbId"); + + // Candidates list must carry both rows with stringified kbId. + JSONArray candidates = obj.getJSONArray("candidates"); + assertThat(candidates).hasSize(2); + assertThat(candidates.getJSONObject(0).getStr("kbId")) + .isEqualTo(String.valueOf(DUP_BOUND_KB)); + assertThat(candidates.getJSONObject(0).getBool("boundToAgent")).isTrue(); + assertThat(candidates.getJSONObject(1).getStr("kbId")) + .isEqualTo(String.valueOf(DUP_SHARED_KB)); + assertThat(candidates.getJSONObject(1).getBool("boundToAgent")).isFalse(); + } + + @Test + @DisplayName("kbId == 0 is treated as absent (LLM default for unused numeric optionals)") + void kbIdZeroTreatedAsAbsent() { + wirePages(); + when(kbService.resolvePrimaryKb(AGENT)).thenReturn(kb(PRIMARY_KB, "Primary", AGENT)); + // findVisibleById must NOT be consulted when kbId=0 — that path + // would return null and surface a spurious "kbId=0 not visible" error, + // which is exactly the production regression this test prevents. + + String json = tool.wiki_list_pages(AGENT, null, null, 0L); + JSONObject obj = JSONUtil.parseObj(json); + + assertThat(obj.getStr("error")) + .as("kbId=0 must fall through to primary, NOT raise a not-visible error") + .isNull(); + assertThat(obj.getJSONArray("pages").getJSONObject(0).getStr("slug")) + .isEqualTo("primary-only-slug"); + } + + @Test + @DisplayName("kbId outside the agent's visibility set fails closed") + void kbIdOutOfVisibilityFailsClosed() { + // Visibility gate returns null for an unrelated id. + when(kbService.findVisibleById(AGENT, 99999L)).thenReturn(null); + when(kbService.resolvePrimaryKb(AGENT)).thenReturn(kb(PRIMARY_KB, "Primary", AGENT)); + + String json = tool.wiki_list_pages(AGENT, null, null, 99999L); + JSONObject obj = JSONUtil.parseObj(json); + + assertThat(obj.getStr("error")) + .contains("99999") + .contains("not visible") + .contains("wiki_list_kbs"); + } + + @Test + @DisplayName("no resolvable KB at all emits the legacy no-KB error") void noResolvableKbReturnsLegacyError() { when(kbService.resolvePrimaryKb(AGENT)).thenReturn(null); - String json = tool.wiki_list_pages(AGENT, null, null); + String json = tool.wiki_list_pages(AGENT, null, null, null); JSONObject obj = JSONUtil.parseObj(json); assertThat(obj.getStr("error")).contains("No wiki knowledge base found"); @@ -149,7 +251,7 @@ class WikiToolKbNameRoutingTest { // ==================== wiki_list_kbs ==================== @Test - @DisplayName("wiki_list_kbs enumerates every visible KB with isPrimary / boundToAgent") + @DisplayName("wiki_list_kbs enumerates every visible KB with stringified kbId + flags") void wikiListKbsEnumeratesAll() { when(kbService.listByAgentId(AGENT)).thenReturn(List.of( kb(OTHER_KB, "Other", null), @@ -166,11 +268,15 @@ class WikiToolKbNameRoutingTest { assertThat(kbs).hasSize(2); JSONObject other = kbs.getJSONObject(0); + assertThat(other.getStr("kbId")) + .as("kbId must be a String to preserve Snowflake precision") + .isEqualTo(String.valueOf(OTHER_KB)); assertThat(other.getStr("name")).isEqualTo("Other"); assertThat(other.getBool("isPrimary")).isFalse(); assertThat(other.getBool("boundToAgent")).isFalse(); JSONObject primary = kbs.getJSONObject(1); + assertThat(primary.getStr("kbId")).isEqualTo(String.valueOf(PRIMARY_KB)); assertThat(primary.getStr("name")).isEqualTo("Primary"); assertThat(primary.getBool("isPrimary")).isTrue(); assertThat(primary.getBool("boundToAgent")).isTrue(); diff --git a/mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolSpringBindingTest.java b/mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolSpringBindingTest.java new file mode 100644 index 00000000..ae1c9d18 --- /dev/null +++ b/mateclaw-server/src/test/java/vip/mate/wiki/tool/WikiToolSpringBindingTest.java @@ -0,0 +1,163 @@ +package vip.mate.wiki.tool; + +import cn.hutool.json.JSONArray; +import cn.hutool.json.JSONObject; +import cn.hutool.json.JSONUtil; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.springframework.ai.support.ToolCallbacks; +import org.springframework.ai.tool.ToolCallback; +import vip.mate.wiki.model.WikiKnowledgeBaseEntity; +import vip.mate.wiki.model.WikiPageEntity; +import vip.mate.wiki.service.HybridRetriever; +import vip.mate.wiki.service.WikiKnowledgeBaseService; +import vip.mate.wiki.service.WikiPageService; +import vip.mate.wiki.service.WikiRawMaterialService; + +import java.util.Arrays; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Spring AI binding round-trip for the wiki tools' KB-routing contract. + * + *

The unit tests in {@link WikiToolKbNameRoutingTest} cover the Java-level + * routing logic, but the LLM never calls those methods directly — it serializes + * a JSON tool call which Spring AI's {@link ToolCallbacks#from} layer deserializes + * back into method arguments. Two coercion hops in that pipeline are easy to + * break unnoticed: + * + *

    + *
  1. {@code wiki_list_kbs} returns {@code "kbId": ""} as a JSON + * string (workspace-wide Snowflake-precision rule). When the LLM hands + * that exact string back as {@code kbId} on a follow-up call, the Java + * method declares {@code Long kbId} — so the framework must coerce + * string → Long without precision loss.
    2. + *
  2. OpenAI-style chat models frequently populate "unused numeric + * optionals" with {@code 0}. The routing layer treats {@code kbId > 0} + * as the only "supplied" sentinel; any binding change that lets a real + * 19-digit id collapse to 0 (e.g. silent float coercion) would also + * break the round-trip even though the unit tests still pass.
    3. + *
+ * + * These two tests pin the contract end-to-end. + */ +class WikiToolSpringBindingTest { + + private static final Long AGENT = 7L; + private static final long PRIMARY_KB = 100L; + // Real-shape Snowflake id — 19 digits, beyond JS Number.MAX_SAFE_INTEGER. + // Verifies the precision-safe round-trip the workspace rule mandates. + private static final long SNOWFLAKE_KB = 2054907618529591298L; + + private final WikiPageService pageService = mock(WikiPageService.class); + private final WikiKnowledgeBaseService kbService = mock(WikiKnowledgeBaseService.class); + private final WikiRawMaterialService rawService = mock(WikiRawMaterialService.class); + private final HybridRetriever hybridRetriever = mock(HybridRetriever.class); + private final ObjectMapper objectMapper = new ObjectMapper(); + + private final WikiTool tool = new WikiTool(pageService, kbService, rawService, + hybridRetriever, objectMapper); + + private ToolCallback callbackFor(String functionName) { + return Arrays.stream(ToolCallbacks.from(tool)) + .filter(cb -> functionName.equals(cb.getToolDefinition().name())) + .findFirst() + .orElseThrow(() -> new AssertionError("No ToolCallback for " + functionName)); + } + + private static WikiKnowledgeBaseEntity kb(long id, String name, Long agentId) { + WikiKnowledgeBaseEntity e = new WikiKnowledgeBaseEntity(); + e.setId(id); + e.setName(name); + e.setAgentId(agentId); + e.setPageCount(0); + return e; + } + + private static WikiPageEntity page(String slug, String title) { + WikiPageEntity p = new WikiPageEntity(); + p.setSlug(slug); + p.setTitle(title); + p.setPageType("user"); + return p; + } + + @Test + @DisplayName("wiki_list_kbs emits kbId as a JSON STRING and round-trips back through Long kbId") + void kbIdRoundTripsAsStringWithoutPrecisionLoss() { + // Arrange: one agent-bound KB whose id is a real-shape 19-digit + // Snowflake. wiki_list_kbs must surface this as a string so a JS + // hop never truncates it; the follow-up tool call then has to + // accept that same string and coerce it back to a Long without loss. + WikiKnowledgeBaseEntity snowflakeKb = kb(SNOWFLAKE_KB, "Big Data KB", AGENT); + when(kbService.listByAgentId(AGENT)).thenReturn(List.of(snowflakeKb)); + when(kbService.resolvePrimaryKb(AGENT)).thenReturn(snowflakeKb); + when(kbService.findVisibleById(AGENT, SNOWFLAKE_KB)).thenReturn(snowflakeKb); + when(pageService.listSummaries(eq(SNOWFLAKE_KB))).thenReturn(List.of( + page("only-page", "Only Page"))); + + // Step 1: wiki_list_kbs through the real Spring AI ToolCallback binding. + ToolCallback listKbs = callbackFor("wiki_list_kbs"); + String listJson = listKbs.call("{\"agentId\":" + AGENT + "}"); + JSONObject listObj = JSONUtil.parseObj(listJson); + + JSONArray kbs = listObj.getJSONArray("kbs"); + assertThat(kbs).hasSize(1); + JSONObject row = kbs.getJSONObject(0); + // kbId MUST be a JSON string. Reading it back via getStr should equal + // the exact 19-digit id; reading via getLong should also work (Hutool + // parses string-or-number). The two checks combined catch a regression + // that emits the id as a JSON number (which the LLM/JS hop would round). + String advertisedKbId = row.getStr("kbId"); + assertThat(advertisedKbId) + .as("wiki_list_kbs MUST publish kbId as a string") + .isEqualTo(String.valueOf(SNOWFLAKE_KB)); + assertThat(row.get("kbId")) + .as("the raw JSON node must be a String, not a Number") + .isInstanceOf(String.class); + + // Step 2: feed that exact string back to wiki_list_pages as kbId, + // exactly as an LLM tool call would. The framework must coerce + // String → Long with no precision loss, and the routing layer must + // resolve via findVisibleById (NOT fall back to primary). + ToolCallback listPages = callbackFor("wiki_list_pages"); + String pagesJson = listPages.call("{\"agentId\":" + AGENT + + ",\"kbId\":\"" + advertisedKbId + "\"}"); + JSONObject pagesObj = JSONUtil.parseObj(pagesJson); + + assertThat(pagesObj.getStr("error")) + .as("string-kbId round-trip must NOT raise a not-visible error") + .isNull(); + assertThat(pagesObj.getJSONArray("pages").getJSONObject(0).getStr("slug")) + .isEqualTo("only-page"); + } + + @Test + @DisplayName("kbId=0 from an LLM tool call falls through to primary instead of failing closed") + void kbIdZeroFromToolCallFallsThroughToPrimary() { + // The openai-chatgpt family was observed populating every unused + // numeric optional with 0 in tool-call JSON. The routing layer + // must treat that as "absent" — otherwise every wiki_* call + // surfaces a spurious "kbId=0 not visible" error. + WikiKnowledgeBaseEntity primary = kb(PRIMARY_KB, "Primary", AGENT); + when(kbService.resolvePrimaryKb(AGENT)).thenReturn(primary); + when(pageService.listSummaries(eq(PRIMARY_KB))).thenReturn(List.of( + page("primary-page", "Primary Page"))); + + ToolCallback listPages = callbackFor("wiki_list_pages"); + // Note: kbId arrives as JSON number 0 — the exact shape the + // production regression had. + String json = listPages.call("{\"agentId\":" + AGENT + ",\"kbId\":0}"); + JSONObject obj = JSONUtil.parseObj(json); + + assertThat(obj.getStr("error")).isNull(); + assertThat(obj.getJSONArray("pages").getJSONObject(0).getStr("slug")) + .isEqualTo("primary-page"); + } +}