Repository navigation
feat(AIC-3439): let wildcard handlers accept only named providers - #111
andrewklatzke wants to merge 1 commit into
Conversation
A LangChain adapter used for one provider no longer has to match every provider, and two scoped wildcards of the same mode can be registered together. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Human message: I think this is a "nice to have" not necessarily "must do it". I'm thinking of situations where, for example, a user might want to use the Google ADK and scope it only to gemini ( You can technically achieve this now by properly including the right subset of handlers, but this makes it more obvious/colocates it with the code if you want to scope them down. tldr; if we think this is confusing or don't want it, we can close it |
jeffdupont
left a comment
There was a problem hiding this comment.
Selection is right in config(), in graph node selection, and in the three judge entry points. The registry keeps scoped wildcards apart the way the PR body says. One judge path skips the allowlist (1 below), and the registry orders handlers differently from Python #136 (2). Those two need fixing before this merges.
At 6cd4ce42, build, tsc, Biome and Sherif are clean, and yarn test gives 1177 passed. I trial-merged it onto today's main (#73, #68 and the rest) and got no conflicts. The merged tree gives 1325 passed, with typecheck and Biome clean. The #73 parameter allowlists in langchain-messages and langchain-agents handler.ts merge without trouble next to the new providers option, and the factory signatures are unchanged.
I probed selectModeHandler, the registry and the judge functions directly:
- An exact handler beats a scoped wildcard, and a scoped wildcard beats an unscoped one, whatever the registration order.
- The shortest matching list wins. On equal lengths the earlier handler wins.
- Names match exactly, so
bedrockdoesn't match['Bedrock']. Exact handlers follow the same rule (client.ts:39-44). [],['']and[' ']throw atcreateHandler, and the message mentionsproviders.- If a scoped wildcard doesn't list the provider,
config()throwsHandler for provider OpenAI not found. That's the same message as having no handler at all, and Python gives the same string. - Two scoped wildcards of one mode both stay registered. Registering the same set again, in either order, warns and replaces.
['Bedrock', 'Bedrock']counts as the same identity as['Bedrock'].compose(a, b)letsbwin on a matching set and keeps different sets. runJudges,buildJudgeTasksandrunJudgeall pick the shortest list. Each falls back to a scoped agent handler withcollapseMessages: true, and each skips a judge whose provider isn't in any list.
I tried 24 mutations, and 20 of them fail a test. The 4 that get through are in the inline comments.
- The graph-level judge ignores the allowlist. graph.ts:796-807 calls
runJudgeswithouthandlers, sorunJudgestakes thejudgeHandler = handlerbranch and never checks the provider (judges.ts:157-158). I rangraph().invoke()with one handler,['*', 'messages']scoped to['Bedrock'], and agraphJudgewhose provider is Anthropic. The Bedrock-only handler ran the Anthropic judge. Adding an Anthropic-scoped handler didn't help, because the root handler still ran it. The missinghandlersargument predates this PR. Before, though, it only affected which of two acceptable handlers ran. Now it lets a scoped handler run a provider it was set up to refuse. Python passeshandlers=resolved_handlerson the same call (graph.py:1016-1024 and :1238-1250). Please passhandlers: judgeHandlersand add agraphJudgetest with a provider outside the list. - Re-registering moves a handler to the end, but Python replaces it in place. registry.ts:29 filters out the old entry and pushes the new one. Python's registry.py:74 assigns into the same index. Ties go to the earlier registration, so the two SDKs now pick different handlers. As a repro, register
['Bedrock', 'Gemini'], then['Bedrock', 'Anthropic'], then['Bedrock', 'Gemini']again. JS ends up with[BA, BG], and a Bedrock config runsBA. Python keeps[BG, BA]and runsBG.composeshows the same split: JS gives[aU, bB, bA], Python[bB, aU, bA]. Before this PR it didn't matter, because only one wildcard per mode could be registered. Monorepo #39 doesn't say which is right. I'd go with in place, so a replacement takes the slot of what it replaces. That needs a line in #39 §3.6 and a test in both SDKs. - Repeated names count toward list length. The registry key collapses
['Bedrock', 'Bedrock']to['Bedrock'](registry.ts:6), butbestWildcardcomparesallow.length(utils.ts:92). So['Bedrock', 'Bedrock']ties with['Bedrock', 'Anthropic']and loses on registration order, even though the registry treats it as the narrower list. Python does the same (utils.py:97). Deduping increateHandlerfixes this, and then #39 can say "fewest distinct names". createHandlerkeeps the caller's array. utils.ts:60 stores the array itself, not a copy. If the caller later pushes'OpenAI'onto it, an OpenAI config routes to that handler, and its registry identity changes too. Python copies into a tuple.Object.freeze([...new Set(providers)])would fix 3 and 4 together.
Questions and smaller items
- The
createHandlersignature freezes at 1.0.providersis the fifth positional argument, so a caller writescreateHandler(pf, fn, undefined, undefined, ['Bedrock']), and #39's A.14 has to explain theundefineds. Python made it keyword-only. An options object ({ stream, captureContent, providers }) is harder to get wrong, but we can't switch to one after 1.0 without a major release. I'm fine either way, but let's decide now. The name and type match Python (providers,readonly string[], and Python'sSequence[str]stored as a tuple). The selection helpers aren't exported from index.ts, which is right. - API report.
mainhas no API Extractor setup yet, because it's on #71's branch. I trial-merged this PR into #71 (c27005cc). There were no conflicts, butapi:checkfails on two lines:createHandler(..., providers?: readonly string[])andProviderHandler.providers?. Whichever of the two lands second needsyarn api:update. - Whitespace.
[' Bedrock']is accepted and never matches anything. Python's docstring says surrounding whitespace is rejected, but its code accepts it too. Either both should rejectname !== name.trim(), or the claim should go. - Case. Routing is exact, which fits exact handlers and #39. The LangChain handlers lower-case the provider themselves (langchain-messages handler.ts:70 and :198), though. So a config with
provider.name = 'bedrock'would take the Bedrock model path, but it won't route to['Bedrock']. That's fine if LaunchDarkly always serves the canonical spelling, which I haven't checked. - Exact handlers that have a list. Routing ignores the list on an exact handler, but the registry key still includes it (registry.ts:5-6). So
['OpenAI', 'messages']with a list and without one both stay registered, and the first one wins. #39 says two exact handlers collide. Python does the same thing. I'd leaveprovidersout of the key whenprovidesFor[0] !== '*'. - Calling
createHandlertwice on one function.createHandlerchanges the function in place, so two calls with different lists return the same object, holding the last list. A later unscoped call on the same function keeps the old list, because utils.ts:60 only sets the property when one is passed.captureContentalready works this way. The LangChain factories build a new closure each time, so they aren't affected. A JSDoc note is enough. - The priority comment at judges.ts:132-138 doesn't mention scoped lists. Nit.
The judge example (from the PR body)
examples/judge.ts:56 resolves ./judge-worker.js, but only examples/judge-worker.ts exists. Both files date from the initial commit (977dba8), and this PR doesn't touch examples/, so the problem already exists on main. I didn't run it.
Parity with python-ai-sdk #136
On my probe cases, the two SDKs agree on selection order, ties, case, a missing provider (undefined / None), empty and blank lists, the error strings and set identity. They differ in five places:
- re-registration position (2)
- the graph-level judge (1)
- copying the caller's list (4)
- identity of
['a,b']vs['a', 'b']: JS joins with commas, so these collide and one replaces the other (registry.ts:6), while Python'sfrozensetkeeps both - a bare string or a non-string name: Python raises
ValueErrormentioningproviders, while JS throws aTypeError(providers.some is not a function). That's acceptable, since the TS types already rule it out.
Python also routes judges in its evaluations runner. JS has no runner, and #39's A.14 says so.
|
|
||
| const wildcard = handlers.find((h) => h.providesFor?.[0] === '*' && h.providesFor?.[1] === mode); | ||
| if (wildcard) return wildcard; | ||
| const chosen = selectModeHandler(handlers, provider, mode); |
There was a problem hiding this comment.
Node selection is right. The graphJudge call at :796-807 doesn't pass handlers, though, so runJudges runs the judge on the root handler without checking the provider (judges.ts:157-158). I set up a single ['*', 'messages'] handler scoped to ['Bedrock'] and an Anthropic graphJudge, and graph().invoke() called the scoped handler for the judge. Python passes handlers=resolved_handlers there. Could you pass handlers: judgeHandlers and add a test?
| this.handlers = this.handlers.filter( | ||
| (h) => !h.providesFor || `${h.providesFor[0]}:${h.providesFor[1]}` !== key, | ||
| ); | ||
| this.handlers = this.handlers.filter((h) => registrationKey(h) !== key); |
There was a problem hiding this comment.
This removes the old entry and appends the new one. Python's registry replaces in the same slot (registry.py:74). Since ties now go to the earlier registration, the SDKs pick differently. For example, register BG, then BA, then BG again: JS runs BA for Bedrock and Python runs BG. compose differs the same way. Can we pick one, in place I'd say, and write it into #39 §3.6?
| /** `providesFor` plus the allowlist as a set. Order and repeated names do not make a new identity. */ | ||
| function registrationKey(handler: ProviderHandler): string | undefined { | ||
| if (!handler.providesFor) return undefined; | ||
| const base = `${handler.providesFor[0]}:${handler.providesFor[1]}`; |
There was a problem hiding this comment.
Two small identity things. (a) Joining with , makes ['a,b'] and ['a', 'b'] the same key, so one replaces the other. Python's frozenset keeps both. JSON.stringify of the sorted set avoids that. (b) The key includes providers on exact handlers too, so ['OpenAI', 'messages'] with a list and without one both stay registered, and the first one wins. #39 says two exact handlers collide. I'd only add the list when providesFor[0] === '*'.
| if (!unscoped) unscoped = handler; | ||
| continue; | ||
| } | ||
| if (!bestScoped || allow.length < bestScoped.length) { |
There was a problem hiding this comment.
allow.length counts repeated names, but the registry treats ['Bedrock', 'Bedrock'] as ['Bedrock']. As a result ['Bedrock', 'Bedrock'] ties with ['Bedrock', 'Anthropic'] and loses on order. Deduping in createHandler fixes it. Python has the same issue (utils.py:97).
| ph.providesFor = providesFor; | ||
| if (streamHandler) ph.stream = streamHandler; | ||
| if (captureContent !== undefined) ph.captureContent = captureContent; | ||
| if (providers !== undefined) ph.providers = providers; |
There was a problem hiding this comment.
This stores the caller's array. If the caller pushes 'OpenAI' onto it after creating the handler, OpenAI configs route here and the registry key changes. Python copies into a tuple. Object.freeze([...new Set(providers)])? Separately: since this function is changed in place, a later createHandler on the same function without providers keeps the old list. captureContent already behaves that way, so a JSDoc note is enough.
| captureContent?: boolean, | ||
| providers?: readonly string[], | ||
| ): ProviderHandler { | ||
| if (providers !== undefined && (providers.length === 0 || providers.some((name) => name.trim() === ''))) { |
There was a problem hiding this comment.
[' Bedrock'] gets through and never matches. Python's docstring says surrounding whitespace is rejected, but neither SDK actually rejects it. Could both check name !== name.trim(), or should we drop the claim from Python and #39?
| expect(wildcard).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('falls back to an unscoped wildcard when no scoped list matches', async () => { |
There was a problem hiding this comment.
This test only covers the OpenAI half of #39's "unscoped wildcard is the fallback" bullet. If I change bestWildcard to return unscoped ?? bestScoped?.handler, so unscoped beats scoped, all 445 client tests still pass. Could you add the Bedrock half: [scoped(['Bedrock']), unscoped] with a Bedrock variation should run the scoped handler?
| if (wildcard) return wildcard; | ||
|
|
||
| if (!resolvedHandlers?.some((h) => h.providesFor?.[0] === provider || h.providesFor?.[0] === '*')) { | ||
| if (!resolvedHandlers?.some((handler) => coversProviderName(handler, provider))) { |
There was a problem hiding this comment.
If I put back the old providesFor[0] === '*' coverage check here, the tests still pass. The error then becomes ... with mode messages not found, but the test at :393 only matches /OpenAI/. Could it assert the full Handler for provider OpenAI not found, since that's the string both SDKs give?
| wildcardCovers, | ||
| } from './utils.js'; | ||
|
|
||
| function parentCovers(handler: ProviderHandler, provider: string | undefined): boolean { |
There was a problem hiding this comment.
No test covers parentCovers with a scoped parent. If I change it to providesFor[0] === '*', the suite passes. A test where the parent is a Bedrock-scoped wildcard, the judge is Anthropic, and no other handler applies would cover it.
|
|
||
| const judgeHandler = exactMatch ?? agentFallback; | ||
| if (!judgeHandler) return null; | ||
| const selected = selectJudgeExecution(handlers, judgeProvider, judgeMode); |
There was a problem hiding this comment.
runJudge, the async task runner, isn't tested with a scoped handler. If I swap this back to the old any-* match, nothing fails. #39 §3.13 wants the allowlist honoured on the prepareAsyncJudge path too. My probe returns null here for an Anthropic task with only a Bedrock-scoped handler, so the behaviour is right and only the test is missing.
Summary
providerslist, so a LangChain adapter used only for Bedrock acceptsconfig.provider.name === "Bedrock"and no other provider.[provider, mode]still wins. Among scoped wildcards that contain the name, the shortest list wins, then the earlier registration. An unscoped*is the fallback.providesForplus the provider set, so two scoped wildcards of the same mode can be registered together. Judges and graph selection use the same order.Test plan
tscon the touched packagesjudgeexample still looks forexamples/judge-worker.jswhile onlyjudge-worker.tsis present, so that case does not startMade with Cursor