Skip to content

fix(bash): sed replacement escaping, BSD portability, dead cleanup in update-agent-context.sh#2090

Open
404prefrontalcortexnotfound wants to merge 7 commits intogithub:mainfrom
404prefrontalcortexnotfound:fix/update-agent-context-sed-portability
Open

fix(bash): sed replacement escaping, BSD portability, dead cleanup in update-agent-context.sh#2090
404prefrontalcortexnotfound wants to merge 7 commits intogithub:mainfrom
404prefrontalcortexnotfound:fix/update-agent-context-sed-portability

Conversation

@404prefrontalcortexnotfound
Copy link
Copy Markdown

@404prefrontalcortexnotfound 404prefrontalcortexnotfound commented Apr 4, 2026

Summary

Three bugs in scripts/bash/update-agent-context.sh that cause silent failures on macOS and potential data corruption with special characters in project/technology names.

Bug 1: sed escaping targets wrong side (lines 318-320)

The escaping function escapes regex pattern characters ([, ., *, ^, $, +, {, }, |) but these variables are used as sed replacement strings, not patterns. In sed replacements, only two characters are special:

  • & — inserts the entire matched pattern
  • \ — escape character

Impact: A technology name containing & (e.g., "AT&T", "R&D") silently corrupts the agent context file. The & gets replaced with the entire matched text from the left side of the substitution.

Fix: Escape only & and \ in replacement strings. Also adds escaping for project_name which was used unescaped.

Bug 2: BSD sed newline insertion fails on macOS (lines 364-366)

# Current — fails silently on BSD sed (macOS)
newline=$(printf '\n')
sed -i.bak2 "s/\\\\n/${newline}/g" "$temp_file"

BSD sed does not support literal newlines in replacement strings via variable expansion. GNU sed (Linux) does. This means the \n → newline conversion silently fails on macOS, leaving literal \n sequences in the generated agent context files.

Fix: Replace with portable awk approach:

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

Bug 3: cleanup() removes non-existent files (lines 125-126)

rm -f /tmp/agent_update_*_$$
rm -f /tmp/manual_additions_$$

The script never creates files matching these patterns — all temp files use mktemp with random suffixes. The wildcard with $$ (PID) in /tmp is a code smell that could theoretically match unrelated files.

Fix: Remove the dead rm commands. Temp files from mktemp are cleaned up inline after use.

Testing

  • bash -n syntax check passes
  • Tested on macOS 15 (Darwin, BSD sed) — all three fixes verified

Related Issues

Fixes #154 — update-agent-context.sh seems to be failing on macOS
Fixes #293 — update-agent-context.sh script fails with sed expression errors
Related: #338 — Problems detected by shellcheck on the provided bash scripts

@404prefrontalcortexnotfound 404prefrontalcortexnotfound force-pushed the fix/update-agent-context-sed-portability branch 2 times, most recently from 44072cb to dd96d15 Compare April 4, 2026 08:29
@mnriem mnriem requested a review from Copilot April 6, 2026 13:10
Copilot AI reviewed Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates scripts/bash/update-agent-context.sh to be safer and more portable across GNU/BSD toolchains (notably macOS), focusing on correct escaping for sed replacements, portable newline handling, and safer temp-file cleanup.

Changes:

  • Fixes sed replacement escaping to prevent corruption when values contain replacement-special characters (notably & and \), and applies escaping to additional substituted values (e.g., project_name).
  • Replaces BSD-incompatible sed newline insertion with a portable awk-based \n → newline conversion.
  • Reworks cleanup to track and remove only known temp files; also adjusts agent support/path mappings (notably Copilot path and Forge handling).
Show a summary per file
File Description
scripts/bash/update-agent-context.sh Improves sed replacement safety, macOS portability, and temp-file cleanup; updates agent path/support mappings.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 2

mnriem
mnriem requested changes Apr 6, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@mnriem mnriem left a comment

Choose a reason for hiding this comment

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

Please address Copilot feedback. You might have to pull in upstream/main

Bo Bobson and others added 2 commits April 7, 2026 12:34
fix(bash): sed replacement escaping, BSD portability, dead cleanup code
8f97d7e 
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)
@claude
fix: restore forge case and revert copilot path change
1661ddb 
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 404prefrontalcortexnotfound force-pushed the fix/update-agent-context-sed-portability branch from dd96d15 to 1661ddb Compare April 7, 2026 02:49
@404prefrontalcortexnotfound 404prefrontalcortexnotfound mentioned this pull request Apr 7, 2026
Closed
Bo Bobson and others added 5 commits April 7, 2026 12:57
@claude
refactor: extract repeated sed escaping into _esc_sed helper
3280cd7 
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>
@claude
fix: restore combined AGENTS_FILE label in update_all_existing_agents
e4933f9 
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>
@claude
fix: remove pre-escaped && in JS/TS commands now that _esc_sed handle…
e26a1e6 
…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>
@claude
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>
@claude
fix: guard empty _CLEANUP_FILES array for Bash 3.2 compatibility
aac7864 
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>
@mnriem mnriem requested a review from Copilot April 7, 2026 12:17
Copilot AI reviewed Apr 7, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes portability and correctness issues in scripts/bash/update-agent-context.sh so agent context files can be generated/updated safely on both GNU and BSD (macOS) toolchains, including when project/technology values contain sed-sensitive characters.

Changes:

  • Introduces a dedicated sed replacement-side escaping helper and applies it to template substitutions (including project_name).
  • Replaces the non-portable BSD sed newline replacement with a portable awk-based \n → newline conversion.
  • Reworks temp-file cleanup to track mktemp outputs and remove them safely via a trap, avoiding broad /tmp globs.
Show a summary per file
File Description
scripts/bash/update-agent-context.sh Makes sed substitutions safe for replacement strings, improves macOS portability, and strengthens temp-file cleanup behavior.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@mnriem mnriem mnriem requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

update-agent-context.sh script fails with sed expression errors and incorrect template path update-agent-context.sh seems to be failing on macOS

3 participants

@404prefrontalcortexnotfound @mnriem