-
Notifications
You must be signed in to change notification settings - Fork 53.3k
Fix ollama url path and thinking tokens #23963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Fix ollama url path and thinking tokens #23963
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 Open source vulnerabilities detected - high severity
Aikido detected 3 vulnerabilities across 3 packages, it includes 3 high vulnerabilities.
Remediation Aikido suggests bumping the vulnerable packages to a safe version.
View details in Aikido Security
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 8 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/@n8n/nodes-langchain/nodes/vendors/Ollama/actions/image/analyze.operation.ts">
<violation number="1" location="packages/@n8n/nodes-langchain/nodes/vendors/Ollama/actions/image/analyze.operation.ts:439">
P1: Using `!!` to convert `processedOptions.think` treats `undefined` (option not set) the same as `false` (explicitly disabled). Per the PR description, the intent is to only send `think: false` when explicitly disabled. When the option isn't added, `undefined` should result in not sending the think field at all.</violation>
</file>
<file name="packages/@n8n/nodes-langchain/nodes/vendors/Ollama/actions/text/message.operation.ts">
<violation number="1" location="packages/@n8n/nodes-langchain/nodes/vendors/Ollama/actions/text/message.operation.ts:404">
P1: The double negation `!!processedOptions.think` incorrectly treats `undefined` (option not set) the same as `false` (explicitly disabled). When the user doesn't add the Thinking option, `processedOptions.think` is `undefined`, which becomes `false` via `!!`, causing `body.think = false` to be sent. This contradicts the PR's intent to only send `think: false` when explicitly disabled. Use strict equality check instead.</violation>
</file>
<file name="packages/@n8n/nodes-langchain/nodes/vendors/Ollama/transport/index.ts">
<violation number="1" location="packages/@n8n/nodes-langchain/nodes/vendors/Ollama/transport/index.ts:32">
P1: URL concatenation doesn't handle trailing slash on `baseUrl`. When `baseUrl` ends with `/` (e.g., `http://localhost:11434/`) and `endpoint` starts with `/` (e.g., `/api/tags`), this produces a double slash (`http://localhost:11434//api/tags`). The existing test 'should handle baseUrl with trailing slash' expects the normalized URL without double slashes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
packages/@n8n/nodes-langchain/nodes/vendors/Ollama/actions/image/analyze.operation.ts
Outdated
Show resolved
Hide resolved
packages/@n8n/nodes-langchain/nodes/vendors/Ollama/actions/text/message.operation.ts
Outdated
Show resolved
Hide resolved
packages/@n8n/nodes-langchain/nodes/vendors/Ollama/transport/index.ts
Outdated
Show resolved
Hide resolved
|
Hey @Gulianrdgd, Thank you for your contribution. We appreciate the time and effort you’ve taken to submit this pull request. Before we can proceed, please ensure the following: Regarding new nodes: If your node integrates with an AI service that you own or represent, please email [email protected] and we will be happy to discuss the best approach. About review timelines: Thank you again for contributing to n8n. |
…ndex.ts Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
…-tokens' into fix-ollama-url-path-and-thinking-tokens
|
Here is the langchain PR btw, but they tend to not release very quickly |
Summary
This PR fixes two issues with the Ollama nodes:
@langchain/ollamapackage to the latest version and added an option to disable thinking output. I have tested this extensively and it works really well. However currently in the AI Agent node with a chat model, we cannot see the thinking output. I have written some code for that but it is rather ugly so I did not want to include it into this pull request. I noticed that the other AI models also do not show the thinking tokens, but I do think there is some use for it. If there is maybe we can think about changing the AI agent node.We only send the think field if it is explicitly set to false. The default on Ollama is set to true. When we send
think: trueand the model does not support it, we get an error.The think field should also be there on the Ollama node instead of only the OllamaChat node but sadly this is not yet implemented in LangChain. I will create a pull request there and when they have a new release I will create a new pull reuqest here.
https://openwebui/ollama+/api/tagsresulted inhttps://openwebui/api/tagsinstead ofhttps://openwebui/ollama/api/tags. Simplified the URL composition logic to handle paths correctly. I know that technically this should be another pull request but the change is so small that I included it here.Thinking on:

Thinking off:

Related Linear tickets, Github issues, and Community forum posts
None
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)