fix(harness): bound memory_search results with maxResults and line truncation

[aMetric]

Closes #3266

What does this PR do?

Bounds memory_search tool results so a broad query can no longer overflow the model context
window.

• Result cap: adds an optional maxResults tool parameter (default 30, matching the
  documented "up to 30 hits" that the code never implemented). Missing/blank/<= 0 values fall
  back to the default, mirroring session_search's handling of the same parameter.
• Per-line truncation: matched lines longer than 500 chars are truncated with a
  use memory_get pointer; the full line remains reachable via memory_get (Source: <file>#<line> output format is unchanged, so the pairing still works).
• Truncation note: when the hit count is capped, the result ends with a note telling the
  model to refine the query or use memory_get.
• Updates the eviction-exclusion rationale in ToolResultEvictionConfig Javadoc (the
  "small/paginated results" claim was false before this change) and extends the en/zh memory
  docs to document maxResults and line truncation.

Compatibility

• Tool signature: one new optional parameter. Callers omitting it get the documented
  30-hit behavior; the JSON schema gains a non-required property.
• Output format: unchanged for all results that fit (same Found N matches header, same
  Source: <file>#<line>: <text> rows); only oversized/overlong results gain a truncation
  note.
• The Java-level method now takes Integer maxResults (was 2 args). In-repo callers are only
  the reflective @Tool registration in HarnessAgent; no other source call sites exist.

Verification

• New MemorySearchToolTest (8 tests): default cap, maxResults override, invalid-value
  fallback, truncation note presence/absence, long-line truncation, blank-query and no-match
  paths.
• Sabotage run: disabling the cap/truncation constants makes the cap test fail; re-enabling
  makes all 8 pass — the regression tests bite.
• Full mvn -pl agentscope-harness test run (result in the PR discussion).

Risk

Low: the changed code path only affects the single memory_search tool; file scanning and
pattern matching are untouched.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...gentscope/harness/agent/tool/MemorySearchTool.java	92.59%	1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

CLA Not Signed

The Contributor License Agreement (CLA) check is currently pending on this PR (license/cla: Contributor License Agreement is not signed yet.). This PR cannot be merged until the CLA is signed.

@aMetric 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 license/cla status will turn green.

---

Automated check by github-manager-bot

[oss-maintainer]

Summary

Bounds memory_search with a maxResults parameter, a 500-char per-line truncation and a truncation note, plus a new MemorySearchToolTest (8 cases) and en/zh doc updates. The direction is right and the test suite is genuinely useful — the "sabotage run" evidence in the PR body is exactly what a context-bounding fix needs.

The theme of my comments: the cap is applied to the default but not to the parameter. maxResults is model-controlled and unbounded, so the overflow path that #3266 describes is still reachable through the parameter that was added to close it (see the inline comment on MemorySearchTool.java:78, with session_search carrying the same shape). Two smaller accuracy issues on the model-visible output: truncatedByLimit is derived from loop position instead of an actual extra match (false "refine your query" notes), and truncateLine can split a UTF-16 surrogate pair on CJK/emoji memory content.

Not approving: the license/cla status is still pending for this head commit, so this review is comment-only. None of the findings above look hard to land — happy to re-review once they are addressed and the CLA is green.

Verified independently

• McpClientWrapper / @ToolParam(required = false) Integer usage matches the existing session_search convention — the new parameter is declared consistently, and the JSON schema change is additive (not required), so callers that omit maxResults keep documented behavior.
• memorySearch(...) has no in-repo callers besides the reflective @Tool registration in HarnessAgent.java:2691, so the 2-arg → 3-arg signature change is contained as the PR claims.
• Output format for the non-truncated path is unchanged (Found N matches + Source: <file>#<line>: <text>), so memory_get pairing keeps working.

---

Automated review by github-manager-bot

[oss-maintainer]

maxResults is accepted without an upper bound, so a model that passes maxResults=100000 can still overflow the context window — which is the exact failure mode #3266 is about. The default is bounded, but the parameter path is not.

Consider a hard ceiling next to the default:

static final int DEFAULT_MAX_RESULTS = 30;
static final int MAX_RESULTS_CEILING = 200; // hard cap, model cannot exceed
...
int limit = maxResults != null && maxResults > 0
        ? Math.min(maxResults, MAX_RESULTS_CEILING)
        : DEFAULT_MAX_RESULTS;

session_search has the same unbounded-maxResults shape (line 77 there), so either is fine as a convention — but since this PR's stated purpose is bounding the result, the ceiling feels like the missing half of the fix. The Javadoc/@ToolParam description should then say "...(default: 30, max: 200)" so the model is not surprised by a silently clamped value.

[oss-maintainer]

truncatedByLimit reports "there were more results" from loop position rather than from an actual extra match, so it is a false positive whenever the cap is reached before the real end of the data. Two cases from this check and the sibling one at line 102:

• the 30th match lands on a non-final line and the remaining lines do not match → break, note emitted
• the 30th match is the last line of file X and file Y exists but has no matches → outer check fires, note emitted

The model is told to "refine the query" when refining would change nothing. Cheap way to make the note accurate: keep scanning after the cap and only set the flag on a real extra hit.

boolean hasMore = false;
...
if (pattern.matcher(lines[i]).find()) {
    if (matchCount >= maxResults) {
        hasMore = true;
        break;
    }
    results.add(...);
    matchCount++;
}

The scan stays O(files) after the cap because results.add stops growing, and it costs one regex pass over the remainder — acceptable for memory files, and the note then means what it says. The new test notesThatResultsWereTruncated would keep passing; worth adding the exact-30-matches case as a companion regression test.

[oss-maintainer]

substring(0, MAX_LINE_CHARS) cuts on UTF-16 code units, so when the cut lands between the halves of a surrogate pair (emoji, and CJK Extension B+ ideographs — realistic in a memory file written by a Chinese-language agent) the result carries a lone surrogate. That usually surfaces as ?/mojibake once the tool result is JSON-encoded for the model.

private static String truncateLine(String line) {
    if (line.length() <= MAX_LINE_CHARS) {
        return line;
    }
    int cut = MAX_LINE_CHARS;
    if (Character.isHighSurrogate(line.charAt(cut - 1))) {
        cut--; // do not split a surrogate pair
    }
    return line.substring(0, cut) + "... [line truncated, use memory_get]";
}

Not a correctness break for ASCII-only lines, hence the low urgency — but the truncation pointer explicitly tells the model to re-read via memory_get, and a garbled tail makes the truncated preview misleading.

[oss-maintainer]

Two smaller readability points on the user-visible string, both model-facing:

1. When capped, matchCount is the cap, not the total — Found 30 matches reads as an exhaustive count and can make the model stop refining. Found 30+ matches (or Showing first 30 matches) states the bound explicitly, and then the truncation note below becomes a detail rather than a correction.
2. Pre-existing, but Found 1 matches is ungrammatical; since this line is being touched anyway, a tiny pluralize helper would be free.

Either is fine to skip if you would rather keep the format byte-stable for anything parsing the header — I did not find an in-repo parser of this string, but worth a look before changing it.

[oss-maintainer]

Correction on the CLA gate: the license/cla status on the current head (d027963) has since turned green — the CLA has been signed, so please disregard the earlier "CLA Not Signed" reminder and the CLA caveat in my review summary.

The review verdict itself is unchanged: it stays a COMMENT (not an approve) because of the four findings in #3267 (review) — mainly that maxResults is accepted without an upper bound, which leaves the context-overflow path from #3266 reachable through the parameter added to close it.

---

Automated review by github-manager-bot

[aMetric]

Review feedback addressed in 57417bf:

• maxResults is now clamped to a hard ceiling (MAX_RESULTS_CEILING = 200) via Math.min — a maxResults=100000 call can no longer re-open the overflow path; the @ToolParam description reads "(default: 30, max: 200)" so the model sees the bound.
• hasMoreMatches is set only from an actual extra match beyond the cap (scan continues to the next hit, then stops) — the false-positive "refine the query" notes are gone. Covered by noTruncationNoteWhenCapReachedAtExactEnd (30 exact matches + a non-matching second file).
• truncateLine backs the cut off a high surrogate — no split pairs; covered by truncateLineDoesNotSplitSurrogatePairs (CJK Extension B pairs).
• Header: Found 30+ matches when capped, singular Found 1 match otherwise.
• Also adopted session_search-style clamping only here as suggested; the session_search unbounded parameter is left for a follow-up (noted in case maintainers want symmetry).

I checked for in-repo parsers of the Found N matches header — none found, so the wording change is safe.

[oss-maintainer]

Summary

Re-review after 57417bfb ("address review on memory_search bounding"). All three findings from my previous review are resolved, and each was fixed at the right layer rather than patched around:

• Unbounded model-controlled maxResults → MAX_RESULTS_CEILING = 200 applied via Math.min at the single point where the parameter becomes a limit, so no other path can bypass it. maxResultsClampedToHardCeiling proves the case that mattered (100000 → 200).
• Truncation note derived from loop position → hasMoreMatches is now set only when a further match was actually observed, with the inner and outer break propagating it. noTruncationNoteWhenCapReachedAtExactEnd is precisely the right test for this, and stopping the scan at the cap also removes the previously wasted walk over the remaining files.
• Surrogate-pair split → isHighSurrogate(charAt(cut - 1)) guard; a lone high surrogate at that offset is the only way a pair can split, so the check is complete, and truncateLineDoesNotSplitSurrogatePairs covers it with real CJK Extension B code points.

license/cla is now success on this head, and build (ubuntu-latest), validate, Check License, Check Module Sync and codecov/patch all pass (build (windows-latest) still running).

Withholding approval for one reason only: the two new bounds multiply past this project's own context budget, and because memory_search is excluded from eviction nothing catches that case (inline on MemorySearchTool.java:47). It is a small change — either how the budget is expressed or the ceiling value — and I would approve on the next push.

Verified independently

• Eviction really is size-independent for these tools: ToolResultEvictionConfig.getExcludedToolNames() is documented as "never evicted regardless of result size", and ToolResultEvictionMiddleware.maybeEvict() returns before any size comparison. That is why the 200 x ~550 multiplication matters rather than being a theoretical nit.
• No remaining unbounded path inside this tool: limit is computed once before keywordSearch and every consumer (matchCount >= maxResults, the Found N+ header, the truncation note) reads the same already-clamped value.
• The non-truncated output shape is unchanged (Found N matches plus Source: <file>#<line>: <text>), so memory_get pairing and any prompt text relying on the format keep working; the new singular branch only differs from before where the old code emitted "1 matches".

---

Automated review by github-manager-bot

[oss-maintainer]

The ceiling closes the unbounded-parameter hole I raised last round — thank you, that was the main thing. One consequence worth tightening: the two new constants multiply past the project's own context budget. Worst case per hit is MAX_LINE_CHARS (500) plus the 'Source: #: ' prefix and the 37-char '... [line truncated, use memory_get]' suffix, so ~550 chars; 200 x 550 is roughly 110 K chars from a single tool result. That exceeds ToolResultEvictionConfig.DEFAULT_MAX_RESULT_CHARS (~80 K, documented as ~20 K tokens) — and because memory_search is in DEFAULT_EXCLUDED_TOOLS, ToolResultEvictionMiddleware.maybeEvict() returns early for it regardless of size, so nothing downstream catches that case. The default path (30 x 550 = ~16 K) is comfortably safe, so this only bites when a caller or the model asks for a large maxResults, which is exactly the input class the ceiling was added to tame. Suggest either bounding by accumulated characters (which would also make the line cap and the hit cap one coherent budget) or lowering MAX_RESULTS_CEILING so ceiling x per-hit worst case stays under DEFAULT_MAX_RESULT_CHARS.

[oss-maintainer]

This javadoc is the stated justification for excluding memory_search and session_search from eviction, so its accuracy is load-bearing. It is now correct for memory_search in the default path, with two caveats: (1) per the multiplication above, 'bounded' is not the same as 'under the eviction threshold' at the ceiling; (2) session_search still has no ceiling at all — SessionSearchTool.java:77 is 'int limit = maxResults != null && maxResults > 0 ? maxResults : 10;', so a model-supplied maxResults there remains unbounded, which is the same hole this PR closed here. Both are follow-up scope, not reasons to change this PR; filing a sibling issue for session_search would keep this exclusion's justification honest.

[aMetric]

Budget finding addressed in a6570df:

• MAX_RESULTS_CEILING lowered 200 → 100. Worst case is now 100 × ~550 chars ≈ 55K, under DEFAULT_MAX_RESULT_CHARS (80K) with headroom; the Javadoc records the arithmetic so the next editor of either constant sees the dependency.
• @ToolParam description updated to "(default: 30, max: 100)".
• Clamp regression test updated (100000 → Found 100+ matches); all 11 tests pass.

The session_search sibling hole is now filed as #3270 (unbounded maxResults at SessionSearchTool.java:77, same eviction-exclusion dependency) — happy to take that one too.

[oss-maintainer]

Summary

Re-review after a6570df3: the one open blocker from my previous review is resolved, and I verified it against the code rather than the commit message. MAX_RESULTS_CEILING dropped from 200 to 100 at the single point where the model-supplied parameter becomes a limit (Math.min(maxResults, MAX_RESULTS_CEILING) in memorySearch), so no path bypasses it.

Verified independently

• The budget arithmetic in the new Javadoc holds: ToolResultEvictionConfig.DEFAULT_MAX_RESULT_CHARS is 80_000 and memory_search is in DEFAULT_EXCLUDED_TOOLS, so nothing downstream trims an oversized result — 100 x ~550 chars per hit (~55K worst case) now stays under that threshold instead of passing it at 200. That makes the "bounded results" claim in the eviction Javadoc true for this tool.
• The tool description (max: 100) and maxResultsClampedToHardCeiling were both moved in the same commit, so the advertised contract and the regression assertion cannot drift from the constant. The test still proves the case that matters: 100000 clamps to the ceiling and line 101 is never returned.
• noTruncationNoteWhenCapReachedAtExactEnd and truncateLineDoesNotSplitSurrogatePairs are untouched by this push and still pin the two fixes from the previous round.
• license/cla is success on this head; Check License, validate, Check Module Sync and codecov/patch pass. build (ubuntu-latest) / build (windows-latest) were still in progress at review time — approving the code, not the merge readiness.

One non-blocking follow-up

The same eviction Javadoc line groups session_search with memory_search as having a "capped hit count", but SessionSearchTool still reads int limit = maxResults != null && maxResults > 0 ? maxResults : 10; with no ceiling, so a model can pass maxResults=100000 there. Out of scope for this PR (the change is about memory_search), but worth a separate issue so the exclusion rationale stops overstating what session_search guarantees.

Nice work tracking the budget down to an actual constant rather than a guess — and thanks for the precise test updates in the same commit.

---

Automated review by github-manager-bot

[oss-maintainer]

Correction to my approval review above (review 5298023931): one CI line was inaccurate for this head.

codecov/patch is not reported on a6570df3 yet — that check only existed on the previously reviewed head 57417bfb. What is actually complete on the current head is validate, Check License, Check Module Sync and license/cla; build (ubuntu-latest) and build (windows-latest) are still in progress.

The approval itself stands unchanged — it was based on the ceiling fix and the budget arithmetic verified against ToolResultEvictionConfig and ToolResultEvictionMiddleware, not on the coverage status. Flagging this only so nobody treats coverage as green before the builds finish.

---

Automated notification by github-manager-bot

[oss-maintainer]

⚠️ Merge conflict detected

This PR currently conflicts with agentscope-java main and cannot be merged. Please rebase (or merge main in) and resolve the conflicts:

git fetch upstream
git checkout fix/memory-search-bounded-results
git rebase upstream/main
# resolve conflicts, then:
git push --force-with-lease

Everything else in the review so far is already captured in the earlier review comments on this PR, so no re-review work is needed beyond the rebase. This is a one-time reminder — feel free to @mention me once the branch is clean and I will re-check.

---

Automated notification by github-manager-bot

[oss-maintainer]

Re-reviewed the new head b6cb6094. It is a merge sync only: ToolResultEvictionConfig.java, MemorySearchTool.java and MemorySearchToolTest.java are byte-identical to the a6570df3 tree I approved, so the bounding logic and the eviction-ceiling rationale are unchanged.

The conflict resolution in docs/v2/{en,zh}/docs/harness/memory.md kept both sides correctly — this PR's maxResults / 500-char truncation wording is intact, and the upstream additions (memory_save, session_search, the zh_link/en_link front matter) came through rather than being dropped by a --ours resolution. The tool count sentence was updated to match, in both languages.

Checks: CLA signed, MERGEABLE/CLEAN, and Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch, validate all pass on this head.

---

Automated review by github-manager-bot

[oss-maintainer]

Re-confirmed on the current head b6cb6094 (the previous approval was anchored to the older a6570df3 tree). The merge sync is docs-only: ToolResultEvictionConfig.java, MemorySearchTool.java and MemorySearchToolTest.java are byte-identical to the approved tree, and docs/v2/{en,zh}/docs/harness/memory.md kept both sides of the conflict (this PR's maxResults / 500-char truncation wording plus the upstream memory_save / session_search additions). CLA signed; MERGEABLE/CLEAN; Check License, Check Module Sync, build (ubuntu-latest), build (windows-latest), codecov/patch, validate all pass. No objections.

---

Automated review by github-manager-bot

[oss-maintainer]

Summary

Bounds memory_search with a model-tunable maxResults (default 30, hard ceiling 100), truncates over-long match lines at 500 chars with a surrogate-safe cut, updates both EN and ZH docs, and adds a focused test class. The bounding design and the ceiling arithmetic are the right instinct for the #3266 context-overflow issue. Holding at comment level: one off-by-one in the "is there more beyond the cap?" probe, and the build is red on formatting so the new tests have not actually run in CI yet.

Findings

• [Critical] MemorySearchTool.java:152 — the cap check runs before matcher.test(lines[i]), so the line that trips the cap is never tested; the 31st match can vanish with no + marker.
• [Warning] MemorySearchTool.java:155 — memoryPaths.indexOf(relativePath) is an O(n) scan inside the loop, resolves to the first duplicate, and only probes the single next file.
• [Warning] MemorySearchTool.java:215 — spotless:check fails here (lines 151-152 plus MemorySearchToolTest.java), the file ends without a trailing newline, and that is what turns both OS builders red before tests run.
• [Info] MemorySearchTool.java:113 — maxResults of 0 or a negative value silently becomes 30; worth making discoverable to the caller.

Suggestions

Replace the one-line peek with "test first, then decide", which keeps the never-a-false-positive property without skipping a line:

for (int i = 0; i < lines.length; i++) {
    if (!matcher.test(lines[i])) {
        continue;
    }
    if (matchCount >= maxResults) {
        hasMoreMatches = true;
        break outer;
    }
    results.add(String.format("Source: %s#%d: %s", relativePath, i + 1, truncateLine(lines[i])));
    matchCount++;
}

Then add a 31-match ledger test (capsResultsAtDefaultLimit uses 40 consecutive matches, which passes today only because the neighbours all match), run mvn spotless:apply -pl agentscope-harness, and re-push so the test suite actually executes.

The overload you kept for memorySearch(rc, query) / memorySearch(rc, query, matchMode) is the right call for callers of the tool outside the @Tool path.

---

Automated review by github-manager-bot

[oss-maintainer]

Off-by-one: the line that trips the cap is never tested, so a match disappears with no signal. The cap check runs before matcher.test(lines[i]), so when the loop enters iteration i with matchCount >= maxResults, line i is still untested — and this peek only looks at lines[i + 1]. Concrete case with the default cap of 30 and 31 matching facts: at i = 30 the code tests lines[31], which is the empty string split("\n", -1) leaves behind, gets false, and returns Found 30 matches without a +, so the 31st match is silently dropped. The same class of miss applies to anything beyond i + 1 in the current file and to every file after nextFileIdx.

Keeping the "never a false positive" property without skipping a line:

for (int i = 0; i < lines.length; i++) {
    if (!matcher.test(lines[i])) {
        continue;
    }
    if (matchCount >= maxResults) {
        hasMoreMatches = true;
        break outer;
    }
    results.add(String.format("Source: %s#%d: %s", relativePath, i + 1, truncateLine(lines[i])));
    matchCount++;
}

capsResultsAtDefaultLimit() (40 consecutive matching lines) passes today only because its neighbours all match — a 31-match ledger would pin this down.

[oss-maintainer]

memoryPaths.indexOf(relativePath) is an O(n) scan inside the file loop and resolves to the first occurrence, so a duplicated path in the listing sends the probe to the wrong file — and only the immediately next file is probed, so a hit in file n+2 is missed (same false-negative class as the line peek above). An indexed loop over memoryPaths gives the current position for free and lets the fallback walk the remaining files, or you can drop the probe entirely and print Found N matches (showing first N) when the cap is reached.

[oss-maintainer]

CI is red on this head and it is all formatting: spotless:check fails lines 151-152 (the assignment fits on one line) and MemorySearchToolTest.java as well, and this file now ends without a trailing newline (\ No newline at end of file on the diff). That is what turns build (ubuntu-latest) and build (windows-latest) red. mvn spotless:apply -pl agentscope-harness plus a final newline should clear both. Because spotless aborts before compilation, the tests in this PR have not actually run in CI yet — please re-push and confirm they are green.

[oss-maintainer]

The ceiling math here is well argued and the surrogate back-off in truncateLine is a nice detail. One semantics note: maxResults of 0 or a negative value silently becomes 30 (maxResultsIgnoredWhenInvalid asserts exactly that). A model asking for 0 most likely means "just tell me whether anything matches", and 30 hits is the opposite of the request — either say so in the @ToolParam description or return a one-line hint (maxResults must be > 0; using the default of 30). Either is fine; just make the fallback discoverable to the caller.