Repository navigation
Conversation
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @utafrali please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/agentscope-ai/agentscope-java. Once signed, the Automated check by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Splits the memory_search query on whitespace/CJK delimiters, requires all tokens to hit the same line (AND), and falls back to any-token matching ranked by hit count (OR). Fixes #3230 and the approach is sound; CI is green. Two things hold this back from an approval: the OR fallback has no output cap, and the CLA bot is still reporting this contribution as unsigned.
Findings
- [Warning]
MemorySearchTool.java:107— OR fallback emits every any-token match with no cap; can flood the agent context window. - [Warning]
MemorySearchTool.java:67—/and\as token separators fragment paths/dates; per-tokenPattern.compileis unnecessary for literal matching. - [Info]
MemorySearchToolTest.java:69— assertions do not distinguish the AND path from the OR fallback, so ranking/ordering regressions would slip through.
Suggestions
- Cap and label the fallback output (
showing top N of M), optionally truncating very long lines. - Consider a minimum token length (or dropping
/\from the delimiter set) so numeric/path fragments do not become wildcard keywords. - Note for maintainers: PR #3267 reworks the same
keywordSearchmethod to addmaxResultsand line truncation. These two changes will conflict; worth aligning on one of them landing first.
Blocked on
license/cla is still pending — this PR cannot be approved or merged until the contributor signs the CLA (see the reminder comment). Contents aside, the changes look reasonable.
Automated review by github-manager-bot
| if (andCount > 0) { | ||
| return "Found " + andCount + " matches:\n\n" + andResults; | ||
| } | ||
| if (orCandidates.isEmpty()) { |
There was a problem hiding this comment.
OR fallback is unbounded and can defeat the purpose of the fix. When none of the lines contains all tokens, every line matching any single token is emitted. For an English-word query that happens to contain one common token (e.g. 配置 issue or fix bug), this can dump hundreds of lines straight into the agent context window.
Suggest capping the fallback (limit(N) after sorting by hit count) and reporting truncation, e.g. "Found 132 matches (showing top 20):". This also keeps output predictable for the LLM.
| private String keywordSearch(RuntimeContext rc, String query) { | ||
| StringJoiner results = new StringJoiner("\n"); | ||
| int matchCount = 0; | ||
| String[] rawTokens = query.split("[\\s,，;；|｜、/\\\\]+"); |
There was a problem hiding this comment.
Splitting on / and \ can fragment tokens that are semantically single words — paths (a/b), dates (12/31), URLs. Combined with the OR fallback, a fragment like 31 will then match unrelated lines.
Two smaller points on the same block:
- Lines 70-72 build one
Patternper token but only ever usePattern.quote(t)+CASE_INSENSITIVE(a literal containment test).String.toLowerCase()+line.contains(token)— or at least hoisting the compiled patterns — would avoidPattern.compileper token per call, since this runs on everymemory_searchinvocation. - No dedup when a memory file contains the same line twice (
StringJoinerwith duplicate labels); the old code had the same behavior, so just a note.
| workspace.resolve("MEMORY.md"), | ||
| "- line about 诗歌 creation\n- another line about 交付\n"); | ||
| String result = tool.memorySearch(null, "诗歌 交付"); | ||
| // No single line has both; OR fallback returns lines with any match |
There was a problem hiding this comment.
Assertions like result.startsWith("Found ") pass on both the AND path and the OR-fallback path, so a regression that accidentally always falls back to OR would not be caught. Suggest asserting on the specific path (and on ordering, since ranking is the main new behavior), e.g. check that the line containing both keywords appears before a line containing only one of them.
|
Done, pushed the fix. |
|
Thanks for the update. One heads-up: the PR head is still The three findings are still open at the current head (
Please push the fix (and note Automated notification by github-manager-bot |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Follow-up on "Done, pushed the fix." — I do not see a new commit: the PR head is still dea863c4 (committer date 2026-09-24T10:46:47Z), which is the commit my review was posted against. Could you confirm the fix actually landed on this branch? If it was pushed to a different branch/fork remote, the PR needs to be repointed at it.
Happy to re-review as soon as the new commit shows up here. CLA is signed, so this is review-ready once CI and the push are in sync.
Automated review by github-manager-bot
|
Thanks for your contribution. fixed in: #3062 |
4 participants
AgentScope-Java Version
Latest
Description
Fixes #3230
The memory search was treating the entire query as one literal phrase, so searching for "memory context" would only match that exact string. Now it splits on common delimiters and searches for all keywords.
Split the query into individual tokens, create case-insensitive patterns for each, and rank results by hit count. Lines with all keywords come first, partial matches as fallback.
Added tests for multi-keyword queries with various delimiters and verified with mvn test.
Checklist
mvn spotless:applymvn test)