Skip to content

fix(chatopenai): support stop in base options#6472

Open
Javi-vahe wants to merge 1 commit into
FlowiseAI:mainfrom
Javi-vahe:fix/chatopenai-stop-base-options
Open

fix(chatopenai): support stop in base options#6472
Javi-vahe wants to merge 1 commit into
FlowiseAI:mainfrom
Javi-vahe:fix/chatopenai-stop-base-options

Conversation

@Javi-vahe
Copy link
Copy Markdown

Summary

  • Support stop when provided through ChatOpenAI Base Options.
  • Extract stop from parsed base options and apply it to the model options instead of sending it as a default header.
  • Preserve the existing Stop Sequence field behavior by only using Base Options stop when obj.stop is not already set.

Fixes #2499

Tests

  • git diff --check

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for parsing and applying the stop parameter from parsedBaseOptions in the ChatOpenAI component. The reviewer identified two critical issues: potential compatibility problems with reasoning models (which do not support stop sequences) and unexpected side effects from mutating the original input object. A code suggestion was provided to resolve both issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +289 to +306
if (parsedBaseOptions?.stop) {
const stop = parsedBaseOptions.stop

if (!obj.stop) {
obj.stop = Array.isArray(stop)
? stop.map((item) => String(item).trim()).filter(Boolean)
: String(stop)
.split(',')
.map((item) => item.trim())
.filter(Boolean)
}

delete parsedBaseOptions.stop

if (Object.keys(parsedBaseOptions).length === 0) {
parsedBaseOptions = undefined
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

There are two issues with this implementation:

  1. Reasoning Models Compatibility: OpenAI's reasoning models (e.g., o1 series) do not support stop sequences, which is why obj.stop is explicitly deleted on lines 266-268. However, because this new block runs after that check, any stop sequence provided in baseOptions will still be assigned to obj.stop, potentially causing API errors. We should ensure stop is not set for reasoning models.
  2. Input Object Mutation: If baseOptions is passed as an object, parsedBaseOptions holds a direct reference to it. Mutating it with delete parsedBaseOptions.stop will modify the original nodeData.inputs.baseOptions object, which can cause unexpected side effects if the node is re-initialized or reused. We should shallow copy the object before deleting the property.

Here is the suggested fix to address both issues:

        if (parsedBaseOptions?.stop) {
            const stop = parsedBaseOptions.stop

            if (!obj.stop && !isReasoningModelOpenAI(modelName)) {
                obj.stop = Array.isArray(stop)
                    ? stop.map((item) => String(item).trim()).filter(Boolean)
                    : String(stop)
                          .split(',')
                          .map((item) => item.trim())
                          .filter(Boolean)
            }

            if (typeof baseOptions === 'object') {
                parsedBaseOptions = { ...parsedBaseOptions }
            }
            delete parsedBaseOptions.stop

            if (Object.keys(parsedBaseOptions).length === 0) {
                parsedBaseOptions = undefined
            }
        }

@Javi-vahe
Copy link
Copy Markdown
Author

Hi maintainers, I opened this PR to address #2499.

The issue happens because Base Options are currently parsed into the OpenAI configuration/default headers path, so a stop value provided there does not reach the model invocation options. This PR extracts stop from parsed Base Options and applies it to obj.stop, while preserving the existing Stop Sequence field behavior.

Please let me know if you'd prefer a different approach.

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.

[BUG] Additional Parameters did not take effect.

1 participant