ralph: #25 — Design Generated Case Templates — define 5 LLM generation template types#54
ralph: #25 — Design Generated Case Templates — define 5 LLM generation template types#54jharris1679 wants to merge 1 commit intomainfrom
Conversation
WalkthroughAdded a new templates module to the cases package, defining five template types (style-conformance, missing-coverage, duplication, type-safety, dependency-updates) with associated metadata, configurations, validation rules, and utility functions for template management and case validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/cases/templates.ts`:
- Around line 221-276: Extract the duplicated required_field rules into a single
shared constant (e.g., baseValidationRules) containing the eight common rule
objects for id, title, prompt, files, source, language, difficulty, and
category; then in each template's validationRules replace the duplicated array
with a spread composition like [...baseValidationRules,
...templateSpecificRules] where templateSpecificRules contains only the
differing final pattern rule (the prompt pattern). Update all occurrences that
define validationRules (the arrays shown in the validationRules blocks) to use
the shared base constant and remove the duplicated objects from those
template-specific arrays.
- Around line 876-883: The code constructs a RegExp from untrusted input
(rule.criteria.pattern) which can cause ReDoS; modify the pattern-check block
where RegExp is created (the "Check pattern" handling referencing
rule.criteria.pattern) to validate and safely compile the regex: wrap new
RegExp(...) in a try/catch to handle invalid patterns, enforce limits (e.g. max
pattern length and disallow high-risk constructs) or check against a static
allowlist or a safe-regex utility before compiling, and on failure return a
clear error (instead of throwing) so malformed or malicious patterns cannot
cause catastrophic backtracking or runtime crashes.
- Around line 884-888: The enum validation uses
"rule.criteria.enum.includes(value as string)" unsafely; update the check in the
enum branch (referencing rule.criteria.enum) to first verify the value is a
string (e.g., typeof value === "string") and if not return a clear type error
(e.g., `${rule.field} must be one of: ...` or a message indicating it must be a
string), otherwise perform the includes check on the string; ensure you do not
use unchecked type assertions like "as string".
- Around line 866-870: The current required check only tests `value ===
undefined`, letting `null` and empty strings pass; update the validation in the
block that checks `rule.criteria.required` (where `value` and `rule.field` are
used) to treat null and empty-string as missing too — e.g., consider `value ==
null || value === ''` (or equivalent) as missing and return the same `{ error:
`${rule.field} is required` }` when that condition is met so `null` and `""`
from LLM-parsed `caseData` are rejected.
🧹 Nitpick comments (3)
src/cases/templates.ts (3)
15-20: Trailing semicolon in union type member.Line 20 has a stray
;after the comment, which is harmless but inconsistent with the other members that use no trailing punctuation.- | 'dependency-updates' // Find deprecated or outdated API usage; + | 'dependency-updates'; // Find deprecated or outdated API usage
914-916:getTemplateandgetTemplateConfigcan never returnundefinedgiven aTemplateTypekey.Since the parameter is typed as
TemplateTypeand the records areRecord<TemplateType, ...>, a lookup will always succeed. The| undefinedreturn type is misleading to callers who then have to do unnecessary null checks. If you want to keepundefinedfor future extensibility (e.g., dynamic template registration), acceptstringinstead and useisTemplateAvailableas a type guard first.Also applies to: 928-930
104-106:customvalidator function won't survive serialization.
ValidationCriteria.customholds a function reference, which means templates with custom validators can't be serialized to JSON/YAML and rehydrated—something to be aware of if templates are ever stored or transmitted as data. Consider documenting this limitation or using a named-validator registry pattern instead.
| validationRules: [ | ||
| { | ||
| type: 'required_field', | ||
| field: 'id', | ||
| criteria: { required: true, pattern: '^[a-z0-9-]+$' }, | ||
| errorMessage: 'Case ID must contain only lowercase letters, numbers, and hyphens', | ||
| }, | ||
| { | ||
| type: 'required_field', | ||
| field: 'title', | ||
| criteria: { required: true }, | ||
| errorMessage: 'Case title is required', | ||
| }, | ||
| { | ||
| type: 'required_field', | ||
| field: 'prompt', | ||
| criteria: { required: true }, | ||
| errorMessage: 'Case prompt is required', | ||
| }, | ||
| { | ||
| type: 'required_field', | ||
| field: 'files', | ||
| criteria: { required: true }, | ||
| errorMessage: 'Case files are required', | ||
| }, | ||
| { | ||
| type: 'required_field', | ||
| field: 'source', | ||
| criteria: { required: true, enum: ['generated'] }, | ||
| errorMessage: 'Case source must be "generated"', | ||
| }, | ||
| { | ||
| type: 'required_field', | ||
| field: 'language', | ||
| criteria: { required: true }, | ||
| errorMessage: 'Case language is required', | ||
| }, | ||
| { | ||
| type: 'required_field', | ||
| field: 'difficulty', | ||
| criteria: { required: true, enum: ['easy', 'medium', 'hard'] }, | ||
| errorMessage: 'Case difficulty must be easy, medium, or hard', | ||
| }, | ||
| { | ||
| type: 'required_field', | ||
| field: 'category', | ||
| criteria: { required: true }, | ||
| errorMessage: 'Case category is required', | ||
| }, | ||
| { | ||
| type: 'pattern', | ||
| field: 'prompt', | ||
| criteria: { pattern: 'style|convention|naming|formatting' }, | ||
| errorMessage: 'Prompt should mention style or conventions', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Validation rules are copy-pasted across all five template configs.
The eight required_field rules (id, title, prompt, files, source, language, difficulty, category) are identical in every config. Only the final pattern rule differs. Extract a shared base array and spread template-specific rules on top.
♻️ Sketch
+const BASE_VALIDATION_RULES: TemplateValidationRule[] = [
+ {
+ type: 'required_field',
+ field: 'id',
+ criteria: { required: true, pattern: '^[a-z0-9-]+$' },
+ errorMessage: 'Case ID must contain only lowercase letters, numbers, and hyphens',
+ },
+ // ... remaining 7 shared rules ...
+];
+
// Then in each config:
- validationRules: [ /* 9 identical + 1 unique */ ],
+ validationRules: [
+ ...BASE_VALIDATION_RULES,
+ {
+ type: 'pattern',
+ field: 'prompt',
+ criteria: { pattern: 'style|convention|naming|formatting' },
+ errorMessage: 'Prompt should mention style or conventions',
+ },
+ ],Also applies to: 358-413, 509-564, 631-686, 750-805
🤖 Prompt for AI Agents
In `@src/cases/templates.ts` around lines 221 - 276, Extract the duplicated
required_field rules into a single shared constant (e.g., baseValidationRules)
containing the eight common rule objects for id, title, prompt, files, source,
language, difficulty, and category; then in each template's validationRules
replace the duplicated array with a spread composition like
[...baseValidationRules, ...templateSpecificRules] where templateSpecificRules
contains only the differing final pattern rule (the prompt pattern). Update all
occurrences that define validationRules (the arrays shown in the validationRules
blocks) to use the shared base constant and remove the duplicated objects from
those template-specific arrays.
|
|
||
| // Check required | ||
| if (rule.criteria.required && value === undefined) { | ||
| return { error: `${rule.field} is required` }; | ||
| } |
There was a problem hiding this comment.
required check only catches undefined, not null or empty string.
value === undefined lets null and "" slip through as valid for required fields. Since caseData comes from LLM output (parsed JSON/YAML), null values are common.
🐛 Proposed fix
// Check required
- if (rule.criteria.required && value === undefined) {
+ if (rule.criteria.required && (value === undefined || value === null)) {
return { error: `${rule.field} is required` };
}
// Skip validation if value is not present and not required
- if (value === undefined) {
+ if (value === undefined || value === null) {
return {};
}🤖 Prompt for AI Agents
In `@src/cases/templates.ts` around lines 866 - 870, The current required check
only tests `value === undefined`, letting `null` and empty strings pass; update
the validation in the block that checks `rule.criteria.required` (where `value`
and `rule.field` are used) to treat null and empty-string as missing too — e.g.,
consider `value == null || value === ''` (or equivalent) as missing and return
the same `{ error: `${rule.field} is required` }` when that condition is met so
`null` and `""` from LLM-parsed `caseData` are rejected.
|
|
||
| // Check pattern | ||
| if (rule.criteria.pattern && typeof value === 'string') { | ||
| const regex = new RegExp(rule.criteria.pattern); | ||
| if (!regex.test(value)) { | ||
| return { error: `${rule.field} does not match required pattern` }; | ||
| } | ||
| } |
There was a problem hiding this comment.
ReDoS risk: regex constructed from variable input.
rule.criteria.pattern is used directly in new RegExp(...). While current patterns are hardcoded, the TemplateConfig interface is public and extensible—custom templates could introduce pathological patterns. Wrap the construction in a try/catch at minimum, and consider a static allowlist or a safe-regex check.
🛡️ Proposed minimal hardening
// Check pattern
if (rule.criteria.pattern && typeof value === 'string') {
- const regex = new RegExp(rule.criteria.pattern);
- if (!regex.test(value)) {
- return { error: `${rule.field} does not match required pattern` };
+ try {
+ const regex = new RegExp(rule.criteria.pattern);
+ if (!regex.test(value)) {
+ return { error: `${rule.field} does not match required pattern` };
+ }
+ } catch {
+ return { error: `${rule.field} has an invalid validation pattern` };
}
}🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 878-878: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(rule.criteria.pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@src/cases/templates.ts` around lines 876 - 883, The code constructs a RegExp
from untrusted input (rule.criteria.pattern) which can cause ReDoS; modify the
pattern-check block where RegExp is created (the "Check pattern" handling
referencing rule.criteria.pattern) to validate and safely compile the regex:
wrap new RegExp(...) in a try/catch to handle invalid patterns, enforce limits
(e.g. max pattern length and disallow high-risk constructs) or check against a
static allowlist or a safe-regex utility before compiling, and on failure return
a clear error (instead of throwing) so malformed or malicious patterns cannot
cause catastrophic backtracking or runtime crashes.
|
|
||
| // Check enum | ||
| if (rule.criteria.enum && !rule.criteria.enum.includes(value as string)) { | ||
| return { error: `${rule.field} must be one of: ${rule.criteria.enum.join(', ')}` }; | ||
| } |
There was a problem hiding this comment.
Unsafe cast in enum check.
value as string is applied without confirming the value is actually a string. If an LLM returns a number or boolean for a field with an enum constraint, Array.includes will silently pass/fail without a type error, producing a confusing validation message.
Proposed fix
// Check enum
- if (rule.criteria.enum && !rule.criteria.enum.includes(value as string)) {
+ if (rule.criteria.enum && (typeof value !== 'string' || !rule.criteria.enum.includes(value))) {
return { error: `${rule.field} must be one of: ${rule.criteria.enum.join(', ')}` };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check enum | |
| if (rule.criteria.enum && !rule.criteria.enum.includes(value as string)) { | |
| return { error: `${rule.field} must be one of: ${rule.criteria.enum.join(', ')}` }; | |
| } | |
| // Check enum | |
| if (rule.criteria.enum && (typeof value !== 'string' || !rule.criteria.enum.includes(value))) { | |
| return { error: `${rule.field} must be one of: ${rule.criteria.enum.join(', ')}` }; | |
| } |
🤖 Prompt for AI Agents
In `@src/cases/templates.ts` around lines 884 - 888, The enum validation uses
"rule.criteria.enum.includes(value as string)" unsafely; update the check in the
enum branch (referencing rule.criteria.enum) to first verify the value is a
string (e.g., typeof value === "string") and if not return a clear type error
(e.g., `${rule.field} must be one of: ...` or a message indicating it must be a
string), otherwise perform the includes check on the string; ensure you do not
use unchecked type assertions like "as string".
1 participant
Issue
Closes #25
Status: ✓ verified
Build, tests, and lint all pass locally.
Summary
Automated implementation by Ralph (rlmkit + MiniMax M2.5).
Review the changes carefully — this was generated by a local model.
Summary by CodeRabbit
Release Notes