-
Notifications
You must be signed in to change notification settings - Fork 749
feat/regex-replace-guardrail #1312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat/regex-replace-guardrail #1312
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New regex replace plugin implementation looks solid with proper error handling and transformation logic. Minor improvements needed for type safety and performance.
| ) => { | ||
| let error = null; | ||
| let verdict = true; | ||
| let data: any = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: Using any type weakens type safety and can lead to runtime errors
Fix: Use proper typing for the data object
Impact: Prevents potential runtime type errors and improves code maintainability
| let data: any = null; | |
| let data: { regexPattern?: string; verdict?: boolean; explanation?: string } | null = null; |
| if (!regexPattern) { | ||
| throw new Error('Missing regex pattern'); | ||
| } | ||
| if (!content) { | ||
| throw new Error('Missing text to match'); | ||
| } | ||
|
|
||
| const regex = new RegExp(regexPattern, 'g'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚡️ Performance Improvement
Issue: Creating new RegExp instance inside the handler for every execution is inefficient
Fix: Move regex creation after validation but before the loop to avoid recreation
Impact: Reduces object allocation overhead and improves performance for multiple text processing
| if (!regexPattern) { | |
| throw new Error('Missing regex pattern'); | |
| } | |
| if (!content) { | |
| throw new Error('Missing text to match'); | |
| } | |
| const regex = new RegExp(regexPattern, 'g'); | |
| if (!regexPattern) { | |
| throw new Error('Missing regex pattern'); | |
| } | |
| if (!content) { | |
| throw new Error('Missing text to match'); | |
| } | |
| const regex = new RegExp(regexPattern, 'g'); |
| const matches = text.match(regex); | ||
| if (matches && matches.length > 0) { | ||
| hasMatches = true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐛 Bug Fix
Issue: The match() method already returns null when no matches are found, making the length check redundant
Fix: Simplify the condition to just check for matches existence
Impact: Cleaner code and slightly better performance by avoiding unnecessary array length check
| const matches = text.match(regex); | |
| if (matches && matches.length > 0) { | |
| hasMatches = true; | |
| } | |
| const matches = text.match(regex); | |
| if (matches) { | |
| hasMatches = true; | |
| } |
| explanation: transformed | ||
| ? `Pattern '${regexPattern}' matched and was replaced with '${redactText}'` | ||
| : `The regex pattern '${regexPattern}' did not match any text.`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Code Refactor
Issue: The explanation message construction could be more readable and maintainable
Fix: Use template literals consistently and improve readability
Impact: Better code maintainability and consistent string formatting
| explanation: transformed | |
| ? `Pattern '${regexPattern}' matched and was replaced with '${redactText}'` | |
| : `The regex pattern '${regexPattern}' did not match any text.`, | |
| explanation: transformed | |
| ? `Pattern '${regexPattern}' matched and was replaced with '${redactText}'` | |
| : `The regex pattern '${regexPattern}' did not match any text.`, |
|
Code adds modalities support and inline data handling for Google Vertex AI. Implementation looks solid with proper type definitions and consistent patterns across files. |
Description
This PR adds a new guardrail regex replace to portkey's default gurardrail
Motivation
the ability to replace text while running a regex guardrail on both input and output
Type of Change
How Has This Been Tested?
Checklist
Related Issues
Fixes and Cloaes #1291