Skip to content

fix(bash): sed replacement escaping, BSD portability, dead cleanup code #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
404prefrontalcortexnotfound wants to merge 7 commits into from
+32 −16
Closed

fix(bash): sed replacement escaping, BSD portability, dead cleanup code #1

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f97d7e
fix(bash): sed replacement escaping, BSD portability, dead cleanup code
Apr 4, 2026
1661ddb
fix: restore forge case and revert copilot path change
Apr 7, 2026
3280cd7
refactor: extract repeated sed escaping into _esc_sed helper
Apr 7, 2026
e4933f9
fix: restore combined AGENTS_FILE label in update_all_existing_agents
Apr 7, 2026
e26a1e6
fix: remove pre-escaped && in JS/TS commands now that _esc_sed handle…
Apr 7, 2026
414d016
fix: split awk && mv to let set -e catch awk failures
Apr 7, 2026
aac7864
fix: guard empty _CLEANUP_FILES array for Bash 3.2 compatibility
Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 33 additions & 16 deletions scripts/bash/update-agent-context.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,17 @@ log_warning() {
echo "WARNING: $1" >&2
}

# Track temporary files for cleanup on interrupt
_CLEANUP_FILES=()

# Cleanup function for temporary files
cleanup() {
local exit_code=$?
# Disarm traps to prevent re-entrant loop
trap - EXIT INT TERM
rm -f /tmp/agent_update_*_$$
rm -f /tmp/manual_additions_$$
for f in "${_CLEANUP_FILES[@]+"${_CLEANUP_FILES[@]}"}"; do
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

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

high

The array expansion "${_CLEANUP_FILES[@]+"${_CLEANUP_FILES[@]}"}" inside double quotes will expand to an empty string "" if the array is empty in older Bash versions (like 3.2 on macOS). This causes the loop to run once with f as an empty string, leading to rm -f "" ".bak" ".tmp". This is dangerous as it will attempt to delete files named .bak and .tmp in the current working directory if they exist. To fix this, remove the outer double quotes so it expands to nothing when the array is empty.

Suggested change
for f in "${_CLEANUP_FILES[@]+"${_CLEANUP_FILES[@]}"}"; do
for f in ${_CLEANUP_FILES[@]+"${_CLEANUP_FILES[@]}"}; do

rm -f "$f" "$f.bak" "$f.tmp"
done
exit $exit_code
}

Expand Down Expand Up @@ -281,10 +285,15 @@ get_language_conventions() {
echo "$lang: Follow standard conventions"
}

# Escape sed replacement-side specials for | delimiter.
# & and \ are replacement-side specials; | is our sed delimiter.
_esc_sed() { printf '%s\n' "$1" | sed 's/[\\&|]/\\&/g'; }
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

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

medium

The _esc_sed function uses printf '%s\n' which appends a trailing newline to the output. While Bash command substitution $(...) strips trailing newlines, this behavior can be problematic if the input string itself contains intended trailing newlines or if the function is used in a context where newlines are not stripped. More importantly, if the input contains internal newlines, the resulting string will break the sed commands later in the script (e.g., line 367) because sed replacement strings cannot contain unescaped literal newlines. It is safer to use printf '%s' to avoid adding unnecessary characters.

Suggested change
_esc_sed() { printf '%s\n' "$1" | sed 's/[\\&|]/\\&/g'; }
_esc_sed() { printf '%s' "$1" | sed 's/[\\&|]/\\&/g'; }

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

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

medium

The _esc_sed function correctly handles \, &, and the | delimiter, which are the primary special characters in a sed replacement string. However, it does not escape literal newline characters. If an input variable (e.g., from a plan file) were to contain a newline, the sed command in create_new_agent_file would fail due to an unterminated s command. While current inputs are expected to be single-line, adding a substitution to escape newlines as \n (backslash followed by a real newline) would make this utility more robust against multi-line data.


