Skip to content

t1323: Fix TTSR shell-local-params rule false-positives on currency/pricing text #2296

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.

Sign up for GitHub

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

Merged
marcusquinn merged 1 commit into from
Feb 25, 2026
Merged

t1323: Fix TTSR shell-local-params rule false-positives on currency/pricing text #2296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions .agents/plugins/opencode-aidevops/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1510,9 +1510,11 @@ const BUILTIN_TTSR_RULES = [
// Only match bare $N at the start of a line or after whitespace in what looks
// like a shell assignment/command context — avoids matching $1 inside prose,
// documentation, quoted examples, or tool output from file reads.
// Excludes currency/pricing patterns: $[1-9] followed by digits, decimal, comma,
// or slash (e.g. $28/mo, $1.99, $1,000) to prevent false-positives in markdown tables.
pattern: "^\\s+(?:echo|printf|return|if|\\[\\[).*\\$[1-9](?![0-9.,/])(?!.*local\\s+\\w+=)",
// Excludes currency/pricing patterns:
// - $[1-9] followed by digits, decimal, comma, or slash (e.g. $28/mo, $1.99, $1,000)
// - $[1-9] followed by pipe (markdown table cell boundary)
// - $[1-9] followed by common currency/pricing unit words (per, mo, month, flat, etc.)
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+=)",
Copy link

@gemini-code-assist gemini-code-assist bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For improved readability and maintainability of this complex regular expression, you can make it more concise by grouping related alternatives. For example, year|yr can be combined into y(?:ea)?r, and hr|hour can be h(?:ou)?r. This makes the pattern slightly shorter and easier to parse visually.

Suggested change
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*[|])(?!\\s+(?:per|mo(?:nth)?|y(?:ea)?r|day|week|h(?:ou)?r|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)\\b)(?!.*local\\s+\\w+=)",

Copy link

@augmentcode augmentcode bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new (?!\\s*[|]) exclusion will also suppress matches for real shell pipelines like echo $1 | ..., potentially turning true violations into false negatives. Is that tradeoff intended, given | is common shell syntax and not unique to markdown tables?

Severity: medium

Other Locations
  • .agents/scripts/linters-local.sh:194
  • .agents/scripts/pre-commit-hook.sh:83

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

