docs(langchain): add finance example to getting started - #86
Conversation
|
Droid finished @itsakhilyou's task —— View job Droid review complete. Summary: Clean PR that updates the langchain package to document |
b43d2c8 to
3055e9e
Compare
|
Droid finished @itsakhilyou's task —— View job Droid review complete. Summary: The PR introduces individual tool functions with a critical resource leak: the MCP client is never closed when getTools() fails or returns empty, leaking HTTP connections under error conditions. The README incorrectly claims each invoke() creates a new connection, but the tool is actually single-use — a second invoke silently breaks on a closed client. A test claiming to verify invoke-error cleanup tests a different code path, leaving the critical finally-block untested. |
| const client = await createYouClient({ ...config, tools: 'you-search' }) | ||
| const tools = await client.getTools() | ||
| const tool = tools[0] | ||
|
|
||
| if (!tool) { | ||
| throw new Error('you-search tool not found') | ||
| } | ||
|
|
||
| const originalInvoke = tool.invoke.bind(tool) | ||
| tool.invoke = async (input, options) => { | ||
| try { | ||
| return await originalInvoke(input, options) | ||
| } finally { | ||
| await client.close() | ||
| } | ||
| } | ||
|
|
||
| return tool | ||
| } |
There was a problem hiding this comment.
[P1] [security] MCP client leaked when getTools() fails or returns empty
In all four individual tool functions (youSearch line 47, youResearch line 78, youContents line 109, youFinance line 140), the MCP client is created via createYouClient() but never closed if client.getTools() throws (e.g., network error, auth failure) or returns an empty array (causing the if (!tool) guard to throw). The client holds an active HTTP connection to api.you.com that remains open. Under transient failures in a retry loop, leaked connections accumulate and can exhaust file descriptors (DoS via resource exhaustion). Wrap the post-creation logic in a try/catch that calls client.close() before re-throwing.
| const client = await createYouClient({ ...config, tools: 'you-search' }) | |
| const tools = await client.getTools() | |
| const tool = tools[0] | |
| if (!tool) { | |
| throw new Error('you-search tool not found') | |
| } | |
| const originalInvoke = tool.invoke.bind(tool) | |
| tool.invoke = async (input, options) => { | |
| try { | |
| return await originalInvoke(input, options) | |
| } finally { | |
| await client.close() | |
| } | |
| } | |
| return tool | |
| } | |
| const client = await createYouClient({ ...config, tools: 'you-search' }) | |
| try { | |
| const tools = await client.getTools() | |
| const tool = tools[0] | |
| if (!tool) { | |
| throw new Error('you-search tool not found') | |
| } | |
| const originalInvoke = tool.invoke.bind(tool) | |
| tool.invoke = async (input, options) => { | |
| try { | |
| return await originalInvoke(input, options) | |
| } finally { | |
| await client.close() | |
| } | |
| } | |
| return tool | |
| } catch (error) { | |
| await client.close() | |
| throw error | |
| } | |
| } |
| const financeResult = await financeTool.invoke({ query: 'Apple cash flow analysis' }); | ||
| ``` | ||
|
|
||
| **Note:** Each `invoke()` call creates and closes a new HTTP connection. For multiple invocations in a loop, use `createYouClient()` and manage the client lifecycle manually for better performance. |
There was a problem hiding this comment.
[P1] README incorrectly states each invoke creates a new connection — tool is actually single-use
The README claims "Each invoke() call creates and closes a new HTTP connection", implying the tool can be invoked multiple times with each call getting a fresh connection. However, the implementation creates ONE MultiServerMCPClient when the tool function is called (e.g., youSearch()), monkey-patches tool.invoke to call client.close() in a finally block, and never recreates the client. After the first invoke, the client is closed and a second invoke on the same tool object will call originalInvoke over a closed MCP connection, producing an opaque transport error. Users following the README will write code that breaks on the second invocation. The documentation should accurately describe the tool as single-use, or the implementation should be changed to create a fresh client per invoke.
| **Note:** Each `invoke()` call creates and closes a new HTTP connection. For multiple invocations in a loop, use `createYouClient()` and manage the client lifecycle manually for better performance. | |
| **Note:** The MCP client is automatically closed after the first `invoke()` call, making each tool single-use. For multiple invocations, create a new tool for each call or use `createYouClient()` to manage the client lifecycle manually. |
| expect(closeMock).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('tool invoke closes client even on error', async () => { |
There was a problem hiding this comment.
[P2] Test "tool invoke closes client even on error" is mislabeled and doesn't test the invoke error path
The test name implies it verifies that client.close() is called when tool.invoke() throws during execution, but it actually mocks getToolsMock to throw — this causes the error during tool creation (before invoke is ever wrapped or called). The test never reaches the invoke monkey-patch, so the finally { await client.close() } cleanup path inside invoke is completely untested. Additionally, the test doesn't assert closeMock was called, and the client IS leaked in this error path (consistent with the resource-leak finding above). The invoke-error path (where originalInvoke throws and client.close() runs in finally) has zero test coverage despite being the critical cleanup guarantee.
3055e9e to
a9f8f4e
Compare
|
Droid finished @itsakhilyou's task —— View job Droid review complete. Summary: Clean PR that adds you-finance to the default tool set in the LangChain package, bumps the version to 1.3.0, and updates the test mock accordingly. No source code changes to main.ts — default tools are determined server-side, so the client implementation is unaffected. No bugs or security issues found. |
a9f8f4e to
9850cfb
Compare
|
Droid finished @itsakhilyou's task —— View job Droid review complete. Summary: The PR adds a finance usage example to the getting-started section while the README still explicitly documents you-finance as a non-default tool in multiple locations (lines 200, 289), creating a contradiction that will confuse users who follow the guide. The PR title and description claim to add you-finance as a 4th default tool, but the README text and default-tool references (lines 3, 95) still list only 3 defaults — the documentation needs to be internally consistent before merging. |
| - "What are the latest developments in quantum computing?" | ||
| - "Research the pros and cons of WebAssembly vs JavaScript" | ||
| - "Extract and analyze the content from https://anthropic.com" | ||
| - "Compare the free cash flow of Apple, Microsoft, and Alphabet" |
There was a problem hiding this comment.
[P1] Finance example in "Test your setup" won't work with default tools
The PR adds "Compare the free cash flow of Apple, Microsoft, and Alphabet" to the getting-started test examples, but the getting-started guide uses createYouClient() without a tools parameter, which only returns the default tool set (you-search, you-research, you-contents per lines 3, 95, 190). Lines 200 and 289 both state you-finance is not a default tool. A user following the getting-started guide will try the finance example, but no finance tool will be available — the agent will either fall back to you-search (producing inferior results) or fail to answer. Either remove this example from the getting-started section or update all default-tool references (lines 3, 95, 190) and remove the "not included in the default tool set" statements (lines 200, 289) to reflect you-finance as a default tool.
| { | ||
| "name": "@youdotcom-oss/langchain", | ||
| "version": "1.2.2", | ||
| "version": "1.3.0", |
There was a problem hiding this comment.
We actually never manually bump packages this way. We have actions for this.
|
|
||
| `you-finance` is not included in the default tool set. Request it explicitly with `tools`. | ||
|
|
||
| Lead with the smallest explicit form when you only want finance: |
There was a problem hiding this comment.
Why move this outside of the configuration block?
| - "What are the latest developments in quantum computing?" | ||
| - "Research the pros and cons of WebAssembly vs JavaScript" | ||
| - "Extract and analyze the content from https://anthropic.com" | ||
| - "Compare the free cash flow of Apple, Microsoft, and Alphabet" |
| Lead with the smallest explicit form when you only want finance: | ||
|
|
||
| ```typescript | ||
| const client = await createYouClient({ | ||
| tools: 'you-finance', | ||
| }); | ||
|
|
||
| const financeTools = await client.getTools(); | ||
| ``` | ||
|
|
||
| If you want the default tools plus finance, request all of them explicitly: | ||
|
|
||
| ```typescript | ||
| const client = await createYouClient({ | ||
| tools: ['you-search', 'you-research', 'you-contents', 'you-finance'], | ||
| }); | ||
|
|
||
| const tools = await client.getTools(); | ||
| ``` | ||
|
|
There was a problem hiding this comment.
| - `you-finance` | ||
|
|
||
| --- | ||
| `you-finance` is not included in the default tool set. Request it explicitly with `tools`. |
There was a problem hiding this comment.
9850cfb to
b73f3b7
Compare
|
Droid finished @itsakhilyou's task —— View job Droid review complete. Summary: Clean, minimal documentation-only PR that adds a single finance example prompt to the getting-started section. No code-level or security findings. The prior concern about you-finance not being a default tool was already flagged and acknowledged by the maintainer, who explicitly approved the added line. |
b73f3b7 to
db3d256
Compare
|
Droid finished @itsakhilyou's task —— View job Droid review complete. Summary: No candidates to validate. This is a minimal docs-only PR that adds a single finance example prompt to the getting-started section of the README. The already-reported concern about you-finance requiring a non-default tool (comment 3336202811) remains the only substantive observation. |
|
Droid encountered an error —— View job Droid is reviewing code and running a security check… |
146c5f3 to
d0a7f01
Compare
|
Droid finished @itsakhilyou's task —— View job Droid review complete. Summary: Documentation-only PR that adds a finance example to the getting-started section and restructures the README for clarity. The primary concern about the finance prompt not working with default tools was already flagged and explicitly approved by the maintainer. No new high-confidence bugs or security issues found. |
Summary
Add a
you-financeusage example to the getting started test prompts.Change
Added to the "Test your setup" section:
Note
The
you-financetool must be requested explicitly via thetoolsparameter (not included in default tools). The existing documentation already covers this.Example Usage