Skip to content

Conversation

@pokey
Copy link
Contributor

@pokey pokey commented Oct 3, 2025

@changeset-bot
Copy link

changeset-bot bot commented Oct 3, 2025

⚠️ No Changeset found

Latest commit: aed9402

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 3, 2025

@pokey is attempting to deploy a commit to the LangChain Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Oct 3, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Updated (UTC)
langchainjs-docs Ignored Ignored Oct 14, 2025 8:47am

@pokey
Copy link
Contributor Author

pokey commented Oct 3, 2025

Do we want an integration test?

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@christian-bromann
Copy link
Member

Do we want an integration test?

If you can come up with an easy one that validates the functionality and is stable, let's do it!

Copy link
Member

@hntrl hntrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an integration test would be handy! And because this is a new feature add it would be good to add a changeset

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

am fine to keep this, but as commentary I'm not sure how much benefit we get from testing invocationParams directly (it's only part of the messages + options -> sdk payload "chain")

ideal state is the conversion logic of messages + options is collapsed into something like getRequestPayload and that's what we test (this is how python does it iirc). Not needed for this PR though.

ok I'll get off my soap box now

@pokey
Copy link
Contributor Author

pokey commented Oct 7, 2025

I have a forthcoming PR that will add integration tests for this PR as well as #9108, because the end-to-end won't work without both changes and another forthcoming change.

Which argues that maybe I went a bit too fine-grained with my PRs here 😅. Hopefully made them nice and easy to review tho

Are we ok to merge this one without the integration test and I'll fast follow with the integration test?

pokey added a commit to pokey/langchainjs that referenced this pull request Oct 8, 2025
Anthropic has a bug where if we pass them back one of their bash_code_execution_output blocks which contains a file output, they return a 500. I have reported the error to them, but in the meantime this works around the problem by dropping the problematic content blocks before we send them to anthropic

Also adds an integration test that exercises this PR as well as:

- langchain-ai#9108
- langchain-ai#9109
Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@christian-bromann christian-bromann merged commit 0ea1111 into langchain-ai:v1 Oct 14, 2025
32 checks passed
pokey added a commit to pokey/langchainjs that referenced this pull request Oct 14, 2025
Anthropic has a bug where if we pass them back one of their bash_code_execution_output blocks which contains a file output, they return a 500. I have reported the error to them, but in the meantime this works around the problem by dropping the problematic content blocks before we send them to anthropic

Also adds an integration test that exercises this PR as well as:

- langchain-ai#9108
- langchain-ai#9109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants