-
Notifications
You must be signed in to change notification settings - Fork 6
fix(portability): replace sed -i with portable sed_inplace wrapper (t145) #479
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,6 +4,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Comprehensive script to fix remaining SonarCloud issues | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Targets: S7679 (positional parameters), S7682 (return statements), S1192 (string literals) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Source shared constants (provides sed_inplace and other utilities) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+7
to
+9
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent failure if The Proposed fix # Source shared constants (provides sed_inplace and other utilities)
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit
-source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true
+source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || {
+ echo "⚠️ shared-constants.sh not found; sed_inplace/sed_append_after unavailable" >&2
+ exit 1
+}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cd providers || exit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "🚀 Starting comprehensive quality fix..." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -14,7 +18,7 @@ fix_misplaced_returns() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "Fixing misplaced returns in $file..." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Remove return statements that are not at the end of functions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '/^ return 0$/d' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_inplace '/^ return 0$/d' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Add return statements to function endings that need them | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # This is a more targeted approach based on SonarCloud line numbers | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -29,20 +33,20 @@ replace_string_literals() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "$file" in | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "mainwp-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Replace remaining "Site ID is required" occurrences | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i 's/"Site ID is required"/"$ERROR_SITE_ID_REQUIRED"/g' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i 's/"At least one site ID is required"/"$ERROR_AT_LEAST_ONE_SITE_ID"/g' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_inplace 's/"Site ID is required"/"$ERROR_SITE_ID_REQUIRED"/g' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_inplace 's/"At least one site ID is required"/"$ERROR_AT_LEAST_ONE_SITE_ID"/g' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "code-audit-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Already done in previous commit | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "dns-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Replace remaining cloudflare occurrences in case statements | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i "s/\"namecheap\"/\"\$PROVIDER_NAMECHEAP\"/g" "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i "s/\"route53\"/\"\$PROVIDER_ROUTE53\"/g" "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_inplace "s/\"namecheap\"/\"\$PROVIDER_NAMECHEAP\"/g" "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_inplace "s/\"route53\"/\"\$PROVIDER_ROUTE53\"/g" "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "git-platforms-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Replace remaining platform occurrences | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i "s/\"gitea\"/\"\$PLATFORM_GITEA\"/g" "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_inplace "s/\"gitea\"/\"\$PLATFORM_GITEA\"/g" "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| *) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| echo "No string literal replacements needed for $file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -61,22 +65,22 @@ add_missing_returns() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| case "$file" in | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "closte-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Lines 134, 249 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '133a\ return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '248a\ return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_append_after 133 ' return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_append_after 248 ' return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "cloudron-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Lines 74, 202 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '73a\ return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '201a\ return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_append_after 73 ' return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_append_after 201 ' return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "coolify-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Line 236 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '235a\ return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_append_after 235 ' return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "dns-helper.sh") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Lines 95, 259 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '94a\ return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed -i '258a\ return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_append_after 94 ' return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sed_append_after 258 ' return 0' "$file" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ;; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
66
to
84
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sequential Each For example in
Fix by either processing insertions bottom-up (highest line number first) or incrementing subsequent targets: Proposed fix (bottom-up ordering) "closte-helper.sh")
# Lines 134, 249
- sed_append_after 133 ' return 0' "$file"
- sed_append_after 248 ' return 0' "$file"
+ sed_append_after 248 ' return 0' "$file"
+ sed_append_after 133 ' return 0' "$file"
;;
"cloudron-helper.sh")
# Lines 74, 202
- sed_append_after 73 ' return 0' "$file"
- sed_append_after 201 ' return 0' "$file"
+ sed_append_after 201 ' return 0' "$file"
+ sed_append_after 73 ' return 0' "$file"
;;
...
"dns-helper.sh")
# Lines 95, 259
- sed_append_after 94 ' return 0' "$file"
- sed_append_after 258 ' return 0' "$file"
+ sed_append_after 258 ' return 0' "$file"
+ sed_append_after 94 ' return 0' "$file"
;;By inserting at the later line first, earlier line numbers remain stable. 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| *) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -30,6 +30,7 @@ readonly NC='\033[0m' # No Color | |||||||||||
|
|
||||||||||||
| # Configuration | ||||||||||||
| readonly SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit | ||||||||||||
| source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent fallback when Line 375 calls Consider either removing Proposed guard source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true
+if ! command -v sed_inplace &>/dev/null; then
+ sed_inplace() { if [[ "$(uname)" == "Darwin" ]]; then sed -i '' "$@"; else sed -i "$@"; fi; }
+fiAs per coding guidelines, 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| readonly AIDEVOPS_DIR="${HOME}/.aidevops" | ||||||||||||
| readonly CONFIG_DIR="${AIDEVOPS_DIR}/config" | ||||||||||||
| readonly PATTERNS_FILE="${CONFIG_DIR}/privacy-patterns.txt" | ||||||||||||
|
|
@@ -116,8 +117,6 @@ print_error() { | |||||||||||
| return 0 | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| # Cross-platform sed in-place edit (macOS vs GNU/Linux) | ||||||||||||
| sed_inplace() { if [[ "$(uname)" == "Darwin" ]]; then sed -i '' "$@"; else sed -i "$@"; fi; } | ||||||||||||
|
|
||||||||||||
| print_header() { | ||||||||||||
| local message="$1" | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ unset _p | |
| # Configuration - resolve relative to this script's location | ||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit | ||
| readonly SCRIPT_DIR | ||
| source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Silent failure when sourcing a now-required dependency.
Consider at minimum logging a warning if the source fails, so debugging doesn't require tracing back from a sed wrapper that doesn't exist: Suggested improvement-source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true
+source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || log_warn "shared-constants.sh not found; sed_inplace/sed_append_after unavailable"Note: As per coding guidelines, automation scripts should focus on "Clear logging and feedback" and "Error recovery mechanisms". 🤖 Prompt for AI Agents |
||
| readonly SUPERVISOR_DIR="${AIDEVOPS_SUPERVISOR_DIR:-$HOME/.aidevops/.agent-workspace/supervisor}" | ||
| readonly SUPERVISOR_DB="$SUPERVISOR_DIR/supervisor.db" | ||
| readonly MAIL_HELPER="${SCRIPT_DIR}/mail-helper.sh" # Used by pulse command (t128.2) | ||
|
|
@@ -160,8 +161,6 @@ log_cmd() { | |
| return $rc | ||
| } | ||
|
|
||
| # Cross-platform sed in-place edit (macOS vs GNU/Linux) | ||
| sed_inplace() { if [[ "$(uname)" == "Darwin" ]]; then sed -i '' "$@"; else sed -i "$@"; fi; } | ||
|
|
||
| ####################################### | ||
| # Get the number of CPU cores on this system | ||
|
|
@@ -4394,13 +4393,7 @@ update_todo_on_blocked() { | |
| else | ||
| # Insert a new Notes line after the task | ||
| local notes_line="${indent} - Notes: BLOCKED by supervisor: ${safe_reason}" | ||
| # sed append syntax differs between BSD and GNU - sed_inplace can't abstract this | ||
| if [[ "$(uname)" == "Darwin" ]]; then | ||
| sed -i '' "${line_num}a\\ | ||
| ${notes_line}" "$todo_file" | ||
| else | ||
| sed -i "${line_num}a\\${notes_line}" "$todo_file" | ||
| fi | ||
| sed_append_after "$line_num" "$notes_line" "$todo_file" | ||
| fi | ||
|
|
||
| log_success "Updated TODO.md: $task_id marked blocked ($reason)" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soft-sourcing shared-constants.sh but hard-depending on
sed_inplaceat line 869.The
2>/dev/null || truepattern means sourcing silently succeeds even if the file is missing. However,cmd_create()at line 869 callssed_inplace, which will fail with an opaque "command not found" error underset -euo pipefail. This applies to several other scripts in this PR that follow the same pattern (quality-fix.sh, auto-version-bump.sh, coderabbit-cli.sh, readme-helper.sh, security-helper.sh).Consider adding a guard after sourcing:
Proposed fix
Alternatively, keep the source mandatory (remove
|| true) for scripts that cannot function without the shared utilities. As per coding guidelines, automation scripts should focus on reliability, robustness, and clear logging/feedback.🤖 Prompt for AI Agents