Conversation
Codacy's Analysis Summary0 new issue (≤ 1 medium issue) Review Pull Request in Codacy →
|
There was a problem hiding this comment.
Pull request overview
This pull request adds custom internationalization (i18n) rules for JavaScript/TypeScript codebases and introduces a parameter placeholder replacement mechanism that converts comma-separated allow lists into regex patterns. The changes enable detection of hardcoded strings in alerts, locale-specific formatting, and raw JSX text that should be localized.
Key changes:
- Added
convertListToRegexfunction to convert comma-separated lists (e.g., model allow lists) into regex alternation patterns with negative lookahead support - Introduced four new JS/TS i18n rules to detect hardcoded strings in alerts, hardcoded locale strings in date/time formatting, improper number formatting with
toFixed, and raw text in JSX elements - Updated the AI rule configuration to use the new
MODEL_ALLOW_LISTparameter format instead of hardcoded regex patterns
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tool/configuration.go | Implements convertListToRegex function and updates replaceParameterPlaceholders to handle _ALLOW_LIST suffixed parameters by converting them to regex patterns |
| docs/codacy-rules-i18n.yaml | Adds four new JS/TS i18n rules (no-hardcoded-alert-concat, no-hardcoded-locale-date, no-hardcoded-number-format, no-raw-jsx-text) and updates confidence level for the Java rule |
| docs/codacy-rules-ai.yaml | Changes from hardcoded MODEL_REGEX to MODEL_ALLOW_LIST placeholder and updates confidence level to LOW |
| docs/multiple-tests/i18n/patterns.xml | Adds three new JS i18n pattern configurations for testing |
| docs/multiple-tests/i18n/results.xml | Adds expected test results for the new JS/TS i18n rules showing violations in OrderList.js and Orderlist.jsx |
| docs/multiple-tests/ai/patterns.xml | Updates to use modelAllowList parameter instead of direct regex value |
| docs/multiple-tests/ai/results.xml | Updates expected test result to flag the non-allowed model "deepseek-v3.2" |
| docs/multiple-tests/ai/src/cs/GeminiExample.cs | Adds test case with "deepseek-v3.2" model (not in allow list) to verify the pattern detection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Escape dots for regex | ||
| item = strings.ReplaceAll(item, ".", "\\.") |
There was a problem hiding this comment.
The regex escaping in this function is incomplete and could cause incorrect pattern matching. Currently, only dots are escaped, but regex has many other special characters that need escaping (e.g., +, *, ?, [, ], (, ), {, }, ^, $, |, ). For example, if a model name contains "gpt-4+" or "model[v2]", these would be interpreted as regex operators rather than literal characters. Use a proper regex escaping function like regexp.QuoteMeta() to escape all special characters.
| // Escape dots for regex | |
| item = strings.ReplaceAll(item, ".", "\\.") | |
| // Escape all regex metacharacters | |
| item = regexp.QuoteMeta(item) |
| } | ||
|
|
||
| // convertListToRegex converts a comma-separated list into a regex alternation pattern | ||
| // Example: "gemini-2.5-flash,gpt-3.5-turbo,old-llama-model" -> "^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$" |
There was a problem hiding this comment.
The example in this comment is misleading. It shows the output format for the include=true case (^(...)$), but when this function is actually called on line 184, it's always called with include=false, which produces a different pattern: ^(?!(...)$).* (a negative lookahead). Update the example to show the actual pattern generated for the include=false case, or include both examples to clarify the difference.
| // Example: "gemini-2.5-flash,gpt-3.5-turbo,old-llama-model" -> "^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$" | |
| // Examples: | |
| // convertListToRegex("gemini-2.5-flash,gpt-3.5-turbo,old-llama-model", true) -> "^(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$" | |
| // convertListToRegex("gemini-2.5-flash,gpt-3.5-turbo,old-llama-model", false) -> "^(?!(gemini-2\\.5-flash|gpt-3\\.5-turbo|old-llama-model)$).*" |
| func convertListToRegex(list string, include bool) string { | ||
| // Split by comma and trim spaces | ||
| items := strings.Split(list, ",") | ||
| for i, item := range items { | ||
| // Trim whitespace | ||
| item = strings.TrimSpace(item) | ||
| // Escape dots for regex | ||
| item = strings.ReplaceAll(item, ".", "\\.") | ||
| items[i] = item | ||
| } | ||
|
|
||
| // Join with pipe separator and wrap in regex anchors | ||
| if include { | ||
| return fmt.Sprintf("^(%s)$", strings.Join(items, "|")) | ||
| } | ||
|
|
||
| return fmt.Sprintf("^(?!(%s)$).*", strings.Join(items, "|")) | ||
| } |
There was a problem hiding this comment.
The new convertListToRegex function lacks test coverage. Given that this function performs critical regex pattern generation with special character escaping and complex logic for allow/deny lists, it should have comprehensive unit tests covering edge cases such as: empty lists, lists with special regex characters, single-item lists, and both include=true and include=false branches.
| languages: | ||
| - js | ||
| - ts | ||
| pattern-regex: "\\.(toLocale(Date|Time)?String)\\(\"[^\"]+\"" |
There was a problem hiding this comment.
This regex pattern may have unintended matches. The pattern "\\.(toLocale(Date|Time)?String)\\(\"[^\"]+\"" will only match cases where the locale string is immediately after the opening parenthesis with no whitespace. It won't match cases like .toLocaleDateString( "en-US") or .toLocaleDateString(options, "en-US"). Consider using a more flexible pattern that accounts for optional whitespace and multiple parameters.
| pattern-regex: "\\.(toLocale(Date|Time)?String)\\(\"[^\"]+\"" | |
| pattern-regex: "\\.(toLocale(Date|Time)?String)\\([^)]*\"[^\"]+\"" |
| languages: | ||
| - js | ||
| - ts | ||
| pattern-regex: "<(h1|h2|h3|h4|h5|h6|p|span|div|td|th)[^>]*>[^<{]*[A-Za-z][^<{]*</\\1>" |
There was a problem hiding this comment.
This regex pattern is overly broad and will produce many false positives. The pattern <(h1|h2|h3|h4|h5|h6|p|span|div|td|th)[^>]*>[^<{]*[A-Za-z][^<{]*</\\1> will flag any element containing a single letter, including legitimate cases like variable names (e.g., <span>{x}</span>), single-letter abbreviations, or code examples. The pattern also doesn't account for numbers or punctuation-only content that might be acceptable. Consider making this pattern more specific to catch meaningful text content (e.g., multiple words) rather than any alphabetic character.
| pattern-regex: "<(h1|h2|h3|h4|h5|h6|p|span|div|td|th)[^>]*>[^<{]*[A-Za-z][^<{]*</\\1>" | |
| pattern-regex: "<(h1|h2|h3|h4|h5|h6|p|span|div|td|th)[^>]*>[^<{]*[A-Za-z]{2,}[^<{]*\\s+[A-Za-z]{2,}[^<{]*</\\1>" |
| var response = await client.Models.GenerateContentAsync( | ||
| model: "deepseek-v3.2", contents: "Explain how AI works in a few words" | ||
| ); | ||
| var response2 = await client.Models.GenerateContentAsync( |
No description provided.