create_new_agent_file() {
local target_file="$1"
local temp_file="$2"
local project_name="$3"
local project_name
project_name=$(_esc_sed "$3")
local current_date="$4"

if [[ ! -f "$TEMPLATE_FILE" ]]; then
Expand All @@ -307,18 +316,19 @@ create_new_agent_file() {
# Replace template placeholders
local project_structure
project_structure=$(get_project_structure "$NEW_PROJECT_TYPE")
project_structure=$(_esc_sed "$project_structure")

local commands
commands=$(get_commands_for_language "$NEW_LANG")

local language_conventions
language_conventions=$(get_language_conventions "$NEW_LANG")
# Perform substitutions with error checking using safer approach
# Escape special characters for sed by using a different delimiter or escaping
local escaped_lang=$(printf '%s\n' "$NEW_LANG" | sed 's/[\[\.*^$()+{}|]/\\&/g')
local escaped_framework=$(printf '%s\n' "$NEW_FRAMEWORK" | sed 's/[\[\.*^$()+{}|]/\\&/g')
local escaped_branch=$(printf '%s\n' "$CURRENT_BRANCH" | sed 's/[\[\.*^$()+{}|]/\\&/g')

local escaped_lang=$(_esc_sed "$NEW_LANG")
local escaped_framework=$(_esc_sed "$NEW_FRAMEWORK")
commands=$(_esc_sed "$commands")
language_conventions=$(_esc_sed "$language_conventions")
local escaped_branch=$(_esc_sed "$CURRENT_BRANCH")

# Build technology stack and recent change strings conditionally
local tech_stack
Expand Down Expand Up @@ -361,17 +371,17 @@ create_new_agent_file() {
fi
done

# Convert \n sequences to actual newlines
newline=$(printf '\n')
sed -i.bak2 "s/\\\\n/${newline}/g" "$temp_file"
# Convert literal \n sequences to actual newlines (portable — works on BSD + GNU)
awk '{gsub(/\\n/,"\n")}1' "$temp_file" > "$temp_file.tmp" && mv "$temp_file.tmp" "$temp_file"
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

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

medium

In Bash, when set -e is active, a failure in a command that is part of a && or || list (except for the last command in the list) will not trigger an immediate exit. Here, if awk fails, the mv command is skipped, but the script will continue executing, leading to a silent failure where the template placeholders are not correctly expanded. It is safer to separate these into two distinct commands so that set -e can catch a failure in either.

Suggested change
awk '{gsub(/\\n/,"\n")}1' "$temp_file" > "$temp_file.tmp" && mv "$temp_file.tmp" "$temp_file"
awk '{gsub(/\\n/,"\\n")}1' "$temp_file" > "$temp_file.tmp"
mv "$temp_file.tmp" "$temp_file"


# Clean up backup files
rm -f "$temp_file.bak" "$temp_file.bak2"
# Clean up backup files from sed -i.bak
rm -f "$temp_file.bak"

# Prepend Cursor frontmatter for .mdc files so rules are auto-included
if [[ "$target_file" == *.mdc ]]; then
local frontmatter_file
frontmatter_file=$(mktemp) || return 1
_CLEANUP_FILES+=("$frontmatter_file")
printf '%s\n' "---" "description: Project Development Guidelines" "globs: [\"**/*\"]" "alwaysApply: true" "---" "" > "$frontmatter_file"
cat "$temp_file" >> "$frontmatter_file"
mv "$frontmatter_file" "$temp_file"
Expand All @@ -395,6 +405,7 @@ update_existing_agent_file() {
log_error "Failed to create temporary file"
return 1
}
_CLEANUP_FILES+=("$temp_file")

# Process the file in one pass
local tech_stack=$(format_technology_stack "$NEW_LANG" "$NEW_FRAMEWORK")
Expand Down Expand Up @@ -519,6 +530,7 @@ update_existing_agent_file() {
if ! head -1 "$temp_file" | grep -q '^---'; then
local frontmatter_file
frontmatter_file=$(mktemp) || { rm -f "$temp_file"; return 1; }
_CLEANUP_FILES+=("$frontmatter_file")
printf '%s\n' "---" "description: Project Development Guidelines" "globs: [\"**/*\"]" "alwaysApply: true" "---" "" > "$frontmatter_file"
cat "$temp_file" >> "$frontmatter_file"
mv "$frontmatter_file" "$temp_file"
Expand Down Expand Up @@ -571,6 +583,7 @@ update_agent_file() {
log_error "Failed to create temporary file"
return 1
}
_CLEANUP_FILES+=("$temp_file")

if create_new_agent_file "$target_file" "$temp_file" "$project_name" "$current_date"; then
if mv "$temp_file" "$target_file"; then
Expand Down Expand Up @@ -743,7 +756,11 @@ update_all_existing_agents() {
_update_if_new "$COPILOT_FILE" "GitHub Copilot" || _all_ok=false
_update_if_new "$CURSOR_FILE" "Cursor IDE" || _all_ok=false
_update_if_new "$QWEN_FILE" "Qwen Code" || _all_ok=false
_update_if_new "$AGENTS_FILE" "Codex/opencode/Amp/Kiro/Bob/Pi/Forge" || _all_ok=false
_update_if_new "$AGENTS_FILE" "Codex/opencode" || _all_ok=false
_update_if_new "$AMP_FILE" "Amp" || _all_ok=false
_update_if_new "$KIRO_FILE" "Kiro CLI" || _all_ok=false
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

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

high

The split of AGENTS_FILE updates into individual calls for Amp, Kiro CLI, IBM Bob, and Forge is redundant and causes a regression in logging. Because _update_if_new uses realpath to deduplicate updates, only the first call (Codex/opencode) will actually execute. Subsequent calls for the same file will be skipped, meaning the specific agent names like "Forge" will never be logged during a full update. Additionally, "Pi" has been removed from the list entirely. It is better to use a combined name for shared files to ensure all relevant agents are represented in the logs and no agents are missed.

Suggested change
_update_if_new "$CURSOR_FILE" "Cursor IDE" || _all_ok=false
_update_if_new "$QWEN_FILE" "Qwen Code" || _all_ok=false
_update_if_new "$AGENTS_FILE" "Codex/opencode/Amp/Kiro/Bob/Pi/Forge" || _all_ok=false
_update_if_new "$AGENTS_FILE" "Codex/opencode" || _all_ok=false
_update_if_new "$AMP_FILE" "Amp" || _all_ok=false
_update_if_new "$KIRO_FILE" "Kiro CLI" || _all_ok=false
_update_if_new "$AGENTS_FILE" "Codex/opencode/Amp/Kiro/Bob/Pi/Forge" || _all_ok=false

_update_if_new "$BOB_FILE" "IBM Bob" || _all_ok=false
_update_if_new "$FORGE_FILE" "Forge" || _all_ok=false
_update_if_new "$WINDSURF_FILE" "Windsurf" || _all_ok=false
_update_if_new "$JUNIE_FILE" "Junie" || _all_ok=false
_update_if_new "$KILOCODE_FILE" "Kilo Code" || _all_ok=false
Expand Down