-
Notifications
You must be signed in to change notification settings - Fork 103
fix: surface MaxTokensError when max_tokens truncates tool input JSON #1065
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -323,29 +323,79 @@ describe('Model', () => { | |
| ) | ||
| }) | ||
|
|
||
| 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 () => { | ||
| const provider = new TestModelProvider(async function* () { | ||
| yield { type: 'modelMessageStartEvent', role: 'assistant' } | ||
| yield { | ||
| type: 'modelContentBlockStartEvent', | ||
| start: { type: 'toolUseStart', toolUseId: 'tool1', name: 'get_weather' }, | ||
| start: { type: 'toolUseStart', toolUseId: 't', name: 'tool' }, | ||
| } | ||
| yield { | ||
| type: 'modelContentBlockDeltaEvent', | ||
| delta: { type: 'toolUseInputDelta', input: '{invalid json' }, | ||
| delta: { type: 'toolUseInputDelta', input: '{"field": "value"' }, | ||
| } | ||
| yield { type: 'modelContentBlockStopEvent' } | ||
| yield { type: 'modelMessageStopEvent', stopReason: 'maxTokens' } | ||
| }) | ||
|
|
||
| const messages = [new Message({ role: 'user', content: [new TextBlock('Hi')] })] | ||
|
|
||
| await expect(async () => await collectGenerator(provider.streamAggregated(messages))).rejects.toThrow( | ||
| MaxTokensError | ||
| ) | ||
| }) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: Missing test coverage for the path where the stream ends without a Suggestion: Add a test case where the generator yields a tool input delta +
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test added |
||
|
|
||
| it('surfaces SyntaxError as cause when tool input JSON is malformed and stopReason is not maxTokens', async () => { | ||
| const provider = new TestModelProvider(async function* () { | ||
| yield { type: 'modelMessageStartEvent', role: 'assistant' } | ||
| yield { | ||
| type: 'modelContentBlockStartEvent', | ||
| start: { type: 'toolUseStart', toolUseId: 't', name: 'tool' }, | ||
| } | ||
| yield { | ||
| type: 'modelContentBlockDeltaEvent', | ||
| delta: { type: 'toolUseInputDelta', input: '{invalid json' }, | ||
| } | ||
| yield { type: 'modelContentBlockStopEvent' } | ||
| yield { type: 'modelMessageStopEvent', stopReason: 'toolUse' } | ||
| }) | ||
|
|
||
| const messages = [new Message({ role: 'user', content: [new TextBlock('Hi')] })] | ||
|
|
||
| try { | ||
| await collectGenerator(provider.streamAggregated(messages)) | ||
| expect.fail('Expected error to be thrown') | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(ModelError) | ||
| expect(error).not.toBeInstanceOf(MaxTokensError) | ||
| expect((error as ModelError).message).toBe('unable to parse tool input JSON') | ||
| expect((error as ModelError).cause).toBeInstanceOf(SyntaxError) | ||
| } | ||
| }) | ||
|
|
||
| it('attaches SyntaxError as cause when stream ends without a stop event after malformed tool input', async () => { | ||
| const provider = new TestModelProvider(async function* () { | ||
| yield { type: 'modelMessageStartEvent', role: 'assistant' } | ||
| yield { | ||
| type: 'modelContentBlockStartEvent', | ||
| start: { type: 'toolUseStart', toolUseId: 't', name: 'tool' }, | ||
| } | ||
| yield { | ||
| type: 'modelContentBlockDeltaEvent', | ||
| delta: { type: 'toolUseInputDelta', input: '{invalid json' }, | ||
| } | ||
| yield { type: 'modelContentBlockStopEvent' } | ||
| }) | ||
|
|
||
| const messages = [new Message({ role: 'user', content: [new TextBlock('Hi')] })] | ||
|
|
||
| try { | ||
| await collectGenerator(provider.streamAggregated(messages)) | ||
| expect.fail('Expected error to be thrown') | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(ModelError) | ||
| expect(error).not.toBeInstanceOf(MaxTokensError) | ||
| expect((error as ModelError).message).toBe('Stream ended without completing a message') | ||
| expect((error as ModelError).cause).toBeInstanceOf(SyntaxError) | ||
| } | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: typo in the test description —
MaxTokenErrorshould beMaxTokensError(plural, matching the class inerrors.ts). The assertion itself is correct; just theit(...)string.