New "Add Prefix" Guardrail #1258
Conversation
|
DescriptionSummary By MatterAI
🔄 What ChangedThe 🔍 Impact of the ChangeThis change enhances the clarity and debuggability of errors originating from the 📁 Total Files Changed
🧪 Test AddedNo new tests were added in this Pull Request. The existing test file 🔒Security VulnerabilitiesNo new security vulnerabilities were introduced. The change improves error reporting, which can indirectly aid in debugging and identifying potential issues more efficiently. MotivationTo provide more informative and structured error messages from the 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 PH as PluginHandler
participant IPL as InternalPluginLogic
PH->>IPL: executePluginLogic(data)
alt Error Occurs
IPL--xPH: throws Error (e)
PH->>PH: catch(e)
Note over PH: delete e.stack
PH->>PH: formatError(e.message, e)
PH-->>PH: error = { message, originalError }
else Success
IPL-->>PH: result
end
PH-->>Caller: { error, verdict, data, transformedData, transformed }
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR implements a well-structured and thoroughly tested "Add Prefix" guardrail plugin. The code is clean, well-organized, and includes comprehensive test coverage. I've identified a few minor improvements that could enhance error handling and code clarity.
Skipped files
plugins/default/manifest.json: Skipped file pattern
plugins/default/addPrefix.ts
Outdated
| newTransformedData = addPrefixToChatCompletion( | ||
| context, | ||
| prefix, | ||
| parameters.applyToRole || 'user', | ||
| parameters.addToExisting !== false, // default to true | ||
| parameters.onlyIfEmpty === true // default to false | ||
| ); |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: The default value for addToExisting is set in two different places (function parameter and when calling the function), which could lead to confusion.
Fix: Standardize how defaults are handled by setting them in one place.
Impact: Improves code maintainability and reduces the risk of inconsistent behavior if one default is changed but not the other.
| newTransformedData = addPrefixToChatCompletion( | |
| context, | |
| prefix, | |
| parameters.applyToRole || 'user', | |
| parameters.addToExisting !== false, // default to true | |
| parameters.onlyIfEmpty === true // default to false | |
| ); | |
| newTransformedData = addPrefixToChatCompletion( | |
| context, | |
| prefix, | |
| parameters.applyToRole || 'user', | |
| parameters.addToExisting === undefined ? true : parameters.addToExisting, | |
| parameters.onlyIfEmpty === true // default to false | |
| ); |
plugins/default/addPrefix.ts
Outdated
| const json = context.request.json; | ||
| const updatedJson = { ...json }; | ||
| const messages = [...json.messages]; | ||
|
|
||
| // Find the target role message | ||
| const targetIndex = messages.findIndex((msg) => msg.role === applyToRole); |
There was a problem hiding this comment.
⚡️ Performance Improvement
Issue: The code creates a new array with [...json.messages] even when it might not be necessary (if the request is rejected early).
Fix: Move the array creation after the validation checks to avoid unnecessary object creation.
Impact: Slightly improves performance by avoiding unnecessary array copying when the operation won't proceed.
| const json = context.request.json; | |
| const updatedJson = { ...json }; | |
| const messages = [...json.messages]; | |
| // Find the target role message | |
| const targetIndex = messages.findIndex((msg) => msg.role === applyToRole); | |
| const json = context.request.json; | |
| const updatedJson = { ...json }; | |
| // Find the target role message | |
| const targetIndex = json.messages.findIndex((msg) => msg.role === applyToRole); | |
| // Create a copy of the messages array only if we're going to modify it | |
| const messages = [...json.messages]; |
There was a problem hiding this comment.
looks good to me, please add testing done and some sample curls @vrushankportkey
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Co-authored-by: matter-code-review[bot] <150888575+matter-code-review[bot]@users.noreply.github.com>
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
✅ Testing Done
Example Requests
Response:
📋 Configuration Options
prefixapplyToRole"user""user","system","assistant"addToExistingtrueonlyIfEmptyfalse