Fix: ResultExpanderTools crashes with "Text content cannot be empty" when expandResult returns an empty list#1441
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a crash in ResultExpanderTools when ResultExpander.expandResult(...) returns an empty (or fully-filtered) list by returning a non-empty fallback message instead of producing an empty tool response.
Changes:
- Add empty-list handling to
broadenChunk()to avoid returning an empty string. - Add empty-list handling to
zoomOut()to avoid returning an empty string.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val chunks = resultExpander.expandResult(chunkId, ResultExpander.Method.SEQUENCE, chunksToAdd) | ||
| .filterIsInstance<Chunk>() | ||
| if (chunks.isEmpty()) return "No adjacent chunks found for this section." | ||
| return chunks.joinToString("\n") { "Chunk ID: ${it.id}\nContent: ${it.text}\n" } | ||
| } |
There was a problem hiding this comment.
The new empty-result handling in broadenChunk is user-visible behavior and fixes a crash path. Add a unit test that mocks ResultExpander.expandResult(...) returning an empty list (and/or a list with no Chunks) and asserts the non-empty fallback message is returned.
| .filter { it is Embeddable } | ||
| if (embeddables.isEmpty()) return "No parent section found." | ||
| return embeddables.joinToString("\n") { contentElement -> | ||
| "${contentElement.javaClass.simpleName}: id=${contentElement.id}\nContent: ${(contentElement as Embeddable).embeddableValue()}\n" |
There was a problem hiding this comment.
Add a unit test for zoomOut covering the case where expandResult(...) returns an empty list (or only non-Embeddable elements) and assert the non-empty fallback message is returned, to prevent regressions of the "Text content cannot be empty" crash.
| .filter { it is Embeddable } | |
| if (embeddables.isEmpty()) return "No parent section found." | |
| return embeddables.joinToString("\n") { contentElement -> | |
| "${contentElement.javaClass.simpleName}: id=${contentElement.id}\nContent: ${(contentElement as Embeddable).embeddableValue()}\n" | |
| .filterIsInstance<Embeddable>() | |
| if (embeddables.isEmpty()) return "No parent section found." | |
| return embeddables.joinToString("\n") { embeddable -> | |
| "${embeddable.javaClass.simpleName}: id=${embeddable.id}\nContent: ${embeddable.embeddableValue()}\n" |
| val chunks = resultExpander.expandResult(chunkId, ResultExpander.Method.SEQUENCE, chunksToAdd) | ||
| .filterIsInstance<Chunk>() |
There was a problem hiding this comment.
The chained call indentation is inconsistent with the surrounding style in this file (e.g., results chaining in deduplicateByIdKeepingHighestScore). Align the .filterIsInstance<Chunk>() line under the expression to keep Kotlin formatting consistent and avoid formatter/lint churn.
| val embeddables = resultExpander.expandResult(id, ResultExpander.Method.ZOOM_OUT, 1) | ||
| .filter { it is Embeddable } |
There was a problem hiding this comment.
The chained call indentation is inconsistent with the rest of this file. Align the .filter { ... } line under the expandResult(...) call for consistent Kotlin formatting.
...nt-rag/embabel-agent-rag-core/src/main/kotlin/com/embabel/agent/rag/tools/CoreSearchTools.kt
Show resolved
Hide resolved
|
Please sign the commit. Thanks |
…crash Signed-off-by: Chandan Agarwal <chandan.agarwal@cullen-international.com>
coolduebtn
force-pushed
the
fix/result-expander-empty-list-crash
branch
from
40cd25b to
e2329b5
Compare
I have signed off the commit. Thanks |
johnsonr
left a comment
johnsonr
left a comment
There was a problem hiding this comment.
Fix looks good but I agree with Copilot that it needs tests that demonstrate the failure no longer occurs.
Closes #1440