quality-debt: .agents/scripts/linters-local.sh — PR #2923 review feedback (medium)

[marcusquinn]

Unactioned Review Feedback

Source PR: #2923
File: .agents/scripts/linters-local.sh
Reviewers: gemini
Findings: 1
Max severity: medium

---

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:387

While this change correctly attempts to kill the process group with a fallback, suppressing stderr for both kill commands with 2>/dev/null can hide important errors, such as "Operation not permitted". This goes against the project's general rules which advise against blanket error suppression. While removing the suppression may result in "No such process" messages in the log if the process has already completed, it ensures that actual failures in the timeout mechanism are visible for debugging.

(sleep "$sc_timeout" && kill -- -"$sc_bg_pid" || kill "$sc_bg_pid") &

References

1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

View comment

---

Auto-generated by quality-feedback-helper.sh scan-merged. Review each finding and either fix the code or dismiss with a reason.

[marcusquinn]

Additional Review Feedback (PR #2855)

Reviewers: coderabbit
Findings: 1
Max severity: high

---

HIGH: coderabbit (coderabbitai[bot])

File: .agents/scripts/linters-local.sh:376
⚠️ Potential issue | 🟠 Major

Fallback path reopens the runaway ShellCheck risk.

Line 350 runs shellcheck -x -P SCRIPTDIR with no timeout when neither timeout nor gtimeout exists, which can recreate the long-running expansion scenario.

🔧 Suggested fix

 		else
-			file_result=$(shellcheck -x -P SCRIPTDIR --severity=warning --format=gcc "$file" 2>&1) || true
+			print_warning "ShellCheck: timeout utility not found; running without -x to avoid recursive expansion risk"
+			file_result=$(shellcheck -P SCRIPTDIR --severity=warning --format=gcc "$file" 2>&1) || true
 		fi

📝 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.

		else
			print_warning "ShellCheck: timeout utility not found; running without -x to avoid recursive expansion risk"
			file_result=$(shellcheck -P SCRIPTDIR --severity=warning --format=gcc "$file" 2>&1) || true
		fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/linters-local.sh around lines 349 - 351, The fallback branch
currently runs shellcheck without a timeout (the line assigning file_result from
shellcheck -x -P SCRIPTDIR ...), risking long-running expansions; replace that
direct call by invoking a small portable timeout wrapper (e.g., start shellcheck
in the background, capture its PID, launch a watcher that sleeps a configured
TIMEOUT and kills the PID if still running) and assign its stdout/stderr to
file_result the same way; ensure the wrapper is used only when neither timeout
nor gtimeout is available and references the same shellcheck invocation (the
shellcheck -x -P SCRIPTDIR --severity=warning --format=gcc "$file") and
preserves exit handling (|| true).

✅ Addressed in commits db8a514 to 959e12d

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[marcusquinn]

Additional Review Feedback (PR #2296)

Reviewers: gemini
Findings: 1
Max severity: medium

---

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:196

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.

            /\$[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 }

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[marcusquinn]

Additional Review Feedback (PR #2733)

Reviewers: coderabbit, gemini
Findings: 4
Max severity: high

---

HIGH: coderabbit (coderabbitai[bot])

File: .agents/scripts/linters-local.sh:689
⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check the actual file and verify the line range
cd /repo || exit 1
wc -l .agents/scripts/linters-local.sh

Repository: marcusquinn/aidevops

Length of output: 119

---

🏁 Script executed:

# Read the specific lines mentioned in the review
sed -n '666,674p' .agents/scripts/linters-local.sh | cat -n

Repository: marcusquinn/aidevops

Length of output: 306

---

🏁 Script executed:

# Get broader context around those lines to understand usage
sed -n '655,685p' .agents/scripts/linters-local.sh | cat -n

Repository: marcusquinn/aidevops

Length of output: 1121

---

🏁 Script executed:

# Search for other usages of frontmatter parsing in the file
rg -n "in_fm|frontmatter|---" .agents/scripts/linters-local.sh | head -30

Repository: marcusquinn/aidevops

Length of output: 794

---

🏁 Script executed:

#!/bin/bash

# Create test files to validate the concern

# Test 1: Normal frontmatter (should work with both)
cat > /tmp/test1.md <<'EOF'
---
name: "test-skill"
description: "A test skill"
---
# Body content
Some documentation here.
EOF

# Test 2: Body contains --- marker (current impl has false positive risk)
cat > /tmp/test2.md <<'EOF'
---
name: "correct-name"
description: "A test skill"
---
# Body content

Example code block:
---
name: "false-positive"
---
EOF

# Test 3: Frontmatter with inline comments
cat > /tmp/test3.md <<'EOF'
---
name: "valid-name" # comment here
description: "A test skill"
---
Body content
EOF

echo "=== Test 1: Normal frontmatter ==="
echo "Current implementation:"
awk '
  /^---$/ { in_fm = !in_fm; next }
  in_fm && /^name:/ {
    sub(/^name: */, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test1.md

echo ""
echo "=== Test 2: Body contains --- (FALSE POSITIVE RISK) ==="
echo "Current implementation:"
awk '
  /^---$/ { in_fm = !in_fm; next }
  in_fm && /^name:/ {
    sub(/^name: */, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test2.md

echo ""
echo "Proposed implementation:"
awk '
  NR == 1 && /^---$/ { in_fm = 1; next }
  in_fm && /^---$/ { exit }
  in_fm && /^[[:space:]]*name:[[:space:]]*/ {
    sub(/^[[:space:]]*name:[[:space:]]*/, "")
    sub(/[[:space:]]+#.*$/, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test2.md

echo ""
echo "=== Test 3: Frontmatter with inline comments ==="
echo "Current implementation:"
awk '
  /^---$/ { in_fm = !in_fm; next }
  in_fm && /^name:/ {
    sub(/^name: */, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test3.md

echo ""
echo "Proposed implementation (handles comments):"
awk '
  NR == 1 && /^---$/ { in_fm = 1; next }
  in_fm && /^---$/ { exit }
  in_fm && /^[[:space:]]*name:[[:space:]]*/ {
    sub(/^[[:space:]]*name:[[:space:]]*/, "")
    sub(/[[:space:]]+#.*$/, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test3.md

rm -f /tmp/test*.md

Repository: marcusquinn/aidevops

Length of output: 423

---

Restrict name extraction to opening frontmatter block and handle inline YAML comments.

Current toggling on every --- risks reading name: from the document body when frontmatter lacks it, producing false passes. Additionally, inline YAML comments are not stripped, causing validation mismatches.

🔧 Parser hardening fix

-		fm_name=$(awk '
-			/^---$/ { in_fm = !in_fm; next }
-			in_fm && /^name:/ {
-				sub(/^name: */, "")
-				gsub(/^["'"'"']|["'"'"']$/, "")
-				print
-				exit
-			}
-		' "$local_path")
+		fm_name=$(awk '
+			NR == 1 && /^---$/ { in_fm = 1; next }
+			in_fm && /^---$/ { exit }
+			in_fm && /^[[:space:]]*name:[[:space:]]*/ {
+				sub(/^[[:space:]]*name:[[:space:]]*/, "")
+				sub(/[[:space:]]+#.*$/, "")
+				gsub(/^["'"'"']|["'"'"']$/, "")
+				print
+				exit
+			}
+		' "$local_path")

Automation scripts require robust input validation. This hardening prevents mismatched skill validation and ensures clean value extraction.

📝 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.

		fm_name=$(awk '
			NR == 1 && /^---$/ { in_fm = 1; next }
			in_fm && /^---$/ { exit }
			in_fm && /^[[:space:]]*name:[[:space:]]*/ {
				sub(/^[[:space:]]*name:[[:space:]]*/, "")
				sub(/[[:space:]]+#.*$/, "")
				gsub(/^["'"'"']|["'"'"']$/, "")
				print
				exit
			}
		' "$local_path")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/linters-local.sh around lines 666 - 674, The fm_name
extraction should only search the initial frontmatter block and strip inline
YAML comments: change the awk script used for fm_name (reading "$local_path") to
first detect an opening frontmatter only at the file start (e.g., begin scanning
only after the first /^---$/ and stop after the next /^---$/ so you don't toggle
on every ---), then when matching /^name:/ remove the key prefix, strip any
inline comment portion (e.g., sub(/#.*/, "") or similar) and then strip
surrounding quotes (the existing gsub for quotes can remain), print the cleaned
value and exit to avoid reading from the document body.

✅ Addressed in commit aadc945

View comment

HIGH: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:689

The check_skill_frontmatter function reads file paths from skill-sources.json and passes them directly to awk. If a file path starts with a hyphen (e.g., -f/path/to/script), awk will interpret it as an option. An attacker can exploit this to execute arbitrary awk code, which can in turn execute shell commands via the system() function. This is particularly dangerous in CI/CD environments where the linter might be run on untrusted PR content.

		fm_name=$(awk '
			/^---$/ { in_fm = !in_fm; next }
			in_fm && /^name:/ {
				sub(/^name: */, "")
				gsub(/^[\"'"'"']|[\"'"'"']$/, "")
				print
				exit
			}
		' -- "$local_path")

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:647

Suppressing stderr with 2>/dev/null can hide important syntax errors if skill-sources.json is malformed. You can make the jq query more robust by using the alternative operator // to provide a default empty array if .skills is null or doesn't exist. This avoids the error that || echo "0" is intended to catch, and allows you to remove the error suppression so that actual file format errors are visible.

This aligns with the project's general rule to avoid blanket error suppression.

	skill_count=$(jq '(.skills // []) | length' "$skill_sources")

References

1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:685

Similar to the count check, suppressing stderr here with 2>/dev/null can hide JSON syntax errors. By using (.skills // [])[], the query will gracefully handle cases where skills is missing or null by iterating over an empty array, producing no output. This makes the error suppression unnecessary and ensures that developers are alerted to malformed JSON files.

This follows the project's general rule to avoid unnecessary error suppression.

	done < <(jq -r '(.skills // [])[] | "\(.name)|\(.local_path)"' "$skill_sources")

References

1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[marcusquinn]

Additional Review Feedback (PR #2296)

Reviewers: gemini
Findings: 1
Max severity: medium

---

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:196

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.

            /\$[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 }

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[marcusquinn]

Additional Review Feedback (PR #2733)

Reviewers: coderabbit, gemini
Findings: 4
Max severity: high

---

HIGH: coderabbit (coderabbitai[bot])

File: .agents/scripts/linters-local.sh:689
⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's check the actual file and verify the line range
cd /repo || exit 1
wc -l .agents/scripts/linters-local.sh

Repository: marcusquinn/aidevops

Length of output: 119

---

🏁 Script executed:

# Read the specific lines mentioned in the review
sed -n '666,674p' .agents/scripts/linters-local.sh | cat -n

Repository: marcusquinn/aidevops

Length of output: 306

---

🏁 Script executed:

# Get broader context around those lines to understand usage
sed -n '655,685p' .agents/scripts/linters-local.sh | cat -n

Repository: marcusquinn/aidevops

Length of output: 1121

---

🏁 Script executed:

# Search for other usages of frontmatter parsing in the file
rg -n "in_fm|frontmatter|---" .agents/scripts/linters-local.sh | head -30

Repository: marcusquinn/aidevops

Length of output: 794

---

🏁 Script executed:

#!/bin/bash

# Create test files to validate the concern

# Test 1: Normal frontmatter (should work with both)
cat > /tmp/test1.md <<'EOF'
---
name: "test-skill"
description: "A test skill"
---
# Body content
Some documentation here.
EOF

# Test 2: Body contains --- marker (current impl has false positive risk)
cat > /tmp/test2.md <<'EOF'
---
name: "correct-name"
description: "A test skill"
---
# Body content

Example code block:
---
name: "false-positive"
---
EOF

# Test 3: Frontmatter with inline comments
cat > /tmp/test3.md <<'EOF'
---
name: "valid-name" # comment here
description: "A test skill"
---
Body content
EOF

echo "=== Test 1: Normal frontmatter ==="
echo "Current implementation:"
awk '
  /^---$/ { in_fm = !in_fm; next }
  in_fm && /^name:/ {
    sub(/^name: */, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test1.md

echo ""
echo "=== Test 2: Body contains --- (FALSE POSITIVE RISK) ==="
echo "Current implementation:"
awk '
  /^---$/ { in_fm = !in_fm; next }
  in_fm && /^name:/ {
    sub(/^name: */, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test2.md

echo ""
echo "Proposed implementation:"
awk '
  NR == 1 && /^---$/ { in_fm = 1; next }
  in_fm && /^---$/ { exit }
  in_fm && /^[[:space:]]*name:[[:space:]]*/ {
    sub(/^[[:space:]]*name:[[:space:]]*/, "")
    sub(/[[:space:]]+#.*$/, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test2.md

echo ""
echo "=== Test 3: Frontmatter with inline comments ==="
echo "Current implementation:"
awk '
  /^---$/ { in_fm = !in_fm; next }
  in_fm && /^name:/ {
    sub(/^name: */, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test3.md

echo ""
echo "Proposed implementation (handles comments):"
awk '
  NR == 1 && /^---$/ { in_fm = 1; next }
  in_fm && /^---$/ { exit }
  in_fm && /^[[:space:]]*name:[[:space:]]*/ {
    sub(/^[[:space:]]*name:[[:space:]]*/, "")
    sub(/[[:space:]]+#.*$/, "")
    gsub(/^["'"'"']|["'"'"']$/, "")
    print
    exit
  }
' /tmp/test3.md

rm -f /tmp/test*.md

Repository: marcusquinn/aidevops

Length of output: 423

---

Restrict name extraction to opening frontmatter block and handle inline YAML comments.

Current toggling on every --- risks reading name: from the document body when frontmatter lacks it, producing false passes. Additionally, inline YAML comments are not stripped, causing validation mismatches.

🔧 Parser hardening fix

-		fm_name=$(awk '
-			/^---$/ { in_fm = !in_fm; next }
-			in_fm && /^name:/ {
-				sub(/^name: */, "")
-				gsub(/^["'"'"']|["'"'"']$/, "")
-				print
-				exit
-			}
-		' "$local_path")
+		fm_name=$(awk '
+			NR == 1 && /^---$/ { in_fm = 1; next }
+			in_fm && /^---$/ { exit }
+			in_fm && /^[[:space:]]*name:[[:space:]]*/ {
+				sub(/^[[:space:]]*name:[[:space:]]*/, "")
+				sub(/[[:space:]]+#.*$/, "")
+				gsub(/^["'"'"']|["'"'"']$/, "")
+				print
+				exit
+			}
+		' "$local_path")

Automation scripts require robust input validation. This hardening prevents mismatched skill validation and ensures clean value extraction.

📝 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.

		fm_name=$(awk '
			NR == 1 && /^---$/ { in_fm = 1; next }
			in_fm && /^---$/ { exit }
			in_fm && /^[[:space:]]*name:[[:space:]]*/ {
				sub(/^[[:space:]]*name:[[:space:]]*/, "")
				sub(/[[:space:]]+#.*$/, "")
				gsub(/^["'"'"']|["'"'"']$/, "")
				print
				exit
			}
		' "$local_path")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.agents/scripts/linters-local.sh around lines 666 - 674, The fm_name
extraction should only search the initial frontmatter block and strip inline
YAML comments: change the awk script used for fm_name (reading "$local_path") to
first detect an opening frontmatter only at the file start (e.g., begin scanning
only after the first /^---$/ and stop after the next /^---$/ so you don't toggle
on every ---), then when matching /^name:/ remove the key prefix, strip any
inline comment portion (e.g., sub(/#.*/, "") or similar) and then strip
surrounding quotes (the existing gsub for quotes can remain), print the cleaned
value and exit to avoid reading from the document body.

✅ Addressed in commit aadc945

View comment

HIGH: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:689

The check_skill_frontmatter function reads file paths from skill-sources.json and passes them directly to awk. If a file path starts with a hyphen (e.g., -f/path/to/script), awk will interpret it as an option. An attacker can exploit this to execute arbitrary awk code, which can in turn execute shell commands via the system() function. This is particularly dangerous in CI/CD environments where the linter might be run on untrusted PR content.

		fm_name=$(awk '
			/^---$/ { in_fm = !in_fm; next }
			in_fm && /^name:/ {
				sub(/^name: */, "")
				gsub(/^[\"'"'"']|[\"'"'"']$/, "")
				print
				exit
			}
		' -- "$local_path")

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:647

Suppressing stderr with 2>/dev/null can hide important syntax errors if skill-sources.json is malformed. You can make the jq query more robust by using the alternative operator // to provide a default empty array if .skills is null or doesn't exist. This avoids the error that || echo "0" is intended to catch, and allows you to remove the error suppression so that actual file format errors are visible.

This aligns with the project's general rule to avoid blanket error suppression.

	skill_count=$(jq '(.skills // []) | length' "$skill_sources")

References

1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

View comment

MEDIUM: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:685

Similar to the count check, suppressing stderr here with 2>/dev/null can hide JSON syntax errors. By using (.skills // [])[], the query will gracefully handle cases where skills is missing or null by iterating over an empty array, producing no output. This makes the error suppression unnecessary and ensures that developers are alerted to malformed JSON files.

This follows the project's general rule to avoid unnecessary error suppression.

	done < <(jq -r '(.skills // [])[] | "\(.name)|\(.local_path)"' "$skill_sources")

References

1. Avoid using '2>/dev/null' for blanket suppression of command errors in shell scripts to ensure that authentication, syntax, or system issues remain visible for debugging.

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[marcusquinn]

Additional Review Feedback (PR #2238)

Reviewers: gemini
Findings: 1
Max severity: high

---

HIGH: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:192

This awk rule is too broad and will cause false negatives. It skips any line containing a currency-like pattern (e.g., $10), even if the line also contains a valid violation (e.g., echo $1). This will cause the linter to miss the violation.

The rule should be more specific to only skip the line if it contains currency patterns but no actual positional parameter violations.

            /\$[1-9][0-9.,\/]/ && !/\$[1-9]([^0-9.,\/]|$)/ { next }

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[marcusquinn]

Additional Review Feedback (PR #2238)

Reviewers: gemini
Findings: 1
Max severity: high

---

HIGH: gemini (gemini-code-assist[bot])

File: .agents/scripts/linters-local.sh:192

This awk rule is too broad and will cause false negatives. It skips any line containing a currency-like pattern (e.g., $10), even if the line also contains a valid violation (e.g., echo $1). This will cause the linter to miss the violation.

The rule should be more specific to only skip the line if it contains currency patterns but no actual positional parameter violations.

            /\$[1-9][0-9.,\/]/ && !/\$[1-9]([^0-9.,\/]|$)/ { next }

View comment

---

Appended by quality-feedback-helper.sh scan-merged (cross-PR file dedup, t1411).

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!