From 24e8a258cd8fa1ff42c02cf15408079f1dbe07b0 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Wed, 7 Jan 2026 11:26:53 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=A7=20fix:=20Clean=20empty=20strings?= =?UTF-8?q?=20from=20`model=5Fparameters`=20for=20Agents/OpenAI=20(#11248)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- api/server/controllers/agents/v1.js | 9 ++ api/server/controllers/agents/v1.spec.js | 83 +++++++++++++++++++ packages/api/src/endpoints/openai/llm.spec.ts | 71 ++++++++++++++++ packages/api/src/endpoints/openai/llm.ts | 8 +- 4 files changed, 170 insertions(+), 1 deletion(-) diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 01e33f913b..5c2ac8bb06 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -109,6 +109,10 @@ const createAgentHandler = async (req, res) => { const validatedData = agentCreateSchema.parse(req.body); const { tools = [], ...agentData } = removeNullishValues(validatedData); + if (agentData.model_parameters && typeof agentData.model_parameters === 'object') { + agentData.model_parameters = removeNullishValues(agentData.model_parameters, true); + } + const { id: userId } = req.user; agentData.id = `agent_${nanoid()}`; @@ -259,6 +263,11 @@ const updateAgentHandler = async (req, res) => { // Preserve explicit null for avatar to allow resetting the avatar const { avatar: avatarField, _id, ...rest } = validatedData; const updateData = removeNullishValues(rest); + + if (updateData.model_parameters && typeof updateData.model_parameters === 'object') { + updateData.model_parameters = removeNullishValues(updateData.model_parameters, true); + } + if (avatarField === null) { updateData.avatar = avatarField; } diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index bfdee7eb79..1bcf6c2fa3 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -357,6 +357,46 @@ describe('Agent Controllers - Mass Assignment Protection', () => { }); }); + test('should remove empty strings from model_parameters (Issue Fix)', async () => { + // This tests the fix for empty strings being sent to API instead of being omitted + // When a user clears a numeric field (like max_tokens), it should be removed, not sent as "" + const dataWithEmptyModelParams = { + provider: 'azureOpenAI', + model: 'gpt-4', + name: 'Agent with Empty Model Params', + model_parameters: { + temperature: 0.7, // Valid number - should be preserved + max_tokens: '', // Empty string - should be removed + maxContextTokens: '', // Empty string - should be removed + topP: 0, // Zero value - should be preserved (not treated as empty) + frequency_penalty: '', // Empty string - should be removed + }, + }; + + mockReq.body = dataWithEmptyModelParams; + + await createAgentHandler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(201); + + const createdAgent = mockRes.json.mock.calls[0][0]; + expect(createdAgent.model_parameters).toBeDefined(); + // Valid numbers should be preserved + expect(createdAgent.model_parameters.temperature).toBe(0.7); + expect(createdAgent.model_parameters.topP).toBe(0); + // Empty strings should be removed + expect(createdAgent.model_parameters.max_tokens).toBeUndefined(); + expect(createdAgent.model_parameters.maxContextTokens).toBeUndefined(); + expect(createdAgent.model_parameters.frequency_penalty).toBeUndefined(); + + // Verify in database + const agentInDb = await Agent.findOne({ id: createdAgent.id }); + expect(agentInDb.model_parameters.temperature).toBe(0.7); + expect(agentInDb.model_parameters.topP).toBe(0); + expect(agentInDb.model_parameters.max_tokens).toBeUndefined(); + expect(agentInDb.model_parameters.maxContextTokens).toBeUndefined(); + }); + test('should handle invalid avatar format', async () => { const dataWithInvalidAvatar = { provider: 'openai', @@ -539,6 +579,49 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(updatedAgent.tool_resources.invalid_tool).toBeUndefined(); }); + test('should remove empty strings from model_parameters during update (Issue Fix)', async () => { + // First create an agent with valid model_parameters + await Agent.updateOne( + { id: existingAgentId }, + { + model_parameters: { + temperature: 0.5, + max_tokens: 1000, + maxContextTokens: 2000, + }, + }, + ); + + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + // Simulate user clearing the fields (sends empty strings) + mockReq.body = { + model_parameters: { + temperature: 0.7, // Change to new value + max_tokens: '', // Clear this field (should be removed, not sent as "") + maxContextTokens: '', // Clear this field (should be removed, not sent as "") + }, + }; + + await updateAgentHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + + const updatedAgent = mockRes.json.mock.calls[0][0]; + expect(updatedAgent.model_parameters).toBeDefined(); + // Valid number should be updated + expect(updatedAgent.model_parameters.temperature).toBe(0.7); + // Empty strings should be removed, not sent as "" + expect(updatedAgent.model_parameters.max_tokens).toBeUndefined(); + expect(updatedAgent.model_parameters.maxContextTokens).toBeUndefined(); + + // Verify in database + const agentInDb = await Agent.findOne({ id: existingAgentId }); + expect(agentInDb.model_parameters.temperature).toBe(0.7); + expect(agentInDb.model_parameters.max_tokens).toBeUndefined(); + expect(agentInDb.model_parameters.maxContextTokens).toBeUndefined(); + }); + test('should return 404 for non-existent agent', async () => { mockReq.user.id = existingAgentAuthorId.toString(); mockReq.params.id = `agent_${uuidv4()}`; // Non-existent ID diff --git a/packages/api/src/endpoints/openai/llm.spec.ts b/packages/api/src/endpoints/openai/llm.spec.ts index a4eb7c78e3..8e92332e24 100644 --- a/packages/api/src/endpoints/openai/llm.spec.ts +++ b/packages/api/src/endpoints/openai/llm.spec.ts @@ -56,6 +56,77 @@ describe('getOpenAILLMConfig', () => { }); }); + describe('Empty String Handling (Issue Fix)', () => { + it('should remove empty string values for numeric parameters', () => { + const result = getOpenAILLMConfig({ + apiKey: 'test-api-key', + streaming: true, + modelOptions: { + model: 'gpt-4', + temperature: '' as unknown as number, + topP: '' as unknown as number, + max_tokens: '' as unknown as number, + }, + }); + + expect(result.llmConfig).not.toHaveProperty('temperature'); + expect(result.llmConfig).not.toHaveProperty('topP'); + expect(result.llmConfig).not.toHaveProperty('maxTokens'); + expect(result.llmConfig).not.toHaveProperty('max_tokens'); + }); + + it('should remove empty string values for frequency and presence penalties', () => { + const result = getOpenAILLMConfig({ + apiKey: 'test-api-key', + streaming: true, + modelOptions: { + model: 'gpt-4', + frequency_penalty: '' as unknown as number, + presence_penalty: '' as unknown as number, + }, + }); + + expect(result.llmConfig).not.toHaveProperty('frequencyPenalty'); + expect(result.llmConfig).not.toHaveProperty('presencePenalty'); + expect(result.llmConfig).not.toHaveProperty('frequency_penalty'); + expect(result.llmConfig).not.toHaveProperty('presence_penalty'); + }); + + it('should preserve valid numeric values while removing empty strings', () => { + const result = getOpenAILLMConfig({ + apiKey: 'test-api-key', + streaming: true, + modelOptions: { + model: 'gpt-4', + temperature: 0.7, + topP: '' as unknown as number, + max_tokens: 4096, + }, + }); + + expect(result.llmConfig).toHaveProperty('temperature', 0.7); + expect(result.llmConfig).not.toHaveProperty('topP'); + expect(result.llmConfig).toHaveProperty('maxTokens', 4096); + }); + + it('should preserve zero values (not treat them as empty)', () => { + const result = getOpenAILLMConfig({ + apiKey: 'test-api-key', + streaming: true, + modelOptions: { + model: 'gpt-4', + temperature: 0, + frequency_penalty: 0, + presence_penalty: 0, + }, + }); + + expect(result.llmConfig).toHaveProperty('temperature', 0); + expect(result.llmConfig).toHaveProperty('frequencyPenalty', 0); + expect(result.llmConfig).toHaveProperty('presencePenalty', 0); + }); + }); + describe('OpenAI Reasoning Models (o1/o3/gpt-5)', () => { const reasoningModels = [ 'o1', diff --git a/packages/api/src/endpoints/openai/llm.ts b/packages/api/src/endpoints/openai/llm.ts index c88122310a..f25971735c 100644 --- a/packages/api/src/endpoints/openai/llm.ts +++ b/packages/api/src/endpoints/openai/llm.ts @@ -139,6 +139,12 @@ export function getOpenAILLMConfig({ }): Pick & { azure?: t.AzureOptions; } { + /** Clean empty strings from model options (e.g., temperature: "" should be removed) */ + const cleanedModelOptions = removeNullishValues( + _modelOptions, + true, + ) as Partial; + const { reasoning_effort, reasoning_summary, @@ -147,7 +153,7 @@ export function getOpenAILLMConfig({ frequency_penalty, presence_penalty, ...modelOptions - } = _modelOptions; + } = cleanedModelOptions; const llmConfig = Object.assign( {