Tool injector guardrail #1297
Tool injector guardrail #1297
Conversation
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
|
DescriptionSummary By MatterAI
🔄 What ChangedThis pull request introduces two new default plugins: 🔍 Impact of the ChangeThe 📁 Total Files Changed
🧪 Test AddedN/A 🔒Security VulnerabilitiesThe new plugins include basic input validation for their respective parameters ( MotivationThe motivation is to provide more robust and flexible mechanisms for interacting with AI models, specifically by allowing programmatic prefixing of prompts for guardrails or context, and by enabling dynamic injection and management of tools for enhanced AI capabilities. Additionally, the pre-commit hook aims to improve code quality and consistency. Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Related IssuesN/A Tip Quality Recommendations
Tanka Poem ♫
Sequence DiagramsequenceDiagram
participant Client as Client Application
participant Gateway as API Gateway
participant PluginHandler as Plugin Handler (addPrefix/injectTools)
participant AIModel as AI Model
Client->>Gateway: AI Request (e.g., POST /chat/completions)
Note over Gateway: Intercepts request via 'beforeRequestHook'
alt Request Type is 'chatComplete' or 'messages'
Gateway->>+PluginHandler: handler(context, parameters, 'beforeRequestHook')
Note over PluginHandler: Determines if addPrefix or injectTools is active
alt addPrefix Plugin Active
PluginHandler->>PluginHandler: getCurrentContentPart(context, eventType)
PluginHandler->>PluginHandler: addPrefixToChatCompletion(context, prefix, applyToRole, addToExisting, onlyIfEmpty, eventType)
Note over PluginHandler: Modifies 'messages' content with prefix
PluginHandler->>PluginHandler: setCurrentContentPart(context, eventType, transformedData, updatedTexts)
else injectTools Plugin Active
PluginHandler->>PluginHandler: normalizeFunctionTool(tool: AnyTool)
PluginHandler->>PluginHandler: dedupeTools(existing: AnyTool[], incoming: AnyTool[])
Note over PluginHandler: Modifies 'tools' array and 'tool_choice'
end
PluginHandler-->>-Gateway: Transformed Request JSON
Gateway->>AIModel: Forward Modified Request (with prefix/tools)
AIModel-->>Gateway: AI Response
Gateway-->>Client: Final Response
else Other Request Type or Event Type
Gateway-->>AIModel: Forward Original Request
AIModel-->>Gateway: AI Response
Gateway-->>Client: Final Response
end
alt Error in Plugin
PluginHandler--xGateway: Error Object
Gateway--xClient: Error Response
end
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| ): Record<string, any> => { | ||
| const json = context.request.json; | ||
| const updatedJson = { ...json }; | ||
| const messages = Array.isArray(json.messages) ? [...json.messages] : []; |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Potential null pointer exception when json.messages is null/undefined
Fix: Add null check before array operations
Impact: Prevents runtime crashes when messages property is missing
| const messages = Array.isArray(json.messages) ? [...json.messages] : []; | |
| const messages = Array.isArray(json?.messages) ? [...json.messages] : []; |
| const isEmptyContent = | ||
| (typeof content === 'string' && content.trim().length === 0) || | ||
| (Array.isArray(content) && content.length === 0); |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Logical OR operator creates incorrect boolean condition - will always be true
Fix: Use logical AND operator for proper empty content detection
Impact: Fixes empty content detection logic to work correctly
| const isEmptyContent = | |
| (typeof content === 'string' && content.trim().length === 0) || | |
| (Array.isArray(content) && content.length === 0); | |
| const isEmptyContent = | |
| (typeof content === 'string' && content.trim().length === 0) || | |
| (Array.isArray(content) && content.length === 0) || | |
| content == null; |
| const existingTools: AnyTool[] = Array.isArray(originalJson.tools) | ||
| ? [...originalJson.tools] | ||
| : []; |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Potential null pointer exception when accessing originalJson.tools
Fix: Add null check before array operations
Impact: Prevents runtime crashes when tools property is missing
| const existingTools: AnyTool[] = Array.isArray(originalJson.tools) | |
| ? [...originalJson.tools] | |
| : []; | |
| const existingTools: AnyTool[] = Array.isArray(originalJson?.tools) | |
| ? [...originalJson.tools] | |
| : []; |
| const hasExistingToolChoice = | ||
| (originalJson as Record<string, unknown>)?.tool_choice !== undefined && | ||
| (originalJson as Record<string, unknown>)?.tool_choice !== null; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Complex type assertions make code hard to read and maintain
Fix: Extract type definitions for better readability
Impact: Improves code maintainability and type safety
| const hasExistingToolChoice = | |
| (originalJson as Record<string, unknown>)?.tool_choice !== undefined && | |
| (originalJson as Record<string, unknown>)?.tool_choice !== null; | |
| type JsonWithToolChoice = Record<string, unknown> & { | |
| tool_choice?: unknown; | |
| }; | |
| const hasExistingToolChoice = | |
| (originalJson as JsonWithToolChoice)?.tool_choice !== undefined && | |
| (originalJson as JsonWithToolChoice)?.tool_choice !== null; |
| const exists = nextTools.some( | ||
| (t) => | ||
| t && | ||
| (t as { type?: unknown; function?: { name?: unknown } }).type === | ||
| 'function' && | ||
| (t as { function?: { name?: unknown } }).function?.name === | ||
| requiredFunctionName | ||
| ); |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Deeply nested type assertions are hard to read and error-prone
Fix: Extract type guard function for tool validation
Impact: Improves code readability and reduces type assertion complexity
| const exists = nextTools.some( | |
| (t) => | |
| t && | |
| (t as { type?: unknown; function?: { name?: unknown } }).type === | |
| 'function' && | |
| (t as { function?: { name?: unknown } }).function?.name === | |
| requiredFunctionName | |
| ); | |
| const isValidFunctionTool = (t: AnyTool): boolean => { | |
| return t?.type === 'function' && | |
| typeof t.function === 'object' && | |
| t.function?.name === requiredFunctionName; | |
| }; | |
| const exists = nextTools.some(isValidFunctionTool); |
1 participant
Description
Motivation
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Related Issues