fix(bash): sed replacement escaping, BSD portability, dead cleanup code by 404prefrontalcortexnotfound · Pull Request #1 · 404prefrontalcortexnotfound/spec-kit

404prefrontalcortexnotfound · 2026-04-07T02:50:04Z

Summary

Mirror of github#2090 for Gemini code review loop before pushing to upstream.

Fix sed escaping to target replacement-side characters (&, \) instead of pattern-side
Replace BSD-incompatible sed newline insertion with portable awk
Remove dead cleanup code referencing non-existent temp file patterns
Restore forge case and revert out-of-scope copilot path change (review feedback)

Test plan

bash -n syntax check
Verified on macOS 15 (BSD sed)

Three bugs in update-agent-context.sh: 1. **sed escaping targets wrong side** (line 318-320): The escaping function escapes regex pattern characters (`[`, `.`, `*`, `^`, `$`, `+`, `{`, `}`, `|`) but these variables are used as sed *replacement* strings, not patterns. Only `&` (insert matched text), `\` (escape char), and `|` (our sed delimiter) are special in the replacement context. Also adds escaping for `project_name` which was used unescaped. 2. **BSD sed newline insertion fails on macOS** (line 364-366): Uses bash variable expansion to insert a literal newline into a sed replacement string. This works on GNU sed (Linux) but fails silently on BSD sed (macOS). Replaced with portable awk approach that works on both platforms. 3. **cleanup() removes non-existent files** (line 125-126): The cleanup trap attempts `rm -f /tmp/agent_update_*_$$` and `rm -f /tmp/manual_additions_$$` but the script never creates files matching these patterns — all temp files use `mktemp`. The wildcard with `$$` (PID) in /tmp could theoretically match unrelated files. Fixes github#154 (macOS sed failure) Fixes github#293 (sed expression errors) Related: github#338 (shellcheck findings)

Address PR review feedback: - Restore forge) case in update_specific_agent since src/specify_cli/integrations/forge/__init__.py still exists - Revert COPILOT_FILE path from .github/agents/ back to .github/ to stay consistent with Python integration and tests - Restore FORGE_FILE variable, comments, and usage strings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

404prefrontalcortexnotfound · 2026-04-07T02:50:50Z

/gemini review

gemini-code-assist

Code Review

This pull request improves the robustness of the update-agent-context.sh script by implementing a more reliable temporary file cleanup mechanism using a tracking array and enhancing sed replacement safety through character escaping. Additionally, it replaces sed with awk for better portability when handling newline conversions and refactors agent update logic for better granularity. The reviewer suggested extracting the repeated sed escaping logic into a helper function to adhere to DRY principles and improve maintainability.

gemini-code-assist · 2026-04-07T02:52:21Z

scripts/bash/update-agent-context.sh

+    local escaped_lang=$(printf '%s\n' "$NEW_LANG" | sed 's/[\\&|]/\\&/g')
+    local escaped_framework=$(printf '%s\n' "$NEW_FRAMEWORK" | sed 's/[\\&|]/\\&/g')
+    commands=$(printf '%s\n' "$commands" | sed 's/[\\&|]/\\&/g')
+    language_conventions=$(printf '%s\n' "$language_conventions" | sed 's/[\\&|]/\\&/g')
+    local escaped_branch=$(printf '%s\n' "$CURRENT_BRANCH" | sed 's/[\\&|]/\\&/g')


The sed escaping logic sed 's/[\\&|]/\\&/g' is repeated multiple times in this function (lines 292, 315, and 325-329). To improve maintainability and follow the DRY (Don't Repeat Yourself) principle, consider extracting this into a helper function.

Suggested change

local escaped_lang=$(printf '%s\n' "$NEW_LANG" | sed 's/[\\&|]/\\&/g')

local escaped_framework=$(printf '%s\n' "$NEW_FRAMEWORK" | sed 's/[\\&|]/\\&/g')

commands=$(printf '%s\n' "$commands" | sed 's/[\\&|]/\\&/g')

language_conventions=$(printf '%s\n' "$language_conventions" | sed 's/[\\&|]/\\&/g')

local escaped_branch=$(printf '%s\n' "$CURRENT_BRANCH" | sed 's/[\\&|]/\\&/g')

# Helper function to escape special characters for sed replacement strings

escape_sed() {

printf '%s\n' "$1" | sed 's/[\\&|]/\\&/g'

}

local escaped_lang=$(escape_sed "$NEW_LANG")

local escaped_framework=$(escape_sed "$NEW_FRAMEWORK")

commands=$(escape_sed "$commands")

language_conventions=$(escape_sed "$language_conventions")

local escaped_branch=$(escape_sed "$CURRENT_BRANCH")

Address Gemini review feedback — the inline sed escaping pattern appeared 7 times in create_new_agent_file(). Extract to a single helper function for maintainability and readability. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

gemini-code-assist

Code Review

This pull request enhances the update-agent-context.sh script by introducing a centralized temporary file cleanup mechanism using a _CLEANUP_FILES array and improving portability by replacing sed -i with awk for newline conversions. It also refines the escaping of special characters for sed replacement strings. A regression was identified where splitting the $AGENTS_FILE updates into multiple calls causes logging issues due to path deduplication and results in the accidental removal of the 'Pi' agent.

gemini-code-assist · 2026-04-07T02:58:15Z

scripts/bash/update-agent-context.sh

    _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


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

Gemini correctly identified that splitting AGENTS_FILE updates into individual calls is redundant — _update_if_new deduplicates by realpath, so only the first call logs. Restore the combined label and add back missing Pi reference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

404prefrontalcortexnotfound · 2026-04-07T03:08:31Z

/gemini review

404prefrontalcortexnotfound · 2026-04-07T03:13:55Z

Gemini review loop complete. 2 comments resolved:

DRY extraction (medium) — extracted repeated sed escaping into _esc_sed() helper
AGENTS_FILE dedup regression (high) — restored combined label, added back missing Pi

No remaining suggestions. Both comments verified substantive (1 via blind audit, 1 directly).

gemini-code-assist

Code Review

This pull request improves the robustness and portability of the update-agent-context.sh script. Key changes include the introduction of a centralized temporary file tracking system (_CLEANUP_FILES) for reliable cleanup on exit or interrupt, and a new _esc_sed function to properly escape special characters in sed replacement strings. Additionally, the script now uses awk instead of sed -i for more portable newline conversions. Feedback was provided regarding the _esc_sed function to avoid appending unnecessary newlines during string processing.

gemini-code-assist · 2026-04-07T03:14:20Z

scripts/bash/update-agent-context.sh


+# 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'; }


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'; }

…s it The old code manually pre-escaped & as \& in get_commands_for_language because the broken escaping function didn't handle &. Now that _esc_sed properly escapes replacement-side specials, the pre-escaping causes double-escaping: && becomes \&\& in generated files. Found by blind audit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

404prefrontalcortexnotfound · 2026-04-07T03:30:32Z

/gemini review

404prefrontalcortexnotfound · 2026-04-07T03:36:21Z

/gemini review

gemini-code-assist

Code Review

This pull request improves temporary file management by introducing a tracking array and a robust cleanup mechanism. It also fixes an escaping issue in JavaScript/TypeScript command strings and enhances sed safety by introducing a dedicated escaping function for replacement strings. Additionally, it replaces a non-portable sed newline conversion with an awk implementation. Feedback was provided regarding the awk and mv command chain, suggesting they be split into separate lines to ensure that failures are properly caught by set -e.

gemini-code-assist · 2026-04-07T03:36:24Z

scripts/bash/update-agent-context.sh

-    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"


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"

gemini-code-assist

Code Review

This pull request improves the update-agent-context.sh script by implementing a robust temporary file cleanup mechanism, fixing command escaping for JavaScript/TypeScript, and enhancing portability by using awk for newline conversions. It also introduces a helper function to safely escape strings for sed. A significant issue was found in the cleanup function where improper array expansion could cause accidental file deletions in older Bash versions.

gemini-code-assist · 2026-04-07T03:41:49Z

scripts/bash/update-agent-context.sh

    trap - EXIT INT TERM
-    rm -f /tmp/agent_update_*_$$
-    rm -f /tmp/manual_additions_$$
+    for f in "${_CLEANUP_FILES[@]+"${_CLEANUP_FILES[@]}"}"; do


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

Under set -e, the left side of && does not trigger errexit on failure. Split into two statements so awk failures are fatal instead of silent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

On Bash 3.2, the ${arr[@]+"${arr[@]}"} pattern expands to a single empty string when the array is empty, causing rm to target .bak and .tmp in the current directory. Use explicit length check instead, which also avoids the word-splitting risk of unquoted expansion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

404prefrontalcortexnotfound · 2026-04-07T03:54:25Z

/gemini review

404prefrontalcortexnotfound · 2026-04-07T03:59:42Z

Gemini review loop complete (round 2). 2 comments resolved, 1 verified-pedantic:

Resolved:

set -e && anti-pattern (medium) — split awk ... && mv ... into separate statements so set -e catches awk failures
Bash 3.2 empty array expansion (high) — replaced ${arr[@]+"${arr[@]}"} with explicit length guard to prevent rm targeting .bak/.tmp in cwd

Verified-pedantic (blind audit):

printf %s\n vs %s (medium) — %s\n is required for POSIX sed compliance; %s risks silent data loss on BSD sed. No change.

gemini-code-assist

Code Review

This pull request improves the robustness and portability of the update-agent-context.sh script. Key changes include a centralized temporary file cleanup mechanism using a tracking array, the introduction of a _esc_sed utility to safely handle special characters in sed replacements, and a switch to awk for more portable newline conversion. A fix was also applied to the JavaScript/TypeScript test command escaping. Feedback was provided regarding the _esc_sed function, specifically suggesting that it should also escape literal newline characters to ensure stability when processing multi-line data.

gemini-code-assist · 2026-04-07T04:00:51Z

scripts/bash/update-agent-context.sh


+# 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'; }


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.

Bo Bobson and others added 2 commits April 7, 2026 12:34

gemini-code-assist bot reviewed Apr 7, 2026

View reviewed changes

404prefrontalcortexnotfound closed this Apr 7, 2026

gemini-code-assist bot reviewed Apr 7, 2026

View reviewed changes

404prefrontalcortexnotfound reopened this Apr 7, 2026

gemini-code-assist bot reviewed Apr 7, 2026

View reviewed changes

Bo Bobson and others added 2 commits April 7, 2026 13:42

fix: split awk && mv to let set -e catch awk failures

414d016

Under set -e, the left side of && does not trigger errexit on failure. Split into two statements so awk failures are fatal instead of silent. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

404prefrontalcortexnotfound closed this Apr 7, 2026

gemini-code-assist bot reviewed Apr 7, 2026

View reviewed changes

-    local escaped_lang=$(printf '%s\n' "$NEW_LANG" | sed 's/[\\&|]/\\&/g')
-    local escaped_framework=$(printf '%s\n' "$NEW_FRAMEWORK" | sed 's/[\\&|]/\\&/g')
-    commands=$(printf '%s\n' "$commands" | sed 's/[\\&|]/\\&/g')
-    language_conventions=$(printf '%s\n' "$language_conventions" | sed 's/[\\&|]/\\&/g')
-    local escaped_branch=$(printf '%s\n' "$CURRENT_BRANCH" | sed 's/[\\&|]/\\&/g')
+    # Helper function to escape special characters for sed replacement strings
+    escape_sed() {
+        printf '%s\n' "$1" | sed 's/[\\&|]/\\&/g'
+    }
+    local escaped_lang=$(escape_sed "$NEW_LANG")
+    local escaped_framework=$(escape_sed "$NEW_FRAMEWORK")
+    commands=$(escape_sed "$commands")
+    language_conventions=$(escape_sed "$language_conventions")
+    local escaped_branch=$(escape_sed "$CURRENT_BRANCH")

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

	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"

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

Conversation

404prefrontalcortexnotfound commented Apr 7, 2026

Summary

Test plan

Uh oh!

404prefrontalcortexnotfound commented Apr 7, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

404prefrontalcortexnotfound commented Apr 7, 2026

Uh oh!

404prefrontalcortexnotfound commented Apr 7, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

404prefrontalcortexnotfound commented Apr 7, 2026

Uh oh!

404prefrontalcortexnotfound commented Apr 7, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

404prefrontalcortexnotfound commented Apr 7, 2026

Uh oh!

404prefrontalcortexnotfound commented Apr 7, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant