fix: guard limitTaskHistory against negative historyLength - #1039
Conversation
kabir
left a comment
There was a problem hiding this comment.
Hi @ez-lbz thank you for the PR.
I've looked and I don't think this is a viable scenario. Essentially, TaskQueryParams has a guard against negative history lengths. And this class is used both to send from the client, as well as used when deserializing requests on the server. So it seems its check will kick in before limitTaskHistory can ever be called.
Let me know if you disagree - there are quite a few PRs to go through today :-)
|
Thanks for looking at this carefully — your analysis is right that The intent here was defense-in-depth: I'm happy to keep it as a defensive measure, or close the PR if you'd prefer not to carry it — your call. |
- Fix import ordering of TaskQueryParams in DefaultRequestHandlerTest - Remove fragile reflection-based test for negative historyLength guard (the two public-API tests already exercise the limitTaskHistory path) - Trim verbose 4-line comment on negative historyLength guard to one line - Fix Javadoc on bufferFlushDelayMs(): negative values are clamped to 0, not returned as the default - Replace fully-qualified class names with imports in EventQueueTest Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
What changed
1. Guard
limitTaskHistoryagainst negativehistoryLengthProblem: The guard in
limitTaskHistoryonly checkedhistoryLength >= task.history().size(), which does not catch negative values. A negativehistoryLengthfell through tosubList(size - historyLength, size)— i.e.subList(size + 1, size)— throwingIndexOutOfBoundsException. WhileTaskQueryParamsrejects negative values at construction today,limitTaskHistorylacked defensive validation and would crash if a negative value ever reached it.Fix (server-common/src/main/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandler.java):
historyLength < 0, so a negative value returns the task untouched instead of crashing.apply_history_length): negative → history untouched;0→ empty history;N >= history size→ no limiting. The existing0 → empty historybehavior is preserved (unlike a<= 0guard, which would have broken it).Fix (server-common/src/test/java/org/a2aproject/sdk/server/requesthandlers/DefaultRequestHandlerTest.java):
testLimitTaskHistoryNegativeHistoryLengthReturnsTaskUnchanged— invokes the privatelimitTaskHistoryvia reflection with-1and asserts no exception is thrown and the task is returned unchanged.testOnGetTaskHistoryLengthLimitsHistory—historyLength=2returns only the 2 most recent messages.testOnGetTaskHistoryLengthZeroReturnsEmptyHistory— locks in thathistoryLength=0still returns an empty history.Behavior change: none for valid inputs (
>= 0). NegativehistoryLengthpreviously crashed; it now returns the task unchanged.Testing
mvn -pl server-common test— 450 tests run, 0 failures, 0 errors, 0 skipped (BUILD SUCCESS), including the 3 new regression tests.