Skip to content

Conversation

@marcusquinn
Copy link
Owner

@marcusquinn marcusquinn commented Feb 7, 2026

Summary

  • Replace all non-portable sed -i usage across 17 active scripts with the shared sed_inplace() wrapper from shared-constants.sh
  • Add sed_append_after() to shared-constants.sh for portable line insertion (BSD sed a command has different syntax than GNU)
  • Eliminate 6 duplicate local sed_inplace definitions by sourcing shared-constants.sh instead

Problem

BSD sed (macOS) requires sed -i '' while GNU sed (Linux/CI) requires sed -i with no argument. Multiple scripts used one or the other, causing silent failures or errors on the other platform. Some scripts also used sed -i.tmp/sed -i.bak which left orphan backup files.

Changes by Category

Category Files Change
New shared utility shared-constants.sh Added sed_append_after() for portable line insertion
Removed duplicate definitions agent-test-helper.sh, privacy-filter-helper.sh, quality-fix.sh, security-helper.sh, supervisor-helper.sh, auto-version-bump.sh, version-manager.sh Replaced local sed_inplace() with source shared-constants.sh
Fixed GNU-only sed -i comprehensive-quality-fix.sh, mass-fix-returns.sh Replaced bare sed -i with sed_inplace / sed_append_after
Fixed sed -i.tmp/.bak coderabbit-cli.sh, readme-helper.sh, sonarcloud-autofix.sh, aidevops.sh, unstract-helper.sh, watercrawl-helper.sh Replaced with sed_inplace (no leftover temp files)
Fixed append syntax supervisor-helper.sh Replaced platform-specific sed -i 'Na\...' with sed_append_after
Platform-aware instruction webhosting-helper.sh User-facing sed instruction now shows correct syntax per platform
Standalone CLI aidevops.sh Added inline sed_inplace() (can't depend on .agents/ being available)

Not Changed

  • setup.sh: Already uses correct BSD-then-GNU fallback pattern (sed -i '' ... || sed -i ...)
  • _archive/ scripts: Not actively used, left as-is
  • Markdown docs: cloudron-app-packaging.md uses sed -i correctly (runs inside Linux containers)

Testing

  • Verified sed_inplace works on macOS with basic substitution and -E flag
  • Verified sed_append_after correctly inserts lines with proper newlines on macOS
  • ShellCheck passes with zero warnings on all 17 modified files

Closes #440

Summary by CodeRabbit

Release Notes

  • Refactor

    • Consolidated shell script utilities into a centralized shared library to improve code maintainability and consistency across automation scripts.
    • Simplified cross-platform file editing compatibility by centralizing utility functions rather than implementing them individually.
    • Removed duplicate helper implementations from multiple scripts.
  • Chores

    • Updated internal automation scripts to rely on shared utilities.

…145)

Replace all non-portable sed -i usage across 17 active scripts with the
shared sed_inplace() wrapper from shared-constants.sh. BSD sed requires
'sed -i ""' while GNU sed requires 'sed -i' (no argument), causing
silent failures on Linux/CI.

