Repository navigation
feat: add maxRetries to the messages handler factories - #165
aknight-ld wants to merge 1 commit into
Conversation
a706687 to
77104e3
Compare
aknight-ld
added
the
api-compatible
a67619a to
718a1dd
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 718a1dd. Configure here.
718a1dd to
28bf5ad
Compare
28bf5ad to
8850922
Compare
jeffdupont
left a comment
jeffdupont
left a comment
There was a problem hiding this comment.
The maxRetries work itself matches ai-sdks-monorepo#88 on every §1.1a rule. Each of the five factories validates at construction, passes the value only when it's set, refuses it with llm/client/clientFactory where the count is a client setting, applies it per call in vercel-messages, and the wrappers forward it. The tests cover every bullet, including LangChain's AsyncCaller count.
Two changes needed, both as suggestions below:
- Revert the two renamed usage IDs (
openai-messages.createOpenAIHandler,langchain-messages.createLangChainHandler). §3.27 says IDs are frozen: "Renaming a function in one language does not change its ID". The registry (api-surface/usage-ids.txt) and the Python SDK still use the old IDs, so the rename would split usage counts by language. - Mark
validateMaxRetries@internallikecreateHandlerInternal, since only handler packages call it. After applying that suggestion, runyarn api:updateand commit the regeneratedpackages/client/etc/ai-server.api.md(it currently lists the function as@publicat :1102). I've asked on #88 for the spec list to mark it# internaltoo.
Heads-up: #190 removes the captureContent = false default from these factories, and a trial merge conflicts in the claude, langchain, openai and vercel messages handlers. Whichever lands second should keep #190's unset captureContent and its run-time handler.captureContent read, and add maxRetries on top. Also, the "A.22" in utils.test.ts:834 will need the new appendix number once #88 is renumbered.
| # + Name joining the public surface; may be exported or not | ||
|
|
||
| createOpenAIMessagesHandler usage=openai-messages.createOpenAIHandler | ||
| createOpenAIMessagesHandler usage=openai-messages.createOpenAIMessagesHandler |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| createOpenAIMessagesHandler usage=openai-messages.createOpenAIMessagesHandler | |
| createOpenAIMessagesHandler usage=openai-messages.createOpenAIHandler |
| # + Name joining the public surface; may be exported or not | ||
|
|
||
| createLangChainMessagesHandler usage=langchain-messages.createLangChainHandler | ||
| createLangChainMessagesHandler usage=langchain-messages.createLangChainMessagesHandler |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| createLangChainMessagesHandler usage=langchain-messages.createLangChainMessagesHandler | |
| createLangChainMessagesHandler usage=langchain-messages.createLangChainHandler |
| */ | ||
| maxRetries?: number; | ||
| } = {}): ProviderHandler { | ||
| reportUsage('openai-messages.createOpenAIMessagesHandler', LD_AI_PACKAGE_NAME, LD_AI_PACKAGE_VERSION); |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| reportUsage('openai-messages.createOpenAIMessagesHandler', LD_AI_PACKAGE_NAME, LD_AI_PACKAGE_VERSION); | |
| reportUsage('openai-messages.createOpenAIHandler', LD_AI_PACKAGE_NAME, LD_AI_PACKAGE_VERSION); |
| } = {}, | ||
| ): ProviderHandler { | ||
| reportUsage('langchain-messages.createLangChainHandler', LD_AI_PACKAGE_NAME, LD_AI_PACKAGE_VERSION); | ||
| reportUsage('langchain-messages.createLangChainMessagesHandler', LD_AI_PACKAGE_NAME, LD_AI_PACKAGE_VERSION); |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| reportUsage('langchain-messages.createLangChainMessagesHandler', LD_AI_PACKAGE_NAME, LD_AI_PACKAGE_VERSION); | |
| reportUsage('langchain-messages.createLangChainHandler', LD_AI_PACKAGE_NAME, LD_AI_PACKAGE_VERSION); |
| const CALLS: Record<string, Call[]> = { | ||
| 'openai-messages.openaiMessages': [(fn) => settle(fn('k', 'q', ctx))], | ||
| 'openai-messages.createOpenAIHandler': [(fn) => fire(() => fn())], | ||
| 'openai-messages.createOpenAIMessagesHandler': [(fn) => fire(() => fn())], |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| 'openai-messages.createOpenAIMessagesHandler': [(fn) => fire(() => fn())], | |
| 'openai-messages.createOpenAIHandler': [(fn) => fire(() => fn())], |
| const calls = client.track.mock.calls.filter((entry) => entry[0] === '$ld:ai:sdk:usage'); | ||
| expect(calls).toHaveLength(1); | ||
| expect(calls[0]?.[2]).toEqual(expectedPayload('openai-messages.createOpenAIHandler')); | ||
| expect(calls[0]?.[2]).toEqual(expectedPayload('openai-messages.createOpenAIMessagesHandler')); |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| expect(calls[0]?.[2]).toEqual(expectedPayload('openai-messages.createOpenAIMessagesHandler')); | |
| expect(calls[0]?.[2]).toEqual(expectedPayload('openai-messages.createOpenAIHandler')); |
| const CALLS: Record<string, Call[]> = { | ||
| 'langchain-messages.langchainMessages': [(fn) => settle(fn('k', 'q', ctx))], | ||
| 'langchain-messages.createLangChainHandler': [(fn) => fire(() => fn())], | ||
| 'langchain-messages.createLangChainMessagesHandler': [(fn) => fire(() => fn())], |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| 'langchain-messages.createLangChainMessagesHandler': [(fn) => fire(() => fn())], | |
| 'langchain-messages.createLangChainHandler': [(fn) => fire(() => fn())], |
| const calls = client.track.mock.calls.filter((entry) => entry[0] === '$ld:ai:sdk:usage'); | ||
| expect(calls).toHaveLength(1); | ||
| expect(calls[0]?.[2]).toEqual(expectedPayload('langchain-messages.createLangChainHandler')); | ||
| expect(calls[0]?.[2]).toEqual(expectedPayload('langchain-messages.createLangChainMessagesHandler')); |
There was a problem hiding this comment.
Keep the frozen usage ID here. §3.27 freezes usage IDs ("Renaming a function in one language does not change its ID"), and the registry and Python still use the old one.
| expect(calls[0]?.[2]).toEqual(expectedPayload('langchain-messages.createLangChainMessagesHandler')); | |
| expect(calls[0]?.[2]).toEqual(expectedPayload('langchain-messages.createLangChainHandler')); |
| * | ||
| * Throws `TypeError` for a value that is not an integer and `RangeError` for a negative one. Both | ||
| * messages name the option. | ||
| */ |
There was a problem hiding this comment.
Only the handler packages call this, so mark it @internal like createHandlerInternal (then yarn api:update).
| */ | |
| * | |
| * @internal Exported for the LaunchDarkly handler packages; applications should not call it. | |
| */ |
| - toSemconvFinishReason | ||
| TrackData | ||
| - TraverseVisitor | ||
| + validateMaxRetries # checks a messages handler factory's maxRetries option (A.22) |
There was a problem hiding this comment.
List it as internal to match the @internal tag, the same way as createHandlerInternal.
| + validateMaxRetries # checks a messages handler factory's maxRetries option (A.22) | |
| - validateMaxRetries # internal: plumbing for the handler packages; moves to the internal entry point |
|
Correction: please ignore my 7 "keep the frozen usage ID" suggestions. The freeze in TESTING.md §3.27 applies from 1.0.0 onward, so these IDs can still change, and your new IDs are the right ones. Keep them. I'm renaming the same IDs in the spec, Python and Go, plus |
Add in a max retries passthrough argument to let handler factories natively retry.
Note
Overview
Adds an optional
maxRetriessetting on every messages handler factory and convenience wrapper (Claude, OpenAI, LangChain, LiteLLM, Vercel), wired to the underlying vendor client or AI SDK call so callers can raise retries for rate-limited batch work without changingmodel.parameters.Introduces public
validateMaxRetrieson@launchdarkly/ai-server, used at handler construction to reject invalid values (non-integers, negatives, etc.) before provider SDKs would accept them silently. LangChain/LiteLLM error ifmaxRetriesis combined with a caller-supplied model/client, since retries must be set on that object instead.Also fixes SDK usage reporting IDs for
createLangChainMessagesHandler/createOpenAIMessagesHandler, adds §1.1a-style tests per package, and mocks the OTLP trace exporter insdk-usagetests to avoid CI flush timeouts.Reviewed by Cursor Bugbot for commit 8850922. Bugbot is set up for automated code reviews on this repo. Configure here.