Brace/anthropic tools #4978
Brace/anthropic tools #4978
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dosubot
bot
added
size:XXL
![dosubot[bot]](https://avatars.githubusercontent.com/in/324583?s=60&v=4){kind=small}
| @@ -0,0 +1,100 @@ | |||
| import { ChatAnthropic } from "@langchain/anthropic"; | |||
There was a problem hiding this comment.
Hey there! 👋 I've flagged this PR for your review because it adds code that explicitly accesses an environment variable using process.env. Please take a look and ensure that the handling of environment variables aligns with best practices. Let me know if you need any further assistance with this!
| @@ -0,0 +1,64 @@ | |||
| import { ChatAnthropic } from "@langchain/anthropic"; | |||
There was a problem hiding this comment.
Hey there! I've reviewed the code and noticed that the added code explicitly accesses an environment variable via process.env. I've flagged this for your review to ensure it aligns with security best practices. Let me know if you need further assistance with this.
| @@ -0,0 +1,90 @@ | |||
| import { ChatAnthropic } from "@langchain/anthropic"; | |||
There was a problem hiding this comment.
Hey team, just a heads up that I've flagged the latest change in the PR for review. It explicitly accesses the ANTHROPIC_API_KEY environment variable using process.env, so it's important to ensure that this is handled securely. Keep up the great work!
| @@ -401,11 +615,7 @@ export class ChatAnthropicMessages< | |||
| const params = this.invocationParams(options); | |||
There was a problem hiding this comment.
Hey there! I noticed that this PR introduces a new external HTTP request using fetch or axios. I've flagged this change for your review to ensure it aligns with the project's requirements. Let me know if you have any questions or need further clarification!
| const headers = tools ? toolUseBetaHeader : undefined; | ||
| return { | ||
| signal: options.signal, | ||
| ...(body ? { body } : {}), |
There was a problem hiding this comment.
This is confusing but I guess we'll delete it soon anyway
| return { | ||
| signal: options.signal, | ||
| ...(body ? { body } : {}), | ||
| ...(headers ? { headers } : {}), |
| const requestOptions = this.invocationOptions( | ||
| { | ||
| ...params, | ||
| stream: false, |
| ): Promise<Stream<AnthropicMessageStreamEvent>> { | ||
| if (!this.streamingClient) { | ||
| const options = this.apiUrl ? { baseURL: this.apiUrl } : undefined; | ||
| const options_ = this.apiUrl ? { baseURL: this.apiUrl } : undefined; |
There was a problem hiding this comment.
For the future: I despise this pattern. Just call requestOptions or something
| return this.caller.callWithOptions( | ||
| { signal: options.signal }, | ||
| { signal: options.signal ?? undefined }, |
There was a problem hiding this comment.
signal in caller has to be signal or undefined, their sdk types it as signal, null or undefined and I'm using their types for this.
| const name = config?.name; | ||
| const method = config?.method; | ||
| const includeRaw = config?.includeRaw; | ||
| if (method === "jsonMode") { |
There was a problem hiding this comment.
We should make this type just string at some point
No description provided.