Fix/tools null properties#1
Conversation
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
|
PR review started! Estimated time: 5-10 minutes. Learn MoreAsk Codity questions: Mention Trigger a manual review: Comment Generate unit tests: Comment Run security scan again: Comment |
Greptile SummaryThis PR introduces a
Confidence Score: 3/5Merging as-is will silently alter the wire format for tools that have no parameters and will cause the nil-parameters test to fail. The sanitization logic applies to the entire function object rather than only the null-containing properties, so Functions with no parameters have their Parameters field changed from nil to an empty map, mutating the serialized JSON from parameters:null to parameters:{}, which could affect downstream template rendering. pkg/functions/functions.go and pkg/functions/functions_test.go need the most attention — the former has the logic gap and the latter has a test assertion that contradicts the implementation's actual behavior. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming OpenAI Request with Tools] --> B{len tools > 0?}
B -- No --> Z[toolsJSON = empty string]
B -- Yes --> C[functions.SanitizeTools called]
subgraph SanitizeTools[SanitizeTools - 5 JSON passes]
C1[Pass 1 - Marshal Tools to JSON] --> C2[Pass 2 - Unmarshal to generic map]
C2 --> C3[Iterate each tool]
C3 --> C4[sanitizeValue on ENTIRE function object]
C4 --> C5{value nil?}
C5 -- Yes --> C6[Convert nil to empty map]
C5 -- No map --> C7[Recurse into map keys]
C5 -- No slice --> C8[Recurse into slice elements]
C5 -- No primitive --> C9[Return as-is]
C6 --> C10[Pass 3 - Marshal sanitized map to JSON]
C7 --> C10
C8 --> C10
C9 --> C10
C10 --> C11[Pass 4 and 5 - Unmarshal JSON back to Tools struct]
end
C11 --> D[Pass 6 - json.Marshal sanitizedTools in caller]
D --> E[toolsJSON string sent to backend inference]
Reviews (1): Last reviewed commit: "recursive" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func sanitizeValue(value interface{}, path string) interface{} { | ||
| if value == nil { | ||
| // Convert null to empty object | ||
| xlog.Debug("SanitizeTools: found null value, converting to empty object", "path", path) | ||
| return map[string]interface{}{} | ||
| } | ||
|
|
||
| switch v := value.(type) { | ||
| case map[string]interface{}: | ||
| // Recursively sanitize map values | ||
| sanitized := make(map[string]interface{}) | ||
| for key, val := range v { | ||
| newPath := path | ||
| if newPath != "" { | ||
| newPath += "." | ||
| } | ||
| newPath += key | ||
| sanitized[key] = sanitizeValue(val, newPath) | ||
| } | ||
| return sanitized | ||
|
|
||
| case []interface{}: | ||
| // Recursively sanitize slice elements | ||
| sanitized := make([]interface{}, len(v)) | ||
| for i, val := range v { | ||
| newPath := fmt.Sprintf("%s[%d]", path, i) | ||
| sanitized[i] = sanitizeValue(val, newPath) | ||
| } | ||
| return sanitized | ||
|
|
||
| default: | ||
| // For primitive types (string, number, bool), return as-is | ||
| return value | ||
| } | ||
| } | ||
|
|
||
| // SanitizeTools removes null values from tool.parameters.properties and converts them to empty objects. | ||
| // This prevents Jinja template errors when processing tools with malformed parameter schemas. | ||
| // It works by marshaling to JSON, recursively sanitizing the JSON structure, and unmarshaling back. | ||
| func SanitizeTools(tools Tools) Tools { | ||
| if len(tools) == 0 { | ||
| return tools | ||
| } | ||
|
|
||
| xlog.Debug("SanitizeTools: processing tools", "count", len(tools)) | ||
|
|
||
| // Marshal to JSON to work with the actual JSON representation | ||
| toolsJSON, err := json.Marshal(tools) | ||
| if err != nil { | ||
| xlog.Warn("SanitizeTools: failed to marshal tools to JSON", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| // Parse JSON into a generic structure | ||
| var toolsData []map[string]interface{} | ||
| if err := json.Unmarshal(toolsJSON, &toolsData); err != nil { | ||
| xlog.Warn("SanitizeTools: failed to unmarshal tools JSON", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| // Recursively sanitize the JSON structure | ||
| for i, tool := range toolsData { | ||
| if function, ok := tool["function"].(map[string]interface{}); ok { | ||
| // Recursively sanitize the entire tool structure | ||
| tool["function"] = sanitizeValue(function, fmt.Sprintf("tools[%d].function", i)) | ||
| } | ||
| toolsData[i] = tool | ||
| } | ||
|
|
||
| // Marshal back to JSON | ||
| sanitizedJSON, err := json.Marshal(toolsData) | ||
| if err != nil { | ||
| xlog.Warn("SanitizeTools: failed to marshal sanitized tools", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| // Unmarshal back into Tools structure | ||
| var sanitized Tools | ||
| if err := json.Unmarshal(sanitizedJSON, &sanitized); err != nil { | ||
| xlog.Warn("SanitizeTools: failed to unmarshal sanitized tools", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| return sanitized | ||
| } |
There was a problem hiding this comment.
nil Parameters silently mutated to empty map
sanitizeValue is called on the entire function map, so when Function.Parameters is nil it is serialized to JSON null, then converted to map[string]interface{}{} by the nil-branch, and finally unmarshaled back as a non-nil empty map. The result is "parameters": {} instead of "parameters": null, which changes the wire format for every tool that has no parameters. The test "handles tools without parameters field" asserts Parameters.BeNil() but that assertion will fail under the current implementation — the value returned is an empty (non-nil) map.
| // Sanitize tools JSON to remove null values from tool.parameters.properties | ||
| // This prevents Jinja template errors when processing tools with malformed parameter schemas |
There was a problem hiding this comment.
These two comment lines describe sanitization logic that was never implemented in this file. They sit between the closing brace of parse_options and the kv_cache_types vector definition, unattached to any function. A future reader will look for the C++ implementation referenced by the comment and not find it, which could cause confusion about where null-sanitization actually occurs.
| // SanitizeTools removes null values from tool.parameters.properties and converts them to empty objects. | ||
| // This prevents Jinja template errors when processing tools with malformed parameter schemas. | ||
| // It works by marshaling to JSON, recursively sanitizing the JSON structure, and unmarshaling back. | ||
| func SanitizeTools(tools Tools) Tools { | ||
| if len(tools) == 0 { | ||
| return tools | ||
| } | ||
|
|
||
| xlog.Debug("SanitizeTools: processing tools", "count", len(tools)) | ||
|
|
||
| // Marshal to JSON to work with the actual JSON representation | ||
| toolsJSON, err := json.Marshal(tools) | ||
| if err != nil { | ||
| xlog.Warn("SanitizeTools: failed to marshal tools to JSON", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| // Parse JSON into a generic structure | ||
| var toolsData []map[string]interface{} | ||
| if err := json.Unmarshal(toolsJSON, &toolsData); err != nil { | ||
| xlog.Warn("SanitizeTools: failed to unmarshal tools JSON", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| // Recursively sanitize the JSON structure | ||
| for i, tool := range toolsData { | ||
| if function, ok := tool["function"].(map[string]interface{}); ok { | ||
| // Recursively sanitize the entire tool structure | ||
| tool["function"] = sanitizeValue(function, fmt.Sprintf("tools[%d].function", i)) | ||
| } | ||
| toolsData[i] = tool | ||
| } | ||
|
|
||
| // Marshal back to JSON | ||
| sanitizedJSON, err := json.Marshal(toolsData) | ||
| if err != nil { | ||
| xlog.Warn("SanitizeTools: failed to marshal sanitized tools", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| // Unmarshal back into Tools structure | ||
| var sanitized Tools | ||
| if err := json.Unmarshal(sanitizedJSON, &sanitized); err != nil { | ||
| xlog.Warn("SanitizeTools: failed to unmarshal sanitized tools", "error", err) | ||
| return tools | ||
| } | ||
|
|
||
| return sanitized | ||
| } |
There was a problem hiding this comment.
SanitizeTools internally performs marshal → unmarshal → sanitize → marshal → unmarshal (5 JSON passes), and both callers (chat.go and inference.go) immediately call json.Marshal on the returned value, adding a 6th pass on every inference request that carries tools. The same effect could be achieved with a single pass by walking the Tools struct directly and deep-copying only the affected fields, avoiding unnecessary allocations on the hot path.
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaTool Schema Handling: Added Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The function uses in-place mutation for simplicity and performance, accepting that the input map may be modified. This avoids copying large schemas unnecessarily. The choice to convert Merge StatusMERGEABLE — PR Score 73/100, above threshold (50). All gates passed. |
Nitpicks (Low Priority)Found 1 low-priority suggestions for code improvement Click to expand nitpicks
|
Security Scan Summary
No critical security issues detected Scan completed in 25.2sSecurity scan powered by Codity.ai |
Dependency vulnerability scanning
View vulnerability details (4 items)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
Powered by Codity.ai · Docs |
License Compliance Scan
Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required Medium Risk Licenses - 4 packagesMPL-2.0 (4 packages):
Unknown Licenses - 45 packages
...and 25 more Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/LocalAI · PR #1Scanned: 2026-05-16 17:08 UTC | Score: 55/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-001]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
pkg/functions/functions.go |
0 | 2 | 2 | 1 | 5 |
pkg/functions/functions_test.go |
0 | 0 | 1 | 0 | 1 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaTool Schema Handling: Added Files Changed
Review Focus Areas
ArchitectureDesign Decisions: The function uses in-place mutation on a copy of the input to avoid modifying original schemas, balancing safety and performance. Converting |
Security Scan Summary
No critical security issues detected Scan completed in 24.2sSecurity scan powered by Codity.ai |
Dependency vulnerability scanning
View vulnerability details (4 items)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
Powered by Codity.ai · Docs |
|
@codity review |
Policy Check Failed✗ 3/3 policy checks failed: • Need 2 more approval(s) (0/2) — comment LGTM or approve via review To merge this PR:
|
PR SummaryWhat Changed
Key Changes by AreaTool Schema Handling: Added sanitization logic to convert Files Changed
Review Focus Areas
ArchitectureDesign Decisions: Sanitization runs before JSON marshaling to ensure downstream Jinja templates receive valid schemas. Using JSON round-trip for deep copying is intentional but introduces a dependency on struct tags. Merge StatusMERGEABLE — PR Score 68/100, above threshold (50). All gates passed. |
![codity-dm[bot]](https://avatars.githubusercontent.com/u/234452856?s=60&v=4){kind=small}
| toolsBytes, err := json.Marshal(input.Tools) | ||
| // Sanitize tools to remove null values from parameters.properties | ||
| sanitizedTools := functions.SanitizeTools(input.Tools) | ||
| toolsBytes, err := json.Marshal(sanitizedTools) |
There was a problem hiding this comment.
The error from json.Marshal(sanitizedTools) is silently ignored when err != nil, resulting in toolsJSON remaining an empty string and tools being silently dropped; the error should be logged.
Suggested fix
toolsBytes, err := json.Marshal(sanitizedTools)
if err != nil {
log.Error("Failed to marshal sanitized tools", "error", err)
} else {
toolsJSON = string(toolsBytes)
}Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: core/http/endpoints/openai/chat.go
Lines: 627-627
Issue Type: robustness-medium
Severity: medium
Issue Description:
The error from `json.Marshal(sanitizedTools)` is silently ignored when `err != nil`, resulting in `toolsJSON` remaining an empty string and tools being silently dropped; the error should be logged.
Current Code:
toolsBytes, err := json.Marshal(sanitizedTools)
if err == nil {
toolsJSON = string(toolsBytes)
}
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
| originalProperties := tools[0].Function.Parameters["properties"].(map[string]interface{}) | ||
| originalNullValue := originalProperties["null_param"] | ||
|
|
||
| sanitized := SanitizeTools(tools) | ||
|
|
||
| // Original should still have nil | ||
| Expect(originalNullValue).To(BeNil()) |
There was a problem hiding this comment.
The immutability test captures originalNullValue before calling SanitizeTools, so it will always be nil regardless of whether SanitizeTools mutates the original; the test does not actually verify that the original map entry was not modified in-place. To properly test immutability, re-read originalProperties["null_param"] after the call.
Suggested fix
originalProperties := tools[0].Function.Parameters["properties"].(map[string]interface{})
sanitized := SanitizeTools(tools)
// Original should still have nil (re-read after the call to detect in-place mutation)
Expect(originalProperties["null_param"]).To(BeNil())Prompt for AI assistance
Copy the prompt below and paste it into ChatGPT, Claude, or any LLM:
You are an expert go developer with deep knowledge of security, performance, and best practices.
### Context
File: pkg/functions/functions_test.go
Lines: 270-276
Issue Type: functional-medium
Severity: medium
Issue Description:
The immutability test captures `originalNullValue` before calling `SanitizeTools`, so it will always be nil regardless of whether `SanitizeTools` mutates the original; the test does not actually verify that the original map entry was not modified in-place. To properly test immutability, re-read `originalProperties["null_param"]` after the call.
Current Code:
originalProperties := tools[0].Function.Parameters["properties"].(map[string]interface{})
originalNullValue := originalProperties["null_param"]
sanitized := SanitizeTools(tools)
// Original should still have nil
Expect(originalNullValue).To(BeNil())
---
### Instructions
1. Fix the issue described above
2. Maintain the exact indentation and code style from the original
3. Follow go best practices and language-specific idioms
4. Ensure the fix addresses the root cause, not just the symptoms
5. Add brief inline comments explaining the fix if needed
### Constraints
- Do not change functionality beyond fixing the identified issue
- Preserve existing variable names and function signatures unless they are part of the problem
- Ensure the fix is production-ready
---
Nitpicks (Low Priority)Found 1 low-priority suggestions for code improvement Click to expand nitpicks
|
Security Scan Summary
No critical security issues detected Scan completed in 19.6sSecurity scan powered by Codity.ai |
Dependency vulnerability scanning
View vulnerability details (4 items)1. pip 24.0 CVE: GHSA-4xh5-x5gv-qwph
2. pip 24.0 CVE: GHSA-6vgw-5pg2-w6jp
3. pip 24.0 CVE: GHSA-58qw-9mgm-455v
4. pip 24.0 CVE: GHSA-jp4c-xjxw-mgf9
Powered by Codity.ai · Docs |
License Compliance Scan
Weak copyleft licenses found - verify compatibility Some packages have unknown licenses - manual review required Medium Risk Licenses - 4 packagesMPL-2.0 (4 packages):
Unknown Licenses - 54 packages
...and 34 more Powered by Codity.ai · Docs |
Code Quality Report — test-org-codity/LocalAI · PR #1Scanned: 2026-05-16 17:41 UTC | Score: 54/100 | Provider: github Executive Summary
Top Findings[CQ-LLM-001]
|
| File | Critical | High | Medium | Low | Total |
|---|---|---|---|---|---|
pkg/functions/functions.go |
0 | 2 | 2 | 2 | 6 |
pkg/functions/functions_test.go |
0 | 0 | 1 | 0 | 1 |
Recommendations
- Resolve High severity issues, especially error handling gaps and performance bottlenecks.
- Run automated tests after applying fixes to verify no regressions.
2 participants
No description provided.