t311.5: Update tooling for module structure#1209
Merged
marcusquinn merged 3 commits intomainfrom Feb 12, 2026
Merged
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Contributor
WalkthroughThe pull request refactors shell script collection and linting workflows to support nested modular scripts in Changes
Sequence Diagram(s)No sequence diagrams are applicable. The changes are primarily refactoring and structural improvements focused on file discovery patterns and script organization, without introducing new component interactions or significant control flow modifications. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
marcusquinn
force-pushed
the
feature/t311.5
branch
from
49aac6f to
ea20dc7
Compare
🔍 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 12 03:07:47 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
…311.5) Modularised scripts (memory/, supervisor-modules/) need executable permissions set during both local set_permissions() and remote deploy_aidevops_agents(). Uses find -mindepth 2 to handle any current and future subdirectory modules.
…s (t311.5) Add collect_shell_files() helper that uses find to gather all .sh files including subdirectories (memory/, supervisor-modules/) while excluding _archive/. All check functions now iterate ALL_SH_FILES instead of globbing .agents/scripts/*.sh. Also added -x flag to shellcheck for cross-file source resolution in modular scripts.
…re (t311.5) quality-fix.sh: Replace for-glob loops with find-based iteration to include modularised subdirectory scripts (memory/, supervisor-modules/). Add -x flag to shellcheck for cross-file source resolution. test-smoke-help.sh: Extend git ls-files patterns to include subdirectory scripts. Add module files (_common.sh, store.sh, recall.sh, etc.) to SKIP_HELP list since they are sourced, not standalone.
marcusquinn
force-pushed
the
feature/t311.5
branch
from
ea20dc7 to
4b712b4
Compare
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
🔍 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 12 03:17:19 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
marcusquinn
added a commit
that referenced
this pull request
Feb 12, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/scripts/quality-fix.sh (1)
177-213:⚠️ Potential issue | 🟠 Majorawk uses undefined
$_arg1(never emits results)
$_arg1isn’t an awk field; this prevents any constants from being reported.🛠️ Suggested fix
- (grep -o '"[^"]*"' "$file" || true) | sort | uniq -c | sort -nr | awk '$_arg1 >= 3 { + (grep -o '"[^"]*"' "$file" || true) | sort | uniq -c | sort -nr | awk '$1 >= 3 { ... - printf "readonly %s=\"%s\" # Used %d times\n", constant_name, $2, $_arg1 + printf "readonly %s=\"%s\" # Used %d times\n", constant_name, $2, $1
🤖 Fix all issues with AI agents
In @.agents/scripts/linters-local.sh:
- Around line 85-136: check_return_statements currently misses patterns like
"return $?" and "exit $?" because the grep patterns for return_statements and
exit_statements only match literal "$" or numeric literals; update the two
assignments so they match returns/exits with numeric literals, variable-dollar
forms (e.g., $?, $VAR), and simple command substitutions. Specifically, in
check_return_statements replace the return_statements capture with a grep -cE
that matches "return [0-9]+|return \$[A-Za-z0-9_?]+|return \$\([^)]+\)"
(properly escaped in shell), and likewise replace exit_statements with a grep
-cE matching "^exit [0-9]+|^exit \$[A-Za-z0-9_?]+|^exit \$\([^)]+\)"; keep the
rest of the logic (functions_count, numeric coercion, totals, and threshold
check) unchanged.
In @.agents/scripts/quality-fix.sh:
- Around line 61-135: The temp-file rewrite in fix_return_statements can strip
executable bits because mv replaces the original with the temp file's mode;
capture the original file mode (e.g., via stat on "$file") before writing to
temp_file, then restore that mode on the moved file (or set the mode on
temp_file prior to mv) using chmod so executable permissions are preserved;
ensure changes reference the temp_file and "$file" variables and are applied
whenever mv "$temp_file" "$file" is executed.
- Around line 137-175: In fix_positional_parameters(), the sed replacements risk
variable-concat bugs by replacing $2/$3/$4/$_arg1 with unbraced names; update
the sed_inplace replacement to use braced variable references (${account_name},
${target}, ${options}, ${command}) so occurrences like $20 don’t become
${account_name}0 incorrectly, and ensure the sed patterns escape the braces
(e.g. replace '\$2' with '\${account_name}', '\$3' with '\${target}', '\$4' with
'\${options}', and '$_arg1' with '\${command}') when operating on temp_file
after inserting the local variables in main().
- Around line 48-59: The backup_files function uses GNU-only cp --parents which
fails on macOS/BSD; replace that usage with a portable loop: use find to locate
.sh files under .agents/scripts (excluding _archive), for each file create the
target directory with mkdir -p "$backup_dir/$(dirname "$rel_path")" and then cp
the file into that path so the directory structure is preserved; update the find
-exec to call a small sh -c snippet or a while read loop that computes the
relative path, makes the destination dirs, and copies the file, keeping the
original exclusion and redirecting errors to /dev/null as before.
In `@tests/test-smoke-help.sh`:
- Around line 181-184: The exit status is being masked by the trailing "||
true", so help_exit always becomes 0; remove the "|| true" and capture the
timeout command's exit code immediately after the command substitution (keep
help_output=$(timeout 5 bash "$abs_path" help 2>&1) and then set help_exit=$?),
ensuring the timeout/command exit is preserved rather than overwritten by the
forced true.
🧹 Nitpick comments (1)
.agents/scripts/linters-local.sh (1)
55-83: Add curl timeouts to avoid local lint hangsA short timeout prevents the local lint run from stalling on network blips.
🛠️ Suggested tweak
- if response=$(curl -s "https://sonarcloud.io/api/issues/search?componentKeys=marcusquinn_aidevops&impactSoftwareQualities=MAINTAINABILITY&resolved=false&ps=1"); then + if response=$(curl -s --max-time 10 "https://sonarcloud.io/api/issues/search?componentKeys=marcusquinn_aidevops&impactSoftwareQualities=MAINTAINABILITY&resolved=false&ps=1"); then ... - if breakdown_response=$(curl -s "https://sonarcloud.io/api/issues/search?componentKeys=marcusquinn_aidevops&impactSoftwareQualities=MAINTAINABILITY&resolved=false&ps=10&facets=rules"); then + if breakdown_response=$(curl -s --max-time 10 "https://sonarcloud.io/api/issues/search?componentKeys=marcusquinn_aidevops&impactSoftwareQualities=MAINTAINABILITY&resolved=false&ps=10&facets=rules"); then
Comment on lines
85
to
136
| check_return_statements() { | ||
| echo -e "${BLUE}Checking Return Statements (S7682)...${NC}" | ||
|
|
||
| local violations=0 | ||
| local files_checked=0 | ||
|
|
||
| for file in .agents/scripts/*.sh; do | ||
| if [[ -f "$file" ]]; then | ||
| ((files_checked++)) | ||
|
|
||
| # Count multi-line functions (exclude one-liners like: func() { echo "x"; }) | ||
| # One-liners don't need explicit return statements | ||
| local functions_count | ||
| functions_count=$(grep -c "^[a-zA-Z_][a-zA-Z0-9_]*() {$" "$file" 2>/dev/null || echo "0") | ||
|
|
||
| # Count all return patterns: return 0, return 1, return $var, return $((expr)) | ||
| local return_statements | ||
| return_statements=$(grep -cE "return [0-9]+|return \\\$" "$file" 2>/dev/null || echo "0") | ||
|
|
||
| # Also count exit statements at script level (exit 0, exit $?) | ||
| local exit_statements | ||
| exit_statements=$(grep -cE "^exit [0-9]+|^exit \\\$" "$file" 2>/dev/null || echo "0") | ||
|
|
||
| # Ensure variables are numeric | ||
| functions_count=${functions_count//[^0-9]/} | ||
| return_statements=${return_statements//[^0-9]/} | ||
| exit_statements=${exit_statements//[^0-9]/} | ||
| functions_count=${functions_count:-0} | ||
| return_statements=${return_statements:-0} | ||
| exit_statements=${exit_statements:-0} | ||
|
|
||
| # Total returns = return statements + exit statements (for main) | ||
| local total_returns=$((return_statements + exit_statements)) | ||
|
|
||
| if [[ $total_returns -lt $functions_count ]]; then | ||
| ((violations++)) | ||
| print_warning "Missing return statements in $file" | ||
| fi | ||
| fi | ||
| done | ||
|
|
||
| echo "Files checked: $files_checked" | ||
| echo "Files with violations: $violations" | ||
|
|
||
| if [[ $violations -le $MAX_RETURN_ISSUES ]]; then | ||
| print_success "Return statements: $violations violations (within threshold)" | ||
| else | ||
| print_error "Return statements: $violations violations (exceeds threshold of $MAX_RETURN_ISSUES)" | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 | ||
| echo -e "${BLUE}Checking Return Statements (S7682)...${NC}" | ||
|
|
||
| local violations=0 | ||
| local files_checked=0 | ||
|
|
||
| for file in "${ALL_SH_FILES[@]}"; do | ||
| [[ -f "$file" ]] || continue | ||
| ((files_checked++)) | ||
|
|
||
| # Count multi-line functions (exclude one-liners like: func() { echo "x"; }) | ||
| # One-liners don't need explicit return statements | ||
| local functions_count | ||
| functions_count=$(grep -c "^[a-zA-Z_][a-zA-Z0-9_]*() {$" "$file" 2>/dev/null || echo "0") | ||
|
|
||
| # Count all return patterns: return 0, return 1, return $var, return $((expr)) | ||
| local return_statements | ||
| return_statements=$(grep -cE "return [0-9]+|return \\\$" "$file" 2>/dev/null || echo "0") | ||
|
|
||
| # Also count exit statements at script level (exit 0, exit $?) | ||
| local exit_statements | ||
| exit_statements=$(grep -cE "^exit [0-9]+|^exit \\\$" "$file" 2>/dev/null || echo "0") | ||
|
|
||
| # Ensure variables are numeric | ||
| functions_count=${functions_count//[^0-9]/} | ||
| return_statements=${return_statements//[^0-9]/} | ||
| exit_statements=${exit_statements//[^0-9]/} | ||
| functions_count=${functions_count:-0} | ||
| return_statements=${return_statements:-0} | ||
| exit_statements=${exit_statements:-0} | ||
|
|
||
| # Total returns = return statements + exit statements (for main) | ||
| local total_returns=$((return_statements + exit_statements)) | ||
|
|
||
| if [[ $total_returns -lt $functions_count ]]; then | ||
| ((violations++)) | ||
| print_warning "Missing return statements in $file" | ||
| fi | ||
| done | ||
|
|
||
| echo "Files checked: $files_checked" | ||
| echo "Files with violations: $violations" | ||
|
|
||
| if [[ $violations -le $MAX_RETURN_ISSUES ]]; then | ||
| print_success "Return statements: $violations violations (within threshold)" | ||
| else | ||
| print_error "Return statements: $violations violations (exceeds threshold of $MAX_RETURN_ISSUES)" | ||
| return 1 | ||
| fi | ||
|
|
||
| return 0 | ||
| } |
Contributor
There was a problem hiding this comment.
Return detection misses return $? / exit $?
The regex only counts return $ or exit $ literals, so valid patterns like return $? are missed and inflate violations.
🛠️ Suggested fix
- return_statements=$(grep -cE "return [0-9]+|return \\\$" "$file" 2>/dev/null || echo "0")
+ return_statements=$(grep -cE 'return ([0-9]+|\$[^[:space:]]+)' "$file" 2>/dev/null || echo "0")
...
- exit_statements=$(grep -cE "^exit [0-9]+|^exit \\\$" "$file" 2>/dev/null || echo "0")
+ exit_statements=$(grep -cE '^exit ([0-9]+|\$[^[:space:]]+)' "$file" 2>/dev/null || echo "0")📝 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.
Suggested change
| check_return_statements() { | |
| echo -e "${BLUE}Checking Return Statements (S7682)...${NC}" | |
| local violations=0 | |
| local files_checked=0 | |
| for file in .agents/scripts/*.sh; do | |
| if [[ -f "$file" ]]; then | |
| ((files_checked++)) | |
| # Count multi-line functions (exclude one-liners like: func() { echo "x"; }) | |
| # One-liners don't need explicit return statements | |
| local functions_count | |
| functions_count=$(grep -c "^[a-zA-Z_][a-zA-Z0-9_]*() {$" "$file" 2>/dev/null || echo "0") | |
| # Count all return patterns: return 0, return 1, return $var, return $((expr)) | |
| local return_statements | |
| return_statements=$(grep -cE "return [0-9]+|return \\\$" "$file" 2>/dev/null || echo "0") | |
| # Also count exit statements at script level (exit 0, exit $?) | |
| local exit_statements | |
| exit_statements=$(grep -cE "^exit [0-9]+|^exit \\\$" "$file" 2>/dev/null || echo "0") | |
| # Ensure variables are numeric | |
| functions_count=${functions_count//[^0-9]/} | |
| return_statements=${return_statements//[^0-9]/} | |
| exit_statements=${exit_statements//[^0-9]/} | |
| functions_count=${functions_count:-0} | |
| return_statements=${return_statements:-0} | |
| exit_statements=${exit_statements:-0} | |
| # Total returns = return statements + exit statements (for main) | |
| local total_returns=$((return_statements + exit_statements)) | |
| if [[ $total_returns -lt $functions_count ]]; then | |
| ((violations++)) | |
| print_warning "Missing return statements in $file" | |
| fi | |
| fi | |
| done | |
| echo "Files checked: $files_checked" | |
| echo "Files with violations: $violations" | |
| if [[ $violations -le $MAX_RETURN_ISSUES ]]; then | |
| print_success "Return statements: $violations violations (within threshold)" | |
| else | |
| print_error "Return statements: $violations violations (exceeds threshold of $MAX_RETURN_ISSUES)" | |
| return 1 | |
| fi | |
| return 0 | |
| echo -e "${BLUE}Checking Return Statements (S7682)...${NC}" | |
| local violations=0 | |
| local files_checked=0 | |
| for file in "${ALL_SH_FILES[@]}"; do | |
| [[ -f "$file" ]] || continue | |
| ((files_checked++)) | |
| # Count multi-line functions (exclude one-liners like: func() { echo "x"; }) | |
| # One-liners don't need explicit return statements | |
| local functions_count | |
| functions_count=$(grep -c "^[a-zA-Z_][a-zA-Z0-9_]*() {$" "$file" 2>/dev/null || echo "0") | |
| # Count all return patterns: return 0, return 1, return $var, return $((expr)) | |
| local return_statements | |
| return_statements=$(grep -cE "return [0-9]+|return \\\$" "$file" 2>/dev/null || echo "0") | |
| # Also count exit statements at script level (exit 0, exit $?) | |
| local exit_statements | |
| exit_statements=$(grep -cE "^exit [0-9]+|^exit \\\$" "$file" 2>/dev/null || echo "0") | |
| # Ensure variables are numeric | |
| functions_count=${functions_count//[^0-9]/} | |
| return_statements=${return_statements//[^0-9]/} | |
| exit_statements=${exit_statements//[^0-9]/} | |
| functions_count=${functions_count:-0} | |
| return_statements=${return_statements:-0} | |
| exit_statements=${exit_statements:-0} | |
| # Total returns = return statements + exit statements (for main) | |
| local total_returns=$((return_statements + exit_statements)) | |
| if [[ $total_returns -lt $functions_count ]]; then | |
| ((violations++)) | |
| print_warning "Missing return statements in $file" | |
| fi | |
| done | |
| echo "Files checked: $files_checked" | |
| echo "Files with violations: $violations" | |
| if [[ $violations -le $MAX_RETURN_ISSUES ]]; then | |
| print_success "Return statements: $violations violations (within threshold)" | |
| else | |
| print_error "Return statements: $violations violations (exceeds threshold of $MAX_RETURN_ISSUES)" | |
| return 1 | |
| fi | |
| return 0 | |
| } | |
| check_return_statements() { | |
| echo -e "${BLUE}Checking Return Statements (S7682)...${NC}" | |
| local violations=0 | |
| local files_checked=0 | |
| for file in "${ALL_SH_FILES[@]}"; do | |
| [[ -f "$file" ]] || continue | |
| ((files_checked++)) | |
| # Count multi-line functions (exclude one-liners like: func() { echo "x"; }) | |
| # One-liners don't need explicit return statements | |
| local functions_count | |
| functions_count=$(grep -c "^[a-zA-Z_][a-zA-Z0-9_]*() {$" "$file" 2>/dev/null || echo "0") | |
| # Count all return patterns: return 0, return 1, return $var, return $((expr)) | |
| local return_statements | |
| return_statements=$(grep -cE 'return ([0-9]+|\$[^[:space:]]+)' "$file" 2>/dev/null || echo "0") | |
| # Also count exit statements at script level (exit 0, exit $?) | |
| local exit_statements | |
| exit_statements=$(grep -cE '^exit ([0-9]+|\$[^[:space:]]+)' "$file" 2>/dev/null || echo "0") | |
| # Ensure variables are numeric | |
| functions_count=${functions_count//[^0-9]/} | |
| return_statements=${return_statements//[^0-9]/} | |
| exit_statements=${exit_statements//[^0-9]/} | |
| functions_count=${functions_count:-0} | |
| return_statements=${return_statements:-0} | |
| exit_statements=${exit_statements:-0} | |
| # Total returns = return statements + exit statements (for main) | |
| local total_returns=$((return_statements + exit_statements)) | |
| if [[ $total_returns -lt $functions_count ]]; then | |
| ((violations++)) | |
| print_warning "Missing return statements in $file" | |
| fi | |
| done | |
| echo "Files checked: $files_checked" | |
| echo "Files with violations: $violations" | |
| if [[ $violations -le $MAX_RETURN_ISSUES ]]; then | |
| print_success "Return statements: $violations violations (within threshold)" | |
| else | |
| print_error "Return statements: $violations violations (exceeds threshold of $MAX_RETURN_ISSUES)" | |
| return 1 | |
| fi | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
In @.agents/scripts/linters-local.sh around lines 85 - 136,
check_return_statements currently misses patterns like "return $?" and "exit $?"
because the grep patterns for return_statements and exit_statements only match
literal "$" or numeric literals; update the two assignments so they match
returns/exits with numeric literals, variable-dollar forms (e.g., $?, $VAR), and
simple command substitutions. Specifically, in check_return_statements replace
the return_statements capture with a grep -cE that matches "return [0-9]+|return
\$[A-Za-z0-9_?]+|return \$\([^)]+\)" (properly escaped in shell), and likewise
replace exit_statements with a grep -cE matching "^exit [0-9]+|^exit
\$[A-Za-z0-9_?]+|^exit \$\([^)]+\)"; keep the rest of the logic
(functions_count, numeric coercion, totals, and threshold check) unchanged.
Comment on lines
48
to
59
| backup_files() { | ||
| print_info "Creating backup of provider files..." | ||
| print_info "Creating backup of provider files..." | ||
|
|
||
| local backup_dir="backups/$(date +%Y%m%d_%H%M%S)" | ||
| mkdir -p "$backup_dir" | ||
| local backup_dir="backups/$(date +%Y%m%d_%H%M%S)" | ||
| mkdir -p "$backup_dir" | ||
|
|
||
| cp .agents/scripts/*.sh "$backup_dir/" | ||
| print_success "Backup created in $backup_dir" | ||
| return 0 | ||
| cp .agents/scripts/*.sh "$backup_dir/" | ||
| # Also backup modularised subdirectory scripts | ||
| find .agents/scripts -mindepth 2 -name "*.sh" -not -path "*/_archive/*" -exec cp --parents {} "$backup_dir/" \; 2>/dev/null || true | ||
| print_success "Backup created in $backup_dir" | ||
| return 0 | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and read the specific lines
cat -n .agents/scripts/quality-fix.sh | sed -n '48,59p'Repository: marcusquinn/aidevops
Length of output: 572
🏁 Script executed:
# Also get more context around those lines
cat -n .agents/scripts/quality-fix.sh | sed -n '40,65p'Repository: marcusquinn/aidevops
Length of output: 874
🏁 Script executed:
# Check overall structure and find function definition
grep -n "backup_files()" .agents/scripts/quality-fix.shRepository: marcusquinn/aidevops
Length of output: 83
🏁 Script executed:
# Run ShellCheck on the script
shellcheck .agents/scripts/quality-fix.sh 2>&1 || trueRepository: marcusquinn/aidevops
Length of output: 46
🌐 Web query:
cp --parents flag BSD macOS compatibility portable
💡 Result:
cp --parents is a GNU coreutils extension (it recreates the source path under the destination directory) and is not POSIX, so the default BSD cp on macOS doesn’t support it. GNU documents --parents like this: it copies a/b/c to DEST/a/b/c, creating missing intermediate dirs. [1]
Portable replacements (work on macOS + Linux)
1) Use rsync --relative (-R) (closest drop-in)
rsync -aR path/to/file1 other/dir/file2 DESTDIR/
# or without -a if you only need recursion/relative paths:
rsync -rR path/to/file1 DESTDIR/-R/--relative tells rsync to preserve the relative path components from the command line. [2]
2) Pure shell + POSIX tools (mkdir -p + cp)
dest=DESTDIR
for src in "$@"; do
mkdir -p "$dest/$(dirname "$src")"
cp -p "$src" "$dest/$src"
done(mkdir -p is standard. [3])
If you specifically want cp --parents on macOS
Install GNU coreutils and use gcp (Homebrew prefixes conflicting tools with g). [4]
brew install coreutils
gcp --parents a/b/c DESTDIR/Sources: [1] [2] [3] [4]
Replace cp --parents with portable POSIX approach for macOS/BSD compatibility
Line 56: cp --parents is a GNU-only extension unsupported on BSD cp (default on macOS), causing backup operations to fail silently or error. This breaks cross-platform reliability in the DevOps automation.
Use the portable shell loop pattern:
🛠️ Portable backup implementation
- find .agents/scripts -mindepth 2 -name "*.sh" -not -path "*/_archive/*" -exec cp --parents {} "$backup_dir/" \; 2>/dev/null || true
+ while IFS= read -r -d '' f; do
+ dest="$backup_dir/$(dirname "$f")"
+ mkdir -p "$dest"
+ cp "$f" "$dest/"
+ done < <(find .agents/scripts -mindepth 2 -name "*.sh" -not -path "*/_archive/*" -print0 2>/dev/null)📝 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.
Suggested change
| backup_files() { | |
| print_info "Creating backup of provider files..." | |
| print_info "Creating backup of provider files..." | |
| local backup_dir="backups/$(date +%Y%m%d_%H%M%S)" | |
| mkdir -p "$backup_dir" | |
| local backup_dir="backups/$(date +%Y%m%d_%H%M%S)" | |
| mkdir -p "$backup_dir" | |
| cp .agents/scripts/*.sh "$backup_dir/" | |
| print_success "Backup created in $backup_dir" | |
| return 0 | |
| cp .agents/scripts/*.sh "$backup_dir/" | |
| # Also backup modularised subdirectory scripts | |
| find .agents/scripts -mindepth 2 -name "*.sh" -not -path "*/_archive/*" -exec cp --parents {} "$backup_dir/" \; 2>/dev/null || true | |
| print_success "Backup created in $backup_dir" | |
| return 0 | |
| } | |
| backup_files() { | |
| print_info "Creating backup of provider files..." | |
| local backup_dir="backups/$(date +%Y%m%d_%H%M%S)" | |
| mkdir -p "$backup_dir" | |
| cp .agents/scripts/*.sh "$backup_dir/" | |
| # Also backup modularised subdirectory scripts | |
| while IFS= read -r -d '' f; do | |
| dest="$backup_dir/$(dirname "$f")" | |
| mkdir -p "$dest" | |
| cp "$f" "$dest/" | |
| done < <(find .agents/scripts -mindepth 2 -name "*.sh" -not -path "*/_archive/*" -print0 2>/dev/null) | |
| print_success "Backup created in $backup_dir" | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
In @.agents/scripts/quality-fix.sh around lines 48 - 59, The backup_files
function uses GNU-only cp --parents which fails on macOS/BSD; replace that usage
with a portable loop: use find to locate .sh files under .agents/scripts
(excluding _archive), for each file create the target directory with mkdir -p
"$backup_dir/$(dirname "$rel_path")" and then cp the file into that path so the
directory structure is preserved; update the find -exec to call a small sh -c
snippet or a while read loop that computes the relative path, makes the
destination dirs, and copies the file, keeping the original exclusion and
redirecting errors to /dev/null as before.
Comment on lines
61
to
135
| fix_return_statements() { | ||
| print_info "Fixing missing return statements (S7682)..." | ||
| local files_fixed=0 | ||
| for file in .agents/scripts/*.sh; do | ||
| if [[ -f "$file" ]]; then | ||
| # Find functions that don't end with return statement | ||
| local temp_file | ||
| temp_file=$(mktemp) | ||
| local in_function=false | ||
| local function_name="" | ||
| local brace_count=0 | ||
| local fixed_functions=0 | ||
| while IFS= read -r line; do | ||
| echo "$line" >> "$temp_file" | ||
| # Detect function start | ||
| if [[ $line =~ ^[a-zA-Z_][a-zA-Z0-9_]*\(\)[[:space:]]*\{ ]]; then | ||
| in_function=true | ||
| function_name=$(echo "$line" | sed 's/().*//') | ||
| brace_count=1 | ||
| elif [[ $in_function == true ]]; then | ||
| # Count braces to track function scope | ||
| local open_braces | ||
| open_braces=$(echo "$line" | grep -o '{' | wc -l 2>/dev/null || echo "0") | ||
| local close_braces | ||
| close_braces=$(echo "$line" | grep -o '}' | wc -l 2>/dev/null || echo "0") | ||
|
|
||
| # Ensure variables are numeric | ||
| open_braces=${open_braces//[^0-9]/} | ||
| close_braces=${close_braces//[^0-9]/} | ||
| open_braces=${open_braces:-0} | ||
| close_braces=${close_braces:-0} | ||
|
|
||
| # Fix arithmetic expansion | ||
| local diff | ||
| diff=$((open_braces - close_braces)) | ||
| brace_count=$((brace_count + diff)) | ||
| # Check if function is ending | ||
| if [[ $brace_count -eq 0 && $line == "}" ]]; then | ||
| # Check if previous line has return statement | ||
| local last_line | ||
| last_line=$(tail -2 "$temp_file" | head -1) | ||
| if [[ ! $last_line =~ return[[:space:]]+[01] ]]; then | ||
| # Remove the closing brace and add return statement | ||
| sed_inplace '$ d' "$temp_file" | ||
| echo " return 0" >> "$temp_file" | ||
| echo "}" >> "$temp_file" | ||
| ((fixed_functions++)) | ||
| print_info "Fixed function: $function_name" | ||
| fi | ||
| in_function=false | ||
| function_name="" | ||
| fi | ||
| fi | ||
| done < "$file" | ||
| if [[ $fixed_functions -gt 0 ]]; then | ||
| mv "$temp_file" "$file" | ||
| ((files_fixed++)) | ||
| print_success "Fixed $fixed_functions functions in $file" | ||
| else | ||
| rm -f "$temp_file" | ||
| fi | ||
| fi | ||
| done | ||
| print_success "Return statements: Fixed $files_fixed files" | ||
| return 0 | ||
| print_info "Fixing missing return statements (S7682)..." | ||
|
|
||
| local files_fixed=0 | ||
|
|
||
| while IFS= read -r -d '' file; do | ||
| if [[ -f "$file" ]]; then | ||
| # Find functions that don't end with return statement | ||
| local temp_file | ||
| temp_file=$(mktemp) | ||
| local in_function=false | ||
| local function_name="" | ||
| local brace_count=0 | ||
| local fixed_functions=0 | ||
|
|
||
| while IFS= read -r line; do | ||
| echo "$line" >>"$temp_file" | ||
|
|
||
| # Detect function start | ||
| if [[ $line =~ ^[a-zA-Z_][a-zA-Z0-9_]*\(\)[[:space:]]*\{ ]]; then | ||
| in_function=true | ||
| function_name=$(echo "$line" | sed 's/().*//') | ||
| brace_count=1 | ||
| elif [[ $in_function == true ]]; then | ||
| # Count braces to track function scope | ||
| local open_braces | ||
| open_braces=$(echo "$line" | grep -o '{' | wc -l 2>/dev/null || echo "0") | ||
| local close_braces | ||
| close_braces=$(echo "$line" | grep -o '}' | wc -l 2>/dev/null || echo "0") | ||
|
|
||
| # Ensure variables are numeric | ||
| open_braces=${open_braces//[^0-9]/} | ||
| close_braces=${close_braces//[^0-9]/} | ||
| open_braces=${open_braces:-0} | ||
| close_braces=${close_braces:-0} | ||
|
|
||
| # Fix arithmetic expansion | ||
| local diff | ||
| diff=$((open_braces - close_braces)) | ||
| brace_count=$((brace_count + diff)) | ||
|
|
||
| # Check if function is ending | ||
| if [[ $brace_count -eq 0 && $line == "}" ]]; then | ||
| # Check if previous line has return statement | ||
| local last_line | ||
| last_line=$(tail -2 "$temp_file" | head -1) | ||
|
|
||
| if [[ ! $last_line =~ return[[:space:]]+[01] ]]; then | ||
| # Remove the closing brace and add return statement | ||
| sed_inplace '$ d' "$temp_file" | ||
| echo " return 0" >>"$temp_file" | ||
| echo "}" >>"$temp_file" | ||
| ((fixed_functions++)) | ||
| print_info "Fixed function: $function_name" | ||
| fi | ||
|
|
||
| in_function=false | ||
| function_name="" | ||
| fi | ||
| fi | ||
| done <"$file" | ||
|
|
||
| if [[ $fixed_functions -gt 0 ]]; then | ||
| mv "$temp_file" "$file" | ||
| ((files_fixed++)) | ||
| print_success "Fixed $fixed_functions functions in $file" | ||
| else | ||
| rm -f "$temp_file" | ||
| fi | ||
| fi | ||
| done < <(find .agents/scripts -name "*.sh" -not -path "*/_archive/*" -print0 2>/dev/null) | ||
|
|
||
| print_success "Return statements: Fixed $files_fixed files" | ||
| return 0 | ||
| } |
Contributor
There was a problem hiding this comment.
Temp-file rewrite can drop executable permissions
When a file is replaced via mv, it inherits the temp file’s mode, which can strip +x and break scripts.
🛠️ Preserve original mode
- temp_file=$(mktemp)
+ temp_file=$(mktemp)
+ local file_mode
+ file_mode=$(stat -c %a "$file" 2>/dev/null || stat -f %Lp "$file")
...
- mv "$temp_file" "$file"
+ mv "$temp_file" "$file"
+ [[ -n "$file_mode" ]] && chmod "$file_mode" "$file"🤖 Prompt for AI Agents
In @.agents/scripts/quality-fix.sh around lines 61 - 135, The temp-file rewrite
in fix_return_statements can strip executable bits because mv replaces the
original with the temp file's mode; capture the original file mode (e.g., via
stat on "$file") before writing to temp_file, then restore that mode on the
moved file (or set the mode on temp_file prior to mv) using chmod so executable
permissions are preserved; ensure changes reference the temp_file and "$file"
variables and are applied whenever mv "$temp_file" "$file" is executed.
Comment on lines
137
to
175
| fix_positional_parameters() { | ||
| print_info "Fixing positional parameter violations (S7679)..." | ||
|
|
||
| local files_fixed=0 | ||
|
|
||
| for file in .agents/scripts/*.sh; do | ||
| if [[ -f "$file" ]]; then | ||
| local temp_file | ||
| temp_file=$(mktemp) | ||
|
|
||
|
|
||
| # Process main() functions specifically | ||
| if grep -q "^main() {" "$file"; then | ||
| # Add local variable assignments to main function | ||
| sed '/^main() {/,/^}$/ { | ||
| print_info "Fixing positional parameter violations (S7679)..." | ||
|
|
||
| local files_fixed=0 | ||
|
|
||
| while IFS= read -r -d '' file; do | ||
| if [[ -f "$file" ]]; then | ||
| local temp_file | ||
| temp_file=$(mktemp) | ||
|
|
||
| # Process main() functions specifically | ||
| if grep -q "^main() {" "$file"; then | ||
| # Add local variable assignments to main function | ||
| sed '/^main() {/,/^}$/ { | ||
| /^main() {/a\ | ||
| # Assign positional parameters to local variables\ | ||
| local command="${1:-help}"\ | ||
| local account_name="$2"\ | ||
| local target="$3"\ | ||
| local options="$4" | ||
| }' "$file" > "$temp_file" | ||
| # Replace direct positional parameter usage in case statements | ||
| sed_inplace 's/\$_arg1/$command/g; s/\$2/$account_name/g; s/\$3/$target/g; s/\$4/$options/g' "$temp_file" | ||
| if ! diff -q "$file" "$temp_file" > /dev/null; then | ||
| mv "$temp_file" "$file" | ||
| ((files_fixed++)) | ||
| print_success "Fixed positional parameters in main() function of $file" | ||
| else | ||
| rm -f "$temp_file" | ||
| fi | ||
| fi | ||
| fi | ||
| done | ||
| print_success "Positional parameters: Fixed $files_fixed files" | ||
| return 0 | ||
| }' "$file" >"$temp_file" | ||
|
|
||
| # Replace direct positional parameter usage in case statements | ||
| sed_inplace 's/\$_arg1/$command/g; s/\$2/$account_name/g; s/\$3/$target/g; s/\$4/$options/g' "$temp_file" | ||
|
|
||
| if ! diff -q "$file" "$temp_file" >/dev/null; then | ||
| mv "$temp_file" "$file" | ||
| ((files_fixed++)) | ||
| print_success "Fixed positional parameters in main() function of $file" | ||
| else | ||
| rm -f "$temp_file" | ||
| fi | ||
| fi | ||
| fi | ||
| done < <(find .agents/scripts -name "*.sh" -not -path "*/_archive/*" -print0 2>/dev/null) | ||
|
|
||
| print_success "Positional parameters: Fixed $files_fixed files" | ||
| return 0 | ||
| } |
Contributor
There was a problem hiding this comment.
Brace replacements to avoid variable-concat bugs
Replacing $2 with $account_name without braces can turn $20 into $account_name0 (wrong var). Use ${...} to stay safe.
🛠️ Suggested fix
- sed_inplace 's/\$_arg1/$command/g; s/\$2/$account_name/g; s/\$3/$target/g; s/\$4/$options/g' "$temp_file"
+ sed_inplace 's/\$_arg1/\${command}/g; s/\$2/\${account_name}/g; s/\$3/\${target}/g; s/\$4/\${options}/g' "$temp_file"🤖 Prompt for AI Agents
In @.agents/scripts/quality-fix.sh around lines 137 - 175, In
fix_positional_parameters(), the sed replacements risk variable-concat bugs by
replacing $2/$3/$4/$_arg1 with unbraced names; update the sed_inplace
replacement to use braced variable references (${account_name}, ${target},
${options}, ${command}) so occurrences like $20 don’t become ${account_name}0
incorrectly, and ensure the sed patterns escape the braces (e.g. replace '\$2'
with '\${account_name}', '\$3' with '\${target}', '\$4' with '\${options}', and
'$_arg1' with '\${command}') when operating on temp_file after inserting the
local variables in main().
Comment on lines
+181
to
+184
| # Run help command with timeout (5s max) and capture output | ||
| help_output=$(timeout 5 bash "$abs_path" help 2>&1) || true | ||
| help_exit=$? | ||
|
|
Contributor
There was a problem hiding this comment.
Preserve the timeout exit code for accurate diagnostics
Line 182-184: help_exit is always 0 because || true masks the timeout exit status, so timeouts won’t be classified correctly.
🛠️ Suggested fix
- help_output=$(timeout 5 bash "$abs_path" help 2>&1) || true
- help_exit=$?
+ if help_output=$(timeout 5 bash "$abs_path" help 2>&1); then
+ help_exit=0
+ else
+ help_exit=$?
+ 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.
Suggested change
| # Run help command with timeout (5s max) and capture output | |
| help_output=$(timeout 5 bash "$abs_path" help 2>&1) || true | |
| help_exit=$? | |
| # Run help command with timeout (5s max) and capture output | |
| if help_output=$(timeout 5 bash "$abs_path" help 2>&1); then | |
| help_exit=0 | |
| else | |
| help_exit=$? | |
| fi | |
🤖 Prompt for AI Agents
In `@tests/test-smoke-help.sh` around lines 181 - 184, The exit status is being
masked by the trailing "|| true", so help_exit always becomes 0; remove the "||
true" and capture the timeout command's exit code immediately after the command
substitution (keep help_output=$(timeout 5 bash "$abs_path" help 2>&1) and then
set help_exit=$?), ensuring the timeout/command exit is preserved rather than
overwritten by the forced true.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Update tooling scripts to handle the modularised script structure introduced by t311.3 (supervisor-modules/) and t311.4 (memory/).
Changes
setup.sh
set_permissions(): Addfind -mindepth 2to chmod +x scripts in module subdirectoriesdeploy_aidevops_agents(): Same fix for deployed agents at~/.aidevops/agents/scripts/linters-local.sh
collect_shell_files()helper usingfindto gather all.shfiles including subdirectories, excluding_archive/check_return_statements,check_positional_parameters,check_string_literals,run_shfmt,run_shellcheck) now iterateALL_SH_FILESinstead of globbingscripts/*.sh-xflag to shellcheck for cross-file source resolution in modular scriptsquality-fix.sh
for file in .agents/scripts/*.shloops withfind-based iteration-xflag to shellcheck validationtests/test-smoke-help.sh
git ls-filespatterns to includescripts/**/*.sh_common.sh,store.sh,recall.sh,maintenance.sh,verification.sh) toSKIP_HELPlistVerification
bash -nsyntax checkcollect_shell_files()correctly finds subdirectory modules and excludes_archive/Summary by CodeRabbit
Refactor
Tests