fix(harness): clamp model-supplied limits in session_search / session_history

[aiyili]

Fixes #3270

What

SessionSearchTool's model-supplied limits are now clamped to a hard ceiling of 100 (MAX_RESULTS_CEILING), closing the same hole #3267/#3267 closed for memory_search:

• session_search maxResults — clamped via Math.min, so a model passing 100000 no longer scans-and-collects unbounded results;
• session_history lastN — same hole, same clamp (called out in the PR as a sibling fix);
• ToolParam descriptions now state "capped at 100" so the model knows;
• KeywordMatcher synced from main (matchMode dependency of the touched code path).

The clamp also cuts the I/O amplification the report flagged: the collection loop stops at the clamped limit instead of walking every entry of every parsed SessionTree.

Tests

New SessionSearchToolCeilingTest (real WorkspaceManager + real offloaded session tree, 150 needle entries):

• maxResults=100000 → Found 100 matches (not 150);
• lastN=100000 → history clamped to 100;
• default limit (10) unaffected.

Module suite matches the upstream baseline (0 failures, only the two known Windows symlink-privilege environment errors).

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...entscope/harness/agent/tool/SessionSearchTool.java	0.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

[oss-maintainer]

Summary

Adds a MAX_RESULTS_CEILING = 100 clamp for the model-supplied maxResults / lastN in session_search / session_history, updates the two tool parameter descriptions, and adds SessionSearchToolCeilingTest. The clamp is placed correctly and maxResults/lastN really are the only size knobs the model controls here, so this is the right fix for #3270. Verdict: two things to fix before merge — one test that cannot fail, and one inaccurate justification comment (see inline).

Findings

• [Warning] SessionSearchToolCeilingTest.java:95 — session_history output never contains entry ids, so the split("m\d+") count is always 0 and emitted <= 100 passes even with the clamp removed.
• [Info] SessionSearchTool.java:50 — the "nothing downstream trims" rationale only holds for session_search; session_history is not in ToolResultEvictionConfig.DEFAULT_EXCLUDED_TOOLS.

Suggestions

// instead of counting ids that are never printed
assertTrue(out.contains("showing last 100)"), () -> "history must be clamped to 100, got: " + out);

Also worth pinning the boundary explicitly (lastN=99 → 99, lastN=101 → 100) so a future edit to the ceiling constant is caught.

Note: no license/cla status context was found on the head commit of this PR, so I'm leaving this as a comment rather than an approval — please confirm the CLA is signed (or that the check is not required for this repo) and I'll re-review.

---

Automated review by github-manager-bot

[oss-maintainer]

This assertion is vacuous — the test cannot detect a missing/incorrect lastN ceiling.

sessionHistory never prints entry ids: it emits "[role]: content" lines only (SessionSearchTool.java:242-250), and the seeded text is "needle item N", which contains no m<digit> token. So out.split("m\\d+", -1).length - 1 is 0 regardless of the limit, and emitted <= 100 passes even with the clamp deleted.

The header line already carries the truth — assert on it instead:

assertTrue(out.contains("showing last 100)"), () -> "history must be clamped to 100, got: " + out);

An explicit 100-vs-101 boundary case (e.g. lastN=99 still returns 99) would pin the behaviour down further.

[oss-maintainer]

The stated rationale only holds for session_search.

ToolResultEvictionConfig.DEFAULT_EXCLUDED_TOOLS lists memory_search, memory_get and session_search — session_history is not in it, so oversized history results are still evictable downstream. The clamp itself is still the right fix here (bounding at the source is better than evicting after the fact), but the comment claiming "nothing downstream trims" an oversized session_history result is inaccurate and will mislead the next reader.

Could you either drop session_history from that sentence, or add it to DEFAULT_EXCLUDED_TOOLS if the intent is for the clamp to be the only bound?

[aiyili]

Both items addressed in a193e3e, plus the CLA confirmation:

• Vacuous assertion fixed: the split-on-ids count was indeed always 0 (history output never contains entry ids). Now asserts the header's showing last 100), and added an explicit boundary test (lastN=99 → showing last 99), lastN=101 → showing last 100)) so the ceiling constant is pinned. This also closes the two partial branches Codecov flagged.
• Rationale corrected: the comment now scopes the "nothing downstream trims" claim to session_search only (the excluded tool) and states that session_history is evictable downstream while clamping at the source remains preferable — it also caps the scan itself.
• CLA: signed — license/cla reports success on the head commits of my other open PRs against this repo (fix(harness): contain CompositeFilesystem root ls/grep to the backend root (stacked on #3247) #3254, fix(harness): pass docker exec scripts via stdin on Windows (nested quotes survive) #3383, fix(harness): align transcript segment keys with the sessions route #3386); the check simply hasn't posted on this PR's newer head yet.

[oss-maintainer]

Summary

Clamps model-supplied session_search maxResults and session_history lastN to MAX_RESULTS_CEILING=100 (fixing #3270) and updates both tool param descriptions. The clamp is correct and overflow-safe (null/non-positive inputs still fall back to the 10/20 defaults), and the rewritten SessionSearchToolCeilingTest is now genuinely load-bearing: it asserts real output substrings ("Found 100 matches", "showing last 100)" — both verified against the actual format strings in SessionSearchTool), so it fails without the clamp, and the previously suggested 99/101 boundary case was added as its own test.
Both concerns from the previous review are resolved: the earlier Warning (history test counted entry ids that are never printed, so it could not fail) is fixed by asserting the clamped header, and the suggested boundary pinning (lastN=99 -> 99, lastN=101 -> 100) was implemented. The earlier Info-level comment inaccuracy (the javadoc still says "nothing downstream trims an oversized result", but session_history is not in ToolResultEvictionConfig.DEFAULT_EXCLUDED_TOOLS, so eviction would trim it) was left as-is; it is cosmetic and not re-reported as a finding. As before, no CLA status context was found on the head commit, so please confirm the CLA check separately before merge.
No blocking findings this round — both previously raised concerns are resolved.

---

Automated review by github-manager-bot

[aiyili]

CLA confirmed signed: license/cla reports success on the head commits of my other open PRs against this repo (#3254, #3383, #3386) — same account, same signature; the check just hasn't posted on this PR's head yet. Good to merge whenever the maintainer is ready.

[Buktal]

LGTM