correction: "Use `local var=\"$1\"` pattern — never use positional parameters directly (SonarCloud S7679).",
severity: "warn",
systemPrompt: "Shell scripts: use `local var=\"$1\"` — never use $1 directly in function bodies.",
Expand Down
6 changes: 5 additions & 1 deletion .agents/scripts/linters-local.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,12 @@ check_positional_parameters() {
/print.*\$[1-9]/ { next }
/Usage:/ { next }
# Skip currency/pricing patterns: $[1-9] followed by digit, decimal, comma,
# or slash (e.g. $28/mo, $1.99, $1,000) — false-positives in markdown tables.
# slash (e.g. $28/mo, $1.99, $1,000), pipe (markdown table), or common
# currency/pricing unit words (per, mo, month, flat, etc.).
/\$[1-9][0-9.,\/]/ { next }
/\$[1-9][[:space:]]*\|/ { next }
/\$[1-9][[:space:]]+(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)[[:space:][:punct:]]/ { next }
/\$[1-9][[:space:]]+(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)$/ { next }
Comment on lines +195 to +196
Copy link

@gemini-code-assist gemini-code-assist bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

These two lines are nearly identical, with the only difference being the match at the end of the line ([[:space:][:punct:]] vs. $). You can combine them into a single, more maintainable rule by using an alternation ([[:space:][:punct:]]|$). This avoids duplicating the long list of keywords and makes future updates easier. I've also included a small simplification for the keywords themselves.

Suggested change
/\$[1-9][[:space:]]+(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)[[:space:][:punct:]]/ { next }
/\$[1-9][[:space:]]+(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)$/ { next }
/\$[1-9][[:space:]]+(per|mo(nth)?|y(ea)?r|day|week|h(ou)?r|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)([[:space:][:punct:]]|$)/ { next }

in_func && /\$[1-9]/ && !/local.*=.*\$[1-9]/ {
print FILENAME ":" NR ": " $0
}
Expand Down
7 changes: 4 additions & 3 deletions .agents/scripts/pre-commit-hook.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,11 @@ validate_positional_parameters() {

for file in "$@"; do
# Exclude currency/pricing patterns: $[1-9] followed by digit, decimal, comma,
# or slash (e.g. $28/mo, $1.99, $1,000) — false-positives in markdown tables.
if [[ -f "$file" ]] && grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' >/dev/null; then
# slash (e.g. $28/mo, $1.99, $1,000), pipe (markdown table cell), or common
# currency/pricing unit words (per, mo, month, flat, etc.).
if [[ -f "$file" ]] && grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' | grep -vE '\$[1-9]\s*\|' | grep -vE '\$[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' >/dev/null; then
Copy link

@augmentcode augmentcode bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grep -E (ERE/POSIX) doesn’t reliably support escapes like \\s and \\b, so these new exclusions may not work and could reintroduce the currency false-positives this PR is trying to eliminate. This affects both the if check and the diagnostic pipeline below.

Severity: high

Other Locations
  • .agents/scripts/pre-commit-hook.sh:85

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

print_error "Direct positional parameter usage in $file"
grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' | head -3
grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' | grep -vE '\$[1-9]\s*\|' | grep -vE '\$[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' | head -3
Comment on lines +83 to +85
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find and read the pre-commit-hook.sh file
find . -name "pre-commit-hook.sh" -type f

Repository: marcusquinn/aidevops

Length of output: 100

🏁 Script executed:

# Read the file around the reported lines
if [ -f ".agents/scripts/pre-commit-hook.sh" ]; then
  wc -l ".agents/scripts/pre-commit-hook.sh"
fi

Repository: marcusquinn/aidevops

Length of output: 102

🏁 Script executed:

# Get context around lines 80-85
if [ -f ".agents/scripts/pre-commit-hook.sh" ]; then
  sed -n '75,90p' ".agents/scripts/pre-commit-hook.sh"
fi

Repository: marcusquinn/aidevops

Length of output: 1083

🌐 Web query:

macOS BSD grep \s \b support extended regular expressions ERE compatibility

💡 Result:

On macOS’s default (BSD/Apple) grep, -E enables POSIX ERE, but grep also enables Apple’s “enhanced” regex features from re_format(7), which include the shortcuts and assertions you’re asking about:

  • \s / \S are supported as enhanced shortcuts (\s ≡ [[:space:]]). [1]
  • \b / \B are supported as enhanced word-boundary assertions (and \</\> are available too, with [[:<:]] / [[:>:]] equivalents). [1]
  • This is not POSIX ERE portability: many greps treat \s and \b as non-special (Autoconf explicitly warns they’re nonportable). [2]

If you need portable ERE, use:

  • whitespace: [[:space:]]
  • word boundaries (BSD-style): [[:<:]]word[[:>:]] (or use grep -w for whole words when that’s sufficient). [1]

Also note: other macOS tools (notably sed, find -E -regex) may not be compiled with the enhanced flag even though re_format(7) documents it, so \s/\b can fail there—grep is the common one that does work. [3]

Sources:
[1] macOS re_format(7) (“ENHANCED FEATURES”: \s, \b, \</\> and their [[:...:]] equivalents) https://www.unix.com/man-page/osx/7/re_format
[2] GNU Autoconf manual: portability warning to avoid \b, \s, etc. https://www.gnu.org/software/autoconf/manual/autoconf-2.65/html_node/Limitations-of-Usual-Tools.html
[3] Discussion of macOS enhanced vs extended support and that grep supports enhanced features while some other tools don’t https://stackoverflow.com/questions/12178924/os-x-sed-e-doesnt-accept-extended-regular-expressions

🏁 Script executed:

# Check if ShellCheck is available and run it on the file
which shellcheck || echo "ShellCheck not available"
if command -v shellcheck &> /dev/null; then
  shellcheck ".agents/scripts/pre-commit-hook.sh" || true
fi

Repository: marcusquinn/aidevops

Length of output: 149

🏁 Script executed:

# Also check the specific lines for context and verify the duplication more clearly
sed -n '76,90p' ".agents/scripts/pre-commit-hook.sh" | cat -n

Repository: marcusquinn/aidevops

Length of output: 1168

Refactor the grep pipeline to eliminate duplication and improve readability, but note that \s and \b work on macOS.

While \s and \b are supported by macOS BSD grep as enhanced regex features (per re_format(7)), the grep pipeline duplicated on lines 83 and 85 violates the DRY principle and creates a maintenance liability. If either line changes, the other must be manually updated or behavior diverges silently.

Extract the grep result into a variable to eliminate duplication:

local matches
matches=$(grep -n '\$[1-9]' "$file" \
	| grep -v 'local.*=.*\$[1-9]' \
	| grep -vE '\$[1-9][0-9.,/]' \
	| grep -vE '\$[1-9]\s*\|' \
	| grep -vE '\$[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' \
	|| true)
if [[ -f "$file" ]] && [[ -n "$matches" ]]; then
	print_error "Direct positional parameter usage in $file"
	echo "$matches" | head -3
	((violations++))
fi

This also improves readability by breaking the long pipeline into logical sections.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/pre-commit-hook.sh around lines 83 - 85, Replace the
duplicated grep pipeline by capturing its output into a variable (e.g., matches)
and reusing that variable for the existence check and printing; specifically,
run the pipeline that starts with grep -n '\$[1-9]' "$file" and the subsequent
grep -v/grep -vE filters once, assign its (possibly empty) result to matches
(use "|| true" to avoid non-zero exit), then change the if condition to check [[
-f "$file" ]] && [[ -n "$matches" ]], call print_error "Direct positional
parameter usage in $file", echo "$matches" | head -3, and increment
((violations++)); update references to the original duplicated pipeline
locations to use matches instead.

((violations++))
fi
Comment on lines +83 to 87
Copy link

@gemini-code-assist gemini-code-assist bot Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The long grep pipeline is executed twice: once in the if condition to check for violations, and then again to print the first few violating lines. This is inefficient and creates significant code duplication, making the script harder to maintain. A better approach is to run the pipeline once, store its output in a variable, and then use that variable for both the check and the output. This improves performance and eliminates the duplicated code.

Suggested change
if [[ -f "$file" ]] && grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' | grep -vE '\$[1-9]\s*\|' | grep -vE '\$[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' >/dev/null; then
print_error "Direct positional parameter usage in $file"
grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' | head -3
grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' | grep -vE '\$[1-9]\s*\|' | grep -vE '\$[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' | head -3
((violations++))
fi
if [[ -f "$file" ]]; then
local violations_output
violations_output=$(grep -n '\$[1-9]' "$file" | grep -v 'local.*=.*\$[1-9]' | grep -vE '\$[1-9][0-9.,/]' | grep -vE '\$[1-9]\s*\|' | grep -vE '\$[1-9]\s+(per|mo(nth)?|y(ea)?r|day|week|h(ou)?r|flat|each|off|fee|plan|tier|user|seat|unit|addon|setup|trial|credit|annual|quarterly|monthly)\b')
if [[ -n "$violations_output" ]]; then
print_error "Direct positional parameter usage in $file"
echo "$violations_output" | head -3
((violations++))
fi
fi
References
  1. The principle of consolidating multiple executions of a command/pipeline into a single pass to improve performance and efficiency applies here, even though it's a grep pipeline instead of jq.
  2. Storing the output of the grep pipeline in violations_output before using it multiple times is analogous to moving a loop-invariant calculation outside a loop, as it avoids redundant computation/execution and improves efficiency.

done
Expand Down
Loading