Repository navigation
fix(js): skip re-registering tools already registered in the same registry - #6387
aslamalkarywk7 wants to merge 2 commits into
Conversation
…istry (WIP, awaiting issue approval) Do not open PR before maintainer approval on #6381.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request exports and updates maybeRegisterDynamicTools to prevent re-registering dynamic tools that are already registered, addressing a bug where 'already registered' errors are thrown. It also adds corresponding unit tests. The review feedback correctly identifies that since generate() wraps the registry in a child registry, the current strict equality check on the registry will bypass the guard. The reviewer suggests traversing the parent registry chain to resolve this and adding a unit test to cover the parent-child registry scenario.
generate() wraps the registry in a child registry, so a strict equality check against the tool's __registry always misses for tools registered in the parent. Walk the parent chain instead and cover it with a parent-registry unit test. Signed-off-by: aslamalkarywk7 <[email protected]>
|
Hi maintainers - the bot feedback here is already addressed on the current head: maybeRegisterDynamicTools now walks the parent registry chain instead of a strict equality check (generate() wraps the registry in a child), covered by a new parent-registry unit test. Verified locally: 33/33 tests pass in js/ai/tests/generate/generate_test.ts. Could a human reviewer please take a look when you get a chance? Thanks! |
Fixes #6381
A v2 plugin init() may return tool() actions, which bypass defineTool() and stay flagged dynamic while already registered. Passing them to ai.generate({ tools: [...] }) made maybeRegisterDynamicTools re-register them every time, tripping already-registered errors for what is a no-op overwrite.
This skips re-registration when the action is already registered in the same registry (t.__registry === registry), exports maybeRegisterDynamicTools (matching its middleware sibling), and adds unit tests (registers new, skips same-registry, still registers into a different registry).
Note: I chose the register-path guard over exporting a new plugin-facing tool factory — happy to switch if you prefer the factory.