Changes:
- Add sed_append_after() to shared-constants.sh for portable line insertion
- Remove 6 duplicate local sed_inplace definitions, source shared-constants.sh
- Replace bare sed -i (GNU-only) in comprehensive-quality-fix.sh, mass-fix-returns.sh
- Replace sed -i.tmp/.bak with sed_inplace (eliminates leftover temp files)
- Add inline sed_inplace to aidevops.sh (standalone CLI, can't depend on .agents/)
- Fix supervisor-helper.sh append operations to use sed_append_after
- Make webhosting-helper.sh user instruction platform-aware
- Skip _archive/ scripts and setup.sh (already uses BSD-then-GNU fallback)

Closes #440
@gemini-code-assist
Copy link

Warning

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 7, 2026

Walkthrough

Centralizes cross-platform sed utilities by consolidating duplicated sed_inplace functions across multiple shell scripts into a shared utility file (shared-constants.sh). Scripts now source this file instead of maintaining individual implementations, introducing a new sed_append_after helper for portable line insertion. This resolves BSD/GNU sed compatibility issues flagged in issue #440.

Changes

Cohort / File(s) Summary
Shared Utility Centralization
.agents/scripts/agent-test-helper.sh, privacy-filter-helper.sh, quality-fix.sh, security-helper.sh, version-manager.sh
Removes in-file sed_inplace implementations and adds sourcing of shared-constants.sh for centralized cross-platform sed handling.
Script Updates with sed_inplace Replacement
.agents/scripts/auto-version-bump.sh, coderabbit-cli.sh, comprehensive-quality-fix.sh, mass-fix-returns.sh, readme-helper.sh, sonarcloud-autofix.sh, unstract-helper.sh, watercrawl-helper.sh
Sources shared-constants.sh and replaces direct sed -i or sed -i.tmp invocations with sed_inplace helper, eliminating temporary file cleanup logic.
New Helper Functions
.agents/scripts/shared-constants.sh
Adds sed_append_after function (defined twice with identical logic) to provide portable line-insertion capability for both macOS (BSD sed) and GNU/Linux systems.
Public API Expansion
.agents/scripts/supervisor-helper.sh
Introduces and exports MEMORY_HELPER variable alongside existing MAIL_HELPER; replaces sed_inplace usage with sed_append_after in update_todo_on_blocked for TODO.md mutations.
Primary Script Enhancement
aidevops.sh
Adds sed_inplace function and replaces three instances of sed -i.tmp with the new helper in uninstall, upgrade planning (TODO.md, PLANS.md, and .aidevops.json updates), removing explicit temporary file cleanup.
Platform-Specific sed Handling
.agents/scripts/webhosting-helper.sh
Adds conditional logic to differentiate macOS (sed -i '') from Linux (sed -i) for domain removal from /etc/hosts.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🔧 Sed wars end where shells align,
BSD and GNU now share one design,
From scattered helpers to constants divine,
Cross-platform wisdom in one shared line,
A-grade automation—the code shines ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly and specifically describes the main change: replacing non-portable sed -i with a portable sed_inplace wrapper for cross-platform compatibility.
Linked Issues check ✅ Passed The PR fully addresses issue #440 by implementing a portable sed_inplace() wrapper in shared-constants.sh, centralizing platform detection for BSD/GNU sed differences, adding sed_append_after() for portable line insertion, removing six duplicate local definitions, and updating all 17 affected scripts to use the shared helpers.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing sed portability across scripts as specified in issue #440; no unrelated modifications detected beyond the intended sed wrapper implementation and helper sourcing patterns.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/t145

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Feb 7, 2026

🔍 Code Quality Report

�[0;35m[MONITOR]�[0m Code Review Monitoring Report

�[0;34m[INFO]�[0m Latest Quality Status:
SonarCloud: 0 bugs, 0 vulnerabilities, 28 code smells

�[0;34m[INFO]�[0m Recent monitoring activity:
Sat Feb 7 20:32:38 UTC 2026: Code review monitoring started
Sat Feb 7 20:32:38 UTC 2026: SonarCloud - Bugs: 0, Vulnerabilities: 0, Code Smells: 28
Sat Feb 7 20:32:39 UTC 2026: Qlty - 0 issues found, auto-formatting applied
Sat Feb 7 20:32:41 UTC 2026: Codacy analysis completed with auto-fixes

📈 Current Quality Metrics

  • BUGS: 0
  • CODE SMELLS: 28
  • VULNERABILITIES: 0

Generated on: Sat Feb 7 20:32:41 UTC 2026


Generated by AI DevOps Framework Code Review Monitoring

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 7, 2026

@marcusquinn marcusquinn merged commit 486039e into main Feb 7, 2026
10 of 11 checks passed
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.agents/scripts/sonarcloud-autofix.sh (1)

136-136: ⚠️ Potential issue | 🔴 Critical

Bug: $_arg2 is likely undefined; should be $2.

$_arg2 is not a standard variable or parameter — this should be "$2" to match the positional argument from the main case dispatch. This is a pre-existing bug but will cause the fix command to always operate on an empty/wrong path.

Proposed fix
-            apply_sonarcloud_fixes "$_arg2"
+            apply_sonarcloud_fixes "$2"
🤖 Fix all issues with AI agents
In @.agents/scripts/agent-test-helper.sh:
- Line 53: The script soft-sources shared-constants.sh but later unconditionally
calls sed_inplace in cmd_create, which leads to a cryptic "command not found"
under set -euo pipefail if the file is missing; to fix, after the existing
source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true add an explicit
guard that verifies required symbols (e.g., test for the sed_inplace
function/variable and any other utilities used by cmd_create) and exit with a
clear error message if missing, or change the source to be mandatory (remove the
"|| true") for scripts that cannot run without shared-constants.sh; update
cmd_create references to rely on that guard so failures surface with a
descriptive log rather than an opaque shell error.

In @.agents/scripts/comprehensive-quality-fix.sh:
- Around line 66-84: The sed_append_after calls use hardcoded line numbers and
are executed top-down which shifts subsequent targets; update each case block
(e.g., the "closte-helper.sh", "cloudron-helper.sh", "coolify-helper.sh",
"dns-helper.sh" blocks) to perform insertions bottom-up by ordering
sed_append_after calls from highest to lowest line number or by collecting
target lines into an array and sorting it descending before calling
sed_append_after (ensuring the function name sed_append_after remains unchanged
and only the call order/loop logic is changed).
- Around line 7-9: The script silently ignores failures sourcing
shared-constants.sh leaving helpers like sed_inplace and sed_append_after
undefined; change the sourcing to explicitly check the file and abort (or at
least warn) if it cannot be loaded: ensure SCRIPT_DIR is computed, verify
"$SCRIPT_DIR/shared-constants.sh" exists and is readable, source it without
redirecting errors (or capture and handle failure), then confirm required
symbols (e.g., sed_inplace, sed_append_after) are available and call process
exit with a clear error message if they are missing so subsequent commands don't
fail with "command not found."

In @.agents/scripts/privacy-filter-helper.sh:
- Line 33: The script silently ignores failures when sourcing
shared-constants.sh, which can leave the sed_inplace function undefined and
cause apply_redactions() to fail later; remove the "|| true" fallback so
sourcing errors surface, or add an explicit guard after sourcing that checks for
the sed_inplace symbol (e.g., test if the function/command sed_inplace exists)
and print a clear error and exit if missing; ensure this guard references
SCRIPT_DIR and shared-constants.sh so maintainers can locate where the
dependency is loaded and avoid the opaque "command not found" failure under set
-euo pipefail.

In @.agents/scripts/supervisor-helper.sh:
- Line 81: The script silently ignores failure when sourcing shared-constants.sh
(which defines sed_inplace and sed_append_after) because of "2>/dev/null ||
true", causing later "command not found" errors; remove the silent suppression
and add an explicit warning if the source fails (or move the source below the
log_warn definition and use log_warn), e.g., try sourcing shared-constants.sh
without redirecting stderr and on failure emit a clear message referencing
shared-constants.sh and the missing functions (sed_inplace, sed_append_after)
and either exit or continue with a documented fallback; ensure references to
log_warn (or a temporary echo >&2 fallback placed before log_warn is defined)
are used so the warning is visible.

In @.agents/scripts/version-manager.sh:
- Around line 22-24: The script sources shared-constants.sh to obtain
sed_inplace but does not handle a failed source; update_version_in_files and
auto_mark_tasks_complete call sed_inplace and will break the release if
undefined. Fix by adding a fail-fast check immediately after the source (verify
sed_inplace is defined and executable) and abort with a clear error if not, or
alternatively implement a minimal inline fallback for sed_inplace before any
calls (declare a portable sed_inplace function used by update_version_in_files
and auto_mark_tasks_complete). Reference symbols: shared-constants.sh,
SCRIPT_DIR, sed_inplace, update_version_in_files, auto_mark_tasks_complete.
🧹 Nitpick comments (1)
.agents/scripts/watercrawl-helper.sh (1)

39-41: SCRIPT_DIR is defined twice — lines 40 and 56.

The first definition (line 40) is used for sourcing shared-constants.sh. Then line 56 redefines it identically and line 57 makes it readonly. This is harmless but redundant. Consider removing the duplicate:

Proposed cleanup
 # 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
 
 # Colors for output
 readonly GREEN='\033[0;32m'
 ...
 
 # Constants
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit
-readonly SCRIPT_DIR
+readonly SCRIPT_DIR

Also applies to: 56-57

SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit
# shellcheck disable=SC2034
readonly SCRIPT_DIR
source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Soft-sourcing shared-constants.sh but hard-depending on sed_inplace at line 869.

The 2>/dev/null || true pattern means sourcing silently succeeds even if the file is missing. However, cmd_create() at line 869 calls sed_inplace, which will fail with an opaque "command not found" error under set -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
 source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true
+if ! declare -f sed_inplace >/dev/null 2>&1; then
+    echo "ERROR: shared-constants.sh failed to load (sed_inplace unavailable)" >&2
+    exit 1
+fi

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
In @.agents/scripts/agent-test-helper.sh at line 53, The script soft-sources
shared-constants.sh but later unconditionally calls sed_inplace in cmd_create,
which leads to a cryptic "command not found" under set -euo pipefail if the file
is missing; to fix, after the existing source "$SCRIPT_DIR/shared-constants.sh"
2>/dev/null || true add an explicit guard that verifies required symbols (e.g.,
test for the sed_inplace function/variable and any other utilities used by
cmd_create) and exit with a clear error message if missing, or change the source
to be mandatory (remove the "|| true") for scripts that cannot run without
shared-constants.sh; update cmd_create references to rely on that guard so
failures surface with a descriptive log rather than an opaque shell error.

Comment on lines +7 to +9
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Silent failure if shared-constants.sh fails to load leaves sed_inplace/sed_append_after undefined.

The 2>/dev/null || true suppresses all errors, so if the source fails, the script continues but every subsequent sed_inplace / sed_append_after call will error out with "command not found." Consider adding a guard or at least a warning:

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

‼️ 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
# 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 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 || {
echo "⚠️ shared-constants.sh not found; sed_inplace/sed_append_after unavailable" >&2
exit 1
}
🤖 Prompt for AI Agents
In @.agents/scripts/comprehensive-quality-fix.sh around lines 7 - 9, The script
silently ignores failures sourcing shared-constants.sh leaving helpers like
sed_inplace and sed_append_after undefined; change the sourcing to explicitly
check the file and abort (or at least warn) if it cannot be loaded: ensure
SCRIPT_DIR is computed, verify "$SCRIPT_DIR/shared-constants.sh" exists and is
readable, source it without redirecting errors (or capture and handle failure),
then confirm required symbols (e.g., sed_inplace, sed_append_after) are
available and call process exit with a clear error message if they are missing
so subsequent commands don't fail with "command not found."

Comment on lines 66 to 84
"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"
;;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Sequential sed_append_after calls with hardcoded line numbers produce off-by-one errors after the first insertion.

Each sed_append_after inserts a line, shifting all subsequent line numbers by +1. The second call in each case block still uses the original line number, so it targets the wrong line.

For example in closte-helper.sh (comment says "Lines 134, 249"):

  • sed_append_after 133 inserts after line 133 → file grows by 1 line.
  • sed_append_after 248 now targets what was originally line 247, not 249.

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

‼️ 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
"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"
;;
"closte-helper.sh")
# Lines 134, 249
sed_append_after 248 ' return 0' "$file"
sed_append_after 133 ' return 0' "$file"
;;
"cloudron-helper.sh")
# Lines 74, 202
sed_append_after 201 ' return 0' "$file"
sed_append_after 73 ' return 0' "$file"
;;
"coolify-helper.sh")
# Line 236
sed_append_after 235 ' return 0' "$file"
;;
"dns-helper.sh")
# Lines 95, 259
sed_append_after 258 ' return 0' "$file"
sed_append_after 94 ' return 0' "$file"
;;
🤖 Prompt for AI Agents
In @.agents/scripts/comprehensive-quality-fix.sh around lines 66 - 84, The
sed_append_after calls use hardcoded line numbers and are executed top-down
which shifts subsequent targets; update each case block (e.g., the
"closte-helper.sh", "cloudron-helper.sh", "coolify-helper.sh", "dns-helper.sh"
blocks) to perform insertions bottom-up by ordering sed_append_after calls from
highest to lowest line number or by collecting target lines into an array and
sorting it descending before calling sed_append_after (ensuring the function
name sed_append_after remains unchanged and only the call order/loop logic is
changed).


# Configuration
readonly SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit
source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Silent fallback when shared-constants.sh fails to source leaves sed_inplace undefined.

Line 375 calls sed_inplace in apply_redactions(). If sourcing fails (file missing, permission issue), sed_inplace will be undefined and the script will crash at runtime with a "command not found" error — which is opaque to the user. Given set -euo pipefail on line 20, the script will exit immediately with no helpful message.

Consider either removing || true (so sourcing failure is caught early), or adding a guard:

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; }
+fi

As per coding guidelines, .agents/scripts/*.sh scripts should focus on reliability, robustness, and error recovery mechanisms.

📝 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
source "$SCRIPT_DIR/shared-constants.sh" 2>/dev/null || true
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; }
fi
🤖 Prompt for AI Agents
In @.agents/scripts/privacy-filter-helper.sh at line 33, The script silently
ignores failures when sourcing shared-constants.sh, which can leave the
sed_inplace function undefined and cause apply_redactions() to fail later;
remove the "|| true" fallback so sourcing errors surface, or add an explicit
guard after sourcing that checks for the sed_inplace symbol (e.g., test if the
function/command sed_inplace exists) and print a clear error and exit if
missing; ensure this guard references SCRIPT_DIR and shared-constants.sh so
maintainers can locate where the dependency is loaded and avoid the opaque
"command not found" failure under set -euo pipefail.

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Silent failure when sourcing a now-required dependency.

shared-constants.sh provides sed_inplace and sed_append_after, which are called later (lines 4227, 4303, 4392, 4396) with no local fallback. The 2>/dev/null || true will silently swallow a missing or broken source, leading to cryptic "command not found" errors deep in TODO.md update paths.

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: log_warn is defined on line 139, after this source line on 81. You'd need to either move the source below the log function definitions, or use a simpler echo >&2 fallback here.

As per coding guidelines, automation scripts should focus on "Clear logging and feedback" and "Error recovery mechanisms".

🤖 Prompt for AI Agents
In @.agents/scripts/supervisor-helper.sh at line 81, The script silently ignores
failure when sourcing shared-constants.sh (which defines sed_inplace and
sed_append_after) because of "2>/dev/null || true", causing later "command not
found" errors; remove the silent suppression and add an explicit warning if the
source fails (or move the source below the log_warn definition and use
log_warn), e.g., try sourcing shared-constants.sh without redirecting stderr and
on failure emit a clear message referencing shared-constants.sh and the missing
functions (sed_inplace, sed_append_after) and either exit or continue with a
documented fallback; ensure references to log_warn (or a temporary echo >&2
fallback placed before log_warn is defined) are used so the warning is visible.

Comment on lines +22 to +24
# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: sed_inplace undefined during release would corrupt the release process.

This is the release-critical script (version-manager.sh release [major|minor|patch]). If shared-constants.sh fails to source, sed_inplace calls in update_version_in_files() (lines 422, 433, 444, 455, 471, 490) and auto_mark_tasks_complete() (lines 708, 711) will fail mid-release, potentially leaving version files in an inconsistent state.

For this script specifically, I'd recommend either failing fast or including an inline fallback:

Option A: Fail fast (preferred for release tooling)
 # 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" || { echo "[ERROR] Failed to source shared-constants.sh" >&2; exit 1; }
Option B: Inline fallback
 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; }
+fi

As per coding guidelines, .agents/scripts/version-manager.sh is the release pipeline entry point and should prioritize reliability and proper error recovery.

📝 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
# 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 shared constants (provides sed_inplace and other utilities)
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit
source "$SCRIPT_DIR/shared-constants.sh" || { echo "[ERROR] Failed to source shared-constants.sh" >&2; exit 1; }
🤖 Prompt for AI Agents
In @.agents/scripts/version-manager.sh around lines 22 - 24, The script sources
shared-constants.sh to obtain sed_inplace but does not handle a failed source;
update_version_in_files and auto_mark_tasks_complete call sed_inplace and will
break the release if undefined. Fix by adding a fail-fast check immediately
after the source (verify sed_inplace is defined and executable) and abort with a
clear error if not, or alternatively implement a minimal inline fallback for
sed_inplace before any calls (declare a portable sed_inplace function used by
update_version_in_files and auto_mark_tasks_complete). Reference symbols:
shared-constants.sh, SCRIPT_DIR, sed_inplace, update_version_in_files,
auto_mark_tasks_complete.

marcusquinn added a commit that referenced this pull request Feb 7, 2026
…urce lines

PR #480 (shared-constants adoption) added source lines to scripts that
already had them from PR #479 (sed portability), causing double-sourcing.
Under set -euo pipefail, the second source fails on readonly re-declaration,
silently breaking supervisor-helper.sh (batch, help, all commands return 1).

Fix: Add include guard (_SHARED_CONSTANTS_LOADED) so shared-constants.sh
is safe to source multiple times. Also remove 5 duplicate source lines.
marcusquinn added a commit that referenced this pull request Feb 7, 2026
…urce lines (#483)

PR #480 (shared-constants adoption) added source lines to scripts that
already had them from PR #479 (sed portability), causing double-sourcing.
Under set -euo pipefail, the second source fails on readonly re-declaration,
silently breaking supervisor-helper.sh (batch, help, all commands return 1).

Fix: Add include guard (_SHARED_CONSTANTS_LOADED) so shared-constants.sh
is safe to source multiple times. Also remove 5 duplicate source lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

t145: bug: sed -i '' is macOS-only, breaks on Linux/CI

1 participant