t1323: Fix TTSR shell-local-params rule false-positives on currency/pricing text#2388
Conversation
Expand false-positive exclusions in both validatePositionalParams() and the TTSR shell-local-params regex to handle: - $N followed by pricing words (flat, fee, per, monthly, etc.) - Escaped dollar signs \ (literal dollar in shell strings) - Markdown table rows (lines starting with |) - $N followed by pipe (table cell boundary) The existing exclusion only caught $N followed by [0-9.,/] which missed single-digit amounts like $5 flat or $3 fee. The TTSR regex also gains a negative lookbehind for backslash-escaped dollar signs. Closes #2194
Summary of ChangesHello @marcusquinn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the logic for detecting direct positional parameter usage in shell scripts, specifically addressing false positives related to currency and pricing text. By introducing more sophisticated exclusion rules within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughUpdates detection of unescaped positional params: added escaping-aware pre-filtering and multiple false-positive filters (currency, pricing/unit phrases, markdown table/pipe cases) in validator; adjusted TTSR Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Feb 26 18:35:57 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/plugins/opencode-aidevops/index.mjs:
- Around line 811-813: The check that currently does if (/\\\$[1-9]/.test(line))
{ continue; } skips the entire line when it contains any escaped dollar, hiding
real unescaped positional params; instead detect unescaped positional params by
either removing escaped dollars first (e.g., let s = line.replace(/\\\$\d/g, '')
and test s) or use a negative-lookbehind to match unescaped tokens
(/((?<!\\)\$[1-9])/) and only continue when no unescaped match exists—update the
if to use one of these approaches on the variable line so mixed content like
`"\$5 fee $1"` still triggers detection for $1.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses false positives in shell parameter validation by expanding the exclusion patterns for currency and pricing information. The addition of a negative lookbehind in the TTSR rule to handle escaped dollar signs is also a solid improvement. I have one suggestion to consolidate the new conditional checks for better readability and maintainability.
| if (/\$[1-9][0-9.,/]/.test(trimmed)) { | ||
| continue; | ||
| } | ||
| if (/\$[1-9]\s+(?:per|mo(?:nth)?|year|yr|day|week|hr|hour|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)\b/.test(trimmed)) { | ||
| continue; | ||
| } | ||
| if (/\\\$[1-9]/.test(line)) { | ||
| continue; | ||
| } | ||
| if (/^\s*\|/.test(line)) { | ||
| continue; | ||
| } | ||
| if (/\$[1-9]\s*\|/.test(trimmed)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
For better readability and maintainability, you can consolidate these consecutive if statements into a single condition using the logical OR (||) operator. This makes the code more compact and easier to read, and simplifies adding or removing patterns in the future.
| if (/\$[1-9][0-9.,/]/.test(trimmed)) { | |
| continue; | |
| } | |
| if (/\$[1-9]\s+(?:per|mo(?:nth)?|year|yr|day|week|hr|hour|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)\b/.test(trimmed)) { | |
| continue; | |
| } | |
| if (/\\\$[1-9]/.test(line)) { | |
| continue; | |
| } | |
| if (/^\s*\|/.test(line)) { | |
| continue; | |
| } | |
| if (/\$[1-9]\s*\|/.test(trimmed)) { | |
| continue; | |
| } | |
| if ( | |
| /\$[1-9][0-9.,/]/.test(trimmed) || | |
| /\$[1-9]\s+(?:per|mo(?:nth)?|year|yr|day|week|hr|hour|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)\b/.test(trimmed) || | |
| /\\\$[1-9]/.test(line) || | |
| /^\s*\|/.test(line) || | |
| /\$[1-9]\s*\|/.test(trimmed) | |
| ) { | |
| continue; | |
| } |
…nescaped dollars Replace whole-line skip for escaped dollar signs with strip-then-test approach: remove escaped $N patterns first, then check if any unescaped $[1-9] remains. This ensures mixed content like '$5 fee $1' still detects the real $1 violation instead of skipping the entire line. Addresses CodeRabbit review feedback on PR #2388.
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Feb 26 18:48:24 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/plugins/opencode-aidevops/index.mjs:
- Around line 821-822: The pipe-exclusion regex if
(/\$[1-9]\s*\|/.test(stripped)) continue; is too broad and skips valid shell
uses (e.g. echo "$1" | ...); narrow it to apply only when the line is a markdown
table row (or remove it entirely because table-row detection already exists).
Change the check in the runtime validator to first verify the markdown-table
context (use the existing table-row detection helper or a boolean like
isTableRow/insideTable) before applying the /\$[1-9]\s*\|/ test, and make the
same adjustment to the TTSR pattern usage referenced at line ~1539 so pipe
skipping only occurs for real markdown table rows.
- Around line 812-814: The current check if (/\$[1-9][0-9.,/]/.test(stripped))
incorrectly treats patterns like "$1/config" as currency; update the regex used
in that conditional to exclude '/' (e.g., change the character class to [0-9.,]
and ensure it matches currency-like sequences such as decimals or comma groups
only), so the if condition on stripped only skips true currency patterns and not
paths like "$1/config".
| if (/\$[1-9][0-9.,/]/.test(stripped)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
$1/… is being misclassified as pricing and skipped.
Line 812 treats any $[1-9] followed by / as currency. That hides real violations like "$1/config" in shell scripts, reducing enforcement coverage.
Proposed fix
- if (/\$[1-9][0-9.,/]/.test(stripped)) {
+ if (
+ // Multi-digit or formatted amounts, optionally with billing unit
+ /\$[1-9]\d[0-9.,]*(?:\/(?:mo(?:nth)?|yr|year|day|week|hr|hour))?\b/.test(stripped) ||
+ // Single-digit amount with explicit billing unit (e.g. $5/mo)
+ /\$[1-9]\/(?:mo(?:nth)?|yr|year|day|week|hr|hour)\b/.test(stripped)
+ ) {
continue;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/plugins/opencode-aidevops/index.mjs around lines 812 - 814, The
current check if (/\$[1-9][0-9.,/]/.test(stripped)) incorrectly treats patterns
like "$1/config" as currency; update the regex used in that conditional to
exclude '/' (e.g., change the character class to [0-9.,] and ensure it matches
currency-like sequences such as decimals or comma groups only), so the if
condition on stripped only skips true currency patterns and not paths like
"$1/config".
| if (/\$[1-9]\s*\|/.test(stripped)) { | ||
| continue; |
There was a problem hiding this comment.
Pipe exclusion is too broad and misses real shell violations.
The current \$N ... | exclusions skip valid detections such as echo "$1" | sed .... This affects both the runtime validator (Line 821) and TTSR pattern (Line 1539). For A-grade shell quality, constrain pipe-based skipping to markdown-table context only (or remove it here, since table-row detection already exists).
Proposed fix
- if (/\$[1-9]\s*\|/.test(stripped)) {
- continue;
- }
+ // Keep pipeline cases detectable; markdown table rows are already
+ // handled by the line-start pipe check above.- pattern: "^\\s+(?:echo|printf|return|if|\\[\\[).*(?<!\\\\)\\$[1-9](?![0-9.,/])(?!\\s*[|])(?!\\s+(?:per|mo(?:nth)?|year|yr|day|week|hr|hour|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)\\b)(?!.*local\\s+\\w+=)",
+ pattern: "^\\s+(?:echo|printf|return|if|\\[\\[).*(?<!\\\\)\\$[1-9](?![0-9.,/])(?!\\s+(?:per|mo(?:nth)?|year|yr|day|week|hr|hour|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)\\b)(?!.*local\\s+\\w+=)",Also applies to: 1539-1539
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/plugins/opencode-aidevops/index.mjs around lines 821 - 822, The
pipe-exclusion regex if (/\$[1-9]\s*\|/.test(stripped)) continue; is too broad
and skips valid shell uses (e.g. echo "$1" | ...); narrow it to apply only when
the line is a markdown table row (or remove it entirely because table-row
detection already exists). Change the check in the runtime validator to first
verify the markdown-table context (use the existing table-row detection helper
or a boolean like isTableRow/insideTable) before applying the /\$[1-9]\s*\|/
test, and make the same adjustment to the TTSR pattern usage referenced at line
~1539 so pipe skipping only occurs for real markdown table rows.
…onditional Addresses Gemini code review suggestion on PR #2388 to merge 4 separate if-continue blocks into a single compound condition for better readability and maintainability. No logic change — all 27 test cases still pass.
|
@coderabbitai review Both review comments have been addressed:
All 27 test cases pass (22 false-positive exclusions + 5 true-positive detections). |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Feb 26 18:54:44 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Great to hear all 27 test cases are passing. I'll review the updated changes now. ✅ Actions performedReview triggered.
|
…path false-positives - Remove '/' from currency character class so $1/config is detected as a violation instead of being misclassified as pricing - Add explicit $N/billing-unit pattern (mo, yr, year, day, week, hr, hour) to still skip genuine pricing like $5/mo - Remove overly broad pipe-exclusion regex ($N|) that was hiding valid shell violations like echo "$1" | sed; markdown table rows are already handled by the line-start pipe check - Update TTSR pattern to match runtime validator changes
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Thu Feb 26 19:08:54 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
|
@coderabbitai review All three review findings have been addressed in commit e29a13c:
Verified with 32 test cases covering all edge cases (paths, billing units, pipes, markdown tables, escaped dollars, mixed content). |
|
32 test cases covering all the edge cases (paths, billing units, pipes, markdown tables, escaped dollars, mixed content) is thorough validation. I'll review the updated changes now. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedReview triggered.
|
marcusquinn
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
marcusquinn
deleted the
bugfix/ttsr-shell-local-params-currency-false-positives
branch
1 participant
Summary
validatePositionalParams()quality hook to handle single-digit amounts ($5 flat,$3 fee), escaped dollar signs (\$49/mo), markdown table rows, and pipe-delimited cells\$N) in the TTSRshell-local-paramsregex patternChanges
.agents/plugins/opencode-aidevops/index.mjsvalidatePositionalParams()(quality hook for.shfiles): Added 4 new exclusion checks:$Nfollowed by pricing/unit words (per, flat, fee, monthly, etc.)\$N(literal dollar sign in shell strings)|(markdown table rows in heredocs)$Nfollowed by|(markdown table cell boundary)TTSR
shell-local-paramsrule: Added(?<!\\)negative lookbehind before\$[1-9]to skip escaped dollar signs in assistant output scanning.Testing
validatePositionalParamstests pass (23 false-positive exclusions + 4 true-positive detections)domain-research-helper.sh) — correctly identifies true violations while excluding\$49/mopricing patternsCloses #2194
Summary by CodeRabbit