feat(harness): support multi-keyword search modes - #3056
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
All CI checks are now green, including Ubuntu and Windows builds, license, module sync, Codecov, and CLA. The PR preserves the existing phrase behavior and Java overloads, with focused coverage for phrase/all/any matching. Ready for maintainer review. |
|
A parallel PR #3062 now proposes the same multi-keyword search modes for #3054. It was opened after this PR and follows a similar compatibility boundary. I am flagging the overlap so maintainers can choose one implementation path; I am happy to align with the preferred design or close this PR if #3062 is selected. The current PR remains ready for review with all required CI checks passing. |
oss-maintainer
left a comment
oss-maintainer
left a comment
There was a problem hiding this comment.
Summary
Adds an optional matchMode (phrase / all / any) to memory_search and session_search, backed by a shared SearchMatchMode enum plus unit tests. The design is clean and the back-compatible Java overloads for existing callers are a nice touch. Two things hold this back from approval: a hot-path regression in the matcher, and a feature overlap with #3062.
Findings
- [Warning]
SearchMatchMode.java:45— the phrasePatternis now compiled once per scanned line instead of once per search; previouslyMemorySearchTool.keywordSearchhoistedPattern.compile(Pattern.quote(query), CASE_INSENSITIVE)out of the loop. Memory files are scanned line by line, so one compile becomes thousands. - [Warning]
SearchMatchMode.java:50— the keyword split is recomputed per line for the same reason.\\s+is also ASCII-only here, so an ideographic space (Pattern.compile("\\s+", Pattern.UNICODE_CHARACTER_CLASS)would. - [Warning]
SearchMatchMode.java:36— an unrecognisedmatchModesilently falls back toPHRASE. Since the parameter is filled in by the model, a value likeor/keywordsyields phrase semantics with no signal, so the agent sees "no results" and retries instead of correcting the argument. The tools already return error strings ("No query provided","Error: query is required"), so an explicit error would be consistent. - [Info]
MemorySearchTool.java:85— call site where the per-record cost lands; hoisting a prepared matcher out of the loop keeps identical behaviour at the original cost.
Tests
SearchMatchModeTest covers the mode matrix well at the unit level. Gaps: no end-to-end coverage through memory_search / session_search via Toolkit (so schema generation and reflective invocation of the new optional parameter are untested), and no non-ASCII query — either would have surfaced the two issues above.
Cross-PR Note
This overlaps with #3062 (same matchMode=phrase|all|any parameter on the same two tools, same whitespace-split semantics, touching the same files), so the two will conflict. #3062 has already been approved by @dailingtao, hoists a prepared Predicate<String> out of the loop, errors on an invalid mode, and ships docs/v2/{en,zh} updates for the new parameter — which this PR does not.
If #3062 is the one going in, consider closing this to save the review effort; if you would rather keep this design, it is worth porting the non-ASCII split and the explicit-mode-error into it. Either way the tool schema and docs should land only once.
Automated review by github-manager-bot
| return false; | ||
| } | ||
| if (this == PHRASE) { | ||
| return Pattern.compile(Pattern.quote(query), Pattern.CASE_INSENSITIVE) |
There was a problem hiding this comment.
[Warning] Hot-path regression: the phrase pattern is compiled on every line. Before this change MemorySearchTool.keywordSearch hoisted Pattern.compile(Pattern.quote(query), CASE_INSENSITIVE) out of the loop once per search; now matches() recompiles it per record (see MemorySearchTool.java:85). Memory files are scanned line-by-line, so this turns one compile into thousands.
Suggest resolving the matcher once per search instead of per record, e.g. make the mode produce a Predicate<String>:
static Predicate<String> matcher(SearchMatchMode mode, String query) { ... }so the loop body becomes if (matcher.test(lines[i])).
| .find(); | ||
| } | ||
| List<String> keywords = | ||
| Arrays.stream(query.trim().split("\\s+")) |
There was a problem hiding this comment.
[Warning] Same issue as above — query.trim().split("\\s+") and the keywords list are rebuilt for every line scanned. Combined with the per-line Pattern.compile at line 45, the per-record cost is now dominated by work that is constant for the whole search.
Also \\s+ is ASCII-only here, so an ideographic space (\u3000) or non-breaking space (\u00a0) inside a Chinese query will not split into keywords. If that matters, Pattern.compile("\\s+", Pattern.UNICODE_CHARACTER_CLASS) covers it.
| try { | ||
| return valueOf(value.trim().toUpperCase(Locale.ROOT)); | ||
| } catch (IllegalArgumentException ignored) { | ||
| return PHRASE; |
There was a problem hiding this comment.
[Warning] Silently downgrading an unrecognised matchMode to PHRASE hides caller mistakes. matchMode is filled in by the model, so a value like "ANY" (handled) vs "or" / "keywords" (not handled) will produce phrase semantics with no signal — the agent sees "no results" and cannot tell that its parameter was ignored, which usually leads to retries rather than a fix.
Since the tool already returns error strings ("No query provided", "Error: query is required"), returning an explicit error for an invalid mode would be consistent and much easier to debug.
| String[] lines = content.split("\n", -1); | ||
| for (int i = 0; i < lines.length; i++) { | ||
| if (pattern.matcher(lines[i]).find()) { | ||
| if (matchMode.matches(lines[i], query)) { |
There was a problem hiding this comment.
[Info] This is the call site that makes the per-record compilation visible: matchMode.matches(lines[i], query) runs the mode dispatch + pattern compile + keyword split inside the inner loop. Hoisting a prepared matcher out of the loop (as #3062 does with a Predicate<String>) keeps the same behaviour at the original cost.
2 participants
Closes #3054
What does this PR do?
Adds an optional
matchModeparameter tomemory_searchandsession_search.phrase(default) preserves the existing literal full-query behavior.allmatches records containing every whitespace-separated keyword.anymatches records containing at least one keyword.Matching remains case-insensitive and literal. Memory searches remain line-scoped, session searches remain entry-scoped, and keywords are never combined across records.
Compatibility
phrase.Verification
mvn -s /Users/yohanes/.m2/settings-public-central.xml -pl agentscope-harness -am -Dtest=SearchMatchModeTest -Dsurefire.failIfNoSpecifiedTests=false -Dcheckstyle.skip=true -Dspotless.check.skip=true testResult: 4 tests passed.