fix: surface MaxTokensError when max_tokens truncates tool input JSON#1065
fix: surface MaxTokensError when max_tokens truncates tool input JSON#1065cmbullock wants to merge 2 commits into
Conversation
| await expect(async () => await collectGenerator(provider.streamAggregated(messages))).rejects.toThrow( | ||
| MaxTokensError | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Issue: Missing test coverage for the path where the stream ends without a modelMessageStopEvent while deferredToolInputParseError is set (i.e., the 'Stream ended without completing a message' error with the SyntaxError cause attached at line 477).
Suggestion: Add a test case where the generator yields a tool input delta + contentBlockStopEvent (triggering the parse error) but does NOT yield a modelMessageStopEvent. This exercises the new cause attachment on the "stream ended" error path.
| } | ||
|
|
||
| if (deferredToolInputParseError !== undefined) { | ||
| throw deferredToolInputParseError |
There was a problem hiding this comment.
Issue: When stopReason is not maxTokens, the deferred SyntaxError is thrown raw and relies on the outer catch block to wrap it in a ModelError. This produces a generic message like "Expected ',' or '}' in JSON at position X" rather than something actionable.
Suggestion: Consider wrapping it explicitly to preserve the diagnostic context already in the log message:
if (deferredToolInputParseError !== undefined) {
throw new ModelError('unable to parse tool input JSON', { cause: deferredToolInputParseError })
}This makes the error message self-descriptive and avoids depending on the outer catch's generic wrapping behavior.
There was a problem hiding this comment.
Error explicitly wrapped and test updated
|
Assessment: Comment Good fix that correctly surfaces Review Details
Clean, minimal change that solves the reported issue. |
|
This repository has been merged into the strands-agents/harness-sdk monorepo and will be archived shortly. All new development happens there. If this PR is still relevant, please recreate it against the monorepo. The code now lives under Apologies for the disruption, and thank you for contributing! |
|
Assessment: Approve Both previous review comments have been addressed well. The error is now explicitly wrapped with a descriptive message, and the missing test case for the incomplete stream path has been added. All three error scenarios are properly tested and the logic is correct. No further changes needed from a code quality perspective. |
| }) | ||
|
|
||
| it('preserves SyntaxError instead of overwriting with MaxTokensError when tool input JSON is malformed', async () => { | ||
| it('throws MaxTokenError when contentBlockStop arrives with truncated tool input JSON and stopReason is maxTokens', async () => { |
There was a problem hiding this comment.
Nit: typo in the test description — MaxTokenError should be MaxTokensError (plural, matching the class in errors.ts). The assertion itself is correct; just the it(...) string.
|
@cmbullock Hi , thank you again for raising this PR.
|
I can probably get this taken care of in a few days - if you have an opportunity to first, please feel free to!
Absolutely, just a small oversight! 👍 |
Description
streamAggregatedcurrently throws aSyntaxErrorfromJSON.parseof a truncated tool input buffer before the subsequentmodelMessageStopEventis processed. When the truncation was caused bymaxTokens, the resultingModelErrorcarrying aSyntaxErrorcause obscures the actionable root cause - users see a misleading "Expected ',' or '}'..." parse error instead of the documentedMaxTokensErrorpath.The same scenario surfaces
MaxTokensReachedExceptioncorrectly in the Python SDK - the TS SDK regressed in #680.Related Issues
strands-agents/harness-sdk#2451
Documentation PR
Type of Change
Bug fix
Testing
How have you tested the change?
npm run checkChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.